Skip to content

ci: fail loudly when artifact download times out #29039

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jarred-Sumner merged 1 commit into from
Apr 8, 2026
Merged

ci: fail loudly when artifact download times out #29039

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions scripts/runner.node.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1972,14 +1972,20 @@
args.push("--build", buildId);
}

await spawnSafe({
const { error } = await spawnSafe({
command: "buildkite-agent",
args,
timeout: 60000,
timeout: 120000,
});
if (error === "timeout") {
throw new Error(
`buildkite-agent artifact download timed out after 120s for step '${target}'. ` +
`Refusing to continue with a partial download (would silently fall back to the wrong binary).`,
);
}

zipPath = readdirSync(releasePath, { recursive: true, encoding: "utf-8" })
.filter(filename => /^bun.*\.zip$/i.test(filename))

Check failure on line 1988 in scripts/runner.node.mjs

View check run for this annotation

Claude / Claude Code Review

Non-timeout spawnSafe failures silently ignored 

The PR only checks `error === "timeout"` but does not check the `ok` field, leaving all other spawnSafe failure modes (non-zero exit codes like `"code 1"`, `"spawn error"`, signals like `"SIGKILL"`) silently ignored. When buildkite-agent artifact download fails for any non-timeout reason, the code falls through to `readdirSync(releasePath)` and — if a stale or partial zip exists from a prior run or an earlier loop iteration — breaks out of the download loop and proceeds to use that wrong binary.
Comment on lines +1975 to 1988

@claude claude Bot Apr 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The PR only checks error === "timeout" but does not check the ok field, leaving all other spawnSafe failure modes (non-zero exit codes like "code 1", "spawn error", signals like "SIGKILL") silently ignored. When buildkite-agent artifact download fails for any non-timeout reason, the code falls through to readdirSync(releasePath) and — if a stale or partial zip exists from a prior run or an earlier loop iteration — breaks out of the download loop and proceeds to use that wrong binary. This is the exact silent fallback the PR description says it wants to prevent; the fix only closes it for the timeout case. To fix, also capture ok and throw when !ok: const { ok, error } = await spawnSafe({...}); if (!ok) { throw new Error(...); }

Extended reasoning...

What the bug is and how it manifests

The PR introduces a check for error === "timeout" after calling spawnSafe for buildkite-agent artifact download, but it never checks the ok field that spawnSafe returns. The ok field is exitCode === 0 && !signalCode && !spawnError — so it is false for any non-zero exit code, spawn errors, and signals, not just timeouts. When buildkite-agent exits non-zero for reasons other than the 120 s timeout (authentication failure, a transient network reset that causes exit code 1, an internal buildkite-agent timeout returning a signal, etc.), the code silently swallows the error and drops straight into readdirSync(releasePath).

The specific code path that triggers it

At lines 1975-1988, the destructuring const { error } = await spawnSafe(...) discards the ok field. Only the error === "timeout" case is handled. All other failure values for error ("code 1", "spawn error", "SIGKILL", etc.) cause the code to fall through directly to readdirSync(releasePath). If any zip is found there, zipPath is set, break downloadLoop fires, and the stale artifact is used.

Why existing code does not prevent it

The final safety check (throw new Error("Could not find ${target}.zip")) only fires when zipPath is undefined after all 10 iterations. If any zip file is present in releasePath — from a previous CI build (Buildkite agents commonly reuse workspaces), from a partial write by a prior loop iteration, or from a prior run of the same job — the readdirSync call finds it and the loop breaks immediately with that stale artifact. The sort logic preferring *-profile.zip mirrors the exact scenario from the PR description (profile zip vs release zip).

Step-by-step proof of the scenario

  1. Buildkite agent with a warm workspace runs iteration i=0 of the download loop. The release/ directory already contains bun-windows-x64.zip from the previous build.
  2. buildkite-agent artifact download starts downloading but exits with code 1 (e.g., network reset mid-transfer, or an auth error).
  3. spawnSafe returns { ok: false, error: "code 1", ... }.
  4. The if (error === "timeout") check is false — no throw.
  5. readdirSync(releasePath) finds bun-windows-x64.zip (the stale one from the previous build).
  6. zipPath is set, break downloadLoop fires.
  7. The stale zip is unzipped and bun.exe from the previous build is used for the entire test run — exactly the "silently fell back to the wrong binary" scenario the PR description explicitly called out.

What the impact would be

CI jobs would silently use the wrong binary (wrong commit, wrong profile/release variant) and potentially report misleading test results — the same class of failure the PR was written to fix. The error is completely invisible in the logs since spawnSafe does not throw.

How to fix it

Capture and check ok alongside error:

const { ok, error } = await spawnSafe({
  command: "buildkite-agent",
  args,
  timeout: 120000,
});
if (!ok) {
  throw new Error(
    `buildkite-agent artifact download failed for step '${target}': ${error}. ` +
      `Refusing to continue (would silently fall back to the wrong binary).`
  );
}

This closes the gap for all failure modes, not just timeouts. If retry behavior is desired for transient non-timeout failures, the loop should at minimum continue rather than fall through to readdirSync so that a stale zip is never used.

.map(filename => join(releasePath, filename))
.sort((a, b) => b.includes("profile") - a.includes("profile"))
.at(0);
Expand Down
Loading