diff --git a/changelog.d/1119.fixed.md b/changelog.d/1119.fixed.md index e3e9ad60..ef801994 100644 --- a/changelog.d/1119.fixed.md +++ b/changelog.d/1119.fixed.md @@ -8,9 +8,9 @@ `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`. + completed: refused on its own. (The age-gated takeover this originally + fell back to for a persistently unreadable pid file was itself removed + in the same release -- see #1127.) - 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 diff --git a/changelog.d/1122.fixed.md b/changelog.d/1122.fixed.md index f3962df5..c66badea 100644 --- a/changelog.d/1122.fixed.md +++ b/changelog.d/1122.fixed.md @@ -10,10 +10,9 @@ 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). + patch immediately as a regression. The re-read compared the renamed + file's inode against what was captured just before the rename, and + `stat` needs no read permission on the file's content to do that, unlike + the re-read itself (#1122). (This whole takeover path -- the one this + inode check protected -- was removed in the same release after five + review rounds kept finding new two-holder races in it; see #1127.) diff --git a/changelog.d/1127.removed.md b/changelog.d/1127.removed.md new file mode 100644 index 00000000..546d4e06 --- /dev/null +++ b/changelog.d/1127.removed.md @@ -0,0 +1,74 @@ +- Removed: automatic takeover of a save/consolidation/staging lock whose + pid file cannot be read (#1127) -- the dataless cloud-sync placeholder case + (OneDrive, iCloud: the remote copy of a synced file was deleted after + the placeholder itself synced). Since v0.45.1 (#1101, #1103), a lock + left in that state was eventually taken over automatically once it had + sat untouched long enough. Five review rounds in a row (#1119, #1122, + #1124, #1125, and the reviews on pull request #1126) each found a new + way for two processes to end up believing they both held the same + lock through that takeover, however it was tightened in response to + the previous finding. Rather than continue patching an unsafe design, + this takeover is switched off: a pid file that cannot be read, at any + age, is refused rather than stolen or adopted, the same as before + v0.45.1. Two further review rounds on this fix's own pull request + (#1129) found three more races in the behaviour this change keeps -- + closed in the same set of commits: an EMPTY pid file (not merely + unreadable) is now also never grounds to steal, at any age, because + every legitimate winner's own pid write passes through a briefly-empty + state that an unrelated dead holder's permanently-empty pid file is + indistinguishable from; releasing a lock now refuses whenever its own + read of the pid file comes back empty, rather than treating an empty + read as "ownership unproven, but proceed anyway"; and every place + this file restores a claim it took back (an abandoned-claim cleanup, + a failed steal's hand-back, a confirmed-stealer's pid put-back during + release) now does so with a hard link instead of a plain rename, so + it can never silently overwrite a pid a faster process wrote into the + same spot in the meantime. + How you would notice this happening: a save, a consolidation round, + or an append to today's running notes quietly stops completing -- + nothing crashes and nothing shows an error unless debug logging is + on, so the way most people notice is that nothing new is being + remembered for a while. + + Before touching anything, make sure it is actually safe to clear a + lock: closing the Claude window is not enough on its own, because + saving and consolidating can both keep running in the background for + a short while after you close it (consolidation in particular can + take up to three minutes). Close Claude, then wait at least five + minutes before clearing anything. If you want to double-check rather + than just wait: on macOS or Linux, open a terminal and run + `ps aux | grep -i save-session` and `ps aux | grep -i run-consolidation` + -- if either prints a matching line (other than the `grep` command + itself), one is still running and you should wait longer. On Windows, + open Task Manager, go to "Details", and look for a process whose + command line mentions `save-session` or `run-consolidation` (the + "Command line" column may need to be added via "Select columns"). + + Finding the right folder to clear takes one more step, because where + memory lives is not always the same folder your project is in -- it + can be redirected elsewhere, and some project setups store memory + somewhere other than the folder you have open. The reliable way to + find it: run the `/remember:doctor` command -- it prints a line + starting with `REMEMBER_DIR` naming the exact folder. (The same + folder also shows up as `REMEMBER_DIR=...` in this plugin's own + debug log, if you have that turned on.) Inside that folder is a + `tmp` subfolder, and inside `tmp` are the three lock folders this can + affect: `save.lock`, `consolidation.lock` and `staging.lock`. Once + you have confirmed nothing is still running, delete whichever of + those three folders exists (just that one small lock folder, not + anything else in the memory folder) and the next save or + consolidation will work normally again. + Everything else from v0.45.1 is unchanged: the bounded (2-second) pid + read, reclaiming a dead holder's pid file that reads back as garbage + (non-numeric, but not empty), the Ubuntu 26.04 `noclobber` guard, and + the age-gated takeover of a holder that is still alive but has held + the lock past `REMEMBER_LOCK_MAX_HOLD_SECONDS`. That last one still + requires a pid file that reads back successfully and non-empty, and + is kept because both sides of it (the steal and the ordinary + release) resolve their race through the same atomic-rename mechanism + already proven for the confirmed-dead-holder case, now with the + empty-read ambiguity above closed on both sides too. +- Compatibility: compatible - a lock that used to recover on its own + after a long wait now instead needs a manual delete of the lock + folder; no command, config key or file format changes, and no + caller of `lock_acquire`/`lock_release` changes its own interface. diff --git a/scripts/lib-lock.sh b/scripts/lib-lock.sh index 9fa56d11..7ea56836 100644 --- a/scripts/lib-lock.sh +++ b/scripts/lib-lock.sh @@ -92,19 +92,19 @@ # 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. +# whole hook's own timeout budget, but a read that does not complete +# in time is NEVER grounds to reclaim, at any age (#1127): v0.45.1 +# (#1101/#1103) once let a later acquirer take such a lock over once +# the directory itself sat past `_LOCK_MAX_HOLD_SECONDS`, and five +# review rounds in a row (#1119, #1122, #1124/#1125, the reviews on +# the still-unmerged PR #1126) each found a new two-simultaneous- +# holders interleaving in that takeover, however it was tightened. +# Refusing unconditionally trades that outage for a lock that has to +# be cleared by hand once its pid file genuinely cannot be read -- +# the same thing v0.45.0 already did here, before #1101/#1103 ever +# touched this branch. The bound on the read itself is kept +# regardless: it still stops one stuck read from consuming a whole +# hook's own timeout budget, it just no longer licenses a takeover. [ -n "${_REMEMBER_LIB_LOCK_SOURCED:-}" ] && return 0 _REMEMBER_LIB_LOCK_SOURCED=1 @@ -186,13 +186,15 @@ _LOCK_PID_READ_TIMEOUT="${REMEMBER_LOCK_PID_READ_TIMEOUT:-2}" # 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 +# was about to finish normally. The SAME bound of zero also reaches the +# post-rename re-read every steal path shares (below): a legitimate +# fresh pid write losing that race to its own zero-length grace period +# reports a false timeout there too -- but every caller of that re-read +# now treats ANY timeout as "refuse, restore, hand back" uniformly +# (#1127, #1129), never as grounds to proceed with a steal, so a false +# timeout here costs the winner an unnecessary refusal of its own claim, +# not a second process believing it holds 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). @@ -423,13 +425,11 @@ _lock_read_pid_timeout() { # 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. + # has at every caller: refuse, unconditionally, at any age (#1127, + # #1129) -- the age-gated takeover this once fell back to for a + # read failure was itself removed, since a timeout or a fast-fail + # read is exactly as uninformative about whether a holder is alive + # as the other, and neither is proof enough to steal or adopt. if [ "$_rc" -ne 0 ]; then rm -f "$_tmp" 2>/dev/null || true return 1 @@ -446,10 +446,38 @@ _lock_read_pid_timeout() { return 0 } +# Restore (or discard) a claim file at without ever silently +# clobbering a pid some other process already wrote there in the +# meantime (#1127 review finding 3, PR #1129). A plain `mv "$_claim" +# "$_dest"` always succeeds and always OVERWRITES an existing +# destination with no error -- so a blind restore can silently replace +# a legitimate pid a faster acquirer just wrote in the meantime. +_lock_restore_claim() { + local _claim="$1" _dest="$2" + if ln "$_claim" "$_dest" 2>/dev/null; then + rm -f "$_claim" 2>/dev/null || true + return 0 + fi + if [ -e "$_dest" ]; then + # Something is genuinely there now -- a race, not a tool failure. + # Its content won; ours did not. This claim's owner is THIS + # process, alive by construction, so nothing else in this file + # will ever sweep it up on its own -- drop it here. + rm -f "$_claim" 2>/dev/null || true + return 1 + fi + # is still absent, so `ln` itself did not work: no hard-link + # support on this filesystem (FAT/exFAT and similar), not a race. + # Never fall back to `mv` -- that would reopen exactly the clobber + # this function exists to close. Leave the claim in place and + # refuse instead. + return 1 +} + # 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. @@ -470,143 +498,97 @@ _lock_try_steal() { _owner="${_abandoned##*.}" [[ "$_owner" == *[!0-9]* ]] && continue kill -0 "$_owner" 2>/dev/null && continue - # First dead claim becomes the pid again; any further ones are - # litter from earlier abandoned takeovers — drop them rather than - # leave them accumulating in the lock directory forever. - if [ ! -e "${_dir}/pid" ]; then - mv "$_abandoned" "${_dir}/pid" 2>/dev/null || true - else - rm -f "$_abandoned" 2>/dev/null || true - fi + # First dead claim restores the pid; any further ones are + # litter from earlier abandoned takeovers. #1127 review finding + # 3: the `[ ! -e ... ]` guard this replaced, right before a + # separate `mv`, left exactly the TOCTOU window its own name + # says -- a legitimate pid write landing between the check and + # the move was silently overwritten with no error. + # `_lock_restore_claim`'s `ln` makes the existence check and + # the restore one atomic step, and decides on its own whether + # this claim is debris (its own internal existence check on an + # `ln` failure already covers the "already restored by an + # earlier iteration of this same loop" case the old explicit + # else-branch handled). + _lock_restore_claim "$_abandoned" "${_dir}/pid" || true 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. + # #1127: a pid file whose read fails or times out is NEVER grounds + # to steal, at any age -- v0.45.0's own behaviour, restored. v0.45.1 + # (#1101/#1103) added an age-gated takeover for exactly this case, + # and five review rounds (#1119, #1122, #1124/#1125, plus the + # reviews on the still-unmerged PR #1126) each found a NEW two- + # simultaneous-holders interleaving in the tightened version that + # followed the previous fix -- the pattern never converged, it only + # ever moved. A single timeout or fast read failure cannot tell a + # permanently stuck placeholder (a dataless OneDrive/iCloud file + # whose remote copy is gone for good) from a live holder's pid file + # that is merely slow, or transiently unreadable, for reasons that + # have nothing to do with how long the lock has been held -- + # guessing wrong there is exactly what let a second process believe + # it held a lock a healthy holder still owned. # - # 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 + # Refusing here is a liveness cost, not a safety one, and it is + # accepted deliberately: a lock left behind by a holder whose pid + # file can no longer be read must now be cleared by hand (delete + # the lock directory) rather than being taken over automatically. + # The bounded read above is kept on its own merits regardless -- + # it still stops a single stuck read from consuming a whole hook's + # own timeout budget (#1103) -- it simply no longer licenses a + # takeover once it fails. return 1 fi - # 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 + # Confirmed empty, confirmed non-numeric, or a live holder -- the read + # genuinely completed, so each of these is a fact rather than a guess, + # unlike the timeout case above. Empty and non-numeric are NOT treated + # alike, on purpose (#1127 review finding 1, PR #1129): an EMPTY pid + # file can be a holder that created the directory microseconds ago and + # has not written its pid yet (a live acquisition in flight) -- or a + # faster contender's own noclobber-created-but-not-yet-written claim, + # passing through this exact same empty state as part of the ordinary + # steal/write sequence below. Age cannot tell those apart, and neither + # can content, because every legitimate winner's pid file is briefly + # EMPTY too: `( set -o noclobber; echo "$_LOCK_SELF" > "${_dir}/pid" )` + # creates the file before the shell's own write of "$_LOCK_SELF" lands + # in it. Two contenders racing a genuinely dead holder's empty pid file + # on an old-enough directory can each pass the age check, with one + # (X) winning the `mv` below, re-reading "" against its own "" and + # matching, then stalling in that exact create-but-not-yet-written + # window; a second (Y) then `mv`s X's brand-new empty claim, re-reads + # "" against ITS OWN "", also matches, and writes its own pid there + # successfully -- after which X's still-pending write lands in Y's + # claim through X's own already-open file descriptor. Both return 0. + # Reproduced by construction, not merely reasoned: nothing about this + # needs rare timing, since every winner's own write passes through the + # identical empty state every single time. An EMPTY pid file is + # therefore NEVER grounds to steal, at any age -- v0.45.0's own + # behaviour, restored for this one case; see `_lock_try_steal`'s + # unreadable-pid branch above for the same argument made once already + # about the read-failure case. + # + # Non-numeric (garbage, but non-empty) is kept age-gated, because the + # empty-vs-empty ambiguity above does not reproduce for it: no code + # in this file ever writes non-numeric content into a pid file -- every + # writer here (this function's own noclobber write, `_lock_try_adopt`'s) + # writes a real numeric pid, so a re-read of the renamed claim can only + # ever see the SAME garbage it saw before the rename (safe to steal), + # the empty intermediate state of some OTHER contender's write (`""`, + # which mismatches non-empty garbage and refuses), or that contender's + # own finished numeric pid (also mismatches and refuses) -- never a + # second copy of the SAME garbage string, which is the one case that + # could reopen this ambiguity and which nothing here can produce. + # `_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 garbage, or was dead-and-empty-forever before + # this fix, has no other reclaim path; the garbage case alone keeps one. + if [ -z "$_pid" ]; then + return 1 + elif [[ "$_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 @@ -646,12 +628,12 @@ _lock_try_steal() { # "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 + _lock_restore_claim "$_claim" "${_dir}/pid" || true return 1 fi _seen="$_LOCK_PID_READ_RESULT" if [ "$_seen" != "$_pid" ]; then - mv "$_claim" "${_dir}/pid" 2>/dev/null || true + _lock_restore_claim "$_claim" "${_dir}/pid" || true return 1 fi @@ -699,36 +681,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 @@ -1279,7 +1231,22 @@ _lock_release_impl() { fi _lock_self_set - if [ -n "$_pid" ] && [ "$_pid" != "$_LOCK_SELF" ]; then + # An EMPTY read is NOT "ownership unproven, proceed anyway" -- it used + # to skip this check entirely (`[ -n "$_pid" ]` false short-circuits + # the whole condition), which is review finding 2 on PR #1129: a live + # holder past _LOCK_MAX_HOLD_SECONDS, releasing legitimately, can read + # "${_dir}/pid" during the exact window a concurrent stealer's own + # noclobber write has created the path but not yet written into it + # (the same empty-create-before-write state finding 1, above, already + # names). That empty read used to sail through this check, then match + # "" against the stealer's own later-renamed claim at the `_seen != + # $_pid` compare below -- also "" at that same instant -- and the + # release proceeded to `rm -rf` the directory out from under the + # stealer's in-flight takeover. A third process's `mkdir` then + # recreates the path while the stealer still believes it holds the + # original: two holders. An empty read proves nothing about ownership + # either way and must refuse, the same as a mismatched one already did. + if [ -z "$_pid" ] || [ "$_pid" != "$_LOCK_SELF" ]; then return 1 fi @@ -1332,8 +1299,11 @@ _lock_release_impl() { # 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 + # $_pid. #1127 review finding 3: that restore used to be a bare + # `mv`, which can silently clobber a FOURTH process's own + # legitimate pid if it acquires in the gap between this capture + # and the restore -- `_lock_restore_claim` makes that gap atomic. + _lock_restore_claim "$_claim" "${_dir}/pid" || true return 1 fi # An ADOPTER creates a BRAND NEW "${_dir}/pid" AFTER this mv, from diff --git a/tests/test_lock_primitive.py b/tests/test_lock_primitive.py index db8bf2bc..cca022fd 100644 --- a/tests/test_lock_primitive.py +++ b/tests/test_lock_primitive.py @@ -587,24 +587,22 @@ def test_an_unreadable_pid_file_is_refused_quickly_rather_than_hanging( ) -def test_a_permanently_dataless_pid_file_becomes_acquirable_once_old_enough( +def test_a_permanently_dataless_pid_file_is_never_reclaimed_even_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. + """#1127: switched off. v0.45.1's own fix for this exact test pinned a + 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 took it over -- and five review rounds in a + row (#1119, #1122, #1124/#1125, the reviews on the still-unmerged PR + #1126) each found a NEW two-simultaneous-holders interleaving in that + takeover, however it was tightened. The maintainer's call: refuse + unconditionally, at any age, the same as v0.45.0 -- a lock left behind + by a holder whose pid file can no longer be read now has to be + cleared by hand (delete the lock directory) rather than taken over + automatically. `REMEMBER_LOCK_MAX_HOLD_SECONDS=0` makes any real age + qualify, so this proves the refusal holds even in the one case that + used to be the recovery path's whole reason to exist. """ lock_dir = tmp_path / "dataless-recoverable.lock" lock_dir.mkdir() @@ -618,43 +616,31 @@ def test_a_permanently_dataless_pid_file_becomes_acquirable_once_old_enough( """, 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: " + assert "REFUSED" in result.stdout, ( + f"a permanently dataless pid file must never be reclaimed " + f"automatically, however old the lock directory is -- a timeout " + f"is not proof the holder is dead: " 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}" + assert (lock_dir / "pid").exists(), ( + "a refused steal must leave the original (FIFO) pid file in " + "place, not replace or remove it" ) -def test_concurrent_takeover_of_a_permanently_dataless_pid_file_has_one_winner( +def test_concurrent_contenders_on_a_permanently_dataless_pid_file_never_steal( 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. - """ + """#1127: switched off. v0.45.1 pinned a single-winner guarantee here + (of N contenders racing the same permanently-stuck pid file, exactly + one won the takeover) -- but a winner existing at all is exactly what + five later review rounds in a row (#1119, #1122, #1124, #1125, and + the reviews on PR #1126) each found a new way to duplicate under + concurrency. The maintainer's call + removes the takeover outright rather than continuing to patch it: + with no takeover path left, every contender must refuse, every time, + a strictly stronger guarantee than "at most one winner" -- there is + never a winner to race over in the first place.""" lock_dir = tmp_path / "dataless-race.lock" lock_dir.mkdir() os.mkfifo(lock_dir / "pid") @@ -685,14 +671,65 @@ def test_concurrent_takeover_of_a_permanently_dataless_pid_file_has_one_winner( 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}" + assert len(wins) == 0, ( + f"a permanently dataless pid file must never be stolen by any " + f"contender, concurrent or not -- got {len(wins)} winner(s): " + f"{lines!r}" + ) + assert (lock_dir / "pid").exists(), ( + "every contender refusing must leave the original (FIFO) pid " + "file in place" + ) + + +def test_concurrent_contenders_on_a_dead_holders_pid_have_one_winner( + tmp_path, +): + """Positive control for the test above (#1129 review finding 8): same + harness, same N, but a READABLE pid naming a definitely-dead process + instead of a permanently-stuck FIFO. Exactly one of N concurrent + contenders must win -- the ordinary single-winner guarantee + `test_stale_takeover_has_exactly_one_winner` already pins for this + shape. Without this, the all-REFUSED assertion in the test above + could pass against a broken harness that never calls + `_lock_try_steal` at all, or one that always returns non-zero + regardless of input -- this proves the harness can show a winner + when a winner is correct, not just REFUSED either way.""" + lock_dir = tmp_path / "dead-holder-race.lock" + lock_dir.mkdir() + (lock_dir / "pid").write_text("111111", encoding="utf-8") + events_file = tmp_path / "events-control.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}" ) - 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}" + wins = [line for line in lines if line.startswith("STOLE")] + assert len(wins) == 1, ( + f"exactly one of {n} concurrent contenders racing a dead " + f"holder's readable pid must win -- a harness that cannot show " + f"a winner here proves nothing by also showing zero winners " + f"elsewhere: got {len(wins)}: {lines!r}" ) @@ -918,15 +955,27 @@ def test_a_reader_that_ignores_sigterm_is_still_bounded(tmp_path): ) -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.""" +def test_a_confirmed_empty_pid_file_is_never_reclaimed_even_old(tmp_path): + """#1129 review finding 1: a confirmed-empty pid file used to be + reclaimable once old enough (the ONLY path that ever reclaimed a pid + file left empty forever by a holder killed between `mkdir` and + writing its pid, since `_lock_try_adopt` explicitly refuses whenever + the pid file exists at all) -- but a confirmed-read "" is NOT + distinguishable from the empty state every winner's own pid file + passes through for a moment, between its `noclobber` create and the + write landing in it (see `_lock_try_steal`'s own comment on this + branch). Two contenders racing a genuinely dead holder's empty pid + file on an old-enough directory can each pass the age check, with + one winning the `mv` and re-reading "" against its own "" match, + stalling in that exact create-but-not-yet-written window, while a + second `mv`s the first one's brand-new empty claim, also matches "" + against its own "", and writes ITS pid there successfully -- after + which the first contender's still-pending write lands through its + own already-open descriptor into the second's claim. Both return 0. + An empty pid file must therefore never be reclaimed, at any age -- + the liveness cost (a holder that died mid-acquisition now needs the + lock directory cleared by hand) is accepted in exchange for never + producing two holders.""" lock_dir = tmp_path / "confirmed-empty.lock" lock_dir.mkdir() (lock_dir / "pid").write_text("", encoding="utf-8") @@ -938,9 +987,9 @@ def test_a_confirmed_empty_and_old_pid_file_is_still_reclaimed(tmp_path): _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}" + assert "REFUSED" in result.stdout, ( + f"a confirmed-empty pid file must never be reclaimed, however " + f"old the lock directory is: {result.stdout!r} {result.stderr!r}" ) @@ -1077,6 +1126,40 @@ def test_release_refuses_rather_than_removes_on_a_timed_out_pid_read(tmp_path): ) +def test_release_refuses_rather_than_removes_on_a_confirmed_empty_pid_read( + tmp_path, +): + """#1129 review finding 2: a release whose own pid read COMPLETES but + comes back empty used to skip the ownership check entirely -- + `[ -n "$_pid" ] && [ "$_pid" != "$_LOCK_SELF" ]` is false whenever + `$_pid` is "", so the whole condition short-circuits and the + function proceeds as though ownership were confirmed. An empty read + here is not a fact about ownership either way: it is exactly the + state a concurrent stealer's own in-flight `noclobber` write passes + through (created, not yet written), and finding 1 above shows it is + also exactly the state a long-dead holder's pid file can sit in + forever. A release that reads "" must refuse, the same as a release + that reads a mismatched pid already does -- never proceed as if + ownership were simply unproven-but-fine.""" + lock_dir = tmp_path / "confirmed-empty-release.lock" + lock_dir.mkdir() + (lock_dir / "pid").write_text("", encoding="utf-8") + + result = _run(f""" + source {LIB_LOCK_SH} + lock_release "{lock_dir}" && echo RELEASED || echo REFUSED + """) + assert "REFUSED" in result.stdout, ( + f"a release whose own pid read comes back confirmed-empty must " + f"refuse, not treat the empty read as proof of ownership: " + f"{result.stdout!r} {result.stderr!r}" + ) + assert lock_dir.exists(), ( + "a refused release must leave the lock directory in place, not " + "rm -rf it out from under whatever is actually using it" + ) + + 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 @@ -1981,18 +2064,18 @@ def test_a_readable_pid_of_a_live_holder_is_the_positive_control_1119(tmp_path): holder.wait(timeout=10) -def test_a_persistently_unreadable_pid_is_still_recoverable_once_old_enough_1119( +def test_a_persistently_unreadable_pid_is_never_reclaimed_even_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.""" + """#1127: switched off. #1119's own fix pinned a recovery path here -- + once the lock directory was at least as old as `_LOCK_MAX_HOLD_SECONDS`, + a persistently unreadable pid file was still reclaimable, the same + recovery path the dataless-FIFO case had + (test_a_permanently_dataless_pid_file_is_never_reclaimed_even_once_old_enough). + Both recovery paths are switched off together: refusing forever is now + the accepted liveness cost, in exchange for never duplicating a + holder -- the pattern five review rounds (#1119, #1122, #1124/#1125, + PR #1126) kept finding a new way to do.""" reason = _chmod_unreadable_skip_reason() if reason: pytest.skip(reason) @@ -2007,10 +2090,9 @@ def test_a_persistently_unreadable_pid_is_still_recoverable_once_old_enough_1119 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: " + assert "REFUSED" in result.stdout, ( + f"a persistently unreadable pid file must never be reclaimed " + f"automatically, however old the lock directory is: " f"{result.stdout!r} {result.stderr!r}" ) finally: @@ -2069,39 +2151,20 @@ def test_a_fast_failing_pid_read_via_the_mktemp_fallback_also_refuses_1119( 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. - """ +def test_a_persistently_unreadable_pid_file_is_never_stolen_1122(tmp_path): + """#1122/#1127 consolidated. #1122's own fix told apart "still the + same stuck placeholder" from "a faster contender's own write already + recreated the pid file" by re-reading the renamed claim and comparing + inodes -- a mechanism that only mattered because a successful takeover + was still reachable at all. #1127 removes the takeover outright (five + review rounds in a row -- #1119, #1122, #1124, #1125, and the reviews + on pull request #1126 -- each found a NEW two-simultaneous-holders + interleaving in it, however it was tightened), so the inode-comparison + machinery this test used to exercise no longer exists in + `_lock_try_steal` at all: a persistently unreadable pid file is + refused, unconditionally, with or without a racing contender. + REMEMBER_LOCK_MAX_HOLD_SECONDS=0 makes every real age qualify, the + strongest case the old takeover ever had to work with.""" reason = _chmod_unreadable_skip_reason() if reason: pytest.skip(reason) @@ -2110,64 +2173,12 @@ def test_a_fast_failing_claim_reread_does_not_steal_a_racing_winners_pid_1122( 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 @@ -2177,73 +2188,130 @@ def test_a_fast_failing_claim_reread_does_not_steal_a_racing_winners_pid_1122( 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}" + f"a persistently unreadable pid file must never be stolen, " + f"with or without a racing contender: " + f"{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}" + f"a refused steal must leave A pid file in place, not leave " + f"the lock 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}" + assert pid_file.read_text(encoding="utf-8") == "111111", ( + f"the original pid file must survive untouched: " + f"{pid_file.read_text(encoding='utf-8')!r}" ) -def test_a_persistently_unreadable_claim_with_no_race_still_steals_1122( +def test_a_readable_pid_of_a_dead_process_is_the_positive_control_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" + """Positive control for the test above (#1129 review finding 8): same + harness and bounds, but a READABLE pid file naming a definitely-dead + process must still be stolen. Without this, the REFUSED assertion + above could pass against a broken harness that never calls + `_lock_try_steal` at all, or one that always returns non-zero -- + this proves the harness can show STOLE when stealing is correct, + not just REFUSED regardless of what actually happened.""" + lock_dir = tmp_path / "dead-holder.lock" lock_dir.mkdir() pid_file = lock_dir / "pid" + # 111111 is not a real pid on any machine this suite runs on -- kill -0 + # on it always fails, the same "confirmed dead" shape + # test_stale_lock_from_a_dead_holder_is_taken_over already relies on. 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) + 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 readable pid naming a dead process must still be stolen -- " + f"a harness that cannot show this proves nothing by also " + f"showing REFUSED elsewhere: {result.stdout!r} {result.stderr!r}" + ) 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}" + assert new_pid.isdigit() and new_pid != "111111", ( + f"after a real steal, the pid file must hold the stealer's own " + f"numeric pid, not the dead holder's: {new_pid!r}" + ) + + +def test_lock_restore_claim_never_clobbers_an_existing_destination(tmp_path): + """#1127 review finding 3, PR #1129: every restore-by-rename site in + lib-lock.sh used to be a bare `mv "$_claim" "$_dest"`. `mv` to an + EXISTING destination always succeeds and always overwrites, + silently -- so a legitimate pid a faster process wrote into `$_dest` + in the window between whatever existence check preceded the mv and + the mv itself was clobbered with no error, and a later contender + could then steal the clobbered, stale content: two holders. + `_lock_restore_claim` closes this with `ln` (a hard link), which + POSIX requires to fail with EEXIST if the destination exists. + + This exercises the state a race leaves behind -- a destination that + already exists when the restore is attempted -- the same state + regardless of how many nanoseconds separated the two writes, so it + needs no concurrency to be deterministic.""" + lock_dir = tmp_path / "restore-race.lock" + lock_dir.mkdir() + dest = lock_dir / "pid" + claim = lock_dir / "pid.stealing.111111" + dest.write_text("900000", encoding="utf-8") + claim.write_text("333333", encoding="utf-8") + + # Positive control for the bug this replaces: a bare `mv`, the exact + # shape every restore site in lib-lock.sh used before this fix, DOES + # silently clobber here -- proving this scenario is a real + # regression test, not one that would pass regardless of mechanism. + _run(f'mv "{claim}" "{dest}" 2>/dev/null') + assert dest.read_text(encoding="utf-8") == "333333", ( + "sanity check failed: a bare mv must still clobber in this " + "harness, or the assertion below proves nothing" + ) + + # Re-create the same starting state for the real function under test. + dest.write_text("900000", encoding="utf-8") + claim.write_text("333333", encoding="utf-8") + result = _run(f""" + source {LIB_LOCK_SH} + _lock_restore_claim "{claim}" "{dest}" && echo RESTORED || echo DISCARDED + """) + assert "DISCARDED" in result.stdout, ( + f"a restore onto an existing destination must discard its own " + f"claim, never overwrite it: {result.stdout!r} {result.stderr!r}" + ) + assert dest.read_text(encoding="utf-8") == "900000", "winner must survive" + assert not claim.exists(), "a discarded claim must not linger as litter" + + +def test_lock_restore_claim_restores_when_nothing_is_in_the_way(tmp_path): + """Positive control for the test above: with no race (the destination + genuinely absent), the SAME function must still restore the claim -- + otherwise it could satisfy the no-clobber assertion by refusing + unconditionally, which would strand every abandoned claim forever.""" + lock_dir = tmp_path / "restore-no-race.lock" + lock_dir.mkdir() + dest = lock_dir / "pid" + claim = lock_dir / "pid.stealing.111111" + claim.write_text("333333", encoding="utf-8") + result = _run(f""" + source {LIB_LOCK_SH} + _lock_restore_claim "{claim}" "{dest}" && echo RESTORED || echo DISCARDED + """) + assert "RESTORED" in result.stdout, ( + f"a restore with nothing in the way must still succeed -- " + f"refusing unconditionally would strand every abandoned claim " + f"forever: {result.stdout!r} {result.stderr!r}" ) + assert dest.read_text(encoding="utf-8") == "333333", "restored content wrong" + assert not claim.exists(), "a successful restore must not leave the claim behind too" 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.