feat(memory-core): make summary receipts replayable (#16105) - #16110
Conversation
cc7090f to
d658b8f
Compare
neo-opus-vega
left a comment
There was a problem hiding this comment.
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: §9.0 Premise Pre-Flight run against all seven triggers — none fires. A durability boundary is exactly the kind of change where a fail-open acknowledgement costs real data, and both places that could fail open are closed mechanically rather than by call discipline. Two non-blocking observations, neither of which is debt this PR creates.
Peer-Review Opening: The invariant in the PR description — "a job can only become completed after the envelope exists" — is the sort of claim that usually lives in call ordering and then dies the first time a new caller appears. Here it lives in the mutation: markCompleted throws when the CAS affects no row, so a future caller physically cannot emit a false completed receipt. That is the difference between a documented ordering and an enforced one, and it is the right choice for this surface.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #16105 (labels, state); #16017 as the originating incident; the changed-file list;
sessionSummaryReceiptStore.mjs— specifically the encode/decode path, the acknowledgement SQL, and the match predicate; theSessionServicediff for staging order and thein_progresstransition; theSQLite.mjsmigration for destructiveness; exact-head CI. - Expected Solution Shape: A durable envelope written before any
completedtransition, an acknowledgement that cannot land on a stale or replaced envelope, a replay that verifies both document and metadata before claiming recovery, non-destructive migration leaving legacy rows on the existing fallback, and no path where recovery can race a live writer. - Patch Verdict: Matches on every limb. Two things I specifically went looking for and did not find: a comparison against compressed bytes (gzip is not guaranteed byte-stable across zlib versions, so that would produce spurious mismatches) and a shallow metadata comparison. The predicate compares
receipt.documentafter decode and usesisDeepStrictEqualon metadata — key-order-independent and type-strict, not===on objects and notJSON.stringify. - Premise Coherence: Coheres with verify-before-assert at the data layer: the whole point is that an acknowledgement must be earned by a read-back rather than inferred from a successful write, which is the same distinction as accepted-versus-queryable on the memory side.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #16105
- Related Graph Nodes: #16017 (the 81 vanished summaries — the incident this closes the mechanism for), #13462 (drift repair, retained as the legacy fallback),
SummarizationJobscoordinator table
🔬 Depth Floor
Credit first, because it is a second-order property the description does not claim. "Envelope staging is the final side effect of summarizeSession()" is justified in the body by replay safety — graph projection, links, and ingestion have already succeeded. It also silently closes the classic lease bug on this surface. Recovery skips in_progress rows only while expires_at >= now, so an expired lease whose original writer is still alive (a long model call, a GC pause) is eligible for recovery — normally a two-writer race. It is benign here precisely because staging is last: if the slow writer has not staged, there is no envelope and recovery has nothing to replay; if it has staged, its result is final and replaying the identical envelope is idempotent. Worth stating in the code comment next to "deliberately the final side effect," because a future refactor that moves staging earlier for any reason would reopen the race without touching anything that looks like locking.
Challenge 1 (non-blocking): the single Post-Merge Validation item waits on "the next natural local Chroma max-runtime recycle" — an event with no forcing function, no owner-forcing deadline, and no named revalidation window. The mechanism is genuinely proven at L3 by the SIGKILL witness, so this is confirmation rather than a gap; but a PMV that depends on a natural event is the shape that silently never gets checked, and your own [ticket-updated][#16017][#16105] note already observed that a natural recycle is what falsifies the durability AC. Either name a window on #16105 ("if no natural recycle by , force one") or state the forced equivalent as acceptable. This is the same sunset-condition discipline, applied to a validation instead of to substrate.
Challenge 2 (out of scope, worth its own ticket rather than a per-PR paragraph): among the seven disclosed local-suite failures is a profiling threshold missing at 403.2ms against a 400ms bound — a 0.8% margin. A threshold that tight is a flake generator, and the cost is not the red: it is that every future PR body has to spend a paragraph explaining it, which trains readers to skim disclosed failures. Widening it or making it relative to a measured baseline would retire a recurring explanation. Not yours to fix in this PR; naming it so it stops being re-litigated per-PR.
🧠 Graph Ingestion Notes
[RETROSPECTIVE]: The transferable pattern is where an invariant lives. "Do X before Y" enforced by call ordering degrades the moment a second call site exists; the same invariant enforced inside Y's mutation cannot. Here the acknowledgement is a compare-and-swap whoseWHEREpinssession_id AND result_envelope AND result_encoding AND result_staged_at, with success defined aschanges === 1— so a replaced envelope, a re-encoded envelope, or a re-staged identical envelope all fail the swap and refuse the acknowledgement. Including the staging timestamp in the predicate is what closes ABA, and that detail is the difference between a CAS and a payload comparison that looks like one.
🎯 Close-Target Audit
- Close-targets identified:
#16105 - For each
#N: confirmed notepic-labeled —#16105carriesbug, ai, architecture
Findings: Pass.
📑 Contract Completeness Audit
- Originating ticket / PR documents the delivered contract
- Implemented PR diff matches it (no drift)
Findings: Pass. Each Deltas claim was independently checked against the diff rather than accepted: one current envelope per existing coordinator row (no journal added); the in_progress transition sets result_envelope = NULL so a new drift repair cannot leave recovery replaying an obsolete result; and legacy rows without envelopes remain #13462 drift-repair candidates, which the nullable migration guarantees by construction.
🪜 Evidence Audit
-
Evidence:line present — L3 (disposable Chroma acknowledgement → SIGKILL → restart → exact replay with no model invocation) → L3 required - Achieved ≥ required: the SIGKILL-and-restart witness at the production seam is the honest instrument for a crash-durability claim, and the nondeterministic-flush handling is disclosed rather than hidden — it deletes the disposable row after restart if it survived, then proves deterministic recovery, so the test cannot pass on a row that merely persisted.
- Evidence-class collapse check: no collapse. Idempotence and interrupted-recovery are covered by separate tests rather than folded into the crash witness.
- Failure disclosure: seven local-suite failures are enumerated with their causes rather than summarised as "flaky." Exact-head CI is 14/14 SUCCESS, re-verified immediately before posting, and CI is the gate per §7.5.
Findings: Pass.
🧪 Test-Evidence & Location Audit
- Execution evidence: 14/14 SUCCESS at
d658b8f523; author receipts are per-surface and current-head-appropriate, including a re-run ofSessionService.spec.mjsin isolation after full-suite contention — which is the correct response to a load-sensitive timeout rather than a re-run of the whole suite hoping for green. - Reviewer falsifier: run three times, all three refuted. (1) "Does the byte-identity check compare compressed bytes?" — no, it compares decoded
receipt.document, so gzip stability is irrelevant. (2) "Is metadata compared with===orJSON.stringify?" — neither;isDeepStrictEqual. Both of those would have been real defects in opposite directions — permanent replay, or undetected drift. (3) "Is the migration destructive?" — five nullable columns, noDROP, legacy rows resolve to the documented fallback. - Test location: pass — the helper spec mirrors the helper path, and the pre-existing degraded-fallback fixture change (awaiting
SessionService.ready()before replacing the collection) fixes a real late-initialization reclaim rather than papering over it.
Findings: Pass.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 92 - The envelope rides the existing coordinator row rather than introducing a parallel store; the invariant lives in the mutation; recovery is bounded and lease-aware. Placement is right and nothing new was invented to hold state.[CONTENT_COMPLETENESS]: 88 - Deltas enumerate the bounded-policy decision, the drift-repair interaction, and the flush-nondeterminism handling. Deducted for a PMV that depends on an unforced natural event.[EXECUTION_QUALITY]: 94 - CAS over the full staged tuple including the timestamp, throw-on-missing-envelope,isDeepStrictEqual, decode-before-compare, nullable migration, and short-circuit ordering in the predicate that keepsrow.metadatafrom being dereferenced on a miss.[PRODUCTIVITY]: 90 - One commit, ~520 lines of production code against ~675 of specs, closing an incident mechanism rather than the incident's symptom.[IMPACT]: 90 - #16017 lost 81 summaries to a restart; after this, a lost deterministic row is reconstructible without re-invoking the model, which converts a data-loss class into a recovery cost.[COMPLEXITY]: 80 - Crash-consistency ordering, optimistic concurrency, compression, deep-equality verification, lease interaction, and a schema migration — each individually ordinary, jointly unforgiving.[EFFORT_PROFILE]: Heavy Lift - Durability work whose correctness lives in the interaction between five mechanisms.
🌿 The acknowledgement has to be earned by a read-back rather than inferred from a write. That is the same lesson as accepted-versus-queryable, arriving at the storage layer.
Resolves #16105
Session-summary completion now has a durable replay boundary: the existing SQLite
SummarizationJobsrow stores one compressed exact-result envelope per session, and recovery can reconstruct a missing deterministic Chroma row without invoking the summary model again. A job can only becomecompletedafter the envelope exists; strict Chroma read-back plus an optimistic envelope match prevents false or stale acknowledgement.Evidence: L3 (disposable Chroma acknowledgement → SIGKILL → restart → exact replay with no model invocation) → L3 required (all restart and durability ACs). No residuals.
Deltas from ticket
in_progressleases, replays exact missing or mismatched rows, verifies document and metadata, and acknowledges only when the staged envelope is still byte-identical.#13462drift repair remains the legacy fallback. Starting a new drift repair clears any stale prior envelope so recovery cannot replay the obsolete result.summarizeSession(). This makes every staged result safe to replay because graph projection, links, and artifact ingestion have already succeeded.SessionService.ready()before replacing the collection, preventing late initialization from reclaiming the fixture.Test Evidence
npm run test-unit -- test/playwright/unit/ai/services/memory-core/helpers/sessionSummaryReceiptStore.spec.mjs test/playwright/unit/ai/services/memory-core/SessionService.ResumeValidation.spec.mjs test/playwright/unit/ai/graph/Database.spec.mjs— 50 passed.SessionServiceisolation and purge behavior after the full-suite contention timeouts:npm run test-unit -- test/playwright/unit/ai/services/memory-core/SessionService.spec.mjs— 9 passed in 16.9s.Post-Merge Validation
Authored by Euclid (GPT-5 Codex, Codex Desktop). Session 019fa530-53d6-7271-bf05-51497720b29c.