fix(inject-compartments): scope the injection cache to its source database - #308
Conversation
…abase `injectionCache` is process-global and keyed on `sessionId` alone, but the value it holds is rendered FROM a database. Two independent stores that share a session id therefore see each other's blocks: the second store's defer pass replays the first store's `<project-memory>` block instead of rendering its own. Reproduced on clean master @ 885e93d with a two-database probe -- store A seeds a memory and renders, store B is empty and shares the session id: PROBE A: injected=true hasMarker=true PROBE B: injected=false LEAKED_FROM_A=true Cache entries now carry their `Database` handle; a cached entry whose `db` is not the current one is discarded rather than replayed, and every write records the db. Where this bites today is test isolation: two suites that both use `ses-1` with their own temp database poison each other, so a failure appears only when they co-run and the pair passes in isolation. That is a nasty shape to debug -- the failing assertion is in a file that never touched the cache. Production sessions are unique per store, so this is not a live-session bug I can demonstrate; the invariant is simply that a cache keyed by session id must not serve content derived from a different database. Anywhere one process opens more than one store -- explicit database paths, a migration or maintenance pass over a second file, a harness driving several -- the same replay applies. Added a regression test that RED-checks: with the fix reverted it fails with the first store's marker present in the second store's block. Gates on this branch: plugin 3758/0, pi-plugin 0 fail, typecheck 0 across three packages. The one `bun run lint` error is pre-existing on clean master (`latch-permanence-guard.test.ts`, byte-identical here, fails there too).
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…match too Review catch (cortexkit#308): scoping only `injectionCache` left the degraded-mode bookkeeping — `degradedRebuildCountBySession` and `reAnchorLoggedBySession` — keyed by session id alone, so two stores sharing a session id still leaked into each other through that path. That path is the worse half. `degradedCount` gates the layer-B re-anchor, which CHANGES the injected bytes: a store inheriting another's count can re-anchor on its own first degraded pass, one pass early, and the log-once latch can swallow the announcement that it happened. The mismatch branch now calls `clearInjectionCache` rather than deleting the map entry directly. That function already resets the degraded state alongside the cache entry — the two are coupled deliberately (a cache clear means compartment state moved, so an in-flight degraded episode is stale), and the same reasoning applies here. Regression test RED-checks: with a bare `injectionCache.delete`, store B re-anchors to `msg_c1_end` on its first degraded pass instead of staying degraded. Plugin 3759/0, pi-plugin 0 fail, typecheck 0 across three packages.
…match too Review catch (cortexkit#308): scoping only `injectionCache` left the degraded-mode bookkeeping — `degradedRebuildCountBySession` and `reAnchorLoggedBySession` — keyed by session id alone, so two stores sharing a session id still leaked into each other through that path. That path is the worse half. `degradedCount` gates the layer-B re-anchor, which CHANGES the injected bytes: a store inheriting another's count can re-anchor on its own first degraded pass, one pass early, and the log-once latch can swallow the announcement that it happened. The mismatch branch now calls `clearInjectionCache` rather than deleting the map entry directly. That function already resets the degraded state alongside the cache entry — the two are coupled deliberately (a cache clear means compartment state moved, so an in-flight degraded episode is stale), and the same reasoning applies here. Regression test RED-checks: with a bare `injectionCache.delete`, store B re-anchors to `msg_c1_end` on its first degraded pass instead of staying degraded. Plugin 3759/0, pi-plugin 0 fail, typecheck 0 across three packages.
…match too Review catch (cortexkit#308): scoping only `injectionCache` left the degraded-mode bookkeeping — `degradedRebuildCountBySession` and `reAnchorLoggedBySession` — keyed by session id alone, so two stores sharing a session id still leaked into each other through that path. That path is the worse half. `degradedCount` gates the layer-B re-anchor, which CHANGES the injected bytes: a store inheriting another's count can re-anchor on its own first degraded pass, one pass early, and the log-once latch can swallow the announcement that it happened. The mismatch branch now calls `clearInjectionCache` rather than deleting the map entry directly. That function already resets the degraded state alongside the cache entry — the two are coupled deliberately (a cache clear means compartment state moved, so an in-flight degraded episode is stale), and the same reasoning applies here. Regression test RED-checks: with a bare `injectionCache.delete`, store B re-anchors to `msg_c1_end` on its first degraded pass instead of staying degraded. Plugin 3759/0, pi-plugin 0 fail, typecheck 0 across three packages.
|
Merged to master. Reviewed and verified independently: ran your regression tests on current master (71/71 on the two touched suites), re-ran the red-check myself (fix hunk reverted → the new cross-database test fails with store A's marker leaking; restored → green), and the full plugin suite (3,807/0). The clearInjectionCache-on-mismatch choice — dropping the degraded re-anchor bookkeeping with the block cache rather than leaving it to inherit across stores — is the part that makes this complete rather than cosmetic; the count gates a byte-changing re-anchor, so the leak's expensive form was the one you closed. Appreciated the honest scope note distinguishing the test-isolation repro from an unproven live-session claim; the invariant belongs at the cache regardless, exactly as argued. Rides the next release. |
Closes #307. Off clean
master@885e93dc.injectionCacheis keyed onsessionIdalone while holding database-derived content, so two independent stores sharing a session id replay each other's blocks. Cache entries now carry theirDatabasehandle; an entry whosedbis not the current one is discarded instead of replayed, and every write records the db.Reproduction (clean master, before the fix)
Store A seeds a memory and renders; store B is empty, same session id, defer pass:
With the fix:
LEAKED_FROM_A=false, and B correctly returnsnull.What I deliberately did not touch
The defer-replay path is load-bearing for prompt-cache stability — it's what keeps
<session-history>byte-identical across passes so a historian publish doesn't bust the prefix. The change adds an identity check before that path and leaves its behavior alone: same replay, same bytes, same conditions, for every entry whose db matches. Cache-stability tests (replays date-bearing m[0]/m[1] bytes unchanged on consecutive defer passes, the memories-only defer replay) pass unchanged.I went looking for a cheaper fix first — clearing the cache in test teardown would paper over the symptom, but the invariant is a property of the cache, not of the tests, so it belongs at the cache.
Verification
RED-check: with the fix reverted, the new regression test fails with store A's marker present in store B's block — it genuinely guards the bug rather than passing vacuously.
bun run lintreports one pre-existing error inlatch-permanence-guard.test.ts. That file is byte-identical to master here and fails identically on a clean master worktree under the lockfile-pinned Biome, so it's not from this change.Honest scope
I found this because it broke test isolation downstream — two suites using
ses-1with separate temp databases, failing only when co-run. Session ids are unique per store in a normal run, so I'm not claiming a live-session failure and haven't constructed one. The claim is narrower: a cache keyed by session id shouldn't serve content rendered from a different database, and the check is cheap enough that the invariant seems worth holding regardless of whether a production path reaches it today.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Scopes the process-global injection cache to its source database and clears degraded re-anchor state on db mismatch. This prevents cross-database replays and early re-anchors when session IDs collide.
dbtoinjectionCacheentries and callclearInjectionCache(sessionId)on adbmismatch to drop the entry and any degraded state.dbon all cache writes; same-database defer replay is unchanged.Written for commit c6375ba. Summary will update on new commits.
Greptile Summary
The PR scopes process-global compartment-injection cache entries to their source database while preserving replay behavior for matching database handles.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Prepare compartment injection] --> B{Cached session entry?} B -- No --> E[Build from current database] B -- Yes --> C{Cached database matches current database?} C -- No --> D[Clear injection and degraded-reanchor state] D --> E C -- Yes --> F{Cache-busting pass?} F -- No --> G[Replay cached result] F -- Yes --> E E --> H[Cache result with current database handle]Reviews (2): Last reviewed commit: "fix(inject-compartments): drop degraded ..." | Re-trigger Greptile