fix(modal): recover the sandbox after a cancelled command - #151
Conversation
A cancelled command called sandbox.terminate, destroying the whole sandbox (the Modal SDK's ContainerProcess has no per-process kill). Nothing reset self._sandbox and _create_sandbox was only called from __init__, so every later exec hit a terminated sandbox and returned exit 1 with empty output for the rest of the session. Any command timeout triggers this via _ThreadedProcessHandle.kill(), so one slow patch write or test run bricked a worker's terminal. Modal-backed kanban lanes reported this as 'the runner is down' and blocked five CPE PR cards over ~5h on 2026-07-25 while Modal itself was healthy. Store the sandbox factory + base image, add _sandbox_is_live() (Sandbox.poll returns None while running) and _ensure_live_sandbox() at the top of _run_bash, and null out self._sandbox in cancel() so the next command rebuilds. Rebuild from the base image rather than a snapshot: a restore could carry back the state that wedged the previous sandbox, and the same factory re-applies credential/skills mounts either way. Reentrancy needs both an RLock and a _respawning flag: init_session() and the re-sync issue their own commands through _run_bash, which calls back into _ensure_live_sandbox. A plain Lock self-deadlocks (observed) and an RLock alone recurses forever. Verified live against real Modal, same script both revisions. Before: command after cancel returns rc=1 out='' permanently. After: rc=0, and the respawned sandbox still has git 2.47.3 + gh 2.96.0 from the baked image. Patch note: ~/.hermes/plans/hermes-patches/modal-sandbox-recover.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: bc2ef5f222
ℹ️ 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: 50d1ad523f
ℹ️ 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".
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?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: 6840d15dff
ℹ️ 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: 420380bf60
ℹ️ 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: 6084e47ba3
ℹ️ 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: eb302dac71
ℹ️ 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: 2ceb6b6088
ℹ️ 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: 894c505309
ℹ️ 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".
| proc = await self._sandbox.exec.aio( | ||
| "bash", "-c", quoted_rm_command(batch) | ||
| ) | ||
| await proc.wait.aio() |
There was a problem hiding this comment.
When any recovery rm -f batch returns a nonzero status—for example because a restored path is read-only or the remote shell encounters an I/O error—this callback still returns successfully, so FileSyncManager.sync() commits the deletion and disables replay until another sandbox reset even though the credential, skill, or cache file remains present. Fresh evidence beyond the earlier tombstone reports is that the new batched transport discards every wait.aio() result; check each exit code and raise so the transactional manager retains retry state. The added batching test only validates command length and cannot detect this remote-path failure.
AGENTS.md reference: AGENTS.md:L55-L55
Useful? React with 👍 / 👎.
The last commit (894c505) added _respawn_lock, _respawning, and the _sandbox_generation/_sandbox_checked_generation pair, but three __new__-constructed fixtures were not updated, so they hit AttributeError on the new attribute reads. Fixture-only change; no production code touched.
d656e78 to
51ddec8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51ddec8fde
ℹ️ 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".
| if checked: | ||
| self._sandbox_checked_generation = None | ||
| if not checked: | ||
| self._ensure_live_sandbox() |
There was a problem hiding this comment.
When execute calls overlap and one cancels the sandbox after another has returned from _before_execute() but before it reaches _run_bash(), this guard notices the generation change and respawns, but BaseEnvironment.execute() has already captured and wrapped the old effective_cwd. If that directory existed only in the terminated sandbox, the recovered command still runs the stale cd ... || exit 126 wrapper and fails. Fresh evidence after the prior generation-based fix is that revalidation still occurs only after wrapper construction; serialize preflight through handle creation or arrange to rebuild the wrapper after respawn. The single-threaded marker test cannot cover this remote-backend race.
AGENTS.md reference: AGENTS.md:L55-L55
Useful? React with 👍 / 👎.
| ) | ||
| await proc.wait.aio() | ||
|
|
||
| self._worker.run_coroutine(_rm(), timeout=15) |
There was a problem hiding this comment.
When retained tombstones span enough batches, this single 15-second timeout covers every sequential Modal exec rather than each batch. A long-lived environment that churns many cache files can therefore delete only an initial subset before timing out; FileSyncManager rolls its tracking back and retries the complete tombstone set on every later preflight, so recovery may never finish reconciling the restored snapshot. Bound each batch independently or scale the aggregate timeout with the batch count. The helper-only batching test does not exercise this remote-I/O deadline.
AGENTS.md reference: AGENTS.md:L55-L55
Useful? React with 👍 / 👎.
|
Superseded by #155. This PR accepted "cancel must destroy the sandbox" as a constraint and built recovery machinery underneath it: liveness polling, generation counters, RLock + The premise is false. The one independent bug this PR surfaced (dead sandbox reaching the agent as a bare exit 1) already shipped as #152. Leaving this open for now so #155 can be compared against it; will close once #155 merges. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84b747ee7e
ℹ️ 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".
| try: | ||
| worker.run_coroutine(sandbox.terminate.aio(), timeout=15) |
There was a problem hiding this comment.
When another env.execute() starts while cancellation is in progress—an explicitly supported pattern in the execute-code RPC loop—the cancellation increments the generation and then releases _respawn_lock before the remote termination completes. That caller can therefore poll the still-live old sandbox, mark the new generation as checked, and launch its process against the sandbox that this call is about to terminate, causing the overlapping command to fail instead of using the replacement. Fresh evidence beyond the prior generation-based fix is that the generation changes before this unlocked remote call, so a later preflight treats the doomed sandbox as validated; keep a termination-in-progress state or serialize preflight/handle creation through confirmed termination.
Useful? React with 👍 / 👎.
|
Closing in favor of #160, which fixes the same symptom without the recovery machinery. I tested this PR's premises against live Modal (
Credit where it is due: #155 reached the same process-group conclusion before #160 did, and its Branch |
1 participant
Symptom
Modal-backed kanban worker lanes reported the terminal "being down": every shell command returned exit 1 with empty output, including
pwd,/bin/echo hello, andtrue. Workers blocked their cards asking for the runner to be restored.Five CPE PR cards blocked this way over ~5h on 2026-07-25 (NousResearch#459, NousResearch#468, NousResearch#472, NousResearch#474, NousResearch#483). Modal itself was healthy throughout, and PR NousResearch#475 completed normally on the same lane and image between two "outages".
Root cause
Not an outage. Each worker destroyed its own sandbox and had no way back.
_run_bash'scancel_fncalledsandbox.terminate— tearing down the entire sandbox, not the one running command. The Modal SDK'sContainerProcessexposes onlypoll/wait/stdout/stderr, no kill, so this is the only cancel available._ThreadedProcessHandle.kill()(base.py:364) firescancel_fnon any command timeout — a slowpatchwrite or test run is enough.self._sandbox, and_create_sandboxwas only called from__init__. No reconnect or recreate path existed.sandbox.exechit a terminated sandbox, raised, and the handle's worker setreturncode = 1with an empty pipe — the exact "exit 1, no output" signature, permanently.The NousResearch#468 worker's own note named the trigger: "after a patch-tool timeout, every Modal terminal invocation now exits 1 with no output."
Fix
_sandbox_is_live()—Sandbox.poll()returnsNonewhile running, an exit code once terminated. Any SDK error counts as not-live._ensure_live_sandbox()at the top of_run_bash— recreates from the base image, re-syncs~/.hermes, rebuilds the env snapshot viainit_session().cancel()nullsself._sandboxafter terminating, so the next command rebuilds.Rebuilding from the base image rather than a snapshot is deliberate: a restore could carry back the state that wedged the previous sandbox, and the same factory re-applies credential/skills mounts either way.
Reentrancy (the non-obvious part)
init_session()and the re-sync issue their own commands through_run_bash, which calls back into_ensure_live_sandbox. A plainLockself-deadlocks here (observed: the first patched run hung after "creating a replacement" and never reached step 3). AnRLockalone recurses forever. Both are needed:RLockplus a_respawningflag making nested calls no-ops.Verification — live, against real Modal
A script drove the real wedge (baseline command ->
kill()a running command -> command again), run on both revisions.BEFORE (
origin/live-config):AFTER (this branch):
Step 4 confirms the respawned sandbox keeps the baked image's tooling, so a worker resumes git/gh work rather than landing in a bare container.
Decisions made
cancel()— keeps cancel fast and avoids paying a ~30s sandbox boot for a session that's about to end.RLock+_respawningflag rather than restructuringinit_session— smallest change that makes the nested call safe.~/.hermesdoes not survive a respawn. Same guarantee as any other sandbox loss; workers clone fresh into/root/workper themodal-backed-worker-lanesskill.Out of scope (pre-existing, seen in both runs)
file_sync: sync failed ... ARG_MAX. Got 121721 bytes— the bulk-upload path exceeds the shell arg limit on a large~/.hermes. Untouched here; worth its own fix.Patch note:
~/.hermes/plans/hermes-patches/modal-sandbox-recover.md