fix(bin): bound lock stealing to one .steal level and refuse uncreatable locks - #2796
fix(bin): bound lock stealing to one .steal level and refuse uncreatable locks#2796benlamlih wants to merge 13 commits into
Conversation
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (2): Last reviewed commit: "no-mistakes(lint): suppress SC2016 on li..." | Re-trigger Greptile |
Reviewer note: which lock the race regression test drives, and why it matters
The double-publish window opens between the primary reclaim's staleness re-proof and its publish. This is worth stating because the first version of this fix was validated with a 40-racer hammer on a plain lock and passed, while the hole was real. The test creates the stalls by copying the two real functions with |
|
Speaking as Kun's firstmate: Corrective for #2787: VISION (per rule):
Security: none. Workflows approved this pass after a clean diff review (CI 32573464807, Require no-mistakes 32573464806). Class: corrective. Overlap: CI: no structured Ahead 7, behind 0, mergeable, mergeStateStatus UNSTABLE. Not merge-eligible. Waiting on the author for a green no-mistakes raise whose body attestation matches this HEAD ( |
…lock fm_lock_try_acquire treated a lock it could not create as a stale lock to recover, then serialized that recovery by calling itself on "<lock>.steal". For a steal mutex that meant "<lock>.steal.steal", and so on with no base case, so a filesystem that refused every create walked the name down a chain of .steal suffixes until the path was rejected. Reproduced on a full filesystem and on an unwritable lock parent: over 1400 suffixes, 6.3 MB of stderr, and bash itself killed by SIGSEGV before the chain could stop. A steal mutex is now a leaf: it reclaims its own stale holder directly and never appends a second .steal, for internal callers and for fm_autoarm_release_abandoned alike. The mutex is kept, because it is what stops two processes both publishing the primary lock; the reclaim stays exclusive because a live holder is never evicted, and two racers on a dead holder are separated by the ownership rechecks the primary reclaim already performs before it touches the lock. Creation failure is also now separated from contention. A create that leaves no lock behind means the filesystem refused, not that a holder exists, so acquisition refuses there instead of entering recovery with nothing to recover. FM_LOCK_FAIL_REASON reports which of the two happened, and the refusal explains itself once to the operator rather than being silent until the path grows too long to print. fm_lock_acquire_wait keeps blocking rather than returning, so no caller can walk into its critical section believing it holds a lock it never got. Legacy .steal chains already on disk are swept when stale and left alone when a holder is still live, so a rolling update cannot evict one.
BSD wc -l pads its count with leading blanks, so comparing an unstripped "$(wc -l < ...)" against a bare 1 can never match on macOS and the three one-line report assertions failed there while passing on GNU coreutils in CI. One count in the same file already stripped; these three did not.
… steal-bound coverage
…e locks in three consumers
…ntion, cover last two consumers
…uisition regressions
14442f0 to
49011bd
Compare
Intent
Fix the lock steal-mutex recursion in bin/fm-wake-lib.sh that builds a filename of endlessly repeated .steal suffixes until the filesystem refuses it.
This is a real observed failure, not a theoretical one. The captain hit it on his own machine and reported two symptoms together: "...steal.steal.steal.steal.steal...: File name too long" and "bin/fm-wake-lib.sh: line 357: [: : integer expression expected". It is filed upstream as issue #2787, "Lock acquisition recurses through .steal suffixes when initial lock creation fails".
Where it comes from: fm_lock_try_acquire, when it decides the current owner is stale, serializes the reclaim through a steal mutex by calling itself with "$lockdir.steal". If the steal lock itself looks stale, that call computes "$lockdir.steal.steal" and recurses again. There was no depth bound and no base case, so under the right conditions it walks down a chain of .steal suffixes until the path exceeds the filesystem limit.
The second symptom, the integer-expression error, was ALREADY FIXED before this task: fm_path_age returns 999999 when fm_path_mtime fails, so fm_lock_mid_acquire_is_fresh can no longer be handed an empty string. That was verified and deliberately left alone. It is not part of this change and must not be re-fixed or reported as a finding.
What triggers it in the wild: the captain hit this while his machine was out of disk space. Under ENOSPC, mkdir, ln -s and stat all start failing, so lock creation fails while the lock still looks reclaimable, which is exactly the state that sends the code down the recursion. The runaway was reproduced first, on the record, before anything was changed, and the reproduction is what proves the fix.
What the fix had to preserve, and the deliberate decisions taken:
Accepted acceptance criteria for this change:
Constraints that shaped the change: this is firstmate's own shared, tracked material, so firstmate-coding-guidelines governs it (one sentence per line in Markdown, plain dash not em dash, no agent co-author, shellcheck-clean bin scripts, colocated tests). bin/ scripts must be shellcheck-clean. This code is the session lock every firstmate home in the fleet depends on, including live ones, so nothing may touch another home's lock state and tests use scratch directories rather than the real state/ directory.
This diff has already been reviewed and accepted upstream on PR #2796 (class corrective, vision-aligned, no security concerns). Every substantive check on that PR is already green: lint, coverage guard, all behavior suites, Herdr, stock-Bash snapshot, repo invariants, and Greptile. The single remaining red check is "PR must be raised via no-mistakes", which needs a green no-mistakes raise whose structured body attestation matches this HEAD. The change itself is not to be re-scoped, re-designed, or expanded; only what is needed to satisfy the pipeline's own gates should change, because a changed diff would need re-reviewing upstream.
ACCEPTED SCOPE ADDED AFTER UPSTREAM REVIEW, all of it required and none of it optional:
Review of the reviewed head found a regression this change itself introduced, which has been reproduced independently: a stale ".steal" left on disk while the primary lock path is ABSENT was treated as a held lock, so the lock could never be reclaimed and never self-healed, where the base revision reclaimed it on the first attempt. Because fm_lock_acquire_wait never returns without the lock, that was an unbounded hang on the session lock every home depends on. It is fixed, with a regression test that was demonstrated failing against the pre-fix head and passing against the base revision. Also fixed: fm_autoarm_release_abandoned removing the primary lock without re-proving it still owned the steal mutex while its header asserted a guarantee that the deliberately non-exclusive leaf mutex no longer provides, and a leaked FM_LOCK_FAIL_REASON on a success path.
A repo-wide audit then established the single root cause behind those: this change widened the lock's outcome vocabulary by adding FM_LOCK_FAIL_REASON=unavailable and making the leaf mutex deliberately non-exclusive, while existing consumers still reason in the old held-or-not-held binary. The base revision could never hand a caller an empty FM_LOCK_HELD_PID, because it treated an uncreatable lock as a stale lock to recover, so every consequence below is reachable ONLY because of this change and is owned by it.
The audit classified every consumer of the lock outcome surface. Seven sites report SUCCESS when no lock was taken and no work happened, and fixing all seven is accepted, required scope for this change: bin/fm-watch.sh:856/869/871 (prints "watcher: already running" and exits 0 when no watcher exists), bin/fm-claude-stop-autoarm.sh:148/150 (Stop auto-arm silently declines to arm and reports success, so supervision can stop existing with nobody told), bin/fm-turnend-guard.sh:158 (block budget silently never reset), bin/fm-watch-arm.sh:149 and :198 (cycle record silently lost), bin/fm-push-transition-lib.sh:48 (delivery record silently dropped), and bin/fm-herdr-session-cleanup.sh:218/227 (warns but names the wrong cause). Each gets a behavioural regression test in the test file that already owns that script's contract.
Two things were deliberately ruled OUT of this change and must not be treated as omissions. First, roughly 22 further sites refuse CORRECTLY but describe the cause as contention rather than a refusing filesystem; that is a diagnosis-quality defect with nothing proceeding unsafely, and it is filed as separate follow-up work so this change stays focused and reviewable. bin/backends/herdr.sh:2923 is already correct and is left alone. Second, fm_lock_acquire_wait hanging rather than proceeding under an unavailable filesystem is the intended refuse-rather-than-proceed behaviour and is deliberately unchanged.
The private ".reclaiming.*" marker that fm_lock_claim_stale_by_rename can leave behind if a process is killed between its rename and its unlink was examined and deliberately left alone: nothing reads that name, fm_lock_claim_blocked_by_steal consults only ".steal", there is no dotglob anywhere in bin/, nothing walks the state directory with find, and lock paths are dot-prefixed so plain globs cannot see it. That is precisely why it cannot repeat the wedge above, which happened because ".steal" IS consulted by the claim path.
What Changed
fm_lock_try_acquireno longer recurses on"$lockdir.steal"; a stale steal mutex is now reclaimed by the new leaf pathfm_lock_try_acquire_leaf, so no input can produce more than one.steallevel. Primary-lock exclusivity is preserved by re-proving steal-mutex ownership and stale-holder identity immediately before eviction (fm_lock_claim_stale_by_rename,fm_lock_claim_blocked_by_steal), andfm_autoarm_release_abandonedre-proves it still owns the mutex before removing the lock.fm_lock_clear_legacy_steal_chainsweeps.steal.steal…chains left on disk by the old implementation, bounded byFM_LOCK_LEGACY_STEAL_SCAN_MAX, and a leftover.stealbeside an absent primary lock is reclaimed instead of read as held.FM_LOCK_CREATE_FAILURErecords the cause at the failing primitive (including parsingln -s's "File exists" underLC_ALL=C),FM_LOCK_FAIL_REASON=unavailablesurfaces it to callers,fm_lock_note_unavailableprints a throttled operator line (FM_LOCK_UNAVAILABLE_REPORT_EVERY), andfm_lock_create_retrying_vanishedretries exactly once when the contending path is already gone.fm-watch.shexits 1 saying no watcher is running instead of "already running",fm-claude-stop-autoarm.shprints that supervision is not running for the home,fm-turnend-guard.shsays the block budget stays spent,fm-watch-arm.shandfm-push-transition-lib.shname the cycle/delivery record that was not written, andfm-herdr-session-cleanup.shnames a refusing filesystem rather than a busy peer. Covered by a newtests/fm-watcher-lock.test.sh(recursion bound, ENOSPC refusal, concurrency) plus regression tests colocated in each affected script's existing suite, and documented indocs/watcher-continuity.mdanddocs/configuration.md.Risk Assessment
✅ Low: The recursion bound, the ENOSPC refusal gate, and primary-lock mutual exclusion each trace cleanly against concrete adversarial interleavings, all seven required consumer sites landed with behavioural regression tests that drive real interfaces, and the only surviving issue is operator-facing wording that names the primary lock when a sibling steal mutex was the thing refused.
Testing
I first reproduced the reported failure end-to-end before validating the fix: on a firstmate home whose state directory refuses new entries (the out-of-disk condition the captain hit), the base revision's fm_lock_try_acquire walked down over 900 .steal levels, flooded stderr with "File name too long", and never returned, while bin/fm-watch.sh on that same home hung in the runaway and had to be killed. Against this branch's HEAD the identical input stops at exactly one .steal level, refuses with FM_LOCK_FAIL_REASON=unavailable, and bin/fm-watch.sh prints a single line naming the filesystem as the cause and exits 1. I also captured the Claude Stop auto-arm hook's real refusal text through the fake harness with a Stop payload, and demonstrated its new regression test failing against pre-fix code and passing at HEAD. On top of that I ran the colocated suite that owns the lock contract (44 cases covering the recursion bound, concurrent single-publisher mutual exclusion, the ENOSPC refusal, on-disk .steal chain cleanup, and the contention-vs-unavailable labelling) and every consumer-site test file the change touched - all green, no failures and no flakiness observed. Evidence is CLI transcripts and hook stderr rather than screenshots because this change has no rendered UI surface; the end-user experience here is the terminal output an operator sees when arming supervision. Temporary trees and scratch homes created for the comparison runs were removed and the worktree is clean.
Evidence: Reproduction of firstmate#2787: base vs HEAD, runaway .steal chain bounded to one level
Source: Reproduction of firstmate#2787: base vs HEAD, runaway .steal chain bounded to one level
### BASE-8714c9a-before-the-fix exit status : 137 (killed by the watchdog - it never returned) deepest lock path attempted: .watch.lock.steal.steal.steal.steal.steal.steal.steal.steal.steal.stea... (truncated) steal levels: 1444 operator-visible filesystem errors: 883 x: basename: <state>/.watch.lock.steal.steal....: File name too long ### HEAD-d21fc96-with-the-fix exit status : 1 outcome : RESULT: refused (exit 1) FM_LOCK_FAIL_REASON=unavailable FM_LOCK_HELD_PID=99999 deepest lock path attempted: .watch.lock.steal... (truncated) steal levels: 1 operator-visible filesystem errors: 0 x: basename: <state>/.watch.lock.steal.steal....: File name too longEvidence: Operator CLI transcript: bin/fm-watch.sh on an unwritable home, base vs HEAD
Source: Operator CLI transcript: bin/fm-watch.sh on an unwritable home, base vs HEAD
===== BASE 8714c9a - before the fix ===== $ FM_STATE_OVERRIDE=<home>/state bin/fm-watch.sh ... 736 x "basename: <home>/state/.watch.lock.steal.steal.steal...: File name too long" deepest path attempted: .watch.lock + 905 x ".steal" -> [killed after 25s - it never returned] ===== HEAD d21fc96 - with the fix ===== $ FM_STATE_OVERRIDE=<home>/state bin/fm-watch.sh fm_lock: <home>/state/.watch.lock: cannot create the lock and none exists to reclaim - the filesystem refused the operation (out of space, read-only, or not writable); refusing to acquire PR_CHECK_MIGRATION: watcher exclusion could not be acquired; review state/.watch.lock before rearming polls watcher: PR check migration blocked; refusing to execute state checks -> [exit 1]Evidence: Claude Stop auto-arm hook refusal text, plus fail-before / pass-after on its regression test
Source: Claude Stop auto-arm hook refusal text, plus fail-before / pass-after on its regression test
$ echo '{"session_id":"sess-autoarm","stop_hook_active":false}' | claude -c 'bin/fm-claude-stop-autoarm.sh' HEAD d21fc96 - stderr the hook emits: fm_lock: <home>/state/.claude-autoarm.lock: cannot create the lock and none exists to reclaim - the filesystem refused the operation (out of space, read-only, or not writable); refusing to acquire firstmate watcher auto-arm REFUSED - the filesystem refused to create or reclaim <home>/state/.claude-autoarm.lock (out of space, read-only, or not writable), so no owner claim was taken and this Stop armed no watcher. Supervision is NOT running for this home until that is fixed. Same test against PRE-FIX code (base lock library, this branch's tests): not ok - an owner lock the filesystem refused must be reported, not silently skipped (missing: 'auto-arm REFUSED') Against HEAD d21fc96: ok - auto-arm: refuses loudly when the owner lock cannot be created ok - auto-arm: refuses loudly when an abandoned claim cannot be reclaimedEvidence: Behavioural cases carrying each accepted acceptance criterion (tests/fm-watcher-lock.test.sh)
Source: Behavioural cases carrying each accepted acceptance criterion (tests/fm-watcher-lock.test.sh)
ok - stale-owner reclaim never appends a second .steal level ok - legacy .steal chains are cleared without evicting a live holder ok - a lock the filesystem cannot create is refused once, not retried into a longer path ok - a failing filesystem primitive is distinguishable from a lock that is simply held ok - concurrent reclaim through a stale steal mutex yields exactly one winner ok - a stalled reclaim race through one steal mutex yields at most one publisher ok - an orphan steal mutex beside an absent primary lock is reclaimed, not treated as held ok - an abandoned-claim release refuses to remove the lock once its steal mutex is gone ok - an EEXIST race whose winner then releases is contention, not a refusing filesystem ok - a contended lock that vanished before the caller looked is acquired, not reported as held ok - a wait on an uncreatable lock keeps refusing, loudly and boundedly ok - watcher refuses loudly when the lock cannot be createdPipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-watch.sh:860- The new operator refusals name the primary lock as the thing that could not be created, butunavailableis also returned when the primary lock plainly exists and it was the steal mutex beside it that the filesystem refused.Concrete path:
$STATE/.watch.lockexists with a dead, non-fresh pid, sofm_lock_try_acquirereachesfm_lock_try_acquire_leaf "$lockdir.steal"(bin/fm-wake-lib.sh:1157). On a full or read-only volume the leaf'sfm_lock_owner_dir(mktemp -d) fails,FM_LOCK_CREATE_FAILUREstaysunavailable, andfm_lock_after_failed_createsetsFM_LOCK_FAIL_REASON=unavailable. Back infm_lock_try_acquirethe reason is deliberately preserved (bin/fm-wake-lib.sh:1158-1160), so bin/fm-watch.sh:860 prints "cannot create $WATCH_LOCK - the filesystem refused the operation" for a path an operator can see on disk. The library's own stderr line one moment earlier correctly names<lock>.steal, so the two lines contradict each other.Same shape at bin/fm-watch-arm.sh:105, bin/fm-turnend-guard.sh:165, bin/fm-push-transition-lib.sh:50 and bin/fm-herdr-session-cleanup.sh:49 ("could not be created") whenever their lock exists with a stale holder. bin/fm-claude-stop-autoarm.sh:173 is already phrased correctly ("refused to create or reclaim"), which is the wording the others could adopt. Low impact - the refusal itself, the exit code and the "no watcher is running" claim are all correct - but it is the one inaccurate label left in the surface this change exists to make honest, so the wording is the author's call.
✅ **Test** - passed
✅ No issues found.
bin/fm-test-run.sh tests/fm-watcher-lock.test.sh- 44 cases, all pass; carries the recursion bound (stale-owner reclaim never appends a second .steal level), the concurrency properties (concurrent reclaim through a stale steal mutex yields exactly one winner,a stalled reclaim race through one steal mutex yields at most one publisher), the ENOSPC path (a lock the filesystem cannot create is refused once, not retried into a longer path), on-disk chain cleanup (legacy .steal chains are cleared without evicting a live holder), and the unavailable-vs-contended labelling casesbin/fm-test-run.sh tests/fm-claude-stop-autoarm.test.sh tests/fm-turnend-guard.test.sh- passbin/fm-test-run.sh tests/fm-watch-arm.test.sh tests/fm-supervision-events.test.sh tests/fm-herdr-session-cleanup.test.sh tests/fm-tool-update-check.test.sh- passManual E2E reproduction of firstmate#2787: stale.watch.lock(dead recorded pid, backdated mtime) inside a state directory that refuses new entries, somktemp -d/mkdir/ln -sfail as under ENOSPC;fm_lock_try_acquirerun underset -xagainst base8714c9avs HEADd21fc96with a 25s watchdog. Base: 1444.steallevels, 883 xFile name too long, killed (never returned). HEAD: 1 level, refused withFM_LOCK_FAIL_REASON=unavailable, exit 1Manual operator CLI transcript:FM_STATE_OVERRIDE=<home>/state bin/fm-watch.shon the same unwritable home, base vs HEAD. Base ran away through 905.steallevels and had to be killed; HEAD printedfm_lock: ...: cannot create the lock and none exists to reclaim - the filesystem refused the operation ...; refusing to acquireand exited 1Manual capture of the Claude Stop auto-arm hook's real stderr, driven as a child of the fakeclaudeharness with a Stop payload on stdin:firstmate watcher auto-arm REFUSED - the filesystem refused to create or reclaim <home>/state/.claude-autoarm.lock ... Supervision is NOT running for this home until that is fixed.Fail-before / pass-after check: rantests/fm-claude-stop-autoarm.test.shfrom a scratch tree holding this branch's tests withbin/fm-wake-lib.shandbin/fm-claude-stop-autoarm.shreverted to base8714c9a->not ok - an owner lock the filesystem refused must be reported, not silently skipped (missing: 'auto-arm REFUSED'); the same two cases pass at HEAD✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix: suppress SC2016 on literal watcher fixture body
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.