fix(store): make item field + parent-link update atomic (BUG-2013)#868
Merged
Conversation
handleUpdateItem committed the field write, then ran SetParentLink/ ClearParentLink as a SEPARATE store transaction. A failure there (cycle discovered late, DB error) returned 500 with half the patch already applied — a code-acknowledged partial-commit window. Fold the parent-link mutation into the SAME transaction as the field write: - store: extract setParentLinkTx / clearParentLinkTx from the public SetParentLink / ClearParentLink (which keep their own tx). Add UpdateItemWithParentLink(id, input, precheck, *ParentLinkUpdate) — UpdateItemWithPreCheck now delegates to it with a nil link. The link write runs after the field UPDATE but before COMMIT, so a failing link write rolls the field write back too. - lock ordering: the NEW parent's advisory key is folded into the update's initial sorted AcquireParentChildrenLocks batch (extraKeys on acquireParentChildrenLocksForUpdate), so setParentLinkTx's later re-lock is an idempotent no-op and the combined update stays deadlock-free. checkParentCycle is parameterized over the queryer so the cycle walk reads inside the tx. - handler: restructured into validate / atomic-write / post-commit stages. The parent-link directive is built once and threaded through all three UpdateItemWithParentLink call sites; the post-commit SetParentLink/ClearParentLink block is removed. Works on both SQLite and Postgres (advisory locks are pg-only; SQLite gets atomicity from BEGIN IMMEDIATE). Adds store tests proving a failing parent-link write rolls back the field change (no partial state) and that the happy path commits both together. Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS
) Codex review flagged a cycle-check TOCTOU: setParentLinkTx walked the ancestor chain BEFORE acquiring the parent-children advisory locks, so two concurrent reparents could each pass on a stale snapshot, block on the lock, then both insert — forming a cycle (A→B→C→A). Move the cycle check to AFTER lock acquisition; under the lock the tx-scoped walk sees the edge the just-unblocked peer committed and rejects the cycle. Pre-existing behavior (the old SetParentLink checked cycles on s.db before even beginning its tx), hardened here since this function was already being refactored. Residual: cycles closed via an edge on an item neither endpoint locks remain possible — a limitation of the per-endpoint lock scheme, tracked separately. Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS
xarmian
added a commit
that referenced
this pull request
Jul 8, 2026
…BUG-2073) (#870) The public SetParentLink/ClearParentLink paths and the shared acquireParentChildrenLocksForUpdate helper had two residual TOCTOU races (pre-existing on main; the NEW atomic UpdateItemWithParentLink path was already made cycle-safe in PR #868 / BUG-2013): 1. Cycle race: SetParentLink acquired only the old+new parent advisory keys, never the CHILD's own (itemID) key. Concurrent SetParentLink(A,B) and SetParentLink(B,A) locked disjoint keys ({B} vs {A}), so both cycle walks passed on stale snapshots and both inserts committed — forming an A<->B cycle. 2. Stale-old-parent race: oldParent was read BEFORE the parent-children locks were acquired and never re-read. A concurrent reparent of the same child committing while this tx waited on locks let it DELETE the newly-committed parent link without holding that real old parent's lock, breaking the open-children guard serialization. The shared read-then-lock helper (UpdateItem/RestoreItem/MoveItem) had the same defect: it read the parent set before locking itemID. Fix, consistent with the PR #868 pattern (sorted lock batches, tx-scoped cycle walk), and deadlock-free: - setParentLinkTx / clearParentLinkTx: fold itemID into the lock set and acquire {itemID + old + new parent} in ONE sorted batch, then RE-READ the old parent under the (now-held) child lock. New readParentLinkTarget helper. - acquireParentChildrenLocksForUpdate: after the sorted acquisition, re-read the parent set under the itemID lock (keysNotIn detects any parent that appeared during the acquisition window). - When a re-read shows the parent set moved, signal the errParentSetChanged sentinel instead of acquiring the moved key out of the canonical sorted order (which could deadlock). The tx-owning callers — SetParentLink, ClearParentLink, UpdateItemWithParentLink, RestoreItem, MoveItemWithPreCheck — wrap their bodies in retryOnParentSetChanged, which rolls back (releasing every advisory lock) and retries from a fresh read. Every acquisition stays a single in-order sorted batch. The signal fires before any commit, so a retry never leaves partial state; bounded by maxParentLockRetries. - RestoreItem: route through acquireParentChildrenLocksForUpdate so it also holds the item's own lock and gets the re-read correction. - CreateItemLink / DeleteItemLink: for child link types, lock the SOURCE item's key in addition to the target's. Attaching/detaching sourceID as a child mutates sourceID's parent set, so sourceID's own lock must be held for the "the child lock freezes an item's parent set" invariant the re-read/retry above depends on. Both keys go through the sorted helper, so the two-key grab stays deadlock-free. Postgres-only races (SQLite serializes writers via BEGIN IMMEDIATE), so the new concurrency tests are gated on the Postgres dialect. They pass with the fix and reproduce the A<->B cycle without it. Out of scope (pre-existing, tracked separately): cycles closed via an edge on an item that NEITHER endpoint locks (e.g. A->B->C->D->A) still slip through the per-endpoint cycle walk — a documented limitation of the per-endpoint lock scheme, not the direct A<->B race this bug names. Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS
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.
Summary
handleUpdateItemcommitted the field write, then ranSetParentLink/ClearParentLinkas a separate store transaction. A failure there (a cycle discovered late, a DB error) returned 500 with half the patch already applied — a code-acknowledged partial-commit window (BUG-2013).This folds the parent-link mutation into the same transaction as the field write, so a single item update is all-or-nothing.
Changes
setParentLinkTx/clearParentLinkTxfrom the publicSetParentLink/ClearParentLink(which keep their own tx). AddedUpdateItemWithParentLink(id, input, precheck, *ParentLinkUpdate);UpdateItemWithPreChecknow delegates to it with a nil link. The link write runs after the fieldUPDATEbut beforeCOMMIT, so a failing link write rolls the field write back too.AcquireParentChildrenLocksbatch (extraKeysonacquireParentChildrenLocksForUpdate), sosetParentLinkTx's later re-lock is an idempotent no-op and the combined update stays deadlock-free.checkParentCycleis parameterized over the queryer (checkParentCycleQ) so the cycle walk reads inside the tx, and now runs after lock acquisition.UpdateItemWithParentLinkcall sites; the old post-commitSetParentLink/ClearParentLinkblock is removed.Works on both SQLite (atomicity from
BEGIN IMMEDIATE) and Postgres (advisory locks).Tests
TestUpdateItemWithParentLink_AtomicRollback— a failing (cycle) parent-link write rolls back the field change; asserts no partial state and no committed link.TestUpdateItemWithParentLink_AtomicCommit— happy path commits field + link together; also covers atomic clear.Gates
golangci-lint run— 0 issuesgo test ./...— greenmake test-pg(Postgres suite) — greenOut of scope / follow-up (BUG-2073)
Codex review surfaced two pre-existing concurrency TOCTOU races in the parent-link locking scheme (present on
mainverbatim, orthogonal to this partial-commit fix): (1) publicSetParentLinkdoesn't lock the child's own key, so a symmetric A↔B reparent can form a cycle; (2)oldParentis read before the lock and not re-read after (the same read-then-lock pattern the whole parent-children protocol uses). This PR already makes the new atomic path cycle-safe (it holds the child + parents + new-parent locks up front). The residual public-path races are tracked in BUG-2073 rather than expanding this PR.https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS