Skip to content

fix(bin): bound lock stealing to one .steal level and refuse uncreatable locks - #2796

Open
benlamlih wants to merge 13 commits into
kunchenguid:mainfrom
benlamlih:fm/fm-lock-steal-recursion
Open

fix(bin): bound lock stealing to one .steal level and refuse uncreatable locks#2796
benlamlih wants to merge 13 commits into
kunchenguid:mainfrom
benlamlih:fm/fm-lock-steal-recursion

Conversation

@benlamlih

@benlamlih benlamlih commented Aug 22, 2026

Copy link
Copy Markdown

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:

  • The steal mutex exists for a reason: it is the serialization that stops two processes both deciding an owner is stale and both publishing the primary lock. fm_autoarm_release_abandoned depends on exactly that property. Removing the mutex to remove the recursion was explicitly ruled out. The chosen shape is a bounded steal - one level, never a chain - decided deliberately rather than by default.
  • When it cannot acquire, it must REFUSE rather than proceed. A lock that gives up its serialization under disk pressure is worse than one that reports it cannot run: two sessions both believing they hold the home's session lock is the failure this whole mechanism exists to prevent. Refusing to proceed is the intended behavior here, not a missing fallback.
  • A failure of the underlying primitive must be distinguishable from a lock that is simply held. Under ENOSPC today they looked the same to the caller, which is part of why the runaway was silent until the filename blew up. The distinct unavailable/refused reporting is deliberate.
  • Whatever the operator needs to understand what happened is left behind, because the current failure was silent right up until an unreadable filesystem error.

Accepted acceptance criteria for this change:

  • No input can produce more than one .steal level, demonstrated by a test rather than asserted.
  • Mutual exclusion is intact: concurrent stale-owner reclaim still cannot let two processes publish the primary lock, proven with a concurrent test.
  • Acquisition failure under a failing filesystem primitive is refused clearly, not silently retried into a longer path.
  • Behavioural tests colocated the way this repo does it, covering the recursion bound, the ENOSPC path, and the concurrency property.
  • Any .steal chains this bug already left on disk are handled sensibly, or there is a stated reason they need no cleanup.

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_acquire no longer recurses on "$lockdir.steal"; a stale steal mutex is now reclaimed by the new leaf path fm_lock_try_acquire_leaf, so no input can produce more than one .steal level. 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), and fm_autoarm_release_abandoned re-proves it still owns the mutex before removing the lock. fm_lock_clear_legacy_steal_chain sweeps .steal.steal… chains left on disk by the old implementation, bounded by FM_LOCK_LEGACY_STEAL_SCAN_MAX, and a leftover .steal beside an absent primary lock is reclaimed instead of read as held.
  • Acquisition now distinguishes a lock that is held from one the filesystem refused to create: FM_LOCK_CREATE_FAILURE records the cause at the failing primitive (including parsing ln -s's "File exists" under LC_ALL=C), FM_LOCK_FAIL_REASON=unavailable surfaces it to callers, fm_lock_note_unavailable prints a throttled operator line (FM_LOCK_UNAVAILABLE_REPORT_EVERY), and fm_lock_create_retrying_vanished retries exactly once when the contending path is already gone.
  • Seven consumers that reported success on a lock they never took now report the refusal: fm-watch.sh exits 1 saying no watcher is running instead of "already running", fm-claude-stop-autoarm.sh prints that supervision is not running for the home, fm-turnend-guard.sh says the block budget stays spent, fm-watch-arm.sh and fm-push-transition-lib.sh name the cycle/delivery record that was not written, and fm-herdr-session-cleanup.sh names a refusing filesystem rather than a busy peer. Covered by a new tests/fm-watcher-lock.test.sh (recursion bound, ENOSPC refusal, concurrency) plus regression tests colocated in each affected script's existing suite, and documented in docs/watcher-continuity.md and docs/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 long

Reproduction of firstmate#2787 - "Lock acquisition recurses through .steal
suffixes when initial lock creation fails" - driven through the real
fm_lock_try_acquire, base revision vs this branch's HEAD.

Setup, identical for both runs: a firstmate home holding a stale .watch.lock
(recorded pid is dead, mtime backdated past every freshness grace) inside a
state directory that accepts no new entries, so mktemp -d, mkdir and ln -s all
fail exactly as they do when the volume is out of space - the condition the
captain hit. A 25s watchdog bounds each run because the pre-fix path never
returns on its own.

\### BASE-8714c9a-before-the-fix
exit status : 137  (killed by the watchdog - it never returned)
outcome     : 
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 long
Evidence: 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]

