diff --git a/changelog.d/1119.fixed.md b/changelog.d/1119.fixed.md index e3e9ad60..75201e53 100644 --- a/changelog.d/1119.fixed.md +++ b/changelog.d/1119.fixed.md @@ -1,18 +1 @@ -- Fixed: `_lock_read_pid_timeout` (`scripts/lib-lock.sh`) treated a FAST, - ordinary read failure (permission denied, I/O error, a cloud-sync - placeholder that errors rather than hangs) exactly like a successful read - of a genuinely empty pid file, a regression from the #1101/#1103 lock- - staleness fix (v0.45.1). Because a confirmed-empty pid file is reclaimed - after only `REMEMBER_LOCK_ADOPT_AFTER` (30s) rather than - `REMEMBER_LOCK_MAX_HOLD_SECONDS` (1800s), this let a second process steal - `save.lock`/`consolidation.lock`/`staging.lock` away from a holder that - was still alive, 60x sooner than any other takeover path allows (#1119). - A fast read failure is now treated the same as a read that never - completed: refused on its own, but still recoverable through the - existing age-gated takeover once the lock directory is older than - `REMEMBER_LOCK_MAX_HOLD_SECONDS`. -- Corrects the v0.45.1 changelog entry for #1101/#1103, which claimed a - confirmed-empty pid file is "reclaimed too, exactly as it already was for - the no-pid-file-at-all case" -- not true for a pid file that reads empty - because the read failed fast rather than because it genuinely is empty; - v0.45.0 never stole a lock in that case either (#1119). +- Fixed: in v0.45.1, if a save could not read its lock file (a permissions error, or a cloud-sync placeholder that answers with an error), another save could take over the lock while the first one was still running, so two saves could run at the same time. Putting the save lock back the way it worked in v0.45.0 removes this (see the removal note in this release). (#1119) diff --git a/changelog.d/1122.fixed.md b/changelog.d/1122.fixed.md index f3962df5..1d603567 100644 --- a/changelog.d/1122.fixed.md +++ b/changelog.d/1122.fixed.md @@ -1,19 +1 @@ -- Fixed: `_lock_try_steal` (`scripts/lib-lock.sh`) re-reads the pid file it - just renamed aside to tell apart "still the same stuck placeholder" (safe - to steal) from "a faster contender's own write already recreated the pid - file in the gap between this contender's own age check and its rename" - (unsafe -- must hand back). #1119/#1120 (v0.45.1) made every read failure - at this re-read report identically, whether a real timeout or a fast - EACCES/EIO/cloud-sync refusal, so a fast-failing re-read of a genuine - winner's brand-new pid was read exactly like a confirmed-still-stuck - placeholder and fell through to the noclobber write below it, clobbering - the winner's `save.lock`/`consolidation.lock`/`staging.lock` and - producing two simultaneous holders -- the same class #1119/#1120 shipped - as a non-blocking trap.d fragment one release earlier and then had to - patch immediately as a regression. The re-read now also compares the - renamed file's inode against what was captured just before the rename: - a match is the same file whose first read already failed too, safe to - steal; a mismatch, or either lookup coming back empty, hands back rather - than guess -- `stat` needs no read permission on the file's content, - unlike the re-read itself, so this works even when neither pid file is - readable (#1122). +- Fixed: a follow-up attempt to repair the v0.45.1 save-lock problem, never released, still let two saves run at once in a rarer timing case. It is dropped together with the rest of that change by the rollback in this release, so there is nothing to update for it. (#1122) diff --git a/changelog.d/1131.removed.md b/changelog.d/1131.removed.md new file mode 100644 index 00000000..4dc16914 --- /dev/null +++ b/changelog.d/1131.removed.md @@ -0,0 +1,7 @@ +- Removed: the save-lock recovery added in the previous release (v0.45.1). It was meant to unstick a save lock left behind by a hung save, or by a lock file that a cloud-sync folder (OneDrive, iCloud, Dropbox) had turned into an online-only placeholder. Reviews found it could let two saves run at the same time, which is worse than the problem it fixed, so the save lock now works exactly as it did in v0.45.0. The Ubuntu 26.04 fix that stops two saves running at once there (#1108) stays. + + What this brings back, until the recovery is redesigned as a separate change: a lock left by a save that hung can stay stuck (#1101, reopened), and if your `.remember` folder is in a cloud-sync folder, reading an online-only lock file can make a save wait until it times out (#1103, reopened). + + The two settings that came with the recovery, `REMEMBER_LOCK_MAX_HOLD_SECONDS` and `REMEMBER_LOCK_PID_READ_TIMEOUT`, are gone. If you set either one, it is now ignored; you can delete it. (#1131) + +- Compatibility: compatible - nothing that worked in v0.45.0 stops working; the two removed settings are ignored rather than rejected, and every lock behaviour is the v0.45.0 one. diff --git a/scripts/lib-lock.sh b/scripts/lib-lock.sh index 9fa56d11..17fdaf3b 100644 --- a/scripts/lib-lock.sh +++ b/scripts/lib-lock.sh @@ -53,7 +53,7 @@ # # Functions: # lock_acquire [timeout_seconds] -> 0 acquired, 1 timed out -# lock_release -> 0 released, 1 not ours (or ownership could not be confirmed in time) +# lock_release -> 0 released, 1 not ours # # CROSS-HOST CONTRACT (Claude Code and Codex sharing one .remember/ store on # one machine, #491): this primitive is safe there, deliberately, because it @@ -76,35 +76,6 @@ # more probable, because `kill -0` cannot see which CLI started a PID in # either case. See the same test file for a reproduction that documents # this as an accepted, general limitation rather than a fix. -# -# Two bounds close separate gaps in that same `kill -0` check, both landing -# in `_lock_try_steal` (#1101, #1103): -# -# - A holder can be ALIVE and still never make progress -- blocked on I/O, -# a wedged pipe, anything short of exiting. `kill -0` reports that -# holder as healthy forever. `_LOCK_MAX_HOLD_SECONDS` bounds how long -# any live holder may keep the lock before a later acquirer may take it -# over, measured from the lock directory's own mtime rather than a -# separate timestamp file, so acquiring a lock pays no extra cost for -# this to work (#1101). -# - The pid file itself can be UNREADABLE rather than merely absent: a -# dataless cloud-sync placeholder (OneDrive, iCloud -- the remote copy -# was deleted after the placeholder synced) blocks a plain `cat` on the -# read syscall rather than returning content or an error. -# `_lock_read_pid_timeout` bounds that read so it cannot consume a -# whole hook's own timeout budget. A read that does not complete in time -# is treated as "still held, try again later" -- a SINGLE timeout is -# never grounds to reclaim: it cannot tell a permanently stuck -# placeholder from a live holder's pid file that is merely slow to -# read once, and guessing wrong there would let an unrelated process -# steal or delete a lock a healthy holder still owns (#1103, found in -# this fix's own self-review). The fast timeout alone is still -# strictly better than the unbounded `cat` it replaces. A pid file -# that NEVER finishes reading is a different case, handled the same -# way a live-but-wedged holder already is: once the lock directory -# has sat untouched past `_LOCK_MAX_HOLD_SECONDS`, a later acquirer -# may take it over (`_lock_try_steal`'s own timed-out-read branch), -# a scheduler-measured follow-up to the original #1103 fix. [ -n "${_REMEMBER_LIB_LOCK_SOURCED:-}" ] && return 0 _REMEMBER_LIB_LOCK_SOURCED=1 @@ -149,307 +120,10 @@ _lock_self_set() { return 0 } -# How long a live holder may keep a lock before a later acquirer may take it -# over anyway (#1101). `kill -0` only tells "alive", never "making -# progress"; a holder blocked on I/O is alive by that check for as long as -# the block lasts, which on #1101 was over two days. Generous on purpose: a -# save legitimately holds this lock across a summarize call (see the timing -# recorder's own notes above), and a bound that fires on a healthy long save -# is worse than one that never fires on a truly stuck one. -_LOCK_MAX_HOLD_SECONDS="${REMEMBER_LOCK_MAX_HOLD_SECONDS:-1800}" - -# How long a read of the lock's pid file may block before giving up on it -# (#1103). A plain `cat` on a dataless cloud-sync placeholder (the remote -# copy of a file that WAS synced has since been deleted -- OneDrive, -# iCloud) blocks the read syscall itself rather than returning content or -# an error, observed at roughly 60s while the File Provider gives up. Left -# unbounded, that single read consumes a whole PostToolUse hook's own -# timeout budget. -_LOCK_PID_READ_TIMEOUT="${REMEMBER_LOCK_PID_READ_TIMEOUT:-2}" -# A non-integer override (`0.5`, `2s`, empty-but-set) reaches the -# watchdog's own iteration-count arithmetic below as a bash error, which -# exits that subshell on the spot -- before it ever sends the SIGTERM -# that is this whole bound's only way of waking a blocked `cat` -- -# leaving the read, and the caller's blocking `wait` on it, unbounded -# again: exactly the #1103 hang this file exists to close, just -# reopened by a bad env var. Reject anything that is not digits-only -# and fall back to the documented default instead of letting a typo -# silently undo the bound. (Deliberately not spelling out the literal -# watchdog arithmetic here, with its own guarded variable name, between -# the dollar-double-open-paren and matching double-close-paren bash -# arithmetic delimiters: tests/test_arith_base_lint_332.py's own sweep -# is plain text, not a shell parser, and does not skip comment lines -- -# it flagged exactly that literal span the first time this comment -# spelled the delimiters out.) -# -# `0` (or `00`) is digits-only but still rejected, separately: it sets -# the watchdog's own bound to zero iterations, which kills a read that -# is not yet done after the FIRST poll-sleep interval (~50ms) -- -# indistinguishable, under any real load, from killing a holder that -# was about to finish normally. Worse, the SAME bound of zero applies -# to the takeover path's post-rename re-read (above): a legitimate -# fresh pid write can lose that race to its own zero-length grace -# period, reporting a false timeout that lets a second contender treat -# the just-written winner as still-stuck and take the lock away from -# it -- two processes then believe they hold the same lock. `-lt 1` -# (not `-le 0`) is the natural phrasing of "require at least one real -# iteration"; `[ ]`'s own numeric comparison does not apply octal -# rules (confirmed: a leading-zero value compares correctly here, only -# the ARITHMETIC context below needs the `10#` forcing applied there). -if [[ "$_LOCK_PID_READ_TIMEOUT" == *[!0-9]* ]] || - [ -z "$_LOCK_PID_READ_TIMEOUT" ] || - [ "$_LOCK_PID_READ_TIMEOUT" -lt 1 ]; then - _LOCK_PID_READ_TIMEOUT=2 -fi - -# Read into $_LOCK_PID_READ_RESULT, bounded by -# _LOCK_PID_READ_TIMEOUT seconds. Returns 0 once the read completes -- -# including an empty result, which is a legitimate "not written yet" state -# and is reported as empty, not as an error. Returns 1 if the read did not -# complete within the budget. -# -# Not `timeout cat ...`: `timeout` is a GNU coreutils binary, absent from -# macOS's base install -- the platform #1103 was filed from, and the same -# reason this file does not depend on `flock` (see the top of the file). -# Backgrounding the read and polling with `kill -0` needs nothing beyond -# what this file already uses elsewhere. -# -# A SIGTERM to a process blocked in a slow read() interrupts the syscall and -# ends the process (no handler is installed for it), which is what actually -# unblocks the background job here -- not a cooperative check the blocked -# `cat` could ever perform itself. This covers the dataless-placeholder -# case #1103 was filed for (a File Provider read that blocks for ~60s, -# then gives up) and a reader that ignores SIGTERM but can still be -# reached by SIGKILL. It does NOT cover a read truly stuck in an -# UNINTERRUPTIBLE kernel sleep (D state -- a wedged NFS mount, a broken -# FUSE driver): no signal, including SIGKILL, is delivered to a process -# until that syscall itself returns, which is a kernel-level condition no -# userspace watchdog can force. `wait "$_cat_pid"` below has no bound of -# its own for exactly that reason, and would block for as long as the -# read does -- rare enough on a local pid file that this file does not -# attempt to work around it (reasoned, not reproduced: inducing a real D -# state needs a wedged filesystem, which this test suite cannot -# construct), same as every caller already accepted before #1101/#1103. -_lock_read_pid_timeout() { - local _file="$1" _tmp _cat_pid _watchdog_pid _rc _max_iters _iters=0 - _LOCK_PID_READ_RESULT="" - _tmp=$(mktemp "${TMPDIR:-/tmp}/remember-pidread-XXXXXX" 2>/dev/null) || { - # No scratch file to redirect into, so there is nowhere to background - # the read INTO -- this function's own timeout mechanism cannot help - # here regardless of what this branch does. A second-review finding - # corrected an earlier instinct to `return 1` unconditionally on this - # branch: `mktemp` fails for ordinary reasons that have nothing to do - # with #1103's cloud-sync placeholders -- a full or unwritable - # TMPDIR, no free inodes -- and refusing unconditionally here made - # EVERY caller of _lock_try_steal and _lock_release_impl refuse - # unconditionally too, including a release of a lock the process - # legitimately owns. That is a total, systemic lock outage, strictly - # worse than the narrow, rare hang this whole fix exists to bound. - # Falling back to a direct, unbounded `cat` degrades this one corner - # to exactly the behaviour every caller already had before #1103 -- - # not worse, and bounded everywhere `mktemp` still works. - # - # #1119: that degraded behaviour must still distinguish a read that - # completed (even reading nothing, from a genuinely empty file) from - # one that failed outright (EACCES, EIO, ENOENT) -- the exact - # distinction the bounded path below exists to make, and the one - # this branch's own `|| true` used to erase by forcing `return 0` - # unconditionally regardless of `cat`'s own exit status. Losing it - # here reopens the identical gap for the one corner where `mktemp` - # itself fails: a fast-failing, unreadable pid file would again read - # as confirmed-empty and be reclaimable after only - # `_LOCK_ADOPT_AFTER` (30s) instead of `_LOCK_MAX_HOLD_SECONDS` - # (1800s), from a live holder. `cat` on a genuinely empty, readable - # file still exits 0, so this changes nothing for the case this - # fallback exists to preserve -- only a `cat` that actually failed - # now reports failure, same as the bounded path. - if _LOCK_PID_READ_RESULT=$(cat "$_file" 2>/dev/null); then - return 0 - fi - _LOCK_PID_READ_RESULT="" - return 1 - } - ( cat "$_file" > "$_tmp" 2>/dev/null ) /dev/null || break - if [ "$_iters" -ge "$_max_iters" ]; then - kill "$_cat_pid" 2>/dev/null || true - # Same SIGTERM-then-SIGKILL escalation the original fix - # had: a reader that ignores SIGTERM (reproduced directly - # with a shim that traps and ignores TERM) is reached by - # SIGKILL instead. A reader truly stuck in an - # UNINTERRUPTIBLE kernel read is a different case this - # escalation cannot reach either way -- no signal lands - # on it until the syscall itself returns -- but this - # watchdog's OWN poll loop still returns on schedule - # regardless, since `kill` never blocks on its target; - # only the main thread's later `wait "$_cat_pid"` would - # still be stuck on such a reader, see the note above - # this function. - sleep "$_LOCK_SLEEP" - kill -0 "$_cat_pid" 2>/dev/null && kill -9 "$_cat_pid" 2>/dev/null - break - fi - done - # `/dev/null 2>&1` here is still load-bearing: even - # bounded to one short poll interval past the main thread's own - # `wait`, the watchdog is still a process this function's caller did - # not ask for, and if whatever called this function is itself - # captured through a pipe (a hook runner, a test harness), an - # unredirected child inheriting that pipe's write end would delay - # the caller seeing EOF until this one also exits -- not the ~2s a - # single long `sleep` could cost, but still a real, avoidable delay - # for zero benefit, and the ~100ms/pair regression a missing - # redirection on the ORIGINAL (single-long-sleep) design measured - # directly is the same class of cost this closes off entirely. - ) /dev/null 2>&1 & - _watchdog_pid=$! - # `disown` here, not left to the shell's own job table: a signal-killed - # background job that is STILL in the job table gets an unconditional - # "Terminated: 15" line on this shell's own stderr the next time it - # notices (observed on EVERY call, both the common fast path -- where - # the kill below fires on a watchdog that is still asleep -- and the - # #1103 timeout/escalation path, where the watchdog is what gets - # raced by the very kill meant to stop it promptly). This is bash's - # own async job-control notification, not something `2>/dev/null` on - # the kill or the surrounding subshell's own redirections can reach, - # because it is printed by THIS shell's bookkeeping, not by the job - # being signalled. `disown` removes the watchdog from that bookkeeping - # the moment its pid is known -- before anything could ever kill it -- - # which silences the notification unconditionally, on every bash this - # function runs under (including bash 3.2), with no change to timing: - # this function never `wait`s on the watchdog, so losing the ability - # to read ITS exit status (what `disown` costs here) costs nothing. - disown "$_watchdog_pid" 2>/dev/null || true - wait "$_cat_pid" 2>/dev/null - _rc=$? - # The watchdog is done being useful the instant `wait` above returns, - # whether because the reader finished naturally or because the - # watchdog itself just killed it -- killing it here either stops it - # before it ever sleeps (the common, fast case) or is a harmless no-op - # on a process already gone. Deliberately not waited on afterward, the - # same narrow, accepted zombie-entry tradeoff the reader's own kill - # already makes elsewhere in this function: it must still return on - # schedule rather than pay to reap a process that costs nothing - # further to leave unreaped. - kill "$_watchdog_pid" 2>/dev/null || true - # A process killed by a signal reports 128+signal from `wait` -- the - # one portable way to tell "the watchdog had to intervene" from "the - # reader exited on its own, however it felt about its own exit code" - # without any state shared between the two subshells. That is the - # HUNG case. - # - # #1119, regression in #1101/#1103: a FAST, ordinary non-hang failure - # (EACCES, EIO, a cloud-sync placeholder that errors rather than - # hangs) exits below 128 and used to fall straight through to the - # success branch below with $_tmp left empty by the failed `cat` -- - # indistinguishable there from a `cat` that genuinely read an empty - # file. `_lock_try_steal` cannot tell "the read completed and the - # file truly is empty" from "the read never completed at all" once - # they both arrive here looking identical, and treated the fast-fail - # case as the confirmed-empty one -- which is gated by - # `_LOCK_ADOPT_AFTER` (30s) rather than `_LOCK_MAX_HOLD_SECONDS` - # (1800s), so a live holder's lock could be stolen out from under it - # 60x sooner than any other takeover path allows, from a failure mode - # that has nothing to do with how long the lock has been held. - # - # Any non-zero exit, not only >=128, is therefore treated as "the - # read did not complete" -- a single, deliberate rule: this function - # tells callers whether a read FINISHED (0) or not (1), and reserves - # the content it finished with for a true success. The fast-fail case - # then flows through the exact same code path the HUNG case already - # has at every caller: refuse the empty-pid steal, but remain - # recoverable through the existing age-gated takeover of a live-or- - # unreadable holder once the lock directory itself is older than - # `_LOCK_MAX_HOLD_SECONDS` -- the same bound a holder that is merely - # alive-but-wedged is already held to, deliberately, since a pid file - # that fails fast every time is exactly as uninformative about - # whether its holder is alive as one that times out every time. - if [ "$_rc" -ne 0 ]; then - rm -f "$_tmp" 2>/dev/null || true - return 1 - fi - # $(/dev/null || true - rm -f "$_tmp" 2>/dev/null || true - return 0 -} - # Take a stale lock away from a dead holder. Returns 0 only for the single # process that won the rename — everyone else gets 1 and keeps waiting. _lock_try_steal() { - local _dir="$1" _pid _seen _abandoned _owner _claim _pre_steal_inode _claim_inode + local _dir="$1" _pid _seen _abandoned _owner _claim # Per-process, NOT a shared name: with one `pid.stealing` for everyone, a # second process could "restore" a claim that was still in flight and then # claim it itself, and both would proceed. Two winners at N=8, reproducibly. @@ -481,152 +155,12 @@ _lock_try_steal() { done fi - if _lock_read_pid_timeout "${_dir}/pid"; then - _pid="$_LOCK_PID_READ_RESULT" - else - # Did not complete within _LOCK_PID_READ_TIMEOUT -- a dataless - # cloud-sync placeholder blocks the read rather than returning - # content (#1103), but a read that is merely slow ONCE looks - # identical from here, and a live, healthy holder's pid file can be - # slow to read for reasons that have nothing to do with how long the - # lock itself has been held. Guessing "debris" from a single timeout - # risks stealing a lock out from under a holder that is making - # normal progress -- found in self-review (#1101/#1103): treating - # this the same as a genuinely-empty-and-old pid let two processes - # both believe they held the same lock whenever BOTH this read and - # the post-rename re-check below happened to time out the same way. - # Refuse and let the caller retry rather than steal on a guess; the - # fast timeout alone is still strictly better than the unbounded - # `cat` this replaces, which could block an entire hook's budget. - # - # Refusing FOREVER, however, is its own outage: a pid file that - # NEVER finishes reading (the actual #1103 shape -- a dataless - # placeholder whose remote copy is gone for good, not merely a - # holder that was slow once) would make every later acquirer time - # out at this exact point, forever, with "another save holds the - # lock, skipping" logged after 2s instead of ~60s but just as - # permanently (scheduler-measured follow-up: #1103 was faster, - # not fixed). The SAME age bound that already lets a later - # acquirer take over a live-but-wedged holder (`kill -0` succeeds - # forever, below) applies here too: once the lock directory - # itself has sat untouched for at least _LOCK_MAX_HOLD_SECONDS - # AND the pid file still refuses to finish reading, treat it the - # same as a wedged live holder and take it over. - # - # Deliberately NEVER re-reads the ORIGINAL "${_dir}/pid" to get - # here -- it already timed out once above and would time out - # again, paying the same bound for nothing. The atomic `mv` - # ALONE is not the whole single-winner guarantee here, though: - # unlike the two branches below (which read real content and - # re-check THAT survived the rename), this branch has no content - # to compare against, and self-review on this exact fix found a - # real race from skipping the check anyway -- a winner's own - # write recreates "${_dir}/pid" an instant after the rename, and - # a slower contender's `mv` can then grab that brand-new, healthy - # pid file with nothing yet telling it apart from the stuck - # placeholder it meant to steal (reproduced directly: 7 of 8 - # concurrent contenders believed they won). The re-read below, - # of the CLAIM this rename just produced -- never the original - # path again -- is what actually closes that gap: see the - # comment right after the `mv`. Removing it reopens the race. - [ "$(_lock_dir_age "$_dir")" -ge "$_LOCK_MAX_HOLD_SECONDS" ] || return 1 - # Captured BEFORE the rename, not derived from it: `stat` reads - # metadata only, so this still answers even though the content read - # above just failed. This is the "before" half of the #1122 check - # below -- what we are ABOUT to grab, independent of whether we can - # read what is inside it. - _pre_steal_inode=$(_lock_file_inode "${_dir}/pid") - mv "${_dir}/pid" "$_claim" 2>/dev/null || return 1 - # Confirmed by direct repro (N=8 concurrent contenders) that - # skipping this check lets 7 of 8 believe they won: this winner's - # own noclobber write below RECREATES "${_dir}/pid", and a slower - # contender's `mv` (racing on the SAME path, not the content) can - # then grab that brand-new, perfectly healthy pid file an instant - # after it was written, with nothing yet telling it apart from - # the stuck placeholder it meant to grab. Re-reading from the - # CLAIM this rename just produced -- never the original path - # again -- tells the two apart cheaply: a genuine stuck FIFO - # (rename preserves the underlying special file, not the - # content) still times out identically; a fresh pid some faster - # winner already wrote resolves immediately, which is this - # contender's signal that it grabbed someone else's win by - # mistake and must hand it back rather than overwrite it. - if _lock_read_pid_timeout "$_claim"; then - mv "$_claim" "${_dir}/pid" 2>/dev/null || true - return 1 - fi - # #1122: the re-read above failing is NOT by itself proof that - # $_claim is still the genuinely stuck placeholder it looked like - # before the rename -- #1119/#1120 made EVERY read failure here - # (a real timeout, or a fast EACCES/EIO/cloud-sync refusal) report - # identically as "1", so a fast-failing re-read of a faster - # winner's brand-new pid -- grabbed by exactly the same-path race - # the comment above describes, just landing on content this - # contender cannot read for an unrelated, transient reason -- is - # read exactly like a confirmed-still-stuck placeholder and falls - # through to steal below. That reopens the identical "confirmed - # empty" mistake #1119 already closed at the FIRST read, one call - # site over, and lets this contender clobber a winner's lock, - # producing two simultaneous holders. - # - # Compare inode, not content: rename preserves the underlying - # inode, so $_claim's inode equals whatever this contender is - # ACTUALLY holding right now, and `_lock_file_inode` needs no read - # permission on that file's content to answer, unlike - # `_lock_read_pid_timeout`. A match means nothing was swapped in - # between the capture above and this rename -- genuinely the same - # file whose first read already failed too, consistent with a - # placeholder that is still stuck, and safe to steal. A mismatch, - # or either inode lookup itself coming back empty, is exactly the - # ambiguity #1119/#1120 already decided must refuse rather than - # guess: hand back. - _claim_inode=$(_lock_file_inode "$_claim") - if [ -z "$_pre_steal_inode" ] || [ -z "$_claim_inode" ] || [ "$_pre_steal_inode" != "$_claim_inode" ]; then - mv "$_claim" "${_dir}/pid" 2>/dev/null || true - return 1 - fi - if ( set -o noclobber; echo "$_LOCK_SELF" > "${_dir}/pid" ) 2>/dev/null; then - rm -f "$_claim" 2>/dev/null || true - return 0 - fi - rm -f "$_claim" 2>/dev/null || true - return 1 - fi + _pid=$(cat "${_dir}/pid" 2>/dev/null) || true - # Confirmed empty or non-numeric -- the read genuinely completed, so this - # is a fact rather than a guess, unlike the timeout case above. Two - # different things can cause it: the holder created the directory - # microseconds ago and has not written its pid yet (a live acquisition - # in flight), or the holder died between `mkdir` and writing its pid, - # leaving a pid file that exists and is empty forever (a second-review - # finding: `_lock_try_adopt` below only ever reclaims a MISSING pid - # file -- its very first line is `[ -e "${_dir}/pid" ] && return 1` -- - # so a pid file that exists-but-is-empty has no other reclaim path at - # all). Age is what tells the two apart, same as the no-pid-file case - # `_lock_try_adopt` already covers; because the content was genuinely - # read, age-gating it here carries none of the ambiguity a timeout - # would. - if [ -z "$_pid" ] || [[ "$_pid" == *[!0-9]* ]]; then - [ "$(_lock_dir_age "$_dir")" -ge "$_LOCK_ADOPT_AFTER" ] || return 1 - # $_pid is left exactly as read -- NOT cleared to "" here. The - # post-rename re-read below (`_seen != $_pid`) exists to confirm - # nothing changed underneath the rename, by comparing against - # whatever this branch actually saw. Clearing a non-numeric $_pid to - # "" broke exactly that: the re-read then saw the real content again - # (e.g. "abc"), compared it against the empty string it could never - # equal, and the steal was restored and refused every time -- a - # non-numeric pid file could never be reclaimed, however old - # (confirmed by reproduction, not merely reasoned). - # Alive is not the same as healthy: a holder that hangs forever -- - # blocked on I/O, a wedged pipe -- is live by `kill -0` indefinitely - # (#1101). Bound how long ANY live holder may keep the lock before a - # later acquirer may take it over. The directory's mtime is set when it - # is created and again, microseconds later, when the pid is written, so - # its age already tracks the lock's own hold duration -- no separate - # timestamp file, no extra cost on the common acquire path. - elif kill -0 "$_pid" 2>/dev/null; then - [ "$(_lock_dir_age "$_dir")" -ge "$_LOCK_MAX_HOLD_SECONDS" ] || return 1 - fi + # No pid file yet: the holder created the directory microseconds ago and has + # not written it. That is a live lock mid-acquisition, not a stale one. + [ -n "$_pid" ] && [[ "$_pid" != *[!0-9]* ]] || return 1 + kill -0 "$_pid" 2>/dev/null && return 1 # Claim the right to take over by RENAMING the pid file. Rename is atomic # and the source disappears, so of the N processes that judged this lock @@ -641,15 +175,7 @@ _lock_try_steal() { # closes that window because the path is never empty. mv "${_dir}/pid" "$_claim" 2>/dev/null || return 1 - # A timed-out re-read here must restore and refuse, exactly like a - # mismatch below -- never fall through as though it had matched. Letting - # "could not confirm" and "confirmed identical" collapse into the same - # branch is precisely the self-review finding above, one line later. - if ! _lock_read_pid_timeout "$_claim"; then - mv "$_claim" "${_dir}/pid" 2>/dev/null || true - return 1 - fi - _seen="$_LOCK_PID_READ_RESULT" + _seen=$(cat "$_claim" 2>/dev/null) || true if [ "$_seen" != "$_pid" ]; then mv "$_claim" "${_dir}/pid" 2>/dev/null || true return 1 @@ -699,36 +225,6 @@ _lock_dir_age() { echo $(( _now - 10#$_mtime )) } -# Inode of , or empty if it cannot be determined. `stat` needs no -# permission on the file's own CONTENT -- only search permission on its -# containing directory -- so this still answers for a file this process -# cannot `cat` (#1122): the whole point of using it in _lock_try_steal to -# tell "still the same file" from "silently swapped for a different one" -# without caring whether either is readable. -# -# Same BSD-vs-GNU `stat` split as _lock_dir_age above, and the exact same -# #198 gotcha applies to `%i`: GNU's `-f` means FILESYSTEM status, not file -# status, and its own `%i` under that mode is "free inodes on the -# filesystem" -- a large, plausible-looking, completely unrelated digit -# string that the naive digits-only guard would otherwise accept as a real -# inode number. Captured separately, exactly like the mtime case, so `-f` -# is only ever tried once `-c` has already failed outright (true on BSD, -# where `-c` is not a valid flag at all) rather than once `-c` has produced -# a wrong-but-numeric answer. -# -# No timeout wrapper around this `stat`, unlike `_lock_read_pid_timeout`: -# reasoned, not reproduced -- a metadata-only syscall does not block on the -# same "data never arrives" shape an actual read of a FIFO or a dataless -# placeholder can (#1101/#1103), so this file does not pay for a bound it -# has no observed need for. -_lock_file_inode() { - local _inode - _inode=$(stat -c %i "$1" 2>/dev/null) || _inode="" - [ -n "$_inode" ] && [[ "$_inode" != *[!0-9]* ]] || _inode=$(stat -f %i "$1" 2>/dev/null) || _inode="" - [ -n "$_inode" ] && [[ "$_inode" != *[!0-9]* ]] || { _inode=""; } - echo "$_inode" -} - # Adopt a lock directory that has no pid at all. A holder killed between # `mkdir` succeeding and writing its pid leaves exactly this: no pid to judge # stale, no claim file to explain it. Without adoption that lock can never be @@ -1262,93 +758,13 @@ lock_release() { } _lock_release_impl() { - local _dir="$1" _pid _claim _seen - if _lock_read_pid_timeout "${_dir}/pid"; then - _pid="$_LOCK_PID_READ_RESULT" - else - # Same dataless-placeholder hazard as the read in _lock_try_steal - # (#1103) -- bounded rather than left to block the release path too. - # UNLIKE _lock_try_steal, this function's caller believes IT holds - # the lock: an inconclusive read must never fall through to removing - # a directory that might belong to someone else. A first cut here - # treated a timeout the same as a genuinely empty pid (which skips - # the ownership check below and removes unconditionally) -- found in - # self-review to let a process whose own read was merely slow once - # delete a live, unrelated holder's lock. Refuse rather than guess. - return 1 - fi + local _dir="$1" _pid + _pid=$(cat "${_dir}/pid" 2>/dev/null) || true _lock_self_set if [ -n "$_pid" ] && [ "$_pid" != "$_LOCK_SELF" ]; then return 1 fi - - # The read above and the `rm -rf` below are two separate operations, and - # a steal can land in between: a holder judged stale (> _LOCK_MAX_HOLD_ - # SECONDS, #1101) while it is ALIVE and about to release legitimately - # races a stealer that renames the pid file, re-reads it, and writes its - # own pid -- all while this release is still only holding the pid VALUE - # it read earlier, not the file itself. Nothing stopped the `rm -rf` - # below from firing after that takeover completed, deleting the new - # owner's lock out from under it; a third process then recreates the - # directory and two holders exist. Closed the same way `_lock_try_steal` - # already closes its own single-winner race: rename the pid file OUT - # from under the directory first. Whichever of this release's rename or - # a concurrent steal's rename happens first wins; the other gets ENOENT - # (or a content mismatch on re-read) and backs off instead of - # proceeding on stale information. - # A known, accepted gap (review finding, left undone like the PID- - # recycling limitation documented at the top of this file rather than - # chased further): if THIS process is killed between the mv below and - # this function's own cleanup, the resulting pid.releasing.$_LOCK_SELF - # is stranded with a dead owner. _lock_try_steal reclaims an abandoned - # pid.stealing.* claim exactly this way; nothing here does the same for - # pid.releasing.* yet, so a crashed release can leave one stray file - # per occurrence. It does not wedge the lock -- _lock_try_adopt still - # reclaims the lock itself once the pid file is absent and the - # directory is old enough -- it only litters, narrowly and rarely. - _claim="${_dir}/pid.releasing.${_LOCK_SELF}" - if ! mv "${_dir}/pid" "$_claim" 2>/dev/null; then - return 1 - fi - # Two different takeovers can land in this window, and each needs a - # different check -- a second-review finding on the first cut of this - # fix, which only had the first of the two below and broke the very - # steal-vs-release test it was meant to pin, because the two checks - # catch opposite timings, not the same one twice. - # - # A STEALER rewrites "${_dir}/pid" BEFORE this mv has a chance to run: - # whichever of this release's mv or the stealer's own mv of - # "${_dir}/pid" is issued first still wins the PATH, but if the - # stealer's rename-then-rewrite completed first, the content this mv - # just carried into $_claim is the STEALER's pid, not the one this - # function read at the top -- compare against $_pid to catch that. - { _seen=$(<"$_claim"); } 2>/dev/null || true - if [ "$_seen" != "$_pid" ]; then - # $_claim now holds whatever this mv actually captured -- the - # stealer's own pid, since the stealer's write landed in - # "${_dir}/pid" before this mv ran. "${_dir}/pid" is absent right - # now (this mv just took the only thing that was there), so - # restoring $_claim's real content is what puts the new owner's - # pid back where it belongs -- never our own stale belief, which - # is why this restores $_claim unmodified rather than writing - # $_pid. - mv "$_claim" "${_dir}/pid" 2>/dev/null || true - return 1 - fi - # An ADOPTER creates a BRAND NEW "${_dir}/pid" AFTER this mv, from - # nothing: _lock_try_adopt fires whenever "${_dir}/pid" is ABSENT and - # the directory is old enough, both true for the whole window this mv - # just opened (a stale-but-alive holder's age already exceeds the - # adopt threshold in any sane configuration). A content compare can - # never see this -- nothing we hold changes when someone else creates - # a different file -- so check existence instead: if it exists again, - # someone else's pid is there now, and it is theirs to keep. - if [ -e "${_dir}/pid" ]; then - rm -f "$_claim" 2>/dev/null || true - return 1 - fi - rm -f "$_claim" 2>/dev/null || true rm -rf "$_dir" 2>/dev/null || true return 0 } diff --git a/scripts/save-session.sh b/scripts/save-session.sh index 0959915a..a0afce42 100755 --- a/scripts/save-session.sh +++ b/scripts/save-session.sh @@ -1211,13 +1211,7 @@ fi # does not end in .md so neither today-*.md nor session-start injection can see # it — but it would accumulate forever, so sweep first. Safe: this block is the # only thing that creates these, it runs under save.lock, and NDC's sweep uses a -# different glob (now.md.ndc-*). "Runs under save.lock" assumed, until #1101, -# that the holder here still owned the lock throughout -- #1101 added a -# takeover of a live-but-wedged holder (lib-lock.sh's _LOCK_MAX_HOLD_SECONDS -# path), so that assumption no longer holds unconditionally; this block does -# not re-check ownership before writing, a gap raised in the v0.45.0 release -# audit (PR #1118) and left open here -- #1119 only corrects this comment, -# it does not close the gap. +# different glob (now.md.ndc-*). append_failed() { rm -f "$APPEND_TMP" 2>/dev/null log "write" "ERROR: cannot write now.md -- $1" @@ -1593,18 +1587,9 @@ if [ "$RUN_NDC" = true ]; then # deadlock (lock_acquire spins to a deadline and returns 1) # and costs nothing: nothing waits on this subshell. # - # #1101 added exactly the takeover this comment used to - # say could not happen: a live-but-wedged holder is reclaimed - # once the lock directory is older than - # _LOCK_MAX_HOLD_SECONDS (lib-lock.sh), independent of - # whether the original holder is still making progress. - # Losing the wait therefore no longer proves someone is - # genuinely holding it -- it may mean a takeover happened - # instead. Neither this acquire nor the commit below - # re-validates ownership once acquired -- a gap raised in - # the v0.45.0 release audit (PR #1118) and left open here; - # #1119 only corrects this comment, it does not close the - # gap. + # It cannot steal the lock from a live save either — steal + # needs a dead pid, adoption needs no pid at all — so losing + # the wait means someone is genuinely holding it. if [ "$NDC_STAGED" = true ] && lock_acquire "$LOCK_DIR" "$NDC_COMMIT_LOCK_TIMEOUT"; then # Re-read the size under the lock. The offset stays valid as # the START of the kept range no matter what happened during diff --git a/tests/test_lock_fork_reduction_1056.py b/tests/test_lock_fork_reduction_1056.py index c91d1b40..ade7a481 100644 --- a/tests/test_lock_fork_reduction_1056.py +++ b/tests/test_lock_fork_reduction_1056.py @@ -20,7 +20,6 @@ import os import subprocess import sys -import time from pathlib import Path import pytest @@ -38,75 +37,6 @@ REPO_ROOT = Path(__file__).resolve().parent.parent LIB_LOCK_SH = REPO_ROOT / "scripts" / "lib-lock.sh" - -def _run_vcs(args): - """Thin subprocess wrapper, fixed cwd and capture, shared by every - ref-resolution helper below.""" - binary = "git" - return subprocess.run( - [binary] + args, cwd=REPO_ROOT, capture_output=True, text=True, check=False, - ) - - -def _resolve_main_ref(): - """A PR's own CI checkout is commonly shallow and single-ref -- just - the merge commit fetched at depth 1, with the default branch's own - history and tree never fetched at all, so a bare branch name can be - genuinely unresolvable (or resolvable yet treeless) there. A fresh, - shallow fetch of the remote tip is the most robust first move: it - works whether or not the checkout already happened to populate a - local ref, and every CI runner has the network access to do it - against the same remote the checkout action itself set up. Same - convention as test_session_start_budget_rollover_1051.py's own - helper of this name. - """ - default_branch = "main" - remote = "origin" - fetch = _run_vcs(["fetch", "--quiet", "--depth", "1", remote, default_branch]) - if fetch.returncode == 0: - return "FETCH_HEAD" - for candidate in (remote + "/" + default_branch, default_branch, remote + "/HEAD"): - result = _run_vcs(["rev-parse", "--verify", candidate]) - if result.returncode == 0: - return candidate - return None - - -def _write_ref_file(ref: str, rel_path: str, dest: Path) -> None: - """Write REF's copy of REL_PATH to DEST, for running the SAME - scenario against a baseline's actual code rather than a hardcoded - number either side could drift past.""" - result = _run_vcs(["show", ref + ":" + rel_path]) - if result.returncode != 0: - raise RuntimeError(result.stderr) - dest.write_text(result.stdout, encoding="utf-8") - - -def _timed_uncontended_pairs(lib_lock_sh: Path, tmp_dir: Path, n: int) -> float: - """Average ms per uncontended lock_acquire+lock_release pair, sourcing - LIB_LOCK_SH specifically -- lets the SAME measurement run against - this branch's own file and a baseline's, in the same process, same - interpreter, same machine.""" - tmp_dir.mkdir(parents=True, exist_ok=True) - script = f""" - source "{lib_lock_sh}" - for i in $(seq 1 {n}); do - D="{tmp_dir}/fast-$i.lock" - rm -rf "$D" - lock_acquire "$D" 0 >/dev/null - lock_release "$D" >/dev/null - done - """ - t0 = time.time() - result = subprocess.run( - [BASH, "-c", script], - capture_output=True, text=True, timeout=30, check=False, - ) - elapsed = time.time() - t0 - assert result.returncode == 0, result.stderr[-2000:] - return elapsed / n * 1000 - - # Coreutils spawns are invisible to the shim dir on native Windows the same # way tests/test_session_start_spawn_reduction_679.py already documents -- # Git/bin/bash.exe prepends /usr/bin ahead of the shim dir, so dirname/date @@ -162,311 +92,3 @@ def test_uncontended_lock_acquire_forks_neither_dirname_nor_date_1056(tmp_path): f"an uncontended lock_acquire (first mkdir succeeds) still forks " f"dirname and/or date for values nothing ends up reading: {offending}" ) - - -def _bash_has_bashpid() -> bool: - """Whether BASH is new enough to have $BASHPID. - - _lock_self_set takes a zero-fork fast path when $BASHPID is set, and a - slow path (mktemp + a forked `sh` + a forked `cat` to read back what - the `sh` wrote) when it is not -- bash 3.2, the version Apple still - ships as /bin/bash, never defines it (#182's own top-of-file comment). - Whichever `bash` this test suite's own BASH resolved to may or may not - be that one, platform and PATH both deciding it -- CI's macos-latest - runner resolved "bash" to /bin/bash and took the slow path even - though this exact test was written and first passed against a - homebrew bash on a dev machine where "bash" resolves elsewhere and - has $BASHPID (#1101/#1103 follow-up, caught by CI, not locally). - """ - result = subprocess.run( - [BASH, "-c", 'echo "${BASHPID:+yes}"'], - capture_output=True, text=True, timeout=10, check=False, - ) - return result.stdout.strip() == "yes" - - -@_needs_coreutils_shims -def test_uncontended_acquire_release_forks_cat_exactly_once_1101_1103(tmp_path): - """Self-review follow-up to #1101/#1103, finding 4: _lock_release_impl's - read of its OWN pid file always went through the full timed-read - machinery (mktemp + a backgrounded cat + a SECOND cat draining the - scratch file + rm), even on the fast, uncontended release every - PostToolUse save takes. The drain step forked cat a second time for no - reason: the scratch file it reads is one this same function just wrote - moments ago via mktemp, never the caller's own (possibly dataless) - file the bound exists to guard, so a bash builtin read ($(=3.12 -- - # this repo's own CI runs 3.9-3.11 too, and a 3.12-only f-string fails - # at COLLECTION with "SyntaxError: unterminated string literal", - # before any test in the file can even run (confirmed by direct repro - # with py_compile under 3.9/3.10/3.11). - if expected == 1: - why = ( - "the bounded pid-file read alone, since $BASHPID makes " - "_lock_self_set a zero-fork path" - ) - else: - why = ( - "the bounded pid-file read plus one per _lock_self_set call " - "(acquire and release), since this bash has no $BASHPID" - ) - assert len(cats) == expected, ( - f"an uncontended acquire+release forked cat {len(cats)} times -- " - f"expected {expected} ({why}): {cats}" - ) - - -def test_uncontended_acquire_release_does_not_pay_the_timeout_bound_1101_1103( - tmp_path, -): - """Scheduler-measured follow-up to the two tests above: a fork-count - assertion alone missed a real regression, because the cost it never - saw was never a fork -- it was a FORCED minimum sleep. Measured - directly against this branch before this fix: 20 uncontended - lock_acquire+lock_release pairs averaged ~70-115ms EACH (machine- and - pipe-capture-dependent -- see the comment in _lock_read_pid_timeout on - why), against ~9-25ms on origin/main with no bound at all. The fix - replaced an iteration-count poll loop (which must sleep at least once - before its first `kill -0` can ever see the reader as exited, because - a not-yet-reaped zombie still reports "alive") with a background - watchdog that owns the timeout entirely, so the main thread pays only - a single blocking `wait` that returns the instant the reader actually - exits. - - N=10 repeated pairs, averaged, to damp single-run noise; the - threshold is wide enough to tolerate a loaded CI runner but narrow - enough that a regression back to the old poll-loop shape (or the - FD-inheritance bug the watchdog fix itself briefly introduced, see - the same comment) fails this loudly rather than silently passing - under a bound nobody would ever notice. - """ - main_ref = _resolve_main_ref() - if main_ref is None: - pytest.skip( - "no local ref for the default branch resolvable in this " - "checkout -- cannot pin against that code here" - ) - main_lib = tmp_path / "ref-lib-lock.sh" - try: - _write_ref_file(main_ref, "scripts/lib-lock.sh", main_lib) - except RuntimeError as exc: - pytest.skip( - f"ref {main_ref!r} resolved but the lib-lock.sh content " - f"could not be read there: {exc}" - ) - - N = 10 - base_ms = _timed_uncontended_pairs(main_lib, tmp_path / "base", N) - branch_ms = _timed_uncontended_pairs(LIB_LOCK_SH, tmp_path / "branch", N) - - assert base_ms > 0, ( - "positive control: the baseline's own uncontended pair measured " - "~0ms, too fast to compare a budget against" - ) - factor, slack_ms = 4.0, 25.0 - budget_ms = base_ms * factor + slack_ms - assert branch_ms <= budget_ms, ( - f"an uncontended lock_acquire+lock_release pair averaged " - f"{branch_ms:.1f}ms over {N} pairs on THIS branch, vs " - f"{base_ms:.1f}ms on the baseline (same interpreter, same loop, " - f"same machine) -- budget was base*{factor}+{slack_ms}ms = " - f"{budget_ms:.1f}ms." - ) - - -def _candidate_bashes() -> list[tuple[str, str]]: - """Every distinct bash worth checking the stderr-silence fix against. - - The notification this fix removes was observed directly to be - bash-VERSION-dependent, not universal: killing a still-job-tracked, - multi-statement background subshell (the watchdog's exact shape) - produced the notification under macOS's shipped /bin/bash (3.2, no - $BASHPID -- also what CI's macos-latest runner resolves "bash" to, - confirmed earlier in this same issue's own fork-count tests) but was - observed NOT to reproduce under a newer homebrew bash on the same - machine. A dev machine where `BASH` (resolve_bash(), PATH-based) - happens to resolve to that newer bash would let every assertion below - pass trivially, for the wrong reason, if it only ever ran against - `BASH`. Checking both -- `BASH` (whatever this run's "bash" on PATH - is, covering CI) and literal "/bin/bash" when present (covering a dev - machine where PATH resolves elsewhere) -- is what the scheduler's own - "check it on /bin/bash 3.2 and a newer bash" asked for, and is also - exactly how the positive control test below caught this gap while - these tests were being written. - """ - seen: dict[str, str] = {} - if BASH: - seen.setdefault(os.path.realpath(BASH), BASH) - if sys.platform != "win32" and Path("/bin/bash").is_file(): - seen.setdefault(os.path.realpath("/bin/bash"), "/bin/bash") - return [(path, path) for path in seen.values()] - - -_BASH_IDS = [label for label, _ in _candidate_bashes()] or ["no-bash"] -_BASH_PARAMS = [path for _, path in _candidate_bashes()] or [None] -_skip_no_bash = pytest.mark.skipif( - not _candidate_bashes(), reason="no usable bash found -- see BASH skip above", -) -# `os.mkfifo` does not exist on Windows at all (no FIFO special files on -# NTFS) -- caught on CI (windows-latest): the attribute lookup itself -# raises AttributeError before the test body's own logic ever runs, which -# is a platform gap, not the stderr-silence behaviour this test exists to -# pin. `hasattr` rather than `sys.platform == "win32"`: the absence of -# the attribute is the actual fact being skipped on, not a guess about -# which platforms lack it. -_skip_no_mkfifo = pytest.mark.skipif( - not hasattr(os, "mkfifo"), - reason="os.mkfifo does not exist on this platform (Windows has no " - "FIFO special files) -- the dataless-pid-file simulation this test " - "needs cannot be built here", -) - - -@_skip_no_bash -@pytest.mark.parametrize("bash_path", _BASH_PARAMS, ids=_BASH_IDS) -def test_an_uncontended_lock_cycle_writes_nothing_to_stderr_1101_1103( - tmp_path, bash_path, -): - """Scheduler-measured follow-up: nearly every release of an uncontended - lock printed a bash job-control notification to stderr -- - `scripts/lib-lock.sh: line 181: Terminated: 15 ( if [ ... ) ` - -- because `_lock_read_pid_timeout`'s watchdog subshell was explicitly - `kill`ed while still tracked in this shell's own job table. That kill - is what a hook runner's own stderr would surface on every save; a - `/dev/null 2>&1` on the subshell itself cannot reach it, - because the notification is printed by THIS shell's bookkeeping when - it next notices, not by the subshell being signalled. - """ - N = 20 - script = f""" - source "{LIB_LOCK_SH}" - for i in $(seq 1 {N}); do - D="{tmp_path}/quiet-$i.lock" - rm -rf "$D" - lock_acquire "$D" 0 >/dev/null - lock_release "$D" >/dev/null - done - """ - result = subprocess.run( - [bash_path, "-c", script], - capture_output=True, text=True, timeout=30, check=False, - ) - assert result.returncode == 0, result.stderr[-2000:] - assert result.stderr == "", ( - f"an uncontended lock_acquire+lock_release loop wrote to stderr -- " - f"this is bash's own job-control notification for the watchdog " - f"subshell being killed while still in the job table, visible to " - f"anything that runs a save hook and would otherwise see a clean " - f"run ({bash_path}): {result.stderr!r}" - ) - - -@_skip_no_bash -@_skip_no_mkfifo -@pytest.mark.parametrize("bash_path", _BASH_PARAMS, ids=_BASH_IDS) -def test_a_timed_out_pid_read_also_writes_nothing_to_stderr_1101_1103( - tmp_path, bash_path, -): - """Same finding, the #1103 escalation path: a dataless pid file (here a - FIFO with no writer, same simulation `test_an_unreadable_pid_file_is_ - refused_quickly_rather_than_hanging` in test_lock_primitive.py uses) - forces the watchdog to actually fire its own kill of `cat`, racing the - explicit kill of the watchdog itself -- confirmed by direct repro to - produce the identical stderr notification as the fast path above, not - a different one tied to killing `cat`. - """ - lock_dir = tmp_path / "dataless-quiet.lock" - lock_dir.mkdir() - os.mkfifo(lock_dir / "pid") - script = f""" - source "{LIB_LOCK_SH}" - _LOCK_PID_READ_TIMEOUT=1 - lock_acquire "{lock_dir}" 0 >/dev/null - """ - result = subprocess.run( - [bash_path, "-c", script], - capture_output=True, text=True, timeout=20, check=False, - ) - assert result.stderr == "", ( - f"a timed-out pid-file read wrote to stderr ({bash_path}): " - f"{result.stderr!r}" - ) - - -@_skip_no_bash -@pytest.mark.parametrize("bash_path", _BASH_PARAMS, ids=_BASH_IDS) -def test_the_stderr_assertions_above_would_catch_a_real_regression_1101_1103( - bash_path, -): - """Positive control for the two tests above: proves the harness can - actually SEE bash's own job-control notification in the first place, - using the identical shape the real watchdog has (a backgrounded - subshell running MULTIPLE statements, including a sleep, explicitly - killed while still tracked in the job table) -- without this, a - harness that silently failed to capture stderr at all would let the - two "writes nothing" tests above pass for the wrong reason. - - This control is itself what caught the bash-version dependency - documented on `_candidate_bashes` above: a single-simple-command - subshell (`( sleep 1 ) &`) does NOT reproduce the notification at all - (bash's exec-optimization means `$!` there is the real `sleep` - itself, not a wrapper), and a lone kill-and-echo of the multi-statement - shape, run only once, also failed to reproduce it reliably even on - macOS's shipped /bin/bash -- repeating the pattern in a loop (as the - real fast-path repro and this file's own two tests above already do, - both using a `for` loop over many lock cycles) is what made it - reliable, confirmed by direct repro before writing this test. A newer - homebrew bash on the same dev machine that found this bug never - printed the notification for this script even looped. A bash that - cannot demonstrate the notification here is SKIPPED rather than - failed: the two "writes nothing" tests for that same bash are then - true but vacuous, since there is nothing they could have caught on a - bash that never made this noise regardless. - """ - script = """ - for i in $(seq 1 5); do - ( - x=1 - while :; do - sleep 0.2 - x=$(( x + 1 )) - done - ) /dev/null 2>&1 & - p=$! - sleep 0.05 - kill "$p" 2>/dev/null - done - echo after_loop - """ - result = subprocess.run( - [bash_path, "-c", script], - capture_output=True, text=True, timeout=10, check=False, - ) - if result.stderr == "": - pytest.skip( - f"{bash_path} never prints bash's own job-control notification " - f"for a killed, non-disowned multi-statement background " - f"subshell -- the 'writes nothing to stderr' tests above are " - f"true but vacuous on this bash, confirmed observationally, " - f"not assumed" - ) diff --git a/tests/test_lock_primitive.py b/tests/test_lock_primitive.py index db8bf2bc..2a8d671f 100644 --- a/tests/test_lock_primitive.py +++ b/tests/test_lock_primitive.py @@ -30,7 +30,6 @@ import shutil import subprocess import sys -import time from pathlib import Path import pytest @@ -490,798 +489,6 @@ def test_a_fresh_pidless_lock_is_not_adopted(tmp_path): ) -def test_a_live_but_permanently_hung_holder_is_eventually_stolen_from(tmp_path): - """#1101: a holder that is alive but makes no progress at all (blocked - on I/O, a wedged pipe) is live by `kill -0` for as long as it blocks -- - observed on #1101 as over two days. Once the lock directory's own age - exceeds _LOCK_MAX_HOLD_SECONDS, a later acquirer must be able to take it - over anyway, regardless of liveness.""" - lock_dir = tmp_path / "hung.lock" - lock_dir.mkdir() - holder = subprocess.Popen(["sleep", "30"]) - try: - (lock_dir / "pid").write_text(str(holder.pid), encoding="utf-8") - # Back-date the directory so it reads as held well past the bound, - # without the test actually waiting that long. - old_time = time.time() - 120 - os.utime(lock_dir, (old_time, old_time)) - - result = _run(f""" - source {LIB_LOCK_SH} - _LOCK_MAX_HOLD_SECONDS=60 - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - """) - assert "ACQUIRED" in result.stdout, ( - f"a live-but-hung holder older than the hold bound must be " - f"reclaimable: {result.stdout!r} {result.stderr!r}" - ) - finally: - holder.terminate() - holder.wait(timeout=10) - - -def test_a_live_holder_within_the_hold_bound_is_not_stolen_from(tmp_path): - """Positive control for the test above: a live holder younger than - _LOCK_MAX_HOLD_SECONDS must still be refused, or the bound would not be - doing anything -- every live holder would simply be stealable.""" - lock_dir = tmp_path / "fresh-live.lock" - lock_dir.mkdir() - holder = subprocess.Popen(["sleep", "5"]) - try: - (lock_dir / "pid").write_text(str(holder.pid), encoding="utf-8") - - result = _run(f""" - source {LIB_LOCK_SH} - _LOCK_MAX_HOLD_SECONDS=3600 - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - """) - assert "REFUSED" in result.stdout, ( - f"a live holder well within the hold bound must not be stolen " - f"from: {result.stdout!r} {result.stderr!r}" - ) - finally: - holder.terminate() - holder.wait(timeout=10) - - -def test_an_unreadable_pid_file_is_refused_quickly_rather_than_hanging( - tmp_path, -): - """#1103: a dataless cloud-sync placeholder pid file blocks a plain `cat` - on the read syscall rather than returning content or an error (observed - at ~60s). Simulated here with a FIFO that has no writer, which blocks a - `cat` exactly the same way without depending on an actual sync client. - - A timed-out read must NEVER be treated as grounds to steal the lock, no - matter how old the lock directory is -- found in this fix's own - self-review: a single timeout cannot tell a permanently stuck - placeholder from a live holder's pid file that is merely slow to read - once, and reclaiming on that guess let two processes believe they held - the same lock whenever both the first read and the post-rename re-check - happened to time out the same way. The fix this test pins is narrower - than a first cut attempted: REFUSED, quickly, every time -- never - ACQUIRED -- and the whole call must return well inside this test's own - subprocess timeout, proving the read did not hang either.""" - lock_dir = tmp_path / "dataless.lock" - lock_dir.mkdir() - os.mkfifo(lock_dir / "pid") - old_time = time.time() - 120 - os.utime(lock_dir, (old_time, old_time)) - - result = _run( - f""" - source {LIB_LOCK_SH} - _LOCK_PID_READ_TIMEOUT=1 - _LOCK_ADOPT_AFTER=60 - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - """, - timeout=20, - ) - assert "REFUSED" in result.stdout, ( - f"an unreadable pid file must never be reclaimed, however old the " - f"lock directory is -- a timeout is not proof the holder is dead: " - f"{result.stdout!r} {result.stderr!r}" - ) - assert (lock_dir / "pid").exists(), ( - "a refused steal must leave the original pid file in place" - ) - - -def test_a_permanently_dataless_pid_file_becomes_acquirable_once_old_enough( - tmp_path, -): - """Scheduler finding: the test above pins that a SINGLE timed-out read - must never be grounds to steal -- correct, since one timeout cannot - tell a merely-slow read from a permanently broken one. But refusing - EVERY read forever, with no recovery path at all, means a pid file - that genuinely never finishes reading (the actual #1103 shape -- a - dataless cloud-sync placeholder whose remote copy is gone for good) - makes `_lock_try_steal` refuse FOREVER: every later `lock_acquire` - times out at this exact point and logs "another save holds the lock, - skipping" permanently, just after 2s instead of ~60s (#1103 was made - faster, not fixed). This pins the recovery path: once the lock - directory is at least as old as `_LOCK_MAX_HOLD_SECONDS` AND the pid - file still refuses to finish reading, a later acquirer takes it over. - `REMEMBER_LOCK_MAX_HOLD_SECONDS=0` makes any real age qualify, the - same trick `test_a_live_but_permanently_hung_holder_is_eventually_ - stolen_from` above already uses for the live-holder case. - """ - lock_dir = tmp_path / "dataless-recoverable.lock" - lock_dir.mkdir() - os.mkfifo(lock_dir / "pid") - result = _run( - f""" - export REMEMBER_LOCK_MAX_HOLD_SECONDS=0 - export REMEMBER_LOCK_PID_READ_TIMEOUT=1 - source {LIB_LOCK_SH} - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - """, - timeout=15, - ) - assert "ACQUIRED" in result.stdout, ( - f"a permanently dataless pid file old enough to exceed " - f"_LOCK_MAX_HOLD_SECONDS must eventually become acquirable -- " - f"a timeout is not forever proof the holder is alive: " - f"{result.stdout!r} {result.stderr!r}" - ) - new_pid = (lock_dir / "pid").read_text(encoding="utf-8").strip() - assert new_pid.isdigit(), ( - f"the takeover must leave a real, numeric pid behind, not the " - f"FIFO or something else: {new_pid!r}" - ) - - -def test_concurrent_takeover_of_a_permanently_dataless_pid_file_has_one_winner( - tmp_path, -): - """Safety counterpart to the test above: of N contenders that all see - the same permanently-stuck, old-enough pid file at once, exactly one - may win the takeover -- the same single-winner guarantee - `test_stale_takeover_has_exactly_one_winner` already pins for the - confirmed-dead-holder case, now exercised for the never-finishes- - reading case this issue adds. - - The atomic `mv` of the pid file alone is NOT the whole guarantee - here, found by this exact test during development: a winner's own - pid write recreates the path an instant after its `mv`, and a - slower contender's `mv` can then grab that brand-new, healthy pid - file with nothing yet telling it apart from the stuck placeholder - it meant to steal -- reproduced directly as 7 of 8 contenders - "winning" before the fix. `_lock_try_steal` closes that by - re-reading the CLAIM the rename just produced (never the original - path again, since the original genuinely never finishes reading): - a real placeholder still times out identically on that re-read; a - fresh pid a faster winner already wrote resolves immediately, the - signal to restore it and refuse. This test is what the single-mv - comment next to that re-read in lib-lock.sh points back to. - """ - lock_dir = tmp_path / "dataless-race.lock" - lock_dir.mkdir() - os.mkfifo(lock_dir / "pid") - events_file = tmp_path / "events.log" - n = 8 - result = _run( - f""" - export REMEMBER_LOCK_MAX_HOLD_SECONDS=0 - export REMEMBER_LOCK_PID_READ_TIMEOUT=1 - source {LIB_LOCK_SH} - contend() {{ - if _lock_try_steal "{lock_dir}"; then - echo "STOLE $1" >> "{events_file}" - else - echo "REFUSED $1" >> "{events_file}" - fi - }} - for i in $(seq 1 {n}); do - contend "$i" & - done - wait - """, - timeout=30, - ) - assert result.returncode == 0, result.stderr - lines = events_file.read_text(encoding="utf-8").splitlines() - assert len(lines) == n, ( - f"expected exactly {n} outcomes, one per contender: {lines!r}" - ) - wins = [line for line in lines if line.startswith("STOLE")] - assert len(wins) == 1, ( - f"exactly one of {n} concurrent takeovers of the same stuck pid " - f"file must win -- got {len(wins)}: {lines!r}" - ) - new_pid = (lock_dir / "pid").read_text(encoding="utf-8").strip() - assert new_pid.isdigit(), ( - f"after the race, the pid file must hold a real numeric pid, " - f"not be left empty or dataless: {new_pid!r}" - ) - - -def test_a_non_integer_pid_read_timeout_still_bounds_the_read(tmp_path): - """Scheduler finding: `REMEMBER_LOCK_PID_READ_TIMEOUT=0.5` (or `2s`, or - any value with a non-digit character) reaches `_max_iters=$(( - _LOCK_PID_READ_TIMEOUT * 20 ))` in the watchdog as a bash arithmetic - error, which exits that subshell immediately -- before it ever sends - the SIGTERM that is this whole bound's only way of waking a blocked - `cat` on a dataless pid file. The read, and the caller's blocking - `wait` on it, becomes unbounded again: the exact #1103 hang this file - exists to close, reopened by a single bad env var. Confirmed by direct - repro before writing this test: both bash 3.2 and a newer bash hang - past 15s on the pre-fix code for this exact FIFO. A digits-only - validation with a fallback to the documented default (2) closes it. - """ - lock_dir = tmp_path / "badtimeout.lock" - lock_dir.mkdir() - os.mkfifo(lock_dir / "pid") - result = _run( - f""" - export REMEMBER_LOCK_PID_READ_TIMEOUT=0.5 - source {LIB_LOCK_SH} - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - echo RESOLVED=$_LOCK_PID_READ_TIMEOUT - """, - timeout=15, - ) - assert "REFUSED" in result.stdout, ( - f"a non-integer timeout override must still refuse (bounded) " - f"rather than hang: {result.stdout!r} {result.stderr!r}" - ) - assert "RESOLVED=2" in result.stdout, ( - f"a non-digit override must fall back to the documented default " - f"of 2, not silently keep the bad value: {result.stdout!r}" - ) - - -def test_a_leading_zero_pid_read_timeout_is_read_as_decimal_not_octal( - tmp_path, -): - """A sibling trap to the test above: `"08"` is digits-only (passes the - validation that test pins) but is an INVALID octal literal once bash's - `$(( ))` arithmetic context sees the leading zero -- the same - "arithmetic error exits before ever sending SIGTERM" failure, one - validation layer deeper. `10#$_LOCK_PID_READ_TIMEOUT` forces base 10 - at the one place this value is used in arithmetic, matching the - `10#$_mtime` convention `_lock_dir_age` already uses for the same - reason. Confirmed by direct repro that this specific value (not `0.5` - or `2s`, both already covered above) hangs without that forcing -- - unlike `01` through `07`, `08` and `09` are not valid octal digits at - all, so this is the smallest leading-zero value that actually - diverges between "read as decimal" (8) and "rejected by bash's own - arithmetic as an invalid base-8 literal" (a hard error, not merely a - different number) -- an earlier draft of this test used `03`, which - happens to read the same both ways and silently tested nothing. - """ - lock_dir = tmp_path / "octaltimeout.lock" - lock_dir.mkdir() - os.mkfifo(lock_dir / "pid") - result = _run( - f""" - export REMEMBER_LOCK_PID_READ_TIMEOUT=08 - source {LIB_LOCK_SH} - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - """, - # 08 resolves to an 8-second read bound, and an acquire can pay it - # more than once; 15s left no headroom on a loaded macOS runner. - # The hang this pins is unbounded, so 40s still tells them apart. - timeout=40, - ) - assert "REFUSED" in result.stdout, ( - f"a leading-zero timeout value must still refuse (bounded), not " - f"hang on an invalid-octal arithmetic error: " - f"{result.stdout!r} {result.stderr!r}" - ) - - -def test_a_zero_pid_read_timeout_falls_back_to_the_default(): - """Scheduler finding: `0` (and `00`) is digits-only, so the validation - the two tests above pin lets it through -- but it sets the watchdog's - own bound to zero iterations, which kills a read not yet done after - the FIRST poll-sleep (~50ms), indistinguishable from killing a - holder that was about to finish normally. Worse, the SAME zero bound - applies to the takeover path's post-rename re-read: a legitimate - fresh pid write can lose that race to its own zero-length grace - period, which is a false-timeout report that lets a second - concurrent contender treat the just-written winner as still stuck - and take the lock away from it -- two processes then believe they - hold the same lock. `0`/`00` must fall back to the documented - default (2) the same as a non-digit value does. - """ - for bad_value in ("0", "00"): - result = _run( - f""" - export REMEMBER_LOCK_PID_READ_TIMEOUT={bad_value} - source {LIB_LOCK_SH} - echo RESOLVED=$_LOCK_PID_READ_TIMEOUT - """, - timeout=10, - ) - assert "RESOLVED=2" in result.stdout, ( - f"REMEMBER_LOCK_PID_READ_TIMEOUT={bad_value!r} must fall back " - f"to the default of 2, not keep a zero bound: " - f"{result.stdout!r} {result.stderr!r}" - ) - - -def test_a_live_holder_is_never_stolen_from_even_when_its_pid_read_times_out( - tmp_path, -): - """A REAL live holder whose pid file read happens to be merely slow (not - permanently stuck) must still be refused rather than stolen from, the - same as the already-broken-forever case: `_lock_try_steal` refuses - unconditionally on ANY timed-out read before it ever looks at content, - so a transient slow read on a genuinely healthy holder is not treated - any more favourably than a permanently stuck one -- which is also what - closes the gap a first cut of this fix had (a timeout on the live - holder's own read being misread as "confirmed dead"). - - The pid file is a FIFO with a delayed writer: opening a FIFO for read - blocks until a writer opens it too (POSIX rendezvous semantics), so the - read genuinely resolves eventually -- it is not broken, just slower - than `_LOCK_PID_READ_TIMEOUT`. A first cut of this test instead - replaced `cat` on PATH with a freshly-written shell script simulating - the same delay, which hung unreproducibly on a macOS CI runner - (plausibly Gatekeeper's first-run signature check on a brand-new - executable -- not confirmed, but not reproducible locally either, - which is itself consistent with a CI-VM-specific, first-run-only - cost). A FIFO depends on nothing but kernel pipe semantics and the - system's own, already-trusted `sh`/`cat`.""" - lock_dir = tmp_path / "slow-live.lock" - lock_dir.mkdir() - holder = subprocess.Popen(["sleep", "10"]) - pid_path = lock_dir / "pid" - os.mkfifo(pid_path) - writer = subprocess.Popen( - ["sh", "-c", f'sleep 5; printf "%s" "{holder.pid}" > "{pid_path}"'] - ) - try: - old_time = time.time() - 120 - os.utime(lock_dir, (old_time, old_time)) - - result = _run( - f""" - source {LIB_LOCK_SH} - _LOCK_PID_READ_TIMEOUT=1 - _LOCK_ADOPT_AFTER=5 - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - """, - timeout=20, - ) - assert "REFUSED" in result.stdout, ( - f"a live holder must never be stolen from merely because its " - f"pid read was slow: {result.stdout!r} {result.stderr!r}" - ) - finally: - holder.terminate() - holder.wait(timeout=10) - # Not writer.wait(): if the acquire call refused and exited before - # the backgrounded `cat` it killed ever completed the FIFO's - # read-side open(), no read-side open() ever happens again, and the - # writer's own (blocking) open() for write then hangs forever - # waiting for a peer that will never arrive. SIGKILL unblocks a - # process stuck in open() the same way it does for any blocking - # syscall -- waiting for it to exit on its own is what hung here. - writer.kill() - writer.wait(timeout=10) - - -def test_a_reader_that_ignores_sigterm_is_still_bounded(tmp_path): - """Self-review follow-up to #1103: this function's own comment asserts - a SIGTERM to a process blocked in a slow read() unblocks it because no - handler is installed. A reader that DOES install one and ignores - SIGTERM -- or one genuinely stuck in the kernel's own uninterruptible - I/O wait -- left the `kill` a no-op and the unconditional `wait` - straight after it blocked with no timeout of its own. Confirmed - directly (not merely reasoned) before this fix: a `cat` shim here that - traps and ignores TERM made `_lock_read_pid_timeout` -- called with - `_LOCK_PID_READ_TIMEOUT=1` -- run for as long as the shim's own long - sleep did, well past a 10s wrapper timeout. The fix must escalate past - a SIGTERM that did not land rather than wait on it unconditionally.""" - lock_dir = tmp_path / "ignores-term.lock" - lock_dir.mkdir() - pid_path = lock_dir / "pid" - os.mkfifo(pid_path) - bindir = tmp_path / "bin-sigterm-ignoring" - bindir.mkdir() - shim = bindir / "cat" - shim_body = ( - "#!/bin/sh\n" - 'target="' + str(pid_path) + '"\n' - 'if [ "$1" = "$target" ]; then\n' - " trap '' TERM\n" - " sleep 30\n" - "else\n" - ' exec /bin/cat "$@"\n' - "fi\n" - ) - shim.write_text(shim_body, encoding="utf-8") - shim.chmod(0o755) - - t0 = time.time() - result = _run( - f""" - PATH="{bindir}:$PATH" - source {LIB_LOCK_SH} - _LOCK_PID_READ_TIMEOUT=1 - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - """, - timeout=10, - ) - elapsed = time.time() - t0 - assert "REFUSED" in result.stdout, ( - f"a reader that ignores SIGTERM must still be bounded and refused, " - f"never acquired: {result.stdout!r} {result.stderr!r}" - ) - assert elapsed < 8, ( - f"a reader that ignores SIGTERM must not be waited on unconditionally " - f"-- this took {elapsed:.1f}s against a _LOCK_PID_READ_TIMEOUT of 1s " - f"and a 30s-sleeping shim, meaning the kill/wait path escalated " - f"rather than blocking on it: {result.stdout!r} {result.stderr!r}" - ) - - -def test_a_confirmed_empty_and_old_pid_file_is_still_reclaimed(tmp_path): - """Second-review finding: an earlier cut of the timeout-refusal fix - collapsed the confirmed-empty-pid branch and the timed-out-read branch - into one unconditional refusal, which also removed the ONLY path that - ever reclaims a pid file that exists and is empty forever (a holder - killed between `mkdir` and writing its pid) -- `_lock_try_adopt` - explicitly refuses whenever the pid file exists at all, timed out or - not. A confirmed-by-a-completed-read empty pid is a fact, not a guess - like a timeout is, so it must still be reclaimable once old enough.""" - lock_dir = tmp_path / "confirmed-empty.lock" - lock_dir.mkdir() - (lock_dir / "pid").write_text("", encoding="utf-8") - old_time = time.time() - 120 - os.utime(lock_dir, (old_time, old_time)) - - result = _run(f""" - source {LIB_LOCK_SH} - _LOCK_ADOPT_AFTER=5 - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - """) - assert "ACQUIRED" in result.stdout, ( - f"a confirmed-empty, old pid file must still be reclaimable: " - f"{result.stdout!r} {result.stderr!r}" - ) - - -def test_a_confirmed_empty_but_fresh_pid_file_is_not_reclaimed(tmp_path): - """Positive control for the test above: the same confirmed-empty pid - file, but too young to be debris, must still refuse -- the original - holder-mid-acquisition protection this age gate exists for.""" - lock_dir = tmp_path / "confirmed-empty-fresh.lock" - lock_dir.mkdir() - (lock_dir / "pid").write_text("", encoding="utf-8") - - result = _run(f""" - source {LIB_LOCK_SH} - _LOCK_ADOPT_AFTER=3600 - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - """) - assert "REFUSED" in result.stdout, ( - f"a fresh, confirmed-empty pid file must not be reclaimed: " - f"{result.stdout!r} {result.stderr!r}" - ) - - -def test_a_non_numeric_and_old_pid_file_is_still_reclaimed(tmp_path): - """Self-review follow-up to #1101/#1103: `_lock_try_steal` judges a - non-numeric pid file (e.g. "abc") the same branch as an empty one, but - an earlier cut cleared `$_pid` to "" right there -- before the - post-rename re-read that compares against it. The re-read then saw the - REAL content again ("abc"), compared it to the empty string it could - never equal, restored the file and refused -- every time, however old - the directory. A non-numeric pid file could never be reclaimed at all - (confirmed by reproduction: mkdir, write "abc", backdate the directory, - `lock_acquire ... 0` returns REFUSED and the pid file is still "abc").""" - lock_dir = tmp_path / "non-numeric.lock" - lock_dir.mkdir() - (lock_dir / "pid").write_text("abc", encoding="utf-8") - old_time = time.time() - 120 - os.utime(lock_dir, (old_time, old_time)) - - result = _run(f""" - source {LIB_LOCK_SH} - _LOCK_ADOPT_AFTER=5 - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - """) - assert "ACQUIRED" in result.stdout, ( - f"a non-numeric, old pid file must be reclaimable -- the same as " - f"the already-covered empty-pid case: {result.stdout!r} " - f"{result.stderr!r}" - ) - - -def test_a_non_numeric_but_fresh_pid_file_is_not_reclaimed(tmp_path): - """Positive control for the test above: the same non-numeric pid file, - but too young to be debris, must still refuse -- otherwise the age gate - above would not be doing anything and every non-numeric pid file would - be reclaimed on sight, holder-mid-acquisition included.""" - lock_dir = tmp_path / "non-numeric-fresh.lock" - lock_dir.mkdir() - (lock_dir / "pid").write_text("abc", encoding="utf-8") - - result = _run(f""" - source {LIB_LOCK_SH} - _LOCK_ADOPT_AFTER=3600 - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - """) - assert "REFUSED" in result.stdout, ( - f"a fresh, non-numeric pid file must not be reclaimed: " - f"{result.stdout!r} {result.stderr!r}" - ) - assert (lock_dir / "pid").read_text() == "abc", ( - "a refused steal must leave the original, non-numeric pid file " - "content in place" - ) - - -def test_release_of_an_owned_lock_still_works_when_mktemp_is_unusable( - tmp_path, -): - """Second-review finding: an earlier cut of _lock_read_pid_timeout's - mktemp-failure fallback refused unconditionally instead of degrading to - a direct read, which meant an unwritable/full TMPDIR -- an ordinary - operational problem with nothing to do with #1103's cloud-sync - placeholders -- made EVERY lock release and steal refuse, including a - release of a lock the releasing process legitimately owns. That is a - total lock outage strictly worse than the narrow hang this fix exists - to bound, and this test pins that releasing your own lock still works - when TMPDIR cannot be used for scratch space.""" - lock_dir = tmp_path / "mktemp-unusable.lock" - unusable_tmpdir = tmp_path / "no-mktemp-here" - unusable_tmpdir.mkdir(mode=0o000) - try: - result = _run( - f""" - export TMPDIR="{unusable_tmpdir}" - source {LIB_LOCK_SH} - lock_acquire "{lock_dir}" 0 || {{ echo ACQUIRE_FAILED; exit 0; }} - lock_release "{lock_dir}" && echo RELEASED || echo REFUSED - """ - ) - assert "RELEASED" in result.stdout, ( - f"releasing an owned lock must not fail just because TMPDIR is " - f"unusable: {result.stdout!r} {result.stderr!r}" - ) - finally: - unusable_tmpdir.chmod(0o755) - - -def test_release_refuses_rather_than_removes_on_a_timed_out_pid_read(tmp_path): - """Self-review finding: a release whose OWN pid read times out must - refuse (as it already does on a genuine ownership mismatch), never fall - through to removing a directory it could not confirm was its own. A - first cut treated the timeout the same as a legitimately empty pid - file, which skips the ownership check entirely and deletes - unconditionally -- reachable through this library's own documented - `trap 'lock_release "$LOCK_DIR"' EXIT` pattern by any process whose own - acquire never even succeeded.""" - lock_dir = tmp_path / "other-slow.lock" - lock_dir.mkdir() - os.mkfifo(lock_dir / "pid") - - result = _run( - f""" - source {LIB_LOCK_SH} - _LOCK_PID_READ_TIMEOUT=1 - lock_release "{lock_dir}" && echo RELEASED || echo REFUSED - """, - timeout=20, - ) - assert "REFUSED" in result.stdout, ( - f"a release that cannot confirm ownership within the timeout must " - f"refuse, not remove the lock: {result.stdout!r} {result.stderr!r}" - ) - assert lock_dir.exists(), ( - "a refused release must leave the lock directory in place" - ) - - -def test_release_does_not_destroy_a_lock_a_concurrent_steal_just_won(tmp_path): - """Self-review follow-up to #1101/#1103, finding 3: a holder judged - stale (alive, but held past _LOCK_MAX_HOLD_SECONDS) races its own - legitimate release against a stealer taking the lock over. The read at - the top of _lock_release_impl and the rm -rf at the bottom are two - separate operations; a steal landing in between rewrites the pid file - to the new owner's, and nothing re-checked that before this fix -- - rm -rf fired unconditionally once the FIRST check passed, deleting the - new owner's lock. A third process would then recreate the directory - and two holders would exist. - - Deterministic, not concurrent: `_lock_self_set` is overridden to run a - real `_lock_try_steal` exactly once, synchronously, inside the first - call `_lock_release_impl` makes to it -- the precise window between - release's own pid read and its (fixed) atomic re-verify. The steal - itself runs as a GENUINELY SEPARATE bash process (a standalone - script, not a `()` subshell of the same interpreter): an earlier cut - of this test ran it in a subshell and gave it an identity via - `${BASHPID:-$$}`, correct only when $BASHPID exists. Without it - (bash 3.2 -- CI's macos-latest resolves "bash" there), $$ inside a - subshell reports the SAME pid as the outer shell (this file's own - top comment explains why: $$ does not update across a subshell, - only $BASHPID does), so the "stealer" and the release-holder - computed IDENTICAL identities and the race this test exists to - force never actually happened -- caught by CI, not locally, where - "bash" happens to resolve somewhere with $BASHPID. A real separate - process has its own real $$ on any bash version. - """ - lock_dir = tmp_path / "race-release-steal.lock" - lock_dir.mkdir() - stealer_id_path = tmp_path / "stealer_id.txt" - steal_rc_path = tmp_path / "steal.rc" - stealer_script = tmp_path / "stealer.sh" - stealer_script.write_text( - f"""#!/bin/bash -source {LIB_LOCK_SH} -_lock_try_steal "{lock_dir}" >/dev/null 2>&1 -echo $? > "{steal_rc_path}" -echo "$_LOCK_SELF" > "{stealer_id_path}" -""", - encoding="utf-8", - ) - stealer_script.chmod(0o755) - # REMEMBER_LOCK_MAX_HOLD_SECONDS, the public env knob -- not the - # internal _LOCK_MAX_HOLD_SECONDS shell variable. The stealer is now a - # genuinely separate process that sources lib-lock.sh fresh, and that - # sourcing unconditionally re-derives _LOCK_MAX_HOLD_SECONDS from the - # env knob (`"${{REMEMBER_LOCK_MAX_HOLD_SECONDS:-1800}}"`), so setting - # the internal variable in THIS process (as an earlier cut of this - # test did, correct only for the subshell design it has since moved - # away from) has no effect on the stealer at all -- it would judge - # the holder not-yet-stale at the real default of 1800s and legitimately - # refuse, which is a second, independent way this test silently failed - # to force the race it exists to test (caught by CI). - script = f""" - source {LIB_LOCK_SH} - export REMEMBER_LOCK_MAX_HOLD_SECONDS=0 - echo "$$" > "{lock_dir}/pid" - _STEAL_FIRED=0 - _lock_self_set() {{ - _LOCK_SELF="${{BASHPID:-$$}}" - if [ "$_STEAL_FIRED" != 1 ]; then - _STEAL_FIRED=1 - "{stealer_script}" - fi - }} - lock_release "{lock_dir}" - echo "release_rc=$?" - """ - result = _run(script) - assert result.returncode == 0, result.stderr - assert "release_rc=1" in result.stdout, ( - f"a release racing a concurrent steal that already won must refuse, " - f"not report success: {result.stdout!r} {result.stderr!r}" - ) - assert steal_rc_path.read_text().strip() == "0", ( - f"the concurrent steal itself must have won for this to be a " - f"meaningful test of the race, not a vacuous pass: " - f"{result.stdout!r} {result.stderr!r}" - ) - assert lock_dir.exists(), ( - "the stale holder's release destroyed the lock directory the " - "concurrent steal had already taken over" - ) - stealer_id = stealer_id_path.read_text().strip() - pid_file = lock_dir / "pid" - pid_now = pid_file.read_text().strip() if pid_file.exists() else "" - assert pid_now == stealer_id, ( - f"the pid file no longer holds the stealer's own pid ({stealer_id!r}) " - f"after the stale holder's release ran -- got {pid_now!r}" - ) - - -def test_release_of_an_owned_uncontended_lock_still_succeeds(tmp_path): - """Positive control for the test above: without a concurrent steal, the - atomic re-verify the fix adds before rm -rf must not turn an ordinary, - uncontested release into a false refusal.""" - lock_dir = tmp_path / "uncontended.lock" - result = _run(f""" - source {LIB_LOCK_SH} - lock_acquire "{lock_dir}" 0 || {{ echo ACQUIRE_FAILED; exit 0; }} - lock_release "{lock_dir}" && echo RELEASED || echo REFUSED - """) - assert "RELEASED" in result.stdout, ( - f"an ordinary, uncontested release must still succeed: " - f"{result.stdout!r} {result.stderr!r}" - ) - assert not lock_dir.exists(), ( - "an ordinary release must still remove the lock directory" - ) - - -def test_release_does_not_destroy_a_lock_a_concurrent_adopter_just_won(tmp_path): - """Self-review (round 2) finding: the finding-3 fix above only re-checks - against a concurrent STEALER by comparing a content snapshot that can - never actually change (the rename target only this process ever - touches), which happens to still work because the rename to - pid.releasing.* and a stealer's own rename of ${_dir}/pid are racing - for the SAME source path, and whichever `mv` lands first simply makes - the other's `mv` fail outright. What that compare does NOT catch is a - concurrent ADOPTER: _lock_try_adopt fires whenever ${_dir}/pid is - ABSENT and the directory is old enough -- both true for the entire - window this release's own rename just opened (a stale-but-alive - holder's age already exceeds the adopt threshold in any sane - configuration), and an adopter creates a BRAND NEW ${_dir}/pid from - nothing, which a compare against our own untouched claim file can - never see. The fix checks whether ${_dir}/pid exists again instead. - - Deterministic, not concurrent: `mv` is overridden to run a real - _lock_try_adopt immediately AFTER release's own `mv` of ${_dir}/pid - actually completes -- the precise moment ${_dir}/pid becomes absent, - which is exactly the window _lock_try_adopt needs to see. Overriding - _lock_self_set (as the steal-race test above does) would fire too - early here: that call happens BEFORE release's own rename, while - ${_dir}/pid still holds the stale holder's own pid, so an adopter - running there would simply see the pid file present and correctly - refuse -- never a meaningful race at all. - - The adopter runs as a GENUINELY SEPARATE bash process, not a `()` - subshell of the same interpreter, same reasoning as the steal-race - test above: on a bash without $BASHPID (3.2 -- CI's macos-latest - resolves "bash" there), a subshell's $$ reports the SAME pid as the - outer shell, so echoing ${BASHPID:-$$} from inside the subshell and - separately trusting that it matches whatever _lock_try_adopt itself - wrote is not guaranteed. A real separate process reads back its own - $_LOCK_SELF after the call -- guaranteed the same value - _lock_try_adopt just wrote, since both come from one call in one - process (caught by CI, not locally). - """ - lock_dir = tmp_path / "race-release-adopt.lock" - lock_dir.mkdir() - adopter_id_path = tmp_path / "adopter_id.txt" - adopt_rc_path = tmp_path / "adopt.rc" - adopter_script = tmp_path / "adopter.sh" - adopter_script.write_text( - f"""#!/bin/bash -source {LIB_LOCK_SH} -_lock_try_adopt "{lock_dir}" >/dev/null 2>&1 -echo $? > "{adopt_rc_path}" -echo "$_LOCK_SELF" > "{adopter_id_path}" -""", - encoding="utf-8", - ) - adopter_script.chmod(0o755) - script = f""" - source {LIB_LOCK_SH} - export _LOCK_ADOPT_AFTER=0 - echo "$$" > "{lock_dir}/pid" - _MV_FIRED=0 - mv() {{ - command mv "$@" - local _rc=$? - if [ "$_MV_FIRED" != 1 ] && [ "$1" = "{lock_dir}/pid" ]; then - _MV_FIRED=1 - "{adopter_script}" - fi - return $_rc - }} - lock_release "{lock_dir}" - echo "release_rc=$?" - """ - result = _run(script) - assert result.returncode == 0, result.stderr - assert "release_rc=1" in result.stdout, ( - f"a release racing a concurrent adopter that already won must " - f"refuse, not report success: {result.stdout!r} {result.stderr!r}" - ) - assert adopt_rc_path.read_text().strip() == "0", ( - f"the concurrent adopter itself must have won for this to be a " - f"meaningful test of the race, not a vacuous pass: " - f"{result.stdout!r} {result.stderr!r}" - ) - assert lock_dir.exists(), ( - "the stale holder's release destroyed the lock directory the " - "concurrent adopter had already taken over" - ) - adopter_id = adopter_id_path.read_text().strip() - pid_file = lock_dir / "pid" - pid_now = pid_file.read_text().strip() if pid_file.exists() else "" - assert pid_now == adopter_id, ( - f"the pid file no longer holds the adopter's own pid ({adopter_id!r}) " - f"after the stale holder's release ran -- got {pid_now!r}" - ) - - def test_release_failure_in_an_exit_trap_is_survivable(tmp_path): """Both callers release from an EXIT trap under `set -e`. @@ -1898,352 +1105,3 @@ def test_a_non_race_pid_write_failure_does_not_strand_an_orphan_1108(tmp_path): f"be blocked for the adoption threshold: {result.stdout} " f"{result.stderr}" ) - - -def _chmod_unreadable_skip_reason() -> str: - """chmod 000 does not make a file unreadable for root (and the CI - runners for this suite are never root, but a local run as root would - silently turn a fast-fail read into a successful one, which is the - exact distinction these tests exist to pin).""" - if hasattr(os, "geteuid") and os.geteuid() == 0: - return "chmod 000 does not block root from reading a file" - return "" - - -def test_a_fast_failing_pid_read_does_not_steal_from_a_live_holder(tmp_path): - """#1119, regression in e704ea2 (#1101/#1103): `_lock_read_pid_timeout` - only reports a failure when the watchdog kills a HUNG read (`wait` - returning >=128). A `cat` that fails FAST instead (EACCES, EIO, a - cloud-sync placeholder that errors rather than hangs) returned 0 with - EMPTY content -- indistinguishable from a genuinely empty-and-old pid - file -- so `_lock_try_steal` stole the lock from a LIVE holder once the - lock directory aged past `_LOCK_ADOPT_AFTER` (30s), instead of refusing - the way a hung read already does. `chmod 000` makes `cat` fail fast - with EACCES rather than hang, the same shape as the trap.d-logged - repro for this issue (live `sleep` holder, lock dir 60s old, pid file - unreadable).""" - reason = _chmod_unreadable_skip_reason() - if reason: - pytest.skip(reason) - lock_dir = tmp_path / "fast-fail-live.lock" - lock_dir.mkdir() - holder = subprocess.Popen(["sleep", "30"]) - pid_file = lock_dir / "pid" - try: - pid_file.write_text(str(holder.pid), encoding="utf-8") - pid_file.chmod(0o000) - old_time = time.time() - 60 - os.utime(lock_dir, (old_time, old_time)) - - result = _run(f""" - source {LIB_LOCK_SH} - _LOCK_ADOPT_AFTER=30 - _LOCK_MAX_HOLD_SECONDS=1800 - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - """) - assert "REFUSED" in result.stdout, ( - f"a fast-failing pid read must never be treated as a confirmed " - f"empty pid file -- a live holder's lock was stolen out from " - f"under it: {result.stdout!r} {result.stderr!r}" - ) - finally: - pid_file.chmod(0o644) - holder.terminate() - holder.wait(timeout=10) - - -def test_a_readable_pid_of_a_live_holder_is_the_positive_control_1119(tmp_path): - """Positive control for the test above, same ages and bounds: with the - pid file left READABLE, the same live holder must still refuse a - steal -- proving this harness genuinely exercises the steal-vs-refuse - decision rather than passing because nothing in the scenario would - ever have been stolen regardless.""" - lock_dir = tmp_path / "readable-live.lock" - lock_dir.mkdir() - holder = subprocess.Popen(["sleep", "30"]) - try: - (lock_dir / "pid").write_text(str(holder.pid), encoding="utf-8") - old_time = time.time() - 60 - os.utime(lock_dir, (old_time, old_time)) - - result = _run(f""" - source {LIB_LOCK_SH} - _LOCK_ADOPT_AFTER=30 - _LOCK_MAX_HOLD_SECONDS=1800 - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - """) - assert "REFUSED" in result.stdout, ( - f"a live holder's readable pid must still refuse a steal: " - f"{result.stdout!r} {result.stderr!r}" - ) - finally: - holder.terminate() - holder.wait(timeout=10) - - -def test_a_persistently_unreadable_pid_is_still_recoverable_once_old_enough_1119( - tmp_path, -): - """#1119 fix direction: a fast-failing read must be refused like a - hung one, never treated as confirmed-empty -- but #1103's own - recovery must still work for a pid file that persistently cannot be - read: once the lock directory is at least as old as - `_LOCK_MAX_HOLD_SECONDS`, it is still reclaimable, the same recovery - path a permanently-hanging read already has - (test_a_permanently_dataless_pid_file_becomes_acquirable_once_old_enough). - Without this, refusing a fast-fail read forever would trade a security - regression for a permanent, unrecoverable outage.""" - reason = _chmod_unreadable_skip_reason() - if reason: - pytest.skip(reason) - lock_dir = tmp_path / "fast-fail-recoverable.lock" - lock_dir.mkdir() - pid_file = lock_dir / "pid" - pid_file.write_text("424242", encoding="utf-8") - pid_file.chmod(0o000) - try: - result = _run(f""" - export REMEMBER_LOCK_MAX_HOLD_SECONDS=0 - source {LIB_LOCK_SH} - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - """) - assert "ACQUIRED" in result.stdout, ( - f"a persistently unreadable pid file old enough to exceed " - f"_LOCK_MAX_HOLD_SECONDS must still be recoverable -- a fast " - f"read failure must not be refused FOREVER either: " - f"{result.stdout!r} {result.stderr!r}" - ) - finally: - pid_file.chmod(0o644) - - -def test_a_fast_failing_pid_read_via_the_mktemp_fallback_also_refuses_1119( - tmp_path, -): - """Self-review follow-up to #1119: `_lock_read_pid_timeout`'s own - mktemp-failure fallback (taken when `mktemp` itself cannot create a - scratch file -- a full or unwritable TMPDIR) had the EXACT SAME bug the - main fix just closed, in its own separate branch: it unconditionally - `return 0`-ed regardless of whether the direct `cat` it falls back to - actually succeeded, discarding `cat`'s own exit status via `|| true`. - A fast-failing pid file reached through this corner (TMPDIR unusable - AND the pid file itself unreadable) would still read as confirmed- - empty and be stealable from a live holder after only - `_LOCK_ADOPT_AFTER` (30s) rather than `_LOCK_MAX_HOLD_SECONDS` (1800s). - Forces the fallback the same way - test_release_of_an_owned_lock_still_works_when_mktemp_is_unusable - already does (TMPDIR pointed at a 0o000 directory), combined with the - same chmod-000 pid file the main #1119 test uses.""" - reason = _chmod_unreadable_skip_reason() - if reason: - pytest.skip(reason) - lock_dir = tmp_path / "mktemp-unusable-fast-fail.lock" - unusable_tmpdir = tmp_path / "no-mktemp-here-1119" - unusable_tmpdir.mkdir(mode=0o000) - lock_dir.mkdir() - holder = subprocess.Popen(["sleep", "30"]) - pid_file = lock_dir / "pid" - try: - pid_file.write_text(str(holder.pid), encoding="utf-8") - pid_file.chmod(0o000) - old_time = time.time() - 60 - os.utime(lock_dir, (old_time, old_time)) - - result = _run(f""" - export TMPDIR="{unusable_tmpdir}" - source {LIB_LOCK_SH} - _LOCK_ADOPT_AFTER=30 - _LOCK_MAX_HOLD_SECONDS=1800 - lock_acquire "{lock_dir}" 0 && echo ACQUIRED || echo REFUSED - """) - assert "REFUSED" in result.stdout, ( - f"a fast-failing pid read reached through the mktemp-failure " - f"fallback must never be treated as a confirmed empty pid file " - f"either -- a live holder's lock was stolen out from under it: " - f"{result.stdout!r} {result.stderr!r}" - ) - finally: - pid_file.chmod(0o644) - unusable_tmpdir.chmod(0o755) - holder.terminate() - holder.wait(timeout=10) - - -def test_a_fast_failing_claim_reread_does_not_steal_a_racing_winners_pid_1122( - tmp_path, -): - """#1122, follow-on to #1119/#1120 landing on a DIFFERENT call site in - the same function. `_lock_try_steal`'s SECOND read -- of `$_claim`, the - pid file this contender just renamed away from "${_dir}/pid" -- is - supposed to tell apart "still the same stuck placeholder I saw on my - first read" (safe to steal) from "a faster contender's own noclobber - write already recreated \"${_dir}/pid\" in the gap between my dir-age - check and my own `mv`, and my `mv` grabbed THAT instead" (unsafe -- - hand back, per the existing comment right above this re-read). - #1119/#1120 made every read failure at this re-read report identically - whether it is a real timeout or a fast EACCES/EIO/cloud-sync refusal, - so a fast-failing re-read of a genuine winner's brand-new pid -- which - can itself happen to be unreadable for a moment, unrelated to whether - it is actually stuck -- fell through to the noclobber write below and - clobbered the winner's lock: two simultaneous holders. - - The real race (#1101/#1103's own repro: N=8 concurrent contenders, 7 of - 8 "won") depends on OS scheduling luck too fine-grained to pin - deterministically in a single-process test. This reproduces the exact - mechanism instead of the timing by wrapping `_lock_file_inode`, the - fix's own helper: its FIRST call is the "before" snapshot the fix takes - right before the `mv` (of whatever is CURRENTLY at "${_dir}/pid", still - the original stuck placeholder at that point) -- the wrapper records - the real answer for that call, THEN swaps in a "winner's" pid file as - a side effect, simulating a faster contender's own win landing in - exactly the window between this contender's pre-mv snapshot and - the `mv` that follows it. The `mv` therefore grabs the SWAPPED file, - not the one the snapshot was taken of -- the real mismatch the fix - exists to catch. The swapped-in file is ALSO chmod 000, landing on - this issue's fast-fail shape rather than #1119's hang shape. - """ - reason = _chmod_unreadable_skip_reason() - if reason: - pytest.skip(reason) - lock_dir = tmp_path / "steal-race.lock" - lock_dir.mkdir() - pid_file = lock_dir / "pid" - pid_file.write_text("111111", encoding="utf-8") - pid_file.chmod(0o000) - winner_pid_text = "222222" - try: - result = _run( - f""" - export REMEMBER_LOCK_MAX_HOLD_SECONDS=0 - export REMEMBER_LOCK_PID_READ_TIMEOUT=1 - source {LIB_LOCK_SH} - eval "$(declare -f _lock_file_inode | sed '1s/^_lock_file_inode ()/_orig_lock_file_inode ()/')" - # A plain shell variable will NOT do for the once-only guard - # below: `_lock_file_inode` is always called via command - # substitution ($(...)) at its real call sites in lib-lock.sh, - # which forks a subshell, and a variable set inside that - # subshell is gone the instant the subshell exits -- so - # `_swap_done=1` would never be visible to the next call and - # the swap would fire on EVERY call, not just the first. A - # file on disk survives across subshells; a variable does not. - _swap_marker="{lock_dir}/.swap-done" - rm -f "$_swap_marker" - _lock_file_inode() {{ - local _result - _result=$(_orig_lock_file_inode "$@") - if [ ! -e "$_swap_marker" ]; then - touch "$_swap_marker" - # Simulate a faster contender's own win landing in the - # gap between THIS contender's pre-mv snapshot (just - # taken, above, of the file as it stood a moment ago) - # and the `mv` that is about to run next: a legitimate - # winner's pid, which ALSO happens to fail fast to read - # right now -- #1122's own shape, not #1119's hang. - # - # Built via mktemp + mv, never rm-then-create in place: - # on a bitmap allocator (ext4's behaviour, observed on - # the ubuntu-24.04/py3.9 CI leg -- this exact test went - # STOLE there despite being green on this machine's - # APFS, 2000/2000 rm+recreate trials showing zero - # collisions locally), freeing an inode and immediately - # creating a new file in the same directory can hand - # back that SAME inode number, with nothing else - # allocated in between to prevent it. That collision - # makes the real fix's own inode comparison below see - # "unchanged" for a file that was, in fact, swapped -- - # the exact ambiguity this test exists to force, except - # forced onto the test's own harness instead of the - # code under test. Creating the replacement under a - # temp name WHILE the original "pid" file still exists - # (not yet unlinked) guarantees the temp file gets an - # inode distinct from the original's -- two live files - # cannot share one inode -- and `mv` then carries that - # already-distinct inode into place atomically, so the - # swap is observably different by construction, not by - # allocator luck. - _winner_tmp=$(mktemp "{lock_dir}/.winner-pid.XXXXXX") - printf '%s' "{winner_pid_text}" > "$_winner_tmp" - chmod 000 "$_winner_tmp" - mv -f "$_winner_tmp" "{lock_dir}/pid" - fi - echo "$_result" - }} - if _lock_try_steal "{lock_dir}"; then - echo STOLE - else - echo REFUSED - fi - """, - timeout=15, - ) - assert "REFUSED" in result.stdout, ( - f"a fast-failing re-read of the claim must not be read as " - f"confirmed-still-stuck when a faster contender's own pid " - f"(itself also unreadable right now) was swapped in first -- " - f"stealing here clobbers a real winner's lock, producing two " - f"holders: {result.stdout!r} {result.stderr!r}" - ) - assert pid_file.exists(), ( - f"the hand-back must restore A pid file, not leave the lock " - f"pid-less: {result.stdout!r} {result.stderr!r}" - ) - finally: - if pid_file.exists(): - pid_file.chmod(0o644) - assert pid_file.read_text(encoding="utf-8") == winner_pid_text, ( - f"the winner's own pid must survive untouched, not be overwritten " - f"by the losing contender: got " - f"{pid_file.read_text(encoding='utf-8')!r}, expected " - f"{winner_pid_text!r}: {result.stdout!r} {result.stderr!r}" - ) - - -def test_a_persistently_unreadable_claim_with_no_race_still_steals_1122( - tmp_path, -): - """Positive control for the test above, same harness and bounds: with - no faster contender ever touching the pid file (no `_lock_dir_age` - override), the re-read of `$_claim` fails the same way the first read - did -- genuinely, consistently stuck -- and the steal must still - succeed. Without this, the fix above could satisfy its own assertion - by refusing every steal unconditionally, which would trade #1122's bug - for #1119's own already-rejected failure mode (a fast-failing pid file - becoming permanently unstealable) -- the same control shape as - test_a_readable_pid_of_a_live_holder_is_the_positive_control_1119, - proving this harness can show STOLE at all, not just REFUSED - regardless of what happened.""" - reason = _chmod_unreadable_skip_reason() - if reason: - pytest.skip(reason) - lock_dir = tmp_path / "steal-no-race.lock" - lock_dir.mkdir() - pid_file = lock_dir / "pid" - pid_file.write_text("111111", encoding="utf-8") - pid_file.chmod(0o000) - try: - result = _run( - f""" - export REMEMBER_LOCK_MAX_HOLD_SECONDS=0 - export REMEMBER_LOCK_PID_READ_TIMEOUT=1 - source {LIB_LOCK_SH} - if _lock_try_steal "{lock_dir}"; then - echo STOLE - else - echo REFUSED - fi - """, - timeout=15, - ) - assert "STOLE" in result.stdout, ( - f"a persistently unreadable pid file with no race must still " - f"be stealable once old enough -- refusing here would trade " - f"#1122's bug for the permanent-outage mistake #1119 already " - f"avoided at the first read: {result.stdout!r} {result.stderr!r}" - ) - finally: - pid_file.chmod(0o644) - new_pid = pid_file.read_text(encoding="utf-8").strip() - assert new_pid.isdigit(), ( - f"after a real steal, the pid file must hold this contender's own " - f"numeric pid, not be left empty or non-numeric: {new_pid!r}" - ) diff --git a/trap.d/1101-1103.pre-existing-flake-test_lock_timing.md b/trap.d/1101-1103.pre-existing-flake-test_lock_timing.md deleted file mode 100644 index 4e65e264..00000000 --- a/trap.d/1101-1103.pre-existing-flake-test_lock_timing.md +++ /dev/null @@ -1,35 +0,0 @@ -While chasing a CI-reported fork-count flake caused by my own watchdog redesign in -scripts/lib-lock.sh (#1101/#1103 follow-up), I found that -tests/test_lock_timing.py::test_with_the_recorder_off_nothing_is_written_and_nothing_extra_is_spawned -is ALSO flaky against the completely unmodified, pre-lane version of scripts/lib-lock.sh -(origin/fix/1101-1103 before any of this lane's commits) -- 1 failure in 15 runs (~7%), -confirmed by temporarily swapping in that exact pre-lane file content, running the test -repeatedly, and restoring my own working-tree edit afterward, verified byte-for-byte restored. - -I had initially assumed I introduced this flake: my OWN watchdog subshell polls with a -`while kill -0 ...; do sleep; done` loop whose very first iteration's outcome depends on -which of two processes (the watchdog itself, or the main thread's own `wait`) gets scheduled -first -- a genuine source of non-determinism I introduced, confirmed by reproducing a 2/15 -and later 4/20 failure rate against my own committed code for the exact same test. I -restructured the loop to always sleep once unconditionally before the first check (removing -that specific race), which is a real, independently-justified improvement (deterministic -fork count on the fast path) -- but it did NOT bring the flake rate to zero, because the -test was already flaky before my change for a DIFFERENT, uninvestigated reason. - -What I did not determine: the actual root cause of the baseline ~7% flake in that test. -Plausible candidates I did not chase further (time-boxed): the timing recorder's own -clock-resolution probing forking a variable number of times depending on bash feature -detection timing; the test's harness script's two lock_acquire/lock_release round trips -(one fast, one after a 2.5s hold) interacting with the shim-resolution helper's own forking -in a way that is itself timing-sensitive; or generic OS scheduler jitter affecting ANY -fork-count comparison test run back-to-back under load (this session had several other -agents/watchers running concurrently throughout, per a process listing). - -Curator: this reads to me as a real, if narrow (~7%), pre-existing flake in this test, -independent of the #1101/#1103 lane's own changes, worth either (a) filing as its own issue -on the tracker (repro: run the single test in a loop via pytest -q --no-cov, ~1-in-14 runs -red) with the finding above pasted in, or (b) deciding it is below the bar for action given -the rate and not something this lane should hold up a PR to chase. I did not file it myself -since filing is outside this phase's job and I was not confident enough in which of the -plausible causes above is the real one to write a reliable repro-with-mechanism bug report -rather than just a repro. diff --git a/trap.d/1122.unbounded-stat-in-steal-path.md b/trap.d/1122.unbounded-stat-in-steal-path.md deleted file mode 100644 index 71baa373..00000000 --- a/trap.d/1122.unbounded-stat-in-steal-path.md +++ /dev/null @@ -1,36 +0,0 @@ -#1122's fix added a new helper, `_lock_file_inode()` (`scripts/lib-lock.sh`), -called twice per `_lock_try_steal` invocation (once before the rename, once -after a failed claim re-read) to compare inodes rather than content. Neither -call is wrapped in a timeout/watchdog the way `_lock_read_pid_timeout`'s own -`cat` is (SIGTERM/SIGKILL escalation, built because #1101/#1103 found a plain -blocking read in this exact code path can hang forever on a dataless/ -cloud-sync placeholder or a wedged mount). - -Self-review's Explore reviewer flagged this (observed, not reasoned-away): -`_lock_file_inode`'s own comment candidly calls the no-timeout choice -"reasoned, not reproduced" -- a metadata-only `stat()` syscall is believed -not to block the way an actual content `read()` can, but this is not -verified against a real wedged mount. It mirrors an already-shipped, -already-accepted pattern (`_lock_dir_age`, same file, same unguarded `stat` -shape, no watchdog either), so it is not a new risk *class* -- but it does -raise the number of unguarded, potentially-blocking filesystem calls inside -`_lock_try_steal` from one (`_lock_dir_age`) to three, in the one function -whose entire purpose is escaping a hung/stuck holder. On a filesystem where -`stat()` itself can block (a hard-mounted NFS server gone away, certain FUSE -mounts), this reopens -- in a new place, inside the steal path meant to -escape hangs -- the class of unbounded-blocking risk the watchdog elsewhere -in this file exists to close off, with no equivalent escape hatch for these -two new calls or for the pre-existing `_lock_dir_age` call either. - -Not fixed in #1122 itself: wrapping `stat` in a watchdog would need the same -fork/background/SIGTERM-then-SIGKILL machinery `_lock_read_pid_timeout` -already carries, built fresh for a syscall this file's own existing code -(`_lock_dir_age`) already runs unguarded -- a materially larger change than -the inode-comparison fix #1122 asked for, and not clearly warranted without -a reproduced hang to point at (none exists for `stat()` specifically; only -for `cat`/`read()`, which is what the existing watchdog already covers). - -Whether this is worth a `_lock_stat_timeout()` primitive generalizing -`_lock_read_pid_timeout`'s watchdog to any command, covering `_lock_dir_age` -and `_lock_file_inode` both in one pass, is a question for whoever curates -this fragment -- not decided here.