Conversation
…ep the runner alive when a host goes down The com.buildkite.cleanup daemon of the bare macOS agents removed builds/* and rebooted at 06:27 local time, also under a running job. The script now stops the agent service first, waits for the agent to exit (it finishes its job and exits 0), and only then wipes. After two hours it reboots without the wipe. `agent.ts start` passes SIGTERM on to buildkite-agent, which is what makes the stop a graceful one. The test runner looks the user up once instead of once per test file, and a node test file that is gone since the tests were listed fails its own step. Both were throws that ended a shard with exit 1 once its host had begun to shut down, which Buildkite records as a test failure and does not retry. The shard now ends as a lost agent, which is retried.
|
Updated 11:57 PM PT - Sep 19th, 2026
❌ @robobun, your commit 01a20d4 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 43611That installs a local version of the PR into your bun-43611 --bun |
|
Status: waits for a maintainer decision in #43630. The code is ready for review at 143a6f3. This PR is now the daemon change only ( Reproduction: the job log of build 118828 (host biscuit) shows CI: this PR changes The agent change takes effect on a bare mini only after |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. WalkthroughThe agent now forwards configured termination signals to child commands. macOS cleanup waits for agent exit before removing files and rebooting. Service reinstall waits for removed services. Integration tests cover signal forwarding and exit-code failures. ChangesAgent runtime behavior
Suggested reviewers: Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
scripts/agent.ts— An operator re-runninginstallon a Mac whose agent service is running now gets a failed install and a box with no agent, where the base branch replaced the service in place.launchctl bootout(scripts/agent.ts:972) sends SIGTERM to thestartprocess, which since scripts/agent.ts:1129 forwards it and stays alive until buildkite-agent has disconnected; bootout returns before that, and the immediatelaunchctl bootstrapat scripts/agent.ts:973 fails with EIO, sorun()throws and the cleanup plist is never (re)loaded. Fix: after bootout, wait untillaunchctl print system/buildkite-agentfails before bootstrapping (as scripts/darwin-ci/lib/agent.ts:113-116 does), for both plists in the loop. [also at: scripts/agent.ts:1129 - Operators re-runninginstallon a mini that already runs this script can now get a failed deploy that leaves the agent service unloaded.]Extended reasoning...
scripts/darwin-ci/lib/agent.ts:112 records the launchd behaviour this depends on: "bootout returns before a busy agent has finished its cancel grace period, and bootstrap fails with EIO until it has", and its unload() polls
launchctl printfor that reason. On the base branch the service's main process (node runningstart) had no SIGTERM listener, so bootout's SIGTERM killed node at once, launchd killed the process group, and the service was gone before bootstrap ran. With this change run() installsprocess.on("SIGTERM", () => child.kill("SIGTERM"))(scripts/agent.ts:75-79) and keeps waiting forclose. buildkite-agent answers the forwarded SIGTERM by stopping gracefully: it logs "Gracefully stopping agent", calls the Buildkite API to disconnect, then exits; even idle that takes a network round trip, and with a job it takes until the job ends or launchd's 20 s ExitTimeOut SIGKILL. install() runsrun(["launchctl", "bootstrap", "system", p])(scripts/agent.ts:973) milliseconds after…Verification: normal — triggered whenever an operator re-runs
sudo node scripts/agent.ts installon a bare mini whose service is already running this PR'sstart(the PR's own documented redeploy step, repeated for any future update; alsoinstallBareAgent()in scripts/darwin-ci/lib/agent.ts:105). Recovery is a secondinstallrun (bootout then fails harmlessly and bootstrap succeeds), but the first run…
…however the agent ended, start the agent again if the reboot does not come `launchctl bootout` returns before a service that stops gracefully is gone, and `bootstrap` fails with EIO until it is, so `install` polls `launchctl print` between the two, as scripts/darwin-ci does. After a forwarded signal run() no longer throws on the command's exit status. `start` then exits 0, so launchd (KeepAlive, SuccessfulExit=false) does not start a new agent under the cleanup that waits for the old one. The cleanup script starts the agent again if it is still running ten minutes after it asked for the reboot.
|
On the finding about |
There was a problem hiding this comment.
I re-reviewed after 01a20d4 and found no bugs; the two points from my earlier inline comments are addressed in the code (run() returns normally after a forwarded signal so the service exits 0 whatever the agent's status, and the cleanup script kickstarts the agent if the host is still up 600 s after asking for the reboot). Because this rewrites a fleet LaunchDaemon, changes how systemctl stop/launchctl stops reach buildkite-agent, and needs a manual install on each bare mini, a human infra owner should still sign off on the rollout.
What was reviewed:
run()signal forwarding: listeners are removed in.finally,askedonly short-circuits the throw when a forwarded signal was actually delivered, and the non-forwarded exit-7 path still throws.- Double-signal risk on Linux: the systemd unit uses
KillMode=process, so only node gets SIGTERM and the forward does not make the agent see a second (forceful) SIGTERM. getUser(): the only remaininguserInfo()caller inscripts/is the memoized wrapper itself;runner.node.tsandbuildkite.tsboth use it.installbootout/bootstrap loop: bounded to 60 x 1 s, well above launchd's default 20 s ExitTimeOut, and the ENOENT-only rethrow inrunner.node.tskeeps other read errors loud.
Extended reasoning...
Overview
The PR touches only CI tooling: scripts/agent.ts (a forward option on run(), a memoized getUser(), a rewritten macOS nightly-cleanup LaunchDaemon script, and a bootout-wait-bootstrap loop in install), the two callers in scripts/buildkite.ts and scripts/runner.node.ts that switch to getUser(), an ENOENT-tolerant readFileSync in the runner's node-test loop, and a new test/internal/ci-agent.test.ts. No runtime, native, or user-facing code changes. The second commit (01a20d4) is a direct response to the two inline findings from the prior run: run() no longer throws on a non-zero/signal exit once a forwarded signal was delivered, and the cleanup script restarts the agent after a 600 s wait if the reboot never came.
Security risks
None that reach users. The cleanup script runs as root and does rm -rf "${homePath}/builds" /tmp/* /var/tmp/*; homePath is derived from the install-time paths in agent.ts, not from external input, and the script now removes builds itself rather than builds/*, so a symlink there is not followed. The generated shell is embedded in a plist CDATA block with fixed content. The getUser() cache is a process-lifetime memo of os.userInfo(), which cannot change meaningfully for a running service.
Level of scrutiny
Medium. The code itself is small and I could trace every branch of run() and the cleanup script by reading. What warrants a human is the operational surface: the LaunchDaemon on every bare Mac mini is replaced, systemctl stop on the Linux agents now reaches buildkite-agent (previously only node was signalled, and KillMode=process meant the agent kept running), and the PR itself states that the agent half only takes effect after someone runs sudo node scripts/agent.ts install on each idle mini. Those are fleet decisions, not code-correctness questions, and CODEOWNERS does not cover scripts/, so nobody is auto-assigned.
Other factors
The bug hunt ended on a dry streak and the single ruled-out candidate (the bare catch {} around the cleanup process.kill in the test) is a nit on a best-effort teardown of an already-dead PID. I could not execute the new test in this environment (shell execution was denied), so the claim that the three forwarding variants pass rests on the PR description and on reading the fixture logic; the fixture matrix (exit 0, exit 1, death by signal, and the non-forwarded exit-7 throw) does cover each load-bearing clause of the run() change. The test file is new rather than appended to an existing one and uses *.fixture.mjs names inside a tempDir, which does not affect the test runner's discovery; the USE_SYSTEM_BUN=1 rule is not meaningful here because the code under test is plain TypeScript in scripts/, not a native change.
…in-nightly-cleanup-under-job
The cached user lookup and the node test read are in #43629, with getUser() in scripts/buildkite.ts. This branch now changes scripts/agent.ts only: the cleanup daemon, the SIGTERM forwarding in `start`, and the wait between bootout and bootstrap in `install`.
This branch has not been deployed
1 participant
Needs a maintainer decision first: #43630 (this is option 1, drain). #43629 stops the red builds without a rollout.
Problem
com.buildkite.cleanupLaunchDaemon of the bare macOS agents (scripts/agent.ts, scripts/agent.mjs: macOS launchd install + start support #29672) runsrm -rf builds/* /tmp/*andshutdown -r nowat 06:27 host-local time, with no check for a running job. Build 118828:Test filter ".../import-custom-condition.test.ts" had no matches, thenspawn .../bun-profile ENOENT.Fix
launchctl kill SIGTERM), waits until nobuildkite-agentprocess is left, then wipes and reboots. After two hours it reboots without the wipe. If the host is still up 600 s later, it starts the agent again.agent.ts startforwards SIGTERM to buildkite-agent, then exits 0 however the agent ended, so launchd does not restart the service under the cleanup.installwaits for the old service to be gone betweenlaunchctl bootoutandbootstrap.test/internal/ci-agent.test.tsand the generated script against stubs (Notes). The launchd path was never run on a Mac. It needs a trial on one mini.Background
node agent.ts start, the launchd service. launchd signals node, not the agent.scripts/agent.tsis part of the name of every CI image. This PR bakes all of them again.Notes
What a merge changes, and where.
installon them (see Deploy).startwith their next image bake, which this PR triggers. The systemd unit hasKillMode=process, sosystemctl stopsent SIGTERM to node only and left the agent running. With the forwarding it reaches buildkite-agent, which stops gracefully. The cloud agents are ephemeral (one job, then the machine is deleted), so nothing there stops the service in normal operation. On Windows a SIGTERM listener never fires.scripts/darwin-ci/lib/agent.ts:74-81) boots the agent out before it deletes anything.On the hosts (read-only).
last rebooton crouton shows06:27for every day./Library/LaunchDaemons/com.buildkite.cleanup.plistis byte-identical (same sha1) on crouton, breadstick, hardtack, biscuit, matzo, cornbread, bagel and pretzel, and it is the template fromscripts/agent.ts. The agent log of crouton for build 118830 (local time) shows that the agent already waits for its job on a first SIGTERM:The service on these hosts is
node agent.mjs start(pid 341 on crouton) withbuildkite-agent startas its child (pid 400).pgrep -x buildkite-agentfinds the child.launchctl print system/buildkite-agentreportsexit timeout = 5.The wipe set. The old script also removed
$BASE_PREFIX/{var,etc}/buildkite-agent/{builds,cache}/*and<home>/cache/*, then ranchownandchmodon the Homebrew directories. None of those paths exists on the eight minis above: every bare mini uses the~/Library/Services/buildkite-agentlayout thatinstallcreates, and the agent cache is~/Library/Caches/buildkite-agent, which the old script did not touch either. The new script removesbuilds,/tmp/*and/var/tmp/*. It removesbuildsitself and notbuilds/*, so that a symlink atbuildsis not followed. The agent creates the directory again for its next job.The two hour bound. Darwin test jobs have
timeout_in_minutes: 45, so a drain takes less than that. The bound is for an agent that does not exit. In that case the checkout stays, and the reboot ends the job as a lost agent. A reboot also ends the script. If the script is still alive 600 s after it asked for the reboot, it starts the agent again (launchctl kickstart), so a host never stays in the pool without an agent. The script writes to~/Library/Logs/buildkite-agent/cleanup.log, one line on a normal night.A stop always exits 0. launchd restarts this service when it exits with anything but 0 (
KeepAlive,SuccessfulExit=false). A restarted agent would take jobs while the cleanup waits for the old one. So after a forwarded signalrun()returns whatever the exit status of the agent is, andstartexits 0. Without a signal, a failed agent is still an error and launchd restarts it.installon a host that already runs this.launchctl bootoutreturns before a service that stops gracefully is gone, andbootstrapfails with EIO until it is.installpollslaunchctl print system/<label>between the two (at most 60 s), asscripts/darwin-ci/lib/agent.tsdoes for the Tart agent.Checks of the cleanup script. The script that
installgenerates was extracted fromagent.tsand run under/bin/shwith aPATHthat holds only stubs that log their arguments. Six cases: an idle agent, a job that ends after three polls, an agent that never exits (240 polls, norm), no agent at all, a failedshutdown(it runsreboot), and a failedshutdownandreboot. Nothing ends the script in this harness, so every case also reachessleep 600and thekickstart.sh -n,dash -nandbash --posix -naccept it. The generated plist parses, and its CDATA section gives back the script byte for byte. This harness is not in the PR: a similar test file was removed from #40349 by a maintainer.The test.
run()is checked with a service process that gets SIGTERM, for a command that then exits 0, exits 1, or dies of the signal: the service exits 0 each time. Withrun()from main the service dies of the signal (exitCode 143) and the command never sees it. A fourth case checks that a command that fails with no signal is still an error.Deploy. On each bare mini, from a checkout of main with this change, while the agent is idle (
installboots the service out, which ends a running job):sudo env PATH=$PATH node scripts/agent.ts installIt reuses the token from the existing
buildkite-agent.cfg.darwin-ci provision <host> bareends in the sameinstall(scripts/darwin-ci/lib/agent.ts:102-106), so a provision from a ref without this change writes the old daemon again. This is also the first time the minis run the single-fileagent.mtsfrom #43595: they still run theagent.mjsandutils.mjscopies from 2026-08-11. The pass needs an owner, and a trial on one mini first. I can run both over ssh if a maintainer says yes in #43630.History. This PR first carried the runner change too. A review asked for the split, because the runner part needs no rollout and no decision. Supersedes #40349, which predates the TypeScript conversion in #43595. The self-review raised five points: the split,
getUser()outsideagent.ts, the test location, a recorded maintainer decision with a rollout owner and a trial, and the missing note about the Linux agents. All five are addressed here, in #43629 and in #43630.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/internal/ci-agent.test.ts