Skip to content

fix(bin): stop leftover away daemons and make process identity independent of the host time zone - #6414

Open
sgerlach wants to merge 13 commits into
kunchenguid:mainfrom
sgerlach:fm/fm-orphan-away-daemon-upstream
Open

sgerlach wants to merge 13 commits into
kunchenguid:mainfrom
sgerlach:fm/fm-orphan-away-daemon-upstream

Conversation

@sgerlach

@sgerlach sgerlach commented Oct 2, 2026 •

Copy link
Copy Markdown

This PR fixes worker results that stop reaching the attended firstmate.
It has two parts: an away-mode daemon that outlives away mode no longer takes worker events, and a host time-zone change no longer makes a live lock owner look dead.

Part 1: the leftover away daemon

On 2026-10-01, worker results stopped reaching the attended firstmate.
An away-mode supervise daemon outlived away mode, with state/.afk and its lock both gone.
The attended arm attached to the watcher that daemon owned, and the daemon drained and acknowledged every worker event into state/.subsuper-escalations, which nobody reads while away mode is off.

  • bin/fm-supervise-daemon.sh exits by itself, before it drains another wake, when state/.afk is gone or when it no longer holds its lock.
    On exit, it releases only the lock and pid file that it owns.
  • The new bin/fm-afk-daemon-lib.sh finds every live away daemon of a home.
    The lock proof names a live pid whose recorded identity matches.
    The watcher proof names the live parent of this home's watcher when that parent runs the daemon script, so it also finds a daemon that lost its lock.
    The command proof keeps a lock whose live pid runs the daemon script, but it never allows a signal.
  • bin/fm-afk-start.sh never deletes the lock of a live daemon, and bin/fm-afk-launch.sh stop signals every daemon that the lock or watcher proof names.
  • bin/fm-watch-arm.sh and the Codex checkpoint bin/fm-watch-checkpoint.sh never attach to a watcher whose parent is an away daemon while state/.afk is absent.
    They stop that daemon and take over with a fresh cycle.

Part 2: lock owner identity across a host time-zone change

fm_pid_identity recorded ps -o lstart, which macOS renders in the local time zone.
After the host time zone changed, 11 lock users read a live owner as dead: the watcher lock, the away-daemon lock (two readers), the away-mode launch lock, auto-arm claims, wake grants, process-event claims, supervision-host records, the main-session key, pending-reply senders, and remote job owners.
This is how the daemon in part 1 lost its lock: the away-mode return skipped the live daemon, and the next entry deleted its lock.

  • The new bin/fm-pid-identity-lib.sh owns fm_pid_identity.
    Its ps form reads lstart under TZ=UTC0 and records it with an lstart-utc= key.
    The Linux /proc form was already independent of the time zone and does not change.
  • fm_pid_identity_matches is the one comparison rule, and every lock user calls it.
    A keyed record must match exactly.
    A record from a build before this fix matches only the same command line, started at the same instant, at a whole-quarter-hour offset between -12:00 and +14:00.
    So a live owner from before the upgrade survives it and any later zone change, while a dead or reused pid still does not match.
  • The extension host has the same rule in bin/fm-pid-identity.mjs.
    The remote job worker, teardown, and the labs read lstart in UTC too, and the remote job worker and the labs still accept a record that a build before this change wrote.

Why the two parts belong together

Part 2 removes the cause of the lost lock in part 1, and part 1 removes the damage when a daemon outlives away mode for any other cause.
Both parts change the same check: the away-daemon lock proof in bin/fm-afk-daemon-lib.sh now calls fm_pid_identity_matches, so a lock that a daemon wrote before the upgrade still proves that live daemon after a zone change.
The drifted-watcher proof from part 1 stays for a record that no match can bridge, such as a pre-upgrade record written under another locale.

Test evidence

Live runs drove the real scripts in a disposable lab home on a private tmux socket, on macOS.

Part 1: the leftover away daemon

  • On the base code, a leftover daemon with no lock and no state/.afk takes and acknowledges a worker event (incident reproduced, base repro).
  • On this branch, the daemon exits by itself when away mode ends, and the next worker event reaches the attended drain (exit, event delivery).
  • The arm and the Codex checkpoint stop a leftover daemon and take over, including from another time zone and with a stale watcher beacon (arm, checkpoint, cases tz-arm, tz-stop, stale-arm, host-stale).
  • The return stops a daemon that lost its lock, through the watcher it runs (lockless stop).
  • While away mode is on, the arm attaches to the legitimate daemon's watcher and stops nothing (adversarial guard).
  • Suites: fm-afk-launch, fm-watch-arm, fm-watch-checkpoint, and fm-wake-daemon-lifecycle-e2e pass on the combined code.

