[aviggiano]

Summary

• fixes The overseer discards worker_exit_code, so unattended sandbox deaths cannot be diagnosed #302
• three Aave v4 runs lost their sandbox at stateful-invariant-setup and none can be diagnosed, because the field that would explain it is computed and then discarded
• the fix is one probe at a call site that already holds everything it needs

What is undiagnosable, and why

run	node	exit_category	succeeded	cost	wall clock
R45	stateful-invariant-setup	sandbox-exited	20	$404	~2h24m
R46	stateful-invariant-setup	sandbox-exited	20	$394	~2h18m

Neither recorded a last_error — the sandbox went away rather than the agent reporting a failure — and worker.log on the volume is a heartbeat only (worker-started / operation-started / operation-finished), carrying no agent stderr. The obvious next question is the sandbox's exit code, and it is missing.

probeModalSandbox (runner.ts:1180-1194) already obtains it:

const exitCode = await sandbox.poll();
return exitCode === null ? { state: "live", exitCode } : { state: "exited", exitCode };

and the foreground launchOrResumeModel path forwards it (runner.ts:680, :696). But on an unattended run — which is every benchmark run — the overseer is what observes these terminations, and its reservation force-closes the dying attempt's lifecycle row with every diagnostic hardcoded (launch-state.ts:1147-1160):

trigger_action: "unknown", start_reason: "unknown", terminal_reason: "unknown",
worker_exit_code: "unknown", model_work_started: "unknown", ...

R46's durable state, all three rows:

{"attempt": 2, "terminal_reason": "operational-failure", "worker_exit_code": "unknown",
 "model_work_started": true, "node_counts_after": {"succeeded": 20, "failed": 3, "remaining": 9}}

The fix

launchModalRecoveryWorker probes the outgoing sandbox before reserving its replacement. It already holds the Modal client and the outgoing sandbox_id, so this is one call rather than a new plumbing path, and the value is threaded through reserveModalLaunchAttempt into the lifecycle row.

The test pins three cases, two of which are easy to get wrong

• an observed 137 is recorded
• an absent observation still records "unknown" rather than inventing a code, so a caller that could not probe stays distinguishable from one that saw a clean exit
• an observed 0 survives as 0 — a falsy check here would silently discard the most interesting observation of the three, and a clean exit at this node would be a genuinely surprising result worth seeing

Proven red before the fix.

Relationship to #286

Same shape, different field. #286 threads observedModelWorkStarted through this exact reservation for the same reason: the overseer path defaults a diagnostic to "unknown" and nobody notices. Both reviewers of that PR independently recommended making the parameter required so omission becomes a compile error. That is the right end state and would subsume both changes — I have kept this one optional to match the existing shape rather than conflict with #286, and #302 records the suggestion that they be unified.

Validation

• packages/modal 310 tests / 310 pass
• typecheck, eslint, prettier clean
• full monorepo suite running

What this does not do

It does not fix the sandbox deaths. It makes the next one legible. Given that each relaunch of the mystery costs roughly $400 and two hours, and three runs have now paid that, one recorded integer is worth the change.

Greptile Summary

The PR preserves the sandbox exit code already observed by Modal recovery ownership resolution and records it when force-closing the replaced launch attempt.

• Returns the exit code alongside sandbox-liveness results.
• Threads observed exit codes through recovery launch reservation.
• Tests exited, live, missing, zero, and nonzero observations.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains within the eligible follow-up review scope.

Important Files Changed

Filename	Overview
packages/modal/src/runner.ts	Extends sandbox probing to return observed exit codes and forwards the original launch sandbox’s code through recovery replacement.
packages/modal/src/launch-state.ts	Accepts an optional observed exit code and records it while force-closing the replaced attempt.
packages/modal/test/runner.test.ts	Covers live, missing, cleanly exited, and nonzero-exited sandbox probe results.
packages/modal/test/launch-state.test.ts	Verifies lifecycle persistence for omitted, zero, and nonzero observed exit codes.

