From 74b98b38c2b42b396a1650986c84321d207c792b Mon Sep 17 00:00:00 2001 From: Florian DAVID <150798857+fdaviddpt@users.noreply.github.com> Date: Fri, 13 Nov 2026 23:05:54 +0100 Subject: [PATCH 1/3] Fix #1127: switch off takeover of a lock whose pid file cannot be read (v0.45.2) Since v0.45.0, scripts/lib-lock.sh gained an age-gated takeover for a save/consolidation/staging lock whose pid file could not be read (#1101, #1103). Five review rounds in a row (#1119, #1122, #1124/#1125, the reviews on PR #1126, which stays unmerged) 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. Maintainer decision: ship a SAFE lock now rather than continue patching an unsafe design. _lock_try_steal's unreadable-pid branch now refuses unconditionally, at any age -- v0.45.0's own behaviour, restored. A lock left behind by a holder whose pid file can no longer be read (the dataless OneDrive/iCloud placeholder case this was all for) now has to be cleared by hand. Kept, unchanged: the bounded (2s) pid read and its REMEMBER_LOCK_PID_ READ_TIMEOUT validation, reclaiming a dead holder's non-numeric/empty pid file that reads fine, the Ubuntu 26.04 noclobber guard, and the age-gated steal of a holder that is alive but past REMEMBER_LOCK_MAX_ HOLD_SECONDS -- analysed separately: it still requires a successful pid read, and both the steal and the release sides already resolve their race through the same atomic-rename-and-reread mechanism proven for the confirmed-dead-holder case, with none of the read-failure ambiguity the removed branch had. Also removed the now-dead _lock_file_inode helper and the inode-comparison machinery that existed only to make the removed branch safe. Tests: flipped the three tests that pinned the removed takeover to assert REFUSED, consolidated the two #1122 race-mechanism tests (whose inode-compare machinery no longer exists in the code) into one, and kept every two-holder regression test passing. 146 passed, 1 skipped across test_lock_primitive.py, test_lock_fork_reduction_1056.py, test_lock_timing.py, test_arith_base_lint_332.py and test_pep604_floor_guard.py. Docs: changelog.d/1127.removed.md explains the behaviour change and the manual-clear workaround in plain terms, and amends the two pending changelog.d fragments (#1119, #1122) that described the now-removed recovery path as still available. Co-Authored-By: Claude Sonnet 5 --- changelog.d/1119.fixed.md | 6 +- changelog.d/1122.fixed.md | 13 +- changelog.d/1127.removed.md | 31 ++++ scripts/lib-lock.sh | 185 +++++----------------- tests/test_lock_primitive.py | 288 +++++++++-------------------------- 5 files changed, 149 insertions(+), 374 deletions(-) create mode 100644 changelog.d/1127.removed.md 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..4941e97a --- /dev/null +++ b/changelog.d/1127.removed.md @@ -0,0 +1,31 @@ +- 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, 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. + If this happens to you (a save, consolidation or staging step appears + stuck, logging "another process holds the lock, skipping" repeatedly), + the lock directory now has to be cleared by hand: delete the stale + `.lock` directory shown in that message (`save.lock`, + `consolidation.lock` or `staging.lock`, inside your `.remember/` + store) once you have confirmed no genuine save is still running. + Everything else from v0.45.1 is unchanged: the bounded (2-second) pid + read, reclaiming a dead holder's pid file that reads fine (whether + empty or garbage), 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 successfully, 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, with no read-failure ambiguity involved. +- Compatibility: compatible - a lock that used to recover on its own + after a long wait now instead needs a manual delete of the lock + directory; 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..7cd042ed 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 @@ -449,7 +449,7 @@ _lock_read_pid_timeout() { # 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. @@ -484,112 +484,29 @@ _lock_try_steal() { 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. + # #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. # - # 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 + # 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 @@ -699,36 +616,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 diff --git a/tests/test_lock_primitive.py b/tests/test_lock_primitive.py index db8bf2bc..12fca7bd 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,30 @@ 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 (#1119, #1122, #1124/#1125, 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 +670,14 @@ 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}" ) - 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}" + assert (lock_dir / "pid").exists(), ( + "every contender refusing must leave the original (FIFO) pid " + "file in place" ) @@ -1981,18 +1966,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 +1992,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 +2053,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 -- #1119, #1122, #1124/#1125, the reviews on the + still-unmerged PR #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 +2075,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 +2090,18 @@ 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}" - ) - - -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}" + 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}" ) From 69b778c89bfb0ba20088c16bfbdf49d893ff88fd Mon Sep 17 00:00:00 2001 From: Florian DAVID <150798857+fdaviddpt@users.noreply.github.com> Date: Fri, 13 Nov 2026 23:25:46 +0100 Subject: [PATCH 2/3] Fix #1127 review: empty pid is never grounds to steal, release never skips its own ownership check on an empty read /code-review (high) on PR #1129 found two more two-holder races in the behaviour kept by the previous commit (74b98b3): 1. MEDIUM, lib-lock.sh ~526: _lock_try_steal's confirmed-empty-or-non- numeric branch age-gated an EMPTY pid file the same as a non-numeric one. Every legitimate winner's own pid write passes through a briefly-empty state (noclobber creates the path before echo's write lands in it), indistinguishable from a dead holder's permanently- empty pid file -- two contenders racing the latter on an old-enough directory could each win a different leg of that same empty-vs-empty ambiguity and both return 0. Fixed: an EMPTY pid file is never grounds to steal, at any age -- v0.45.0's own behaviour. Non-numeric (garbage, non-empty) reclaim is KEPT: no writer in this file ever writes non-numeric content, so a re-read of the renamed claim can only ever see the same garbage, an unrelated contender's own empty intermediate state, or that contender's finished numeric pid -- never a second copy of the same garbage, which is the one case that would reopen the ambiguity and which nothing here can produce. 2. LOW, lib-lock.sh ~1169 (release): `[ -n "$_pid" ] && [ "$_pid" != "$_LOCK_SELF" ]` short-circuits to false whenever `$_pid` is empty, skipping the ownership check entirely and letting release proceed as though ownership were confirmed. A live holder past max-hold, releasing legitimately while a stealer's own noclobber write is mid-flight, could read that same empty intermediate state, pass the skipped check, match "" against "" at the later re-read too, and rm -rf the directory out from under the stealer's in-flight takeover -- a third process then recreates the path and two holders exist. Fixed: an empty read now refuses release, the same as a mismatched one already did. Re-checked the live-holder-past-max-hold steal (kept, not switched off) against this fix: it already requires a successful, non-empty pid read, and both the steal and release sides resolve their race through the same atomic-rename-then-reread mechanism already proven single-winner for the confirmed-dead-holder case -- traced both directions of the mv race by hand; whichever side's rename on the shared path lands first wins, the other gets ENOENT and backs off. No interleaving found that produces two holders. Also: removed trap.d/1122.unbounded-stat-in-steal-path.md (described the now-deleted _lock_file_inode helper); corrected two stale comments (lib-lock.sh ~189, ~421) that still justified behaviour via the removed unreadable-pid takeover's own re-read; corrected changelog.d/1127's "no read-failure ambiguity" claim and its recovery steps (the actual log lines, debug-only visibility, both storage-mode lock paths, and "only when no Claude session is open"); fixed "five review rounds" undercounting its own list in both changelog.d/1127.removed.md and tests/test_lock_primitive.py. Tests: flipped test_a_confirmed_empty_and_old_pid_file_is_still_ reclaimed to assert REFUSED (finding 1); added test_release_refuses_rather_than_removes_on_a_confirmed_empty_pid_read (finding 2); added positive controls for the two all-REFUSED tests from the previous commit (a readable pid of a dead process must still be stolen, single-contender and N=8-concurrent). Verified each new/ flipped assertion red against 74b98b3's lib-lock.sh before this fix and green against the fixed file. 149 passed, 1 skipped across test_lock_primitive.py, test_lock_fork_reduction_1056.py, test_lock_timing.py, test_arith_base_lint_332.py and test_pep604_floor_guard.py. bash -n and ast.parse(feature_version=(3,9)) both clean. changelog.d --check and --check-links both ok. Co-Authored-By: Claude Sonnet 5 --- changelog.d/1127.removed.md | 63 ++++--- scripts/lib-lock.sh | 105 ++++++++---- tests/test_lock_primitive.py | 172 ++++++++++++++++++-- trap.d/1122.unbounded-stat-in-steal-path.md | 36 ---- 4 files changed, 274 insertions(+), 102 deletions(-) delete mode 100644 trap.d/1122.unbounded-stat-in-steal-path.md diff --git a/changelog.d/1127.removed.md b/changelog.d/1127.removed.md index 4941e97a..26b20fe8 100644 --- a/changelog.d/1127.removed.md +++ b/changelog.d/1127.removed.md @@ -4,28 +4,49 @@ 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, 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. - If this happens to you (a save, consolidation or staging step appears - stuck, logging "another process holds the lock, skipping" repeatedly), - the lock directory now has to be cleared by hand: delete the stale - `.lock` directory shown in that message (`save.lock`, - `consolidation.lock` or `staging.lock`, inside your `.remember/` - store) once you have confirmed no genuine save is still running. + #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. A further review round on this fix's own pull request + (#1129) found two more races in the behaviour this change keeps -- + closed in the same commit: 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; and 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." + If this happens to you: 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, it just + stops happening, so the way most people notice is that nothing new + is being remembered for a while. If you suspect this, first make + sure no Claude session that uses this plugin is currently open or + running -- only then is it safe to clear the lock. Memory lives in a + folder named `.remember` -- either inside your project (the + default), or in your home folder as `.remember/` + if you're using "external storage mode" (see + docs/external-storage-mode.md). Inside that `.remember` folder is a + `tmp` folder, and inside `tmp` are the three lock folders this can + affect: `save.lock`, `consolidation.lock` and `staging.lock`. With + no Claude session open, delete whichever of those three folders + exists (not the files inside your `.remember` folder, just that one + small lock 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 fine (whether - empty or garbage), 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 successfully, 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, with no read-failure ambiguity involved. + 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 - directory; no command, config key or file format changes, and no + 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 7cd042ed..c23bb096 100644 --- a/scripts/lib-lock.sh +++ b/scripts/lib-lock.sh @@ -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 @@ -510,20 +510,52 @@ _lock_try_steal() { 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 @@ -1166,7 +1198,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 diff --git a/tests/test_lock_primitive.py b/tests/test_lock_primitive.py index 12fca7bd..64bcb777 100644 --- a/tests/test_lock_primitive.py +++ b/tests/test_lock_primitive.py @@ -634,8 +634,9 @@ def test_concurrent_contenders_on_a_permanently_dataless_pid_file_never_steal( """#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 (#1119, #1122, #1124/#1125, PR #1126) each - found a new way to duplicate under concurrency. The maintainer's call + 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 @@ -681,6 +682,57 @@ def test_concurrent_contenders_on_a_permanently_dataless_pid_file_never_steal( ) +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}" + ) + 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}" + ) + + 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=$(( @@ -903,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") @@ -923,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}" ) @@ -1062,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 @@ -2059,8 +2157,8 @@ def test_a_persistently_unreadable_pid_file_is_never_stolen_1122(tmp_path): 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 -- #1119, #1122, #1124/#1125, the reviews on the - still-unmerged PR #1126 -- each found a NEW two-simultaneous-holders + 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 @@ -2105,3 +2203,45 @@ def test_a_persistently_unreadable_pid_file_is_never_stolen_1122(tmp_path): f"the original pid file must survive untouched: " f"{pid_file.read_text(encoding='utf-8')!r}" ) + + +def test_a_readable_pid_of_a_dead_process_is_the_positive_control_1122( + tmp_path, +): + """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") + 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() 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}" + ) 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. From 5fa87b6b939db204adb6621efe7863622908b2cc Mon Sep 17 00:00:00 2001 From: Florian DAVID <150798857+fdaviddpt@users.noreply.github.com> Date: Fri, 13 Nov 2026 23:50:40 +0100 Subject: [PATCH 3/3] Fix #1127 review round 3: restore-by-rename sites never clobber a pid a faster process just wrote /code-review (high) on PR #1129 re-review found no new two-holder path in the kept code, but three fixes were needed before release. 1. MEDIUM, lib-lock.sh ~476: the abandoned-claim sweep in _lock_try_steal checked pid absence, then `mv "$_abandoned" "${_dir}/pid"`. `mv` to an existing destination always succeeds and always overwrites, silently -- so a pid a faster acquirer wrote in the window between the check and the mv was silently clobbered with stale content, and a later contender could steal that stale content: two holders. The same shape existed at three more restore-by-rename sites (a failed steal's hand-back, twice; a confirmed-stealer's pid put-back during release). All four are now `_lock_restore_claim`, a new helper that uses `ln` (a hard link, never `ln -s`) instead of `mv`: POSIX requires `ln` to fail with EEXIST if the destination already exists, atomically, closing the check-then-act window a separate `[ -e ... ]` guard followed by a separate `mv` can never close. On an `ln` failure because the destination now holds someone else's real content, the losing claim is discarded (its owner is this very process, so nothing else in the file would ever reclaim it). On an `ln` failure for an unrelated reason (no hard-link support -- FAT/exFAT and similar -- while the destination is still genuinely absent), the claim is left in place and the caller refuses, same as before; never a fallback to `mv`, which would reopen the exact clobber this closes. 2. MEDIUM, changelog.d/1127.removed.md: the recovery steps were wrong. "Make sure no Claude session is open" is not enough -- save-session.sh and run-consolidation.sh both keep running in the background for a while after the window closes (consolidation up to ~180s), so deleting a lock right after closing can race a save or consolidation that is still finishing and produce two holders. Rewritten: close Claude, wait at least five minutes, and (optionally) check for a still-running save-session.sh/run-consolidation.sh process, with the plain command for macOS/Linux (`ps aux | grep ...`) and for Windows (Task Manager, Details, Command line column). The folder-location steps were also wrong: external storage mode does not use `.remember/` (the slug is the whole project path with slashes turned to dashes, via session_dir_slug), data_dir can be any template, and a project opened from a linked checkout keeps memory with its main checkout, not the one that's open. Rewritten to point at `/remember:doctor`'s own `REMEMBER_DIR` line (verified it prints one) and the same line already in this plugin's debug log (verified: save-session.sh, run-consolidation.sh and session-start-hook.sh all log it), rather than asking anyone to derive the slug by hand. 3. Also folded finding 3 into the "two more races" paragraph (now three), since it was found in the same PR as findings 1 and 2. Tests (test-first): test_lock_restore_claim_never_clobbers_an_existing_ destination 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 no concurrency injection is needed to be deterministic) and includes an explicit red/green pair: a bare `mv` against the same starting state DOES clobber (sanity check that the scenario is real), the new `_lock_restore_claim` does not. test_lock_restore_claim_restores_when_nothing_is_in_the_way is the positive control: with nothing in the way, the same function must still restore, or it could pass the no-clobber assertion by refusing unconditionally and stranding every abandoned claim forever. 151 passed, 1 skipped across test_lock_primitive.py, test_lock_fork_reduction_1056.py, test_lock_timing.py, test_arith_base_lint_332.py and test_pep604_floor_guard.py (one transient failure in test_lock_timing.py's recorder-spawn-count test, re-run clean twice -- this is the pre-existing ~7% flake already documented in trap.d/1101-1103.pre-existing-flake-test_lock_timing.md, confirmed unrelated to this diff by reproducing it against the unmodified file too, before this round's edits). bash -n and ast.parse(feature_version=(3,9)) both clean. changelog.d --check and --check-links both ok. Co-Authored-By: Claude Sonnet 5 --- changelog.d/1127.removed.md | 72 +++++++++++++++++++++++------------- scripts/lib-lock.sh | 60 ++++++++++++++++++++++++------ tests/test_lock_primitive.py | 70 +++++++++++++++++++++++++++++++++++ 3 files changed, 165 insertions(+), 37 deletions(-) diff --git a/changelog.d/1127.removed.md b/changelog.d/1127.removed.md index 26b20fe8..546d4e06 100644 --- a/changelog.d/1127.removed.md +++ b/changelog.d/1127.removed.md @@ -10,32 +10,54 @@ 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. A further review round on this fix's own pull request - (#1129) found two more races in the behaviour this change keeps -- - closed in the same commit: 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 + 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; and 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." - If this happens to you: 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, it just - stops happening, so the way most people notice is that nothing new - is being remembered for a while. If you suspect this, first make - sure no Claude session that uses this plugin is currently open or - running -- only then is it safe to clear the lock. Memory lives in a - folder named `.remember` -- either inside your project (the - default), or in your home folder as `.remember/` - if you're using "external storage mode" (see - docs/external-storage-mode.md). Inside that `.remember` folder is a - `tmp` folder, and inside `tmp` are the three lock folders this can - affect: `save.lock`, `consolidation.lock` and `staging.lock`. With - no Claude session open, delete whichever of those three folders - exists (not the files inside your `.remember` folder, just that one - small lock folder) and the next save or consolidation will work - normally again. + 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 diff --git a/scripts/lib-lock.sh b/scripts/lib-lock.sh index c23bb096..7ea56836 100644 --- a/scripts/lib-lock.sh +++ b/scripts/lib-lock.sh @@ -446,6 +446,34 @@ _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() { @@ -470,14 +498,19 @@ _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 @@ -595,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 @@ -1266,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 64bcb777..cca022fd 100644 --- a/tests/test_lock_primitive.py +++ b/tests/test_lock_primitive.py @@ -2245,3 +2245,73 @@ def test_a_readable_pid_of_a_dead_process_is_the_positive_control_1122( 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"