fix(modal): cancel the command's process group, not the sandbox - #155
fix(modal): cancel the command's process group, not the sandbox#155exiao wants to merge 11 commits into
Conversation
_run_bash's cancel_fn called sandbox.terminate(), so any command timeout destroyed the whole sandbox. _ThreadedProcessHandle.kill() fires cancel_fn on ANY timeout, and nothing recreated the sandbox, so every later exec hit a terminated sandbox and returned exit 1 with empty output for the rest of the session. Modal-backed worker lanes responded by blocking their cards asking a human to restore the terminal: 29 of 64 kanban blocks since the 2026-07-24 migration carry this signature. The Modal SDK's ContainerProcess exposes no kill, but the sandbox is a Linux box: run each command under setsid so it leads its own process group, record the group id in a /tmp pid file, and signal -<pgid> on cancel (SIGTERM, then SIGKILL after a grace period). That reaches the command and every descendant it spawned, which is the actual goal since the runaway is usually a grandchild, and leaves the sandbox running. Exit status, stdout, and stderr pass through unchanged. Falls back to a plain background job when setsid is absent. Verified live against real Modal on both revisions: before, the third command returns rc=1 with empty output; after, the cancelled command returns 143, zero descendants survive, sandbox.poll() stays None, and the next command returns 0. Patch note: ~/.hermes/plans/hermes-patches/modal-cancel-process-group.md
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 849b93a4ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Both were introduced by this branch's diff and are real. P1: a cancel arriving before the wrapper published its pid (exec still queued) hit the 'no pid file' path and reported success, while the wrapper went on to start the command, leaving it running after Hermes returned an interrupted result. cancel() now drops a cancel marker BEFORE reading the pid file, and the wrapper checks that marker both before launching and immediately after publishing its pid, closing the window from both sides. P2: in the no-setsid fallback the child shares the wrapper's process group, so $! is a pid, not a pgid. Signalling -$pid there targets a nonexistent group (ESRCH) and leaves the command running on every cancel. The recorded value is now tagged G:<pid> (own group, signal the group) or P:<pid> (fallback, signal the pid), and cancel() resolves the tag. Re-verified live on real Modal: cancel still reaps all descendants (survivors 0), a SIGTERM-ignoring command is still killed (rc=137), sandbox.poll() stays None, exit codes/stdout/stderr/login unchanged, and pid files do not accumulate.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ced793c17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc2be59ebf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Third review finding, also real. $! names the newly forked process before it has necessarily executed setsid(2), so publishing G:$! from the parent let a cancel in that window signal a process group that did not exist yet, take ESRCH, and leave the command running. The setsid'd shell now publishes its OWN pgid (ps -o pgid= -p $$) and then execs the command, so the recorded group is only ever one that already exists. Two consequences handled: - The wrapper's post-launch cancel-marker check can no longer use the group (not published yet), so it signals the direct child pid, valid the moment fork returns, and escalates to SIGKILL. - cancel() no longer treats a missing pid file as 'nothing to do'; it retries the read for the grace period, since publication is now slightly later than fork. Verified live on real Modal, including a cancel fired with zero delay after starting the command (races publication): rc=143, zero survivors. Descendant reaping, SIGTERM-ignoring kills, exit codes, stdout/stderr, and login shells all unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 934ff3443a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Fourth review finding, also real. An image that ships setsid but lacks procps (or a compatible ps) failed the command substitution while echo still wrote a bare 'G:'. cancel() then resolved that record to the target '-', signalled nothing, and the command survived, with the wrapper's startup marker check already past. Publish P:$$ instead when ps yields nothing, so the record always names a real target. Verified live on real Modal with ps shadowed by a stub that exits 127: cancel returns 143, zero survivors, sandbox still alive.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b518bf30e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Fifth review finding, and it points at a simplification. The previous commit's ps-less fallback recorded P:$$, which signals only the setsid shell and leaves its children running, exactly the descendant reaping this PR exists to provide. But setsid makes that shell a session AND process-group leader, so its pgid IS its own $$. Publishing G:$$ from inside needs no ps, no tr, and no fallback branch: group signaling now works on any image, and the wrapper is shorter than before the finding. Confirmed on real Modal that a setsid'd shell's $$ equals its pgid across repeated spawns (pid=4/pgid=4, 25/25, 28/28). Verified live with ps shadowed by a stub that exits 127 AND the command spawning a child: cancel returns 143 and zero descendants survive. Stubborn-SIGTERM (137), immediate-cancel race, exit codes, stdout/stderr, and login shells all unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8db79aacbd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Sixth review finding, and this one is created by this PR's own diff.
The SDK exec deadline and _wait_for_process's local deadline were both
exactly `timeout`. Before this PR a tie was harmless: Modal reaping the
ContainerProcess killed the command itself. Now the real command is a
background child in its own setsid session, so if Modal's deadline won,
it would kill the outer bash while the command kept running, the handle
would report completion, and cancel() would never fire to reap the
group. A warm exec starting inside the poll loop's ~200ms backoff is
enough to lose that race.
Give the SDK deadline 15s of headroom so the local deadline always wins
and cancellation is what stops a command.
Verified against the real backend through the full execute() path, not
generated strings: execute('sleep 300 & sleep 300', timeout=10) returns
'[Command timed out after 10s]', and 4s later zero sleep processes
survive and the sandbox still serves commands.
Adversarial self-reviewReviewing my own PR against the case for rejecting it. Six review findings landed on this branch and every one of them was real and caused by this diff, so the honest summary is that the idea was right on the first commit and the implementation needed six rounds to be correct. What's left that a reviewer should push on: 1. The wrapper is shell string-building, and that's a real smell. It's ~15 lines of generated bash embedded in Python f-strings, and the escaping is load-bearing ( 2. The fallback path is the weakest part and is nearly untested live. When 3. Cancellation is now best-effort where it used to be absolute. 4. Every command now pays for cancellation it will never use. 5. The tests are mostly assertions about generated strings. Fourteen of the twenty unit tests inspect the shell text rather than executing it, which is exactly the class of test that passes while the behavior is broken; findings 3-5 all slipped past green tests of that shape. The live probes are what actually establish correctness here, and they run by hand, not in CI. A reviewer would be right to ask for a marked integration test that runs the real wedge against a real sandbox, so this can't silently regress. 6. I deleted #151's What I'd insist on before merging: nothing blocking. What I'd want after: an integration test for the wedge, and a decision on whether the no-setsid fallback should exist at all. |
Simplification pass. Three of this PR's six review findings existed only because the setsid design had a window between fork and the process group actually existing: - publishing $! before setsid(2) had run (signalled a nonexistent group) - the no-setsid fallback that could only signal a bare pid - deriving the pgid via ps/tr, which slim images may not ship `set -m` removes the window entirely. With job control on, bash puts the backgrounded command in a NEW process group at fork time and $! IS that group's id, so the recorded value is a valid signal target the instant it exists. That deletes the G:/P: tagging, the tag parsing in cancel(), the setsid/ps probing, the fallback branch, and the wait-for-publication retry loop. Net -83 lines against the previous head, and the remaining wrapper is 8 shell statements. Verified live on real Modal, full matrix: descendants reaped (survivors 0), SIGTERM-ignoring command killed (137), zero-delay cancel that races publication (143, 0 survivors), sandbox alive throughout (poll() None), exit codes 42/3/0 exact, stdout+stderr, login shell, no-newline output, heredoc stdin, and the execute() timeout path leaving 0 orphans. A/B'd the execute()-timeout latency against the setsid head over 3 runs each: median 96.6s (setsid) vs 97.7s (set -m). No regression; that cost is inherent to the timeout path, not the design.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40c8cedbc3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| f"if [ -e {cancel_file} ]; then " | ||
| f'kill -TERM -"$__hermes_pgid" 2>/dev/null; fi; ' |
There was a problem hiding this comment.
When cancellation touches the marker after the wrapper's initial check but before the PID file is published, the cancellation exec reads no PID and exits; this branch then becomes the only cancellation path, but it sends only SIGTERM. A command that ignores TERM therefore remains in wait and can continue after Hermes reports the timeout or interrupt. Fresh evidence beyond the resolved earlier comment is that the new job-control wrapper reintroduces the TERM-only post-publication branch; apply the same grace-period SIGKILL escalation here or make the cancellation exec wait for PID publication, and exercise this interleaving through the real remote path rather than string assertions.
AGENTS.md reference: AGENTS.md:L55-L55
Useful? React with 👍 / 👎.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| # _wait_for_process, which is exactly `timeout`. If Modal fired | ||
| # first it would kill the outer bash while the real command, | ||
| # now a background child in its own setsid session, kept | ||
| # running: the handle would report completion and cancel() |
There was a problem hiding this comment.
Comment incorrectly references 'setsid session' but the implementation uses job control (set -m), not setsid. While this is just a documentation issue, it's misleading:
# Give the SDK's own deadline headroom over the local one in
# _wait_for_process, which is exactly `timeout`. If Modal fired
# first it would kill the outer bash while the real command,
# now a background child in its own process group, keptShould say 'process group' instead of 'setsid session' to match the actual implementation.
| # _wait_for_process, which is exactly `timeout`. If Modal fired | |
| # first it would kill the outer bash while the real command, | |
| # now a background child in its own setsid session, kept | |
| # running: the handle would report completion and cancel() | |
| # _wait_for_process, which is exactly `timeout`. If Modal fired | |
| # first it would kill the outer bash while the real command, | |
| # now a background child in its own process group, kept | |
| # running: the handle would report completion and cancel() | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
Two review findings on the job-control rewrite, both real. P2: when a cancel lands after the wrapper's initial marker check but before the pid file is published, cancel() reads no pid and exits, so the wrapper's own post-launch marker check is the ONLY cancellation path for that command. It sent TERM only, so a command that ignores TERM sat in wait() and kept running past the reported timeout. It now escalates to SIGKILL after the same grace period cancel() uses. My simplification reintroduced this; the setsid version had the escalation. Also fix a stale comment that still said 'setsid session' after the switch to job control. Verified live: a zero-delay cancel against "trap '' TERM; sleep 200" (forcing the late-marker path) returns 137, and zero processes survive. Full suite 122 passed, ruff clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c556ef3902
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Cancelled before we were scheduled: never start the command. | ||
| f"if [ -e {cancel_file} ]; then exit {128 + 15}; fi; " | ||
| # Job control: the background job leads a new group, and $! IS its pgid. | ||
| f"set -m; bash {flags} {quoted} & __hermes_pgid=$!; " |
There was a problem hiding this comment.
On every normally completed Modal command, set -m makes non-interactive Bash emit a [1]+ Done ... or Exit ... job notification on stderr when wait returns. _do() concatenates stderr into the returned output, and CWD extraction preserves content after its marker, so even true returns bogus text while internal cat reads and code-execution RPC payloads gain extra bytes. Disable monitor mode after capturing $! or otherwise suppress these notifications, and cover the wrapper with a real-shell/Modal behavior test rather than only string assertions.
AGENTS.md reference: AGENTS.md:L55-L55
Useful? React with 👍 / 👎.
Review raised a P1 that `set -m` would emit '[1]+ Done' job notices on stderr for every completed command, corrupting internal cat reads and RPC payloads. Not reproducible: bash only prints job notifications in an INTERACTIVE shell, and the sandbox exec is non-interactive ($- has no i). Checked against real Modal at the raw SDK level, below _run_bash's stdout/stderr concatenation: stderr is empty for true, echo hi, exit 7 and sleep 1, byte-identical to the same commands with monitor mode off. Through _run_bash: 'true' returns '', 'cat /etc/hostname' returns exactly 'debuerreotype\n', and a user command that backgrounds its own job returns only 'started\n'. Add a test pinning the property that makes this safe (monitor mode is enabled inline in the wrapper, never exported into the command's environment) so a future refactor cannot silently make the finding true.
|
Checked this against real Modal rather than by reading, and it does not reproduce. Bash only prints Raw stderr at the SDK level, below And through the real
I've added Resolving as not-a-defect on the evidence above. Happy to reopen if you can produce a sandbox image where a non-interactive bash emits the notice. The |
The _MODAL_EXEC_TIMEOUT_HEADROOM comment still said "the setsid'd command" after the switch to job control. The remaining mention in _wrap_for_group_cancel's docstring is deliberate: it explains why the design does NOT depend on setsid or ps being installed.
|
Closing this PR: it stopped converging (10 bot comments concentrated on tools/environments/modal.py across 3 review waves (>= 8 on one file over >= 3 waves)). This is a review loop chasing an unbounded edge-case vein, not a PR one more fix will land. A fresh minimal rewrite from the original plan has been routed to the orchestrator (kanban |
|
Superseded by #160, which is merge-ready. Staying closed — but the record should show this PR had the right idea first. I worked the same problem independently without knowing this branch existed, tested the premises against live Modal, and arrived at the same conclusion: don't destroy the sandbox to stop a command, signal the command's process group. This PR got there first, and its core diff is smaller than mine (110 vs 145 lines in Its #160 ships only because it is finished: 0 open review threads vs 2 here, CI green, and all five bot findings reproduced on live Modal with a regression test for each that I verified fails against the previous logic. On the closure itself: this was closed as non-convergent at 10 bot comments across 3 waves. Worth noting that one of the two threads still open at closing time was the monitor-mode P1 that the comment above it had already disproved with live SDK-level stderr captures. The detector counted comment volume, not whether the findings were still valid, so a PR with a correct design and a disproved blocker got closed while its weaker sibling (#151) stayed open. Flagging for the detector's heuristics, not to reopen this. The rewrite card that this closure spawned (kanban |
1 participant
Replaces #151. Same symptom, one-tenth the machinery, because the premise #151 was built on turns out to be false.
Symptom
Every Modal-backed worker lane (
dev,code-reviewer,pr-babysitter-modal) wedged mid-session: every shell command returned exit 1 with empty output, includingtrueandpwd. Workers blocked their cards asking a human to "restore the terminal backend." 29 of the 64 kanban blocks since the 2026-07-24 Modal migration carry this signature.Root cause
_run_bash'scancel_fncalledsandbox.terminate()— tearing down the entire sandbox to stop one command._ThreadedProcessHandle.kill()(base.py:364) firescancel_fnon any command timeout, and a slowpatchwrite or test run is enough. Nothing recreated the sandbox, so every latersandbox.exechit a terminated one and surfaced as exit 1 with an empty pipe, permanently.The Modal SDK's
ContainerProcessexposes onlypoll/wait/stdout/stderrwith no kill, so the original code reached for the only cancel the sandbox object offered.The fix
Don't destroy the sandbox to stop a command. The sandbox is a Linux box.
Run each command under
setsidso it leads its own process group, record that group id in a/tmppid file, and havecancel()signal-<pgid>: SIGTERM, then SIGKILL after a 5s grace period. That reaches the command and every descendant it spawned — the actual goal, since the runaway is typically a grandchild like an unboundedfind— and leaves the sandbox running.Exit status, stdout, and stderr pass through unchanged. If
setsidis missing, the wrapper falls back to a plain background job: cancellation then reaches only the command itself, still better than destroying the sandbox.Why not #151
#151 accepted "cancel must destroy the sandbox" as a constraint and built recovery machinery underneath it: liveness polling, generation counters, an
RLockplus a_respawningreentrancy flag, sync-state reset, deletion tombstones retained across respawns, and rm-batching for the resulting oversized replay. 383 lines across 4 files, 9 commits, 22 review rounds, and 3 P2 threads still open (concurrent-respawn races, per-batch timeouts, failed deletion batches).All of that complexity exists to survive a wound we inflict on ourselves. Nobody checked whether the wound was necessary. It isn't.
The one genuinely independent bug #151 surfaced — a dead sandbox reaching the agent as a bare
exit 1rather than a real error — already shipped separately as #152.tools/environments/file_sync.pyis untouched here; its #151 changes existed only to support respawning.Verification — live, against real Modal
The same script drives the real wedge (baseline command → cancel a running command → command again) on both revisions.
BEFORE (
origin/live-config):AFTER (this branch):
Also verified directly against the Modal SDK: a process-group kill of a command whose child
sleepoutlives its parent reaps every descendant (survivors: 0) whilesandbox.poll()staysNoneand later execs return 0. A command that traps and ignores SIGTERM is still killed by the escalation.Unit: 11 new tests in
tests/tools/test_modal_cancel_process_group.py. 90 passed across the modal + file_sync suites on 3 consecutive runs (no flake, random order). 112 passed across the terminal/env suites.ruffclean.Diff
tools/environments/modal.py—_wrap_for_group_cancel()+ process-groupcancel()tests/tools/test_modal_cancel_process_group.py— newPatch note:
~/.hermes/plans/hermes-patches/modal-cancel-process-group.md