From 5390a0b27b7519eba2f0eade48bae5dea8ffa7c7 Mon Sep 17 00:00:00 2001 From: Bruno Borges Date: Wed, 5 Aug 2026 03:12:05 -0400 Subject: [PATCH 1/2] Stop a heredoc syntax error from costing a whole run to find The cache save helper declared `manifest` twice in one embedded script, so every slot died on a SyntaxError before it saved anything. Nothing local could have caught it: the code inside `<<'NODE'` is a string to prettier, `node --test` never imports it, and shellcheck sees an opaque heredoc. Rename the colliding binding, and add a check that extracts every embedded block and runs `node --check` over it, wired into `npm test` so the next one fails in a second rather than after a full benchmark run. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1 --- README.md | 4 +- package.json | 2 +- scripts/cache-save.sh | 16 +++---- scripts/check-embedded-node.mjs | 77 +++++++++++++++++++++++++++++++++ 4 files changed, 89 insertions(+), 10 deletions(-) create mode 100644 scripts/check-embedded-node.mjs diff --git a/README.md b/README.md index 5c5b83d..1d85cfb 100644 --- a/README.md +++ b/README.md @@ -169,7 +169,9 @@ Network throughput, hosted-runner image changes, upstream artifact availability ## Local checks ```bash -npm test +npm test # unit tests, plus a syntax check of the JS embedded in heredocs bash -n scripts/*.sh shellcheck scripts/*.sh ``` + +`npm test` includes `scripts/check-embedded-node.mjs` because several helpers run node inline through a heredoc, and that code is a string to everything else here: prettier does not format it, `node --test` never imports it, and shellcheck sees an opaque block. A syntax error in one of them otherwise stays invisible until a runner reaches it, which costs a whole benchmark run to find out. diff --git a/package.json b/package.json index 1690756..eed6a96 100644 --- a/package.json +++ b/package.json @@ -3,6 +3,6 @@ "private": true, "type": "module", "scripts": { - "test": "node --test" + "test": "node --test scripts/*.test.mjs && node scripts/check-embedded-node.mjs" } } diff --git a/scripts/cache-save.sh b/scripts/cache-save.sh index 5277a7a..5e055db 100755 --- a/scripts/cache-save.sh +++ b/scripts/cache-save.sh @@ -219,7 +219,7 @@ import { createRequire } from "node:module"; // by package name fails outright. Resolving its manifest and requiring the file // its `main` points at goes around the map, and keeps working whichever version // a given setup-java ref happens to pin. -function loadCacheClient(require, manifest) { +function loadCacheClient(require, callerManifest) { const { readFileSync } = require("node:fs"); const { dirname, join } = require("node:path"); let manifestPath; @@ -229,15 +229,15 @@ function loadCacheClient(require, manifest) { // Some versions do not expose "./package.json" through the map either, in // which case the install layout is the only thing left to go on. manifestPath = join( - dirname(manifest), + dirname(callerManifest), "node_modules", "@actions", "cache", "package.json", ); } - const manifest = JSON.parse(readFileSync(manifestPath, "utf8")); - return require(join(dirname(manifestPath), manifest.main ?? "lib/cache.js")); + const packageJson = JSON.parse(readFileSync(manifestPath, "utf8")); + return require(join(dirname(manifestPath), packageJson.main ?? "lib/cache.js")); } const [manifest] = process.argv.slice(2); @@ -265,7 +265,7 @@ import { pathToFileURL } from "node:url"; // by package name fails outright. Resolving its manifest and requiring the file // its `main` points at goes around the map, and keeps working whichever version // a given setup-java ref happens to pin. -function loadCacheClient(require, manifest) { +function loadCacheClient(require, callerManifest) { const { readFileSync } = require("node:fs"); const { dirname, join } = require("node:path"); let manifestPath; @@ -275,15 +275,15 @@ function loadCacheClient(require, manifest) { // Some versions do not expose "./package.json" through the map either, in // which case the install layout is the only thing left to go on. manifestPath = join( - dirname(manifest), + dirname(callerManifest), "node_modules", "@actions", "cache", "package.json", ); } - const manifest = JSON.parse(readFileSync(manifestPath, "utf8")); - return require(join(dirname(manifestPath), manifest.main ?? "lib/cache.js")); + const packageJson = JSON.parse(readFileSync(manifestPath, "utf8")); + return require(join(dirname(manifestPath), packageJson.main ?? "lib/cache.js")); } const [manifest, fixtureDir, key, resultsFile, sample, arm, slot] = diff --git a/scripts/check-embedded-node.mjs b/scripts/check-embedded-node.mjs new file mode 100644 index 0000000..0762238 --- /dev/null +++ b/scripts/check-embedded-node.mjs @@ -0,0 +1,77 @@ +// Syntax-check the JavaScript embedded in shell heredocs. +// +// Several benchmark helpers run node inline, via `node --input-type=module - +// <<'NODE'`. That code is a string as far as every other check here is +// concerned: prettier does not format it, `node --test` never imports it, and +// shellcheck sees an opaque heredoc. A syntax error in one of those blocks is +// therefore invisible until a runner reaches it, which costs a whole benchmark +// run to discover — that is exactly how `Identifier 'manifest' has already been +// declared` reached a live job. + +import { readFile, writeFile, unlink } from "node:fs/promises"; +import { execFile } from "node:child_process"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { promisify } from "node:util"; +import { glob } from "node:fs/promises"; +import { pathToFileURL } from "node:url"; + +const run = promisify(execFile); +const BLOCK = /<<'NODE'\n([\s\S]*?)\nNODE\n/g; + +export function extractBlocks(source) { + return [...source.matchAll(BLOCK)].map((match, index) => ({ + index, + code: match[1], + // Line of the heredoc opener, so a failure points somewhere useful. + line: source.slice(0, match.index).split("\n").length, + })); +} + +export async function checkBlock(block, file) { + const scratch = join( + tmpdir(), + `embedded-${process.pid}-${file.replace(/\W/g, "_")}-${block.index}.mjs`, + ); + await writeFile(scratch, block.code); + try { + await run("node", ["--check", scratch]); + return null; + } catch (error) { + return `${file}:${block.line}: ${String(error.stderr).trim().split("\n").pop()}`; + } finally { + await unlink(scratch).catch(() => {}); + } +} + +export async function main() { + const files = []; + for await (const file of glob("scripts/*.sh")) files.push(file); + files.sort(); + + const failures = []; + let checked = 0; + for (const file of files) { + for (const block of extractBlocks(await readFile(file, "utf8"))) { + checked += 1; + const failure = await checkBlock(block, file); + if (failure) failures.push(failure); + } + } + + if (failures.length > 0) { + throw new Error( + `Embedded node scripts failed to parse:\n${failures.map((failure) => ` ${failure}`).join("\n")}`, + ); + } + console.log( + `${checked} embedded node scripts parse in ${files.length} files`, + ); +} + +if ( + process.argv[1] && + import.meta.url === pathToFileURL(process.argv[1]).href +) { + await main(); +} From 8c27cbd8144a0c01862dd37bb0a32f36c192215d Mon Sep 17 00:00:00 2001 From: Bruno Borges Date: Wed, 5 Aug 2026 03:15:15 -0400 Subject: [PATCH 2/2] Say when a run could not have found anything MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first live run measured caching as 4.5x faster — 22.4s on Spring PetClinic — and reported it as inconclusive. The permutation test is a sign flip over n paired runners, so it has 2^n assignments and a smallest attainable p-value of 2^-n. One of six runners stalled and was discarded, leaving four, whose floor is 0.0625. No effect of any size could have cleared 0.05 in that run. Reporting that as inconclusive invites exactly the wrong reading, because the two cases call for opposite responses: inconclusive means the data did not show an effect, underpowered means the design could not have shown one. Classify it separately and say how many pairs would be needed. Raise the cache value default to ten runners so a couple of stalls cannot take it back under the floor. Intervals printed one decimal, which rendered a 13 ms difference as `0.0s (95% CI -0.0 to 0.0)` in the action overhead report — the scenario built specifically to resolve tens of milliseconds. Print three. Point the transfer overlap baseline at v5.6.0. The concurrent-restore change it exists to detect landed between v5.6.0 and main, and starting from v4.8.0 straddled several unrelated changes whose sum read as no effect. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 71c45320-1417-4029-8402-69075d61dac1 --- .github/workflows/cache-value.yml | 6 +++++- .github/workflows/transfer-overlap.yml | 5 ++++- README.md | 2 ++ scripts/paired.mjs | 9 +++++++- scripts/report-action-overhead.mjs | 2 +- scripts/stats.mjs | 28 ++++++++++++++++++++++-- scripts/stats.test.mjs | 30 +++++++++++++++++++++++++- 7 files changed, 75 insertions(+), 7 deletions(-) diff --git a/.github/workflows/cache-value.yml b/.github/workflows/cache-value.yml index 8be7081..ed7ef7e 100644 --- a/.github/workflows/cache-value.yml +++ b/.github/workflows/cache-value.yml @@ -11,7 +11,11 @@ on: - "2" - "6" - "10" - default: "6" + - "14" + # The sign-flip test needs at least five usable pairs to reach 0.05 at + # all, and this workflow discards stalled runners, so a nominal six can + # land on four and report nothing about a 4.5x effect. Ten leaves room. + default: "10" setup-java-repository: description: Repository containing the setup-java action required: true diff --git a/.github/workflows/transfer-overlap.yml b/.github/workflows/transfer-overlap.yml index 962f27c..fea370d 100644 --- a/.github/workflows/transfer-overlap.yml +++ b/.github/workflows/transfer-overlap.yml @@ -21,7 +21,10 @@ on: baseline-ref: description: Git ref for the baseline arm required: true - default: v4.8.0 + # The concurrent-restore change this scenario exists to detect landed + # between v5.6.0 and main. Defaulting to v4.8.0 straddled several other + # changes at once and measured their sum, which read as no effect. + default: v5.6.0 type: string candidate-ref: description: Git ref for the candidate arm diff --git a/README.md b/README.md index 1d85cfb..8bcb8b0 100644 --- a/README.md +++ b/README.md @@ -25,6 +25,8 @@ Every job also runs one unmeasured warm-up slot first. The first setup in a job **Stalled runners are discarded.** A slot can stall on the cache service for several seconds. Which arm the stall lands on is arbitrary, so that runner contributes an arbitrarily large difference and, with ten runners, one such slot moves the mean by more than any effect being measured. Runners whose _own arm disagrees with itself_ by more than a robust threshold are dropped. That decision is made purely on within-arm spread, which has the same distribution whether or not the arms differ, so unlike filtering on the arm difference it cannot bias the result. Every report lists what it discarded and why. +**A design that cannot reach significance says so.** The sign-flip test has only 2^n distinct assignments for n paired runners, so its smallest attainable p-value is 2^-n however large the effect is. At four usable runners that floor is 0.0625, above the 0.05 a verdict requires — the run cannot report a finding even for an effect it measured perfectly. Those runs are reported as `underpowered` rather than `inconclusive`, because the two call for opposite readings: `inconclusive` means the data did not show an effect, `underpowered` means the design could not have shown one. The first live run of **Cache value** measured caching as 4.5x faster, 22.4 s, and reported `inconclusive` on four surviving runners; that is what this exists to stop. + Every report also publishes two guard rails: - A **noise floor**, the median spread between the two slots of the same arm on one runner. An effect smaller than this is reported as `within-noise` even when its interval excludes zero. diff --git a/scripts/paired.mjs b/scripts/paired.mjs index eb8dbca..188c4db 100644 --- a/scripts/paired.mjs +++ b/scripts/paired.mjs @@ -27,6 +27,7 @@ import { pairedInterval, pairedPermutationTest, quantile, + significanceReachable, standardDeviation, } from "./stats.mjs"; @@ -214,7 +215,12 @@ export function analyzePairs( interval, pValue, shiftSeconds: hodgesLehmann(candidateValues, baselineValues), - verdict: classify(interval, { noiseFloor: floor, pValue }), + verdict: classify(interval, { + noiseFloor: floor, + pValue, + pairCount: pairs.length, + }), + significanceReachable: significanceReachable(pairs.length), droppedRunners: dropped, stallThresholdSeconds: thresholdSeconds, control: { @@ -292,6 +298,7 @@ export function analyzeAgainstReference(rows, arms, reference) { verdict: isReference ? "reference" : classify(interval, { + pairCount: runners.length, noiseFloor: floor, pValue: pairedPermutationTest(differences, { seed: 50 + index }), }), diff --git a/scripts/report-action-overhead.mjs b/scripts/report-action-overhead.mjs index ddaf49c..440e6d4 100644 --- a/scripts/report-action-overhead.mjs +++ b/scripts/report-action-overhead.mjs @@ -282,7 +282,7 @@ export function markdown( if (overall) { lines.push( - `Pooled across ${overall.pairs.length} configurations: ${seconds(overall.interval.estimate)} ${formatInterval(overall.interval)}, permutation p = ${overall.pValue.toFixed(3)}, noise floor ${seconds(overall.noiseFloorSeconds)}.`, + `Pooled across ${overall.pairs.length} configurations: ${formatInterval(overall.interval)}, permutation p = ${overall.pValue.toFixed(3)}, noise floor ${seconds(overall.noiseFloorSeconds)}.`, "", `The A/A control — the two baseline slots against each other, which differ by nothing — reads ${overall.control.verdict}. Anything other than \`within-noise\` or \`inconclusive\` there means the harness is measuring something it should not, and the comparison above cannot be trusted.`, "", diff --git a/scripts/stats.mjs b/scripts/stats.mjs index ce5e203..21f3b42 100644 --- a/scripts/stats.mjs +++ b/scripts/stats.mjs @@ -7,6 +7,7 @@ const BOOTSTRAP_ITERATIONS = 10000; const DEFAULT_CONFIDENCE = 0.95; +const SIGNIFICANCE_LEVEL = 0.05; // Deterministic PRNG so a given set of samples always produces the same // interval. Reports are compared across runs and must not wobble because the @@ -168,6 +169,20 @@ export function pairedPermutationTest(differences, options = {}) { return (atLeastAsExtreme + 1) / (iterations + 1); } +// The sign-flip test has only 2^n distinct assignments for n paired +// observations, so its smallest attainable p-value is 2^-n no matter how large +// the effect is. At four pairs that floor is 0.0625, above the 0.05 the verdict +// requires — so a run with four usable runners cannot report a finding even for +// an effect it measured perfectly. That is a property of the design, not of the +// data, and it has to be said rather than dressed up as "collect more samples". +export function significanceReachable(pairCount, alpha = SIGNIFICANCE_LEVEL) { + return pairCount > 0 && Math.pow(2, -pairCount) <= alpha; +} + +export function pairsNeededForSignificance(alpha = SIGNIFICANCE_LEVEL) { + return Math.ceil(Math.log2(1 / alpha)); +} + // Holm's step-down correction controls the family-wise error rate when one // report makes several comparisons against the same reference. export function holmAdjust(pValues) { @@ -210,10 +225,17 @@ export function classify(interval, options = {}) { noiseFloor = 0, lowerIsBetter = true, pValue = null, - alpha = 0.05, + alpha = SIGNIFICANCE_LEVEL, + pairCount = null, } = options; if (!interval) return "unknown"; const { low, high, estimate } = interval; + // Reported before anything else, because when the design cannot reach alpha + // the p-value carries no information and "collect more samples" is the only + // honest reading of any verdict built on it. + if (pairCount !== null && !significanceReachable(pairCount, alpha)) { + return "underpowered"; + } if (low <= 0 && high >= 0) return "inconclusive"; if (pValue !== null && pValue >= alpha) return "inconclusive"; if (Math.abs(estimate) < noiseFloor) return "within-noise"; @@ -224,7 +246,7 @@ export function classify(interval, options = {}) { // The label follows the interval's own confidence level rather than assuming // 95%, so a caller that widens or narrows the interval cannot end up publishing // a number under the wrong label. -export function formatInterval(interval, { digits = 1, unit = "s" } = {}) { +export function formatInterval(interval, { digits = 3, unit = "s" } = {}) { if (!interval) return "n/a"; const { estimate, low, high, confidence = DEFAULT_CONFIDENCE } = interval; const level = Number((confidence * 100).toFixed(2)); @@ -241,6 +263,8 @@ export function describeVerdict(verdict) { return "No usable signal — the effect is smaller than the harness noise floor."; case "inconclusive": return "Inconclusive — the interval includes zero or the permutation test does not agree; collect more samples."; + case "underpowered": + return `Underpowered — too few paired runners for the permutation test to reach significance at all; it needs at least ${pairsNeededForSignificance()}. Any effect shown below may be real, but this run cannot establish it.`; default: return "Unknown."; } diff --git a/scripts/stats.test.mjs b/scripts/stats.test.mjs index 17a8bd2..0209fe1 100644 --- a/scripts/stats.test.mjs +++ b/scripts/stats.test.mjs @@ -5,6 +5,7 @@ import { bootstrapInterval, classify, createRandom, + describeVerdict, differenceInterval, formatInterval, holmAdjust, @@ -14,7 +15,9 @@ import { medianAbsoluteDeviation, pairedInterval, pairedPermutationTest, + pairsNeededForSignificance, quantile, + significanceReachable, standardDeviation, } from "./stats.mjs"; @@ -148,7 +151,7 @@ test("formatInterval labels the interval's own confidence level", () => { // Intervals built before `confidence` was carried still read as 95%. assert.equal( formatInterval({ estimate: 0, low: -1, high: 1 }), - "0.0s (95% CI -1.0 to 1.0)", + "0.000s (95% CI -1.000 to 1.000)", ); }); @@ -156,3 +159,28 @@ test("Holm adjustment controls a family of p-values", () => { assert.deepEqual(holmAdjust([0.01, 0.04, 0.2]), [0.03, 0.08, 0.2]); assert.deepEqual(holmAdjust([null, 0.01, 0.2]), [null, 0.02, 0.2]); }); + +// A four-runner design cannot clear 0.05 no matter how large the effect is, +// because the sign-flip test only has sixteen assignments to draw on. Reporting +// that as "inconclusive" invites someone to read the number anyway. +test("calls a design underpowered when significance is unreachable", () => { + assert.equal(significanceReachable(4), false); + assert.equal(significanceReachable(5), true); + assert.equal(pairsNeededForSignificance(), 5); + + const decisive = { estimate: -22, low: -28, high: -18 }; + assert.equal( + classify(decisive, { pValue: 0.061, pairCount: 4 }), + "underpowered", + ); + assert.equal( + classify(decisive, { pValue: 0.01, pairCount: 10 }), + "improvement", + ); +}); + +test("explains what an underpowered run can and cannot show", () => { + const described = describeVerdict("underpowered"); + assert.match(described, /at least 5/); + assert.match(described, /may be real/); +});