mirror of
https://github.com/stablyai/orca.git
synced 2026-10-17 08:03:14 +00:00
fix: prevent renderer heap leak and white-screen crashes
Three independent root causes all produce unhandled Promise rejections
that accumulate in the renderer heap and eventually exhaust it:
1. node-pty Windows backend: WindowsTerminal.kill(signal) throws
"Signals not supported on windows." for any signal argument.
Add killPtyProcess() helper in pty-handler.ts that catches the
Windows signal error and falls back to kill() with no argument.
Update isPtyAlreadyGoneError() in main/ipc/pty.ts to classify
the Windows signal error as an already-gone PTY so it does not
re-throw and propagate to the renderer as an unhandled rejection.
2. Uncaught pty.kill() calls in renderer: three call sites in repos.ts,
pty-transport.ts fire-and-forget window.api.pty.kill() without
.catch(). On SSH disconnect or Windows PTY teardown these produce
unhandled rejections that the crash-diagnostics breadcrumb system
retains in memory without releasing. Add .catch(() => {}) to each.
3. ResizeObserver loop: TerminalPane.scheduleSync ran syncPaneTitleOverlayRects
inside a single requestAnimationFrame, which fires while the browser
is still dispatching ResizeObserver notifications. The resulting React
state update re-triggers the observer, producing a "ResizeObserver loop
completed" error on every resize and potentially freezing the renderer
thread. Switch to a double-rAF so the work defers to the next paint
frame after the observation cycle completes. Also suppress the
ResizeObserver loop ErrorEvent in crash-diagnostics.ts so that
any that slip through do not accumulate in the breadcrumb buffer.
Add .catch(() => callback(undefined)) to the Promise.all in
terminal-link-handlers.ts so SSH-disconnect errors don't flood
the unhandled-rejection count.
Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
+5
-1
@@ -476,7 +476,11 @@ 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)
|
||||
return (
|
||||
isSshPtyNotFoundError(err) ||
|
||||
/Session not found/i.test(message) ||
|
||||
/Signals not supported on windows/i.test(message)
|
||||
)
|
||||
}
|
||||
|
||||
function delay(ms: number): Promise<void> {
|
||||
|
||||
@@ -25,6 +25,28 @@ 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
|
||||
}
|
||||
throw err
|
||||
}
|
||||
}
|
||||
|
||||
// Why: node-pty is a native addon that may not be installed on the remote.
|
||||
// Dynamic import keeps the require() lazy so loadPty() returns null gracefully
|
||||
// when the native module is unavailable. The static type import lets vitest
|
||||
@@ -710,11 +732,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)
|
||||
term.kill('SIGTERM')
|
||||
killPtyProcess(term, 'SIGTERM')
|
||||
managed.killTimer = setTimeout(() => {
|
||||
const still = this.ptys.get(id)
|
||||
if (still && !still.disposed) {
|
||||
still.pty.kill('SIGKILL')
|
||||
killPtyProcess(still.pty, '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
|
||||
@@ -839,7 +861,7 @@ export class PtyHandler {
|
||||
if (immediate) {
|
||||
this.releaseStartupCommand(managed)
|
||||
this.flushPtyOutput(id)
|
||||
managed.pty.kill('SIGKILL')
|
||||
killPtyProcess(managed.pty, '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
|
||||
@@ -858,7 +880,7 @@ export class PtyHandler {
|
||||
this.clearPtyFlowState(id)
|
||||
} else {
|
||||
this.releaseStartupCommand(managed)
|
||||
managed.pty.kill('SIGTERM')
|
||||
killPtyProcess(managed.pty, '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
|
||||
@@ -872,7 +894,7 @@ export class PtyHandler {
|
||||
managed.killTimer = setTimeout(() => {
|
||||
const still = this.ptys.get(id)
|
||||
if (still && !still.disposed) {
|
||||
still.pty.kill('SIGKILL')
|
||||
killPtyProcess(still.pty, 'SIGKILL')
|
||||
this.flushPtyOutput(id)
|
||||
// Why: emit pty.exit BEFORE disposeManagedPty sets disposed=true.
|
||||
// The natural onExit short-circuits on `managed.disposed`, so
|
||||
@@ -907,7 +929,7 @@ export class PtyHandler {
|
||||
if (!managed || managed.disposed) {
|
||||
throw new Error(`PTY "${id}" not found`)
|
||||
}
|
||||
managed.pty.kill(signal)
|
||||
killPtyProcess(managed.pty, signal)
|
||||
}
|
||||
|
||||
private async getCwd(params: Record<string, unknown>): Promise<string> {
|
||||
|
||||
@@ -2387,9 +2387,17 @@ 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 = null
|
||||
syncPaneTitleOverlayRects()
|
||||
frame = requestAnimationFrame(() => {
|
||||
frame = null
|
||||
syncPaneTitleOverlayRects()
|
||||
})
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -732,7 +732,7 @@ 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)
|
||||
window.api.pty.kill(spawnResult.id).catch(() => {})
|
||||
return
|
||||
}
|
||||
|
||||
@@ -918,7 +918,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra
|
||||
inputWriteQueue.clear()
|
||||
if (ptyId) {
|
||||
const id = ptyId
|
||||
window.api.pty.kill(id)
|
||||
window.api.pty.kill(id).catch(() => {})
|
||||
connected = false
|
||||
ptyId = null
|
||||
unregisterPtyHandlers(id)
|
||||
|
||||
@@ -231,6 +231,9 @@ export function createFilePathLinkProvider(
|
||||
}
|
||||
callback(links.length > 0 ? links : undefined)
|
||||
})
|
||||
.catch(() => {
|
||||
callback(undefined)
|
||||
})
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -64,6 +64,15 @@ 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({
|
||||
|
||||
@@ -2814,7 +2814,7 @@ 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)
|
||||
window.api.pty.kill(ptyId).catch(() => {})
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user