Skip to content

Commit 9199a64

Browse files
matt423claude
andcommitted
Bound the post-open handshake wait so a silent server can't hang the spinner
If the terminal server accepted the WebSocket but never sent its hello / status:connected, the UI hung forever on the yellow "Connecting" (initial / resume) or "Reconnecting" (auto-reconnect) box. The 30s connection timeout only covers the pre-open CONNECTING phase, so once a socket opened there was nothing to bound the wait — reproduced both in the unit harness and the live dashboard. Arm an "awaiting hello" timeout (12s) when a socket opens; if it elapses before we reach `connected`, close the socket with a private recoverable code (4901) so the reconnect scheduler retries — with fresh credentials — and ultimately lands on the recoverable "Press Enter" prompt instead of a frozen spinner. The timer is cleared on connected and on close. DX-1379 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent e9404f1 commit 9199a64

2 files changed

Lines changed: 230 additions & 1 deletion

File tree

packages/react-web-cli/src/AblyCliTerminal.inactivity.test.tsx

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ vi.mock("./utils/crypto", () => ({
7777
// visibility changes trigger real re-renders.
7878
import { AblyCliTerminal } from "./AblyCliTerminal";
7979
import { CONTROL_MESSAGE_PREFIX } from "./terminal-shared";
80+
import * as GlobalReconnect from "./global-reconnect";
8081

8182
const createControlMessage = (payload: unknown) =>
8283
CONTROL_MESSAGE_PREFIX + JSON.stringify(payload);
@@ -582,4 +583,151 @@ describe("DX-1379: inactivity pause & resume-on-return", () => {
582583
expect(authSend?.signature).toBe(SIGNATURE);
583584
},
584585
);
586+
587+
// The reporter's "stuck on Connecting/Reconnecting to Ably" symptom: a socket
588+
// that opens but never receives the server's hello. Before the await-hello
589+
// timeout this hung forever (the 30s connection timeout only covers the
590+
// pre-open CONNECTING phase). It must now be force-closed for retry. (DX-1379)
591+
const AWAIT_HELLO_MS = 12_000;
592+
593+
test("a socket that opens but never receives hello is force-closed for retry (not left hanging)", async () => {
594+
await act(async () => {
595+
render(
596+
<AblyCliTerminal
597+
websocketUrl={WS_URL}
598+
signedConfig={SIGNED_CONFIG}
599+
signature={SIGNATURE}
600+
resumeOnReload
601+
inactivityTimeoutMs={INACTIVITY_MS}
602+
maxReconnectAttempts={15}
603+
/>,
604+
);
605+
});
606+
await flush();
607+
const ws = sockets[0];
608+
await act(async () => {
609+
ws.fireOpen(); // opens, but the server stays silent (no hello)
610+
});
611+
await flush();
612+
613+
// Just before the threshold: still waiting, not force-closed.
614+
await act(async () => {
615+
vi.advanceTimersByTime(AWAIT_HELLO_MS - 1000);
616+
});
617+
expect(
618+
ws.close.mock.calls.some(
619+
([, reason]) => reason === "awaiting-hello-timeout",
620+
),
621+
).toBe(false);
622+
623+
// Cross the await-hello threshold -> the silent socket is closed for retry.
624+
await act(async () => {
625+
vi.advanceTimersByTime(1500);
626+
});
627+
await flush();
628+
const helloTimeoutClose = ws.close.mock.calls.find(
629+
([code, reason]) => code === 4901 && reason === "awaiting-hello-timeout",
630+
);
631+
expect(helloTimeoutClose).toBeTruthy();
632+
});
633+
634+
test("a resumed socket that never receives hello is closed for retry (the stuck-reconnecting repro)", async () => {
635+
await renderConnected("sess-hang");
636+
637+
await act(async () => {
638+
setVisibility("hidden");
639+
});
640+
await flush();
641+
await act(async () => {
642+
vi.advanceTimersByTime(INACTIVITY_MS + 10);
643+
});
644+
await flush();
645+
646+
const afterPause = sockets.length;
647+
await act(async () => {
648+
setVisibility("visible");
649+
});
650+
await flush();
651+
await act(async () => {
652+
vi.advanceTimersByTime(50);
653+
});
654+
await flush();
655+
656+
const resumeSocket = sockets[afterPause];
657+
await act(async () => {
658+
resumeSocket.fireOpen(); // resume socket opens; server never sends hello
659+
});
660+
await flush();
661+
662+
// It must not hang: after the await-hello timeout the resume socket is closed.
663+
await act(async () => {
664+
vi.advanceTimersByTime(AWAIT_HELLO_MS + 500);
665+
});
666+
await flush();
667+
const helloTimeoutClose = resumeSocket.close.mock.calls.find(
668+
([code, reason]) => code === 4901 && reason === "awaiting-hello-timeout",
669+
);
670+
expect(helloTimeoutClose).toBeTruthy();
671+
});
672+
673+
test("a hello within the await-hello window keeps the connection (no spurious close)", async () => {
674+
await act(async () => {
675+
render(
676+
<AblyCliTerminal
677+
websocketUrl={WS_URL}
678+
signedConfig={SIGNED_CONFIG}
679+
signature={SIGNATURE}
680+
resumeOnReload
681+
inactivityTimeoutMs={INACTIVITY_MS}
682+
/>,
683+
);
684+
});
685+
await flush();
686+
const ws = sockets[0];
687+
await act(async () => {
688+
ws.fireOpen();
689+
});
690+
await flush();
691+
// Server replies in time.
692+
await act(async () => {
693+
ws.fireMessage(
694+
createControlMessage({ type: "hello", sessionId: "sess-ok" }),
695+
);
696+
});
697+
await flush();
698+
699+
// Advancing past the await-hello window must NOT close the live socket.
700+
await act(async () => {
701+
vi.advanceTimersByTime(AWAIT_HELLO_MS + 2000);
702+
});
703+
await flush();
704+
// The live socket is not closed and stays usable.
705+
expect(
706+
ws.close.mock.calls.some(
707+
([, reason]) => reason === "awaiting-hello-timeout",
708+
),
709+
).toBe(false);
710+
expect(ws.readyState).toBe(FakeWebSocket.OPEN);
711+
});
712+
713+
test("the await-hello close code (4901) routes to a reconnect, not a terminal disconnect", async () => {
714+
const ws = await renderConnected("sess-route");
715+
// Isolate the effect of the 4901 close from the initial-connect bookkeeping.
716+
vi.mocked(GlobalReconnect.increment).mockClear();
717+
vi.mocked(GlobalReconnect.scheduleReconnect).mockClear();
718+
719+
// A 4901 close (what the await-hello timeout emits) must be treated as
720+
// recoverable: schedule a reconnect rather than purging the session.
721+
await act(async () => {
722+
ws.fireClose(4901, "awaiting-hello-timeout");
723+
});
724+
await flush();
725+
726+
expect(GlobalReconnect.increment).toHaveBeenCalled();
727+
expect(GlobalReconnect.scheduleReconnect).toHaveBeenCalled();
728+
// Recoverable => the session is NOT purged.
729+
expect(sessionStorage.getItem(`ably.cli.sessionId.${URL_HOST}`)).toBe(
730+
"sess-route",
731+
);
732+
});
585733
});

packages/react-web-cli/src/AblyCliTerminal.tsx

Lines changed: 82 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,18 @@ const BASE_NON_RECOVERABLE_CLOSE_CODES = [
206206
// never confused.
207207
const INACTIVITY_PAUSE_CLOSE_CODE = 4900;
208208

209+
// Private recoverable close code used when a socket opens but the server never
210+
// sends its `hello`/`connected` within AWAIT_HELLO_TIMEOUT_MS. Closing it as a
211+
// (recoverable, not in NON_RECOVERABLE) failure lets the reconnect scheduler
212+
// retry rather than leaving the UI stuck on "Connecting"/"Reconnecting" forever.
213+
const AWAIT_HELLO_CLOSE_CODE = 4901;
214+
215+
// How long to wait, after a socket has *opened*, for the server's first
216+
// `hello`/`status: connected` before treating the attempt as failed. The 30s
217+
// connection timeout only covers the pre-open CONNECTING phase, so without this
218+
// a silent-but-accepting server hangs the spinner indefinitely (DX-1379).
219+
const AWAIT_HELLO_TIMEOUT_MS = 12_000;
220+
209221
const AblyCliTerminalInner = (
210222
{
211223
websocketUrl,
@@ -685,9 +697,14 @@ const AblyCliTerminalInner = (
685697
(status: ConnectionStatus) => {
686698
// updateConnectionStatusAndExpose debug removed
687699
// A successful connection clears any in-flight inactivity-resume guard so
688-
// a later unrelated disconnect isn't misread as a failed resume.
700+
// a later unrelated disconnect isn't misread as a failed resume, and the
701+
// "awaiting hello" timeout (the handshake completed).
689702
if (status === "connected") {
690703
resumeAttemptReference.current = false;
704+
if (awaitHelloTimerReference.current) {
705+
clearTimeout(awaitHelloTimerReference.current);
706+
awaitHelloTimerReference.current = null;
707+
}
691708
}
692709
setComponentConnectionStatusState(status);
693710
// (window as any).componentConnectionStatusForTest = status; // Keep for direct inspection if needed, but primary is below
@@ -761,6 +778,26 @@ const AblyCliTerminalInner = (
761778
}
762779
}, []);
763780

781+
// "Awaiting hello" timeout: armed once a socket opens, cleared on
782+
// connected/close. Guards against a server that accepts the socket but never
783+
// completes the handshake.
784+
const awaitHelloTimerReference = useRef<NodeJS.Timeout | null>(null);
785+
const clearAwaitHelloTimer = useCallback(() => {
786+
if (awaitHelloTimerReference.current) {
787+
clearTimeout(awaitHelloTimerReference.current);
788+
awaitHelloTimerReference.current = null;
789+
}
790+
}, []);
791+
792+
// Same guard for the split-screen secondary socket (its own timer).
793+
const secondaryAwaitHelloTimerReference = useRef<NodeJS.Timeout | null>(null);
794+
const clearSecondaryAwaitHelloTimer = useCallback(() => {
795+
if (secondaryAwaitHelloTimerReference.current) {
796+
clearTimeout(secondaryAwaitHelloTimerReference.current);
797+
secondaryAwaitHelloTimerReference.current = null;
798+
}
799+
}, []);
800+
764801
const clearInstallInstructionsTimer = useCallback(() => {
765802
if (installInstructionsTimerReference.current) {
766803
clearTimeout(installInstructionsTimerReference.current);
@@ -1577,6 +1614,24 @@ const AblyCliTerminalInner = (
15771614
sock.send(JSON.stringify(payload));
15781615
}
15791616

1617+
// Arm the "awaiting hello" timeout: the socket is open, but until the
1618+
// server sends its hello/connected we have no session. If it never
1619+
// arrives, force-close this socket as a recoverable failure so the
1620+
// reconnect scheduler retries instead of the UI hanging on the spinner.
1621+
clearAwaitHelloTimer();
1622+
awaitHelloTimerReference.current = setTimeout(() => {
1623+
if (connectionStatusReference.current !== "connected") {
1624+
debugLog(
1625+
`⚠️ DIAGNOSTIC: No hello within ${AWAIT_HELLO_TIMEOUT_MS}ms of open - closing socket to retry`,
1626+
);
1627+
try {
1628+
sock.close(AWAIT_HELLO_CLOSE_CODE, "awaiting-hello-timeout");
1629+
} catch {
1630+
/* ignore */
1631+
}
1632+
}
1633+
}, AWAIT_HELLO_TIMEOUT_MS);
1634+
15801635
// Set up initial command to be sent when prompt is detected
15811636
// Skip initial command if we're resuming an existing session
15821637
if (initialCommand && !sessionId) {
@@ -1600,6 +1655,7 @@ const AblyCliTerminalInner = (
16001655
clearPtyBuffer,
16011656
sessionId,
16021657
clearConnectionTimeout,
1658+
clearAwaitHelloTimer,
16031659
refreshAuth,
16041660
],
16051661
);
@@ -1844,6 +1900,7 @@ const AblyCliTerminalInner = (
18441900
`[AblyCLITerminal] WebSocket closed. Code: ${event.code}, Reason: ${event.reason}, Clean: ${event.wasClean}`,
18451901
);
18461902
clearConnectionTimeout(); // Clear timeout on close
1903+
clearAwaitHelloTimer();
18471904
clearTerminalBoxOnly();
18481905
updateSessionActive(false);
18491906

@@ -2070,6 +2127,7 @@ const AblyCliTerminalInner = (
20702127
resumeOnReload,
20712128
sessionId,
20722129
clearConnectionTimeout,
2130+
clearAwaitHelloTimer,
20732131
clearPtyBuffer,
20742132
],
20752133
);
@@ -2796,6 +2854,11 @@ const AblyCliTerminalInner = (
27962854
]);
27972855

27982856
useEffect(() => () => clearInactivityTimer(), [clearInactivityTimer]);
2857+
useEffect(() => () => clearAwaitHelloTimer(), [clearAwaitHelloTimer]);
2858+
useEffect(
2859+
() => () => clearSecondaryAwaitHelloTimer(),
2860+
[clearSecondaryAwaitHelloTimer],
2861+
);
27992862

28002863
// Cleanup install instructions timer on unmount
28012864
useEffect(
@@ -3118,6 +3181,19 @@ const AblyCliTerminalInner = (
31183181
newSocket.send(JSON.stringify(payload));
31193182
}
31203183

3184+
// Bound the wait for the server's hello on the secondary socket too, so
3185+
// a silent-but-accepting server can't hang the secondary pane's spinner.
3186+
clearSecondaryAwaitHelloTimer();
3187+
secondaryAwaitHelloTimerReference.current = setTimeout(() => {
3188+
if (secondaryConnectionStatusReference.current !== "connected") {
3189+
try {
3190+
newSocket.close(AWAIT_HELLO_CLOSE_CODE, "awaiting-hello-timeout");
3191+
} catch {
3192+
/* ignore */
3193+
}
3194+
}
3195+
}, AWAIT_HELLO_TIMEOUT_MS);
3196+
31213197
// Set up initial command to be sent when prompt is detected
31223198
// Skip initial command if we're resuming an existing session
31233199
if (initialCommand && !secondarySessionId) {
@@ -3346,6 +3422,7 @@ const AblyCliTerminalInner = (
33463422
debugLog(
33473423
`[AblyCLITerminal] [Secondary] WebSocket closed. Code: ${event.code}, Reason: ${event.reason}`,
33483424
);
3425+
clearSecondaryAwaitHelloTimer();
33493426
setIsSecondarySessionActive(false);
33503427
updateSecondaryConnectionStatus("disconnected");
33513428

@@ -3740,6 +3817,10 @@ const AblyCliTerminalInner = (
37403817
// Update internal state for the secondary terminal
37413818
setSecondaryConnectionStatus(status);
37423819
secondaryConnectionStatusReference.current = status;
3820+
if (status === "connected" && secondaryAwaitHelloTimerReference.current) {
3821+
clearTimeout(secondaryAwaitHelloTimerReference.current);
3822+
secondaryAwaitHelloTimerReference.current = null;
3823+
}
37433824

37443825
// We intentionally don't call onConnectionStatusChange here
37453826
// as per requirements - only the primary terminal status should be reported

0 commit comments

Comments
 (0)