mirror of
https://github.com/stablyai/orca.git
synced 2026-10-19 00:03:20 +00:00
fix: make renderer teardown failures safe and diagnosable
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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."
|
||||
}
|
||||
]
|
||||
}
|
||||
|
||||
Generated
+3
-3
@@ -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
|
||||
|
||||
|
||||
+1
-5
@@ -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<void> {
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
})
|
||||
@@ -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', () => {
|
||||
|
||||
+22
-26
@@ -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<typeof setTimeout>
|
||||
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<string, unknown>): Promise<string> {
|
||||
@@ -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 */
|
||||
}
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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<ILink[]>((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<boolean>()
|
||||
|
||||
@@ -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<ProvidedFileLink | null>()
|
||||
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<ProvidedFileLink | null> => {
|
||||
@@ -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)
|
||||
})
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,19 @@
|
||||
import { recordRendererCrashBreadcrumb } from '@/lib/crash-breadcrumb-recorder'
|
||||
|
||||
export function createTerminalLinkProbeResultCollector<T>(): (
|
||||
results: PromiseSettledResult<T>[]
|
||||
) => 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] : []))
|
||||
}
|
||||
}
|
||||
@@ -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({
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
@@ -2814,7 +2814,9 @@ export const createRepoSlice: StateCreator<AppState, [], [], RepoSlice> = (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)
|
||||
})
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user