fix(wake): #927 enqueue TOCTOU — move stale-tmp cleanup off the hot enqueue path (no concurrent in-flight-write clobber) #928
Assignees
jason.woltje
Clear assignees
jason.woltje
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: mosaicstack/stack#928
Reference in New Issue
Block a user
Delete Branch "fix/wake-927-enqueue-toctou"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Fixes a genuine product-level TOCTOU race in the wake store enqueue path (#927), reachable under live co-feed (detector + reconciler concurrently enqueue).
The race (RED, deterministic)
cmd_enqueuecalled_wake_init_dir()at store.sh:166 — which invoked_wake_clean_stale_tmp(), an unconditional delete of EVERY.wake.tmp.*— BEFORE_wake_lock_acquire()at store.sh:174. So a 2nd enqueue B, still in its pre-lock setup, deleted the live in-flight tmp of a 1st enqueue A that held the enqueue lock throughout its atomic write. A'smv tmp -> pending.jsonlthen failed with 'No such file or directory' → spuriousdurable pending write FAILEDabort of a perfectly valid enqueue. A's obligation was dropped on the floor.Root cause is broader than enqueue-vs-enqueue:
_wake_init_dir(hence the reap) is also called without the enqueue lock bycmd_consume,cmd_cursors, andack.sh— any of those could clobber a concurrent enqueue's in-flight tmp. That ruled out the 'move it inside the enqueue lock' alternative (it would leave cursors/consume/ack still racing) in favour of Mos's lean: take the reap off the hot path entirely.The fix (scalpel)
_wake_init_dir: no longer reaps tmps — it only ensures the layout + seeds cursor files. Nothing on the enqueue/consume/cursors/ack hot paths can delete another process's live write._wake_clean_stale_tmp: AGE-SCOPED (find -mmin +${WAKE_TMP_STALE_MIN:-5}). A live in-flight atomic write's tmp is ms-old, so it can never match — the reap only removes demonstrably-orphaned (crash-left) tmps, safe even if it ever overlaps a concurrent write.store.sh init(daemon-start) and the detector poll tick (detector.sh), bounding accumulation once-per-pass instead of racing per-enqueue.Invariants preserved
Tests / RED→GREEN
test-wake-store-enqueue-race.sh(separate file to avoid the #923/#926 T10 union-rebase). It PATH-shadowsmv(freezes enqueue A exactly at its rename with a live tmp, lock held) andflock(fires the instant enqueue B finishes its pre-lock cleanup), so the race window is held open by signal files, not scheduling luck. RED against unmodified store: A aborts withdurable pending write FAILED(its tmp deleted by B's pre-lock reap). GREEN post-fix: both succeed with distinct gapless seqs {1,2}, zero spurious aborts, observed_seq=2, depth=2, CONSUMED 2 valid.store.sh init) reaps a demonstrably-orphaned tmp. T5's core crash-safety invariant is intact.Gates
pnpm run test:framework-shellrc=0 normal AND openssl-masked (command -v opensslempty)Files changed:
_wake-common.sh,store.sh,detector.sh,test-wake-store-ack.sh(T5),test-wake-store-enqueue-race.sh(new),manifest.txt,package.json(suite wiring).Closes #927
Part of #892
cmd_enqueue called _wake_init_dir() (which reaped EVERY .wake.tmp.* unconditionally) BEFORE taking the enqueue lock. So a 2nd enqueue's PRE-LOCK cleanup deleted the LIVE in-flight tmp of a 1st enqueue that held the lock through its atomic write -> spurious "durable pending write FAILED" abort of a valid enqueue. Reachable under live co-feed (detector + reconciler concurrently enqueue). Fix (scalpel, Mos's lean = off the hot path): - _wake_init_dir no longer reaps tmps; it only ensures the layout. Nothing on the enqueue/consume/cursors/ack hot paths can clobber a concurrent live write. - _wake_clean_stale_tmp is AGE-SCOPED (-mmin +${WAKE_TMP_STALE_MIN:-5}) so it can only remove demonstrably-orphaned crash-left tmps, never a live (ms-old) in-flight write — safe even if it ever overlaps a concurrent enqueue. - Reaping runs as an explicit MAINTENANCE action at store.sh init (daemon-start) and the detector poll tick, bounding accumulation once-per-pass, not raced per-enqueue. #908 seq-integrity is unchanged (single store-side allocator, atomic allocate+enqueue under flock, arrow-1 no-burn, anti-swallow fail-loud). reconcile.sh is unchanged: an aborted enqueue advances neither the seen-ledger nor observed_seq, so the source is re-enumerated next reconcile cycle (no obligation loss). Tests: new deterministic RED->GREEN harness test-wake-store-enqueue-race.sh (PATH-shadowed mv/flock freeze enqueue A at its rename while B runs its pre-lock cleanup — RED pre-fix: A aborts; GREEN post-fix: both succeed, distinct gapless seqs {1,2}, zero spurious aborts). T5 updated to assert the hot path no longer reaps tmps and that the maintenance path reaps a demonstrably-orphaned tmp. All store-ack groups incl T10 stay green. manifest 0.6.4 -> 0.6.5. Closes #927 Part of #892 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>Record of Review — PR #928 (issue #927): enqueue TOCTOU fix (off-hot-path + age-scoped reap) — FINAL, CI-GREEN
VERDICT: APPROVE — GO. REVIEWED-HEAD (full-40):
a8af5ed7f09631352a2d6bac084c0fc45781ecdd. CI: TERMINAL-GREEN (pipeline 2075; validates the combined #926-T10-deflake + #928-T5 store-ack file).Review basis
Independent reviewer
a3774b40(opus, ≠ buildermosaic-coder) — APPROVE, all 7 criteria PASS, criteria 3/4 independently REPRODUCED:a8af5ed7…; mergeable; id2 ≠ commit-authormosaic-coder; bodyCloses #927+Part of #892; net delta = #928s 7 files; T10 UNCHANGED vs main (#926s de-flake preserved; only T5 in the store-ack test); reconcile.sh BYTE-UNCHANGED; manifest 0.6.4→0.6.5._wake_init_dirhelper, so it closes the race for EVERY pre-lock caller at once (enqueue/consume/cursors/ack ×3), not just enqueue-vs-enqueue (the builders all-callers analysis showed "inside-the-lock" would be incomplete). Reap now runs ONLY as maintenance at storecmd_init+ detectorcmd_poll_once(bounded)._wake_clean_stale_tmpusesfind -mmin +${WAKE_TMP_STALE_MIN:-5}(non-numeric guard). Reviewer: a fresh ms-old.wake.tmpSURVIVES, a 10-min-aged orphan is REAPED. A sub-second atomic write can never match +5min → cannot clobber a live write.test-wake-store-enqueue-race.sh(mv+flock PATH-shadow holds the window open): fix → GREEN (distinct gapless {1,2}, zero abort); mains PRE-FIX code under the same harness → RED ("durable pending write FAILED for seq 1", A aborts). Harness catches the bug; fix closes it.6-check — GO
a8af5ed7f09631352a2d6bac084c0fc45781ecdd✓ · 3. reviewer(a3774b40)≠builder + MS-LEAD git-verified update, durable RoR ✓ · 4. reviewed==CI==merge-head alla8af5ed7✓ · 5. bodyCloses #927+Part of #892✓ · 6. queue-guard at merge.GO for id-11 + squash-merge at FULL-40
a8af5ed7f09631352a2d6bac084c0fc45781ecdd. Closes #927 (the ELEVATED live-co-feed enqueue TOCTOU, fixed at root off the hot path). Non-executor.GO — Gate-16 id-11 stamp, pinned to exact FULL head
a8af5ed7f0.Basis: verbatim FINAL RoR (comment 19191) — independent review APPROVE 7/7 with the load-bearing property reproduced: the off-hot-path fix is COMPLETE BY CONSTRUCTION (removing the reap from the shared _wake_init_dir helper closes the race for every pre-lock caller simultaneously — enqueue, consume, cursors, ack — the builder's caller-sweep having proven inside-the-lock would be incomplete); age-scoped maintenance verified safe both ways (fresh tmp survives, aged orphan reaped); the deterministic PATH-shadow race harness RED pre-fix, GREEN post-fix; #908 seq-integrity and the reconciler recovery model byte-untouched. Union-validated against post-#926 main in CI. Closes #927 — the elevated live-co-feed race fixed at root while its designed safety net (loud abort, structural retry, fallback bounds) covered the interim, exactly as calibrated. Part of #892. Named executor: Mos (id-11), squash pinned.