test(serve): create the websocket client outside the scope that holds the server

[robobun]

Problem

• bun-server.test.ts "server stays alive while a websocket is connected, then collects after close" fails on every Windows x64 run since b7b4ddd (Remove dead code from simdutf_sys, ncrypto, js_parser, bun_install, built-in JS, bindgen, uSockets, and 66 Cargo manifests #39732): afterClose is 2, expected 1.
• The server survives through the client's onclose handler, whose scope holds server. A GC debugging snapshot shows no retainer for the handler. Its address sits in a slot of uv_run's frame that nothing writes, and uv_run is live during every GC the test runs. Remove dead code from simdutf_sys, ncrypto, js_parser, bun_install, built-in JS, bindgen, uSockets, and 66 Cargo manifests #39732 moved the stale copy there. The test structure is the latent cause.

Fix

• Create the client and its handlers in a function outside the scope that holds server. The stale pointer then retains nothing the test measures. The server side of the test is unchanged.
• Correct because the surviving reference came from the test, not from the code under test. whileConnected > baseline still holds through the ServerWebSocket root.
• Verified with the b7b4ddd Windows x64 CI bun.exe: main's test fails 3 of 3 runs, the fixed test passes 5 of 5, the whole file passes. The gate cannot show the failure: it needs the Windows release layout.

Background

• JSC scans the native stack conservatively. sanitizeStack zeroes dead stack below the stack pointer, not a stale word inside a live frame.
• A function keeps the scope it was created in alive, so a pointer to any function created next to server keeps server alive.
• The close event runs from a tick-level task, so its callee's address lands where the next tick's uv_run frame sits. Server-side handlers run inside uv_run, deeper.

Notes

Evidence, taken with the b7b4ddd Windows x64 CI artifacts (bun.exe is byte-identical to bun-profile.exe, so its PDB applies). Every step ran in the same synchronous JS turn as a fullGC() that kept the server alive.

1. require("bun:jsc").generateHeapSnapshotForDebugging(): the retained DebugHTTPServer instance has one incoming edge, Variable "server" from a JSLexicalEnvironment. That environment's only live referrer is the Function of the client's onclose handler (named clientOnClose for the experiment). The function has zero incoming edges and no roots entry. Neither the server cell nor the environment is on the stack.

2. Stack walk (frame records through the JS frames, then RtlLookupFunctionEntry and RtlVirtualUnwind), 30 frames from the ffi host function to the thread start, symbolized with llvm-symbolizer against the PDB. The function's address is on the live stack exactly once, inside frame 16: uv_run, vendor/libuv/src/win/core.c:815 (the uv__run_timers call after poll), frame size 0x1110, word at frame offset 0xa40. Below it: uv__run_timers, the Bun.sleep timer fire, EventLoop::exit draining microtasks, runInternalMicrotask, asyncModuleExecutionResume, the test's JS. Above it: tick_with_timeout, wait_for_promise, run_command, main.

3. Disassembly of uv_run: 8 pushes plus 0x10c8 bytes, 0x1110 in total, matching the walk. The inlined uv__poll passes its OVERLAPPED_ENTRY overlappeds[128] to GetQueuedCompletionStatusEx at rsp+0x70 with count 0x80. Offset 0xa40 is array byte 0x9d0: entry 78, field offset 16, Internal. The array is never initialized. Only a poll that returns 79 or more completions writes that slot.

4. The fixed script on the same binary collects the server on the first attempt, 8 of 8 runs. The stale copy of the close handler is still on the stack (in that stack shape it sits in runInternalMicrotask's frame). It now retains only the handler and its own scope. This is why the fix is in the test and not in libuv: the array is the largest window, not the only one.

5. The 1b88ad3 binary passes both versions. Main build 102067 (1b88ad3) was green on this lane, 102154 (b7b4ddd) red, and Remove dead code from simdutf_sys, ncrypto, js_parser, bun_install, built-in JS, bindgen, uSockets, and 66 Cargo manifests #39732's own builds 102053 and 102077 were red on it before merge.

6. Whole-file run with the fixed file and the b7b4ddd binary: 79 pass, 1 skip, 0 fail. On Linux, bun bd test test/js/bun/http/bun-server.test.ts -t "handler GC tracing": 10 pass.

#39858 changes the same test differently and does not address the chain above. #39882 adds the diagnosis rule this followed. A runtime mitigation (for example a sanitizeStackForVM call from the event loop before it polls) would have a per-tick cost and is a separate decision.

[coderabbitai]

Warning

Review limit reached

Your included review limit has been reached.

You’re in a promotional period — use the checkbox below to run this review for free:

• Run review for free

_{On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file.}

How can I continue?

Run this review now using the option above, or comment @coderabbitai review --use-credits.

You can also wait for the limit to reset (next review available in 8 minutes), then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 15e2d1ed-2750-40da-b69b-bfc878db1203

📥 Commits

Reviewing files that changed from the base of the PR and between 7c9a51e and 3c1ac84.

📒 Files selected for processing (1)

• test/js/bun/http/bun-server.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

LGTM — test-only de-flake that moves the WebSocket client handlers out of the lexical scope containing server, so a stale conservative-stack pointer to onclose no longer retains the server.

What was reviewed:

• Confirmed connect() is defined in the outer scope (with client/clientOpen/echoed/closed), so its closures no longer share server's JSLexicalEnvironment.
• Assertions are unchanged (whileConnected > baseline, echo dispatch, afterClose === baseline) — the property under test is preserved, only the test's own retention chain is cut.
• Checked that client is still assigned for the post-IIFE send/close/= null sequence.

Extended reasoning...

Overview

This PR touches a single test in test/js/bun/http/bun-server.test.ts — the "server stays alive while a websocket is connected, then collects after close" case inside the "handler GC tracing (heapStats wrapper-count)" describe block. The change hoists the client WebSocket construction and its onopen/onmessage/onclose handlers into a connect(url) function declared outside the async IIFE that holds const server. Previously the handlers were created inline next to server, so they shared its lexical environment; the PR description traces (via heap snapshot, native stack walk, and uv_run disassembly) a stale pointer to the onclose closure sitting in an uninitialized slot of uv_run's OVERLAPPED_ENTRY overlappeds[128] on Windows x64, which the conservative scanner then treats as a root — keeping server alive and making afterClose read 2 instead of baseline.

Security risks

None. This is a test-only refactor of an inline bun -e script; no runtime code, no inputs, no network surface, no auth/crypto paths are touched.

Level of scrutiny

Low-to-moderate. It's a de-flake, and REVIEW.md flags de-flakes for "keep asserting the property the original assertion protected." That holds here: the three assertions (whileConnected > baseline, the echo regex proving the cycle-captured message handler dispatched, and afterClose === baseline) are byte-identical. The server side of the test — including the message handler that intentionally closes over server to exercise the ServerWebSocket → m_server root — is unchanged. Only the client-side retention chain, which is test scaffolding rather than the code under test, moves.

Other factors

The root-cause analysis is unusually rigorous (heap-snapshot edge, PDB-symbolized frame offset, array slot arithmetic), and the fix matches the diagnosis rather than papering over it with extra GC attempts or a loosened assertion. The updated comments explain the scoping constraint for future readers. No prior review comments to address; no runtime behavior change to require new test coverage. The PR notes that #39858 touches the same test differently and #39882 adds a related diagnosis rule — those are separate decisions and don't block this change.

[robobun]

^{Updated 12:55 AM PT - Aug 21st, 2026}

❌ @robobun, your commit 3c1ac84 has 2 failures in Build #102455 (All Failures):

• test/js/node/test/parallel/test-cluster-shared-leak.js - timeout on 🪟 11 aarch64
• test/bake/deinitialization.test.ts - pid 728 segmentation fault at address 0x0 on 🪟 2019 x64

---

🧪   To try this PR locally:

bunx bun-pr 39893

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

bun-39893 --bun

[claude]

LGTM — test-only de-flake that moves client handler creation out of the server's lexical scope so a stale conservatively-scanned stack pointer to onclose no longer transitively retains server.

What was reviewed:

• Confirmed the handlers now close over the outer scope (client, clientOpen, echoed, closed) only — connect is defined outside the async IIFE, so its activation's parent scope excludes server.
• Checked that all assertions are preserved unchanged (whileConnected > baseline, echo dispatch, afterClose === baseline) — the test still proves the ServerWebSocket → JSServer root keeps the server alive and releases after close.
• Verified the server-side setup and server.stop() graceful path are untouched.

Extended reasoning...

Overview

This PR touches a single test in test/js/bun/http/bun-server.test.ts — the "server stays alive while a websocket is connected, then collects after close" case inside the "handler GC tracing (heapStats wrapper-count)" describe block. The change extracts WebSocket client construction and its onopen/onmessage/onclose handlers into a connect(url) function declared in the outer test scope, replacing four inline lines inside the async IIFE that holds server. Comments are updated to explain why. No production code, no assertion, and no server-side wiring is changed.

Security risks

None. Test-only refactor of closure placement inside a spawned bun -e script. No auth, crypto, network egress, or user-facing surface touched.

Level of scrutiny

Low-to-moderate. It is a small mechanical restructuring of a GC-sensitive test, but the class of change (de-flaking a heapStats-based test by adjusting scope chains) is exactly where the review guide warns against silently weakening a test. I verified that:

• The property under test — that the connected ServerWebSocket roots the server wrapper across a full GC, and that closing the last ws lets it collect — is still asserted identically at bun-server.test.ts:3252-3254.
• The message handler still closes over server (server.port + ":" + m), so the intended cycle through wsHandlers is preserved.
• connect is a function declaration in the outer scope, so the arrow handlers it creates have a scope chain of connect's activation → outer test scope, which does not include the IIFE's environment record holding server. This is precisely the fix the PR description derives from the heap snapshot and stack-walk evidence.

Other factors

The PR description demonstrates the mechanism end-to-end (heap snapshot edge, uv_run frame offset into the uninitialized OVERLAPPED_ENTRY[128], before/after runs on the exact failing b7b4ddd Windows binary), which satisfies the "prove the mechanism, don't correlate" bar. The fix is placed at the layer that owns the defect — the test's own scope structure created the spurious retention path, not the runtime. The added comments are long but explain a genuinely non-obvious conservative-stack-scan interaction that a future reader would otherwise spend significant effort rediscovering. No prior reviews or outstanding comments on the PR.

[robobun]

CI: the Windows x64 test lane passes bun-server.test.ts in both builds of this PR (102434: 8 of 8 shards green; 102455: the one red shard is a segfault in test/bake/deinitialization.test.ts, unrelated to this change). The remaining red lanes are unrelated to this diff and are reported separately: fetch-abort-slow-connect.test.ts on darwin aarch64 (102434), deinitialization.test.ts on Windows x64 and test-cluster-shared-leak.js on Windows aarch64 (102455). The diff itself is ready.