From 9f9b85004c7c6fdc3a7e503cdef6da4d6e403e2f Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Wed, 12 Aug 2026 23:02:32 -0700 Subject: [PATCH] fix(terminal): fence split source incarnation --- src/main/ipc/pty.test.ts | 12 +- src/main/ipc/pty.ts | 9 + src/main/persistence.test.ts | 55 +++++ ...a-runtime-terminal-split-authority.test.ts | 222 ++++++++++++++++-- src/main/runtime/orca-runtime.test.ts | 7 +- src/main/runtime/orca-runtime.ts | 54 +++-- 6 files changed, 324 insertions(+), 35 deletions(-) diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index e89a4c21806..44ca39429dd 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -6253,7 +6253,7 @@ describe('registerPtyHandlers', () => { expect(runtime.onPtyExit).toHaveBeenCalledWith('remote-pty', -1, undefined) }) - it('preserves an SSH lease when runtime controller kill shutdown fails transiently', async () => { + it('retires a rejected SSH PTY after generic kill shutdown fails transiently', async () => { const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) const store = { markSshRemotePtyLease: vi.fn() @@ -6296,22 +6296,30 @@ describe('registerPtyHandlers', () => { ) const controller = runtime.setPtyController.mock.calls[0]?.[0] as { kill: (ptyId: string) => boolean + retireRejectedPty: (ptyId: string) => void } try { expect(controller.kill('remote-pty')).toBe(true) await new Promise((resolve) => setImmediate(resolve)) + expect(store.markSshRemotePtyLease).not.toHaveBeenCalledWith( + 'ssh-1', + 'remote-pty', + 'terminated' + ) + controller.retireRejectedPty('remote-pty') } finally { warnSpy.mockRestore() deletePtyOwnership('remote-pty') } - expect(store.markSshRemotePtyLease).not.toHaveBeenCalledWith( + expect(store.markSshRemotePtyLease).toHaveBeenCalledWith( 'ssh-1', 'remote-pty', 'terminated' ) expect(runtime.onPtyExit).toHaveBeenCalledWith('remote-pty', -1, undefined) + expect(runtime.onPtyExit).toHaveBeenCalledWith('remote-pty', 0, undefined) }) it('strips ORCA_PANE_KEY/TAB_ID/WORKTREE_ID from SSH spawn env when remote agent hooks are disabled', async () => { diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index ddc829d2d13..9e52ebaf179 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -5506,6 +5506,15 @@ export function registerPtyHandlers( } return killWithCurrentProvider() }, + retireRejectedPty: (ptyId) => { + let connectionId: string | null | undefined = ptyOwnership.get(ptyId) + const parsedSshId = connectionId === undefined ? parseAppSshPtyId(ptyId) : null + connectionId ??= parsedSshId?.connectionId + const incarnationId = finishPtyShutdown(ptyId, connectionId, store) + runtime?.onPtyExit(ptyId, 0, incarnationId) + rememberSyntheticKillExit(ptyId) + sendPtyExitToRenderer({ id: ptyId, code: 0 }) + }, markReversibleStops: (ptyIds) => { for (const ptyId of ptyIds) { reversibleStopOwnersByPtyId.set(ptyId, (reversibleStopOwnersByPtyId.get(ptyId) ?? 0) + 1) diff --git a/src/main/persistence.test.ts b/src/main/persistence.test.ts index 06ee8a9d6a0..8283da99ffa 100644 --- a/src/main/persistence.test.ts +++ b/src/main/persistence.test.ts @@ -9357,6 +9357,61 @@ describe('Store', () => { }) }) + it('requires an expected split source incarnation in the owning host partition', async () => { + for (const hostId of [undefined, 'ssh:ssh-1']) { + const store = await createStore() + const sourceSession: WorkspaceSessionState = { + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-source' })] + }, + terminalLayoutsByTabId: { + tab1: { + root: { type: 'leaf', leafId: TEST_LEAF_1 }, + activeLeafId: TEST_LEAF_1, + expandedLeafId: null, + ptyIdsByLeafId: { [TEST_LEAF_1]: 'pty-source' } + } + } + } + if (hostId) { + store.setWorkspaceSession( + { + ...sourceSession, + terminalPtyIncarnationsByPaneKey: { + [`tab1:${TEST_LEAF_1}`]: 'live-incarnation' + } + }, + undefined + ) + } + store.setWorkspaceSession(sourceSession, hostId) + + expect( + store.persistPtyBinding( + { + worktreeId: 'wt1', + tabId: 'tab1', + leafId: TEST_LEAF_2, + ptyId: 'pty-split', + expectedSourceBinding: { + worktreeId: 'wt1', + tabId: 'tab1', + leafId: TEST_LEAF_1, + ptyId: 'pty-source', + incarnationId: 'live-incarnation' + } + }, + hostId + ) + ).toBe(false) + expect(store.getWorkspaceSession(hostId).terminalLayoutsByTabId.tab1.root).toEqual({ + type: 'leaf', + leafId: TEST_LEAF_1 + }) + } + }) + it('rejects competing PTY and incarnation changes during reconciliation', async () => { const store = await createStore() const paneKey = `tab1:${TEST_LEAF_1}` diff --git a/src/main/runtime/orca-runtime-terminal-split-authority.test.ts b/src/main/runtime/orca-runtime-terminal-split-authority.test.ts index c13f8825380..be07279c65b 100644 --- a/src/main/runtime/orca-runtime-terminal-split-authority.test.ts +++ b/src/main/runtime/orca-runtime-terminal-split-authority.test.ts @@ -69,35 +69,69 @@ function remoteSnapshot(): RuntimeMobileSessionTabsSnapshot { } } -function createHarness(includeSource = true) { +function createHarness( + includeSource = true, + options: { + connectionId?: string | null + deferReveal?: boolean + deferSpawn?: boolean + includePairedSnapshot?: boolean + rendererMounted?: boolean + sourceIncarnationId?: string + stopAndWaitResult?: boolean + } = {} +) { let session = persistedSession(includeSource) + const connectionId = options.connectionId ?? null + const ownerHostId = connectionId ? `ssh:${connectionId}` : 'local' + const requestedSessionHostIds: (string | undefined)[] = [] const repo = { id: REPO_ID, path: '/workspace', displayName: 'repo', badgeColor: 'blue', - addedAt: 1 + addedAt: 1, + ...(connectionId ? { connectionId } : {}) } const store = { getRepos: () => [repo], getRepo: (id: string) => (id === REPO_ID ? repo : undefined), - getWorkspaceSession: () => session, + getWorkspaceSession: (hostId?: string) => { + requestedSessionHostIds.push(hostId) + return hostId === undefined || hostId === ownerHostId ? session : getDefaultWorkspaceSession() + }, setWorkspaceSession: (next: WorkspaceSessionState) => { session = next }, persistPtyBinding: () => true } - const spawn = vi.fn(async () => ({ id: SPLIT_PTY_ID })) + let resolveSpawn: ((result: { id: string }) => void) | undefined + const spawn = options.deferSpawn + ? vi.fn( + () => + new Promise<{ id: string }>((resolve) => { + resolveSpawn = resolve + }) + ) + : vi.fn(async () => ({ id: SPLIT_PTY_ID })) const kill = vi.fn(() => true) - const revealTerminalSession = vi - .fn() - .mockRejectedValue(new Error(`Terminal tab ${TAB_ID} not found`)) + const retireRejectedPty = vi.fn() + const stopAndWait = vi.fn(async () => options.stopAndWaitResult ?? true) + let resolveReveal: ((result: { tabId: string }) => void) | undefined + const revealTerminalSession = options.deferReveal + ? vi.fn( + () => + new Promise<{ tabId: string }>((resolve) => { + resolveReveal = resolve + }) + ) + : vi.fn().mockRejectedValue(new Error(`Terminal tab ${TAB_ID} not found`)) const runtime = new OrcaRuntimeService(store as never) Object.assign(runtime, { resolveTerminalWorkspaceLaunchScope: vi.fn(async () => ({ id: WORKTREE_ID, path: '/workspace', - connectionId: null, + connectionId, repo, folderWorkspace: null })) @@ -106,35 +140,77 @@ function createHarness(includeSource = true) { spawn, write: () => true, kill, + retireRejectedPty, + ...(options.stopAndWaitResult !== undefined ? { stopAndWait } : {}), getForegroundProcess: async () => null }) runtime.setNotifier({ revealTerminalSession } as never) runtime.syncWindowGraph(1, { - tabs: [], - leaves: [], - mobileSessionTabs: includeSource ? [remoteSnapshot()] : [] + tabs: + includeSource && options.rendererMounted + ? [ + { + tabId: TAB_ID, + worktreeId: WORKTREE_ID, + title: 'Restored terminal', + activeLeafId: SOURCE_LEAF_ID, + layout: { type: 'leaf', leafId: SOURCE_LEAF_ID } as const + } + ] + : [], + leaves: + includeSource && options.rendererMounted + ? [ + { + tabId: TAB_ID, + worktreeId: WORKTREE_ID, + leafId: SOURCE_LEAF_ID, + paneRuntimeId: 1, + ptyId: SOURCE_PTY_ID + } + ] + : [], + mobileSessionTabs: (options.includePairedSnapshot ?? includeSource) ? [remoteSnapshot()] : [] }) - runtime.registerPty(SOURCE_PTY_ID, WORKTREE_ID, null, { + runtime.registerPty(SOURCE_PTY_ID, WORKTREE_ID, connectionId, { tabId: TAB_ID, - leafId: SOURCE_LEAF_ID + leafId: SOURCE_LEAF_ID, + ...(options.sourceIncarnationId ? { incarnationId: options.sourceIncarnationId } : {}) }) const internals = runtime as unknown as { - getTerminalHandleForPaneKey: (paneKey: string) => string | null issuePtyHandle: (pty: unknown) => string mobileSessionTabsByWorktree: Map ptysById: Map } - const handle = - internals.getTerminalHandleForPaneKey(makePaneKey(TAB_ID, SOURCE_LEAF_ID)) ?? - internals.issuePtyHandle(internals.ptysById.get(SOURCE_PTY_ID)) + const handle = internals.issuePtyHandle(internals.ptysById.get(SOURCE_PTY_ID)) return { runtime, handle, spawn, kill, + retireRejectedPty, + stopAndWait, revealTerminalSession, getSession: () => session, - getSnapshot: () => internals.mobileSessionTabsByWorktree.get(WORKTREE_ID) + getSnapshot: () => internals.mobileSessionTabsByWorktree.get(WORKTREE_ID), + requestedSessionHostIds, + replaceSourceIncarnation: (incarnationId: string) => + runtime.registerPty(SOURCE_PTY_ID, WORKTREE_ID, connectionId, { + tabId: TAB_ID, + leafId: SOURCE_LEAF_ID, + incarnationId + }), + replacePersistedSourceIncarnation: (incarnationId: string) => { + session = { + ...session, + terminalPtyIncarnationsByPaneKey: { + ...session.terminalPtyIncarnationsByPaneKey, + [makePaneKey(TAB_ID, SOURCE_LEAF_ID)]: incarnationId + } + } + }, + resolveReveal: () => resolveReveal?.({ tabId: TAB_ID }), + resolveSpawn: () => resolveSpawn?.({ id: SPLIT_PTY_ID }) } } @@ -185,4 +261,114 @@ describe('remote runtime terminal split authority', () => { expect.soft(harness.kill).not.toHaveBeenCalled() expect.soft(harness.revealTerminalSession).not.toHaveBeenCalled() }) + + it.each([ + { label: 'local', connectionId: null, expectedHostId: 'local' }, + { label: 'SSH', connectionId: 'ssh-1', expectedHostId: 'ssh:ssh-1' } + ])( + 'rejects a same-ID $label source replacement when restored persistence lacks an incarnation', + async ({ connectionId, expectedHostId }) => { + const harness = createHarness(true, { + connectionId, + deferSpawn: true, + includePairedSnapshot: false, + rendererMounted: true, + sourceIncarnationId: 'source-before' + }) + + const split = harness.runtime.splitTerminal(harness.handle, { direction: 'vertical' }) + await vi.waitFor(() => expect(harness.spawn).toHaveBeenCalledOnce()) + + expect( + harness.getSession().terminalPtyIncarnationsByPaneKey?.[makePaneKey(TAB_ID, SOURCE_LEAF_ID)] + ).toBeUndefined() + expect(harness.spawn).toHaveBeenCalledWith( + expect.objectContaining({ + expectedSourceBinding: expect.objectContaining({ + ptyId: SOURCE_PTY_ID, + incarnationId: 'source-before' + }) + }) + ) + + harness.replaceSourceIncarnation('source-after') + harness.resolveSpawn() + + await expect(split).rejects.toThrow('terminal_split_source_not_found') + expect(harness.kill).toHaveBeenCalledWith(SPLIT_PTY_ID) + expect(harness.requestedSessionHostIds).toContain(expectedHostId) + } + ) + + it('rejects a same-ID paired-runtime source replacement recovered without an incarnation map', async () => { + const harness = createHarness(true, { + deferSpawn: true, + includePairedSnapshot: true, + rendererMounted: false, + sourceIncarnationId: 'remote-before' + }) + + const split = harness.runtime.splitTerminal(harness.handle, { direction: 'horizontal' }) + await vi.waitFor(() => expect(harness.spawn).toHaveBeenCalledOnce()) + expect(harness.spawn).toHaveBeenCalledWith( + expect.objectContaining({ + expectedSourceBinding: expect.objectContaining({ incarnationId: 'remote-before' }) + }) + ) + + harness.replaceSourceIncarnation('remote-after') + harness.resolveSpawn() + + await expect(split).rejects.toThrow('terminal_split_source_not_found') + expect(harness.kill).toHaveBeenCalledWith(SPLIT_PTY_ID) + expect(harness.revealTerminalSession).not.toHaveBeenCalled() + }) + + it('rejects a persisted-only source incarnation change during spawn', async () => { + const harness = createHarness(true, { + deferSpawn: true, + includePairedSnapshot: false, + stopAndWaitResult: true + }) + harness.replacePersistedSourceIncarnation('persisted-before') + + const split = harness.runtime.splitTerminal(harness.handle, { direction: 'horizontal' }) + await vi.waitFor(() => expect(harness.spawn).toHaveBeenCalledOnce()) + harness.replacePersistedSourceIncarnation('persisted-after') + harness.resolveSpawn() + + await expect(split).rejects.toThrow('terminal_split_source_not_found') + expect(harness.stopAndWait).toHaveBeenCalledWith( + SPLIT_PTY_ID, + expect.objectContaining({ deadlineMs: expect.any(Number) }) + ) + expect(harness.kill).not.toHaveBeenCalled() + expect(harness.retireRejectedPty).toHaveBeenCalledWith(SPLIT_PTY_ID) + }) + + it('revalidates a projected paired-runtime source after renderer adoption', async () => { + const harness = createHarness(false, { + deferReveal: true, + includePairedSnapshot: true, + sourceIncarnationId: 'projected-before', + stopAndWaitResult: false + }) + + const split = harness.runtime.splitTerminal(harness.handle, { direction: 'horizontal' }) + await vi.waitFor(() => expect(harness.revealTerminalSession).toHaveBeenCalledOnce()) + expect(harness.spawn).toHaveBeenCalledWith( + expect.not.objectContaining({ expectedSourceBinding: expect.anything() }) + ) + + harness.replaceSourceIncarnation('projected-after') + harness.resolveReveal() + + await expect(split).rejects.toThrow('terminal_split_source_not_found') + expect(harness.stopAndWait).toHaveBeenCalledWith( + SPLIT_PTY_ID, + expect.objectContaining({ deadlineMs: expect.any(Number) }) + ) + expect(harness.kill).toHaveBeenCalledWith(SPLIT_PTY_ID) + expect(harness.retireRejectedPty).toHaveBeenCalledWith(SPLIT_PTY_ID) + }) }) diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index ec4d930ad79..cb73139a830 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -14862,6 +14862,9 @@ describe('OrcaRuntimeService', () => { }, terminalLayoutsByTabId: { [tabId]: makeHeadlessTerminalLayout({ [HEADLESS_LEAF_ID]: ptyId }) + }, + terminalPtyIncarnationsByPaneKey: { + [makePaneKey(tabId, HEADLESS_LEAF_ID)]: 'live-source-incarnation' } }) ) @@ -14898,7 +14901,9 @@ describe('OrcaRuntimeService', () => { const splitSpawn = spawn.mock.calls[0]?.[0] as | { expectedSourceBinding?: { incarnationId?: string } } | undefined - expect(splitSpawn?.expectedSourceBinding).not.toHaveProperty('incarnationId') + expect(splitSpawn?.expectedSourceBinding).toMatchObject({ + incarnationId: 'live-source-incarnation' + }) runtime.syncWindowGraph(1, { tabs: [ diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 7cad4ad94bc..1084567b1bd 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -1724,6 +1724,7 @@ type RuntimePtyController = { * False on doubt (absent session, SSH-scoped id, non-daemon provider). */ attach?(ptyId: string): Promise kill(ptyId: string): boolean + retireRejectedPty?(ptyId: string): void stopAndWait?( ptyId: string, opts?: { keepHistory?: boolean; deadlineMs?: number } @@ -1827,6 +1828,7 @@ const BRACKETED_PASTE_END = '\x1b[201~' const BRACKETED_PASTE_QUIET_MS = 1500 const DRAFT_PASTE_READY_TIMEOUT_MS = 8000 const MOBILE_TERMINAL_SURFACE_TIMEOUT_MS = 10_000 +const REJECTED_SPLIT_PTY_STOP_TIMEOUT_MS = 10_000 const MOBILE_TERMINAL_READY_FALLBACK_MS = 1000 const SSH_PANE_RECOVERY_GRACE_MS = 30_000 // Why: long enough that a keystroke burst to a proven-dead leaf probes once, @@ -27362,6 +27364,8 @@ export class OrcaRuntimeService { if (!sourceAuthority) { throw new Error('terminal_split_source_not_found') } + const sourceIncarnationId = + sourceAuthority.liveIncarnationId ?? sourceAuthority.persistedIncarnationId const leafId = randomUUID() const preAllocatedHandle = this.createPreAllocatedTerminalHandle() const paneKey = makePaneKey(parentTabId, leafId) @@ -27388,9 +27392,7 @@ export class OrcaRuntimeService { tabId: parentTabId, leafId: parsedPaneKey.leafId, ptyId: pty.ptyId, - ...(sourceAuthority.persistedIncarnationId - ? { incarnationId: sourceAuthority.persistedIncarnationId } - : {}) + ...(sourceIncarnationId ? { incarnationId: sourceIncarnationId } : {}) } } : {}) @@ -27426,17 +27428,26 @@ export class OrcaRuntimeService { } try { - const revalidatedAuthority = this.resolveTerminalSplitSourceAuthority( - workspace.id, - parentTabId, - parsedPaneKey.leafId, - pty.ptyId - ) - if (!revalidatedAuthority || (sourceAuthority.persisted && !revalidatedAuthority.persisted)) { - throw new Error('terminal_split_source_not_found') + const revalidateSourceAuthority = (): void => { + const current = this.resolveTerminalSplitSourceAuthority( + workspace.id, + parentTabId, + parsedPaneKey.leafId, + pty.ptyId + ) + if ( + !current || + (sourceAuthority.persisted && !current.persisted) || + (sourceIncarnationId !== null && + (current.liveIncarnationId ?? current.persistedIncarnationId) !== sourceIncarnationId) + ) { + throw new Error('terminal_split_source_not_found') + } } + revalidateSourceAuthority() if (!sourceAuthority.persisted) { await revealSplit() + revalidateSourceAuthority() } if (createdPty) { const persisted = this.persistHeadlessTerminalSplit({ @@ -27460,7 +27471,19 @@ export class OrcaRuntimeService { } } catch (error) { this.setPairedRendererSessionOwnership(result.id, false) - this.ptyController.kill?.(result.id) + let stopped = false + try { + stopped = + (await this.ptyController.stopAndWait?.(result.id, { + deadlineMs: Date.now() + REJECTED_SPLIT_PTY_STOP_TIMEOUT_MS + })) ?? false + } catch { + // Best-effort fallback below preserves the original split authority error. + } + if (!stopped) { + this.ptyController.kill(result.id) + } + this.ptyController.retireRejectedPty?.(result.id) throw error } const committedSourceAuthority = sourceAuthority.persisted @@ -27489,6 +27512,7 @@ export class OrcaRuntimeService { rendererMounted: boolean persistedWorktreeId: string | null persistedIncarnationId: string | null + liveIncarnationId: string | null } | null { const session = this.getWorkspaceSessionForWorktree(worktreeId) const sessionWorktreeId = session ? resolveTerminalSessionWorktreeId(session, worktreeId) : null @@ -27527,7 +27551,8 @@ export class OrcaRuntimeService { persisted: true, rendererMounted, persistedWorktreeId: sessionWorktreeId, - persistedIncarnationId + persistedIncarnationId, + liveIncarnationId } } // Why: renderer adoption can precede graph sync; this path still requires reveal success before commit. @@ -27549,7 +27574,8 @@ export class OrcaRuntimeService { persisted: false, rendererMounted, persistedWorktreeId: null, - persistedIncarnationId: null + persistedIncarnationId: null, + liveIncarnationId } }