diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 261e9a607952..36e94a0d75cf 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -166,10 +166,13 @@ jobs: # 21.8s local to 32-73s on CI. Keep this bound honest by re-measuring # rather than by analogy -- but if you add a test here that waits on a new # budget, it lands on the bound directly, so check the total. + # BLO-32670: 3, not 1. Measured p100 53s across 58 sampled runs (p50 24s) + # — 88% of the old one-minute bound, the tightest margin of all 41 + # bounded steps in this job. It had not tripped yet; it was next. - name: Test pending-migration pre-flight phase budgets (BLO-31254) if: ${{ !cancelled() }} run: node --test ./scripts/check-pending-migration-preflight-phases.test.js - timeout-minutes: 1 + timeout-minutes: 3 # BLO-29008: executes the two-tier convergence gate's shell against # recorded cluster states. The gate fails OPEN if its string accumulation @@ -180,10 +183,20 @@ jobs: run: node --test ./scripts/check-docker-two-tier-convergence.test.js timeout-minutes: 1 + # BLO-32670: 3, not 1. This suite forks 150 short-lived `bash` processes + # across its 96 tests, because it exercises the shipping backoff function + # by sourcing it rather than by asserting against a copy. That makes its + # wall time track runner CPU contention rather than its own work: ~9s on + # an idle host, 44s p100 across 58 sampled runs, and 73s observed on + # 2026-09-07 — which tripped the old one-minute bound and failed `policy` + # on a PR whose diff touched only `server/src/services/recovery/*`. + # Sizing an ~9s task at 60s reads as 7x headroom; against a p100 that is + # already 5x the median it was not. The bound still catches a hang, far + # inside the 10m job cap — it is no longer a duration tripwire. - name: Test approval admissibility-probe backoff (BLO-28471) if: ${{ !cancelled() }} run: node --test ./scripts/approve-paperclip-api-digest.test.js - timeout-minutes: 1 + timeout-minutes: 3 - name: Test pnpm setup retry wrapper (BLO-28813) if: ${{ !cancelled() }} @@ -344,7 +357,10 @@ jobs: # blocks against a stubbed `gh` and asserts the exit codes. if: ${{ !cancelled() }} run: node --test ./scripts/skill-bounded-pr-check-polling.test.mjs - timeout-minutes: 1 + # BLO-32670: 3, not 1. Measured p100 53s across 58 sampled runs against + # a p50 of 7s — a 7.6x spread, the widest in this job, because the + # fenced blocks fork a stubbed `gh` per case. + timeout-minutes: 3 # BLO-31405: the chart render suite deliberately does NOT run here. It has # its own `helm_chart` job below; see the comment there before adding it diff --git a/scripts/__tests__/policy-node-test-timeouts.test.mjs b/scripts/__tests__/policy-node-test-timeouts.test.mjs index 0152f32c28ce..a531ab5cc3d0 100644 --- a/scripts/__tests__/policy-node-test-timeouts.test.mjs +++ b/scripts/__tests__/policy-node-test-timeouts.test.mjs @@ -16,10 +16,55 @@ function nodeTestSteps() { return policySteps().filter((step) => step.includes("node --test")); } -function assertTimeouts(steps) { +// BLO-32670. GitHub Actions accepts fractional minutes. Matching only `(\d+)` +// makes `timeout-minutes: 1.5` read as declaring no bound at all, so the guard +// rejects such a step with the exact opposite of what its author wrote. Parse +// the fraction and let the numeric comparisons below do the work. +const TIMEOUT_MINUTES = String.raw`timeout-minutes: (\d+(?:\.\d+)?)`; + +function timeoutMinutes(text, indent) { + return Number(text.match(new RegExp(`\\n {${indent}}${TIMEOUT_MINUTES}\\n`))?.[1]); +} + +// Sizing is per-step evidence and belongs with the step; this reads only the +// ceiling that every step bound has to sit under. +function policyJobCap() { + const cap = timeoutMinutes(jobRegion("policy"), 4); + assert.ok(cap > 0, "policy must declare a job-level timeout-minutes"); + return cap; +} + +function stepName(step) { + return step.split("\n")[0].trim(); +} + +// BLO-32670. This used to assert the literal `timeout-minutes: 1` on every +// step, which made the bound a fixed number rather than a sufficient one: a +// step whose measured p100 outgrew 60s could not be right-sized without failing +// this test, so the only in-repo remedy was to leave it as a tripwire. It fired +// exactly that way on 2026-09-07, red-lining a PR that had not touched the +// script under test. The invariant worth gating is the one already written for +// the chart render step below — that a bound EXISTS and sits under the job cap. +// +// `bound < cap` is a ceiling, not a guarantee of attributability at every value +// under it: a bound close to the cap is still reached only after earlier steps +// have spent part of the job budget, so the job cap kills it first and the +// failure is a bare `cancelled` again. It holds here because the real bounds are +// 1-3m against a 10m cap. Summing the bounds against the cap would be the wrong +// stronger rule — bounds are per-step worst cases and every step is expected to +// run, so that sum exceeds any sane cap by design. +function assertTimeouts(steps, cap = policyJobCap()) { assert.ok(steps.length > 0, "policy must contain node --test steps"); for (const step of steps) { - assert.match(step, /\n timeout-minutes: 1\n/, "each policy node --test step must have a one-minute bound"); + const bound = timeoutMinutes(step, 8); + assert.ok( + bound > 0, + `policy node --test step "${stepName(step)}" must declare a step-level timeout-minutes`, + ); + assert.ok( + bound < cap, + `"${stepName(step)}" bound ${bound}m must sit below the ${cap}m policy job cap`, + ); } } @@ -28,8 +73,78 @@ test("every policy node --test step has a step-level timeout", () => { }); test("the timeout guard fails when a node --test bound is removed", () => { - const mutated = nodeTestSteps().map((step) => step.replace("\n timeout-minutes: 1\n", "\n")); - assert.throws(() => assertTimeouts(mutated), /one-minute bound/); + const mutated = nodeTestSteps().map((step) => + step.replace(new RegExp(`\\n {8}${TIMEOUT_MINUTES}\\n`), "\n"), + ); + assert.throws(() => assertTimeouts(mutated), /must declare a step-level timeout-minutes/); +}); + +test("the timeout guard fails when a node --test bound reaches the job cap", () => { + const cap = policyJobCap(); + const mutated = nodeTestSteps().map((step) => + step.replace(new RegExp(`\\n {8}${TIMEOUT_MINUTES}\\n`), `\n timeout-minutes: ${cap}\n`), + ); + assert.throws(() => assertTimeouts(mutated, cap), /must sit below the/); +}); + +// BLO-32670. Three of the 41 bounded steps in `policy` measured a p100 above +// 50% of the old 60s budget over 58 sampled runs; the other 38 were all at or +// under 25%. These three are the fork-heavy ones — they shell out per case, so +// their wall time tracks runner CPU contention rather than their own work, +// which is why they inflate while their neighbours (dominated by deliberate +// sleeps) barely move. Against the in-band contention barometer — this job's +// own unbounded `Checkout repository` step, which ranges 47s to 280s on the +// same runners — they correlate at r ~= 0.35 and run 2.3x slower in the +// high-load tercile, where trivial steps sit at r ~= 0.05 and 1.0x. +// +// Pinned as a floor, not an exact value, so raising one further stays a +// one-line change. What must fail loudly is a revert to 1: that reads as +// harmless cleanup, restores the tripwire, and the next red `policy` again +// lands on whichever PR happens to be in the contention window. +const CONTENTION_SENSITIVE_FLOOR_MINUTES = 3; +const CONTENTION_SENSITIVE_STEPS = [ + "Test approval admissibility-probe backoff (BLO-28471)", + "Test pending-migration pre-flight phase budgets (BLO-31254)", + "Test bounded PR-check polling skills", +]; + +function assertContentionFloor(steps) { + for (const name of CONTENTION_SENSITIVE_STEPS) { + const step = steps.find((candidate) => stepName(candidate) === name); + assert.ok(step, `policy must still contain the contention-sensitive step "${name}"`); + const bound = timeoutMinutes(step, 8); + assert.ok( + bound >= CONTENTION_SENSITIVE_FLOOR_MINUTES, + `"${name}" is fork-heavy and measured a p100 above 50% of a one-minute budget, so its bound must stay at or above ${CONTENTION_SENSITIVE_FLOOR_MINUTES}m — found ${bound}m`, + ); + } +} + +// BLO-32670. The floor and the `bound < cap` ceiling are coupled constraints: a +// policy job cap at or below the floor makes them jointly unsatisfiable. Without +// this the collision surfaces as a per-step "must sit below the 3m policy job +// cap" failure, which reads as a problem with whichever step is checked first +// rather than with the pair of rules. #1642 is the PR that sizes that cap, so +// name the collision here instead of letting it land on a step. +test("the contention floor and the policy job cap stay jointly satisfiable", () => { + const cap = policyJobCap(); + assert.ok( + CONTENTION_SENSITIVE_FLOOR_MINUTES < cap, + `the ${CONTENTION_SENSITIVE_FLOOR_MINUTES}m contention floor cannot coexist with a ${cap}m policy job cap — raise the cap, or re-measure the floor and lower it`, + ); +}); + +test("fork-heavy policy steps keep a bound sized against their measured p100", () => { + assertContentionFloor(policySteps()); +}); + +test("the contention floor fails when a fork-heavy step is re-tightened to one minute", () => { + const mutated = policySteps().map((step) => + CONTENTION_SENSITIVE_STEPS.includes(stepName(step)) + ? step.replace(new RegExp(`\\n {8}${TIMEOUT_MINUTES}\\n`), "\n timeout-minutes: 1\n") + : step, + ); + assert.throws(() => assertContentionFloor(mutated), /must stay at or above/); }); test("policy continues after a bounded test failure unless cancelled", () => { @@ -88,20 +203,20 @@ test("the chart render suite runs in exactly one job, and that job is helm_chart // BLO-29182 observed this exact invocation hang, and its fix bounded the copy // that used to live in `policy`. Removing that copy has to carry the bound with -// it, or the one `node --test` step known to hang is unbounded again — a hung -// step would burn the whole job budget instead of failing attributably. The -// margin (4 min against a 73s p100) lives in the workflow comment; the -// invariant worth gating is only that a step bound exists and is under the cap. +// it, or the one `node --test` step known to hang is unbounded again, and burns +// the whole job budget instead of failing as itself. The margin (4 min against a +// 73s p100) lives in the workflow comment; the invariant worth gating is only +// that a step bound exists and is under the cap. test("the chart render step is bounded, and inside its job's budget (BLO-29182)", () => { const region = jobRegion("helm_chart"); - const jobCap = Number(region.match(/\n timeout-minutes: (\d+)\n/)?.[1]); + const jobCap = timeoutMinutes(region, 4); assert.ok(jobCap > 0, "helm_chart must declare a job-level timeout-minutes"); const step = region .split("\n - name: ") .slice(1) .find((candidate) => candidate.includes(CHART_SUITE)); assert.ok(step, `helm_chart must contain the ${CHART_SUITE} step`); - const stepBound = Number(step.match(/\n timeout-minutes: (\d+)\n/)?.[1]); + const stepBound = timeoutMinutes(step, 8); assert.ok(stepBound > 0, "the chart render step must declare a step-level timeout-minutes"); assert.ok(stepBound < jobCap, `step bound ${stepBound}m must sit below the ${jobCap}m job cap`); });