Skip to content

fix(socket): recover from stale disconnected socket to prevent permanent "Connecting..." #2487

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
YellowSnnowmann wants to merge 2 commits into
base: main
Choose a base branch
from
+26 −0
Draft

fix(socket): recover from stale disconnected socket to prevent permanent "Connecting..." #2487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
006ad8a
fix(socketService): handle stale disconnected socket instances to ens…
YellowSnnowmann May 22, 2026
301bb33
test(socketService): add test for reconnecting stale disconnected soc…
YellowSnnowmann May 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions app/src/services/__tests__/socketService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,25 @@ describe('socketService — resolveCoreSocketBaseUrl uses getCoreRpcUrl', () =>
);
expect(latestSocket.once).not.toHaveBeenCalledWith('queued-on-event', expect.any(Function));
});

it('reconnects when a stale disconnected socket exists for the same token', async () => {
const { io } = await import('socket.io-client');
const ioMock = vi.mocked(io);
ioMock.mockClear();

hoisted.getCoreRpcUrlMock.mockResolvedValue('http://127.0.0.1:7788/rpc');

const { socketService } = await import('../socketService');
socketService.disconnect();

socketService.connect('mock-jwt-stale-socket');
await pollUntil(() => expect(ioMock).toHaveBeenCalledTimes(1));

// Same token, previous socket is disconnected=true in our mock.
// We should still create a fresh socket instead of returning early.
socketService.connect('mock-jwt-stale-socket');
await pollUntil(() => expect(ioMock).toHaveBeenCalledTimes(2));
Copy link
Copy Markdown
Contributor

@M3gA-Mind M3gA-Mind May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor] The test validates that io() is called twice (new socket created) which is the core regression being guarded. A small addition would increase confidence that the reconnect path actually executes past the stale-socket guard rather than taking some other code path:

// After the second connect(), verify the reconnect path dispatched 'connecting'
await pollUntil(() =>
  expect(storeMock.dispatch).toHaveBeenCalledWith(
    expect.objectContaining({ payload: expect.objectContaining({ status: 'connecting' }) })
  )
);

Not a blocker, but would catch a regression where io() gets called for an unrelated reason.

});
});

describe('socketService — connectivity dispatch on socket events (lines 164, 212, 230, 237, 240)', () => {
Expand Down
7 changes: 7 additions & 0 deletions app/src/services/socketService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,13 @@ class SocketService {
} else if (!this.socket.disconnected) {
// Socket is connecting, wait for it
return;
} else {
// Stale disconnected socket instance for the same token.
// Drop it so this connect attempt can create a fresh socket;
// otherwise the async stale-invocation guard below (`|| this.socket`)
// returns early and leaves connectivity stuck at "connecting".
this.socket = null;
Copy link
Copy Markdown
Contributor

@M3gA-Mind M3gA-Mind May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[major] The stale socket is abandoned via null-assignment rather than shut down cleanly. Two problems:

  1. Stale reconnect timers: socket.io-client has internal reconnect logic (reconnectionAttempts: 5 is configured). Setting this.socket = null drops your reference but doesn't cancel socket.io's internal reconnect timers. If a queued attempt fires after the new socket is created, the abandoned socket's connect handler (still wired up from connectAsync) will dispatch setStatusForUser({ status: 'connected' }) and setSocketIdForUser({ socketId: oldId }) into Redux — overwriting state that belongs to the new socket.

  2. Orphaned listenerMap entries: listenerMap is not cleared here. Any socketService.on(event, cb) listeners registered during the previous connection are still in the map but were never attached to the new socket. Redux-connected components that registered handlers before the disconnect will silently stop receiving events. Meanwhile off(event, cb) will look up those entries, try to remove them from the new socket (where they don't exist), and remove them from listenerMap — so the caller gets no error but the handler is gone with no way to recover.

Suggested fix:

} else {
  // Stale disconnected socket, same token. Shut it down before clearing
  // to cancel any queued socket.io reconnect timers and avoid stale Redux
  // dispatches from the abandoned socket's event handlers.
  this.socket.disconnect();
  this.socket = null;
  this.mcpTransport = null;
  this.listenerMap.clear();
  this.pendingListeners = [];
}

This matches what disconnect() does for the token-changed path and puts listenerMap in a consistent empty state so callers re-register cleanly after reconnection.

this.mcpTransport = null;
}
}

Expand Down
Loading