mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-25 03:42:06 +00:00
xarmian
55bfed543e
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