fix(db,runtime,mcp,comm): guard six read-modify-write paths with compare-and-swap - #2233
Closed
ohdearquant wants to merge 10 commits into
Closed
fix(db,runtime,mcp,comm): guard six read-modify-write paths with compare-and-swap#2233ohdearquant wants to merge 10 commits into
ohdearquant wants to merge 10 commits into
Conversation
Entity and edge writes went through unconditional upserts, so a write built from a stale read could silently overwrite a concurrent writer's change. Add replace_entity_if_unchanged/replace_edge_if_unchanged, matching the existing replace_note_if_unchanged compare-and-swap shape: the update only applies when the row's revision still matches what was read, otherwise it reports zero affected rows instead of clobbering.
Runtime entity/edge updates and their atomic-plan counterparts wrote patched rows back unconditionally, so a write built from a stale read could silently overwrite whatever a concurrent writer had already committed. Route entity and non-symmetric edge updates, plus their atomic update plans, through the new replace-if-unchanged primitives, and surface a conflict error (or, for atomic plans, a whole-unit rollback) when the row's revision no longer matches what was read. Also fixes a latent gap in edge updates: the non-symmetric branch never bumped updated_at before writing, which under the old unconditional-upsert path was harmless but would make every legitimate edge update fail the new revision guard.
handle_heartbeat wrote its patched channel-status note back with an unconditional upsert, so two concurrent heartbeat reports for the same channel could both write from the same snapshot, with the second silently discarding the first's fields. Guard the write with replace_note_if_unchanged when a prior heartbeat row exists, and surface a conflict error naming the channel when it moved since it was read. The first heartbeat for a channel still writes unconditionally, since there is no prior revision to guard against.
The scheduler's drain loop finalized fired events (writing terminal status and payload) guarded only by claim-token predicates, with no check for a property write landing between the loop's last read and its terminal write — such a write would be silently overwritten. Add a fresh raw-text read of the stored properties column immediately before the terminal write, and extend the finalize guard to also require that column to still match. A mismatch leaves the row firing instead of finalizing over the concurrent change.
ohdearquant
marked this pull request as ready for review
August 26, 2026 21:11
…edule finalization, and revision advance Extends the entity/edge optimistic-concurrency guard to the symmetric-edge update path (both the canonical raw-SQL branch and the atomic plan), which previously carried no expected-revision predicate and could silently overwrite a concurrent writer. Makes every schedule-event finalization branch read the row's current properties at the same read boundary and pass them as a mandatory exact-match guard, so a property write landing between claim and finalization is no longer discarded — the guard parameter is no longer optional, so a future caller cannot silently drop it. Clamps entity and edge replacement revisions to strictly advance past the read snapshot (mirroring the existing note-path fix) instead of assigning a raw clock read, so two writes inside one microsecond or a backward clock step are not misreported as a stale conflict. Adds regression tests that exercise the production entry points themselves (`update_entity_with_embedding_report`, `update_edge`, `handle_heartbeat`) under a deterministic test-only read/write barrier, rather than only the underlying store primitives, so a reverted production wiring is caught. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fix finalizer/heartbeat revision handling Symmetric-edge conflict absorption (canonical update_edge_symmetric_dml and the --atomic prepare path) deleted the non-canonical row without checking whether its own revision had changed since the caller's snapshot, letting a stale writer silently discard a concurrent edit. Both paths now bind the fetched snapshot's updated_at/deleted_at as a CAS fence and refuse with the same typed conflict the in-place update arm already used. Schedule finalization's dispatch-success and pre-action call sites in the pending-events drain loop rebuilt their written properties from the pre-claim page-query snapshot instead of the freshly read current-properties value obtained for the CAS guard, discarding any property written between claim and finalization. All six call sites now build their write from the fresh read. Comm heartbeat derived its replacement revision from Utc::now() alone, which could equal or trail the stored revision on same-microsecond writes or a backward clock step and spuriously refuse a legitimate heartbeat. It now advances strictly past the existing snapshot, mirroring the note/edge CAS paths. Also corrects stale atomic_prepare.md documentation describing the non-symmetric edge update as an unconditional upsert; it has been a guarded CAS replace since the revision-advance work landed.
…e prose The concurrent-race fixtures cannot attribute a failure to a specific conjunct of the entity CAS. In their setup `updated_at = ?13` and `?8 > updated_at` are each independently sufficient to refuse the losing writer, so tautologizing either one alone leaves them green. Mutation over the suite showed `?8 > updated_at` had no regression coverage at all: defeating it reddened nothing. Add an isolating fixture that supplies the correct expected revision and deletion marker, leaving the strict-advance conjunct as the only predicate that can refuse the write. Under the same mutation it is now the single reddening test. Correct the race fixtures' doc comments to state what they actually detect, and scope the revision-clamp tests as clamp coverage rather than CAS regression coverage. Move the pending-events race seam from before the claim to immediately before the pre-finalize read. Parked earlier, a concurrent write landed inside the candidate-page snapshot, so the drain regression passed whether finalization rebuilt from that stale page or from the fresh read. Parked at the new position it reddens on the stale-page variant. Correct the symmetric-edge comment that named the plan-shape builders as what the canonical path binds. Canonical binds the shared SQL constants directly; the builders serve the atomic prepare path.
…laims Finalization re-read the row's properties and guarded its write on them, but still computed the repeat decision from `trigger_at`, `trigger_offset` and `repeat` parsed off the pre-claim candidate page. That protects the properties blob while leaving the scheduling decision stale: a writer who cleared `repeat` or moved `trigger_at` in the claim window had their edit retained as the compare-and-swap base and then immediately contradicted by a next occurrence computed from the value they replaced. The three scheduling inputs are now re-derived from the same fresh read the guard uses. The pre-claim values keep their real job, which is admission — is this due, is it inside the grace window — a decision correctly made from what was observed before the claim. What must not come from them is anything written. A `trigger_at` that stops parsing in that window now fails the finalization rather than falling back to the stale pair. Its regression makes the two behaviours differ in terminal state rather than in a timestamp: the event is seeded repeating, the concurrent write clears the repeat, and scheduling from the fresh read ends `fired` while scheduling from the page snapshot reschedules to `pending`. Narrow the entity conjunct-attribution comment to what its fixture can actually establish. Both racers there take their replacement revision from a wall-clock read, and the fixture pins only the two expected revisions equal, never the two replacement revisions, so whether the loser is still refused with the revision-equality predicate removed depends on which clock read won. That is a race, not a property of the fixture, and the previous comment stated it as measured. What the fixture does establish deterministically is unchanged: with the revision predicate intact it cannot see the strict-advance predicate disappear, which is why the isolating fixture exists. Correct the heartbeat scope comment, which said the test never invokes the handler. It does, once, to seed the row. The conclusion was right and is now stated from the wiring mutation that establishes it rather than from a false premise. Delete the claim that a brand-new heartbeat row has no prior state to lose. It answers the wrong question: the hazard on that branch is two concurrent first writes, which both observe an absent row and both take the unguarded insert. The branch is knowingly left unguarded and its comment now says so; closing it needs an insert-if-absent primitive that reports whether it inserted, which does not exist yet and is tracked separately.
The revision-based race fixtures cannot reach `deleted_at IS ?14`, because a soft delete does not move the revision: the soft-delete UPDATE sets `deleted_at` where it is currently NULL and never touches `updated_at`. A writer holding a pre-delete snapshot therefore still matches on the expected revision after the row is tombstoned, and its replacement revision still advances, so the deletion marker is the only conjunct that can refuse it. Drop that conjunct and the stale write lands with `deleted_at = NULL`, resurrecting a deleted entity with no revision conflict anywhere to signal it. Measured: tautologizing the conjunct reddens this test and nothing else in the crate — 1391 other tests stay green — so it had no regression coverage at all. The fixture asserts both other conjuncts against the post-delete row before running the compare-and-swap, so a change that makes one of them refuse instead turns the test red rather than quietly converting it into a whole-guard test. Also corrects the adjacent doc comment, which still asserted that the revision and strict-advance guards are each independently sufficient in the concurrent fixtures. They are not: those fixtures pin the racers' expected revisions equal but never their replacement revisions, so the outcome of removing one guard depends on which clock read won. The comment now states only what the fixture forces.
Owner
Author
|
Closing in favour of two narrower changes. This branch bundled two independent fixes. Splitting them keeps each test Guarded entity and edge replacement — including the tombstone-resurrection The No commit from this branch is being carried forward as-is. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1753.
Six production read-modify-write paths read a row, computed a new value from that snapshot, and wrote it back unconditionally. A concurrent writer landing between the read and the write was silently overwritten: no error, no counter, no log. The loser of the race simply lost, and the surviving row looked normal.
Each of the six now goes through a compare-and-swap write. The pattern is the one already established by
replace_note_if_unchanged(crates/khive-db/src/stores/note.rs): the update carries its snapshot's revision in theWHEREclause, so a writer whose snapshot has gone stale affects zero rows and finds out, instead of clobbering.What changed
Two new shared primitives, mirroring the existing note primitive:
replace_entity_if_unchangedreplace_edge_if_unchangedBoth express the write as
UPDATE ... WHERE id = ? AND updated_at = ? AND deleted_at IS ?with a monotonic-timestamp condition, and report affected rows so the caller can distinguish "applied" from "someone else moved it".Threaded through the six sites, each using the guard form appropriate to its layer:
khive-pack-commkhive-runtime(curation)khive-runtime(operations)khive-runtime(atomic_prepare)khive-runtime(atomic_prepare)khive-mcpSchedule finalization is deliberately not a plain revision check. Its update matches on the claim itself — status, firing timestamp, and dispatch invocation id, with an optional exact-properties equality — and returns success only on exactly one affected row. Pinning the claim rather than the revision is the stronger condition for that path, because it also rejects a finalize from a superseded dispatch.
Not in scope
khive-pack-code's ingest sites share the same shape and are deferred to their own change; they are a different subsystem with a different concurrency story, and bundling them here would put two independently reviewable subjects in one PR.No accepted contract changes. Every guard here is an application of an already-ratified mechanism to a new call site, not new policy.
Tests
Each site has a regression that deterministically reproduces two writers proceeding from one revision and asserts the stale writer is refused rather than merged. The
atomic_preparetests additionally assert that a zero-row result fails the enclosing unit rather than passing silently, which is the arm that would otherwise turn a detected race back into a silent one.The per-site regressions are whole-guard tests, and it is worth saying what that does and does not buy. They cannot attribute a failure to one condition of the guard: both racers take their replacement revision from a wall-clock read, and the fixture pins the two expected revisions equal without pinning the two replacement revisions, so removing the revision-equality condition alone leaves the outcome dependent on which clock read won. What they do establish is that with the revision-equality condition intact they cannot observe the monotonic-timestamp condition disappear at all. Mutating that condition on its own reddened nothing in the crate, so it had no regression coverage. An isolating fixture now covers it directly, supplying a correct snapshot revision and deletion marker so it is the only predicate that can refuse the write, and under that same mutation it is the single test that reddens.