fix(terminal): guard PTY native calls against dead-process Napi::Error crash (#916)

This commit is contained in:
Jinwoo Hong 2026-04-22 14:16:43 -04:00 committed by GitHub
parent 245b29d604
commit 6feb93ab78
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 102 additions and 7 deletions

View File

@ -33,6 +33,32 @@ export function parseArgs(argv: string[]): { socketPath: string; tokenPath: stri
async function main(): Promise<void> {
const { socketPath, tokenPath } = parseArgs(process.argv.slice(2))
// Why: node-pty can throw a C++ Napi::Error that escapes all JS try/catch
// blocks (e.g. writing to a PTY whose fd was closed between the native
// exit signal and the JS onExit callback). Without this handler, Node's
// default behavior is to print the stack and exit — killing the entire
// daemon and all terminal sessions. Logging and continuing is safe because
// the individual PTY is already dead; the daemon itself is still healthy.
// Non-PTY errors (logic bugs, corrupt state) are re-thrown so they still
// crash the daemon — masking those would hide real issues.
process.on('uncaughtException', (err) => {
const msg = err?.message ?? ''
const isNativeError =
err?.name === 'Error' &&
(msg.includes('pty') ||
msg.includes('Pty') ||
msg.includes('EIO') ||
msg.includes('EPIPE') ||
msg.includes('EBADF') ||
msg.includes('ENXIO'))
if (isNativeError) {
console.error('[daemon] Native PTY exception (suppressed):', err)
return
}
console.error('[daemon] Uncaught exception (fatal):', err)
throw err
})
let daemon: DaemonHandle | null = null
const shutdown = async (): Promise<void> => {

View File

@ -72,11 +72,48 @@ export function createPtySubprocess(opts: PtySubprocessOptions): SubprocessHandl
proc.onData((data) => onDataCb?.(data))
proc.onExit(({ exitCode }) => onExitCb?.(exitCode))
// Why: node-pty's native NAPI layer throws a C++ Napi::Error when
// write/resize/kill is called on a PTY whose underlying fd is already
// closed. This happens in the race window between the child process
// exiting and the JS onExit callback firing. An uncaught Napi::Error
// propagates to std::terminate, killing the entire daemon process.
let dead = false
proc.onExit(() => {
dead = true
})
return {
pid: proc.pid,
write: (data) => proc.write(data),
resize: (cols, rows) => proc.resize(cols, rows),
kill: () => proc.kill(),
write: (data) => {
if (dead) {
return
}
try {
proc.write(data)
} catch {
dead = true
}
},
resize: (cols, rows) => {
if (dead) {
return
}
try {
proc.resize(cols, rows)
} catch {
dead = true
}
},
kill: () => {
if (dead) {
return
}
try {
proc.kill()
} catch {
dead = true
}
},
forceKill: () => {
try {
process.kill(proc.pid, 'SIGKILL')

View File

@ -53,8 +53,13 @@ export class Session {
cols: opts.cols,
rows: opts.rows,
scrollback: opts.scrollback,
// Why: xterm.js generates query responses (DA1, DSR) asynchronously.
// If the subprocess has already exited, writing to it would hit a dead
// NAPI handle. Check session state before forwarding.
onData: (data) => {
// Forward xterm.js query responses (DA1 etc.) to subprocess
if (this._state === 'exited' || this._disposed) {
return
}
opts.subprocess.write(data)
}
})

View File

@ -27,6 +27,17 @@ function isCodexPaneStale(args: { tabId: string; panePtyId: string | null }): bo
return false
}
// Why: daemon session IDs use the format `${worktreeId}@@${shortUuid}`.
// This validates that a session ID actually belongs to the given worktree,
// preventing cross-workspace contamination during restore.
function isSessionOwnedByWorktree(sessionId: string, worktreeId: string): boolean {
const separatorIdx = sessionId.lastIndexOf('@@')
if (separatorIdx === -1) {
return true
}
return sessionId.slice(0, separatorIdx) === worktreeId
}
export function connectPanePty(
pane: ManagedPane,
manager: PaneManager,
@ -334,12 +345,22 @@ export function connectPanePty(
: null
: existingPtyId
: null
const deferredReattachSessionId =
const candidateReattachSessionId =
restoredSessionId && restoredSessionId !== detachedLivePtyId
? restoredSessionId
: daemonEnabled
? detachedLivePtyId
: null
// Why: daemon session IDs encode `${worktreeId}@@${uuid}`. After a daemon
// crash + cold restore, corrupted or stale session-to-tab mappings can
// cause a tab in workspace A to hold a ptyId from workspace B. Restoring
// that session would paint the wrong terminal content in this pane. Drop
// the reattach and spawn a fresh session instead.
const deferredReattachSessionId =
candidateReattachSessionId &&
isSessionOwnedByWorktree(candidateReattachSessionId, deps.worktreeId)
? candidateReattachSessionId
: null
if (deferredReattachSessionId) {
allowInitialIdleCacheSeed = true

View File

@ -287,9 +287,15 @@ export function useTerminalPaneGlobalEffects({
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [isVisible])
// Why: only the active tab's terminal should process file drops. Registering
// a listener per mounted tab causes a MaxListenersExceededWarning when 11+
// tabs are open. Gating on isActive ensures at most one listener exists.
useEffect(() => {
if (!isActive) {
return
}
return window.api.ui.onFileDrop((data) => {
if (!isActiveRef.current || data.target !== 'terminal') {
if (data.target !== 'terminal') {
return
}
const manager = managerRef.current
@ -314,5 +320,5 @@ export function useTerminalPaneGlobalEffects({
transport.sendInput(`${shellEscapePath(path)} `)
}
})
}, [isActiveRef, managerRef, paneTransportsRef])
}, [isActive, managerRef, paneTransportsRef])
}