Repository navigation
test: await the upload instead of expect().rejects in fetch-abort-stream-body.test.ts - #45862
dylan-conway wants to merge 1 commit into
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 4 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| controller.abort(reason); | ||
| } | ||
| await expect(upload).rejects.toBe(reason); | ||
| expect(await settle(upload)).toBe(reason); |
There was a problem hiding this comment.
🔴 This edit routes the test around a runtime bug instead of fixing it, so CI stops catching a hang that bun test users still hit. await expect(p).rejects calls wait_for_promise (src/runtime/test_runner/expect.rs:513), which re-enters us_loop_run_bun_tick; the nested tick overwrites ready_polls, num_ready_polls and current_ready_poll (packages/bun-usockets/src/eventing/epoll_kqueue.c:536,550), so the outer dispatch loop at epoll_kqueue.c:260 exits early and the undispatched one-shot pipe polls are never read. Fix: make a nested tick preserve and dispatch the outer batch's remaining entries (or refuse to re-enter from dispatch), and keep a regression test for that path; the runtime defect itself is pre-existing.
Why this was flagged
Any user test that reaches expect(promise).rejects or .resolves from a socket/HTTP callback (a microtask continuation inside us_internal_dispatch_ready_polls) while another concurrent test awaits proc.stdout.text() on a Bun.spawn pipe. process_promise at src/runtime/test_runner/expect.rs:507-513 calls wait_for_promise, which loops tick()/auto_tick() (src/jsc/event_loop.rs:1141-1151); auto_tick calls tick_with_timeout (src/runtime/jsc_hooks.rs:1076) with no re-entrancy guard, and us_loop_run_bun_tick only bumps tick_depth (epoll_kqueue.c:482) before overwriting loop->ready_polls/num_ready_polls at epoll_kqueue.c:536 and resetting current_ready_poll at :550. When the nested tick returns, the outer for at epoll_kqueue.c:260 compares the nested counters and exits, so the remaining outer entries are dropped; a Bun.spawn pipe FilePoll is one-shot, so it is not reported again and the test's .text() never settles.
Verification: normal (the user-facing hang itself is pre-existing on base; what this change introduces is routing the one test that exposes it around the runtime bug, which the repository's review instructions name as blocking: "Never edit a test to route around a runtime bug it exposed" and "Fix bugs at the layer that owns the violated invariant, never where the symptom appears") — acknowledged in diff: the PR description says "This only stops the file from running into that behaviour of the event loop", i.e. the author states the runtime defect is left in place; that claim is accurate.
|
Folded into #45873. |
### What does this PR do? Fixes 20 causes of flaky tests, found in the annotations of the last 400 CI builds. Only files under `test/` change. No limit or deadline is raised, and no assertion is dropped except where the table says on which platform. Each row was first its own PR. That PR has the CI sightings, the cause and the numbers in full. | test file | what went wrong | what changes | write-up | |---|---|---|---| | `cli/install/bun-install-streaming-extract.test.ts` | the line taken for the error could be one of the verbose extract log | those lines are removed before the `error:` line is looked for | #45842 | | `cli/install/bun-install-streaming-extract.test.ts` | a body that is all there at the client's first read is not streamed, so `Streamed ` is not printed | the fixture is larger than one read of the client holds | #45844 | | `js/bun/shell/yield.test.ts` | Windows keeps the working directory of a killed process busy for a while (`EBUSY` on `rm`); the pid file exists before the pid is in it | the pid file is named by its full path and nothing has the directory as its cwd; the waiter waits for the file's content | #45846 | | `internal/harness-max-rss.test.ts` | a busy macOS machine no longer keeps the runner's 256 MiB resident when they are read | the two assertions on the runner's own RSS are for Linux, the one platform where a child inherits it | #45847 | | `js/web/fetch/body-clone.test.ts` | the FIFO case also wrote to a stdin that its child never reads (`EPIPE`) | that case gets no stdin | #45848 | | `js/first_party/ws/ws-server-compat-close.test.ts` | the count of sends that were written assumed the loopback takes bytes at once | it counts up to the last send that left nothing buffered | #45849 | | `js/bun/spawn/spawn-stdin-destroy.test.ts` | `sleep(80)` stood for "the child has exited" | `await child.exited` | #45850 | | `regression/issue/29181.test.ts` | file descriptors are closed on the work pool, so some are still open right after the last response | the count is polled until it is under the same limit, with a deadline | #45852 | | `js/web/fetch/fetch-keepalive.test.ts` | Windows Server 2019 delivers loopback bytes late while other loopback connections are busy, as macOS does | the five TCP cases are left out on `10.0.17763` as they are on macOS | #45853 | | `js/bun/jsc/bun-jsc.test.ts` | one run of 10 ms gets no sample when the sampler thread gets no CPU | `profile()` is repeated until it has a sample, with a deadline | #45854 | | `js/third_party/jsonwebtoken/async_sign.test.js` | two tokens signed on either side of a second differ in `iat` | the payload has a fixed `iat` | #45855 | | `js/bun/test/parallel/test-http-should-emit-events-in-the-right-order.ts` | the fixture asked a host on the internet | it asks a local server | #45856 | | `cli/run/transpiler-cache.test.ts` | a killed helper still had the temp directory as its cwd when the next test removed it (`EBUSY`, Windows) | the helper is held with `await using` | #45857 | | `js/web/fetch/fetch-abort-stream-body.test.ts` | `expect(p).rejects` in a socket callback runs a nested tick of the event loop, after which a concurrent test's `proc.stdout.text()` never settles | the promise is awaited | #45862 | | `js/bun/gc/gc-controller-cadence.test.ts` | the wait ended at `<= limit`, the assertion wants `< limit` | the wait goes on at `>= limit`, and the reading that ends it is the one asserted | #45863 | | `js/web/websocket/websocket-client.test.ts` | a limit of 1 s per test from 2023, now with a real closing handshake with a `node` process inside it | the limit is removed, the runner's applies | #45864 | | `js/node/tls/node-tls-connect.test.ts` | in TLS 1.3 the server's handshake ends after the client's, and the test waited for the client's | it waits for both | #45866 | | `js/node/tls/tls-shutdown-before-handshake-fixture.mjs` | the "refused" port was one `connect()` chose, which Linux can hand to the socket under test as its own: it connects to itself | `bind()` chooses the port | #45869 | | `js/bun/module-graph/module-graph-io.test.ts` | on macOS `fs.watch()` returns before anything is watched, and the case wrote once | it writes until the listener is called | #45870 | | `js/bun/module-graph/module-graph-io.test.ts` | the closed UDP port can be given to the socket under test, which then hears itself and is never refused | the port stays bound until that socket exists | #45871 | ### How did you verify your code works? Each change on its own, with release and ASAN binaries that CI built from main's code: the failure reproduced with main's file (under load, or with the losing order forced by a delayed loopback, a blocked thread, a stopped process or a narrow port range), the same conditions with the fix, and a negative test that shows the test still fails on what it guards. Linux x64 locally, Windows 11 x64 for the Windows rows. Some of the numbers: | | main | fix | |---|---|---| | `yield.test.ts`, Windows, 96 at a time | 2878 of 4000 fail | 0 of 4000 | | `transpiler-cache.test.ts`, Windows | 99 of 120 | 0 of 120 | | `bun-install-streaming-extract.test.ts` (body size), Windows | 326 of 6000 | 0 of 6000 | | `29181.test.ts` | 143 of 280 | 0 of 280 | | `bun-jsc.test.ts`, contended CPU | 111 of 200 | 0 of 200 | | `node-tls-connect.test.ts`, loopback delayed by 5 ms | 3 of 3 in each round | 0 | | `tls-shutdown-before-handshake-fixture.mjs`, 32 ports, node and Bun | 12 of 100 each | 0 of 100 each | All of it together, on Linux x64: every changed file passes, but for two tests that fail with main's files on my machine too ("a fifo in place of an entry is removed instead of opened" in `transpiler-cache.test.ts`, and "under node > a session that is set before the handshake starts is resumed" in `node-tls-connect.test.ts`, which runs under my `node` v26.7.0). Not run on the platform they are for: the macOS rows (`harness-max-rss`, `ws-server-compat-close`, the `fs.watch` case), which rest on the CI failures, the source and an emulation on Linux. The Server 2019 numbers of `fetch-keepalive` are from a CI run.
What does this PR do?
Fixes a flake of
test/js/web/fetch/fetch-abort-stream-body.test.ts. "Bun.serve response of new Response(res.body), abort before the response starts" (once "after the response started") timed out in the parallel batch and passed alone in builds 127795 (macOS x64), 127799 (Debian 13 x64), 127811 (Ubuntu 25.04 aarch64) and 127813 (Alpine 3.23 aarch64).The test that times out is not the one that causes it. Its child process finishes and exits with 0. What never settles is
proc.stdout.text()andproc.stderr.text()in the test process: in a captured hang, its ends of the child's pipes hold 26 and 502 unread bytes and have no peer.The cause is another test that runs concurrently with it, "fetch(url, { body: res.body })", in this line:
expect(promise).rejectswaits for a pending promise synchronously (wait_for_promiseinsrc/runtime/test_runner/expect.rs), by running the event loop on the spot. Here it is reached from a socket callback, so from inside the dispatch of one batch of ready polls. The nested tick fetches a new batch into the sameloop->ready_polls, and what was left of the outer batch is not dispatched. A socket's poll is level-triggered and is reported again. The poll of aBun.spawnpipe is one-shot, so it is not, and the pipe is never read.The line now awaits the promise, with the
settle()helper that the other tests of thatdescribeuse for the same thing. It asserts the same, and no nested tick runs. The other two.rejectsof the file are left: one gets a promise that is already rejected, the other is not reached from an I/O callback.This only stops the file from running into that behaviour of the event loop.
How did you verify your code works?
Linux x64, with release binaries CI built from main's code.
The mechanism, forced. A test that writes to a socket and spawns a child that prints, then blocks for 1 s, so that one
epoll_waitreports the socket first and the child's stdout second. Withawait expect(p).rejectsin the socket'sdatacallback,proc.stdout.text()never settles, 5 of 5 times. Withawait p.catch(...)there, it passes 5 of 5 times.The file, whole, main's and this PR's in alternating rounds. Runs that hang:
nice 19on those and one more, 4 at a timeOn a busier machine main's file hung in 10 of 450 runs with nothing added.
The changed tests still catch what they are for (#43699): with
fetch()made to reject with a freshAbortError, and with it made to resolve, both fail.The whole file passes (73 pass, 3 skip).