fix(bin): fix lock ordering race behind wake-queue deadlocks

[x45dev]

Intent

Fix the wake-queue lock deadlock that killed fleet supervision for about twenty minutes on 2026-08-12 with four tasks in flight, and recurred live on 2026-08-22 at 03:22 with seven workers unwatched. Both times supervision died and could not self-recover; both were recovered by hand and the defect itself was unfixed.

Reported mechanism, in order: (1) bin/fm-watch-arm.sh gives a starting watcher FM_ARM_CONFIRM_TIMEOUT (default 10s) to touch state/.last-watcher-beat, then TERMs it. (2) bin/fm-watch.sh spends a reproducible 4.3s wall in fm-pr-check-migrate.sh --checks-safe before it even reaches the lock, and more before the loop's first beat. (3) A watcher TERMed in that window can die holding state/.wake-queue.lock; its trap was trap 'exit 1' HUP INT TERM, which released nothing. (4) fm_lock_acquire_wait was reported as an unbounded spin with no dead-holder steal. Evidence: six consecutive state/.watch-cycle-exits.log records with reason=confirmation-timeout and beacon_age climbing 162, 175, 443; in the second occurrence both state/.wake-queue.lock and state/.watch.lock existed with owner dirs naming dead pids (1744796 and 947306 on the queue lock, 947306 on the watch lock) plus an orphaned queue-lock owner dir that had survived since 11 Aug. Manual recovery both times: confirm the recorded holder pid is dead, remove the lock and its owner dir, re-arm.

The brief scoped this to item 4 only, and explicitly excluded two things: do NOT change the arm confirmation timeout, and do NOT optimise the pre-lock startup cost. Those make the window reachable but are not the defect, and fixing either would make this rarer while leaving the trap in place. Those exclusions are honoured; neither is touched. The brief also asked a specific open question: check whether bin/fm-watch.sh's TERM trap should release the locks it holds, fix it if so, and say which.

Investigation found the brief's item 4 premise does not hold at HEAD, and this was verified rather than assumed. The dead-holder steal lives in fm_lock_try_acquire and is generic to every lock path, not specific to the singleton watch lock, so fm_lock_acquire_wait already inherits it. Both assertions the brief mandated - that a second acquirer takes over a lock whose recorded holder is dead, and that a lock held by a LIVE process is never stolen - pass unchanged against the pre-change library, confirmed by running them against a clean git archive HEAD tree. They still ship, as regression guards rather than as proof.

The real mechanism is cost, not unboundedness: a dead-holder steal is roughly 20-30 forks, measured at 3.0s wall for one lock on an unloaded host. Added to the 4.3s pre-lock startup, a successor watcher exceeded the same 10s confirmation timeout before its first beat, was TERMed, abandoned the same locks again, and billed the next one identically. That is a self-perpetuating loop, which is why it read as a deadlock and could not clear itself.

The root defect, found by instrumenting a reproduction, is an ordering inversion in fm_lock_try_create: the lock becomes visible on disk at ln -s and carries this process's pid at fm_lock_claim, several forks before _fm_lock_track_held records it in memory. Every one of those forks is a signal-delivery point, so a signal landing in that window leaves a lock held by this process that its own exit sweep cannot see. The identical inversion around mktemp -d is what leaks the .owner.* directories, which explains the orphaned 11 Aug owner dir observed in the live incident. The fix establishes one invariant - record before creating, forget only after removing - so the in-memory record is always a superset of what is durably on disk, and adds pending-owner tracking so an interrupted acquisition does not leak its owner directory.

Answering the brief's open question explicitly: yes, the TERM trap does need to release its locks, and both halves are required. Without the trap nothing sweeps at all; without the ordering fix the sweep saw only about half of what was held, which is why releasing on the trap alone fixed the end-to-end failure only about 50% of the time.

Tests follow the repo's existing convention in tests/fm-watcher-lock.test.sh; no new harness. A signalled watcher must hand back every lock it holds - this failed on the pre-change library and passed only ~50% of the time with the trap fix alone, and now passes 12 runs out of 12. A direct assertion of the ordering invariant overrides the two steps that each follow a durable write, because a timing race cannot check an ordering reliably; it fails cleanly against the pre-change library. The two brief-mandated acquire_wait assertions are included as guards.

