diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 9c73b861..3e6681c8 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -17,9 +17,22 @@ on: permissions: contents: read +# Every job below declares `timeout-minutes` (#2333). It is a HUNG-JOB GUARD, +# not a flake remedy: GitHub runners are not the contended machine the local +# gate runs on, and nothing measured implicates them — so no value here is +# sized for load. The rule, for every job in this file, is roughly TWICE the +# slowest run observed, rounded up to the next five minutes: real headroom for +# a slow runner, and a hang cut off in minutes instead of at GitHub's +# 360-minute default. Each job states the range it was read from; when one +# legitimately outgrows its number, raise it here with the new range. jobs: build: runs-on: ubuntu-latest + # Observed 7.6–11.8 min across the 40 push runs before this landed + # (2026-09-11 → 09-13), all since the coverage split (#2159). A release run + # before that split reached 15.6 min, with the coverage gate still inside + # this job — that shape no longer exists. + timeout-minutes: 25 steps: - name: Checkout code @@ -150,6 +163,8 @@ jobs: # time tests out at the 5s default. Separate jobs get separate runners. coverage: runs-on: ubuntu-latest + # Observed 5.9–8.5 min across the same 40 push runs as `build`. + timeout-minutes: 20 steps: - name: Checkout code @@ -195,6 +210,11 @@ jobs: concurrency: group: publish-npm cancel-in-progress: false + # Observed 2.3–2.8 min across the last three releases (2.4.0–2.6.0), of + # which `pack:verify` is about half and the only step that reaches the + # registry. The publish itself is atomic on npm's side, so a cut-off here + # leaves nothing half-published. + timeout-minutes: 10 permissions: contents: read # Required for npm provenance (`--provenance` mints a signed attestation @@ -283,6 +303,9 @@ jobs: if: github.event_name == 'release' environment: release needs: [build, coverage] + # Observed 14.5–15.4 min across the last three releases (2.4.0–2.6.0); + # the linux/arm64 half of the build runs under QEMU and is most of it. + timeout-minutes: 35 permissions: contents: read packages: write diff --git a/AGENTS.md b/AGENTS.md index bf5a4415..2cc2f883 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -472,8 +472,19 @@ free to run a full `local:gate`). Raising a budget nobody chose hides no race. per-project cap without re-running that measurement. - **The Playwright locator budgets the web smokes use are named constants in `scripts/lib/browser-timeouts.mjs`** — raise one there, not at a call site. - The *remaining* smoke-helper budgets and CI's missing `timeout-minutes` are - tracked on #2333 rather than settled here. + The shared flow helpers (`deep-link-connect.mjs`, `mcp-app-flow.mjs`) + default to the same constants, so a budget that moves there moves for every + smoke at once. The process-level budgets are stated the same way in their + own modules — `DEFAULTS` in `announced-child.mjs` and `render-smoke.mjs`, + `SCRIPT_PROBE_TIMEOUT_MS` in `pty.mjs` — each with the measurement that + sized it (#2333). +- **Every CI job declares `timeout-minutes`, sized from observed runs.** It is + a hung-job guard, not a flake remedy — GitHub runners are not the contended + machine — so the rule is roughly twice the slowest run observed, rounded up + to the next five minutes, stated per job in `.github/workflows/main.yml` + with the range it was read from. A job that starts taking longer gets its + number raised there, with the new range; the 360-minute default it replaces + is how a hang blocks a runner for six hours. - **Do not scale a fixed sleep.** A `setTimeout(r, N)` with no condition is not a timeout: it always waits the full window, so raising it slows every passing run and still races on a loaded one. Replace one with a condition wait when it diff --git a/scripts/lib/announced-child.mjs b/scripts/lib/announced-child.mjs index 52bc18a1..71f314d7 100644 --- a/scripts/lib/announced-child.mjs +++ b/scripts/lib/announced-child.mjs @@ -23,6 +23,34 @@ import { spawn } from "node:child_process"; import { setTimeout as delay } from "node:timers/promises"; +/** + * The readiness budget and poll interval, stated once (#2333) — the same shape + * as `render-smoke.mjs`'s `DEFAULTS`, and for the same reason: a budget that + * lives in a parameter default is one nobody revisits. + * + * `timeoutMs` is a ceiling on a poll, not a sleep: the wait returns on the + * first poll that sees the announcement, so a passing start pays only the + * child's real boot time plus, at most, one `pollMs`. Every smoke that needs a + * test server goes through this, which makes it the widest of the smoke + * budgets — and the one with the most headroom. Measured on the composable + * test server (`server-composable.js --config mcp-app-http`) through this + * helper with a 25ms poll, ten starts on a machine at load average 15: boot to + * announce was 135ms median, 137ms max. 30s is more than two hundred times + * that; what it has to absorb is not a slow server but a machine several + * gates deep, where a Node process can take seconds to reach its first line. + * + * `pollMs` is the passing path's cost. The same measurement at the default + * interval landed at 251–254ms every run, which is one interval exactly: the + * child announces well inside the first poll, so a start pays ~250ms of which + * ~115ms is waiting to look. That is under a second across a whole gate, so + * it is stated rather than tuned — lowering it buys nothing anyone would + * notice and costs a tighter scan loop on the child's output. + */ +export const DEFAULTS = Object.freeze({ + timeoutMs: 30_000, + pollMs: 250, +}); + /** * @param {object} opts * @param {string} opts.command Executable to spawn. @@ -34,8 +62,8 @@ import { setTimeout as delay } from "node:timers/promises"; * Called synchronously with the child immediately after spawn, before any * waiting. This is the caller's teardown handle. * @param {string} opts.what Noun used in error messages ("MCP test server"). - * @param {number} [opts.timeoutMs] Readiness budget (default 30s). - * @param {number} [opts.pollMs] Poll interval (default 250ms). + * @param {number} [opts.timeoutMs] Readiness budget (default `DEFAULTS.timeoutMs`). + * @param {number} [opts.pollMs] Poll interval (default `DEFAULTS.pollMs`). * @returns {Promise<{ child: import("node:child_process").ChildProcess, match: RegExpMatchArray }>} */ export async function startAnnouncedChild({ @@ -45,8 +73,8 @@ export async function startAnnouncedChild({ pattern, onSpawn, what, - timeoutMs = 30_000, - pollMs = 250, + timeoutMs = DEFAULTS.timeoutMs, + pollMs = DEFAULTS.pollMs, }) { const child = spawn(command, args, { cwd, diff --git a/scripts/lib/browser-timeouts.mjs b/scripts/lib/browser-timeouts.mjs index 37de15a6..87e383c2 100644 --- a/scripts/lib/browser-timeouts.mjs +++ b/scripts/lib/browser-timeouts.mjs @@ -8,7 +8,9 @@ * same three or four decisions repeated, so they are named once here and the * scripts import them — the same shape `render-smoke.mjs`'s `DEFAULTS` already * has, and for the same reason: a budget nobody can find is a budget nobody - * revisits. + * revisits. The shared flow helpers (`deep-link-connect.mjs`, + * `mcp-app-flow.mjs`) default their budgets to these too (#2333); they used to + * carry `ui`/`roundTrip` as literals of their own. * * Three of the four are ceilings on a poll rather than sleeps — `ui`, * `roundTrip` and `nested` each return the instant their locator condition diff --git a/scripts/lib/deep-link-connect.mjs b/scripts/lib/deep-link-connect.mjs index aaa7dfe4..17b51bc1 100644 --- a/scripts/lib/deep-link-connect.mjs +++ b/scripts/lib/deep-link-connect.mjs @@ -25,8 +25,17 @@ * Takes only the slice of Playwright's `page` it uses (`goto`, `locator` → * `waitFor`/`getAttribute`), which is what lets its failure branches be * unit-tested against a stand-in — a smoke only ever drives its happy path. + * + * The two budgets default to `BROWSER_TIMEOUTS` rather than restating them + * (#2333): `goto` and the status element attaching are the page loading and + * painting (`ui`), and `data-status="connected"` only appears after a round + * trip to the MCP server (`roundTrip`). `driveAppFlow` in `mcp-app-flow.mjs` + * passes the same two through, so the two helpers cannot drift apart — which + * is the point: they used to carry the same literals by hand. */ +import { BROWSER_TIMEOUTS } from "./browser-timeouts.mjs"; + /** * The deep link that connects to `mcpUrl`. * @@ -57,8 +66,8 @@ export async function connectViaDeepLink({ page, url, expectDeepLink = true, - gotoTimeoutMs = 30_000, - connectTimeoutMs = 45_000, + gotoTimeoutMs = BROWSER_TIMEOUTS.ui, + connectTimeoutMs = BROWSER_TIMEOUTS.roundTrip, }) { const response = await page.goto(url, { waitUntil: "domcontentloaded", diff --git a/scripts/lib/deep-link-connect.test.mjs b/scripts/lib/deep-link-connect.test.mjs index effd1457..113aff37 100644 --- a/scripts/lib/deep-link-connect.test.mjs +++ b/scripts/lib/deep-link-connect.test.mjs @@ -17,24 +17,35 @@ import assert from "node:assert/strict"; import { describe, it } from "node:test"; +import { BROWSER_TIMEOUTS } from "./browser-timeouts.mjs"; import { buildConnectDeepLink, connectViaDeepLink, } from "./deep-link-connect.mjs"; -/** Minimal Playwright `page` stand-in. `plan` maps a selector to its behavior. */ +/** + * Minimal Playwright `page` stand-in. `plan` maps a selector to its behavior. + * `gotoTimeouts` and `waits` record the budget each call was given, so a test + * can pin the defaults without a real browser. + */ function fakePage(plan, { gotoStatus = 200 } = {}) { const gotos = []; + const gotoTimeouts = []; + const waits = {}; return { gotos, - goto: async (url) => { + gotoTimeouts, + waits, + goto: async (url, opts) => { gotos.push(url); + gotoTimeouts.push(opts?.timeout); return { ok: () => gotoStatus === 200, status: () => gotoStatus }; }, locator: (selector) => { const entry = plan[selector] ?? {}; return { - waitFor: async () => { + waitFor: async (opts) => { + waits[selector] = opts?.timeout; if (entry.waitFails) throw new Error(`timeout: ${selector}`); }, getAttribute: async (name) => entry.attrs?.[name] ?? null, @@ -96,6 +107,18 @@ describe("connectViaDeepLink", () => { assert.deepEqual(page.gotos, ["http://x/"]); }); + it("defaults each budget to the shared BROWSER_TIMEOUTS entry (#2333)", async () => { + // These used to be 30_000 / 45_000 literals of the helper's own — the same + // values `browser-timeouts.mjs` names `ui` and `roundTrip`, kept in step by + // hand. Pinning the defaults to the constants is what makes a raise there + // reach this helper and `driveAppFlow` at once. + const page = fakePage(happyPlan()); + await connectViaDeepLink({ page, url: "http://x/" }); + assert.deepEqual(page.gotoTimeouts, [BROWSER_TIMEOUTS.ui]); + assert.equal(page.waits[STATUS], BROWSER_TIMEOUTS.ui); + assert.equal(page.waits[CONNECTED], BROWSER_TIMEOUTS.roundTrip); + }); + it("reports a non-200 document instead of waiting on selectors", async () => { await assert.rejects( connectViaDeepLink({ diff --git a/scripts/lib/mcp-app-flow.mjs b/scripts/lib/mcp-app-flow.mjs index 4a8082ff..4253bb29 100644 --- a/scripts/lib/mcp-app-flow.mjs +++ b/scripts/lib/mcp-app-flow.mjs @@ -30,6 +30,7 @@ import { join } from "node:path"; import { startAnnouncedChild } from "./announced-child.mjs"; +import { BROWSER_TIMEOUTS } from "./browser-timeouts.mjs"; import { buildConnectDeepLink, connectViaDeepLink, @@ -167,6 +168,13 @@ export function buildAppDeepLink({ * Takes only the small slice of Playwright's `page` it uses (`goto`, `locator` * → `waitFor`/`getAttribute`/`count`), which is what lets its failure paths be * unit-tested against a stand-in — a smoke only ever exercises its happy path. + * + * The three budgets are `BROWSER_TIMEOUTS` entries, not restated literals + * (#2333). The first two are handed straight to `connectViaDeepLink`, so the + * two helpers share one pair by construction. `ready` is a `roundTrip` too: + * the widget has to load inside the sandbox and answer through the bridge + * before the attribute can flip, which is the same shape as the connect wait, + * not a render of something already on screen (`nested`). */ export async function driveAppFlow({ page, @@ -174,9 +182,9 @@ export async function driveAppFlow({ expectDeepLink = true, what = "app", extraDiagnostics = null, - gotoTimeoutMs = 30_000, - connectTimeoutMs = 45_000, - readyTimeoutMs = 45_000, + gotoTimeoutMs = BROWSER_TIMEOUTS.ui, + connectTimeoutMs = BROWSER_TIMEOUTS.roundTrip, + readyTimeoutMs = BROWSER_TIMEOUTS.roundTrip, }) { // 1-2. Navigated, the deep link accepted by the token gate, and connected. await connectViaDeepLink({ diff --git a/scripts/lib/mcp-app-flow.test.mjs b/scripts/lib/mcp-app-flow.test.mjs index 1c331c8c..985ae79a 100644 --- a/scripts/lib/mcp-app-flow.test.mjs +++ b/scripts/lib/mcp-app-flow.test.mjs @@ -16,6 +16,7 @@ import assert from "node:assert/strict"; import { describe, it } from "node:test"; +import { BROWSER_TIMEOUTS } from "./browser-timeouts.mjs"; import { buildAppDeepLink, driveAppFlow, @@ -23,9 +24,15 @@ import { sandboxProxyPageFor, } from "./mcp-app-flow.mjs"; -/** Minimal Playwright `page` stand-in. `plan` maps a selector to its behavior. */ +/** + * Minimal Playwright `page` stand-in. `plan` maps a selector to its behavior. + * `waits` records the budget each `waitFor` was given, so a test can pin the + * defaults without a real browser. + */ function fakePage(plan, { gotoStatus = 200 } = {}) { + const waits = {}; return { + waits, goto: async () => ({ ok: () => gotoStatus === 200, status: () => gotoStatus, @@ -33,7 +40,8 @@ function fakePage(plan, { gotoStatus = 200 } = {}) { locator: (selector) => { const entry = plan[selector] ?? {}; return { - waitFor: async () => { + waitFor: async (opts) => { + waits[selector] = opts?.timeout; if (entry.waitFails) throw new Error(`timeout: ${selector}`); }, getAttribute: async (name) => entry.attrs?.[name] ?? null, @@ -110,6 +118,17 @@ describe("driveAppFlow", () => { await driveAppFlow({ page: fakePage(happyPlan()), url: "http://x/" }); }); + it("shares its connect budgets with connectViaDeepLink and waits a roundTrip for ready (#2333)", async () => { + // The connect pair is handed straight through, so `ui`/`roundTrip` here + // are the same two `connectViaDeepLink` defaults to; `ready` is a round + // trip of its own (sandbox load + bridge handshake), not a `nested` render. + const page = fakePage(happyPlan()); + await driveAppFlow({ page, url: "http://x/" }); + assert.equal(page.waits[STATUS], BROWSER_TIMEOUTS.ui); + assert.equal(page.waits[CONNECTED], BROWSER_TIMEOUTS.roundTrip); + assert.equal(page.waits[READY], BROWSER_TIMEOUTS.roundTrip); + }); + it("reports a non-200 document instead of waiting on selectors", async () => { await assert.rejects( driveAppFlow({ diff --git a/scripts/lib/pty.mjs b/scripts/lib/pty.mjs index d311ba68..74be1485 100644 --- a/scripts/lib/pty.mjs +++ b/scripts/lib/pty.mjs @@ -108,6 +108,26 @@ export function ptyCommand({ command, args = [], flavor }) { throw new Error(`unknown script(1) flavor: ${flavor}`); } +/** + * How long `script --version` gets to exit (#2333). + * + * A ceiling on a synchronous spawn, not a sleep: the probe returns the moment + * the process exits, and `script --version` exits at once on every flavor — + * BSD `script` with a usage error, util-linux and busybox with a version line. + * Measured here on darwin, twenty spawns with the machine at load average 4 + * (recent peak 19): 1.3ms median, 3.6ms max. Five seconds is over a thousand + * times that, and what it has to absorb is process creation on a machine + * several gates deep, not the command. + * + * The number matters less than what its expiry means. `spawnSync` reports a + * timeout in the same `error` slot as ENOENT, and `probeScriptVersion` used to + * read both as "no `script(1)`" — so a starved probe turned `smoke:tui` into a + * documented **skip**, exit 0, with a message blaming the machine's PATH. A + * false skip on the one pre-push gate is worse than a false failure, which is + * why the timeout is now told apart and thrown instead (see below). + */ +export const SCRIPT_PROBE_TIMEOUT_MS = 5_000; + /** * Probe the local `script(1)`: is it there, and what does it say about itself? * @@ -124,23 +144,45 @@ export function ptyCommand({ command, args = [], flavor }) { * answers `illegal option -- -` plus its usage on stderr — which is exactly the * evidence `scriptFlavorFor` reads. Only `error` means "could not run it". * - * @param {(cmd: string, args: string[]) => { stdout?: string, stderr?: string, error?: Error }} [runner] + * A **timeout** is not unavailability either, and it is the one `error` that + * must not be folded into "not available": `script(1)` was found and started, + * and the machine did not let it finish. Reporting that as "no `script(1)` on + * PATH" would make `smoke:tui` skip — a green exit on a run that tested + * nothing — so it throws, naming the budget, and the caller fails loudly. + * + * @param {(cmd: string, args: string[]) => { stdout?: string, stderr?: string, error?: Error & { code?: string } }} [runner] * Injected for tests; defaults to a real `spawnSync`. * @returns {{ available: boolean, output: string }} + * @throws {Error} when the probe timed out (`error.code === "ETIMEDOUT"`). */ export function probeScriptVersion( runner = (cmd, args) => - spawnSync(cmd, args, { encoding: "utf8", timeout: 5000 }), + spawnSync(cmd, args, { + encoding: "utf8", + timeout: SCRIPT_PROBE_TIMEOUT_MS, + }), ) { + let r; try { - const r = runner("script", ["--version"]); - // `error` covers ENOENT, EACCES and the timeout; a missing result at all - // means the runner told us nothing, which is not evidence of a working one. - if (!r || r.error) return { available: false, output: "" }; - return { available: true, output: `${r.stdout ?? ""}${r.stderr ?? ""}` }; - } catch { - return { available: false, output: "" }; + r = runner("script", ["--version"]); + } catch (err) { + // A runner that throws outright (injected, or a future spawn shape) is + // normalized onto the same path as one that returns `{ error }`, so the + // timeout check below applies to both shapes — a thrown ETIMEDOUT must not + // slip back into "unavailable" (Copilot, #2333). + r = { error: err }; } + if (r?.error?.code === "ETIMEDOUT") { + throw new Error( + `\`script --version\` did not exit within ${SCRIPT_PROBE_TIMEOUT_MS}ms — ` + + "a starved or hung probe, not a missing `script(1)`, so smoke:tui " + + "must not skip on it", + ); + } + // `error` covers ENOENT and EACCES; a missing result at all means the runner + // told us nothing, which is not evidence of a working one. + if (!r || r.error) return { available: false, output: "" }; + return { available: true, output: `${r.stdout ?? ""}${r.stderr ?? ""}` }; } /** @@ -151,11 +193,16 @@ export function probeScriptVersion( * invocation" send the reader somewhere different — so the failure carries a * `reason` rather than being a bare `null` the caller has to narrate. * + * A probe that **timed out** is neither — it throws through here untouched, + * because "the machine would not run `script`" is a failure to report, not a + * reason to skip (see `probeScriptVersion`). + * * @param {object} [opts] * @param {string} [opts.platform] * @param {() => { available: boolean, output: string }} [opts.probe] * @returns {{ ok: true, flavor: string, wrap: (spec: { command: string, args?: string[] }) => { command: string, args: string[] } } * | { ok: false, reason: string }} + * @throws {Error} when the probe timed out. */ export function resolvePtyWrapper({ platform = process.platform, diff --git a/scripts/lib/pty.test.mjs b/scripts/lib/pty.test.mjs index ceb1fbcb..3f4d4fc0 100644 --- a/scripts/lib/pty.test.mjs +++ b/scripts/lib/pty.test.mjs @@ -4,6 +4,7 @@ import test from "node:test"; import { ptyCommand, probeScriptVersion, + SCRIPT_PROBE_TIMEOUT_MS, resolvePtyWrapper, scriptFlavorFor, shellQuote, @@ -120,7 +121,7 @@ test("shellQuote survives an embedded single quote", () => { assert.equal(shellQuote("plain"), "'plain'"); }); -test("probeScriptVersion combines both streams and never throws", () => { +test("probeScriptVersion combines both streams and reads a runner failure as unavailable", () => { assert.deepEqual( probeScriptVersion(() => ({ stdout: "out ", stderr: "err" })), { available: true, output: "out err" }, @@ -168,13 +169,46 @@ test("probeScriptVersion treats result.error as unavailable, not as empty output probeScriptVersion(() => ({ error: enoent, stdout: "", stderr: "" })), { available: false, output: "" }, ); - // The timeout shape reports the same way. + // An `error` with no recognisable code is still "could not run it". assert.equal( - probeScriptVersion(() => ({ error: new Error("ETIMEDOUT") })).available, + probeScriptVersion(() => ({ error: new Error("EACCES") })).available, false, ); }); +// The one `error` that must NOT read as unavailable (#2333). `script(1)` was +// found and started; the machine did not let it finish. Folded into +// `available: false` it becomes a `smoke:tui` SKIP — exit 0, blaming PATH — on +// exactly the loaded machine the gate is supposed to be believable on. +test("probeScriptVersion throws on a timed-out probe instead of reporting no script(1)", () => { + const timedOut = Object.assign(new Error("spawnSync script ETIMEDOUT"), { + code: "ETIMEDOUT", + }); + assert.throws( + () => + probeScriptVersion(() => ({ error: timedOut, stdout: "", stderr: "" })), + new RegExp(`did not exit within ${SCRIPT_PROBE_TIMEOUT_MS}ms`), + ); + // A runner that THROWS the same error is normalized onto the same path — the + // `catch` must not quietly turn a thrown ETIMEDOUT back into "unavailable". + assert.throws( + () => + probeScriptVersion(() => { + throw timedOut; + }), + /did not exit within/, + ); + // And it reaches the caller: resolvePtyWrapper must not turn it into a skip. + assert.throws( + () => + resolvePtyWrapper({ + platform: "darwin", + probe: () => probeScriptVersion(() => ({ error: timedOut })), + }), + /did not exit within/, + ); +}); + test("resolvePtyWrapper reports a missing script(1) rather than guessing", () => { // linux would otherwise be classified util-linux off the empty output. const r = resolvePtyWrapper({ diff --git a/scripts/smoke-tui.mjs b/scripts/smoke-tui.mjs index 7f17a288..23fe2f96 100644 --- a/scripts/smoke-tui.mjs +++ b/scripts/smoke-tui.mjs @@ -93,7 +93,16 @@ if (!existsSync(launcher)) { // No `script(1)` (Windows). Running without a PTY is not a weaker check, it is // a guaranteed failure — so say why rather than report a crash as a defect. // `node-pty` is the portable answer if this ever needs to run there. -const pty = resolvePtyWrapper(); +let pty; +try { + pty = resolvePtyWrapper(); +} catch (err) { + // The probe timed out: `script(1)` exists and the machine did not let it + // finish. That is a starved gate, not a platform without a PTY — reported as + // a failure so a load spike cannot quietly turn this smoke into a skip + // (#2333). The budget and the measurement are on `SCRIPT_PROBE_TIMEOUT_MS`. + fail(err instanceof Error ? err.message : String(err)); +} if (!pty.ok) { skip( `${pty.reason} — the Ink TUI needs raw mode, which requires a real ` +