fix(net): destroy socket on native close to prevent server.close() hang#28732
fix(net): destroy socket on native close to prevent server.close() hang#28732robobun wants to merge 3 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSchedule deferred destruction via Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
@robobun The darwin failures timeouts are not related to this change and seem to be happening on recent prs as well. |
|
@robobun Can you try again ? |
robobun
force-pushed
the
farm/577dccf9/fix-socket-destroy-on-native-close
branch
4 times, most recently
from
a66bd4e to
3f6f199
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/13184.test.ts`:
- Around line 9-13: Remove the inline explanatory paragraph in the test body of
issue/13184.test.ts (the lines starting "When the native socket closes, ..."
through the explanatory sentences) and leave only the single issue URL/reference
line(s) that point to the bug; i.e., trim the prose while keeping the minimal
issue reference so the test matches the repo regression-test style and contains
no extra context comments.
🪄 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: c6554c16-1575-42b6-86bb-8dc0194acea4
📥 Commits
Reviewing files that changed from the base of the PR and between a66bd4eb272899976a3917efb1e3fdb3b50af759 and 3f6f199.
📒 Files selected for processing (1)
test/regression/issue/13184.test.ts
robobun
force-pushed
the
farm/577dccf9/fix-socket-destroy-on-native-close
branch
from
3f6f199 to
d64d8b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/13184.test.ts`:
- Around line 16-20: The test currently verifies socket destruction only
indirectly; add an explicit assertion using the Jest expect helper to validate
socket.destroyed after handler runs: import expect (or ensure the testing
framework's expect is available) at the top of the file and inside the
net.createServer callback add an assertion such as
expect(socket.destroyed).toBeTruthy() (or toBe(true)) alongside the existing
socket.on("close", onServerSocketClose) to fail fast and give clearer
diagnostics; update references in the callback where socket, handler,
onServerReady, and onServerSocketClose are used.
🪄 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: 9c4f3b98-2ea0-44bd-8cc9-7bea5bd6047e
📥 Commits
Reviewing files that changed from the base of the PR and between 3f6f199 and d64d8b282b35af532468a672887e2f7d6f61a4bf.
📒 Files selected for processing (1)
test/regression/issue/13184.test.ts
robobun
force-pushed
the
farm/577dccf9/fix-socket-destroy-on-native-close
branch
from
d64d8b2 to
2290256
Compare
|
@robobun Can you try again from main? Tests were deflaked. We might have luck this time. |
When a native socket closes, the close handlers push null to the readable stream but never call destroy(). If the readable side is paused or the writable was never ended, autoDestroy never fires. The socket stays in a zombie state (_handle=null, destroyed=false), server._connections never decrements, and server.close() hangs forever. Add process.nextTick(destroyNT, self) to all three close handlers (SocketHandlers, ServerHandlers, SocketHandlers2) so the socket always transitions to destroyed=true after native close, matching Node.js behavior.
robobun
force-pushed
the
farm/577dccf9/fix-socket-destroy-on-native-close
branch
from
57c675f to
5d8c41e
Compare
There was a problem hiding this comment.
This fix touches the core socket lifecycle ( → decrement) for all TCP connections, so it warrants a human sign-off before merging.
Extended reasoning...
Overview
The PR adds three guarded calls — one each in , , and in . It also adds a regression test suite () covering paused-readable, pipe/unpipe, and graceful-end teardown scenarios.
Security risks
None — this is a socket lifecycle correctness fix with no auth, crypto, or permission changes.
Level of scrutiny
The change is small and mechanical (identical 1-line pattern in three handlers, using an already-existing helper), but it modifies the fundamental socket destruction path that affects every TCP connection in Bun. The method decrements and gates completion, so an incorrect guard or double-destroy regression could break all server teardown. The PR includes solid test coverage and a clear root cause analysis, but the blast radius justifies a human reviewer confirming the fix.
Other factors
All coderabbitai review comments have been addressed (prose removal, socket.destroyed assertion discussion resolved). The build is in progress. No bugs were found by the automated system. The fix correctly mirrors Node.js behavior by using rather than a synchronous call.
|
Superseded by #31654, which is a strict superset of this change: it keeps the |
2 participants
Problem
When a native socket closes, Bun's close handlers (
SocketHandlers,ServerHandlers,SocketHandlers2) pushnullto the readable stream but never calldestroy(). If the readable side is paused or the writable was never ended,autoDestroynever fires. The socket stays in a zombie state (_handle=null,destroyed=false),server._connectionsnever decrements, andserver.close()hangs forever.Fixes #28731, #13184, #19563, #23648
Root Cause
In
src/js/node/net.ts, all three close handlers rely onautoDestroyto eventually call_destroy(), which decrementsserver._connectionsand emits thecloseevent. ButautoDestroyonly fires after both the readable side ends (endEmitted) and the writable side finishes. When the readable is paused or data hasn't been consumed, the readableendevent never emits, soautoDestroynever triggers.Fix
Add
if (!self.destroyed) process.nextTick(destroyNT, self)at the end of each close handler.destroyNTalready exists (line 94) and simply callsself.destroy(err). Usingprocess.nextTickensures the current close handler finishes before destruction runs, matching Node.js behavior.Verification
USE_SYSTEM_BUN=1 bun test test/regression/issue/13184.test.ts: 2 pass, 6 fail (timeouts)bun bd test test/regression/issue/13184.test.ts: 8 pass, 0 failtest/js/node/net/tests: no new failures (all failures pre-existing)