From 583dbb534c269ae95344289eea97deec04fcbea2 Mon Sep 17 00:00:00 2001 From: gustavosmendes <87918773+gustavosmendes@users.noreply.github.com> Date: Tue, 15 Sep 2026 03:05:27 -0300 Subject: [PATCH] fix(tui): preserve sessions across gateway reconnects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An attached (dashboard-embedded) Ink TUI whose WebSocket dropped never recovered even though the backend stayed alive, and a spawned gateway that crashed resumed the wrong session. - GatewayClient no longer resets `subscribed` on each transport generation: the renderer drain()s once on mount, so every post-reconnect event (gateway.ready included) stayed buffered forever. - clearReconnect() keeps the attempt counter; it is reset on gateway.ready (and kill()), so backoff actually grows across failed reconnects. - useMainApp's exit handler no longer calls start() for an attached socket closure — GatewayClient owns that reconnect; it only respawns a still-owned child, and plans the resume with the durable stored_session_id (what session.resume takes) instead of the process-local runtime sid. - session.create's stored_session_id is carried into ui state / the active session file so recovery and the exit epilogue target the durable id. - The recovery target is cleared only after resumeById resolves into a live sid, so a second disconnect during setup/history loading keeps it. - Stale-socket identity guards on 'open'/'message'. Salvaged from #111599 (@gustavosmendes) with trims: kept the client-side backoff reconnect for spawned children too (the "keeps trying to reconnect in the background" copy depends on it), kept the RPC-triggered reconnect during backoff, kept the spawn-mode "reply in progress was lost" wording (true for a dead child) and added attached-mode copy in userMessages.ts, dropped the config_warning contract regen and the SessionCreateResponse re-export (local type gains stored_session_id instead). Fixes #111594 --- .../createGatewayEventHandler.test.ts | 15 ++++++--- ui-tui/src/app/createGatewayEventHandler.ts | 13 ++++---- ui-tui/src/app/interfaces.ts | 6 ++-- ui-tui/src/app/useMainApp.ts | 33 ++++++++++++++----- ui-tui/src/app/useSessionLifecycle.ts | 18 ++++++---- ui-tui/src/app/userMessages.ts | 6 ++++ ui-tui/src/gatewayClient.ts | 31 ++++++++++++----- ui-tui/src/gatewayTypes.ts | 3 ++ 8 files changed, 89 insertions(+), 36 deletions(-) diff --git a/ui-tui/src/__tests__/createGatewayEventHandler.test.ts b/ui-tui/src/__tests__/createGatewayEventHandler.test.ts index e984b4675a..6cba2e8504 100644 --- a/ui-tui/src/__tests__/createGatewayEventHandler.test.ts +++ b/ui-tui/src/__tests__/createGatewayEventHandler.test.ts @@ -1074,12 +1074,17 @@ describe('createGatewayEventHandler', () => { const appended: Msg[] = [] const newSession = vi.fn() const resumeById = vi.fn() + const resumed = Promise.withResolvers() const ctx = buildCtx(appended) ctx.session.newSession = newSession // Mimic resumeById's synchronous status write so the test proves the // "recovering session…" label is applied *after* (and survives) it. - ctx.session.resumeById = resumeById.mockImplementation(() => patchUiState({ status: 'resuming…' })) + ctx.session.resumeById = resumeById.mockImplementation(() => { + patchUiState({ status: 'resuming…' }) + + return resumed.promise.then(() => patchUiState({ sid: 'sess-recovered', status: 'ready' })) + }) ctx.session.STARTUP_RESUME_ID = '' ctx.session.recoverSidRef = ref('sess-crashed') @@ -1089,10 +1094,12 @@ describe('createGatewayEventHandler', () => { await vi.waitFor(() => expect(resumeById).toHaveBeenCalledWith('sess-crashed')) expect(newSession).not.toHaveBeenCalled() - // One-shot: the ref is consumed so a later ordinary restart forges/resumes - // per config instead of re-resuming the recovered session. - expect(ctx.session.recoverSidRef.current).toBeNull() + expect(ctx.session.recoverSidRef.current).toBe('sess-crashed') expect(getUiState().status).toBe('recovering session…') + + resumed.resolve() + await vi.waitFor(() => expect(ctx.session.recoverSidRef.current).toBeNull()) + expect(getUiState().sid).toBe('sess-recovered') }) it('on gateway.ready with auto_resume on and a recent session, resumes it', async () => { diff --git a/ui-tui/src/app/createGatewayEventHandler.ts b/ui-tui/src/app/createGatewayEventHandler.ts index 8f878fd266..882beedd77 100644 --- a/ui-tui/src/app/createGatewayEventHandler.ts +++ b/ui-tui/src/app/createGatewayEventHandler.ts @@ -715,15 +715,16 @@ export function createGatewayEventHandler(ctx: GatewayEventHandlerContext): (ev: }) .catch((e: unknown) => turnController.pushActivity(`command catalog unavailable: ${rpcErrorMessage(e)}`, 'info')) - // Crash recovery: a respawn triggered by an unexpected gateway death - // resumes the session that was live, not a brand-new one. One-shot — the - // ref is cleared so an ordinary later restart still forges/resumes per - // config. No startup prompt here (this is mid-session, not a cold boot). + // Keep the recovery target until resume succeeds, including across a second + // disconnect during setup or history loading. Recovery never resends the prompt. const recoverSid = recoverSidRef?.current if (recoverSidRef && recoverSid) { - recoverSidRef.current = null - resumeById(recoverSid) + void resumeById(recoverSid).then(() => { + if (getUiState().sid && recoverSidRef.current === recoverSid) { + recoverSidRef.current = null + } + }) // After resumeById: it synchronously sets status to 'resuming…' on entry, // so override it here to keep the distinct "recovering" label visible for // the duration of the resume RPC (which later flips status to 'ready'). diff --git a/ui-tui/src/app/interfaces.ts b/ui-tui/src/app/interfaces.ts index e469186f00..ad58681533 100644 --- a/ui-tui/src/app/interfaces.ts +++ b/ui-tui/src/app/interfaces.ts @@ -483,12 +483,10 @@ export interface GatewayEventHandlerContext { STARTUP_RESUME_ID: string colsRef: MutableRefObject newSession: (msg?: string, title?: string) => void - // Set by useMainApp's exit handler to the session that was live when the - // gateway died unexpectedly; consumed once by the next `gateway.ready` so a - // respawn resumes that session instead of forging a fresh one. + // Session carried across a transport loss or child exit, cleared after resume. recoverSidRef?: MutableRefObject resetSession: () => void - resumeById: (id: string) => void + resumeById: (id: string) => Promise setCatalog: StateSetter } submission: { diff --git a/ui-tui/src/app/useMainApp.ts b/ui-tui/src/app/useMainApp.ts index 86b7516fcb..0c9b196ec7 100644 --- a/ui-tui/src/app/useMainApp.ts +++ b/ui-tui/src/app/useMainApp.ts @@ -72,6 +72,8 @@ import { BACKEND_RESTARTING, BACKEND_RESTARTING_ACTIVITY, backendGaveUp, + CONNECTION_LOST, + CONNECTION_LOST_ACTIVITY, lastStderrLine } from './userMessages.js' import { useSessionLifecycle } from './useSessionLifecycle.js' @@ -962,16 +964,31 @@ export function useMainApp(gw: GatewayClient) { const exitHandler = (code: null | number) => { turnController.reset() + const state = getUiState() + const storedSid = state.info?.stored_session_id || null + + // Attached socket closed: the backend (and any live turn) is still there — + // GatewayClient owns the backoff reconnect, and the next gateway.ready + // resumes the durable session id. Calling start() here would race that + // reconnect and reset its backoff. + if (gw.attached) { + recoverSidRef.current = storedSid ?? recoverSidRef.current + patchUiState({ busy: false, compacting: false, sid: null, status: 'reconnecting…' }) + + if (state.sid) { + turnController.pushActivity(CONNECTION_LOST_ACTIVITY, 'warn') + sys(CONNECTION_LOST) + } + + return + } // A still-owned child dying while the TUI is alive is an *unexpected* - // death — a user /quit exits Node before this fires, and a replaced child - // is identity-skipped in GatewayClient. Rather than stranding a long - // session (the user's complaint), respawn the gateway and resume the - // persisted session via the next gateway.ready, so a single crash / OOM / - // signal doesn't lose their work. planGatewayRecovery bounds the attempts - // so a gateway that crash-loops on startup can't spawn-storm, and falls - // back to recoverSidRef when sid was already cleared by a prior exit. - const plan = planGatewayRecovery(getUiState().sid, recoverSidRef.current, recoveryAtRef.current, Date.now()) + // death — respawn the gateway and resume the persisted session via the + // next gateway.ready. session.resume takes the durable stored id, not the + // process-local runtime sid. planGatewayRecovery bounds the attempts so a + // crash-looping gateway can't spawn-storm. + const plan = planGatewayRecovery(storedSid, recoverSidRef.current, recoveryAtRef.current, Date.now()) // Clear sid immediately: while the gateway is down, sid-guarded effects // (session.active_list poll, queue drain) would otherwise fire RPCs at a diff --git a/ui-tui/src/app/useSessionLifecycle.ts b/ui-tui/src/app/useSessionLifecycle.ts index 959c469e39..5f05761c8a 100644 --- a/ui-tui/src/app/useSessionLifecycle.ts +++ b/ui-tui/src/app/useSessionLifecycle.ts @@ -208,13 +208,16 @@ export function useSessionLifecycle(opts: UseSessionLifecycleOptions) { return null } - const info = r.info ?? null + // The durable id lives on the create result; the lazy-create `info` does + // not carry it, and session.resume / the exit epilogue need the stored id. + const storedSid = r.stored_session_id || r.session_id + const info = r.info ? { ...r.info, stored_session_id: storedSid } : null const requestedTitle = title?.trim() ?? '' resetSession() setSessionStartedAt(Date.now()) - writeActiveSessionFile(r.session_id) + writeActiveSessionFile(storedSid) patchUiState({ info, sid: r.session_id, @@ -331,7 +334,7 @@ export function useSessionLifecycle(opts: UseSessionLifecycleOptions) { patchOverlayState({ sessions: false }) patchUiState({ status: 'resuming…' }) - rpc('setup.status', {}).then(setup => { + return rpc('setup.status', {}).then(setup => { if (setup?.provider_configured === false) { panel(SETUP_REQUIRED_TITLE, buildSetupRequiredSections()) patchUiState({ status: 'setup required' }) @@ -341,7 +344,7 @@ export function useSessionLifecycle(opts: UseSessionLifecycleOptions) { const previousSid = getUiState().sid - gw.request('session.resume', { cols: colsRef.current, session_id: id }) + return gw.request('session.resume', { cols: colsRef.current, session_id: id }) .then(raw => { const r = asRpcResult(raw) @@ -351,7 +354,10 @@ export function useSessionLifecycle(opts: UseSessionLifecycleOptions) { return patchUiState({ status: 'ready' }) } - const info = r.info ?? null + const info = r.info + ? { ...r.info, stored_session_id: r.info.stored_session_id || r.stored_session_id || r.resumed || id } + : null + const running = Boolean(r.running || r.status === 'working' || r.status === 'waiting') resetSession() @@ -360,7 +366,7 @@ export function useSessionLifecycle(opts: UseSessionLifecycleOptions) { const resumed = [...toTranscriptMessages(r.messages), ...liveSessionInflightMessages(r.inflight)] setHistoryItems(info ? [introMsg(info), ...resumed] : resumed) - writeActiveSessionFile(r.resumed ?? r.session_id) + writeActiveSessionFile(info?.stored_session_id || r.resumed || id) patchUiState({ busy: running, info, diff --git a/ui-tui/src/app/userMessages.ts b/ui-tui/src/app/userMessages.ts index dc48162b83..881620833f 100644 --- a/ui-tui/src/app/userMessages.ts +++ b/ui-tui/src/app/userMessages.ts @@ -39,6 +39,12 @@ export const BACKEND_RESTARTING = export const BACKEND_RESTARTING_ACTIVITY = 'Hermes stopped unexpectedly · restarting…' +// Attached (dashboard / embedded) mode: only the socket dropped; Hermes and any +// reply in progress are still alive on the backend and come back on reconnect. +export const CONNECTION_LOST = 'Connection to Hermes lost — reconnecting and reopening your chat…' + +export const CONNECTION_LOST_ACTIVITY = 'connection lost · reconnecting…' + export const backendGaveUp = (code: null | number, lastLine?: string): string => { const exit = code === null ? '' : ` (exit code ${code})` const detail = detailLine(lastLine) diff --git a/ui-tui/src/gatewayClient.ts b/ui-tui/src/gatewayClient.ts index 31f8881a06..2175def6bc 100644 --- a/ui-tui/src/gatewayClient.ts +++ b/ui-tui/src/gatewayClient.ts @@ -170,9 +170,15 @@ export class GatewayClient extends EventEmitter { }) } + get attached(): boolean { + return this.attachUrl !== null + } + private publish(ev: AnyGatewayEvent) { if (ev.type === 'gateway.ready') { this.ready = true + this.clearReconnect() + this.reconnectAttempts = 0 if (this.readyTimer) { clearTimeout(this.readyTimer) @@ -276,8 +282,6 @@ export class GatewayClient extends EventEmitter { clearTimeout(this.reconnectTimer) this.reconnectTimer = null } - - this.reconnectAttempts = 0 } private resetStartupState() { @@ -288,7 +292,9 @@ export class GatewayClient extends EventEmitter { // attached to a discarded child / socket. this.channel.detach(new Error('gateway restarting')) this.ready = false - this.subscribed = false + // `subscribed` is NOT reset here: the renderer drain()s once on mount, so a + // reset would strand every post-reconnect event (gateway.ready included) in + // the buffer forever (#111594). // Invalidate any pending deferred drain() flush from a prior transport so // its queued microtask becomes a no-op (it captured the old generation). this.drainGeneration += 1 @@ -325,6 +331,7 @@ export class GatewayClient extends EventEmitter { private handleTransportExit(code: null | number, reason?: string) { this.clearReadyTimer() + this.ready = false this.closeSidecarSocket() this.lifecycle(`[lifecycle] transport exit code=${code ?? 'null'} reason=${reason ?? 'none'}`) this.channel.detach(new Error(reason || `gateway exited${code === null ? '' : ` (${code})`}`)) @@ -332,9 +339,10 @@ export class GatewayClient extends EventEmitter { // Self-heal: a dropped transport (real close OR silent drop caught by the // heartbeat) should reconnect instead of stranding the UI on a dead socket // (issue #32997). Intentional shutdown sets `disposed` and skips this. - // Schedule before the synchronous 'exit' emission: useMainApp's existing + // Schedule before the synchronous 'exit' emission: in spawn mode useMainApp's // recovery subscriber may call start() immediately, and start() cancels this - // timer so there is only one recovery owner. + // timer so there is only one recovery owner; the attempt counter survives + // until gateway.ready so backoff keeps growing across failed restarts. this.scheduleReconnect() if (this.subscribed) { @@ -534,12 +542,15 @@ export class GatewayClient extends EventEmitter { ws.addEventListener( 'open', () => { + if (this.ws !== ws) { + return + } + if (!settled) { settled = true resolve() } - this.clearReconnect() this.connectSidecarMirror() }, { once: true } @@ -577,7 +588,11 @@ export class GatewayClient extends EventEmitter { connectPromise.catch(() => {}) this.wsConnectPromise = connectPromise - ws.addEventListener('message', ev => this.handleWebSocketFrame(ev.data)) + ws.addEventListener('message', ev => { + if (this.ws === ws) { + this.handleWebSocketFrame(ev.data) + } + }) ws.addEventListener('close', ev => { // Skip close events from sockets that have already been // replaced — start() / closeGatewaySocket() can swap `this.ws` @@ -618,7 +633,6 @@ export class GatewayClient extends EventEmitter { this.attachUrl = attachUrl this.sidecarUrl = sidecarUrl this.resetStartupState() - this.clearReconnect() if (this.proc && !this.proc.killed && this.proc.exitCode === null) { this.lifecycle(`[lifecycle] replacing live gateway child ${describeChild(this.proc)}`) @@ -767,6 +781,7 @@ export class GatewayClient extends EventEmitter { kill(reason = 'requested') { this.disposed = true this.clearReconnect() + this.reconnectAttempts = 0 const proc = this.proc const killed = proc?.kill() diff --git a/ui-tui/src/gatewayTypes.ts b/ui-tui/src/gatewayTypes.ts index 99a23092ef..ec6cc4dea4 100644 --- a/ui-tui/src/gatewayTypes.ts +++ b/ui-tui/src/gatewayTypes.ts @@ -179,6 +179,9 @@ export interface SystemBatteryResponse { export interface SessionCreateResponse { info?: SessionInfo & { config_warning?: string; credential_warning?: string } session_id: string + // Durable id (state.db row) — what session.resume takes; `session_id` is the + // process-local runtime handle. + stored_session_id?: string } export type LiveSessionStatus = 'idle' | 'starting' | 'waiting' | 'working'