fix(terminal): preserve teardown ownership until confirmed

This commit is contained in:
Brennan Benson
2026-07-12 16:31:19 -07:00
parent 8ea0622587
commit 3dd79e7433
15 changed files with 455 additions and 145 deletions
+19 -15
View File
@@ -10627,7 +10627,7 @@ describe('registerPtyHandlers', () => {
expect(piClearPtyMock).toHaveBeenCalledWith(spawnResult.id)
})
it('disposes PTY listeners before manual kill IPC', async () => {
it('keeps PTY listeners until manual native kill is accepted', async () => {
const onDataDisposable = makeDisposable()
const onExitDisposable = makeDisposable()
// Why: hold a stable reference to the kill spy. On POSIX, destroyPtyProcess
@@ -10654,15 +10654,15 @@ describe('registerPtyHandlers', () => {
await handlers.get('pty:kill')!(null, { id: spawnResult.id })
expect(onDataDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan(
killSpy.mock.invocationCallOrder[0]
expect(killSpy.mock.invocationCallOrder[0]).toBeLessThan(
onDataDisposable.dispose.mock.invocationCallOrder[0]
)
expect(onExitDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan(
killSpy.mock.invocationCallOrder[0]
expect(killSpy.mock.invocationCallOrder[0]).toBeLessThan(
onExitDisposable.dispose.mock.invocationCallOrder[0]
)
})
it('disposes PTY listeners before runtime controller kill', async () => {
it('keeps PTY listeners until runtime-controller native kill is accepted', async () => {
const onDataDisposable = makeDisposable()
const onExitDisposable = makeDisposable()
const killSpy = vi.fn()
@@ -10695,11 +10695,11 @@ describe('registerPtyHandlers', () => {
}
expect(runtimeController.kill(spawnResult.id)).toBe(true)
expect(onDataDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan(
killSpy.mock.invocationCallOrder[0]
expect(killSpy.mock.invocationCallOrder[0]).toBeLessThan(
onDataDisposable.dispose.mock.invocationCallOrder[0]
)
expect(onExitDisposable.dispose.mock.invocationCallOrder[0]).toBeLessThan(
killSpy.mock.invocationCallOrder[0]
expect(killSpy.mock.invocationCallOrder[0]).toBeLessThan(
onExitDisposable.dispose.mock.invocationCallOrder[0]
)
})
@@ -10994,7 +10994,7 @@ describe('registerPtyHandlers', () => {
expect(ids).toContain(ptyB.id)
})
it('clears PTY state even when kill reports the process is already gone', async () => {
it('retains PTY state when native kill throws without an exit proof', async () => {
const proc = {
onData: vi.fn(() => makeDisposable()),
onExit: vi.fn(() => makeDisposable()),
@@ -11014,11 +11014,15 @@ describe('registerPtyHandlers', () => {
rows: 24
})) as { id: string }
await handlers.get('pty:kill')!(null, { id: spawnResult.id })
await expect(handlers.get('pty:kill')!(null, { id: spawnResult.id })).rejects.toThrow(
'already dead'
)
expect(await handlers.get('pty:hasChildProcesses')!(null, { id: spawnResult.id })).toBe(false)
expect(openCodeClearPtyMock).toHaveBeenCalledWith(spawnResult.id)
expect(piClearPtyMock).toHaveBeenCalledWith(spawnResult.id)
expect((await getLocalPtyProvider().listProcesses()).map(({ id }) => id)).toContain(
spawnResult.id
)
expect(openCodeClearPtyMock).not.toHaveBeenCalledWith(spawnResult.id)
expect(piClearPtyMock).not.toHaveBeenCalledWith(spawnResult.id)
})
describe('agent_started telemetry', () => {
+4
View File
@@ -3174,6 +3174,7 @@ export function registerPtyHandlers(
markNativeWindowsConptyPty(result.id)
}
const relayResultId = getRelayPtyId(args.connectionId, result.id)
const relayInstanceId = parseAppSshPtyId(result.id)?.relayInstanceId
const persistSshLease = (): void => {
if (!store || !args.connectionId) {
return
@@ -3183,6 +3184,7 @@ export function registerPtyHandlers(
store.upsertSshRemotePtyLease({
targetId: args.connectionId,
ptyId: relayResultId,
...(relayInstanceId ? { relayInstanceId } : {}),
...(typeof args.worktreeId === 'string' ? { worktreeId: args.worktreeId } : {}),
...(typeof args.tabId === 'string' ? { tabId: args.tabId } : {}),
...(typeof args.leafId === 'string' && isTerminalLeafId(args.leafId)
@@ -4153,6 +4155,7 @@ export function registerPtyHandlers(
}
}
const relayResultId = getRelayPtyId(args.connectionId, result.id)
const relayInstanceId = parseAppSshPtyId(result.id)?.relayInstanceId
if (store && args.connectionId) {
// Why: remote PTYs live in the SSH relay grace window after Orca
// detaches. Persist their IDs immediately so reconnect can reattach
@@ -4160,6 +4163,7 @@ export function registerPtyHandlers(
store.upsertSshRemotePtyLease({
targetId: args.connectionId,
ptyId: relayResultId,
...(relayInstanceId ? { relayInstanceId } : {}),
...(typeof args.worktreeId === 'string' ? { worktreeId: args.worktreeId } : {}),
...(typeof args.tabId === 'string' ? { tabId: args.tabId } : {}),
...(validatedLeafId ? { leafId: validatedLeafId } : {}),
+3 -2
View File
@@ -8783,12 +8783,12 @@ describe('Store', () => {
expect(session.terminalLayoutsByTabId.tab1.ptyIdsByLeafId).toEqual({})
})
it('stores scoped SSH remote PTY leases as raw relay ids', async () => {
it('stores scoped SSH remote PTY leases as raw ids with their relay generation', async () => {
const store = await createStore()
store.upsertSshRemotePtyLease({
targetId: 'ssh-1',
ptyId: 'ssh:ssh-1@@remote-pty',
ptyId: 'ssh:ssh-1@@relay-generation@@remote-pty',
state: 'attached'
})
@@ -8796,6 +8796,7 @@ describe('Store', () => {
expect.objectContaining({
targetId: 'ssh-1',
ptyId: 'remote-pty',
relayInstanceId: 'relay-generation',
state: 'attached'
})
])
+4 -2
View File
@@ -116,7 +116,7 @@ import {
toSshExecutionHostId,
type ExecutionHostId
} from '../shared/execution-host'
import { toRelaySshPtyId } from './providers/ssh-pty-id'
import { parseAppSshPtyId, toRelaySshPtyId } from './providers/ssh-pty-id'
import {
migrateUiHostScopeSshTargetId,
migrateWorkspaceSessionSshTargetId
@@ -6317,8 +6317,10 @@ export class Store {
if (normalizedLease.leafId !== undefined && !isTerminalLeafId(normalizedLease.leafId)) {
delete normalizedLease.leafId
}
const parsedPtyId = parseAppSshPtyId(normalizedLease.ptyId)
normalizedLease.relayInstanceId ??= parsedPtyId?.relayInstanceId
// Why: app-facing SSH PTY ids are globally scoped; durable relay leases
// stay target-local so reconnect can call relay pty.attach with raw ids.
// keep raw ids plus their boot generation so reconnect can reject reuse.
normalizedLease.ptyId = this.getRelayPtyIdForSshLeaseStorage(
normalizedLease.targetId,
normalizedLease.ptyId
@@ -980,6 +980,27 @@ describe('LocalPtyProvider', () => {
expect(destroySpy).not.toHaveBeenCalled()
})
it('retains Windows ownership when native shutdown throws so retry can succeed', async () => {
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' })
const kill = vi.fn().mockImplementationOnce(() => {
throw new Error('native close failed')
})
spawnMock.mockReturnValue({ ...mockProc, kill })
const { id } = await provider.spawn({ cols: 80, rows: 24 })
await expect(provider.shutdown(id, { immediate: true })).rejects.toThrow(
'native close failed'
)
expect(provider.hasPty(id)).toBe(true)
expect(await provider.listProcesses()).toEqual(
expect.arrayContaining([expect.objectContaining({ id })])
)
await expect(provider.shutdown(id, { immediate: true })).resolves.toBeUndefined()
expect(kill).toHaveBeenCalledTimes(2)
expect(provider.hasPty(id)).toBe(false)
})
it('cancels pending shell-ready startup delivery on forced shutdown', async () => {
vi.useFakeTimers()
try {
+28 -11
View File
@@ -145,6 +145,9 @@ function disposePtyListeners(id: string): void {
}
}
const ptyShutdownInProgress = new Set<string>()
const ptyExitDuringShutdown = new Map<string, number>()
function runPtyCleanup(id: string): void {
const cleanup = ptyCleanupCallbacks.get(id)
if (!cleanup) {
@@ -186,6 +189,8 @@ function clearPtyState(id: string): void {
ptyAgentForegroundContextPaths.delete(id)
ptyTerminalHandle.delete(id)
ptyLoadGeneration.delete(id)
ptyShutdownInProgress.delete(id)
ptyExitDuringShutdown.delete(id)
}
/**
@@ -797,6 +802,10 @@ export class LocalPtyProvider implements IPtyProvider {
if (process.platform !== 'win32') {
;(proc as unknown as { kill: (sig?: string) => void }).kill = () => {}
}
if (ptyShutdownInProgress.has(id)) {
ptyExitDuringShutdown.set(id, exitCode)
return
}
if (shellReadyTimeout) {
clearTimeout(shellReadyTimeout)
shellReadyTimeout = null
@@ -893,23 +902,31 @@ export class LocalPtyProvider implements IPtyProvider {
if (!proc) {
return
}
// Why: disposePtyListeners removes the onExit callback, so the natural
// exit cleanup path from node-pty won't fire. Cleanup and notification
// must happen unconditionally after the try/catch.
// Timer/writer cleanup must happen here too: disposing listeners prevents
// the natural onExit callback from running the usual clearPtyState path.
runPtyCleanup(id)
disposePtyListeners(id)
// Why: keep listeners live until native shutdown is accepted so a thrown
// kill retains retry ownership. The in-progress marker makes a synchronous
// onExit an independent death proof without racing duplicate cleanup.
ptyShutdownInProgress.add(id)
let exitCode = -1
try {
proc.kill()
} catch {
/* Process may already be dead */
} catch (error) {
if (!ptyExitDuringShutdown.has(id)) {
throw error
}
} finally {
ptyShutdownInProgress.delete(id)
exitCode = ptyExitDuringShutdown.get(id) ?? -1
ptyExitDuringShutdown.delete(id)
}
// Timer/writer cleanup happens only after native shutdown was accepted;
// disposing first would suppress the natural exit path on a thrown kill.
runPtyCleanup(id)
disposePtyListeners(id)
destroyPtyProcess(proc, { alreadyKilled: true })
clearPtyState(id)
this.opts.onExit?.(id, -1)
this.opts.onExit?.(id, exitCode)
for (const cb of exitListeners) {
cb({ id, code: -1 })
cb({ id, code: exitCode })
}
}
+38
View File
@@ -22258,7 +22258,11 @@ describe('OrcaRuntimeService', () => {
await expect(runtime.createTerminal(`id:${TEST_WORKTREE_ID}`)).rejects.toThrow(
'terminal_admission_blocked_for_project_removal'
)
await expect(
runtime.createManagedWorktree({ repoSelector: `id:${TEST_REPO_ID}`, name: 'too-late' })
).rejects.toThrow('project_removal_in_progress')
expect(spawn).not.toHaveBeenCalled()
expect(addWorktree).not.toHaveBeenCalled()
runtime.onPtyExit('pty-1', -1)
stop.resolve(true)
@@ -22301,6 +22305,40 @@ describe('OrcaRuntimeService', () => {
expect(removeProject).toHaveBeenCalledWith(TEST_REPO_ID)
})
it('drains a create-first managed worktree before project removal snapshots ownership', async () => {
const removeProject = vi.fn()
const runtime = new OrcaRuntimeService({ ...store, removeProject } as never)
const runCreate = deferred<never>()
const runtimeWithCreate = runtime as unknown as {
runCreateManagedWorktree: () => Promise<never>
}
const runCreateSpy = vi
.spyOn(runtimeWithCreate, 'runCreateManagedWorktree')
.mockReturnValue(runCreate.promise)
runtime.setPtyController({
write: () => true,
kill: () => false,
getForegroundProcess: async () => null,
listProcesses: async () => []
})
runtime.attachWindow(1)
runtime.syncWindowGraph(1, { tabs: [], leaves: [] })
const create = runtime.createManagedWorktree({
repoSelector: `id:${TEST_REPO_ID}`,
name: 'create-first'
})
await vi.waitFor(() => expect(runCreateSpy).toHaveBeenCalledOnce())
const removal = runtime.removeProject(`id:${TEST_REPO_ID}`)
await Promise.resolve()
expect(removeProject).not.toHaveBeenCalled()
runCreate.resolve(undefined as never)
await expect(create).resolves.toBeUndefined()
await expect(removal).resolves.toEqual({ removed: true })
expect(removeProject).toHaveBeenCalledWith(TEST_REPO_ID)
})
it.each([
['renderer-backed mobile create', 'mobile'],
['selector-less renderer create', 'global']
+111 -43
View File
@@ -2122,6 +2122,43 @@ async function hasLocalWorktreeBaseRef(
)
}
export type CreateManagedWorktreeArgs = {
repoSelector: string
name: string
baseBranch?: string
compareBaseRef?: string
branchNameOverride?: string
linkedIssue?: number | null
linkedPR?: number | null
linkedLinearIssue?: string
linkedLinearIssueWorkspaceId?: string | null
linkedLinearIssueOrganizationUrlKey?: string | null
linkedGitLabMR?: number | null
linkedGitLabIssue?: number | null
linkedBitbucketPR?: number | null
linkedAzureDevOpsPR?: number | null
linkedGiteaPR?: number | null
comment?: string
displayName?: string
telemetrySource?: WorkspaceCreateTelemetrySource
workspaceStatus?: string
manualOrder?: number
sparseCheckout?: { directories: string[]; presetId?: string }
pushTarget?: GitPushTarget
runHooks?: boolean
activate?: boolean
setupDecision?: 'run' | 'skip' | 'inherit'
createdWithAgent?: TuiAgent
startupAgent?: TuiAgent
startupPrompt?: string
pendingFirstAgentMessageRename?: boolean
automationProvenance?: AutomationWorkspaceProvenance
startup?: WorktreeStartupLaunch
startupDraft?: string
startupDraftPaste?: WorktreeStartupDraftPaste
lineage?: WorktreeLineageInput
}
export class OrcaRuntimeService {
private readonly runtimeId = randomUUID()
private readonly startedAt = Date.now()
@@ -2229,6 +2266,8 @@ export class OrcaRuntimeService {
// respect to paired/mobile terminal creation, or a late spawn is orphaned.
private terminalAdmissionBlockedWorktreeIds = new Set<string>()
private terminalAdmissionsByWorktreeId = new Map<string, Set<Promise<void>>>()
private projectRemovalBlockedRepoIds = new Set<string>()
private worktreeCreateAdmissionsByRepoId = new Map<string, Set<Promise<void>>>()
private titleObservationSequence = 0
private headlessTerminals = new Map<string, RuntimeHeadlessTerminal>()
private ptyOutputSequenceById = new Map<string, number>()
@@ -12247,21 +12286,71 @@ export class OrcaRuntimeService {
}
}
private beginWorktreeCreateAdmission(repoId: string): () => void {
if (this.projectRemovalBlockedRepoIds.has(repoId)) {
throw new Error('project_removal_in_progress')
}
let resolveDone = (): void => {}
const done = new Promise<void>((resolve) => {
resolveDone = resolve
})
const admissions = this.worktreeCreateAdmissionsByRepoId.get(repoId) ?? new Set()
admissions.add(done)
this.worktreeCreateAdmissionsByRepoId.set(repoId, admissions)
let released = false
return () => {
if (released) {
return
}
released = true
admissions.delete(done)
if (admissions.size === 0) {
this.worktreeCreateAdmissionsByRepoId.delete(repoId)
}
resolveDone()
}
}
private async withWorktreeCreateAdmission<T>(
repoId: string,
action: () => Promise<T>
): Promise<T> {
const release = this.beginWorktreeCreateAdmission(repoId)
try {
return await action()
} finally {
release()
}
}
private async drainWorktreeCreateAdmissions(repoId: string): Promise<void> {
await Promise.all([...(this.worktreeCreateAdmissionsByRepoId.get(repoId) ?? [])])
}
async removeProject(repoSelector: string): Promise<{ removed: true }> {
if (!this.store?.removeProject) {
throw new Error('runtime_unavailable')
}
const repo = await this.resolveRepoSelector(repoSelector)
const worktreeIds = (await this.listResolvedWorktrees())
.filter((worktree) => worktree.repoId === repo.id)
.map((worktree) => worktree.id)
if (worktreeIds.some((id) => this.terminalAdmissionBlockedWorktreeIds.has(id))) {
if (this.projectRemovalBlockedRepoIds.has(repo.id)) {
throw new Error('project_removal_in_progress')
}
for (const id of worktreeIds) {
this.terminalAdmissionBlockedWorktreeIds.add(id)
}
this.projectRemovalBlockedRepoIds.add(repo.id)
let worktreeIds: string[] = []
try {
// Why: a create that already resolved this repo can publish a new
// worktree and terminal after a one-time removal snapshot. Block later
// creates and let earlier ones finish before enumerating owned worktrees.
await this.drainWorktreeCreateAdmissions(repo.id)
worktreeIds = (await this.listResolvedWorktrees())
.filter((worktree) => worktree.repoId === repo.id)
.map((worktree) => worktree.id)
if (worktreeIds.some((id) => this.terminalAdmissionBlockedWorktreeIds.has(id))) {
throw new Error('project_removal_in_progress')
}
for (const id of worktreeIds) {
this.terminalAdmissionBlockedWorktreeIds.add(id)
}
// Why: a create that passed admission before the block can still have a
// provider spawn in flight. Let it register first so the fresh stop
// snapshot sees and terminates it.
@@ -12280,6 +12369,7 @@ export class OrcaRuntimeService {
for (const id of worktreeIds) {
this.terminalAdmissionBlockedWorktreeIds.delete(id)
}
this.projectRemovalBlockedRepoIds.delete(repo.id)
}
}
@@ -14604,42 +14694,20 @@ export class OrcaRuntimeService {
})
}
async createManagedWorktree(args: {
repoSelector: string
name: string
baseBranch?: string
compareBaseRef?: string
branchNameOverride?: string
linkedIssue?: number | null
linkedPR?: number | null
linkedLinearIssue?: string
linkedLinearIssueWorkspaceId?: string | null
linkedLinearIssueOrganizationUrlKey?: string | null
linkedGitLabMR?: number | null
linkedGitLabIssue?: number | null
linkedBitbucketPR?: number | null
linkedAzureDevOpsPR?: number | null
linkedGiteaPR?: number | null
comment?: string
displayName?: string
telemetrySource?: WorkspaceCreateTelemetrySource
workspaceStatus?: string
manualOrder?: number
sparseCheckout?: { directories: string[]; presetId?: string }
pushTarget?: GitPushTarget
runHooks?: boolean
activate?: boolean
setupDecision?: 'run' | 'skip' | 'inherit'
createdWithAgent?: TuiAgent
startupAgent?: TuiAgent
startupPrompt?: string
pendingFirstAgentMessageRename?: boolean
automationProvenance?: AutomationWorkspaceProvenance
startup?: WorktreeStartupLaunch
startupDraft?: string
startupDraftPaste?: WorktreeStartupDraftPaste
lineage?: WorktreeLineageInput
}): Promise<CreateWorktreeResult> {
async createManagedWorktree(args: CreateManagedWorktreeArgs): Promise<CreateWorktreeResult> {
if (!this.store) {
throw new Error('runtime_unavailable')
}
const repo = await this.resolveRepoSelector(args.repoSelector)
return await this.withWorktreeCreateAdmission(repo.id, () =>
this.runCreateManagedWorktree(args)
)
}
private async runCreateManagedWorktree(
args: CreateManagedWorktreeArgs
): Promise<CreateWorktreeResult> {
if (!this.store) {
throw new Error('runtime_unavailable')
}
@@ -0,0 +1,133 @@
import { beforeEach, describe, expect, it, vi } from 'vitest'
import { SshRelaySession } from './ssh-relay-session'
import { createMockDeps, mockDeploySuccess } from './ssh-relay-session-test-fixtures'
vi.mock('./ssh-relay-deploy', () => ({ deployAndLaunchRelay: vi.fn() }))
vi.mock('./ssh-channel-multiplexer', () => ({
SshChannelMultiplexer: class MockSshChannelMultiplexer {
notify = vi.fn()
request = vi.fn().mockResolvedValue([])
onNotification = vi.fn().mockReturnValue(() => {})
onRequest = vi.fn().mockReturnValue(() => {})
onDispose = vi.fn().mockReturnValue(() => {})
dispose = vi.fn()
isDisposed = vi.fn().mockReturnValue(false)
}
}))
vi.mock('../providers/ssh-pty-provider', () => ({
isSshPtyNotFoundError: () => false,
isSshPtyIdentityMismatchError: () => false,
SshPtyProvider: class MockSshPtyProvider {
onData = vi.fn().mockReturnValue(() => {})
onReplay = vi.fn().mockReturnValue(() => {})
onExit = vi.fn().mockReturnValue(() => {})
dispose = vi.fn()
}
}))
vi.mock('../providers/ssh-filesystem-provider', () => ({
SshFilesystemProvider: class MockSshFilesystemProvider {
dispose = vi.fn()
}
}))
vi.mock('../providers/ssh-git-provider', () => ({
SshGitProvider: class MockSshGitProvider {}
}))
vi.mock('../ipc/pty', () => ({
registerSshPtyProvider: vi.fn(),
unregisterSshPtyProvider: vi.fn(),
getSshPtyProvider: vi.fn(),
getPtyIdsForConnection: vi.fn().mockReturnValue([]),
clearPtyOwnershipForConnection: vi.fn(),
clearProviderPtyState: vi.fn(),
deletePtyOwnership: vi.fn(),
setPtyOwnership: vi.fn(),
answerStartupTerminalColorQueriesForPty: vi.fn((_id: string, data: string) => data)
}))
vi.mock('../providers/ssh-filesystem-dispatch', () => ({
registerSshFilesystemProvider: vi.fn(),
unregisterSshFilesystemProvider: vi.fn(),
getSshFilesystemProvider: vi.fn().mockReturnValue({ dispose: vi.fn() })
}))
vi.mock('../providers/ssh-git-dispatch', () => ({
registerSshGitProvider: vi.fn(),
unregisterSshGitProvider: vi.fn()
}))
const { getSshPtyProvider, getPtyIdsForConnection, setPtyOwnership } = await import('../ipc/pty')
describe('SSH relay generation reattach', () => {
beforeEach(() => {
vi.clearAllMocks()
mockDeploySuccess()
})
it('reattaches a durable lease only within the relay generation that minted it', async () => {
const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps()
const attachForReconnect = vi.fn().mockResolvedValue({})
vi.mocked(getSshPtyProvider).mockReturnValue({
attachForReconnect,
getRelayInstanceId: () => 'relay-generation',
dispose: vi.fn()
} as unknown as ReturnType<typeof getSshPtyProvider>)
vi.mocked(getPtyIdsForConnection).mockReturnValue([])
vi.mocked(mockStore.getSshRemotePtyLeases).mockReturnValue([
{
targetId: 'target-1',
ptyId: 'pty-live',
relayInstanceId: 'relay-generation',
state: 'detached'
},
{ targetId: 'target-1', ptyId: 'pty-expired', state: 'expired' }
] as ReturnType<typeof mockStore.getSshRemotePtyLeases>)
const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward)
await session.establish(mockConn)
expect(attachForReconnect).toHaveBeenCalledWith('pty-live')
expect(attachForReconnect).not.toHaveBeenCalledWith('pty-expired')
expect(setPtyOwnership).toHaveBeenCalledWith(
'ssh:target-1@@relay-generation@@pty-live',
'target-1'
)
})
it('expires an older-generation lease before its recycled relay id can attach', async () => {
const { mockConn, mockStore, mockPortForward, getMainWindow, mockWindow } = createMockDeps()
const attachForReconnect = vi.fn()
vi.mocked(getSshPtyProvider).mockReturnValue({
attachForReconnect,
getRelayInstanceId: () => 'relay-current',
dispose: vi.fn()
} as unknown as ReturnType<typeof getSshPtyProvider>)
vi.mocked(getPtyIdsForConnection).mockReturnValue([])
vi.mocked(mockStore.getSshRemotePtyLeases).mockReturnValue([
{
targetId: 'target-1',
ptyId: 'pty-recycled',
relayInstanceId: 'relay-old',
state: 'detached'
}
] as ReturnType<typeof mockStore.getSshRemotePtyLeases>)
const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward)
await session.establish(mockConn)
expect(attachForReconnect).not.toHaveBeenCalled()
expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith(
'target-1',
'pty-recycled',
'expired'
)
expect(mockWindow.webContents.send).toHaveBeenCalledWith('pty:exit', {
id: 'ssh:target-1@@relay-old@@pty-recycled',
code: -1
})
})
})
-26
View File
@@ -556,32 +556,6 @@ describe('SshRelaySession', () => {
expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith('target-1', 'pty-1', 'attached')
})
it('establish re-attaches durable leases after app restart', async () => {
const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps()
const { getSshPtyProvider } = await import('../ipc/pty')
const mockAttach = vi.fn().mockResolvedValue(undefined)
vi.mocked(getSshPtyProvider).mockReturnValue({
attachForReconnect: mockAttach,
getRelayInstanceId: () => 'relay-generation',
dispose: vi.fn()
} as unknown as ReturnType<typeof getSshPtyProvider>)
vi.mocked(getPtyIdsForConnection).mockReturnValue([])
vi.mocked(mockStore.getSshRemotePtyLeases).mockReturnValue([
{ targetId: 'target-1', ptyId: 'pty-live', state: 'detached' },
{ targetId: 'target-1', ptyId: 'pty-expired', state: 'expired' }
] as ReturnType<typeof mockStore.getSshRemotePtyLeases>)
const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward)
await session.establish(mockConn)
expect(mockAttach).toHaveBeenCalledWith('pty-live')
expect(mockAttach).not.toHaveBeenCalledWith('pty-expired')
const expectedAppPtyId = 'ssh:target-1@@relay-generation@@pty-live'
expect(setPtyOwnership).toHaveBeenCalledWith(expectedAppPtyId, 'target-1')
expect(mockStore.markSshRemotePtyLease).toHaveBeenCalledWith('target-1', 'pty-live', 'attached')
})
it('forwards a lease tab identity to reattach so a reset relay cannot cross-wire it', async () => {
const { mockConn, mockStore, mockPortForward, getMainWindow } = createMockDeps()
const { getSshPtyProvider } = await import('../ipc/pty')
+46 -16
View File
@@ -19,7 +19,7 @@ import {
isSshPtyIdentityMismatchError,
isSshPtyNotFoundError
} from '../providers/ssh-pty-provider'
import { toAppSshPtyId, toRelaySshPtyId } from '../providers/ssh-pty-id'
import { parseAppSshPtyId, toAppSshPtyId, toRelaySshPtyId } from '../providers/ssh-pty-id'
import { SshFilesystemProvider } from '../providers/ssh-filesystem-provider'
import { SshGitProvider } from '../providers/ssh-git-provider'
import { agentHookServer } from '../agent-hooks/server'
@@ -1200,16 +1200,43 @@ export class SshRelaySession {
}
}
private expireReattachPty(relayPtyId: string, appPtyId: string): void {
clearProviderPtyState(appPtyId)
deletePtyOwnership(appPtyId)
this.forwardedReattachReplayByPty.delete(appPtyId)
this.store.markSshRemotePtyLease(this.targetId, relayPtyId, 'expired')
const win = this.getMainWindow()
if (win && !win.isDestroyed()) {
win.webContents.send('pty:exit', { id: appPtyId, code: -1 })
}
}
private async reattachKnownPtys(shouldContinue: () => boolean): Promise<void> {
const ptyProvider = getSshPtyProvider(this.targetId) as SshPtyProvider | undefined
if (!ptyProvider) {
return
}
const relayInstanceId = ptyProvider.getRelayInstanceId?.()
const activeLeases = this.store
.getSshRemotePtyLeases(this.targetId)
.filter((lease) => lease.state !== 'terminated' && lease.state !== 'expired')
const leasedPtyIds = activeLeases.map((lease) => lease.ptyId)
const attachableLeases = activeLeases.filter((lease) => {
if (!lease.relayInstanceId || !relayInstanceId || lease.relayInstanceId === relayInstanceId) {
return true
}
const staleAppPtyId = toAppSshPtyId(this.targetId, lease.ptyId, lease.relayInstanceId)
console.warn(
`[ssh-relay-session] Expiring stale PTY ${lease.ptyId} for ${this.targetId} after relay generation changed`
)
this.expireReattachPty(lease.ptyId, staleAppPtyId)
return false
})
const leasedPtyIds = attachableLeases.map((lease) => lease.ptyId)
// Why: carry each lease's pane identity into the attach so the relay can
// reject cross-generation id collisions even between split panes in one tab.
// Older leases may lack leafId, so tabId remains the compatibility fallback.
const expectedIdentityByPtyId = new Map(
activeLeases
attachableLeases
.map((lease): [string, ExpectedPtyIdentity] | null => {
const expected = expectedIdentityForLease(lease)
return expected ? [lease.ptyId, expected] : null
@@ -1218,19 +1245,22 @@ export class SshRelaySession {
)
// Why: after app restart, ptyOwnership is empty but durable SSH leases
// still describe remote PTYs that survived in the relay grace window.
const ptyIds = Array.from(
new Set([
...getPtyIdsForConnection(this.targetId).map((ptyId) =>
toRelaySshPtyId(this.targetId, ptyId)
),
...leasedPtyIds
])
)
const ptyProvider = getSshPtyProvider(this.targetId) as SshPtyProvider | undefined
if (!ptyProvider) {
return
}
const relayInstanceId = ptyProvider.getRelayInstanceId?.()
const ownedPtyIds = getPtyIdsForConnection(this.targetId).flatMap((appPtyId) => {
const parsed = parseAppSshPtyId(appPtyId)
if (
parsed?.relayInstanceId &&
relayInstanceId &&
parsed.relayInstanceId !== relayInstanceId
) {
console.warn(
`[ssh-relay-session] Expiring stale PTY ${parsed.relayPtyId} for ${this.targetId} after relay generation changed`
)
this.expireReattachPty(parsed.relayPtyId, appPtyId)
return []
}
return [toRelaySshPtyId(this.targetId, appPtyId)]
})
const ptyIds = Array.from(new Set([...ownedPtyIds, ...leasedPtyIds]))
for (const ptyId of ptyIds) {
if (!shouldContinue()) {
return
@@ -16,7 +16,7 @@ describe('PTY kill retry ownership', () => {
vi.restoreAllMocks()
})
it('bounds retained ids and retries at most one per lifecycle event', async () => {
it('retains every live id and retries at most one per lifecycle event', async () => {
const kill = vi.fn().mockRejectedValue(new Error('provider disconnected'))
vi.stubGlobal('window', { api: { pty: { kill } } })
vi.spyOn(console, 'warn').mockImplementation(() => {})
@@ -26,12 +26,15 @@ describe('PTY kill retry ownership', () => {
}
kill.mockResolvedValue(undefined)
retryRetainedPtyKills()
await Promise.resolve()
await Promise.resolve()
for (const _id of IDS) {
retryRetainedPtyKills()
await Promise.resolve()
await Promise.resolve()
}
expect(kill).toHaveBeenCalledTimes(66)
expect(kill.mock.calls.filter(([id]) => id === IDS[0])).toHaveLength(1)
expect(kill.mock.calls.filter(([id]) => id === IDS[1])).toHaveLength(2)
expect(kill).toHaveBeenCalledTimes(IDS.length * 2)
for (const id of IDS) {
expect(kill.mock.calls.filter(([calledId]) => calledId === id)).toHaveLength(2)
}
})
})
@@ -11,7 +11,6 @@ export type PtyKillIdentity = {
expectedTabId?: string
}
const MAX_RETAINED_PTY_KILLS = 64
const MAX_RETRY_BACKOFF_MS = 30_000
const retainedPtyKills = new Map<string, RetainedPtyKill>()
@@ -26,13 +25,9 @@ export function killPtyRetainingRetryOwnership(
): Promise<void> {
let retained = retainedPtyKills.get(id)
if (!retained) {
if (retainedPtyKills.size >= MAX_RETAINED_PTY_KILLS) {
const oldestId = retainedPtyKills.keys().next().value
if (typeof oldestId === 'string') {
retainedPtyKills.delete(oldestId)
console.warn('[pty] Dropped oldest retained shutdown retry at bounded capacity')
}
}
// Why: every entry represents a provider process whose shutdown is still
// unconfirmed. Dropping one to cap bookkeeping would orphan the real PTY;
// repeated failures for the same PTY still coalesce into this single record.
retained = { diagnostic, inFlight: null, options, attempts: 0, nextRetryAt: 0 }
}
retained.diagnostic = diagnostic
+2
View File
@@ -119,6 +119,8 @@ export type SshRemotePtyLeaseState = 'attached' | 'detached' | 'terminated' | 'e
export type SshRemotePtyLease = {
targetId: string
ptyId: string
/** Relay boot that minted ptyId; absent only on legacy persisted leases. */
relayInstanceId?: string
worktreeId?: string
tabId?: string
leafId?: string