diff --git a/src/renderer/src/components/Terminal.tsx b/src/renderer/src/components/Terminal.tsx index 1e212c8c66a..bb762fca72e 100644 --- a/src/renderer/src/components/Terminal.tsx +++ b/src/renderer/src/components/Terminal.tsx @@ -61,7 +61,6 @@ import { resolveRepairedActiveTerminalTabId } from './terminal/active-terminal-r import { scheduleBackgroundTerminalWorktreeMeasure } from './terminal/background-terminal-worktree-visibility' import { applyBackgroundMountTabRestriction, - collectDeferredMountTabIds, planColdActivationTabDeferral, pruneClosedBackgroundMountTabs, revealActivationDeferredTabs, @@ -85,6 +84,7 @@ import { import { getTerminalParkingPolicyOverrides } from './terminal-pane/terminal-parking-e2e-overrides' import { canWatcherCoverParkedTerminalTab, + disposeAllParkedTerminalWatchers, pruneParkedTerminalWatchers, shouldDeferParkedPtyExitTabClose, syncParkedTerminalTabWatchers @@ -785,6 +785,10 @@ function Terminal(): React.JSX.Element | null { // wake/resume) must not instantiate a TerminalPane per saved tab. A worktree // absent from this map mounts all of its tabs. const backgroundMountTabIdsByWorktreeRef = useRef(new Map>()) + // Why: targeted background mounts share the allowed-tab map above, but only + // cold activation deferral should immediately create watcher coverage for + // every unmounted tab. + const activationDeferredMountTabIdsByWorktreeRef = useRef(new Map>()) // Why: the cold-activation deferral decision must run once per activation // transition, not on every re-render of an already-active worktree. const lastActivationWorktreeIdRef = useRef(null) @@ -799,6 +803,18 @@ function Terminal(): React.JSX.Element | null { worktreeId, detail.tabIds ) + // Why: a targeted wake can reveal a tab that was deferred by an earlier + // user activation. Remove it from watcher ownership before its pane mounts. + const worktreeTabIds = (useAppStore.getState().tabsByWorktree[worktreeId] ?? []).map( + (tab) => tab.id + ) + revealActivationDeferredTabs({ + restrictions: backgroundMountTabIdsByWorktreeRef.current, + deferredMountTabIdsByWorktree: activationDeferredMountTabIdsByWorktreeRef.current, + worktreeId, + allTabIds: worktreeTabIds, + immediateTabIds: new Set(detail.tabIds ?? worktreeTabIds) + }) scheduleBackgroundTerminalWorktreeMeasure({ mountedWorktreeIds: mountedWorktreeIdsRef.current, measurableBackgroundWorktreeIds: measurableBackgroundWorktreeIdsRef.current, @@ -1024,6 +1040,7 @@ function Terminal(): React.JSX.Element | null { const tabById = new Map(worktreeTabs.map((tab) => [tab.id, tab])) planColdActivationTabDeferral({ restrictions: backgroundMountTabIdsByWorktreeRef.current, + deferredMountTabIdsByWorktree: activationDeferredMountTabIdsByWorktreeRef.current, worktreeId: renderedActiveWorktreeId, allTabIds: worktreeTabs.map((tab) => tab.id), isTabLive: hasRegisteredRuntimeTerminalTab, @@ -1046,9 +1063,19 @@ function Terminal(): React.JSX.Element | null { // Why: flipping either kill switch while active must restore the eager // mount behavior immediately, not strand an old activation restriction. backgroundMountTabIdsByWorktreeRef.current.delete(renderedActiveWorktreeId) + activationDeferredMountTabIdsByWorktreeRef.current.delete(renderedActiveWorktreeId) } else { + // Why: tabs added after activation never passed the original coverage + // gate. Uncoverable/no-PTY tabs must mount now so they can spawn or keep + // their non-snapshot-backed live transport. + for (const tab of worktreeTabs) { + if (!canWatcherCoverParkedTerminalTab(renderedActiveWorktreeId, tab)) { + immediateTabIds.add(tab.id) + } + } revealActivationDeferredTabs({ restrictions: backgroundMountTabIdsByWorktreeRef.current, + deferredMountTabIdsByWorktree: activationDeferredMountTabIdsByWorktreeRef.current, worktreeId: renderedActiveWorktreeId, allTabIds: worktreeTabs.map((tab) => tab.id), immediateTabIds @@ -1063,7 +1090,8 @@ function Terminal(): React.JSX.Element | null { pruneClosedBackgroundMountTabs( backgroundMountTabIdsByWorktreeRef.current, mountedWorktreeIdsRef.current, - tabsByWorktree + tabsByWorktree, + activationDeferredMountTabIdsByWorktreeRef.current ) // Prune IDs of worktrees that no longer exist (deleted/removed) const allWorktreeIds = new Set(workspaceSurfaces.map((workspace) => workspace.id)) @@ -1071,6 +1099,7 @@ function Terminal(): React.JSX.Element | null { if (!allWorktreeIds.has(id)) { mountedWorktreeIdsRef.current.delete(id) backgroundMountTabIdsByWorktreeRef.current.delete(id) + activationDeferredMountTabIdsByWorktreeRef.current.delete(id) } } const anyMountedWorktreeHasLayout = computeAnyMountedWorktreeHasLayout( @@ -1115,16 +1144,14 @@ function Terminal(): React.JSX.Element | null { } } } - // Why: mount-restricted (activation-deferred / targeted-background) - // tabs are unmounted like parked ones; the same byte watchers own - // their side effects until first reveal. - deferredTabIds = collectDeferredMountTabIds( - backgroundMountTabIdsByWorktreeRef.current.get(workspace.id) ?? null, - tabs.map((tab) => tab.id) - ) + // Why: activation-deferred tabs are unmounted like parked ones; the + // same byte watchers own their side effects until first reveal. + // Targeted restrictions keep their existing delayed parking policy. + deferredTabIds = + activationDeferredMountTabIdsByWorktreeRef.current.get(workspace.id) ?? null for (const tab of tabs) { if ( - deferredTabIds.has(tab.id) && + deferredTabIds?.has(tab.id) && !parkedTabIds.has(tab.id) && canWatcherCoverParkedTerminalTab(workspace.id, tab) && !findActivityTerminalPortal(activityTerminalPortals, { @@ -1152,19 +1179,25 @@ function Terminal(): React.JSX.Element | null { activeTabId, activeView, activityTerminalPortals, + activeTabIdByWorktree, anyMountedWorktreeHasLayout, backgroundMountRevision, getEffectiveLayoutForWorktree, + groupsByWorktree, parkedTerminalWorktreeIds, + pendingStartupByTabId, renderedActiveWorktreeId, tabsByWorktree, + terminalParkingEnabled, + terminalTitleSnapshotAuthorityEnabled, + workspaceSessionReady, workspaceSurfaces ]) // Why: symmetric with useTerminalTabColdParking's unmount cleanup — when // the terminal host unmounts, no reconciliation effect will run again, so // dispose every remaining parked watcher here (overlay-layer children have // already disposed theirs by the time this parent cleanup runs). - useEffect(() => () => pruneParkedTerminalWatchers(new Set()), []) + useEffect(() => () => disposeAllParkedTerminalWatchers(), []) // Auto-create first tab when worktree activates useEffect(() => { if (!workspaceSessionReady) { @@ -2219,6 +2252,9 @@ function Terminal(): React.JSX.Element | null { backgroundMountTabIds={ backgroundMountTabIdsByWorktreeRef.current.get(workspace.id) ?? null } + activationDeferredMountTabIds={ + activationDeferredMountTabIdsByWorktreeRef.current.get(workspace.id) ?? null + } /> ) })} @@ -2516,7 +2552,8 @@ const WorktreeSplitSurface = React.memo(function WorktreeSplitSurface({ shouldMeasureHiddenWorktree, shouldColdParkTerminalPanes, activityTerminalPortals, - backgroundMountTabIds + backgroundMountTabIds, + activationDeferredMountTabIds }: { worktreeId: string worktreePath: string @@ -2527,6 +2564,7 @@ const WorktreeSplitSurface = React.memo(function WorktreeSplitSurface({ shouldColdParkTerminalPanes: boolean activityTerminalPortals: ActivityTerminalPortalTarget[] backgroundMountTabIds: ReadonlySet | null + activationDeferredMountTabIds: ReadonlySet | null }): React.JSX.Element { const browserPageIds = useAppStore( useShallow((state) => @@ -2569,6 +2607,7 @@ const WorktreeSplitSurface = React.memo(function WorktreeSplitSurface({ shouldMeasureHiddenWorktree={shouldMeasureHiddenWorktree} activityTerminalPortals={activityTerminalPortals} backgroundMountTabIds={backgroundMountTabIds} + activationDeferredMountTabIds={activationDeferredMountTabIds} /> {isVisible || backgroundMountTabIds === null ? ( <> diff --git a/src/renderer/src/components/terminal-pane/TerminalPaneOverlayLayer.tsx b/src/renderer/src/components/terminal-pane/TerminalPaneOverlayLayer.tsx index 35a1f61b32a..15db9323386 100644 --- a/src/renderer/src/components/terminal-pane/TerminalPaneOverlayLayer.tsx +++ b/src/renderer/src/components/terminal-pane/TerminalPaneOverlayLayer.tsx @@ -11,10 +11,7 @@ import { } from '../activity/activity-terminal-portal' import TerminalPane from './TerminalPane' import { closeTerminalTab } from '../terminal/terminal-tab-actions' -import { - collectDeferredMountTabIds, - shouldMountBackgroundWorktreeTab -} from '../terminal/background-terminal-worktree-mount' +import { shouldMountBackgroundWorktreeTab } from '../terminal/background-terminal-worktree-mount' import { useNativeChatToggleShortcut } from '../native-chat/use-native-chat-toggle-shortcut' import { shouldDeferParkedPtyExitTabClose } from './terminal-parked-tab-watchers' import { useTerminalTabColdParking } from './use-terminal-tab-cold-parking' @@ -64,7 +61,6 @@ type TerminalOverlaySlotProps = { activityTerminalPortal: ActivityTerminalPortalTarget | null onFocusOwningGroup: ((groupId: string) => void) | undefined consumeSuppressedPtyExit: (ptyId: string) => boolean - closeTab: (tabId: string) => void leaveWorktreeIfEmpty: () => void } @@ -81,7 +77,6 @@ const TerminalOverlaySlot = memo(function TerminalOverlaySlot({ activityTerminalPortal, onFocusOwningGroup, consumeSuppressedPtyExit, - closeTab, leaveWorktreeIfEmpty }: TerminalOverlaySlotProps): React.JSX.Element { const anchorName = groupId !== undefined ? tabGroupBodyAnchorName(groupId) : undefined @@ -249,15 +244,13 @@ const TerminalOverlaySlot = memo(function TerminalOverlaySlot({ if (shouldDeferParkedPtyExitTabClose(terminalTabId, ptyId)) { return } - closeTab(terminalTabId) - leaveWorktreeIfEmpty() + closeTerminalTab(terminalTabId, { onClosed: leaveWorktreeIfEmpty }) }} onCloseTab={() => { // Why: route through closeTerminalTab (not the raw store closeTab) so a // pinned tab hits the confirmation guard. The overlay's direct // store.closeTab was the path that closed pinned terminals silently. - closeTerminalTab(terminalTabId) - leaveWorktreeIfEmpty() + closeTerminalTab(terminalTabId, { onClosed: leaveWorktreeIfEmpty }) }} /> ) @@ -293,7 +286,8 @@ const TerminalPaneOverlayLayer = memo(function TerminalPaneOverlayLayer({ coldParkTerminalPanes = false, shouldMeasureHiddenWorktree = false, activityTerminalPortals = EMPTY_ACTIVITY_PORTALS, - backgroundMountTabIds = null + backgroundMountTabIds = null, + activationDeferredMountTabIds = null }: { worktreeId: string worktreePath: string @@ -304,6 +298,9 @@ const TerminalPaneOverlayLayer = memo(function TerminalPaneOverlayLayer({ /** Non-null for targeted background mounts: only these terminal tabs get a * TerminalPane, so waking one slept agent does not connect every saved tab. */ backgroundMountTabIds?: ReadonlySet | null + /** Only cold-activation deferred tabs receive immediate parked watcher + * coverage; targeted mounts keep their existing delayed parking policy. */ + activationDeferredMountTabIds?: ReadonlySet | null }): React.JSX.Element | null { const { terminalTabs, unifiedTabs, groups, activeGroupId } = useAppStore( useShallow((state) => ({ @@ -315,7 +312,6 @@ const TerminalPaneOverlayLayer = memo(function TerminalPaneOverlayLayer({ ) const focusGroup = useAppStore((state) => state.focusGroup) const consumeSuppressedPtyExit = useAppStore((state) => state.consumeSuppressedPtyExit) - const closeTab = useAppStore((state) => state.closeTab) const setActiveWorktree = useAppStore((state) => state.setActiveWorktree) const reconcileWorktreeTabModel = useAppStore((state) => state.reconcileWorktreeTabModel) @@ -324,9 +320,8 @@ const TerminalPaneOverlayLayer = memo(function TerminalPaneOverlayLayer({ // Why: legacy TabGroupPanel routed terminal closes through // commands.closeItem → leaveWorktreeIfEmpty, which deselected the worktree // when the last renderable tab closed and sent the user back to Landing. - // The overlay layer calls store.closeTab directly, so replicate that - // post-close check here; otherwise closing the last terminal leaves an - // empty TabGroupPanel body selected. + // Run this only after the guarded close resolves; a pending/cancelled pinned + // close must leave the worktree and paired-web mirror selected. const leaveWorktreeIfEmpty = useCallback(() => { const state = useAppStore.getState() if (state.activeWorktreeId !== worktreeId) { @@ -366,16 +361,6 @@ const TerminalPaneOverlayLayer = memo(function TerminalPaneOverlayLayer({ return entries }, [groupActiveTabById, unifiedTabs]) - // Why: mount-restricted tabs render no pane (the filter below), so they - // need parked-watcher coverage exactly like cold-parked tabs. - const deferredMountTabIds = useMemo( - () => - collectDeferredMountTabIds( - backgroundMountTabIds, - terminalTabs.map((tab) => tab.id) - ), - [backgroundMountTabIds, terminalTabs] - ) const parkedTerminalTabIds = useTerminalTabColdParking({ worktreeId, terminalTabs, @@ -384,7 +369,7 @@ const TerminalPaneOverlayLayer = memo(function TerminalPaneOverlayLayer({ coldParkTerminalPanes, shouldMeasureHiddenWorktree, activityTerminalPortals, - deferredMountTabIds + activationDeferredMountTabIds }) if (!worktreePath) { @@ -425,7 +410,6 @@ const TerminalPaneOverlayLayer = memo(function TerminalPaneOverlayLayer({ activityTerminalPortal={activityTerminalPortal} onFocusOwningGroup={focusOwningGroup} consumeSuppressedPtyExit={consumeSuppressedPtyExit} - closeTab={closeTab} leaveWorktreeIfEmpty={leaveWorktreeIfEmpty} /> ) diff --git a/src/renderer/src/components/terminal-pane/pty-connection.test.ts b/src/renderer/src/components/terminal-pane/pty-connection.test.ts index 6c72e259c72..bb6425f338d 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -4957,6 +4957,7 @@ describe('connectPanePty', () => { connectPanePty(pane as never, manager as never, deps as never) capturedDataCallback.current?.('\x1b]133;D;130\x07thebr ~/repo $ ') + await flushAsyncTicks(1) expect(mockStoreState.dropAgentStatus).toHaveBeenCalledWith(paneKey) expect(mockStoreState.removeAgentStatus).not.toHaveBeenCalled() @@ -4998,6 +4999,7 @@ describe('connectPanePty', () => { expect.objectContaining({ agentType: 'codex' }) ) capturedDataCallback.current?.('\x1b]133;D;130\x07thebr ~/repo $ ') + await flushAsyncTicks(1) expect(mockStoreState.clearAgentLaunchConfig).toHaveBeenCalledWith(paneKey) expect(mockStoreState.dropAgentStatus).not.toHaveBeenCalled() @@ -7836,6 +7838,65 @@ describe('connectPanePty', () => { expect(transport.sendInput).not.toHaveBeenCalled() }) + it('renders the reattach snapshot before live bytes delivered during the spawn reply', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport('tab-pty') + transport.connect.mockImplementation( + async ({ sessionId, callbacks }: { sessionId?: string; callbacks?: ConnectCallbacks }) => { + if (!sessionId) { + return null + } + // Why: the real dispatcher drains bytes emitted after snapshot capture + // as soon as the spawn IPC resolves, before connect() returns. + callbacks?.onData?.('post-snapshot-live') + return { id: sessionId, snapshot: 'authoritative-snapshot' } + } + ) + transportFactoryQueue.push(transport) + const pane = createPane(1) + const { writes } = captureCallbackTerminalWrites(pane) + const deps = createDeps({ + restoredLeafId: LEAF_1, + restoredPtyIdByLeafId: { [LEAF_1]: 'tab-pty' } + }) + + connectPanePty(pane as never, createManager(1) as never, deps as never) + await flushAsyncTicks(20) + + const snapshotIndex = writes.indexOf('authoritative-snapshot') + const liveIndex = writes.indexOf('post-snapshot-live') + expect(snapshotIndex).toBeGreaterThanOrEqual(0) + expect(liveIndex).toBeGreaterThan(snapshotIndex) + }) + + it('does not fresh-spawn after a dead deferred session delivers its buffered exit', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport('tab-pty') + transport.connect.mockImplementation( + async ({ sessionId, callbacks }: { sessionId?: string; callbacks?: ConnectCallbacks }) => { + if (!sessionId) { + return 'unexpected-fresh-pty' + } + callbacks?.onData?.('dead-session-final-frame') + return { id: sessionId, exitedBeforeAttach: true } + } + ) + transportFactoryQueue.push(transport) + const pane = createPane(1) + const { writes } = captureCallbackTerminalWrites(pane) + const deps = createDeps({ + restoredLeafId: LEAF_1, + restoredPtyIdByLeafId: { [LEAF_1]: 'tab-pty' } + }) + + connectPanePty(pane as never, createManager(1) as never, deps as never) + await flushAsyncTicks(20) + + expect(transport.connect).toHaveBeenCalledTimes(1) + expect(writes).toContain('dead-session-final-frame') + expect(deps.updateTabPtyId).not.toHaveBeenCalled() + }) + // Why: Phase 6 deleted the hidden-skip eligibility grammar. With the kill // switch off, EVERY hidden chunk — plain, control-heavy, rich glyphs, // synchronized frames, embedded queries — rides the bounded background diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index ae752d77d97..2294c8ef9d7 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -3117,6 +3117,10 @@ export function connectPanePty( const hadExistingPaneTransportAtConnect = deps.paneTransportsRef.current.size > 0 let lastTerminalInputAt = Number.NEGATIVE_INFINITY let hasReceivedPtyOutput = false + let deferredReattachLiveData: { data: string; meta?: PtyDataMeta }[] | null = null + let deferredReattachLiveDataChars = 0 + const MAX_DEFERRED_REATTACH_LIVE_CHARS = 512 * 1024 + const MAX_DEFERRED_REATTACH_LIVE_CHUNKS = 1_024 const markTerminalInputSent = (): void => { lastTerminalInputAt = performance.now() } @@ -6275,6 +6279,26 @@ export function connectPanePty( } const dataCallback = (data: string, meta?: PtyDataMeta): void => { + if (deferredReattachLiveData !== null) { + deferredReattachLiveData.push({ data, ...(meta ? { meta } : {}) }) + deferredReattachLiveDataChars += data.length + let dropped = false + while ( + deferredReattachLiveData.length > MAX_DEFERRED_REATTACH_LIVE_CHUNKS || + deferredReattachLiveDataChars > MAX_DEFERRED_REATTACH_LIVE_CHARS + ) { + const removed = deferredReattachLiveData.shift() + deferredReattachLiveDataChars -= removed?.data.length ?? 0 + dropped = true + } + if (dropped && deferredReattachLiveData[0]) { + deferredReattachLiveData[0].meta = { + ...deferredReattachLiveData[0].meta, + droppedOutput: true + } + } + return + } if (data.length > 0) { hasReceivedPtyOutput = true recordAgentHibernationPaneOutput(cacheKey) @@ -6464,17 +6488,44 @@ export function connectPanePty( } }) + const beginReattachLiveDataDeferral = (): void => { + deferredReattachLiveData = [] + deferredReattachLiveDataChars = 0 + } + + const finishReattachLiveDataDeferral = (deliver: boolean): void => { + const chunks = deferredReattachLiveData + deferredReattachLiveData = null + deferredReattachLiveDataChars = 0 + if (!deliver || disposed || !chunks) { + return + } + // Why: createOrAttach snapshots precede bytes emitted before its IPC + // reply. Paint the authoritative replay first, then admit those live + // chunks so the replay clear cannot erase newer output. + for (const chunk of chunks) { + dataCallback(chunk.data, chunk.meta) + } + } + const handleReattachResult = ( result: PtyConnectResult | string | void, staleSessionId?: string | null, coldRestoreStartup?: ColdRestoreAgentResumeStartup | null - ): void => { + ): boolean => { if (disposed) { - return + return false } const connectResult = result && typeof result === 'object' && 'id' in result ? (result as PtyConnectResult) : null + if (connectResult?.exitedBeforeAttach) { + // Why: the transport already delivered the dead session's final frame + // and exit. Treat it as terminal state, not a failed reattach that + // should silently replace a cancelled pinned exit with a fresh shell. + return true + } + const ptyId = connectResult?.id ?? (typeof result === 'string' ? result : transport.getPtyId()) if (!ptyId) { @@ -6498,7 +6549,7 @@ export function connectPanePty( startFreshColdRestoreAgentResume(coldRestoreStartup, { forceBlankRestoredViewport: true }) - return + return false } registerEffectiveLaunchConfig(connectResult?.launchConfig, { ...(coldRestoreStartup ? { launchToken: coldRestoreStartup.launchToken } : {}), @@ -6523,7 +6574,7 @@ export function connectPanePty( startFreshColdRestoreAgentResume(coldRestoreStartup, { forceBlankRestoredViewport: true }) - return + return false } setPanePtyFitBinding(ptyId) reportPanePtyVisibility(ptyId, deps.isVisibleRef.current) @@ -6673,6 +6724,7 @@ export function connectPanePty( scheduleReattachIdleAgentCursorReset() scheduleRuntimeGraphSync() + return true } // Why: if this tab has a deferred SSH session ID, trigger the SSH @@ -6867,6 +6919,7 @@ export function connectPanePty( const coldRestoreStartup = buildColdRestoreAgentResumeStartup() clearPaneMode2031State() clearHiddenOutputRestoreState() + beginReattachLiveDataDeferral() const reattachPromise = transport.connect({ url: '', cols, @@ -6908,6 +6961,7 @@ export function connectPanePty( : 'undefined' ) if (!result && expiredReattachError) { + finishReattachLiveDataDeferral(false) const gen = await preSignalPromise if (typeof gen === 'number') { void window.api.pty.clearPendingPaneSerializer(cacheKey, gen).catch(() => {}) @@ -6922,7 +6976,8 @@ export function connectPanePty( }) return } - handleReattachResult(result, pendingSessionId, coldRestoreStartup) + const accepted = handleReattachResult(result, pendingSessionId, coldRestoreStartup) + finishReattachLiveDataDeferral(accepted) const gen = await preSignalPromise if (typeof gen === 'number') { if (!isRemoteRuntimePtyId(pendingSessionId)) { @@ -6931,6 +6986,7 @@ export function connectPanePty( } }) .catch(async (err) => { + finishReattachLiveDataDeferral(false) const gen = await preSignalPromise if (typeof gen === 'number') { void window.api.pty.clearPendingPaneSerializer(cacheKey, gen).catch(() => {}) @@ -7053,6 +7109,7 @@ export function connectPanePty( let expiredReattachError = false const coldRestoreStartup = buildColdRestoreAgentResumeStartup() + beginReattachLiveDataDeferral() const reattachPromise = transport.connect({ url: '', cols, @@ -7084,6 +7141,7 @@ export function connectPanePty( void Promise.resolve(reattachPromise) .then(async (result) => { if (!result && expiredReattachError) { + finishReattachLiveDataDeferral(false) const gen = await preSignalPromise if (typeof gen === 'number') { void window.api.pty.clearPendingPaneSerializer(cacheKey, gen).catch(() => {}) @@ -7098,7 +7156,12 @@ export function connectPanePty( }) return } - handleReattachResult(result, deferredReattachSessionId, coldRestoreStartup) + const accepted = handleReattachResult( + result, + deferredReattachSessionId, + coldRestoreStartup + ) + finishReattachLiveDataDeferral(accepted) const gen = await preSignalPromise if (typeof gen === 'number') { if (!isRemoteRuntimePtyId(deferredReattachSessionId)) { @@ -7107,6 +7170,7 @@ export function connectPanePty( } }) .catch(async (err) => { + finishReattachLiveDataDeferral(false) const gen = await preSignalPromise if (typeof gen === 'number') { void window.api.pty.clearPendingPaneSerializer(cacheKey, gen).catch(() => {}) diff --git a/src/renderer/src/components/terminal-pane/pty-dispatcher-pi-routing.test.ts b/src/renderer/src/components/terminal-pane/pty-dispatcher-pi-routing.test.ts index 2df88ac2973..ae74fb79d34 100644 --- a/src/renderer/src/components/terminal-pane/pty-dispatcher-pi-routing.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-dispatcher-pi-routing.test.ts @@ -309,6 +309,31 @@ describe('dispatcher → transport → onTitleChange for Pi spinner', () => { transport.disconnect() }) + it('preserves buffered live-session data when reconnect re-admits a consumed id', async () => { + const { consumePreHandlerPtyState } = await import('./pty-pre-handler-buffer') + const { createIpcPtyTransport } = await import('./pty-transport') + const { ensurePtyDispatcher } = await import('./pty-dispatcher') + const onData = vi.fn() + + ensurePtyDispatcher() + consumePreHandlerPtyState('pty-persisted') + dispatcherCallback?.({ id: 'pty-persisted', data: 'live before pane\r\n', rawLength: 18 }) + ;(window.api.pty.spawn as ReturnType).mockResolvedValueOnce({ + id: 'pty-persisted', + isReattach: true + }) + + const transport = createIpcPtyTransport() + await transport.connect({ + url: '', + sessionId: 'pty-persisted', + callbacks: { onData } + }) + + expect(onData).toHaveBeenCalledWith('live before pane\r\n', { rawLength: 18 }) + transport.disconnect() + }) + it('replays pre-handler PTY data before a pre-handler exit', async () => { const { createIpcPtyTransport } = await import('./pty-transport') const events: string[] = [] @@ -320,7 +345,12 @@ describe('dispatcher → transport → onTitleChange for Pi spinner', () => { }) ) - const transport = createIpcPtyTransport() + const cleanupError = vi.spyOn(console, 'error').mockImplementation(() => {}) + const transport = createIpcPtyTransport({ + onPtyExit: () => { + throw new Error('pre-attach cleanup failed') + } + }) const connectPromise = transport.connect({ url: '', callbacks: { @@ -332,8 +362,46 @@ describe('dispatcher → transport → onTitleChange for Pi spinner', () => { dispatcherCallback?.({ id: 'pty-fast-exit', data: 'last line\r\n', rawLength: 11 }) exitDispatcherCallback?.({ id: 'pty-fast-exit', code: 3 }) resolveSpawn({ id: 'pty-fast-exit' }) - await connectPromise + const connectResult = await connectPromise expect(events).toEqual(['data:last line\r\n', 'exit:3']) + expect(connectResult).toEqual({ id: 'pty-fast-exit', exitedBeforeAttach: true }) + expect(cleanupError).toHaveBeenCalled() + + exitDispatcherCallback?.({ id: 'pty-fast-exit', code: 4 }) + const duplicateExit = vi.fn() + const { drainPreHandlerPtyExit } = await import('./pty-pre-handler-buffer') + drainPreHandlerPtyExit('pty-fast-exit', duplicateExit) + expect(duplicateExit).not.toHaveBeenCalled() + }) + + it('finalizes a throwing primary exit and still delivers every sidecar', async () => { + const { ensurePtyDispatcher, ptyExitHandlers, subscribeToPtyExit } = + await import('./pty-dispatcher') + const primary = vi.fn(() => { + throw new Error('primary exit failed') + }) + const firstSidecar = vi.fn(() => { + throw new Error('sidecar exit failed') + }) + const secondSidecar = vi.fn() + + ensurePtyDispatcher() + ptyExitHandlers.set('pty-throwing-exit', primary) + subscribeToPtyExit('pty-throwing-exit', firstSidecar) + subscribeToPtyExit('pty-throwing-exit', secondSidecar) + + expect(() => exitDispatcherCallback?.({ id: 'pty-throwing-exit', code: 9 })).toThrow( + 'primary exit failed' + ) + expect(firstSidecar).toHaveBeenCalledWith(9, { hadPrimary: true }) + expect(secondSidecar).toHaveBeenCalledWith(9, { hadPrimary: true }) + + expect(() => exitDispatcherCallback?.({ id: 'pty-throwing-exit', code: 10 })).not.toThrow() + expect(primary).toHaveBeenCalledTimes(1) + const duplicateExit = vi.fn() + const { drainPreHandlerPtyExit } = await import('./pty-pre-handler-buffer') + drainPreHandlerPtyExit('pty-throwing-exit', duplicateExit) + expect(duplicateExit).not.toHaveBeenCalled() }) }) diff --git a/src/renderer/src/components/terminal-pane/pty-dispatcher.ts b/src/renderer/src/components/terminal-pane/pty-dispatcher.ts index b563443d1ee..2b8781a5221 100644 --- a/src/renderer/src/components/terminal-pane/pty-dispatcher.ts +++ b/src/renderer/src/components/terminal-pane/pty-dispatcher.ts @@ -15,11 +15,11 @@ import { import { clampUtf8Tail, type EagerBufferChunk } from './pty-eager-buffer-clamp' import { bufferPreHandlerPtyData, - bufferPreHandlerPtyExit, clearPreHandlerPtyState, drainPreHandlerPtyData, drainPreHandlerPtyExit } from './pty-pre-handler-buffer' +import { deliverPtyExitToHandlers } from './pty-exit-delivery' import { clearReceivedPtyCharTotal, isPtyPushDeliveryBlackholed, @@ -57,7 +57,10 @@ export const ptyDataSidecars = new Map void>>() * guard and suppress xterm auto-replies during replay. */ export const ptyReplayHandlers = new Map void>() export const ptyExitHandlers = new Map void>() -const ptyExitSidecars = new Map void>>() +const ptyExitSidecars = new Map< + string, + Set<(code: number, context: { hadPrimary: boolean }) => void> +>() /** Per-PTY teardown callbacks registered by each transport to clear closure * state (stale-title timer, agent tracker) that would otherwise fire after * the data handler is removed. */ @@ -231,21 +234,22 @@ function attachPtySecondaryPushListeners(unsubscribes: (() => void)[]): void { // cumulative totals too so a reused id restarts at zero on both sides. clearProcessedPtyCharTotal(payload.id) clearReceivedPtyCharTotal(payload.id) - const handler = ptyExitHandlers.get(payload.id) - if (handler) { - clearPreHandlerPtyState(payload.id) - handler(payload.code) - } else { - bufferPreHandlerPtyExit(payload.id, payload.code) - } const sidecars = ptyExitSidecars.get(payload.id) - if (sidecars && sidecars.size > 0) { - const snapshot = Array.from(sidecars) + if (sidecars) { ptyExitSidecars.delete(payload.id) - for (const sidecar of snapshot) { - sidecar(payload.code) - } } + const primary = ptyExitHandlers.get(payload.id) + if (primary) { + // Why: exit handlers are one-shot owners. Remove before invocation so + // a throwing callback cannot stay registered for a duplicate exit. + ptyExitHandlers.delete(payload.id) + } + deliverPtyExitToHandlers({ + ptyId: payload.id, + code: payload.code, + ...(primary ? { primary } : {}), + sidecars: sidecars ? Array.from(sidecars) : [] + }) }) ) // Why: main probes when delivery looks stuck on lost ACKs (data arriving @@ -267,7 +271,10 @@ function attachPtySecondaryPushListeners(unsubscribes: (() => void)[]): void { window.api.pty.rendererDispatcherReady?.() } -export function subscribeToPtyExit(ptyId: string, watcher: (code: number) => void): () => void { +export function subscribeToPtyExit( + ptyId: string, + watcher: (code: number, context: { hadPrimary: boolean }) => void +): () => void { ensurePtyDispatcher() let set = ptyExitSidecars.get(ptyId) if (!set) { diff --git a/src/renderer/src/components/terminal-pane/pty-exit-delivery.ts b/src/renderer/src/components/terminal-pane/pty-exit-delivery.ts new file mode 100644 index 00000000000..9180aabf890 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-exit-delivery.ts @@ -0,0 +1,49 @@ +import { + bufferPreHandlerPtyExit, + clearPreHandlerPtyState, + consumePreHandlerPtyState +} from './pty-pre-handler-buffer' + +type PtyExitDelivery = { + ptyId: string + code: number + primary?: (code: number) => void + sidecars: readonly ((code: number, context: { hadPrimary: boolean }) => void)[] +} + +/** Delivers one exit to its primary owner and every observational sidecar. */ +export function deliverPtyExitToHandlers(delivery: PtyExitDelivery): void { + let firstError: unknown + let hasError = false + try { + if (delivery.primary) { + clearPreHandlerPtyState(delivery.ptyId) + try { + delivery.primary(delivery.code) + } finally { + // Why: ownership is final even when cleanup throws; a duplicate exit + // must not become a new pre-handler event for a future mount. + consumePreHandlerPtyState(delivery.ptyId) + } + } else { + bufferPreHandlerPtyExit(delivery.ptyId, delivery.code) + } + } catch (error) { + firstError = error + hasError = true + } + + for (const sidecar of delivery.sidecars) { + try { + sidecar(delivery.code, { hadPrimary: delivery.primary !== undefined }) + } catch (error) { + if (!hasError) { + firstError = error + hasError = true + } + } + } + if (hasError) { + throw firstError + } +} diff --git a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts index 7d95304ba90..461e95edfed 100644 --- a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts @@ -1,17 +1,29 @@ -import { afterEach, describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import { bufferPreHandlerPtyData, + bufferPreHandlerPtyExit, + clearConsumedPreHandlerPtyExit, clearPreHandlerPtyState, - drainPreHandlerPtyData + consumePreHandlerPtyState, + drainPreHandlerPtyData, + drainPreHandlerPtyExit, + discardPreHandlerPtyState, + hasPreHandlerPtyExit } from './pty-pre-handler-buffer' const RESCAN_PTY_ID = 'pty-pre-handler-rescan' const TRIM_PTY_ID = 'pty-pre-handler-trim' +const EXIT_PTY_ID = 'pty-pre-handler-exit' +const CAPPED_EXIT_PTY_IDS = Array.from({ length: 65 }, (_, index) => `pty-capped-exit-${index}`) describe('pre-handler PTY buffer', () => { afterEach(() => { clearPreHandlerPtyState(RESCAN_PTY_ID) clearPreHandlerPtyState(TRIM_PTY_ID) + clearPreHandlerPtyState(EXIT_PTY_ID) + for (const ptyId of CAPPED_EXIT_PTY_IDS) { + clearPreHandlerPtyState(ptyId) + } }) it('does not rescan historical chunks while buffering small startup output', () => { @@ -71,4 +83,91 @@ describe('pre-handler PTY buffer', () => { expect(drained).toHaveLength(512) expect(drained.join('')).toHaveLength(512 * 1_024) }) + + it('suppresses duplicate exits after an owner consumes the pre-handler state', () => { + const onExit = vi.fn() + bufferPreHandlerPtyExit(EXIT_PTY_ID, 1) + consumePreHandlerPtyState(EXIT_PTY_ID) + bufferPreHandlerPtyExit(EXIT_PTY_ID, 2) + + drainPreHandlerPtyExit(EXIT_PTY_ID, onExit) + expect(onExit).not.toHaveBeenCalled() + + bufferPreHandlerPtyData(EXIT_PTY_ID, 'reattach output') + clearConsumedPreHandlerPtyExit(EXIT_PTY_ID) + const data: string[] = [] + drainPreHandlerPtyData(EXIT_PTY_ID, (chunk) => data.push(chunk)) + bufferPreHandlerPtyExit(EXIT_PTY_ID, 3) + drainPreHandlerPtyExit(EXIT_PTY_ID, onExit) + expect(data).toEqual(['reattach output']) + expect(onExit).toHaveBeenCalledWith(3) + }) + + it('consumes a drained exit even when its handler throws', () => { + bufferPreHandlerPtyExit(EXIT_PTY_ID, 7) + expect(() => + drainPreHandlerPtyExit(EXIT_PTY_ID, () => { + throw new Error('exit handler failed') + }) + ).toThrow('exit handler failed') + + bufferPreHandlerPtyExit(EXIT_PTY_ID, 8) + const duplicateExit = vi.fn() + drainPreHandlerPtyExit(EXIT_PTY_ID, duplicateExit) + expect(duplicateExit).not.toHaveBeenCalled() + }) + + it('reports whether an undelivered exit is waiting for admission', () => { + expect(hasPreHandlerPtyExit(EXIT_PTY_ID)).toBe(false) + bufferPreHandlerPtyExit(EXIT_PTY_ID, 7) + expect(hasPreHandlerPtyExit(EXIT_PTY_ID)).toBe(true) + drainPreHandlerPtyExit(EXIT_PTY_ID, vi.fn()) + expect(hasPreHandlerPtyExit(EXIT_PTY_ID)).toBe(false) + }) + + it('discards delayed data and exit until an explicit reconnect', () => { + discardPreHandlerPtyState(EXIT_PTY_ID) + bufferPreHandlerPtyData(EXIT_PTY_ID, 'late data') + bufferPreHandlerPtyExit(EXIT_PTY_ID, 9) + const data = vi.fn() + const exit = vi.fn() + drainPreHandlerPtyData(EXIT_PTY_ID, data) + drainPreHandlerPtyExit(EXIT_PTY_ID, exit) + expect(data).not.toHaveBeenCalled() + expect(exit).not.toHaveBeenCalled() + + clearConsumedPreHandlerPtyExit(EXIT_PTY_ID) + bufferPreHandlerPtyData(EXIT_PTY_ID, 'new incarnation') + drainPreHandlerPtyData(EXIT_PTY_ID, data) + expect(data).toHaveBeenCalledWith('new incarnation', undefined) + }) + + it('retains discard protection beyond the bounded exit-buffer capacity', () => { + for (const ptyId of CAPPED_EXIT_PTY_IDS) { + discardPreHandlerPtyState(ptyId) + } + + bufferPreHandlerPtyData(CAPPED_EXIT_PTY_IDS[0], 'late data') + bufferPreHandlerPtyExit(CAPPED_EXIT_PTY_IDS[0], 9) + const data = vi.fn() + const exit = vi.fn() + drainPreHandlerPtyData(CAPPED_EXIT_PTY_IDS[0], data) + drainPreHandlerPtyExit(CAPPED_EXIT_PTY_IDS[0], exit) + expect(data).not.toHaveBeenCalled() + expect(exit).not.toHaveBeenCalled() + }) + + it('caps exits buffered without a primary handler', () => { + for (const [code, ptyId] of CAPPED_EXIT_PTY_IDS.entries()) { + bufferPreHandlerPtyExit(ptyId, code) + } + + const exits: number[] = [] + for (const ptyId of CAPPED_EXIT_PTY_IDS) { + drainPreHandlerPtyExit(ptyId, (code) => exits.push(code)) + } + expect(exits).toHaveLength(64) + expect(exits).not.toContain(0) + expect(exits).toContain(64) + }) }) diff --git a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts index c3d12fed9fb..83b3a10f1e2 100644 --- a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts +++ b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts @@ -15,12 +15,16 @@ type BufferedPreHandlerPtyState = { const preHandlerPtyData = new Map() const preHandlerPtyExit = new Map() +const consumedPreHandlerPtyExits = new Map() +const discardedPreHandlerPtyStates = new Map>() +const DISCARDED_PRE_HANDLER_PTY_STATE_TTL_MS = 60_000 // Why: Windows startup commands can emit output before pty:spawn resolves and // the pane registers its handler. Hold that tiny race window instead of ACKing // and dropping the first setup-script bytes. const PRE_HANDLER_PTY_DATA_MAX_BYTES = 512 * 1024 const PRE_HANDLER_PTY_DATA_MAX_PTYS = 64 +const PRE_HANDLER_PTY_EXIT_MAX_PTYS = 64 // Why: legit pre-attach windows drain within milliseconds and hold little // data. Sustained accumulation means a pane lost its data handler (the // frozen-pane detach/attach race) — leave a breadcrumb for trace capture. @@ -28,6 +32,9 @@ const PRE_HANDLER_PTY_DATA_WARN_BYTES = 64 * 1024 const warnedLostHandlerPtyIds = new Set() export function bufferPreHandlerPtyData(ptyId: string, data: string, meta?: PtyDataMeta): void { + if (discardedPreHandlerPtyStates.has(ptyId)) { + return + } const chunk = clampUtf8Tail(data, PRE_HANDLER_PTY_DATA_MAX_BYTES) if (!chunk.data) { return @@ -90,16 +97,83 @@ export function drainPreHandlerPtyData( } export function bufferPreHandlerPtyExit(ptyId: string, code: number): void { + if (consumedPreHandlerPtyExits.has(ptyId) || discardedPreHandlerPtyStates.has(ptyId)) { + return + } + if (!preHandlerPtyExit.has(ptyId) && preHandlerPtyExit.size >= PRE_HANDLER_PTY_EXIT_MAX_PTYS) { + const oldestPtyId = preHandlerPtyExit.keys().next().value + if (typeof oldestPtyId === 'string') { + preHandlerPtyExit.delete(oldestPtyId) + } + } preHandlerPtyExit.set(ptyId, code) } +// Why: primary handlers and pane-less parked owners have fully handled this +// exit. Keep a bounded tombstone so duplicate IPC exits cannot be replayed to +// a future mount or accumulate in the pre-handler map. +export function consumePreHandlerPtyState(ptyId: string): void { + clearPreHandlerPtyState(ptyId) + consumedPreHandlerPtyExits.set(ptyId, true) + if (consumedPreHandlerPtyExits.size > PRE_HANDLER_PTY_EXIT_MAX_PTYS) { + const oldestPtyId = consumedPreHandlerPtyExits.keys().next().value + if (typeof oldestPtyId === 'string') { + consumedPreHandlerPtyExits.delete(oldestPtyId) + } + } +} + +// Why: a deliberate reconnect can reuse a live session id after a prior +// incarnation's consumed-exit mark. Re-admit exits without discarding bytes +// already buffered for the still-live session. +export function clearConsumedPreHandlerPtyExit(ptyId: string): void { + consumedPreHandlerPtyExits.delete(ptyId) + const discardTimer = discardedPreHandlerPtyStates.get(ptyId) + if (discardTimer) { + clearTimeout(discardTimer) + } + discardedPreHandlerPtyStates.delete(ptyId) +} + +export function isPreHandlerPtyStateDiscarded(ptyId: string): boolean { + return discardedPreHandlerPtyStates.has(ptyId) +} + +// Why: removed worktrees have no future pane consumer. Suppress both delayed +// kill data and exit until an explicit same-id reconnect establishes a new +// admission boundary. +export function discardPreHandlerPtyState(ptyId: string): void { + consumePreHandlerPtyState(ptyId) + const priorTimer = discardedPreHandlerPtyStates.get(ptyId) + if (priorTimer) { + clearTimeout(priorTimer) + } + // Why: a large worktree can remove more PTYs than the bounded data maps. + // Time retention protects every delayed kill flush without permanent growth. + const timer = setTimeout( + () => discardedPreHandlerPtyStates.delete(ptyId), + DISCARDED_PRE_HANDLER_PTY_STATE_TTL_MS + ) + discardedPreHandlerPtyStates.set(ptyId, timer) +} + +export function hasPreHandlerPtyExit(ptyId: string): boolean { + return preHandlerPtyExit.has(ptyId) +} + export function drainPreHandlerPtyExit(ptyId: string, handler: (code: number) => void): void { const code = preHandlerPtyExit.get(ptyId) if (code === undefined) { return } preHandlerPtyExit.delete(ptyId) - handler(code) + try { + handler(code) + } finally { + // Why: draining transfers ownership to this handler. Even when it throws, + // a duplicate exit must not become a new pre-handler event. + consumePreHandlerPtyState(ptyId) + } } export function clearPreHandlerPtyData(ptyId: string): void { @@ -110,5 +184,11 @@ export function clearPreHandlerPtyData(ptyId: string): void { export function clearPreHandlerPtyState(ptyId: string): void { preHandlerPtyData.delete(ptyId) preHandlerPtyExit.delete(ptyId) + consumedPreHandlerPtyExits.delete(ptyId) + const discardTimer = discardedPreHandlerPtyStates.get(ptyId) + if (discardTimer) { + clearTimeout(discardTimer) + } + discardedPreHandlerPtyStates.delete(ptyId) warnedLostHandlerPtyIds.delete(ptyId) } diff --git a/src/renderer/src/components/terminal-pane/pty-transport-types.ts b/src/renderer/src/components/terminal-pane/pty-transport-types.ts index 32da5a2053b..1777091c376 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport-types.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport-types.ts @@ -39,6 +39,9 @@ export type LocalPtySessionMetadata = { export type PtyConnectResult = { id: string + /** The requested session exited while it had no primary pane handler. Its + * buffered final data/exit were delivered, so callers must not fresh-spawn. */ + exitedBeforeAttach?: boolean launchAgent?: TuiAgent launchConfig?: SleepingAgentLaunchConfig snapshot?: string diff --git a/src/renderer/src/components/terminal-pane/pty-transport.test.ts b/src/renderer/src/components/terminal-pane/pty-transport.test.ts index 564c6ab4bf3..e3bf3152e7c 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.test.ts @@ -342,6 +342,54 @@ describe('createIpcPtyTransport', () => { transport.disconnect() }) + it('mints a fresh id instead of reopening a discarded same-id session', async () => { + const { discardPreHandlerPtyState, clearPreHandlerPtyState } = + await import('./pty-pre-handler-buffer') + const { createIpcPtyTransport } = await import('./pty-transport') + const spawn = window.api.pty.spawn as unknown as ReturnType + const discardedId = 'removed-worktree@@discarded-session' + discardPreHandlerPtyState(discardedId) + + await createIpcPtyTransport({ cwdFallback: 'worktree' }).connect({ + url: '', + sessionId: discardedId, + callbacks: {} + }) + + expect(spawn).toHaveBeenCalledWith(expect.objectContaining({ cwdFallback: 'worktree' })) + expect(spawn).toHaveBeenCalledWith(expect.not.objectContaining({ sessionId: discardedId })) + clearPreHandlerPtyState(discardedId) + }) + + it('delivers a buffered dead-session exit without respawning the same session id', async () => { + const { bufferPreHandlerPtyData, bufferPreHandlerPtyExit } = + await import('./pty-pre-handler-buffer') + const { createIpcPtyTransport } = await import('./pty-transport') + const spawn = window.api.pty.spawn as unknown as ReturnType + const onDataCallback = vi.fn() + const onExitCallback = vi.fn() + const onDisconnect = vi.fn() + const onPtyExit = vi.fn() + const sessionId = 'dead-parked-session' + bufferPreHandlerPtyData(sessionId, 'final output') + bufferPreHandlerPtyExit(sessionId, 17) + + const transport = createIpcPtyTransport({ onPtyExit }) + const result = await transport.connect({ + url: '', + sessionId, + callbacks: { onData: onDataCallback, onExit: onExitCallback, onDisconnect } + }) + + expect(result).toEqual({ id: sessionId, exitedBeforeAttach: true }) + expect(spawn).not.toHaveBeenCalled() + expect(onDataCallback).toHaveBeenCalledWith('final output') + expect(onExitCallback).toHaveBeenCalledWith(17) + expect(onDisconnect).toHaveBeenCalledTimes(1) + expect(onPtyExit).toHaveBeenCalledWith(sessionId) + expect(transport.isConnected()).toBe(false) + }) + it('returns startup cwd fallback metadata to the connection layer', async () => { const { createIpcPtyTransport } = await import('./pty-transport') const spawn = window.api.pty.spawn as unknown as ReturnType @@ -858,7 +906,7 @@ describe('createIpcPtyTransport', () => { onExit?.({ id: 'pty-restored', code: 0 }) expect(eagerExit).not.toHaveBeenCalled() - expect(sidecarExit).toHaveBeenCalledWith(0) + expect(sidecarExit).toHaveBeenCalledWith(0, { hadPrimary: true }) }) it('fires onBell for bare BELs but ignores BELs inside OSC sequences', async () => { diff --git a/src/renderer/src/components/terminal-pane/pty-transport.ts b/src/renderer/src/components/terminal-pane/pty-transport.ts index a58a117212f..d2e946e8a9d 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.ts @@ -22,7 +22,13 @@ import { ensurePtyDispatcher, getEagerPtyBufferHandle } from './pty-dispatcher' -import { drainPreHandlerPtyData, drainPreHandlerPtyExit } from './pty-pre-handler-buffer' +import { + clearConsumedPreHandlerPtyExit, + drainPreHandlerPtyData, + drainPreHandlerPtyExit, + hasPreHandlerPtyExit, + isPreHandlerPtyStateDiscarded +} from './pty-pre-handler-buffer' import { createPtyInputWriteQueue } from './pty-input-write-queue' import type { PtyDataMeta } from './pty-dispatcher' import type { IpcPtyTransportOptions, PtyConnectResult, PtyTransport } from './pty-transport-types' @@ -649,7 +655,8 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra } } - function registerPtyExitHandler(id: string): void { + function registerPtyExitHandler(id: string): boolean { + const hadBufferedExit = hasPreHandlerPtyExit(id) const exitHandler = (code: number): void => { if (ptyId !== null && ptyId !== id) { // Why: a preserved sleep/reconnect session can report its old exit @@ -674,7 +681,17 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra // would otherwise fire stale notifications after the data handler // is removed but before the exit event arrives. ptyTeardownHandlers.set(id, clearAccumulatedState) - drainPreHandlerPtyExit(id, exitHandler) + try { + drainPreHandlerPtyExit(id, exitHandler) + } catch (error) { + if (!hadBufferedExit) { + throw error + } + // Why: cleanup callback failures must not turn an already-delivered + // pre-attach exit into a generic connect rejection and fallback spawn. + console.error('[pty] buffered pre-attach exit cleanup failed', error) + } + return hadBufferedExit } return { @@ -686,12 +703,35 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra return } + if (options.sessionId && hasPreHandlerPtyExit(options.sessionId)) { + // Why: an exited parked session can be recreated under the same id by + // createOrAttach. Deliver its buffered final frame/exit before spawn so + // the dead incarnation cannot disconnect and orphan a fresh shell. + ptyId = options.sessionId + connected = true + registerPtyDataHandler(options.sessionId) + registerPtyExitHandler(options.sessionId) + return { id: options.sessionId, exitedBeforeAttach: true } satisfies PtyConnectResult + } + + const admittedSessionId = + options.sessionId && !isPreHandlerPtyStateDiscarded(options.sessionId) + ? options.sessionId + : undefined + + // Why: reconnecting may intentionally reuse a session id whose prior + // exit was consumed. Re-admit exits without clearing bytes already + // buffered for a still-live session before its pane registered. + if (admittedSessionId) { + clearConsumedPreHandlerPtyExit(admittedSessionId) + } + try { // Why: missing-cwd recovery is only valid for fresh local spawns — // reattach must keep the session's exact cwd and SSH-tagged transports // resolve cwd on the remote host. const shouldSendLocalCwdFallback = - cwdFallback === 'worktree' && !connectionId && !options.sessionId + cwdFallback === 'worktree' && !connectionId && !admittedSessionId const result = await window.api.pty.spawn({ cols: options.cols ?? 80, rows: options.rows ?? 24, @@ -712,7 +752,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra ? { startupCommandDelivery: options.startupCommandDelivery ?? startupCommandDelivery } : {}), ...(connectionId ? { connectionId } : {}), - ...(options.sessionId ? { sessionId: options.sessionId } : {}), + ...(admittedSessionId ? { sessionId: admittedSessionId } : {}), // Why: hidden-at-spawn mark must land in main before the PTY's // first byte, so it rides the spawn IPC instead of the pane's // first visibility sync (terminal-query-authority.md). @@ -747,7 +787,10 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra } registerPtyDataHandler(spawnResult.id) - registerPtyExitHandler(spawnResult.id) + const exitedBeforeAttach = registerPtyExitHandler(spawnResult.id) + if (exitedBeforeAttach) { + return { id: spawnResult.id, exitedBeforeAttach: true } satisfies PtyConnectResult + } if (!connected || ptyId !== spawnResult.id) { return undefined } diff --git a/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.test.ts b/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.test.ts index 6c5ee048ead..3bb11c1a3ea 100644 --- a/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.test.ts @@ -28,22 +28,36 @@ vi.mock('./parked-terminal-byte-watcher', () => ({ type ExitSubscription = { ptyId: string - callback: (code: number) => void + callback: (code: number, context: { hadPrimary: boolean }) => void unsubscribe: ReturnType } const exitSubscriptions: ExitSubscription[] = [] -const subscribeToPtyExit = vi.fn((ptyId: string, callback: (code: number) => void) => { - const unsubscribe = vi.fn() - exitSubscriptions.push({ ptyId, callback, unsubscribe }) - return unsubscribe -}) +const subscribeToPtyExit = vi.fn( + (ptyId: string, callback: (code: number, context: { hadPrimary: boolean }) => void) => { + const unsubscribe = vi.fn() + exitSubscriptions.push({ ptyId, callback, unsubscribe }) + return unsubscribe + } +) vi.mock('./pty-dispatcher', () => ({ subscribeToPtyExit: (ptyId: string, callback: (code: number) => void) => subscribeToPtyExit(ptyId, callback) })) +const consumePreHandlerPtyState = vi.fn() +vi.mock('./pty-pre-handler-buffer', () => ({ + discardPreHandlerPtyState: (ptyId: string) => consumePreHandlerPtyState(ptyId) +})) + +type CloseTerminalTabOptions = { onClosed?: () => void; onCancel?: () => void } +const closeTerminalTab = vi.fn() +vi.mock('../terminal/terminal-tab-actions', () => ({ + closeTerminalTab: (tabId: string, options?: CloseTerminalTabOptions) => + closeTerminalTab(tabId, options) +})) + type MockStoreState = { terminalLayoutsByTabId: Record< string, @@ -224,7 +238,7 @@ describe('terminal-parked-tab-watchers', () => { syncParked() const exited = exitSubscriptions.find((entry) => entry.ptyId === PTY_ID) - exited?.callback(0) + exited?.callback(0, { hadPrimary: false }) expect(startedWatchers[0].dispose).toHaveBeenCalledTimes(1) expect(startedWatchers[1].dispose).not.toHaveBeenCalled() @@ -244,16 +258,64 @@ describe('terminal-parked-tab-watchers', () => { expect(startedWatchers[1].options.initialTitle).toBeUndefined() }) - it('drops the parked tab entry when the pty-exit sidecar disposes the last watcher', () => { + it('consumes the exit and drops the parked entry after the last-watcher close resolves', () => { capturePanes([{ ptyId: PTY_ID, paneId: 1, leafId: LEAF_ID, drivesTabTitle: true }]) syncParked() - exitSubscriptions.find((entry) => entry.ptyId === PTY_ID)?.callback(0) + exitSubscriptions.find((entry) => entry.ptyId === PTY_ID)?.callback(0, { hadPrimary: false }) + expect(consumePreHandlerPtyState).not.toHaveBeenCalled() + expect(getParkedTerminalWatcherTabIds()).toEqual([TAB_ID]) + const options = closeTerminalTab.mock.calls[0]?.[1] as CloseTerminalTabOptions + options.onClosed?.() + + expect(consumePreHandlerPtyState).toHaveBeenCalledWith(PTY_ID) expect(startedWatchers[0].dispose).toHaveBeenCalledTimes(1) expect(getParkedTerminalWatcherTabIds()).toEqual([]) }) + it('retains the buffered exit and empty registry entry when pinned close is cancelled', () => { + capturePanes([{ ptyId: PTY_ID, paneId: 1, leafId: LEAF_ID, drivesTabTitle: true }]) + syncParked() + + exitSubscriptions.find((entry) => entry.ptyId === PTY_ID)?.callback(0, { hadPrimary: false }) + const options = closeTerminalTab.mock.calls[0]?.[1] as CloseTerminalTabOptions + options.onCancel?.() + syncParked({ parkedTabIds: [] }) + + expect(consumePreHandlerPtyState).not.toHaveBeenCalled() + expect(startParkedTerminalByteWatcher).toHaveBeenCalledTimes(1) + expect(getParkedTerminalWatcherTabIds()).toEqual([TAB_ID]) + + expect(shouldDeferParkedPtyExitTabClose(TAB_ID, PTY_ID)).toBe(true) + expect(getParkedTerminalWatcherTabIds()).toEqual([]) + expect(shouldDeferParkedPtyExitTabClose(TAB_ID, PTY_ID)).toBe(false) + }) + + it('consumes a queued parked exit when another confirmation closes the tab first', () => { + capturePanes([{ ptyId: PTY_ID, paneId: 1, leafId: LEAF_ID, drivesTabTitle: true }]) + syncParked() + exitSubscriptions.find((entry) => entry.ptyId === PTY_ID)?.callback(0, { hadPrimary: false }) + + // Why: a manual pinned close can be ahead of this autonomous exit request + // in the confirmation queue and remove the tab before the exit resolves. + syncParked({ tabs: [] }) + + expect(consumePreHandlerPtyState).toHaveBeenCalledWith(PTY_ID) + expect(getParkedTerminalWatcherTabIds()).toEqual([]) + }) + + it('does not queue a second close when a retained primary handled the parked exit', () => { + capturePanes([{ ptyId: PTY_ID, paneId: 1, leafId: LEAF_ID, drivesTabTitle: true }]) + syncParked() + + exitSubscriptions.find((entry) => entry.ptyId === PTY_ID)?.callback(0, { hadPrimary: true }) + + expect(closeTerminalTab).not.toHaveBeenCalled() + expect(startedWatchers[0].dispose).toHaveBeenCalledTimes(1) + expect(getParkedTerminalWatcherTabIds()).toEqual([TAB_ID]) + }) + it('synchronously disposes watchers for the given PTY ids without unparking the tab', () => { capturePanes([ { ptyId: PTY_ID, paneId: 1, leafId: LEAF_ID, drivesTabTitle: true }, @@ -325,6 +387,18 @@ describe('terminal-parked-tab-watchers', () => { expect(getParkedTerminalWatcherTabIds()).toEqual([]) }) + it('consumes parked state when child cleanup observes a removed worktree', () => { + capturePanes([{ ptyId: PTY_ID, paneId: 1, leafId: LEAF_ID, drivesTabTitle: true }]) + syncParked() + + disposeParkedTerminalWatchersForWorktree(WORKTREE_ID, { + consumePreHandlerState: true + }) + + expect(consumePreHandlerPtyState).toHaveBeenCalledWith(PTY_ID) + expect(getParkedTerminalWatcherTabIds()).toEqual([]) + }) + describe('shouldDeferParkedPtyExitTabClose', () => { const closeTab = vi.fn() @@ -397,7 +471,7 @@ describe('terminal-parked-tab-watchers', () => { expect(closeTab).toHaveBeenCalledWith(TAB_ID) }) - it('collapses the exited leaf via the watcher exit sidecar (no host handler while parked)', () => { + it('collapses a dead leaf then closes when the last parked split leaf exits', () => { // Why: hosts' onPtyExit runs from a mounted TerminalPane, so an exit // that lands while parked reaches ONLY the watcher sidecar — it must run // the layout collapse itself or the leaf resurrects on reveal. @@ -418,8 +492,11 @@ describe('terminal-parked-tab-watchers', () => { ptyIdsByLeafId: { [LEAF_ID]: PTY_ID, [SECOND_LEAF_ID]: SECOND_PTY_ID } } - exitSubscriptions.find((entry) => entry.ptyId === SECOND_PTY_ID)?.callback(0) + exitSubscriptions + .find((entry) => entry.ptyId === SECOND_PTY_ID) + ?.callback(0, { hadPrimary: false }) + expect(consumePreHandlerPtyState).toHaveBeenCalledWith(SECOND_PTY_ID) expect(mockStoreState.clearRuntimePaneTitle).toHaveBeenCalledWith(TAB_ID, 2) expect(mockStoreState.setTabLayout).toHaveBeenCalledWith(TAB_ID, { root: { type: 'leaf', leafId: LEAF_ID }, @@ -427,6 +504,11 @@ describe('terminal-parked-tab-watchers', () => { expandedLeafId: null, ptyIdsByLeafId: { [LEAF_ID]: PTY_ID } }) + + exitSubscriptions.find((entry) => entry.ptyId === PTY_ID)?.callback(0, { hadPrimary: false }) + const options = closeTerminalTab.mock.calls[0]?.[1] as CloseTerminalTabOptions + options.onClosed?.() + expect(consumePreHandlerPtyState).toHaveBeenCalledWith(PTY_ID) }) it('does not touch the layout when the last parked watcher exits (tab-level close owns it)', () => { @@ -439,7 +521,7 @@ describe('terminal-parked-tab-watchers', () => { ptyIdsByLeafId: { [LEAF_ID]: PTY_ID } } - exitSubscriptions.find((entry) => entry.ptyId === PTY_ID)?.callback(0) + exitSubscriptions.find((entry) => entry.ptyId === PTY_ID)?.callback(0, { hadPrimary: false }) expect(mockStoreState.setTabLayout).not.toHaveBeenCalled() }) @@ -453,7 +535,7 @@ describe('terminal-parked-tab-watchers', () => { // First leaf dies: deferred, then its exit sidecar drops the watcher. hostOnPtyExit(TAB_ID, PTY_ID) - exitSubscriptions.find((entry) => entry.ptyId === PTY_ID)?.callback(0) + exitSubscriptions.find((entry) => entry.ptyId === PTY_ID)?.callback(0, { hadPrimary: false }) expect(closeTab).not.toHaveBeenCalled() hostOnPtyExit(TAB_ID, SECOND_PTY_ID) diff --git a/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.ts b/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.ts index 162952a68f8..da3c9710717 100644 --- a/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.ts +++ b/src/renderer/src/components/terminal-pane/terminal-parked-tab-watchers.ts @@ -13,9 +13,11 @@ import { isTerminalLeafId } from '../../../../shared/stable-pane-id' import type { TerminalTab } from '../../../../shared/types' import { useAppStore } from '@/store' +import { closeTerminalTab } from '../terminal/terminal-tab-actions' import { collectLeafIdsInOrder } from './terminal-layout-leaf-ids' import { detachTerminalLayoutLeaf } from './terminal-layout-leaf-detach' import { subscribeToPtyExit } from './pty-dispatcher' +import { discardPreHandlerPtyState } from './pty-pre-handler-buffer' import { startParkedTerminalByteWatcher } from './parked-terminal-byte-watcher' import { isSnapshotBackedTerminalPty } from './terminal-hidden-view-parking' import { @@ -29,6 +31,8 @@ import { // split exists only to break the store-slice import cycle. export { captureParkedTerminalPaneCandidates, + disposeAllParkedTerminalWatchers, + disposeRemovedWorktreeParkedTerminalWatchers, disposeParkedTerminalWatchersForPtyIds, disposeParkedTerminalWatchersForWorktree, getParkedTerminalWatcherTabIds, @@ -151,25 +155,47 @@ function startParkedTabWatchers( }) // Why: a PTY that exits while parked has no pane to run exit cleanup; at // minimum its watcher must not outlive it. - const unsubscribeExit = subscribeToPtyExit(ptyId, () => { + const unsubscribeExit = subscribeToPtyExit(ptyId, (_code, { hadPrimary }) => { // Why: while parked this sidecar is the ONLY exit observer — the hosts' // onPtyExit runs from a mounted TerminalPane. Run the observed-exit - // teardown's data half here for a multi-leaf tab, or the dead leaf's - // stale binding reattaches on reveal and the daemon re-creates the - // exited session id as a fresh shell, resurrecting the pane. - if (disposersByPtyId.size > 1) { - useAppStore.getState().clearRuntimePaneTitle(tab.id, pane.paneId) - collapseParkedExitedLeaf(tab.id, ptyId) + // teardown here or a sole tab survives, while a dead split leaf can + // reattach on reveal and resurrect its exited session as a fresh shell. + useAppStore.getState().clearRuntimePaneTitle(tab.id, pane.paneId) + if (hadPrimary) { + // Why: detach intentionally retains the pane transport's primary exit + // owner. It already performs the tab/leaf close; this sidecar only + // retires parked observation so one exit cannot queue two confirms. + disposersByPtyId.get(ptyId)?.() + disposersByPtyId.delete(ptyId) + return } + if (disposersByPtyId.size > 1) { + // Why: this dead split leaf has no future mount. Consume its buffered + // frame and exit, and tombstone duplicate exit notifications. + discardPreHandlerPtyState(ptyId) + collapseParkedExitedLeaf(tab.id, ptyId) + disposersByPtyId.get(ptyId)?.() + disposersByPtyId.delete(ptyId) + return + } + + // Why: the close may wait on pinned-tab confirmation. Dispose side + // effects now but retain the empty registry entry so parking does not + // restart a watcher against the dead PTY while the dialog is pending. disposersByPtyId.get(ptyId)?.() disposersByPtyId.delete(ptyId) - // Why: with the last watcher gone there is nothing left to watch or - // dispose; dropping the entry keeps the registry bounded to parked - // tabs that still hold live PTYs. - const entry = parkedWatchersByTabId.get(tab.id) - if (disposersByPtyId.size === 0 && entry?.disposersByPtyId === disposersByPtyId) { - parkedWatchersByTabId.delete(tab.id) - } + closeTerminalTab(tab.id, { + onClosed: () => { + discardPreHandlerPtyState(ptyId) + const entry = parkedWatchersByTabId.get(tab.id) + if (entry?.disposersByPtyId === disposersByPtyId) { + parkedWatchersByTabId.delete(tab.id) + } + }, + // Why: cancellation keeps the buffered final frame and exit for the + // reveal-mounted pane, matching a pinned terminal that exited mounted. + onCancel: () => {} + }) }) paneIdByPtyId.set(ptyId, pane.paneId) disposersByPtyId.set(ptyId, () => { @@ -207,6 +233,13 @@ export function shouldDeferParkedPtyExitTabClose(tabId: string, ptyId: string): } const remaining = entry.disposersByPtyId.size if (remaining === 0) { + if (paneId !== undefined) { + // Why: an empty entry is the cancelled/pending pinned-close tombstone. + // The reveal-mounted pane owns the buffered exit now, so suppress its + // close once and drop the registry tombstone. + parkedWatchersByTabId.delete(tabId) + return true + } return false } // Why: this runs from the PTY exit handler, before the exit sidecar above @@ -241,6 +274,18 @@ function collapseParkedExitedLeaf(tabId: string, ptyId: string): void { } } +function disposeClosedParkedTabWatchers( + tabId: string, + entry: { paneIdByPtyId: ReadonlyMap } +): void { + // Why: another queued pinned-close request may close the tab before this + // exit request resolves. No pane can drain its retained final frame then. + for (const ptyId of entry.paneIdByPtyId.keys()) { + discardPreHandlerPtyState(ptyId) + } + disposeParkedTabWatchers(tabId) +} + /** * Reconciles watchers for one worktree against its rendered parked set. * Callers run this from an effect keyed on the committed render state, so @@ -259,7 +304,11 @@ export function syncParkedTerminalTabWatchers(args: { if (entry.worktreeId !== args.worktreeId) { continue } - if (!args.parkedTabIds.has(tabId) || !liveTabIds.has(tabId)) { + if (!liveTabIds.has(tabId)) { + disposeClosedParkedTabWatchers(tabId, entry) + continue + } + if (!args.parkedTabIds.has(tabId) && entry.disposersByPtyId.size > 0) { disposeParkedTabWatchers(tabId) } } diff --git a/src/renderer/src/components/terminal-pane/terminal-parked-watcher-registry.test.ts b/src/renderer/src/components/terminal-pane/terminal-parked-watcher-registry.test.ts new file mode 100644 index 00000000000..8af5ca19207 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-parked-watcher-registry.test.ts @@ -0,0 +1,54 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + bufferPreHandlerPtyData, + bufferPreHandlerPtyExit, + clearPreHandlerPtyState, + drainPreHandlerPtyData, + drainPreHandlerPtyExit, + hasPreHandlerPtyExit +} from './pty-pre-handler-buffer' +import { + parkedWatchersByTabId, + pruneParkedTerminalWatchers +} from './terminal-parked-watcher-registry' + +const TAB_ID = 'removed-parked-tab' +const PTY_ID = 'removed-worktree@@parked-pty' + +describe('terminal parked watcher registry removal', () => { + afterEach(() => { + parkedWatchersByTabId.delete(TAB_ID) + clearPreHandlerPtyState(PTY_ID) + }) + + it('consumes retained state and suppresses a delayed exit for a removed worktree', () => { + const dispose = vi.fn() + parkedWatchersByTabId.set(TAB_ID, { + worktreeId: 'removed-worktree', + tabPtyId: PTY_ID, + paneIdByPtyId: new Map([[PTY_ID, 1]]), + disposersByPtyId: new Map([[PTY_ID, dispose]]) + }) + bufferPreHandlerPtyData(PTY_ID, 'final frame') + bufferPreHandlerPtyExit(PTY_ID, 17) + + pruneParkedTerminalWatchers(new Set()) + + expect(dispose).toHaveBeenCalledTimes(1) + expect(parkedWatchersByTabId.has(TAB_ID)).toBe(false) + expect(hasPreHandlerPtyExit(PTY_ID)).toBe(false) + const data = vi.fn() + drainPreHandlerPtyData(PTY_ID, data) + expect(data).not.toHaveBeenCalled() + + // Why: the actual kill exit can arrive after sidecar disposal and prune. + bufferPreHandlerPtyExit(PTY_ID, 18) + bufferPreHandlerPtyData(PTY_ID, 'delayed final frame') + const exit = vi.fn() + const delayedData = vi.fn() + drainPreHandlerPtyExit(PTY_ID, exit) + drainPreHandlerPtyData(PTY_ID, delayedData) + expect(exit).not.toHaveBeenCalled() + expect(delayedData).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/components/terminal-pane/terminal-parked-watcher-registry.ts b/src/renderer/src/components/terminal-pane/terminal-parked-watcher-registry.ts index d6dd76e817a..e9e800991dc 100644 --- a/src/renderer/src/components/terminal-pane/terminal-parked-watcher-registry.ts +++ b/src/renderer/src/components/terminal-pane/terminal-parked-watcher-registry.ts @@ -7,6 +7,7 @@ * mid-evaluation. Keeping the maps and pure disposal here lets the slice * import cycle-free, mirroring how pty-dispatcher exports its handler maps. */ +import { discardPreHandlerPtyState } from './pty-pre-handler-buffer' export type ParkedTerminalPaneCapture = { ptyId: string | null @@ -84,19 +85,55 @@ export function disposeParkedTerminalWatchersForPtyIds(ptyIds: readonly string[] } } -export function disposeParkedTerminalWatchersForWorktree(worktreeId: string): void { +export function disposeParkedTerminalWatchersForWorktree( + worktreeId: string, + options?: { consumePreHandlerState?: boolean } +): void { for (const [tabId, entry] of parkedWatchersByTabId) { if (entry.worktreeId === worktreeId) { - disposeParkedTabWatchers(tabId) + if (options?.consumePreHandlerState) { + disposeRemovedWorktreeParkedTabWatchers(tabId, entry) + } else { + disposeParkedTabWatchers(tabId) + } } } } +export function disposeRemovedWorktreeParkedTerminalWatchers( + worktreeId: string, + authoritativePtyIds: readonly string[] = [] +): void { + for (const ptyId of authoritativePtyIds) { + discardPreHandlerPtyState(ptyId) + } + disposeParkedTerminalWatchersForWorktree(worktreeId, { consumePreHandlerState: true }) +} + +export function disposeAllParkedTerminalWatchers(): void { + for (const tabId of Array.from(parkedWatchersByTabId.keys())) { + disposeParkedTabWatchers(tabId) + } +} + +function disposeRemovedWorktreeParkedTabWatchers( + tabId: string, + entry: ParkedTabWatcherEntry +): void { + // Why: removal unregisters sidecars before PTY kill finishes. Tombstone each + // old PTY now so its delayed final flush/exit cannot refill bounded buffers + // after this worktree loses every future pane consumer. + for (const ptyId of entry.paneIdByPtyId.keys()) { + discardPreHandlerPtyState(ptyId) + } + disposeParkedTabWatchers(tabId) +} + /** Drops watchers and captures for worktrees that no longer exist. */ export function pruneParkedTerminalWatchers(liveWorktreeIds: ReadonlySet): void { for (const [tabId, entry] of parkedWatchersByTabId) { if (!liveWorktreeIds.has(entry.worktreeId)) { - disposeParkedTabWatchers(tabId) + disposeRemovedWorktreeParkedTabWatchers(tabId, entry) } } for (const [tabId, capture] of capturedPanesByTabId) { diff --git a/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.ts b/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.ts index f7303f828de..56172164490 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.ts @@ -53,9 +53,9 @@ export function useTerminalTabColdParking(args: { * mounted for their first xterm fit, mirroring the worktree-level guard. */ shouldMeasureHiddenWorktree: boolean activityTerminalPortals: ActivityTerminalPortalTarget[] - /** Tabs the mount restriction keeps unmounted (activation deferral / - * targeted background mounts) — parked-equivalent for watcher purposes. */ - deferredMountTabIds?: ReadonlySet + /** Tabs cold activation keeps unmounted — parked-equivalent for watcher + * purposes. Targeted background restrictions intentionally stay bounded. */ + activationDeferredMountTabIds?: ReadonlySet | null }): ReadonlySet { const { worktreeId, @@ -65,7 +65,7 @@ export function useTerminalTabColdParking(args: { coldParkTerminalPanes, shouldMeasureHiddenWorktree, activityTerminalPortals, - deferredMountTabIds + activationDeferredMountTabIds } = args const pendingStartupByTabId = useAppStore((state) => state.pendingStartupByTabId) const terminalParkingEnabled = useAppStore( @@ -216,12 +216,11 @@ export function useTerminalTabColdParking(args: { ) { parked.add(terminalTab.id) } - // Why: mount-restricted tabs render no pane regardless of the park - // policy (the layer filters them out before parking is consulted), so - // the watchers must own their side effects. The measure probe never - // mounts them either, hence no shouldMeasureHiddenWorktree gate. + // Why: activation-deferred tabs render no pane regardless of the park + // policy, so watchers must own their side effects immediately. Targeted + // restrictions do not enter this set or add a new eager watcher burst. if ( - deferredMountTabIds?.has(terminalTab.id) && + activationDeferredMountTabIds?.has(terminalTab.id) && !hasActivityTerminalPortal && canWatcherCoverParkedTerminalTab(worktreeId, terminalTab) ) { @@ -234,7 +233,7 @@ export function useTerminalTabColdParking(args: { assignments, coldParkTerminalPanes, coldParkedTerminalTabIds, - deferredMountTabIds, + activationDeferredMountTabIds, isWorktreeActive, shouldMeasureHiddenWorktree, terminalTabs, @@ -250,11 +249,11 @@ export function useTerminalTabColdParking(args: { worktreeId, tabs: terminalTabs, parkedTabIds: parkedTerminalTabIds, - // Why: mount-restricted tabs have no prior pane-owned title slot; pull - // main's title-only snapshot when their watcher starts. - restoreTitleOnStartTabIds: deferredMountTabIds + // Why: activation-deferred tabs have no prior pane-owned title slot; + // pull main's title-only snapshot when their watcher starts. + restoreTitleOnStartTabIds: activationDeferredMountTabIds ?? undefined }) - }, [deferredMountTabIds, parkedTerminalTabIds, terminalTabs, worktreeId]) + }, [activationDeferredMountTabIds, parkedTerminalTabIds, terminalTabs, worktreeId]) useEffect(() => () => disposeParkedTerminalWatchersForWorktree(worktreeId), [worktreeId]) diff --git a/src/renderer/src/components/terminal/background-terminal-worktree-mount.test.ts b/src/renderer/src/components/terminal/background-terminal-worktree-mount.test.ts index 28af7dca43a..2fbf00a5dc4 100644 --- a/src/renderer/src/components/terminal/background-terminal-worktree-mount.test.ts +++ b/src/renderer/src/components/terminal/background-terminal-worktree-mount.test.ts @@ -145,6 +145,26 @@ describe('pruneClosedBackgroundMountTabs', () => { expect(pruneClosedBackgroundMountTabs(restrictions, mounted, {})).toBe(false) expect(mounted).toEqual(new Set(['wt-visited', 'wt-whole'])) }) + + it('keeps an activation mounted when its last allowed tab closes before deferred tabs', () => { + const restrictions = new Map>([['wt-1', new Set(['tab-visible'])]]) + const deferredMountTabIdsByWorktree = new Map>([ + ['wt-1', new Set(['tab-deferred', 'tab-closed'])] + ]) + const mounted = new Set(['wt-1']) + + expect( + pruneClosedBackgroundMountTabs( + restrictions, + mounted, + { 'wt-1': [{ id: 'tab-deferred' }] }, + deferredMountTabIdsByWorktree + ) + ).toBe(true) + expect(restrictions.get('wt-1')).toEqual(new Set()) + expect(deferredMountTabIdsByWorktree.get('wt-1')).toEqual(new Set(['tab-deferred'])) + expect(mounted.has('wt-1')).toBe(true) + }) }) describe('cold activation tab deferral', () => { @@ -153,8 +173,10 @@ describe('cold activation tab deferral', () => { it('mounts everything at once when few tabs would defer', () => { const restrictions = new Map>([['wt-1', new Set(['tab-1'])]]) + const deferredMountTabIdsByWorktree = new Map>() const deferring = planColdActivationTabDeferral({ restrictions, + deferredMountTabIdsByWorktree, worktreeId: 'wt-1', allTabIds: tabIds(COLD_ACTIVATION_TAB_DEFER_THRESHOLD + 1), isTabLive: () => false, @@ -163,12 +185,15 @@ describe('cold activation tab deferral', () => { }) expect(deferring).toBe(false) expect(restrictions.has('wt-1')).toBe(false) + expect(deferredMountTabIdsByWorktree.has('wt-1')).toBe(false) }) it('restricts a cold activation with many tabs to the immediate set', () => { const restrictions = new Map>() + const deferredMountTabIdsByWorktree = new Map>() const deferring = planColdActivationTabDeferral({ restrictions, + deferredMountTabIdsByWorktree, worktreeId: 'wt-1', allTabIds: tabIds(10), isTabLive: () => false, @@ -177,12 +202,17 @@ describe('cold activation tab deferral', () => { }) expect(deferring).toBe(true) expect(restrictions.get('wt-1')).toEqual(new Set(['tab-3'])) + expect(deferredMountTabIdsByWorktree.get('wt-1')).toEqual( + new Set(['tab-1', 'tab-2', 'tab-4', 'tab-5', 'tab-6', 'tab-7', 'tab-8', 'tab-9', 'tab-10']) + ) }) it('keeps live, previously allowed, and non-deferrable tabs mounted', () => { const restrictions = new Map>([['wt-1', new Set(['tab-2'])]]) + const deferredMountTabIdsByWorktree = new Map>() const deferring = planColdActivationTabDeferral({ restrictions, + deferredMountTabIdsByWorktree, worktreeId: 'wt-1', allTabIds: tabIds(12), isTabLive: (tabId) => tabId === 'tab-5', @@ -196,8 +226,10 @@ describe('cold activation tab deferral', () => { it('does not defer when most tabs are already live', () => { const restrictions = new Map>() + const deferredMountTabIdsByWorktree = new Map>() const deferring = planColdActivationTabDeferral({ restrictions, + deferredMountTabIdsByWorktree, worktreeId: 'wt-1', allTabIds: tabIds(10), isTabLive: (tabId) => tabId !== 'tab-10', @@ -211,18 +243,24 @@ describe('cold activation tab deferral', () => { it('reveals newly visible tabs and lifts the restriction once all are revealed', () => { const restrictions = new Map>([['wt-1', new Set(['tab-1'])]]) const allTabIds = tabIds(3) + const deferredMountTabIdsByWorktree = new Map>([ + ['wt-1', new Set(['tab-2', 'tab-3'])] + ]) revealActivationDeferredTabs({ restrictions, + deferredMountTabIdsByWorktree, worktreeId: 'wt-1', allTabIds, immediateTabIds: new Set(['tab-2']) }) expect(restrictions.get('wt-1')).toEqual(new Set(['tab-1', 'tab-2'])) + expect(deferredMountTabIdsByWorktree.get('wt-1')).toEqual(new Set(['tab-3'])) const unchanged = restrictions.get('wt-1') revealActivationDeferredTabs({ restrictions, + deferredMountTabIdsByWorktree, worktreeId: 'wt-1', allTabIds, immediateTabIds: new Set(['tab-2']) @@ -231,22 +269,47 @@ describe('cold activation tab deferral', () => { revealActivationDeferredTabs({ restrictions, + deferredMountTabIdsByWorktree, worktreeId: 'wt-1', allTabIds, immediateTabIds: new Set(['tab-3']) }) expect(restrictions.has('wt-1')).toBe(false) + expect(deferredMountTabIdsByWorktree.has('wt-1')).toBe(false) }) - it('reveal is a no-op for worktrees without a restriction', () => { - const restrictions = new Map>() + it('does not reveal a targeted background restriction as activation deferral', () => { + const restrictions = new Map>([['wt-1', new Set(['tab-1'])]]) + const deferredMountTabIdsByWorktree = new Map>() revealActivationDeferredTabs({ restrictions, + deferredMountTabIdsByWorktree, worktreeId: 'wt-1', allTabIds: tabIds(3), - immediateTabIds: new Set(['tab-1']) + immediateTabIds: new Set(['tab-2']) }) - expect(restrictions.has('wt-1')).toBe(false) + expect(restrictions.get('wt-1')).toEqual(new Set(['tab-1'])) + expect(deferredMountTabIdsByWorktree.has('wt-1')).toBe(false) + }) + + it('hands a targeted wake from activation watcher ownership to its pane', () => { + const restrictions = new Map>([['wt-1', new Set(['tab-1'])]]) + const deferredMountTabIdsByWorktree = new Map>([ + ['wt-1', new Set(['tab-2', 'tab-3'])] + ]) + const mounted = new Set(['wt-1']) + + applyBackgroundMountTabRestriction(restrictions, mounted, 'wt-1', ['tab-2']) + revealActivationDeferredTabs({ + restrictions, + deferredMountTabIdsByWorktree, + worktreeId: 'wt-1', + allTabIds: tabIds(3), + immediateTabIds: new Set(['tab-2']) + }) + + expect(restrictions.get('wt-1')).toEqual(new Set(['tab-1', 'tab-2'])) + expect(deferredMountTabIdsByWorktree.get('wt-1')).toEqual(new Set(['tab-3'])) }) it('collects the tabs a restriction keeps unmounted', () => { diff --git a/src/renderer/src/components/terminal/background-terminal-worktree-mount.ts b/src/renderer/src/components/terminal/background-terminal-worktree-mount.ts index f9486d0d135..aa15f16ec08 100644 --- a/src/renderer/src/components/terminal/background-terminal-worktree-mount.ts +++ b/src/renderer/src/components/terminal/background-terminal-worktree-mount.ts @@ -141,6 +141,24 @@ export function shouldMountBackgroundWorktreeTab( // until first reveal, parked byte watchers own their side effects meanwhile. export const COLD_ACTIVATION_TAB_DEFER_THRESHOLD = 4 +function replaceActivationDeferredMountTabs( + deferredMountTabIdsByWorktree: Map>, + worktreeId: string, + restrictedTabIds: ReadonlySet | null, + allTabIds: readonly string[] +): void { + const next = collectDeferredMountTabIds(restrictedTabIds, allTabIds) + if (next.size === 0) { + deferredMountTabIdsByWorktree.delete(worktreeId) + return + } + const current = deferredMountTabIdsByWorktree.get(worktreeId) + if (current?.size === next.size && Array.from(next).every((tabId) => current.has(tabId))) { + return + } + deferredMountTabIdsByWorktree.set(worktreeId, next) +} + /** * Decides whether activating `worktreeId` should defer mounting its hidden * terminal tabs until each is first revealed. Installs the restriction (tabs @@ -149,6 +167,7 @@ export const COLD_ACTIVATION_TAB_DEFER_THRESHOLD = 4 */ export function planColdActivationTabDeferral(opts: { restrictions: Map> + deferredMountTabIdsByWorktree: Map> worktreeId: string allTabIds: readonly string[] isTabLive: (tabId: string) => boolean @@ -157,7 +176,15 @@ export function planColdActivationTabDeferral(opts: { isTabDeferrable: (tabId: string) => boolean immediateTabIds: ReadonlySet }): boolean { - const { restrictions, worktreeId, allTabIds, isTabLive, isTabDeferrable, immediateTabIds } = opts + const { + restrictions, + deferredMountTabIdsByWorktree, + worktreeId, + allTabIds, + isTabLive, + isTabDeferrable, + immediateTabIds + } = opts const previouslyAllowed = restrictions.get(worktreeId) const initial = new Set() for (const tabId of allTabIds) { @@ -175,9 +202,11 @@ export function planColdActivationTabDeferral(opts: { const deferredCount = allTabIds.length - initial.size if (deferredCount <= COLD_ACTIVATION_TAB_DEFER_THRESHOLD) { restrictions.delete(worktreeId) + deferredMountTabIdsByWorktree.delete(worktreeId) return false } restrictions.set(worktreeId, initial) + replaceActivationDeferredMountTabs(deferredMountTabIdsByWorktree, worktreeId, initial, allTabIds) return true } @@ -189,13 +218,21 @@ export function planColdActivationTabDeferral(opts: { */ export function revealActivationDeferredTabs(opts: { restrictions: Map> + deferredMountTabIdsByWorktree: Map> worktreeId: string allTabIds: readonly string[] immediateTabIds: ReadonlySet }): void { - const { restrictions, worktreeId, allTabIds, immediateTabIds } = opts + const { restrictions, deferredMountTabIdsByWorktree, worktreeId, allTabIds, immediateTabIds } = + opts + // Why: targeted background mounts share the allowed-tab restriction map, + // but only activation deferral may eagerly fan out parked watcher coverage. + if (!deferredMountTabIdsByWorktree.has(worktreeId)) { + return + } const existing = restrictions.get(worktreeId) if (!existing) { + deferredMountTabIdsByWorktree.delete(worktreeId) return } let grew = false @@ -208,11 +245,13 @@ export function revealActivationDeferredTabs(opts: { const next = grew ? new Set([...existing, ...immediateTabIds]) : existing if (allTabIds.length > 0 && allTabIds.every((tabId) => next.has(tabId))) { restrictions.delete(worktreeId) + deferredMountTabIdsByWorktree.delete(worktreeId) return } if (grew) { restrictions.set(worktreeId, next) } + replaceActivationDeferredMountTabs(deferredMountTabIdsByWorktree, worktreeId, next, allTabIds) } /** Tabs a restriction currently keeps unmounted — the set that needs parked @@ -234,25 +273,49 @@ export function collectDeferredMountTabIds( } /** - * Releases targeted mounts after their owning tabs close. Whole-worktree and - * user-visited mounts have no restriction entry and are intentionally retained. + * Releases closed tabs from targeted and activation restrictions. Whole-worktree + * and fully user-visited mounts have no restriction entry and are retained. */ export function pruneClosedBackgroundMountTabs( restrictions: Map>, mountedWorktreeIds: Set, - tabsByWorktree: Record + tabsByWorktree: Record, + deferredMountTabIdsByWorktree?: Map> ): boolean { let changed = false for (const [worktreeId, tabIds] of restrictions) { const liveTabIds = new Set((tabsByWorktree[worktreeId] ?? []).map((tab) => tab.id)) const retained = new Set([...tabIds].filter((tabId) => liveTabIds.has(tabId))) + const deferred = deferredMountTabIdsByWorktree?.get(worktreeId) + const retainedDeferred = deferred + ? new Set([...deferred].filter((tabId) => liveTabIds.has(tabId))) + : null + const deferredChanged = deferred !== undefined && retainedDeferred?.size !== deferred.size + if (deferredChanged) { + changed = true + if (retainedDeferred && retainedDeferred.size > 0) { + deferredMountTabIdsByWorktree?.set(worktreeId, retainedDeferred) + } else { + deferredMountTabIdsByWorktree?.delete(worktreeId) + // Why: once every deferred tab is gone, the remaining live tabs are + // already allowed; release the now-redundant activation restriction. + restrictions.delete(worktreeId) + continue + } + } if (retained.size === tabIds.size) { continue } changed = true if (retained.size === 0) { - restrictions.delete(worktreeId) - mountedWorktreeIds.delete(worktreeId) + // Why: an activation restriction may legitimately have no allowed tabs + // while live deferred tabs remain; keep the active surface mounted. + if (retainedDeferred && retainedDeferred.size > 0) { + restrictions.set(worktreeId, retained) + } else { + restrictions.delete(worktreeId) + mountedWorktreeIds.delete(worktreeId) + } } else { restrictions.set(worktreeId, retained) } diff --git a/src/renderer/src/components/terminal/terminal-tab-actions.test.ts b/src/renderer/src/components/terminal/terminal-tab-actions.test.ts index a070cdc9331..3fa945bef7b 100644 --- a/src/renderer/src/components/terminal/terminal-tab-actions.test.ts +++ b/src/renderer/src/components/terminal/terminal-tab-actions.test.ts @@ -389,6 +389,7 @@ describe('closeTerminalTab', () => { it('closes the pinned tab when the confirmation callback runs', () => { const requestPinnedTabCloseConfirm = vi.fn() const closeUnifiedTab = vi.fn() + const onClosed = vi.fn() getStateMock.mockReturnValue( makePinnedTabState({ confirmClosePinnedTab: true, @@ -397,11 +398,35 @@ describe('closeTerminalTab', () => { }) ) - closeTerminalTab('pinned-entity-1') + closeTerminalTab('pinned-entity-1', { onClosed }) + expect(onClosed).not.toHaveBeenCalled() const { onConfirm } = requestPinnedTabCloseConfirm.mock.calls[0][0] as { onConfirm: () => void } onConfirm() expect(closeUnifiedTab).toHaveBeenCalledWith('unified-pinned-1') + expect(onClosed).toHaveBeenCalledTimes(1) + }) + + it('reports cancellation without finalizing a pinned tab close', () => { + const requestPinnedTabCloseConfirm = vi.fn() + const closeUnifiedTab = vi.fn() + const onClosed = vi.fn() + const onCancel = vi.fn() + getStateMock.mockReturnValue( + makePinnedTabState({ + confirmClosePinnedTab: true, + requestPinnedTabCloseConfirm, + closeUnifiedTab + }) + ) + + closeTerminalTab('pinned-entity-1', { onClosed, onCancel }) + const request = requestPinnedTabCloseConfirm.mock.calls[0][0] as { onCancel?: () => void } + request.onCancel?.() + + expect(closeUnifiedTab).not.toHaveBeenCalled() + expect(onClosed).not.toHaveBeenCalled() + expect(onCancel).toHaveBeenCalledTimes(1) }) it('guards a pinned tab closed by its unified id (workspace overlay path)', () => { diff --git a/src/renderer/src/components/terminal/terminal-tab-actions.ts b/src/renderer/src/components/terminal/terminal-tab-actions.ts index 9991b7648ea..3254f775702 100644 --- a/src/renderer/src/components/terminal/terminal-tab-actions.ts +++ b/src/renderer/src/components/terminal/terminal-tab-actions.ts @@ -149,10 +149,14 @@ export function createNewTerminalTab( state.setTabBarOrder(activeWorktreeId, order) } -export function closeTerminalTab(tabId: string, options?: { force?: boolean }): void { +export function closeTerminalTab( + tabId: string, + options?: { force?: boolean; onClosed?: () => void; onCancel?: () => void } +): void { const state = useAppStore.getState() const target = resolveCloseTerminalTabTarget(state, tabId) if (!target) { + options?.onClosed?.() return } const { worktreeId: owningWorktreeId, terminalTabId } = target @@ -163,7 +167,8 @@ export function closeTerminalTab(tabId: string, options?: { force?: boolean }): guardPinnedTabClose({ isPinned: true, tabLabel: resolvePinnedTabLabel(state, owningWorktreeId, terminalTabId), - onClose: () => closeTerminalTab(tabId, { force: true }) + onClose: () => closeTerminalTab(tabId, { ...options, force: true }), + ...(options?.onCancel ? { onCancel: options.onCancel } : {}) }) return } @@ -193,6 +198,7 @@ export function closeTerminalTab(tabId: string, options?: { force?: boolean }): tabId: hostBackedTabId, environmentId: runtimeEnvironmentId }) + options?.onClosed?.() return } @@ -217,6 +223,7 @@ export function closeTerminalTab(tabId: string, options?: { force?: boolean }): } } } + options?.onClosed?.() return } @@ -230,6 +237,7 @@ export function closeTerminalTab(tabId: string, options?: { force?: boolean }): } closeLocalTerminalTabState(terminalTabId) + options?.onClosed?.() } export function closeOtherTerminalTabs(tabId: string, activeWorktreeId: string | null): void { diff --git a/src/renderer/src/store/slices/pinned-tab-close-confirm.test.ts b/src/renderer/src/store/slices/pinned-tab-close-confirm.test.ts index 50f70d178f0..5a39df64b24 100644 --- a/src/renderer/src/store/slices/pinned-tab-close-confirm.test.ts +++ b/src/renderer/src/store/slices/pinned-tab-close-confirm.test.ts @@ -89,4 +89,66 @@ describe('createPinnedTabCloseConfirmSlice', () => { expect(onCancel).toHaveBeenCalledTimes(1) expect(store.getState().pinnedTabCloseConfirm).toBeNull() }) + + it('queues concurrent requests and advances them in request order', () => { + const now = vi.spyOn(Date, 'now').mockReturnValue(1_000) + const store = makeStore() + const firstConfirm = vi.fn() + const secondConfirm = vi.fn() + store.getState().requestPinnedTabCloseConfirm({ + tabLabel: 'First terminal', + onConfirm: firstConfirm + }) + store.getState().requestPinnedTabCloseConfirm({ + tabLabel: 'Second terminal', + onConfirm: secondConfirm + }) + + expect(store.getState().pinnedTabCloseConfirm?.tabLabel).toBe('First terminal') + store.getState().confirmPinnedTabClose() + expect(firstConfirm).toHaveBeenCalledTimes(1) + expect(secondConfirm).not.toHaveBeenCalled() + expect(store.getState().pinnedTabCloseConfirm?.tabLabel).toBe('Second terminal') + + now.mockReturnValue(1_351) + store.getState().confirmPinnedTabClose() + expect(secondConfirm).toHaveBeenCalledTimes(1) + expect(store.getState().pinnedTabCloseConfirm).toBeNull() + }) + + it('advances the queue when an earlier request is dismissed', () => { + const store = makeStore() + const firstCancel = vi.fn() + store.getState().requestPinnedTabCloseConfirm({ + tabLabel: 'First terminal', + onConfirm: vi.fn(), + onCancel: firstCancel + }) + store.getState().requestPinnedTabCloseConfirm({ + tabLabel: 'Second terminal', + onConfirm: vi.fn() + }) + + store.getState().dismissPinnedTabClose() + + expect(firstCancel).toHaveBeenCalledTimes(1) + expect(store.getState().pinnedTabCloseConfirm?.tabLabel).toBe('Second terminal') + }) + + it('ignores a rapid second action on the newly advanced request', () => { + vi.spyOn(Date, 'now').mockReturnValue(1_000) + const store = makeStore() + const secondConfirm = vi.fn() + store.getState().requestPinnedTabCloseConfirm({ tabLabel: 'First', onConfirm: vi.fn() }) + store.getState().requestPinnedTabCloseConfirm({ + tabLabel: 'Second', + onConfirm: secondConfirm + }) + + store.getState().confirmPinnedTabClose() + store.getState().confirmPinnedTabClose() + + expect(secondConfirm).not.toHaveBeenCalled() + expect(store.getState().pinnedTabCloseConfirm?.tabLabel).toBe('Second') + }) }) diff --git a/src/renderer/src/store/slices/pinned-tab-close-confirm.ts b/src/renderer/src/store/slices/pinned-tab-close-confirm.ts index 58f257298a3..df56122b285 100644 --- a/src/renderer/src/store/slices/pinned-tab-close-confirm.ts +++ b/src/renderer/src/store/slices/pinned-tab-close-confirm.ts @@ -21,29 +21,60 @@ export const createPinnedTabCloseConfirmSlice: StateCreator< [], [], PinnedTabCloseConfirmSlice -> = (set, get) => ({ - pinnedTabCloseConfirm: null, +> = (set, get) => { + const queuedRequests: PinnedTabCloseConfirmRequest[] = [] + let nextRequestActionAllowedAt = 0 + const INTER_REQUEST_ACTION_GUARD_MS = 350 - requestPinnedTabCloseConfirm: (request) => set({ pinnedTabCloseConfirm: request }), - - confirmPinnedTabClose: () => { - const request = get().pinnedTabCloseConfirm - if (!request) { - return - } - // Why: clear before running onConfirm so a re-entrant close path can't see - // the stale request and re-open the dialog. - set({ pinnedTabCloseConfirm: null }) - request.onConfirm() - }, - - dismissPinnedTabClose: () => { - const request = get().pinnedTabCloseConfirm - if (!request) { - return - } - // Why: CLI close requests wait for a response even when the user cancels. - set({ pinnedTabCloseConfirm: null }) - request.onCancel?.() + const advanceRequest = (): boolean => { + const next = queuedRequests.shift() ?? null + set({ pinnedTabCloseConfirm: next }) + return next !== null } -}) + + return { + pinnedTabCloseConfirm: null, + + requestPinnedTabCloseConfirm: (request) => { + if (get().pinnedTabCloseConfirm) { + // Why: autonomous PTY exits can request multiple confirmations in one + // tick. Queue them so replacing the visible request cannot strand the + // first tab's close cleanup and buffered exit state. + queuedRequests.push(request) + return + } + set({ pinnedTabCloseConfirm: request }) + }, + + confirmPinnedTabClose: () => { + if (Date.now() < nextRequestActionAllowedAt) { + return + } + const request = get().pinnedTabCloseConfirm + if (!request) { + return + } + // Why: advance before running onConfirm so a re-entrant close queues + // behind the next real request instead of seeing the stale one. + if (advanceRequest()) { + nextRequestActionAllowedAt = Date.now() + INTER_REQUEST_ACTION_GUARD_MS + } + request.onConfirm() + }, + + dismissPinnedTabClose: () => { + if (Date.now() < nextRequestActionAllowedAt) { + return + } + const request = get().pinnedTabCloseConfirm + if (!request) { + return + } + // Why: CLI close requests wait for a response even when the user cancels. + if (advanceRequest()) { + nextRequestActionAllowedAt = Date.now() + INTER_REQUEST_ACTION_GUARD_MS + } + request.onCancel?.() + } + } +} diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index dfd32303e25..e4110964f82 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -29,6 +29,7 @@ import { findRepoForHost } from './repo-host-identity' import { ensureHooksConfirmed } from '@/lib/ensure-hooks-confirmed' import { cleanupEphemeralVmRuntimesForDeleted } from '@/lib/ephemeral-vm-runtime-cleanup' import { tabHasLivePty } from '@/lib/tab-has-live-pty' +import { disposeRemovedWorktreeParkedTerminalWatchers } from '../../components/terminal-pane/terminal-parked-watcher-registry' import { callRuntimeRpc, getActiveRuntimeTarget, @@ -3261,6 +3262,9 @@ export const createWorktreeSlice: StateCreator const worktreeBeforeRemoval = get() .allWorktrees() .find((entry) => entry.id === worktreeId) + const terminalPtyIdsBeforeRemoval = (get().tabsByWorktree[worktreeId] ?? []).flatMap( + (tab) => get().ptyIdsByTabId[tab.id] ?? [] + ) const currentOwner = resolveWorktreeRemovalHost(get(), worktreeId) if ( currentOwner.ambiguous || @@ -3348,6 +3352,10 @@ export const createWorktreeSlice: StateCreator // batch) rather than only the context menu. requestVirtualizedScrollAnchorRecord('[data-worktree-sidebar]') + // Why: explicit deletion provenance is available here before child + // unmount cleanup; identity migration and recoverable remounts are not + // destructive and must keep their buffered live PTY state. + disposeRemovedWorktreeParkedTerminalWatchers(worktreeId, terminalPtyIdsBeforeRemoval) set((s) => { const next = { ...s.worktreesByRepo } for (const repoId of Object.keys(next)) { diff --git a/tests/e2e/terminal-cold-activation-deferral.spec.ts b/tests/e2e/terminal-cold-activation-deferral.spec.ts index 5e1c2683256..25cedfaedaa 100644 --- a/tests/e2e/terminal-cold-activation-deferral.spec.ts +++ b/tests/e2e/terminal-cold-activation-deferral.spec.ts @@ -1,7 +1,12 @@ import type { Page } from '@stablyai/playwright-test' import { test, expect } from './helpers/orca-app' import { getActiveTabId, waitForActiveWorktree, waitForSessionReady } from './helpers/store' -import { waitForActiveTerminalManager, waitForPaneIdentitySnapshot } from './helpers/terminal' +import { + execInTerminal, + waitForActivePanePtyId, + waitForActiveTerminalManager, + waitForPaneIdentitySnapshot +} from './helpers/terminal' // Why 8: deferral engages only when more than COLD_ACTIVATION_TAB_DEFER_THRESHOLD // (4) tabs would mount cold; 8 tabs with one visible leaves 7 deferred. @@ -47,6 +52,23 @@ async function getMountedTabIds(page: Page, tabIds: readonly string[]): Promise< ) } +async function enableTerminalAccessibilityDom(page: Page, tabId: string): Promise { + await page.evaluate((id) => { + const manager = window.__paneManagers?.get(id) + const pane = manager?.getActivePane?.() ?? manager?.getPanes?.()[0] ?? null + if (!pane) { + throw new Error(`Terminal pane for ${id} is unavailable`) + } + // Why: xterm paints to canvas by default; screen-reader mode mirrors the + // visible buffer into DOM rows so the reveal oracle is user-observable. + pane.terminal.options.screenReaderMode = true + pane.terminal.refresh(0, pane.terminal.rows - 1) + }, tabId) + await expect( + page.locator(`[data-terminal-tab-id=${JSON.stringify(tabId)}] .xterm-accessibility-tree`) + ).toBeAttached({ timeout: 10_000 }) +} + test.describe('cold worktree activation deferral', () => { test('mounts only the visible tab cold and reveals deferred tabs on demand', async ({ orcaPage: page @@ -95,9 +117,27 @@ test.describe('cold worktree activation deferral', () => { const tabIds = (await getTerminalTabSnapshots(page, worktreeId)).map((tab) => tab.id) const lastActiveTabId = await getActiveTabId(page) - // Why: workspace-session persistence is debounced; the reload below must - // find every tab (and its per-tab layout snapshot) already on disk. - await page.waitForTimeout(3_000) + // Why: trigger the production synchronous shutdown flush and verify its + // persisted result instead of sleeping through the debounce. + await page.evaluate(() => window.dispatchEvent(new Event('beforeunload'))) + await expect + .poll( + () => + page.evaluate( + async ({ id, expectedTabIds }) => { + const session = await window.api.session.get() + const persistedTabIds = new Set( + (session.tabsByWorktree[id] ?? []).map((tab) => tab.id) + ) + return expectedTabIds.every( + (tabId) => persistedTabIds.has(tabId) && session.terminalLayoutsByTabId[tabId] + ) + }, + { id: worktreeId, expectedTabIds: tabIds } + ), + { timeout: 10_000, message: 'terminal tabs and layouts were not persisted before reload' } + ) + .toBe(true) // Why reload: a fresh renderer against live daemon sessions is the field // scenario — persisted tabs hydrate cold (unregistered, no pending spawn) @@ -120,19 +160,29 @@ test.describe('cold worktree activation deferral', () => { expect(mountedAfterActivation).toContain(lastActiveTabId) } - // Deferred tabs stay unmounted — there is no timer that mounts them later. - await page.waitForTimeout(2_000) - const mountedAfterSettle = await getMountedTabIds(page, tabIds) - expect(mountedAfterSettle.length).toBe(mountedAfterActivation.length) - // Why: unmounted tabs must not go silent — the parked byte watchers own - // their side effects while deferred. - const watcherCoveredTabIds = await page.evaluate( - () => - ( - window as Window & { __terminalParkingDebug?: { parkedTabIds?: () => string[] } } - ).__terminalParkingDebug?.parkedTabIds?.() ?? [] - ) + // their side effects while deferred. This also deterministically proves + // the post-commit watcher effect flushed; no settle sleep is needed. + let watcherCoveredTabIds: string[] = [] + await expect + .poll( + async () => { + watcherCoveredTabIds = await page.evaluate( + () => + ( + window as Window & { __terminalParkingDebug?: { parkedTabIds?: () => string[] } } + ).__terminalParkingDebug?.parkedTabIds?.() ?? [] + ) + const mounted = await getMountedTabIds(page, tabIds) + return tabIds.every( + (tabId) => mounted.includes(tabId) || watcherCoveredTabIds.includes(tabId) + ) + }, + { timeout: 10_000, message: 'deferred tabs did not receive watcher coverage' } + ) + .toBe(true) + const mountedAfterSettle = await getMountedTabIds(page, tabIds) + expect(mountedAfterSettle).toEqual(mountedAfterActivation) for (const tabId of tabIds) { if (!mountedAfterSettle.includes(tabId)) { expect(watcherCoveredTabIds).toContain(tabId) @@ -142,16 +192,43 @@ test.describe('cold worktree activation deferral', () => { // Revealing a deferred tab mounts it on demand. const deferredTabId = tabIds.find((tabId) => !mountedAfterSettle.includes(tabId)) expect(deferredTabId).toBeDefined() + if (!deferredTabId) { + throw new Error('cold activation left no deferred tab to reveal') + } await page.evaluate((tabId) => { const state = window.__store?.getState() state?.setActiveTab(tabId) state?.setActiveTabType('terminal') - }, deferredTabId as string) + }, deferredTabId) await expect - .poll(async () => (await getMountedTabIds(page, [deferredTabId as string])).length, { + .poll(async () => (await getMountedTabIds(page, [deferredTabId])).length, { timeout: 15_000, message: 'revealed deferred tab did not mount' }) .toBe(1) + + await expect + .poll( + () => + page.evaluate((tabId) => { + const parkedTabIds = + ( + window as Window & { __terminalParkingDebug?: { parkedTabIds?: () => string[] } } + ).__terminalParkingDebug?.parkedTabIds?.() ?? [] + return !parkedTabIds.includes(tabId) + }, deferredTabId), + { timeout: 10_000, message: 'revealed tab retained its parked watcher' } + ) + .toBe(true) + + const revealedPtyId = await waitForActivePanePtyId(page, 15_000) + await enableTerminalAccessibilityDom(page, deferredTabId) + const outputMarker = `COLD_DEFER_REVEAL_OK_${Date.now()}` + await execInTerminal(page, revealedPtyId, `echo ${outputMarker}`) + await expect( + page.locator( + `[data-terminal-tab-id=${JSON.stringify(deferredTabId)}] .xterm-accessibility-tree` + ) + ).toContainText(outputMarker, { timeout: 15_000 }) }) })