[aviggiano]

Fixes #378.

This PR has been rejected by review three times. Each round found a real defect that would have
wedged or destroyed a run. The rejected approaches are summarised at the bottom, because the reasons
matter more than the diff.

What is broken

R54 lost its entire no-progress budget without doing any work. Generation 0 failed during workflow
submission (an unrelated npm publish race, #377). Generations 1 and 2 then each died in ~92 seconds having
written no events at all, so the node-error channel and the volume event log were both silent. The
cause existed only in the sandbox's own stderr:

worker terminated: Error: resume requires exactly one linked durable run, found 0

runs.jsonl is the journal linking an evaluation row to the durable run executing it, and that link is
appended only after the launcher returns. startRun creates the run root, writes state.json,
compiles the workflow and submits it before returning. So a real durable run can exist with nothing in the
journal pointing at it. The worker rejected that outright and, because the state lives on a durable volume,
did so again every generation. Recovery could never succeed at any budget.

The fix

Treat the journal as a cache, not the source of truth. When it holds no link, consult the run directory:

• A run linked to a workflow exists → resumable. The link is missing, not the run.
  repairModalEvalRunRecord writes it back when the run finalizes, so the next generation does not have to
  rediscover it. The repaired row is the row a successful launch would have written — including
  ultrafuzz_run_root and status: "launched", without which scoreEvalRun cannot resolve the report.
• No linked run exists → genuinely not started, and the caller is told what to clear before restarting.

The workflow link is the dividing line, and it is meaningful: persistSmithersEvidence writes it right
after compilation and before submission, and readLinkedWorkflowEvidence refuses any run without it.
So "compiled, then failed to submit" is resumable (the R54 case), while "died during compilation" never can
be — calling the latter resumable only relocates the wedge.

The destructive step, which is the most review-worthy part

Because both ids involved are deterministic, a restart trips over leftovers: runEvalSuite refuses an eval
run id whose directory exists (EVAL_RUN_ALREADY_EXISTS), and planRun refuses an existing run root
(RUN_ALREADY_EXISTS). The not-started branch therefore removes:

• staleEvalRunIds — eval run directories under control/, including ones with no eval.json, since the
  directory is created before that file is written;
• staleRunRootIds — run roots under target/ that carry no workflow link.

What bounds the second one: orphaned is a positive determination — the metadata was read successfully
and carries no link — not merely "not resumable". A root that is linked but is missing its state is
reported as damage and never deleted, because the link is written before submission and such a root is one
a workflow may have run from. Read faults (EACCES, EIO, ESTALE) are not swallowed on this path; only
ENOENT/ENOTDIR count as absence.

A malformed journal line still throws, deliberately: appendJsonLine does not fsync (#382), so a torn line
is possible, and the torn line may be the one carrying the link.

Tests

23 in packages/modal/test/resume.test.ts, covering each finding across the three rounds: an unlinked run
root, a linked root with no state, an eval run directory with no eval.json, a linked run with no
evaluation to attach it to, a journal that was never written, several durable runs on disk, a symlinked
control at zero candidates, the repaired row's ultrafuzz_run_root and status, and a journal pointing
at a different durable run.

Two couplings are pinned directly rather than restated, because restating them is what caused two of the
three rejections:

• staleEvalRunIds must name exactly evalRunRoot(control, id), the path runEvalSuite refuses to reuse;
• the workflow link must be read from layoutForRunRoot(root).runMetadataPath, and the fixture builds it
  through the same helper. Reverting that one line fails 5 tests.

Verification

• packages/modal/test/resume.test.ts — 23/23.
• npx vitest run packages/modal/test/ packages/evals/test/ — 523 pass. ci-config.test.ts fails 17/17
  with and without this change (verified by stashing): in this sandbox it resolves the repo root to
  /home. Pre-existing and environmental.
• tsc --noEmit, eslint, prettier clean.

Known limitation, stated rather than hidden

modelWorkStarted is still set before eval run on the not-started path, so a pre-model failure there
reports model work and resets the streak that MODAL_PRE_MODEL_RETRY_LIMIT counts. It was moved below the
durable-state check on the resume path, but the bound is narrowed, not restored. The real fix is the
corroborateModelWork pattern the public worker already has (#320); the private worker has no equivalent.
Out of scope here, and worth its own issue.

Note also that gating the flag on counts.succeeded > 0 would be wrong: reference nodes are marked
succeeded at plan time (plan-run.ts:689), before any model runs.

Rejected approaches, and why

Attempt 1 — restart instead of resume. A no-op: eval run ids are deterministic, so it hit
EVAL_RUN_ALREADY_EXISTS and wedged identically. Worse, it reported model work and so disabled the
3-attempt pre-model bound. Its premise — "zero linked runs means nothing was recorded, so restarting loses
nothing by construction" — was false, and false for R54, whose volume held a durable run with 9 succeeded
nodes. I had verified three downstream consumer claims about the repair; all three were correct and all
three were moot, because the path never executed.

Attempt 2 — resume, but repair the row badly. The repaired row lacked ultrafuzz_run_root, so the run
would resume, succeed, write the link, and then die in scoring. And state.json was used as the test for
resumability, though it is written long before compilation.

Attempt 3 — the right predicate, read from the wrong file. hasLinkedWorkflow read
run-metadata.json; the runtime writes run.json. Every real run root classified as unlinked, so #378 was
still unfixed and the new deletion pointed at precisely the roots it was meant to protect. The fixture
invented the same filename, so the suite agreed with the bug and 19/19 passed.

The common thread in all three: a claim about another module asserted rather than derived. The two pinned
couplings above exist to stop the fourth instance.

Greptile Summary

The PR makes Modal recovery distinguish resumable durable runs from attempts that never reached workflow linkage.

• Derives workflow metadata paths from the runtime layout helper and fails closed on damaged or ambiguous state.
• Clears deterministic eval and durable-run leftovers only when restarting a genuinely unstarted evaluation.
• Repairs missing journal links with the durable run root and launched status needed by downstream scoring.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
packages/modal/src/resume.ts	Adds conservative workspace classification, runtime-derived workflow-link detection, stale-path reporting, and missing-journal repair.
packages/modal/src/worker.ts	Routes recovery through the new classification and clears only paths identified as stale before restarting.
packages/modal/test/resume.test.ts	Covers linked and unlinked durable roots, damaged state, absent journals, deterministic collision cleanup, and repaired record fields.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Inspect eval journal and durable roots] --> B{Exactly one workflow-linked run?}
  B -->|Yes| C[Resume durable run]
  C --> D[Repair missing journal link]
  B -->|No| E{Only positively unlinked leftovers?}
  E -->|Yes| F[Remove stale deterministic paths]
  F --> G[Start evaluation normally]
  E -->|No: linked, damaged, or ambiguous| H[Refuse destructive recovery]

Loading

_{Reviews (6): Last reviewed commit: "fix(modal): classify eval run entries th..." | Re-trigger Greptile}

Context used:

• Knowledge Base — Modal remote benchmark execution
• Knowledge Base — Runtime: Run Orchestration Lifecycle

[aviggiano]

Reworked after both reviews. Thank you — both findings were correct and the first version was a no-op.

What changed. The fix no longer restarts. It repairs the missing link and resumes: when the journal
holds no ultrafuzz_run_id, the durable run directory is consulted, a run found there is resumed, and the
link is written back on finalize. Restarting is now reserved for the one state where no durable run exists
at all — and there the stale eval run directory is cleared first, so EVAL_RUN_ALREADY_EXISTS cannot fire.

On the premise. "Restarting loses nothing by construction" is withdrawn. It was false in general and
false for R54 specifically: its volume held a durable run with 9 succeeded nodes, and the monitor read
those counts from that run's state.json. Only the link was missing. public-worker.ts reporting
unknown rather than none for a submission failure was the existing evidence that I should have found.

Also fixed from the reviews: a missing runs.jsonl is classified rather than crashing ENOENT; non-ENOENT
readdir faults stay loud; the symlink assertion applies at every candidate count; and the doc comments
that asserted properties the code lacked have been rewritten to describe what it actually does.

On test level — the point that all three tests only exercised the newly added function was the sharpest
one, since that is exactly where the fix broke. The coupling is now pinned directly: staleEvalRunId is
asserted to be exactly evalRunRoot(control, id), the path runEvalSuite refuses to reuse. I want to be
straight that this is a coupling assertion, not a full runEvalSuite rerun.

Re-review requested on the new approach, particularly the link write-back in repairModalEvalRunRecord
and whether adopting a row is safe when the journal is empty and the row id comes from matrix.json.

[aviggiano]

Second round applied. Both re-reviews were blocking and both were right; three fixes pushed.

1. The repaired row was not scoreable. Confirmed: cloning the failed-launch row and stamping a link on
it produced a row with no ultrafuzz_run_root, so scoreEvalRun falls back to a path built from
row.run_id rather than the bounded run id on disk, and the run would drive to success and then die in
scoring — the wedge moved one step downstream. The repaired row now carries ultrafuzz_run_root and
status: "launched" (the latter because efficiency and status reporting read status, not final_status,
and would otherwise call a launch failed for a run that succeeded).

2. state.json was the wrong test for resumability. createRunLayout writes it long before
compilation, while persistSmithersEvidence writes the workflow link into run-metadata.json right after
compilation and before submission. So the link is exactly the line between "failed to submit"
(resumable — the R54 case) and "died while compiling" (resume refuses it forever with
WORKFLOW_RUN_ID_MISSING). A run root is now resumable only when linked; unlinked roots are named for
clearing, since a restart would otherwise trip RUN_ALREADY_EXISTS on their deterministic id. This closes
the second reviewer's plan-run-wrote-state-then-failed route as well.

3. modelWorkStarted. Now claimed only after the durable run state is readable, so a run that cannot
load its state fails inside the 3-attempt pre-model bound instead of burning the whole no-progress budget.

Claims withdrawn, both flagged as overstated:

• "a wide window — all of compilation and submission" — the compilation half is not resumable; that is
  now stated and handled rather than glossed.
• "the one state where there is provably nothing to abandon" — withdrawn. Note the reassurance that the
  restart never touches target/ no longer holds either: it now clears unlinked run roots there. The
  narrower true claim is that it only ever removes a run root carrying no workflow link, which means no
  workflow was ever submitted from it and resume could never have used it.

One correction to the re-review, on the causal story. The suggestion that R54's 9 succeeded nodes imply
the append must already have happened, and therefore that the lost-append fsync asymmetry is the cause,
does not fit its journal: the row read status=failed, ultrafuzz_run_id=None, which is exactly what
launchEvalRow writes when launch.ok is false. The append succeeded and correctly recorded a failed
launch; the 9 nodes were reference materialization during planning, before submission. So R54 is the
launch-failed window, not the lost-append window. The durability asymmetry is real and separate, and I have
filed it as #382 rather than folding it in here.

Not changed, deliberately: the soleRowId cross-check against boundedEvalId. It is good hardening,
but it is unreachable for this worker today (single-row suite, and >1 durable run already throws), and I
would rather not add a new coupling to the id-derivation formula in a PR that has already been wrong twice.
Worth a follow-up if soleRowId is ever used for a multi-row suite.

Tests: 19, including the two states that produced the blocking findings — an unlinked run root, and the
repaired row's ultrafuzz_run_root/status. The misnamed ENOTDIR test now says what it does and asserts
that nothing is named for deletion on that route.