fix(terminal): kill floating, setup, and folder-workspace PTYs on tab close (#10810)

This commit is contained in:
Neil 2026-07-26 18:16:43 -07:00 committed by GitHub
parent 4ada3f8b2c
commit 29c40e3353
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 345 additions and 39 deletions

View File

@ -3,7 +3,10 @@ import type { AppState } from '@/store/types'
import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../shared/constants'
import { brandEphemeralSetupTerminalWorktreeId } from '../../../shared/ephemeral-setup-terminal-worktree-id'
import { folderWorkspaceKey } from '../../../shared/workspace-scope'
import { resolveTerminalWorktreeRoute } from './terminal-worktree-route'
import {
resolveTerminalHostOwnership,
resolveTerminalWorktreeRoute
} from './terminal-worktree-route'
const EPHEMERAL_ID = brandEphemeralSetupTerminalWorktreeId(
'settings-mobile-emulator-orca-cli-skill-terminal'
@ -70,3 +73,107 @@ describe('resolveTerminalWorktreeRoute', () => {
expect(resolveTerminalWorktreeRoute(localState(), folderWorkspaceKey('abc-123'))).not.toBeNull()
})
})
// STA-2639: teardown must follow the PTY's real host while spawn stays free to prefer a focused
// runtime. Both purposes are asserted per shape so they cannot silently drift into agreement.
describe('resolveTerminalHostOwnership teardown', () => {
const focusedState = (overrides: Partial<AppState> = {}): AppState =>
localState({
settings: { activeRuntimeEnvironmentId: 'hub-a' },
runtimeEnvironments: [{ id: 'hub-a' }],
...overrides
} as unknown as Partial<AppState>)
it('scopes an ephemeral setup terminal to the runtime for spawn but not for teardown', () => {
const state = focusedState()
expect(resolveTerminalHostOwnership(state, EPHEMERAL_ID, 'spawn')).toEqual({
kind: 'runtime',
runtimeEnvironmentId: 'hub-a'
})
expect(resolveTerminalHostOwnership(state, EPHEMERAL_ID, 'teardown')).toEqual({
kind: 'local-or-ssh',
runtimeEnvironmentId: null
})
})
it('keeps a plain local folder workspace local for teardown while a runtime is focused', () => {
const state = focusedState({
folderWorkspaces: [{ id: 'fw-1', projectGroupId: 'pg-1', connectionId: null }],
projectGroups: [{ id: 'pg-1', connectionId: null, executionHostId: null }]
} as unknown as Partial<AppState>)
expect(resolveTerminalHostOwnership(state, folderWorkspaceKey('fw-1'), 'teardown')).toEqual({
kind: 'local-or-ssh',
runtimeEnvironmentId: null
})
})
it('keeps a HUB-owned folder workspace on its runtime for teardown', () => {
const state = focusedState({
folderWorkspaces: [{ id: 'fw-1', projectGroupId: 'pg-1', connectionId: null }],
projectGroups: [{ id: 'pg-1', connectionId: null, executionHostId: 'runtime:hub-a' }]
} as unknown as Partial<AppState>)
expect(resolveTerminalHostOwnership(state, folderWorkspaceKey('fw-1'), 'teardown')).toEqual({
kind: 'runtime',
runtimeEnvironmentId: 'hub-a'
})
})
it('keeps a hostless worktree on the focused runtime for teardown too', () => {
// Why: unlike the host-agnostic surfaces, an ownerless worktree row really does spawn on the
// focused HUB, and its wake hint is `ssh:`-shaped — downgrading it would kill the wrong host.
const state = focusedState({
repos: [{ id: 'repo-1', connectionId: null, executionHostId: null }],
worktreesByRepo: { 'repo-1': [{ id: 'repo-1::/w', repoId: 'repo-1' }] }
} as unknown as Partial<AppState>)
for (const purpose of ['spawn', 'teardown'] as const) {
expect(resolveTerminalHostOwnership(state, 'repo-1::/w', purpose)).toEqual({
kind: 'runtime',
runtimeEnvironmentId: 'hub-a'
})
}
})
it('keeps an explicitly runtime-owned worktree on its runtime for teardown', () => {
const state = focusedState({
worktreesByRepo: {
'repo-1': [{ id: 'repo-1::/w', repoId: 'repo-1', runtimeOwnerEnvironmentId: 'hub-a' }]
}
} as unknown as Partial<AppState>)
expect(resolveTerminalHostOwnership(state, 'repo-1::/w', 'teardown')).toEqual({
kind: 'runtime',
runtimeEnvironmentId: 'hub-a'
})
})
it('keeps an SSH-owned worktree killable by the paired client', () => {
const state = localState({
repos: [{ id: 'repo-1', connectionId: 'conn-1', executionHostId: 'ssh:conn-1' }],
worktreesByRepo: { 'repo-1': [{ id: 'repo-1::/w', repoId: 'repo-1', hostId: 'ssh:conn-1' }] }
} as unknown as Partial<AppState>)
expect(resolveTerminalHostOwnership(state, 'repo-1::/w', 'teardown')).toEqual({
kind: 'local-or-ssh',
runtimeEnvironmentId: null
})
})
it('fails a worktree with unparseable host metadata closed', () => {
// Why: a host string we cannot classify is not evidence of a local process.
const state = localState({
repos: [{ id: 'repo-1', connectionId: null, executionHostId: 'nonsense:' }],
worktreesByRepo: { 'repo-1': [{ id: 'repo-1::/w', repoId: 'repo-1', hostId: 'nonsense:' }] }
} as unknown as Partial<AppState>)
for (const purpose of ['spawn', 'teardown'] as const) {
expect(resolveTerminalHostOwnership(state, 'repo-1::/w', purpose)).toEqual({
kind: 'unresolved',
runtimeEnvironmentId: null
})
}
})
it('fails a missing worktree id closed for teardown', () => {
expect(resolveTerminalHostOwnership(localState(), null, 'teardown')).toEqual({
kind: 'unresolved',
runtimeEnvironmentId: null
})
})
})