Part 2: lock owner identity across a time-zone change

  • On the base code, a reproduction reads a live owner as dead after a zone change for all 11 lock users; on this branch, it reads none as dead.
  • A live process keeps one identity in Denver, Tokyo, London, and UTC, while the base code gives four different ones; a record from the base code, written in Denver, still matches in Tokyo, Kolkata, Eucla, and UTC (identity across zones).
  • Records that name another process do not match: a start one second or one hour off, another command, a pre-upgrade record off a quarter hour or outside -12:00..+14:00, an empty record, and an exited process (mismatch guards).
  • Away mode entered in Denver and refreshed and ended in Tokyo keeps the live daemon, starts no second one, and then stops it; the base code reads the live daemon as dead and leaves a stale lock (branch, base).
  • A daemon that the base code started in Denver is kept and then stopped by this branch in Tokyo after the upgrade (upgrade).
  • The shell rule and the Node mirror give the same verdicts (parity).
  • fm-live-lab down and the fm-herdr-lab.sh viewer stop still find processes from a lab started before the update, from another zone, and a reused lab root pid cannot adopt a new process tree (tests/fm-live-lab.test.sh, tests/fm-herdr-lab.test.sh).
  • Each lock user has a regression test that records an identity in one zone and checks it in another, for the new and the pre-upgrade record forms, and all of them pass on this branch.
    Against the base code, the watcher lock, away-daemon return, wake grant, remote job, and extension capture cases fail.
    Without the fm_pid_identity_matches call in the away-daemon lock proof, the combined branch fails its pre-upgrade cases: the entry reads the live daemon as dead, and the return skips it.
  • Suites that either part touches pass on the combined code, among them fm-watcher-lock, fm-claude-stop-autoarm, fm-wake-queue, fm-procevent, fm-pending-reply, fm-remote-job, fm-supervision-host, fm-extension-binding, fm-teardown, and fm-live-lab.

CI on this fork PR

This PR comes from a fork, so GitHub holds its workflow runs (CI and Require no-mistakes) until a maintainer of this repository approves them.
Until then, those checks show action_required with no jobs, and no check has run on the code.

@greptile-apps

greptile-apps Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[High risk] Refactors daemon lifecycle and process identity tracking across shell scripts.

The PR does not appear safe to merge while the outstanding legacy-identity ownership issue remains.

Reviews (9) · Last reviewed commit: "no-mistakes(ci): I fixed the new Greptil..."

Comment thread bin/fm-afk-daemon-lib.sh Outdated
Comment thread bin/fm-watch-checkpoint.sh Outdated
@greptile-apps

This comment has been minimized.

Comment thread bin/fm-wake-lib.sh Outdated
Comment thread bin/fm-wake-lib.sh Outdated
…ker events

An away-mode supervise daemon kept running after away mode ended, with
state/.afk and its singleton lock both gone. Its watcher child was the home's
only watcher, the attended primary's arm attached to it and followed its cycles
forever, and the daemon drained and acknowledged every worker event into
state/.subsuper-escalations, whose injection is gated off while away mode is
off. Done handoffs and decisions never reached the attended firstmate.

How the daemon lost its lock: the lock's recorded identity is fm_pid_identity,
whose macOS form renders `ps -o lstart` in the local time zone. After the host
time zone changes, a live daemon no longer matches it. The return
(fm-afk-launch.sh stop) then saw no daemon and cleared state/.afk without a
signal, and the next entry (fm-afk-start.sh) treated the live daemon as a
reused pid, deleted its lock, and started a second daemon. The first daemon
kept running without its lock, and nothing made it exit.

The fix:
- bin/fm-supervise-daemon.sh exits on its own, before it drains another wake,
  once state/.afk is gone or once it no longer holds its lock, and it removes
  only its own pid file and instance record.
- bin/fm-afk-daemon-lib.sh (new) owns finding a home's live daemons: a
  per-instance record with a UTC-rendered identity, the lock, and the parent
  of this home's verified watcher.
- bin/fm-afk-launch.sh stop signals every live daemon those proofs name,
  including one that lost its lock.
