fix(reflection): guard the derived session cache write against a concurrent delete's invalidation

[gorkem2020]

Problem

Two related gaps around reflectionDerivedBySession, both from the trail #924 left open (its cross-process test covered reflectionByAgentCache and flagged the derived-by-session case as an unexplained discrepancy instead of claiming coverage).

1. Same-process race: the command:new reflection path caches its freshly generated derived slices after awaiting storeReflectionToLanceDB. A delete landing during those awaits bumps the invalidation generation and clears the map, but the late cache write then resurrects derived lines whose rows the delete just removed, and they keep being injected for up to one cache TTL. The by-agent path already guards exactly this window (the TOCTOU check in loadAgentReflectionSlices); the derived write did not.

2. The flagged discrepancy itself turns out to be a test-construction artifact, not an invalidation gap: the derived-focus injector is two caches in series (the session cache, then the independently clocked by-agent fallback inside loadAgentReflectionSlices), so cross-process staleness is bounded from the last DB read, not from the derived cache's priming. A test that primes the two caches at different times and asserts one TTL from the derived priming measures the wrong clock, which reproduces exactly the inconsistency the earlier round saw.

Change

• index.ts: capture the invalidation generation before the reflection store awaits and skip the reflectionDerivedBySession write when it moved (mirrors the existing guard in loadAgentReflectionSlices). The invalidation comment now names the two-caches-in-series fallback and the last-DB-read staleness bound.
• test/reflection-derived-cache-invalidation.test.mjs (new, registered in the package.json test chain and the CI manifest under core-regression):
  • the missing cross-process derived-by-session case, primed through the real generation path (mocked runEmbeddedPiAgent) rather than seeded rows, so the assertion is single-clock and deterministic
  • a series-semantics test pinning the real bound: an intervening by-agent read keeps serving the deleted line until that fallback's own TTL closes
  • the same-process race regression, using a deterministic seam that holds the window between the store resolving and the cache write open until the delete has landed

Validation

• Red before, green after: with the guard reverted the race test fails (2 pass, 1 fail); with the guard in place all 3 pass. Five consecutive cold-cache runs stayed green.
• npm run build clean, tsc --noEmit clean, full npm test chain green, core-regression CI group green, CI manifest verifier green, dist rebuilt in the same commit.
• Deployed to a live gateway: reflection generation on a real /new exercised the changed path end to end (generation, mapped-row admission, reflection markdown write) with no new warnings or errors.

[rwmjhb]

Reviewed head 551d825. The targeted tests, full suite, and repository CI pass. The generation snapshot/check is a conservative fail-safe around the derived-session cache publication and introduces no demonstrated HIGH/CRITICAL regression. Approving.

The new race test would be stronger if it proved the unfixed implementation fails under a production-reachable dispatch action and directly asserted the session-cache publication rather than allowing DB-backed repopulation to satisfy the result. Also note that the process-wide generation counter may suppress an unrelated session cache fill, which is correctness-safe but can cause extra reads.

[rwmjhb]

Thanks for the update. After the recent merges, current head 551d825 now has merge conflicts with the latest master, so the approved revision cannot be merged as-is. Please rebase onto the latest master, resolve the conflicts, rerun the relevant targeted tests and full CI suite, and push the resolved head for a quick re-check.

[gorkem2020]

Rebased onto current master (post #952) as requested. Full suite green, dist rebuilt.

[rwmjhb]

Reviewed head e376345 after the rebase. The generation guard is conservative, source/dist are aligned, CI and the orchestrator full suite pass, and I found no merge-blocking regression.

Non-blocking follow-up: strengthen the race regression with a direct positive control proving the derived-session cache publication path is exercised; the process-global generation may also cause avoidable cache misses across unrelated sessions.

[rwmjhb]

Thanks for the update. After #934 merged, current head e376345 now has merge conflicts with the latest master, so the approved revision cannot be merged as reviewed. Please rebase onto the latest master, resolve the conflicts while preserving the reviewed abort-safe derived session cache behavior, rerun the relevant targeted tests and full CI suite, and push the resolved head for a quick re-check.

[gorkem2020]

Rebased onto current master (post #934): package.json test-chain union was the only conflict. Typecheck, build (dist recommitted), manifest verifier, and the full suite are green. Mergeable again.

[rwmjhb]

Re-reviewed rebased head 67fa7e4. The rebase only unions the test registration, and the generation check still prevents a concurrent same-process delete from being followed by a stale derived-session cache publication. The focused race tests, full suite, and GitHub CI are green.

The synthetic dispatch used by the race test and the process-wide generation counter are reasonable follow-ups; the latter can cause an unrelated cache miss but remains fail-safe for correctness.

Approving.

[rwmjhb]

PR #941 has now merged, and this branch currently has merge conflicts with the latest master. Please rebase onto current master, resolve the conflicts, and push the updated branch. We will re-review the new head after CI completes.

[gorkem2020]

Rebased onto current master (post #941); registration files re-unioned, typecheck and the full suite pass, dist rebuilt in-commit.

[rwmjhb]

Re-reviewed rebased head f44c5fe. The conflict resolution preserves the generation guard and cache-invalidation race fix; targeted tests, the full suite, and GitHub CI are green.

The production-event positive control, broader DB-store/delete ordering, and process-wide generation granularity remain non-blocking follow-ups.

Approved.

[rwmjhb]

PR #944 has now merged, and this branch currently has merge conflicts with the latest master. Please rebase onto current master, resolve the conflicts, and push the updated branch. We will re-review the new head after CI completes.

[gorkem2020]

Rebased onto current master (post #944 and #986), registration union resolved, full gates green. Mergeable again.

[rwmjhb]

Re-reviewed rebased head 3a3c061. The conflict resolution preserves the previously approved generation guard; targeted tests, the full local suite, and all GitHub CI checks pass. The retriever dist changes are regenerated output for source behavior already present on the current base.

A production-event positive control for the synthetic non-boundary dispatch, broader delete-before-store ordering coverage, and process-wide generation granularity remain non-blocking follow-ups.

Approved.

[gorkem2020]

Rebased onto master after #942's merge (registration-chain union only, no source conflicts). Full gates green: typecheck, fresh dist, manifest verifier, full suite.

[gorkem2020]

Rebased onto master after #955's merge (registration unions only). Full gates green.

[gorkem2020]

Rebased onto master after #960's merge (registration unions only). Full gates green.

[gorkem2020]

Rebased onto master after #964's merge (registration unions only). Full gates green.