Repository navigation
test: wait for what is asserted in the two memory tests of gc-controller-cadence.test.ts - #45863
dylan-conway wants to merge 1 commit into
Conversation
…ler-cadence.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 3 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}
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 checked that the >= polls cannot hang when RSS rests at the limit: both keep their deadlines (8 s at line 229, 6 s at line 330), inside the 15 s test timeouts, so the assertion still fails with the value rather than timing out. The burst test's poll at lines 331-332 has the same read-twice shape as the Worker test (one read ends the loop, a second read is asserted), so whatever is done about the inline finding applies to both sites.
Extended reasoning...
The diff changes two comparison operators in test/js/bun/gc/gc-controller-cadence.test.ts so the RSS polls keep waiting while resident() equals the limit the following toBeLessThan asserts; no source, harness, or CLAUDE.md files change and no security-sensitive surface is touched. An inline finding was confirmed on the Worker test's second resident() read, and the same two-read pattern exists in the burst test, so a human should decide whether to assert the polled value instead.
| if (resident() >= full - 80 && performance.now() < deadline) return; | ||
| console.log(JSON.stringify({ full, now: resident() })); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: CI can still see the Worker test fail with Received equal to full - 80 after this merges, the exact flake the PR says it removes. The poll at test/js/bun/gc/gc-controller-cadence.test.ts:231 reads resident() once below the limit, then line 232 reads it again for now. Between the two reads the main thread itself runs readFileSync, a regex and JSON.stringify, which can grow its own RSS by a few KB. When the first read sat just under a MB boundary, the second truncates one MB higher and expect(now).toBeLessThan(full - 80) at line 247 fails. Fix: report the value the poll accepted instead of re-reading; store the resident() result in a local in the setInterval callback and print that, and do the same single read at line 331/332 of the burst test.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
The trigger is RSS resting within a few KB below the full - 80 MB boundary when the poll's read at test/js/bun/gc/gc-controller-cadence.test.ts:231 exits; the test is reached by every Linux non-ASAN non-debug CI run. resident() at line 225 truncates kB to MB with >> 10, so one extra page of RSS can move the second read up by a whole unit. The reads at 231 and 232 are in the same process whose RSS is being measured: readFileSync of /proc/self/status, the regex exec and JSON.stringify at line 232 allocate on the main heap between them. On the base branch the same two reads exist; without the PR's strace harness, RSS steps by pages, so the second read can land one MB above the first. The user then gets Expected: < 47, Received: 47 from line 247 even though the poll had observed a value below the limit. Remedy: capture the polled resident() in a local and report that value, not a fresh read.
Verification: Line 225 resident() in test/js/bun/gc/gc-controller-cadence.test.ts truncates kB to MB with >> 10. Line 231 is one read; line 232 now: resident() is a second, independent read, and it is this now that line 247 asserts. Between the two reads the same process whose RSS is measured runs JS, so VmRSS can rise by a page. The base has the same double-read hazard; merging makes nothing worse.
|
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 the flaky "a burst's garbage is given back within seconds of the idle collection" in
test/js/bun/gc/gc-controller-cadence.test.ts. It failed in the parallel batch and passed alone in builds 127693 (Debian 13 aarch64) and 127840 (Ubuntu 25.04 x64), both times with exactly the limit:The wait and the assertion disagree about that one value:
A poll that reads exactly
full - 250ends the wait, and the read right after it can give the same number of MB. The memory is on its way down at that moment: in a trace of one run, RSS went from 386 MB to 86 MB in about 27 ms, which is 50 MB under the limit, and was at the limit's value for 0.14 ms.The loop now goes on while
resident() >= full - 250, so it waits for what the next line asserts. The Worker test of the file has the same pair (> full - 80, thentoBeLessThan(full - 80)) and gets the same change. The limits, the deadlines and the assertions stay as they are.The file's other failures in CI, timeouts of "the second of two drops it, and it comes back", are all on one branch that changes
--compile --bytecode, and are not touched here.How did you verify your code works?
Linux x64, with a release binary CI built from main's code (d0831bc).
To make the test see RSS while it falls, the child runs under
stracewith a delay of 110 ms, longer than the poll's 100 ms, injected before eachmadvisearound the crossing. RSS then rests at values about 3.5 MB apart, so some runs rest on the limit. Main's file and this PR's in alternating rounds:Expected: < 135, Received: 135four times,< 136,136once)Expected: < 47, Received: 47)The tests still fail when the garbage is not given back. With
BUN_IDLE_GC_SECONDS: "0"in place of"2"in the environment the tests give their child, the burst test gets 390 where less than 140 is expected and the Worker test 131 where less than 50 is. With Bun 1.3.14 as the child they get 403 (< 152) and 145 (< 62).The whole file passes (18 pass).