diff --git a/src/main/editor-launcher-name.ts b/src/main/editor-launcher-name.ts new file mode 100644 index 00000000000..b88511dfea3 --- /dev/null +++ b/src/main/editor-launcher-name.ts @@ -0,0 +1,44 @@ +import { basename, win32 } from 'node:path' + +export function stripMatchingQuotes(value: string): string { + const trimmed = value.trim() + const quote = trimmed[0] + if ((quote === '"' || quote === "'") && trimmed.endsWith(quote)) { + return trimmed.slice(1, -1) + } + return trimmed +} + +export function hasMatchingOuterQuotes(value: string): boolean { + const trimmed = value.trim() + const quote = trimmed[0] + return (quote === '"' || quote === "'") && trimmed.endsWith(quote) +} + +function getLeadingShellCommandToken(command: string): string { + const trimmed = command.trim() + const quote = trimmed[0] + if (quote === '"' || quote === "'") { + const closingIndex = trimmed.indexOf(quote, 1) + if (closingIndex > 0) { + return trimmed.slice(1, closingIndex) + } + } + return trimmed.split(/\s+/, 1)[0] ?? '' +} + +/** Lowercased launcher name without directory or Windows executable extension. */ +export function getLauncherBaseName( + command: string, + options: { shellCommand?: boolean } = {} +): string { + const normalized = options.shellCommand + ? getLeadingShellCommandToken(command) + : stripMatchingQuotes(command) + const name = normalized.includes('\\') ? win32.basename(normalized) : basename(normalized) + return name.replace(/\.(?:cmd|exe|bat)$/i, '').toLowerCase() +} + +export function isWindowsBatchLauncher(command: string): boolean { + return /\.(?:cmd|bat)$/i.test(stripMatchingQuotes(command)) +} diff --git a/src/main/external-editor-launch.test.ts b/src/main/external-editor-launch.test.ts index 5422267f6e1..638e1ff02f7 100644 --- a/src/main/external-editor-launch.test.ts +++ b/src/main/external-editor-launch.test.ts @@ -32,6 +32,168 @@ describe('resolveExternalEditorLaunchSpec', () => { }) }) + it('prefers the JetBrains *64.exe colocated with the resolved idea.cmd on Windows', () => { + const installBin = 'C:\\Program Files\\JetBrains\\IntelliJ IDEA\\bin' + resolveCliCommandMock.mockImplementation((command: string) => + command === 'idea' ? `${installBin}\\idea.cmd` : command + ) + + expect( + resolveExternalEditorLaunchSpec('idea', 'C:\\workspaces\\orca', { + platform: 'win32', + fileExists: (candidate) => candidate === `${installBin}\\idea64.exe` + }) + ).toEqual({ + kind: 'executable', + hideWindowsConsole: true, + spawnCmd: `${installBin}\\idea64.exe`, + spawnArgs: ['C:\\workspaces\\orca'] + }) + // Why: a bare PATH `idea64` may belong to a different, stale install. + expect(resolveCliCommandMock).toHaveBeenCalledWith('idea', { platform: 'win32' }) + expect(resolveCliCommandMock).not.toHaveBeenCalledWith('idea64', expect.anything()) + }) + + it('prefers a colocated .exe over an idea64.cmd shim when the user names idea64', () => { + const installBin = 'C:\\Program Files\\JetBrains\\GoLand\\bin' + resolveCliCommandMock.mockImplementation((command: string) => + command === 'goland64' ? `${installBin}\\goland64.cmd` : command + ) + + expect( + resolveExternalEditorLaunchSpec('goland64', 'C:\\workspaces\\orca', { + platform: 'win32', + fileExists: (candidate) => candidate === `${installBin}\\goland64.exe` + }).spawnCmd + ).toBe(`${installBin}\\goland64.exe`) + }) + + it('keeps the Toolbox idea.cmd shim and detaches it when no GUI exe sits beside it', () => { + const toolboxShim = 'C:\\Users\\me\\AppData\\Local\\JetBrains\\Toolbox\\scripts\\idea.cmd' + resolveCliCommandMock.mockImplementation((command: string) => + command === 'idea' ? toolboxShim : command + ) + + expect( + resolveExternalEditorLaunchSpec('idea', 'C:\\workspaces\\orca', { + platform: 'win32', + fileExists: () => false + }) + ).toEqual({ + kind: 'executable', + hideWindowsConsole: true, + detachedGui: true, + spawnCmd: toolboxShim, + spawnArgs: ['C:\\workspaces\\orca'] + }) + }) + + it('detaches a directly configured JetBrains shim path when no GUI exe is beside it', () => { + expect( + resolveExternalEditorLaunchSpec('C:\\Tools\\WebStorm\\bin\\webstorm.bat', 'C:\\ws', { + platform: 'win32', + fileExists: () => false + }) + ).toEqual({ + kind: 'executable', + hideWindowsConsole: true, + detachedGui: true, + spawnCmd: 'C:\\Tools\\WebStorm\\bin\\webstorm.bat', + spawnArgs: ['C:\\ws'] + }) + }) + + it('upgrades a direct JetBrains .cmd path to the colocated *64.exe', () => { + const installBin = 'C:\\Program Files\\JetBrains\\WebStorm\\bin' + expect( + resolveExternalEditorLaunchSpec(`${installBin}\\webstorm.cmd`, 'C:\\ws', { + platform: 'win32', + fileExists: (candidate) => candidate === `${installBin}\\webstorm64.exe` + }) + ).toEqual({ + kind: 'executable', + hideWindowsConsole: true, + spawnCmd: `${installBin}\\webstorm64.exe`, + spawnArgs: ['C:\\ws'] + }) + }) + + it('upgrades a short-name console idea.exe stub to the colocated idea64.exe', () => { + const installBin = 'C:\\Program Files\\JetBrains\\IntelliJ IDEA\\bin' + expect( + resolveExternalEditorLaunchSpec(`${installBin}\\idea.exe`, 'C:\\ws', { + platform: 'win32', + fileExists: (candidate) => candidate === `${installBin}\\idea64.exe` + }) + ).toEqual({ + kind: 'executable', + hideWindowsConsole: true, + spawnCmd: `${installBin}\\idea64.exe`, + spawnArgs: ['C:\\ws'] + }) + }) + + it('leaves a direct idea64.exe path unchanged even when a sibling exists', () => { + const guiExe = 'C:\\Program Files\\JetBrains\\IntelliJ IDEA\\bin\\idea64.exe' + expect( + resolveExternalEditorLaunchSpec(guiExe, 'C:\\ws', { + platform: 'win32', + fileExists: () => true + }) + ).toEqual({ + kind: 'executable', + hideWindowsConsole: true, + spawnCmd: guiExe, + spawnArgs: ['C:\\ws'] + }) + }) + + it('does not rewrite non-JetBrains .exe launchers via colocation', () => { + const codeExe = 'C:\\Program Files\\Microsoft VS Code\\Code.exe' + expect( + resolveExternalEditorLaunchSpec(codeExe, 'C:\\ws', { + platform: 'win32', + fileExists: () => true + }).spawnCmd + ).toBe(codeExe) + }) + + it('keeps idea.exe when no colocated idea64.exe exists', () => { + const consoleExe = 'C:\\Program Files\\JetBrains\\IntelliJ IDEA\\bin\\idea.exe' + expect( + resolveExternalEditorLaunchSpec(consoleExe, 'C:\\ws', { + platform: 'win32', + fileExists: () => false + }) + ).toEqual({ + kind: 'executable', + hideWindowsConsole: true, + spawnCmd: consoleExe, + spawnArgs: ['C:\\ws'] + }) + }) + + // Why: `start` re-parses argv, so only JetBrains shims may take that path. + it.each([ + ['code', 'C:\\Tools\\code.cmd', ['C:\\workspaces\\orca']], + [ + 'cursor', + 'C:\\Users\\me\\AppData\\Local\\Programs\\cursor\\bin\\cursor.cmd', + ['--new-window', 'C:\\workspaces\\orca'] + ] + ])('does not detach the %s batch shim through start', (command, resolvedCommand, spawnArgs) => { + resolveCliCommandMock.mockImplementation(() => resolvedCommand) + + expect( + resolveExternalEditorLaunchSpec(command, 'C:\\workspaces\\orca', { platform: 'win32' }) + ).toEqual({ + kind: 'executable', + hideWindowsConsole: true, + spawnCmd: resolvedCommand, + spawnArgs + }) + }) + it('appends escaped paths to compound macOS open commands', () => { expect( resolveExternalEditorLaunchSpec('open -a "Typora"', "/tmp/note's.md", { @@ -85,6 +247,19 @@ describe('resolveExternalEditorLaunchSpec', () => { }) }) + it('runs GUI compound Windows commands verbatim instead of re-parsing them under start', () => { + expect( + resolveExternalEditorLaunchSpec('code --reuse-window', 'C:\\workspaces\\orca', { + platform: 'win32' + }) + ).toEqual({ + kind: 'shell', + hideWindowsConsole: true, + spawnCmd: getCmdExePath(), + spawnArgs: ['/d', '/s', '/c', 'code --reuse-window C:\\workspaces\\orca'] + }) + }) + it('quotes Windows paths with spaces in compound commands', () => { expect( resolveExternalEditorLaunchSpec('start "" notepad', 'C:\\my notes.md', { platform: 'win32' }) @@ -245,7 +420,7 @@ describe('resolveExternalEditorLaunchSpec', () => { ).toEqual([pathValue]) }) - it('does not rewrite compound VS Code commands', () => { + it('does not rewrite compound VS Code commands into WSL remote args', () => { const pathValue = '\\\\wsl.localhost\\Ubuntu\\home\\ada\\project' expect( diff --git a/src/main/external-editor-launch.ts b/src/main/external-editor-launch.ts index a0283b18f46..6fa0e867508 100644 --- a/src/main/external-editor-launch.ts +++ b/src/main/external-editor-launch.ts @@ -1,26 +1,46 @@ +import { spawn } from 'node:child_process' import { existsSync } from 'node:fs' -import { basename, posix, win32 } from 'node:path' +import { posix, win32 } from 'node:path' import { parseWslUncPath } from '../shared/wsl-paths' import { isVsCodeLauncherExecutable } from '../shared/vscode-remote-ssh-launcher' import { resolveCliCommand } from './codex-cli/command' -import { getCmdExePath } from './win32-utils' +import { + getLauncherBaseName, + hasMatchingOuterQuotes, + stripMatchingQuotes +} from './editor-launcher-name' +import { + isJetBrainsConsoleShim, + resolveColocatedJetBrainsGuiExecutable +} from './jetbrains-windows-gui-launchers' +import { getCmdExePath, getSpawnArgsForWindows } from './win32-utils' export const EXTERNAL_EDITOR_CLI_COMMAND = 'code' const WINDOWS_CONSOLE_EDITORS = new Set(['nvim', 'vim']) +export type ExternalEditorExecutableLaunchSpec = { + kind: 'executable' + hideWindowsConsole: boolean + /** + * Set only for Windows batch shims that would otherwise leave a Command + * Prompt behind; routes the spawn through `start` with an empty title and + * `/B`. Left unset elsewhere because `start` re-parses quoted argv. + */ + detachedGui?: boolean + spawnCmd: string + spawnArgs: string[] +} + +export type ExternalEditorShellLaunchSpec = { + kind: 'shell' + hideWindowsConsole: boolean + spawnCmd: string + spawnArgs: string[] +} + export type ExternalEditorLaunchSpec = - | { - kind: 'executable' - hideWindowsConsole: boolean - spawnCmd: string - spawnArgs: string[] - } - | { - kind: 'shell' - hideWindowsConsole: boolean - spawnCmd: string - spawnArgs: string[] - } + | ExternalEditorExecutableLaunchSpec + | ExternalEditorShellLaunchSpec function escapePosixPathForShell(pathValue: string): string { if (/^[a-zA-Z0-9_./@:-]+$/.test(pathValue)) { @@ -39,41 +59,6 @@ function escapePathForShell(pathValue: string, platform: NodeJS.Platform): strin : escapePosixPathForShell(pathValue) } -function getLauncherBaseName(command: string, options: { shellCommand?: boolean } = {}): string { - const normalized = options.shellCommand - ? getLeadingShellCommandToken(command) - : stripMatchingQuotes(command) - const name = normalized.includes('\\') ? win32.basename(normalized) : basename(normalized) - return name.replace(/\.(?:cmd|exe|bat)$/i, '').toLowerCase() -} - -function getLeadingShellCommandToken(command: string): string { - const trimmed = command.trim() - const quote = trimmed[0] - if (quote === '"' || quote === "'") { - const closingIndex = trimmed.indexOf(quote, 1) - if (closingIndex > 0) { - return trimmed.slice(1, closingIndex) - } - } - return trimmed.split(/\s+/, 1)[0] ?? '' -} - -function stripMatchingQuotes(value: string): string { - const trimmed = value.trim() - const quote = trimmed[0] - if ((quote === '"' || quote === "'") && trimmed.endsWith(quote)) { - return trimmed.slice(1, -1) - } - return trimmed -} - -function hasMatchingOuterQuotes(value: string): boolean { - const trimmed = value.trim() - const quote = trimmed[0] - return (quote === '"' || quote === "'") && trimmed.endsWith(quote) -} - function isWindowsExecutablePath(command: string): boolean { return win32.isAbsolute(command) && /\.(?:cmd|exe|bat|com)$/i.test(command) } @@ -133,6 +118,46 @@ function isCompoundShellCommand(command: string): boolean { return /\s/.test(command) } +function preferJetBrainsGuiExecutable( + editorCommand: string, + platform: NodeJS.Platform, + fileExists: (path: string) => boolean +): string { + if (platform !== 'win32') { + return editorCommand + } + return resolveColocatedJetBrainsGuiExecutable(editorCommand, fileExists) ?? editorCommand +} + +function resolveSimpleEditorCommand( + command: string, + platform: NodeJS.Platform, + fileExists: (path: string) => boolean +): string { + return preferJetBrainsGuiExecutable( + resolveCliCommand(command, { platform }), + platform, + fileExists + ) +} + +function buildExecutableLaunchSpec( + editorCommand: string, + pathValue: string, + platform: NodeJS.Platform +): ExternalEditorExecutableLaunchSpec { + const spec: ExternalEditorExecutableLaunchSpec = { + kind: 'executable', + hideWindowsConsole: !shouldShowWindowsConsole(editorCommand, platform), + spawnCmd: editorCommand, + spawnArgs: buildExecutableArgs(editorCommand, pathValue, platform) + } + if (spec.hideWindowsConsole && isJetBrainsConsoleShim(editorCommand, platform)) { + spec.detachedGui = true + } + return spec +} + function buildShellLaunchSpec( command: string, pathValue: string, @@ -140,6 +165,11 @@ function buildShellLaunchSpec( ): ExternalEditorLaunchSpec { const shellCommand = `${command} ${escapePathForShell(pathValue, platform)}` if (platform === 'win32') { + // Why: no `start` wrap here. `start` re-parses the line — it swallows the + // first quoted operand as a window title and mangles remote paths with + // spaces — and on a batch target it leaves a resident nested `cmd /K`. + // User-authored compound commands run verbatim; users who want a detached + // launch already write `start` themselves. return { kind: 'shell', hideWindowsConsole: !shouldShowWindowsConsole(command, platform, { shellCommand: true }), @@ -165,26 +195,24 @@ export function resolveExternalEditorLaunchSpec( const trimmed = command?.trim() || EXTERNAL_EDITOR_CLI_COMMAND if (isDirectExecutablePath(trimmed, platform, fileExists)) { - const editorCommand = stripMatchingQuotes(trimmed) - return { - kind: 'executable', - hideWindowsConsole: !shouldShowWindowsConsole(editorCommand, platform), - spawnCmd: editorCommand, - spawnArgs: buildExecutableArgs(editorCommand, pathValue, platform) - } + // Why: settings often store a full path to idea.cmd / idea.exe; upgrade + // those the same way as PATH-resolved names when *64.exe is colocated. + return buildExecutableLaunchSpec( + preferJetBrainsGuiExecutable(stripMatchingQuotes(trimmed), platform, fileExists), + pathValue, + platform + ) } if (isCompoundShellCommand(trimmed)) { return buildShellLaunchSpec(trimmed, pathValue, platform) } - const editorCommand = resolveCliCommand(trimmed, { platform }) - return { - kind: 'executable', - hideWindowsConsole: !shouldShowWindowsConsole(editorCommand, platform), - spawnCmd: editorCommand, - spawnArgs: buildExecutableArgs(editorCommand, pathValue, platform) - } + return buildExecutableLaunchSpec( + resolveSimpleEditorCommand(trimmed, platform, fileExists), + pathValue, + platform + ) } export function resolveVsCodeRemoteSshLaunchSpec( @@ -217,3 +245,52 @@ export function resolveVsCodeRemoteSshLaunchSpec( spawnArgs: ['--remote', `ssh-remote+${authority}`, pathValue] } } + +function resolveExternalEditorSpawn(launchSpec: ExternalEditorLaunchSpec): { + spawnCmd: string + spawnArgs: string[] + windowsHide: boolean +} { + // Why: only shims flagged during resolution (JetBrains) take the start /B + // detach; every other launcher keeps the waiting form so its argv survives. + if (launchSpec.kind === 'executable') { + const spawned = getSpawnArgsForWindows(launchSpec.spawnCmd, launchSpec.spawnArgs, { + detachedGui: launchSpec.detachedGui === true + }) + return { ...spawned, windowsHide: launchSpec.hideWindowsConsole } + } + return { + spawnCmd: launchSpec.spawnCmd, + spawnArgs: launchSpec.spawnArgs, + windowsHide: launchSpec.hideWindowsConsole + } +} + +export async function launchExternalEditor(launchSpec: ExternalEditorLaunchSpec): Promise { + const { spawnCmd, spawnArgs, windowsHide } = resolveExternalEditorSpawn(launchSpec) + await new Promise((resolvePromise, rejectPromise) => { + const child = spawn(spawnCmd, spawnArgs, { detached: true, stdio: 'ignore', windowsHide }) + let settled = false + function cleanup(): void { + child.off('error', onError) + child.off('spawn', onSpawn) + } + function settle(callback: () => void): void { + if (settled) { + return + } + settled = true + cleanup() + callback() + } + function onError(error: Error): void { + settle(() => rejectPromise(error)) + } + function onSpawn(): void { + child.unref() + settle(resolvePromise) + } + child.once('error', onError) + child.once('spawn', onSpawn) + }) +} diff --git a/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts b/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts index 83aab9d2fab..1fb1bb12d7b 100644 --- a/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts +++ b/src/main/ipc/crash-reporting-renderer-breadcrumbs.test.ts @@ -298,6 +298,49 @@ describe('renderer breadcrumb IPC routing', () => { ]) }) + // Why: the renderer guard is once per tab-id/verdict, so one stale worktree + // map can emit enough crumbs to evict the pre-crash trail. + it('coalesces duplicate-tab-owner notices across tabs', () => { + emitRendererBreadcrumb({ + name: 'terminal_tab_id_owned_by_multiple_worktrees', + data: { ownerCount: 2, resolvedToActiveWorktree: true } + }) + emitRendererBreadcrumb({ + name: 'terminal_tab_id_owned_by_multiple_worktrees', + data: { ownerCount: 3, resolvedToActiveWorktree: true } + }) + + expect(recordCrashBreadcrumbMock).not.toHaveBeenCalled() + expect(recordCoalescedCrashBreadcrumbMock).toHaveBeenCalledTimes(2) + for (const call of recordCoalescedCrashBreadcrumbMock.mock.calls) { + expect(call[0]).toMatchObject({ + coalesceKey: 'terminal_tab_id_owned_by_multiple_worktrees:true' + }) + } + }) + + // Why flag-scoped: coalescing keeps only the newest payload, and the verdict + // flips under a persisting duplicate, so one would erase the other. + it('keeps a non-converging duplicate-tab-owner notice out of the converging one', () => { + emitRendererBreadcrumb({ + name: 'terminal_tab_id_owned_by_multiple_worktrees', + data: { ownerCount: 2, resolvedToActiveWorktree: false } + }) + emitRendererBreadcrumb({ + name: 'terminal_tab_id_owned_by_multiple_worktrees', + data: { ownerCount: 2, resolvedToActiveWorktree: true } + }) + + expect( + recordCoalescedCrashBreadcrumbMock.mock.calls.map( + (call) => (call[0] as { coalesceKey: string }).coalesceKey + ) + ).toEqual([ + 'terminal_tab_id_owned_by_multiple_worktrees:false', + 'terminal_tab_id_owned_by_multiple_worktrees:true' + ]) + }) + it('records non-error renderer breadcrumbs without coalescing', () => { emitRendererBreadcrumb({ name: 'renderer_bootstrap_started', data: { dev: true } }) diff --git a/src/main/ipc/crash-reporting.ts b/src/main/ipc/crash-reporting.ts index 4cdd4bdbbb4..f028c7bc3b3 100644 --- a/src/main/ipc/crash-reporting.ts +++ b/src/main/ipc/crash-reporting.ts @@ -326,11 +326,13 @@ function buildUncapturedCrashReportText( // storm, #8260) can flush the whole fixed-size breadcrumb ring in seconds, // erasing the pre-crash trail. Coalesce repeats into one entry that carries a // suppressed count instead. +const DUPLICATE_TAB_OWNER_BREADCRUMB = 'terminal_tab_id_owned_by_multiple_worktrees' const COALESCED_RENDERER_BREADCRUMB_NAMES = new Set([ 'renderer_error', 'renderer_unhandled_rejection', 'terminal_park_verdict_churn', 'terminal_safe_fit_retry_exhausted', + DUPLICATE_TAB_OWNER_BREADCRUMB, TERMINAL_WEBGL_DIAGNOSTIC_BREADCRUMB ]) const RENDERER_BREADCRUMB_COALESCE_MS = 30_000 @@ -362,6 +364,11 @@ function rendererBreadcrumbCoalesceKey( if (name === TERMINAL_WEBGL_DIAGNOSTIC_BREADCRUMB) { return `${name}:${String(data?.kind ?? '')}` } + // Why: a stale map can emit once per tab-id/verdict; key by verdict so + // last-write coalescing cannot erase the other signal while remaining bounded. + if (name === DUPLICATE_TAB_OWNER_BREADCRUMB) { + return `${name}:${String(data?.resolvedToActiveWorktree ?? '')}` + } const primaryMessage = name === 'renderer_error' ? data?.message : data?.reasonMessage const fallbackMessage = name === 'renderer_error' ? data?.errorMessage : undefined const message = diff --git a/src/main/ipc/shell.test.ts b/src/main/ipc/shell.test.ts index 7193da4beaa..9671eb7a09c 100644 --- a/src/main/ipc/shell.test.ts +++ b/src/main/ipc/shell.test.ts @@ -297,9 +297,13 @@ describe('registerShellHandlers', () => { expect(resolveCliCommandMock).toHaveBeenCalledWith(EXTERNAL_EDITOR_CLI_COMMAND, { platform: process.platform }) - expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith('editor-cli', [ - normalize(workspacePath) - ]) + expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith( + 'editor-cli', + [normalize(workspacePath)], + { + detachedGui: false + } + ) expect(spawnMock).toHaveBeenCalledWith('editor-cli', [normalize(workspacePath)], { detached: true, stdio: 'ignore', @@ -320,9 +324,13 @@ describe('registerShellHandlers', () => { expect(resolveCliCommandMock).toHaveBeenCalledWith(EXTERNAL_EDITOR_CLI_COMMAND, { platform: process.platform }) - expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith('editor-cli', [ - normalize(workspacePath) - ]) + expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith( + 'editor-cli', + [normalize(workspacePath)], + { + detachedGui: false + } + ) expect(spawnMock).toHaveBeenCalledWith('editor-cli', [normalize(workspacePath)], { detached: true, stdio: 'ignore', @@ -334,16 +342,25 @@ describe('registerShellHandlers', () => { }) it('uses a provided launcher command', async () => { + resolveCliCommandMock.mockReturnValueOnce('custom-editor') const workspacePath = resolve('workspace') const handler = getHandler('shell:openInExternalEditor') - await expect(handler({}, { path: workspacePath, command: 'cursor' })).resolves.toEqual({ - ok: true + await expect(handler({}, { path: workspacePath, command: 'custom-editor' })).resolves.toEqual( + { + ok: true + } + ) + expect(resolveCliCommandMock).toHaveBeenCalledWith('custom-editor', { + platform: process.platform }) - expect(resolveCliCommandMock).toHaveBeenCalledWith('cursor', { platform: process.platform }) - expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith('editor-cli', [ - normalize(workspacePath) - ]) + expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith( + 'custom-editor', + [normalize(workspacePath)], + { + detachedGui: false + } + ) }) it.runIf(process.platform === 'win32')( @@ -357,11 +374,13 @@ describe('registerShellHandlers', () => { await expect(handler({}, { path: workspacePath, command: 'code' })).resolves.toEqual({ ok: true }) - expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith(codeShim, [ - '--remote', - 'wsl+Ubuntu Preview', - '/home/Ada Lovelace/project' - ]) + expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith( + codeShim, + ['--remote', 'wsl+Ubuntu Preview', '/home/Ada Lovelace/project'], + { + detachedGui: false + } + ) } ) @@ -377,9 +396,13 @@ describe('registerShellHandlers', () => { ok: true }) expect(resolveCliCommandMock).not.toHaveBeenCalled() - expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith(nvimPath, [ - normalize(workspacePath) - ]) + expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith( + nvimPath, + [normalize(workspacePath)], + { + detachedGui: false + } + ) expect(spawnMock).toHaveBeenCalledWith(nvimPath, [normalize(workspacePath)], { detached: true, stdio: 'ignore', @@ -392,6 +415,37 @@ describe('registerShellHandlers', () => { } }) + it('detaches JetBrains batch shims on Windows but leaves other launchers waiting', async () => { + const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform') + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + const workspacePath = normalize(resolve('workspace')) + const ideaShim = 'C:\\Users\\me\\AppData\\Local\\JetBrains\\Toolbox\\scripts\\idea.cmd' + const codeShim = 'C:\\Tools\\code.cmd' + const handler = getHandler('shell:openInExternalEditor') + + try { + resolveCliCommandMock.mockReturnValueOnce(ideaShim) + await expect(handler({}, { path: workspacePath, command: 'idea' })).resolves.toEqual({ + ok: true + }) + expect(getSpawnArgsForWindowsMock).toHaveBeenLastCalledWith(ideaShim, [workspacePath], { + detachedGui: true + }) + + resolveCliCommandMock.mockReturnValueOnce(codeShim) + await expect(handler({}, { path: workspacePath, command: 'code' })).resolves.toEqual({ + ok: true + }) + expect(getSpawnArgsForWindowsMock).toHaveBeenLastCalledWith(codeShim, [workspacePath], { + detachedGui: false + }) + } finally { + if (platformDescriptor) { + Object.defineProperty(process, 'platform', platformDescriptor) + } + } + }) + it('forces Cursor launcher folders into a new window', async () => { resolveCliCommandMock.mockReturnValueOnce('/usr/local/bin/cursor') const workspacePath = resolve('workspace') @@ -400,18 +454,24 @@ describe('registerShellHandlers', () => { await expect(handler({}, { path: workspacePath, command: 'cursor' })).resolves.toEqual({ ok: true }) - expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith('/usr/local/bin/cursor', [ - '--new-window', - normalize(workspacePath) - ]) + expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith( + '/usr/local/bin/cursor', + ['--new-window', normalize(workspacePath)], + { + detachedGui: false + } + ) resolveCliCommandMock.mockReturnValueOnce('C:\\Cursor\\cursor.cmd') await expect(handler({}, { path: workspacePath, command: 'cursor' })).resolves.toEqual({ ok: true }) - expect(getSpawnArgsForWindowsMock).toHaveBeenLastCalledWith('C:\\Cursor\\cursor.cmd', [ - '--new-window', - normalize(workspacePath) - ]) + expect(getSpawnArgsForWindowsMock).toHaveBeenLastCalledWith( + 'C:\\Cursor\\cursor.cmd', + ['--new-window', normalize(workspacePath)], + { + detachedGui: false + } + ) }) it('falls back to VS Code when command is blank', async () => { @@ -438,9 +498,13 @@ describe('registerShellHandlers', () => { expect(resolveCliCommandMock).toHaveBeenCalledWith(EXTERNAL_EDITOR_CLI_COMMAND, { platform: process.platform }) - expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith('editor-cli', [ - normalize(workspacePath) - ]) + expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith( + 'editor-cli', + [normalize(workspacePath)], + { + detachedGui: false + } + ) expect(spawnMock).toHaveBeenCalledWith('platform-runner', ['platform-arg'], { detached: true, stdio: 'ignore', @@ -510,11 +574,13 @@ describe('registerShellHandlers', () => { handler({}, { path: remotePath, command: 'code', connectionId: 'ssh-1' }) ).resolves.toEqual({ ok: true }) expect(statMock).not.toHaveBeenCalled() - expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith('/usr/local/bin/code', [ - '--remote', - 'ssh-remote+builder', - remotePath - ]) + expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith( + '/usr/local/bin/code', + ['--remote', 'ssh-remote+builder', remotePath], + { + detachedGui: false + } + ) }) it('preserves Windows-form SSH paths and uses the manual port-22 authority', async () => { @@ -535,11 +601,13 @@ describe('registerShellHandlers', () => { handler({}, { path: remotePath, command: 'code', connectionId: 'ssh-1' }) ).resolves.toEqual({ ok: true }) expect(statMock).not.toHaveBeenCalled() - expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith('C:\\Tools\\code.cmd', [ - '--remote', - 'ssh-remote+Ada@win-builder.example.com', - remotePath - ]) + expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith( + 'C:\\Tools\\code.cmd', + ['--remote', 'ssh-remote+Ada@win-builder.example.com', remotePath], + { + detachedGui: false + } + ) }) it('opens a manual port-22 target with a host-only authority when username is blank', async () => { @@ -558,11 +626,13 @@ describe('registerShellHandlers', () => { await expect( handler({}, { path: '/srv/project', command: 'code', connectionId: 'ssh-1' }) ).resolves.toEqual({ ok: true }) - expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith('/usr/local/bin/code', [ - '--remote', - 'ssh-remote+builder.example.com', - '/srv/project' - ]) + expect(getSpawnArgsForWindowsMock).toHaveBeenCalledWith( + '/usr/local/bin/code', + ['--remote', 'ssh-remote+builder.example.com', '/srv/project'], + { + detachedGui: false + } + ) }) it('rejects relative SSH paths before resolving or spawning a launcher', async () => { diff --git a/src/main/ipc/shell.ts b/src/main/ipc/shell.ts index 760b32eed2a..80f02552f18 100644 --- a/src/main/ipc/shell.ts +++ b/src/main/ipc/shell.ts @@ -1,5 +1,4 @@ import { ipcMain, shell, dialog } from 'electron' -import { spawn } from 'node:child_process' import { constants, copyFile, readFile, stat } from 'node:fs/promises' import { basename, extname, isAbsolute, normalize, posix, win32 } from 'node:path' import { fileURLToPath } from 'node:url' @@ -10,12 +9,11 @@ import type { } from '../../shared/shell-open-types' import { MAX_REPO_ICON_UPLOAD_BYTES } from '../../shared/repo-icon' import type { Store } from '../persistence' -import { getSpawnArgsForWindows } from '../win32-utils' import { EXTERNAL_EDITOR_CLI_COMMAND, + launchExternalEditor, resolveExternalEditorLaunchSpec, - resolveVsCodeRemoteSshLaunchSpec, - type ExternalEditorLaunchSpec + resolveVsCodeRemoteSshLaunchSpec } from '../external-editor-launch' import { resolveVsCodeSshAuthority } from '../ssh/vscode-ssh-authority' @@ -72,49 +70,6 @@ async function openInFileManager( } } -async function launchExternalEditor(launchSpec: ExternalEditorLaunchSpec): Promise { - const { spawnCmd, spawnArgs } = - launchSpec.kind === 'executable' - ? getSpawnArgsForWindows(launchSpec.spawnCmd, launchSpec.spawnArgs) - : { spawnCmd: launchSpec.spawnCmd, spawnArgs: launchSpec.spawnArgs } - - await new Promise((resolvePromise, rejectPromise) => { - const child = spawn(spawnCmd, spawnArgs, { - detached: true, - stdio: 'ignore', - // Why: terminal editors such as nvim need a visible console on Windows; - // GUI editor launches stay hidden to avoid command-shim flashes. - windowsHide: launchSpec.hideWindowsConsole - }) - let settled = false - - function cleanup(): void { - child.off('error', onError) - child.off('spawn', onSpawn) - } - - function settle(callback: () => void): void { - if (settled) { - return - } - settled = true - cleanup() - callback() - } - - function onError(error: Error): void { - settle(() => rejectPromise(error)) - } - - function onSpawn(): void { - child.unref() - settle(resolvePromise) - } - child.once('error', onError) - child.once('spawn', onSpawn) - }) -} - async function openInExternalEditor( store: Store, request: ShellOpenExternalEditorRequest diff --git a/src/main/jetbrains-windows-gui-launchers.ts b/src/main/jetbrains-windows-gui-launchers.ts new file mode 100644 index 00000000000..900b1339745 --- /dev/null +++ b/src/main/jetbrains-windows-gui-launchers.ts @@ -0,0 +1,91 @@ +import { win32 } from 'node:path' +import { + getLauncherBaseName, + isWindowsBatchLauncher, + stripMatchingQuotes +} from './editor-launcher-name' + +// Why: JetBrains installers ship `*64.exe` GUI-subsystem binaries. The +// Toolbox/installer `.cmd`/`.bat` shims (and short-name console `idea.exe` +// stubs) chain through helpers that allocate a visible Command Prompt even +// when the parent spawn uses windowsHide — the STA-3040 prompt-window loop. +// Maps each short console launcher name to its GUI sibling. +const JETBRAINS_WINDOWS_CONSOLE_TO_GUI: Readonly> = { + idea: 'idea64', + webstorm: 'webstorm64', + pycharm: 'pycharm64', + phpstorm: 'phpstorm64', + goland: 'goland64', + rider: 'rider64', + clion: 'clion64', + rubymine: 'rubymine64', + datagrip: 'datagrip64', + rustrover: 'rustrover64', + studio: 'studio64' +} + +const JETBRAINS_WINDOWS_GUI_NAMES = new Set(Object.values(JETBRAINS_WINDOWS_CONSOLE_TO_GUI)) + +/** GUI binary basename for a console or GUI launcher name, or null if unknown. */ +function getGuiExecutableName(launcherBaseName: string): string | null { + if (JETBRAINS_WINDOWS_GUI_NAMES.has(launcherBaseName)) { + return launcherBaseName + } + return JETBRAINS_WINDOWS_CONSOLE_TO_GUI[launcherBaseName] ?? null +} + +/** + * The GUI `*64.exe` beside an already-resolved console entry point, or null. + * + * Why sibling-only: a bare PATH lookup for `idea64` can land in a different, + * stale install, and it costs a second full PATH walk on Electron's main + * thread. Toolbox script directories ship no exe, so those keep the shim and + * rely on the detached-GUI spawn instead. + * + * Upgrades: + * - `idea.cmd` / `idea.bat` / `idea64.cmd` → colocated `idea64.exe` when present + * - short-name console stubs `idea.exe` → colocated `idea64.exe` when present + * - leaves `idea64.exe` and non-JetBrains launchers unchanged + */ +export function resolveColocatedJetBrainsGuiExecutable( + resolvedCommand: string, + fileExists: (path: string) => boolean +): string | null { + const unquoted = stripMatchingQuotes(resolvedCommand) + const baseName = getLauncherBaseName(unquoted) + const isBatch = /\.(?:cmd|bat)$/i.test(unquoted) + const isExe = /\.exe$/i.test(unquoted) + if (!isBatch && !isExe) { + return null + } + + // .exe: only short console names (idea.exe), never the GUI binary itself. + if (isExe) { + const guiName = JETBRAINS_WINDOWS_CONSOLE_TO_GUI[baseName] + if (!guiName) { + return null + } + const candidate = win32.join(win32.dirname(unquoted), `${guiName}.exe`) + return fileExists(candidate) ? candidate : null + } + + // .cmd/.bat: idea.cmd and idea64.cmd both prefer idea64.exe when colocated. + const guiName = getGuiExecutableName(baseName) + if (!guiName) { + return null + } + const candidate = win32.join(win32.dirname(unquoted), `${guiName}.exe`) + return fileExists(candidate) ? candidate : null +} + +// Why: only JetBrains batch shims chain through console helpers that outlive +// the batch and re-open a prompt. Other GUI shims (code.cmd, cursor.cmd) exit +// on their own, and running them under `start` would re-parse their quoted +// WSL/SSH remote argv. detachedGui only rewrites batch targets. +export function isJetBrainsConsoleShim(command: string, platform: NodeJS.Platform): boolean { + return ( + platform === 'win32' && + isWindowsBatchLauncher(command) && + getGuiExecutableName(getLauncherBaseName(command)) !== null + ) +} diff --git a/src/main/win32-utils.test.ts b/src/main/win32-utils.test.ts index 4159d7d89b4..381fef6643e 100644 --- a/src/main/win32-utils.test.ts +++ b/src/main/win32-utils.test.ts @@ -82,6 +82,55 @@ describe('getSpawnArgsForWindows', () => { } }) + it('routes GUI Open In .cmd launches through start /B with an inner cmd /c', () => { + withPlatform('win32', () => { + const { spawnCmd, spawnArgs } = getSpawnArgsForWindows( + 'C:\\Tools\\idea.cmd', + ['C:\\workspaces\\orca'], + { detachedGui: true } + ) + expect(spawnCmd).toBe(getCmdExePath()) + // Why: `start` runs a batch target under a nested `cmd /K` that never + // exits; the inner `cmd /d /c` is what keeps the hidden shell from leaking. + // Title is empty string so libuv emits `""` — not the two-char `'""'`. + expect(spawnArgs).toEqual([ + '/d', + '/c', + 'start', + '', + '/B', + getCmdExePath(), + '/d', + '/c', + 'C:\\Tools\\idea.cmd', + 'C:\\workspaces\\orca' + ]) + expect(spawnArgs[3]).toBe('') + expect(spawnArgs).not.toContain('/K') + expect(spawnArgs).not.toContain('""') + expect(spawnArgs[spawnArgs.indexOf('/B') + 1]).not.toMatch(/\.(?:cmd|bat)$/i) + }) + }) + + it('keeps the waiting form for batch launches without detachedGui', () => { + withPlatform('win32', () => { + const { spawnArgs } = getSpawnArgsForWindows('C:\\Tools\\idea.cmd', ['C:\\workspaces\\orca']) + expect(spawnArgs).toEqual(['/d', '/c', 'C:\\Tools\\idea.cmd', 'C:\\workspaces\\orca']) + }) + }) + + it('leaves .exe GUI launches alone even when detachedGui is requested', () => { + withPlatform('win32', () => { + const { spawnCmd, spawnArgs } = getSpawnArgsForWindows( + 'C:\\Program Files\\JetBrains\\IntelliJ IDEA\\bin\\idea64.exe', + ['C:\\workspaces\\orca'], + { detachedGui: true } + ) + expect(spawnCmd).toBe('C:\\Program Files\\JetBrains\\IntelliJ IDEA\\bin\\idea64.exe') + expect(spawnArgs).toEqual(['C:\\workspaces\\orca']) + }) + }) + it('preserves VS Code WSL remote arguments with spaces through .cmd launchers', () => { withPlatform('win32', () => { const { spawnCmd, spawnArgs } = getSpawnArgsForWindows('C:\\tools\\code.cmd', [ diff --git a/src/main/win32-utils.ts b/src/main/win32-utils.ts index c1031c7c55e..151dc9306f0 100644 --- a/src/main/win32-utils.ts +++ b/src/main/win32-utils.ts @@ -8,7 +8,8 @@ export { isWindowsBatchScript, WINDOWS_BATCH_UNSAFE_ARGUMENTS_ERROR, WINDOWS_BATCH_UNSAFE_CHARACTERS_LABEL, - UnsafeWindowsBatchArgumentsError + UnsafeWindowsBatchArgumentsError, + type GetSpawnArgsForWindowsOptions } from '../shared/windows-batch-spawn' function execFileWithoutBlocking( diff --git a/src/renderer/src/components/Terminal.tsx b/src/renderer/src/components/Terminal.tsx index 561dd4e8c40..4189b0e44a1 100644 --- a/src/renderer/src/components/Terminal.tsx +++ b/src/renderer/src/components/Terminal.tsx @@ -64,7 +64,7 @@ import { import TabGroupSplitLayout from './tab-group/TabGroupSplitLayout' import AiVaultSessionDropLayer from './tab-group/AiVaultSessionDropLayer' import { shouldAutoCreateInitialTerminal } from './terminal/initial-terminal' -import { resolveRepairedActiveTerminalTabId } from './terminal/active-terminal-repair' +import { useActiveTerminalRepair } from './terminal/use-active-terminal-repair' import { scheduleBackgroundTerminalWorktreeMeasure } from './terminal/background-terminal-worktree-visibility' import { applyBackgroundMountTabRestriction, @@ -390,7 +390,10 @@ function Terminal(): React.JSX.Element | null { }, [foregroundTerminalTabIds]) const tabs = useMemo( - () => (renderedActiveWorktreeId ? (tabsByWorktree[renderedActiveWorktreeId] ?? []) : []), + () => + renderedActiveWorktreeId !== null && Object.hasOwn(tabsByWorktree, renderedActiveWorktreeId) + ? tabsByWorktree[renderedActiveWorktreeId] + : [], [renderedActiveWorktreeId, tabsByWorktree] ) useTerminalProviderSnapshotCapability(workspaceSessionReady && hydrationSucceeded) @@ -749,32 +752,15 @@ function Terminal(): React.JSX.Element | null { ) }, [queueEditorCloseRequests]) - useEffect(() => { - const rememberedTabId = renderedActiveWorktreeId - ? (activeTabIdByWorktree[renderedActiveWorktreeId] ?? null) - : null - // Why: prefer the remembered active tab so a repair on a transient switch render doesn't reset selection to Terminal 1. - const repairedTabId = resolveRepairedActiveTerminalTabId({ - activeTabType, - activeTabId, - rememberedTabId, - tabs - }) - if (!repairedTabId) { - return - } - // Why: run in an effect (Zustand mutation during render trips React's cross-component update warning); keep terminal-only so inactive CLI-created tabs can't steal editor/browser focus. - setActiveTab(repairedTabId) - // Why: `tabs` is the dependency so the repair reacts to tab-order/content changes, not just scalar IDs. - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ + // Why: repair after render so Zustand mutation cannot trip React's cross-component update warning. + useActiveTerminalRepair({ activeTabId, activeTabType, setActiveTab, tabs, activeTabIdByWorktree, renderedActiveWorktreeId - ]) + }) // Why: only mount TerminalPanes for visited worktrees, else restoring many saved tabs mass-spawns PTYs. const measurableBackgroundWorktreeTimersRef = useRef(new Map()) diff --git a/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.ts b/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.ts index 9384890b4ea..e83a565bd95 100644 --- a/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.ts +++ b/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.test.ts @@ -63,6 +63,44 @@ describe('createRemoteRuntimePtyTransport', () => { return payload } + function subscribedTerminalHandles(): string[] { + return subscriptionSendBinary.mock.calls + .map((call) => decodeTerminalStreamFrame(call[0])) + .flatMap((frame) => { + if (frame?.opcode !== TerminalStreamOpcode.Subscribe) { + return [] + } + const payload = decodeTerminalStreamJson<{ terminal: string }>(frame.payload) + return payload ? [payload.terminal] : [] + }) + } + + function readyHostSessionInventoryResponse(terminal: string, hostTabId = 'host-tab-1'): unknown { + return { + ok: true, + result: { + worktree: 'wt-1', + publicationEpoch: 'epoch-ready', + snapshotVersion: 2, + activeGroupId: null, + activeTabId: `${hostTabId}::pane:1`, + activeTabType: 'terminal', + tabs: [ + { + type: 'terminal', + id: `${hostTabId}::pane:1`, + parentTabId: hostTabId, + leafId: 'pane:1', + title: 'Terminal', + isActive: true, + status: 'ready', + terminal + } + ] + } + } + } + function emitOutput(streamId: number, data: string): void { subscriptionCallbacks?.onBinary?.( encodeTerminalStreamFrame({ @@ -1954,6 +1992,770 @@ describe('createRemoteRuntimePtyTransport', () => { }) }) + it('retries inventory and reattaches the same HUB handle after a stream ends', async () => { + vi.useFakeTimers() + try { + const { createRemoteRuntimePtyTransport } = await import('./remote-runtime-pty-transport') + const onPtyExit = vi.fn() + const onPtyRebind = vi.fn() + const transport = createRemoteRuntimePtyTransport('hub-env', { + worktreeId: 'wt-1', + tabId: 'web-terminal-host-tab-1', + leafId: 'pane:1', + onPtyExit, + onPtyRebind + }) + + resolvedPaneHandle = 'terminal-stable' + transport.attach({ + existingPtyId: 'remote:hub-env@@terminal-stable', + cols: 100, + rows: 30, + callbacks: {} + }) + await vi.waitFor(() => expect(subscriptionSendBinary).toHaveBeenCalled()) + const oldStreamId = latestSubscribePayload().streamId + emitSnapshot(oldStreamId, 'before stream end') + expect(transport.isConnected()).toBe(true) + + let inventoryAvailable = false + let hostListCalls = 0 + runtimeCall.mockImplementation(async (args: { method: string }) => { + if (args.method !== 'session.tabs.list') { + return { ok: true, result: {} } + } + hostListCalls += 1 + if (!inventoryAvailable) { + throw new Error('runtime reconnect in progress') + } + return readyHostSessionInventoryResponse('terminal-stable') + }) + + subscriptionCallbacks?.onResponse({ + ok: true, + result: { type: 'end', streamId: oldStreamId, code: 0 } + }) + await vi.advanceTimersByTimeAsync(16_000) + + const subscribeCount = (): number => + subscriptionSendBinary.mock.calls + .map((call) => decodeTerminalStreamFrame(call[0])) + .filter((frame) => frame?.opcode === TerminalStreamOpcode.Subscribe).length + expect(subscribeCount()).toBe(1) + expect(hostListCalls).toBeGreaterThan(1) + expect(hostListCalls).toBeLessThan(25) + inventoryAvailable = true + await vi.advanceTimersByTimeAsync(16_000) + + expect(hostListCalls).toBeLessThan(40) + expect(subscribedTerminalHandles()).toEqual(['terminal-stable', 'terminal-stable']) + expect(onPtyRebind).not.toHaveBeenCalled() + expect(onPtyExit).not.toHaveBeenCalled() + expect(transport.getPtyId()).toBe('remote:hub-env@@terminal-stable') + expect(transport.isConnected()).toBe(false) + + emitSnapshot(latestSubscribePayload().streamId, 'same handle reattached') + expect(transport.isConnected()).toBe(true) + } finally { + vi.useRealTimers() + } + }) + + it('caps unavailable host inventory at two recovery windows', async () => { + vi.useFakeTimers() + try { + resolvedPaneHandle = 'terminal-stable' + const { createRemoteRuntimePtyTransport } = await import('./remote-runtime-pty-transport') + const transport = createRemoteRuntimePtyTransport('hub-env', { + worktreeId: 'wt-1', + tabId: 'web-terminal-host-tab-1', + leafId: 'pane:1' + }) + + transport.attach({ + existingPtyId: 'remote:hub-env@@terminal-stable', + callbacks: {} + }) + await vi.waitFor(() => expect(subscriptionSendBinary).toHaveBeenCalled()) + const oldStreamId = latestSubscribePayload().streamId + emitSnapshot(oldStreamId, 'before inventory outage') + + let hostListCalls = 0 + runtimeCall.mockImplementation(async (args: { method: string }) => { + if (args.method === 'session.tabs.list') { + hostListCalls += 1 + throw new Error('runtime reconnect in progress') + } + return { ok: true, result: {} } + }) + + subscriptionCallbacks?.onResponse({ + ok: true, + result: { type: 'end', streamId: oldStreamId, code: 0 } + }) + await vi.advanceTimersByTimeAsync(32_000) + + const callsAfterTwoWindows = hostListCalls + expect(callsAfterTwoWindows).toBeGreaterThan(25) + expect(callsAfterTwoWindows).toBeLessThan(40) + await vi.advanceTimersByTimeAsync(20_000) + expect(hostListCalls).toBe(callsAfterTwoWindows) + expect(transport.getRecoveryState?.().phase).toBe('recovering') + + await vi.advanceTimersByTimeAsync(9_000) + expect(transport.getRecoveryState?.().phase).toBe('disconnected') + expect(subscribedTerminalHandles()).toEqual(['terminal-stable']) + transport.destroy?.() + } finally { + vi.useRealTimers() + } + }) + + it('reattaches from prior ready evidence when the trailing inventory poll fails', async () => { + vi.useFakeTimers() + try { + resolvedPaneHandle = 'terminal-stable' + const { createRemoteRuntimePtyTransport } = await import('./remote-runtime-pty-transport') + const onPtyExit = vi.fn() + const transport = createRemoteRuntimePtyTransport('hub-env', { + worktreeId: 'wt-1', + tabId: 'web-terminal-host-tab-1', + leafId: 'pane:1', + onPtyExit + }) + + transport.attach({ + existingPtyId: 'remote:hub-env@@terminal-stable', + callbacks: {} + }) + await vi.waitFor(() => expect(subscriptionSendBinary).toHaveBeenCalled()) + const oldStreamId = latestSubscribePayload().streamId + emitSnapshot(oldStreamId, 'before trailing inventory failure') + + let hostListCalls = 0 + runtimeCall.mockImplementation(async (args: { method: string }) => { + if (args.method !== 'session.tabs.list') { + return { ok: true, result: {} } + } + hostListCalls += 1 + if (hostListCalls === 1) { + return readyHostSessionInventoryResponse('terminal-stable') + } + throw new Error('final inventory poll failed') + }) + + subscriptionCallbacks?.onResponse({ + ok: true, + result: { type: 'end', streamId: oldStreamId, code: 0 } + }) + await vi.advanceTimersByTimeAsync(16_000) + + expect(hostListCalls).toBeGreaterThan(1) + expect(hostListCalls).toBeLessThan(25) + expect(subscribedTerminalHandles()).toEqual(['terminal-stable', 'terminal-stable']) + expect(onPtyExit).not.toHaveBeenCalled() + expect(transport.isConnected()).toBe(false) + emitSnapshot(latestSubscribePayload().streamId, 'reattached from ready evidence') + expect(transport.isConnected()).toBe(true) + transport.destroy?.() + } finally { + vi.useRealTimers() + } + }) + + it('strengthens repeated same-handle end recovery until it disconnects', async () => { + vi.useFakeTimers() + try { + resolvedPaneHandle = 'terminal-flapping' + const { createRemoteRuntimePtyTransport } = await import('./remote-runtime-pty-transport') + const transport = createRemoteRuntimePtyTransport('hub-env', { + worktreeId: 'wt-1', + tabId: 'web-terminal-host-tab-1', + leafId: 'pane:1' + }) + + transport.attach({ + existingPtyId: 'remote:hub-env@@terminal-flapping', + callbacks: {} + }) + await vi.waitFor(() => expect(subscriptionSendBinary).toHaveBeenCalled()) + runtimeCall.mockImplementation(async (args: { method: string }) => + args.method === 'session.tabs.list' + ? readyHostSessionInventoryResponse('terminal-flapping') + : { ok: true, result: {} } + ) + emitSnapshot(latestSubscribePayload().streamId, 'initial stream') + + for (let cycle = 0; cycle < 2; cycle += 1) { + const endingStreamId = latestSubscribePayload().streamId + subscriptionCallbacks?.onResponse({ + ok: true, + result: { type: 'end', streamId: endingStreamId, code: 0 } + }) + await vi.advanceTimersByTimeAsync(16_000) + expect(subscribedTerminalHandles()).toHaveLength(cycle + 2) + emitSnapshot(latestSubscribePayload().streamId, `same handle cycle ${cycle}`) + expect(transport.isConnected()).toBe(true) + } + + subscriptionCallbacks?.onResponse({ + ok: true, + result: { type: 'end', streamId: latestSubscribePayload().streamId, code: 0 } + }) + await vi.advanceTimersByTimeAsync(16_000) + + expect(subscribedTerminalHandles()).toEqual([ + 'terminal-flapping', + 'terminal-flapping', + 'terminal-flapping' + ]) + expect(transport.isConnected()).toBe(false) + await vi.advanceTimersByTimeAsync(45_000) + expect(transport.getRecoveryState?.().phase).toBe('disconnected') + transport.destroy?.() + } finally { + vi.useRealTimers() + } + }) + + it('ignores a delayed stale send after the pane has rebound to a healthy handle', async () => { + resolvedPaneHandle = 'terminal-old' + let resolveOldSend: (response: unknown) => void = () => {} + const oldSendResponse = new Promise((resolve) => { + resolveOldSend = resolve + }) + let hostListCalls = 0 + const { createRemoteRuntimePtyTransport } = await import('./remote-runtime-pty-transport') + const onPtyExit = vi.fn() + const transport = createRemoteRuntimePtyTransport('env-1', { + worktreeId: 'wt-1', + tabId: 'web-terminal-host-tab-1', + leafId: 'pane:1', + onPtyExit + }) + + transport.attach({ + existingPtyId: 'remote:env-1@@terminal-old', + callbacks: {} + }) + await vi.waitFor(() => expect(subscriptionSendBinary).toHaveBeenCalled()) + const oldStreamId = latestSubscribePayload().streamId + emitSnapshot(oldStreamId, 'old handle') + runtimeCall.mockImplementation((args: { method: string }) => { + if (args.method === 'terminal.send') { + return oldSendResponse + } + if (args.method === 'session.tabs.list') { + hostListCalls += 1 + return Promise.resolve(readyHostSessionInventoryResponse('terminal-new')) + } + return Promise.resolve({ ok: true, result: {} }) + }) + const sendInputAccepted = transport.sendInputAccepted + if (!sendInputAccepted) { + throw new Error('Expected acknowledged remote terminal input') + } + const pendingSend = sendInputAccepted('sent-before-rebind') + await vi.waitFor(() => + expect(runtimeCall).toHaveBeenCalledWith(expect.objectContaining({ method: 'terminal.send' })) + ) + + subscriptionCallbacks?.onResponse({ + ok: true, + result: { type: 'error', streamId: oldStreamId, message: 'terminal_handle_stale' } + }) + await vi.waitFor(() => + expect(latestSubscribePayload()).toMatchObject({ terminal: 'terminal-new' }) + ) + emitSnapshot(latestSubscribePayload().streamId, 'new handle') + expect(transport.isConnected()).toBe(true) + + resolveOldSend({ + ok: false, + error: { code: 'terminal_handle_stale', message: 'terminal_handle_stale' } + }) + await expect(pendingSend).resolves.toBe(false) + + expect(subscribedTerminalHandles()).toEqual(['terminal-old', 'terminal-new']) + expect(hostListCalls).toBe(1) + expect(onPtyExit).not.toHaveBeenCalled() + expect(transport.getPtyId()).toBe('remote:env-1@@terminal-new') + expect(transport.isConnected()).toBe(true) + }) + + it('honors a sticky replacement requirement during non-web pane resolution', async () => { + resolvedPaneHandle = 'terminal-old' + const { createRemoteRuntimePtyTransport } = await import('./remote-runtime-pty-transport') + const onPtyExit = vi.fn() + const transport = createRemoteRuntimePtyTransport('env-1', { + worktreeId: 'wt-1', + tabId: 'tab-1', + leafId: 'pane:1', + onPtyExit + }) + + transport.attach({ + existingPtyId: 'remote:env-1@@terminal-old', + callbacks: {} + }) + await vi.waitFor(() => expect(subscriptionSendBinary).toHaveBeenCalled()) + emitSnapshot(latestSubscribePayload().streamId, 'old handle') + + let resolveOldSend: (response: unknown) => void = () => {} + const oldSendResponse = new Promise((resolve) => { + resolveOldSend = resolve + }) + let resolvePane: (response: unknown) => void = () => {} + const paneResponse = new Promise((resolve) => { + resolvePane = resolve + }) + runtimeCall.mockImplementation((args: { method: string }) => { + if (args.method === 'terminal.send') { + return oldSendResponse + } + if (args.method === 'terminal.resolvePane') { + return paneResponse + } + return Promise.resolve({ ok: true, result: {} }) + }) + const sendInputAccepted = transport.sendInputAccepted + if (!sendInputAccepted) { + throw new Error('Expected acknowledged remote terminal input') + } + const pendingSend = sendInputAccepted('sent-before-close') + await vi.waitFor(() => + expect(runtimeCall).toHaveBeenCalledWith(expect.objectContaining({ method: 'terminal.send' })) + ) + + subscriptionCallbacks?.onClose?.() + await vi.waitFor(() => + expect(runtimeCall).toHaveBeenCalledWith( + expect.objectContaining({ method: 'terminal.resolvePane' }) + ) + ) + resolveOldSend({ + ok: false, + error: { code: 'terminal_handle_stale', message: 'terminal_handle_stale' } + }) + await expect(pendingSend).resolves.toBe(false) + resolvePane({ + ok: true, + result: { + terminal: { + handle: 'terminal-old', + tabId: 'tab-1', + leafId: 'pane:1', + worktreeId: 'wt-1' + } + } + }) + + await vi.waitFor(() => expect(onPtyExit).toHaveBeenCalledOnce()) + expect(subscribedTerminalHandles()).toEqual(['terminal-old']) + expect(onPtyExit).toHaveBeenCalledWith('remote:env-1@@terminal-old') + expect(transport.getPtyId()).toBeNull() + expect(transport.isConnected()).toBe(false) + }) + + it('does not subscribe a same handle condemned after its inventory wait returns', async () => { + vi.useFakeTimers() + try { + resolvedPaneHandle = 'terminal-old' + const { createRemoteRuntimePtyTransport } = await import('./remote-runtime-pty-transport') + const onPtyRebind = vi.fn() + const transport = createRemoteRuntimePtyTransport('env-1', { + worktreeId: 'wt-1', + tabId: 'web-terminal-host-tab-1', + leafId: 'pane:1', + onPtyRebind + }) + + transport.attach({ + existingPtyId: 'remote:env-1@@terminal-old', + callbacks: {} + }) + await vi.waitFor(() => expect(subscriptionSendBinary).toHaveBeenCalled()) + const oldStreamId = latestSubscribePayload().streamId + emitSnapshot(oldStreamId, 'old handle') + + let resolveOldSend: (response: unknown) => void = () => {} + const oldSendResponse = new Promise((resolve) => { + resolveOldSend = resolve + }) + let hostListCalls = 0 + runtimeCall.mockImplementation( + (args: { method: string; timeoutMs?: number }): Promise => { + if (args.method === 'terminal.send') { + return oldSendResponse + } + if (args.method !== 'session.tabs.list') { + return Promise.resolve({ ok: true, result: {} }) + } + hostListCalls += 1 + const response = readyHostSessionInventoryResponse('terminal-old') + if ((args.timeoutMs ?? 15_000) > 1_000) { + return Promise.resolve(response) + } + return new Promise((resolve) => { + setTimeout(() => { + // Why: settle the stale send between the inner wait and its caller's continuation. + queueMicrotask(() => + queueMicrotask(() => + resolveOldSend({ + ok: false, + error: { + code: 'terminal_handle_stale', + message: 'terminal_handle_stale' + } + }) + ) + ) + resolve(response) + }, args.timeoutMs) + }) + } + ) + const sendInputAccepted = transport.sendInputAccepted + if (!sendInputAccepted) { + throw new Error('Expected acknowledged remote terminal input') + } + const pendingSend = sendInputAccepted('sent-before-stream-end') + await vi.waitFor(() => + expect(runtimeCall).toHaveBeenCalledWith( + expect.objectContaining({ method: 'terminal.send' }) + ) + ) + + subscriptionCallbacks?.onResponse({ + ok: true, + result: { type: 'end', streamId: oldStreamId, code: 0 } + }) + await vi.advanceTimersByTimeAsync(16_000) + await expect(pendingSend).resolves.toBe(false) + + expect(hostListCalls).toBeGreaterThan(1) + expect(subscribedTerminalHandles()).toEqual(['terminal-old']) + expect(transport.getPtyId()).toBe('remote:env-1@@terminal-old') + expect(transport.isConnected()).toBe(false) + + const handleEvents = await import('../../runtime/web-session-terminal-handle-events') + handleEvents.queueAcceptedWebSessionTerminalSnapshot( + { + worktree: 'wt-1', + publicationEpoch: 'epoch-replacement', + snapshotVersion: 3, + activeGroupId: null, + activeTabId: 'host-tab-1::pane:1', + activeTabType: 'terminal', + tabs: [ + { + type: 'terminal', + id: 'host-tab-1::pane:1', + parentTabId: 'host-tab-1', + leafId: 'pane:1', + title: 'Terminal', + isActive: true, + status: 'ready', + terminal: 'terminal-new' + } + ] + }, + 'env-1' + ) + await vi.advanceTimersByTimeAsync(0) + await vi.waitFor(() => + expect(latestSubscribePayload()).toMatchObject({ terminal: 'terminal-new' }) + ) + expect(onPtyRebind).toHaveBeenCalledWith( + 'remote:env-1@@terminal-new', + 'remote:env-1@@terminal-old' + ) + } finally { + vi.useRealTimers() + } + }) + + it('uses a strengthened sticky policy when an inventory retry starts', async () => { + vi.useFakeTimers() + try { + resolvedPaneHandle = 'terminal-old' + const { createRemoteRuntimePtyTransport } = await import('./remote-runtime-pty-transport') + const transport = createRemoteRuntimePtyTransport('env-1', { + worktreeId: 'wt-1', + tabId: 'web-terminal-host-tab-1', + leafId: 'pane:1' + }) + + transport.attach({ + existingPtyId: 'remote:env-1@@terminal-old', + callbacks: {} + }) + await vi.waitFor(() => expect(subscriptionSendBinary).toHaveBeenCalled()) + const oldStreamId = latestSubscribePayload().streamId + emitSnapshot(oldStreamId, 'old handle') + + let resolveOldSend: (response: unknown) => void = () => {} + const oldSendResponse = new Promise((resolve) => { + resolveOldSend = resolve + }) + let hostListCalls = 0 + runtimeCall.mockImplementation((args: { method: string }) => { + if (args.method === 'terminal.send') { + return oldSendResponse + } + if (args.method === 'session.tabs.list') { + hostListCalls += 1 + return Promise.reject(new Error('runtime reconnect in progress')) + } + return Promise.resolve({ ok: true, result: {} }) + }) + const sendInputAccepted = transport.sendInputAccepted + if (!sendInputAccepted) { + throw new Error('Expected acknowledged remote terminal input') + } + const pendingSend = sendInputAccepted('sent-before-stream-end') + await vi.waitFor(() => + expect(runtimeCall).toHaveBeenCalledWith( + expect.objectContaining({ method: 'terminal.send' }) + ) + ) + + subscriptionCallbacks?.onResponse({ + ok: true, + result: { type: 'end', streamId: oldStreamId, code: 0 } + }) + await vi.advanceTimersByTimeAsync(15_000) + expect(transport.getRecoveryState?.().phase).toBe('backoff') + + resolveOldSend({ + ok: false, + error: { code: 'terminal_handle_stale', message: 'terminal_handle_stale' } + }) + await expect(pendingSend).resolves.toBe(false) + const callsBeforeRetry = hostListCalls + await vi.advanceTimersByTimeAsync(1_000) + + expect(hostListCalls).toBe(callsBeforeRetry) + expect(subscribedTerminalHandles()).toEqual(['terminal-old']) + expect(transport.getPtyId()).toBe('remote:env-1@@terminal-old') + expect(transport.isConnected()).toBe(false) + } finally { + vi.useRealTimers() + } + }) + + it('requires replacement after repeated same-handle stream-end flaps', async () => { + vi.useFakeTimers() + try { + resolvedPaneHandle = 'terminal-flapping' + const { createRemoteRuntimePtyTransport } = await import('./remote-runtime-pty-transport') + const onPtyExit = vi.fn() + const transport = createRemoteRuntimePtyTransport('env-1', { + worktreeId: 'wt-1', + tabId: 'web-terminal-host-tab-1', + leafId: 'pane:1', + onPtyExit + }) + + transport.attach({ + existingPtyId: 'remote:env-1@@terminal-flapping', + callbacks: {} + }) + await vi.waitFor(() => expect(subscriptionSendBinary).toHaveBeenCalled()) + emitSnapshot(latestSubscribePayload().streamId, 'initial handle') + runtimeCall.mockImplementation((args: { method: string }) => + args.method === 'session.tabs.list' + ? Promise.resolve(readyHostSessionInventoryResponse('terminal-flapping')) + : Promise.resolve({ ok: true, result: {} }) + ) + + for (let cycle = 0; cycle < 2; cycle += 1) { + const endedStreamId = latestSubscribePayload().streamId + subscriptionCallbacks?.onResponse({ + ok: true, + result: { type: 'end', streamId: endedStreamId, code: 0 } + }) + await vi.advanceTimersByTimeAsync(16_000) + expect(subscribedTerminalHandles()).toHaveLength(cycle + 2) + expect(latestSubscribePayload().terminal).toBe('terminal-flapping') + emitSnapshot(latestSubscribePayload().streamId, `same handle ${cycle + 1}`) + expect(transport.isConnected()).toBe(true) + } + + const condemnedStreamId = latestSubscribePayload().streamId + subscriptionCallbacks?.onResponse({ + ok: true, + result: { type: 'end', streamId: condemnedStreamId, code: 0 } + }) + await vi.advanceTimersByTimeAsync(16_000) + + expect(subscribedTerminalHandles()).toEqual([ + 'terminal-flapping', + 'terminal-flapping', + 'terminal-flapping' + ]) + expect(transport.getPtyId()).toBe('remote:env-1@@terminal-flapping') + expect(transport.isConnected()).toBe(false) + expect(onPtyExit).not.toHaveBeenCalled() + + await vi.advanceTimersByTimeAsync(44_001) + expect(transport.getRecoveryState?.().phase).toBe('disconnected') + expect(subscribedTerminalHandles()).toHaveLength(3) + } finally { + vi.useRealTimers() + } + }) + + it('reuses prior ready evidence when the trailing inventory poll fails', async () => { + vi.useFakeTimers() + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + resolvedPaneHandle = 'terminal-stable' + const { createRemoteRuntimePtyTransport } = await import('./remote-runtime-pty-transport') + const transport = createRemoteRuntimePtyTransport('env-1', { + worktreeId: 'wt-1', + tabId: 'web-terminal-host-tab-1', + leafId: 'pane:1' + }) + + transport.attach({ + existingPtyId: 'remote:env-1@@terminal-stable', + callbacks: {} + }) + await vi.waitFor(() => expect(subscriptionSendBinary).toHaveBeenCalled()) + const oldStreamId = latestSubscribePayload().streamId + emitSnapshot(oldStreamId, 'before trailing failure') + let hostListCalls = 0 + runtimeCall.mockImplementation( + async (args: { method: string; timeoutMs?: number }): Promise => { + if (args.method !== 'session.tabs.list') { + return { ok: true, result: {} } + } + hostListCalls += 1 + if ((args.timeoutMs ?? 15_000) <= 1_000) { + throw new Error('final inventory poll failed') + } + return readyHostSessionInventoryResponse('terminal-stable') + } + ) + + subscriptionCallbacks?.onResponse({ + ok: true, + result: { type: 'end', streamId: oldStreamId, code: 0 } + }) + await vi.advanceTimersByTimeAsync(16_000) + + expect(hostListCalls).toBeGreaterThan(1) + expect(subscribedTerminalHandles()).toEqual(['terminal-stable', 'terminal-stable']) + expect(warn).not.toHaveBeenCalled() + expect(transport.isConnected()).toBe(false) + emitSnapshot(latestSubscribePayload().streamId, 'ready evidence reused') + expect(transport.isConnected()).toBe(true) + } finally { + warn.mockRestore() + vi.useRealTimers() + } + }) + + it('does not carry a stale-handle requirement onto the replacement stream', async () => { + vi.useFakeTimers() + try { + let rejectedReplacement = false + subscriptionSendBinary.mockImplementation((bytes: Uint8Array) => { + const frame = decodeTerminalStreamFrame(bytes) + if (frame?.opcode !== TerminalStreamOpcode.Subscribe) { + return + } + const payload = decodeTerminalStreamJson<{ terminal: string }>(frame.payload) + if (payload?.terminal === 'terminal-replacement' && !rejectedReplacement) { + rejectedReplacement = true + throw new Error('Remote runtime connection closed.') + } + }) + const { createRemoteRuntimePtyTransport } = await import('./remote-runtime-pty-transport') + const onPtyRebind = vi.fn() + const transport = createRemoteRuntimePtyTransport('hub-env', { + worktreeId: 'wt-1', + tabId: 'web-terminal-host-tab-1', + leafId: 'pane:1', + onPtyRebind + }) + + resolvedPaneHandle = 'terminal-stale' + transport.attach({ + existingPtyId: 'remote:hub-env@@terminal-stale', + callbacks: {} + }) + await vi.waitFor(() => expect(subscriptionSendBinary).toHaveBeenCalled()) + const staleStreamId = latestSubscribePayload().streamId + emitSnapshot(staleStreamId, 'before stale handle') + + runtimeCall.mockImplementation(async (args: { method: string }) => { + if (args.method !== 'session.tabs.list') { + return { ok: true, result: {} } + } + return { + ok: true, + result: { + worktree: 'wt-1', + publicationEpoch: 'epoch-replacement', + snapshotVersion: 2, + activeGroupId: null, + activeTabId: 'host-tab-1::pane:1', + activeTabType: 'terminal', + tabs: [ + { + type: 'terminal', + id: 'host-tab-1::pane:1', + parentTabId: 'host-tab-1', + leafId: 'pane:1', + title: 'Terminal', + isActive: true, + status: 'ready', + terminal: 'terminal-replacement' + } + ] + } + } + }) + + subscriptionCallbacks?.onResponse({ + ok: true, + result: { + type: 'error', + streamId: staleStreamId, + message: 'terminal_handle_stale' + } + }) + await vi.waitFor(() => expect(rejectedReplacement).toBe(true)) + await vi.advanceTimersByTimeAsync(250) + await vi.waitFor(() => expect(runtimeSubscribe.mock.calls.length).toBeGreaterThanOrEqual(2)) + + const subscribedTerminals = subscriptionSendBinary.mock.calls + .map((call) => decodeTerminalStreamFrame(call[0])) + .flatMap((frame) => { + if (frame?.opcode !== TerminalStreamOpcode.Subscribe) { + return [] + } + const payload = decodeTerminalStreamJson<{ terminal: string }>(frame.payload) + return payload ? [payload.terminal] : [] + }) + expect(subscribedTerminals).toEqual([ + 'terminal-stale', + 'terminal-replacement', + 'terminal-replacement' + ]) + expect(onPtyRebind).toHaveBeenCalledOnce() + await vi.waitFor(() => + expect(latestSubscribePayload()).toMatchObject({ terminal: 'terminal-replacement' }) + ) + emitSnapshot(latestSubscribePayload().streamId, 'replacement reattached') + expect(transport.isConnected()).toBe(true) + } finally { + vi.useRealTimers() + } + }) + it('coalesces concurrent stale errors for the handle that was replaced', async () => { const { createRemoteRuntimePtyTransport } = await import('./remote-runtime-pty-transport') const onPtyExit = vi.fn() diff --git a/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts b/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts index 615761567ef..1f9aef116a2 100644 --- a/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/remote-runtime-pty-transport.ts @@ -86,8 +86,29 @@ const REMOTE_TERMINAL_VIEWPORT_FLUSH_MS = 33 const HOST_SESSION_ATTACH_POLL_MS = 150 const HOST_SESSION_REPLACEMENT_POLL_MAX_MS = 1_000 const HOST_SESSION_ATTACH_TIMEOUT_MS = 15_000 +const HOST_SESSION_INVENTORY_MAX_WINDOWS_PER_RECOVERY = 2 +const HOST_SESSION_SAME_HANDLE_END_REUSE_LIMIT = 2 const TERMINAL_CREATE_RETRY_DELAYS_MS = [250, 500, 1000, 2000, 4000, 8000, 15_000, 30_000] as const +type HostHandleReplacementPolicy = 'reuse' | 'prefer-replacement' | 'require-replacement' + +type HostSessionHandleWaitResult = { + handle: string | null | undefined + inventoryFailed: boolean +} + +function stricterReplacementPolicy( + left: HostHandleReplacementPolicy, + right: HostHandleReplacementPolicy +): HostHandleReplacementPolicy { + const rank: Record = { + reuse: 0, + 'prefer-replacement': 1, + 'require-replacement': 2 + } + return rank[left] >= rank[right] ? left : right +} + type RemoteAgentSessionLaunchResult = | RuntimeEnsureAgentSessionResult | RuntimeCreateAgentSessionResult @@ -162,10 +183,16 @@ export function createRemoteRuntimePtyTransport( let storedCallbacks: Parameters[0]['callbacks'] = {} let resubscribeEpoch: number | null = null let resubscribeRequestedHandle: string | null = null - let resubscribeRequestedRequiresReplacement = false - let recoveryRequiresReplacement = false + let resubscribeRequestedReplacementPolicy: HostHandleReplacementPolicy = 'reuse' + let recoveryReplacementPolicy: HostHandleReplacementPolicy = 'reuse' + let recoveryReplacementPolicyHandle: string | null = null let stopWaitingForPublishedHandle: (() => void) | null = null let settleHostSessionAttachRetry: ((retry: boolean) => void) | null = null + let resubscribeInventoryEpoch: number | null = null + let resubscribeInventoryWindows = 0 + let sameHandleEndReuseHandle: string | null = null + let sameHandleEndReuseCount = 0 + let sameHandleEndReuseAttachedAt: number | null = null let attachGeneration = 0 let subscriptionGeneration = 0 @@ -240,6 +267,62 @@ export function createRemoteRuntimePtyTransport( let lastAttachOptions: Parameters[0] | null = null let resolvePaneUnavailable = false let recoveringPaneHandle: string | null = null + const getRecoveryReplacementPolicy = (targetHandle: string): HostHandleReplacementPolicy => + recoveryReplacementPolicyHandle === targetHandle ? recoveryReplacementPolicy : 'reuse' + const resetRecoveryReplacementPolicy = (): void => { + recoveryReplacementPolicy = 'reuse' + recoveryReplacementPolicyHandle = null + } + const strengthenRecoveryReplacementPolicy = ( + targetHandle: string, + replacementPolicy: HostHandleReplacementPolicy + ): void => { + if (recoveryReplacementPolicyHandle !== targetHandle) { + recoveryReplacementPolicyHandle = targetHandle + recoveryReplacementPolicy = replacementPolicy + return + } + recoveryReplacementPolicy = stricterReplacementPolicy( + recoveryReplacementPolicy, + replacementPolicy + ) + } + const beginResubscribeInventoryWindow = (recoveryEpoch: number): number => { + if (resubscribeInventoryEpoch !== recoveryEpoch) { + resubscribeInventoryEpoch = recoveryEpoch + resubscribeInventoryWindows = 0 + } + resubscribeInventoryWindows += 1 + return resubscribeInventoryWindows + } + const resetSameHandleEndReuse = (): void => { + sameHandleEndReuseHandle = null + sameHandleEndReuseCount = 0 + sameHandleEndReuseAttachedAt = null + } + const recordSameHandleEndReuse = (targetHandle: string): void => { + if (sameHandleEndReuseHandle !== targetHandle) { + sameHandleEndReuseHandle = targetHandle + sameHandleEndReuseCount = 0 + } + sameHandleEndReuseCount += 1 + sameHandleEndReuseAttachedAt = Date.now() + } + const replacementPolicyAfterWebStreamEnd = ( + targetHandle: string + ): HostHandleReplacementPolicy => { + if ( + sameHandleEndReuseHandle !== targetHandle || + sameHandleEndReuseAttachedAt === null || + Date.now() - sameHandleEndReuseAttachedAt >= REMOTE_RUNTIME_AUTO_RECOVERY_TIMEOUT_MS + ) { + resetSameHandleEndReuse() + return 'prefer-replacement' + } + return sameHandleEndReuseCount >= HOST_SESSION_SAME_HANDLE_END_REUSE_LIMIT + ? 'require-replacement' + : 'prefer-replacement' + } const adoptExecutionMetadata = (terminal: { executionHostId?: ExecutionHostId hostPlatform?: NodeJS.Platform @@ -528,29 +611,46 @@ export function createRemoteRuntimePtyTransport( async function waitForResubscribeHostSessionHandle( hostTabId: string, previousHandle: string, - requireReplacement: boolean - ): Promise { + replacementPolicy: HostHandleReplacementPolicy, + recoveryEpoch: number + ): Promise { if (!worktreeId) { - return null + return { handle: null, inventoryFailed: false } } const worktree = toRuntimeWorktreeSelector(worktreeId) const startedAt = Date.now() let pollMs = HOST_SESSION_ATTACH_POLL_MS let lastListError: unknown = null - const finishWithUnknownLiveness = (): undefined => { + let lastReadyHandle: string | null = null + let sawSuccessfulInventory = false + const finishBoundedWait = (): HostSessionHandleWaitResult => { + const effectivePolicy = stricterReplacementPolicy( + replacementPolicy, + getRecoveryReplacementPolicy(previousHandle) + ) + if (effectivePolicy === 'prefer-replacement' && lastReadyHandle) { + return { handle: lastReadyHandle, inventoryFailed: false } + } if (lastListError) { console.warn( - '[remote-runtime-pty] host session inventory unavailable during reconnect:', + sawSuccessfulInventory + ? '[remote-runtime-pty] final host session inventory poll failed during reconnect:' + : '[remote-runtime-pty] host session inventory unavailable during reconnect:', runtimeTerminalErrorMessage(lastListError) ) } // Why: a bounded wait without removal evidence is unknown liveness; keep the pane for a later snapshot to reattach. - return undefined + return { handle: undefined, inventoryFailed: lastListError !== null } } - while (!destroyed && connected && handle === previousHandle) { + while ( + !destroyed && + connected && + handle === previousHandle && + recovery.isCurrent(recoveryEpoch) + ) { const requestRemainingMs = HOST_SESSION_ATTACH_TIMEOUT_MS - (Date.now() - startedAt) if (requestRemainingMs <= 0) { - return finishWithUnknownLiveness() + return finishBoundedWait() } try { const listed = await listRemoteRuntimeSessionTabsDeduped({ @@ -566,12 +666,20 @@ export function createRemoteRuntimePtyTransport( ) }) lastListError = null + sawSuccessfulInventory = true const nextHandle = findReadyHostSessionHandle(listed, hostTabId) - if (nextHandle && (!requireReplacement || nextHandle !== previousHandle)) { - return nextHandle + if (nextHandle) { + lastReadyHandle = nextHandle + } + const effectivePolicy = stricterReplacementPolicy( + replacementPolicy, + getRecoveryReplacementPolicy(previousHandle) + ) + if (nextHandle && (effectivePolicy === 'reuse' || nextHandle !== previousHandle)) { + return { handle: nextHandle, inventoryFailed: false } } if (!hasHostSessionTerminalSurface(listed, hostTabId)) { - return null + return { handle: null, inventoryFailed: false } } } catch (error) { // Why: the inventory can race the reconnect that invalidated the handle; unknown liveness must not retire the pane. @@ -579,13 +687,13 @@ export function createRemoteRuntimePtyTransport( } const remainingMs = HOST_SESSION_ATTACH_TIMEOUT_MS - (Date.now() - startedAt) if (remainingMs <= 0) { - return finishWithUnknownLiveness() + return finishBoundedWait() } // Why: a stale response can precede its replacement; bounded backoff avoids retrying the stale handle in a hot loop. await new Promise((resolve) => setTimeout(resolve, Math.min(pollMs, remainingMs))) pollMs = Math.min(pollMs * 2, HOST_SESSION_REPLACEMENT_POLL_MAX_MS) } - return undefined + return { handle: undefined, inventoryFailed: false } } async function attachHostSessionMirror( @@ -1068,7 +1176,9 @@ export function createRemoteRuntimePtyTransport( return true } catch (error) { // Why: stale-handle errors must retire the mirror (recoverable via next snapshot), not dead-end in a red xterm banner (#7718). - handleRemoteTerminalError(error) + if (handle === targetHandle) { + handleRemoteTerminalError(error) + } return false } } @@ -1093,7 +1203,9 @@ export function createRemoteRuntimePtyTransport( client: { id: clientId, type: 'desktop' }, ...(desiredViewport ? { viewport: desiredViewport, claimViewport: true as const } : {}) }).catch((error) => { - handleRemoteTerminalError(error) + if (handle === targetHandle) { + handleRemoteTerminalError(error) + } }) }) @@ -1159,7 +1271,8 @@ export function createRemoteRuntimePtyTransport( function retireRemoteTerminalId(): void { recovery.cancel() - recoveryRequiresReplacement = false + resetRecoveryReplacementPolicy() + resetSameHandleEndReuse() connected = false connecting = false terminalEnded = true @@ -1183,6 +1296,8 @@ export function createRemoteRuntimePtyTransport( unregisterShutdownHandlers(replacedPtyId) handle = nextHandle remotePtyId = toRemoteRuntimePtyId(nextHandle, currentRuntimeEnvironmentId) + resetRecoveryReplacementPolicy() + resetSameHandleEndReuse() registerShutdownHandlers(remotePtyId) setAttachmentReady(false) // Why: host handle rotation preserves the pane generation; only the store identity changes, not spawn/exit semantics. @@ -1239,7 +1354,7 @@ export function createRemoteRuntimePtyTransport( if (tabId && leafId && worktreeId) { // Why: reconnect can re-mint a pane handle while its host coordinates live; keep xterm state mounted while re-resolving. closeMultiplexedStream() - scheduleResubscribeAfterTransportClose(true) + scheduleResubscribeAfterTransportClose('require-replacement') } else { retireRemoteTerminalId() } @@ -1290,16 +1405,19 @@ export function createRemoteRuntimePtyTransport( // Why: after a transport drop the host may have re-minted this handle; re-derive from the snapshot so we don't mirror/type into whatever PTY now sits behind the stale one (#7718). async function resubscribeAfterTransportClose( previousHandle: string, - requireReplacement: boolean, + replacementPolicy: HostHandleReplacementPolicy, recoveryEpoch: number ): Promise { if (tabId && isWebTerminalSurfaceTabId(tabId)) { const hostTabId = toHostSessionTabId(tabId) - const nextHandle = await waitForResubscribeHostSessionHandle( + const inventoryWindow = beginResubscribeInventoryWindow(recoveryEpoch) + const waitResult = await waitForResubscribeHostSessionHandle( hostTabId, previousHandle, - requireReplacement + replacementPolicy, + recoveryEpoch ) + const nextHandle = waitResult.handle if ( destroyed || !connected || @@ -1309,6 +1427,19 @@ export function createRemoteRuntimePtyTransport( return } if (nextHandle === undefined) { + const effectivePolicy = stricterReplacementPolicy( + replacementPolicy, + getRecoveryReplacementPolicy(previousHandle) + ) + if ( + effectivePolicy !== 'require-replacement' && + waitResult.inventoryFailed && + inventoryWindow < HOST_SESSION_INVENTORY_MAX_WINDOWS_PER_RECOVERY + ) { + throw Object.assign(new Error('Remote runtime session inventory polling failed.'), { + code: 'remote_runtime_unavailable' + }) + } return } if (!nextHandle) { @@ -1316,15 +1447,36 @@ export function createRemoteRuntimePtyTransport( retireRemoteTerminalId() return } + const effectivePolicy = stricterReplacementPolicy( + replacementPolicy, + getRecoveryReplacementPolicy(previousHandle) + ) + // Why: a stale error can strengthen policy after inventory returns but before this continuation runs. + if (effectivePolicy === 'require-replacement' && nextHandle === previousHandle) { + return + } if (nextHandle !== previousHandle) { rebindRemoteTerminalHandle(nextHandle) } + clearPublishedHandleWait() + await subscribeToHandle( + recoveryEpoch, + nextHandle === previousHandle && effectivePolicy === 'prefer-replacement' + ) + return } else if (tabId && leafId && worktreeId) { const resolved = await resolvePersistedHostPane() if (destroyed || !connected || handle !== previousHandle) { return } - if (!resolved || (requireReplacement && resolved.handle === previousHandle)) { + const effectivePolicy = stricterReplacementPolicy( + replacementPolicy, + getRecoveryReplacementPolicy(previousHandle) + ) + if ( + !resolved || + (effectivePolicy === 'require-replacement' && resolved.handle === previousHandle) + ) { retireRemoteTerminalId() return } @@ -1337,7 +1489,7 @@ export function createRemoteRuntimePtyTransport( } function scheduleResubscribeAfterTransportClose( - requireReplacement = false, + replacementPolicy: HostHandleReplacementPolicy = 'reuse', requestedRecoveryEpoch?: number ): void { if (destroyed || !connected || !handle) { @@ -1354,8 +1506,8 @@ export function createRemoteRuntimePtyTransport( viewportBatcher.clear() clearPendingViewportClaim() } - recoveryRequiresReplacement ||= requireReplacement - if (requireReplacement && stopWaitingForPublishedHandle) { + strengthenRecoveryReplacementPolicy(handle, replacementPolicy) + if (replacementPolicy === 'require-replacement' && stopWaitingForPublishedHandle) { // Why: once recovery is handed to accepted snapshots, repeated sends to the stale handle must not re-arm inventory RPCs. return } @@ -1363,9 +1515,12 @@ export function createRemoteRuntimePtyTransport( // Why: concurrent stale errors belong to their own handle; don't carry an old handle's replacement requirement onto its successor. if (resubscribeRequestedHandle !== handle) { resubscribeRequestedHandle = handle - resubscribeRequestedRequiresReplacement = requireReplacement + resubscribeRequestedReplacementPolicy = replacementPolicy } else { - resubscribeRequestedRequiresReplacement ||= requireReplacement + resubscribeRequestedReplacementPolicy = stricterReplacementPolicy( + resubscribeRequestedReplacementPolicy, + replacementPolicy + ) } return } @@ -1377,16 +1532,19 @@ export function createRemoteRuntimePtyTransport( } resubscribeEpoch = recoveryEpoch resubscribeRequestedHandle = null - resubscribeRequestedRequiresReplacement = false + resubscribeRequestedReplacementPolicy = 'reuse' let retryScheduled = false - void resubscribeAfterTransportClose(resubscribeHandle, requireReplacement, recoveryEpoch) + void resubscribeAfterTransportClose(resubscribeHandle, replacementPolicy, recoveryEpoch) .catch((error) => { if (!destroyed && connected && handle && recovery.isCurrent(recoveryEpoch)) { clearPendingViewportClaim() const clientError = toRemoteRuntimeClientErrorLike(error) if (isRecoverableRemoteRuntimeConnectionError(clientError)) { retryScheduled = recovery.schedule(recoveryEpoch, (nextEpoch) => { - scheduleResubscribeAfterTransportClose(requireReplacement, nextEpoch) + const currentReplacementPolicy = handle + ? getRecoveryReplacementPolicy(handle) + : 'reuse' + scheduleResubscribeAfterTransportClose(currentReplacementPolicy, nextEpoch) }) } else { recovery.cancel() @@ -1400,9 +1558,9 @@ export function createRemoteRuntimePtyTransport( } resubscribeEpoch = null const pendingHandle = resubscribeRequestedHandle - const pendingRequiresReplacement = resubscribeRequestedRequiresReplacement + const pendingReplacementPolicy = resubscribeRequestedReplacementPolicy resubscribeRequestedHandle = null - resubscribeRequestedRequiresReplacement = false + resubscribeRequestedReplacementPolicy = 'reuse' if ( !retryScheduled && recovery.isCurrent(recoveryEpoch) && @@ -1411,7 +1569,7 @@ export function createRemoteRuntimePtyTransport( pendingHandle === handle && !getCurrentMultiplexedStream(pendingHandle) ) { - scheduleResubscribeAfterTransportClose(pendingRequiresReplacement) + scheduleResubscribeAfterTransportClose(pendingReplacementPolicy) } }) } @@ -1428,11 +1586,14 @@ export function createRemoteRuntimePtyTransport( clearPendingViewportClaim() } recovery.schedule(recoveryEpoch, (nextEpoch) => { - scheduleResubscribeAfterTransportClose(false, nextEpoch) + scheduleResubscribeAfterTransportClose('reuse', nextEpoch) }) } - async function subscribeToHandle(expectedRecoveryEpoch?: number): Promise { + async function subscribeToHandle( + expectedRecoveryEpoch?: number, + sameHandleEndRecovery = false + ): Promise { if (!handle) { return } @@ -1495,10 +1656,13 @@ export function createRemoteRuntimePtyTransport( desiredOutputPaused, nextStream.setOutputPaused(desiredOutputPaused) ) + if (!subscriptionAttached && sameHandleEndRecovery) { + recordSameHandleEndReuse(subscribedHandle) + } subscriptionAttached = true setAttachmentReady(true) connecting = false - recoveryRequiresReplacement = false + resetRecoveryReplacementPolicy() recovery.markHealthy() emitRecoveryState() storedCallbacks.onConnect?.() @@ -1514,8 +1678,10 @@ export function createRemoteRuntimePtyTransport( multiplexedStream = null multiplexedStreamHandle = null clearPendingViewportClaim() - // Why: a HUB restart ends the old handle's stream before its replacement snapshot arrives; the HUB snapshot, not the paired viewer, decides whether the pane exited. - scheduleResubscribeAfterTransportClose(true) + // Why: repeated same-handle end/reuse cycles must eventually stop on a replacement boundary. + scheduleResubscribeAfterTransportClose( + replacementPolicyAfterWebStreamEnd(subscribedHandle) + ) return } unregisterShutdownHandlers(subscribedPtyId) @@ -1564,6 +1730,7 @@ export function createRemoteRuntimePtyTransport( multiplexedStream = null multiplexedStreamHandle = null setAttachmentReady(false) + resetSameHandleEndReuse() if (recoverable) { if (retryWithBackoff) { scheduleCapacityPressureRetry() @@ -1596,7 +1763,7 @@ export function createRemoteRuntimePtyTransport( multiplexedStreamHandle = subscribedHandle setAttachmentReady(subscriptionAttached) if (subscriptionAttached) { - recoveryRequiresReplacement = false + resetRecoveryReplacementPolicy() recovery.markHealthy() } // Why: a viewport change during the subscribe round-trip hit the no-op one-shot fallback; replay the latest viewport so the PTY isn't stuck at subscribe-time size. @@ -1629,7 +1796,8 @@ export function createRemoteRuntimePtyTransport( lastConnectOptions = options lastAttachOptions = null storedCallbacks = options.callbacks - recoveryRequiresReplacement = false + resetRecoveryReplacementPolicy() + resetSameHandleEndReuse() terminalEnded = false connecting = true emitRecoveryState(true) @@ -1846,7 +2014,8 @@ export function createRemoteRuntimePtyTransport( const generation = ++attachGeneration cancelTerminalCreateRetryWait() recovery.cancel() - recoveryRequiresReplacement = false + resetRecoveryReplacementPolicy() + resetSameHandleEndReuse() clearPublishedHandleWait() lastAttachOptions = options storedCallbacks = options.callbacks @@ -1944,7 +2113,8 @@ export function createRemoteRuntimePtyTransport( attachGeneration += 1 cancelTerminalCreateRetryWait() recovery.cancel() - recoveryRequiresReplacement = false + resetRecoveryReplacementPolicy() + resetSameHandleEndReuse() clearPublishedHandleWait() inputBatcher.flush() inputBatcher.clear() @@ -1975,7 +2145,8 @@ export function createRemoteRuntimePtyTransport( attachGeneration += 1 cancelTerminalCreateRetryWait() recovery.cancel() - recoveryRequiresReplacement = false + resetRecoveryReplacementPolicy() + resetSameHandleEndReuse() clearPublishedHandleWait() inputBatcher.flush() inputBatcher.clear() @@ -2033,7 +2204,9 @@ export function createRemoteRuntimePtyTransport( client: { id: clientId, type: 'desktop' }, ...(desiredViewport ? { viewport: desiredViewport, claimViewport: true as const } : {}) }).catch((error) => { - handleRemoteTerminalError(error) + if (handle === targetHandle) { + handleRemoteTerminalError(error) + } }) return true }, @@ -2134,7 +2307,7 @@ export function createRemoteRuntimePtyTransport( return false } const recoveryEpoch = recovery.begin() - scheduleResubscribeAfterTransportClose(recoveryRequiresReplacement, recoveryEpoch) + scheduleResubscribeAfterTransportClose(getRecoveryReplacementPolicy(handle), recoveryEpoch) return true }, diff --git a/src/renderer/src/components/terminal/active-terminal-repair-loop.react185.test.tsx b/src/renderer/src/components/terminal/active-terminal-repair-loop.react185.test.tsx new file mode 100644 index 00000000000..ab3ff3ac45e --- /dev/null +++ b/src/renderer/src/components/terminal/active-terminal-repair-loop.react185.test.tsx @@ -0,0 +1,327 @@ +/** @vitest-environment happy-dom */ +import { act, useMemo } from 'react' +import { createRoot } from 'react-dom/client' +import { afterEach, describe, expect, it } from 'vitest' +import { useAppStore } from '@/store' +import type { Tab, TabGroup, TerminalTab } from '../../../../shared/types' +import { useActiveTerminalRepair } from './use-active-terminal-repair' + +;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true + +// Why: React throws #185 at 51 nested commits; 400 proves divergence, not slowness. +const MAX_PASSES = 400 + +function terminalTab(id: string, worktreeId: string): TerminalTab { + return { id, worktreeId, title: id, createdAt: 0, sortOrder: 0 } as unknown as TerminalTab +} + +function unifiedTerminalTab( + id: string, + entityId: string, + worktreeId: string, + groupId: string +): Tab { + return { + id, + entityId, + worktreeId, + groupId, + contentType: 'terminal', + label: id, + customLabel: null, + color: null, + sortOrder: 0, + createdAt: 0 + } +} + +function tabGroup( + id: string, + worktreeId: string, + activeTabId: string, + tabOrder: string[] +): TabGroup { + return { id, worktreeId, activeTabId, tabOrder, recentTabIds: [activeTabId] } +} + +function RepairEffectHarness(): null { + const activeTabId = useAppStore((s) => s.activeTabId) + const activeTabIdByWorktree = useAppStore((s) => s.activeTabIdByWorktree) + const activeTabType = useAppStore((s) => s.activeTabType) + const tabsByWorktree = useAppStore((s) => s.tabsByWorktree) + const renderedActiveWorktreeId = useAppStore((s) => s.activeWorktreeId) + const setActiveTab = useAppStore((s) => s.setActiveTab) + const tabs = useMemo( + () => + renderedActiveWorktreeId !== null && Object.hasOwn(tabsByWorktree, renderedActiveWorktreeId) + ? tabsByWorktree[renderedActiveWorktreeId] + : [], + [renderedActiveWorktreeId, tabsByWorktree] + ) + + useActiveTerminalRepair({ + activeTabId, + activeTabType, + setActiveTab, + tabs, + activeTabIdByWorktree, + renderedActiveWorktreeId + }) + return null +} + +let cleanup: (() => void) | null = null + +afterEach(() => { + cleanup?.() + cleanup = null +}) + +function measureRepairPasses(): number { + let passes = 0 + const setActiveTab = useAppStore.getState().setActiveTab + useAppStore.setState({ + setActiveTab: (tabId) => { + passes += 1 + if (passes <= MAX_PASSES) { + setActiveTab(tabId) + } + } + }) + const container = document.createElement('div') + document.body.appendChild(container) + const root = createRoot(container) + cleanup = () => { + act(() => root.unmount()) + useAppStore.setState({ setActiveTab }) + container.remove() + } + act(() => { + root.render() + }) + return passes +} + +describe('active-terminal repair effect cannot drive a React #185 update loop', () => { + it('settles when the repaired tab is owned by the active worktree', () => { + useAppStore.setState({ + activeWorktreeId: 'wt-active', + activeTabType: 'terminal', + activeTabId: 'stale-tab', + activeTabIdByWorktree: {}, + tabsByWorktree: { 'wt-active': [terminalTab('t1', 'wt-active')] }, + unifiedTabsByWorktree: {} + }) + expect(measureRepairPasses()).toBeLessThan(10) + expect(useAppStore.getState().activeTabId).toBe('t1') + }) + + it('settles when another worktree reuses the tab id and is scanned first', () => { + // Why regression: first-match ownership skipped activeTabId while reallocating + // activeTabIdByWorktree, retriggering the repair effect indefinitely. + useAppStore.setState({ + activeWorktreeId: 'wt-active', + activeTabType: 'terminal', + activeTabId: 'stale-tab', + activeTabIdByWorktree: {}, + tabsByWorktree: { + 'wt-other': [terminalTab('t1', 'wt-other')], + 'wt-active': [terminalTab('t1', 'wt-active')] + }, + unifiedTabsByWorktree: {} + }) + expect(measureRepairPasses()).toBeLessThan(10) + // Why: settling by refusing to write would leave the repair permanently + // unsatisfied — quiet, but with activeTabId stuck on a tab that is gone. + expect(useAppStore.getState().activeTabId).toBe('t1') + expect(useAppStore.getState().activeTabIdByWorktree['wt-active']).toBe('t1') + }) + + it('activates the active worktree unified tab when another worktree reuses the entity id', () => { + const otherTab = unifiedTerminalTab('t1', 't1', 'wt-other', 'g-other') + const otherPreviousTab = unifiedTerminalTab( + 'other-previous', + 'other-previous', + 'wt-other', + 'g-other' + ) + const activeTab = unifiedTerminalTab('t1', 't1', 'wt-active', 'g-active') + const previousActiveTab = unifiedTerminalTab('u-previous', 't2', 'wt-active', 'g-active') + useAppStore.setState({ + activeWorktreeId: 'wt-active', + activeTabId: 't2', + activeTabIdByWorktree: { 'wt-active': 't2' }, + tabsByWorktree: { + 'wt-other': [terminalTab('t1', 'wt-other')], + 'wt-active': [terminalTab('t1', 'wt-active'), terminalTab('t2', 'wt-active')] + }, + unifiedTabsByWorktree: { + 'wt-other': [otherTab, otherPreviousTab], + 'wt-active': [activeTab, previousActiveTab] + }, + groupsByWorktree: { + 'wt-other': [ + tabGroup('g-other', 'wt-other', otherPreviousTab.id, [otherTab.id, otherPreviousTab.id]) + ], + 'wt-active': [ + tabGroup('g-active', 'wt-active', previousActiveTab.id, [ + activeTab.id, + previousActiveTab.id + ]) + ] + }, + activeGroupIdByWorktree: { 'wt-other': 'g-other', 'wt-active': 'g-active' } + }) + + act(() => { + useAppStore.getState().setActiveTab('t1') + }) + + expect(useAppStore.getState().groupsByWorktree['wt-active'][0].activeTabId).toBe(activeTab.id) + expect(useAppStore.getState().groupsByWorktree['wt-other'][0].activeTabId).toBe( + otherPreviousTab.id + ) + }) + + it('keeps unified-only terminal activation as a fallback', () => { + const targetTab = unifiedTerminalTab('u-target', 't1', 'wt-active', 'g-active') + const previousTab = unifiedTerminalTab('u-previous', 't2', 'wt-active', 'g-active') + useAppStore.setState({ + activeWorktreeId: 'wt-active', + activeTabId: null, + activeTabIdByWorktree: {}, + tabsByWorktree: {}, + unifiedTabsByWorktree: { 'wt-active': [targetTab, previousTab] }, + groupsByWorktree: { + 'wt-active': [ + tabGroup('g-active', 'wt-active', previousTab.id, [targetTab.id, previousTab.id]) + ] + }, + activeGroupIdByWorktree: { 'wt-active': 'g-active' } + }) + + act(() => { + useAppStore.getState().setActiveTab('t1') + }) + + expect(useAppStore.getState().groupsByWorktree['wt-active'][0].activeTabId).toBe(targetTab.id) + expect(useAppStore.getState().activeTabId).toBeNull() + }) + + it('does not reallocate activeTabIdByWorktree when the tab is already active', () => { + // Why: that map is a dependency of both the repair effect and the parked + // watcher sync, so a redundant activation must not re-run either. + useAppStore.setState({ + activeWorktreeId: 'wt-active', + activeTabType: 'terminal', + activeTabId: 't1', + activeTabIdByWorktree: {}, + tabsByWorktree: { 'wt-active': [terminalTab('t1', 'wt-active')] }, + unifiedTabsByWorktree: {} + }) + act(() => { + useAppStore.getState().setActiveTab('t1') + }) + const settled = useAppStore.getState().activeTabIdByWorktree + act(() => { + useAppStore.getState().setActiveTab('t1') + }) + expect(useAppStore.getState().activeTabIdByWorktree).toBe(settled) + }) + + it('keeps bell attribution off a background worktree tab', () => { + useAppStore.setState({ + activeWorktreeId: 'wt-active', + activeTabType: 'terminal', + activeTabId: 'visible-tab', + activeTabIdByWorktree: {}, + tabsByWorktree: { + 'wt-active': [terminalTab('visible-tab', 'wt-active')], + 'wt-background': [terminalTab('bg-tab', 'wt-background')] + }, + unifiedTabsByWorktree: {} + }) + act(() => { + useAppStore.getState().setActiveTab('bg-tab') + }) + expect(useAppStore.getState().activeTabId).toBe('visible-tab') + expect(useAppStore.getState().activeTabIdByWorktree['wt-background']).toBe('bg-tab') + }) + + it('records activation for a falsy-but-valid worktree id', () => { + useAppStore.setState({ + activeWorktreeId: '', + activeTabId: null, + activeTabIdByWorktree: {}, + tabsByWorktree: { '': [terminalTab('t1', '')] }, + unifiedTabsByWorktree: {} + }) + act(() => { + useAppStore.getState().setActiveTab('t1') + }) + expect(useAppStore.getState().activeTabId).toBe('t1') + expect(useAppStore.getState().activeTabIdByWorktree['']).toBe('t1') + }) + + it('repairs a falsy-but-valid active worktree id through the production hook', () => { + useAppStore.setState({ + activeWorktreeId: '', + activeTabType: 'terminal', + activeTabId: 'stale-tab', + activeTabIdByWorktree: { '': 't1' }, + tabsByWorktree: { '': [terminalTab('t1', '')] }, + unifiedTabsByWorktree: {} + }) + expect(measureRepairPasses()).toBeLessThan(10) + expect(useAppStore.getState().activeTabId).toBe('t1') + }) + + it('does not read inherited unified tabs for a prototype-named owner', () => { + useAppStore.setState({ + activeWorktreeId: 'toString', + activeTabId: null, + activeTabIdByWorktree: {}, + tabsByWorktree: { toString: [terminalTab('t1', 'toString')] }, + unifiedTabsByWorktree: {} + }) + expect(() => useAppStore.getState().setActiveTab('t1')).not.toThrow() + expect(useAppStore.getState().activeTabId).toBe('t1') + }) + + it('activates own unified tabs for a prototype-named owner', () => { + const target = unifiedTerminalTab('t1', 't1', 'toString', 'g-target') + const previous = unifiedTerminalTab('t2', 't2', 'toString', 'g-target') + useAppStore.setState({ + activeWorktreeId: 'toString', + activeTabId: 't2', + activeTabIdByWorktree: { toString: 't2' }, + tabsByWorktree: { + toString: [terminalTab('t1', 'toString'), terminalTab('t2', 'toString')] + }, + unifiedTabsByWorktree: { toString: [target, previous] }, + groupsByWorktree: { + toString: [tabGroup('g-target', 'toString', previous.id, [target.id, previous.id])] + }, + activeGroupIdByWorktree: { toString: 'g-target' } + }) + act(() => useAppStore.getState().setActiveTab('t1')) + expect(useAppStore.getState().groupsByWorktree.toString[0].activeTabId).toBe(target.id) + }) + + it('does not activate a tab with no owner when no worktree is active', () => { + useAppStore.setState({ + activeWorktreeId: null, + activeTabId: null, + activeTabIdByWorktree: {}, + tabsByWorktree: {}, + unifiedTabsByWorktree: {}, + unreadTerminalTabs: { 'missing-tab': true } + }) + act(() => { + useAppStore.getState().setActiveTab('missing-tab') + }) + expect(useAppStore.getState().activeTabId).toBeNull() + expect(useAppStore.getState().activeTabIdByWorktree).toEqual({}) + expect(useAppStore.getState().unreadTerminalTabs['missing-tab']).toBe(true) + }) +}) diff --git a/src/renderer/src/components/terminal/use-active-terminal-repair.ts b/src/renderer/src/components/terminal/use-active-terminal-repair.ts new file mode 100644 index 00000000000..d495c80c330 --- /dev/null +++ b/src/renderer/src/components/terminal/use-active-terminal-repair.ts @@ -0,0 +1,61 @@ +import { useEffect } from 'react' +import type { TerminalTab, WorkspaceVisibleTabType } from '../../../../shared/types' +import { resolveRepairedActiveTerminalTabId } from './active-terminal-repair' + +type ActiveTerminalRepairInput = { + activeTabType: WorkspaceVisibleTabType + activeTabId: string | null + activeTabIdByWorktree: Record + renderedActiveWorktreeId: string | null + setActiveTab: (tabId: string) => void + tabs: TerminalTab[] +} + +export function repairActiveTerminalTab({ + activeTabType, + activeTabId, + activeTabIdByWorktree, + renderedActiveWorktreeId, + setActiveTab, + tabs +}: ActiveTerminalRepairInput): boolean { + const rememberedTabId = + renderedActiveWorktreeId !== null && + Object.hasOwn(activeTabIdByWorktree, renderedActiveWorktreeId) + ? (activeTabIdByWorktree[renderedActiveWorktreeId] ?? null) + : null + const repairedTabId = resolveRepairedActiveTerminalTabId({ + activeTabType, + activeTabId, + rememberedTabId, + tabs + }) + if (!repairedTabId) { + return false + } + setActiveTab(repairedTabId) + return true +} + +export function useActiveTerminalRepair(input: ActiveTerminalRepairInput): void { + const { + activeTabId, + activeTabIdByWorktree, + activeTabType, + renderedActiveWorktreeId, + setActiveTab, + tabs + } = input + useEffect(() => { + repairActiveTerminalTab(input) + // Why: `tabs` is the dependency so repair reacts to order/content changes, not just scalar ids. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [ + activeTabId, + activeTabType, + setActiveTab, + tabs, + activeTabIdByWorktree, + renderedActiveWorktreeId + ]) +} diff --git a/src/renderer/src/hooks/remote-workspace-snapshot-duplicate-tab-repair.test.ts b/src/renderer/src/hooks/remote-workspace-snapshot-duplicate-tab-repair.test.ts new file mode 100644 index 00000000000..2664475db9e --- /dev/null +++ b/src/renderer/src/hooks/remote-workspace-snapshot-duplicate-tab-repair.test.ts @@ -0,0 +1,224 @@ +/** + * A direct-SSH snapshot can retain the same tab under old and new worktree IDs + * after a path or repo-ID change. This exercises that hydration path and proves + * active-tab repair converges; it deliberately does not remove the duplicate or + * reproduce React's scheduler-level #185 throw. See PR #11950 for that evidence. + */ +import { describe, expect, it, vi } from 'vitest' +import type * as AgentStatusModule from '@/lib/agent-status' +import type { RemoteWorkspaceSnapshot } from '../../../shared/remote-workspace-types' +import type { DirectSshAuthority, SshProviderEpoch } from '../../../shared/ssh-types' +import { createTestStore, makeWorktree } from '../store/slices/store-test-helpers' +import { applyDirectSshRemoteWorkspaceSnapshot } from './remote-workspace-snapshot-apply' +import type { DirectSshSnapshotApplyToken } from './direct-ssh-reconnect-coordinator-types' +import { repairActiveTerminalTab } from '../components/terminal/use-active-terminal-repair' + +vi.mock('sonner', () => ({ toast: { info: vi.fn(), success: vi.fn(), error: vi.fn() } })) +vi.mock('@/lib/agent-status', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, detectAgentStatusFromTitle: vi.fn().mockReturnValue(null) } +}) + +const TARGET_ID = 'ssh-target-1' +const OLD_PATH = '/srv/proj/wt' +const NEW_PATH = '/srv/proj/wt-renamed' +const OLD_ID = `repoA::${OLD_PATH}` +const NEW_ID = `repoA::${NEW_PATH}` +// Why a cap and not a while(true): on unfixed code this cycle never terminates. +const MAX_REPAIR_PASSES = 200 + +const authority: DirectSshAuthority = { + targetId: TARGET_ID, + providerEpoch: 'provider-epoch-1' as SshProviderEpoch, + connectionGeneration: 1 +} + +function token(snapshotRevision: number): DirectSshSnapshotApplyToken { + return { + authority, + catalogRevision: 0, + repoFingerprint: 'fp', + authorityRequirement: 'required', + snapshotRevision, + outcome: 'complete' + } +} + +function snapshot( + revision: number, + worktreePath: string, + tabIds: readonly string[], + activeTabId: string | null +): RemoteWorkspaceSnapshot { + return { + namespace: 'workspace', + revision, + updatedAt: revision, + schemaVersion: 1, + session: { + activeWorktreePath: worktreePath, + activeTabId, + tabsByWorktreePath: { + [worktreePath]: tabIds.map((tabId, index) => ({ + id: tabId, + worktreePath, + ptyId: `pty-${tabId}`, + title: `Terminal ${index + 1}`, + customTitle: null, + color: null, + sortOrder: index, + createdAt: index + 1 + })) + }, + terminalLayoutsByTabId: {}, + activeWorktreePathsOnShutdown: [], + activeTabIdByWorktreePath: { [worktreePath]: activeTabId }, + remoteSessionIdsByTabId: Object.fromEntries(tabIds.map((id) => [id, `pty-${id}`])), + lastVisitedAtByWorktreePath: { [worktreePath]: revision }, + defaultTerminalTabsAppliedByWorktreePath: { [worktreePath]: true } + } + } satisfies RemoteWorkspaceSnapshot +} + +type TestStore = ReturnType + +async function applySnapshot(store: TestStore, snap: RemoteWorkspaceSnapshot): Promise { + await applyDirectSshRemoteWorkspaceSnapshot({ + store, + snapshot: snap, + token: token(snap.revision), + arrival: 1, + isArrivalCurrent: () => true, + isPreparationTokenCurrent: () => true, + waitForWorkspaceSessionReady: async () => true, + finalizeHydratedTerminals: () => 0 + }) +} + +function worktreeIdsOwningTab(store: TestStore, tabId: string): string[] { + return Object.entries(store.getState().tabsByWorktree) + .filter(([, tabs]) => tabs.some((tab) => tab.id === tabId)) + .map(([worktreeId]) => worktreeId) +} + +function seedCatalog(store: TestStore, worktreePath: string): void { + store.setState({ + worktreesByRepo: { + repoA: [ + makeWorktree({ + id: `repoA::${worktreePath}`, + repoId: 'repoA', + path: worktreePath, + hostId: `ssh:${TARGET_ID}` + } as never) + ] + } + }) +} + +/** + * One turn of the loop in Terminal.tsx's active-terminal repair effect: + * recompute the repaired id from live state, then activate it. Reports how many + * turns it took to stop and how often `activeTabIdByWorktree` — a declared dep + * of that effect, so a fresh identity re-runs it — was reallocated. + */ +function runRepairCycle(store: TestStore): { + converged: boolean + passes: number + depIdentityChurn: number +} { + let passes = 0 + let depIdentityChurn = 0 + for (; passes < MAX_REPAIR_PASSES; passes += 1) { + const live = store.getState() + const depsBefore = live.activeTabIdByWorktree + const repaired = repairActiveTerminalTab({ + activeTabType: 'terminal', + activeTabId: live.activeTabId, + activeTabIdByWorktree: live.activeTabIdByWorktree, + renderedActiveWorktreeId: live.activeWorktreeId, + setActiveTab: live.setActiveTab, + tabs: live.activeWorktreeId ? (live.tabsByWorktree[live.activeWorktreeId] ?? []) : [] + }) + if (!repaired) { + return { converged: true, passes, depIdentityChurn } + } + if (store.getState().activeTabIdByWorktree !== depsBefore) { + depIdentityChurn += 1 + } + } + return { converged: false, passes, depIdentityChurn } +} + +describe('direct-SSH snapshot apply, tab id owned by two worktrees', () => { + it('converges the active-terminal repair instead of re-running it forever', async () => { + const store = createTestStore() + + store.setState({ + repos: [ + { + id: 'repoA', + path: '/srv/proj', + displayName: 'Proj', + badgeColor: '#000', + addedAt: 0, + connectionId: TARGET_ID + } as never + ], + // Load-bearing, do not drop: the IPC attach is the only thing stubbed, and + // it leaves behind exactly what a real reconnect leaves behind — one + // registered live PTY per tab. Without that the orphan sweep on the next + // worktree visit treats the duplicated tab as dead, cleans it up, and the + // bug evaporates before the repair effect ever sees it. + reconnectPersistedTerminals: (async () => { + const live = store.getState() + const registered: Record = { ...live.ptyIdsByTabId } + for (const tabs of Object.values(live.tabsByWorktree)) { + for (const tab of tabs) { + registered[tab.id] = [`pty-${tab.id}`] + } + } + store.setState({ ptyIdsByTabId: registered }) + }) as never, + markRemoteWorkspaceHydrated: (() => {}) as never, + setRemoteWorkspaceSyncStatus: (() => {}) as never + }) + + seedCatalog(store, OLD_PATH) + await applySnapshot(store, snapshot(1, OLD_PATH, ['tab-1', 'tab-2'], 'tab-1')) + store.getState().setActiveWorktree(OLD_ID) + + // The worktree is renamed on the host; the catalog re-detects it at the new + // path, so the worktree id changes while the tab ids do not. + seedCatalog(store, NEW_PATH) + await applySnapshot(store, snapshot(2, NEW_PATH, ['tab-1', 'tab-2'], 'tab-1')) + store.getState().setActiveWorktree(NEW_ID) + + // The remote deselects; importRemoteWorkspaceSession nulls an activeTabId it + // cannot find among the imported tabs, which is what arms the repair effect. + await applySnapshot(store, snapshot(3, NEW_PATH, ['tab-1', 'tab-2'], null)) + + // Why assert the precondition and not its removal: the fix stops the owner + // resolver being fooled by the duplicate, it does not remove the duplicate. + // Pinned so the test cannot pass vacuously if hydration stops producing one. + expect(worktreeIdsOwningTab(store, 'tab-1')).toEqual([OLD_ID, NEW_ID]) + expect(store.getState().activeTabId).toBeNull() + + const repair = runRepairCycle(store) + + expect(repair.converged).toBe(true) + expect(repair.passes).toBeLessThanOrEqual(store.getState().tabsByWorktree[NEW_ID].length) + expect(store.getState().activeTabId).toBe('tab-1') + const activeGroupId = store.getState().activeGroupIdByWorktree[NEW_ID] + expect( + store.getState().groupsByWorktree[NEW_ID].find((group) => group.id === activeGroupId) + ?.activeTabId + ).toBe('tab-1') + + // The dep identity settles: re-running the effect body after convergence + // reallocates nothing, so the effect does not schedule itself again. + const settled = runRepairCycle(store) + expect(settled.depIdentityChurn).toBe(0) + expect(settled.passes).toBe(0) + }) +}) diff --git a/src/renderer/src/store/slices/active-tab-owner-worktree.test.ts b/src/renderer/src/store/slices/active-tab-owner-worktree.test.ts new file mode 100644 index 00000000000..7e8c092a6c7 --- /dev/null +++ b/src/renderer/src/store/slices/active-tab-owner-worktree.test.ts @@ -0,0 +1,125 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { TerminalTab } from '../../../../shared/types' + +const recordRendererCrashBreadcrumb = vi.fn() +vi.mock('../../lib/crash-breadcrumb-recorder', () => ({ + recordRendererCrashBreadcrumb: (...args: unknown[]) => recordRendererCrashBreadcrumb(...args) +})) + +const { resolveActiveTabOwnerWorktreeId, _resetDuplicateTabOwnerBreadcrumbsForTests } = + await import('./active-tab-owner-worktree') + +function tab(id: string, worktreeId: string): TerminalTab { + return { id, worktreeId, title: id, createdAt: 0, sortOrder: 0 } as unknown as TerminalTab +} + +beforeEach(() => { + recordRendererCrashBreadcrumb.mockClear() + _resetDuplicateTabOwnerBreadcrumbsForTests() +}) + +describe('resolveActiveTabOwnerWorktreeId', () => { + it('returns the sole owner and stays quiet', () => { + const owner = resolveActiveTabOwnerWorktreeId( + { 'wt-a': [tab('t1', 'wt-a')], 'wt-b': [tab('t2', 'wt-b')] }, + 'wt-a', + 't1' + ) + expect(owner).toBe('wt-a') + expect(recordRendererCrashBreadcrumb).not.toHaveBeenCalled() + }) + + it('returns null when no worktree owns the tab', () => { + expect(resolveActiveTabOwnerWorktreeId({ 'wt-a': [tab('t1', 'wt-a')] }, 'wt-a', 'gone')).toBe( + null + ) + }) + + it('prefers the active worktree over an earlier-scanned duplicate', () => { + const owner = resolveActiveTabOwnerWorktreeId( + { 'wt-other': [tab('t1', 'wt-other')], 'wt-active': [tab('t1', 'wt-active')] }, + 'wt-active', + 't1' + ) + expect(owner).toBe('wt-active') + expect(recordRendererCrashBreadcrumb).toHaveBeenCalledWith( + 'terminal_tab_id_owned_by_multiple_worktrees', + { ownerCount: 2, resolvedToActiveWorktree: true } + ) + }) + + it('falls back to first match when the active worktree is not an owner', () => { + const owner = resolveActiveTabOwnerWorktreeId( + { 'wt-x': [tab('t1', 'wt-x')], 'wt-y': [tab('t1', 'wt-y')] }, + 'wt-active', + 't1' + ) + expect(owner).toBe('wt-x') + expect(recordRendererCrashBreadcrumb).toHaveBeenCalledWith( + 'terminal_tab_id_owned_by_multiple_worktrees', + { ownerCount: 2, resolvedToActiveWorktree: false } + ) + }) + + // Why: a truthiness guard on the active id would drop this back to first-match. + it('prefers a falsy-but-valid active worktree id', () => { + const owner = resolveActiveTabOwnerWorktreeId( + { 'wt-other': [tab('t1', 'wt-other')], '': [tab('t1', '')] }, + '', + 't1' + ) + expect(owner).toBe('') + }) + + it('breadcrumbs a given tab id once per verdict so it cannot flood the ring', () => { + const maps = { 'wt-a': [tab('t1', 'wt-a')], 'wt-b': [tab('t1', 'wt-b')] } + for (let i = 0; i < 5; i += 1) { + resolveActiveTabOwnerWorktreeId(maps, 'wt-a', 't1') + } + expect(recordRendererCrashBreadcrumb).toHaveBeenCalledTimes(1) + }) + + // Why: the active worktree changes under a persisting duplicate, and coalescing + // keeps only the newest payload — keyed on the tab id alone, whichever verdict + // a tab reported first would suppress the other for the rest of the session. + it('still reports a non-converging verdict after that tab id reported a converging one', () => { + const maps = { 'wt-other': [tab('t1', 'wt-other')], 'wt-active': [tab('t1', 'wt-active')] } + resolveActiveTabOwnerWorktreeId(maps, 'wt-active', 't1') + resolveActiveTabOwnerWorktreeId(maps, 'wt-third', 't1') + resolveActiveTabOwnerWorktreeId(maps, 'wt-third', 't1') + + expect(recordRendererCrashBreadcrumb.mock.calls).toEqual([ + [ + 'terminal_tab_id_owned_by_multiple_worktrees', + { ownerCount: 2, resolvedToActiveWorktree: true } + ], + [ + 'terminal_tab_id_owned_by_multiple_worktrees', + { ownerCount: 2, resolvedToActiveWorktree: false } + ] + ]) + }) + + // Why the count and not just "reports twice": a guard keyed on the active + // worktree id passes the two tests above yet emits once per worktree, which is + // the flood this guard exists to prevent. + it('never exceeds two crumbs for one tab id however the active worktree moves', () => { + const maps = { 'wt-a': [tab('t1', 'wt-a')], 'wt-b': [tab('t1', 'wt-b')] } + const activeWorktreeIds = ['wt-a', 'wt-b', 'wt-c', '', 'wt-d', 'wt-a'] + for (let i = 0; i < 600; i += 1) { + resolveActiveTabOwnerWorktreeId(maps, activeWorktreeIds[i % activeWorktreeIds.length], 't1') + } + expect(recordRendererCrashBreadcrumb).toHaveBeenCalledTimes(2) + }) + + // Why: the guard set is never pruned and tab ids are minted per created tab. + it('stops recording once the per-session sample cap is reached', () => { + for (let i = 0; i < 400; i += 1) { + const id = `t-${i}` + const maps = { 'wt-a': [tab(id, 'wt-a')], 'wt-b': [tab(id, 'wt-b')] } + resolveActiveTabOwnerWorktreeId(maps, 'wt-a', id) + resolveActiveTabOwnerWorktreeId(maps, 'wt-c', id) + } + expect(recordRendererCrashBreadcrumb).toHaveBeenCalledTimes(256) + }) +}) diff --git a/src/renderer/src/store/slices/active-tab-owner-worktree.ts b/src/renderer/src/store/slices/active-tab-owner-worktree.ts new file mode 100644 index 00000000000..270c5d44f1c --- /dev/null +++ b/src/renderer/src/store/slices/active-tab-owner-worktree.ts @@ -0,0 +1,82 @@ +import type { TerminalTab } from '../../../../shared/types' +import { recordRendererCrashBreadcrumb } from '../../lib/crash-breadcrumb-recorder' + +const reportedDuplicateTabVerdicts = new Set() +// Why capped: this set is never pruned and each tab id adds up to two verdict +// keys. 256 keys cover 128–256 duplicated ids, enough evidence for a bundle. +const MAX_REPORTED_DUPLICATE_TAB_VERDICTS = 256 + +/** Test seam: the duplicate breadcrumb is once-per-tab-id-per-verdict per session. */ +export function _resetDuplicateTabOwnerBreadcrumbsForTests(): void { + reportedDuplicateTabVerdicts.clear() +} + +/** + * Resolve which worktree owns a terminal tab, preferring the active worktree. + * + * Why the preference: a stale map can leave one tab id under two worktrees, and + * attributing it to an arbitrary first match leaves `activeTabId` permanently + * unconvergeable — which strands Terminal's active-terminal repair effect in a + * self-retriggering loop (React #185). + */ +export function resolveActiveTabOwnerWorktreeId( + tabsByWorktree: Record, + activeWorktreeId: string | null, + tabId: string +): string | null { + let firstOwnerId: string | null = null + let ownerCount = 0 + // Why tracked in-loop rather than re-read by key: `tabsByWorktree[activeWorktreeId]` + // resolves inherited members for ids like `toString`, and `?.some` would then throw. + // Why the id and not a boolean: a falsy-but-valid active id ('') would fail a + // truthiness guard below and silently fall back to the first match — the very + // misattribution this function exists to remove. + let activeOwnerId: string | null = null + // Why keys and not entries: entries allocates a pair array per worktree on a path + // that runs per tab activation. Own keys stay safe to index by. + for (const worktreeId of Object.keys(tabsByWorktree)) { + const tabs = tabsByWorktree[worktreeId] + if (!tabs.some((tab) => tab.id === tabId)) { + continue + } + ownerCount += 1 + if (firstOwnerId === null) { + firstOwnerId = worktreeId + } + if (worktreeId === activeWorktreeId) { + activeOwnerId = worktreeId + } + } + + // Why breadcrumb: hydration can retain duplicates after a worktree id change, + // but current field reports predate this signal. + // Reading it: `ownerCount > 1` is the load-bearing datum; the verdict only + // hints at the caller. A sustained repair loop shows up as `true`, since that + // effect picks from the active worktree's own list — but it does not prove the + // caller, and the repair effect can emit `false` too: its closure holds the + // worktree from its render while this runs against live state, so a worktree + // switch landing in between (an earlier-flushed effect, or IPC before the + // passive flush) reattributes the tab. So `false` covers that race as well as + // a deliberate background activation such as jump-to-agent — discard neither. + // Why the verdict is in the guard key: it flips under a persisting duplicate, + // and coalescing keeps only the newest payload, so one would erase the other. + // Still at most two crumbs per tab id. + const resolvedToActiveWorktree = activeOwnerId !== null + const verdictKey = `${tabId}:${resolvedToActiveWorktree}` + if ( + ownerCount > 1 && + !reportedDuplicateTabVerdicts.has(verdictKey) && + reportedDuplicateTabVerdicts.size < MAX_REPORTED_DUPLICATE_TAB_VERDICTS + ) { + reportedDuplicateTabVerdicts.add(verdictKey) + recordRendererCrashBreadcrumb('terminal_tab_id_owned_by_multiple_worktrees', { + ownerCount, + resolvedToActiveWorktree + }) + } + + if (ownerCount > 1 && activeOwnerId !== null) { + return activeOwnerId + } + return firstOwnerId +} diff --git a/src/renderer/src/store/slices/tabs.ts b/src/renderer/src/store/slices/tabs.ts index 9574cd3a6d1..72e22adc136 100644 --- a/src/renderer/src/store/slices/tabs.ts +++ b/src/renderer/src/store/slices/tabs.ts @@ -120,7 +120,7 @@ export type TabsSlice = { entityId: string, contentType?: TabContentType ) => Tab | null - activateTab: (tabId: string, opts?: { preservePreview?: boolean }) => void + activateTab: (tabId: string, opts?: { preservePreview?: boolean; worktreeId?: string }) => void closeUnifiedTab: ( tabId: string, opts?: { recordInteraction?: boolean; terminalRetirementHandled?: boolean } @@ -826,7 +826,17 @@ export const createTabsSlice: StateCreator = (set, activateTab: (tabId, opts) => { set((state) => { - const found = findTabAndWorktree(state.unifiedTabsByWorktree, tabId) + const scopedWorktreeId = opts?.worktreeId + let found: ReturnType + if (scopedWorktreeId !== undefined) { + const scopedTabs = Object.hasOwn(state.unifiedTabsByWorktree, scopedWorktreeId) + ? state.unifiedTabsByWorktree[scopedWorktreeId] + : [] + const scopedTab = scopedTabs.find((tab) => tab.id === tabId) + found = scopedTab ? { tab: scopedTab, worktreeId: scopedWorktreeId } : null + } else { + found = findTabAndWorktree(state.unifiedTabsByWorktree, tabId) + } if (!found) { return {} } diff --git a/src/renderer/src/store/slices/terminals.ts b/src/renderer/src/store/slices/terminals.ts index 11fbb55c158..cb1b5809018 100644 --- a/src/renderer/src/store/slices/terminals.ts +++ b/src/renderer/src/store/slices/terminals.ts @@ -38,6 +38,7 @@ import { } from '../../../../shared/stable-pane-id' import { isValidHostTerminalTabId, isValidTerminalTabId } from '../../../../shared/terminal-tab-id' import { buildByIdIndex, buildWorktreeByIdIndex } from './worktree-by-id-index' +import { resolveActiveTabOwnerWorktreeId } from './active-tab-owner-worktree' import { isSameCodexRestartNoticeAccount } from './codex-restart-notice-account-identity' import { getRepoIdFromWorktreeId, @@ -1903,17 +1904,18 @@ export const createTerminalSlice: StateCreator }, setActiveTab: (tabId) => { + let tabOwnerWorktreeId: string | null = null set((s) => { // Why: focusing a terminal tab clears its bell, but only for the active worktree — clearing a not-yet-visible background tab (worktree activation / jump-to-agent) would swallow the signal. - let tabOwnerWorktreeId: string | null = null - for (const [wId, tabs] of Object.entries(s.tabsByWorktree)) { - if (tabs.some((t) => t.id === tabId)) { - tabOwnerWorktreeId = wId - break - } - } + tabOwnerWorktreeId = resolveActiveTabOwnerWorktreeId( + s.tabsByWorktree, + s.activeWorktreeId, + tabId + ) + const isActiveWorktreeTab = + tabOwnerWorktreeId !== null && tabOwnerWorktreeId === s.activeWorktreeId const nextUnreadTerminalTabs = - tabOwnerWorktreeId === s.activeWorktreeId && s.unreadTerminalTabs[tabId] + isActiveWorktreeTab && s.unreadTerminalTabs[tabId] ? (() => { const copy = { ...s.unreadTerminalTabs } delete copy[tabId] @@ -1921,20 +1923,37 @@ export const createTerminalSlice: StateCreator })() : s.unreadTerminalTabs // Why: only pin global activeTabId to active-worktree tabs — markTerminalTabUnread treats it as "the visible tab" and would swallow BELs on a background tab (e.g. jump-to-agent). - const isActiveWorktreeTab = tabOwnerWorktreeId === s.activeWorktreeId return { activeTabId: isActiveWorktreeTab ? tabId : s.activeTabId, - activeTabIdByWorktree: tabOwnerWorktreeId - ? { ...s.activeTabIdByWorktree, [tabOwnerWorktreeId]: tabId } - : s.activeTabIdByWorktree, + // Why: a redundant activation must not reallocate this map — Terminal's + // active-terminal repair effect depends on it, so a re-activation that + // can't converge activeTabId (tab id reused by an earlier-scanned + // worktree) would otherwise re-trigger itself into React error #185. + activeTabIdByWorktree: + tabOwnerWorktreeId !== null && s.activeTabIdByWorktree[tabOwnerWorktreeId] !== tabId + ? { ...s.activeTabIdByWorktree, [tabOwnerWorktreeId]: tabId } + : s.activeTabIdByWorktree, unreadTerminalTabs: nextUnreadTerminalTabs } }) - const item = Object.values(get().unifiedTabsByWorktree) - .flat() - .find((entry) => entry.contentType === 'terminal' && entry.entityId === tabId) + const state = get() + const ownerUnifiedTabs = + tabOwnerWorktreeId !== null && Object.hasOwn(state.unifiedTabsByWorktree, tabOwnerWorktreeId) + ? state.unifiedTabsByWorktree[tabOwnerWorktreeId] + : [] + // Why: a duplicated entity id must activate the same owner chosen above. + const item = + ownerUnifiedTabs.find( + (entry) => entry.contentType === 'terminal' && entry.entityId === tabId + ) ?? + Object.values(state.unifiedTabsByWorktree) + .flat() + .find((entry) => entry.contentType === 'terminal' && entry.entityId === tabId) if (item) { - get().activateTab(item.id) + state.activateTab( + item.id, + tabOwnerWorktreeId !== null ? { worktreeId: tabOwnerWorktreeId } : undefined + ) } }, diff --git a/src/shared/windows-batch-spawn.ts b/src/shared/windows-batch-spawn.ts index 4afddccae73..d8312d2ec27 100644 --- a/src/shared/windows-batch-spawn.ts +++ b/src/shared/windows-batch-spawn.ts @@ -39,9 +39,23 @@ function hasUnsafeWindowsBatchSyntax(value: string): boolean { return UNSAFE_WINDOWS_BATCH_SYNTAX.test(value) } +export type GetSpawnArgsForWindowsOptions = { + /** + * GUI launchers (Open In apps) should not leave a lingering Command Prompt. + * `start "" /B` returns immediately and keeps console-subsystem children of + * `.cmd`/`.bat` shims from allocating a fresh visible prompt window. + * + * Opt-in only: `start` re-parses the command line, so callers whose argv can + * carry quoted operands (VS Code `--remote` authorities and remote paths with + * spaces) must leave this off. + */ + detachedGui?: boolean +} + export function getSpawnArgsForWindows( command: string, - args: string[] + args: string[], + options: GetSpawnArgsForWindowsOptions = {} ): { spawnCmd: string; spawnArgs: string[] } { if (isWindowsBatchScript(command)) { for (const value of [command, ...args]) { @@ -51,6 +65,24 @@ export function getSpawnArgsForWindows( } // Why: separate argv entries let Node quote spaces without breaking cmd. + if (options.detachedGui) { + // Why: `start` launches a batch target through a nested `cmd /K`, which + // stays resident after the script ends — `/B` only suppresses a *new* + // console, so the shim leaks a hidden cmd.exe. Handing `start` an inner + // `cmd /d /c` makes that interpreter exit with the script. + // + // Window title must be an *empty argv entry* (`''`). libuv's Windows + // quoter turns empty into `""` on the CreateProcess command line — the + // empty title `start` requires so a later quoted path is not eaten as + // the title. The two-character string `'""'` is wrong: libuv re-escapes + // it to `"\"\""`. (Default ComSpec has no spaces, so the bad form often + // still "works"; quoted Program Files paths are where it breaks.) + const cmdExePath = getCmdExePath() + return { + spawnCmd: cmdExePath, + spawnArgs: ['/d', '/c', 'start', '', '/B', cmdExePath, '/d', '/c', command, ...args] + } + } return { spawnCmd: getCmdExePath(), spawnArgs: ['/d', '/c', command, ...args] } } return { spawnCmd: command, spawnArgs: args }