From ce4c99ae0761dc93a7bf540b52546d8fe179c171 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Tue, 6 Oct 2026 17:13:59 -0400 Subject: [PATCH] fix(terminal): keep a pane closed just before quit from coming back on relaunch (#25711) * fix(terminal): keep a pane closed just before quit from coming back on relaunch The daemon now lists a session whose kill it accepted as state 'exiting' (still live), the inventory carries that through to pty:listSessions, and the worktree activation gate never adopts an exiting PTY into a new tab. Co-Authored-By: Claude * test(e2e): use the Linux close-pane chord (Ctrl+W) in the closed-pane relaunch spec --------- Co-authored-by: Claude --- ...-adapter-closing-session-inventory.test.ts | 62 +++++++ .../daemon/daemon-pty-session-inventory.ts | 1 + ...minal-host-closing-session-listing.test.ts | 72 ++++++++ .../daemon/terminal-host-session-listing.ts | 4 +- .../pty-startup-barrier-and-listing.test.ts | 7 +- src/main/ipc/pty/ipc/inspect.ts | 1 + src/main/providers/pty-process-info.ts | 3 + .../pty-process-list-admission.test.ts | 11 ++ .../providers/pty-process-list-admission.ts | 1 + .../worktree-agent-activation-gate.test.ts | 25 +++ .../src/lib/worktree-agent-activation-gate.ts | 9 +- src/shared/pty-listed-session.ts | 2 + ...terminal-closed-pane-quit-relaunch.spec.ts | 161 ++++++++++++++++++ 13 files changed, 354 insertions(+), 5 deletions(-) create mode 100644 src/main/daemon/daemon-pty-adapter-closing-session-inventory.test.ts create mode 100644 src/main/daemon/terminal-host-closing-session-listing.test.ts create mode 100644 tests/e2e/terminal-closed-pane-quit-relaunch.spec.ts diff --git a/src/main/daemon/daemon-pty-adapter-closing-session-inventory.test.ts b/src/main/daemon/daemon-pty-adapter-closing-session-inventory.test.ts new file mode 100644 index 00000000000..d5baac44911 --- /dev/null +++ b/src/main/daemon/daemon-pty-adapter-closing-session-inventory.test.ts @@ -0,0 +1,62 @@ +import './mock-descendant-sweep' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { rmSync } from 'node:fs' +import { DaemonPtyAdapter } from './daemon-pty-adapter' +import type { DaemonServer } from './daemon-server' +import { createMockSubprocess, startDaemonAdapterHarness } from './daemon-pty-adapter-test-harness' + +describe('daemon inventory of a pane closed just before quit', () => { + let dir: string + let socketPath: string + let tokenPath: string + let server: DaemonServer + let quittingApp: DaemonPtyAdapter + let relaunchedApp: DaemonPtyAdapter | null = null + const subprocesses: ReturnType[] = [] + + beforeEach(async () => { + subprocesses.length = 0 + const harness = await startDaemonAdapterHarness(() => { + const subprocess = createMockSubprocess() + // Why: a shell inside its kill grace -- it exits only when the test says so. + vi.mocked(subprocess.kill).mockImplementation(() => {}) + vi.mocked(subprocess.forceKill).mockImplementation(() => {}) + subprocesses.push(subprocess) + return subprocess + }) + ;({ dir, socketPath, tokenPath, server } = harness) + quittingApp = harness.adapter + }) + + afterEach(async () => { + for (const subprocess of subprocesses) { + subprocess._simulateExit(0) + } + quittingApp.dispose() + relaunchedApp?.dispose() + relaunchedApp = null + await server.shutdown() + rmSync(dir, { recursive: true, force: true }) + }) + + it('reports the closed pane as live but exiting to the relaunched app, then gone', async () => { + const kept = await quittingApp.spawn({ cols: 80, rows: 24, sessionId: 'wt@@kept' }) + const closed = await quittingApp.spawn({ cols: 80, rows: 24, sessionId: 'wt@@closed' }) + + const shutdown = quittingApp.shutdown(closed.id, { immediate: true }) + await vi.waitFor(() => expect(subprocesses[1]!.forceKill).toHaveBeenCalled()) + // Quit before the daemon has finished the kill. + quittingApp.dispose() + await shutdown.catch(() => {}) + + relaunchedApp = new DaemonPtyAdapter({ socketPath, tokenPath }) + const listed = await relaunchedApp.listProcesses() + expect(listed).toContainEqual(expect.objectContaining({ id: closed.id, exiting: true })) + expect(listed.find((entry) => entry.id === kept.id)).not.toHaveProperty('exiting') + + subprocesses[1]!._simulateExit(0) + await vi.waitFor(async () => + expect((await relaunchedApp!.listProcesses()).map((entry) => entry.id)).toEqual([kept.id]) + ) + }) +}) diff --git a/src/main/daemon/daemon-pty-session-inventory.ts b/src/main/daemon/daemon-pty-session-inventory.ts index b08c34830b4..39d0bdafcce 100644 --- a/src/main/daemon/daemon-pty-session-inventory.ts +++ b/src/main/daemon/daemon-pty-session-inventory.ts @@ -63,6 +63,7 @@ export abstract class DaemonPtySessionInventory extends DaemonPtyProcessInspecti ...(worktreeId ? { worktreeId } : {}), ...(session.terminalHandle ? { terminalHandle: session.terminalHandle } : {}), ...(session.wslDistro !== undefined ? { wslDistro: session.wslDistro } : {}), + ...(session.state === 'exiting' ? { exiting: true as const } : {}), ...this.validatedAgentSessionOwners(session.agentSessionOwners) }) ) diff --git a/src/main/daemon/terminal-host-closing-session-listing.test.ts b/src/main/daemon/terminal-host-closing-session-listing.test.ts new file mode 100644 index 00000000000..bd787b928e4 --- /dev/null +++ b/src/main/daemon/terminal-host-closing-session-listing.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, it, vi, type Mock } from 'vitest' +import { createMockSubprocess } from './daemon-pty-adapter-test-harness' +import { TerminalHost, type TerminalHostOptions } from './terminal-host' + +// Why mocked: the plain-shell teardown sweeps for real, and an unmocked run would put a live +// process-table probe behind these tests. +vi.mock('../pty-descendant-termination', () => ({ + killWithDescendantSweep: vi.fn() +})) + +type SpawnSubprocess = TerminalHostOptions['spawnSubprocess'] + +/** Shells that exit only when the test says so, standing in for one inside its kill grace. */ +function spawnManuallyExitedSubprocess(): { + spawnSubprocess: Mock + handles: ReturnType[] +} { + const handles: ReturnType[] = [] + const spawnSubprocess = vi.fn(() => { + const handle = createMockSubprocess() + vi.mocked(handle.kill).mockImplementation(() => {}) + vi.mocked(handle.forceKill).mockImplementation(() => {}) + handles.push(handle) + return handle + }) + return { spawnSubprocess, handles } +} + +const streamClient = (): { onData: Mock; onExit: Mock } => ({ + onData: vi.fn(), + onExit: vi.fn() +}) + +describe('TerminalHost listing of a session it is killing', () => { + it.each([ + ['an immediate kill', true], + ['a graceful kill', false] + ])('lists the session as live but exiting after %s, until it exits', async (_, immediate) => { + const { spawnSubprocess, handles } = spawnManuallyExitedSubprocess() + const host = new TerminalHost({ spawnSubprocess }) + const sessionId = 'wt-1@@closed-pane' + await host.createOrAttach({ sessionId, cols: 80, rows: 24, streamClient: streamClient() }) + expect(host.listSessions()).toMatchObject([{ sessionId, state: 'running', isAlive: true }]) + + const killed = host.kill(sessionId, { immediate }) + + // The relaunched app lists here: the closed pane must read as closing, not restorable. + expect(host.listSessions()).toMatchObject([{ sessionId, state: 'exiting', isAlive: true }]) + expect(host.hasLiveSessions()).toBe(true) + + handles[0]!._simulateExit(0) + await killed + expect(host.listSessions()).toEqual([]) + await host.dispose() + }) + + it('lists a session as running again when its kill signal is refused', async () => { + const { spawnSubprocess, handles } = spawnManuallyExitedSubprocess() + const host = new TerminalHost({ spawnSubprocess }) + const sessionId = 'wt-1@@unsignalled-pane' + await host.createOrAttach({ sessionId, cols: 80, rows: 24, streamClient: streamClient() }) + vi.mocked(handles[0]!.kill).mockImplementation(() => { + throw new Error('EPERM') + }) + + expect(() => host.kill(sessionId)).toThrow('EPERM') + + expect(host.listSessions()).toMatchObject([{ sessionId, state: 'running', isAlive: true }]) + handles[0]!._simulateExit(0) + await host.dispose() + }) +}) diff --git a/src/main/daemon/terminal-host-session-listing.ts b/src/main/daemon/terminal-host-session-listing.ts index 3ae2d5e845e..41f4fecc811 100644 --- a/src/main/daemon/terminal-host-session-listing.ts +++ b/src/main/daemon/terminal-host-session-listing.ts @@ -15,7 +15,9 @@ export function listLiveTerminalHostSessions( result.push({ sessionId: session.sessionId, incarnationId: session.incarnationId, - state: session.state, + // Why: a session whose kill was accepted is still live but no longer restorable; readers that + // adopt or restore sessions must skip it, while liveness readers keep counting it. + state: session.isTerminating ? 'exiting' : session.state, shellState: session.shellState, isAlive: true, ...(session.terminalHandle ? { terminalHandle: session.terminalHandle } : {}), diff --git a/src/main/ipc/pty-startup-barrier-and-listing.test.ts b/src/main/ipc/pty-startup-barrier-and-listing.test.ts index a4463d9d865..c61651ada25 100644 --- a/src/main/ipc/pty-startup-barrier-and-listing.test.ts +++ b/src/main/ipc/pty-startup-barrier-and-listing.test.ts @@ -201,7 +201,8 @@ describe('registerPtyHandlers', () => { it('lists sessions from both local and SSH providers', async () => { registerPtyHandlers(mainWindow as never) const sshListProcesses = vi.fn(async () => [ - { id: 'remote-pty', cwd: '/remote', title: 'ssh-shell' } + { id: 'remote-pty', cwd: '/remote', title: 'ssh-shell' }, + { id: 'closing-pty', cwd: '/remote', title: 'ssh-shell', exiting: true as const } ]) const sshShutdown = vi.fn(async () => undefined) registerSshPtyProvider('ssh-1', { @@ -234,9 +235,11 @@ describe('registerPtyHandlers', () => { expect(sshListProcesses).toHaveBeenCalled() expect(sessions).toEqual( expect.arrayContaining([ - expect.objectContaining({ cwd: '/remote', id: 'remote-pty', title: 'ssh-shell' }) + expect.objectContaining({ cwd: '/remote', id: 'remote-pty', title: 'ssh-shell' }), + expect.objectContaining({ id: 'closing-pty', exiting: true }) ]) ) + expect(sessions.find((session) => session.id === 'remote-pty')).not.toHaveProperty('exiting') await handlers.get('pty:kill')!(null, { id: 'remote-pty' }) expect(sshShutdown).toHaveBeenCalledWith('remote-pty', { diff --git a/src/main/ipc/pty/ipc/inspect.ts b/src/main/ipc/pty/ipc/inspect.ts index 37226017b8f..74a80b480a0 100644 --- a/src/main/ipc/pty/ipc/inspect.ts +++ b/src/main/ipc/pty/ipc/inspect.ts @@ -77,6 +77,7 @@ export function installPtyInspectIpcHandlers(deps: { cwd: session.cwd, title: session.title, ...(session.worktreeId !== undefined ? { worktreeId: session.worktreeId } : {}), + ...(session.exiting === true ? { exiting: true as const } : {}), // Why: the renderer's binding map is empty during restore, so ownership is the only // liveness evidence it has. Absence is authoritative only from a provider that // serializes claims — otherwise it is 'unknown', never 'absent' (#8459). diff --git a/src/main/providers/pty-process-info.ts b/src/main/providers/pty-process-info.ts index 942cab84f73..532ce9b1564 100644 --- a/src/main/providers/pty-process-info.ts +++ b/src/main/providers/pty-process-info.ts @@ -27,4 +27,7 @@ export type PtyProcessInfo = { /** The client identity the OWNING host recorded as having asked it to create this PTY. Absent * whenever the host could not attest one, and absence must never be read as "unowned". */ ownerClientInstanceId?: string + /** The owning host accepted a kill and is terminating this PTY. It is still live — liveness + * readers count it — but nothing may adopt or restore it. Absent from hosts that predate it. */ + exiting?: true } diff --git a/src/main/providers/pty-process-list-admission.test.ts b/src/main/providers/pty-process-list-admission.test.ts index 8b56ba26b1f..9086e0eefa8 100644 --- a/src/main/providers/pty-process-list-admission.test.ts +++ b/src/main/providers/pty-process-list-admission.test.ts @@ -40,6 +40,17 @@ describe('PtyProcessListAdmission', () => { ).toThrow('invalid_pty_process_list') }) + it('keeps the host verdict that a PTY is exiting', () => { + const admission = new PtyProcessListAdmission() + + expect(admission.admit({ id: 'pty-1', cwd: '/repo', title: 'shell', exiting: true })).toEqual({ + id: 'pty-1', + cwd: '/repo', + title: 'shell', + exiting: true + }) + }) + it('strips unknown provider payloads from admitted process metadata', () => { const admission = new PtyProcessListAdmission() diff --git a/src/main/providers/pty-process-list-admission.ts b/src/main/providers/pty-process-list-admission.ts index feecf06744b..103c22f6a2b 100644 --- a/src/main/providers/pty-process-list-admission.ts +++ b/src/main/providers/pty-process-list-admission.ts @@ -126,6 +126,7 @@ export class PtyProcessListAdmission { ...(value.worktreeId !== undefined ? { worktreeId: value.worktreeId } : {}), ...(value.terminalHandle !== undefined ? { terminalHandle: value.terminalHandle } : {}), ...(value.wslDistro !== undefined ? { wslDistro: value.wslDistro } : {}), + ...(value.exiting === true ? { exiting: true as const } : {}), ...(value.foregroundProcessEvidence !== undefined ? { foregroundProcessEvidence: cloneForegroundProcessEvidence( diff --git a/src/renderer/src/lib/worktree-agent-activation-gate.test.ts b/src/renderer/src/lib/worktree-agent-activation-gate.test.ts index 10cf28584a2..ee54c4e32cb 100644 --- a/src/renderer/src/lib/worktree-agent-activation-gate.test.ts +++ b/src/renderer/src/lib/worktree-agent-activation-gate.test.ts @@ -299,6 +299,31 @@ describe('worktree agent activation gate', () => { expect(resume).not.toHaveBeenCalled() }) + it('never gives a surface to a PTY its host is killing', async () => { + // A pane closed just before quit is still dying when the relaunched app lists it. + const closingPtyId = `${WORKTREE_ID}@@closed-pane` + const livePtyId = `${WORKTREE_ID}@@kept-pane` + const { deps, createTab } = testDeps({ + sessions: [ + { ...listed(closingPtyId), title: 'zsh', agentOwnership: 'absent', exiting: true }, + { ...listed(livePtyId), title: 'zsh', agentOwnership: 'absent' } + ], + surfaceOwners: new Map([ + [closingPtyId, UNOWNED], + [livePtyId, UNOWNED] + ]) + }) + + await runWorktreeAgentActivationGate(WORKTREE_ID, deps) + + expect(createTab).toHaveBeenCalledOnce() + expect(createTab).toHaveBeenCalledWith(WORKTREE_ID, undefined, undefined, { + initialPtyId: livePtyId, + activate: false, + recordInteraction: false + }) + }) + it('rebinds an unowned PTY to the recorded pane this renderer still holds', async () => { const ptyId = `${WORKTREE_ID}@@live-pty` const recorded = { paneKey: `tab-live:${LIVE_LEAF_ID}`, ptyId, tabId: 'tab-live' } diff --git a/src/renderer/src/lib/worktree-agent-activation-gate.ts b/src/renderer/src/lib/worktree-agent-activation-gate.ts index 12ea8d33eb2..37c8c93ab4d 100644 --- a/src/renderer/src/lib/worktree-agent-activation-gate.ts +++ b/src/renderer/src/lib/worktree-agent-activation-gate.ts @@ -192,15 +192,20 @@ export async function runWorktreeAgentActivationGate( sessionBelongsToWorkspace(session.id, worktreeId) ) const liveWorkspacePtyIds = new Set(liveWorkspaceSessions.map((session) => session.id)) + // Why: a session the host is killing was closed by the user (often just before a quit); a + // surface for it would bring the closed pane back. It still counts as live work below. + const adoptablePtyIds = liveWorkspaceSessions + .filter((session) => session.exiting !== true) + .map((session) => session.id) let liveSurfaceAdopted = false - if (liveWorkspaceSessions.length > 0) { + if (adoptablePtyIds.length > 0) { // Why: an unreadable census adopts nothing and mints nothing, so reporting 'adopted' // would suppress the caller's seed and leave the workspace with no surface at all — // fail-closed must still leave the user a usable pane (STA-5701). const adoption = await adoptLiveWorkspacePtySurfaces( deps.getState, worktreeId, - [...liveWorkspacePtyIds], + adoptablePtyIds, deps.listSurfaceOwners ) liveSurfaceAdopted = adoption.surfaced diff --git a/src/shared/pty-listed-session.ts b/src/shared/pty-listed-session.ts index 7594837e3d2..fb960695dad 100644 --- a/src/shared/pty-listed-session.ts +++ b/src/shared/pty-listed-session.ts @@ -26,6 +26,8 @@ export type PtyListedSession = { * Manager force-kill live agent sessions (#8459). */ agentOwnership: AgentOwnershipEvidence + /** Still live, but its host is terminating it after an accepted kill: never adopt or restore it. */ + exiting?: true } /** Only proven absence authorizes destroying a session without asking. */ diff --git a/tests/e2e/terminal-closed-pane-quit-relaunch.spec.ts b/tests/e2e/terminal-closed-pane-quit-relaunch.spec.ts new file mode 100644 index 00000000000..d405633b080 --- /dev/null +++ b/tests/e2e/terminal-closed-pane-quit-relaunch.spec.ts @@ -0,0 +1,161 @@ +/** + * A pane closed just before quit must stay closed after relaunch. + * + * Why this suite exists: closing a pane asks the terminal daemon to kill its shell, and on macOS + * that kill waits out a descendant grace before it finishes. Quitting inside that window left the + * daemon listing the dying session as live, so the relaunched app adopted it as an extra tab and its + * attach spawned a fresh shell under the closed id. + */ + +import type { ElectronApplication, Page } from '@stablyai/playwright-test' +import { test, expect } from './helpers/orca-app' +import { createRestartSession } from './helpers/orca-restart' +import { + focusActiveTerminalInput, + sendToTerminal, + splitActiveTerminalPane, + waitForActiveTerminalManager, + waitForPaneCount +} from './helpers/terminal' +import { bootstrapFirstLaunch, seededRepoPathOrSkip } from './helpers/terminal-restart-persistence' +import { ensureTerminalVisible, getActiveWorktreeId, waitForSessionReady } from './helpers/store' +import { SORTABLE_TAB } from './helpers/terminal-tab-menu' +import { RuntimeClient } from '../../src/cli/runtime/client' +import type { RuntimeTerminalListResult } from '../../src/shared/runtime-types' + +test.describe.configure({ mode: 'serial' }) + +type BoundPane = { leafId: string; ptyId: string } + +/** Setup read: the active tab's panes with their bound PTY ids, once every pane has one. */ +async function waitForBoundPanes(page: Page, count: number): Promise { + await waitForPaneCount(page, count, 30_000) + let panes: BoundPane[] = [] + await expect + .poll( + async () => { + panes = await page.evaluate(() => { + const state = window.__store!.getState() + const tabId = state.activeTabId + const manager = tabId ? window.__paneManagers?.get(tabId) : undefined + const bound = tabId ? state.terminalLayoutsByTabId[tabId]?.ptyIdsByLeafId : undefined + return (manager?.getPanes() ?? []).map((pane) => { + const leafId = pane.leafId + return { leafId, ptyId: bound?.[leafId] ?? '' } + }) + }) + return panes.length === count && panes.every((pane) => pane.ptyId !== '') + }, + { timeout: 30_000, message: 'A pane never bound its PTY' } + ) + .toBe(true) + return panes +} + +/** Closes the focused pane through the user's chord, confirming the stop prompt if one appears. */ +async function closeFocusedPaneWithChord(page: Page, panesBefore: number): Promise { + await focusActiveTerminalInput(page) + await page.keyboard.press(process.platform === 'darwin' ? 'Meta+w' : 'Control+w') + const confirm = page.getByRole('button', { name: 'Stop and Close' }) + await expect + .poll( + async () => { + if (await confirm.isVisible().catch(() => false)) { + await confirm.click() + } + return page.locator('.pane[data-leaf-id]:visible').count() + }, + { timeout: 10_000, intervals: [50] } + ) + .toBe(panesBefore - 1) +} + +/** Waits until every listed pane's own buffer shows its marker (per pane, not just the active one). */ +async function waitForPaneMarkers(page: Page, leafIds: string[]): Promise { + await expect + .poll( + () => + page.evaluate((leafIds) => { + const state = window.__store!.getState() + const manager = state.activeTabId + ? window.__paneManagers?.get(state.activeTabId) + : undefined + const textByLeaf = new Map( + (manager?.getPanes() ?? []).map((pane) => [ + pane.leafId, + pane.serializeAddon?.serialize?.() ?? '' + ]) + ) + return leafIds.every((leafId) => + (textByLeaf.get(leafId) ?? '').includes(`KEEP_${leafId.slice(0, 8)}`) + ) + }, leafIds), + { timeout: 15_000, message: 'A pane lost its scrollback marker' } + ) + .toBe(true) +} + +async function listHostPtyIds(userDataDir: string, worktreeId: string): Promise { + const client = new RuntimeClient(userDataDir, 30_000) + const listed = await client.call('terminal.list', { + worktree: `id:${worktreeId}` + }) + return listed.result.terminals.map((terminal) => terminal.ptyId ?? '') +} + +test('a pane closed right before quit does not come back after relaunch', async (// oxlint-disable-next-line no-empty-pattern -- Playwright's second fixture arg is testInfo; the first must be an object destructure to opt out of the default fixture set. +{}, testInfo) => { + const repoPath = seededRepoPathOrSkip() + const session = createRestartSession(testInfo) + let app: ElectronApplication | null = null + try { + const first = await session.launch() + app = first.app + const { worktreeId } = await bootstrapFirstLaunch(first.page, repoPath) + await waitForBoundPanes(first.page, 1) + await splitActiveTerminalPane(first.page, 'vertical') + await waitForBoundPanes(first.page, 2) + await splitActiveTerminalPane(first.page, 'horizontal') + const three = await waitForBoundPanes(first.page, 3) + for (const pane of three) { + await sendToTerminal(first.page, pane.ptyId, `echo KEEP_${pane.leafId.slice(0, 8)}\r`) + } + await waitForPaneMarkers( + first.page, + three.map((pane) => pane.leafId) + ) + + await closeFocusedPaneWithChord(first.page, 3) + const kept = await waitForBoundPanes(first.page, 2) + const closed = three.find((pane) => !kept.some((k) => k.leafId === pane.leafId)) + expect(closed).toBeDefined() + // Quit at once: the daemon is still inside the closed shell's kill grace. + await session.close(app) + app = null + + const second = await session.launch() + app = second.app + await waitForSessionReady(second.page) + await expect.poll(() => getActiveWorktreeId(second.page), { timeout: 10_000 }).toBe(worktreeId) + await ensureTerminalVisible(second.page) + await waitForActiveTerminalManager(second.page, 30_000) + await waitForBoundPanes(second.page, 2) + // Why a settle window: the resurrection arrived as a late adopted tab after the kill finished. + await second.page.waitForTimeout(4_000) + + await expect(second.page.locator(SORTABLE_TAB)).toHaveCount(1) + await expect(second.page.locator('.pane[data-leaf-id]:visible')).toHaveCount(2) + await waitForPaneMarkers( + second.page, + kept.map((pane) => pane.leafId) + ) + const hostPtyIds = await listHostPtyIds(session.userDataDir, worktreeId) + expect(hostPtyIds).not.toContain(closed!.ptyId) + expect(hostPtyIds.toSorted()).toEqual(kept.map((pane) => pane.ptyId).toSorted()) + } finally { + if (app) { + await session.close(app) + } + await session.dispose() + } +})