Skip to content

test: await expectMaxObjectTypeCount in the named-pipe leak check#32093

Open
robobun wants to merge 2 commits into
mainfrom
farm/fbeb058b/await-named-pipe-leak-check
Open

test: await expectMaxObjectTypeCount in the named-pipe leak check#32093
robobun wants to merge 2 commits into
mainfrom
farm/fbeb058b/await-named-pipe-leak-check

Conversation

@robobun

@robobun robobun commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Fixes #32092

Problem

test/js/node/net/node-net.test.ts, inside the Windows-only test should work with named pipes:

const before = heapStats().objectTypeCounts.TLSSocket || 0;
// ... ~700 named-pipe connections ...
expectMaxObjectTypeCount(expect, "TCPSocket", before);

expectMaxObjectTypeCount is async (it polls heapStats() with Bun.sleep between GC passes) but the call is not awaited, the only un-awaited call site in the repo. Two consequences:

  1. The leak assertion is vacuous for the named-pipe test itself: the test completes before the poll finishes.
  2. The floating promise keeps polling across test boundaries, and its eventual failure gets attributed to whichever test is running at that moment. Observed on the Windows lanes of build 61865: later tests failed with expectMaxObjectTypeCount ... Expected: <= 0, Received: 5 despite never calling the helper.

The baseline is also read from the wrong object type: TLSSocket (always 0 in this file) instead of TCPSocket, so the effective bound was "TCPSocket count settles to <= 0".

Fix

  • Read the baseline from objectTypeCounts.TCPSocket.
  • await the call so the assertion actually runs inside this test and cannot poison later ones.
  • Allow before + 10 with a 5s poll window. In build 61865 the count was still 5 over baseline after more than a second of polling; the sibling node-tls-namedpipes.test.ts documents the same behavior (server.close() can resolve before the last accepted pipe socket's finalizer runs) and allows a small absolute margin for it. A genuine leak on this path would leave hundreds of sockets behind (the test opens ~1400), which the now-live assertion will catch.

Verification

The edited test is it.if(isWindows), so Windows CI is the real signal. On Linux, bun bd test test/js/node/net/node-net.test.ts produces identical results with and without this change (the named-pipe test is skipped; the 10 failures in my sandbox are pre-existing container-networking failures that reproduce on main).

The call in 'should work with named pipes' was not awaited, so the
assertion was vacuous for that test and the floating promise's eventual
failure (Expected: <= 0, Received: 5) got attributed to whichever test
happened to be running later on the Windows lanes.

Also read the baseline from the TCPSocket count (it read TLSSocket, which
is always 0 in this file) and allow a small straggler margin over the
baseline, matching node-tls-namedpipes.test.ts: server.close() can resolve
before the last accepted pipe socket's finalizer runs.
@robobun

robobun commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 12:44 AM PT - Jun 11th, 2026

@robobun, your commit 6ed3ec1 has 2 failures in Build #61878 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 32093

That installs a local version of the PR into your bun-32093 executable, so you can run:

bun-32093 --bun

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e39b4529-01a4-4ede-958f-3797df792d63

📥 Commits

Reviewing files that changed from the base of the PR and between f8723b1 and c945852.

📒 Files selected for processing (1)
  • test/js/node/net/node-net.test.ts

Walkthrough

The PR updates the Windows named-pipes test in test/js/node/net/node-net.test.ts to fix an issue where an un-awaited async assertion was polluting unrelated subsequent tests. The baseline heap object count is switched from TLSSocket to TCPSocket, and the assertion is relaxed to permit up to 10 straggler socket instances instead of requiring exact equality.

Changes

Named-pipes test reliability fix

Layer / File(s) Summary
Named-pipes test heap object assertion
test/js/node/net/node-net.test.ts
Baseline heap measurement switches from TLSSocket to TCPSocket, and the leak assertion is relaxed to allow up to before + 10 TCPSocket instances instead of requiring equality, with explanatory comments about async finalization and stragglers.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: awaiting the expectMaxObjectTypeCount call in a named-pipe leak check test.
Description check ✅ Passed The description thoroughly addresses the problem, fix, and verification against the template sections (What does this PR do, How did you verify).
Linked Issues check ✅ Passed The PR directly addresses all requirements from #32092: awaits expectMaxObjectTypeCount, reads baseline from correct object type (TCPSocket), allows small margin for late finalizers, and isolates the assertion within the test.
Out of Scope Changes check ✅ Passed All changes are narrowly scoped to fixing the named-pipe leak check test as outlined in #32092; no unrelated modifications are present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@robobun

robobun commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: this PR only changes code inside an it.if(isWindows) test in test/js/node/net/node-net.test.ts, and every Windows test lane passed on both builds 61870 and 61878 (x64, x64-baseline, aarch64), so the changed test is verified green with the awaited assertion.

The only hard failures on both builds are on debian Linux lanes this diff cannot execute on:

  • test/js/web/streams/streams-leak.test.ts:39, a wire-coalescing timing assertion (chunks.length > 20, got 8-20 across attempts), failed on 13 x64 in 61870 and 13 x64-baseline in 61878.
  • test/js/third_party/@duckdb/node-api/duckdb.test.ts segfault on 13 x64-baseline in 61878; the identical failure appears on unrelated branches in builds 61857 and 61863.

Not pushing further retriggers; ready for review/merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

node-net.test.ts: un-awaited expectMaxObjectTypeCount in named-pipe test fails unrelated subsequent tests on Windows

1 participant