node:tls: keep every addContext() entry for the next listen()

[robobun]

Problem

• server.addContext(name, ctx) on a listening tls.Server is lost after server.close() + server.listen(). The new listener serves the default certificate, or the older one from an addContext(name, old) before listen(). Node v26.3.0 keeps the newest entry.
• addContext (src/js/node/tls.ts:1244) only calls the native addServerName when a listener exists. It skips the contexts map, and each listen() loads that map into the new listener. The first addContext() creates the map, so a cluster worker's pending listen() holds null and misses a later entry.
• addContext("") does not throw Node's ERR_TLS_REQUIRED_SERVER_NAME. Before listen() the empty name is kept, and listen() fails with hostname pattern cannot be empty.

Fix

• addContext always records the entry in contexts, after the live listener accepted it. The map exists from construction, so a pending listen() shares it. A re-added name moves to the end, so listen() replays the calls in order.
• A falsy servername throws the new ERR_TLS_REQUIRED_SERVER_NAME (ErrorCode.ts, ErrorCode.cpp) with Node's message, before anything is recorded.
• Not in this PR: a socket wrapped through server.emit("connection") still gets the default certificate (no SNI tree on that path, cluster: hand TLS workers their connections on Windows instead of sharing the listening socket #37896).
• Verified: 5 new tests in test/js/node/tls/node-tls-server.test.ts, each fails without the fix. Also node-tls-context.test.ts and 8 upstream test-tls-* tests.

Background

• addContext(name, ctx) adds a certificate that the server presents when the client's SNI name matches name.
• Node keeps the entries in server._contexts and checks them newest first.
• Bun matches SNI natively. Each listener (Bun.listen) owns an SNI tree. listen() fills it from the contexts map, addServerName updates it, and close() destroys it.

Notes

Repro on bun 1.4.3 canary and on main at 630e921 (agent1 is the default certificate):

added while listening    : agent2
after close() + listen() : agent1   (node v26.3.0: agent2)

replaced while listening : agent3
after close() + listen() : agent2   (node v26.3.0: agent3)

cluster worker, listen() then addContext() : agent1   (node v26.3.0: agent2)

addContext("") before listen() : no throw, then listen() emits 'hostname pattern cannot be empty'
addContext("") while listening : TypeError 'hostname pattern cannot be empty'
node v26.3.0, both             : Error ERR_TLS_REQUIRED_SERVER_NAME '"servername" is required parameter for Server.addContext'

Tests:

• The 4 map tests fail on a debug build of the base commit and on the canary, on Linux x64 and on Windows x64. They pass on a debug build of this branch on both. The cluster test passed 12 of 12 runs on Windows.
• The ERR_TLS_REQUIRED_SERVER_NAME test replaces the earlier test for a rejected empty name. It checks "", undefined and null before listen(), "" while listening, and that the next listen() works. The same assertions pass on node v26.3.0. Checked on Linux x64 only.
• No $delete before $set: the call order test fails.
• The map write comes after addServerName. With the new guard, the live listener rejects only a name with more than 10 labels whose twin is registered (tls.Server: a listen() that fails while it loads addContext() entries leaves the listener bound and accepting #43082). Node accepts that input, so no test pins the order for it.
• The branch includes main at b7ea95a. tsc --noEmit -p src/js/tsconfig.json passes.
• SNICallback runs even when the requested servername matches the bind hostname in the same file fails in my Linux container with and without this change. localhost resolves to ::1 for the listener and to 127.0.0.1 for the client there. It passes on Windows.

Details:

• addServerName already replaces an existing entry for the same name (Listener::add_server_name removes, then adds). The live listener and the map agree that the last call wins.
• The map is keyed by name, so repeated addContext() calls for one name do not grow it. Node's _contexts array grows with every call.
• The native SNI tree splits a name on . and ignores an empty last label, so a.example. and a.example are one entry. With the calls a.example, a.example., a.example, the old map order replayed the second call last. Node serves the third.
• The servername guard is Node's if (!servername) (https://github.com/nodejs/node/blob/v26.3.0/lib/internal/tls/wrap.js#L1571-L1574). It runs before the existing hostname must be a string check, as in Node. node:tls: build the tls.Server SecureContext in setSecureContext() #42050 carries the same lines for the guard and the error code. The PR that lands second drops its duplicate.
• A listen() that fails while it loads an entry leaves the listener bound and accepting. Main does the same for entries added before listen(). tls.Server: a listen() that fails while it loads addContext() entries leaves the listener bound and accepting #43082 tracks it, with a fix open in node:net: stop the listener when listen() fails after the socket is bound #43093. TLS SNI: names with more than 10 labels are added to the SNI tree but never found or removed #43092 tracks the label limit of the SNI tree. An entry that the live listener rejects is not recorded, so it cannot cause that. One new sequence reaches it: a name with more than 10 labels added while the server listens, then close(), then the same name with a trailing dot, then listen(). The native tree cannot replace a name with more than 10 labels, and such a name never matches a handshake.
• addContext uses the $delete and $set Map intrinsics, as src/js/CLAUDE.md asks.
• Node source for _contexts: https://github.com/nodejs/node/blob/v26.3.0/lib/internal/tls/wrap.js#L1571-L1611
• Cluster faux handle: a tls.Server cannot reach it on main. listenInCluster sets sharedOnly for TLS, so a TLS worker always gets a native listener, and addServerName never sees the faux handle.

Overlap with open PRs:

• cluster: hand TLS workers their connections on Windows instead of sharing the listening socket #37896 (Windows TLS cluster workers, open since 2026-08-12) rewrites addContext for wrapped connections and carries the same map change inside a larger one. This PR is the standalone fix. The PR that lands second needs a small rebase in addContext.
• node:tls: make Server.setSecureContext() replace the listening socket's TLS context #43015 (setSecureContext() on a listening server) reads [buntls]() again when the primary answers a worker's listen(), and its cluster fixture also covers listen() then addContext(). Both changes work together. The PR that lands second can drop its duplicate of that test.
• Not changed: node:https: add addContext and other tls.Server methods to https.Server #32435 (https.Server#addContext), node:tls: keep ALPN across an SNI SecureContext selection #33253 (ALPN on SNI contexts), tls: select the certificate by server name for connections accepted before close() / graceful stop() #42355 (SNI for connections accepted before close()).

---

no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/tls/node-tls-server.test.ts

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: bdfd9cca-a0d0-43ef-b46e-e4df32ad5a57

📥 Commits

Reviewing files that changed from the base of the PR and between b52d513 and 5236135.

📒 Files selected for processing (2)

• src/js/node/tls.ts
• test/js/node/tls/node-tls-server.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.

---

Walkthrough

Server.addContext() now preserves SNI contexts across listener lifecycles, maintains duplicate-pattern order, and records only accepted contexts. Tests cover relistening, replacement, rejection, and cluster-worker behavior.

Changes

addContext lifecycle

Layer / File(s)	Summary
Context tracking and replay src/js/node/tls.ts	Server now maintains an ordered context map. addContext() updates live listeners before recording contexts and moves replaced hostnames to the end.
Lifecycle and cluster validation test/js/node/tls/node-tls-server.test.ts	Tests cover relistening, live replacement, rejected contexts, duplicate-pattern order, and contexts added after a cluster worker starts listening.

Suggested reviewers: jarred-sumner

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 52361

The TLS context lifecycle change has no identified actionable merge risk in the supplied evidence.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly identifies the main change: preserving every addContext() entry for the next listen().
Description check	✅ Passed	The description provides the problem, fix, verification details, scope limits, and relevant background. It does not use the template headings exactly, but it contains the required information in equiv…

---

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

[robobun]

Status

Reproduced on bun 1.4.3 canary and on a debug build of main, on Linux x64 and on Windows x64. agent1 is the default certificate:

const server = tls.createServer(agent1, s => s.end());
server.listen(0, "127.0.0.1");           // wait for 'listening'
server.addContext("a.example", agent2);  // "a.example" gets agent2
server.close();                          // wait for 'close'
server.listen(0, "127.0.0.1");           // "a.example" gets agent1 (node v26.3.0: agent2)

server.addContext("", agent2) before listen() does not throw, and listen() then fails with hostname pattern cannot be empty. Node v26.3.0 throws ERR_TLS_REQUIRED_SERVER_NAME from addContext().

• Run bun bd test test/js/node/tls/node-tls-server.test.ts -t "addContext\(\) entries apply to every listen". Without the changes in src/, all 5 new tests fail. With them, all 5 pass.
• Self-reviewed: 3 concerns raised, 3 addressed. The PR body names the overlap with cluster: hand TLS workers their connections on Windows instead of sharing the listening socket #37896, node:tls: make Server.setSecureContext() replace the listening socket's TLS context #43015 and node:tls: build the tls.Server SecureContext in setSecureContext() #42050, and it states what this PR does not change.
• Review: the ERR_TLS_REQUIRED_SERVER_NAME request is done in c6cd813. tls.Server: a listen() that fails while it loads addContext() entries leaves the listener bound and accepting #43082 tracks the listen() failure state that the review found on main.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked whether re-adding a hostname on a live listener or replaying the map on the next listen() could double-register or leak an SSL_CTX ref — Listener.add_server_name (src/runtime/socket/Listener.rs:798-799) removes the existing entry before adding, and close() tears down the old SNI tree, so each listener ends up with one entry per name.

Extended reasoning...

The inline findings cover the intrinsic-call convention and the re-listen replay failure path. Separately, the native add_server_name path in src/runtime/socket/Listener.rs does a remove_server_name before add_server_name, so a repeated addContext() for the same name while listening replaces rather than appends and the previous SSL_CTX reference is released; the contexts replay in net.ts:4010-4014 runs against a freshly created handle, so entries added while listening are not registered twice on the same SNI tree. Nothing in this run contradicts the PR's ordering claim (delete-then-set keeps last-write-wins order matching the native tree's last-add-wins).

[claude]

LGTM — the follow-up commit switches to the $delete/$set intrinsics, and the runtime change is a small, self-contained bookkeeping fix that matches Node's _contexts semantics.

What was reviewed:

• The always-present contexts Map against its only consumer (kRealListen in net.ts iterates it after _handle is set) — an empty map is a no-op there, so servers that never call addContext() are unaffected.
• Ordering of addServerName() before the map write, so a name the live listener rejects is not replayed on the next listen(); the delete-then-set keeps call order for a re-added name.
• The five new tests assert exact CNs via toEqual on a single object, drain subprocess pipes concurrently, and the cluster fixture covers the listen()-before-handle window.

Extended reasoning...

Overview

The PR changes ~5 lines of src/js/node/tls.ts inside Server's addContext: contexts becomes an always-present Map created at construction, and every addContext() call forwards to the live handle via addServerName() (if one exists) and then records the entry with contexts.$delete(hostname); contexts.$set(hostname, context). The map is handed to net.ts through this[buntls]() and replayed into each new native listener in kRealListen. The rest of the diff is a new describe block in test/js/node/tls/node-tls-server.test.ts with five tests using the agent1/2/3 fixture certificates.

Security risks

None introduced. The change only affects which SNI certificate the server presents for a name the application itself registered; no trust decisions, verification flags, or key material handling change. Calling the native addServerName() before recording an entry means a rejected name is never persisted, which is strictly safer than the previous ordering for names added before listen(). The nit from the prior review (public Map.prototype methods in a builtin) was addressed in the second commit by using the $-intrinsics.

Level of scrutiny

Low-to-moderate. The runtime diff is tiny and the intent is clear: mirror Node's server._contexts so entries survive close()/listen() and land in a cluster worker's deferred listener. I traced contexts to its single consumer in net.ts (if (contexts) for (const [name, context] of contexts) addServerName(...)), which handles an empty map identically to the old null. The native add_server_name already replaces an existing entry for the same name, so the live listener and the map agree that the last call wins. No CODEOWNERS entry covers either file.

Other factors

The tests assert exact certificate CNs with toEqual on a single object, wire cleanup through finally/await using, use port: 0, and cover the four behavioral clauses the PR claims (survive relisten, replacement survives relisten, rejected entry not kept, call order preserved) plus the cluster-worker window. The previously noted pre-existing issue (a replay failure in kRealListen leaving the handle open for names with >10 labels) predates this PR and is not made worse in any realistic case. The bug hunt exited on dry_streak with no findings, and the only change since the prior review is the intrinsic fix that resolved the earlier inline nit.

Still open from earlier reviews (1):

• Unresolved: 1 minor or pre-existing.

[robobun]

^{Updated 5:02 PM PT - Sep 21st, 2026}

✅ @robobun, your commit c6cd8133f8db8f84ca84fcc7125c3fef50b465f3 passed in Build #119442! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 43080

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

bun-43080 --bun

[claude]

LGTM — the $delete/$set intrinsics nit from the earlier pass is addressed, and the remaining change is a small, well-tested bookkeeping fix in addContext().

What was reviewed:

• The write order in addContext(): addServerName on the live handle runs before the map write, so a natively rejected name (empty hostname) never reaches the next listen() replay.
• The contexts consumer in net.ts (setupListenHandle, ~line 4010): it already tolerates a Map and the if (contexts) guard is still fine with an always-present, possibly empty, Map.
• The five new tests: each awaits listening/close/secureConnect events rather than sleeping, the cluster test drains stdout/stderr concurrently, and every server is closed in finally.

Extended reasoning...

Overview

The PR changes nine lines in src/js/node/tls.ts so that tls.Server#addContext() always records the entry in the per-server contexts Map (created at construction instead of lazily), after first pushing it to a live native listener when one exists. net.ts already replays that Map into each new native handle on listen(), so the fix makes entries added while listening survive close() + listen() and reach a cluster worker whose handle is created asynchronously. The test file adds a describe block with four in-process relisten tests and one cluster-worker subprocess test.

Security risks

No new attack surface. The change only affects which already-configured certificate is served for an SNI name; it does not touch TLS verification, credentials, or the native SNI matching. The map is closure-private and, after the follow-up commit, is mutated only through $delete/$set intrinsics, so Map.prototype tampering cannot affect it. The delete-before-set ordering is what makes the replay honour node's last-call-wins semantics for names the native tree collapses (a.example vs a.example.), and the test for call order covers that.

Level of scrutiny

Low to moderate. The code change is small and self-contained, the consumer side in net.ts was inspected and needs no change, and the behavior matches node's _contexts handling. The one previously noted pre-existing concern (a throw during listen() replay leaving the handle bound) is unchanged in scope by this PR and was already marked non-blocking; it does not warrant holding this fix.

Other factors

Both prior review comments led to a follow-up commit: the intrinsics nit was fixed and the comments were shortened. The tests await observable events rather than timers, drain subprocess pipes concurrently, use tempDir/bunExe/bunEnv, and close resources in finally. The bug-hunting run exited on a dry streak with no findings, and the diff stat confirms only the two listed files changed.

[cirospaciari]

@robobun On Node v26.3.0 server.addContext("", ctx) throws ERR_TLS_REQUIRED_SERVER_NAME with the message "servername" is required parameter for Server.addContext (lib/internal/tls/wrap.js:1572), but the test "an entry that the live listener rejects is not kept for the next listen()" expects Bun's "hostname pattern cannot be empty". Please throw Node's code and message so the test passes on Node and Bun.
Repro: node -e 'const s=require("tls").createServer(()=>{}).listen(0,()=>{try{s.addContext("",{})}catch(e){console.log(e.code,e.message)}s.close()})'.

[robobun]

@cirospaciari Done in c6cd813.

• addContext() now starts with Node's check (if (!servername)). It throws ERR_TLS_REQUIRED_SERVER_NAME with the message "servername" is required parameter for Server.addContext. The code is new in ErrorCode.ts and ErrorCode.cpp, and it is a plain Error, as in Node.
• The check runs before anything is recorded, with and without a live listener. An empty name added before listen() no longer makes listen() fail with hostname pattern cannot be empty.
• The test is now addContext() without a servername throws ERR_TLS_REQUIRED_SERVER_NAME and keeps no entry. It checks "", undefined and null before listen(), "" while the server listens, and that the next listen() works. The same assertions pass on node v26.3.0.
• The branch also has main merged in (b7ea95a). tsc --noEmit -p src/js/tsconfig.json passes.

#42050 carries the same guard and error code inside a larger change. The lines are identical, so the PR that lands second can drop its copy.

[claude]

LGTM — the latest push resolves the earlier $delete/$set nit and the new ERR_TLS_REQUIRED_SERVER_NAME check is a faithful port of Node's contract.

What was reviewed:

• addContext in src/js/node/tls.ts: the map is now always allocated and written only after a live handle accepts the entry, so a rejected name is never replayed; net.ts's kRealListen reads the same Map by reference via [buntls](), so a cluster worker's deferred listen sees later entries.
• The new error code is registered in all three places (ErrorCode.ts as Error, ErrorCode.cpp message, builtins.d.ts) with Node's verbatim message, and the falsy-name check runs before the typeof check, matching Node's ordering.
• Tests: read (not executed here — no debug build in this checkout); they assert exact code/name/message, use port: 0, tempDir, and drain pipes concurrently in the cluster case.

Extended reasoning...

The change is 13 lines in src/js/node/tls.ts plus a three-site registration of a new Node error code and ~150 lines of tests in the existing node-tls-server.test.ts. It touches the TLS SNI context bookkeeping in the node: compat layer but does not change certificate validation, handshake, or any trust decision — it only persists user-supplied contexts across listen() calls. The earlier inline nit (public Map methods) was addressed in the diff, and the pre-existing replay-failure concern is acknowledged in the description and tracked separately. The small, self-contained diff, the Node source citation for the new behavior, and the direct test coverage of each clause decided approve.