codex audit findings

[mmv08]

I was having a little bit of fun trying to understand more about Signalium with Codex, and it found some potential bugs. I don't have enough knowledge to see if they're significant bugs or design choices, so I thought I'd share them here:

# Signalium Audit (State Management Readiness)

## Scope

- Reviewed `packages/signalium/src` with focus on runtime reactivity, caching, scope/context behavior, and React integration.
- Re-validated each finding in this document directly against source.
- Ran unit tests: `npm run -w signalium test:unit` (17 files, 394 tests passing).
- React/browser tests were not executed in this environment (Playwright browser binaries unavailable).

## Validation Summary

| #   | Original Severity | Validation                                          | Updated Severity |
| --- | ----------------- | --------------------------------------------------- | ---------------- |
| 1   | Critical          | Confirmed                                           | Critical         |
| 2   | High              | Confirmed                                           | High             |
| 3   | High              | Confirmed                                           | High             |
| 4   | High              | Confirmed risk; partly cache-policy/design tradeoff | Medium           |

## Findings

### 1) Critical (Confirmed): `reactive()` cache is keyed only by function identity; later options and initial fallback scope are ignored

- Evidence:
  - `DERIVED_DEFINITION_MAP` is keyed only by `fn`: `packages/signalium/src/internals/core-api.ts:22`
  - Existing entry is returned without re-checking `opts`: `packages/signalium/src/internals/core-api.ts:28-31,47`
  - Fallback scope is captured at first creation and reused: `packages/signalium/src/internals/core-api.ts:33-37`
- Why this is risky:
  - `reactive(fn, optsA)` then `reactive(fn, optsB)` keeps `optsA` semantics (`equals`, `paramKey`, ids/descriptions).
  - First call site scope can become fallback for later call sites, creating cross-boundary behavior surprises.
- Recommendation:
  - Include options/scope identity in cache semantics, or avoid global memoization by raw function identity.
  - At minimum, detect conflicting options for the same `fn` and throw in dev mode.

### 2) High (Confirmed): strong `Map` for derived definitions can permanently retain function objects and captured closures

- Evidence:
  - Global strong map: `packages/signalium/src/internals/core-api.ts:22`
  - Entries are added and never deleted: `packages/signalium/src/internals/core-api.ts:44,68`
- Why this is risky:
  - Dynamic/reactive creation patterns that generate fresh function identities will accumulate entries.
  - In long-lived processes this creates unbounded retention pressure.
- Recommendation:
  - Use `WeakMap` for function-keyed cache storage when possible.
  - Document/guard against dynamic `reactive(() => ...)` creation patterns that bypass stable reuse.

### 3) High (Confirmed): falsy `paramKey` handling breaks key semantics and GC reinsertion edge cases

- Evidence:
  - `paramKey` path uses truthiness (`paramKey ? ... : args`) instead of undefined check: `packages/signalium/src/internals/contexts.ts:105-106`
  - GC reinsertion uses `if (key)` instead of `key !== undefined`: `packages/signalium/src/internals/contexts.ts:127-131`
- Why this is risky:
  - Valid keys like `0` and `''` are treated as if no key was provided.
  - Signals keyed by falsy values may not be reinserted correctly after unwatch/re-watch flows.
- Recommendation:
  - Use explicit undefined checks:
    - `paramKey !== undefined ? [paramKey] : args`
    - `if (key !== undefined) { ... }`

### 4) Medium (Confirmed Risk): parameterized reactive cache has no eviction path for non-watched entries

- Evidence:
  - New signals are always cached in scope map: `packages/signalium/src/internals/contexts.ts:109-112`
  - GC candidates are only added via unwatch transitions: `packages/signalium/src/internals/watch.ts:45-47`
- Why this is risky:
  - High-cardinality argument usage outside watcher lifecycles can grow cache indefinitely.
  - This is especially relevant for server/background code paths with many unique parameter combinations.
- Nuance:
  - Some persistence is intentional (memoization behavior), so this is both a legitimate risk and a policy decision.
- Recommendation:
  - Add cache lifecycle controls (TTL/LRU/manual invalidation), or introduce stale-entry pruning for non-watched keys.

branch diff with PoC for 1 and 3:
main...misha-phantom:signalium:main

[pzuraq]

Thanks for digging in and opening up the issue! There's a lot going on in this repo so it's good to get these things documented

1. Critical (Confirmed): reactive() cache is keyed only by function identity; later options and initial fallback scope are ignored

So, the intended usage of reactive() is to generally pass in the function definition directly:

const foo = reactive(() => {});

It's possible to metaprogram and create your own factory functions, but at that point I would say it's generally advanced user territory and I think that they should be aware of this and handle it themselves.

But they do need to be made aware of it! I want to avoid doing anymore work here in prod, but we can now add an dev-time only check with IS_DEV that throws if it's ever given a different set of parameters for a given function. That would look something like adding a paramsHash to the function definition when IS_DEV is true, and then comparing that when we have a cache hit, again only if IS_DEV is true.

2. High (Confirmed): strong Map for derived definitions can permanently retain function objects and captured closures

Similar to the above, I don't expect this to be common because functions are typically declared in module scope or are deduped, BUT it's true that it can happen in more advanced use cases. To be clear, it wouldn't happen in a React component for instance because we automatically add function hoisting/deduplication via our build transforms, but edge cases with complex libraries maybe.

I do think WeakMaps have slightly more overhead, but we can switch to a WeakMap here.

3. High (Confirmed): falsy paramKey handling breaks key semantics and GC reinsertion edge cases

I think that this one is pretty unlikely but I do tend to make sure I do === checks in most places, couldn't hurt to add them here.

4. Medium (Confirmed Risk): parameterized reactive cache has no eviction path for non-watched entries

This is... true it turns out, very good catch and actually the worst one on here IMO. There are some general issues with caching and the plan is to drop support for the WeakRef polyfill because it turns out there are bugs that cannot be resolved tractably any other way without embedding some not great semantics.

In the meantime, we could fix this by automatically scheduling an unwatch for signals after scope.get() returns IF they are not currently watched. That would potentially cause some issues though and I'd have to think about it, so it might be better if we just merge the WeakRef fix and release v3.