From 786d7048a1adc36e631fced91ec105979916d752 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 1 Aug 2026 13:38:45 -0700 Subject: [PATCH 1/3] fix(win32): suppress Command Prompt window on IDE launches (#11907) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(win32): suppress Command Prompt window on IDE launches - Prefer JetBrains GUI executables (`*64.exe`) over `.cmd` shims to avoid console allocation (STA-3040). - Use `start "" /B` when launching GUI apps via batch scripts; shims chain through console helpers that allocate a visible prompt even with `windowsHide`. `start /B` returns immediately, preventing the lingering window. * fix(win32): suppress Command Prompt window on IDE launches Prevent lingering Command Prompt windows when launching JetBrains IDEs on Windows. Use `start "" /B cmd /d /c` so the nested shell exits with the batch script, but only for JetBrains shims—VS Code and Cursor keep the waiting form because `start` re-parses arguments and breaks remote paths with spaces. Prefer colocated `*64.exe` executables beside the resolved `.cmd` shim over PATH lookups to avoid stale installations. * fix(win32): extend IDE launcher console suppression to direct paths Support IDE paths stored directly in settings (e.g., idea.exe, webstorm.cmd). Detect console idea.exe stubs alongside batch shims for upgrade to GUI *64.exe. Fix start command title escaping: use empty string instead of '""' to prevent libuv re-quoting. --- src/main/editor-launcher-name.ts | 44 +++++ src/main/external-editor-launch.test.ts | 177 ++++++++++++++++- src/main/external-editor-launch.ts | 203 ++++++++++++++------ src/main/ipc/shell.test.ts | 162 +++++++++++----- src/main/ipc/shell.ts | 49 +---- src/main/jetbrains-windows-gui-launchers.ts | 91 +++++++++ src/main/win32-utils.test.ts | 49 +++++ src/main/win32-utils.ts | 3 +- src/shared/windows-batch-spawn.ts | 34 +++- 9 files changed, 653 insertions(+), 159 deletions(-) create mode 100644 src/main/editor-launcher-name.ts create mode 100644 src/main/jetbrains-windows-gui-launchers.ts 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/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/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 } From 036b1e78ba63b3e558064b7958025c942dc5974c Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 1 Aug 2026 13:45:49 -0700 Subject: [PATCH 2/3] fix(terminal): add replacement policies for repeated same-handle stream (#12003) Recovery logic strengthened with replacement-policy tiers (reuse/prefer-replacement/require-replacement) and bounded same-handle end cycles. Prevents infinite flapping by capping reuse attempts and inventory wait windows. Tracks ready evidence to reattach from prior snapshots when inventory becomes unavailable. --- .../remote-runtime-pty-transport.test.ts | 802 ++++++++++++++++++ .../remote-runtime-pty-transport.ts | 265 +++++- 2 files changed, 1021 insertions(+), 46 deletions(-) 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 }, From 36cc8495efecb7cb599a541d69bed6abe1b184b5 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 1 Aug 2026 13:53:14 -0700 Subject: [PATCH 3/3] fix(terminal): stop the active-terminal repair loop from tripping React #185 (#11950) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(terminals): make redundant tab activation idempotent (React #185) setActiveTab always reallocated activeTabIdByWorktree, even when the tab was already active for that worktree. Terminal's active-terminal repair effect depends on that map, so when the repair cannot converge activeTabId -- which happens when an earlier-scanned worktree reuses the tab id -- the effect re-triggers itself every commit until React throws #185. Crash cluster A: 12 reports, boundary terminal.workbench, 1.4.162/1.4.163. Co-authored-by: Orca * fix(terminals): converge activeTabId when a tab id is owned by two worktrees Prefer the active worktree when resolving a terminal tab's owner. First-match ownership left activeTabId permanently unconvergeable under a duplicated tab id, so the active-terminal repair effect re-triggered itself into React #185. Breadcrumb the duplicate-ownership state (once per tab id) so a crash bundle can prove or kill the production origin of the precondition. Co-authored-by: Orca * fix(crash-reporting): coalesce the duplicate-tab-owner breadcrumb Its renderer guard is once-per-tab-id, so the stale worktree map it exists to diagnose duplicates every tab id at once and could evict the whole 30-entry ring. Also drops two keyed re-reads of tabsByWorktree that would throw for a prototype-named worktree id, and pins the activeTabIdByWorktree guard with a test that fails without it. Co-authored-by: Orca * fix(crash-reporting): key the duplicate-tab-owner crumb on its convergence flag Name-only coalescing keeps only the newest payload, so a resolvedToActiveWorktree false sample — the one value saying the activation still could not converge — was erased by any later benign true in the same 30s window. Keys on the flag instead, mirroring the WebGL name:kind branch; two keys still bound the burst. Also: the previous coalescing commit had no test at all (removing the name from both sets broke zero of 2701 tests), the resolver's activeWorktreeId truthiness check was a hole rather than a guard for a '' active id, and the resolver test file failed oxfmt --check. Co-authored-by: Orca * fix(crash-reporting): keep the non-converging duplicate-tab verdict The two earlier commits contradicted each other. Splitting the coalesce key existed so a `false` verdict could not be erased by a later benign `true` — but the renderer guard was keyed on the tab id alone, so for any one id only the first verdict was ever emitted. A duplicated id that first resolves benignly, then stops converging when the user switches worktrees, dropped the `false` sample at the source. That sample is the whole reason the breadcrumb exists: it is the only value saying the activation could not converge activeTabId. Key the guard on id plus verdict. At most two crumbs per tab id, and the main process still folds each verdict into its own ring entry, so the flood bound is unchanged. * fix(crash): correct the duplicate-tab verdict rationale, pin and cap the guard Three comments said `false` is the verdict that matters because it is the only one showing the activation could not converge. That is backwards. The repair effect activates a tab drawn from tabsByWorktree[active], so the React #185 path can only ever emit `true`; `false` is what a deliberate jump-to-agent into a background worktree emits from a fully converged state. A reader of the next bundle would have discarded the exact sample the breadcrumb exists to capture. The mechanism was right, only its stated reason was wrong: the real justification for keying on the verdict is symmetric, since coalescing keeps only the newest payload and either verdict would erase the other. The suite also did not pin the "at most 2 per tab id" bound - a guard keyed on `${tabId}:${activeWorktreeId}` passed all 11 tests while emitting once per worktree, the storm the guard exists to prevent. Adds a count-pinning test that kills it. Caps the never-pruned guard set at 256 distinct verdict keys (~85KB), mirroring MAX_COALESCE_KEYS. Measured 330 B/entry; a realistic thousand duplicated tab ids is ~0.6MB, negligible but unbounded in principle. * perf(crash): scan worktree tabs by key, and soften the verdict rationale Round 6 corrected my own round-5 comment. I had written that `true` is the repair-loop signature and `false` covers a deliberate background activation. The repair effect can emit `false` too: its closure holds the worktree from its render while the guard runs against live state, so a worktree switch landing in between reattributes the tab. The verdict hints at the caller; it does not prove it, and neither value should be discarded. Comment-only. Also take the free scan win the perf review measured: Object.entries allocates a pair array per worktree on a path that runs per tab activation. Own keys are safe to index by, so Object.keys plus an indexed read is behaviour-identical (16.1us -> 5.0us at 170 worktrees x 10 tabs). * fix(terminal): keep a duplicated tab id from re-sorting the active worktree setActiveTab now prefers the active worktree when a tab id is held by more than one, but terminals.ts has a second, older owner resolver: getTerminalTabOwnerWorktreeId, a memoized map built last-writer-wins. Two of its callers — setRuntimePaneTitle and clearRuntimePaneTitle — use the result for the same "is this pane in the active worktree" gate, so under a duplicate the two resolvers disagree: the cache names whichever worktree it saw last, which can be a background one for a pane the user is looking at. The gate then fails open and every classified OSC title frame bumps sortEpoch, reinstating the click-driven sidebar re-sort #209 removed — 20 title frames measured 20 bumps, each one a store write that re-renders every sortEpoch subscriber. isTabInActiveWorktree answers from the active worktree's own tab list instead of a tie-break. It stays behind the cheap id equality so the common non-duplicated path is unchanged, and it is a hasOwn lookup plus one scan of that worktree's tabs rather than resolveActiveTabOwnerWorktreeId, whose full scan would run per title frame and whose breadcrumb would fold a second caller into one verdict. Leaves updateTabTitle and clearTabLaunchAgent on the cache: they pick which copy of a duplicated tab to mutate, where no answer is defensible until the duplication itself is fixed. * test(terminal): pin the SSH-hydration origin of the duplicate tab id Drives the duplicate from real hydration rather than constructing it: a direct-SSH snapshot is keyed by worktree path, so renaming the worktree on the host (or re-adding the repo, which mints a fresh id) re-resolves it to a new worktree id while replaceHydratedRecordKeys retains the old key verbatim. Nothing de-dupes across keys. Fails on unfixed origin/main with converged=false after 200 passes; the two precondition assertions pass on both sides, so the red is the non-convergence itself and not a setup divergence. The reconnectPersistedTerminals stub is load-bearing and marked as such: with no registered PTY the orphan sweep cleans the duplicate up before the repair effect sees it. * docs(test): record the end-to-end #185 reproduction method on the regression test Co-authored-by: Orca * test(terminal): pin the null-active-worktree guard in isTabInActiveWorktree Dropping the `activeWorktreeId === null` early return was killed by nothing: `Object.hasOwn(map, null)` coerces to the string key 'null', so a worktree literally named 'null' would answer for "no active worktree". An untested guard reads as dead code and gets deleted. * rm triage context * rm triage context * rm context files * refactor(terminal): extract repair logic into reusable hook and guard ag Extract the active terminal repair effect from Terminal.tsx into `useActiveTerminalRepair` hook to enable reuse in tests and clarify responsibilities. Replace falsy coercion guards (`obj[id] ?? []`) with explicit `Object.hasOwn()` checks to handle edge cases: empty-string worktree ids (valid but falsy), prototype-named ids like 'toString', and duplicated tab ids across worktrees. Remove the now-unused `isTabInActiveWorktree` helper. Simplify the isActive logic in terminals.ts to rely solely on owner-equality since the repair now uses proper membership checks. --------- Co-authored-by: Orca Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com> --- ...ash-reporting-renderer-breadcrumbs.test.ts | 43 +++ src/main/ipc/crash-reporting.ts | 7 + src/renderer/src/components/Terminal.tsx | 30 +- ...ive-terminal-repair-loop.react185.test.tsx | 327 ++++++++++++++++++ .../terminal/use-active-terminal-repair.ts | 61 ++++ ...pace-snapshot-duplicate-tab-repair.test.ts | 224 ++++++++++++ .../slices/active-tab-owner-worktree.test.ts | 125 +++++++ .../store/slices/active-tab-owner-worktree.ts | 82 +++++ src/renderer/src/store/slices/tabs.ts | 14 +- src/renderer/src/store/slices/terminals.ts | 51 ++- 10 files changed, 924 insertions(+), 40 deletions(-) create mode 100644 src/renderer/src/components/terminal/active-terminal-repair-loop.react185.test.tsx create mode 100644 src/renderer/src/components/terminal/use-active-terminal-repair.ts create mode 100644 src/renderer/src/hooks/remote-workspace-snapshot-duplicate-tab-repair.test.ts create mode 100644 src/renderer/src/store/slices/active-tab-owner-worktree.test.ts create mode 100644 src/renderer/src/store/slices/active-tab-owner-worktree.ts 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/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/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 + ) } },