Operator CLI transcript: arming the watcher in a firstmate home whose state
directory can no longer accept new entries - the out-of-disk shape from #2787.
Same command, same home, base revision vs this branch's HEAD.

===== BASE 8714c9a - before the fix =====
state dir is read-only, so mkdir / ln -s / mktemp all fail, exactly as under ENOSPC

$ 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 =====
state dir is read-only, so mkdir / ln -s / mktemp all fail, exactly as under ENOSPC

$ 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 reclaimed

Claude Stop auto-arm hook: what an operator sees when the home's state directory
refuses the owner lock. Driven through the real hook as a child of the fake
Claude harness, with a real Stop payload on stdin - the way it fires in production.

  $ 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 regression test, same fixture, run against the PRE-FIX code (this branch's
tests with bin/fm-wake-lib.sh and bin/fm-claude-stop-autoarm.sh reverted to base
8714c9a). It fails - the hook stayed silent and let the Stop through as if a
watcher had been armed:

  ok - auto-arm: an abandoned owner claim is reclaimed so a lapsed cycle re-arms
  not ok - an owner lock the filesystem refused must be reported, not silently skipped (missing: 'auto-arm REFUSED')

Against HEAD d21fc96 the same two cases pass:

  ok - auto-arm: refuses loudly when the owner lock cannot be created
  ok - auto-arm: refuses loudly when an abandoned claim cannot be reclaimed
Evidence: 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 created

Behavioural cases that carry each accepted acceptance criterion, from the
colocated suite that owns the lock contract (tests/fm-watcher-lock.test.sh).

  ok - concurrent fm_lock_try_acquire yields exactly one winner
  ok - concurrent stale-lock steal yields exactly one winner
  ok - live steal mutex is not reclaimed
  ok - live-held lock is not stolen
  ok - paused mid-acquire claimant backs off to active stealer
  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 - a claim whose pid write is refused reports unavailable, not a held lock
  ok - a claim whose owner directory a racer removed is contention, not a refusing filesystem
  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 - watcher refuses loudly when the lock cannot be created
  ok - stale-owner reclaim never appends a second .steal level
  ok - legacy .steal chains are cleared without evicting a live holder
  ok - an orphan steal mutex beside an absent primary lock is reclaimed, not treated as held
  ok - an absent primary lock still refuses a live mutex owner and an uncreatable path
  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 abandoned-claim release refuses to remove the lock once its steal mutex is gone
  ok - a wait on an uncreatable lock keeps refusing, loudly and boundedly

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 1 info
  • ℹ️ bin/fm-watch.sh:860 - The new operator refusals name the primary lock as the thing that could not be created, but unavailable is also returned when the primary lock plainly exists and it was the steal mutex beside it that the filesystem refused.

Concrete path: $STATE/.watch.lock exists with a dead, non-fresh pid, so fm_lock_try_acquire reaches fm_lock_try_acquire_leaf &#34;$lockdir.steal&#34; (bin/fm-wake-lib.sh:1157). On a full or read-only volume the leaf's fm_lock_owner_dir (mktemp -d) fails, FM_LOCK_CREATE_FAILURE stays unavailable, and fm_lock_after_failed_create sets FM_LOCK_FAIL_REASON=unavailable. Back in fm_lock_try_acquire the 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 &lt;lock&gt;.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 cases
  • bin/fm-test-run.sh tests/fm-claude-stop-autoarm.test.sh tests/fm-turnend-guard.test.sh - pass
  • bin/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 - pass
  • Manual E2E reproduction of firstmate#2787: stale .watch.lock (dead recorded pid, backdated mtime) inside a state directory that refuses new entries, so mktemp -d/mkdir/ln -s fail as under ENOSPC; fm_lock_try_acquire run under set -x against base 8714c9a vs HEAD d21fc96 with a 25s watchdog. Base: 1444 .steal levels, 883 x File name too long, killed (never returned). HEAD: 1 level, refused with FM_LOCK_FAIL_REASON=unavailable, exit 1
  • Manual operator CLI transcript: FM_STATE_OVERRIDE=&lt;home&gt;/state bin/fm-watch.sh on the same unwritable home, base vs HEAD. Base ran away through 905 .steal levels and had to be killed; HEAD printed fm_lock: ...: cannot create the lock and none exists to reclaim - the filesystem refused the operation ...; refusing to acquire and exited 1
  • Manual capture of the Claude Stop auto-arm hook's real stderr, driven as a child of the fake claude harness with a Stop payload on stdin: firstmate watcher auto-arm REFUSED - the filesystem refused to create or reclaim &lt;home&gt;/state/.claude-autoarm.lock ... Supervision is NOT running for this home until that is fixed.
  • Fail-before / pass-after check: ran tests/fm-claude-stop-autoarm.test.sh from a scratch tree holding this branch's tests with bin/fm-wake-lib.sh and bin/fm-claude-stop-autoarm.sh reverted to base 8714c9a -> not ok - an owner lock the filesystem refused must be reported, not silently skipped (missing: &#39;auto-arm REFUSED&#39;); the same two cases pass at HEAD
