mirror of
https://github.com/stablyai/orca.git
synced 2026-09-27 08:02:35 +00:00
fix(terminal): a remounted new SSH tab keeps the shell its old pane was still spawning (#22578)
* fix(terminal): a pane disposed mid-spawn no longer kills its successor's shell A new terminal tab whose pane remounts while its first pty:spawn is in flight is handed the SAME PTY by main's pane-spawn reservation. The disposed first transport then killed that PTY as an orphan, so the tab closed on pty-exit (focus fell back to tab 1) or stayed bound to a dead shell. Reported on SSH worktrees (scan 22). A transport destroyed mid-spawn now asks the pane surface first and keeps the PTY while the tab exists, the worktree is not being deleted, and any layout still names the leaf (leak-over-kill). A live transport refusing the id via admitPtyId still kills unconditionally (#11003). Adds rate-limited, id-hashed crash breadcrumbs for the next report: terminal_fresh_spawn_retired (killed vs retained), terminal_tab_pty_exit (host kind, ms since spawn, synthetic), terminal_active_tab_auto_move (active-terminal repair, createTab orphan sweep). The two duplicated tab pty-exit handlers now share handleTerminalTabPtyExit, and the FNV id hash used by two crumbs moves to crash-breadcrumb-id-hash.ts. Ports and supersedes #19386 (disposed-spawn-retention and its unit tests, credit to its author); its Docker SSH e2e specs are not included. * fix(terminal): scope spawn retention to its host and keep the PR focused --------- Co-authored-by: m4air <m4air@m4airs-Air.localdomain>
This commit is contained in:
@@ -129,6 +129,18 @@ the renderer retries.
|
||||
The SSH e2e lane must be green and triggering on **source** changes before any of this is attempted.
|
||||
It was skipping for 15 specs; four regressions reached a user during that window.
|
||||
|
||||
## Resolved: a disposed pane killed its successor's new shell
|
||||
|
||||
A pane rebuilt during its first spawn uses the same reservation key, so main can return the
|
||||
same PTY to both transports (#19386, #22578). The disposed transport must keep that shell
|
||||
while its tab and layout leaf remain and its execution host's workspace is not being deleted.
|
||||
A live transport refusing the id still retires it (#11003). This applies to local, WSL and SSH
|
||||
IPC terminals; the remote-runtime transport has no corresponding kill.
|
||||
|
||||
The remount trigger in the Scan-22 user report remains unknown. A retained shell can outlive
|
||||
its tab if the tab closes before a successor binds it; keeping potentially owned work follows
|
||||
the SSH execution boundary.
|
||||
|
||||
## Open: the pane behind a preserved tab does not always rebind
|
||||
|
||||
The merge now keeps a local tab the host has never been told about, so the tab and its title survive
|
||||
|
||||
@@ -39,7 +39,7 @@ export async function connectIpcPty(
|
||||
context: IpcPtyConnectContext
|
||||
): Promise<void | string | PtyConnectResult> {
|
||||
const { transportOptions, handlers } = context
|
||||
const { onPtySpawn } = transportOptions
|
||||
const { onPtySpawn, retainDisposedSpawn } = transportOptions
|
||||
context.setCallbacks(options.callbacks)
|
||||
ensurePtyDispatcher()
|
||||
|
||||
@@ -89,36 +89,37 @@ export async function connectIpcPty(
|
||||
// recorded before we asked for a PTY, so it belongs to that earlier owner, not to us.
|
||||
const priorIncarnationFence = currentPreHandlerPtySequence()
|
||||
const spawnResult = await spawnIpcPty(transportOptions, options, admittedSessionId)
|
||||
const retireFreshSpawn = async (): Promise<void> => {
|
||||
const retireFreshSpawn = async (path: 'disposed' | 'refused'): Promise<void> => {
|
||||
if (context.handleExplicitlyClosedConnect?.(spawnResult.id)) {
|
||||
return
|
||||
}
|
||||
// A newer generation may already own a recycled id; an id-only kill would retire its PTY.
|
||||
if (
|
||||
!spawnResult.isReattach &&
|
||||
!spawnResult.coldRestore &&
|
||||
!context.ownsPtyId(spawnResult.id)
|
||||
) {
|
||||
if (spawnResult.isReattach || spawnResult.coldRestore || context.ownsPtyId(spawnResult.id)) {
|
||||
return
|
||||
}
|
||||
// Only disposed transports can have a successor; live refusal must still retire the PTY (#11003).
|
||||
const retained = path === 'disposed' && retainDisposedSpawn?.() === true
|
||||
if (!retained) {
|
||||
await window.api.pty.kill(spawnResult.id)
|
||||
}
|
||||
}
|
||||
|
||||
if (context.isDestroyed()) {
|
||||
await retireFreshSpawn()
|
||||
await retireFreshSpawn('disposed')
|
||||
return
|
||||
}
|
||||
if (options.admitPtyId && !options.admitPtyId(spawnResult.id)) {
|
||||
await retireFreshSpawn()
|
||||
await retireFreshSpawn('refused')
|
||||
return context.isDestroyed() ? undefined : spawnResult
|
||||
}
|
||||
if (context.isDestroyed()) {
|
||||
await retireFreshSpawn()
|
||||
await retireFreshSpawn('disposed')
|
||||
return
|
||||
}
|
||||
if (spawnResult.isReattach && !admittedSessionId) {
|
||||
context.getCallbacks().onReattachDetermined?.()
|
||||
if (context.isDestroyed()) {
|
||||
await retireFreshSpawn()
|
||||
await retireFreshSpawn('disposed')
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
@@ -221,6 +221,44 @@ describe('connectPanePty', () => {
|
||||
expect(deps.onPtyErrorRef.current).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
// The disposed-spawn kill in ipc-pty-connect asks this callback before retiring a PTY; it must
|
||||
// answer from the live store, or a remounted pane's shell dies under it.
|
||||
it('lets a disposed spawn survive while the pane surface still exists in the store', async () => {
|
||||
const { connectPanePty } = await import('./pty-connection')
|
||||
const transport = createMockTransport()
|
||||
transportFactoryQueue.push(transport)
|
||||
const deps = createDeps({ tabId: 'tab-retain-disposed-spawn' })
|
||||
mockStoreState = {
|
||||
...mockStoreState,
|
||||
tabsByWorktree: { 'wt-1': [{ id: 'tab-retain-disposed-spawn', ptyId: null }] }
|
||||
}
|
||||
|
||||
connectPanePty(createPane(1) as never, createManager(1) as never, deps as never)
|
||||
await flushAsyncTicks()
|
||||
|
||||
const retain = createdTransportOptions[0]?.retainDisposedSpawn
|
||||
if (typeof retain !== 'function') {
|
||||
throw new Error('pane transport was built without retainDisposedSpawn')
|
||||
}
|
||||
expect(retain()).toBe(true)
|
||||
mockStoreState = {
|
||||
...mockStoreState,
|
||||
deleteStateByWorktreeId: { 'wt-1': { isDeleting: true, phase: 'deleting' } }
|
||||
}
|
||||
expect(retain()).toBe(false)
|
||||
mockStoreState = {
|
||||
...mockStoreState,
|
||||
deleteStateByWorktreeId: { 'local|wt-1': { isDeleting: true, phase: 'deleting' } }
|
||||
}
|
||||
expect(retain()).toBe(false)
|
||||
mockStoreState = {
|
||||
...mockStoreState,
|
||||
deleteStateByWorktreeId: {},
|
||||
tabsByWorktree: { 'wt-1': [] }
|
||||
}
|
||||
expect(retain()).toBe(false)
|
||||
})
|
||||
|
||||
it('fresh-spawns normally when the pane worktree is not being deleted', async () => {
|
||||
const { connectPanePty } = await import('./pty-connection')
|
||||
const transport = createMockTransport()
|
||||
|
||||
+120
@@ -0,0 +1,120 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { composeWorktreeHostIdentity } from '../../../../../shared/worktree/host-qualified-identity'
|
||||
import type { TerminalLayoutSnapshot, TerminalTab } from '../../../../../shared/terminal-tab-types'
|
||||
import type { WorktreeDeleteState } from '@/store/slices/worktree-delete-state-types'
|
||||
import { shouldRetainDisposedPaneSpawn } from './disposed-spawn-retention'
|
||||
|
||||
const WT = 'wt'
|
||||
const TAB = 'tab-a'
|
||||
const LEAF = '11111111-1111-4111-8111-111111111111'
|
||||
const OTHER_LEAF = '22222222-2222-4222-8222-222222222222'
|
||||
|
||||
function tab(id: string): TerminalTab {
|
||||
return {
|
||||
id,
|
||||
ptyId: null,
|
||||
worktreeId: WT,
|
||||
title: 'Terminal 1',
|
||||
customTitle: null,
|
||||
color: null,
|
||||
sortOrder: 0,
|
||||
createdAt: 0
|
||||
}
|
||||
}
|
||||
|
||||
function layout(leafId: string): TerminalLayoutSnapshot {
|
||||
return { root: { type: 'leaf', leafId }, activeLeafId: leafId, expandedLeafId: null }
|
||||
}
|
||||
|
||||
function state(overrides: {
|
||||
tabs?: Record<string, TerminalTab[]>
|
||||
layouts?: Record<string, TerminalLayoutSnapshot>
|
||||
deleting?: Record<string, WorktreeDeleteState>
|
||||
}) {
|
||||
return {
|
||||
tabsByWorktree: overrides.tabs ?? { [WT]: [tab(TAB)] },
|
||||
terminalLayoutsByTabId: overrides.layouts ?? {},
|
||||
deleteStateByWorktreeId: overrides.deleting ?? {}
|
||||
}
|
||||
}
|
||||
|
||||
const deleting: WorktreeDeleteState = {
|
||||
isDeleting: true,
|
||||
error: null,
|
||||
canForceDelete: false,
|
||||
forceDeleteReason: null
|
||||
}
|
||||
|
||||
describe('shouldRetainDisposedPaneSpawn', () => {
|
||||
it('keeps the PTY for a tab that still exists and has no layout root yet', () => {
|
||||
expect(shouldRetainDisposedPaneSpawn(state({}), WT, TAB, LEAF)).toBe(true)
|
||||
})
|
||||
|
||||
it('keeps the PTY when the layout still names the leaf', () => {
|
||||
expect(
|
||||
shouldRetainDisposedPaneSpawn(state({ layouts: { [TAB]: layout(LEAF) } }), WT, TAB, LEAF)
|
||||
).toBe(true)
|
||||
})
|
||||
|
||||
it('kills the PTY when the tab is gone from every worktree', () => {
|
||||
expect(
|
||||
shouldRetainDisposedPaneSpawn(state({ tabs: { [WT]: [tab('other-tab')] } }), WT, TAB, LEAF)
|
||||
).toBe(false)
|
||||
})
|
||||
|
||||
it('kills the PTY when the leaf was removed from the layout', () => {
|
||||
expect(
|
||||
shouldRetainDisposedPaneSpawn(
|
||||
state({ layouts: { [TAB]: layout(OTHER_LEAF) } }),
|
||||
WT,
|
||||
TAB,
|
||||
LEAF
|
||||
)
|
||||
).toBe(false)
|
||||
})
|
||||
|
||||
it('kills the PTY when its worktree is being deleted even though the tab is still listed', () => {
|
||||
expect(
|
||||
shouldRetainDisposedPaneSpawn(state({ deleting: { [WT]: deleting } }), WT, TAB, LEAF)
|
||||
).toBe(false)
|
||||
expect(
|
||||
shouldRetainDisposedPaneSpawn(state({ deleting: { other: deleting } }), WT, TAB, LEAF)
|
||||
).toBe(true)
|
||||
})
|
||||
|
||||
it.each(['local', 'ssh:target', 'runtime:paired'] as const)(
|
||||
'retires a disposed spawn when its %s workspace is being deleted',
|
||||
(executionHostId) => {
|
||||
const deletingState = state({
|
||||
deleting: { [composeWorktreeHostIdentity(executionHostId, WT)]: deleting }
|
||||
})
|
||||
|
||||
expect(shouldRetainDisposedPaneSpawn(deletingState, WT, TAB, LEAF, executionHostId)).toBe(
|
||||
false
|
||||
)
|
||||
expect(shouldRetainDisposedPaneSpawn(deletingState, WT, TAB, LEAF, 'ssh:other-target')).toBe(
|
||||
true
|
||||
)
|
||||
}
|
||||
)
|
||||
|
||||
it('keeps legacy deletion entries scoped to their recorded execution host', () => {
|
||||
const deletingState = state({
|
||||
deleting: { [WT]: { ...deleting, executionHostId: 'ssh:target' } }
|
||||
})
|
||||
|
||||
expect(shouldRetainDisposedPaneSpawn(deletingState, WT, TAB, LEAF, 'ssh:target')).toBe(false)
|
||||
expect(shouldRetainDisposedPaneSpawn(deletingState, WT, TAB, LEAF, 'ssh:other-target')).toBe(
|
||||
true
|
||||
)
|
||||
expect(
|
||||
shouldRetainDisposedPaneSpawn(state({ deleting: { [WT]: deleting } }), WT, TAB, LEAF, 'local')
|
||||
).toBe(false)
|
||||
})
|
||||
|
||||
it('finds the tab under a worktree other than the one it was opened in', () => {
|
||||
expect(
|
||||
shouldRetainDisposedPaneSpawn(state({ tabs: { [WT]: [], other: [tab(TAB)] } }), WT, TAB, LEAF)
|
||||
).toBe(true)
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,33 @@
|
||||
import type { AppState } from '@/store/types'
|
||||
import type { ExecutionHostId } from '../../../../../shared/execution-host'
|
||||
import { composeWorktreeHostIdentity } from '../../../../../shared/worktree/host-qualified-identity'
|
||||
import { collectLeafIdsInOrder } from '../terminal-layout-leaf-ids'
|
||||
|
||||
// Main can give a remounted pane the same PTY, so disposal alone does not make it ownerless.
|
||||
export function shouldRetainDisposedPaneSpawn(
|
||||
state: Pick<AppState, 'tabsByWorktree' | 'terminalLayoutsByTabId' | 'deleteStateByWorktreeId'>,
|
||||
worktreeId: string,
|
||||
tabId: string,
|
||||
leafId: string,
|
||||
executionHostId?: ExecutionHostId
|
||||
): boolean {
|
||||
const deleteState =
|
||||
(executionHostId
|
||||
? state.deleteStateByWorktreeId?.[composeWorktreeHostIdentity(executionHostId, worktreeId)]
|
||||
: undefined) ?? state.deleteStateByWorktreeId?.[worktreeId]
|
||||
if (
|
||||
deleteState?.isDeleting &&
|
||||
(!deleteState.executionHostId || deleteState.executionHostId === executionHostId)
|
||||
) {
|
||||
return false
|
||||
}
|
||||
const tabPresent = Object.values(state.tabsByWorktree).some((tabs) =>
|
||||
tabs.some((tab) => tab.id === tabId)
|
||||
)
|
||||
if (!tabPresent) {
|
||||
return false
|
||||
}
|
||||
// A new single-pane tab has no layout root until its first pane binds.
|
||||
const root = state.terminalLayoutsByTabId[tabId]?.root
|
||||
return !root || collectLeafIdsInOrder(root).includes(leafId)
|
||||
}
|
||||
@@ -21,6 +21,7 @@ import {
|
||||
|
||||
import { isRemoteRuntimePtyId } from './paired-parked-terminal-restore'
|
||||
import { TRANSPORT_CONNECT_SETTLE_GRACE_MS } from './pty-connect-limits'
|
||||
import { shouldRetainDisposedPaneSpawn } from './disposed-spawn-retention'
|
||||
|
||||
import type { ConnectPanePtySession } from './connect-pane-pty-session'
|
||||
import { resolveTerminalInlineImagesEnabled } from '../../../../../shared/terminal-inline-images-settings'
|
||||
@@ -112,6 +113,14 @@ export function installPtyInputRecovery(session: ConnectPanePtySession): void {
|
||||
onPtyExit: session.onExit,
|
||||
onPtySpawn: session.onPtySpawn,
|
||||
onPtyRebind: session.onPtyRebind,
|
||||
retainDisposedSpawn: () =>
|
||||
shouldRetainDisposedPaneSpawn(
|
||||
useAppStore.getState(),
|
||||
session.deps.worktreeId,
|
||||
session.deps.tabId,
|
||||
session.pane.leafId,
|
||||
session.executionHostId
|
||||
),
|
||||
...(session.mainSideEffectAuthority
|
||||
? {}
|
||||
: {
|
||||
|
||||
@@ -169,6 +169,46 @@ describe('createIpcPtyTransport', () => {
|
||||
expect(transport.getPtyId()).toBeNull()
|
||||
})
|
||||
|
||||
// A pane remounted mid-spawn is handed the same PTY by main's pane-spawn reservation, so a kill
|
||||
// from the disposed transport lands on the successor's shell. The surface owner decides.
|
||||
it('keeps a fresh spawn that resolves after destroy when the pane surface still owns it', async () => {
|
||||
const { createIpcPtyTransport } = await import('./pty-transport')
|
||||
const kill = vi.mocked(window.api.pty.kill)
|
||||
let resolveSpawn: (result: { id: string }) => void = () => {}
|
||||
vi.mocked(window.api.pty.spawn).mockImplementation(
|
||||
() =>
|
||||
new Promise((resolve) => {
|
||||
resolveSpawn = resolve
|
||||
})
|
||||
)
|
||||
const transport = createIpcPtyTransport({ retainDisposedSpawn: () => true })
|
||||
const pending = transport.connect({ url: '', callbacks: {} })
|
||||
await transport.destroy?.()
|
||||
resolveSpawn({ id: 'pty-shared-with-successor' })
|
||||
|
||||
await expect(pending).resolves.toBeUndefined()
|
||||
expect(kill).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('still kills a fresh spawn that resolves after destroy when nothing owns the surface', async () => {
|
||||
const { createIpcPtyTransport } = await import('./pty-transport')
|
||||
const kill = vi.mocked(window.api.pty.kill)
|
||||
let resolveSpawn: (result: { id: string }) => void = () => {}
|
||||
vi.mocked(window.api.pty.spawn).mockImplementation(
|
||||
() =>
|
||||
new Promise((resolve) => {
|
||||
resolveSpawn = resolve
|
||||
})
|
||||
)
|
||||
const transport = createIpcPtyTransport({ retainDisposedSpawn: () => false })
|
||||
const pending = transport.connect({ url: '', callbacks: {} })
|
||||
await transport.destroy?.()
|
||||
resolveSpawn({ id: 'pty-orphaned' })
|
||||
|
||||
await expect(pending).resolves.toBeUndefined()
|
||||
expect(kill).toHaveBeenCalledWith('pty-orphaned')
|
||||
})
|
||||
|
||||
it('drops the exit observer when abandoning an obsolete reattach without killing it', async () => {
|
||||
const { createIpcPtyTransport } = await import('./pty-transport')
|
||||
const onPtyExit = vi.fn()
|
||||
|
||||
@@ -153,7 +153,10 @@ describe('createIpcPtyTransport', () => {
|
||||
const onDataCallback = vi.fn()
|
||||
const onExitCallback = vi.fn()
|
||||
spawn.mockResolvedValueOnce({ id: 'pty-fresh-fallback', sessionExpired: true })
|
||||
const transport = createIpcPtyTransport({ onPtySpawn })
|
||||
// Built the way installPtyInputRecovery builds it: every real pane supplies
|
||||
// retainDisposedSpawn, and for a live pane refusing its own id it answers "retain". The refusal
|
||||
// must kill anyway — this transport is the pane's only one, so nothing else owns the PTY.
|
||||
const transport = createIpcPtyTransport({ onPtySpawn, retainDisposedSpawn: () => true })
|
||||
|
||||
const result = await transport.connect({
|
||||
url: '',
|
||||
@@ -184,7 +187,7 @@ describe('createIpcPtyTransport', () => {
|
||||
const retirementError = new Error('provider shutdown refused')
|
||||
spawn.mockResolvedValueOnce({ id: 'pty-fresh-fallback', sessionExpired: true })
|
||||
kill.mockRejectedValueOnce(retirementError)
|
||||
const transport = createIpcPtyTransport({ onPtySpawn })
|
||||
const transport = createIpcPtyTransport({ onPtySpawn, retainDisposedSpawn: () => true })
|
||||
|
||||
await expect(
|
||||
transport.connect({
|
||||
|
||||
@@ -278,6 +278,9 @@ export type IpcPtyTransportOptions = {
|
||||
onPtyExit?: (ptyId: string, exitCode?: number) => void
|
||||
onTitleChange?: (title: string, rawTitle: string) => void
|
||||
onPtySpawn?: (ptyId: string) => void
|
||||
/** Asked when a fresh spawn resolves after this transport was destroyed: true keeps the PTY for
|
||||
* the pane's successor (disposed-spawn-retention.ts); absent or false kills it. */
|
||||
retainDisposedSpawn?: () => boolean
|
||||
/** Rebind an existing pane after its provider replaces the PTY identity. */
|
||||
onPtyRebind?: (ptyId: string, replacedPtyId: string, incarnationId?: string | null) => void
|
||||
onBell?: () => void
|
||||
|
||||
+50
@@ -0,0 +1,50 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { installIpcPtyWindow, restorePtySpecWindow } from './pty-transport-test-harness'
|
||||
|
||||
// Scan-22 repro: a new SSH tab whose pane remounts while its first pty:spawn is in flight.
|
||||
// Main's pane-spawn reservation hands the successor the SAME PTY id; the disposed
|
||||
// predecessor then killed it on resolve (tab closes on pty-exit, or goes input-dead).
|
||||
describe('scan22: disposed mid-spawn SSH transport vs successor on the same PTY', () => {
|
||||
const originalWindow = (globalThis as { window?: typeof window }).window
|
||||
beforeEach(() => {
|
||||
vi.resetModules()
|
||||
installIpcPtyWindow(originalWindow, {})
|
||||
})
|
||||
afterEach(() => restorePtySpecWindow(originalWindow))
|
||||
|
||||
it('does not kill the PTY the remounted successor is bound to', async () => {
|
||||
const { createIpcPtyTransport } = await import('./pty-transport')
|
||||
let resolveFirst: (value: { id: string }) => void = () => {}
|
||||
const spawn = vi.mocked(window.api.pty.spawn)
|
||||
spawn.mockImplementationOnce(
|
||||
() =>
|
||||
new Promise((resolve) => {
|
||||
resolveFirst = resolve
|
||||
})
|
||||
)
|
||||
// Successor is handed the same id by main's (worktree, connection, paneKey) reservation.
|
||||
spawn.mockResolvedValueOnce({ id: 'ssh-conn@@pty-7' })
|
||||
// As installPtyInputRecovery wires it: the tab and its leaf still exist in the store.
|
||||
const paneOptions = {
|
||||
connectionId: 'ssh-conn',
|
||||
worktreeId: 'wt',
|
||||
tabId: 'tab-2',
|
||||
leafId: 'leaf-a',
|
||||
retainDisposedSpawn: () => true
|
||||
}
|
||||
|
||||
const first = createIpcPtyTransport(paneOptions)
|
||||
const firstConnect = first.connect({ url: '', callbacks: {} })
|
||||
// Pane remount (generation bump / recovery / park flip) while spawn is in flight.
|
||||
first.destroy?.()
|
||||
|
||||
const successor = createIpcPtyTransport(paneOptions)
|
||||
await successor.connect({ url: '', callbacks: {} })
|
||||
expect(successor.getPtyId()).toBe('ssh-conn@@pty-7')
|
||||
|
||||
resolveFirst({ id: 'ssh-conn@@pty-7' })
|
||||
await firstConnect
|
||||
|
||||
expect(window.api.pty.kill).not.toHaveBeenCalledWith('ssh-conn@@pty-7')
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user