Skip to content

server: close TCP listener when H3 UDP bind fails - #32539

Closed
robobun wants to merge 3 commits into
mainfrom
farm/ba03fc03/h3-listen-fail-cleanup
Closed

robobun wants to merge 3 commits into
mainfrom
farm/ba03fc03/h3-listen-fail-cleanup

Conversation

@robobun

@robobun robobun commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

Fuzzilli found an assertion failure in us_socket_group_deinit:

bun-debug: packages/bun-usockets/src/context.c:67: void us_socket_group_deinit(struct us_socket_group_t *): Assertion `group->head_listen_sockets == ((void*)0)' failed.

Root cause

When Bun.serve({ http3: true, tls }) successfully binds the TCP listen socket but the UDP bind for QUIC fails on the same port, the error path throws and then calls deinit(), which destroys the uws App (~TemplatedApp -> httpContext->free() -> us_socket_group_deinit). That path never runs TemplatedApp::close(), so the TCP listen socket is still linked into group->head_listen_sockets when the group is torn down, tripping the assertion in debug/ASAN builds and leaking the fd + listen socket struct in release.

The port-0 retry path already closed the TCP listener; the fix extends that to the final failure path so the socket is unlinked before deinit() destroys the app.

Repro

const dgram = require("node:dgram");
const udp = dgram.createSocket("udp4");
udp.bind(0, "127.0.0.1", () => {
  const port = udp.address().port;
  Bun.serve({ port, hostname: "127.0.0.1", tls, http3: true, fetch() {} });
});

Fingerprint: f384476a45a89910

When Bun.serve({ http3: true, tls }) successfully binds the TCP listen
socket but the subsequent UDP bind for QUIC fails, the error path called
deinit() which destroys the uws App without running TemplatedApp::close().
The TCP listen socket was still linked into the socket group at that point,
tripping the head_listen_sockets == NULL assertion in us_socket_group_deinit
(and leaking the fd in release builds).

The retry path already closed the listener; extend that to the final
failure path so the socket is unlinked before the app is destroyed.
@robobun

robobun commented Jun 20, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:33 AM PT - Jun 20th, 2026

❌ @robobun, your commit d101c63 has 1 failures in Build #63659 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 32539

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

bun-32539 --bun

@coderabbitai

coderabbitai Bot commented Jun 20, 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: add640f1-6e7b-4ed3-9e0f-ba2e1a4dfd57

📥 Commits

Reviewing files that changed from the base of the PR and between 6c101f9 and d30d299.

📒 Files selected for processing (1)
  • test/js/bun/http/serve-http3.test.ts

Walkthrough

The error-handling path in NewServer::listen() is reordered so that the TCP listen socket is always taken and closed when H3 UDP listener creation fails, regardless of whether a retry will occur. A new test validates this by pre-binding a UDP port and asserting Bun.serve with http3: true throws a clean UDP listen error.

Changes

H3 UDP Bind Failure: Cleanup Fix and Test

Layer / File(s) Summary
TCP socket cleanup on H3 UDP bind failure
src/runtime/server/mod.rs
self.listener.take() + close() is moved before the attempt < max_attempts guard, so the TCP socket is unconditionally released whenever H3 UDP listener creation fails, not only when a retry will follow.
Test: UDP bind failure throws clean error
test/js/bun/http/serve-http3.test.ts
New test pre-binds a UDP socket on an ephemeral port, spawns a script that calls Bun.serve with http3: true on the same port, and asserts a throw with "Failed to listen on UDP port" while the process exits with code 0.

Possibly related PRs

  • oven-sh/bun#30741: Modifies the same NewServer::listen() H3 startup and retry/error-handling path in src/runtime/server/mod.rs, including renaming config.h3 to config.http3 and updating gating variables and error messages.
🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The description provides root cause analysis, reproduction steps, and the technical fix, but lacks explicit sections matching the template structure. Restructure the description to explicitly include 'What does this PR do?' and 'How did you verify your code works?' sections as defined in the template.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: closing the TCP listener when H3 UDP bind fails, which directly addresses the root cause described in the PR.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/bun/http/serve-http3.test.ts`:
- Around line 1095-1100: The success path where console.log("unexpected
success") is called does not actually fail the test or exit, which allows a live
server to remain and hang until timeout. Add a process.exit(1) call or throw an
error immediately after the console.log("unexpected success") statement to
terminate the test deterministically with a non-zero exit code instead of
allowing execution to continue.
🪄 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: df7bc14a-927f-444b-8436-195e52163592

📥 Commits

Reviewing files that changed from the base of the PR and between 8841747 and 6c101f9.

📒 Files selected for processing (2)
  • src/runtime/server/mod.rs
  • test/js/bun/http/serve-http3.test.ts

Comment thread test/js/bun/http/serve-http3.test.ts Resolved

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — minimal error-path reorder so the TCP listener is closed before deinit when the H3 UDP bind fails.

Extended reasoning...

Overview

This PR reorders a single if nesting in NewServer::listen() (src/runtime/server/mod.rs, ~6 effective lines). Previously, when the H3 UDP bind failed, the already-bound TCP listen socket was only take()'d and close()'d on the retry path (attempt < max_attempts); on the final-failure path it was left linked into the uws socket group, tripping a debug assertion in us_socket_group_deinit and leaking the fd in release. The fix hoists the listener.take() + close() above the retry guard so it runs unconditionally, then gates only the continue on attempt < max_attempts. Retry semantics are preserved exactly (still requires both Some(ls) and attempt < max_attempts). A new subprocess regression test in serve-http3.test.ts pre-binds a UDP port and asserts Bun.serve({ http3: true }) throws cleanly with "Failed to listen on UDP port" and exits 0.

Security risks

None. This is pure resource cleanup on an error path. No auth, crypto, input parsing, or user-controlled data flow is touched. The success path is unchanged.

Level of scrutiny

Low. The change is confined to the H3-UDP-bind-failure branch, which is already a throw-then-deinit error path. The reorder is mechanical and easy to verify by inspection: the only behavioral delta is that the listener is now closed when attempt >= max_attempts, which is exactly the stated fix. The http1: false case (where listener is already None) falls through unchanged.

Other factors

The bug-hunting system found no issues. CodeRabbit's only suggestion (fail-fast on unexpected test success) was applied in d30d299 and confirmed resolved. No CODEOWNERS rules cover these files. The test follows existing conventions in the file (subprocess via bunExe() -e, combined {stdout, stderr, exitCode} assertion). The robobun build failures on 6c101f9 span every platform including build-cpp (which this PR doesn't touch), pointing to an infra/base-commit issue rather than this change; CI will gate the merge regardless.

