diff --git a/src/main/daemon/daemon-pty-router.test.ts b/src/main/daemon/daemon-pty-router.test.ts index 46d8146b637..20f89e68a30 100644 --- a/src/main/daemon/daemon-pty-router.test.ts +++ b/src/main/daemon/daemon-pty-router.test.ts @@ -26,6 +26,12 @@ function buildSessionIds(prefix: string, count: number): string[] { return ids } +function stubProcessPlatform(platform: NodeJS.Platform): () => void { + const original = Object.getOwnPropertyDescriptor(process, 'platform')! + Object.defineProperty(process, 'platform', { configurable: true, value: platform }) + return () => Object.defineProperty(process, 'platform', original) +} + function createAdapter( label: string, sessions: string[] = [], @@ -145,6 +151,48 @@ describe('DaemonPtyRouter', () => { expect(current.shutdown).toHaveBeenCalledWith('current-session', { immediate: true }) }) + it.each(['darwin', 'linux'] as const)( + 'does not let a dead legacy daemon strand known current sessions on %s', + async (platform) => { + const restorePlatform = stubProcessPlatform(platform) + const current = createAdapter('current') + const legacy = createAdapter('legacy', ['legacy-session']) + const router = new DaemonPtyRouter({ current, legacy: [legacy] }) + + try { + await router.discoverLegacySessions() + await router.spawn({ sessionId: 'current-close', cols: 80, rows: 24 }) + await router.spawn({ sessionId: 'current-sleep', cols: 80, rows: 24 }) + vi.mocked(legacy.listProcesses).mockRejectedValue(new Error('legacy unavailable')) + + await expect(router.shutdown('current-close', { immediate: true })).resolves.toBeUndefined() + await expect( + router.shutdown('current-sleep', { immediate: true, keepHistory: true }) + ).resolves.toBeUndefined() + current.emitExit('current-sleep', 0) + await router.spawn({ sessionId: 'current-sleep', cols: 80, rows: 24 }) + router.write('legacy-session', 'legacy still routed\n') + + expect(legacy.listProcesses).toHaveBeenCalledOnce() + expect(current.shutdown).toHaveBeenCalledWith('current-close', { immediate: true }) + expect(current.shutdown).toHaveBeenCalledWith('current-sleep', { + immediate: true, + keepHistory: true + }) + expect(legacy.shutdown).not.toHaveBeenCalled() + expect(current.spawn).toHaveBeenLastCalledWith({ + sessionId: 'current-sleep', + cols: 80, + rows: 24 + }) + expect(legacy.spawn).not.toHaveBeenCalled() + expect(legacy.write).toHaveBeenCalledWith('legacy-session', 'legacy still routed\n') + } finally { + restorePlatform() + } + } + ) + it('preflights against the authoritative legacy owner', async () => { const current = createAdapter('current') const legacy = createAdapter('legacy', ['legacy-session']) @@ -447,24 +495,29 @@ describe('DaemonPtyRouter', () => { }) it('inventories a hidden legacy owner before cached current shutdown', async () => { - const current = createAdapter('current') - const legacy = createAdapter('legacy', ['duplicate-session']) - const router = new DaemonPtyRouter({ current, legacy: [legacy] }) + const restorePlatform = stubProcessPlatform('win32') const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) - vi.mocked(legacy.hasPty).mockReturnValue(false) - vi.mocked(legacy.listProcesses).mockRejectedValueOnce(new Error('legacy still starting')) - await router.discoverLegacySessions() - await router.spawn({ sessionId: 'duplicate-session', cols: 80, rows: 24 }) - const deadlineMs = Date.now() + 1000 + try { + const current = createAdapter('current') + const legacy = createAdapter('legacy', ['duplicate-session']) + const router = new DaemonPtyRouter({ current, legacy: [legacy] }) + vi.mocked(legacy.hasPty).mockReturnValue(false) + vi.mocked(legacy.listProcesses).mockRejectedValueOnce(new Error('legacy still starting')) + await router.discoverLegacySessions() + await router.spawn({ sessionId: 'duplicate-session', cols: 80, rows: 24 }) + const deadlineMs = Date.now() + 1000 - await expect( - router.shutdown('duplicate-session', { immediate: true, deadlineMs }) - ).rejects.toThrow('Ambiguous PTY session ownership') - expect(current.listProcesses).toHaveBeenLastCalledWith({ deadlineMs }) - expect(legacy.listProcesses).toHaveBeenLastCalledWith({ deadlineMs }) - expect(current.shutdown).not.toHaveBeenCalled() - expect(legacy.shutdown).not.toHaveBeenCalled() - warn.mockRestore() + await expect( + router.shutdown('duplicate-session', { immediate: true, deadlineMs }) + ).rejects.toThrow('Ambiguous PTY session ownership') + expect(current.listProcesses).toHaveBeenLastCalledWith({ deadlineMs }) + expect(legacy.listProcesses).toHaveBeenLastCalledWith({ deadlineMs }) + expect(current.shutdown).not.toHaveBeenCalled() + expect(legacy.shutdown).not.toHaveBeenCalled() + } finally { + warn.mockRestore() + restorePlatform() + } }) it('keeps duplicate ownership ambiguous when one of three owners exits', async () => { diff --git a/src/main/daemon/daemon-pty-router.ts b/src/main/daemon/daemon-pty-router.ts index 1a6139838ef..805827198d2 100644 --- a/src/main/daemon/daemon-pty-router.ts +++ b/src/main/daemon/daemon-pty-router.ts @@ -108,7 +108,7 @@ export class DaemonPtyRouter implements IPtyProvider { id: string, opts: { immediate?: boolean; keepHistory?: boolean; deadlineMs?: number } ): Promise { - if (this.legacy.length > 0) { + if (this.requiresInventoryBeforeTargetedShutdown(id)) { // Why: a recovered legacy daemon may own an id absent from hasPty's local // cache; await authoritative inventories before selecting an adapter. await this.refreshInventories({ deadlineMs: opts.deadlineMs }) @@ -140,7 +140,7 @@ export class DaemonPtyRouter implements IPtyProvider { } async getShutdownBlockReason(id: string): Promise { - if (this.legacy.length > 0) { + if (this.requiresInventoryBeforeTargetedShutdown(id)) { // Why: preflight must select the same authoritative legacy owner as shutdown. await this.refreshInventories() } @@ -355,6 +355,13 @@ export class DaemonPtyRouter implements IPtyProvider { return this.sessionRouting.get(sessionId) ?? this.current } + private requiresInventoryBeforeTargetedShutdown(sessionId: string): boolean { + // Why: Windows legacy sessions need a fresh ambiguity check before teardown; + // on POSIX, a known route must not be stranded by an unrelated dead daemon. + const hasLegacy = this.legacy.length > 0 + return hasLegacy && (process.platform === 'win32' || !this.sessionRouting.get(sessionId)) + } + private refreshInventories(opts?: { deadlineMs?: number }): Promise<{ results: PtyProcessInfo[][] ambiguousIds: string[] @@ -362,9 +369,8 @@ export class DaemonPtyRouter implements IPtyProvider { return this.sessionRouting.refreshInventories(this.allAdapters(), opts) } - private ambiguousOwnershipError(sessionIds: string[]): Error { - return new Error(`Ambiguous PTY session ownership across daemons: ${sessionIds.join(', ')}`) - } + private ambiguousOwnershipError = (sessionIds: string[]): Error => + new Error(`Ambiguous PTY session ownership across daemons: ${sessionIds.join(', ')}`) private allAdapters(): DaemonPtyAdapter[] { return [this.current, ...this.legacy] diff --git a/src/renderer/src/components/shared/kill-all-terminal-surfaces.test.ts b/src/renderer/src/components/shared/kill-all-terminal-surfaces.test.ts index 075fee29db3..2f4638717de 100644 --- a/src/renderer/src/components/shared/kill-all-terminal-surfaces.test.ts +++ b/src/renderer/src/components/shared/kill-all-terminal-surfaces.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../../shared/constants' import { WINDOWS_LEGACY_PTY_SHUTDOWN_BLOCK_REASON } from '../../../../shared/pty-shutdown-safety' import { useAppStore, type AppState } from '@/store' @@ -101,6 +101,11 @@ describe('snapshotKillAllTerminalSurfaceIds', () => { }) describe('runKillAllTerminalSurfaces', () => { + beforeEach(() => { + vi.stubGlobal('navigator', { userAgent: 'Mozilla/5.0 (Windows NT 10.0; Win64; x64)' }) + delete (globalThis as { __ORCA_WEB_CLIENT__?: boolean }).__ORCA_WEB_CLIENT__ + }) + it('preserves unsafe legacy tabs and mappings before daemon kill-all starts', async () => { const current = state({ tabsByWorktree: { @@ -577,6 +582,50 @@ describe('runKillAllTerminalSurfaces', () => { }) }) + it('keeps a legacy PTY visible when it attaches during a renderer yield', async () => { + let current = state({ + activeWorktreeId: 'wt', + tabsByWorktree: { + wt: [terminal('first', 'wt'), terminal('second', 'wt'), terminal('remaining', 'wt')] + }, + ptyIdsByTabId: { + first: ['pty-first'], + second: ['pty-second'], + remaining: ['pty-safe-before-yield'] + } + }) + const getShutdownBlockReason = vi.fn(async (ptyId: string) => + ptyId === 'pty-v24' ? WINDOWS_LEGACY_PTY_SHUTDOWN_BLOCK_REASON : null + ) + const closeSurface = vi.fn((targetId: string) => removeSurface(current, targetId)) + const killPty = vi.fn().mockResolvedValue(undefined) + vi.stubGlobal('window', { api: { pty: { getShutdownBlockReason } } }) + + await expect( + runKillAllTerminalSurfaces(['first', 'second', 'remaining'], { + getState: () => current, + killDaemonSessions: vi.fn().mockResolvedValue({ killedCount: 0, remainingCount: 0 }), + closeSurface, + killPty, + yieldToRenderer: vi.fn(async () => { + current = state({ + activeWorktreeId: 'wt', + tabsByWorktree: { + wt: [{ ...terminal('remaining', 'wt'), ptyId: 'pty-v24' }] + }, + ptyIdsByTabId: { remaining: ['pty-v24'] } + }) + }), + reportSummary: vi.fn() + }) + ).rejects.toThrow(WINDOWS_LEGACY_PTY_SHUTDOWN_BLOCK_REASON) + + expect(closeSurface.mock.calls.map(([targetId]) => targetId)).toEqual(['first', 'second']) + expect(killPty).not.toHaveBeenCalledWith('pty-v24') + expect(snapshotKillAllTerminalSurfaceIds(current)).toEqual(['remaining']) + expect(current.ptyIdsByTabId.remaining).toEqual(['pty-v24']) + }) + it('records bounded fanout and two-close batches for 100 terminal tabs', async () => { const tabs = Array.from({ length: 100 }, (_, index) => terminal(`tab-${index}`, 'wt')) const ptyIdsByTabId = Object.fromEntries(tabs.map((tab, index) => [tab.id, [`pty-${index}`]])) diff --git a/src/renderer/src/components/shared/kill-all-terminal-surfaces.ts b/src/renderer/src/components/shared/kill-all-terminal-surfaces.ts index 824e179b7ad..77a455083c1 100644 --- a/src/renderer/src/components/shared/kill-all-terminal-surfaces.ts +++ b/src/renderer/src/components/shared/kill-all-terminal-surfaces.ts @@ -114,15 +114,6 @@ function getTargetIndex( return { ownerByTargetId, terminalIdsByWorktree } } -function getNextTerminalId(ids: ReadonlySet, closingId: string): string | null { - for (const id of ids) { - if (id !== closingId) { - return id - } - } - return null -} - function createDefaultDependencies(): KillAllTerminalSurfaceDependencies { return { getState: useAppStore.getState, @@ -228,7 +219,8 @@ export async function runKillAllTerminalSurfaces( const owningWorktreeId = closeWave.ownerByTargetId.get(targetId)! const remainingTerminalIds = closeWave.terminalIdsByWorktree.get(owningWorktreeId) ?? new Set() - const nextTerminalTabId = getNextTerminalId(remainingTerminalIds, targetId) + const nextTerminalTabId = + [...remainingTerminalIds].find((terminalId) => terminalId !== targetId) ?? null const { plan: retirementPlan, newlyScheduledPtyOwners } = reserveTerminalRetirementTeardowns( closeWave.state, closeWave.retirementPlans.get(targetId)!, @@ -285,8 +277,12 @@ export async function runKillAllTerminalSurfaces( closeBatchStartedAt = deps.now() const stateAfterYield = deps.getState() if (mustReplanAfterYield || stateAfterYield !== stateAfterBatch) { - // Why: replanning after a mutable yield prevents killing a new PTY owner. - closeWave = createCloseWave(stateAfterYield) + // Why: a yield can attach an unsafe legacy owner; revalidate before + // rebuilding the wave that will synchronously retire the next target. + if (shouldPreflightTargets) { + await assertTargetsSafe() + } + closeWave = createCloseWave(deps.getState()) } } } diff --git a/src/renderer/src/components/sidebar/sleep-worktree-flow.test.ts b/src/renderer/src/components/sidebar/sleep-worktree-flow.test.ts index 3ec767b730c..d5cfd03f350 100644 --- a/src/renderer/src/components/sidebar/sleep-worktree-flow.test.ts +++ b/src/renderer/src/components/sidebar/sleep-worktree-flow.test.ts @@ -47,6 +47,8 @@ import { runSleepWorktree, runSleepWorktrees } from './sleep-worktree-flow' describe('runSleepWorktree', () => { beforeEach(() => { vi.unstubAllGlobals() + vi.stubGlobal('navigator', { userAgent: 'Mozilla/5.0 (Windows NT 10.0; Win64; x64)' }) + delete (globalThis as { __ORCA_WEB_CLIENT__?: boolean }).__ORCA_WEB_CLIENT__ vi.stubGlobal('window', { api: { ephemeralVm: { @@ -74,25 +76,22 @@ describe('runSleepWorktree', () => { mocks.state.pendingReconnectPtyIdByTabId = {} }) - it('tears down browsers before terminals on the sleep path', async () => { + it('starts checked terminal teardown before browsers on the sleep path', async () => { mocks.state.activeWorktreeId = 'wt-1' await runSleepWorktree('wt-1') - // Why: browsers must run first so destroyPersistentWebview can unregister - // the Chromium guests while browserTabsByWorktree/browserPagesByWorkspace - // are still populated. If terminals ran first and kept its old - // browserTabsByWorktree delete, browsers would no-op and leak webviews. expect(mocks.state.shutdownWorktreeBrowsers).toHaveBeenCalledWith('wt-1') expect(mocks.state.shutdownWorktreeTerminals).toHaveBeenCalledWith('wt-1', { - keepIdentifiers: true + keepIdentifiers: true, + shutdownSafetyChecked: true }) expect(mocks.suspendWorkspace).toHaveBeenCalledWith({ workspaceId: 'wt-1' }) const browsersCallOrder = mocks.state.shutdownWorktreeBrowsers.mock.invocationCallOrder[0] const terminalsCallOrder = mocks.state.shutdownWorktreeTerminals.mock.invocationCallOrder[0] const suspendCallOrder = mocks.suspendWorkspace.mock.invocationCallOrder[0] - expect(browsersCallOrder).toBeLessThan(terminalsCallOrder) - expect(terminalsCallOrder).toBeLessThan(suspendCallOrder) + expect(terminalsCallOrder).toBeLessThan(browsersCallOrder) + expect(browsersCallOrder).toBeLessThan(suspendCallOrder) }) it('clears activeWorktreeId before teardown when the slept worktree is active', async () => { @@ -158,6 +157,39 @@ describe('runSleepWorktree', () => { expect(mocks.clearWorktreeSleepIntent).not.toHaveBeenCalled() }) + it('retires checked PTY ownership before browser teardown can replace it', async () => { + const getShutdownBlockReason = vi.fn().mockResolvedValue(null) + vi.stubGlobal('window', { + api: { + pty: { getShutdownBlockReason }, + ephemeralVm: { suspendWorkspace: mocks.suspendWorkspace } + }, + requestAnimationFrame: vi.fn() + }) + mocks.state.activeWorktreeId = 'wt-1' + mocks.state.tabsByWorktree = { 'wt-1': [{ id: 'tab-1' }] } + mocks.state.ptyIdsByTabId = { 'tab-1': ['pty-safe'] } + mocks.state.shutdownWorktreeTerminals.mockImplementationOnce(async () => { + mocks.state.ptyIdsByTabId = { 'tab-1': [] } + }) + const unsafeReplacement = vi.fn(() => { + if ((mocks.state.ptyIdsByTabId['tab-1'] ?? []).length > 0) { + mocks.state.ptyIdsByTabId = { 'tab-1': ['pty-v24'] } + } + }) + mocks.state.shutdownWorktreeBrowsers.mockImplementationOnce(async () => unsafeReplacement()) + + await runSleepWorktree('wt-1') + + expect(getShutdownBlockReason).toHaveBeenCalledTimes(1) + expect(getShutdownBlockReason).toHaveBeenCalledWith('pty-safe') + expect(unsafeReplacement).toHaveBeenCalledOnce() + expect(mocks.state.ptyIdsByTabId['tab-1']).toEqual([]) + const terminalCall = mocks.state.shutdownWorktreeTerminals.mock.invocationCallOrder[0] + const browserCall = mocks.state.shutdownWorktreeBrowsers.mock.invocationCallOrder[0] + expect(terminalCall).toBeLessThan(browserCall) + }) + it('preserves active row position through section-scoped sidebar row ids', async () => { const requestAnimationFrame = vi.fn(() => 1) const scroller = { @@ -239,7 +271,7 @@ describe('runSleepWorktree', () => { expect(mocks.markWorktreeSleepIntent).not.toHaveBeenCalled() }) - it('surfaces a toast and skips terminals when browsers throws', async () => { + it('surfaces a toast after a post-terminal browser failure', async () => { mocks.state.activeWorktreeId = 'wt-1' mocks.state.shutdownWorktreeBrowsers.mockRejectedValueOnce(new Error('boom')) mocks.state.tabsByWorktree = { 'wt-1': [{ id: 'tab-1' }] } @@ -247,7 +279,10 @@ describe('runSleepWorktree', () => { await runSleepWorktree('wt-1') - expect(mocks.state.shutdownWorktreeTerminals).not.toHaveBeenCalled() + expect(mocks.state.shutdownWorktreeTerminals).toHaveBeenCalledWith('wt-1', { + keepIdentifiers: true, + shutdownSafetyChecked: true + }) expect(mocks.suspendWorkspace).not.toHaveBeenCalled() expect(mocks.clearWorktreeSleepIntent).toHaveBeenCalledWith('wt-1') expect(mocks.toastError).toHaveBeenCalledWith( @@ -266,12 +301,14 @@ describe('runSleepWorktree', () => { await runSleepWorktrees(['wt-1', 'wt-2']) - expect(mocks.state.shutdownWorktreeTerminals).not.toHaveBeenCalledWith('wt-1', { - keepIdentifiers: true + expect(mocks.state.shutdownWorktreeTerminals).toHaveBeenCalledWith('wt-1', { + keepIdentifiers: true, + shutdownSafetyChecked: true }) expect(mocks.state.shutdownWorktreeBrowsers).toHaveBeenCalledWith('wt-2') expect(mocks.state.shutdownWorktreeTerminals).toHaveBeenCalledWith('wt-2', { - keepIdentifiers: true + keepIdentifiers: true, + shutdownSafetyChecked: true }) expect(mocks.suspendWorkspace).toHaveBeenCalledWith({ workspaceId: 'wt-2' }) expect(mocks.toastError).toHaveBeenCalledWith( @@ -289,11 +326,13 @@ describe('runSleepWorktree', () => { expect(mocks.state.setActiveWorktree).toHaveBeenCalledWith(null) expect(mocks.state.shutdownWorktreeBrowsers).toHaveBeenNthCalledWith(1, 'wt-1') expect(mocks.state.shutdownWorktreeTerminals).toHaveBeenNthCalledWith(1, 'wt-1', { - keepIdentifiers: true + keepIdentifiers: true, + shutdownSafetyChecked: true }) expect(mocks.state.shutdownWorktreeBrowsers).toHaveBeenNthCalledWith(2, 'wt-2') expect(mocks.state.shutdownWorktreeTerminals).toHaveBeenNthCalledWith(2, 'wt-2', { - keepIdentifiers: true + keepIdentifiers: true, + shutdownSafetyChecked: true }) expect(mocks.suspendWorkspace).toHaveBeenNthCalledWith(1, { workspaceId: 'wt-1' }) expect(mocks.suspendWorkspace).toHaveBeenNthCalledWith(2, { workspaceId: 'wt-2' }) diff --git a/src/renderer/src/components/sidebar/sleep-worktree-flow.ts b/src/renderer/src/components/sidebar/sleep-worktree-flow.ts index c60bcce07d5..55f3dbb3be5 100644 --- a/src/renderer/src/components/sidebar/sleep-worktree-flow.ts +++ b/src/renderer/src/components/sidebar/sleep-worktree-flow.ts @@ -114,50 +114,41 @@ export async function runSleepWorktrees(worktreeIds: readonly string[]): Promise if (worktreeIds.length === 0) { return } - const worktreeIdSet = new Set(worktreeIds) - try { - // Why: a legacy refusal must leave activation, browsers, and sleep intent untouched. - await assertStableTerminalShutdownSafety({ - surfaceId: worktreeIds.join(':'), - getState: useAppStore.getState, - selectTabIds: (state) => selectTerminalTabIdsForWorktrees(state, worktreeIdSet) - }) - } catch { - // The safety preflight already explains why the workspace stayed awake. - return - } - const { - activeWorktreeId, - setActiveWorktree, - shutdownWorktreeBrowsers, - shutdownWorktreeTerminals - } = useAppStore.getState() let activeSleepIntentWorktreeId: string | null = null - if (activeWorktreeId && worktreeIds.includes(activeWorktreeId)) { - const restoreSidebarPosition = preserveSidebarWorktreePosition(activeWorktreeId) - // Why: clearing the active workspace can unmount TerminalPanes before - // shutdownWorktreeTerminals writes PTY suppressions. Use a non-rendering - // intent marker so those exits do not stamp activity, without inserting an - // extra Zustand update that can disturb the sidebar's scroll restoration. - markWorktreeSleepIntent(activeWorktreeId) - activeSleepIntentWorktreeId = activeWorktreeId - setActiveWorktree(null) - restoreSidebarPosition() - } const errors: string[] = [] try { for (const worktreeId of worktreeIds) { try { - // Why: sleep mirrors removeWorktree's shutdown sequence — browsers first - // so destroyPersistentWebview unregisters the Chromium guests before any - // other teardown runs, terminals second so the PTY kill uses the same - // ordering on both paths. Without the browser thunk here, sleep leaks - // browserPagesByWorkspace entries and live webviews for the slept worktree. - await shutdownWorktreeBrowsers(worktreeId) - } catch (err) { - errors.push(err instanceof Error ? err.message : String(err)) + // Why: verify each worktree immediately before its first mutation; an + // earlier worktree's async teardown must not stale this ownership answer. + await assertStableTerminalShutdownSafety({ + surfaceId: worktreeId, + getState: useAppStore.getState, + selectTabIds: (state) => selectTerminalTabIdsForWorktrees(state, new Set([worktreeId])) + }) + } catch { + // The safety preflight already explains why this workspace stayed awake. continue } + + const { + activeWorktreeId, + setActiveWorktree, + shutdownWorktreeBrowsers, + shutdownWorktreeTerminals + } = useAppStore.getState() + if (activeWorktreeId === worktreeId) { + const restoreSidebarPosition = preserveSidebarWorktreePosition(activeWorktreeId) + // Why: clearing the active workspace can unmount TerminalPanes before + // shutdownWorktreeTerminals writes PTY suppressions. Use a non-rendering + // intent marker so those exits do not stamp activity, without inserting an + // extra Zustand update that can disturb the sidebar's scroll restoration. + markWorktreeSleepIntent(activeWorktreeId) + activeSleepIntentWorktreeId = activeWorktreeId + setActiveWorktree(null) + restoreSidebarPosition() + } + try { // Why: sleep is reversible — the tab record stays in tabsByWorktree, the // layout stays in terminalLayoutsByTabId, only the live PTY processes are @@ -166,7 +157,15 @@ export async function runSleepWorktrees(worktreeIds: readonly string[]): Promise // history dir (local) or relay session id (SSH); it also captures // serializer buffers into buffersByLeafId for SSH wake to reseed // scrollback. See DESIGN_DOC_TERMINAL_HISTORY_FIX_V2.md §3.3.c. - await shutdownWorktreeTerminals(worktreeId, { keepIdentifiers: true }) + // Why: start terminal retirement in the same renderer turn as the stable + // preflight; browser teardown awaits and can otherwise attach a legacy PTY. + await shutdownWorktreeTerminals(worktreeId, { + keepIdentifiers: true, + shutdownSafetyChecked: true + }) + // Why: terminal shutdown no longer mutates browser maps, so Chromium + // guests can be removed afterward without creating a PTY safety gap. + await shutdownWorktreeBrowsers(worktreeId) if (typeof window !== 'undefined' && window.api?.ephemeralVm?.suspendWorkspace) { await window.api.ephemeralVm.suspendWorkspace({ workspaceId: worktreeId }) } diff --git a/src/renderer/src/store/slices/terminal-shutdown-safety.ts b/src/renderer/src/store/slices/terminal-shutdown-safety.ts index 687e1b07dc0..0ad3af03687 100644 --- a/src/renderer/src/store/slices/terminal-shutdown-safety.ts +++ b/src/renderer/src/store/slices/terminal-shutdown-safety.ts @@ -19,6 +19,13 @@ type StableTerminalShutdownPlan = { function getShutdownSafetyPreflight(): | ((id: string) => ReturnType) | undefined { + const isNativeWindowsRenderer = + typeof navigator !== 'undefined' && + navigator.userAgent.includes('Windows') && + (globalThis as { __ORCA_WEB_CLIENT__?: boolean }).__ORCA_WEB_CLIENT__ !== true + if (!isNativeWindowsRenderer) { + return undefined + } return (globalThis.window?.api?.pty as Partial | undefined) ?.getShutdownBlockReason } diff --git a/src/renderer/src/store/slices/terminal-tab-retirement-store.test.ts b/src/renderer/src/store/slices/terminal-tab-retirement-store.test.ts index 176f2d7ae8a..9fc43ef0b68 100644 --- a/src/renderer/src/store/slices/terminal-tab-retirement-store.test.ts +++ b/src/renderer/src/store/slices/terminal-tab-retirement-store.test.ts @@ -60,6 +60,8 @@ function deferred(): { promise: Promise; resolve: (value: T) => void } { describe('terminal tab retirement store boundary', () => { beforeEach(() => { vi.clearAllMocks() + vi.stubGlobal('navigator', { userAgent: 'Mozilla/5.0 (Windows NT 10.0; Win64; x64)' }) + delete (globalThis as { __ORCA_WEB_CLIENT__?: boolean }).__ORCA_WEB_CLIENT__ mockKill.mockResolvedValue(undefined) mockGetShutdownBlockReason.mockResolvedValue(null) mockRuntimeCall.mockResolvedValue({ @@ -272,6 +274,39 @@ describe('terminal tab retirement store boundary', () => { warn.mockRestore() }) + it('keeps ordinary non-Windows close synchronous and skips Windows inventory', () => { + vi.stubGlobal('navigator', { userAgent: 'Mozilla/5.0 (Macintosh; Intel Mac OS X)' }) + const store = createTestStore() + seedStore(store, { + tabsByWorktree: { + 'wt-1': [makeTab({ id: 'tab-1', worktreeId: 'wt-1', ptyId: 'pty-1' })] + }, + ptyIdsByTabId: { 'tab-1': ['pty-1'] } + }) + + store.getState().closeTab('tab-1') + + expect(store.getState().tabsByWorktree['wt-1']).toEqual([]) + expect(mockGetShutdownBlockReason).not.toHaveBeenCalled() + expect(mockKill).toHaveBeenCalledWith('pty-1') + }) + + it('does not run native shutdown inventory in a Windows web client', () => { + ;(globalThis as { __ORCA_WEB_CLIENT__?: boolean }).__ORCA_WEB_CLIENT__ = true + const store = createTestStore() + seedStore(store, { + tabsByWorktree: { + 'wt-1': [makeTab({ id: 'tab-1', worktreeId: 'wt-1', ptyId: 'remote:terminal-1' })] + }, + ptyIdsByTabId: { 'tab-1': ['remote:terminal-1'] } + }) + + store.getState().closeTab('tab-1') + + expect(store.getState().tabsByWorktree['wt-1']).toEqual([]) + expect(mockGetShutdownBlockReason).not.toHaveBeenCalled() + }) + it('keeps a legacy Windows tab and PTY ownership visible when shutdown is unsafe', async () => { const store = createTestStore() mockGetShutdownBlockReason.mockResolvedValueOnce(WINDOWS_LEGACY_PTY_SHUTDOWN_BLOCK_REASON) diff --git a/src/renderer/src/store/slices/terminals.ts b/src/renderer/src/store/slices/terminals.ts index c36a0e53700..15d3fd10cbc 100644 --- a/src/renderer/src/store/slices/terminals.ts +++ b/src/renderer/src/store/slices/terminals.ts @@ -606,6 +606,8 @@ export type TerminalSlice = { shutdownReason?: AgentStatusWorktreeShutdownReason sleepingPaneKeys?: string[] expectedRuntimePtyIds?: string[] + /** Internal handoff from a stable safety check in the same renderer turn. */ + shutdownSafetyChecked?: boolean } ) => Promise shutdownCompletedAgentPaneForHibernation: ( @@ -2302,11 +2304,13 @@ export const createTerminalSlice: StateCreator const shutdownReason: AgentStatusWorktreeShutdownReason = opts?.shutdownReason ?? (keepIdentifiers ? 'manual-sleep' : 'remove-worktree') const worktreeIds = new Set([worktreeId]) - await assertStableTerminalShutdownSafety({ - surfaceId: worktreeId, - getState: get, - selectTabIds: (state) => selectTerminalTabIdsForWorktrees(state, worktreeIds) - }) + if (!opts?.shutdownSafetyChecked) { + await assertStableTerminalShutdownSafety({ + surfaceId: worktreeId, + getState: get, + selectTabIds: (state) => selectTerminalTabIdsForWorktrees(state, worktreeIds) + }) + } const tabs = get().tabsByWorktree[worktreeId] ?? [] const ptyIds = tabs.flatMap((tab) => get().ptyIdsByTabId[tab.id] ?? []) const rendererShutdownPtyIds = sortedUniquePtyIds(ptyIds)