Skip to content

fix(socket-mode): stop orphaning the close-handshake timer on disconnect (#2771) - #2777

Open
WilliamBergamin wants to merge 2 commits into
mainfrom
fix-issue-2771
Open

WilliamBergamin wants to merge 2 commits into
mainfrom
fix-issue-2771

Conversation

@WilliamBergamin

@WilliamBergamin WilliamBergamin commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This pull request fixes a close-handshake timer that could outlive its connection in SlackWebSocket.disconnect().

Fixes #2771. Follow-up to #2710, which added the timer.

The bug: disconnect() starts a 30s timer when it sends a close frame. In two cases that timer survived cleanup, then fired 30s later: a false "Peer did not complete the close handshake" warning and a second close event, which can trigger a duplicate reconnect.

  1. The connection drops after we send a close frame (the reported case). undici fires error with the socket already CLOSED, so disconnect() started a second timer and lost the first.
    • 🟢 disconnect() on a CLOSED socket now cleans up right away.
  2. disconnect() is called before the connection opens. undici closes a connecting socket synchronously inside close(), so cleanup ran before the timer was started.
    • 🟢 The timer now starts before close().

Note: when the connection drops, the client now receives close before error.

Not in this PR:

  • Emitting close only once per instance: client.disconnect() during a reconnect backoff relies on that extra close to resolve.
  • Logging the error at debug level while closing: a separate PR.

Testing

Each case has a new unit test that fails on main.

To check by hand against a build of this branch:

Requirements

🤖 Generated with Claude Code

…ocket closes mid-close (#2771)

disconnect() armed a second 30s close-handshake timer when the socket was
already CLOSED, losing the first. undici fires 'error' only after setting
CLOSED, so a TCP drop after our close frame always hit this. The lost
timer later logged a false handshake warning and emitted a second 'close',
which could start a duplicate reconnect.

- Clean up immediately when disconnect() finds the socket CLOSED.
- Arm the timer before close(): on a CONNECTING socket undici fires
  'error' and 'close' synchronously inside close(), so the re-entrant
  cleanup() now clears it instead of running before it is armed.

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>
@WilliamBergamin WilliamBergamin added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch pkg:socket-mode applies to `@slack/socket-mode` labels Oct 15, 2026
@changeset-bot

changeset-bot Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 94e0050

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@slack/socket-mode Patch

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

codecov Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.39%. Comparing base (1fabb45) to head (94e0050).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2777      +/-   ##
==========================================
- Coverage   89.40%   89.39%   -0.02%     
==========================================
  Files          65       65              
  Lines       10545    10548       +3     
  Branches      478      480       +2     
==========================================
+ Hits         9428     9429       +1     
- Misses       1086     1087       +1     
- Partials       31       32       +1     
Flag Coverage Δ
cli-hooks 89.28% <100.00%> (-0.07%) ⬇️
cli-test 89.28% <100.00%> (-0.07%) ⬇️
logger 89.28% <100.00%> (-0.07%) ⬇️
oauth 89.28% <100.00%> (-0.07%) ⬇️
socket-mode 89.28% <100.00%> (-0.07%) ⬇️
web-api 89.28% <100.00%> (-0.07%) ⬇️
webhook 89.28% <100.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@WilliamBergamin
WilliamBergamin marked this pull request as ready for review October 15, 2026 15:45
@WilliamBergamin
WilliamBergamin requested a review from a team as a code owner October 15, 2026 15:45
@WilliamBergamin WilliamBergamin self-assigned this Oct 15, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented pkg:socket-mode applies to `@slack/socket-mode` semver:patch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

socket-mode: an error while closing re-arms closeHandshakeTimeout and orphans the first timer (false close-handshake warning, spurious close)

1 participant