- bin/fm-afk-start.sh never deletes the lock of a live process that runs the
  daemon script.
- bin/fm-watch-arm.sh never attaches to a watcher whose parent is a daemon
  while state/.afk is absent. It stops that daemon and owns a fresh cycle,
  home-scoped and without pkill.

The repeated "check: procevent lavish ... 14" escalation had the same cause:
fm-procevent.sh reconcile republishes an unhandled result on every watcher
cycle until `handled` runs, and only the attended firstmate runs it. The
lifecycle regression covers it, because the daemon no longer drains wakes
after away mode ends.

Harness reach: the takeover lives in bin/fm-watch-arm.sh, so it covers the
Claude Stop auto-arm, the supervision host, the Cursor turn-end guard, and the
Pi and omp extensions. The Codex checkpoint (bin/fm-watch-checkpoint.sh) runs
fm-watch.sh directly, so a daemon from before this fix on a Codex primary makes
the checkpoint report that a watcher already runs outside it instead of taking
over. A daemon that runs this code exits on its own on every harness.

Separate findings, reported here and not fixed:
- A healthy away daemon (state/.afk present) also buffers the same unhandled
  process-event result again on every wake; escalate_add dedups only unknown
  wakes. A 40-second isolated run buffered 76 lines for 12 distinct results.
- fm_pid_identity is time-zone-sensitive for every caller on macOS (watcher
  lock, procevent claims, wake grants, fm_afk_daemon_owns_supervision). Pinning
  TZ=UTC0 in its ps form is a one-line fix, but it invalidates every identity
  recorded before the upgrade, so it needs its own migration.
After a takeover the stopped watcher records its downtime, so the fresh
watcher always reports check: rearm-resurface for that gap. The arm test
treated that wake as optional and decided too early whether it came: under
CPU load it read the watcher lock, or appended the done handoff, inside the
window before the wake, and failed 22 of 24 runs. It now waits for the arm
to report the gap wake, acknowledges it, and starts the next arm (24 of 24
runs passed under the same load).

