Conversation
|
Updated 7:34 PM PT - Jul 7th, 2026
❌ @robobun, your commit 0f8b25a has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33117That installs a local version of the PR into your bun-33117 --bun |
|
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 (3)
WalkthroughExtracts Buildkite artifact download handling into a helper, updates runner exit-code mapping for infra failures, adds Buildkite retry handling for artifact-download failures, and covers the flow with new tests. ChangesdownloadArtifactZip flow
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/download-artifact.mjs`:
- Around line 57-60: The artifact selection in the zip sorting logic is now
preferring `bun-profile.zip` over the regular `bun-*.zip`, which changes
`getExecPathFromBuildKite()` behavior. Update the comparator in
`download-artifact.mjs` so non-profile archives are ordered ahead of profile
ones, using the existing `zipPath` selection flow, and make sure the
corresponding test expectation is flipped to match that preferred ordering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eec88900-f8d9-4129-99a9-1fa31daa18ec
📒 Files selected for processing (3)
scripts/download-artifact.mjsscripts/runner.node.mjstest/internal/download-artifact.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean extraction of the artifact-download retry into a testable module, with the timeout-retry and clean-dir-per-attempt changes well-justified and covered by tests.
Extended reasoning...
Overview
This PR extracts the Buildkite artifact download loop from getExecPathFromBuildKite in scripts/runner.node.mjs into a new scripts/download-artifact.mjs module exporting downloadArtifactZip, and adds test/internal/download-artifact.test.ts with 6 unit tests. The functional change is that any download error (timeout included) is now retried up to 10 times with linear backoff, and each attempt starts from a wiped releasePath so a partial zip from a killed download can never be picked up. The buildkite-agent args, zip filter/regex, profile-preference comparator, default attempt count, and 120s timeout are all preserved verbatim from the original inline loop.
Security risks
None. This is CI-only tooling (scripts/) that orchestrates buildkite-agent artifact download on the CI agent. The rmSync(releasePath, { recursive: true, force: true }) operates on join(cwd, "release"), a fixed CI scratch directory — not user/attacker-controlled input. No auth, crypto, or permissions code is touched.
Level of scrutiny
Low–medium. This is internal CI plumbing, not the Bun runtime or any user-facing surface. The blast radius of a regression is "CI test jobs fail to fetch the binary" — visible immediately and recoverable. The change is small (~80 net new lines of logic), follows an existing pattern (test/internal/macos-cross-config.test.ts), and the extracted code is now directly unit-tested where it previously was not.
Other factors
- No CODEOWNERS apply to
scripts/ortest/internal/. - The CodeRabbit comment about flipping the profile/non-profile comparator was a false positive (the comparator is copied verbatim from the base) and was withdrawn after the author added a clarifying comment in
14b4c19c; the thread is resolved. - The minor behavioral deltas beyond the headline fix — first backoff is now 1s instead of 0s, and non-timeout download errors now also retry instead of falling through to scan a possibly-stale directory — are both strict improvements.
- The reported
build-rustCI failures are inscripts/build/ci.ts, which this PR does not touch, and appear unrelated to this change. - Bug-hunting system found no issues.
|
CI is green except for It is unrelated to this change: the test ran to completion on the |
robobun
changed the title
|
Update: after my previous comment, the Since the agent itself is the problem, retrying the download on it can't recover. Commit db0b975 makes the runner exit a distinct infra status (4) when the download ultimately fails and has (The |
There was a problem hiding this comment.
LGTM — well-scoped CI tooling fix with good test coverage; the JSDoc nit below is non-blocking.
Extended reasoning...
Overview
This PR fixes #33116 by extracting the Buildkite artifact-download retry loop from scripts/runner.node.mjs into a new standalone scripts/download-artifact.mjs module so timeouts are retried (not just the "artifact not uploaded yet" case), wiping releasePath between attempts to avoid picking up truncated zips. It adds a dedicated infra exit code (4) that the runner emits when the download exhausts all attempts, and .buildkite/ci.mjs now auto-retries exit_status: 4 on test-bun steps. Six new unit tests in test/internal/download-artifact.test.ts cover retry, stale-zip cleanup, zip-selection ordering, error surfacing, and buildId forwarding.
Security risks
None. This touches only internal CI scripts (pipeline generation and the test-runner harness). No auth, crypto, user data, or runtime/product code is involved.
Level of scrutiny
Low-to-moderate. All four files are CI infrastructure that does not ship to users; the blast radius of a regression is test lanes failing visibly in Buildkite, which is easy to spot and revert. The refactor preserves existing behavior — the zip-selection comparator is copied verbatim from the original (confirmed in the resolved CodeRabbit thread), and the new top-level main().catch() keeps the prior exit 1 for every error except the newly-tagged ARTIFACT_DOWNLOAD_FAILED. The Buildkite auto-retry addition is conservative (limit 2, only on the new dedicated exit code, only on test steps).
Other factors
- No CODEOWNERS paths match the changed files.
- The only bug-hunter finding is a cosmetic JSDoc
@paramunion that wasn't updated to include"infra"— purely documentation, no runtime effect. - CodeRabbit's earlier comment about the profile-zip comparator was withdrawn after the author showed it preserves pre-existing behavior; that thread is resolved.
- CI is green apart from a known unrelated musl flake (#33044).
- The PR is thoroughly described and includes a fail-before/fail-after verification table.
There was a problem hiding this comment.
Thanks for fixing the JSDoc nit. I don't have further concerns, but since this introduces a new exit-code → auto-retry contract between the runner and the Buildkite pipeline definition, it's worth a human sign-off on the CI policy change.
Extended reasoning...
Overview
This PR touches four files, all CI infrastructure: .buildkite/ci.mjs (pipeline generator — adds an opt-in exit_status: 4, limit: 2 auto-retry to getRetry() and enables it for test-bun steps), scripts/runner.node.mjs (replaces ~38 lines of inline download logic with a call to the new module, adds an "infra" outcome → exit 4, and wraps main() in a .catch() that maps ARTIFACT_DOWNLOAD_FAILED to that exit code), a new scripts/download-artifact.mjs module that retries timeouts and cleans the release dir between attempts, and a new test/internal/download-artifact.test.ts with 6 tests. No shipped runtime code is touched.
Since my last review, commit cf272d5d addressed the JSDoc nit I raised — getExitCode()'s @param now includes "infra". The CodeRabbit comment about the profile-zip comparator was withdrawn (the comparator was copied verbatim from the original code; preferring bun-profile is intentional). No bugs were found by the bug-hunting system on this revision.
Security risks
None. This is internal CI tooling: it shells out to buildkite-agent artifact download with arguments derived from the step key/build id (already trusted CI inputs), reads/writes a local release/ directory, and adjusts Buildkite retry config. There is no auth, crypto, user input, or network surface beyond what already existed.
Level of scrutiny
Moderate. The change is well-scoped, well-commented, and covered by unit tests, but it is not a mechanical/config tweak: it (a) refactors the artifact-download path that every test-bun shard depends on, (b) changes the failure semantics from throw-on-first-timeout to retry-then-tag, (c) wraps the runner's top-level main() in a new .catch() that remaps a specific error to a distinct exit code, and (d) introduces a new auto-retry policy in the pipeline definition keyed on that exit code. Each piece is small, but together they form a new contract between two critical CI files that affects every build.
Other factors
The extracted logic preserves the original zip-selection comparator and buildId forwarding, and the new per-attempt rmSync of releasePath is a strict safety improvement over the old code (which could pick up a partial zip). The exit-4 auto-retry is scoped to test steps only and capped at limit: 2, so genuine test failures (exit 2) remain unaffected. The main thing a human should weigh in on is the policy itself — whether auto-rescheduling on persistent download failure (up to 2×, on top of 10 in-process retries that can take ~19 min) is the desired CI behavior, vs. surfacing a broken agent more loudly. That's a reasonable design call either way, which is why I'm deferring rather than approving.
|
Fair deferral, this does add a new exit-code contract, so here is the policy in one place. The issue's ask is that these lanes stop going red and needing a manual Retry. Two layers do that:
Bounds that keep it from masking real failures: exit 4 is emitted only on The real call is the one you flagged, auto-recover vs. fail loudly. I went with auto-recover since that is what the issue asks for. If you would prefer a different balance it is cheap to adjust: fewer in-process retries so a broken agent is rescheduled faster, a lower auto-retry limit, or a Buildkite annotation on download failure so a persistently-broken agent still surfaces even when the job recovers. Happy to add any of those. |
|
Good signal from build 67100: the auto-retry works end to end. The The caveat: all three attempts of that shard landed on the same agent, Net: the runner/pipeline side is now as resilient as it can be for transient slowness and for rescheduling when a healthy agent is free. Escaping an agent that is persistently broken and keeps re-grabbing the job is an infra action, The remaining red lanes are unrelated flakes: |
getExecPathFromBuildKite threw on the first artifact-download timeout, so one slow download failed the whole test-bun job before any test ran. The retry loop only covered the "artifact not uploaded yet" case; a timeout hit the throw directly and propagated out on the first slow attempt. Buildkite does not auto-retry the runner's exit 1, so each occurrence needed a manual "Retry job" click. Move the download into scripts/download-artifact.mjs, which retries any download failure (timeout included) and throws only after exhausting every attempt. Each attempt starts from an empty release directory so a killed, partial download can never be picked up by a later attempt or by the unzip that follows, preserving the safety the original throw was added for. Add a unit test covering the retry policy.
The retry loop alone is not enough when an agent has persistently broken
artifact-store connectivity: build 67035 hit the same darwin-aarch64-26-5-1-1
agent from the issue, timed out on all 10 download attempts over ~19 minutes,
then exited 1. Buildkite only auto-retries exit -1 and 255, so exit 1 left the
lane red until a human clicked Retry, which is the manual-retry pain the issue
describes.
Give the download failure a distinct exit status so Buildkite can reschedule
the whole job, ideally onto an agent with working connectivity:
- downloadArtifactZip tags its terminal error with code ARTIFACT_DOWNLOAD_FAILED.
- The runner maps that to getExitCode("infra") = 4 (distinct from test failures
at exit 2, so genuine failures are never auto-retried).
- getTestBunStep opts into an automatic retry on exit 4 (limit 2). The retry is
scoped to test steps, since only the runner emits exit 4.
cf272d5 to
0f8b25a
Compare
|
Closing: the failure this was opened for was resolved at the root. #33116 (darwin-26 aarch64 test-bun failing on most builds with the 120s artifact download timeout) was closed by #33728 (merged 2026-07-08), which stopped the wedged sendfile processes on that runner from exhausting its network buffers. Sampling failed builds since then, the darwin-26 lane has not hit the download timeout again (the last pre-fix sample, builds 70083 to 70305 on 2026-07-07/08, had it in 37 of 40 failed builds). This branch has also fallen behind: both scripts/runner.node.mjs and .buildkite/ci.mjs conflict with main, and the exit-status-4 automatic retry it adds goes against #34684, which since narrowed automatic retries to agent loss. The PR's own CI run (build 67100) showed that layer re-landing on the same broken agent, so it would not have helped the motivating case either way. For the record, a single-attempt 120s download timeout still shows up occasionally on other lanes, at roughly one build in 150 to 200 (for example build 92906, debian 13 x64-asan, and build 89288, alpine 3.23 aarch64; in both the other shards of the same step passed). If that rate is worth addressing, the in-process retry from this PR would be a small standalone change against the current runner; it does not need the exit code or pipeline retry parts. |
1 participant
Summary
Fixes #33116. The
test-bunjobs fail before running a single test when the build-artifact download is slow or the agent has broken artifact-store connectivity. This makes the runner recover from both: it retries the download, and if that still fails it exits a status Buildkite auto-retries so the whole job reschedules, ideally onto a healthy agent.Root cause
getExecPathFromBuildKitewrapped the download in a 10-iteration retry loop, but the loop only retried the "artifact not uploaded yet" case (download succeeds, nobun*.zip). A download timeout hit athrowdirectly and propagated out on the very first slow attempt:Separately, the runner exits
1on that failure, and Buildkite only auto-retries exit-1/255, so the lane stayed red until a human clicked Retry. That is the manual-retry pain the issue describes.What CI showed
The retry alone is not enough against a persistently-broken agent. On build 67035 the job landed on
darwin-aarch64-26-5-1-1(the exact agent from the issue), timed out on all 10 download attempts over ~19 minutes, then exited 1 with the new terminal error. Retrying on the same broken agent can't recover; the job has to reschedule.Fix
Two layers:
scripts/download-artifact.mjs(downloadArtifactZip), which retries any download failure (timeout included) and throws only after exhausting every attempt. Each attempt starts from an empty release dir so a killed, partial download can't be reused. Handles transient slowness on an otherwise-healthy agent.ARTIFACT_DOWNLOAD_FAILED; the runner maps it togetExitCode("infra")=4(distinct from test failures at exit2), andgetTestBunStepopts into a Buildkite automatic retry on exit4(limit 2). Genuine test failures (exit 2) are never auto-retried. The retry is scoped to test steps, since only the runner emits exit 4.Verification
test/internal/download-artifact.test.ts(6 tests, following thetest/internal/macos-cross-config.test.tspattern of importing ascripts/module and stubbing spawn) covers timeout-recovery, not-uploaded retry, zip selection, exhaustion + theARTIFACT_DOWNLOAD_FAILEDtag, partial-zip cleanup, and buildId forwarding.Fail-before: this is a
scripts/-only change, so thesrc/-stash mechanism can't prove it (the fix survives the stash). I verified it by swapping the module back to the old throw-on-first-timeout behavior and re-running: the retry/tag tests fail, the rest pass. The exit-code mapping and the pipeline retry are integration wiring inrunner.node.mjs/.buildkite/ci.mjs(the runner runsmain()on import, so it isn't unit-testable); the next CI run exercises them end to end.