diff --git a/scripts/build/ci.ts b/scripts/build/ci.ts index 9a91c4caad04..603d79a6a382 100644 --- a/scripts/build/ci.ts +++ b/scripts/build/ci.ts @@ -503,9 +503,8 @@ function makeZip(cfg: Config, name: string, files: string[]): string { * after download. * * rust-and-link runs in parallel with build-cpp (no depends_on), so it - * POLLS `buildkite-agent step get outcome` for the cpp step until it passes - * before attempting the download. link-only has depends_on and skips the - * poll. + * POLLS `buildkite-agent step get` for the cpp step until it passes before + * attempting the download. link-only has depends_on and skips the poll. * * Call BEFORE ninja — the downloaded files are ninja's link inputs. */ @@ -601,54 +600,103 @@ function runAsync(argv: string[], cwd: string): Promise { }); } +/** One `buildkite-agent step get ` read. */ +export interface StepGetResult { + ok: boolean; + out: string; + err: string; +} + +function agentStepGet(stepKey: string, attr: string): StepGetResult { + const r = spawnSync("buildkite-agent", ["step", "get", attr, "--step", stepKey], { encoding: "utf8" }); + if (r.error) { + throw new BuildError(`Failed to spawn buildkite-agent step get ${attr} --step ${stepKey}`, { cause: r.error }); + } + return { ok: r.status === 0, out: (r.stdout ?? "").trim(), err: (r.stderr ?? "").trim() }; +} + /** - * Poll `buildkite-agent step get outcome --step ` until the step - * reaches a terminal state. Returns on "passed"; throws on any failure - * outcome so the caller exits 1 with a message that points at the real - * failing step (rather than a downstream "artifact not found"). + * Poll `buildkite-agent step get` until `` reaches a terminal state. + * Returns on "passed"; throws on a terminal failure so the caller exits 1 with + * a message that points at the real failing step (rather than a downstream + * "artifact not found"). + * + * `outcome` alone is not enough: Buildkite reports the last *completed* job's + * outcome, so an earlier attempt that errored/expired reads as `errored` even + * while an automatic or manual retry is queued or running. `state` is the + * step-level state (`ready`/`running`/`failing`/`finished`/…, distinct from + * REST's job states; see https://buildkite.com/docs/agent/v3/cli-step), so a + * failure outcome is only believed once `state` is terminal too. Build 84838's + * windows-x64-build-bun bailed on `outcome: errored [0s]` while + * windows-x64-build-cpp's retry was still queued; build 85043's linux-aarch64 + * logged the expected `state=running` → `state=finished`. + * + * Exported for test/internal/ci-sibling-wait.test.ts; `opts` is the test seam. */ -async function waitForStepOutcome(stepKey: string): Promise { - const failed = new Set(["hard_failed", "soft_failed", "errored", "canceled", "cancelled"]); +export async function waitForStepOutcome( + stepKey: string, + opts: { pollMs?: number; get?: (stepKey: string, attr: string) => StepGetResult } = {}, +): Promise { + const { pollMs = 3000, get = agentStepGet } = opts; + const failedOutcome = new Set(["hard_failed", "soft_failed", "errored", "canceled", "cancelled"]); + // `step get state` returns step states, not job states. The terminal ones per + // Buildkite's glossary are `finished`/`canceled`/`ignored`; everything else + // (`waiting_for_dependencies`/`ready`/`running`/`failing`, or a value we + // haven't seen) means a job for this step may still produce a pass. An empty + // `state` falls back to outcome-only (the pre-retry-aware behaviour). + const terminalState = new Set(["finished", "canceled", "cancelled", "ignored"]); const sleep = (ms: number) => new Promise(r => setTimeout(r, ms)); const start = Date.now(); const deadlineMs = 60 * 60 * 1000; let last = ""; + let terminalReads = 0; console.log(`Waiting for ${stepKey} to finish...`); for (;;) { - const result = spawnSync("buildkite-agent", ["step", "get", "outcome", "--step", stepKey], { encoding: "utf8" }); - if (result.error) { - throw new BuildError(`Failed to spawn buildkite-agent`, { cause: result.error }); - } - if (result.status !== 0) { - const err = (result.stderr ?? "").trim(); - if (err !== last) { - console.log(` buildkite-agent step get exited ${result.status}: ${err}`); - last = err; + const outcome = get(stepKey, "outcome"); + if (!outcome.ok) { + if (outcome.err !== last) { + console.log(` buildkite-agent step get outcome failed: ${outcome.err}`); + last = outcome.err; } if (Date.now() - start > deadlineMs) { - throw new BuildError(`buildkite-agent step get kept failing for ${stepKey}`, { hint: err }); + throw new BuildError(`buildkite-agent step get kept failing for ${stepKey}`, { hint: outcome.err }); } - await sleep(3000); + await sleep(pollMs); continue; } - const outcome = (result.stdout ?? "").trim(); - if (outcome !== last) { + const state = get(stepKey, "state"); + const stateVal = state.ok ? state.out : `<${state.err || "error"}>`; + const display = `${outcome.out || "(none)"} / state=${stateVal || "(none)"}`; + if (display !== last) { const elapsed = Math.round((Date.now() - start) / 1000); - console.log(` ${stepKey} outcome: ${outcome || "(running)"} [${elapsed}s]`); - last = outcome; + console.log(` ${stepKey} outcome=${display} [${elapsed}s]`); + last = display; } - if (outcome === "passed") return; - if (failed.has(outcome)) { - throw new BuildError(`Sibling step ${stepKey} ${outcome} — nothing to link`, { - hint: `See the ${stepKey} job for the real error; this step only downloads its artifacts.`, - }); + if (outcome.out === "passed") return; + // A successful-but-empty `state` falls back to outcome-only (pre-change + // behaviour). A FAILED `state` read is not evidence of anything — the + // outcome read in this same iteration worked, so the agent and API are + // reachable; treat it as non-terminal rather than let a flaky second call + // undo the retry-awareness this poll exists for. + const stateIsTerminal = state.ok && (state.out === "" || terminalState.has(state.out)); + if (failedOutcome.has(outcome.out) && stateIsTerminal) { + // Two consecutive terminal reads before giving up: a retry job appears + // ~1s after its predecessor ends, so a single poll can see `finished` + // before the step goes back to `ready` for the retry. + if (++terminalReads >= 2) { + throw new BuildError(`Sibling step ${stepKey} ${outcome.out} — nothing to link`, { + hint: `See the ${stepKey} job for the real error; this step only downloads its artifacts.`, + }); + } + } else { + terminalReads = 0; } if (Date.now() - start > deadlineMs) { throw new BuildError(`Timed out after 60m waiting for ${stepKey}`, { hint: `${stepKey} never reached a terminal outcome; check that job for a hang.`, }); } - await sleep(3000); + await sleep(pollMs); } } diff --git a/test/internal/ci-sibling-wait.test.ts b/test/internal/ci-sibling-wait.test.ts new file mode 100644 index 000000000000..57688957b791 --- /dev/null +++ b/test/internal/ci-sibling-wait.test.ts @@ -0,0 +1,119 @@ +/** + * Unit tests for the build-bun → build-cpp sibling poll in + * scripts/build/ci.ts::waitForStepOutcome(). + * + * Buildkite's `step get outcome` reports the last *completed* job, so an + * earlier attempt that expired in the queue reads as `errored` even while a + * retry is queued or running. The poll must consult `step get state` (the + * step-level state: `ready`/`running`/`failing`/`finished`/…) and only give up + * once that is terminal too. Build 85043's linux-aarch64-build-bun logged + * `state=running` → `state=finished`, which is the vocabulary these fixtures + * use. + */ +import { describe, expect, test } from "bun:test"; +import { waitForStepOutcome, type StepGetResult } from "../../scripts/build/ci.ts"; + +type Read = string | { ok: false; err: string }; +const fail = (err: string): Read => ({ ok: false, err }); + +/** Drive `waitForStepOutcome` through a canned sequence of `outcome`/`state` reads. */ +function run(script: ReadonlyArray<{ outcome: Read; state: Read }>) { + let i = 0; + const get = (_stepKey: string, attr: "outcome" | "state"): StepGetResult => { + const entry = script[Math.min(i, script.length - 1)]!; + const v = entry[attr]; + // One poll is `outcome` then (if outcome was ok) `state`; advance after the + // last read in the poll so both reads see the same entry. + if (attr === "state" || (attr === "outcome" && typeof v !== "string")) i++; + return typeof v === "string" ? { ok: true, out: v, err: "" } : { ok: false, out: "", err: v.err }; + }; + return waitForStepOutcome("linux-x64-build-cpp", { pollMs: 0, get }); +} + +describe.concurrent("waitForStepOutcome", () => { + test("resolves once the sibling passes", async () => { + await run([ + { outcome: "", state: "running" }, + { outcome: "passed", state: "finished" }, + ]); + }); + + test("keeps polling while a retry is queued after an earlier error", async () => { + // Build 84838: attempt 1 expired (outcome=errored), attempt 2 sat in the + // queue. The old outcome-only poll bailed at index 0. + await run([ + { outcome: "errored", state: "ready" }, + { outcome: "errored", state: "ready" }, + { outcome: "errored", state: "running" }, + { outcome: "passed", state: "finished" }, + ]); + }); + + test("keeps polling through state=failing", async () => { + // `failing` is a documented non-terminal step state (a job failed but the + // step has not settled); a retry can still turn it around. + await run([ + { outcome: "errored", state: "failing" }, + { outcome: "errored", state: "failing" }, + { outcome: "errored", state: "running" }, + { outcome: "passed", state: "finished" }, + ]); + }); + + test("keeps polling through an unknown state value", async () => { + // A state we have not enumerated must not be mistaken for terminal; the + // 60 min deadline still bounds the wait. + await run([ + { outcome: "errored", state: "limiting" }, + { outcome: "errored", state: "limiting" }, + { outcome: "passed", state: "finished" }, + ]); + }); + + test("tolerates the gap between one attempt finishing and its retry appearing", async () => { + // A retry job is created ~1s after its predecessor ends, so one poll can + // land on state=finished before the next attempt takes it back to ready. + await run([ + { outcome: "errored", state: "finished" }, + { outcome: "errored", state: "ready" }, + { outcome: "passed", state: "finished" }, + ]); + }); + + test("throws once the sibling is terminally failed with no retry in flight", async () => { + await expect( + run([ + { outcome: "hard_failed", state: "finished" }, + { outcome: "hard_failed", state: "finished" }, + ]), + ).rejects.toThrow("linux-x64-build-cpp hard_failed"); + }); + + test("falls back to outcome when state is unavailable", async () => { + // A successful-but-empty state must not mask a real failure. + await expect( + run([ + { outcome: "errored", state: "" }, + { outcome: "errored", state: "" }, + ]), + ).rejects.toThrow("linux-x64-build-cpp errored"); + }); + + test("treats a failed state read as non-terminal", async () => { + // `outcome` read in the same poll succeeded, so the agent is up; a flaky + // `state` read must not count as a terminal observation. + await run([ + { outcome: "errored", state: fail("502 Bad Gateway") }, + { outcome: "errored", state: fail("502 Bad Gateway") }, + { outcome: "errored", state: "ready" }, + { outcome: "passed", state: "finished" }, + ]); + }); + + test("retries a transient agent error on outcome", async () => { + await run([ + { outcome: fail("transient 502"), state: "n/a" }, + { outcome: "passed", state: "finished" }, + ]); + }); +});