Skip to content

fix: never treat an unmatched process name as a dead session lock - #157

Open
quinnbot-ai wants to merge 2 commits into
mainfrom
fm/fm-liveness-probe-antipattern-audit
Open

fix: never treat an unmatched process name as a dead session lock#157
quinnbot-ai wants to merge 2 commits into
mainfrom
fm/fm-liveness-probe-antipattern-audit

Conversation

@quinnbot-ai

@quinnbot-ai quinnbot-ai commented Aug 20, 2026

Copy link
Copy Markdown
Owner

The audit

Swept bin/ for pgrep/ps-pattern liveness checks. Classification first, as briefed:

Load-bearing, and wrong (fixed here) - 1

bin/fm-lock.sh decided whether to reclaim the fleet session lock from
fm_harness_pid_alive, which is kill -0 AND a command-name pattern match.
A live session whose process name misses the pattern reported "not a live
harness", and the lock was taken while it was still running. Two sessions then
both hold the fleet lock: both drain the wake queue, both spawn, both merge.
Same shape as the three false readings that prompted this audit - a pattern too
specific to match a wrapped invocation, and an over-specific pattern returning
zero while the work was demonstrably running.

Reproduced against a genuinely live process, so kill -0 succeeded and the name
was the only thing deciding:

lock before: 58298   (a real, running process)
lock acquired: harness pid 58976
lock after : 58976   <- holder still alive, its lock was taken

Load-bearing and already correct - 2 (left alone)

  • bin/backends/tmux.sh fm_backend_tmux_agent_state already folds "no name
    matched" into ambiguous/other rather than dead, and documents why: only
    dead/missing license a relaunch. This is the model the fix follows.
  • bin/fm-remote-job-reap-orphans.sh scans by pattern to pick kill targets, so
    a miss is the safe direction, and it already re-reads the live command, checks
    pgid, and excludes self and ancestors - which is the "matched the probing
    shell's own command line" failure, already handled.

Already preferring a stronger signal - 1 (left alone)

bin/fm-afk-start.sh daemon_pid_matches reads a recorded pid-identity file
first and only falls back to a command pattern when it is absent.

Cosmetic - 1 (annotated, not rewritten)

bin/fm-lint.sh pgrep -x shellcheck is concurrency telemetry that nothing
branches on. Its comment now states that the count is a lower bound and that a
zero means "none matched the name", not "none running".

Everything else matching ps - in bin/ is a recorded-PID probe (ps -p "$pid"),
often bound to a start-time identity - already the strong form the brief asks for.

The fix

The reclaim decision no longer rests on the name. fm_session_lock_holder_state
asks four questions in order of strength and reports the ambiguous case
separately:

verdict meaning action
live alive and identifiable as a harness refuse
unidentified alive, unrecognized, unaccounted for refuse, naming the ambiguity
self-inner alive, unrecognized, our own ancestor claim - a correction, not a takeover
stale gone, or too young to have written the lock claim

Two structural signals keep unidentified from wedging a home permanently:

  • A pid younger than the lock it names cannot have written it, because the
    writer had to die before its number could be reissued. Uses ps -o etime=,
    whose [[dd-]hh:]mm:ss form is locale-invariant, unlike lstart.
  • A holder in this process's own lineage is our own session naming an inner pid.
    This is what the Cursor park fixture does, and dropping it would have broken a
    legitimate reclaim - caught by tests/fm-cursor-primary.test.sh during review.

Both are kernel facts rather than vendor strings, which is the direction
firstmate-coding-guidelines asks harness-dependent checks to move.

The name pattern is untouched and still carries positive matches; it simply no
longer decides the negative case alone.

The pane trap

Noted in the brief, and it is the same failure one level up: a pane's last line
is the composer prompt in every harness that has one, identical whether the
worker is idle or thirteen minutes into a turn. The activity indicator sits
above it, so a tail cut at the bottom line has cut off the only part carrying
the answer. stuck-crewmate-recovery step 1 now says so and points at
bin/fm-crew-state.sh for the signal that actually answers the question.

Verification triple

Stock macOS bash 3.2.57 (the CI compatibility target).

point result
base 8a4036c9 CI run 32421659419 green, 13/13 jobs
broken head exit 1 - not ok - fm-lock.sh acquired a lock still held by the live process 69846
fixed head exit 0 - 7 assertions

The broken-head point has the classifier present and only the decision site
reverted to the old boolean, so the failure is the lock-theft behavior itself
rather than a missing symbol. An earlier attempt that simply removed the new
code failed on command not found, which would have proved nothing.

