Repository navigation
Conversation
On Alpine (openrc) test agents the buildkite-agent service did not declare any ordering against docker, so on a cold boot the agent accepts a job before dockerd is listening. Every Alpine shard's `--- Docker` section shows "failed to connect to /var/run/docker.sock" at runner start; the daemon normally appears a few seconds later and the coordinator's one-shot `docker version` happens to win the race. When it loses (or dockerd fails to come up at all, as on one VM in build 74556), every container-backed test in the shard fails with "Docker is not available" / "A functional docker is required in CI" on all retries. - runner.node.mjs: if docker CLI is present but the daemon is unreachable, poll `docker version` for up to 30s before spawning the coordinator. Costs nothing on hosts where the daemon is already up; gives openrc a bounded window to finish bringing dockerd online. - agent.mjs: order buildkite-agent after docker in both the openrc and systemd unit definitions so future images boot with the correct ordering and the wait loop is not doing load-bearing work.
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 7:03 AM PT - Jul 17th, 2026
❌ @robobun, your commit 0bcc32f has 7 failures in
🧪 To try this PR locally: bunx bun-pr 34480That installs a local version of the PR into your bun-34480 --bun |
|
Status: CI-infrastructure fix for an Alpine-only boot race between the buildkite-agent service and dockerd. No Self-review surfaced one concern which is now addressed in 0bcc32f: when the 30s poll expires with the daemon still unreachable, exit CI on 0bcc32f (build 74580): all 60 Alpine |
|
Arrived at the same fix from a separate report of One data point from that shard's log worth folding in: on |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — CI-infrastructure-only fix for the Alpine cold-boot race between buildkite-agent and dockerd.
What was reviewed:
- The 30s poll loop sits inside the existing
isCI && isLinux && docker compose versionguard, so non-Linux/dockerless hosts and hosts with a healthy daemon are unaffected (one extraspawnSyncon the fast path);setTimeoutPromiseis already imported. - openrc
use ... docker/after dockerand systemdAfter=docker.serviceare ordering-only (notneed/Requires=), so images without docker still boot the agent. - Checked that
docker compose version(the outer guard) does not need the daemon whiledocker versiondoes, so the new inner check is the right probe.
Extended reasoning...
Overview
Two CI-script changes to fix a boot-ordering race observed on Alpine ephemeral agents where the buildkite-agent service accepted a job before dockerd was listening:
scripts/agent.mjs: addsuse ... docker+after dockerto the openrc init script'sdepend()andAfter=docker.serviceto the systemd unit, so newly-baked images order the agent after docker.scripts/runner.node.mjs: inside the existingisCI && isLinux && spawnSync('docker', ['compose', 'version'])block, pollsdocker versionfor up to 30s before spawning the docker-service coordinator, falling through (not blocking) if the daemon never comes up.
No src/, test/, or user-facing changes.
Security risks
None. This is internal CI orchestration; no new inputs are parsed, no secrets touched, no network endpoints added. The added spawnSync calls invoke docker version with fixed argv and stdio: 'ignore'.
Level of scrutiny
Low. This is non-production CI plumbing with a clear, bounded blast radius: the runner poll only executes on CI Linux hosts that already pass the docker compose version gate, adds at most 30s of wait when the daemon is genuinely down (after which behaviour is identical to today's), and is a no-op on healthy hosts. The service-unit changes use soft ordering directives (use/after in openrc, After= without Requires= in systemd), so agents on images without docker are unaffected.
Other factors
setTimeoutPromiseis already imported fromnode:timers/promisesin this file and used elsewhere, so no new import needed.- The outer guard's
docker compose versionis a client-side plugin check that succeeds without a running daemon, whereasdocker versioncontacts the daemon — so the new inner probe correctly distinguishes "CLI present but daemon not ready" from "docker missing entirely". - The 10-line explanatory comment matches the style of the existing coordinator comment block immediately above it in the same file.
- The PR body documents log evidence from build 74556 showing the race window is well within the 30s budget, and correctly notes this cannot be demonstrated fail-before/pass-after locally (the affected tests skip when docker is unreachable outside CI).
- The bug-hunting system found no issues.
When the poll budget expires and the daemon is still unreachable, the shard is on a VM where dockerd failed to start (the motivating build 74556 alpine VM was still failing 3+ minutes in). Running on would exercise 4x retries of every container-backed test against a dead host. The Linux test agents are ephemeral (acquire-job + disconnect-after-job), so exit EX_TEMPFAIL and let Buildkite's existing automatic-retry hook (limit: 1) re-run the shard on a fresh VM. A fleet-wide docker regression still surfaces on the second attempt.
05c82c6 to
0bcc32f
Compare
There was a problem hiding this comment.
LGTM — small, well-scoped CI infra fix.
What was reviewed:
runner.node.mjs: new poll sits inside the existingisCI && isLinux && docker compose versionguard;docker compose versiondoesn't need the daemon so the guard still passes when dockerd is down.getHostname/reportAnnotationToBuildKite/setTimeoutPromiseare already imported, and the annotate call is synchronous soprocess.exit(75)won't cut it off.agent.mjs: openrcuse/after dockerand systemdAfter=docker.serviceare soft ordering — no-ops on hosts without docker.ci.mjs:getRetry()is shared across all step types, so exit 75 anywhere now auto-retries once; bounded bylimit: 1and 75 is conventional EX_TEMPFAIL, so harmless.
Extended reasoning...
Overview
CI-only change across three script files: (1) .buildkite/ci.mjs adds { exit_status: 75, limit: 1 } to the shared getRetry() automatic-retry list; (2) scripts/agent.mjs adds use/after docker to the openrc service and After=docker.service to the systemd unit so the buildkite-agent boots after dockerd on newly baked images; (3) scripts/runner.node.mjs polls docker version for up to 30s before spawning the docker test-service coordinator, and if the daemon never comes up, annotates the build and exits 75 so Buildkite reschedules once on a fresh ephemeral VM.
Security risks
None. No user-controlled input is parsed; the only new subprocess calls are spawnSync("docker", ["version"]) with a fixed argv. Service-unit edits are ordering-only declarations.
Level of scrutiny
Low-to-medium. This is CI plumbing with no src/ or test changes, and every branch is guarded (isCI && isLinux plus a successful docker compose version). The failure mode it addresses (agent accepting a job before dockerd listens on Alpine) is well-evidenced in the PR body, and the fallback (process.exit(75) → one Buildkite retry) is bounded. Worst-case regressions are a 30s wait on a broken host or one spurious retry of a step that happens to exit 75.
Other factors
- Checked that
reportAnnotationToBuildKiteis synchronous (nodeSpawnSync) and early-returns when not on Buildkite, soprocess.exit(75)immediately after is safe and non-Buildkite CI just gets a nonzero exit with a clear message. After=docker.servicewithoutWants=/Requires=and openrcuse dockerare soft deps, so images without docker (e.g. the mac/windows paths, or a future dockerless linux image) are unaffected.getRetry()is used by build/link/image steps too, not just tests; adding exit 75 there is fine since it's capped at one retry and EX_TEMPFAIL is the conventional meaning.- The PR body's "falls through" wording predates the exit-75 addition folded in from the follow-up branch, but the code is consistent with the final intent and the timeline explains the evolution.
- No CODEOWNERS on these paths.
There was a problem hiding this comment.
LGTM — small, self-contained CI infrastructure fix.
What was reviewed:
- The 30s poll sits inside the existing
isCI && isLinux && docker compose versionguard, so non-Linux/dockerless hosts are untouched;docker compose versionis client-only so the outer guard passing while the daemon is down is the exact case being handled. reportAnnotationToBuildKiteis synchronous (spawnSync-based), so the annotation posts beforeprocess.exit(75);getHostname/setTimeoutPromiseare already imported.- openrc
use/afterand systemdAfter=are ordering-only (noneed/Requires=), so agents on images without docker still start. getRetry()is applied togetTestBunStep, so exit 75 triggers the newlimit: 1auto-retry on a fresh ephemeral VM.
Extended reasoning...
Overview
CI-infrastructure only: three script edits with no src/ or test changes. .buildkite/ci.mjs adds { exit_status: 75, limit: 1 } to the existing automatic-retry list. scripts/agent.mjs adds ordering-only dependencies on docker to the openrc and systemd unit definitions. scripts/runner.node.mjs adds a bounded 30s poll for the docker daemon before spawning the test-service coordinator, and if the daemon never comes up, annotates and exits EX_TEMPFAIL (75) so Buildkite reschedules the shard on a fresh ephemeral VM.
Security risks
None. No user-controllable input is parsed; the only new external calls are spawnSync("docker", ["version"]) with fixed argv and a Buildkite annotation via the existing helper. The service-unit additions are declarative ordering hints.
Level of scrutiny
Low-to-medium. This is build/CI plumbing, not runtime code, and the changes are additive inside existing guards. The failure mode of a mistake here is a mis-scheduled CI job, not a shipped bug. node --check passes on all three files and I traced the imports (getHostname, reportAnnotationToBuildKite, setTimeoutPromise) to confirm they're already present.
Other factors
- Confirmed
reportAnnotationToBuildKiteis synchronous, so it completes beforeprocess.exit(75); no lost annotation. - Confirmed
getRetry()is used bygetTestBunStep, so the newexit_status: 75entry actually applies to the code path that emits it. Other steps sharinggetRetry()are unlikely to exit 75 accidentally, andlimit: 1caps any surprise. - openrc
use docker/after dockerand systemdAfter=docker.serviceare soft ordering — agents on images without docker still boot, matching the PR's stated intent. - The poll loop is placed before the coordinator spawn and before any test runs, so
process.exit(75)at that point leaves nothing to clean up. - No CODEOWNERS cover these paths.
|
Superseded by #39243. Logs from builds 97891, 98647, 98707 and 98746 show the daemon on these alpine agents does come up, but only 5 to 7 minutes into the job, so a 30s poll in the runner would not have covered any of them, and |
What
test/js/sql/sql-mysql-query-string-leak.test.tsandtest/regression/issue/26030.test.tswent red on:alpine: 3.23 x64shard 0 in build 74556 (PR #34477, unrelated to its diff):All four in-shard retries hit the same state because the docker daemon on that VM never came up for the life of the job.
Why
scripts/agent.mjsinstalls the buildkite-agent service with no ordering against docker:so on a cold ephemeral boot the agent can accept a job before
dockerdis listening. Grepping the shard logs for that build: on every Alpine shard the runner's--- Dockersection (docker ps) prints "failed to connect to /var/run/docker.sock", and the coordinator's one-shotdocker versiona few seconds later normally wins the race (shards 1-19 and the x64-baseline twin of shard 0 all went on tocoordinator: mysql_plain ready). On this one VM it lost, anddocker versionwas still failing 3+ minutes later when the test file ran, so the daemon never came up at all on that boot.On Debian/Ubuntu the same
--- Dockersection already shows a healthyCONTAINER ID ...header at runner start, so this is specific to the openrc boot ordering.Fix
scripts/runner.node.mjs: on CI Linux, if the docker CLI is present butdocker versioncan't reach the daemon, poll it for up to 30s before spawning the test-service coordinator. This closes the cold-boot race for the current images at zero cost on hosts where the daemon is already up. If the daemon is still unreachable after 30s the shard is on a VM where dockerd failed to start; the runner annotates and exits75(EX_TEMPFAIL) so Buildkite's automatic-retry (limit: 1, Linux test agents are ephemeralacquire-job/disconnect-after-job) re-runs the shard on a fresh VM instead of burning 4x retries of every container-backed test against a dead host. A fleet-wide docker regression still surfaces on the second attempt..buildkite/ci.mjs: addexit_status: 75togetRetry()'s automatic list alongside the existing-1/255infra-retry codes.scripts/agent.mjs: declareafter docker/use ... dockerin the openrc service andAfter=docker.servicein the systemd unit so newly baked images boot with the correct ordering and the wait loop above is not doing load-bearing work.Verification
This is CI machinery with no
src/or test changes; the reported test already passes on main HEAD whenever docker is reachable (299/300 recent builds). The fix cannot be demonstrated as fail-before/pass-after locally: without a reachable docker daemon the test is skipped (isDockerEnabled()returns false outside CI), and with one it already passed.What I did verify:
node --checkpasses on all three modified files.isCI && isLinux && docker compose versionguard, so non-Linux / dockerless hosts are untouched and a healthy daemon takes the fast path (one extraspawnSync docker version).docker psfailing at runner start and docker becoming ready within the 30s budget, so the poll is sized against real numbers rather than guessed.use/afterand systemdAfter=are ordering-only (notneed/Requires=), so images without docker still boot the agent.no test proof · iteration 0 · build/CI scripts only; test-proof not applicable