Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 8:16 PM PT - Aug 12th, 2026
❌ @robobun, your commit 4603836 has some failures in 🧪 To try this PR locally: bunx bun-pr 37957That installs a local version of the PR into your bun-37957 --bun |
There was a problem hiding this comment.
LGTM — direct port of Node's addServerAbortSignalOption cleanup, mirroring the pattern already used for connect({ signal }) in the same file.
What was reviewed:
- Confirmed
addAbortListeneris the existing lazy module-level var (net.ts:259) andinternal/abort_listenerreturns a disposable whose[Symbol.dispose]doesn't depend onthis, so passing it bare toonce('close', ...)is safe. - Checked the abort-then-close ordering:
once: trueremoves the listener on abort, and the subsequent'close'→ dispose call is a no-op viaremoveEventListener?.(). - Test awaits real events (no sleeps), asserts exact listener counts and close-event count, and per the PR description fails on the unfixed release build.
Extended reasoning...
Overview
Four-line change to addServerAbortSignalOption() in src/js/node/net.ts: instead of a bare signal.addEventListener('abort', onAborted) that is never removed, it now goes through internal/abort_listener's addAbortListener() (a once listener returning a disposable) and detaches it via self.once('close', disposable[Symbol.dispose]). This is byte-for-byte what Node's lib/net.js does and what net.Socket#connect({ signal }) already does at net.ts:1755-1759 in this same file. A new test in node-net-server.test.ts verifies listener counts go 1,0,1,0,1,0 across three listen/close cycles on a shared signal and that a subsequent abort() does not re-emit 'close'.
Security risks
None. This is listener-lifecycle bookkeeping in a Node compat module — no auth, crypto, parsing of untrusted input, or filesystem/network boundary changes.
Level of scrutiny
Low. The diff is tiny, mechanical, and copies an established in-file pattern verbatim. I verified the addAbortListener module-level lazy var already exists, that the disposable's [Symbol.dispose] closes over removeEventListener rather than reading this (so the EventEmitter receiver binding is harmless), and that the abort→close ordering doesn't double-remove (the once: true listener self-removes on abort; the later dispose is removeEventListener?.() which no-ops). The already-aborted branch (process.nextTick(onAborted)) is unchanged.
Other factors
The test is well-constructed per the repo's review guidance: it awaits 'listening'/'close' events rather than sleeping, asserts exact arrays with toEqual, covers both the leak (listener count) and the observable symptom (duplicate 'close' events after abort()), and the PR description demonstrates it fails on release Bun 1.4.0 and matches Node v26.3.0 with the fix. The existing "should call abort with signal" test in the same file continues to cover the abort-while-listening path. tls.Server inherits this listen() so it's fixed transitively. No CODEOWNERS entry covers this path.
|
Closing: #43557 (5d5f03f) moved I checked
On a build from before #43557 (367d939), the test fails and the script leaves 2000 listeners. #46522 also lists this PR as already fixed on |
Problem
net.createServer().listen({ signal })registers an'abort'listener on the signal and never removes it, soserver.close()leaves the listener (and through its closure, the wholeServer) reachable from the signal for as long as the signal lives.'abort'listeners on the signal and about +126 MB RSS on a release build. Node leaves 0.abort()on that signal also callsclose()on every server that was already closed, and each of them emits a second'close'event.addServerAbortSignalOption()insrc/js/node/net.tsdoes a baresignal.addEventListener("abort", onAborted)with no matching removal.Fix
addServerAbortSignalOption()now registers throughinternal/abort_listener'saddAbortListener()(aoncelistener returning a disposable) and disposes it inserver.once("close", ...), which is what node'slib/net.jsdoes and whatnet.Socketalready does forconnect({ signal })in this file.'close'the listener is gone, so nothing holds the server and a later abort is a no-op for it.test/js/node/net/node-net-server.test.ts(new test "listen({ signal }) removes its abort listener from the signal once the server closes"): fails on the current release (1, 2, 2, 3, 3listeners accumulate), passes with this change (1, 0, 1, 0, 1, 0, and 3 close events for 3 servers).test/js/node/test/parallel/test-net-server-listen-options-signal.js,test-net-server-listen-options.js,test-net-server-close.js,test-net-server-async-dispose.mjs,test-net-server-call-listen-multiple-times.js,test-net-server-try-ports.js, andtest/js/node/net/server.spec.tsagainst the debug build: all pass.Background
listen({ signal })lets anAbortSignalclose a listening server: aborting the signal callsserver.close(). The signal holds its listeners strongly, so a listener that closes over a server keeps the server alive until either the listener is removed or the signal itself is collected.internal/abort_listeneris a small helper mirroring node'sevents.addAbortListener(): it adds the'abort'listener withonce: trueand returns an object whose[Symbol.dispose]()removes the listener, so callers can detach it when the thing it protects goes away first.tls.Servershares thislisten()implementation, so it gets the same fix.Reproduction (release bun 1.4.0 vs node v26.3.0)
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/net/node-net-server.test.ts