diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index e100def9346..d349ce7937f 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -73,6 +73,65 @@ index 1ec12f796a822c78fba9ad7f6448c3987e325c23..cec8b67aef02f8199e5606a0d257088b var DEFAULT_FILE = 'sh'; var DEFAULT_NAME = 'xterm'; var DESTROY_SOCKET_TIMEOUT_MS = 200; +diff --git a/lib/windowsTerminal.js b/lib/windowsTerminal.js +index 64afaa33ad620757445ba4e4f2d8c4dc8c840379..09366fc9cd77c89a631eb058761cbbc46123b13e 100644 +--- a/lib/windowsTerminal.js ++++ b/lib/windowsTerminal.js +@@ -141,18 +141,14 @@ var WindowsTerminal = /** @class */ (function (_super) { + WindowsTerminal.prototype.destroy = function () { +- var _this = this; +- this._deferNoArgs(function () { +- _this.kill(); +- }); ++ this.kill(); + }; + WindowsTerminal.prototype.kill = function (signal) { +- var _this = this; +- this._deferNoArgs(function () { +- if (signal) { +- throw new Error('Signals not supported on windows.'); +- } +- _this._close(); +- _this._agent.kill(); +- }); ++ if (signal) { ++ throw new Error('Signals not supported on windows.'); ++ } ++ // Shutdown must not wait for first output; silent shells can stay pre-ready forever. ++ this._deferreds = []; ++ this._close(); ++ this._agent.kill(); + }; + WindowsTerminal.prototype._deferNoArgs = function (deferredFn) { + var _this = this; +diff --git a/src/windowsTerminal.ts b/src/windowsTerminal.ts +index e42e73a93ab31f649fdccc234436f3ec8f7f67de..197c327e5dd09409a80e39dc3dce6ff6cc176075 100644 +--- a/src/windowsTerminal.ts ++++ b/src/windowsTerminal.ts +@@ -158,5 +158,3 @@ export class WindowsTerminal extends Terminal { + public destroy(): void { +- this._deferNoArgs(() => { +- this.kill(); +- }); ++ this.kill(); + } +@@ -164,9 +162,9 @@ export class WindowsTerminal extends Terminal { + public kill(signal?: string): void { +- this._deferNoArgs(() => { +- if (signal) { +- throw new Error('Signals not supported on windows.'); +- } +- this._close(); +- this._agent.kill(); +- }); ++ if (signal) { ++ throw new Error('Signals not supported on windows.'); ++ } ++ // Shutdown must not wait for first output; silent shells can stay pre-ready forever. ++ this._deferreds = []; ++ this._close(); ++ this._agent.kill(); + } diff --git a/lib/conpty_console_list_agent.js b/lib/conpty_console_list_agent.js index ccc111c9e03a4a661ccfd5d8e8f0ee699571b5dd..f92c6bef7d46dc35c941c87ef186aa46d8ed9c44 100644 --- a/lib/conpty_console_list_agent.js diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index f7d8172cb5f..1f3e27bb475 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -3314,6 +3314,91 @@ "No live Electron reload-survival assertion yet — the gate proves the main-side accounting reset, not the full renderer reattach paint." ], "demotionRule": "Demote or quarantine if the gate flakes once without a product bug or harness bug filed to the owner." + }, + { + "id": "terminal-platform.windows-pre-ready-shutdown", + "title": "Windows relay PTYs shut down before first output without losing ownership", + "maturity": "experimental", + "protection": "partial", + "owner": "terminal-runtime", + "layer": "dependency-contract", + "surfaces": [ + "terminal shutdown", + "Windows ConPTY", + "SSH relay", + "renderer teardown" + ], + "platforms": ["macos", "windows"], + "providers": ["ssh", "remote-runtime"], + "coveredPlatforms": ["macos"], + "coveredProviders": ["ssh"], + "coverageNotes": "A macOS unit harness loads the installed patched node-pty WindowsTerminal prototype without constructing native state, proving pre-ready kill/destroy dispatch immediately. Relay tests simulate win32 shutdown, one-shot ownership, and retry after native failure. The same relay contract serves paired web/mobile clients; no live Windows ConPTY or Windows-over-SSH run is recorded yet.", + "motivatingLinks": [ + "https://github.com/stablyai/orca/issues/8260", + "https://github.com/stablyai/orca/pull/8279" + ], + "invariant": "A Windows relay PTY closed before its first output executes exactly one no-argument node-pty shutdown immediately; Orca does not delete ownership for a native kill failure, and a later retry can still target the same managed PTY.", + "oracle": "Load the installed patched WindowsTerminal prototype in a pre-ready state and assert kill and destroy each close the terminal and native agent once with zero deferred work, while signal arguments still throw. In the relay, assert immediate, graceful, fallback, and handler-dispose paths issue at most one no-argument kill per request; injected native failures retain one managed PTY, and a successful retry clears it.", + "commands": [ + "pnpm exec vitest run --config config/vitest.config.ts src/relay/node-pty-windows-pre-ready-shutdown.test.ts src/relay/pty-handler.test.ts" + ], + "testFiles": [ + "src/relay/node-pty-windows-pre-ready-shutdown.test.ts", + "src/relay/pty-handler.test.ts" + ], + "assertionRefs": [ + { + "file": "src/relay/node-pty-windows-pre-ready-shutdown.test.ts", + "assertions": [ + "pre-ready kill and destroy each close the terminal and native agent exactly once without queueing deferred work", + "explicit Windows signal arguments still fail before closing the terminal" + ] + }, + { + "file": "src/relay/pty-handler.test.ts", + "assertions": [ + "Windows immediate, graceful/fallback, and relay-dispose shutdown issue one no-argument kill", + "a native kill failure retains one managed PTY and a later successful retry clears it" + ] + } + ], + "evidenceRuns": [ + { + "date": "2026-07-12", + "runner": "local", + "platform": "macos", + "command": "pnpm exec vitest run --config config/vitest.config.ts src/relay/node-pty-windows-pre-ready-shutdown.test.ts src/relay/pty-handler.test.ts", + "result": "passed", + "durationSeconds": 1, + "summary": "78 tests passed, including three installed-dependency pre-ready assertions and relay shutdown/retry call-count coverage." + } + ], + "runtimeBudget": { + "p95Seconds": 5, + "scope": "dependency and relay unit tests" + }, + "flakeHistory": { + "status": "unknown", + "evidence": "New deterministic unit gate; no CI history yet." + }, + "redGreenEvidence": { + "status": "partial", + "evidence": "Without the node-pty patch, the pre-ready dependency test observes one deferred callback and zero native kill calls. Without relay one-shot ownership, graceful fallback and dispose issue duplicate kills." + }, + "performanceBudget": { + "required": true, + "evidence": "Shutdown removes indefinite deferred retention and preserves at most one native kill and one existing graceful timer per PTY request; it adds no polling, listener, startup await, or steady-state work." + }, + "promotionCriteria": [ + "Run the deterministic command across the required CI platforms without flakes.", + "Attach a live Windows ConPTY close-before-first-output run for local and SSH relay shutdown.", + "Record saved red/green evidence from an unpatched installed dependency." + ], + "knownGaps": [ + "No live Windows ConPTY, Windows SSH relay, WSL, or packaged Electron run exercises close-before-first-output.", + "The installed-dependency harness bypasses native construction and proves dispatch/ownership shape, not Windows native handle release." + ], + "demotionRule": "Keep non-blocking or quarantine if the dependency harness becomes platform-sensitive or flakes without a product defect." } ] } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b48ad9b4f29..bf2c0e1c933 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -18,7 +18,7 @@ patchedDependencies: hash: c32044ff5e9bfa651090eedbec19a13b2343fca3cc163fa8eec04c97880a3e19 path: config/patches/@xterm__addon-webgl@0.20.0-beta.286.patch node-pty@1.1.0: - hash: 407ae07e1e0e2ff2e8b58696449c54c31e51d87535bc6aa4a7a7b0b561407282 + hash: c3e917380aab5b8d6b6dac26101ce6c991127dd07c6dcf5e23b16106b8f9e792 path: config/patches/node-pty@1.1.0.patch importers: @@ -60,7 +60,7 @@ importers: version: 3.3.1 node-pty: specifier: ^1.1.0 - version: 1.1.0(patch_hash=407ae07e1e0e2ff2e8b58696449c54c31e51d87535bc6aa4a7a7b0b561407282) + version: 1.1.0(patch_hash=c3e917380aab5b8d6b6dac26101ce6c991127dd07c6dcf5e23b16106b8f9e792) posthog-node: specifier: ^5.33.3 version: 5.33.3 @@ -11832,7 +11832,7 @@ snapshots: undici: 6.27.0 which: 6.0.1 - node-pty@1.1.0(patch_hash=407ae07e1e0e2ff2e8b58696449c54c31e51d87535bc6aa4a7a7b0b561407282): + node-pty@1.1.0(patch_hash=c3e917380aab5b8d6b6dac26101ce6c991127dd07c6dcf5e23b16106b8f9e792): dependencies: node-addon-api: 7.1.1 diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 2a961ad3c2d..4dcc451324a 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -476,11 +476,7 @@ function normalizeNodePtySpawnError(err: unknown): Error { function isPtyAlreadyGoneError(err: unknown): boolean { const message = err instanceof Error ? err.message : String(err) - return ( - isSshPtyNotFoundError(err) || - /Session not found/i.test(message) || - /Signals not supported on windows/i.test(message) - ) + return isSshPtyNotFoundError(err) || /Session not found/i.test(message) } function delay(ms: number): Promise { diff --git a/src/relay/node-pty-windows-pre-ready-shutdown.test.ts b/src/relay/node-pty-windows-pre-ready-shutdown.test.ts new file mode 100644 index 00000000000..967f0da41fa --- /dev/null +++ b/src/relay/node-pty-windows-pre-ready-shutdown.test.ts @@ -0,0 +1,61 @@ +import { createRequire } from 'node:module' +import { describe, expect, it, vi } from 'vitest' + +type WindowsTerminalHarness = { + _agent: { kill: () => void } + _close: () => void + _deferreds: { run: () => void }[] + _isReady: boolean + destroy: () => void + kill: (signal?: string) => void +} + +const require = createRequire(import.meta.url) +const { WindowsTerminal } = require('node-pty/lib/windowsTerminal') as { + WindowsTerminal: { prototype: WindowsTerminalHarness } +} + +function createPreReadyTerminal(): WindowsTerminalHarness { + const terminal = Object.create(WindowsTerminal.prototype) as WindowsTerminalHarness + terminal._agent = { kill: vi.fn() } + terminal._close = vi.fn() + terminal._deferreds = [] + terminal._isReady = false + return terminal +} + +describe('patched node-pty Windows pre-ready shutdown', () => { + it('executes kill immediately without waiting for first output', () => { + const terminal = createPreReadyTerminal() + const deferred = vi.fn() + terminal._deferreds = [{ run: deferred }] + + terminal.kill() + + expect(terminal._close).toHaveBeenCalledOnce() + expect(terminal._agent.kill).toHaveBeenCalledOnce() + expect(terminal._deferreds).toEqual([]) + expect(deferred).not.toHaveBeenCalled() + }) + + it('executes destroy immediately without waiting for first output', () => { + const terminal = createPreReadyTerminal() + const deferred = vi.fn() + terminal._deferreds = [{ run: deferred }] + + terminal.destroy() + + expect(terminal._close).toHaveBeenCalledOnce() + expect(terminal._agent.kill).toHaveBeenCalledOnce() + expect(terminal._deferreds).toEqual([]) + expect(deferred).not.toHaveBeenCalled() + }) + + it('rejects unsupported signals before closing the terminal', () => { + const terminal = createPreReadyTerminal() + + expect(() => terminal.kill('SIGTERM')).toThrow('Signals not supported on windows.') + expect(terminal._close).not.toHaveBeenCalled() + expect(terminal._agent.kill).not.toHaveBeenCalled() + }) +}) diff --git a/src/relay/pty-handler.test.ts b/src/relay/pty-handler.test.ts index 332828408bd..8618d859290 100644 --- a/src/relay/pty-handler.test.ts +++ b/src/relay/pty-handler.test.ts @@ -1081,6 +1081,134 @@ describe('PtyHandler', () => { expect(mockKill).toHaveBeenCalledWith('SIGKILL') }) + it('uses node-pty no-argument shutdown on Windows without changing signal delivery', async () => { + const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform') + const mockKill = vi.fn() + const destroy = vi.fn(() => mockKill()) + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + kill: mockKill, + destroy, + onData: vi.fn(), + onExit: vi.fn() + }) + + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + try { + await dispatcher.callRequest('pty.spawn', {}) + await dispatcher.callRequest('pty.sendSignal', { id: 'pty-1', signal: 'SIGWINCH' }) + await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) + + expect(mockKill.mock.calls).toEqual([['SIGWINCH'], []]) + expect(destroy).not.toHaveBeenCalled() + } finally { + if (platformDescriptor) { + Object.defineProperty(process, 'platform', platformDescriptor) + } + } + }) + + it('keeps Windows graceful shutdown and its fallback signal-free', async () => { + const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform') + const mockKill = vi.fn() + const destroy = vi.fn(() => mockKill()) + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + kill: mockKill, + destroy, + onData: vi.fn(), + onExit: vi.fn() + }) + + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + try { + await dispatcher.callRequest('pty.spawn', {}) + await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: false }) + vi.advanceTimersByTime(5000) + + expect(mockKill.mock.calls).toEqual([[]]) + expect(destroy).not.toHaveBeenCalled() + expect(handler.activePtyCount).toBe(0) + } finally { + if (platformDescriptor) { + Object.defineProperty(process, 'platform', platformDescriptor) + } + } + }) + + it('keeps a Windows PTY retryable when immediate native shutdown throws', async () => { + const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform') + const mockKill = vi + .fn() + .mockImplementationOnce(() => { + throw new Error('invalid ConPTY handle') + }) + .mockImplementationOnce(() => {}) + const destroy = vi.fn(() => mockKill()) + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + kill: mockKill, + destroy, + onData: vi.fn(), + onExit: vi.fn() + }) + + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + try { + await dispatcher.callRequest('pty.spawn', {}) + await expect( + dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) + ).rejects.toThrow('invalid ConPTY handle') + expect(handler.activePtyCount).toBe(1) + expect(destroy).not.toHaveBeenCalled() + + await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) + expect(mockKill.mock.calls).toEqual([[], []]) + expect(handler.activePtyCount).toBe(0) + } finally { + if (platformDescriptor) { + Object.defineProperty(process, 'platform', platformDescriptor) + } + } + }) + + it('does not arm Windows graceful fallback until native shutdown succeeds', async () => { + const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform') + const mockKill = vi + .fn() + .mockImplementationOnce(() => { + throw new Error('invalid ConPTY handle') + }) + .mockImplementationOnce(() => {}) + const destroy = vi.fn(() => mockKill()) + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + kill: mockKill, + destroy, + onData: vi.fn(), + onExit: vi.fn() + }) + + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + try { + await dispatcher.callRequest('pty.spawn', {}) + await expect( + dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: false }) + ).rejects.toThrow('invalid ConPTY handle') + vi.advanceTimersByTime(5000) + expect(handler.activePtyCount).toBe(1) + + await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: false }) + vi.advanceTimersByTime(5000) + expect(mockKill.mock.calls).toEqual([[], []]) + expect(handler.activePtyCount).toBe(0) + } finally { + if (platformDescriptor) { + Object.defineProperty(process, 'platform', platformDescriptor) + } + } + }) + it('throws for attach on nonexistent PTY', async () => { await expect(dispatcher.callRequest('pty.attach', { id: 'pty-999' })).rejects.toThrow( 'PTY "pty-999" not found' @@ -1775,6 +1903,33 @@ describe('PtyHandler', () => { ]) expect(handler.activePtyCount).toBe(0) }) + + it('disposes Windows PTYs with node-pty no-argument kills', async () => { + const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform') + const mockKill = vi.fn() + const destroy = vi.fn(() => mockKill()) + mockPtySpawn.mockReturnValue({ + ...mockPtyInstance, + kill: mockKill, + destroy, + onData: vi.fn(), + onExit: vi.fn() + }) + + Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) + try { + await dispatcher.callRequest('pty.spawn', {}) + handler.dispose() + + expect(mockKill).toHaveBeenCalledOnce() + expect(mockKill.mock.calls).toEqual([[]]) + expect(destroy).not.toHaveBeenCalled() + } finally { + if (platformDescriptor) { + Object.defineProperty(process, 'platform', platformDescriptor) + } + } + }) }) describe('attachIdentityMismatches', () => { diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index b4448a52c50..405f9837d1a 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -25,25 +25,18 @@ import { type ShellReadyScanState } from '../main/shell-ready-marker-scanner' -// Why: node-pty WindowsTerminal.kill(signal) throws "Signals not supported on -// windows." when called with any argument. Use this helper everywhere instead of -// calling .kill(signal) directly so the call is safe across all PTY backends. -function killPtyProcess(pty: IPty, signal: string): void { - try { - pty.kill(signal) - } catch (err) { - const message = err instanceof Error ? err.message : String(err) - if (/signals not supported/i.test(message)) { - // Windows PTY backend doesn't support signals — kill without a signal arg - // to terminate the process as best the platform allows. - try { - pty.kill() - } catch { - // Already dead or disposed; safe to ignore. - } - return +function killPtyForShutdown(managed: ManagedPty, signal: 'SIGTERM' | 'SIGKILL'): void { + // Why: node-pty's Windows backend rejects signal arguments; its no-arg kill + // is the equivalent shutdown operation for ConPTY. + if (process.platform === 'win32') { + if (!managed.windowsShutdownKillIssued) { + managed.pty.kill() + // Why: a native kill throw can leave the process alive. Mark success only + // after return so provider ownership remains retryable on failure. + managed.windowsShutdownKillIssued = true } - throw err + } else { + managed.pty.kill(signal) } } @@ -71,6 +64,7 @@ type ManagedPty = { buffered: string /** Timer for SIGKILL fallback after a graceful SIGTERM shutdown. */ killTimer?: ReturnType + windowsShutdownKillIssued?: boolean /** True once disposeManagedPty has run. Prevents double-dispose (onExit + an * explicit shutdown can both fire for the same PTY) and converts post-dispose * entry-point calls into a clean "not found" error instead of a silent no-op @@ -131,7 +125,9 @@ function disposeManagedPty(managed: ManagedPty): void { ;(managed.pty as unknown as { kill: (sig?: string) => void }).kill = () => {} } try { - ;(managed.pty as unknown as { destroy?: () => void }).destroy?.() + if (!managed.windowsShutdownKillIssued) { + ;(managed.pty as unknown as { destroy?: () => void }).destroy?.() + } } catch { /* swallow */ } @@ -732,11 +728,11 @@ export class PtyHandler { // response is discarded and no renderer can own this PTY. Shut it down // immediately so it does not linger as an unreachable remote shell. this.releaseStartupCommand(managed) - killPtyProcess(term, 'SIGTERM') + killPtyForShutdown(managed, 'SIGTERM') managed.killTimer = setTimeout(() => { const still = this.ptys.get(id) if (still && !still.disposed) { - killPtyProcess(still.pty, 'SIGKILL') + killPtyForShutdown(still, 'SIGKILL') // Why: stale-spawn cleanup has no client who will ever attach. If // SIGKILL's onExit is missed (kernel edge case, uninterruptible // sleep), the managed entry + ptmx fd would leak forever. Dispose @@ -861,7 +857,7 @@ export class PtyHandler { if (immediate) { this.releaseStartupCommand(managed) this.flushPtyOutput(id) - killPtyProcess(managed.pty, 'SIGKILL') + killPtyForShutdown(managed, 'SIGKILL') // Why: SIGKILL has already reaped the child; release the ptmx fd on the // same tick. Deferring to onExit leaves a window where the fd is live // with a dead child. Idempotent via the disposed guard — if onExit fires @@ -880,7 +876,7 @@ export class PtyHandler { this.clearPtyFlowState(id) } else { this.releaseStartupCommand(managed) - killPtyProcess(managed.pty, 'SIGTERM') + killPtyForShutdown(managed, 'SIGTERM') // Why: Some processes ignore SIGTERM (e.g. a hung child, a custom signal // handler). Without a SIGKILL fallback the PTY process would leak and the @@ -894,7 +890,7 @@ export class PtyHandler { managed.killTimer = setTimeout(() => { const still = this.ptys.get(id) if (still && !still.disposed) { - killPtyProcess(still.pty, 'SIGKILL') + killPtyForShutdown(still, 'SIGKILL') this.flushPtyOutput(id) // Why: emit pty.exit BEFORE disposeManagedPty sets disposed=true. // The natural onExit short-circuits on `managed.disposed`, so @@ -929,7 +925,7 @@ export class PtyHandler { if (!managed || managed.disposed) { throw new Error(`PTY "${id}" not found`) } - killPtyProcess(managed.pty, signal) + managed.pty.kill(signal) } private async getCwd(params: Record): Promise { @@ -1162,7 +1158,7 @@ export class PtyHandler { // ptmx fd release via disposeManagedPty is synchronous, so there is // no graceful-shutdown window to preserve at this point. try { - managed.pty.kill('SIGKILL') + killPtyForShutdown(managed, 'SIGKILL') } catch { /* child may already be dead */ } diff --git a/src/renderer/src/components/terminal-pane/TerminalPane.tsx b/src/renderer/src/components/terminal-pane/TerminalPane.tsx index 009ddd64be5..a3d0286efac 100644 --- a/src/renderer/src/components/terminal-pane/TerminalPane.tsx +++ b/src/renderer/src/components/terminal-pane/TerminalPane.tsx @@ -2387,17 +2387,9 @@ export default function TerminalPane({ if (frame !== null) { cancelAnimationFrame(frame) } - // Why: a single rAF fires while the browser may still be dispatching - // ResizeObserver notifications, which causes syncPaneTitleOverlayRects to - // trigger a React state update that re-fires the observer — producing a - // "ResizeObserver loop completed with undelivered notifications" error and - // a potential freeze. The double-rAF defers work to the next paint frame, - // after the current observation cycle is fully complete. frame = requestAnimationFrame(() => { - frame = requestAnimationFrame(() => { - frame = null - syncPaneTitleOverlayRects() - }) + frame = null + syncPaneTitleOverlayRects() }) } diff --git a/src/renderer/src/components/terminal-pane/pty-transport.test.ts b/src/renderer/src/components/terminal-pane/pty-transport.test.ts index fd2663b5a33..ec955d1123e 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.test.ts @@ -41,7 +41,7 @@ describe('createIpcPtyTransport', () => { write: vi.fn(), writeAccepted: vi.fn().mockResolvedValue(true), resize: vi.fn(), - kill: vi.fn(), + kill: vi.fn().mockResolvedValue(undefined), onData: vi.fn((callback: (payload: { id: string; data: string }) => void) => { onData = callback return () => {} @@ -1354,7 +1354,9 @@ describe('createIpcPtyTransport', () => { spawnControls.resolve = resolve }) const spawnMock = vi.fn().mockReturnValue(spawnPromise) - const killMock = vi.fn() + const killError = new Error('remote connection dropped') + const killMock = vi.fn().mockRejectedValue(killError) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) const onPtySpawn = vi.fn() ;(globalThis as { window: typeof window }).window = { @@ -1380,22 +1382,50 @@ describe('createIpcPtyTransport', () => { } } as unknown as typeof window - const transport = createIpcPtyTransport({ onPtySpawn }) - const connectPromise = transport.connect({ - url: '', - callbacks: {} - }) + try { + const transport = createIpcPtyTransport({ onPtySpawn }) + const connectPromise = transport.connect({ + url: '', + callbacks: {} + }) - transport.destroy?.() - if (!spawnControls.resolve) { - throw new Error('Expected spawn resolver to be captured') + transport.destroy?.() + if (!spawnControls.resolve) { + throw new Error('Expected spawn resolver to be captured') + } + spawnControls.resolve({ id: 'pty-late' }) + await connectPromise + await flushPtySideEffects() + + expect(killMock).toHaveBeenCalledWith('pty-late') + expect(warn).toHaveBeenCalledWith( + '[pty] Failed to stop PTY spawned after transport teardown', + killError + ) + expect(onPtySpawn).not.toHaveBeenCalled() + expect(transport.getPtyId()).toBeNull() + } finally { + warn.mockRestore() } - spawnControls.resolve({ id: 'pty-late' }) - await connectPromise + }) - expect(killMock).toHaveBeenCalledWith('pty-late') - expect(onPtySpawn).not.toHaveBeenCalled() - expect(transport.getPtyId()).toBeNull() + it('settles and diagnoses a rejected PTY kill after transport teardown', async () => { + const { createIpcPtyTransport } = await import('./pty-transport') + const killError = new Error('remote connection dropped') + const kill = vi.fn().mockRejectedValue(killError) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + window.api.pty.kill = kill + const transport = createIpcPtyTransport() + + try { + await transport.connect({ url: '', callbacks: {} }) + transport.disconnect() + await flushPtySideEffects() + + expect(warn).toHaveBeenCalledWith('[pty] Failed to stop disconnected PTY', killError) + } finally { + warn.mockRestore() + } }) it('unregisterPtyDataHandlers prevents final data burst from triggering notifications', async () => { diff --git a/src/renderer/src/components/terminal-pane/pty-transport.ts b/src/renderer/src/components/terminal-pane/pty-transport.ts index bfe45ad5a64..da7b5948900 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.ts @@ -732,7 +732,9 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra // If destroyed while spawn was in flight, kill the new pty and bail if (destroyed) { - window.api.pty.kill(spawnResult.id).catch(() => {}) + void window.api.pty.kill(spawnResult.id).catch((error) => { + console.warn('[pty] Failed to stop PTY spawned after transport teardown', error) + }) return } @@ -918,7 +920,9 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra inputWriteQueue.clear() if (ptyId) { const id = ptyId - window.api.pty.kill(id).catch(() => {}) + void window.api.pty.kill(id).catch((error) => { + console.warn('[pty] Failed to stop disconnected PTY', error) + }) connected = false ptyId = null unregisterPtyHandlers(id) diff --git a/src/renderer/src/components/terminal-pane/terminal-link-handlers.test.ts b/src/renderer/src/components/terminal-pane/terminal-link-handlers.test.ts index 3840c7c962a..c41e8731733 100644 --- a/src/renderer/src/components/terminal-pane/terminal-link-handlers.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-link-handlers.test.ts @@ -37,6 +37,7 @@ const runtimeEnvironmentTransportCallMock = vi.fn() const setActiveWorktreeMock = vi.fn() const createBrowserTabMock = vi.fn() const setPendingEditorRevealMock = vi.fn() +const recordBreadcrumbMock = vi.fn() const deps = { worktreeId: 'wt-1', worktreePath: '/tmp' } const storeState = { @@ -123,7 +124,8 @@ beforeEach(() => { pathExists: fsPathExistsMock, stat: statMock }, - runtimeEnvironments: { call: runtimeEnvironmentTransportCallMock } + runtimeEnvironments: { call: runtimeEnvironmentTransportCallMock }, + crashReports: { recordBreadcrumb: recordBreadcrumbMock } } }) vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback): number => { @@ -1406,6 +1408,46 @@ describe('createFilePathLinkProvider range bounds', () => { }) }) + it('settles a remote path probe failure as one no-links callback', async () => { + setPlatform('Macintosh') + vi.mocked(getConnectionId).mockReturnValue('ssh-one') + const error = new Error('remote connection dropped') + fsPathExistsMock.mockRejectedValueOnce(error).mockRejectedValueOnce(error) + const provider = createProviderSetup([makeBufferLine('missing.ts')], new Map()).provider + const firstCallback = vi.fn() + const secondCallback = vi.fn() + + provider.provideLinks(1, firstCallback) + await flushAsyncWork() + provider.provideLinks(1, secondCallback) + await flushAsyncWork() + + expect(firstCallback).toHaveBeenCalledWith(undefined) + expect(secondCallback).toHaveBeenCalledWith(undefined) + expect(recordBreadcrumbMock).toHaveBeenCalledOnce() + expect(recordBreadcrumbMock).toHaveBeenCalledWith({ + name: 'terminal_file_link_probe_error', + data: { errorName: 'Error', errorMessage: 'remote connection dropped' } + }) + }) + + it('preserves successful sibling links when one remote probe rejects', async () => { + setPlatform('Macintosh') + vi.mocked(getConnectionId).mockReturnValue('ssh-one') + fsPathExistsMock + .mockResolvedValueOnce(true) + .mockRejectedValueOnce(new Error('remote connection dropped')) + const provider = createProviderSetup([makeBufferLine('good.ts missing.ts')], new Map()).provider + + const links = await new Promise((resolve) => { + provider.provideLinks(1, (provided) => resolve(provided ?? [])) + }) + + expect(links.map((link) => link.text)).toContain('good.ts') + expect(links.map((link) => link.text)).not.toContain('missing.ts') + expect(recordBreadcrumbMock).toHaveBeenCalledOnce() + }) + it('opens a single-row file path from a direct modifier-click fallback', async () => { setPlatform('Macintosh') const pathExists = createDeferred() diff --git a/src/renderer/src/components/terminal-pane/terminal-link-handlers.ts b/src/renderer/src/components/terminal-pane/terminal-link-handlers.ts index a7bb69ecd75..614cca60ba7 100644 --- a/src/renderer/src/components/terminal-pane/terminal-link-handlers.ts +++ b/src/renderer/src/components/terminal-pane/terminal-link-handlers.ts @@ -28,6 +28,7 @@ import { readTerminalPathExistsCache, writeTerminalPathExistsCache } from './terminal-path-exists-cache' +import { createTerminalLinkProbeResultCollector } from './terminal-link-probe-results' import { getTerminalHtmlFileOpenHint, getTerminalOrcaFileOpenHint, @@ -95,6 +96,7 @@ export function createFilePathLinkProvider( openLinkHint: string ): ILinkProvider { const { startupCwd, managerRef, pathExistsCache, worktreeId, worktreePath } = deps + const collectProbeResults = createTerminalLinkProbeResultCollector() return { provideLinks: (bufferLineNumber, callback) => { const pane = managerRef.current?.getPanes().find((candidate) => candidate.id === paneId) @@ -121,7 +123,7 @@ export function createFilePathLinkProvider( return } - void Promise.all( + void Promise.allSettled( logicalLines.flatMap((logicalLine) => extractTerminalFileLinkCandidates(logicalLine.text).map( async (parsed): Promise => { @@ -214,7 +216,8 @@ export function createFilePathLinkProvider( } ) ) - ).then((resolvedLinks) => { + ).then((results) => { + const resolvedLinks = collectProbeResults(results) const latestFingerprints = new Set( buildCandidateLogicalLinesForBufferPosition(buffer, bufferLineNumber).map( (logicalLine) => logicalLine.fingerprint @@ -231,9 +234,6 @@ export function createFilePathLinkProvider( } callback(links.length > 0 ? links : undefined) }) - .catch(() => { - callback(undefined) - }) } } } diff --git a/src/renderer/src/components/terminal-pane/terminal-link-probe-results.ts b/src/renderer/src/components/terminal-pane/terminal-link-probe-results.ts new file mode 100644 index 00000000000..15b44c06c3b --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-link-probe-results.ts @@ -0,0 +1,19 @@ +import { recordRendererCrashBreadcrumb } from '@/lib/crash-breadcrumb-recorder' + +export function createTerminalLinkProbeResultCollector(): ( + results: PromiseSettledResult[] +) => T[] { + let failureRecorded = false + return (results) => { + const rejected = results.find((result) => result.status === 'rejected') + if (rejected && !failureRecorded) { + failureRecorded = true + const error = rejected.reason + recordRendererCrashBreadcrumb('terminal_file_link_probe_error', { + errorName: error instanceof Error ? error.name : typeof error, + errorMessage: error instanceof Error ? error.message : String(error) + }) + } + return results.flatMap((result) => (result.status === 'fulfilled' ? [result.value] : [])) + } +} diff --git a/src/renderer/src/lib/crash-diagnostics.ts b/src/renderer/src/lib/crash-diagnostics.ts index 7d0033763e5..bb0875f1433 100644 --- a/src/renderer/src/lib/crash-diagnostics.ts +++ b/src/renderer/src/lib/crash-diagnostics.ts @@ -64,15 +64,6 @@ if (typeof import.meta !== 'undefined' && import.meta.hot) { } function recordRendererError(event: ErrorEvent): void { - // Why: ResizeObserver loop errors are a Chromium/Electron browser quirk fired - // when a ResizeObserver callback causes layout changes mid-dispatch. They are - // benign and self-resolving; recording them fills the breadcrumb buffer and - // inflates the error count without adding diagnostic value. Suppress them - // here so they don't contribute to memory growth or false-positive crash traces. - if (/ResizeObserver loop/i.test(event.message)) { - event.preventDefault() - return - } recordRendererCrashBreadcrumb( 'renderer_error', compactBreadcrumbData({ diff --git a/src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts b/src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts index 15ca69fda3c..3f921690070 100644 --- a/src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts +++ b/src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts @@ -21,11 +21,11 @@ const repo1: Repo = { id: 'repo-1', path: '/r1', displayName: 'R1', badgeColor: const repo2: Repo = { id: 'repo-2', path: '/r2', displayName: 'R2', badgeColor: '#111', addedAt: 2 } const reposRemove = vi.fn().mockResolvedValue(undefined) -const ptyKill = vi.fn() +const ptyKill = vi.fn().mockResolvedValue(undefined) beforeEach(() => { reposRemove.mockReset().mockResolvedValue(undefined) - ptyKill.mockReset() + ptyKill.mockReset().mockResolvedValue(undefined) vi.stubGlobal('window', { api: { repos: { remove: reposRemove }, @@ -84,4 +84,25 @@ describe('removeProject purges per-worktree state (leak regression)', () => { expect(s.gitStatusHugeByWorktree[W2]).toEqual({ limit: 2000 }) expect(s.everActivatedWorktreeIds.has(W2)).toBe(true) }) + + it('purges project state after a rejected best-effort PTY kill', async () => { + const store = createTestStore() + seedTwoProjects(store) + store.setState({ + tabsByWorktree: { [W1]: [{ id: 'tab-1' } as never] }, + ptyIdsByTabId: { 'tab-1': ['pty-1'] } + }) + const error = new Error('remote connection dropped') + ptyKill.mockRejectedValueOnce(error) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + try { + await store.getState().removeProject(repo1.id) + + expect(store.getState().unifiedTabsByWorktree[W1]).toBeUndefined() + expect(warn).toHaveBeenCalledWith('[pty] Failed to stop PTY while removing project', error) + } finally { + warn.mockRestore() + } + }) }) diff --git a/src/renderer/src/store/slices/repos.ts b/src/renderer/src/store/slices/repos.ts index 8265419a952..24cf4302f9f 100644 --- a/src/renderer/src/store/slices/repos.ts +++ b/src/renderer/src/store/slices/repos.ts @@ -2814,7 +2814,9 @@ export const createRepoSlice: StateCreator = (set, for (const ptyId of get().ptyIdsByTabId[tab.id] ?? []) { killedPtyIds.add(ptyId) if (!ptyId.startsWith('remote:')) { - window.api.pty.kill(ptyId).catch(() => {}) + void Promise.resolve(window.api.pty.kill(ptyId)).catch((error) => { + console.warn('[pty] Failed to stop PTY while removing project', error) + }) } } }