Deliberately out of scope and reported rather than fixed, all verified against the default branch on the same host: test_lock_single_winner_under_concurrency is a load-sensitive flake on both trees (1/10 failures on the default branch against 2/10 here in interleaved runs, and it is what aborts the default branch's own full-suite run); test_arm_self_eviction_is_loud_without_successor fails 4/4 on the default branch in isolation; and test_signal_catchup_without_running_watcher fails 3/3 on both, its 4s exit budget being shorter than the pre-lock startup this host needs. Also noted and not changed: other wake-queue users (fm-classify-lib.sh, fm-inactive-reconcile.sh, fm-push-transition-lib.sh, fm-procevent.sh, fm-startup-network.sh) install no equivalent sweep, which likely explains the second dead pid on the queue lock in the live incident.

shellcheck -x is clean on all three changed files.

What Changed

• fm_lock_try_create (and the matching mktemp -d owner-directory path) in bin/fm-wake-lib.sh now records a lock/pending-owner in FM_LOCK_HELD_PATHS before publishing it on disk, and forgets it only after removal, so a signal landing mid-acquisition can no longer leave a lock or owner directory that the process's own exit sweep can't see.
• bin/fm-watch.sh's TERM/HUP/INT trap now releases the locks the watcher holds instead of exiting with no cleanup, closing the second half of the failure (a sweep alone only saw about half of what was held without the ordering fix).
• Added tests/fm-watcher-lock.test.sh coverage: a direct assertion of the record-before-create/forget-after-remove ordering invariant, a signalled-watcher hand-back-all-locks test, and the two dead-holder-steal / live-holder-not-stolen guard tests.
• Updated docs/watcher-continuity.md to list the new lock-handback and ordering tests.

Risk Assessment

✅ Low: Only change since the prior review round is a 4-line explanatory comment at bin/fm-watch.sh:880-883, precisely scoped to the requested fix; verified against fm-wake-lib.sh that fm_lock_prepare_owner does stamp the pid before the lock symlink is published, confirming the comment's claim and that the previously reported gap does not exist.

Testing

Ran tests/fm-watcher-lock.test.sh (the repo's existing convention for this area, no new harness) both as a full suite and as isolated extractions of the fix-specific tests; the four tests directly proving the ordering-invariant fix and the signal-release behavior passed on every run (including 5/5 isolated repeats of test_watcher_signal_releases_held_locks), and a full clean run of all 30 tests in the file passed end-to-end. One earlier full-suite attempt hit the pre-existing, author-documented host-load-sensitive flake in test_lock_single_winner_under_concurrency, which is unrelated to this change and explicitly called out as out of scope in the intent; it passed on immediate retry, consistent with that characterization. No linting or static analysis was run, per the test-phase scope. Working tree is clean with no leftover test artifacts.

• Outcome: ⚠️ 1 info across 1 run (8m47s)

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 1 issue found → auto-fixed ✅

• ⚠️ bin/fm-watch.sh:863 - watcher_bootstrap_cleanup (and later watcher_cleanup) only release the singleton WATCH_LOCK through the recovery-marker branch, gated on cat "$WATCH_LOCK/pid" = "$WATCHER_PID". fm_lock_release_all_held is called with WATCH_LOCK in its keep-list (fm-wake-lib.sh:373-384), so the generic sweep deliberately never touches it. fm_lock_try_create now tracks the singleton lock in FM_LOCK_HELD_PATHS before the ln -s that publishes it (fm-wake-lib.sh:521-528), but fm_lock_claim still needs an intervening readlink fork (fm_lock_points_to_owner, fm-wake-lib.sh:447-450) before it stamps this process's pid into the owner dir's pid file. A TERM landing in that narrow sub-window leaves WATCH_LOCK visible on disk but with no pid recorded yet: the recovery-marker check reads a pid that isn't WATCHER_PID (so it skips its release-lock branch), and the generic sweep also skips it (it's in the keep-list). Neither path releases it, so recovery falls back to the old, expensive route (fm_lock_mid_acquire_is_fresh grace period, then a full dead-holder-style steal) - the exact class of failure this branch sets out to close, just in a much narrower window than before the fix, and specific to the watcher's own singleton lock (every other lock is fully covered by the generic sweep). This narrower window doesn't match the sequence evidenced in the two live incidents (both recorded pids already stamped in the singleton lock's owner dir, which the existing recovery-marker branch already handles), so it's a residual gap rather than a reopening of the demonstrated bug. A boundary fix: only exclude WATCH_LOCK from the sweep once its pid is confirmed as this process's own; otherwise let the sweep fall back to a plain fm_lock_release for it, the same way every other lock is handled.

🔧 Fix: placeholder
✅ Re-checked - no issues remain.

⚠️ **Test** - 1 info

• ℹ️ test_lock_single_winner_under_concurrency (tests/fm-watcher-lock.test.sh) failed once ('got 2' winners) under heavy concurrent host load, then passed cleanly on immediate retry and in a full clean re-run of the whole suite. This matches the user intent's own documented characterization of this test as a pre-existing load-sensitive flake on both the default branch and this branch, explicitly called out as deliberately out of scope and not to be fixed here. No action needed.
• bin/fm-test-run.sh tests/fm-watcher-lock.test.sh (full suite, initial run: 12 tests passed then hit the known concurrency flake)
• bin/fm-test-run.sh tests/fm-watcher-lock.test.sh (full suite, retry: all 30 tests passed cleanly end-to-end)
• Isolated extraction running test_lock_is_recorded_before_it_becomes_visible, test_lock_acquire_wait_takes_over_from_dead_holder, test_lock_acquire_wait_never_takes_a_live_holders_lock, test_watcher_signal_releases_held_locks together: all passed
• Isolated extraction running test_watcher_signal_releases_held_locks alone, 5 consecutive times: 5/5 passed
• git status --short after testing: clean, no leftover artifacts in the working tree

✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the prior pending-owner finding is fixed by tracking the computed path before directory creation and retaining that tracking until cleanup or successful transfer to the held lock.

_{Reviews (3): Last reviewed commit: "chore(tests): drop a temp test runner co..." | Re-trigger Greptile}