fix(socket-mode): tear down leaked sockets on disconnect (#2709) - #2710
Open
WilliamBergamin wants to merge 8 commits into
Open
fix(socket-mode): tear down leaked sockets on disconnect (#2709)#2710WilliamBergamin wants to merge 8 commits into
WilliamBergamin wants to merge 8 commits into
Conversation
First TDD step for issue #2709. Adds and refines red tests reproducing the three defects behind the WebSocket socket leak introduced by the ws -> undici migration: - disconnect() has no close-handshake timeout, so an unresponsive peer leaves the connection hung and 'close' never fires - cleanup() never destroys the underlying socket - the 'close' reconnect path has no active-connection guard and no timer dedup, so stale/duplicate 'close' events can spawn extra connections All four tests fail for their intended reasons; source fixes follow in a later pass. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Porting from `ws` to undici's WebSocket in 3.0.0 dropped every force-close mechanism, leaving ESTABLISHED TCP sockets to accumulate toward Slack's per-app connection cap. Three independent defects, each with a preceding failing test: - disconnect() sent a close frame with no timeout, so a dead peer left the socket in CLOSING forever. Arm a 30s close-handshake timeout that forces cleanup. - cleanup() never destroyed the underlying TCP socket. When no user dispatcher is supplied, capture the raw socket via a custom Agent connector and destroy it (plus the Agent) on cleanup. A user-supplied dispatcher owns its socket and relies on the close-handshake timeout; this limitation is documented on the dispatcher option. - the 'close' handler reconnected on every close. Guard against stale closes while still active and dedupe overlapping closes so at most one reconnect is scheduled. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
🦋 Changeset detectedLatest commit: 654a27b The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2710 +/- ##
==========================================
- Coverage 89.15% 89.07% -0.09%
==========================================
Files 65 65
Lines 10393 10441 +48
Branches 473 482 +9
==========================================
+ Hits 9266 9300 +34
- Misses 1096 1110 +14
Partials 31 31
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…nt Agent tracking Internal refactor of SlackWebSocket with no public API or behavior change. - Lift the inline undici `Agent` construction in `connect()` into a private `buildDefaultDispatcher()` helper, reducing `connect()` to a two-line dispatcher selection. - Remove the `ownAgent` field and its `cleanup()` teardown. undici already evicts and closes its pooled dispatcher when the client disconnects at WebSocket upgrade, so `Agent.destroy()` was a no-op on established connections; the real teardown remains `defaultSocket.destroy()`. - Rename `capturedSocket` -> `defaultSocket` (the socket is only captured on the default path) and widen its type to `Socket | TLSSocket | null` to match undici's connector callback, so the helper stores and forwards the socket without casts. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Reframe the `dispatcher` option comment around overriding the default dispatcher, dropping the omitted/supplied split for a shorter, clearer note on the one practical consequence (force-close vs. timeout fallback). Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Replace the internal-refactor changeset with one describing the user-facing fix: leaked TCP sockets on disconnect (#2709). Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
Rename the undici connector callback param from `cb` to `callback` in buildDefaultDispatcher() for readability. No behavior change. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
WilliamBergamin
added
bug
WilliamBergamin
marked this pull request as ready for review
August 27, 2026 17:33
Add unit tests for the default dispatcher's connect hook, the core of the #2709 socket-leak fix that had no direct coverage: success captures the socket into defaultSocket and calls back (null, socket); a connector error propagates without capturing; a (null, null) result synthesizes a "returned no socket" Error. Also drop the redundant "(issue #2709)" suffix from the two adjacent describe titles for consistency. Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2709.
Porting
@slack/socket-modefrom thewslibrary to undici'sWebSocketin 3.0.0 silently dropped some force-close mechanismwsprovided. 😅 When a peer stops responding, the underlying TCP socket is never torn down and accumulates inESTABLISHEDstate, pushing apps toward Slack's ~10-connections-per-app cap until Socket Mode stops connecting.disconnect()waited forever for the close handshake.SlackWebSocket.close(1000)writes a CLOSE frame with no timer, so a dead peer that never replies leaves the socket stuck inCLOSINGandcleanup()never runs.CLOSE_HANDSHAKE_TIMEOUT_MS(30s) timeout now arms when the close frame is sent and forcescleanup()if the handshake stalls.cleanup()never destroyed the underlying TCP socket.WebSocketexposes no public.socket,terminate(), or abort signal. The raw socket lives on a private handler.SlackWebSocketnow creates its own dispatcher that captures the raw socket, and destroys it duringcleanup(). A user-supplied dispatcher owns its own socket and cannot be force-closed here; it falls back to the close-handshake timeout above. This limitation is documented in thedispatcheroption's JSDoc.Reconnect storm on
close.SocketModeClient's'close'close, with no guard against stale closes fired while a connection is still active, or against multiple closes stacking multiple reconnects.Known limitation: socket-level force-close only applies when the client creates its own
Agent(no user dispatcher). This is intentional and documented.Requirements