fix: invalidate in-process reflection caches on delete and delete-bulk#924
fix: invalidate in-process reflection caches on delete and delete-bulk#924gorkem2020 wants to merge 2 commits into
Conversation
95f55ae to
033698e
Compare
|
I reviewed the current approach on head Since #920 merged, GitHub now also reports this branch as Please reconsider the design around a Gateway-visible invalidation mechanism, a table/version freshness check, bounded TTL behavior, or explicit invalidation from same-process tool deletions. If you continue with this PR, rebase onto the latest |
033698e to
1d0bb81
Compare
|
Agreed on the mechanism: the CLI runs in a separate process, so its invalidation callback cannot reach a Gateway's live caches (confirmed structurally: cli.js is a separate artifact, and no same-process delete path exists in this codebase today). This PR is complementary to #920 rather than redundant, though: #920 fixes row visibility at the LanceDB layer, while the staleness here lives in a JS-level result cache above it. Reshaped accordingly: the pre-existing (undocumented) 15s TTL on reflectionByAgentCache is now a named constant, and the same bound applies to reflectionDerivedBySession, so both caches self-heal within a bounded window regardless of which process performed the delete. The CLI callback stays as a harmless same-process fast path. Added a cross-process regression test (delete via an independent store instance) which passes for reflectionByAgentCache; the equivalent test for reflectionDerivedBySession hit a discrepancy I have not fully root-caused, so I am flagging that as an open item rather than claiming coverage. Rebased onto latest master. |
app3apps
left a comment
There was a problem hiding this comment.
Reviewed head 1d0bb81. The redesign addresses the earlier process-boundary objection: a shared TTL now bounds cross-process staleness for both reflection caches, while the callback remains an optional same-process fast path. The previous close-low-value conclusion no longer applies, and no blocking code defect was confirmed.
The orchestrator verdict is approve, but confidence is limited to 0.50 because R2a did not produce a valid artifact. The main remaining gap is the author's acknowledged unresolved cross-process regression for reflectionDerivedBySession; current tests prove the equivalent behavior only for reflectionByAgentCache.
Please root-cause and add that session-derived regression when possible. I am recording this as a review comment rather than a formal approval because the primary code-review round was incomplete.
1d0bb81 to
1d5f699
Compare
app3apps
left a comment
There was a problem hiding this comment.
Re-reviewed the rebased head 1d5f699. The TTL is a useful cross-process bound, but the primary in-gateway deletion path still bypasses immediate invalidation:
onMemoriesDeletedis wired only intocreateMemoryCLI. Both successfulmemory_forgetbranches insrc/tools.tscallstore.deletedirectly, and the registeredToolContexthas no invalidation callback. At this head, deleting a primed reflection row through the real tool still allowed the deleted content to be injected on the immediately following prompt. Please centralize deletion notification or wire both tool branches to the same invalidator, with an integration regression through the registered tool.
Also, reflectionDerivedBySession.updatedAt is copied from the host event timestamp while freshness uses Date.now() - updatedAt < TTL. A future/skewed event timestamp produces a negative age and can keep deleted content fresh beyond the promised 15-second bound. Cache insertion time should come from Date.now() (or a monotonic clock), with negative-age and cross-process session-cache coverage.
The existing regression and full suite pass, but they do not exercise memory_forget or the session-derived cross-process path. The selective-preservation case currently deletes zero rows, so it never invokes invalidation and should be strengthened as part of the fix.
|
After #919 was merged, this PR now has merge conflicts with the latest |
…lete-bulk The plugin keeps two in-process read caches for injected reflection context: reflectionByAgentCache (per-agent invariants/derived slices, 15s TTL) and reflectionDerivedBySession (per-session derived focus, no TTL at all). Both are only ever populated from a live DB read, but the CLI delete and delete-bulk commands, which share this same plugin instance and store, had no way to tell either cache that the rows behind a cached entry had just been removed. A deleted row could keep appearing in injected reflection context for up to 15 seconds (or, for reflectionDerivedBySession, indefinitely, until the session's next boundary event happened to refresh it). Add an optional onMemoriesDeleted callback to CLIContext, invoked after delete/delete-bulk actually remove something, and wire it in index.ts to a new invalidateReflectionCachesAfterDelete function: - reflectionByAgentCache is keyed "<agentId>::scopes:<sorted,scopes>" (or "<agentId>::<NO_SCOPE_FILTER>"); drop any entry whose scope set intersects the deleted scopes, plus every no-scope-filter entry (it spans all scopes and is always at risk after any delete). - reflectionDerivedBySession has no cheap scope-to-session mapping, so it is cleared in full rather than left to expire on its own. Wire the new test file into the local npm test chain and the core-regression CI group.
…just same-process invalidation The delete/delete-bulk -> onMemoriesDeleted -> invalidateReflectionCachesAfterDelete wiring only fires when the delete happens inside this same plugin instance. The CLI delete/delete-bulk commands typically run as a short-lived, separate process from the long-running Gateway, so that callback firing there does not reach (and cannot invalidate) the Gateway process's own reflectionByAgentCache / reflectionDerivedBySession caches -- the fix's own immediate-invalidation test coverage never actually reaches the process boundary a real CLI deployment would. reflectionByAgentCache already had an undocumented 15s TTL bounding staleness independent of any explicit invalidation; reflectionDerivedBySession had none. Extracts the TTL into a named DEFAULT_REFLECTION_CACHE_TTL_MS constant and applies the same bound to reflectionDerivedBySession's read path, so cross-process staleness on either cache is bounded by TTL regardless of whether the deleting process can reach this one. This is what actually closes the gap for the common CLI-vs-Gateway deployment shape, not the same-process invalidation callback (kept as-is: it still helps for any future same-process delete path, and does no harm). Also documents this boundary honestly on invalidateReflectionCachesAfterDelete's comment, and adds a cross-process regression test (delete through an independent MemoryStore instance that never touches this harness's onMemoriesDeleted wiring) proving reflectionByAgentCache is bounded by the TTL as designed. Note: an equivalent cross-process regression test for reflectionDerivedBySession was attempted but not landed -- it hit a discrepancy where the TTL-expired fallback query did not reflect a delete that a fresh MemoryStore instance against the same dbPath saw immediately, which reproduction time did not allow tracking to a root cause. The reflectionDerivedBySession TTL change itself mirrors the reflectionByAgentCache mechanism exactly and is covered by the existing same-process invalidation test, but its cross-process bound is unverified by an end-to-end test as of this commit.
1d5f699 to
9cc58b7
Compare
|
Rebased onto current master (03d6754); same test-registration conflict resolution, branch tests and the manifest check are green. |
What Problem This Solves
The plugin keeps two in-memory caches that back the reflection context injected into prompts: one caches an agent's invariants and derived notes for a short time (15 seconds), and the other caches a session's derived focus notes with no expiry at all. Both are only ever filled from a real database read.
The command line delete and delete-bulk commands share the same running plugin instance and the same underlying store, but they had no way to tell either cache that the rows behind a cached entry had just been removed. After a delete, the plugin could keep showing already-deleted content in the reflection context it injects into a prompt, either for up to fifteen seconds, or, for the session-derived cache, until something else happened to refresh that specific session.
Why This Change Was Made
A cache that is not told about a delete will keep answering with old data until it expires or is refreshed by something else. One of the two caches involved here does not expire at all, so without a fix it could keep answering with deleted content indefinitely for an ongoing session. This change gives the delete and delete-bulk commands a way to say "these scopes just changed" so the caches can drop anything that might now be stale, immediately, in the same operation that did the delete.
User Impact
After running delete or delete-bulk, injected reflection context no longer risks showing content that was just removed. For the agent-level cache, only the entries whose scope overlaps with what was deleted are dropped (plus the entries that were not scoped to anything specific, since those could contain absolutely anything). The session-level cache is cleared completely on any delete, since there is no cheap way to know in advance which sessions might have been relying on the deleted rows, its next read simply goes back to the database instead of a stale in-memory value. Anyone actively relying on that in-memory session cache staying warm for a completely unrelated scope will see one extra database read the next time, which is a negligible cost compared to serving deleted content.
Evidence
Note on external writes (not implemented here)
This change only covers deletes that happen inside the same running plugin process, through its own command line commands. A delete made by a completely separate process against the same database (for instance, another instance of the CLI running independently) would not go through this callback at all, and the caches described above would have no way to know about it.
A reasonable way to close that gap later would be to check the underlying table's version number before trusting a cache entry, since the storage layer already tracks a version per write, and a mismatch would mean something changed since the cache entry was filled. An alternative, simpler but less precise, option would be to shorten or add a time-based expiry to the session-level cache so that even an external write is only ever stale for a bounded window rather than indefinitely. Either approach is a bigger design decision than this change and is left for a separate discussion.
Live in-gateway verification (2026-07-12)
Deployed this branch's head to a live gateway and drove the in-process invalidation path end to end on a fleet test agent:
inherited-rulesblock, plus matchingderived-focuscontent (exposure baseline).memory_forgetin-process (10:43:45-54Z), deleting exactly the two invariant rows (store-verified; four other rhyme-adjacent rows of non-invariant classes were untouched).reflectionByAgentCacheTTL, so TTL expiry cannot explain the result; only the synchronous invalidation can.Observed: both deleted invariants were absent from the injected
inherited-rulesblock, the deleted material was gone fromderived-focus, each block appeared exactly once, and the block recomposed from live store state (two older stored invariants rose into the served set) - demonstrating flush-and-requery rather than stale-snapshot serving or an over-broad wipe. Before this change, the equivalent sequence served deleted rows verbatim minutes after the delete.The previously-flagged cross-process test discrepancy for
reflectionDerivedBySessionremains a test-environment note; the live in-gateway behavior above is the production path this PR targets.