Repository navigation
fix(bin): stop lock waits in a removed home from spinning, and order pi-watch recovery tests deterministically - #32
Merged
Conversation
fm_lock_try_acquire read a lock it could not create as stale even when no lock existed at all, then tried to steal it through "$lock.steal", which failed the same way one level deeper, without bound. Any caller whose lock directory had been removed recursed like that, forking at every level and burning a CPU core for minutes. It now refuses an absent lock outright; recovering an existing stale lock is unchanged. fm_lock_acquire_wait still waited forever for a lock whose directory was gone, and now fails instead. Most callers already handle a failed wait, and the rest act only on the directory that no longer exists. fm_wake_append stops at that failure, so the deferred startup-network worker, which outlives the session start that launched it and waits on its publication lock and the wake queue, returns once its home is removed. tests/fm-startup-network.test.sh left five such workers behind on every run, once its fixture homes were removed at exit. On a 4-vCPU CI runner they slowed every later suite in the shard two to three times, which turned load-sensitive cases in tests/fm-pi-watch-extension.test.sh into a deterministic failure whenever a shard ran that suite after this one. tests/fm-watcher-lock.test.sh pins both lock calls refusing a removed directory without a steal chain, and tests/fm-startup-network.test.sh pins a worker exiting when its home is removed while it waits on its publication lock or while it delivers its wake; each fails on the previous code.
Several watch-arm recovery cases raced their own timing. A successor
launched through a login shell could still be starting when a 250ms
readiness budget lapsed, so it was retired before it recorded itself or
installed the TERM trap its case depends on. A 120ms sleep stood in for the
OpenCode plugin's unowned launch settling, so a slow launch answered the
next idle event with the lock it had already read and never armed. And the
stub turn-end guards exited without reading the payload the plugin writes,
which failed that write with an unhandled EPIPE on a loaded host.
The recovery cases now run on a mock clock: each readiness, retirement, or
retry budget lapses only when the case elapses it, and a case does so only
after the child that budget measures has visibly reached the state under
test. The lock case waits on the plugin's in-flight launch through its
shared coordinator, and the stub guards read their payload as
bin/fm-turnend-guard.sh does. No budget value changes.
A case that fails leaves its arm fixtures looping on release files that
never appear, so the suite now reaps every process still running from its
own temp root when it exits, instead of leaving that load to later suites.
With 20 CPU spinners on the test's CPU, the previous recovery cases passed
0 or 1 of 3 runs each, failing as CI did ("expected one successor plus two
retries, got 2"), while the new ones pass every run.
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.
Intent
Fix the root cause of the flaky tests/fm-pi-watch-extension.test.sh so it passes deterministically under load and in any CI shard - without widening timeouts.
Context from the queued backlog item fm-pi-watch-extension-suite-flaky: serial runs were clean while concurrent runs failed a random case (OpenCode external-healthy output, Pi hung-successor, OpenCode fleet-lock), pointing at cross-case shared state or a wall-clock readiness race. Widening readiness budgets was explicitly banned: PR 26 had tried six readiness budgets from 250ms to 2s, that did not stop the flake, and it was reverted. New evidence 2026-09-17: #26 adds two serial suites, which rebalanced CI's shard partition and moved this suite into portable-serial-4of4, where it failed 3 of 3 attempts on CI run 35206607313, every time in 'Pi must deliver the actionable wake after bounded hung-successor recovery: expected exit 0, got 1', with the suite at main's exact bytes. Required: reproduce locally (shard-4 neighbours or comparable concurrent load) before changing anything; find the shared state or readiness race; make readiness deterministic with an explicit ready signal or ordering, not a longer budget; prove it (the reproduced failing configuration passes repeatedly, N of N; serial still passes; main's timing budgets unchanged). If the defect is in product code rather than the test, fix it there with a regression test. Once this lands, PR 26's CI rebases onto it. Constraints followed: never run a test suite with a live FM_HOME exported; never stop, restart, or update the shared no-mistakes daemon.
Root cause 1, cross-suite shared state (product defect, fixed in product with regression tests): tests/fm-startup-network.test.sh leaves five detached startup-network workers waiting to deliver results to the test shell's claim. Its EXIT cleanup deletes their homes; fm_lock_try_acquire then treated each absent lock as stale and recursed through ever-deeper .steal locks without bound (measured 231 levels in 5s, about one CPU core per worker for minutes), and fm_lock_acquire_wait would otherwise wait forever. CI logs for both main's and PR 26's shard 4 show every suite after fm-startup-network running 1.3-2.8x slower on the 4-vCPU runner; PR 26's rebalance placed the Pi suite in that window. Decisions: fm_lock_try_acquire refuses a lock that does not exist instead of stealing it (recovery of an existing stale lock is unchanged). fm_lock_acquire_wait now fails once the lock's parent directory no longer exists; this is deliberately in the shared primitive (single owner) rather than a startup-network-local helper, after auditing callers: 30 call sites already handle a failed wait, the rest act only on the directory that is gone, and before this change such a wait could never succeed anyway. fm_wake_append stops at that failure, and bin/fm-startup-network.sh call sites handle a failed wait (the lease path also removes its temp files). A worker-local helper was tried first and dropped because a worker inside fm_wake_append still stranded (observed under 4-CPU pinning). Regression tests: tests/fm-watcher-lock.test.sh proves both lock calls in a removed directory refuse promptly (bounded with fm_run_timed so a regression fails instead of hanging); tests/fm-startup-network.test.sh proves a worker exits when its home is removed while waiting on its publication lock, and while delivering its wake (made deterministic by holding the relevant lock and moving the home atomically). Each fails on the previous code.
Root cause 2, readiness races inside the suite (test defects; the Pi extension and OpenCode plugins are intentionally untouched): the Pi and OpenCode hung, unretired, and late-close recovery cases used a 250ms readiness budget that lapsed before a login-shell successor ran its first line (CI's hidden message was 'expected one successor plus two retries, got 2'); the OpenCode fleet-lock case slept 120ms as a stand-in for the plugin's unowned launch settling, so the next idle event joined the stale launch and never armed; the stub turn-end guards exited without reading the payload the plugin writes, which crashed the test with an unhandled EPIPE under load. Decisions: the recovery cases use node:test MockTimers through a generated recovery-clock helper, so each readiness, retirement, or retry budget lapses only after the child it measures has visibly reached the state under test (fixtures record themselves after installing their TERM trap), with arm-child close tracking via the public node:diagnostics_channel child_process channel; the fleet-lock case awaits the plugin's in-flight launch through its existing shared coordinator and asserts read-only; stub guards read their payload as bin/fm-turnend-guard.sh does; the suite reaps processes still running from its own unique temp root on exit (fixed-string match, SIGKILL because some fixtures ignore TERM) so a failing case cannot load later suites. No budget value, plugin default, or wait bound changed; the only removed wait is the racing 120ms sleep. Works on Node 22.18 and 24 (the suite's existing NODE_NO_WARNINGS=1 silences Node 22's MockTimers experimental warning).
Evidence gathered locally: with 20 CPU spinners on the suite's CPU, main's recovery cases passed 0 or 1 of 3 each (Pi hung, unretired, late; OpenCode hung, unretired, late, fleet-lock), reproducing CI's failure; on 4 CPUs saturated by real leaked workers main's fleet-lock case passed 3/10. With the fix: full suite under the 20-spinner load 5/5, under 4-CPU real leak 10/10, eight concurrent copies times three rounds 24/24, serial 5/5, Node 22.18 30/30, and PR 26's shard-4 suite order (pinned to 4 CPUs and unpinned) leaves 0 workers behind with the Pi suite passing. Known unrelated local-only failures that also fail on main here: fm-on.test.sh (tool PATH), fm-cursor-primary.test.sh (no C compiler), the session-start Herdr husk case, and load-sensitive tests/fm-watch-arm.test.sh. Out-of-scope follow-ups deliberately not done: plugin writes to hook stdin lack an error handler, and tests/fm-watch-arm.test.sh has its own load-sensitive waits.
What Changed
bin/fm-wake-lib.sh):fm_lock_try_acquireno longer treats a lock that does not exist as stale. It retries creation once, which covers a holder releasing mid-acquisition, and then refuses. Previously it recursed through ever-deeper.steallocks without bound.fm_lock_acquire_waitnow fails once the lock's parent directory is gone instead of waiting forever.fm_wake_appendstops at that failure.fm_lock_acquire_waitcall inbin/fm-startup-network.shhandles a failed wait. The sweep-lease path also removes its temp files.bin/fm-lock.shfails closed with a read-only error when it cannot take the fleet-lock claim mutex.tests/fm-watcher-lock.test.shcovers two cases, both bounded withfm_run_timed. Both lock calls refuse promptly in a removed directory without recreating it, and a lock released mid-acquisition is taken with exactly one retry.tests/fm-startup-network.test.shcovers three cases. A worker exits when its home is moved away while it waits on its publication lock. A worker exits when its home is moved away while it delivers its wake. A fleet-lock takeover that loses its home while queued on the claim mutex exits 1 with the read-only error.tests/fm-pi-watch-extension.test.sh:node:testMockTimers through a generatedrecovery-clock.mjshelper. It tracks arm-child closes through thechild_processdiagnostics channel. Each readiness, retirement, or retry budget lapses only after the child it measures has visibly started or closed. No budget value changed.read-only. This replaces the 120ms sleep.Risk Assessment
✅ Low: The fix-round changes are small and do what was prescribed (the fm_lock_try_acquire retry cannot recurse or loop, fm-lock.sh exits 1 with a read-only error before CLAIM_LOCK_HELD is set, and an empty temp root fails the suite before the reaper trap is installed), each has a regression test that runs the code rather than reading source, no timing budget, wait bound or plugin code changed, and the one remaining issue affects only test cleanup when the suite is interrupted.
Testing
I first reproduced the failure on the base commit under comparable load. With 20 CPU spinners pinned to the suite's CPU, main's Pi suite failed 3/3 in CI's exact hung-successor case. In the shard-4 order on 4 CPUs (fm-startup-network, then the Pi suite) it failed 3/3 in the fleet-lock case, with 5 leaked workers still running. At the target commit the same configurations passed every time: 5/5 under the spinner load, 3/3 in shard order with no workers left behind, 24/24 across eight concurrent copies over three rounds, and 5/5 serial. The new lock and startup-network regression tests pass on the fix and fail on the previous product code. The configured timing budgets are identical to main's, the only removed wait is the racing 120ms sleep, and no plugin file changed. Node 22 could not be checked locally because only Node 24 is installed. There is no UI surface, so the evidence is CLI transcripts.
Evidence: Base (main) Pi suite under the 20-spinner pinned load fails 3/3 in CI's failing case
Source: Base (main) Pi suite under the 20-spinner pinned load fails 3/3 in CI's failing case
== BASE 2804ff7 (main): tests/fm-pi-watch-extension.test.sh pinned to cpu 5 with 20 spinners, 3 runs run 1: FAIL rc=1 ok=5 28s not ok - Pi must deliver the actionable wake after bounded hung-successor recovery: expected exit 0, got 1 run 2: FAIL rc=1 ok=5 31s not ok - Pi must deliver the actionable wake after bounded hung-successor recovery: expected exit 0, got 1 run 3: FAIL rc=1 ok=5 30s not ok - Pi must deliver the actionable wake after bounded hung-successor recovery: expected exit 0, got 1 == BASE 2804ff7 (main) result: 0 of 3 passedEvidence: Target Pi suite under the same load passes 5/5
Source: Target Pi suite under the same load passes 5/5
== TARGET 651212b (fix): tests/fm-pi-watch-extension.test.sh pinned to cpu 11 with 20 spinners, 5 runs (651212b, node v24.18.0) run 1: PASS rc=0 ok=30 133s run 2: PASS rc=0 ok=30 135s run 3: PASS rc=0 ok=30 118s run 4: PASS rc=0 ok=30 119s run 5: PASS rc=0 ok=30 124s == TARGET 651212b (fix) result: 5 of 5 passedEvidence: Shard-4 order on 4 CPUs, base: 5 leaked workers, Pi suite fails 3/3 in the fleet-lock case
Source: Shard-4 order on 4 CPUs, base: 5 leaked workers, Pi suite fails 3/3 in the fleet-lock case
round 1: startup-network rc=0 | workers left when Pi suite starts=5, when it ends=7 | Pi suite rc=1 ok=18 18s not ok - OpenCode watch plugin must arm only when this session owns the fleet lock: expected exit 0, got 1 round 2: ... starts=5, ends=9 | Pi suite rc=1 (same case) round 3: ... starts=5, ends=6 | Pi suite rc=1 (same case) == BASE 2804ff7 (main) result: Pi suite passed 0 of 3 roundsEvidence: Shard-4 order on 4 CPUs, target: no workers left behind, Pi suite passes 3/3
Source: Shard-4 order on 4 CPUs, target: no workers left behind, Pi suite passes 3/3
round 1: startup-network rc=0 | workers left when Pi suite starts=4, when it ends=0 | Pi suite rc=0 ok=30 11s round 2: startup-network rc=0 | workers left when Pi suite starts=4, when it ends=0 | Pi suite rc=0 ok=30 11s round 3: startup-network rc=0 | workers left when Pi suite starts=5, when it ends=0 | Pi suite rc=0 ok=30 10s == TARGET 651212b (fix) result: Pi suite passed 3 of 3 roundsEvidence: Leaked startup-network workers, base vs target (process listing over 15 seconds)
Source: Leaked startup-network workers, base vs target (process listing over 15 seconds)
Evidence: New regression tests fail on the previous product code and pass on the fix
Source: New regression tests fail on the previous product code and pass on the fix
Evidence: Target: 8 concurrent copies over 3 rounds passed 24/24; serial passed 5/5
Source: Target: 8 concurrent copies over 3 rounds passed 24/24; serial passed 5/5
round 1: PASS PASS PASS PASS PASS PASS PASS PASS round 2: PASS PASS PASS PASS PASS PASS PASS PASS round 3: PASS PASS PASS PASS PASS PASS PASS PASS == concurrent result: 24 of 24 passed == serial result: 5 of 5 passed (about 10s each)Evidence: Timing budgets identical to main; only the 120ms sleep removed; no plugin files changed
Source: Timing budgets identical to main; only the 120ms sleep removed; no plugin files changed
Evidence: Load harness used for the pinned-CPU spinner runs
Source: Load harness used for the pinned-CPU spinner runs
Evidence: Shard-order harness (startup-network, then the Pi suite, on 4 CPUs)
Source: Shard-order harness (startup-network, then the Pi suite, on 4 CPUs)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
tests/fm-pi-watch-extension.test.sh:19- The new EXIT reaper can SIGKILL unrelated processes if the temp root was never created. It kills every process whose args match "$TMP_ROOT/", and TMP_ROOT is never checked for being non-empty. If fm_test_tmproot fails (mktemp -d error, full or unwritable TMPDIR, registry write failure), line 8 leaves TMP_ROOT empty. The script runs underset -uonly, so it carries on and still installs the trap. At exit the pattern becomes "/", which matches almost every process on the host. On a dev box that includes the user's shells, editors and the shared no-mistakes daemon, which the intent says must never be stopped. Fix: exit right after line 8 when TMP_ROOT is empty ([ -n "$TMP_ROOT" ] || exit 1), and/or return early from cleanup_arm_fixtures when TMP_ROOT is empty or is not an existing directory, before the ps sweep.bin/fm-wake-lib.sh:740- The new absent-lock refusal changes what happens when a lock is released between fm_lock_try_create failing and the existence check. Before this change, that acquirer fell through the stale path and took the lock: an empty pid is not alive, fm_path_age returned 999999, so it stole and recreated the lock. It now returns 1 with FM_LOCK_HELD_PID empty. Waiting callers absorb this on their next 100ms retry. One-shot callers treat it as 'held'. For example, bin/fm-watch.sh:745 prints 'watcher: already running' and exits 0 even though the previous watcher has just released the lock. A caller that checked a moment earlier, while the holder was still alive, would have got the same answer, so this is not a new kind of failure. The window is slightly wider than before. No action needed. If it ever matters, one fm_lock_try_create retry when the lock is absent but its parent directory exists would restore the old behavior without bringing back the recursion.bin/fm-wake-lib.sh:845- fm_lock_acquire_wait can now return 1, and several callers that run underset -uonly ignore that status and carry on as if they held the lock. These are bin/fm-lock.sh:76 (sets CLAIM_LOCK_HELD=1), bin/fm-wake-drain.sh:217 and :230 (set DRAIN_LOCK_HELD=true), bin/fm-watch.sh:524, bin/fm-afk-return.sh:232, and bin/fm-x-lib.sh:942, :969 and :988. I checked each one. The next step touches only the removed directory and fails closed (write, mktemp or[ -f meta ]errors), and fm_lock_release checks the pid, so nothing is corrupted. This matches the author's audit. The one remaining exposure needs the state dir to be recreated between the failed wait and the next write. fm-lock.sh's ownmkdir -p "$STATE"in a second session could do that, and two sessions could then both write and verify state/.lock without the claim mutex. That is far-fetched. If the fleet lock should fail closed literally, the fix is a one-line|| exit 1at bin/fm-lock.sh:76. I am noting this as a tradeoff, not a blocker.🔧 Fix: Guard test reaper, retry released locks, fail-closed claim mutex
1 info still open:
tests/fm-pi-watch-extension.test.sh:20- The fix round's new[ -d "$TMP_ROOT" ]guard means the process sweep is skipped when the suite is interrupted by INT or TERM. tests/lib.sh installstrap 'fm_test_cleanup; exit 130' INTandtrap 'fm_test_cleanup; exit 143' TERM, and this suite overrides only EXIT. On a signal, fm_test_cleanup removes TMP_ROOT first. The EXIT trap then runs cleanup_arm_fixtures, finds no directory, and skips the sweep. Before the fix round the sweep ran on this path. Fixtures that ignore signals are then left running after a Ctrl-C or a TERM: line 593 usestrap '' TERM INTand loops waiting for a release file that never arrives. The ordinaryfail/exit path is not affected. Fix: keep the guard and have the suite install its own signal traps after line 29, so the sweep runs while the root still exists:trap 'cleanup_arm_fixtures; exit 130' INTandtrap 'cleanup_arm_fixtures; exit 143' TERM.taskset -c 5 bash tests/fm-pi-watch-extension.test.shon agit archiveof base 2804ff7, with 20 busy loops pinned to the same CPU, 3 runs. Failed 3/3 withnot ok - Pi must deliver the actionable wake after bounded hung-successor recovery: expected exit 0, got 1, which is CI's failure.taskset -c 11 bash tests/fm-pi-watch-extension.test.shat target 651212b under the same 20-spinner load, 5 runs. Passed 5/5, with 30 ok lines each.Shard-4 order pinned to 4 CPUs:tests/fm-startup-network.test.sh, thentests/fm-pi-watch-extension.test.sh, 3 rounds each. On base, 5 leaked workers were still running when the Pi suite started and it failed 3/3 in the OpenCode fleet-lock case. On target, no workers remained when the Pi suite ended and it passed 3/3.Leaked-worker check aftertests/fm-startup-network.test.sh. On base, 5 detached workers were still alive and using more CPU 15 seconds later, until I killed them. On target, no workers remained within 5 seconds.8 concurrent copies oftests/fm-pi-watch-extension.test.shpinned to 4 CPUs, 3 rounds, at target. Passed 24/24.bash tests/fm-pi-watch-extension.test.shserial and unloaded at target, 5 runs. Passed 5/5 in about 10 seconds each.bash tests/fm-watcher-lock.test.shandbash tests/fm-startup-network.test.shat target. Both passed, including the 5 new regression cases.The target'stests/fm-watcher-lock.test.shandtests/fm-startup-network.test.shrun against basebin/. Both failed:acquiring or waiting for a lock in a removed directory never returned, andthe worker kept waiting on its locks after its home was removed.Compared theFM_*_MSandRETRY_LIMITassignments in the Pi suite between base and target. They are identical. The only real-time wait removed is the 120ms sleep, andgit diff --name-onlyover.piand.opencodelists no files.FM_HOMEwas unset for every run. The no-mistakes daemon was not touched. The temp base trees and the leaked base workers I created were removed, and the worktree is clean.✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.