From 6a07390db4df37b1ea27440618b1c206fb383687 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 23 Jul 2026 16:40:09 -0700 Subject: [PATCH] fix(worktree): stop terminal removal fence error flashing on delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a worktree kills its PTYs for the filesystem teardown, then runs git worktree remove (~1s). During that window a doomed pane races a fresh respawn that main correctly fences with TerminalRemovalInProgressError, but the renderer surfaced that internal fence verbatim as a pane error banner until the worktree unmounted. - startFreshSpawn: skip the respawn when the pane's own worktree is being deleted (isDeleting) — no shell should spawn into a directory being removed and the pane is about to unmount. - reportError: swallow the removal fence at the single pane-error sink so it never reaches the banner, covering the parent-removal-fences-child case the own-worktree skip cannot see. - Share the fence messages between main (thrown) and renderer (recognized) via worktree-removal-fence-error.ts so the thrown text and predicate can't drift. --- src/main/ipc/watcher-removal-gate.test.ts | 31 +++++++ src/main/ipc/watcher-removal-gate.ts | 8 +- .../terminal-pane/pty-connection.test.ts | 82 +++++++++++++++++++ .../terminal-pane/pty-connection.ts | 16 ++++ .../worktree-removal-fence-error.test.ts | 25 ++++++ src/shared/worktree-removal-fence-error.ts | 18 ++++ 6 files changed, 178 insertions(+), 2 deletions(-) create mode 100644 src/shared/worktree-removal-fence-error.test.ts create mode 100644 src/shared/worktree-removal-fence-error.ts diff --git a/src/main/ipc/watcher-removal-gate.test.ts b/src/main/ipc/watcher-removal-gate.test.ts index 37eed327379..520bdd55dac 100644 --- a/src/main/ipc/watcher-removal-gate.test.ts +++ b/src/main/ipc/watcher-removal-gate.test.ts @@ -6,6 +6,7 @@ import { TerminalRemovalInProgressError, WatcherRemovalInProgressError } from './watcher-removal-gate' +import { isWorktreeRemovalFenceError } from '../../shared/worktree-removal-fence-error' describe('watcher removal gate', () => { it('waits for an existing install and rejects later equivalent-path installs', async () => { @@ -106,4 +107,34 @@ describe('watcher removal gate', () => { finishInstall() removal.release() }) + + // Why: the renderer swallows this fence via isWorktreeRemovalFenceError so a + // doomed pane never shows the raw error. That only holds if the thrown message + // still matches the shared predicate — pin the cross-module contract here. + it('throws fence errors the renderer recognizes as benign removal fences', async () => { + const removal = acquireWatcherRemovalGate('/repo') + await removal.ready + + const terminalError = (() => { + try { + beginTerminalInstall('/repo') + } catch (error) { + return error as Error + } + throw new Error('expected terminal install to be fenced') + })() + const watcherError = (() => { + try { + beginWatcherInstall('/repo') + } catch (error) { + return error as Error + } + throw new Error('expected watcher install to be fenced') + })() + + expect(isWorktreeRemovalFenceError(terminalError.message)).toBe(true) + expect(isWorktreeRemovalFenceError(watcherError.message)).toBe(true) + + removal.release() + }) }) diff --git a/src/main/ipc/watcher-removal-gate.ts b/src/main/ipc/watcher-removal-gate.ts index 5df7b7e098a..4ae9c2d3970 100644 --- a/src/main/ipc/watcher-removal-gate.ts +++ b/src/main/ipc/watcher-removal-gate.ts @@ -2,6 +2,10 @@ import { isPathInsideOrEqual, normalizeRuntimePathForComparison } from '../../shared/cross-platform-path' +import { + TERMINAL_REMOVAL_IN_PROGRESS_MESSAGE, + WATCHER_REMOVAL_IN_PROGRESS_MESSAGE +} from '../../shared/worktree-removal-fence-error' type WatcherRemovalGateState = { connectionId: string | null @@ -20,7 +24,7 @@ export class WatcherRemovalInProgressError extends Error { readonly code = 'watcher_removal_in_progress' constructor() { - super('File watcher cannot start while the worktree is being removed') + super(WATCHER_REMOVAL_IN_PROGRESS_MESSAGE) this.name = 'WatcherRemovalInProgressError' } } @@ -29,7 +33,7 @@ export class TerminalRemovalInProgressError extends Error { readonly code = 'terminal_removal_in_progress' constructor() { - super('Terminal cannot start while the worktree is being removed') + super(TERMINAL_REMOVAL_IN_PROGRESS_MESSAGE) this.name = 'TerminalRemovalInProgressError' } } 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 7374e99e551..eee9ca1b125 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -121,6 +121,7 @@ type StoreState = { ptyIdsByTabId?: Record terminalLayoutsByTabId?: Record unreadTerminalTabs?: Record + deleteStateByWorktreeId?: Record worktreesByRepo: Record< string, { @@ -783,6 +784,7 @@ describe('connectPanePty', () => { } }, unreadTerminalTabs: {}, + deleteStateByWorktreeId: {}, worktreesByRepo: { repo1: [{ id: 'wt-1', repoId: 'repo1', path: '/tmp/wt-1', displayName: 'feat/notis' }] }, @@ -1024,6 +1026,86 @@ describe('connectPanePty', () => { expect(staleTracker.flags).toBe(0) }) + // Why: deleting a worktree kills its PTYs for the filesystem teardown; the + // renderer must not race a doomed respawn into a directory main is deleting + // (main fences it with TerminalRemovalInProgressError and the pane is about to + // unmount). See docs — bad UI was the raw fence error flashing on the tab. + it('skips a fresh spawn while the pane worktree is being deleted', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport() + transportFactoryQueue.push(transport) + mockStoreState = { + ...mockStoreState, + deleteStateByWorktreeId: { 'wt-1': { isDeleting: true, phase: 'deleting' } } + } + // Why: a unique tab id keeps this pane's key clear of other tests' pendingSpawnByPaneKey entries so the connect deterministically fresh-spawns. + const deps = createDeps({ tabId: 'tab-removal-skip-spawn' }) + + connectPanePty(createPane(1) as never, createManager(1) as never, deps as never) + await flushAsyncTicks() + + expect(transport.connect).not.toHaveBeenCalled() + expect(deps.onPtyErrorRef.current).not.toHaveBeenCalled() + }) + + it('fresh-spawns normally when the pane worktree is not being deleted', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport() + transportFactoryQueue.push(transport) + // Why: unique tab id → deterministic fresh spawn (mirrors the skip test's control). + const deps = createDeps({ tabId: 'tab-removal-control-spawn' }) + + connectPanePty(createPane(1) as never, createManager(1) as never, deps as never) + await flushAsyncTicks() + + expect(transport.connect).toHaveBeenCalled() + }) + + // Why: a doomed pane (or a child pane whose parent worktree is being removed, + // which startFreshSpawn's own-worktree skip cannot see) can still race a spawn + // that main fences. reportError must swallow that fence so the tab never flashes + // the raw "Terminal cannot start while the worktree is being removed" banner. + it('swallows a worktree-removal fence error instead of surfacing it', async () => { + const { connectPanePty } = await import('./pty-connection') + const { TERMINAL_REMOVAL_IN_PROGRESS_MESSAGE } = + await import('../../../../shared/worktree-removal-fence-error') + const transport = createMockTransport() + const capturedOnError: { current: ((message: string) => void) | null } = { current: null } + transport.connect.mockImplementation(async ({ callbacks }: { callbacks: ConnectCallbacks }) => { + capturedOnError.current = callbacks.onError ?? null + return 'pty-1' + }) + transportFactoryQueue.push(transport) + const deps = createDeps({ tabId: 'tab-fence-swallow' }) + + connectPanePty(createPane(1) as never, createManager(1) as never, deps as never) + await flushAsyncTicks() + + // Electron wraps the rejected ipcMain error with its own prefix; still swallowed. + capturedOnError.current?.( + `Error invoking remote method 'pty:spawn': Error: ${TERMINAL_REMOVAL_IN_PROGRESS_MESSAGE}` + ) + expect(deps.onPtyErrorRef.current).not.toHaveBeenCalled() + }) + + it('still surfaces non-fence spawn errors through the pane error sink', async () => { + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport() + const capturedOnError: { current: ((message: string) => void) | null } = { current: null } + transport.connect.mockImplementation(async ({ callbacks }: { callbacks: ConnectCallbacks }) => { + capturedOnError.current = callbacks.onError ?? null + return 'pty-1' + }) + transportFactoryQueue.push(transport) + const deps = createDeps({ tabId: 'tab-real-error-surface' }) + + connectPanePty(createPane(1) as never, createManager(1) as never, deps as never) + await flushAsyncTicks() + + capturedOnError.current?.('shell exited with code 1') + expect(deps.onPtyErrorRef.current).toHaveBeenCalledWith(1, 'shell exited with code 1') + }) + it('threads the resolved local project runtime into IPC terminal transport options', async () => { const { connectPanePty } = await import('./pty-connection') const transport = createMockTransport() diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index 40785b1cca7..b56b6126a25 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -13,6 +13,7 @@ import { parseWorkspaceKey } from '../../../../shared/workspace-scope' import { TerminalKittyKeyboardModeTracker } from '../../../../shared/terminal-kitty-keyboard-mode-tracker' import { isRuntimeOwnedSshTargetId } from '../../../../shared/execution-host' import { createTerminalZeroDimensionsMessage } from '../../../../shared/terminal-zero-dimensions-diagnostic' +import { isWorktreeRemovalFenceError } from '../../../../shared/worktree-removal-fence-error' import { parseTerminalOscColorQuery } from '../../../../shared/terminal-osc-color-reply' import { HIDDEN_STARTUP_RENDERER_QUERY_PENDING_CHARS, @@ -4203,6 +4204,14 @@ export function connectPanePty( if (disposed) { return } + if (isWorktreeRemovalFenceError(message)) { + // Why: main fences a spawn/reattach whose worktree (or an overlapping + // parent/child root) is being deleted. That is expected teardown, not a + // user-facing failure — the pane unmounts once removal completes, so never + // surface the raw fence error. Covers the parent-removal-fences-child case + // that startFreshSpawn's own-worktree isDeleting skip cannot see. + return + } deps.onPtyErrorRef?.current?.(pane.id, message) } @@ -4740,6 +4749,13 @@ export function connectPanePty( startupOverride?: PendingStartupCommand | null, options: FreshSpawnOptions = {} ): Promise => { + if (useAppStore.getState().deleteStateByWorktreeId?.[deps.worktreeId]?.isDeleting) { + // Why: the worktree is being deleted; its PTYs were just killed for the + // filesystem teardown. A fresh shell must not spawn into a directory the + // removal is about to delete (main fences it anyway), and the pane is + // about to unmount — so skip the doomed respawn instead of racing it. + return Promise.resolve(null) + } clearPaneMode2031State() clearHiddenOutputRestoreState() // Why: a canceled old replay clear can preserve xterm's native diff --git a/src/shared/worktree-removal-fence-error.test.ts b/src/shared/worktree-removal-fence-error.test.ts new file mode 100644 index 00000000000..ce9e3d76bb7 --- /dev/null +++ b/src/shared/worktree-removal-fence-error.test.ts @@ -0,0 +1,25 @@ +import { describe, expect, it } from 'vitest' +import { + TERMINAL_REMOVAL_IN_PROGRESS_MESSAGE, + WATCHER_REMOVAL_IN_PROGRESS_MESSAGE, + isWorktreeRemovalFenceError +} from './worktree-removal-fence-error' + +describe('isWorktreeRemovalFenceError', () => { + it('recognizes the raw terminal and watcher fence messages', () => { + expect(isWorktreeRemovalFenceError(TERMINAL_REMOVAL_IN_PROGRESS_MESSAGE)).toBe(true) + expect(isWorktreeRemovalFenceError(WATCHER_REMOVAL_IN_PROGRESS_MESSAGE)).toBe(true) + }) + + it('recognizes the message after Electron IPC prefixes the reject', () => { + // Electron wraps a rejected ipcMain.handle error with its own prefix. + const wrapped = `Error invoking remote method 'pty:spawn': Error: ${TERMINAL_REMOVAL_IN_PROGRESS_MESSAGE}` + expect(isWorktreeRemovalFenceError(wrapped)).toBe(true) + }) + + it('does not match unrelated terminal errors', () => { + expect(isWorktreeRemovalFenceError('Failed to save terminal session state')).toBe(false) + expect(isWorktreeRemovalFenceError('shell exited with code 1')).toBe(false) + expect(isWorktreeRemovalFenceError('')).toBe(false) + }) +}) diff --git a/src/shared/worktree-removal-fence-error.ts b/src/shared/worktree-removal-fence-error.ts new file mode 100644 index 00000000000..0b0c432652c --- /dev/null +++ b/src/shared/worktree-removal-fence-error.ts @@ -0,0 +1,18 @@ +// Shared between main (which throws these at the PTY/watcher install fence while +// a worktree is being removed) and the renderer (which recognizes them so a +// doomed pane never surfaces the fence as a user-facing terminal error). + +export const TERMINAL_REMOVAL_IN_PROGRESS_MESSAGE = + 'Terminal cannot start while the worktree is being removed' + +export const WATCHER_REMOVAL_IN_PROGRESS_MESSAGE = + 'File watcher cannot start while the worktree is being removed' + +// Why: both fence messages end with this tail. Matching the tail catches the +// terminal and watcher variants even after Electron IPC prefixes the rejected +// error with its own "Error invoking remote method ..." text. +const REMOVAL_IN_PROGRESS_FENCE_TAIL = 'cannot start while the worktree is being removed' + +export function isWorktreeRemovalFenceError(message: string): boolean { + return message.includes(REMOVAL_IN_PROGRESS_FENCE_TAIL) +}