@robobun

robobun commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator Author

CI failures across both runs are unrelated darwin-aarch64 flakes that also appear on other recent builds:

Build Failed test Also seen on
63650 test/js/node/test/parallel/test-tls-client-destroy-soon.js 63640, 63642, 63652, 63657
63650 test/integration/next-pages/test/dev-server.test.ts (puppeteer download) 63642
63659 test/js/web/fetch/fetch-leak.test.ts (RSS threshold) 63645, 63652

284/286 jobs passed on both runs, including all 20 ASAN shards. serve-http3.test.ts is green everywhere. None of these tests reach the H3 listen error path this PR touches.

@robobun

robobun commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator Author

Stale PR review: keep open, rework.

The bug is real and still on main. When Bun.serve({ tls, http3: true, port }) binds TCP and the UDP bind then fails on the last attempt, listen() throws with listener still set (src/runtime/server/mod.rs:3159), and deinit() (mod.rs:2095) destroys the app without closing it. On 1.4.3-canary the call throws Failed to listen on UDP port N for HTTP/3, and a TCP bind on the same port then fails with EADDRINUSE. A debug build aborts on US_ASSERT(group->head_listen_sockets == NULL) (packages/bun-usockets/src/context.c:58). The source hunk here is the right fix and it still applies to main. That branch is the only place where listen() reaches deinit() with a live listener: every other Self::deinit(this) runs before listen_with_config (mod.rs:3119), and on_listen_failed clears listener (mod.rs:1989).

The test is not valid as written, so the PR needs a rework before it can merge:

  1. The test asserts only the throw text and exit code 0. An unfixed release build produces exactly that, so the test passes without the fix. Assert the contract instead. Hold UDP port N with node:dgram, catch the throw from Bun.serve, then bind TCP port N again and expect success. The test in Bun.serve: close listen socket before destroying app in deinit() #32543 does this.
  2. Move the test inside the describe("Bun.serve HTTP/3") block in test/js/bun/http/serve-http3.test.ts. The append at the end of that file is the only conflict with main.

#32543 fixes the same crash one layer down in deinit(). Only one of the two needs to land.

@robobun

robobun commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-06-20, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.

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.

1 participant