Repository navigation
Conversation
…d (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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
… 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/<project-name>` (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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1127
Why
Since v0.45.0,
scripts/lib-lock.shgained an age-gated takeover of a lock whose pid file can't be read (#1101/#1103 via #1106), patched by #1120 and #1123. Five review rounds each found a new way to end up with two simultaneous holders of the same lock (#1119, #1122, #1124/#1125, and the reviews on still-unmerged #1126). The maintainer decided (2026-11-07) to ship a safe lock now and redesign the takeover separately.What changed
_lock_try_steal: when the pid read fails or times out, refuse at any age, as v0.45.0 did. The now-dead_lock_file_inodehelper and its locals are removed; comments say what the code does and why.Removed: automatic takeover of a lock whose pid file cannot be read.
Kept: the bounded 2 s pid read and
REMEMBER_LOCK_PID_READ_TIMEOUTvalidation (non-integer, 0, leading zero); reclaim of a dead holder's empty or non-numeric pid file that reads fine; the Ubuntu 26.04 noclobber guards (#1108); the release-path fixes; the zero-stderr fix; fast-path performance and fork counts; the steal from a holder that is alive but pastREMEMBER_LOCK_MAX_HOLD_SECONDS, which needs a successful pid read and resolves its race with release through the same rename-then-reread used for dead holders (reasoned by hand, not observed).Tests
Takeover tests flipped to assert REFUSED (red on 2fd0547: 5 failed, 2 passed; green after). The concurrent test now asserts zero winners. Locally: lock suites, fork-count, timing, arith lint and pep604 floor: 146 passed, 1 skipped (a chmod-unreadable skip).
bash -nclean; touched tests parse under Python 3.9 grammar. Full matrix is CI's job.Changelog
changelog.d/1127.removed.mdexplains in plain words that such a lock now has to be cleared by deleting the stale.lockfolder in.remember/once no save is running. The pending #1119 and #1122 fragments no longer promise the removed recovery.[AI-generated]