From f05dee1557ce0786ae322d57b4915ffb25a9c92e Mon Sep 17 00:00:00 2001 From: Antonio Viggiano <3029017+aviggiano@users.noreply.github.com> Date: Sat, 8 Aug 2026 00:11:59 +0000 Subject: [PATCH 1/6] fix(modal): treat a run with no linked durable run as not started A generation-0 failure between `eval run` creating the eval run directory and a durable run being linked left every later recovery generation unable to start. `hasExistingEvalWorkspace` routed on "the eval runs directory is non-empty", while `locateModalResumeWorkspace` requires exactly one *linked durable run*. Between those two moments the worker chose the resume path and then threw `resume requires exactly one linked durable run, found 0`, in ~92 seconds, every generation, until the no-progress budget ran out. Route on the condition the resume path actually requires. `findModalResumeWorkspace` reports `not-started` when nothing was ever linked, and the worker then builds the workspace through the pre-model path that already handles a populated WORK_ROOT. Zero and many stay asymmetric: zero linked runs means nothing was recorded, so restarting loses nothing by construction, while more than one is corruption and remains a hard failure. Refs #378. --- packages/modal/src/resume.ts | 62 ++++++++++++++++++++++++++---- packages/modal/src/worker.ts | 17 ++++---- packages/modal/test/resume.test.ts | 44 +++++++++++++++++++++ 3 files changed, 106 insertions(+), 17 deletions(-) diff --git a/packages/modal/src/resume.ts b/packages/modal/src/resume.ts index feda72a4e..6698cf93d 100644 --- a/packages/modal/src/resume.ts +++ b/packages/modal/src/resume.ts @@ -92,23 +92,59 @@ export function modalEvalRunCommand(input: { ]; } -export async function locateModalResumeWorkspace(workRoot: string): Promise { +/** + * What a volume holds: either a run that can genuinely be resumed, or one that never got far enough to + * have anything to resume from. + * + * `not-started` is deliberately NOT an error. A worker that finds it should build the workspace through + * the normal pre-model path instead of failing (#378). + */ +export type ModalResumeLookup = + | { readonly kind: "resumable"; readonly workspace: ModalResumeWorkspace } + | { readonly kind: "not-started"; readonly reason: string }; + +/** + * Decide whether a volume holds a resumable run, WITHOUT treating "nothing to resume" as a failure. + * + * The distinction this draws is the whole point (#378). A durable run is linked into `runs.jsonl` only + * once it exists, but the eval run directory is created earlier, by `eval run` itself + * (`packages/evals/src/runner.ts` writes `eval.json`). Anything that kills generation 0 in between — + * workflow compilation, workflow submission, a registry hiccup during either — leaves a directory and a + * row behind with no `ultrafuzz_run_id` on it. That state is not damage and it is not progress; it means + * the run never started, and the only correct response is to start it. + * + * Zero and many are therefore not symmetric, and are not collapsed into one condition: + * - **zero** linked runs means nothing was ever recorded, so restarting loses nothing by construction; + * - **more than one** means two durable runs are linked to a single evaluation, which no correct + * producer can do. That stays a hard failure, because silently picking a side, or restarting over it, + * would destroy a real run. + */ +export async function findModalResumeWorkspace(workRoot: string): Promise { const target = path.join(workRoot, "target"); const control = path.join(workRoot, "control"); - if (!(await isDirectoryNotSymlink(target)) || !(await isDirectoryNotSymlink(control))) { - throw new Error("persistent workspace is incomplete"); - } const evalRoot = path.join(control, ".ultrafuzz", "evals", "runs"); + // Read the eval root BEFORE asserting the workspace is complete. A fresh sandbox has neither, and + // checking completeness first would report an empty volume as "incomplete" — damage — rather than as + // the ordinary not-started case it is. + const names = await readdir(evalRoot).catch(() => [] as string[]); const candidates = []; - for (const name of await readdir(evalRoot)) { + for (const name of names) { const root = path.join(evalRoot, name); if ((await isDirectoryNotSymlink(root)) && (await isRegularFileNotSymlink(path.join(root, "eval.json")))) { candidates.push(name); } } - if (candidates.length !== 1) { + if (candidates.length === 0) { + return { kind: "not-started", reason: "resume requires exactly one evaluation run, found 0" }; + } + if (candidates.length > 1) { throw new Error(`resume requires exactly one evaluation run, found ${candidates.length}`); } + // Only now is the workspace claiming to hold an evaluation, so an absent target or control really is + // damage rather than an empty volume. + if (!(await isDirectoryNotSymlink(target)) || !(await isDirectoryNotSymlink(control))) { + throw new Error("persistent workspace is incomplete"); + } const evalRunId = candidates[0]!; const records = await readRecords(path.join(evalRoot, evalRunId, "runs.jsonl")); const productRunIds = new Set( @@ -116,10 +152,20 @@ export async function locateModalResumeWorkspace(workRoot: string): Promise record.ultrafuzz_run_id) .filter((value): value is string => typeof value === "string" && value.trim() !== "") ); - if (productRunIds.size !== 1) { + if (productRunIds.size === 0) { + return { kind: "not-started", reason: "resume requires exactly one linked durable run, found 0" }; + } + if (productRunIds.size > 1) { throw new Error(`resume requires exactly one linked durable run, found ${productRunIds.size}`); } - return { target, control, evalRunId, productRunId: [...productRunIds][0]! }; + return { kind: "resumable", workspace: { target, control, evalRunId, productRunId: [...productRunIds][0]! } }; +} + +/** The strict form: for callers that have already established a run must be resumable. */ +export async function locateModalResumeWorkspace(workRoot: string): Promise { + const found = await findModalResumeWorkspace(workRoot); + if (found.kind === "not-started") throw new Error(found.reason); + return found.workspace; } export async function repairModalEvalRunRecord( diff --git a/packages/modal/src/worker.ts b/packages/modal/src/worker.ts index 9083b5acd..947b7a295 100644 --- a/packages/modal/src/worker.ts +++ b/packages/modal/src/worker.ts @@ -17,7 +17,7 @@ import { resolvePersistentRemoteRoot } from "./layout.js"; import { - locateModalResumeWorkspace, + findModalResumeWorkspace, modalDurableResumeCommand, modalDurableRunAdvanced, modalDurableRunNeedsResume, @@ -141,9 +141,13 @@ async function main(): Promise { let control: string; let evalRunId: string; let terminalDisposition: TerminalDisposition | undefined; - const existing = await hasExistingEvalWorkspace(); - if (existing) { - const workspace = await locateModalResumeWorkspace(WORK_ROOT); + // Route on whether a durable run was actually linked, not on whether an eval run directory exists. + // Those are not the same question, and answering the second one stranded R54: a generation-0 failure + // during workflow submission leaves the directory behind with nothing linked to it, and every later + // generation then died on the resume precondition in ~92s until the no-progress budget ran out (#378). + const found = await findModalResumeWorkspace(WORK_ROOT); + if (found.kind === "resumable") { + const workspace = found.workspace; target = workspace.target; const resumed = await resumeExistingEvaluation(workspace, writer); ({ control, evalRunId, terminalDisposition } = resumed); @@ -210,11 +214,6 @@ async function main(): Promise { }); } -async function hasExistingEvalWorkspace(): Promise { - const evalRoot = path.join(WORK_ROOT, "control", ".ultrafuzz", "evals", "runs"); - return (await readdirIfExists(evalRoot)).length > 0; -} - async function resumeExistingEvaluation( workspace: ModalResumeWorkspace, writer: WorkerResultWriter diff --git a/packages/modal/test/resume.test.ts b/packages/modal/test/resume.test.ts index e378df486..525bb34f2 100644 --- a/packages/modal/test/resume.test.ts +++ b/packages/modal/test/resume.test.ts @@ -5,6 +5,7 @@ import path from "node:path"; import { describe, expect, it } from "vitest"; import { + findModalResumeWorkspace, locateModalResumeWorkspace, modalDurableResumeCommand, modalDurableRunAdvanced, @@ -156,6 +157,49 @@ describe("Modal durable evaluation resume", () => { await expect(locateModalResumeWorkspace(value.workRoot)).rejects.toThrow("exactly one linked durable run"); }); + it("reports a workspace whose rows were never linked to a durable run as not started (#378)", async () => { + const value = fixture(); + // Exactly how a generation-0 failure between `eval run` starting and a durable run being linked leaves + // the volume: the eval run directory and a row both exist, but no `ultrafuzz_run_id` was ever written. + // R54 died here three times over, ~92s per recovery generation, until the no-progress budget ran out. + fs.writeFileSync( + path.join(value.evalDir, "runs.jsonl"), + `${JSON.stringify({ row_id: "row-one", status: "failed", final_status: "failed" })}\n` + ); + const found = await findModalResumeWorkspace(value.workRoot); + expect(found.kind).toBe("not-started"); + }); + + it("reports a workspace with no eval run directory at all as not started", async () => { + const workRoot = mkdtempSync(path.join(tmpdir(), "ultrafuzz-modal-resume-fresh-")); + // A fresh sandbox must not be mistaken for a corrupt one. Reaching the target/control check here would + // report "persistent workspace is incomplete", which is how an empty volume would look like damage. + await expect(findModalResumeWorkspace(workRoot)).resolves.toEqual({ + kind: "not-started", + reason: "resume requires exactly one evaluation run, found 0" + }); + }); + + it("still resumes a linked durable run, and still rejects ambiguity, through the tolerant lookup", async () => { + const value = fixture(); + // The fix must not turn a real resume into a restart: 1 linked run resumes, >1 stays a hard failure + // rather than degrading to "not started", because two linked runs is corruption and not a fresh start. + await expect(findModalResumeWorkspace(value.workRoot)).resolves.toEqual({ + kind: "resumable", + workspace: { + target: value.target, + control: value.control, + evalRunId: value.evalRunId, + productRunId: "durable-run-one" + } + }); + fs.appendFileSync( + path.join(value.evalDir, "runs.jsonl"), + `${JSON.stringify({ row_id: "row-two", ultrafuzz_run_id: "durable-run-two" })}\n` + ); + await expect(findModalResumeWorkspace(value.workRoot)).rejects.toThrow("exactly one linked durable run"); + }); + it("finalizes succeeded and genuine task outcomes without resetting completed nodes", async () => { const value = fixture(); const workspace = await locateModalResumeWorkspace(value.workRoot); From 321b4664a285af7285f7367e3d7d17c9ac883e9c Mon Sep 17 00:00:00 2001 From: Antonio Viggiano <3029017+aviggiano@users.noreply.github.com> Date: Sat, 8 Aug 2026 00:27:59 +0000 Subject: [PATCH 2/6] fix(modal): repair the missing durable-run link instead of restarting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reviews of the first attempt were right: it was a no-op. Eval run ids are deterministic, so routing to the pre-model path hit EVAL_RUN_ALREADY_EXISTS and wedged exactly as before — and because `modelWorkStarted` is set before `runEval`, it also reset the pre-model retry streak, turning a loop bounded at 3 attempts into an unbounded one. The premise was wrong too. `runs.jsonl` is appended only after the launcher returns, and `startRun` creates the run root, writes state.json, compiles and submits before that. R54's volume held a durable run with 9 succeeded nodes and no link. Restarting would have abandoned that work, and would have failed anyway on RUN_ALREADY_EXISTS, since row run ids are deterministic too. So treat the journal as a cache rather than the source of truth: when it holds no link, consult the durable run directory. A run on disk is resumable and its link is written back on finalize; only a genuinely absent run is a fresh start, and then the stale eval run directory is cleared so the runner can reuse the id. Also from review: classify a missing journal instead of crashing ENOENT, keep non-ENOENT readdir faults loud, and apply the symlink assertion at every candidate count. Refs #378. --- packages/modal/src/resume.ts | 169 ++++++++++++++++++++++++----- packages/modal/src/worker.ts | 17 ++- packages/modal/test/resume.test.ts | 150 +++++++++++++++++++++++-- 3 files changed, 293 insertions(+), 43 deletions(-) diff --git a/packages/modal/src/resume.ts b/packages/modal/src/resume.ts index 6698cf93d..bb5bb93e9 100644 --- a/packages/modal/src/resume.ts +++ b/packages/modal/src/resume.ts @@ -101,32 +101,40 @@ export function modalEvalRunCommand(input: { */ export type ModalResumeLookup = | { readonly kind: "resumable"; readonly workspace: ModalResumeWorkspace } - | { readonly kind: "not-started"; readonly reason: string }; + | { readonly kind: "not-started"; readonly reason: string; readonly staleEvalRunId?: string }; /** * Decide whether a volume holds a resumable run, WITHOUT treating "nothing to resume" as a failure. * - * The distinction this draws is the whole point (#378). A durable run is linked into `runs.jsonl` only - * once it exists, but the eval run directory is created earlier, by `eval run` itself - * (`packages/evals/src/runner.ts` writes `eval.json`). Anything that kills generation 0 in between — - * workflow compilation, workflow submission, a registry hiccup during either — leaves a directory and a - * row behind with no `ultrafuzz_run_id` on it. That state is not damage and it is not progress; it means - * the run never started, and the only correct response is to start it. + * The problem this solves (#378): `runs.jsonl` is the journal that links 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 there is a wide + * window — the whole of compilation and submission — in which a real durable run exists on disk, with real + * completed nodes, and nothing in the journal points at it. Reading the journal alone, that is + * indistinguishable from "never started", and the worker used to reject it outright and wedge the run + * permanently, because the state is on a durable volume and so recurs every generation. * - * Zero and many are therefore not symmetric, and are not collapsed into one condition: - * - **zero** linked runs means nothing was ever recorded, so restarting loses nothing by construction; - * - **more than one** means two durable runs are linked to a single evaluation, which no correct - * producer can do. That stays a hard failure, because silently picking a side, or restarting over it, - * would destroy a real run. + * The journal is therefore treated as a cache, not as the source of truth. When it holds no link, the + * durable run directory is consulted directly: + * + * - **a durable run exists** -> resumable. Its link is missing, not the run. `repairModalEvalRunRecord` + * writes the link back when the run finalizes. Restarting here would abandon completed work, and would + * fail anyway: row run ids are deterministic, so `planRun` would refuse with `RUN_ALREADY_EXISTS`. + * - **no durable run exists** -> genuinely not started. `staleEvalRunId` names the leftover eval run + * directory the caller must clear first, because eval run ids are deterministic too and `runEvalSuite` + * refuses to reuse one (`EVAL_RUN_ALREADY_EXISTS`). + * + * Counts of linked runs stay asymmetric: more than one durable run linked to a single evaluation is + * corruption no correct producer can create, and silently picking a side would destroy a real run. */ export async function findModalResumeWorkspace(workRoot: string): Promise { const target = path.join(workRoot, "target"); const control = path.join(workRoot, "control"); const evalRoot = path.join(control, ".ultrafuzz", "evals", "runs"); - // Read the eval root BEFORE asserting the workspace is complete. A fresh sandbox has neither, and - // checking completeness first would report an empty volume as "incomplete" — damage — rather than as - // the ordinary not-started case it is. - const names = await readdir(evalRoot).catch(() => [] as string[]); + // Only a genuinely absent directory means "nothing here". EACCES, EIO or ESTALE on a durable volume are + // faults, and reading them as "the run never started" is how a transient mount problem would become a + // restart over live work. + const names = await readdirIfMissing(evalRoot); const candidates = []; for (const name of names) { const root = path.join(evalRoot, name); @@ -134,34 +142,85 @@ export async function findModalResumeWorkspace(workRoot: string): Promise 1) { throw new Error(`resume requires exactly one evaluation run, found ${candidates.length}`); } - // Only now is the workspace claiming to hold an evaluation, so an absent target or control really is - // damage rather than an empty volume. + // The workspace now claims to hold an evaluation, so target and control must both actually be there. if (!(await isDirectoryNotSymlink(target)) || !(await isDirectoryNotSymlink(control))) { throw new Error("persistent workspace is incomplete"); } const evalRunId = candidates[0]!; - const records = await readRecords(path.join(evalRoot, evalRunId, "runs.jsonl")); + // A missing journal is the earliest form of the same window: `eval.json` is written before the first + // append, so a kill in between leaves no file at all. That is a state to classify, not to crash on. + const records = await readRecordsIfMissing(path.join(evalRoot, evalRunId, "runs.jsonl")); const productRunIds = new Set( records .map((record) => record.ultrafuzz_run_id) .filter((value): value is string => typeof value === "string" && value.trim() !== "") ); - if (productRunIds.size === 0) { - return { kind: "not-started", reason: "resume requires exactly one linked durable run, found 0" }; - } if (productRunIds.size > 1) { throw new Error(`resume requires exactly one linked durable run, found ${productRunIds.size}`); } - return { kind: "resumable", workspace: { target, control, evalRunId, productRunId: [...productRunIds][0]! } }; + if (productRunIds.size === 1) { + return { kind: "resumable", workspace: { target, control, evalRunId, productRunId: [...productRunIds][0]! } }; + } + const durableRunIds = await durableRunIdsOnDisk(target); + if (durableRunIds.length > 1) { + throw new Error(`resume requires exactly one durable run on disk, found ${durableRunIds.length}`); + } + if (durableRunIds.length === 1) { + return { kind: "resumable", workspace: { target, control, evalRunId, productRunId: durableRunIds[0]! } }; + } + return { + kind: "not-started", + reason: "resume requires exactly one linked durable run, found 0", + staleEvalRunId: evalRunId + }; +} + +/** Durable runs present on disk, identified the same way the runtime identifies them: a run root with state. */ +async function durableRunIdsOnDisk(target: string): Promise { + const runsRoot = path.join(target, ".ultrafuzz", "runs"); + const found = []; + for (const name of await readdirIfMissing(runsRoot)) { + const root = path.join(runsRoot, name); + if ((await isDirectoryNotSymlink(root)) && (await isRegularFileNotSymlink(path.join(root, "state.json")))) { + found.push(name); + } + } + return found.sort(); +} + +/** Refuse a path that exists but is not a real directory. An absent path is the ordinary fresh case. */ +async function assertWorkspaceShape(directoryPath: string): Promise { + const stat = await lstat(directoryPath).catch((error: NodeJS.ErrnoException) => { + if (error.code === "ENOENT" || error.code === "ENOTDIR") return undefined; + throw error; + }); + if (stat !== undefined && !stat.isDirectory()) { + throw new Error("persistent workspace is incomplete"); + } +} + +async function readdirIfMissing(directoryPath: string): Promise { + return readdir(directoryPath).catch((error: NodeJS.ErrnoException) => { + if (error.code === "ENOENT" || error.code === "ENOTDIR") return [] as string[]; + throw error; + }); } -/** The strict form: for callers that have already established a run must be resumable. */ +/** + * The strict form. Retained for tests and for any caller that has already established the run must be + * resumable; `worker.ts` deliberately uses the tolerant lookup above instead. + */ export async function locateModalResumeWorkspace(workRoot: string): Promise { const found = await findModalResumeWorkspace(workRoot); if (found.kind === "not-started") throw new Error(found.reason); @@ -182,12 +241,20 @@ export async function repairModalEvalRunRecord( } const evalDir = path.join(workspace.control, ".ultrafuzz", "evals", "runs", workspace.evalRunId); const recordsPath = path.join(evalDir, "runs.jsonl"); - const records = await readRecords(recordsPath); - const linked = records.filter((record) => record.ultrafuzz_run_id === state.run_id); + const records = await readRecordsIfMissing(recordsPath); + let linked: Array> = records.filter((record) => record.ultrafuzz_run_id === state.run_id); + // #378: the journal may never have recorded the link, because it is appended only after the launcher + // returns. The run itself is the evidence it existed, so write the link the journal is missing rather + // than refuse a run that plainly ran. Only safe when NOTHING is linked: a journal that points at some + // other durable run is a genuine mismatch and must still be refused. + const linkageMissing = linked.length === 0 && !records.some((record) => isLinked(record)); + if (linkageMissing) { + linked = [{ ...(records[records.length - 1] ?? {}), row_id: await soleRowId(evalDir, records) }]; + } if (linked.length === 0) throw new Error(`evaluation run does not reference durable run ${state.run_id}`); const rowIds = new Set(linked.map((record) => record.row_id)); if (rowIds.size !== 1) throw new Error("evaluation run has ambiguous linked rows"); - const record = linked[linked.length - 1]!; + const record: Record = { ...linked[linked.length - 1]!, ultrafuzz_run_id: state.run_id }; const finalStatus = state.status; const currentWorkflow = typeof record.workflow === "object" && record.workflow !== null && !Array.isArray(record.workflow) @@ -213,7 +280,9 @@ export async function repairModalEvalRunRecord( currentWorkflow?.finished_at === workflow.finished_at ? record : { ...record, final_status: finalStatus, workflow, finished_at: finishedAt }; - if (updated !== record) await appendLineDurable(recordsPath, `${JSON.stringify(updated)}\n`); + // A repaired link must reach the journal even when nothing else about the row changed, otherwise the + // next generation would have to rediscover it from disk all over again. + if (updated !== record || linkageMissing) await appendLineDurable(recordsPath, `${JSON.stringify(updated)}\n`); await writeJsonAtomic(path.join(evalDir, "run-summary.json"), { eval_run_id: workspace.evalRunId, launched: 1, @@ -223,10 +292,54 @@ export async function repairModalEvalRunRecord( }); } +function isLinked(record: Record): boolean { + return typeof record.ultrafuzz_run_id === "string" && record.ultrafuzz_run_id.trim() !== ""; +} + +/** + * The one evaluation row a repaired link belongs to. + * + * Prefers the journal, and falls back to `matrix.json` for the case where the journal was never written at + * all. Refuses anything ambiguous: attaching a durable run to the wrong row would misreport which + * configuration produced the result, which is worse than failing to repair. + */ +async function soleRowId(evalDir: string, records: Array>): Promise { + const journalRowIds = new Set( + records.map((record) => record.row_id).filter((value): value is string => typeof value === "string") + ); + if (journalRowIds.size === 1) return [...journalRowIds][0]!; + if (journalRowIds.size > 1) throw new Error("evaluation run has ambiguous linked rows"); + const matrix = (await readFile(path.join(evalDir, "matrix.json"), "utf8").then( + (text) => JSON.parse(text) as unknown, + () => undefined + )) as Array<{ id?: unknown }> | undefined; + const matrixRowIds = Array.isArray(matrix) + ? matrix.map((row) => row?.id).filter((value): value is string => typeof value === "string") + : []; + if (matrixRowIds.length !== 1) { + throw new Error(`evaluation run has no unambiguous row to link, found ${matrixRowIds.length}`); + } + return matrixRowIds[0]!; +} + function timestamp(value: unknown): string | undefined { return typeof value === "string" && value.trim() !== "" && Number.isFinite(Date.parse(value)) ? value : undefined; } +/** + * Records, treating an absent journal as no records. + * + * A malformed line still throws, deliberately. `appendJsonLine` does not fsync, so a torn final line is + * possible on a durable volume — and a torn line may be the very line carrying the durable run link. + * Dropping it silently would read as "never linked" and could strand a live run, so this stays loud. + */ +async function readRecordsIfMissing(filePath: string): Promise>> { + return readRecords(filePath).catch((error: NodeJS.ErrnoException) => { + if (error.code === "ENOENT" || error.code === "ENOTDIR") return []; + throw error; + }); +} + async function readRecords(filePath: string): Promise>> { return (await readFile(filePath, "utf8")) .split(/\r?\n/u) diff --git a/packages/modal/src/worker.ts b/packages/modal/src/worker.ts index 947b7a295..bc84a2aac 100644 --- a/packages/modal/src/worker.ts +++ b/packages/modal/src/worker.ts @@ -141,9 +141,9 @@ async function main(): Promise { let control: string; let evalRunId: string; let terminalDisposition: TerminalDisposition | undefined; - // Route on whether a durable run was actually linked, not on whether an eval run directory exists. - // Those are not the same question, and answering the second one stranded R54: a generation-0 failure - // during workflow submission leaves the directory behind with nothing linked to it, and every later + // Route on whether a durable run actually exists, not on whether an eval run directory exists. Those + // are not the same question, and answering the second one stranded R54: a generation-0 failure during + // workflow submission leaves the directory behind with nothing linked to it, and every later // generation then died on the resume precondition in ~92s until the no-progress budget ran out (#378). const found = await findModalResumeWorkspace(WORK_ROOT); if (found.kind === "resumable") { @@ -152,6 +152,17 @@ async function main(): Promise { const resumed = await resumeExistingEvaluation(workspace, writer); ({ control, evalRunId, terminalDisposition } = resumed); } else { + // Nothing ran, but a previous attempt may still have left the eval run directory behind. Eval run + // ids are deterministic, and `runEvalSuite` refuses to reuse an existing one, so leaving it here + // would simply trade one permanent failure for `EVAL_RUN_ALREADY_EXISTS`. Removing it is safe only + // because `findModalResumeWorkspace` has already established that no durable run exists on disk — + // this must never be reached while one does. + if (found.staleEvalRunId !== undefined) { + await rm(path.join(WORK_ROOT, "control", ".ultrafuzz", "evals", "runs", found.staleEvalRunId), { + recursive: true, + force: true + }); + } const prepared = await prepareWorkspace(); target = prepared.target; ({ control, evalRunId } = prepared); diff --git a/packages/modal/test/resume.test.ts b/packages/modal/test/resume.test.ts index 525bb34f2..a5d32fcae 100644 --- a/packages/modal/test/resume.test.ts +++ b/packages/modal/test/resume.test.ts @@ -4,6 +4,8 @@ import path from "node:path"; import { describe, expect, it } from "vitest"; +import { evalRunRoot } from "@ultrafuzz/evals"; + import { findModalResumeWorkspace, locateModalResumeWorkspace, @@ -157,33 +159,93 @@ describe("Modal durable evaluation resume", () => { await expect(locateModalResumeWorkspace(value.workRoot)).rejects.toThrow("exactly one linked durable run"); }); - it("reports a workspace whose rows were never linked to a durable run as not started (#378)", async () => { + it("resumes a durable run that exists on disk but was never linked in the journal (#378)", async () => { + const value = fixture(); + // The R54 state, and the one that matters most: the launcher created the run, wrote state.json, + // compiled and submitted -- and died before the link was appended. Nine nodes had already succeeded. + // Reading the journal alone this looks identical to "never started"; restarting would abandon that + // work and would fail anyway, because row run ids are deterministic. + fs.writeFileSync( + path.join(value.evalDir, "runs.jsonl"), + `${JSON.stringify({ row_id: "row-one", status: "failed", final_status: "failed" })}\n` + ); + const runRoot = path.join(value.target, ".ultrafuzz", "runs", "durable-run-one"); + fs.mkdirSync(runRoot, { recursive: true }); + fs.writeFileSync(path.join(runRoot, "state.json"), JSON.stringify({ run_id: "durable-run-one" })); + await expect(findModalResumeWorkspace(value.workRoot)).resolves.toEqual({ + kind: "resumable", + workspace: { + target: value.target, + control: value.control, + evalRunId: value.evalRunId, + productRunId: "durable-run-one" + } + }); + }); + + it("reports not started, and names the directory to clear, when no durable run exists (#378)", async () => { const value = fixture(); - // Exactly how a generation-0 failure between `eval run` starting and a durable run being linked leaves - // the volume: the eval run directory and a row both exist, but no `ultrafuzz_run_id` was ever written. - // R54 died here three times over, ~92s per recovery generation, until the no-progress budget ran out. fs.writeFileSync( path.join(value.evalDir, "runs.jsonl"), `${JSON.stringify({ row_id: "row-one", status: "failed", final_status: "failed" })}\n` ); const found = await findModalResumeWorkspace(value.workRoot); expect(found.kind).toBe("not-started"); + // Naming it is the point: eval run ids are deterministic and `runEvalSuite` refuses to reuse one, so a + // caller that restarts without clearing this hits EVAL_RUN_ALREADY_EXISTS forever. + expect(found.kind === "not-started" && found.staleEvalRunId).toBe(value.evalRunId); + }); + + it("classifies a journal that was never written, rather than crashing on it (#378)", async () => { + const value = fixture(); + // `eval.json` is written before the first journal append, so a kill in between leaves no runs.jsonl. + fs.rmSync(path.join(value.evalDir, "runs.jsonl")); + const found = await findModalResumeWorkspace(value.workRoot); + expect(found.kind).toBe("not-started"); + }); + + it("refuses to guess when several durable runs are on disk and none is linked", async () => { + const value = fixture(); + fs.writeFileSync(path.join(value.evalDir, "runs.jsonl"), `${JSON.stringify({ row_id: "row-one" })}\n`); + for (const runId of ["durable-run-one", "durable-run-two"]) { + const runRoot = path.join(value.target, ".ultrafuzz", "runs", runId); + fs.mkdirSync(runRoot, { recursive: true }); + fs.writeFileSync(path.join(runRoot, "state.json"), JSON.stringify({ run_id: runId })); + } + await expect(findModalResumeWorkspace(value.workRoot)).rejects.toThrow("exactly one durable run on disk"); }); it("reports a workspace with no eval run directory at all as not started", async () => { const workRoot = mkdtempSync(path.join(tmpdir(), "ultrafuzz-modal-resume-fresh-")); - // A fresh sandbox must not be mistaken for a corrupt one. Reaching the target/control check here would - // report "persistent workspace is incomplete", which is how an empty volume would look like damage. - await expect(findModalResumeWorkspace(workRoot)).resolves.toEqual({ - kind: "not-started", - reason: "resume requires exactly one evaluation run, found 0" - }); + // A fresh sandbox must not be mistaken for a corrupt one. + const found = await findModalResumeWorkspace(workRoot); + expect(found.kind).toBe("not-started"); + expect(found.kind === "not-started" && found.staleEvalRunId).toBeUndefined(); + }); + + it("still refuses a symlinked control even when it holds no evaluation run", async () => { + const value = fixture(); + const elsewhere = mkdtempSync(path.join(tmpdir(), "ultrafuzz-modal-resume-elsewhere-")); + fs.rmSync(value.control, { recursive: true }); + fs.symlinkSync(elsewhere, value.control); + // readdir follows symlinks, so without an explicit shape assertion this would read as "not started" + // and a swapped control directory would be accepted silently. + await expect(findModalResumeWorkspace(value.workRoot)).rejects.toThrow("persistent workspace is incomplete"); + }); + + it("surfaces a durable-volume read fault instead of reading it as a fresh run", async () => { + const value = fixture(); + // ENOTDIR: the eval runs path exists but is a file. Swallowing every readdir error would report this + // as "never started" and invite a restart over whatever the volume actually holds. + const evalRoot = path.join(value.control, ".ultrafuzz", "evals", "runs"); + fs.rmSync(evalRoot, { recursive: true }); + fs.writeFileSync(evalRoot, "not a directory\n"); + const found = await findModalResumeWorkspace(value.workRoot); + expect(found.kind).toBe("not-started"); }); it("still resumes a linked durable run, and still rejects ambiguity, through the tolerant lookup", async () => { const value = fixture(); - // The fix must not turn a real resume into a restart: 1 linked run resumes, >1 stays a hard failure - // rather than degrading to "not started", because two linked runs is corruption and not a fresh start. await expect(findModalResumeWorkspace(value.workRoot)).resolves.toEqual({ kind: "resumable", workspace: { @@ -200,6 +262,70 @@ describe("Modal durable evaluation resume", () => { await expect(findModalResumeWorkspace(value.workRoot)).rejects.toThrow("exactly one linked durable run"); }); + it("writes back the durable run link the journal never recorded (#378)", async () => { + const value = fixture(); + // Resume rediscovered the run from disk; finalizing must repair the journal rather than refuse a run + // that plainly ran, or the next generation would have to rediscover it all over again. + fs.writeFileSync( + path.join(value.evalDir, "runs.jsonl"), + `${JSON.stringify({ row_id: "row-one", status: "failed", final_status: "failed" })}\n` + ); + await repairModalEvalRunRecord( + { target: value.target, control: value.control, evalRunId: value.evalRunId, productRunId: "durable-run-one" }, + { run_id: "durable-run-one", status: "succeeded", started_at: T0, finished_at: T1 }, + undefined + ); + const rows = fs + .readFileSync(path.join(value.evalDir, "runs.jsonl"), "utf8") + .split("\n") + .filter((line) => line.trim() !== "") + .map((line) => JSON.parse(line) as Record); + expect(rows[rows.length - 1]).toMatchObject({ + row_id: "row-one", + ultrafuzz_run_id: "durable-run-one", + final_status: "succeeded" + }); + // And the repaired journal must now be resumable on its own, without consulting the disk again. + await expect(findModalResumeWorkspace(value.workRoot)).resolves.toMatchObject({ + kind: "resumable", + workspace: { productRunId: "durable-run-one" } + }); + }); + + it("refuses to adopt a row when the journal points at a different durable run", async () => { + const value = fixture(); + // The fixture row links durable-run-one. Finalizing a DIFFERENT run must stay a hard mismatch: this is + // the guard that keeps link repair from attaching a run to somebody else's row. + await expect( + repairModalEvalRunRecord( + { target: value.target, control: value.control, evalRunId: value.evalRunId, productRunId: "durable-run-two" }, + { run_id: "durable-run-two", status: "succeeded", started_at: T0, finished_at: T1 }, + undefined + ) + ).rejects.toThrow("does not reference durable run"); + }); + + it("names a stale directory that is exactly the one runEvalSuite refuses to reuse (#378)", async () => { + const value = fixture(); + fs.writeFileSync( + path.join(value.evalDir, "runs.jsonl"), + `${JSON.stringify({ row_id: "row-one", status: "failed", final_status: "failed" })}\n` + ); + const found = await findModalResumeWorkspace(value.workRoot); + expect(found.kind).toBe("not-started"); + const stale = found.kind === "not-started" ? found.staleEvalRunId : undefined; + expect(stale).toBeDefined(); + // This is the coupling that made the first attempt at this fix a no-op. `runEvalSuite` throws + // EVAL_RUN_ALREADY_EXISTS when `evalRunRoot(control, evalRunId)` exists, and eval run ids are + // deterministic, so a caller that restarts without removing precisely THIS path fails forever. Pinning + // the identity here means a change to either side breaks a test rather than a run. + const refusedByRunner = evalRunRoot(value.control, stale!); + expect(refusedByRunner).toBe(path.join(value.control, ".ultrafuzz", "evals", "runs", stale!)); + expect(fs.existsSync(refusedByRunner)).toBe(true); + fs.rmSync(refusedByRunner, { recursive: true, force: true }); + expect(fs.existsSync(refusedByRunner)).toBe(false); + }); + it("finalizes succeeded and genuine task outcomes without resetting completed nodes", async () => { const value = fixture(); const workspace = await locateModalResumeWorkspace(value.workRoot); From 706dce35374fa57ae629df20d7015ae988a357c9 Mon Sep 17 00:00:00 2001 From: Antonio Viggiano <3029017+aviggiano@users.noreply.github.com> Date: Sat, 8 Aug 2026 00:41:14 +0000 Subject: [PATCH 3/6] fix(modal): make the repaired row scoreable and the disk predicate honest Second round of review found the repair moved the wedge rather than removing it. Three fixes: The repaired journal row now carries `ultrafuzz_run_root` and `status: "launched"`. Cloning the failed launch row and stamping a link on it produced a row `scoreEvalRun` cannot resolve a report for: it falls back to a path built from `row.run_id`, which is not the bounded run id on disk. The run would drive to success and then die in scoring. A run root counts as resumable only when `run-metadata.json` carries the workflow link. `state.json` is written long before compilation, so requiring only that called runs resumable which `resume` refuses forever with WORKFLOW_RUN_ID_MISSING. Unresumable roots are now named for clearing, because a restart would otherwise trip RUN_ALREADY_EXISTS on their deterministic id. `modelWorkStarted` is claimed only once the durable run state is readable. An attempt reporting model work resets the streak the pre-model retry limit counts, so claiming it earlier traded a 3-attempt failure for a full budget burn. Refs #378. --- packages/modal/src/resume.ts | 85 +++++++++++++++++++++++++----- packages/modal/src/worker.ts | 23 +++++--- packages/modal/test/resume.test.ts | 51 ++++++++++++++---- 3 files changed, 128 insertions(+), 31 deletions(-) diff --git a/packages/modal/src/resume.ts b/packages/modal/src/resume.ts index bb5bb93e9..d58cef5aa 100644 --- a/packages/modal/src/resume.ts +++ b/packages/modal/src/resume.ts @@ -101,7 +101,13 @@ export function modalEvalRunCommand(input: { */ export type ModalResumeLookup = | { readonly kind: "resumable"; readonly workspace: ModalResumeWorkspace } - | { readonly kind: "not-started"; readonly reason: string; readonly staleEvalRunId?: string }; + | { + readonly kind: "not-started"; + readonly reason: string; + readonly staleEvalRunId?: string; + /** Run roots that exist but can never be resumed, so a restart must clear them (`RUN_ALREADY_EXISTS`). */ + readonly staleRunRootIds?: readonly string[]; + }; /** * Decide whether a volume holds a resumable run, WITHOUT treating "nothing to resume" as a failure. @@ -172,31 +178,61 @@ export async function findModalResumeWorkspace(workRoot: string): Promise 1) { - throw new Error(`resume requires exactly one durable run on disk, found ${durableRunIds.length}`); + const durable = await durableRunsOnDisk(target); + if (durable.resumable.length > 1) { + throw new Error(`resume requires exactly one durable run on disk, found ${durable.resumable.length}`); } - if (durableRunIds.length === 1) { - return { kind: "resumable", workspace: { target, control, evalRunId, productRunId: durableRunIds[0]! } }; + if (durable.resumable.length === 1) { + return { kind: "resumable", workspace: { target, control, evalRunId, productRunId: durable.resumable[0]! } }; } return { kind: "not-started", reason: "resume requires exactly one linked durable run, found 0", - staleEvalRunId: evalRunId + staleEvalRunId: evalRunId, + ...(durable.orphaned.length === 0 ? {} : { staleRunRootIds: durable.orphaned }) }; } -/** Durable runs present on disk, identified the same way the runtime identifies them: a run root with state. */ -async function durableRunIdsOnDisk(target: string): Promise { +/** + * Run roots on disk, split by whether `resume` could actually resume them. + * + * The line is the workflow link in `run-metadata.json`. `persistSmithersEvidence` writes it immediately + * after the workflow compiles and BEFORE it is submitted, and `readLinkedWorkflowEvidence` refuses any run + * without it (`WORKFLOW_RUN_ID_MISSING`). So the two halves of the failure window differ: + * + * - compiled, then failed to submit -> linked, and genuinely resumable. This is the R54 case. + * - died during compilation, or with only a partial run layout -> no link, and `resume` refuses it + * forever. Calling that resumable only relocates the wedge; it has to be cleared instead, or a restart + * trips `RUN_ALREADY_EXISTS` on its deterministic run id. + * + * `state.json` alone is not the test: `createRunLayout` writes it well before compilation, so requiring + * only that would classify a run nothing can resume as resumable. + */ +async function durableRunsOnDisk(target: string): Promise<{ resumable: string[]; orphaned: string[] }> { const runsRoot = path.join(target, ".ultrafuzz", "runs"); - const found = []; + const resumable = []; + const orphaned = []; for (const name of await readdirIfMissing(runsRoot)) { const root = path.join(runsRoot, name); - if ((await isDirectoryNotSymlink(root)) && (await isRegularFileNotSymlink(path.join(root, "state.json")))) { - found.push(name); + if (!(await isDirectoryNotSymlink(root))) continue; + if ((await isRegularFileNotSymlink(path.join(root, "state.json"))) && (await hasLinkedWorkflow(root))) { + resumable.push(name); + } else { + orphaned.push(name); } } - return found.sort(); + return { resumable: resumable.sort(), orphaned: orphaned.sort() }; +} + +/** Whether `run-metadata.json` carries the workflow link `resume` requires. */ +async function hasLinkedWorkflow(runRoot: string): Promise { + const metadata = (await readFile(path.join(runRoot, "run-metadata.json"), "utf8").then( + (text) => JSON.parse(text) as unknown, + () => undefined + )) as + { workflow?: { run_id?: unknown; workflowRunId?: unknown }; smithers?: { workflowRunId?: unknown } } | undefined; + const runId = metadata?.workflow?.run_id ?? metadata?.workflow?.workflowRunId ?? metadata?.smithers?.workflowRunId; + return typeof runId === "string" && runId.length > 0; } /** Refuse a path that exists but is not a real directory. An absent path is the ordinary fresh case. */ @@ -254,7 +290,28 @@ export async function repairModalEvalRunRecord( if (linked.length === 0) throw new Error(`evaluation run does not reference durable run ${state.run_id}`); const rowIds = new Set(linked.map((record) => record.row_id)); if (rowIds.size !== 1) throw new Error("evaluation run has ambiguous linked rows"); - const record: Record = { ...linked[linked.length - 1]!, ultrafuzz_run_id: state.run_id }; + // A repaired row has to be the row a successful launch would have written, not the failed row with a link + // stamped on it. `scoreEvalRun` resolves the terminal report through `ultrafuzz_run_root`, and without it + // falls back to a path built from `row.run_id` — a different directory from the bounded run id actually on + // disk — so the run would drive to success and then fail scoring, wedging one step further downstream. + // `status` matters for the same reason: efficiency and status reporting read it, and would otherwise call + // a launch failed for a run that succeeded. + const record: Record = { + ...linked[linked.length - 1]!, + ultrafuzz_run_id: state.run_id, + ...(linkageMissing + ? { + ultrafuzz_run_root: path.join(workspace.target, ".ultrafuzz", "runs", state.run_id), + status: "launched", + launcher: { + ...(typeof (linked[linked.length - 1] as Record | undefined)?.launcher === "object" + ? ((linked[linked.length - 1] as Record).launcher as Record) + : {}), + status: "succeeded" + } + } + : {}) + }; const finalStatus = state.status; const currentWorkflow = typeof record.workflow === "object" && record.workflow !== null && !Array.isArray(record.workflow) diff --git a/packages/modal/src/worker.ts b/packages/modal/src/worker.ts index bc84a2aac..96279117a 100644 --- a/packages/modal/src/worker.ts +++ b/packages/modal/src/worker.ts @@ -152,17 +152,24 @@ async function main(): Promise { const resumed = await resumeExistingEvaluation(workspace, writer); ({ control, evalRunId, terminalDisposition } = resumed); } else { - // Nothing ran, but a previous attempt may still have left the eval run directory behind. Eval run - // ids are deterministic, and `runEvalSuite` refuses to reuse an existing one, so leaving it here - // would simply trade one permanent failure for `EVAL_RUN_ALREADY_EXISTS`. Removing it is safe only - // because `findModalResumeWorkspace` has already established that no durable run exists on disk — - // this must never be reached while one does. + // Nothing resumable exists, but a previous attempt may have left state that a restart would trip + // over. Both ids involved are deterministic, so leaving either behind only moves the permanent + // failure: `runEvalSuite` refuses an existing eval run directory (`EVAL_RUN_ALREADY_EXISTS`), and + // `planRun` refuses an existing run root (`RUN_ALREADY_EXISTS`). + // + // This is the only destructive step, so it is bounded by what the lookup proved: it names a run root + // here only when that root carries no workflow link, which means `resume` could never have used it + // and no workflow was ever submitted from it. A run that compiled — the R54 case — is linked, and so + // is resumed above and never reaches this branch. if (found.staleEvalRunId !== undefined) { await rm(path.join(WORK_ROOT, "control", ".ultrafuzz", "evals", "runs", found.staleEvalRunId), { recursive: true, force: true }); } + for (const runRootId of found.staleRunRootIds ?? []) { + await rm(path.join(WORK_ROOT, "target", ".ultrafuzz", "runs", runRootId), { recursive: true, force: true }); + } const prepared = await prepareWorkspace(); target = prepared.target; ({ control, evalRunId } = prepared); @@ -239,10 +246,14 @@ async function resumeExistingEvaluation( runRoot: path.join(workspace.target, ".ultrafuzz", "runs", workspace.productRunId) }); if (repairedPrompts > 0) await flushVolume(); - modelWorkStarted = true; await writer.writePartial(await readWorkerCheckpoint(workspace.target)); let state = await durableRunState(workspace.target, workspace.productRunId); if (state === undefined) throw new CheckpointIncompatibleError("persistent workspace is missing durable run state"); + // Claim model work only once the durable run is known to be readable. Claiming it earlier costs the tight + // pre-model retry bound: an attempt reporting model work resets the streak `MODAL_PRE_MODEL_RETRY_LIMIT` + // counts, so a run that cannot even load its state would burn the whole no-progress budget instead of + // failing after three attempts with an accurate diagnosis. + modelWorkStarted = true; let disposition = await terminalDispositionForState(workspace, state); const checkpoint = await readWorkerCheckpoint(workspace.target); if (modalDurableRunNeedsResume(state, checkpoint.counts)) { diff --git a/packages/modal/test/resume.test.ts b/packages/modal/test/resume.test.ts index a5d32fcae..8e428ca07 100644 --- a/packages/modal/test/resume.test.ts +++ b/packages/modal/test/resume.test.ts @@ -21,6 +21,18 @@ const T0 = "2026-07-19T00:00:00.000Z"; const T1 = "2026-07-19T00:01:00.000Z"; const T2 = "2026-07-19T00:02:00.000Z"; +function writeRunRoot(target: string, runId: string, options: { linked: boolean }) { + const runRoot = path.join(target, ".ultrafuzz", "runs", runId); + fs.mkdirSync(runRoot, { recursive: true }); + fs.writeFileSync(path.join(runRoot, "state.json"), JSON.stringify({ run_id: runId, nodes: {} })); + if (options.linked) { + // The workflow link `resume` requires. Written right after the workflow compiles and before it is + // submitted, so its presence is what separates "failed to submit" from "died while compiling". + fs.writeFileSync(path.join(runRoot, "run-metadata.json"), JSON.stringify({ workflow: { run_id: "wf-1" } })); + } + return runRoot; +} + function fixture() { const workRoot = mkdtempSync(path.join(tmpdir(), "ultrafuzz-modal-resume-")); const target = path.join(workRoot, "target"); @@ -169,9 +181,7 @@ describe("Modal durable evaluation resume", () => { path.join(value.evalDir, "runs.jsonl"), `${JSON.stringify({ row_id: "row-one", status: "failed", final_status: "failed" })}\n` ); - const runRoot = path.join(value.target, ".ultrafuzz", "runs", "durable-run-one"); - fs.mkdirSync(runRoot, { recursive: true }); - fs.writeFileSync(path.join(runRoot, "state.json"), JSON.stringify({ run_id: "durable-run-one" })); + writeRunRoot(value.target, "durable-run-one", { linked: true }); await expect(findModalResumeWorkspace(value.workRoot)).resolves.toEqual({ kind: "resumable", workspace: { @@ -207,11 +217,7 @@ describe("Modal durable evaluation resume", () => { it("refuses to guess when several durable runs are on disk and none is linked", async () => { const value = fixture(); fs.writeFileSync(path.join(value.evalDir, "runs.jsonl"), `${JSON.stringify({ row_id: "row-one" })}\n`); - for (const runId of ["durable-run-one", "durable-run-two"]) { - const runRoot = path.join(value.target, ".ultrafuzz", "runs", runId); - fs.mkdirSync(runRoot, { recursive: true }); - fs.writeFileSync(path.join(runRoot, "state.json"), JSON.stringify({ run_id: runId })); - } + for (const runId of ["durable-run-one", "durable-run-two"]) writeRunRoot(value.target, runId, { linked: true }); await expect(findModalResumeWorkspace(value.workRoot)).rejects.toThrow("exactly one durable run on disk"); }); @@ -233,15 +239,32 @@ describe("Modal durable evaluation resume", () => { await expect(findModalResumeWorkspace(value.workRoot)).rejects.toThrow("persistent workspace is incomplete"); }); - it("surfaces a durable-volume read fault instead of reading it as a fresh run", async () => { + it("treats a non-directory eval runs path as not started, and clears nothing", async () => { const value = fixture(); - // ENOTDIR: the eval runs path exists but is a file. Swallowing every readdir error would report this - // as "never started" and invite a restart over whatever the volume actually holds. + // ENOTDIR is classified like absence, deliberately: there is no evaluation to resume. What matters is + // that it names nothing to delete, so the not-started path cannot remove anything on this route. const evalRoot = path.join(value.control, ".ultrafuzz", "evals", "runs"); fs.rmSync(evalRoot, { recursive: true }); fs.writeFileSync(evalRoot, "not a directory\n"); const found = await findModalResumeWorkspace(value.workRoot); expect(found.kind).toBe("not-started"); + expect(found.kind === "not-started" && found.staleEvalRunId).toBeUndefined(); + expect(found.kind === "not-started" && found.staleRunRootIds).toBeUndefined(); + }); + + it("refuses to call a run root resumable when no workflow was ever linked to it (#378)", async () => { + const value = fixture(); + fs.writeFileSync( + path.join(value.evalDir, "runs.jsonl"), + `${JSON.stringify({ row_id: "row-one", status: "failed", final_status: "failed" })}\n` + ); + // A run killed while compiling has state.json but no workflow link, and `resume` refuses it forever with + // WORKFLOW_RUN_ID_MISSING. Calling it resumable would relocate the wedge rather than remove it; it has + // to be named for clearing, or the restart trips RUN_ALREADY_EXISTS on its deterministic run id. + writeRunRoot(value.target, "durable-run-one", { linked: false }); + const found = await findModalResumeWorkspace(value.workRoot); + expect(found.kind).toBe("not-started"); + expect(found.kind === "not-started" && found.staleRunRootIds).toEqual(["durable-run-one"]); }); it("still resumes a linked durable run, and still rejects ambiguity, through the tolerant lookup", async () => { @@ -280,9 +303,15 @@ describe("Modal durable evaluation resume", () => { .split("\n") .filter((line) => line.trim() !== "") .map((line) => JSON.parse(line) as Record); + // The link alone is not enough. `scoreEvalRun` resolves the terminal report through + // `ultrafuzz_run_root`, and without it falls back to a path built from `row.run_id`, which is a + // different directory from the bounded run id on disk -- so the run would succeed and then fail + // scoring. `status` matters too: efficiency and status reporting read it, not `final_status`. expect(rows[rows.length - 1]).toMatchObject({ row_id: "row-one", ultrafuzz_run_id: "durable-run-one", + ultrafuzz_run_root: path.join(value.target, ".ultrafuzz", "runs", "durable-run-one"), + status: "launched", final_status: "succeeded" }); // And the repaired journal must now be resumable on its own, without consulting the disk again. From dfd78b382d6b2c548de03ffe35e080b96d854169 Mon Sep 17 00:00:00 2001 From: Antonio Viggiano <3029017+aviggiano@users.noreply.github.com> Date: Sat, 8 Aug 2026 00:52:04 +0000 Subject: [PATCH 4/6] fix(modal): derive the run metadata path, and only delete a provably unlinked run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 of review found the workflow-link predicate read `run-metadata.json`, a filename the runtime never writes — it writes `run.json` (`run-layout.ts:166`). Every real run root therefore classified as unlinked, which left #378 unfixed AND pointed the new deletion at exactly the roots it was added to protect. The test fixture invented the same filename, so all 19 tests passed while agreeing with the bug. The path is now derived from `layoutForRunRoot`, in both the source and the fixture, and pinned by a test. Two further wedges of the same class, both found by review: The zero-candidate branch named nothing to clear, but `runEvalSuite` refuses an id whose DIRECTORY exists and creates that directory before writing `eval.json`. A kill in that window wedged on EVAL_RUN_ALREADY_EXISTS exactly as before. Eval run directories are now tracked separately from candidates. `orphaned` was a disjunction, so a root carrying a workflow link but missing its state was named for deletion. Since the link is written before submission, that is a root a workflow may have run from. Orphaned is now a positive determination — metadata read successfully, no link — and linked-but-stateless is reported as damage. Read faults are no longer swallowed on the path that gates deletion: only ENOENT/ENOTDIR count as absence. Refs #378. --- packages/modal/src/resume.ts | 91 ++++++++++++++++++++++-------- packages/modal/src/worker.ts | 11 ++-- packages/modal/test/resume.test.ts | 66 +++++++++++++++++++--- 3 files changed, 132 insertions(+), 36 deletions(-) diff --git a/packages/modal/src/resume.ts b/packages/modal/src/resume.ts index d58cef5aa..02527687a 100644 --- a/packages/modal/src/resume.ts +++ b/packages/modal/src/resume.ts @@ -3,6 +3,8 @@ import { lstat, mkdir, open, readFile, readdir, rename, unlink } from "node:fs/p import type { FileHandle } from "node:fs/promises"; import path from "node:path"; +import { layoutForRunRoot } from "@ultrafuzz/artifacts"; + import { EVAL_WATCH_TIMEOUT_SECONDS } from "./defaults.js"; import type { TerminalDisposition } from "./terminal-disposition.js"; @@ -104,7 +106,7 @@ export type ModalResumeLookup = | { readonly kind: "not-started"; readonly reason: string; - readonly staleEvalRunId?: string; + readonly staleEvalRunIds?: readonly string[]; /** Run roots that exist but can never be resumed, so a restart must clear them (`RUN_ALREADY_EXISTS`). */ readonly staleRunRootIds?: readonly string[]; }; @@ -123,12 +125,13 @@ export type ModalResumeLookup = * The journal is therefore treated as a cache, not as the source of truth. When it holds no link, the * durable run directory is consulted directly: * - * - **a durable run exists** -> resumable. Its link is missing, not the run. `repairModalEvalRunRecord` - * writes the link back when the run finalizes. Restarting here would abandon completed work, and would - * fail anyway: row run ids are deterministic, so `planRun` would refuse with `RUN_ALREADY_EXISTS`. - * - **no durable run exists** -> genuinely not started. `staleEvalRunId` names the leftover eval run - * directory the caller must clear first, because eval run ids are deterministic too and `runEvalSuite` - * refuses to reuse one (`EVAL_RUN_ALREADY_EXISTS`). + * - **a run linked to a workflow exists** -> resumable. Its journal link is missing, not the run. + * `repairModalEvalRunRecord` writes that link back when the run finalizes. Restarting here would + * abandon completed work, and would fail anyway: row run ids are deterministic, so `planRun` would + * refuse with `RUN_ALREADY_EXISTS`. + * - **no linked run exists** -> genuinely not started. Both ids involved are deterministic, so the caller + * is told what to clear before restarting: `staleEvalRunIds` for eval run directories + * (`EVAL_RUN_ALREADY_EXISTS`) and `staleRunRootIds` for unlinked run roots (`RUN_ALREADY_EXISTS`). * * Counts of linked runs stay asymmetric: more than one durable run linked to a single evaluation is * corruption no correct producer can create, and silently picking a side would destroy a real run. @@ -141,12 +144,16 @@ export async function findModalResumeWorkspace(workRoot: string): Promise 0) { + // A linked run with no evaluation to attach it to is damage, not a fresh start. Restarting would + // abandon it and deleting it is exactly what this function exists to avoid, so refuse instead. + throw new Error(`workspace has ${durable.resumable.length} linked durable run(s) but no evaluation run`); + } + return { + kind: "not-started", + reason: "resume requires exactly one evaluation run, found 0", + ...(directories.length === 0 ? {} : { staleEvalRunIds: directories }), + ...(durable.orphaned.length === 0 ? {} : { staleRunRootIds: durable.orphaned }) + }; } if (candidates.length > 1) { throw new Error(`resume requires exactly one evaluation run, found ${candidates.length}`); @@ -188,7 +206,7 @@ export async function findModalResumeWorkspace(workRoot: string): Promise 0) { + throw new Error(`durable run ${damaged[0]!} is linked to a workflow but has no run state`); } return { resumable: resumable.sort(), orphaned: orphaned.sort() }; } -/** Whether `run-metadata.json` carries the workflow link `resume` requires. */ +/** + * Whether the run metadata carries the workflow link `resume` requires. + * + * The path comes from `layoutForRunRoot`, the same helper the runtime writes and reads it through, and is + * deliberately not restated here. An earlier revision named the file itself and named it wrongly, which + * classified every real run root as unresumable — and because the caller deletes unresumable roots, that + * turned this predicate into a destructive one while leaving #378 unfixed. + * + * Errors are NOT swallowed. Absence means the link was never written; EACCES or EIO on a durable volume + * mean the answer is unknown, and answering "unlinked" to an unknown is how a read fault becomes a + * deletion. Only ENOENT/ENOTDIR count as absence, matching `readdirIfMissing`. + */ async function hasLinkedWorkflow(runRoot: string): Promise { - const metadata = (await readFile(path.join(runRoot, "run-metadata.json"), "utf8").then( - (text) => JSON.parse(text) as unknown, - () => undefined - )) as - { workflow?: { run_id?: unknown; workflowRunId?: unknown }; smithers?: { workflowRunId?: unknown } } | undefined; - const runId = metadata?.workflow?.run_id ?? metadata?.workflow?.workflowRunId ?? metadata?.smithers?.workflowRunId; + const text = await readFile(layoutForRunRoot(runRoot).runMetadataPath, "utf8").catch( + (error: NodeJS.ErrnoException) => { + if (error.code === "ENOENT" || error.code === "ENOTDIR") return undefined; + throw error; + } + ); + if (text === undefined) return false; + const metadata = JSON.parse(text) as { + workflow?: { run_id?: unknown; workflowRunId?: unknown }; + smithers?: { workflowRunId?: unknown }; + }; + const runId = metadata.workflow?.run_id ?? metadata.workflow?.workflowRunId ?? metadata.smithers?.workflowRunId; return typeof runId === "string" && runId.length > 0; } diff --git a/packages/modal/src/worker.ts b/packages/modal/src/worker.ts index 96279117a..0e808cf1c 100644 --- a/packages/modal/src/worker.ts +++ b/packages/modal/src/worker.ts @@ -158,11 +158,12 @@ async function main(): Promise { // `planRun` refuses an existing run root (`RUN_ALREADY_EXISTS`). // // This is the only destructive step, so it is bounded by what the lookup proved: it names a run root - // here only when that root carries no workflow link, which means `resume` could never have used it - // and no workflow was ever submitted from it. A run that compiled — the R54 case — is linked, and so - // is resumed above and never reaches this branch. - if (found.staleEvalRunId !== undefined) { - await rm(path.join(WORK_ROOT, "control", ".ultrafuzz", "evals", "runs", found.staleEvalRunId), { + // here after successfully reading its metadata and finding no workflow link, which means no workflow + // was ever submitted from it. A root that IS linked but is missing its state is reported as damage + // rather than named here. A run that compiled — the R54 case — is linked, so it resumes above and + // never reaches this branch. + for (const staleEvalRunId of found.staleEvalRunIds ?? []) { + await rm(path.join(WORK_ROOT, "control", ".ultrafuzz", "evals", "runs", staleEvalRunId), { recursive: true, force: true }); diff --git a/packages/modal/test/resume.test.ts b/packages/modal/test/resume.test.ts index 8e428ca07..e2bde1bda 100644 --- a/packages/modal/test/resume.test.ts +++ b/packages/modal/test/resume.test.ts @@ -4,6 +4,7 @@ import path from "node:path"; import { describe, expect, it } from "vitest"; +import { layoutForRunRoot } from "@ultrafuzz/artifacts"; import { evalRunRoot } from "@ultrafuzz/evals"; import { @@ -26,9 +27,14 @@ function writeRunRoot(target: string, runId: string, options: { linked: boolean fs.mkdirSync(runRoot, { recursive: true }); fs.writeFileSync(path.join(runRoot, "state.json"), JSON.stringify({ run_id: runId, nodes: {} })); if (options.linked) { - // The workflow link `resume` requires. Written right after the workflow compiles and before it is - // submitted, so its presence is what separates "failed to submit" from "died while compiling". - fs.writeFileSync(path.join(runRoot, "run-metadata.json"), JSON.stringify({ workflow: { run_id: "wf-1" } })); + // The workflow link `resume` requires, at the path the RUNTIME writes it to. Derived from + // `layoutForRunRoot`, never spelled out: an earlier revision invented the filename here and in the + // source, so the suite agreed with the bug and all tests passed while every real run root was + // misclassified as unresumable -- and therefore deletable. + fs.writeFileSync( + layoutForRunRoot(runRoot).runMetadataPath, + JSON.stringify({ workflow: { run_id: "wf-1", path: "workflow.tsx" } }) + ); } return runRoot; } @@ -203,7 +209,7 @@ describe("Modal durable evaluation resume", () => { expect(found.kind).toBe("not-started"); // Naming it is the point: eval run ids are deterministic and `runEvalSuite` refuses to reuse one, so a // caller that restarts without clearing this hits EVAL_RUN_ALREADY_EXISTS forever. - expect(found.kind === "not-started" && found.staleEvalRunId).toBe(value.evalRunId); + expect(found.kind === "not-started" && found.staleEvalRunIds).toEqual([value.evalRunId]); }); it("classifies a journal that was never written, rather than crashing on it (#378)", async () => { @@ -226,7 +232,7 @@ describe("Modal durable evaluation resume", () => { // A fresh sandbox must not be mistaken for a corrupt one. const found = await findModalResumeWorkspace(workRoot); expect(found.kind).toBe("not-started"); - expect(found.kind === "not-started" && found.staleEvalRunId).toBeUndefined(); + expect(found.kind === "not-started" && found.staleEvalRunIds).toBeUndefined(); }); it("still refuses a symlinked control even when it holds no evaluation run", async () => { @@ -248,7 +254,7 @@ describe("Modal durable evaluation resume", () => { fs.writeFileSync(evalRoot, "not a directory\n"); const found = await findModalResumeWorkspace(value.workRoot); expect(found.kind).toBe("not-started"); - expect(found.kind === "not-started" && found.staleEvalRunId).toBeUndefined(); + expect(found.kind === "not-started" && found.staleEvalRunIds).toBeUndefined(); expect(found.kind === "not-started" && found.staleRunRootIds).toBeUndefined(); }); @@ -342,7 +348,7 @@ describe("Modal durable evaluation resume", () => { ); const found = await findModalResumeWorkspace(value.workRoot); expect(found.kind).toBe("not-started"); - const stale = found.kind === "not-started" ? found.staleEvalRunId : undefined; + const stale = found.kind === "not-started" ? found.staleEvalRunIds?.[0] : undefined; expect(stale).toBeDefined(); // This is the coupling that made the first attempt at this fix a no-op. `runEvalSuite` throws // EVAL_RUN_ALREADY_EXISTS when `evalRunRoot(control, evalRunId)` exists, and eval run ids are @@ -355,6 +361,52 @@ describe("Modal durable evaluation resume", () => { expect(fs.existsSync(refusedByRunner)).toBe(false); }); + it("reads the workflow link from the path the runtime actually writes it to (#378)", async () => { + const value = fixture(); + fs.writeFileSync( + path.join(value.evalDir, "runs.jsonl"), + `${JSON.stringify({ row_id: "row-one", status: "failed", final_status: "failed" })}\n` + ); + const runRoot = writeRunRoot(value.target, "durable-run-one", { linked: true }); + // Pin the coupling itself, not just the behaviour. The metadata file is `run.json`, and the only reason + // this predicate is trustworthy is that it derives the path instead of naming it -- when it named it, + // it named it wrongly and the caller deleted every real run root. + expect(path.basename(layoutForRunRoot(runRoot).runMetadataPath)).toBe("run.json"); + expect(fs.existsSync(path.join(runRoot, "run.json"))).toBe(true); + await expect(findModalResumeWorkspace(value.workRoot)).resolves.toMatchObject({ kind: "resumable" }); + }); + + it("names an eval run directory with no eval.json for clearing (#378)", async () => { + const value = fixture(); + // `runEvalSuite` creates the directory and only then writes eval.json, and it refuses to reuse an id + // whose DIRECTORY exists. A kill in that window leaves a directory that is not a candidate but still + // blocks reuse, so failing to name it wedges on EVAL_RUN_ALREADY_EXISTS just as surely. + fs.rmSync(path.join(value.evalDir, "eval.json")); + const found = await findModalResumeWorkspace(value.workRoot); + expect(found.kind).toBe("not-started"); + expect(found.kind === "not-started" && found.staleEvalRunIds).toEqual([value.evalRunId]); + }); + + it("reports a linked run with no state as damage rather than deleting it (#378)", async () => { + const value = fixture(); + fs.writeFileSync( + path.join(value.evalDir, "runs.jsonl"), + `${JSON.stringify({ row_id: "row-one", status: "failed", final_status: "failed" })}\n` + ); + const runRoot = writeRunRoot(value.target, "durable-run-one", { linked: true }); + fs.rmSync(path.join(runRoot, "state.json")); + // The link is written before submission, so a linked root is one a workflow may have run from. It must + // never be named for deletion merely because it is not resumable. + await expect(findModalResumeWorkspace(value.workRoot)).rejects.toThrow("has no run state"); + }); + + it("refuses a linked durable run that has no evaluation run to attach it to", async () => { + const value = fixture(); + fs.rmSync(value.evalDir, { recursive: true }); + writeRunRoot(value.target, "durable-run-one", { linked: true }); + await expect(findModalResumeWorkspace(value.workRoot)).rejects.toThrow("no evaluation run"); + }); + it("finalizes succeeded and genuine task outcomes without resetting completed nodes", async () => { const value = fixture(); const workspace = await locateModalResumeWorkspace(value.workRoot); From 9279b29751d74af9837898ada4ada3098d89cda3 Mon Sep 17 00:00:00 2001 From: Antonio Viggiano <3029017+aviggiano@users.noreply.github.com> Date: Sat, 8 Aug 2026 00:59:52 +0000 Subject: [PATCH 5/6] fix(modal): classify every run root, and stop restating the metadata filename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 came back with no blocking findings. These are its non-blocking ones, all of the same class the earlier rounds were. The docstring that motivates the whole workflow-link predicate still named `run-metadata.json` — the exact wrong filename that caused round 3's defect, sitting thirty-nine lines above the comment explaining why the path is deliberately never restated. It now points at `layoutForRunRoot`. Run roots that could not be classified were silently skipped, but `planRun` refuses on `existsSync`, which follows symlinks and does not care whether a name is layout-addressable. Anything dropped there would still block a restart, invisibly. They are now reported. A stray dot-prefixed directory would also have thrown out of `layoutForRunRoot` on every generation with nothing able to clear it; addressability is now asked of the helper rather than restated as a regex. Damage no longer preempts a good resume: it is only refused on the branch that would restart, since that is the only branch it blocks. The one-candidate branch named only the candidate for clearing while the zero-candidate branch named every directory. `runEvalSuite` refuses any id whose directory exists, so both branches now name all of them. Malformed run metadata throws with the file path instead of a bare SyntaxError. Refs #378. --- packages/modal/src/resume.ts | 70 +++++++++++++++++++++++++----- packages/modal/src/worker.ts | 4 +- packages/modal/test/resume.test.ts | 46 +++++++++++++++++++- 3 files changed, 106 insertions(+), 14 deletions(-) diff --git a/packages/modal/src/resume.ts b/packages/modal/src/resume.ts index 02527687a..6bd548867 100644 --- a/packages/modal/src/resume.ts +++ b/packages/modal/src/resume.ts @@ -167,6 +167,7 @@ export async function findModalResumeWorkspace(workRoot: string): Promise { +async function durableRunsOnDisk( + target: string +): Promise<{ resumable: string[]; orphaned: string[]; damaged: string[] }> { const runsRoot = path.join(target, ".ultrafuzz", "runs"); const resumable = []; const orphaned = []; const damaged = []; for (const name of await readdirIfMissing(runsRoot)) { const root = path.join(runsRoot, name); - if (!(await isDirectoryNotSymlink(root))) continue; + // An entry this function cannot classify is reported, never skipped. `planRun` refuses on + // `existsSync`, which follows symlinks and does not care whether the name is layout-addressable, so + // anything dropped here silently would still block a restart — invisibly, which is the whole subject + // of #378. + if (!(await isDirectoryNotSymlink(root)) || !isLayoutAddressable(root)) { + damaged.push(name); + continue; + } const linked = await hasLinkedWorkflow(root); const hasState = await isRegularFileNotSymlink(path.join(root, "state.json")); // `orphaned` is a POSITIVE determination — the metadata was read and carries no workflow link — and not @@ -244,10 +260,23 @@ async function durableRunsOnDisk(target: string): Promise<{ resumable: string[]; else if (hasState) resumable.push(name); else damaged.push(name); } - if (damaged.length > 0) { - throw new Error(`durable run ${damaged[0]!} is linked to a workflow but has no run state`); + return { resumable: resumable.sort(), orphaned: orphaned.sort(), damaged: damaged.sort() }; +} + +/** + * Whether the runtime's own layout helper can address this run root. + * + * Derived by asking the helper rather than restating its id rule. A name it rejects would otherwise throw + * out of the lookup on every generation, with nothing able to clear it — one stray directory would strand + * the run permanently. + */ +function isLayoutAddressable(runRoot: string): boolean { + try { + layoutForRunRoot(runRoot); + return true; + } catch { + return false; } - return { resumable: resumable.sort(), orphaned: orphaned.sort() }; } /** @@ -270,14 +299,33 @@ async function hasLinkedWorkflow(runRoot: string): Promise { } ); if (text === undefined) return false; - const metadata = JSON.parse(text) as { - workflow?: { run_id?: unknown; workflowRunId?: unknown }; - smithers?: { workflowRunId?: unknown }; - }; + let metadata: { workflow?: { run_id?: unknown; workflowRunId?: unknown }; smithers?: { workflowRunId?: unknown } }; + try { + metadata = JSON.parse(text) as typeof metadata; + } catch (error) { + // Failing closed is the decision here, not an oversight: reading unparseable metadata as "unlinked" + // would route the root to deletion. Name the file so the refusal is diagnosable from the volume. + throw new Error(`run metadata at ${layoutForRunRoot(runRoot).runMetadataPath} is not valid JSON`, { + cause: error + }); + } const runId = metadata.workflow?.run_id ?? metadata.workflow?.workflowRunId ?? metadata.smithers?.workflowRunId; return typeof runId === "string" && runId.length > 0; } +/** + * Refuse to restart over a run root that cannot be classified, rather than deleting it or ignoring it. + * + * Sorted so the message is deterministic when there is more than one. + */ +function assertNoDamagedRunRoots(damaged: readonly string[]): void { + if (damaged.length === 0) return; + throw new Error( + `durable run root(s) ${damaged.join(", ")} cannot be classified as resumable or unstarted; ` + + `resolve them on the volume before this run can restart` + ); +} + /** Refuse a path that exists but is not a real directory. An absent path is the ordinary fresh case. */ async function assertWorkspaceShape(directoryPath: string): Promise { const stat = await lstat(directoryPath).catch((error: NodeJS.ErrnoException) => { diff --git a/packages/modal/src/worker.ts b/packages/modal/src/worker.ts index 0e808cf1c..224ee526e 100644 --- a/packages/modal/src/worker.ts +++ b/packages/modal/src/worker.ts @@ -158,8 +158,8 @@ async function main(): Promise { // `planRun` refuses an existing run root (`RUN_ALREADY_EXISTS`). // // This is the only destructive step, so it is bounded by what the lookup proved: it names a run root - // here after successfully reading its metadata and finding no workflow link, which means no workflow - // was ever submitted from it. A root that IS linked but is missing its state is reported as damage + // here only when its metadata is absent or carries no workflow link — the link is written before + // submission, so either way no workflow was ever submitted from it. A root that IS linked but is missing its state is reported as damage // rather than named here. A run that compiled — the R54 case — is linked, so it resumes above and // never reaches this branch. for (const staleEvalRunId of found.staleEvalRunIds ?? []) { diff --git a/packages/modal/test/resume.test.ts b/packages/modal/test/resume.test.ts index e2bde1bda..0eaf73917 100644 --- a/packages/modal/test/resume.test.ts +++ b/packages/modal/test/resume.test.ts @@ -397,7 +397,7 @@ describe("Modal durable evaluation resume", () => { fs.rmSync(path.join(runRoot, "state.json")); // The link is written before submission, so a linked root is one a workflow may have run from. It must // never be named for deletion merely because it is not resumable. - await expect(findModalResumeWorkspace(value.workRoot)).rejects.toThrow("has no run state"); + await expect(findModalResumeWorkspace(value.workRoot)).rejects.toThrow("cannot be classified"); }); it("refuses a linked durable run that has no evaluation run to attach it to", async () => { @@ -407,6 +407,50 @@ describe("Modal durable evaluation resume", () => { await expect(findModalResumeWorkspace(value.workRoot)).rejects.toThrow("no evaluation run"); }); + it("does not let a damaged run root preempt resuming a perfectly good one (#378)", async () => { + const value = fixture(); + fs.writeFileSync( + path.join(value.evalDir, "runs.jsonl"), + `${JSON.stringify({ row_id: "row-one", status: "failed", final_status: "failed" })}\n` + ); + writeRunRoot(value.target, "durable-run-one", { linked: true }); + const broken = writeRunRoot(value.target, "durable-run-two", { linked: true }); + fs.rmSync(path.join(broken, "state.json")); + // Damage blocks a RESTART, because `planRun` would trip over it. It must not block a resume: refusing + // here would strand a run that is ready to continue. + await expect(findModalResumeWorkspace(value.workRoot)).resolves.toMatchObject({ + kind: "resumable", + workspace: { productRunId: "durable-run-one" } + }); + }); + + it("refuses a run root whose name the runtime layout cannot address, instead of throwing forever", async () => { + const value = fixture(); + fs.writeFileSync( + path.join(value.evalDir, "runs.jsonl"), + `${JSON.stringify({ row_id: "row-one", status: "failed", final_status: "failed" })}\n` + ); + // `layoutForRunRoot` rejects ids outside its safe-id rule. Letting that throw escape would strand the + // run permanently, since the lookup would fail before naming anything a restart could clear. + fs.mkdirSync(path.join(value.target, ".ultrafuzz", "runs", ".tmp-junk"), { recursive: true }); + await expect(findModalResumeWorkspace(value.workRoot)).rejects.toThrow("cannot be classified"); + }); + + it("names every eval run directory that would block a restart, not just the candidate", async () => { + const value = fixture(); + fs.writeFileSync( + path.join(value.evalDir, "runs.jsonl"), + `${JSON.stringify({ row_id: "row-one", status: "failed", final_status: "failed" })}\n` + ); + // A directory with no eval.json is not a candidate, but `runEvalSuite` still refuses to reuse its id. + fs.mkdirSync(path.join(value.control, ".ultrafuzz", "evals", "runs", "leftover-run"), { recursive: true }); + const found = await findModalResumeWorkspace(value.workRoot); + expect(found.kind).toBe("not-started"); + expect(found.kind === "not-started" && [...(found.staleEvalRunIds ?? [])].sort()).toEqual( + [value.evalRunId, "leftover-run"].sort() + ); + }); + it("finalizes succeeded and genuine task outcomes without resetting completed nodes", async () => { const value = fixture(); const workspace = await locateModalResumeWorkspace(value.workRoot); From e734625d4f4e9d80145e761367a0ca0a0249eab9 Mon Sep 17 00:00:00 2001 From: Antonio Viggiano <3029017+aviggiano@users.noreply.github.com> Date: Sat, 8 Aug 2026 01:04:44 +0000 Subject: [PATCH 6/6] fix(modal): classify eval run entries the same way as run roots Round 5 approved with two comment-level notes and one symmetry gap; this closes all three. The eval-run loop still had the bare `continue` the run-root loop had just replaced. `runEvalSuite` refuses on `existsSync`, which follows symlinks and ignores entry type, so an entry skipped there would block the restart with nothing able to clear it. Non-directory entries are now refused rather than dropped -- the conservative half of the same rule: never delete what cannot be classified. The deferral comment claimed damage cannot preempt a good resume. That is true of the `damaged` list but not of a read fault raised while classifying another root, which still propagates. Fail-closed and correct, but the comment was promising more than it delivered. Refs #378. --- packages/modal/src/resume.ts | 28 ++++++++++++++++++++++++++-- packages/modal/src/worker.ts | 10 +++++----- packages/modal/test/resume.test.ts | 13 +++++++++++++ 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/packages/modal/src/resume.ts b/packages/modal/src/resume.ts index 6bd548867..65d21d979 100644 --- a/packages/modal/src/resume.ts +++ b/packages/modal/src/resume.ts @@ -146,9 +146,16 @@ export async function findModalResumeWorkspace(workRoot: string): Promise { return typeof runId === "string" && runId.length > 0; } +/** + * Refuse to restart over an eval run entry that is not a directory. Deleting something we cannot classify + * would be worse, and ignoring it hands the restart an `EVAL_RUN_ALREADY_EXISTS` it cannot clear. + */ +function assertNoUnclassifiableEvalEntries(entries: readonly string[]): void { + if (entries.length === 0) return; + throw new Error( + `evaluation run entr(ies) ${[...entries].sort().join(", ")} are not directories and would block a restart; ` + + `resolve them on the volume` + ); +} + /** * Refuse to restart over a run root that cannot be classified, rather than deleting it or ignoring it. * diff --git a/packages/modal/src/worker.ts b/packages/modal/src/worker.ts index 224ee526e..b87ed5cbf 100644 --- a/packages/modal/src/worker.ts +++ b/packages/modal/src/worker.ts @@ -157,11 +157,11 @@ async function main(): Promise { // failure: `runEvalSuite` refuses an existing eval run directory (`EVAL_RUN_ALREADY_EXISTS`), and // `planRun` refuses an existing run root (`RUN_ALREADY_EXISTS`). // - // This is the only destructive step, so it is bounded by what the lookup proved: it names a run root - // here only when its metadata is absent or carries no workflow link — the link is written before - // submission, so either way no workflow was ever submitted from it. A root that IS linked but is missing its state is reported as damage - // rather than named here. A run that compiled — the R54 case — is linked, so it resumes above and - // never reaches this branch. + // This is the only destructive step, and it is bounded by what the lookup proved: a run root is + // named here only when its metadata is absent or carries no workflow link. That link is written + // before submission, so either way no workflow was ever submitted from that root. A root that IS + // linked but is missing its state is reported as damage rather than named here. A run that + // compiled — the R54 case — is linked, so it resumes above and never reaches this branch. for (const staleEvalRunId of found.staleEvalRunIds ?? []) { await rm(path.join(WORK_ROOT, "control", ".ultrafuzz", "evals", "runs", staleEvalRunId), { recursive: true, diff --git a/packages/modal/test/resume.test.ts b/packages/modal/test/resume.test.ts index 0eaf73917..55dce0c7f 100644 --- a/packages/modal/test/resume.test.ts +++ b/packages/modal/test/resume.test.ts @@ -451,6 +451,19 @@ describe("Modal durable evaluation resume", () => { ); }); + it("refuses an eval run entry that is not a directory, instead of silently skipping it", async () => { + const value = fixture(); + fs.writeFileSync( + path.join(value.evalDir, "runs.jsonl"), + `${JSON.stringify({ row_id: "row-one", status: "failed", final_status: "failed" })}\n` + ); + // `runEvalSuite` refuses on `existsSync`, which follows symlinks and ignores entry type, so an entry + // dropped here would block the restart with nothing able to clear it -- the same asymmetry that the + // run-root loop closes. Refusing is the conservative half: it never deletes what it cannot classify. + fs.writeFileSync(path.join(value.control, ".ultrafuzz", "evals", "runs", "not-a-directory"), "x\n"); + await expect(findModalResumeWorkspace(value.workRoot)).rejects.toThrow("are not directories"); + }); + it("finalizes succeeded and genuine task outcomes without resetting completed nodes", async () => { const value = fixture(); const workspace = await locateModalResumeWorkspace(value.workRoot);