The checkpoint test accepted a quiet 5 s timeout for the same wake, which
leaves the downtime record for its next run. It now expects the gap wake,
and both checkpoint runs get headroom over a loaded takeover; each returns
as soon as its wake arrives.
…quired with a 0s run time. A maintainer must approve workflow runs for a fork PR, so no code change can fix those two checks. Greptile Review failed on two P1 comments. The user chose to fix only g2 ("Host checkpoint skips takeover"), so I fixed g2 and left g1 (the time-zone identity drift) unchanged. Changes, not committed: - bin/fm-watch-arm.sh: The leftover-daemon takeover now runs before any health or beacon check. It runs in plain arm, --restart, and --take-over modes. It does not run in --stop or --handling-delivered mode, or while state/.afk exists. take_over_from_leftover_daemon now takes the holder identity as a second argument. When it re-runs the arm, it passes the original arguments and keeps the FM_WATCH_ARM_TOOK_OVER loop guard. I removed the duplicate check at the plain healthy-attach point because the early check covers it. The attach-path checks for later successors stay. I updated the header. - bin/fm-watch-checkpoint.sh: I moved the takeover block and the two library source lines it needs above the `fm_supervision_host_enabled` branch. A Codex home that runs the supervision host now also stops a leftover daemon. I updated the header. - docs/watcher-continuity.md and docs/architecture.md: I patched the existing takeover sentences. - Tests: I added a test to tests/fm-watch-arm.test.sh for a leftover daemon whose watcher beacon is stale (watcher poll 60s, arm grace 3s, stall bound 600). I added a test to tests/fm-watch-checkpoint.test.sh for a supervision-host home with a real host, a fake codex, and a stale leftover watcher. The leftover-daemon fixtures now take an optional watcher poll value. Verification: - With both old scripts, the new host checkpoint test fails ("lock held by live pid ... heartbeat is stale"). The arm fix alone makes it pass, and the checkpoint move alone also makes it pass. The new arm test fails on the old arm and passes on the new arm. - The scratch repro cases stale-arm and host-stale, run with TZ=UTC for both processes, now show the daemon and its old watcher gone and a fresh attended cycle. TZ must match because the repro starts the daemon under TZ=${TZ:-UTC}, and a mismatch triggers the g1 identity drift, which is out of scope. - tests/fm-watch-checkpoint.test.sh passes completely (11 ok). tests/fm-afk-launch.test.sh passed (rc=0) on an earlier run. That run used the first version of my changes, before the test-race fix. - When I called StructuredOutput, three runs were not finished. The full tests/fm-watch-arm.test.sh rerun after the race fix had no result yet. tests/fm-supervision-host.test.sh had 45 ok and no failures, but had not ended. bin/fm-lint.sh on the changed files had no result yet. - The first full arm run found a race in my new test: the ledger check ran before the arm wrote its row. I moved that check after the arm exits. - Plain shellcheck reported one info, SC2016, on the new host test's single-quoted `codex -c` script. The existing host test uses the same pattern
…ts root cause, as the user selected. CI and Require no-mistakes show action_required because a maintainer must approve workflow runs for a fork PR. No code change can fix them, and the user chose to ignore them. Root cause: the ps fallback in fm_pid_identity (bin/fm-wake-lib.sh) prints lstart in the local time zone on macOS. One live process showed "Fri Oct 2 16:56:20 2026" under TZ=UTC and "Sat Oct 3 01:56:20 2026" under TZ=Asia/Tokyo. So after a time-zone change, a live watcher no longer matches its watcher lock, and a live daemon no longer matches its lock. Changes (not committed): - bin/fm-wake-lib.sh: the ps fallback now pins TZ=UTC, next to the existing COLUMNS and LC_ALL pins, with a comment. - bin/fm-afk-daemon-lib.sh: I removed the header sentence that said a time-zone change breaks the lock match. - docs/architecture.md: I removed the "lock identity drifted after a time-zone change" clause. - tests/fm-watcher-lock.test.sh: new test test_pid_identity_is_time_zone_invariant, registered in the run list. It checks a ps stub and the real ps under TZ=UTC and TZ=Asia/Tokyo. Verification: - The new test failed before the fix ("varied with exported TZ") and passed after it. - tests/fm-watcher-lock.test.sh: rc=0, 44 ok. - tests/fm-watch-arm.test.sh: rc=0, 31 ok. - tests/fm-watch-checkpoint.test.sh: rc=0, 11 ok. - bin/fm-lint.sh on the changed shell files: rc=0. One earlier lint run returned rc=1 only because I passed docs/architecture.md to ShellCheck by mistake. Notes: - Upgrade effect: a lock written by the old code on a host that is not on UTC will not match once after the upgrade. A watcher that does not match gets restarted. A daemon that loses its lock exits on its own because of this PR. - fm_pending_reply_pid_identity (bin/fm-pending-reply-lib.sh:1026) uses the same ps form. It is a different subsystem and outside g1, so I did not change it
…used. CI and Require no-mistakes show action_required because a maintainer must approve workflow runs for a fork PR. No code change can fix those two checks, and the user chose to ignore them in earlier rounds. Root cause: commit 3205c260 pins TZ=UTC in the ps fallback of fm_pid_identity. After that change, the two time-zone fixtures in tests/fm-afk-launch.test.sh failed. Each fixture called fail when the identities under TZ_BEFORE and TZ_AFTER were equal, because the fixture expected them to differ. That was Greptile's second P1 comment. Change (not committed): - tests/fm-afk-launch.test.sh: Both tests now fail when the identity varies with the time zone, not when the identity stays the same. They record the daemon lock identity under TZ_BEFORE and run start or stop under TZ_AFTER, as before. They now check that the live daemon keeps its lock and that stop still reaches the daemon. I updated the fixture comment and the pass messages, which described the identity drift that no longer exists. Verification: - tests/fm-afk-launch.test.sh: rc=0, 99 ok, 0 failures. Both "time-zone start" and "time-zone stop" pass. - bin/fm-lint.sh tests/fm-afk-launch.test.sh: rc=0. - On the code before 3205c260, the new checks fail because the identity changes with TZ. This makes the two tests regression tests for g1 at the level of fm-afk-launch. Not changed: Greptile's first P1 comment ("Pre-upgrade watcher identities stop matching"). It describes a lock that the old code wrote on a host that is not on UTC. That lock will not match one time after the upgrade. The g1 round already recorded this one-time upgrade effect, and no failing test comes from it. I made no code change for it
…I and Require no-mistakes still show action_required because a maintainer must approve workflow runs for a fork PR. No code change can fix those two checks, and the user chose to ignore them. Changes, not committed: 1. Reverts: bin/fm-wake-lib.sh, tests/fm-watcher-lock.test.sh, and docs/architecture.md are now the same as at ad376643. `git diff ad376643 -- bin/fm-wake-lib.sh` is empty, so fm_pid_identity is not changed. The time-zone fixtures in tests/fm-afk-launch.test.sh again fail when they cannot shift the identity. The time-zone sentence in the header of bin/fm-afk-daemon-lib.sh and the "lock identity drifted after a time-zone change" clause in docs/architecture.md are back. 2. g1 fix (bin/fm-afk-daemon-lib.sh): fm_afk_daemon_watcher_owner first calls fm_watcher_lock_matches_pid. If that fails, it calls the new fm_afk_daemon_watcher_drifted. That function accepts the watcher-lock pid only when all of these are true: the lock's fm-home and watcher-path match, the live command line runs the watch path, and the process started no later than the pid-identity file mtime plus 2 seconds. The start time is now minus `ps -o etime=`, and the function reads now before it calls ps. The existing owner code still requires a live parent that runs the daemon script. I updated the header to describe the proof. The ad376643 g2 fix and its tests did not change. 3. New regression tests that run the real scripts: - tests/fm-watch-arm.test.sh: test_arm_takes_over_a_leftover_away_daemon_watcher_whose_identity_drifted. The daemon starts under TZ=AAA8 and the arm runs under TZ=BBB7. The test asserts that the identity texts differ before the arm runs. If TZ gives no difference, as on Linux /proc, the test rewrites pid-identity, keeps its mtime with touch -r, and asserts again. - tests/fm-afk-launch.test.sh: unit_stop_finds_a_lockless_daemon_whose_watcher_identity_drifted. - tests/fm-afk-launch.test.sh: unit_stop_rejects_a_watcher_lock_pid_newer_than_its_identity. This test sets pid-identity to a false identity with a 2020 mtime, then asserts that stop signals neither the daemon nor its watcher. 4. Other fix: commit 61a32737 in this PR added a call to fm_test_track_watcher_state in start_watcher_standin. tests/fm-afk-launch.test.sh does not source tests/lib.sh, so each fixture printed "command not found". The stand-in stops its watcher itself, so I removed the call (removal-first rule). Verification: - Without the proof, both drift tests fail. With the proof, they pass. If I remove the start-time check, the guard test fails. - `greptile-repro.sh <worktree> tz-arm`: the daemon and the old watcher are gone, and the ledger shows reason=leftover-daemon-stopped and then a fresh cycle. `tz-stop`: the watcher proof prints daemon pid 71469 under the recording TZ and under Asia/Tokyo. No repro processes were left. - Test results, all rc=0 with no "not ok": tests/fm-afk-launch.test.sh 101 ok, tests/fm-watch-arm.test.sh 32 ok, tests/fm-watch-checkpoint.test.sh 11 ok, tests/fm-watcher-lock.test.sh 42 ok, tests/fm-wake-daemon-lifecycle-e2e.test.sh 4 ok. bin/fm-lint.sh on the changed shell files: rc=0. Not changed: Greptile's comment "Pre-upgrade watcher identities stop matching" (bin/fm-wake-lib.sh:108) was about the TZ=UTC pin. This round removes that pin, so the comment no longer applies
…quire no-mistakes show action_required because a maintainer must approve workflow runs for a fork PR. No code change can fix those two checks, and the user chose to ignore them. Change (not committed): in tests/fm-watch-checkpoint.test.sh, I added `# shellcheck disable=SC2016 # the fake harness's script expands in its own shell` directly above test_host_checkpoint_takes_over_a_leftover_away_daemon. This is the same line that the existing host test has at line 177. I changed no other file. Verification: - `shellcheck -x tests/fm-watch-checkpoint.test.sh`: rc=0. - With actionlint 1.7.12 on PATH (installed to a temp directory with bin/fm-install-actionlint.sh), bin/fm-lint.sh gives rc=0. It reports ShellCheck 0.11.0 and "3 workflow files valid". - Without actionlint on PATH, bin/fm-lint.sh gives rc=1 only because actionlint is not found. - `git diff ad376643 -- bin/fm-wake-lib.sh` is empty. - I did not run a test suite, because the change is only a comment
fm_pid_identity's ps form rendered lstart in the local time zone, so a
laptop that changed zone between recording a lock owner and checking it
read a live owner as dead: the away-mode return skipped a live daemon,
the next entry reclaimed its lock, and watcher locks, auto-arm claims,
wake grants, process-event claims, supervision-host records, pending
reply senders, and remote job owners were all exposed the same way.

