test: widen net-mongodb-pattern-leak RSS bound; await the named-pipe GC check - #33725
test: widen net-mongodb-pattern-leak RSS bound; await the named-pipe GC check#33725robobun wants to merge 6 commits into
Conversation
…godb-pattern RSS bound Two unrelated node:net test flakes that started in late June: loop.c: #29831 added an is_paused check to the recv repeat-read, so on_data pausing the socket (net.Socket push() returned false) now breaks out of the recv loop with the kernel buffer undrained. kqueue's EV_EOF is set as soon as the peer's FIN lands, so the immediately-following eof block dispatched end with bytes still buffered; on resume the remainder arrived after push(null) and node:net raised ERR_STREAM_PUSH_AFTER_EOF. test-net-write-slow.js on the macOS arm64 lanes. Guard the eof block on !is_paused; us_socket_resume re-arms readable and the level-triggered filter re-reports eof once recv() actually returns 0. net-mongodb-pattern-leak.test.ts: the tcp case's round-2 minus round-1 RSS delta has been observed at 8-13 MB on release CI since the WebKit upgrade in 00a93bd, with heapSize and every object count flat, i.e. allocator and JIT page noise rather than a leak. The precise regression guard for #12117 is the object counts above it; the RSS line is a backstop for native leaks, so give it ~2x headroom over the observed max.
WalkthroughThis PR makes two independent test adjustments: it relaxes the RSS growth threshold in a MongoDB pattern leak regression test from 8 MiB to 24 MiB for non-ASAN/non-debug runs, and it corrects a named-pipe test to track TCPSocket object counts instead of TLSSocket, awaiting the assertion. ChangesLeak Test RSS Threshold Adjustment
Named-Pipe Heap Tracking Correction
Sequence Diagram(s)Not applicable — these changes are test-only adjustments to thresholds and object-count tracking with no observable flow to diagram. Estimated code review effort: Low Suggested labels: test Suggested reviewers: none 🐰 A hop, a nudge, a threshold grown, 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:53 AM PT - Jul 8th, 2026
❌ @robobun, your commit 260fbfd has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33725That installs a local version of the PR into your bun-33725 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the duplicate flag: #32257 is the comprehensive fix for the kqueue On the issue-finder's #31383 and #32231: #31383 is from 1.3.10 (Feb), predating #29831, and its payload is 5 bytes so |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Gating the whole eof block on !is_paused skipped us_socket_is_shut_down -> close_raw, which on epoll is the only consumer of the level-triggered EPOLLHUP a paused+shut-down socket registers at poll events==0, so pause();end();peer-FIN busy-looped. Move the guard inside the block so only on_end is deferred. Adds a Linux-reproducible test for that sequence.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/net/node-net.test.ts`:
- Around line 1006-1059: Both new socket-I/O tests are independent and use their
own dynamic TCP server/client setup, so they can run in parallel. Update the two
`it(...)` cases in `node-net.test.ts` to use `test.concurrent` (or the
equivalent concurrent form used elsewhere in this suite) so they don’t serialize
unnecessarily, while keeping the existing `createServer`, `connect`, and cleanup
logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33290456-d7d4-443e-ba64-016c80730740
📒 Files selected for processing (3)
packages/bun-usockets/src/loop.ctest/js/node/net/net-mongodb-pattern-leak.test.tstest/js/node/net/node-net.test.ts
… clients in the new tests The Windows-only named-pipe test called expectMaxObjectTypeCount without await (and read the TLSSocket count as its TCPSocket baseline), so the assertion floated and fired ~1.5s later inside whatever test was running then. Adding two tests at the end of the file put their sockets in scope when it fired, which CI attributed to the last test. Also destroy the client sockets in those two tests' finally blocks.
…ad READABLE) Deferring for every is_paused busy-looped on epoll when a previously-paused AF_UNIX socket's peer closed: unix_release_sock sets sk_shutdown=MASK on our side so EPOLLHUP fires level-triggered, us_socket_is_shut_down is false (we never called shutdown), and with events&READABLE==0 the recv loop never ran so there are no deferred bytes. Gate the defer on events&READABLE so on_end still fires for prior-paused sockets. Skip the EPOLLHUP tests on Windows where uv_poll never passes eof from its callback.
…on_data's microtask drain on_data's exit_scope drains microtasks before returning to loop.c, so a queueMicrotask'd resume clears is_paused before the recv loop reads it and the is_paused branch under test never runs. test-net-write-slow.js resumes from setTimeout(20).
…d-pipe await fix Three review rounds each found a new level-triggered busy-loop variant the targeted is_paused defer introduces (paused+shut_down EPOLLHUP, AF_UNIX peer-close EPOLLHUP, partial-write-rearms-READABLE + recv()==0). The class is exactly what #32257 addresses by making recv()==0 the sole eof source for readable polls on kqueue, so deferring the test-net-write-slow.js fix there. This PR now only: - widens the release RSS bound in net-mongodb-pattern-leak.test.ts from 8 to 24 MB (observed 8-13 MB with flat heapSize and object counts since the WebKit upgrade in 00a93bd) - awaits the floating expectMaxObjectTypeCount in the Windows named-pipe test and fixes its TLSSocket vs TCPSocket baseline, so its rejection stops being attributed to whatever test is running ~1.5s later
robobun
changed the title
There was a problem hiding this comment.
LGTM — with the loop.c change dropped, what remains is two straightforward test-only fixes.
Extended reasoning...
Overview
After three review rounds on the loop.c is_paused defer (each surfacing a new level-triggered busy-loop on epoll), the author dropped that change entirely in 260fbfd and deferred the whole kqueue EV_EOF class to #32257. What remains is two test files:
net-mongodb-pattern-leak.test.ts: widens the release-build RSS delta bound from 8 MB to 24 MB. The PR description documents that this bound has been failing ~40% of builds since the WebKit upgrade (#33133), with observed deltas of 8.4–13.1 MB whileheapSizeand object-type counts stay flat — allocator/JIT noise, not retention. The comment above the assertion already labels it a "weak backstop"; the precise #12117 regression guard is the object-count assertions above it, which are unchanged. 24 MB is ~2x the observed max and still trips a real 5 KB/iter native leak at ITER=5000. This matches CLAUDE.md's "RSS thresholds need ~2x headroom" guidance.node-net.test.ts(Windows-only named-pipe test): fixes the baseline to readobjectTypeCounts.TCPSocketinstead of.TLSSocket(the assertion two lines below checks"TCPSocket", so the baseline was simply wrong), and adds the missingawaitbeforeexpectMaxObjectTypeCount— confirmedasyncattest/harness.ts:294, and every other call site intest/awaits it. Without theawaitthe assertion floated and rejected inside a later test.
Security risks
None. Test-only; no production code, no untrusted input handling, no auth/crypto/permissions.
Level of scrutiny
Low. Both changes are mechanical test-quality fixes with clear rationale. Widening an RSS bound is the kind of change that warrants scrutiny for whether it's papering over a real leak, but the PR description does that work: flat heapSize, flat object counts, later-round deltas going negative, and a specific attribution to the WebKit upgrade. The named-pipe fix is an obvious typo + missing-await that made an existing assertion vacuous.
Other factors
All my prior inline comments on this PR were on the now-dropped loop.c change and are resolved. The one CI failure on the current build (postgres-invalid-message-length.test.ts on Windows x64-baseline) is unrelated to either touched file. No outstanding reviewer comments.
|
Build 70375 is 282 passed / 2 failed / 2 darwin-14-aarch64 still queued. The two failed jobs are unrelated infra:
Neither touches the two test files in this diff. |
|
Opened #34752 as an alternative: instead of widening the 8 MB bound, it runs one unmeasured warmup round so the RSS delta is taken at steady state (r3-r2 ranged -2.2..+2.0 MB over 30 runs vs -0.0..+11.0 MB for r2-r1). The extra RSS after #33133 is JIT code pages: |
1 participant
net-mongodb-pattern-leak.test.ts: widen the release RSS bound
This test's
expect(after2.rss - after1.rss).toBeLessThan(8 MB)has failed on roughly 40% of PR builds across linux (glibc and musl, x64 and aarch64) and macOS since build #67387, which is the WebKit upgrade in 00a93bd (#33133). It is always thetcpcase, which runs first and so absorbs more warmup in round 2. A Buildkite annotation scan of ~150 affected builds puts the observed delta at 8.4-13.1 MB. Running five rounds locally showsheapStats().heapSizeflat at ~2 MB and theTimeout/Promise/Function/AsyncGeneratorcounts flat, with later-round RSS deltas going negative on some runs: allocator segment and JIT code-page noise, not retention.The precise regression guard for #12117 is the object-count assertions above the RSS line; the RSS check is documented as a backstop for native leaks
heapStatscannot see. Widen the release bound to 24 MB (~2x the observed max) so a real per-iteration native leak atITER=5000(>= 5 KB/iter) still trips it.node-net.test.ts: await the named-pipe GC check
The Windows-only
should work with named pipestest calledexpectMaxObjectTypeCount(expect, "TCPSocket", before)withoutawait(and readheapStats().objectTypeCounts.TLSSocketas itsTCPSocketbaseline), so the assertion floated and rejected ~1.5 s later inside whatever test happened to be running then. Every other call site intest/awaits it.Dropped from this PR
An earlier revision added an
is_pauseddefer inpackages/bun-usockets/src/loop.cto stoptest-net-write-slow.jsfailing on the macOS arm64 lanes (kqueueEV_EOFdispatchingon_endwhileon_datahad paused the recv loop mid-drain). Three review rounds each found a new level-triggered busy-loop the defer introduces on epoll (paused+shut_downEPOLLHUP, AF_UNIX peer-closeEPOLLHUP, partial-write-re-arms-READABLE +recv()==0), so the targeted approach is the wrong layer. #32257 addresses the whole class by makingrecv()==0the soleeofsource for readable polls on kqueue, which coverstest-net-write-slow.jsand every variant surfaced here; deferring that fix to #32257.The Windows-only timeouts (
test-net-connect-econnrefused.js,test-net-socket-reset-twice.js,node-net-server.test.ts500 ms guards) andhandle-leak.test.tson alpine aarch64 (already addressed by #33622) are also separate.Verification
USE_SYSTEM_BUN=1 bun test test/js/node/net/net-mongodb-pattern-leak.test.tson linux x64: 10/10 pass at the new bound (was ~1/10 fail at 8 MB). Thenode-net.test.tschange is Windows-only and test-only.