Sequence Diagram

sequenceDiagram
  participant Overseer
  participant Sandbox
  participant Recovery
  participant LaunchState
  Overseer->>Sandbox: poll current launch
  Sandbox-->>Overseer: exit code or live sandbox
  Overseer->>Recovery: resolve owner
  Recovery-->>Overseer: launchExitCode
  Overseer->>LaunchState: reserve replacement with observed exit code
  LaunchState->>LaunchState: close prior lifecycle row

Loading

_{Reviews (2): Last reviewed commit: "fix(modal): thread the exit code the ove..." | Re-trigger Greptile}

Context used:

• Knowledge Base — Modal remote benchmark execution

[aviggiano]

Both reviews rejected the first version, and the second found a better design than mine. Reworked in 478e4e8.

I asked the reviewers to be hostile about one thing specifically — that this adds a network call to the recovery relaunch path purely to improve logging — and both went straight at it.

The P1 they agreed on: probeModalSandbox rethrows anything that is not NotFoundError, and sandbox.poll() is a real gRPC call, so an exhausted retry budget surfaces as a raw ClientError, propagates out of launchModalRecoveryWorker, escapes overseeModalBenchmarkOnce (no per-model try/catch), and after MODAL_OVERSEER_MAX_CONSECUTIVE_FAILURES = 10 ticks the job is abandoned. Ten minutes of a persistent probe error against a reaped sandbox id = a permanently unsupervised run with 20 succeeded nodes on it. One reviewer noted the exposure is genuinely new in one state: when the recovery worker was already marked stopped in a prior tick, nothing else probes that id and this was the sole caller.

The better fix, which I took instead of the suggested .catch: the value never needed a new probe. runningRecoverySandbox already polls that exact sandbox during ownership resolution — and threw the answer away:

const exitCode = await sandbox.poll();
if (exitCode === null) return sandbox;      // non-null discarded

It now returns { sandbox } while live and { exitCode } once exited, the observation travels on ResolvedModalRecoveryOwner.launchExitCode, and launchModalRecoveryWorker records it. No new network call and no new throw site, so the P1 dissolves rather than being caught.

That also fixes something a .catch would not have: the second probe was strictly worse at capturing the value. Between ownership resolution and the reservation, the tick creates and terminates an inspector sandbox and copies credentials — minutes, not milliseconds — so a sandbox that read exited/137 at decision time could be reaped by then and yield "unknown". The PR was taking its observation at the worst available moment instead of the best.

The runner half was completely unpinned. Both reviewers mutation-tested it: deleting the probe and the spread left the suite green at 304 passed. Same defect class found in #286 and #298, and the third time this session. runningRecoverySandbox is now exported and tested across all four outcomes — exited with 137, exited cleanly with 0, still live, and vanished — proven red by mutation (return { exitCode: observed } → return {} fails exactly that test).

The 0 case matters beyond falsy-guard hygiene: a clean exit at a node that was supposed to keep working would be the most surprising of the four answers, and it is the one a careless guard silently discards.

Dropped as no longer applicable: the worker_exit_code: null concern. A live sandbox now returns { sandbox } with no exitCode field at all, so null can never reach the lifecycle row.

Noted, not fixed: the cancel path (terminateModalBenchmark) has sandbox handles in hand and could record an exit code too, and with a code available the force-close could classify terminal_reason as resource-termination rather than "unknown". Both are follow-ups; neither belongs in a change whose point is to stop a diagnostic from endangering a rescue.

Validation

• packages/modal 311 tests / 311 pass; typecheck, eslint, prettier clean
• new runner test proven red by mutation; the three launch-state cases were already verified red by both reviewers
• full monorepo suite running

On merging while runs are in flight

Both reviewers said the state format is compatible in both directions and that a probe failure mutates nothing. With the re-probe gone there is no added failure mode on the rescue path at all — the only behavioural delta is that a lifecycle row records an integer it previously discarded. I will still hold the merge until the suite is green and CI has reported.