fix(wake): #927 enqueue TOCTOU — move stale-tmp cleanup off the hot enqueue path
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
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>
This commit is contained in:
@@ -110,12 +110,28 @@ _wake_lock_release() {
|
||||
{ exec 8>&-; } 2>/dev/null || true
|
||||
}
|
||||
|
||||
# _wake_clean_stale_tmp DIR — remove leftover atomic-write temp files (e.g. from
|
||||
# a crash mid-write). Safe: these are never the live store.
|
||||
# _wake_clean_stale_tmp DIR — reap ORPHANED atomic-write temp files left by a
|
||||
# crash mid-write (a crash before the rename leaves a .wake.tmp.* that no reader
|
||||
# ever promotes). These are never the live store.
|
||||
#
|
||||
# AGE-SCOPED (#927): only tmp files whose mtime is older than
|
||||
# ${WAKE_TMP_STALE_MIN:-5} minutes are removed. A LIVE in-flight atomic write's
|
||||
# tmp is at most milliseconds old (mktemp -> cat -> sync -> rename all complete
|
||||
# well under a second), so it can NEVER match this age filter. That is what makes
|
||||
# the cleanup safe even if it ever overlaps a concurrent enqueue's atomic write:
|
||||
# it deletes only DEMONSTRABLY-orphaned tmps, never another process's live write.
|
||||
#
|
||||
# This is why #927 is fixed: an UNCONDITIONAL delete of every .wake.tmp.* (the
|
||||
# old behaviour) clobbered a concurrent enqueue's in-flight tmp -> spurious
|
||||
# "durable pending write FAILED". It is ALSO no longer invoked from the per-
|
||||
# enqueue hot path (see _wake_init_dir); it runs only at maintenance / daemon-
|
||||
# start (store.sh init) and the detector poll tick, where accumulation is bounded
|
||||
# once per pass rather than raced on every enqueue.
|
||||
_wake_clean_stale_tmp() {
|
||||
local dir="$1"
|
||||
local dir="$1" min="${WAKE_TMP_STALE_MIN:-5}"
|
||||
[ -d "$dir" ] || return 0
|
||||
find "$dir" -maxdepth 1 -name "${_wake_tmp_prefix}*" -type f -delete 2>/dev/null || true
|
||||
case "$min" in '' | *[!0-9]*) min=5 ;; esac
|
||||
find "$dir" -maxdepth 1 -name "${_wake_tmp_prefix}*" -type f -mmin "+$min" -delete 2>/dev/null || true
|
||||
}
|
||||
|
||||
# _wake_read_int FILE DEFAULT — read a single integer from FILE, or DEFAULT.
|
||||
@@ -133,10 +149,17 @@ _wake_read_int() {
|
||||
}
|
||||
|
||||
# _wake_init_dir STATE_DIR — ensure the state layout exists; idempotent.
|
||||
#
|
||||
# #927: this runs on the HOT enqueue path (cmd_enqueue calls it BEFORE taking the
|
||||
# enqueue lock) as well as on consume/cursors/ack. It must therefore NEVER touch
|
||||
# another process's tmp files: the old _wake_clean_stale_tmp call here deleted a
|
||||
# concurrent enqueue's LIVE in-flight tmp mid-write -> spurious durable-write
|
||||
# abort. Stale-tmp reaping is now an explicit maintenance action (store.sh init /
|
||||
# detector poll tick), NOT a side effect of ensuring the layout. Keep this
|
||||
# function limited to creating the dir + seeding the cursor files.
|
||||
_wake_init_dir() {
|
||||
local dir="$1"
|
||||
mkdir -p "$dir" || return 1
|
||||
_wake_clean_stale_tmp "$dir"
|
||||
[ -f "$dir/observed_seq" ] || printf '0' | _atomic_write "$dir/observed_seq"
|
||||
[ -f "$dir/consumed_seq" ] || printf '0' | _atomic_write "$dir/consumed_seq"
|
||||
[ -f "$dir/observed.set" ] || printf '' | _atomic_write "$dir/observed.set"
|
||||
|
||||
Reference in New Issue
Block a user