ci(build): keep waiting for build-cpp while a retry is queued instead of bailing on a stale errored outcome - #36339
ci(build): keep waiting for build-cpp while a retry is queued instead of bailing on a stale errored outcome#36339robobun wants to merge 6 commits into
Conversation
… of bailing on a stale errored outcome
WalkthroughChangesThe Buildkite step waiter now reads both Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 8:18 AM PT - Jul 29th, 2026
✅ @robobun, your commit 3897ad16836c6d7a4cc3c622df00b4042388ae75 passed in 🧪 To try this PR locally: bunx bun-pr 36339That installs a local version of the PR into your bun-36339 --bun |
There was a problem hiding this comment.
LGTM — both earlier nits are addressed (get() now throws immediately on spawn failure, and the fake-agent stub uses $BUN_EXE instead of system node).
What was reviewed:
- The
state+ two-consecutive-terminal-reads gate inwaitForStepOutcome;terminalReadsis reset on any live/non-failed poll and thepassedfast path is unchanged. - Fallback when
step get stateis unavailable — emptystateValisn't inliveState, so behaviour matches the pre-change outcome-only logic. - Test hermeticity:
withFakeAgentrestoresPATH/AGENT_DIR/BUN_EXEinfinally, usestempDir, and the stub advances its cursor on thestateread so both reads in a poll see the same script entry.
Extended reasoning...
Overview
Changes waitForStepOutcome() in scripts/build/ci.ts — the poll that lets rust-and-link wait for its sibling build-cpp step — to also consult buildkite-agent step get state, so a stale errored outcome from an earlier expired attempt doesn't cause an immediate bail while a retry is queued/running. Requires two consecutive terminal reads before throwing to cover the ~1 s gap between an attempt finishing and its retry job appearing. Adds test/internal/ci-sibling-wait.test.ts with a fake buildkite-agent on PATH covering pass, retry-in-flight, transition gap, hard-fail, and state-unavailable.
Security risks
None. This is Buildkite build orchestration tooling in scripts/build/; it does not touch runtime code, user input, auth, or anything that ships in the binary.
Level of scrutiny
Low-to-medium. The blast radius is CI build steps: worst case is build-bun waits longer than needed or bails when it shouldn't, both of which are visible and recoverable at the CI level. The genuine-failure fast path is preserved (~6 s: two polls at the default 3 s interval). The liveState set is a defined allowlist, so unknown Buildkite state values fall through to the original outcome-only behaviour rather than blocking forever.
Other factors
Two prior review rounds each raised one nit and both were fixed in follow-up commits: (1) the spawn-error fast-fail in get() was restored to match run()/runAsync(), and (2) the test stub now execs $BUN_EXE -e (passed via env from bunExe()) instead of relying on system node. The test file follows the existing test/internal/ pattern of importing build-script code directly, mutates process env only inside a try/finally, and is skipIf(isWindows) since the stub is a bash script (the polled step never runs on Windows-hosted agents anyway). No outstanding reviewer comments remain.
…gate; inject the step-get seam so the test is pure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/build/ci.ts`:
- Around line 610-614: Update the BuildError message in agentStepGet when
spawnSync returns r.error to include both the stepKey and attr being requested,
while preserving the existing error cause.
- Around line 665-686: Handle a failed state read in the sibling-step polling
flow by treating state.ok === false as non-terminal: reset terminalReads and
continue polling rather than converting it to an empty state and entering
outcome-only termination logic. Update the logic around get(stepKey, "state"),
stateVal, and stateIsTerminal while preserving the existing behavior for
successful reads with genuinely unavailable or terminal states.
🪄 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: 24ee2d76-bd51-41dd-8516-0358c14176d1
📒 Files selected for processing (2)
scripts/build/ci.tstest/internal/ci-sibling-wait.test.ts
There was a problem hiding this comment.
LGTM — CI build-script fix with a clean injected test seam; all prior review threads addressed.
What was reviewed:
- The retry-aware poll logic: unknown/non-terminal
statevalues keep waiting, two-consecutive-terminal-reads gate covers the retry-appearance gap, and the 60 min deadline still bounds it. - Failed
stateread is now non-terminal and surfaced in the log; spawn-error fast-fail restored with stepKey/attr in the message. - Test seam is a pure injected
get(no subprocess/bash stub anymore),pollMs: 0, and the index-advance logic in therun()helper matches the outcome-then-state read order.
Extended reasoning...
Overview
This PR modifies waitForStepOutcome() in scripts/build/ci.ts — the Buildkite poll loop that rust-and-link build-bun steps use to wait for the parallel build-cpp step before downloading its artifacts. The old loop treated step get outcome == errored as terminal; under queue backlog, an expired-then-retried build-cpp job reports errored while its retry is still queued, cascading ~40 build-bun failures. The fix reads step get state alongside outcome and only gives up when the step-level state is terminal (finished/canceled/ignored) on two consecutive polls. A new test/internal/ci-sibling-wait.test.ts drives the function through an injected get seam covering pass, retry-queued, failing, unknown state, the finished→ready transition gap, terminal hard-fail, empty state (outcome-only fallback), failed state read, and transient outcome-read error.
Security risks
None. This is internal CI build tooling that only runs inside Buildkite jobs and only shells out to buildkite-agent step get (an existing call, now factored into a helper). No user-facing surface, no untrusted input.
Level of scrutiny
Low-to-moderate. The change is confined to a build script (not shipped runtime code); the failure mode of a bug here is a CI hang bounded by the pre-existing 60-minute deadline or a spurious build-bun failure — both recoverable and visible. The logic change is strictly more permissive than before (waits longer instead of bailing early), the genuine-failure fast path is preserved (~3 s to throw on two consecutive state=finished reads), and the PR description shows it working in a real build (85043).
Other factors
All four prior review threads are resolved: my note on the lost spawn-error errno (restored in a38ab92, message now includes stepKey/attr per 3897ad1), my note on the test's system-node dependency (moot — fd8b149 replaced the bash stub with a pure injected seam so no subprocess is spawned at all), and coderabbit's two notes on the spawn-error message and failed-state-read handling (both in 3897ad1, with a covering test). The test file lives in test/internal/ alongside existing tests that import from scripts/build/, uses pollMs: 0 so there is no sleep, and the run() helper's index-advance matches the outcome-then-conditionally-state read order in the loop. No outstanding human reviewer comments.
|
CI on 3897ad1 (build 85073): all build-cpp/build-bun steps pass with the new poll (logs show |
1 participant
What broke
Build-bun steps across ~40 builds (84714 onward, including main #84716) failed with
even though the sibling build-cpp step later passed on retry. First seen in build 84838 (reported from #31737).
Cause
rust-and-link(introduced in #34782) runs build-bun in parallel with build-cpp and pollsbuildkite-agent step get outcome --step <target>-build-cppbefore downloading the C++ archive. Under last night's load spike the c8g.4xlarge queue backed up past Buildkite's 60-minute scheduled-job expiry, so many build-cpp jobs expired without ever being assigned an agent and were then retried.step get outcomereports the result of the last completed job for a step. With a prior attempt expired and a retry still queued or running, it returnserrored, andwaitForStepOutcome()treated that as terminal:So retrying a build-cpp job did nothing for a concurrently running build-bun, and a retried build-bun would bail immediately on its first poll.
Fix
Poll
statealongsideoutcome.step get statereturns the step-level state (ready/running/failing/finished/..., a different vocabulary from the REST API's job states; see the agent CLI docs), so a failedoutcomeis only acted on oncestateis in the terminal set{finished, canceled, ignored}. Any other state, including values we haven't enumerated, keeps the poll going; an empty/unavailablestatefalls back to the original outcome-only behaviour. Two consecutive terminal reads are required before giving up, covering the ~1 s window between an attempt finishing and its retry job appearing.The genuine-failure fast path is preserved: a build-cpp that hard-fails with no retry reports
state=finishedon two consecutive polls and build-bun still exits within ~3 s of that.Verification
Observed in this PR's own CI (build 85043, linux-aarch64-build-bun):
test/internal/ci-sibling-wait.test.tsdriveswaitForStepOutcome()through an injectedstep getseam (no subprocess, runs on all hosts) and covers: pass, errored-while-retry-queued (state=ready),state=failing, an unrecognised state, the finished->ready transition gap, terminal hard-fail,stateunavailable, and a transient agent error.The underlying c8g.4xlarge backlog is a capacity issue (525 build-cpp jobs expired over ~3 h at a median 92 min queue wait); that is not addressed here, but with this change the retry path works through it instead of cascading into build-bun failures.
[stamp-90s] gate passed · iteration 0 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file