Conversation
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 7 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 6:41 AM PT - Aug 21st, 2026
✅ @robobun, your commit a4d48b74d9e893ca61fff41f6db8bb6c8a63b4b0 passed in 🧪 To try this PR locally: bunx bun-pr 39851That installs a local version of the PR into your bun-39851 --bun |
|
Status: ready for a maintainer. Head is a4d48b7, its CI run is build 102656 (173 of 179 jobs green at the time of writing, none failed). The previous head efd5908 passed build 102179 in full, and the follow-up review of the current code found nothing further. What a4d48b7 changes, after a self-review of efd5908: the port check moved from the runner into How the problem was reproduced: the test-bun shard logs of build 101799 (alpine 3.23 x64 and debian 13 x64, 40 shards) contain How the fix was verified in CI: in build 102141 of this PR, the same 40 shards all print The CodeRabbit comments are rate limit notices, there is nothing to address in them. |
…r it at the first test The runner started the ci-remap server after the root and test/ installs and gave it 5s to print its port. On a fresh CI agent the server takes 3.5s to more than 5s to start, so most shards logged "ci-remap server did not start: timeout" and ran without crash report remapping. The install and the server now start before the docker coordinator and the installs, and the port is awaited right before the first test, with a 30s budget that counts from that point. The outcome line reports how long the first test waited, or how long the server had when it failed. spawnBackground() in scripts/utils.mjs holds the spawn, first-line, timeout and kill-on-exit logic, and test/internal/spawn-background.test.ts covers it under node and bun.
5c69a4c to
c84c97a
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ertions and assert the fixture's output together with its exit code
There was a problem hiding this comment.
LGTM — both earlier nits are addressed in efd5908 and this run found nothing new.
What was reviewed:
startCiRemapServer()failure paths: install failure, spawn ENOENT, non-integer first line, timeout, and early exit all fall through to thedid not startwarning with the server killed andremapPortleft unset — tests still run.spawnBackground():close(notexit) ordering vs. readline'slineevent,settle()idempotence viawithResolvers, and theexit-hook kill; the 12 fixture cases cover each under both node and bun.- Confirmed
spawnBunInstall→spawnBun→spawnSafeaccepts the newstdout/stderrcallbacks, so install output is buffered into the deferred group instead of interleaving with the concurrent setup. - Test file after the fix:
runFixture()asserts{stdout, stderr, exitCode}together; the control case parses the pid before entering try/finally so the orphaned helper is killed even when an assertion throws.
Extended reasoning...
Overview
This PR reworks how scripts/runner.node.mjs starts the CI crash-report remap server. Previously the server was installed and started serially after all other setup with a 5s first-line budget, which timed out on ~75% of fresh CI shards. Now startCiRemapServer() kicks off the install + spawn in the background before the coordinator/installs, and port() is awaited (with a 30s budget counted from the call, not the spawn) right before the first test needs it — so the server's startup overlaps with the 8-68s of other setup and the wait is normally zero. A new spawnBackground() helper in scripts/utils.mjs encapsulates the spawn/first-line/exit-hook pattern, and test/internal/spawn-background.test.ts covers it with 12 cases (6 modes × node/bun).
Security risks
None. This is CI orchestration tooling that runs on Buildkite agents; no user-facing surface, no auth/crypto, no untrusted-input parsing beyond parseInt on the server's own stdout (validated with Number.isInteger, and a bad value is logged and the server killed rather than used).
Level of scrutiny
Low-to-moderate. Nothing here ships in the bun binary — it's the test-runner harness and a script utility. The failure mode is "crash reports aren't remapped" (the pre-PR status quo on most shards), not a broken build or a wrong test result. The change is best-effort by design: every branch of port() that doesn't yield an integer port logs a one-line diagnostic and returns undefined, leaving remapPort unset exactly as before. The PR description cites a CI run (build 102141) where 40/40 shards started the server vs. 10/40 on main.
Other factors
- I reviewed an earlier revision and left two 🟡 nits on the test file (cleanup-before-assertions in the control case, and combined-object assertion so stderr surfaces on failure). The author fixed both in efd5908; the current diff matches the requested shape and both threads are resolved.
- The new test file is thorough for a script helper: it covers line-before-call buffering, exit-without-line failing fast (asserted <15s vs a 30s budget), ENOENT spawn, timeout killing the helper, the exit-hook kill (proven via stderr EOF), and a control case proving the stderr-EOF signal is real.
BUN_FEATURE_FLAG_NO_ORPHANSis deleted from the env so bun's own reaper can't mask a missing hook. - I traced the new
stdout/stderrcallback options throughspawnBunInstall→spawnBun→ the runner'sspawnSafe, which already destructures them with process-stdout defaults, so buffering install output intoinstallOutputfor the deferred group works without further changes. - The PR description notes overlap with #38981 (
killOnExit) and #36079 (adjacent coordinator block); both are textual at most and the author has accounted for them.
…HANS on the remap server spawnBackground() becomes spawnBackgroundServer(): port() reads the first line, requires it to be a port number, and kills a server that printed anything else, so the whole contract the runner relies on lives in utils.mjs and has tests. The SIGTERM choice is documented at the one kill site. The runner passes BUN_FEATURE_FLAG_NO_ORPHANS=1 to the server so it also dies when the runner is killed and runs no exit handlers. The test gains the exit 0 without a line, non-port first line, uncaught exception and signal handler cases. Each of the regressions they guard against (the old non-zero-only exit filter, no port check, the old beforeExit hook) fails exactly those cases.
There was a problem hiding this comment.
LGTM — the two test-hygiene items from the earlier pass are addressed in efd5908, and this run found nothing further.
What was reviewed:
spawnBackgroundServerinscripts/utils.mjs: settle-once viaPromise.withResolvers,closevsexitordering, SIGTERM (not SIGKILL) so thebun runwrapper forwards it,process.once("exit")cleanup, and the port-regex guard replacing the oldparseInt→NaNpath.startCiRemapServerinrunner.node.mjs: install failure / spawn failure / non-port line all resolve to{ error }and fall through to the samedid not startwarning;remapPortstaysundefinedon every failure path soBUN_CRASH_REPORT_URLis never set to a bad value.spawnBunInstallaccepts the newstdout/stderrcallbacks (threaded throughspawnBun's existing options).- The test file's control case now parses the pid before entering the try/finally, and
runFixtureasserts{ stdout, stderr, exitCode }as one object.
Extended reasoning...
Overview
This PR restructures how scripts/runner.node.mjs starts the CI crash-report remap server. The old code installed and started the server synchronously right before the first test with a 5s budget for its first stdout line; on fresh CI agents that budget was routinely missed (30 of 40 shards on build 101799). The new code kicks off the install + server spawn in the background at the top of runTests(), then awaits the port right before the first test with a 30s budget counted from when it's needed — so the server's startup overlaps with the root/test installs that already take 8–68s. A new spawnBackgroundServer() helper in scripts/utils.mjs encapsulates the spawn + first-line + kill-on-exit + timeout pattern, and a new 12-case × 2-runtime test file exercises every failure mode. Verified on real CI: build 102141 started the server on 40/40 shards (39 with a 0.0s wait), and build 102179 passed 179/179 jobs.
Security risks
None. This is CI tooling that runs the test-runner script; it doesn't touch runtime code, user-facing APIs, auth, crypto, or anything that ships in the binary. The server it starts is the existing ci-remap-server bin with the same args and env as before (plus BUN_FEATURE_FLAG_NO_ORPHANS=1 so it dies if the runner is SIGKILLed).
Level of scrutiny
Medium-low. The runner script is important to CI but every failure path here is explicitly best-effort: if the install fails, the server crashes, the server prints garbage, or the timeout fires, the runner logs one line and proceeds without remapping — exactly what happens today when the 5s budget is missed. A bug here surfaces immediately as a broken CI shard, not as a shipped defect. The concurrency is straightforward (one background async chain awaited at one point) and the helper is small and well-commented.
Other factors
- I reviewed this PR once already and left two test-hygiene findings (cleanup armed after assertions in the control case; exitCode asserted before stdout/stderr). Both were fixed in efd5908 exactly as suggested, and both inline threads are resolved.
- The bug-hunting system found nothing on this pass.
- The new helper fixes a latent bug in the old inline block: a non-integer first line used to
parseInttoNaN, pass thetypeof === "number"check, and setBUN_CRASH_REPORT_URL=http://localhost:NaN. The new/^\\d+$/check rejects that and kills the server. - I confirmed
spawnBunalready destructuresstdout/stderrcallbacks, sospawnBunInstall's newstdout/stderroptions thread through correctly via...options. - The PR description notes textual overlap with open PRs #38981 and #36079; that's a merge-order concern, not a correctness one.
|
#38981 (the earlier fix for the |
This branch has not been deployed
1 participant
Problem
ci-remap server did not start: timeoutand run without crash report remapping, so a crash shows as a raw trace string.scripts/runner.node.mjs:797(main) starts the server after the installs and gives it 5s. A fresh agent needs 3.5s to well over 5s.Fix
startCiRemapServer()starts the install and the server before the coordinator and the installs.port()runs right before the first test and waits at most 30s from that call. The installs in between take 8s to 68s, so the wait is normally zero.exitevent (beforeExitnever fires, runner: kill the crash remap server on exit instead of beforeExit, which never fires #38981), plusBUN_FEATURE_FLAG_NO_ORPHANS=1for a killed runner.kill()stays SIGTERM, whichbun runforwards to its script.0.0swait.test/internal/spawn-background-server.test.ts: 20 cases under node and bun, and each guarded regression fails only its own cases (Notes).Background
ci-remap-serverbin ofbun-tracestrings(oven-sh/bun.report). The runner points each test at it withBUN_CRASH_REPORT_URLand fetches remapped stacks after a failure.spawnBackgroundServer()(scripts/utils.mjs) holds the contract:port(timeout)gives the port or the reason there is none and kills a useless server. The server also dies when this process exits.BUN_FEATURE_FLAG_NO_ORPHANSmakes a bun process die with its parent and take its children along. The runner already uses it for some tests.Notes
Why the start is slow: the server loads 122 files of octokit and opens a sqlite database. On a fresh EC2 agent the disk is cold and the docker coordinator is starting the shard's services at the same time. Locally the same start takes 80ms with a release build.
Per shard data from build 101799 (PR #39801, which touches the crash handler and one test, not the runner), from the Buildkite log timestamps:
Shards with and without the napi prebuild time out alike, so that is not the cause. #39446 measured the same rate before it moved the install (17 of 40 and 14 of 40 shards started the server) and left the budget as a follow-up. This is that follow-up.
One alpine shard, which also shows what there is to overlap with (the
test/install is 8s to 14s on debian):This PR's build 102141, same two lanes: 40 of 40 shards print
crash reports parsed on port. The printed wait is0.0son 39 shards and1.5son one debian shard. The install took 1.1s to 10.8s on alpine and 1.6s to 3.6s on debian, all of it hidden behind the test/ install. All 20 alpine shards now printllvm-symbolizer missing, see below.Signals, measured with the release build on Linux against the real
bun run --silent ci-remap-servertree (abun runwrapper with the script as its child). SIGTERM to the wrapper: both gone within 500ms. SIGKILL to the wrapper: the script survives under pid 1 and keeps the runner's stderr open. SIGKILL to the wrapper's parent (the runner's role): both survive without the flag, both gone within 500ms withBUN_FEATURE_FLAG_NO_ORPHANS=1. On macOSbun run --silentexecs the script in place, so there is one process and SIGTERM reaches it directly.The 20 test cases, each under node and bun: the port of a server that stays up, the same with the fixture leaving through an uncaught exception and through a signal handler, a port printed before the call, exit 3 and exit 0 without a line, a command that does not exist, a first line that is not a port, the timeout, and a control server that nothing kills. Mutations run against the test: the old non-zero-only exit filter fails the two exit 0 cases, removing the port check fails the two non-port cases, hooking
beforeExitas before fails the six exit path cases, and stashingscripts/fails all 20.bun bd teston the file takes about 8s here, the bun cases run concurrently.Port line: the whole line has to be decimal digits. The server prints
console.log(server.port), so anything printed before it (abun -e ""banner turned up while writing the test) is reported asprinted "..." instead of a port. The old code putparseIntof such a line,NaN, intoBUN_CRASH_REPORT_URL.Local runs of
CI=true node scripts/runner.node.mjs --exec-path build/debug/bun-debug --vendor=false --quiet internal/parallel-allowlist.test.ts, the second with a temporaryci-remap-serverscript inscripts/ci-remap-server/package.json:The local wait is the debug build loading the server's modules with nothing to hide behind (
--quietskips the installs). The old code needed 3.7s for the same server on an idle 12 core machine, so it was close to its 5s budget even locally.Other behavior changes of the rewritten block: a server that exits 0 without printing fails at once instead of after the timeout, and an install failure is reported on the same
did not startline (it still containsnot installed).Related PRs. #38981 fixes the
beforeExithook of the block this PR deletes and adds the same exit hook to the coordinator, which already has one. This PR supersedes its remap server part, and its test cases are ported into this test. #36079 edits the adjacent coordinator block, a textual conflict at most. The coordinator is not converted tospawnBackgroundServer(): it announces readiness with aCOORDINATOR_READYline inside output the runner streams to the log, and it is tied to the runner through its stdin pipe, so it is a different protocol.Separate finding, not fixed here. On each alpine shard where the server gets as far as loading its modules, it prints
llvm-symbolizer missing(bun.report'sbackend/system-deps.tscallsBun.which("llvm-symbolizer")). The alpine test image has no unversionedllvm-symbolizeron PATH (scripts/bootstrap.shcreates that symlink on the apt path only), so remaps on alpine fail at symbolize time even when the server is up. That is an image change. #27686 attempted it before.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.