ci: fail loudly when artifact download times out - #29039
Conversation
spawnSafe's timeout kills buildkite-agent mid-download but returns
{error: 'timeout'} without throwing. The caller then picked whichever
zip(s) had finished, so on a slow VM the 149 MiB profile zip would be
dropped and tests silently ran against the release bun.exe instead of
bun-profile.exe. Bump the timeout to 120s and throw on timeout instead
of proceeding with a partial download.
|
Updated 1:30 PM PT - Apr 8th, 2026
@dylan-conway, your commit f3ad744 is building: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughModified Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 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)) |
There was a problem hiding this comment.
🔴 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
- Buildkite agent with a warm workspace runs iteration i=0 of the download loop. The
release/directory already containsbun-windows-x64.zipfrom the previous build. buildkite-agent artifact downloadstarts downloading but exits with code 1 (e.g., network reset mid-transfer, or an auth error).spawnSafereturns{ ok: false, error: "code 1", ... }.- The
if (error === "timeout")check is false — no throw. readdirSync(releasePath)findsbun-windows-x64.zip(the stale one from the previous build).zipPathis set,break downloadLoopfires.- The stale zip is unzipped and
bun.exefrom 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.
`getExecPathFromBuildKite` runs `buildkite-agent artifact download` via
`spawnSafe` with a 60 s timeout. On timeout `spawnSafe` kills the
process and returns `{ error: "timeout" }` — it doesn't throw — and the
caller doesn't check it. The loop then picks whichever `bun*.zip` made
it to disk and continues.
On a slow win-aarch64 VM (build #44517, 38 MiB in 52 s) the 149 MiB
`*-profile.zip` never finished, so the runner silently fell back to the
release `bun.exe` instead of `bun-profile.exe`. That in turn made
`which.test.ts` fail because `basename(process.execPath)` became
`"bun.exe"`, which `bootstrap.ps1` bakes into `C:\Windows\System32\` on
the v14 image.
Bump the timeout to 120 s and throw if it's hit so the job fails with a
clear message instead of running the wrong binary.
`getExecPathFromBuildKite` runs `buildkite-agent artifact download` via
`spawnSafe` with a 60 s timeout. On timeout `spawnSafe` kills the
process and returns `{ error: "timeout" }` — it doesn't throw — and the
caller doesn't check it. The loop then picks whichever `bun*.zip` made
it to disk and continues.
On a slow win-aarch64 VM (build #44517, 38 MiB in 52 s) the 149 MiB
`*-profile.zip` never finished, so the runner silently fell back to the
release `bun.exe` instead of `bun-profile.exe`. That in turn made
`which.test.ts` fail because `basename(process.execPath)` became
`"bun.exe"`, which `bootstrap.ps1` bakes into `C:\Windows\System32\` on
the v14 image.
Bump the timeout to 120 s and throw if it's hit so the job fails with a
clear message instead of running the wrong binary.
3 participants
getExecPathFromBuildKiterunsbuildkite-agent artifact downloadviaspawnSafewith a 60 s timeout. On timeoutspawnSafekills the process and returns{ error: "timeout" }— it doesn't throw — and the caller doesn't check it. The loop then picks whicheverbun*.zipmade it to disk and continues.On a slow win-aarch64 VM (build #44517, 38 MiB in 52 s) the 149 MiB
*-profile.zipnever finished, so the runner silently fell back to the releasebun.exeinstead ofbun-profile.exe. That in turn madewhich.test.tsfail becausebasename(process.execPath)became"bun.exe", whichbootstrap.ps1bakes intoC:\Windows\System32\on the v14 image.Bump the timeout to 120 s and throw if it's hit so the job fails with a clear message instead of running the wrong binary.