diff --git a/web/src/components/ChatSidebar.test.tsx b/web/src/components/ChatSidebar.test.tsx index 523faa093c9b3..74042c55e9ba4 100644 --- a/web/src/components/ChatSidebar.test.tsx +++ b/web/src/components/ChatSidebar.test.tsx @@ -3,6 +3,8 @@ import { act, type ReactNode } from "react"; import { createRoot, type Root } from "react-dom/client"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { EVENTS_CONNECT_TIMEOUT_MS } from "@/lib/events-reconnect"; + const apiMocks = vi.hoisted(() => ({ buildWsUrl: vi.fn(async () => "ws://localhost/api/events?channel=chat-1"), getModelInfo: vi.fn(async () => ({ @@ -113,6 +115,10 @@ async function render(ui: ReactNode) { beforeEach(() => { FakeWebSocket.instances = []; vi.clearAllMocks(); + apiMocks.buildWsUrl.mockReset(); + apiMocks.buildWsUrl.mockResolvedValue( + "ws://localhost/api/events?channel=chat-1", + ); reloadMocks.maybeReloadForLoopbackWsAuthFailure.mockReturnValue(true); vi.stubGlobal("WebSocket", FakeWebSocket); }); @@ -180,6 +186,75 @@ describe("ChatSidebar event socket reconnect", () => { expect(apiMocks.buildWsUrl).toHaveBeenCalledTimes(2); }); + it("keeps retrying when reconnect URL construction fails", async () => { + await renderSidebar(); + apiMocks.buildWsUrl + .mockRejectedValueOnce(new Error("ticket endpoint unavailable")) + .mockResolvedValue("ws://localhost/api/events?channel=chat-1"); + + await act(async () => { + FakeWebSocket.instances[0].emit("close", { code: 1006 }); + }); + + await advance(1_000); + expect(FakeWebSocket.instances).toHaveLength(1); + expect(container.textContent).toContain("reconnecting in 2s"); + + await advance(2_000); + expect(FakeWebSocket.instances).toHaveLength(2); + expect(apiMocks.buildWsUrl).toHaveBeenCalledTimes(3); + }); + + it("times out a stalled URL request and retries", async () => { + let resolveStalledRequest!: (url: string) => void; + await renderSidebar(); + apiMocks.buildWsUrl + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveStalledRequest = resolve; + }), + ) + .mockResolvedValue("ws://localhost/api/events?channel=chat-1"); + + await act(async () => { + FakeWebSocket.instances[0].emit("close", { code: 1006 }); + }); + + await advance(1_000 + EVENTS_CONNECT_TIMEOUT_MS); + expect(FakeWebSocket.instances).toHaveLength(1); + expect(container.textContent).toContain("reconnecting in 2s"); + + // A late ticket response from the timed-out attempt must not create a + // superseded socket alongside the scheduled replacement. + await act(async () => { + resolveStalledRequest("ws://localhost/api/events?channel=stale"); + await Promise.resolve(); + }); + expect(FakeWebSocket.instances).toHaveLength(1); + + await advance(2_000); + expect(FakeWebSocket.instances).toHaveLength(2); + expect(apiMocks.buildWsUrl).toHaveBeenCalledTimes(3); + }); + + it("times out a stalled WebSocket handshake and retries", async () => { + await renderSidebar(); + + await act(async () => { + FakeWebSocket.instances[0].emit("close", { code: 1006 }); + }); + await advance(1_000); + expect(FakeWebSocket.instances).toHaveLength(2); + + await advance(EVENTS_CONNECT_TIMEOUT_MS); + expect(FakeWebSocket.instances[1].closed).toBe(true); + expect(container.textContent).toContain("reconnecting in 2s"); + + await advance(2_000); + expect(FakeWebSocket.instances).toHaveLength(3); + }); + it("backs off exponentially across repeated failures", async () => { await renderSidebar(); @@ -210,7 +285,11 @@ describe("ChatSidebar event socket reconnect", () => { FakeWebSocket.instances[0].emit("close", { code: 1006 }); }); - await advance(30_000); + await advance(1_000); + await act(async () => { + FakeWebSocket.instances[1].emit("open", {}); + }); + await advance(29_000); expect(FakeWebSocket.instances).toHaveLength(2); }); diff --git a/web/src/components/ChatSidebar.tsx b/web/src/components/ChatSidebar.tsx index aa4bd69ea32ed..e7669921129c2 100644 --- a/web/src/components/ChatSidebar.tsx +++ b/web/src/components/ChatSidebar.tsx @@ -36,6 +36,7 @@ import { GatewayClient, type ConnectionState } from "@/lib/gatewayClient"; import { api, buildWsUrl } from "@/lib/api"; import { maybeReloadForLoopbackWsAuthFailure } from "@/lib/dashboard-auth-reload"; import { + EVENTS_CONNECT_TIMEOUT_MS, EVENTS_DISCONNECTED_MESSAGE, EVENTS_MAX_RECONNECT_ATTEMPTS, eventsGaveUpMessage, @@ -256,8 +257,17 @@ export function ChatSidebar({ let unmounting = false; let ws: WebSocket | null = null; let reconnectTimer: ReturnType | null = null; + let connectTimer: ReturnType | null = null; + let connectGeneration = 0; let attempt = 0; + const clearConnectTimer = () => { + if (connectTimer) { + clearTimeout(connectTimer); + connectTimer = null; + } + }; + // The banner is shared with `info.credential_warning` and the JSON-RPC // sidecar, and `error` is those messages' only home — the sidecar does // not re-emit. So the events feed may only write over an empty banner @@ -298,14 +308,49 @@ export function ChatSidebar({ if (unmounting) { return; } - // Re-minted every attempt: tickets are single-use with a short TTL, - // so a reconnect cannot replay the URL from the first connection. - const url = await buildWsUrl("/api/events", { channel }); - if (unmounting) { + + const generation = ++connectGeneration; + let socket: WebSocket | null = null; + + // Cover the whole connection attempt, including gated-mode ticket + // minting. A failed or hanging pre-socket request otherwise emits no + // WebSocket close event and permanently strands the retry loop at its + // last "reconnecting in ..." banner. + clearConnectTimer(); + connectTimer = setTimeout(() => { + connectTimer = null; + if (unmounting || generation !== connectGeneration) { + return; + } + + // Invalidate any late ticket result or socket event from this attempt + // before scheduling its replacement. + connectGeneration += 1; + if (socket && ws === socket) { + ws = null; + socket.close(); + } + scheduleReconnect(); + }, EVENTS_CONNECT_TIMEOUT_MS); + + try { + // Re-minted every attempt: tickets are single-use with a short TTL, + // so a reconnect cannot replay the URL from the first connection. + const url = await buildWsUrl("/api/events", { channel }); + if (unmounting || generation !== connectGeneration) { + return; + } + socket = new WebSocket(url); + ws = socket; + } catch { + if (unmounting || generation !== connectGeneration) { + return; + } + clearConnectTimer(); + connectGeneration += 1; + scheduleReconnect(); return; } - const socket = new WebSocket(url); - ws = socket; // A superseded socket's late close must not schedule a retry on top // of the one that replaced it. @@ -315,6 +360,7 @@ export function ChatSidebar({ if (!isCurrent()) { return; } + clearConnectTimer(); attempt = 0; clearEventsBanner(); }); @@ -332,6 +378,7 @@ export function ChatSidebar({ if (!isCurrent()) { return; } + clearConnectTimer(); if (maybeReloadForLoopbackWsAuthFailure(ev.code)) { return; } @@ -374,6 +421,8 @@ export function ChatSidebar({ return () => { unmounting = true; + connectGeneration += 1; + clearConnectTimer(); if (reconnectTimer) { clearTimeout(reconnectTimer); reconnectTimer = null; diff --git a/web/src/lib/events-reconnect.ts b/web/src/lib/events-reconnect.ts index f533bd924ea53..6babf3f2fd3c3 100644 --- a/web/src/lib/events-reconnect.ts +++ b/web/src/lib/events-reconnect.ts @@ -9,6 +9,8 @@ export const EVENTS_RECONNECT_BASE_MS = 1_000; export const EVENTS_RECONNECT_MAX_MS = 30_000; export const EVENTS_MAX_RECONNECT_ATTEMPTS = 15; +/** Bound ticket minting plus the WebSocket opening handshake. */ +export const EVENTS_CONNECT_TIMEOUT_MS = 15_000; /** Normal closure — the server said goodbye, don't chase it. */ const WS_CLOSE_NORMAL = 1000; diff --git a/web/src/lib/pty-reconnect.ts b/web/src/lib/pty-reconnect.ts index c4d94bbf86349..7917bba4c7a34 100644 --- a/web/src/lib/pty-reconnect.ts +++ b/web/src/lib/pty-reconnect.ts @@ -18,6 +18,13 @@ export const PTY_RESUME_RECONNECT_THROTTLE_MS = 1000; // and force-closed so `onclose` → scheduleReconnect can recover it. export const PTY_CONNECTING_TIMEOUT_MS = 8000; +// The same budget for the phase *before* the socket exists: in gated mode a +// connect first awaits a fresh single-use ticket. That request produces no +// WebSocket, so a rejection or a hang is invisible to both `onclose` and the +// CONNECTING timer above — the tab would sit on "connecting" forever with no +// retry. Bound it so the failure routes into the ordinary backoff instead. +export const PTY_TICKET_TIMEOUT_MS = 8000; + // How long after a resumed socket opens we keep suppressing ANSI erase codes // (`ESC[K` / `ESC[X`) from the PTY stream. Ink's two-pass virtual scroll emits // them while replaying a long session; past that replay they are legitimate diff --git a/web/src/pages/ChatPage.test.tsx b/web/src/pages/ChatPage.test.tsx index 5548d9e59ac71..08e6f0245cead 100644 --- a/web/src/pages/ChatPage.test.tsx +++ b/web/src/pages/ChatPage.test.tsx @@ -4,6 +4,8 @@ import { createRoot, type Root } from "react-dom/client"; import { MemoryRouter } from "react-router"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { PTY_TICKET_TIMEOUT_MS } from "@/lib/pty-reconnect"; + class FakeFitAddon { fit() {} } @@ -65,6 +67,9 @@ class FakeTerminal { } const maybeReloadForLoopbackWsAuthFailure = vi.fn(() => false); +const apiMocks = vi.hoisted(() => ({ + buildWsUrl: vi.fn(async () => "ws://localhost/api/pty?channel=chat-1"), +})); vi.mock("@xterm/addon-fit", () => ({ FitAddon: FakeFitAddon })); vi.mock("@xterm/addon-unicode11", () => ({ Unicode11Addon: class {} })); @@ -104,6 +109,10 @@ vi.mock("@/i18n", () => ({ vi.mock("@/lib/dashboard-auth-reload", () => ({ maybeReloadForLoopbackWsAuthFailure, })); +vi.mock("@/lib/api", () => ({ + api: apiMocks, + buildWsUrl: apiMocks.buildWsUrl, +})); class FakeWebSocket { static instances: FakeWebSocket[] = []; @@ -147,6 +156,8 @@ async function render(ui: ReactNode) { beforeEach(() => { FakeWebSocket.instances = []; maybeReloadForLoopbackWsAuthFailure.mockClear(); + apiMocks.buildWsUrl.mockReset(); + apiMocks.buildWsUrl.mockResolvedValue("ws://localhost/api/pty?channel=chat-1"); vi.stubGlobal("WebSocket", FakeWebSocket); vi.stubGlobal( "ResizeObserver", @@ -226,3 +237,90 @@ describe("ChatPage", () => { expect(maybeReloadForLoopbackWsAuthFailure).toHaveBeenCalledWith(4401); }); }); + +// The gated-mode ticket request runs before any socket exists, so a rejection +// or a hang emits no `close` event and never arms PTY_CONNECTING_TIMEOUT_MS +// (that timer is set after `new WebSocket`). Without its own deadline the tab +// strands on "connecting" with no retry. Mirrors the ChatSidebar events-feed +// coverage in src/components/ChatSidebar.test.tsx. +describe("ChatPage PTY ticket connect deadline", () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + async function renderChat() { + const { default: ChatPage } = await import("./ChatPage"); + await render( + + + , + ); + } + + /** Advance timers and flush the async connect that fires on the tick. */ + async function advance(ms: number) { + await act(async () => { + await vi.advanceTimersByTimeAsync(ms); + }); + } + + it("retries when the ticket request rejects", async () => { + apiMocks.buildWsUrl.mockRejectedValueOnce( + new Error("ticket endpoint unavailable"), + ); + + await renderChat(); + await advance(0); + expect(FakeWebSocket.instances).toHaveLength(0); + + // First backoff step is 250ms; the retry must mint a fresh ticket. + await advance(250); + expect(apiMocks.buildWsUrl).toHaveBeenCalledTimes(2); + expect(FakeWebSocket.instances).toHaveLength(1); + }); + + it("times out a stalled ticket request and retries", async () => { + let resolveStalledRequest!: (url: string) => void; + apiMocks.buildWsUrl.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveStalledRequest = resolve; + }), + ); + + await renderChat(); + await advance(0); + expect(FakeWebSocket.instances).toHaveLength(0); + + await advance(PTY_TICKET_TIMEOUT_MS); + expect(FakeWebSocket.instances).toHaveLength(0); + + // A late ticket from the timed-out attempt must not open a socket behind + // the replacement the deadline scheduled. + await act(async () => { + resolveStalledRequest("ws://localhost/api/pty?channel=stale"); + await Promise.resolve(); + }); + expect(FakeWebSocket.instances).toHaveLength(0); + + await advance(250); + expect(FakeWebSocket.instances).toHaveLength(1); + expect(FakeWebSocket.instances[0].url).not.toContain("channel=stale"); + }); + + it("leaves a settled ticket's socket to the CONNECTING timer", async () => { + await renderChat(); + await advance(0); + await vi.waitFor(() => expect(FakeWebSocket.instances).toHaveLength(1)); + + // NS-591 regression: once the socket exists the ticket deadline is + // disarmed, so PTY_CONNECTING_TIMEOUT_MS stays the only thing that may + // force-close a wedged handshake — the two must not both fire. + await advance(PTY_TICKET_TIMEOUT_MS); + expect(apiMocks.buildWsUrl).toHaveBeenCalledTimes(1); + }); +}); diff --git a/web/src/pages/ChatPage.tsx b/web/src/pages/ChatPage.tsx index 84740126dfce9..0e31c6589cfd4 100644 --- a/web/src/pages/ChatPage.tsx +++ b/web/src/pages/ChatPage.tsx @@ -43,6 +43,7 @@ import { PTY_RECONNECT_INPUT_MESSAGE, PTY_RESUME_RECONNECT_THROTTLE_MS, PTY_RESUME_SANITIZE_WINDOW_MS, + PTY_TICKET_TIMEOUT_MS, type PtyConnectionState, shouldBlockPtyInput, shouldReconnectPtyOnPageResume, @@ -951,7 +952,22 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) { connectingTimerRef.current = null; } }; - const scheduleReconnect = (code: number) => { + // The pre-socket half of the connect. A ticket request that rejects or + // never settles leaves no socket behind, so neither `onclose` nor the + // NS-591 CONNECTING timer (armed after `new WebSocket` below) can recover + // it. `ticketSuperseded` invalidates a late ticket result so a timed-out + // attempt cannot open a socket behind the replacement this schedules. + let ticketSuperseded = false; + let ticketTimer: ReturnType | null = null; + const clearTicketTimer = () => { + if (ticketTimer) { + clearTimeout(ticketTimer); + ticketTimer = null; + } + }; + // `code` is null when the attempt died before any socket existed — the + // banner then omits the "(code N)" suffix rather than inventing one. + const scheduleReconnect = (code: number | null) => { if (reconnectTimerRef.current) { return; } @@ -966,6 +982,13 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) { setReconnectNonce((n) => n + 1); }, delayMs); }; + // Give up on the ticket phase and hand off to the ordinary backoff. + const failTicketAttempt = () => { + ticketSuperseded = true; + clearTicketTimer(); + connectInFlightRef.current = false; + scheduleReconnect(null); + }; void (async () => { if (unmounting) return; const params: Record = { channel }; @@ -979,7 +1002,27 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) { // selected profile, so the conversation runs with that profile's model, // skills, memory, and sessions (see web_server._resolve_chat_argv). if (scopedProfile) params.profile = scopedProfile; - const url = await api.buildWsUrl("/api/pty", params); + + ticketTimer = setTimeout(() => { + ticketTimer = null; + if (unmounting || ticketSuperseded) { + return; + } + failTicketAttempt(); + }, PTY_TICKET_TIMEOUT_MS); + + let url: string; + try { + url = await api.buildWsUrl("/api/pty", params); + } catch (err) { + if (unmounting || ticketSuperseded) return; + console.warn(`[chat] PTY ticket request failed: ${err}`); + failTicketAttempt(); + return; + } + if (unmounting || ticketSuperseded) return; + clearTicketTimer(); + const ws = new WebSocket(url); ws.binaryType = "arraybuffer"; wsRef.current = ws; @@ -1250,6 +1293,8 @@ export default function ChatPage({ isActive = true }: { isActive?: boolean }) { if (settleRaf2) cancelAnimationFrame(settleRaf2); clearReconnectTimer(); clearConnectingTimer(); + clearTicketTimer(); + ticketSuperseded = true; connectInFlightRef.current = false; // Phase 5.3: ``ws`` is local to the IIFE that opens it (the gated-mode // ticket fetch makes the open async). The cleanup runs at the outer