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

## Summary

- Fixed a reconnect edge case where socketService.connect() could get stuck when a stale disconnected socket instance existed for the same auth token.
- Prevented false-positive connecting UI state by clearing stale disconnected socket references before async reconnect guards run.
- Preserved existing safety behavior for active sockets (connected) and in-flight sockets (!disconnected) to avoid duplicate connections.
- Added a regression unit test to verify same-token reconnect creates a fresh socket instead of silently no-oping.

## Problem

- Users could be stuck on Connecting... and unable to chat after a disconnect/reconnect cycle.
- Root cause: reconnect logic set state to connecting, but returned early because this.socket was still non-null (stale/disconnected), so no new socket was created and no connect/connect_error transition fired.
- This left connection state stranded and blocked chat flows.

## Solution

- In socketService.connectAsync, when token is unchanged and this.socket.disconnected === true, explicitly clear stale runtime references (this.socket, this.mcpTransport) before continuing.
- Keep existing early returns for:
  - same-token + already connected
  - same-token + currently connecting (!disconnected)
- Added test coverage for the stale-socket scenario: second same-token connect() now creates a new socket instance (verifies io(...) called twice).

## Submission Checklist

- If a section does not apply to this change, mark the item as N/A with a one-line reason. Do not delete items.
- Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy
- Diff coverage ≥ 80% — changed lines (Vitest + cargo-llvm-cov merged via diff-cover) meet the gate enforced by `.github/workflows/coverage.yml`. Run pnpm test:coverage and pnpm test:rust locally; PRs below 80% on changed lines will not merge.
- Coverage matrix updated — added/removed/renamed feature rows in `docs/TEST-COVERAGE-MATRIX.md` reflect this change (or N/A: behaviour-only change)
- All affected feature IDs from the matrix are listed in the PR description under ## Related
- No new external network dependencies introduced (mock backend used per Testing Strategy)
- Manual smoke checklist updated if this touches release-cut surfaces (`docs/RELEASE-MANUAL-SMOKE.md`)
- Linked issue closed via Closes #NNN in the ## Related section

## Impact

- Runtime/platform impact: frontend connection management path (app/src/services/socketService.ts) affecting desktop/web UI behavior where this service is used.
- Compatibility: no API/schema changes.
- Performance: negligible; only additional stale-reference cleanup in reconnect edge case.
- Security: no new credential surface or transport changes.


Co-authored-by: M3gA-Mind <megamind@mahadao.com>
This commit is contained in:
YellowSnnowmann
2026-05-29 04:49:44 +05:30
committed by GitHub
co-authored by M3gA-Mind
parent 1cd617c815
commit 71d9ecb6bc
@@ -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));
});
});
describe('socketService — connectivity dispatch on socket events (lines 164, 212, 230, 237, 240)', () => {