fix(concurrency): fleet-safe decisions + crash windows (step 1, part 1)#483
Draft
plind-junior wants to merge 5 commits into
Draft
fix(concurrency): fleet-safe decisions + crash windows (step 1, part 1)#483plind-junior wants to merge 5 commits into
plind-junior wants to merge 5 commits into
Conversation
step 1.2 of the fleet-concurrency work. move_proposal_to_decided writes decided/ then unlinks proposed/; a crash between the two leaves both files. but get_proposal checked proposed/ first and list_proposals appended both, so a recorded decision was masked by the leftover pending copy — a human "no" could resurface as pending and later flip to a durable "yes", and the proposal showed up twice in the queue. get_proposal now reads decided/ first; list_proposals dedups by id preferring decided and returns a deterministic id-sorted list. move_proposal_to_decided fsyncs the decision before unlinking the pending copy (new _fsync_file helper), so a crash can leave both copies — readers prefer decided — but never neither. pinned by two tests that reconstruct the exact crash window (decided/ written, proposed/ not yet unlinked) and assert the decision wins.
step 1.7 (first of two one-liners). Page.status defaults to DRAFT and the approve path built Page(**payload) without ever promoting it, so every reviewed page read as draft forever — the wiki served approved knowledge with a draft badge, and PageStatus.STALE had no writer to demote from. approve()'s PAGE branch now stamps PageStatus.ACTIVE at the gate, which is where a page becomes live. pinned by a test asserting an approved page is ACTIVE in-memory and on disk. the second one-liner (the id(conn) sqlite-vec load cache, index_db.py) is deferred: it needs the embeddings extra to exercise and rides with the retrieval work.
…n lock step 1.1 of the fleet-concurrency work, and the headline race. approve(), reject() and expire_one() each do a read-check-write — get the proposal, check it is pending, write the artifact, move it to decided/, audit — with no lock. five agents across console + mcp + cli make racing decisions normal, so an approve and a reject of the same proposal could both read it pending and both proceed, leaving a durable claim under a REJECTED record. factor audit's flock body into a reusable `_flock`, add `decision_lock(kb_dir)` on a distinct `decisions.lock`, and hold it across each decision's whole read-check-write. the second caller now observes the first's result and fails the not-pending check. the audit lock nests inside the decision lock in a consistent order (decision outer, audit inner via log_event), so there is no deadlock; expire_pending calls expire_one without holding the lock, so the per-item acquire does not nest either. pinned by a deterministic test: with the decision lock held from another thread, approve() blocks until it is released, then completes.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
storage
the fsynced audit log must never end up attesting to a claim, page or
proposal whose yaml a crash left torn or erased. previously each artifact
write went straight to its final path (open("x") + write, or write_text),
so power loss mid-write could leave a half-written file the subsequent
fsynced audit event then vouched for.
generalize the fsync-before-unlink helper (_fsync_file, added by the
decided/-wins fix) into a shared _atomic_write: write a temp sibling in
the same directory, fsync it, then place it with an atomic rename --
os.replace to overwrite, or os.link (FileExistsError if present) to keep
the create-only guard put_claim / put_page / put_proposal previously got
from open("x"). a crash now leaves either the old bytes or the complete
new bytes, never a mix.
every durable artifact write routes through it: claims, pages, entities,
relations (incl. the idempotent path), evidence, sessions, source meta,
proposals, and the decided-copy in move_proposal_to_decided (whose inline
fsync it subsumes). mirrors migrations.rewriter.atomic_write_text, which
imports from here and so keeps its own copy.
foundation for the audit intent/commit ordering and the fsck crash-window
states, which both assume artifact writes are already atomic.
github-actions
Bot
added
size: L
state.db is derived, but approve() wrote its FTS row on the critical path between put_<kind>() and move_proposal_to_decided(). a locked or broken index there aborted the approve after the artifact was already on disk, leaving the file present with the proposal still pending -- and the retry hit _ensure_no_existing_artifact and bricked the approve for good. route the three approve-path index writes (claim, page, entity) through a new _index_or_degrade: on sqlite3.Error it logs and continues, so the decision is still recorded and the approve event still logged. the missing row rebuilds with `vouch index`. mirrors update_claim's existing degrade-on-sqlite-error handling. open_db now sets journal_mode=WAL and busy_timeout=5000. five concurrent agents on one kb is the dogfood config, so readers should not block the single writer and a contended writer should wait for the lock rather than fail immediately with "database is locked". the -wal/-shm sidecars are already gitignored (state.db-*). update docs/multi-agent.md's concurrency section, which predated this work: atomic durable writes, approvals serialised under the decision lock, a wal-mode index that never blocks a write, and best-effort indexing that degrades instead of bricking an approve.
github-actions
Bot
added
docs
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.
step 1 of the fidelity-pivot roadmap: fleet-safe concurrency + crash windows.
five agents on one KB is the dogfood configuration, so multi-process
correctness is a prerequisite for phase d's mechanical gate, not hygiene. this
PR lands the three highest-value fixes; the remaining durability items
(atomic tmp+fsync+os.replace writes, WAL + busy_timeout, audit intent/commit
ordering, fsck crash-window states) follow.
1.1 — the decision race (headline). approve/reject/expire_one each do an
unlocked read-check-write, so a concurrent approve and reject of the same
proposal could both read it pending and both proceed, leaving a durable claim
under a REJECTED record. audit's flock body is factored into
_flock; a newdecision_lock(kb_dir)on a distinctdecisions.lockis held across each wholedecision. the audit lock nests inside it in a consistent order (no deadlock).
pinned by a deterministic test: approve() blocks while the lock is held from
another thread, then completes.
1.2 — decided/ wins over a stale proposed/ copy. move_proposal_to_decided
writes decided/ then unlinks proposed/; a crash between leaves both. get_proposal
read proposed/ first and list_proposals returned both, so a recorded "no" could
resurface as pending and later flip to a durable "yes". decided/ now wins in
both, and the move fsyncs the decision before unlinking (so a crash can leave
both copies but never neither).
1.7 (first one-liner) — approved pages read as draft forever. Page defaults
to DRAFT and approve never promoted it; the PAGE branch now stamps ACTIVE at the
gate. (the id(conn) sqlite-vec cache one-liner rides with the retrieval work —
it needs the embeddings extra to exercise.)
gate green: 1320 passed, mypy, ruff. every fix test-first.