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 <noreply@anthropic.com>

* test(e2e): use the Linux close-pane chord (Ctrl+W) in the closed-pane relaunch spec

---------

Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Jinwoo Hong
2026-10-06 17:13:59 -04:00
committed by GitHub
co-authored by Claude
parent f7c542c7a3
commit ce4c99ae07
13 changed files with 354 additions and 5 deletions
@@ -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<typeof createMockSubprocess>[] = []
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])
)
})
})
@@ -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)
})
)
@@ -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<SpawnSubprocess>
handles: ReturnType<typeof createMockSubprocess>[]
} {
const handles: ReturnType<typeof createMockSubprocess>[] = []
const spawnSubprocess = vi.fn<SpawnSubprocess>(() => {
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()
})
})
@@ -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 } : {}),
@@ -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', {
+1
View File
@@ -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).
+3
View File
@@ -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
}
@@ -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()
@@ -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(
@@ -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' }
@@ -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
+2
View File
@@ -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. */
@@ -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<BoundPane[]> {
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<void> {
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<void> {
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<string, string>(
(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<string[]> {
const client = new RuntimeClient(userDataDir, 30_000)
const listed = await client.call<RuntimeTerminalListResult>('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()
}
})