fix(bin): isolate process-event runners into their own session and report runner deaths - #5676
Open
Courtneyezra wants to merge 4 commits into
Open
Courtneyezra wants to merge 4 commits into
Courtneyezra wants to merge 4 commits into
Conversation
…port a runner death A process-event runner was made the leader of a fresh process group but stayed in the session of the agent that armed it, so every hangup delivered to that session reached it. It died mid-poll: nothing was captured, and the window was ordinary operation rather than an edge, because it lasted exactly as long as the launching agent lived. Reparenting to init made such a runner look detached while it was not, and it survived a session sweep only when its session leader happened to have exited first, which POSIX shields but which is accident rather than isolation. isolate_process now calls setsid(2) in the forked child, which makes it session leader, leader of a new process group inside that session, and drops its controlling terminal in one step. Isolation is proved before exec - setsid's own return and an independent getpgrp read must both be the child's pid - and either disagreeing exits 125 exactly as a failed fork does, because a runner that half-escaped its session presents as armed while a hangup can still reach it. Every group-leadership property the stop, group-liveness, and guard paths rely on is preserved, since setsid leaves pgid == sid == pid. The other half of the defect was silence. A runner that died inside its source command captured nothing and nothing said so. It needed no new record: the runner marker already means "a runner is inside its source command", and the runner removes it on every ordinary way out, so a marker outliving its runner is already the record of a lost round. Two existing readers now read it as one - the runner's own owner guard, which watches that pid and, now isolated into its own session too, outlives whatever ended the runner's, and reconcile as the backstop - and publish one durable check wake through the existing queue. The marker is cleared only once the wake lands, so exactly one reader announces each death, a failed announcement is retried, and the orphan that silently failed this home's sweep preflight goes with it. Recovery is unchanged: the source stays registered and reconcile starts a replacement. The watcher classifies the new key under its own headline rather than the healthy-looking "result captured" default, for the same reason the strand and launch-failure keys do. Tests: a runner armed from a launcher holding its own session is in neither that session nor its group, and a SIGHUP to every member of that session - which kills the launching leader, so the delivery is proved rather than assumed - leaves the runner and its poll alive and the interrupted round still captures. A runner killed with its whole group inside its source command is announced once, by the guard and, when the guard is killed first, by reconcile. Both fail against the previous code, as does the new headline case.
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
1 participant
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
The process-event runners this fleet depends on can be killed silently by a session-level hangup, because they never leave the launching agent's session. Fix that so a runner genuinely outlives the agent that started it, and so a runner that dies is not silent.
The full verified evidence, which is to be re-established rather than trusted:
THE DEFECT. bin/fm-procevent.sh isolate_process() calls setpgrp(0, 0) and never setsid. Neither fm-procevent.sh nor fm-procevent-lavish.sh contains setsid at all. So a runner gets its own PROCESS GROUP but stays in the LAUNCHING AGENT'S SESSION as a non-leader.
VERIFIED ON THE LIVE LISTENER: a runner at ppid 1 leading its own process group, but with a session id belonging to the launching agent's session, whose session leader is already dead. The own-process-group is why it LOOKS detached at ppid 1 while remaining session-reachable. That listener survives ONLY because its session leader has already exited, so the group is orphaned and POSIX shields it from terminal HUP. That is accidental protection, not design.
CONSEQUENCE. A session-level hangup or sweep kills a runner silently. The poll dies mid-call rather than returning, so no result is ever captured and nothing logs it - which matches the observed missing capture exactly. The window of vulnerability is precisely while the launching agent is still alive, i.e. normal operation.
BLAST RADIUS. Every process-event source in every home: Lavish review boards, remote secondmate reply sources, quota checks, condition->action watches, and the captain-answer intake bindings that feed bin/fm-captain-hold.sh. A source that dies this way loses no queued data - Lavish queues feedback until a poll delivers it - but it stops collecting, and nothing says so until a reconcile sweep happens to notice.
What Changed
bin/fm-procevent.shnow starts each process-event runner and its owner guard withPOSIX::setsid()instead ofsetpgrp(0, 0), so they lead a new session rather than only a new process group. Before exec, the child checks that the returned session id andgetpgrpboth equal its own pid, and it exits 125 if either check fails. After exec,require_isolated_session(which replacesrequire_isolated_group) checks this again through theFM_PROCEVENT_RUNNER_SESSIONmarker and the process group reported byps.procevent:<id>:runner-died:*check wake when it sees that pid gone with its group empty.reconciledoes the same before it relaunches, as a backstop. The marker is removed only after the wake is queued, so each death is announced once. When the owner guard stops a runner on purpose after its lease checks fail, it clears the marker, so that stop is not reported as a death. Leaderless groups are still handled only by the existing stranded wake.bin/fm-watch.shshows these wakes as a separate "process-event source runner died" reason instead of listing them as captures. Tests cover session isolation, reporting from the guard and fromreconcile, and not reporting a lease stop.AGENTS.md,docs/configuration.md, the process-event skill doc and the verification doc were updated to match, including the known limit that extension-owned sources are not covered.🤖 Generated with Claude Code
Risk Assessment
Testing
I drove four live lab scenarios using the real procevent CLI, a tmux-hosted launching shell and the real watcher, all in disposable homes that were removed afterwards. The runner now moves into its own session and survives a hangup sent to the launching agent's session. The same scenario on the base commit reproduces the silent kill. Deaths are reported once, either by the owner guard or by reconcile as a fallback, and the watcher shows them under their own heading. A deliberate stop after the owner's activity lease expires raises no false alarm.
tests/fm-procevent.test.shcould not finish on this host. Itsppid == 1orphan check hits systemd's user service manager, which adopts orphaned processes here, and its launch-timing checks fail at random points on this loaded machine. The base commit's copy fails the same way. CI owns that file's result. The real fleet home and its live listeners were not touched.pkill -HUP -s 2769688the runner survived andliststill shows the owner as livecheck: process-event source runner died: procevent:guard-src:runner-died:… procevent:backstop-src:runner-died:…systemd --user, and unprivileged user namespaces are blocked (unshare: write failed /proc/self/uid_map), so a private PID namespace was not possible. Ru…Evidence: Fixed code: runner in its own session survives a session-wide SIGHUP
Source: Fixed code: runner in its own session survives a session-wide SIGHUP
Evidence: Base c643b57: runner shares the agent's session and a SIGHUP kills it silently (defect reproduced)
Source: Base c643b57: runner shares the agent's session and a SIGHUP kills it silently (defect reproduced)
Evidence: Runner death reported once by the owner guard and once by the reconcile fallback
Source: Runner death reported once by the owner guard and once by the reconcile fallback
Evidence: Watcher shows 'process-event source runner died'
Source: Watcher shows 'process-event source runner died'
Evidence: Lease-backstop stop leaves no marker and raises no false death wake (3 runs)
Source: Lease-backstop stop leaves no marker and raises no false death wake (3 runs)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed (2) ✅
tests/fm-procevent.test.sh:3956- The 'zombie leader' signal-proof retirement fixture still launches_startthrough its own Perl launcher that callssetpgrp(0, 0)and sets$ENV{FM_PROCEVENT_RUNNER_GROUP}. This change renamed that handshake toFM_PROCEVENT_RUNNER_SESSION(bin/fm-procevent.sh:904/938), sorequire_runner_sessionnow dies with "runner session was not isolated" before the runner writesproof-src.runner. Theproof_state=zombieiteration will fail atwait_for .../proof-src.runner("the signal-proof listener never recorded its runner"), which breaks the suite. Fix: make the fixture's launcher callPOSIX::setsid()in place ofsetpgrp(0, 0)and setFM_PROCEVENT_RUNNER_SESSION, so it mirrors isolate_process.docs/verification/process-event-sources.md:224- The 'Portability finding' section still sayssetsidcannot establish the runner's process group and that both launch paths use a Perl launcher that 'callssetpgrp(0, 0)in that child, marks the expected group leader'. After this change the launcher calls POSIX::setsid() (the syscall, not the missing macOSsetsidbinary) and marks the session with FM_PROCEVENT_RUNNER_SESSION, so this verification doc now describes the defect as though it were the design. Update it to say the Perl launcher calls setsid(2) because thesetsidutility is missing on macOS.🔧 Fix applied.
1 warning still open:
bin/fm-procevent.sh:1509- The owner guard can stop the runner on purpose, and reconcile then announces that stop as an unexplained death. When two lease reads in a row fail, the guard callsstop_runner_pidand exits 0. That stop does not remove the runner marker or release the claim. The next reconcile in that home finds a stale claim with the marker still present.report_runner_death(line 1774) then queues arunner-diedcheck wake saying the runner 'died inside its source command' and to 'check whether something is killing the runner'. Concrete sequence: the watcher is idle for more than the 600s default lease (session ended, then resumed in the same home). Each guard stops its runner. When the watcher comes back, reconcile sends one false death alarm per registered source, all caused by the fleet's own designed lease backstop. Fix within the change's own mechanism: after a successfulstop_runner_pidin the guard, take the source lock without waiting (asreport_runner_death_guardeddoes) and remove the runner marker if it still names this pid. That way a deliberate stop is never read later as a lost round.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
pkill -HUP -s 2769688the runner survived andliststill shows the owner as livecheck: process-event source runner died: procevent:guard-src:runner-died:… procevent:backstop-src:runner-died:…systemd --user, and unprivileged user namespaces are blocked (unshare: write failed /proc/self/uid_map), so a private PID namespace was not possible. Ru…/tmp/fm-hup-scenario.sh <worktree> 'change b34074c': lab home, tmux primary on the private fm-lab socket runsbin/fm-procevent.sh register lavish hup-src -- /bin/sleep 900 && bin/fm-procevent.sh reconcile, compares runner and agent session ids withps, thenpkill -HUP -s <agent sid>while the agent is aliveSame scenario againstgit archive c643b57 bin(base, before the fix) to reproduce the defect/tmp/fm-death-scenario.sh:kill -HUP -- -<runner pgid>with the guard alive, then checkstate/.wake-queue; kill guard and runner, runfm-procevent.sh reconciletwice, then check for a single wake and a restarted replacementFM_HOME=<lab> bin/fm-watch.shover that queue to see how the deaths are shown/tmp/fm-lease-scenario.shx3: FM_PROCEVENT_OWNER_LEASE_SECONDS=3, let the home go idle until the owner guard stops the runner, check the registry for a leftover.runnermarker, reconcile, countrunner-diedwakesbash tests/fm-procevent.test.sh(umask 077; 3 attempts), plus a subreaper-tolerant temp copy and the base-commit copy for comparison✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.