View File

@ -1,8 +1,13 @@
import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../shared/constants'
import { isEphemeralSetupTerminalWorktreeId } from '../../../shared/ephemeral-setup-terminal-worktree-id'
import { parseExecutionHostId } from '../../../shared/execution-host'
import { parseWorkspaceKey } from '../../../shared/workspace-scope'
import type { AppState } from '@/store/types'
import { getRuntimeEnvironmentIdForWorktree } from './worktree-runtime-owner'
import {
getExplicitRuntimeEnvironmentIdForWorktree,
getRuntimeEnvironmentIdForWorktree,
type WorktreeRuntimeOwnerState
} from './worktree-runtime-owner'
import { resolveWorktreeOperationRouteResult } from './worktree-operation-route'
import { getSingleFocusedRuntimeEnvironmentId } from './single-runtime-legacy-owner'
@ -10,39 +15,129 @@ export type TerminalWorktreeRoute = {
runtimeEnvironmentId: string | null
}
export function resolveTerminalWorktreeRoute(
state: AppState,
worktreeId: string | null | undefined
): TerminalWorktreeRoute | null {
/**
* Which host owns a terminal surface. Spawn and teardown both resolve it here so they can never
* disagree about whether an id is routable #9994 fixed spawn only, and STA-2639 was the teardown
* half of that same asymmetry.
*/
export type TerminalHostOwnership =
| { kind: 'local-or-ssh'; runtimeEnvironmentId: null }
| { kind: 'runtime'; runtimeEnvironmentId: string }
| { kind: 'unresolved'; runtimeEnvironmentId: null }
/**
* `spawn` asks which host should start a new process, so a merely focused runtime is a fine guess.
* `teardown` asks which provider can kill an existing one, where that same guess strands the local
* PTY the surface was actually spawned on.
*/
export type TerminalHostOwnershipPurpose = 'spawn' | 'teardown'
const UNRESOLVED_TERMINAL_HOST: TerminalHostOwnership = {
kind: 'unresolved',
runtimeEnvironmentId: null
}
const LOCAL_OR_SSH_TERMINAL_HOST: TerminalHostOwnership = {
kind: 'local-or-ssh',
runtimeEnvironmentId: null
}
function ownershipForRuntimeEnvironmentId(
runtimeEnvironmentId: string | null
): TerminalHostOwnership {
return runtimeEnvironmentId
? { kind: 'runtime', runtimeEnvironmentId }
: LOCAL_OR_SSH_TERMINAL_HOST
}
export function resolveTerminalHostOwnership(
state: WorktreeRuntimeOwnerState,
worktreeId: string | null | undefined,
purpose: TerminalHostOwnershipPurpose
): TerminalHostOwnership {
if (!worktreeId) {
return { runtimeEnvironmentId: null }
// Why: a tab with no owning row proves nothing about its host, so teardown cannot claim its PTY.
return purpose === 'teardown' ? UNRESOLVED_TERMINAL_HOST : LOCAL_OR_SSH_TERMINAL_HOST
}
if (
worktreeId === FLOATING_TERMINAL_WORKTREE_ID ||
parseWorkspaceKey(worktreeId)?.type === 'folder'
) {
return { runtimeEnvironmentId: getRuntimeEnvironmentIdForWorktree(state, worktreeId) }
return resolveFloatingScopeOwnership(
state,
worktreeId,
purpose,
getRuntimeEnvironmentIdForWorktree(state, worktreeId)
)
}
// Why: inline setup/onboarding terminals (skill installs, feature tips) have no worktree row,
// so the strict owner resolver reports them as an unresolved cross-host worktree. Scope them to
// the active runtime — so a remote skill install lands on that runtime — falling back to local
// when none is focused, instead of failing them closed.
if (isEphemeralSetupTerminalWorktreeId(worktreeId)) {
return { runtimeEnvironmentId: getSingleFocusedRuntimeEnvironmentId(state) }
return resolveFloatingScopeOwnership(
state,
worktreeId,
purpose,
getSingleFocusedRuntimeEnvironmentId(state)
)
}
const resolution = resolveWorktreeOperationRouteResult(state, worktreeId)
if (resolution.kind === 'resolved') {
return { runtimeEnvironmentId: resolution.route.runtimeEnvironmentId }
if (resolution.route.runtimeEnvironmentId) {
// Why: a real worktree row keeps its runtime owner on teardown. Unlike the host-agnostic
// surfaces above, its HUB-native wake hints are `ssh:`-shaped rather than `remote:`-prefixed,
// so downgrading to local here would kill a paired-client PTY that lives on the HUB (#9994).
return { kind: 'runtime', runtimeEnvironmentId: resolution.route.runtimeEnvironmentId }
}
const parsed = parseExecutionHostId(resolution.route.executionHostId)
return parsed?.kind === 'local' || parsed?.kind === 'ssh'
? LOCAL_OR_SSH_TERMINAL_HOST
: UNRESOLVED_TERMINAL_HOST
}
if (
purpose === 'spawn' &&
state.worktreesByRepo === undefined &&
state.detectedWorktreesByRepo === undefined &&
state.repos === undefined
) {
// Why: narrow unit/legacy adapters can omit all owner catalogs; production stores always provide them and still fail closed above.
return { runtimeEnvironmentId: getSingleFocusedRuntimeEnvironmentId(state) }
// Why: narrow unit/legacy adapters can omit all owner catalogs; production stores always provide
// them and still fail closed above. Teardown never takes this fail-open — it would kill on a guess.
return ownershipForRuntimeEnvironmentId(getSingleFocusedRuntimeEnvironmentId(state))
}
return null
return UNRESOLVED_TERMINAL_HOST
}
/**
* Host ownership for the host-agnostic surfaces floating, folder workspace, ephemeral setup.
* Safe to downgrade on teardown because these never hold an `ssh:`-shaped HUB wake hint: a
* runtime-hosted one carries a `remote:` id, which is routed before ownership is consulted.
*/
function resolveFloatingScopeOwnership(
state: WorktreeRuntimeOwnerState,
worktreeId: string,
purpose: TerminalHostOwnershipPurpose,
runtimeEnvironmentId: string | null
): TerminalHostOwnership {
if (
purpose === 'spawn' ||
!runtimeEnvironmentId ||
getExplicitRuntimeEnvironmentIdForWorktree(state, worktreeId)
) {
return ownershipForRuntimeEnvironmentId(runtimeEnvironmentId)
}
// Why: this surface publishes no runtime owner and only looks runtime-owned because exactly one
// runtime is focused — a guess that flips as catalogs hydrate, stranding the local PTY (STA-2639).
return LOCAL_OR_SSH_TERMINAL_HOST
}
export function resolveTerminalWorktreeRoute(
state: AppState,
worktreeId: string | null | undefined
): TerminalWorktreeRoute | null {
const ownership = resolveTerminalHostOwnership(state, worktreeId, 'spawn')
return ownership.kind === 'unresolved'
? null
: { runtimeEnvironmentId: ownership.runtimeEnvironmentId }
}
export function hasUnroutableTerminalWorktreeOwner(state: AppState, worktreeId: string): boolean {

View File

@ -1,6 +1,9 @@
import { describe, expect, it } from 'vitest'
import type { SleepingAgentSessionRecord } from '../../../../shared/agent-session-resume'
import type { TerminalTab } from '../../../../shared/types'
import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../../shared/constants'
import { brandEphemeralSetupTerminalWorktreeId } from '../../../../shared/ephemeral-setup-terminal-worktree-id'
import { folderWorkspaceKey } from '../../../../shared/workspace-scope'
import {
buildTerminalTabRetirementPlan,
buildTerminalTabRetirementPlans,
@ -232,6 +235,115 @@ describe('terminal tab retirement planning', () => {
expect(plan.unroutablePtyIds).toEqual(['ssh:private@@pty-1'])
})
// STA-2639: these surfaces publish no runtime owner, so teardown read them as unresolved and
// dropped their ordinary local PTYs instead of killing them.
describe('host-agnostic terminal surfaces are killed, not dropped', () => {
const localSurfaces: [string, string][] = [
['floating terminal', FLOATING_TERMINAL_WORKTREE_ID],
['ephemeral setup terminal', brandEphemeralSetupTerminalWorktreeId('panel-1')],
['folder workspace', folderWorkspaceKey('fw-1')]
]
for (const [label, worktreeId] of localSurfaces) {
it(`kills a local ${label} PTY while a runtime is focused`, () => {
// Why: a focused runtime must not make a local surface read as runtime-owned — that focus
// also flips as the runtime catalog hydrates, so teardown cannot trust it.
const state = makeState({
settings: { activeRuntimeEnvironmentId: 'hub-a' },
runtimeEnvironments: [{ id: 'hub-a' }],
folderWorkspaces: [{ id: 'fw-1', projectGroupId: 'pg-1', connectionId: null }],
projectGroups: [{ id: 'pg-1', connectionId: null, executionHostId: null }],
tabsByWorktree: { [worktreeId]: [makeTab('tab-1', worktreeId, 'pty-1')] }
} as unknown as Partial<RetirementState>)
const plan = buildTerminalTabRetirementPlan(state, 'tab-1')
expect(plan.localOrSshPtyIds).toEqual(['pty-1'])
expect(plan.unroutablePtyIds).toEqual([])
})
}
it('closes a runtime-hosted floating terminal over RPC instead of killing it locally', () => {
const state = makeState({
settings: { activeRuntimeEnvironmentId: 'hub-a' },
runtimeEnvironments: [{ id: 'hub-a' }],
tabsByWorktree: {
[FLOATING_TERMINAL_WORKTREE_ID]: [
makeTab('tab-1', FLOATING_TERMINAL_WORKTREE_ID, 'remote:hub-a@@handle-1')
]
}
} as unknown as Partial<RetirementState>)
const plan = buildTerminalTabRetirementPlan(state, 'tab-1')
expect(plan.localOrSshPtyIds).toEqual([])
expect(plan.runtimeTerminals).toEqual([
{ ptyId: 'remote:hub-a@@handle-1', environmentId: 'hub-a', handle: 'handle-1' }
])
})
it('never kills a HUB-owned folder workspace PTY', () => {
const state = makeState({
folderWorkspaces: [{ id: 'fw-1', projectGroupId: 'pg-1', connectionId: null }],
projectGroups: [{ id: 'pg-1', connectionId: null, executionHostId: 'runtime:hub-a' }],
tabsByWorktree: {
[folderWorkspaceKey('fw-1')]: [makeTab('tab-1', folderWorkspaceKey('fw-1'), 'pty-hub')]
}
} as unknown as Partial<RetirementState>)
const plan = buildTerminalTabRetirementPlan(state, 'tab-1')
expect(plan.localOrSshPtyIds).toEqual([])
expect(plan.unroutablePtyIds).toEqual(['pty-hub'])
})
it('never kills a HUB wake hint on a worktree owned only by the focused runtime', () => {
// Why: an ownerless mixed-version row legitimately spawns on the focused HUB (see
// pty-connection "uses the focused runtime only for ownerless mixed-version publications"),
// and its wake hint is `ssh:`-shaped, not `remote:` — killing it would hit the wrong host.
const state = makeState({
repos: [{ id: 'repo1', connectionId: null }],
worktreesByRepo: { repo1: [{ id: 'wt-legacy', repoId: 'repo1' }] },
settings: { activeRuntimeEnvironmentId: 'legacy-hub' },
runtimeEnvironments: [{ id: 'legacy-hub' }],
tabsByWorktree: {
'wt-legacy': [makeTab('tab-1', 'wt-legacy', 'ssh:hub-private@@pty-2')]
}
} as unknown as Partial<RetirementState>)
const plan = buildTerminalTabRetirementPlan(state, 'tab-1')
expect(plan.localOrSshPtyIds).toEqual([])
expect(plan.unroutablePtyIds).toEqual(['ssh:hub-private@@pty-2'])
})
it('never kills a PTY whose owning tab row is already gone', () => {
// Why: a vanished row is the ambiguity #9994 guards — nothing proves which host holds the PTY.
const state = makeState({ ptyIdsByTabId: { 'tab-1': ['pty-ghost'] } })
const plan = buildTerminalTabRetirementPlan(state, 'tab-1')
expect(plan.localOrSshPtyIds).toEqual([])
expect(plan.unroutablePtyIds).toEqual(['pty-ghost'])
})
it('never kills an unknown worktree PTY when every owner catalog is absent', () => {
// Why: spawn falls back to the focused runtime (local when none) while catalogs load, but
// teardown taking that fail-open would kill an unidentified PTY on a guess.
const state = makeState({
worktreesByRepo: undefined,
detectedWorktreesByRepo: undefined,
repos: undefined,
tabsByWorktree: { 'wt-unknown': [makeTab('tab-1', 'wt-unknown', 'pty-1')] }
} as unknown as Partial<RetirementState>)
const plan = buildTerminalTabRetirementPlan(state, 'tab-1')
expect(plan.localOrSshPtyIds).toEqual([])
expect(plan.unroutablePtyIds).toEqual(['pty-1'])
})
})
it('deduplicates batch-owned PTYs while protecting owners outside the close set', () => {
const state = makeState({
tabsByWorktree: {

View File

@ -1,13 +1,13 @@
import type { SleepingAgentSessionRecord } from '../../../../shared/agent-session-resume'
import type { AppState } from '../types'
import {
getExecutionHostIdForWorktree,
getRuntimeEnvironmentIdForWorktree,
type WorktreeRuntimeOwnerState
} from '@/lib/worktree-runtime-owner'
import { parseRemoteRuntimePtyId } from '@/runtime/runtime-terminal-stream'
import { resolveWorktreeOperationRouteResult } from '@/lib/worktree-operation-route'
import { parseExecutionHostId } from '../../../../shared/execution-host'
import { resolveTerminalHostOwnership } from '@/lib/terminal-worktree-route'
import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../../shared/constants'
import { isEphemeralSetupTerminalWorktreeId } from '../../../../shared/ephemeral-setup-terminal-worktree-id'
import { parseWorkspaceKey } from '../../../../shared/workspace-scope'
export type TerminalTabCloseReason = 'user' | 'cleanup' | 'pty-exit'
@ -111,32 +111,20 @@ function hasOwnerOutsideTargets(
return false
}
function getProviderOwnership(
state: TerminalTabRetirementState,
/** Worktree-id shape for diagnostics; raw ids embed absolute paths and must not be logged. */
export function classifyTerminalRetirementWorktree(
worktreeId: string | null
): { kind: 'local-or-ssh' } | { kind: 'runtime'; environmentId: string } | { kind: 'unresolved' } {
): 'floating' | 'ephemeral-setup' | 'folder-workspace' | 'worktree' | 'absent' {
if (!worktreeId) {
return { kind: 'unresolved' }
return 'absent'
}
if (parseWorkspaceKey(worktreeId)?.type === 'folder') {
const parsed = parseExecutionHostId(getExecutionHostIdForWorktree(state, worktreeId))
return parsed?.kind === 'runtime'
? { kind: 'runtime', environmentId: parsed.environmentId }
: parsed?.kind === 'local' || parsed?.kind === 'ssh'
? { kind: 'local-or-ssh' }
: { kind: 'unresolved' }
if (worktreeId === FLOATING_TERMINAL_WORKTREE_ID) {
return 'floating'
}
const resolution = resolveWorktreeOperationRouteResult(state, worktreeId)
if (resolution.kind !== 'resolved') {
return { kind: 'unresolved' }
if (isEphemeralSetupTerminalWorktreeId(worktreeId)) {
return 'ephemeral-setup'
}
if (resolution.route.runtimeEnvironmentId) {
return { kind: 'runtime', environmentId: resolution.route.runtimeEnvironmentId }
}
const parsed = parseExecutionHostId(resolution.route.executionHostId)
return parsed?.kind === 'local' || parsed?.kind === 'ssh'
? { kind: 'local-or-ssh' }
: { kind: 'unresolved' }
return parseWorkspaceKey(worktreeId)?.type === 'folder' ? 'folder-workspace' : 'worktree'
}
export function isTerminalTabPresent(
@ -188,7 +176,7 @@ export function buildTerminalTabRetirementPlans(
const runtimeTerminals: TerminalTabRetirementPlan['runtimeTerminals'] = []
const cleanupOnlyPtyIds: string[] = []
const unroutablePtyIds: string[] = []
const providerOwnership = getProviderOwnership(state, worktreeId)
const providerOwnership = resolveTerminalHostOwnership(state, worktreeId, 'teardown')
for (const ptyId of ptyIds) {
const ownerIdentity = getTerminalPtyOwnershipIdentity(state, ptyId, worktreeId)

View File

@ -113,6 +113,7 @@ import {
} from './agent-status'
import {
buildTerminalTabRetirementPlan,
classifyTerminalRetirementWorktree,
isTerminalTabPresent,
removeSleepingAgentSessionsForTab,
type TerminalTabCloseReason,
@ -1190,8 +1191,11 @@ export const createTerminalSlice: StateCreator<AppState, [], [], TerminalSlice>
}
}
if (retirementPlan.unroutablePtyIds.length > 0) {
console.warn('[terminal-retirement] skipped unroutable runtime handles', {
// Why: log the worktree SHAPE, never the id — worktree ids embed absolute paths. The old
// "runtime handles" wording described ids that are usually plain local ones, hiding STA-2639.
console.warn('[terminal-retirement] skipped PTYs with no resolvable owner', {
tabId,
worktreeKind: classifyTerminalRetirementWorktree(retirementPlan.worktreeId),
count: retirementPlan.unroutablePtyIds.length
})
}