Repository navigation
test: speed up fetch-backpressure.test.ts and tighten its assertions - #40965
Conversation
The four "server stops writing" tests each drained a 1 GiB body after the pause. They now read until the server has written 1 MiB past the point where it stalled, which proves the resume, and cancel the rest. The exact drain of a body that paused is still covered by the 16 MiB tests for each framing. Fixed waits become conditions: a server exposes `settled()` (its write() returned false or the body is out, then sent() holds still), `wrote(n)` and `closed`. The spawned clients report "stalled" on stdout and wait for "go" on stdin instead of sampling RSS for 60 ms. The small-unread-bodies origin holds the last part until the Response exists instead of 2 ms timers. The released-reader client waits for "go" instead of 50 ms. Assertions: every drained body is checked by md5 against the known digest and every streamed chunk is checked to be all "A". The server's sent() must equal the wire size. Fixture children assert stderr === "". The peer-reset tests assert the error name, code and message. The S3 error and AbortError tests assert the message. The previously sequential describes run concurrently. The two tests that count pooled connections stay serial, at the end of the file. The gzip variant uses stored blocks over a body of "A": the wire still carries about as many bytes as the body, and a debug build no longer spends ~2 s compressing 16 MiB of random bytes on the test's main thread.
|
Warning Review limit reached
On-demand reviews are free for the next 21 days. After that, they cost $0.25 per reviewed file. Or wait 8 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's a large refactor of a timing-sensitive test file — replacing RSS sampling with an event-then-sample settled() protocol, and swapping the 1 GiB full-drain assertions for cancel-after-resume — a human look would still be worthwhile to confirm the coverage trade-offs and flake margins hold across CI lanes.
What was reviewed:
- The new
progress()/settled()machinery:settled()awaitsblocked/finishedbefore its 3×10 ms stability samples, so it's a bounded poll after an event, not a time-as-condition wait;wrote(n)/closedare wired on every path that uses them (h3 never awaitsclosed). spawnClient's stdin/stdout handshake: stderr is drained concurrently via.text()whileforEachLineiterates stdout, so no pipe deadlock; result parsing guards on empty output.- Assertion strength: md5 digest +
isAllA()+ exact error name/code/message replace length-only checks;.toEqualcomposites put stderr/stdout before exitCode per convention; no test is deleted or newly skipped. - Remaining timeouts (
20_000on the idle-process race, pre-existing30_000on the RSS test) sit above internal deadlines rather than acting as the wait condition.
Extended reasoning...
Overview
This PR refactors test/js/web/fetch/fetch-backpressure.test.ts (+483/−356) to speed it up (reported 6.2 s → ~2.5 s release, 60 s → ~18 s debug) and tighten assertions. It introduces shared module-level constants (PAYLOAD, DIGEST, BIG/BODY), a progress() helper exposing sent()/settled()/wrote(n)/closed across all transport kinds (h1/h1-chunked/h1-gzip/h1-tls/h2/h3), an md5-based body-integrity check, an isAllA() memcmp helper, and a stdin/stdout stall()/"go" handshake between the test and spawned clients that replaces per-client RSS sampling. The four 1 GiB drain tests now use readUntilResumed() (read until the server has written 1 MiB past its stall point, then cancel) instead of draining the full body; the 16 MiB tests still drain to completion and now assert an md5 digest. Several describe blocks move to describe.concurrent, per-test 60 s timeouts are dropped, gzip bodies switch from randomBytes to level-0 stored blocks, error assertions gain name/code/message, and the two connection-pool-counting tests are relocated into a trailing describe.serial with the "small unread bodies" test rewritten to gate its last packet on a resolver instead of setTimeout.
Security risks
None. This is a test-only change with no production code touched. All servers bind port: 0 on loopback; no external network contact; subprocess -e scripts are constructed from JSON.stringify'd local URLs and static template literals.
Level of scrutiny
Moderate-to-high. Although test-only, this file is the primary regression guard for fetch receive-side backpressure across six transport kinds, and the refactor changes how the pause is observed (from client-side RSS sampling to a server-side "write returned false, then sent() held still" sample) and how much of the resumed body is verified on the 1 GiB paths. Per REVIEW.md's "Never silently weaken an existing test", the 1 GiB → cancel-after-resume swap deserves a human sanity check that the property the original assertion protected (resume after a real kernel-buffer stall delivers the body without deadlock) is still covered — the PR argues it is, via the 16 MiB digest-checked drains plus readUntilResumed's "server wrote 1 MiB more" condition, and I agree the reasoning holds, but it's a judgment call worth a maintainer's eye. The settled() helper's 3×10 ms stability window after blocked.resolve() is a bounded poll gated on an event (write() returned false past 8 chunks, or the whole body is out), which fits the "poll a bounded window for absence" allowance rather than the forbidden "sleep-then-assert" pattern.
Other factors
The change aligns well with test/CLAUDE.md and REVIEW.md conventions: .toEqual on composite {stdout, stderr, exitCode} objects with exitCode asserted last, Buffer.alloc(n, fill) over .repeat(), bunExe()/bunEnv with -e, await using on spawn/serve/listen, tempDir from harness, forEachLine for line-framed stdout, and describe.concurrent for independent subprocess tests. The describe.serial block's justification comment (a busy sibling could delay a body's last packet reaching the HTTP thread and inflate the pooled-connection count) is plausible; whether placing it at end-of-file is sufficient isolation from the concurrent describes above it depends on Bun's runner scheduling that a maintainer will know offhand. The two remaining explicit per-test timeouts (20 s on the idle-process hang guard, pre-existing 30 s on the release-only RSS test) are ceilings over internal 10 s / multi-second workloads rather than wait conditions. No CODEOWNERS entry covers this path. Given the size, the flake-sensitivity of the domain, and the coverage trade-off on the 1 GiB paths, deferring for a human confirmation is the right call even with zero findings.
|
Two points from the review, for whoever picks this up: Serial block at the end of the file. The runner builds its schedule from declaration order: consecutive concurrent tests form one group, and a serial test is a group of its own (the layout is described at the top of The 1 GiB drains. What those four tests still prove is the same pause: the server's Local numbers: release 6.22 s to 2.45-2.69 s over 5 runs, debug 60.5 s to 17.2-18.0 s over 5 runs plus 3 at |
|
Updated 4:32 AM PT - Aug 30th, 2026
✅ @robobun, your commit 68144749b4427c946943a4bd2536001f957e57f8 passed in 🧪 To try this PR locally: bunx bun-pr 40965That installs a local version of the PR into your bun-40965 --bun |
…ix two CI test deadlocks (#42665) ### What does this PR do? Three of the tests that have been turning builds red over the last few days, by root cause. (The fourth and largest, `spawn-pipe-leak.test.ts`, was #42428 and is already fixed on main by #42571.) **1. `Bun.build` left all but one bundler `Worker` alive after the build** (`bun-build-api.test.ts` "bytecode: repeated builds don't retain the generated code", flaky in 129 of 198 builds on 09-13) - At the end of a build each pool thread gets an idle task that tears down its `Worker` (its AST heap, its transpiler), and `wake_for_idle_events()` wakes the pool. It woke every parked thread but posted one notification. One thread consumed it, left `Event::wait` and drained its idle queue. The rest saw `WAITING` again and went back into the futex without looking at their queues. - So after a `Bun.build` every worker but one stayed allocated until its thread next ran a task. With `bpftrace` on main's release build: 13 to 19 `get_worker_slow`, one `Worker::deinit`. RSS after bundling one 2.5 MB file and `Bun.gc(true)`: **~160 MB in 8 of 10 runs, ~60 MB in the 2 where the lucky thread was the one that parsed the file**. With this PR: 57 to 63 MB in 10 of 10. - The IO pool that reads files on macOS and Windows was never woken, so its threads kept theirs too. - The test compares RSS after three bytecode builds against a baseline taken right after one plain build. Since #42428 `Bun.gc(true)` returns freed memory at once, so the baseline became bimodal (60 or 160) and the comparison failed whenever it came out low. Same fixture, 24 runs, 12 at a time: main fails 2 (deltas 96, 101), this branch 0 (max 31). Fix, in `src/threading/ThreadPool.rs`: the upper bits of the `Event` word count `wake_all()` calls, and a waiter drains its idle queue after publishing `WAITING` and before each sleep. A `wake_all()` that lands between the drain and the sleep changes the word `Futex::wait` expects, so nothing is lost; no notification is posted, so every waiter goes back to waiting. `shutdown()` is a `fetch_or` (SHUTDOWN is both state bits) and `notify()` no longer overwrites SHUTDOWN. The bundler's pool wakes its IO pool as well. `bundlerWorkerLiveCount()` is added to `bun:internal-for-testing` (same shape as `sslCtxLiveCount`). New test: build, then the count must reach 0; run with and without `BUN_FEATURE_FLAG_FORCE_IO_POOL`. Without the `Event` change it prints `[15,16]`; without the IO pool wake the forced-IO-pool row stays at 1. **2. `fetch-backpressure.test.ts` "stalled no consumer drains the full body"** (90 s timeout, windows 11 aarch64 only, ~65% of builds flaky, ~5% red; since #40965) Not a fetch bug: the test's handshake can deadlock. `settled()` waits for the server to have written more than 8 chunks, the client waits for `settled()` before it reads, and a client nothing reads pauses its socket after 256 KiB (`BODY_HIGH_WATER_MARK`, 4 chunks). Where loopback buffers take fewer than 5 more chunks, the server is waiting for `'drain'` at 8 chunks or fewer and nobody moves. Reproduced on Linux with main's CI release build in a netns with `tcp_rmem`/`tcp_wmem` capped at 64 KiB: the three h1 "no consumer" cases time out. The threshold is now what a client guarantees to take (the mark). Whole file, patched: 70/70 at 16 KiB, 64 KiB, 96 KiB and default buffers. **3. `bun-patch.test.ts` "packages whose label is longer than 1024 bytes"** (Windows, ~60% of builds flaky, ~4% red) Nothing to do with the label length. `scripts/runner.node.mjs` exports `BUN_INSTALL_CACHE_DIR`, which wins over the `cache` the harness writes to each test's bunfig, and two of the three `test.concurrent` cases install the same tarball spec, so they extract into the same `@T@<hash>` folder. The test now pins the cache per project, as `bun-dedupe`, `bun-install-patch` and `bun-install-offline` already do. That leaves the product side, which this PR does not change: on Windows the loser of that race renames the winner's folder away and `delete_tree`s it (`extract_tarball.rs`, the retry branch), where POSIX does an atomic `renameat2(EXCHANGE)`. The winner then sees `failed to resolve cache dir: EBADF`, `"package.json" failed to open: ENOENT`, or a failed hardlink. A local tarball is always re-extracted and keyed only by its spec string, so "reuse what is there" is not available for it. ### How did you verify your code works? - New test fails with the `Event` change gutted (`[15,16]`), and its IO pool row fails with the IO pool wake removed; both pass with the fix on debug+ASAN and release. - `bytecode: repeated builds don't retain the generated code`: 10/10 on this branch's release build, 7/10 on main's. - 40 rounds each (default and forced IO pool) of 4 concurrent `Bun.build`s + 16 `readFile`s + 4 `Bun.password.hash` on the same pool, release and debug: no worker left, no hang. `bun build` CLI (owned pool, shutdown + join) exits. - `fs.promises.stat` in a loop, which goes through `Event::notify` on every call, interleaved against main's release build on 8 cores: 13.9/13.9/14.2 vs 13.3/15.0/14.0 µs/op. - `test/js/node/fs/fs.test.ts`, `test/bundler/bundler_splitting.test.ts`, `test/bake/dev/bundle.test.ts`, `test/bake/deinitialization.test.ts`, `test/cli/install/bun-patch.test.ts`, `test/js/web/fetch/fetch-backpressure.test.ts` on the debug build. - The Windows halves of 2 and 3 are from CI logs and code; this PR's Windows lanes are the check.
…ix two CI test deadlocks (oven-sh#42665) ### What does this PR do? Three of the tests that have been turning builds red over the last few days, by root cause. (The fourth and largest, `spawn-pipe-leak.test.ts`, was oven-sh#42428 and is already fixed on main by oven-sh#42571.) **1. `Bun.build` left all but one bundler `Worker` alive after the build** (`bun-build-api.test.ts` "bytecode: repeated builds don't retain the generated code", flaky in 129 of 198 builds on 09-13) - At the end of a build each pool thread gets an idle task that tears down its `Worker` (its AST heap, its transpiler), and `wake_for_idle_events()` wakes the pool. It woke every parked thread but posted one notification. One thread consumed it, left `Event::wait` and drained its idle queue. The rest saw `WAITING` again and went back into the futex without looking at their queues. - So after a `Bun.build` every worker but one stayed allocated until its thread next ran a task. With `bpftrace` on main's release build: 13 to 19 `get_worker_slow`, one `Worker::deinit`. RSS after bundling one 2.5 MB file and `Bun.gc(true)`: **~160 MB in 8 of 10 runs, ~60 MB in the 2 where the lucky thread was the one that parsed the file**. With this PR: 57 to 63 MB in 10 of 10. - The IO pool that reads files on macOS and Windows was never woken, so its threads kept theirs too. - The test compares RSS after three bytecode builds against a baseline taken right after one plain build. Since oven-sh#42428 `Bun.gc(true)` returns freed memory at once, so the baseline became bimodal (60 or 160) and the comparison failed whenever it came out low. Same fixture, 24 runs, 12 at a time: main fails 2 (deltas 96, 101), this branch 0 (max 31). Fix, in `src/threading/ThreadPool.rs`: the upper bits of the `Event` word count `wake_all()` calls, and a waiter drains its idle queue after publishing `WAITING` and before each sleep. A `wake_all()` that lands between the drain and the sleep changes the word `Futex::wait` expects, so nothing is lost; no notification is posted, so every waiter goes back to waiting. `shutdown()` is a `fetch_or` (SHUTDOWN is both state bits) and `notify()` no longer overwrites SHUTDOWN. The bundler's pool wakes its IO pool as well. `bundlerWorkerLiveCount()` is added to `bun:internal-for-testing` (same shape as `sslCtxLiveCount`). New test: build, then the count must reach 0; run with and without `BUN_FEATURE_FLAG_FORCE_IO_POOL`. Without the `Event` change it prints `[15,16]`; without the IO pool wake the forced-IO-pool row stays at 1. **2. `fetch-backpressure.test.ts` "stalled no consumer drains the full body"** (90 s timeout, windows 11 aarch64 only, ~65% of builds flaky, ~5% red; since oven-sh#40965) Not a fetch bug: the test's handshake can deadlock. `settled()` waits for the server to have written more than 8 chunks, the client waits for `settled()` before it reads, and a client nothing reads pauses its socket after 256 KiB (`BODY_HIGH_WATER_MARK`, 4 chunks). Where loopback buffers take fewer than 5 more chunks, the server is waiting for `'drain'` at 8 chunks or fewer and nobody moves. Reproduced on Linux with main's CI release build in a netns with `tcp_rmem`/`tcp_wmem` capped at 64 KiB: the three h1 "no consumer" cases time out. The threshold is now what a client guarantees to take (the mark). Whole file, patched: 70/70 at 16 KiB, 64 KiB, 96 KiB and default buffers. **3. `bun-patch.test.ts` "packages whose label is longer than 1024 bytes"** (Windows, ~60% of builds flaky, ~4% red) Nothing to do with the label length. `scripts/runner.node.mjs` exports `BUN_INSTALL_CACHE_DIR`, which wins over the `cache` the harness writes to each test's bunfig, and two of the three `test.concurrent` cases install the same tarball spec, so they extract into the same `@T@<hash>` folder. The test now pins the cache per project, as `bun-dedupe`, `bun-install-patch` and `bun-install-offline` already do. That leaves the product side, which this PR does not change: on Windows the loser of that race renames the winner's folder away and `delete_tree`s it (`extract_tarball.rs`, the retry branch), where POSIX does an atomic `renameat2(EXCHANGE)`. The winner then sees `failed to resolve cache dir: EBADF`, `"package.json" failed to open: ENOENT`, or a failed hardlink. A local tarball is always re-extracted and keyed only by its spec string, so "reuse what is there" is not available for it. ### How did you verify your code works? - New test fails with the `Event` change gutted (`[15,16]`), and its IO pool row fails with the IO pool wake removed; both pass with the fix on debug+ASAN and release. - `bytecode: repeated builds don't retain the generated code`: 10/10 on this branch's release build, 7/10 on main's. - 40 rounds each (default and forced IO pool) of 4 concurrent `Bun.build`s + 16 `readFile`s + 4 `Bun.password.hash` on the same pool, release and debug: no worker left, no hang. `bun build` CLI (owned pool, shutdown + join) exits. - `fs.promises.stat` in a loop, which goes through `Event::notify` on every call, interleaved against main's release build on 8 cores: 13.9/13.9/14.2 vs 13.3/15.0/14.0 µs/op. - `test/js/node/fs/fs.test.ts`, `test/bundler/bundler_splitting.test.ts`, `test/bake/dev/bundle.test.ts`, `test/bake/deinitialization.test.ts`, `test/cli/install/bun-patch.test.ts`, `test/js/web/fetch/fetch-backpressure.test.ts` on the debug build. - The Windows halves of 2 and 3 are from CI logs and code; this PR's Windows lanes are the check.
2 participants
Problem
test/js/web/fetch/fetch-backpressure.test.tstakes about 12 s per CI lane (build #108487: 11.75 s debian 13 x64, 11.2 s asan, 10.3 s windows aarch64). Locally: 6.2 s release, 60 s debug.Fix
settled()(write() returned false or the body is out, thensent()holds still),wrote(n),closed, and a stdout/stdin handshake with the spawned clients.sent() === wire,stderr === ""for children, exact error name, code and message for reset and abort.--max-concurrency=5). No test is deleted or skipped.Background
BODY_HIGH_WATER_MARK,src/http/Signals.rs). The kernel buffers fill, then the server'swrite()returns false. That is the observable pause.settled()has to sample: no event reports "nothing more is coming". It starts only after the socket refused a write (or the body is out), so it waits for the absence of progress, not for a fixed time.Notes
gzipSync(randomBytes(16 MiB))cost 1.9 s on the test's main thread in a debug build (365 ms release). Stored blocks over the "A" body keep the wire size equal to the body (16779789 bytes) and cost 0.2 s. The decoder still runs over every byte.Bun.write(file, res)whose body is still arriving keeps the Response reachable for about 300 event-loop turns (0.4-0.9 s), and the abort of a collected S3 stream takes about as long to reach the origin. Any socket event on the JS loop releases it at once. Waking the HTTP thread alone does not. Debug builds do not show it. The two collection tests here spend that time in their GC loops.USE_SYSTEM_BUN=1), 5 runs, all green.no test proof · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/fetch-backpressure.test.ts