Repository navigation
test: hold the remover with await using in transpiler-cache.test.ts - #45857
dylan-conway wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe transpiler cache test now uses ChangesTranspiler cache test
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly identifies the test change: managing the remover with await using in transpiler-cache.test.ts. |
| Description check | |
The description includes both required sections. It explains the Windows flake, the fix, known limitations, and verification results on Windows and Linux. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
|
Updated 9:36 PM PT - Oct 17th, 2026
@dylan-conway, your commit bc14797 is building: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
LGTM — a two-line, test-only sequencing fix: the remover (cwd = temp_dir) is now awaited after kill(9) so the next test's beforeEach rmSync no longer races it on Windows. The inline nit about the early-failure path is minor and does not block.
Extended reasoning...
The diff adds await remover.exited; plus a one-line why-comment after the existing remover.kill(9) in test/cli/run/transpiler-cache.test.ts; no production code or security-sensitive surface is touched. Awaiting the actual exit event is the sanctioned de-flaking pattern, and the comment explains the Windows cwd-lock reason. The only finding is a nit about the already-failing path (expect(!killing) throwing before the await), which does not change the happy-path correctness of the fix. Beyond that I checked that the remover kill/await ordering sits before the per-process assertions and that the awaited subprocess is already killed unconditionally, so the await cannot hang on a normal run.
dylan-conway
changed the title
There was a problem hiding this comment.
LGTM — the follow-up commit addresses the earlier inline note, and the fix mechanism checks out against the runtime source.
What was reviewed:
- Confirmed
Subprocess[Symbol.asyncDispose](src/runtime/api/bun/subprocess.rs:614) kills and returns theexitedpromise, soawait usingwaits for the remover to exit on every path out of the test, including theexpect(!killing)failure path. - Checked the explicit
kill(9)at line 173 and inonExitracing with dispose:try_killis a no-op once exited, Windows swallows ESRCH, and Unixkillon an unreaped child succeeds, so dispose cannot throw there. - The 50 worker processes are already awaited via
Promise.all(... .exited)before any assertion, so they cannot outlive the test the way the remover could.
Extended reasoning...
The diff is two lines in a single test file, test/cli/run/transpiler-cache.test.ts: the remover subprocess becomes await using with a one-line comment naming the Windows cwd constraint. It touches no security-sensitive surface and no CODEOWNER-covered path. The change follows the test/CLAUDE.md await using convention, the asyncDispose semantics it depends on were verified in src/runtime/api/bun/subprocess.rs, and the only prior review comment (mine) was addressed by the second commit, which decided approve.
|
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/cli/run/transpiler-cache.test.tson Windows. In builds 127609 and 127674 (Windows 11 aarch64) it failed and passed on a retry, starting at "disables the cache instead of falling back to the shared temp directory", inbeforeEach:beforeEachremoves the directory of the test before. That test is "doing 50 buns at once does not crash", which starts aremoverprocess with that directory as its working directory and ends withremover.kill(9), without waiting for it. Windows does not let a directory be removed while it is the working directory of a process.The process is now held with
await using, which kills it and waits for it to exit on every way out of the test, a failed assertion included.Build 127706 has the same error starting at another test ("--feature flag invalidates cache"), whose predecessor waits for everything it spawns. This PR does not explain that one.
How did you verify your code works?
Windows 11 x64, 24 cores, with a binary CI built from main's code (d0831bc), main's file and this PR's in alternating rounds:
All failures on main are that
EBUSY ... rm, starting at that test.Linux x64: the whole file gives 19 pass and 1 fail with main's file and with this PR's alike. The one is "a fifo in place of an entry is removed instead of opened", which fails on my machine either way.