✅ **Document** - passed

✅ No issues found.

🔧 **Lint** - 1 issue found → auto-fixed ✅
  • ⚠️ linter found issues (exit code 1)

🔧 Fix: suppress SC2016 on literal watcher fixture body
✅ Re-checked - no issues remain.

✅ **Push** - passed

✅ No issues found.

@greptile-apps

greptile-apps Bot commented Aug 22, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Reviews (2): Last reviewed commit: "no-mistakes(lint): suppress SC2016 on li..." | Re-trigger Greptile

@benlamlih

Copy link
Copy Markdown
Author

Reviewer note: which lock the race regression test drives, and why it matters

test_lock_stale_steal_mutex_reclaim_race_has_one_publisher deliberately drives $STATE/.watch.lock, not an arbitrary lock name.

The double-publish window opens between the primary reclaim's staleness re-proof and its publish.
For .watch.lock that gap contains _fm_recovery_marker_publish, a multi-fork write-and-rename, which widens the window from a couple of instructions to milliseconds.
On any other lock that gap is nearly empty, the interleaving almost never lands, and the test would pass without exercising the property it exists to protect.

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 bug was only caught by constructing the exact stall interleaving on the lock whose window is actually open.
A future change that "simplifies" this test onto a generic lock name, or replaces the deliberate stalls with more concurrency, would make it vacuous rather than stronger.

The test creates the stalls by copying the two real functions with declare -f and wrapping them in the test process only, so there is no test-only hook, sleep, or seam in bin/fm-wake-lib.sh itself.

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

Corrective for #2787: fm_lock_try_acquire no longer recurses into "$lockdir.steal" unbounded. Steal mutexes are a one-level leaf (fm_lock_try_acquire_leaf); an uncreatable lock is refused with FM_LOCK_FAIL_REASON=unavailable instead of being treated as a stale lock to recover; fm_lock_acquire_wait still never returns without the lock. Matches the filed lock-steal recursion failure.

VISION (per rule):

  • One captain, one interface — aligns. Operator gets a named refusal instead of megabytes of .steal.steal... until SIGSEGV.
  • Authority is explicit — aligns. Fail-closed; does not proceed without the lock.
  • Scripts own the mechanics — aligns. Lock identity stays in deterministic scripts.
  • A restart is a non-event — aligns. Session lock remains exclusive under disk pressure.
  • Delegation with a spine — aligns. Strengthens a refusal path.
  • The fleet outlives any vendor — n/a.
  • Scope — command-layer lock, not workshop validation.

Security: none. Workflows approved this pass after a clean diff review (CI 32573464807, Require no-mistakes 32573464806).

Class: corrective.

Overlap: docs/configuration.md also appears on hold-pair #2637 and #2768; this PR's lock change is otherwise distinct (bin/fm-wake-lib.sh, tests/fm-watcher-lock.test.sh). Not a spawn/teardown lifecycle hold.

CI: no structured no-mistakes-pipeline-attestation:v1 in the body (the "Updates from git push no-mistakes" prose and a git-push signature are not a substitute). Require no-mistakes FAILED (run 32573464806). CI 32573464807 in progress, not green. Greptile is not a merge gate.

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 (14442f00437200eac59cb3667a936605b77037fc) — not waiting on the captain.

…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.
@benlamlih
benlamlih force-pushed the fm/fm-lock-steal-recursion branch from 14442f0 to 49011bd Compare August 23, 2026 07:20
@benlamlih benlamlih changed the title fix(bin): bound lock stealing to one level and refuse an uncreatable lock fix(bin): bound lock stealing to one .steal level and refuse uncreatable locks Aug 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants