Repository navigation
Conversation
When Bun.serve with http3: true fails the UDP bind after the TCP listen has already succeeded, listen() throws and calls Self::deinit directly. The TCP listen socket is still linked into the app's socket group at that point, so App::destroy -> ~TemplatedApp -> httpContext->free() -> us_socket_group_deinit asserts head_listen_sockets == NULL. Close the listener in deinit before destroying the app. In the normal teardown paths self.listener has already been taken by stop_listening, so this only fires on the listen-failure path.
|
Updated 10:36 AM PT - Jun 20th, 2026
❌ @robobun, your commit 6b14be5 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32540That installs a local version of the PR into your bun-32540 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesH3 TCP listener cleanup on UDP bind failure
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #32539 (opened 9 minutes earlier, same root cause, different Fuzzilli fingerprint). Closing in favor of that one. The two fixes differ slightly: #32539 closes the TCP listener at the H3-bind-failure site in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect({ stdout: stdout.trim(), stderr, exitCode, signalCode: proc.signalCode }).toEqual({ | ||
| stdout: expect.stringContaining("threw:Failed to listen on UDP port"), | ||
| stderr: expect.not.stringContaining("head_listen_sockets"), |
There was a problem hiding this comment.
🟡 nit: stderr: expect.not.stringContaining("head_listen_sockets") is the anti-pattern CLAUDE.md:102 calls out — release CI builds compile the assert out so this string never appears even on regression, and in debug an abort already fails exitCode: 0/signalCode: null. The positive assertions (stdout contains threw:Failed to listen on UDP port, exitCode 0, signalCode null) fully prove the fix; if you want stderr in the failure diff for diagnostics, stderr: expect.anything() keeps that without the vacuous check.
Extended reasoning...
What this is
CLAUDE.md line 102 states: "NEVER write tests that check for no "panic" or "uncaught exception" or similar in the test output. These tests will never fail in CI." The new assertion stderr: expect.not.stringContaining("head_listen_sockets") checks for the absence of an assert-failure message in stderr — exactly the pattern the rule targets.
Why the check is dead weight
Walk through both build modes against a regression (TCP listener still linked when App::destroy runs):
- Debug build:
us_socket_group_deinithitsassert(head_listen_sockets == NULL), the process aborts with SIGABRT. In the test'stoEqual,exitCode: 0fails (received non-zero / null) andsignalCode: nullfails (received"SIGABRT"). The stderr matcher would also fail, but it is never the sole failing key — it is strictly redundant with the exit/signal checks. - Release build (CI): the
assert()is compiled out, so the stringhead_listen_socketsis never written to stderr regardless of whether the listener was unlinked.expect.not.stringContaining("head_listen_sockets")is vacuously true. (Whether the regression manifests some other way in release is irrelevant to this matcher — it cannot observe it.)
There is no execution path where this stderr matcher is the assertion that catches the regression. The load-bearing assertions are stdout: expect.stringContaining("threw:Failed to listen on UDP port") (proves Bun.serve threw the expected error and control returned to JS), exitCode: 0, and signalCode: null (prove no abort/crash).
Addressing the "it's there for diagnostics" argument
One could argue stderr is included in the toEqual object so that when the test does fail (e.g. exitCode mismatch), the jest diff prints the actual stderr — which is in the spirit of CLAUDE.md:104 ("expect(stdout).toBe(...) BEFORE expect(exitCode)... gives you a more useful error message"). That's a fair reason to keep stderr as a key in the object. But it doesn't justify the matcher: jest's toEqual diff shows the full received object including the actual stderr value regardless of which asymmetric matcher passed on the expected side. stderr: expect.anything() (or expect.any(String)) preserves the diagnostic-in-diff behavior identically, without encoding a check that reads as if it would catch the assert in CI when it cannot.
Why it's worth flagging despite being minor
The test is not vacuous overall — the other three assertions are correct and will catch a regression — so this is a nit, not a bug. But it's new code that matches a repo-documented anti-pattern verbatim, and these tend to get cargo-culted into future tests. Dropping the line (or swapping the matcher for expect.anything()) costs nothing and keeps the test honest about what it actually verifies.
Suggested fix
Either remove the stderr key entirely, or change it to stderr: expect.anything() if you want stderr surfaced in the failure diff. The latter is probably nicer here given the test spawns a subprocess.
| const dgram = require("dgram"); | ||
| const net = require("net"); | ||
| const tcp = net.createServer(); | ||
| await new Promise(r => tcp.listen(0, "127.0.0.1", r)); | ||
| const port = tcp.address().port; | ||
| tcp.close(); | ||
| await new Promise(r => tcp.on("close", r)); |
There was a problem hiding this comment.
🟡 The TCP net.createServer() / tcp.listen(0) / tcp.close() dance is unnecessary — TCP and UDP port namespaces are independent, so you can bind UDP directly with udp.bind(0, "127.0.0.1", resolve), read udp.address().port, and pass that to Bun.serve. This drops a race window (the freed TCP port can be grabbed before Bun.serve re-binds it) and removes a second one (the specific UDP port might already be in use), aligning with CLAUDE.md's "always use port: 0" guidance.
Extended reasoning...
What the bug is
The new regression test needs a port where (a) UDP is already held by us so Bun.serve's H3 bind fails, and (b) TCP is free so Bun.serve's TCP listen succeeds first. The current implementation achieves this by:
- Binding a TCP server on port 0 → kernel picks port N
- Closing the TCP server
- Binding a UDP socket on port N
- Calling
Bun.serve({ port: N, http3: true, ... })
Steps 1–2 are unnecessary. Because TCP and UDP have independent port namespaces, the test can simply:
- Bind a UDP socket on port 0 → kernel picks port N (UDP is now held)
- Call
Bun.serve({ port: N, http3: true, ... })
Why the current approach is strictly worse
The current code has two race windows:
- Race A (TCP): Between
tcp.close()andBun.serve()'s TCP listen, another process on the CI host can grab TCP port N.Bun.servethen fails with an error that does not contain"Failed to listen on UDP port", so theexpect.stringContaining("threw:Failed to listen on UDP port")assertion fails. - Race B (UDP):
udp.bind(port, ...)binds a specific port N. Since the kernel only guaranteed N was free for TCP, UDP port N might already be in use, causingudp.bindto reject and the test to fail beforeBun.serveis even called.
The simplified approach eliminates Race B entirely (kernel-assigned udp.bind(0) cannot collide) and leaves only the unavoidable "is TCP port N free" check — which is no worse than Race A, since a kernel-assigned UDP ephemeral port is no more likely to collide on TCP than a just-freed TCP port is to be reclaimed.
Step-by-step proof
Consider a busy CI host:
tcp.listen(0)→ kernel assigns port 54321 (TCP).tcp.close()→ TCP 54321 is released back to the ephemeral pool.- Another test process on the same machine calls
listen(0)and the kernel hands it TCP 54321. - Our
udp.bind(54321)succeeds (UDP namespace is separate). Bun.serve({ port: 54321 })tries TCP listen first →EADDRINUSEon TCP, throws something like"Failed to start server. Is port 54321 in use?".stdoutis"threw:Failed to start server...", which does not contain"Failed to listen on UDP port"→ test fails flakily.
With the simplified version, step 2–3 never happen because we never touched TCP 54321; it's just a random ephemeral port that's overwhelmingly likely to be free.
Suggested fix
const dgram = require("dgram");
const udp = dgram.createSocket("udp4");
await new Promise((resolve, reject) => {
udp.on("error", reject);
udp.bind(0, "127.0.0.1", resolve);
});
const port = udp.address().port;
try {
Bun.serve({ port, hostname: "127.0.0.1", tls: ..., http3: true, fetch: () => new Response("x") });
console.log("unexpected-success");
} catch (e) {
console.log("threw:" + e.message);
}
udp.close();This removes the net import and 4 lines of setup while reducing flake surface. CLAUDE.md: "Do not write flaky tests" / "Always use port: 0" — the current approach effectively passes a recycled (just-freed) port to the TCP bind.
Impact
Practical flake probability is very low (~28k ephemeral ports, sub-millisecond window), so this is a code-quality nit rather than a blocker — but it's a strict simplification with no downside.
When
Bun.servewithhttp3: truefails the QUIC UDP bind after the TCP listen has already succeeded,listen()throws and callsSelf::deinit(this)directly. At that point the TCP listen socket is still linked into the uWS app's socket group, soApp::destroy->~TemplatedApp->httpContext->free()->us_socket_group_deinittrips thehead_listen_sockets == NULLassert.Close the listener in
deinitbefore destroying the app. In the normal teardown pathsself.listenerhas already been taken bystop_listening(anddeinit_if_we_cangates on!has_listener()), so this only fires on the listen-failure error path.us_listen_socket_closeis idempotent on already-closed sockets.Repro: hold the UDP port before
Bun.serve({ port, http3: true, tls })so the TCP bind succeeds but the H3 bind fails. Added as a regression test inserve-http3.test.ts.Found by Fuzzilli (fingerprint 897bba1f930ab927).