fix(socket-mode): tear down leaked sockets on disconnect (#2709) - #2710
Draft
WilliamBergamin wants to merge 5 commits into
Draft
fix(socket-mode): tear down leaked sockets on disconnect (#2709)#2710WilliamBergamin wants to merge 5 commits into
WilliamBergamin wants to merge 5 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: 3341c4e 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.05% -0.11%
==========================================
Files 65 65
Lines 10393 10441 +48
Branches 473 482 +9
==========================================
+ Hits 9266 9298 +32
- Misses 1096 1111 +15
- Partials 31 32 +1
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>
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
1 participant
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 every force-close mechanismwsprovided (terminate(), a ~30s close-handshake timeout). 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.This PR is TDD-first: the preceding commit (
test(socket-mode): add failing tests reproducing socket leak) adds RED tests reproducing each defect; this commit makes them GREEN with the minimal correct production change. Three independent defects are addressed: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. ACLOSE_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. undici'sWebSocketexposes no public.socket,terminate(), or abort signal — the raw socket lives on a private handler. When no userdispatcheris supplied,SlackWebSocketnow creates its ownAgentwhosebuildConnector-based connector captures the rawnet.Socket, and destroys that socket (and theAgent) 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'handler scheduled a reconnect on everyclose, with no guard against stale closes fired while a connection is still active, or against multiple closes stacking multiple reconnects. The handler now returns early if the socket is still active or a reconnect is already scheduled.Known limitation: socket-level force-close only applies when the client creates its own
Agent(no user dispatcher). This is intentional and documented.Testing:
npm test,npm run build, and Biome all pass for@slack/socket-mode(32/32 tests). Because the unit tests inject the captured socket rather than opening a real connection, the connector→field wiring should additionally be smoke-tested once against a live Slack app (confirm vialsof -nP -iTCP -sTCP:ESTABLISHEDthat sockets are torn down and don't accumulate across reconnects).Requirements