The ps form now reads lstart under TZ=UTC0 and is keyed lstart-utc= in
the new bin/fm-pid-identity-lib.sh, which every identity reader shares.
fm_pid_identity_matches is the one comparison rule: a keyed record must
match exactly, and an unkeyed record written by a build before the pin
matches only the same command started at the same instant under some
whole-quarter-hour offset between -12:00 and +14:00. Live owners
recorded before an upgrade so survive it and any later zone change,
while a reused pid still mismatches unless an identical command line
restarts on that exact pid at a quarter-hour-aligned second; no build
writes unkeyed records any more, so that rule retires with the last
pre-upgrade owner.

The extension host mirrors the keyed form and the legacy rule through
bin/fm-pid-identity.mjs. It used to read ps under the system zone while
the shell recorded claims under the caller's TZ, so a TZ-exported shell
could already make it refuse a live claim. The remote job worker keys
and compares its start records the same way, teardown and the labs pin
UTC on their own lstart reads, and the duplicate
fm_pending_reply_pid_identity is gone.

The away-daemon proofs in bin/fm-afk-daemon-lib.sh use the same match.
The lock proof calls fm_pid_identity_matches, so a lock that a daemon
wrote before the upgrade still proves that live daemon after a zone
change. The drifted-watcher proof stays for a record that no match
bridges, and its tests now make that drift with a pre-upgrade record
moved off a whole quarter hour, because a zone change alone no longer
moves an identity.

