fix(stores): guard vector reconciles against stale-load overwrites (ARN-216) - #377
fix(stores): guard vector reconciles against stale-load overwrites (ARN-216)#377nerdsane wants to merge 4 commits into
Conversation
…e (ARN-216) RED: the vector backfill is load-then-reconcile with nothing spanning the two store calls, and backfill_entity_vectors is an unconditional delete+insert — a live write landing between them co-commits the new embedding and the stale reconcile then overwrites it, right before the watermark stamps the index complete. 100 seeds, exact production interleave against the store API. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…RN-216) GREEN: backfill_entity_vectors carries as_of_sequence — the journal sequence its rows were derived from — and every store skips the reconcile when the entity's journal has advanced: sim checks under its mutex, turso inside its Immediate transaction, and postgres runs the DELETE first (row locks serialize concurrent reconciles) then re-checks under those locks and rolls back, since READ COMMITTED makes a check-then-delete ordering non-atomic. The loader outcomes carry the replayed sequence for both the index-write and purge arms; the turso write-behind passes its append's sequence so a retried lagging write cannot clobber a later one. Guard-skips count as success: a skip means a newer live write reconciled the entity itself. Also: StorageStack extracted verbatim from storage/mod.rs to storage/stack.rs — the plumbing pushed the workspace's largest file past the readability ceiling; the extraction gives permanent headroom. ADR-0161 records the design, the pg ordering, the key-backfill residual (same clobber shape, symmetric follow-up), and the rolling-deploy guard-skip assumption. Review trail: GREEN r1 FAIL (pg guard non-atomic under READ COMMITTED) fixed with the reviewer's DELETE-first reorder; r2 PASS with all interleaves verified including the PK backstop and deadlock analysis. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Live local E2E evidence (ARN-216)Setup: Live vector surface on the fixed binary — co-commit + Nearest end-to-endThe re-embedded vector governs ranking immediately (co-commit), through the same The race itself — deterministic proofA live race window is inherently timing-dependent, so the before/after proof of the |
|
Process note: the local pre-push gate's test phase rejected this branch once; an immediate standalone |
Independent reviewer (Claude Fable 5, dedicated session) — ARN-216 / PR #377Reviewed the open PR diff and both PR comments; verified the interleave reasoning against the code in a detached worktree at head What is solid (verified, not taken on faith)
Findings[P2] The ADR's postgres bullet asserts the guard "re-checks the journal under the taken row locks, rolling back when it advanced." That is only true when the entity already has index rows for the DELETE to lock. In the initial-backfill case — a pre-existing entity indexed for the first time, i.e. exactly what this backfill exists for — the entity has no prior
It is narrow and self-healing (the entity's next write co-commits a DELETE across all its model partitions), and it is strictly better than main (which corrupts even the mainline). So the code residual itself is P2. What I'd insist on is honesty parity: the ADR records the key-backfill and rolling-deploy residuals but omits this one, and the postgres "under the taken row locks" wording actively overstates the guarantee for the no-prior-rows path. Either tighten the guard for no-prior-rows (e.g. an explicit per-entity/segment lock so the re-check is truly serialized — heavier, the ADR-rejected direction) or, consistent with how the other two residuals are handled, correct the postgres bullet and record this window as a known residual. Right now the ADR claims a guarantee the code does not provide in its own primary scenario. VerdictThe mainline defect (same-model stale reconcile overwrite) is genuinely and well fixed across all three stores, with a real RED/GREEN and correct sim/turso atomicity. The one thing blocking a ship is the ADR overstating the postgres guarantee for the no-prior-rows/cross-model window it doesn't actually cover — a cheap correction, but one I'd insist on before merge. Verdict: FAIL |
…estly (ARN-216) The dedicated PR reviewer's P2: the DELETE-first row-lock serialization is vacuous when no prior rows exist, and a cross-model live re-embed racing that window commits under a different primary key — a transient, self-healing stale row in the old model partition that the previous ADR wording claimed was covered. Corrected the postgres bullet and recorded the window as a known residual, consistent with the other two. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Independent reviewer (Claude Fable 5, dedicated session) — ARN-216 / PR #377 (re-review)Re-reviewed head The postgres Consequences bullet no longer claims the re-check happens "under the taken row locks" unconditionally. It now states the DELETE's row locks serialize concurrent reconciles only when prior rows exist; that in the no-prior-rows case a same-model race is caught fail-safe by the index primary key (stale INSERT collides → errors → the type is not watermarked, so a corrupt index is never declared complete); and that a cross-model live re-embed racing the window between the re-check and the stale INSERT commits under a different PK, leaving a transient stale row in the old model partition that self-heals on the entity's next write. It is recorded as a known residual alongside the key-backfill follow-up rather than closed with the heavier per-entity lock the Alternatives section rejects. Verified against the code: the wording is accurate — the fail-safe/collision reasoning and the cross-model window match the interleave I walked through, and the guard's actual mechanism (DELETE-first, lock-free Everything from my first pass still holds and was not re-litigated: root cause is right; sim (mutex) and turso (Immediate tx) checks are atomic with equality-proceeds correct; write-behind passes its own No open findings. Verdict: PASS |
|
@greptile review |
… pg aggregate (ARN-216) Greptile findings: the turso guard-skip dropped an Immediate transaction without an explicit rollback (async Drop cannot await; the RESERVED lock release was deferred to libsql's synchronous drop hook — now released deterministically, matching the postgres path), and the pg guard used fetch_optional on an aggregate that always returns one row (fetch_one, dead unwrap removed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@greptile review |
|
ARENA SHIPPABLE · Claude Code (Fable 5) · 2026-07-12 22:08 PDT Receipts (final head 69e6028):
Residuals in ADR-0161 (all honest, all tracked for Linear on reconnect): symmetric key-backfill guard (same clobber shape), the pg cross-model no-prior-rows transient window (self-healing), and the rolling-deploy guard-skip assumption. |
Fixes ARN-216 (
Vector backfill races live writes and can mark a stale index complete).Defect
The ADR-0155 vector backfill is two store calls with nothing spanning them: load an entity's state (snapshot/replay at some journal sequence), then
backfill_entity_vectors— an unconditional delete-then-insert reconcile. A live write landing between them co-commits the new embedding, and the reconcile then overwrites it with rows parsed from the stale load. The backfill then stamps the completion watermark, so reads treat the stale index as authoritative until the entity's next write. The Turso write-behind retry path has the same shape (a retried lagging index write can land after a newer one).Fix (root cause)
as_of_sequencestaleness guard. Every reconcile carries the journal sequence its rows were derived from; each store checks the entity's current journal sequence inside its own transaction/lock and skips the reconcile when the journal has advanced — the newer write's co-commit already holds newer rows. Applied to the sim, postgres, and turso stores; the turso write-behind passes its append's sequence;u64::MAXescape for callers with known-current rows.EntityLoadOutcomenow carries the replayed sequence so both the index-write and tombstone/phantom purge arms pass the true as-of. Guard-skips count as success for the watermark — a skip means a newer live write reconciled the entity itself.ADR-0161 records the design, the alternatives (entity-wide locking; re-load-and-diff — both rejected as still racy or heavier), and one residual: the declared-key backfill has the same load-then-write shape (per-key upserts rather than whole-entity reconciles); a symmetric guard is a follow-up, not silently folded in.
Also in this PR (ratchet-driven): the 4 plumbing lines pushed
storage/mod.rs— the workspace's largest file — past thePROD_MAX_FILE_LINESceiling, fixed by extracting the cohesiveStorageStackblock verbatim tostorage/stack.rs(2833 → 2702, permanent headroom).TDD
d2ba6fd5(committed alone): 100-seed DST executing the exact production interleave — stale rows built, liveReembedco-commits E2, stale reconcile — fails on main with E1 clobbering E2, every seed. The pre-commit reviewer additionally ran a causal probe (removing the reconcile makes it pass) proving the failure is precisely the stale overwrite.Verification
dst_entity_vector_index3/3 (100 seeds); lib 575/575 (1 flaky-class failure once under load, clean re-run — same pre-existing class documented on fix(server): journal PATCH/PUT field updates fail-closed (ARN-189) #373/fix(server): re-arm pending state timeouts at boot and on creation (ARN-203) #375/fix(server): release declared-key ownership on delete and null (ARN-238) #376); nearest/projection suites green; clippy-D warnings, ratchet, fmt clean on default and sim builds.Greptile Summary
This PR fixes ARN-216, a race condition where the vector backfill's unconditional delete-then-insert reconcile could overwrite a newer live co-committed embedding with rows built from a stale entity load. The fix adds an
as_of_sequencestaleness guard tobackfill_entity_vectorsthat each store checks inside its own transaction/lock, skipping the reconcile when the journal has advanced.tx.rollback().awaiton skip; Sim uses the mutex-held journal directly.EntityLoadOutcomenow carries the replayed sequence so both the index-write and tombstone/phantom purge arms pass the trueas_of_sequence; write-behind retry passes the append's ownnew_seq.StorageStackextracted verbatim tostorage/stack.rsto stay within the file-line ratchet; a 100-seed DST pins the exact production interleave that was RED on main.Confidence Score: 5/5
Safe to merge. The guard is applied consistently across all three store backends, each ordered to match its isolation model, and a 100-seed DST pins the exact race interleave that was broken on main.
The guard logic in each store is short and directly testable. The Postgres DELETE-first ordering is intentional and documented in both the inline comment and ADR-0161. The Turso Immediate-transaction check-first ordering is correct for that isolation level, and the explicit rollback addresses the previously flagged libsql drop-hook concern. The StorageStack extraction is verbatim with no behavioral change. Residuals are correctly documented and non-blocking.
No files require special attention. The DELETE-first guard ordering in crates/temper-store-postgres/src/store.rs is intentional and documented - reviewing it against ADR-0161 confirms correctness.
Important Files Changed
as_of_sequence: u64parameter to thebackfill_entity_vectorstrait method with a comprehensive doc comment explaining the guard semantics; default no-op updated accordingly.fetch_onereplaces the previousfetch_optional+ deadunwrap_or. Explicittx.rollback()on guard-skip is correctly awaited.tx.rollback().awaitbefore return on guard-skip; write-behind retry path now passesnew_seqasas_of_sequence.sequence_nrdirectly.loaded_seqthrough to everybackfill_entity_vectorscall site including theSkip-branch purge.dst_vector_backfill_must_not_overwrite_newer_live_write- a 100-seed DST that precisely recreates the production interleave and asserts the index holds the live embedding post-guard.Reviews (2): Last reviewed commit: "fix(stores): explicit turso rollback on ..." | Re-trigger Greptile