From e70bb99acf14e357c54307ea355e24fd5a100b7f Mon Sep 17 00:00:00 2001 From: Nathan Turinski Date: Tue, 8 Sep 2026 08:36:13 -0700 Subject: [PATCH] =?UTF-8?q?=EF=BB=BFStop=20treating=20msbench-cli=20extrac?= =?UTF-8?q?t's=20exit=20code=20as=20proof=20it=20extracted?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `msbench-cli extract` refuses to write into a destination that already exists and is nonempty -- it prints "Quitting to avoid overwriting" on stderr and then exits 0, the same status as a successful extraction. Measured on one run id with only the destination varying: fresh dir -> exit 0, output written; existing nonempty dir -> exit 0, nothing written. Both consumers checked only `result.status !== 0`, so both read a refusal as a success and returned a stale cache as though it were a fresh download. gate-health: a single audit taken while run 2026090413313337 was still in flight cached a partial extraction (run_metadata.json, no instance output, because none existed yet). Every later audit then found no instances, re-invoked extract, got the silent refusal, and reported the run as a permanent READER FAULT -- dropping a real run from the corpus in the direction of "never executed", the loudest verdict the tool has. --refresh could not clear it either, because it extracts into the same nonempty directory and hits the identical refusal. extraction.ts: the same refusal made regrade's --refresh a silent no-op. Verified by pointing resolveExtraction at a nonempty directory with refresh: true -- it returned the directory, the stale marker file survived, and no instance output appeared, with nothing reported. Both now clear their own cache before extracting, which is what makes --refresh mean what it says. A caller-supplied --extract-dir is deliberately not deleted; that case is detected after the call and reported with recovery instructions, since it may point somewhere that must not be removed. The post-extraction check counts `incomplete` alongside `instances` so it fires only when extraction wrote nothing at all. An instance tree that arrives without its session.sqlite is a different failure with its own reporting -- on Windows the 260-character path limit truncates extractions under the repo-nested cache, observed here at 254 characters and 35 files -- and folding that into this message would trade one misdiagnosis for another. Falsification: with the same poisoned cache and the same run id, reverting only the gate-health change reproduces "READER FAULT ... yielded no instances" and loads 0 instances; restoring it loads 1. Across the full corpus the audit goes from 67 runs/67 instances with one dropped to 68/68, while the three runs that genuinely have no instance output are still reported as corpus facts rather than reader faults. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- evals/msbench/README.md | 22 ++++++++++++++++++ evals/msbench/extraction.ts | 43 +++++++++++++++++++++++++++++++++++- evals/msbench/gate-health.ts | 19 +++++++++++++++- 3 files changed, 82 insertions(+), 2 deletions(-) diff --git a/evals/msbench/README.md b/evals/msbench/README.md index 4adf31add..4014184bb 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 e09c9df2d..5f12ca732 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 560cb0ec3..5c197b5c4 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', });