Upgrade notes:
- A daemon, watcher, arm, or process-event runner started before the
  upgrade keeps its old in-memory code until it exits. New code reads
  its legacy records; an old process reading a new keyed record sees a
  mismatch, bounded because every lock acquisition it would then
  attempt runs the new on-disk code.
- fm_supervision_host_main_key changes once at the upgrade (one fresh
  engine conversation key) and is zone-stable afterwards.
- The remote job worker code identity now covers the identity library,
  so every remote worker restarts once on this upgrade.
- A remote legacy start record written under a non-C locale does not
  parse (the old reader never pinned LC_ALL), so that one upgrade-window
  case still reads as gone.
The re-arm draining fixture called the setsid binary, which macOS does
not ship, so the holder never started there and the suite failed before
its claim check. Use the perl POSIX setsid wrapper the same file already
uses for its other session leaders.
@sgerlach
sgerlach force-pushed the fm/fm-orphan-away-daemon-upstream branch from 7907297 to bf85236 Compare October 2, 2026 21:58
@sgerlach sgerlach changed the title fix(bin): stop a leftover away daemon from taking worker wakes after away mode ends fix(bin): stop leftover away daemons and make process identity independent of the host time zone Oct 2, 2026
Comment on lines +121 to +124
[ "$recorded_rest" = "$_FM_PID_LSTART_REST" ] || return 1
offset=$((recorded_seconds - _FM_PID_LSTART_SECONDS))
[ $((offset % 900)) -eq 0 ] || return 1
[ "$offset" -ge -43200 ] && [ "$offset" -le 50400 ]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Reused PID passes ownership check

If a pre-upgrade owner's PID is reused by a process with the same command at a permitted 15-minute offset, this legacy check accepts the new process as the original owner. Process-event cleanup can then send TERM and KILL to the unrelated process's entire group.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the intended trade-off of the pre-upgrade rule, so the code keeps it.
A false match needs all of these at once: the same pid, the identical full command line, a start a whole number of quarter hours apart to the second, and a record that a build before this change wrote.
Records written after the upgrade use the keyed lstart-utc= form and must match exactly, so this rule retires as the pre-upgrade records go away.
A stricter rule, where only an exact keyed match can authorize TERM or KILL, is a possible follow-up.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Given the explicit compatibility requirement for pre-upgrade records, the narrow legacy-match window, and exact keyed matching for new records, this is an intentional migration trade-off rather than an actionable defect. I’ll withdraw this finding; a stricter TERM/KILL policy can remain a follow-up once legacy records have expired.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

