diff --git a/apps/code/src/main/services/shell/service.test.ts b/apps/code/src/main/services/shell/service.test.ts index 6cafe2b3fb..c7b6b36177 100644 --- a/apps/code/src/main/services/shell/service.test.ts +++ b/apps/code/src/main/services/shell/service.test.ts @@ -374,6 +374,20 @@ describe("ShellService", () => { expect(service.check("session-1")).toBe(false); }); + it("emits an exit event for explicit teardown", async () => { + const exitHandler = vi.fn(); + service.on(ShellEvent.Exit, exitHandler); + + await service.create("session-1"); + + service.destroy("session-1"); + + expect(exitHandler).toHaveBeenCalledWith({ + sessionId: "session-1", + exitCode: 130, + }); + }); + it("does nothing for non-existent session", () => { expect(() => service.destroy("nonexistent")).not.toThrow(); }); diff --git a/apps/code/src/main/services/shell/service.ts b/apps/code/src/main/services/shell/service.ts index f82fec5da1..8dfead65e6 100644 --- a/apps/code/src/main/services/shell/service.ts +++ b/apps/code/src/main/services/shell/service.ts @@ -23,6 +23,7 @@ declare module "node-pty" { const log = logger.scope("shell"); const PTY_ENCODING = "utf8"; +const DESTROYED_EXIT_CODE = 130; export interface ShellSession { pty: pty.IPty; @@ -314,6 +315,10 @@ export class ShellService extends TypedEventEmitter { } session.pty.destroy(); this.sessions.delete(sessionId); + this.emit(ShellEvent.Exit, { + sessionId, + exitCode: DESTROYED_EXIT_CODE, + }); } } diff --git a/apps/code/src/renderer/features/terminal/services/TerminalManager.test.ts b/apps/code/src/renderer/features/terminal/services/TerminalManager.test.ts new file mode 100644 index 0000000000..6fec18102a --- /dev/null +++ b/apps/code/src/renderer/features/terminal/services/TerminalManager.test.ts @@ -0,0 +1,175 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const mocks = vi.hoisted(() => { + const checkQuery = vi.fn(); + const createMutate = vi.fn(); + const createCommandMutate = vi.fn(); + const writeMutate = vi.fn(); + const resizeMutate = vi.fn(); + const openExternalMutate = vi.fn(); + const logInfo = vi.fn(); + const logError = vi.fn(); + + class MockTerminal { + cols = 80; + rows = 24; + options: Record; + dataHandler: ((data: string) => void) | null = null; + loadAddon = vi.fn(); + attachCustomKeyEventHandler = vi.fn(); + write = vi.fn(); + writeln = vi.fn(); + clear = vi.fn(); + refresh = vi.fn(); + focus = vi.fn(); + dispose = vi.fn(); + + constructor(options: Record) { + this.options = options; + terminalInstances.push(this); + } + + onData(handler: (data: string) => void) { + this.dataHandler = handler; + return { dispose: vi.fn() }; + } + + open(element: HTMLElement) { + const terminalElement = document.createElement("div"); + terminalElement.className = "xterm"; + element.appendChild(terminalElement); + } + + emitData(data: string) { + this.dataHandler?.(data); + } + } + + const terminalInstances: MockTerminal[] = []; + + return { + checkQuery, + createMutate, + createCommandMutate, + writeMutate, + resizeMutate, + openExternalMutate, + logInfo, + logError, + MockTerminal, + terminalInstances, + }; +}); + +vi.mock("@renderer/trpc", () => ({ + trpcClient: { + shell: { + check: { query: mocks.checkQuery }, + create: { mutate: mocks.createMutate }, + createCommand: { mutate: mocks.createCommandMutate }, + write: { mutate: mocks.writeMutate }, + resize: { mutate: mocks.resizeMutate }, + }, + os: { + openExternal: { mutate: mocks.openExternalMutate }, + }, + }, +})); + +vi.mock("@utils/logger", () => ({ + logger: { + scope: () => ({ + info: mocks.logInfo, + error: mocks.logError, + }), + }, +})); + +vi.mock("@utils/platform", () => ({ + isMac: false, +})); + +vi.mock("@xterm/addon-fit", () => ({ + FitAddon: class { + fit = vi.fn(); + }, +})); + +vi.mock("@xterm/addon-serialize", () => ({ + SerializeAddon: class { + serialize = vi.fn(() => "serialized-terminal-state"); + }, +})); + +vi.mock("@xterm/addon-web-links", () => ({ + WebLinksAddon: class {}, +})); + +vi.mock("@xterm/xterm", () => ({ + Terminal: mocks.MockTerminal, +})); + +import { terminalManager } from "./TerminalManager"; + +describe("TerminalManager shell recovery", () => { + const sessionId = "shell-recovery-test"; + + beforeEach(() => { + mocks.checkQuery.mockReset(); + mocks.createMutate.mockReset(); + mocks.createCommandMutate.mockReset(); + mocks.writeMutate.mockReset(); + mocks.resizeMutate.mockReset(); + mocks.openExternalMutate.mockReset(); + mocks.logInfo.mockReset(); + mocks.logError.mockReset(); + mocks.terminalInstances.length = 0; + + mocks.checkQuery.mockResolvedValue(true); + mocks.createMutate.mockResolvedValue(undefined); + mocks.createCommandMutate.mockResolvedValue(undefined); + mocks.writeMutate.mockResolvedValue(undefined); + mocks.resizeMutate.mockResolvedValue(undefined); + }); + + afterEach(() => { + terminalManager.destroy(sessionId); + }); + + it("recreates a missing interactive shell and retries the triggering input", async () => { + terminalManager.create({ + sessionId, + persistenceKey: "task-1-shell", + cwd: "/repo", + taskId: "task-1", + }); + + await vi.waitFor(() => { + expect(mocks.checkQuery).toHaveBeenCalledWith({ sessionId }); + }); + + mocks.checkQuery.mockResolvedValueOnce(false); + mocks.writeMutate + .mockRejectedValueOnce(new Error(`Shell session ${sessionId} not found`)) + .mockResolvedValue(undefined); + + mocks.terminalInstances[0].emitData("a"); + + await vi.waitFor(() => { + expect(mocks.createMutate).toHaveBeenCalledWith({ + sessionId, + cwd: "/repo", + taskId: "task-1", + }); + }); + + await vi.waitFor(() => { + expect(mocks.writeMutate).toHaveBeenCalledTimes(2); + }); + + expect(mocks.writeMutate.mock.calls[1][0]).toEqual({ + sessionId, + data: "a", + }); + }); +}); diff --git a/apps/code/src/renderer/features/terminal/services/TerminalManager.ts b/apps/code/src/renderer/features/terminal/services/TerminalManager.ts index 44d4d6e03a..54bafddfee 100644 --- a/apps/code/src/renderer/features/terminal/services/TerminalManager.ts +++ b/apps/code/src/renderer/features/terminal/services/TerminalManager.ts @@ -1,4 +1,5 @@ import { trpcClient } from "@renderer/trpc"; +import { getErrorMessage } from "@shared/errors"; import { logger } from "@utils/logger"; import { isMac } from "@utils/platform"; import { FitAddon } from "@xterm/addon-fit"; @@ -40,6 +41,8 @@ export interface TerminalInstance { persistenceKey: string; cwd?: string; taskId?: string; + command?: string; + recoveryPromise: Promise | null; } export interface CreateOptions { @@ -139,6 +142,15 @@ function attachKeyHandlers(term: XTerm) { }); } +function isMissingShellSessionError( + error: unknown, + sessionId: string, +): boolean { + return getErrorMessage(error).includes( + `Shell session ${sessionId} not found`, + ); +} + class TerminalManagerImpl { private instances = new Map(); private listeners = new Map>>(); @@ -189,6 +201,8 @@ class TerminalManagerImpl { persistenceKey, cwd, taskId, + command, + recoveryPromise: null, }; if (initialState) { @@ -200,7 +214,10 @@ class TerminalManagerImpl { trpcClient.shell.write .mutate({ sessionId, data }) .catch((error: Error) => { - log.error("Failed to write to shell:", error); + this.handleMissingSessionError(sessionId, instance, error, { + reason: "write", + retryData: data, + }); }); this.scheduleSave(sessionId, instance); }); @@ -297,6 +314,66 @@ class TerminalManagerImpl { } } + private handleMissingSessionError( + sessionId: string, + instance: TerminalInstance, + error: unknown, + options: { reason: "write" | "resize"; retryData?: string }, + ): void { + if (!isMissingShellSessionError(error, sessionId)) { + log.error(`Failed to ${options.reason} shell:`, error); + return; + } + + this.recoverMissingSession(sessionId, instance, options.reason) + .then(() => { + if (options.retryData === undefined || !instance.isReady) { + return; + } + + return trpcClient.shell.write + .mutate({ sessionId, data: options.retryData }) + .catch((retryError: Error) => { + log.error( + "Failed to retry write after shell recovery:", + retryError, + ); + }); + }) + .catch((recoveryError: Error) => { + log.error("Failed to recover missing shell session:", recoveryError); + }); + } + + private recoverMissingSession( + sessionId: string, + instance: TerminalInstance, + reason: "write" | "resize", + ): Promise { + if (instance.command) { + this.handleExit(sessionId); + return Promise.resolve(); + } + + if (instance.recoveryPromise) { + return instance.recoveryPromise; + } + + log.info("Recovering missing shell session", { sessionId, reason }); + instance.isReady = false; + + instance.recoveryPromise = this.initializeSession( + sessionId, + instance, + instance.cwd, + instance.taskId, + ).finally(() => { + instance.recoveryPromise = null; + }); + + return instance.recoveryPromise; + } + private scheduleSave(sessionId: string, instance: TerminalInstance): void { if (instance.saveTimeout) { clearTimeout(instance.saveTimeout); @@ -348,7 +425,9 @@ class TerminalManagerImpl { rows: instance.term.rows, }) .catch((error: Error) => { - log.error("Failed to resize shell:", error); + this.handleMissingSessionError(sessionId, instance, error, { + reason: "resize", + }); }); } } diff --git a/apps/code/src/renderer/features/terminal/stores/terminalStore.test.ts b/apps/code/src/renderer/features/terminal/stores/terminalStore.test.ts new file mode 100644 index 0000000000..05fdeb89f8 --- /dev/null +++ b/apps/code/src/renderer/features/terminal/stores/terminalStore.test.ts @@ -0,0 +1,72 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const { getProcess, managerOn } = vi.hoisted(() => ({ + getProcess: vi.fn(), + managerOn: vi.fn(() => vi.fn()), +})); + +vi.mock("@renderer/trpc/client", () => ({ + trpcClient: { + shell: { + getProcess: { + query: getProcess, + }, + }, + }, +})); + +vi.mock("../services/TerminalManager", () => ({ + terminalManager: { + on: managerOn, + }, +})); + +import { clearPersistedSessionIds, useTerminalStore } from "./terminalStore"; + +describe("terminalStore persistence", () => { + beforeEach(() => { + localStorage.clear(); + getProcess.mockReset(); + managerOn.mockClear(); + useTerminalStore.setState({ + terminalStates: {}, + pollingIntervals: {}, + }); + }); + + it("does not persist process-local shell session ids", () => { + useTerminalStore + .getState() + .setSerializedState("task-1-shell", "scrollback"); + useTerminalStore + .getState() + .setSessionId("task-1-shell", "shell-stale-session"); + + const persisted = JSON.parse(localStorage.getItem("terminal-store") ?? ""); + + expect(persisted.state.terminalStates["task-1-shell"]).toEqual({ + serializedState: "scrollback", + sessionId: null, + }); + }); + + it("clears session ids from old persisted terminal state", () => { + expect( + clearPersistedSessionIds({ + terminalStates: { + "task-1-shell": { + serializedState: "scrollback", + sessionId: "shell-stale-session", + }, + }, + }), + ).toEqual({ + terminalStates: { + "task-1-shell": { + serializedState: "scrollback", + sessionId: null, + }, + }, + }); + }); +}); diff --git a/apps/code/src/renderer/features/terminal/stores/terminalStore.ts b/apps/code/src/renderer/features/terminal/stores/terminalStore.ts index 859aff6ac2..7a1cea25c9 100644 --- a/apps/code/src/renderer/features/terminal/stores/terminalStore.ts +++ b/apps/code/src/renderer/features/terminal/stores/terminalStore.ts @@ -22,12 +22,49 @@ interface TerminalStoreState { stopPolling: (key: string) => void; } +type PersistedTerminalStoreState = { + terminalStates: Record< + string, + { + serializedState: string | null; + sessionId: null; + } + >; +}; + const DEFAULT_TERMINAL_STATE: TerminalState = { serializedState: null, sessionId: null, processName: null, }; +export function clearPersistedSessionIds(persistedState: unknown) { + if (!persistedState || typeof persistedState !== "object") { + return persistedState; + } + + const state = persistedState as { + terminalStates?: Record>; + }; + + if (!state.terminalStates || typeof state.terminalStates !== "object") { + return persistedState; + } + + return { + ...state, + terminalStates: Object.fromEntries( + Object.entries(state.terminalStates).map(([key, value]) => [ + key, + { + ...value, + sessionId: null, + }, + ]), + ), + }; +} + export const useTerminalStore = create()( persist( (set, get) => ({ @@ -132,11 +169,14 @@ export const useTerminalStore = create()( }), { name: "terminal-store", - partialize: (state) => ({ + version: 1, + migrate: (persistedState) => + clearPersistedSessionIds(persistedState) as PersistedTerminalStoreState, + partialize: (state): PersistedTerminalStoreState => ({ terminalStates: Object.fromEntries( Object.entries(state.terminalStates).map(([k, v]) => [ k, - { serializedState: v.serializedState, sessionId: v.sessionId }, + { serializedState: v.serializedState, sessionId: null }, ]), ), }),