Repository navigation
test: write until the watcher hears it in the fs.watch case of module-graph-io.test.ts - #45870
dylan-conway wants to merge 1 commit into
Conversation
…-graph-io.test.ts
|
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 2 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 |
|
Updated 9:40 PM PT - Oct 17th, 2026
@dylan-conway, your commit ced43ed is building: |
|
Folded into #45873. |
![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.
Beyond the inline finding, I also checked two things and ruled them out: again is read in the watch callback before its const declaration at test/js/bun/module-graph/module-graph-io.test.ts:975, but fs.watch only dispatches from the event loop, never synchronously from fs.watch(), so there is no TDZ hit; and a leftover interval cannot hang the fixture, since main.mjs ends with process.exit(0).
Extended reasoning...
The change is a 5-line edit to one spawned-fixture template string in a test file, with no production or security-sensitive surface. The single inline finding (the interval is only cleared when the watcher fires) is the one real concern; the Linux duplicate-event and TDZ questions do not affect correctness.
| const w = fs.watch(dir, () => { clearInterval(again); w.close(); boom(); }); | ||
| // macOS starts watching on another thread after fs.watch() returns, and reports nothing written before that. | ||
| const write = () => fs.writeFileSync(path.join(dir, "f"), "x"); | ||
| write(); | ||
| const again = setInterval(write, 5); |
There was a problem hiding this comment.
🟡 (optional) On any run where the watcher never reports, the fixture keeps rewriting watched/f every 5 ms through all ~20 later cases, so the failure report is polluted by unrelated load. The interval at test/js/bun/module-graph/module-graph-io.test.ts:975 is only cleared inside the watch callback at :971; the 10 s fallback at :1005 resolves the case but stops nothing. The sibling interval case at :1051-1052 exports stop() and main.mjs calls it at :1061; this case has no equivalent. Fix: stop the interval and close the watcher on every exit of the case, e.g. return a stop function the driver calls after told.promise settles, or clear it from the fallback path.
Why this was flagged
Trigger: a platform or build where fs.watch delivers no event for the directory within 10 s (the exact failure this PR is fixing on macOS). The case falls through to the fallback at test/js/bun/module-graph/module-graph-io.test.ts:1005 and the loop at :1010 moves on. Nothing clears again (:975) or closes w (:971), so a synchronous fs.writeFileSync runs every 5 ms from the graph's timer for the rest of the run, until process.exit(0) at :1024. On the base branch a never-reporting watcher left one file and one idle handle; after the diff it leaves a hot timer. On debug and ASAN lanes each write is 10-100x slower, so later timing-sensitive cases can also flip to 'nobody was told', hiding the real culprit. The file's own convention for an interval stimulus (:1048-1062) exports stop() and the driver calls it; REVIEW.md requires every timeout path to clear timers and release resources. Remedy: make the case return a cleanup the driver runs after the case settles, or clear the interval and close the watcher from the fallback.
Verification: Triggers only when the fs.watch callback never fires. The interval at test/js/bun/module-graph/module-graph-io.test.ts:975 is cleared only inside the watch callback at :971. The fallback at :1005 resolves the case and the loop at :1010 moves on with nothing stopping the interval or closing w. process.exit(0) at :1024 means the leaked interval cannot keep the process alive.
### 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/bun/module-graph/module-graph-io.test.tson macOS. "what a listener of the graph's throws, for every kind of thing it can listen to" failed and passed on a retry in build 127805 (macOS aarch64), in one of its cases:The case calls
fs.watch(dir, ...)and writes one file intodirin the same tick. On macOSfs.watch()returns before anything is watched:register_watcherinsrc/runtime/node/fs_events.rsonly queuesschedulefor the CFRunLoop thread, and that creates the event stream fromkFSEventStreamEventIdSinceNow. When the write gets there first it is in the past for the stream, there is no second write, and the listener is never called.The case now writes every 5 ms until the listener is called.
test/js/node/watch/fs.watch.test.tsrepeats its writes for the same reason, and so does the other watcher test of this file.How did you verify your code works?
Not run on macOS. The cause is from the source and from the CI failure.
Linux x64, with a release binary CI built from main's code (d0831bc), and the order forced: in copies of the file, the case starts its watcher 20 ms after the write.
"nobody was told within 10 seconds"The case still catches what it is for: with the listener made to run as the host's (in an
AsyncResourceof the host), it reports"host"where"graph"is expected.The whole file passes 3 of 3 times (28 pass).