ci(build): keep waiting for build-cpp while a retry is queued instead of bailing on a stale errored outcome

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,96 @@ function runAsync(argv: string[], cwd: string): Promise<void> {
   });
 }
 
+/** One `buildkite-agent step get <attr>` 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`, { cause: r.error });
+  return { ok: r.status === 0, out: (r.stdout ?? "").trim(), err: (r.stderr ?? "").trim() };
+}
+
 /**
- * Poll `buildkite-agent step get outcome --step <key>` 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 `<stepKey>` 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<void> {
-  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<void> {
+  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<void>(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 : "";
+    const display = `${outcome.out || "(none)"}${stateVal ? ` / state=${stateVal}` : ""}`;
+    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;
+    const stateIsTerminal = stateVal === "" || terminalState.has(stateVal);
+    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);
   }
 }
 

test/internal/ci-sibling-wait.test.ts

@@ -0,0 +1,108 @@
+/**
+ * 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";
+
+/** Drive `waitForStepOutcome` through a canned sequence of `outcome`/`state` reads. */
+function run(script: ReadonlyArray<{ outcome: string; state: string } | { ok: false; err: string }>) {
+  let i = 0;
+  const get = (_stepKey: string, attr: string): StepGetResult => {
+    const entry = script[Math.min(i, script.length - 1)]!;
+    // One poll = outcome then state; advance on state so both reads see the same entry.
+    if (attr === "state") i++;
+    if ("ok" in entry) {
+      // A transient agent failure: the poll retries without reading `state`.
+      if (attr === "outcome") i++;
+      return { ok: false, out: "", err: entry.err };
+    }
+    return { ok: true, out: entry[attr as "outcome" | "state"], 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 () => {
+    // An empty/unavailable state must not mask a real failure.
+    await expect(
+      run([
+        { outcome: "errored", state: "" },
+        { outcome: "errored", state: "" },
+      ]),
+    ).rejects.toThrow("linux-x64-build-cpp errored");
+  });
+
+  test("retries a transient agent error", async () => {
+    await run([
+      { ok: false, err: "transient 502" },
+      { outcome: "passed", state: "finished" },
+    ]);
+  });
+});