Repository navigation
Fix quadratic backlog drain in bun test --parallel IPC - #37291
Conversation
Channel::flush on POSIX drained the outgoing backlog by memmoving the entire remainder to the front of the buffer after every partial write. The kernel accepts one send-buffer chunk per writable event (about 8KB for macOS socketpairs), so a single 64MB TestDone frame moved roughly 512GB of memory before it finished: about 90 seconds on the macOS x64 CI machines, which made the >64MB result line test time out regularly. Linux socket buffers are large enough that the same drain stayed around a second, so it only showed on darwin lanes. Track a cursor of already-written bytes instead, and compact only once the sent prefix is at least as large as the unsent tail, which keeps total compaction cost linear in bytes sent. With socketpair buffers forced to 8KB on Linux to mimic macOS, the 68MB-frame repro goes from 9.2s to 1.4s in a debug build; on a darwin x64 CI host the same repro took 89s with a current canary build.
|
Warning Review limit reached
Next review available in: 47 seconds 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 (1)
WalkthroughThe POSIX channel now tracks unsent output without repeated buffer shifts. A parallel regression test covers two consecutive 60 MB result frames and checks output integrity, worker stability, concurrent success, and normal exit. ChangesParallel channel buffering
Possibly related PRs
Suggested reviewers: 🚥 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 `@test/cli/test/parallel.test.ts`:
- Around line 909-913: Strengthen the assertions in the parallel test around the
stderr result titles: instead of only checking 10,000-character prefixes,
extract each contiguous A and B title run and assert its complete expected
length, including the 60 MB titles. Preserve the existing crash and pass/fail
assertions while ensuring the checks detect both tail loss and truncation.
🪄 Autofix
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: d6ff972e-1055-4a74-9a11-1328f3b5ace9
📒 Files selected for processing (2)
src/runtime/cli/test/parallel/Channel.rstest/cli/test/parallel.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted fix replacing the O(n²) drain_front-per-partial-write with a cursor + amortized compaction in the POSIX Channel::flush path.
What was reviewed:
- Traced
out_headthroughflush()loop iterations and across separate writable callbacks; verified interaction withsend()(checks!out.is_empty(), only appends),has_pending_writes(), and the re-entry-safewith_mutswap-back — the invariantout_head < out.len() || out_head == 0holds on every exit path, so already-sent bytes are never re-sent and the empty-check insend()still gates correctly. - Confirmed the
head == pending.len()→clear()branch prevents the defensivepending.len() <= headearly-return from ever firing with stale bytes; Windows path (uv_write) is untouched. - New test: drains pipes concurrently, uses
Buffer.alloc, asserts full 60M-char runs (drop + duplication),signalCode === null, and exit code last. All prior bot feedback (title-length assertion, comment length,.repeat) is addressed and resolved.
Extended reasoning...
Overview
Two files touched: src/runtime/cli/test/parallel/Channel.rs (the POSIX backend of the fd-3 IPC channel used exclusively by bun test --parallel) and test/cli/test/parallel.test.ts (one new regression test). The Rust change adds an out_head: Cell<usize> cursor to PosixBackend and rewrites the ~12-line drain loop in flush() to write from &pending[head..] and compact only when the sent prefix is at least half the buffer, instead of drain_front(w) (memmove the whole remainder) after every partial write. No protocol, wire-format, or public-API change; the Windows uv_write path is not touched.
Security risks
None. This is an internal, in-process IPC buffer between the bun test coordinator and its own spawned worker processes. No untrusted input reaches this path (frame lengths are already capped at frame::MAX_PAYLOAD in ingest(), unchanged here), and the cursor is a usize bounded by pending.len() on every use. The one slice operation &pending[head..] is guarded by the preceding pending.len() <= head early-return, so it cannot go out of bounds even in release builds where the debug_assert! compiles out.
Level of scrutiny
Medium — focused perf fix in an internal test-runner subsystem, not user-facing runtime code. I traced correctness carefully because the cursor state persists across callback invocations and interacts with send():
send()gates its fast-path direct write onout.is_empty(); with the cursor,outremains non-empty wheneverout_head > 0(thehead == pending.len()branch clears the vec and resets head atomically), sosend()correctly appends behind the cursor and never races a direct write against a partially-drained backlog.has_pending_writes()checks!out.is_empty(), which stays accurate for the same reason.- The re-entry-safe
self.out.with_mut(|cur| { pending.extend_from_slice(cur); *cur = pending })at loop end still works: any bytes appended to the (temporarily empty)outcell duringsocket.write()land after the cursor. - The amortized-compaction condition
head >= pending.len() - headis the standard half-full trigger; each byte is memmoved at most once, giving linear total work.
Other factors
The PR is a de-flake for a top CI offender (19 failures / 400 builds on darwin-x64) with a well-explained root cause (sample showing 5294/5317 samples in _platform_memmove, ~512GB of memmove for one 64MB frame at 8KB socketpair buffers). All prior automated feedback is resolved: CodeRabbit's title-length-assertion strengthening (a1bde45), the comment-cop length complaints (734d8d7, 9909ace), and my earlier Buffer.alloc nit (already addressed in a1bde45 before that review landed). No CODEOWNERS entry covers this path. The new test follows harness conventions (concurrent pipe drain, tempDir, bunEnv spread, signalCode assertion for hang-guard, exit code last) and the PR honestly notes it only proves the fix on darwin-x64 lanes since Linux socket buffers are large enough to stay under the deadline either way.
## Problem
`test/cli/test/parallel.test.ts > --parallel: a test producing a >64MB
result line is truncated, not treated as a crash` is one of the most
frequent flakes in CI right now: 19 failures across 17 branches in the
last 400 builds, every one on the macOS x64 test lane, spread over six
different agents. Signature:
```
867 | expect(result).not.toBe("TIMEOUT");
error: expect(received).not.toBe(expected)
Expected: not "TIMEOUT"
```
The spawned `bun test --parallel=2` run blows past the test's 60s
deadline. Reproduced on an idle macOS 14 x64 CI host with a current
canary: the two-file repro takes **89 seconds**, CPU-bound (92s user
time). `sample` shows 5294 of 5317 samples in `_platform_memmove` under
the IPC flush path.
## Cause
`Channel::flush` (the fd-3 IPC channel used by `bun test --parallel`)
drained the outgoing backlog like this: write once, then
`drain_front(w)`, i.e. memmove the entire unsent remainder to the front
of the buffer, once per writable event.
The kernel accepts one send-buffer chunk per event, and macOS
socketpairs default to roughly 8KB (the extra-fd socketpair gets no
buffer-size bump, unlike stdio fds 0-2). For the 64MB `TestDone` frame
this test generates, that is ~8200 writable events, each memmoving
~64MB: about **512GB of memmove for one frame**, which is the 89
seconds. Linux socket buffers are ~26x larger, so the same quadratic
stays near a second there and never trips the deadline, which is why the
flake is exclusive to darwin x64 (darwin arm64 has the memory bandwidth
to squeak under 60s).
## Fix
Track a cursor (`out_head`) of bytes already accepted by the kernel
instead of draining the front after every partial write, and compact
only once the sent prefix is at least as large as the unsent tail. Each
compaction is charged to the bytes it discards, so total compaction cost
is linear in bytes sent. No protocol or behavior change; Windows uses
the `uv_write` path and is unaffected.
## Verification
- Linux debug build with the extra-fd socketpair buffers temporarily
forced to 8KB (local-only patch) to mimic macOS: the 68MB-frame repro
goes from **9.2s unfixed to 1.4s fixed** under identical conditions,
with identical output (1 pass, 1 fail, truncation marker present).
- New test: two back-to-back 60MB status lines from the same worker,
which exercises the cursor, the amortized compaction, and appending to a
partially-sent backlog. Asserts both 60MB names arrive byte-intact, no
worker is marked "crashed", and the run beats a 60s kill. On unfixed
macOS x64 the drain alone exceeds the kill; passes in ~6s fixed (debug,
Linux). Also ran the whole suite with the 8KB-buffer build so every
frame goes through the chunked path: no new failures.
- `bun bd test test/cli/test/parallel.test.ts`: failure set identical to
the unfixed baseline on this machine (two scheduling-sensitive tests
that already fail locally under debug ASAN, unrelated).
Note for review: the quadratic is only observable with small socket
buffers, so on Linux both the existing test and the new one pass with or
without the fix; the lanes that prove the fix are the darwin x64 ones,
where the existing `>64MB result line` test stops flaking.
<!-- robobun:evidence:begin -->
---
**no test proof** · iteration 0 · Platform-specific test(s) that do not
run on this machine. Deferring to CI, which covers all platforms:
test/cli/test/parallel.test.ts
<!-- robobun:evidence:end -->
Problem
test/cli/test/parallel.test.ts > --parallel: a test producing a >64MB result line is truncated, not treated as a crashis one of the most frequent flakes in CI right now: 19 failures across 17 branches in the last 400 builds, every one on the macOS x64 test lane, spread over six different agents. Signature:The spawned
bun test --parallel=2run blows past the test's 60s deadline. Reproduced on an idle macOS 14 x64 CI host with a current canary: the two-file repro takes 89 seconds, CPU-bound (92s user time).sampleshows 5294 of 5317 samples in_platform_memmoveunder the IPC flush path.Cause
Channel::flush(the fd-3 IPC channel used bybun test --parallel) drained the outgoing backlog like this: write once, thendrain_front(w), i.e. memmove the entire unsent remainder to the front of the buffer, once per writable event.The kernel accepts one send-buffer chunk per event, and macOS socketpairs default to roughly 8KB (the extra-fd socketpair gets no buffer-size bump, unlike stdio fds 0-2). For the 64MB
TestDoneframe this test generates, that is ~8200 writable events, each memmoving ~64MB: about 512GB of memmove for one frame, which is the 89 seconds. Linux socket buffers are ~26x larger, so the same quadratic stays near a second there and never trips the deadline, which is why the flake is exclusive to darwin x64 (darwin arm64 has the memory bandwidth to squeak under 60s).Fix
Track a cursor (
out_head) of bytes already accepted by the kernel instead of draining the front after every partial write, and compact only once the sent prefix is at least as large as the unsent tail. Each compaction is charged to the bytes it discards, so total compaction cost is linear in bytes sent. No protocol or behavior change; Windows uses theuv_writepath and is unaffected.Verification
bun bd test test/cli/test/parallel.test.ts: failure set identical to the unfixed baseline on this machine (two scheduling-sensitive tests that already fail locally under debug ASAN, unrelated).Note for review: the quadratic is only observable with small socket buffers, so on Linux both the existing test and the new one pass with or without the fix; the lanes that prove the fix are the darwin x64 ones, where the existing
>64MB result linetest stops flaking.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/test/parallel.test.ts