Also green: fm-session-lock-ancestry 7, fm-watcher-lock 31,
fm-turnend-guard 64, fm-watch-arm 14, fm-cursor-primary 26,
fm-secondmate-harness 49, fm-claude-stop-autoarm-live-e2e.
bin/fm-lint.sh, bin/fm-test-run.sh --check-coverage, and
bin/fm-doc-audience-check.sh all clean.

Three repro attempts before the valid one were themselves invalid - two to zsh
eating :c/:a as parameter-expansion modifiers, one to a fake ps that never
answered etime. Worth stating in a PR about probes that look authoritative and
are not.

CI on this branch

All 12 CI jobs green on d9db7e21.

Two failures on the first push were both fixture problems, not the change:
shellcheck SC2217 on a meaningless </dev/null, and fm-on.test.sh exiting 1
from its own temp-dir cleanup race (rm: cannot remove .../remote-jobs/worker.lock: Directory not empty) AFTER printing ALL TESTS PASSED. That suite references
nothing this branch touches and passes locally; it went green on the re-run.

The one remaining red check is PR must be raised via no-mistakes, which is
structural rather than a defect - see below.

Not done

The no-mistakes gate. This task was dispatched direct-PR with an explicit
instruction not to run the no-mistakes pipeline, but this repo's CI requires the
pipeline signature in the PR body. Only that pipeline can write the marker, so
the check cannot pass on a direct-PR and I have not hand-written the marker.
Whoever merges needs to decide between re-running this through no-mistakes and
overriding that check.

No new live-harness-optin guard. This change reduces reliance on vendor
process names rather than adding a check that reads one, so the portable
regression with real processes is the proportionate coverage. Flagging it since
firstmate-coding-guidelines asks for both when a check is harness-dependent.

🤖 Generated with Claude Code

https://claude.ai/code/session_015rQz5F9jXAYQFngFeCn7Q6

QuinnBot added 2 commits August 20, 2026 15:25
`fm_harness_pid_alive` answers "is this pid a process I can RECOGNIZE as a
harness?". A name pattern can only produce evidence FOR a match, so its
no-match result is ambiguous: the pid may be dead, or it may be a live
session the table cannot name - launched through a wrapper script, renamed
by its installer, or running a harness not yet in FM_HARNESS_RE.

`bin/fm-lock.sh` collapsed that ambiguity into "dead" and reclaimed the
lock, so a session whose process name missed the pattern lost its home to a
second session while still running. Both then hold the fleet lock: both
drain the wake queue, both spawn, both merge.

The reclaim decision no longer rests on the name. `fm_session_lock_holder_state`
asks four questions in order of strength and reports the ambiguous case
separately, so callers refuse and say why instead of taking a live home:

  live          alive and identifiable as a harness    -> refuse
  unidentified  alive, unrecognized, unaccounted for   -> refuse, naming it
  self-inner    alive, unrecognized, our own ancestor  -> claim (a correction)
  stale         gone, or too young to have written it  -> claim

Two structural signals keep `unidentified` from wedging a home. A pid younger
than the lock it names cannot have written it, so a reissued number returns to
`stale`; and a holder in this process's own lineage is our own session naming
an inner pid, not a competing one. Both are kernel facts rather than vendor
strings, which is the direction the harness-dependent-check rule asks for.

Also: state the lint shellcheck probe's count as the lower bound it is, and
warn in the recovery skill that a pane's bottom line is the composer prompt
whether the worker is idle or mid-turn - the activity indicator sits above
it, so a truncated tail cuts off the only part carrying the answer.

Verification triple on stock macOS bash 3.2.57:
  base 8a4036c        CI run 32421659419 green (13 jobs)
  broken head          exit 1 - "fm-lock.sh acquired a lock still held by
                       the live process", with the decision site reverted
                       to the old boolean and everything else in place
  fixed head           exit 0 - 7 assertions

Neighbouring suites green: session-lock-ancestry 7, watcher-lock 31,
turnend-guard 64, watch-arm 14, cursor-primary 26.
…xplicitly

Two fixture defects the Linux lane surfaced.

shellcheck SC2217: `</dev/null` on the `sleep` fixture is meaningless - sleep
reads no stdin, and the command-substitution hang it was meant to avoid came
from stdout, which the same line already redirects.

The etime parser cases passed FM_TEST_ETIME as a prefix assignment on a
FUNCTION call, which leaves whether the value reaches the fake `ps` grandchild
up to bash's temporary-environment semantics. Set it on the `bash -c` child
instead, where it is unambiguously in the environment the fixture inherits.
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.

1 participant