fix(watcher): identify processes by kernel start ticks, not drifting lstart - #5
Merged
Merged
Conversation
fm_pid_identity identified a process with `ps -p <pid> -o lstart= -o command=`. lstart is not stored by the kernel; ps computes it as boot time plus the process's start ticks. WSL2 continually re-syncs its boot-time estimate against the Windows host clock, so the same live process yields a different lstart string on successive reads. fm_watcher_lock_matches_pid then rejected a healthy live watcher, and fm-turnend-guard.sh printed "TURN WOULD END BLIND" on every turn while supervision was in fact running, leaving a genuine supervision failure indistinguishable from the noise. Identify by field 22 of /proc/<pid>/stat instead, the kernel's own start time in clock ticks since boot, which cannot drift however the wall clock moves. Field 2 (comm) is parenthesized and may contain spaces or parentheses, so the parse splits on the last ')' and takes the 20th token of the remainder rather than counting positional fields. macOS has no /proc, so the LC_ALL=C-pinned lstart form stays the fallback. The command half of the identity is read from /proc/<pid>/cmdline, so the Linux path forks nothing at all. That is deliberate rather than incidental: fm-afk-launch.sh computes an identity inside its lock-acquire window, before its cleanup trap is installed, and a ps fork there widened that pre-existing window enough to make tests/fm-afk-launch.test.sh fail 2 runs in 5. Fork-free, it is 0 in 8, and cheaper than the single ps fork it replaces. Persisted identities now compare through fm_pid_identity_matches, which also accepts an exact lstart-format record so a live watcher whose lock predates this change is not declared dead on format alone. Both branches are exact comparisons, so dead pids, recycled pids, and changed commands are still rejected.
kirangathani
added a commit
that referenced
this pull request
Sep 15, 2026
…ill replace as superseded (#86) * Draw an ended run's worker as building again, and a CI head the run will replace as superseded The fleet pipeline view drew two frames the captain read wrong on 2026-09-15. A worker whose run had ended `failed` (daemon shutting down) and who had been building again for 27 minutes drew `building 1h29m` finished beside `review FAIL`, with no band and no reason. The collector marked building completed the moment any run existed, whatever its status, and never put the run's error on the wire; the renderer never read run status at all. Now the building box measures the current phase only: Run #1 from dispatch, Run #N from Run #N-1's end, and after a failed or cancelled run it runs again from that run's last write while the worker's endpoint still resolves, with the running band. The run's end reason is stated on the head line in the daemon's own words. A row on Run #5 with review 31 minutes in drew the CI cell amber `4/4 your word` for PR 50's checks on head 4a22cbba, pushed by a cancelled earlier run, and the header counted it ready to merge. Now the collector reads GitHub's head beside the rollup and the run's own head from the daemon, and when a live run past its rebase has a different head it compares the two in the run's own worktree: merge-base against the default branch for "main moved", patch-ids for "new branch commit". The cell draws yellow with those sentences wrapped whole, the checked commit rides the facts line, and the header does not count it. The daemon's base_sha is the previous run's head, not a main base, so it is not consulted. Colour now encodes the verdict alone: every finished box and a CI cell whose checks passed on the head that will land are the runner band's centre green from one named token; "your word" moves to the pre-merge box; Run #N is dim rather than red. The pipeline block gains a fifth detail row because the two sentences wrap to five fifteen-column rows and widening the cell by one column leaves them at five. Tests reproduce both frames from the collector output captured that day and build the superseded fixture with real git so merge-base and patch-id are the tool's own answers. * Tolerate a runs table without worktree_dir, and keep the run counter red The merge gate re-runs main's own copy of each test file against the branch. Main's collector test builds its fixture database from a schema trimmed to the columns the collector read at the time, which has no worktree_dir; the run index now names that column, so against that fixture SQLite refused the whole statement, no run resolved for any task, and every run-dependent assertion in the file failed or was never reached. With only the column added to the fixture, all 61 of main's assertions pass, so the collector now reads the table's columns once and selects worktree_dir only where it exists. The daemon trails its current release by several minor versions, so a column one version records is not one every database carries. The run counter goes back to red, main's behaviour. Painting it dim came from the scout report's own recommendation, not from a captain ruling, and the captain asked for it back. Main's counter test is restored byte for byte. The three main assertions that expect "your word" inside the GITHUB CI cell remain superseded under the captain's ruling that the cell's colour is its verdict alone; their replacements on this branch assert the ruled behaviour.
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
Make firstmate's watcher process-identity check immune to WSL2 wall-clock drift, so bin/fm-turnend-guard.sh stops falsely reporting a healthy live watcher as dead.
The bug: fm_pid_identity in bin/fm-wake-lib.sh identified a process with 'ps -p -o lstart= -o command='. lstart is not stored by the kernel; ps computes it as boot time plus the process's start ticks. WSL2 continually re-syncs its boot-time estimate against the Windows host clock, so the SAME live process yields a DIFFERENT lstart string on successive reads. fm_watcher_lock_matches_pid then rejected the live watcher and the turn-end guard printed 'TURN WOULD END BLIND' on every turn while supervision was actually running, making a genuine supervision failure indistinguishable from constant noise. Verified empirically on this machine against a real bin/fm-watch.sh sampled 40 times over 60s: the old form produced 3 distinct identities for one never-restarted process, the new form produced 1, and fm_watcher_lock_matches_pid now reports LIVE WATCHER RECOGNISED.
Deliberate decisions a reviewer reading only the diff would not know:
Identity is keyed on field 22 (starttime, clock ticks since boot) of /proc//stat. Ticks since boot cannot drift however the wall clock moves. The parse deliberately does NOT use a positional awk '$22': field 2 (comm) is parenthesized and may itself contain spaces or parentheses, which shifts every positional field. It splits on the LAST ')' and takes the 20th whitespace-separated token of the remainder. This was an explicit requirement and is covered by tests with a comm containing a space and one containing a ')', both synthetic stat lines and a real process.
macOS has no /proc and firstmate supports macOS, so the existing 'ps -o lstart=' path is preserved as the fallback, along with its LC_ALL=C pinning and the locale-invariance reason recorded in the original comment. A test simulates an unreadable /proc//stat by overriding the reader.
Back-compat was a required design decision with two acceptable options. I chose exact dual-format acceptance in a new fm_pid_identity_matches: if the recorded string is not in the new format, the legacy lstart form is re-derived and compared exactly. Rationale: both branches remain EXACT comparisons, so the check is never more permissive than the one it replaces (dead pids, recycled pids and changed commands are all still rejected), while a pre-upgrade lock on a non-drifting host keeps matching perfectly. On WSL2 a legacy record may still fail to re-derive, but that host was already broken before this change, so it is not a regression there, and the record self-heals at the owner's next restart. I deliberately did NOT have readers rewrite another process's lock file, since that would be a write from a non-owner into someone else's lock.
The three call sites that compare a PERSISTED identity now route through fm_pid_identity_matches rather than bare string equality (fm_watcher_lock_matches_pid in the lib, fm_afk_launch_lock_owned in bin/fm-afk-launch.sh, daemon_pid_matches in bin/fm-afk-start.sh). This is centralisation in the primitive's owner, not scattering per-caller logic. The site in fm-afk-launch.sh around line 578 that compares two identities both freshly computed in the same run was deliberately left as plain equality: there is no persisted record there, so no format-skew is possible.
The command half of the identity is read from /proc//cmdline rather than forking ps. This is deliberate, not incidental. bin/fm-afk-launch.sh:605 computes an identity inside its lock-acquire window, BEFORE its cleanup trap is installed at line 606 - a pre-existing race that is out of scope for this task. An added ps fork there widened that window enough to make tests/fm-afk-launch.test.sh fail 2 runs in 5 (baseline main: 0 in 3). Fork-free it is 0 in 8, and cheaper than the single ps fork it replaces. The underlying trap-ordering gap is left untouched on purpose: the task scope explicitly said not to change the lock protocol or anything else about supervision.
Two errexit hazards were closed deliberately: fm_pid_identity_matches spells its early return as a full 'if' rather than '[ ... ] && return 0', and fm_pid_start_ticks appends '|| true', because bin/fm-afk-start.sh runs under 'set -e'. The stderr redirect in the /proc reads is placed BEFORE the input redirect so a dead pid's missing stat file fails silently (redirections apply left to right).
Scope boundaries honoured: the lock protocol, the beacon mechanism and everything else about supervision are unchanged. Upstream firstmate has its own fix for this (PR 752 / issue 433) but this fork is 24 commits behind and that sync is a separate stalled task, so this was implemented directly rather than ported; any later conflict is the sync task's problem.
AGENTS.md was deliberately NOT edited: per the binding firstmate-coding-guidelines decision tree this is mechanics (script header comment) plus an incident record (docs/), and adding it to AGENTS.md would violate the size-discipline rule. The incident evidence was recorded in docs/turnend-guard.md with date, exact commands and exact output, as that document owns the consuming contract.
Test status: bin/fm-lint.sh clean. Full suite green except two failures I verified on a clean checkout of main before attributing them - tests/fm-session-start.test.sh ('MISSING diagnostic did not appear at all') fails 3/3 on baseline main, and tests/fm-watch-checkpoint.test.sh is flaky at 1/3 on baseline main. Neither is caused by this change.
What Changed
fm_pid_identityinbin/fm-wake-lib.shnow derives a process identity from field 22 of/proc/<pid>/stat(start time in clock ticks since boot) plus/proc/<pid>/cmdline, instead ofps -o lstart= -o command=.lstartis computed as boot time plus start ticks, and WSL2 re-syncs its boot-time estimate against the Windows host clock, so the same live watcher yielded different identity strings seconds apart andbin/fm-turnend-guard.shreported a healthy watcher as dead. The stat parse splits on the last)rather than using a positional field, so acommcontaining spaces or parentheses cannot shift the read;ps -o lstart=is preserved (with itsLC_ALL=Cpin) as the fallback for hosts without/proc, such as macOS.fm_pid_identity_matches, which compares a live pid against a persisted identity and, for a record still in the legacylstartformat, re-derives and compares that form exactly. Both branches stay exact comparisons, so dead, recycled, and changed-command pids are still rejected. The three call sites that read a persisted identity route through it:fm_watcher_lock_matches_pid,fm_afk_launch_lock_ownedinbin/fm-afk-launch.sh, anddaemon_pid_matchesinbin/fm-afk-start.sh.tests/fm-watcher-lock.test.shgains coverage for identity stability across repeated reads of one live pid, the odd-commstat parse (synthetic lines and a real process nameda b)c), thelstartfallback when/proc/<pid>/statis unreadable, legacy-record matching, and rejection of dead/recycled/start-marker-mismatched pids; the existing locale-invariance test now stubs the/procreader so it actually exercises theLC_ALL=Cpin on Linux.docs/turnend-guard.mdrecords the incident with the measured 40-sample evidence and points its Tests section at the new regression.Risk Assessment
✅ Low: The follow-up commit is a test-only change that correctly closes the coverage regression flagged in round 1 (forcing the lstart fallback so the LC_ALL=C pin is actually exercised, plus a guard that fails loudly if the stub stops working), and the underlying identity change remains well-bounded, centralised in its owning library, and conformant with every stated acceptance criterion.
Testing
Completed 1 recorded test check.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed ✅
tests/fm-watcher-lock.test.sh:683- test_pid_identity_is_locale_invariant no longer exercises the LC_ALL=C pin on Linux: fm_pid_identity now takes the /proc path there and never calls ps, so all three sampled identities are the locale-independent 'proc-starttime:...' form and the assertions hold trivially. Dropping the pin from fm_pid_identity_lstart would regress the ko_KR watcher-rejection bug undetected on Linux CI. The test comment is also stale ('the fix pins LC_ALL=C inside fm_pid_identity' - the pin is now in fm_pid_identity_lstart). Fix by forcing the fallback the way test_pid_identity_falls_back_without_proc_stat does (stub fm_pid_start_ticks to return 1), or by sampling fm_pid_identity_lstart directly.bin/fm-wake-lib.sh:115- The new identity has no boot-unique component, so a persisted record is only meaningful within the boot that wrote it; lstart encoded an absolute wall-clock instant and did not have this property. Intra-boot it is airtight (a recycled pid necessarily has larger start ticks). Across a reboot, pid + ticks + cmdline can in principle repeat. Heavily gated in practice: a colliding process must have an identical cmdline, so it would be a legitimate same-role process that has already rewritten its own lock, and the watcher path also requires a fresh beacon. fm_afk_launch_lock_owned and daemon_pid_matches have no second gate. Noted as a property of the new format; adding /proc/sys/kernel/random/boot_id would close it at the cost of another format change.🔧 Fix: force lstart fallback in locale-invariance identity test
✅ Re-checked - no issues remain.
command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix: silence SC2016 on intentional inner-shell script body
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.