Comment thread bin/fm-live-lab.sh Outdated
…s stop matching", as the user selected. CI and Require no-mistakes show action_required because a maintainer must approve workflow runs for a fork PR. No code change can clear them. I made no change to the pre-upgrade match rule in bin/fm-pid-identity-lib.sh. The "Reused PID passes ownership check" comment stays for its thread. The changes are not committed. Cause: the labs record the start time of a process as a bare key: launch_start= in bin/fm-live-lab.sh, and launcher_start= and viewer_start= in the viewer record. Before this update, that value was in local time. The new code compares it exactly with a UTC value, so after a time-zone change the old records do not match. Changes: 1. bin/fm-live-lab.sh: record_launch_pid now writes launch_start_utc=. The new lab_roots function prints the recorded PIDs that still match. A launch_start_utc record must match the UTC lstart exactly. A bare launch_start record is a pre-upgrade local-time value, and lab_roots checks it with fm_pid_identity_legacy_matches. lab_pids takes its process tree from these roots. It joins them with spaces because BSD awk rejects a newline in an -v value. The script loads bin/fm-pid-identity-lib.sh. 2. bin/fm-herdr-lab-viewer.py writes launcher_start_utc= and viewer_start_utc=. In bin/fm-herdr-lab.sh, the new fm_herdr_lab_viewer_start_matches function matches a <key>_utc record exactly and checks a bare <key> record with fm_pid_identity_legacy_matches. fm_herdr_lab_viewer_owned_pair uses this function. The script loads bin/fm-pid-identity-lib.sh. 3. bin/fm-pid-identity-lib.sh: only one sentence in the header changed. It now says that the labs also read their pre-pin records through fm_pid_identity_legacy_matches. 4. Tests: the existing fixtures now write the _utc keys. There are two new regression tests. Each test records the start time in FM_TEST_TZ_EAST, checks that FM_TEST_TZ_WEST shows a different time, and runs the real command in WEST. - tests/fm-live-lab.test.sh: down stops the pre-upgrade launch process and removes the lab. - tests/fm-herdr-lab.test.sh, test_viewer_stop_signals_a_pre_upgrade_viewer: viewer stop signals the pre-upgrade viewer. Verification: - Both new tests fail on the old code. For live-lab, I used a temporary test copy whose fixture writes the old key. Both tests pass on the new code. - tests/fm-live-lab.test.sh: rc=0, 31 ok. - tests/fm-herdr-lab.test.sh: rc=0, 19 ok, three runs. - bin/fm-lint.sh on the changed shell files: rc=0. The full run gives rc=1 only because actionlint is not on PATH. - python3 -m py_compile on the viewer script passes. - As the user asked, I did not run the four suites that fail the same way on the base commit
Comment thread bin/fm-live-lab.sh Outdated
…n kill unrelated processes" (bin/fm-live-lab.sh:638). The previous CI round of this PR caused it. CI and Require no-mistakes show action_required because a maintainer must approve workflow runs for a fork PR. No code change can clear them. I made no change for "Reused PID passes ownership check". As the user decided, that comment gets an answer on its thread and not in code. The changes are not committed. Cause: the previous round changed lab_roots to check each recorded root with its own `ps -o lstart= -p` call. lab_pids then took a second, separate `ps -axo pid=,ppid=` snapshot to select descendants. A root could exit and a new process could take its PID between the two reads. That replacement and its children then became lab processes, and down could signal them. Fix (bin/fm-live-lab.sh): lab_pids now takes one `TZ=UTC0 ps -axo pid=,ppid=,lstart=` snapshot. It gives that snapshot to lab_roots and walks the tree from the same snapshot. lab_roots reads each root's lstart from the snapshot. A launch_start_utc record must still match exactly. A bare launch_start record still goes through fm_pid_identity_legacy_matches. This restores the single-snapshot property that the code had before this PR. Regression test (tests/fm-live-lab.test.sh, "down checks recorded roots against the snapshot that selects their descendants"): a ps shim answers the per-pid lstart query for the root with the recorded start. The real process table holds a different process with a child under that PID. The test runs the real `fm-live-lab.sh down` and asserts that both processes stay alive. The test fails on the old code ("down killed the process that reused a checked root's PID") and passes on the new code. Verification: tests/fm-live-lab.test.sh rc=0, 32 ok. tests/fm-herdr-lab.test.sh rc=0, 19 ok. `shellcheck -x` on both changed files rc=0. bin/fm-lint.sh reports only that actionlint is not on PATH
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