diff --git a/evals/msbench/README.md b/evals/msbench/README.md index 4adf31ad..4014184b 100644 --- a/evals/msbench/README.md +++ b/evals/msbench/README.md @@ -2657,6 +2657,28 @@ with a clear message: because it explained anything: the artifact scare that prompted the enumeration turned out to be a read that predated the run's completion by about five minutes, and member order was not the cause. Noted here so nobody re-derives it as one. +- **`msbench-cli extract` reports refusal on stderr and still exits 0.** Pointed at a + destination that already exists and is nonempty it prints + `ERROR exists and is nonempty. Quitting to avoid overwriting.`, writes nothing, + and **exits 0** — the same status as a successful extraction. Measured on the same run + id, same CLI, only the destination differing: + + | destination | exit code | instance output written | + | --- | --- | --- | + | fresh directory | 0 | yes | + | existing nonempty directory | 0 | **no** | + + So `result.status !== 0` cannot tell the two apart, and any consumer relying on it + returns a stale cache as though it were a fresh download. This is not hypothetical: it + made `gate-health` report a permanent `READER FAULT` on run `2026090413313337` after a + single audit taken while that run was still in flight cached a partial extraction, and + it made `regrade`'s `--refresh` a silent no-op. `--refresh` could not clear either, + because it re-extracts into the same nonempty directory and hits the identical refusal. + Both now clear their own cache before extracting and verify that something was actually + written; a caller-supplied `--extract-dir` is reported rather than deleted. + + The general rule this is an instance of: **an exit code is a claim, not a + verification.** Where a tool's output is the thing you need, check for the output. - **Name the field that answers your question before you read one.** Every status surface here has a neighbouring field that looks like the answer and isn't, and reading the neighbour has now cost four separate investigations in two days: diff --git a/evals/msbench/extraction.ts b/evals/msbench/extraction.ts index e09c9df2..5f12ca73 100644 --- a/evals/msbench/extraction.ts +++ b/evals/msbench/extraction.ts @@ -25,7 +25,7 @@ */ import { spawnSync } from 'node:child_process'; -import { existsSync, mkdirSync, readdirSync } from 'node:fs'; +import { existsSync, mkdirSync, readdirSync, rmSync } from 'node:fs'; import { dirname, join, resolve } from 'node:path'; /** @@ -115,6 +115,16 @@ export function resolveExtraction(request: ExtractRequest, log: Logger = console args.push('--instance', wanted); } + // `msbench-cli extract` refuses to write into a destination that already exists and is + // nonempty. When the destination is this module's own cache it is derived data, safe to + // discard, and clearing it is what makes `--refresh` mean what it says. A caller-supplied + // `--extract-dir` gets no such treatment — it may point somewhere that must not be deleted, + // so that case is detected and reported after the call instead. + if (!request.extractDir) { + rmSync(dir, { recursive: true, force: true }); + } + const destinationWasNonEmpty = existsSync(dir) && readdirSync(dir).length > 0; + log(`$ msbench-cli ${args.join(' ')}`); const result = spawnSync('msbench-cli', args, { stdio: 'inherit' }); if (result.error && (result.error as NodeJS.ErrnoException).code === 'ENOENT') { @@ -130,6 +140,37 @@ export function resolveExtraction(request: ExtractRequest, log: Logger = console 'extraction reads a stored blob and needs an Azure identity.' ); } + + // Exit 0 does not mean anything was written. `msbench-cli extract` reports + // "exists and is nonempty. Quitting to avoid overwriting." on stderr and **still exits 0**, + // so the status check above cannot tell a real extraction from a refused one. + // + // Left undetected that makes `--refresh` a silent no-op — the caller asks to re-download, + // gets the stale cache back, and is told nothing — and it lets `--instance B` be answered + // by a cache built from `--instance A`. Both are wrong answers rather than failures, which + // is worse than either an error or a slow re-download. + // + // The test is deliberately "did extraction write an instance tree at all", counting + // `incomplete` alongside `instances`. A tree that arrived without its `session.sqlite` is a + // *different* failure with its own reporting downstream — notably the Windows path-length + // limit that truncates extractions under a repo-nested cache — and swallowing that into this + // message would trade one misdiagnosis for another. + const found = findInstances(dir); + const wroteNothing = found.instances.length === 0 && found.incomplete.length === 0; + const missingWanted = wanted !== undefined && + !found.instances.some(candidate => matchesInstance(candidate.name, wanted)) && + !found.incomplete.some(name => matchesInstance(name, wanted)); + if (wroteNothing || missingWanted) { + throw new MsBenchToolError( + `msbench-cli extract exited 0 but wrote no extraction to ${dir}` + + `${missingWanted ? ` matching --instance ${wanted}` : ''}.\n` + + (destinationWasNonEmpty + ? 'The destination already existed and was nonempty, so extraction was almost ' + + 'certainly refused ("Quitting to avoid overwriting") — which still exits 0.\n' + + `Remove ${dir} and retry, or pass --extract-dir .` + : `Run \`msbench-cli extract --run_id ${runId} --output \` directly to see why.`) + ); + } return dir; } diff --git a/evals/msbench/gate-health.ts b/evals/msbench/gate-health.ts index 560cb0ec..5c197b5c 100644 --- a/evals/msbench/gate-health.ts +++ b/evals/msbench/gate-health.ts @@ -61,7 +61,7 @@ */ import { spawnSync } from 'node:child_process'; -import { existsSync, readFileSync, readdirSync } from 'node:fs'; +import { existsSync, readFileSync, readdirSync, rmSync } from 'node:fs'; import { homedir } from 'node:os'; import { dirname, join, resolve } from 'node:path'; import { DatabaseSync } from 'node:sqlite'; @@ -366,6 +366,23 @@ function extractRun(runId: string, refresh: boolean): string | undefined { return dir; } + // `msbench-cli extract` refuses to write into a directory that already exists and is nonempty + // ("Quitting to avoid overwriting") — and it reports that refusal on stderr while still exiting + // **0**. The status check below therefore cannot see it, and the refusal is indistinguishable + // from a successful extraction. + // + // That turned a transient condition into a permanent one. Auditing a run while it was still in + // flight cached a partial extraction — `run_metadata.json` and no instance output, because none + // existed yet. Every later audit then found no instances, re-invoked extract, got the silent + // refusal plus exit 0, and reported the run as a READER FAULT forever. `--refresh` could not + // clear it either: it skips the early return above but extracts into the same nonempty + // directory, so it hit the identical refusal. + // + // Clearing the destination first makes the extraction authoritative rather than advisory. It is + // safe because the cache is derived data, reconstructible from `results.zip` on demand, and it + // is reached only when the cache holds no instances — the case that needs re-extraction anyway. + rmSync(dir, { recursive: true, force: true }); + const result = spawnSync('msbench-cli', ['extract', '--run_id', runId, '--output', dir], { encoding: 'utf8', });