[aviggiano]

Summary

• refs The worker's top-level catch discards the error, so every unanticipated failure is undiagnosable #307
• one line of substance: bind the error the worker died from, and print it

What was there

packages/modal/src/worker.ts:619:

).catch(() => {
  console.error("worker terminated");
  process.exitCode = 1;
});

The rejection value was never bound. Combined with diagnosticCodeForError recognising only two error classes, any other failure produced the string worker terminated, exit code 1, and no cause anywhere.

What that cost

Six sandboxes have now ended at stateful-invariant-setup across R45, R46 and R47. R47 attempt 1 finally recorded worker_exit_code: 1 — available only because #302/#303 fixed the same class of gap one layer out — and exit 1 traces to exactly this handler. The worker exited deliberately, knew why, and discarded the reason.

In the meantime I pursued and published two wrong causal theories: a V8 string ceiling in patch capture (refuted by a reviewer running the code) and a git add cgroup OOM (refuted by /usr/bin/time -v showing peak RSS is per-file — 620 MB across four blobs peaks at 155 MB, against a 32 GiB request). One logged error string would have pre-empted both.

What this deliberately does NOT do

My first attempt also persisted a redacted, bounded failure_message into the worker result contract. The suite refused it, by name:

× writes and flushes an unexpected worker failure without serializing its detail
    expect(serialized).not.toContain("placeholder-private-detail")

That invariant is deliberate and correct: status.json and result.json are in MODAL_COLLECT_RESULT_FILES and there is a public results path, so an arbitrary error string could put target source or configuration into a published artifact. redactSecretsInText strips known secret shapes, not arbitrary content, so it does not license crossing that boundary. Reverted; the invariant stands untouched.

stderr is the right destination here — inside the sandbox, outside the collected contract.

The durable channel is a separate decision

stderr is not durable; by the time anyone looks the sandbox is gone. The obvious fix is the volume's worker.log, except that is also in MODAL_COLLECT_RESULT_FILES, so it inherits the same publication question. #307 records three options (a private uncollected file, a strictly-filtered contract field, or extending diagnosticCodeForError to categorise more error classes) and leaves the choice to whoever owns that boundary. It should not ride quietly on a logging fix.

Validation

packages/modal 311/311; typecheck, eslint, prettier clean; full monorepo suite running.

Greptile Summary

This PR improves Modal worker failure diagnostics without changing the persisted result contract.

• Binds the worker’s top-level rejection reason.
• Prints an error stack or message to sandbox stderr while retaining exit code 1.
• Leaves collected worker result files and lifecycle logs unchanged.

Confidence Score: 5/5

The PR appears safe to merge with no actionable defects identified.

The change improves diagnostics only on the existing worker failure path, preserves exit code 1, and does not alter persisted or collected result data.

Important Files Changed

Filename	Overview
packages/modal/src/worker.ts	The top-level failure handler now reports the rejection reason to stderr while preserving existing termination behavior and artifact boundaries.

_{Reviews (1): Last reviewed commit: "fix(modal): print the error that ended t..." | Re-trigger Greptile}

Context used:

• Knowledge Base — Modal remote benchmark execution

[aviggiano]

Reviewer 2 found my fourth false causal claim, and I verified it. Closing this PR.

The comment I wrote asserts this handler "used to discard [the reason] … which is how five sandboxes ended at one node." That is wrong, and the code is three functions away in the same file:

packages/modal/src/worker.ts:509-513

const streams = [child.stdout, child.stderr].map(async (stream) => {
  for await (const _chunk of stream) {
    // Drain child output without persisting provider responses or benchmark contents.
  }
});

:527-530

if (exitCode !== 0) {
  await appendGenericLog("operation-failed");
  throw new OperationalDispositionError(failureCategory(options));   // no cause
}

The Smithers workflow — where captureWorkspacePatch → runGit → ENOBUFS actually throws — runs as a child process, and its stderr is drained into a no-op loop deliberately, then a fixed-message error is thrown with no cause. So after this PR the output for the exact six deaths under investigation would be:

worker terminated OperationalDispositionError: worker operation failed

No ENOBUFS, no git, no workspace-handoff, no echidna/. The change does not do what its comment says it does. The reason was discarded two layers earlier.

That is my fourth refuted causal claim in this sequence — after a V8 string ceiling, a git add OOM, and ruling out ENOBUFS because it would be "loud". Merging a comment asserting a fifth would repeat exactly the failure mode of #305.

What actually needs doing, per review

Reviewer 2's suggestion is right and is a different change from this one:

1. In runChecked, retain a bounded tail (say 8 KB) of the child's stderr instead of discarding it, and pass it as cause. The drain is deliberate — "without persisting provider responses or benchmark contents" — so it must be routed through sanitizePublicDiagnosticMessage (public-worker.ts:964-980), which is the existing precedent in this package.
2. Only then change the print to console.error("worker terminated", error) so Node renders [cause] chains.

Note the order matters, and reviewer 1 independently found why: execFileSync's ENOBUFS SystemError carries output, stdout and stderr holding up to 32 MB of workspace diff. Printing the object before redaction exists would dump the workspace patch to stderr. And reviewer 2 found the mirror hazard: public-worker.ts:928-933 already embeds 4000 bytes of raw child stderr as a cause, which .stack currently hides — so fixing the formatting without fixing the redaction surfaces an existing leak.

Two reviewers, two different halves of the same trap. That is worth more than this PR was.

Also relevant

#311 now catches the ENOBUFS at source and names the offending paths, which addresses the diagnosis gap for this specific class without touching the worker's privacy boundary at all. Both reviewers said that change was worth more than this one, and I agree.

Leaving #307 open with the stderr-threading design recorded. Closing this rather than merging a comment I cannot stand behind.