From 9f38d7da18ff641dcac6046f8e033f26340003f3 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 13 Jul 2026 03:02:15 -0700 Subject: [PATCH] fix: retain shutdown proof ownership --- config/reliability-gates.jsonc | 5 ++-- src/main/ipc/pty.test.ts | 4 +-- src/main/ipc/pty.ts | 4 ++- src/relay/pty-handler.test.ts | 2 +- src/relay/pty-handler.ts | 52 +++++++++++++++++++++------------- 5 files changed, 41 insertions(+), 26 deletions(-) diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index 06a2231f0f6..9c4321aac78 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -3559,7 +3559,8 @@ "file": "src/relay/pty-handler.test.ts", "assertions": [ "Windows immediate, graceful/fallback, and relay-dispose shutdown issue one no-argument kill", - "stale spawn and explicit shutdown native failures retain one managed PTY and a later successful retry clears it" + "stale spawn native failures retain one managed PTY and retry automatically within eight bounded attempts", + "explicit shutdown native failure retains one managed PTY and a later caller retry clears it" ] }, { @@ -3695,7 +3696,7 @@ "direct local and SSH kill requests share the retained owner and re-drive after in-flight provider replacement", "replacement-provider callers settle only after the replacement shutdown succeeds and retain stronger coalesced identity options", "generationless identity-mismatch failure cannot tombstone a replacement relay-generation lease", - "paired/mobile controller kill fails closed when offline SSH intent cannot be made durable", + "paired/mobile controller close requires immediate provider shutdown and fails closed when offline SSH intent cannot be made durable", "host-scoped liveness lists only the selected provider instead of fanning out to unrelated SSH hosts", "relay generation reuse removes stale retry ownership without touching the new PTY", "lease-less SSH shutdown survives provider unregister and re-drives the replacement transport" diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 95ddf777b62..72d82eb4b10 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -2686,7 +2686,7 @@ describe('registerPtyHandlers', () => { // which adds async hops; a single microtask flush is no longer enough. await new Promise((resolve) => setImmediate(resolve)) - expect(shutdown).toHaveBeenCalledWith('remote-pty', { immediate: false }) + expect(shutdown).toHaveBeenCalledWith('remote-pty', { immediate: true }) expect(store.markSshRemotePtyLease).toHaveBeenCalledWith( 'ssh-1', 'remote-pty', @@ -3028,7 +3028,7 @@ describe('registerPtyHandlers', () => { expect(controller.kill('ssh:ssh-1@@relay-pty')).toBe(true) await new Promise((resolve) => setImmediate(resolve)) - expect(shutdown).toHaveBeenCalledWith('ssh:ssh-1@@relay-pty', { immediate: false }) + expect(shutdown).toHaveBeenCalledWith('ssh:ssh-1@@relay-pty', { immediate: true }) expect(localShutdown).not.toHaveBeenCalled() expect(store.markSshRemotePtyLease).toHaveBeenCalledWith( 'ssh-1', diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index d8981da95f6..539a341eb42 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -3724,7 +3724,9 @@ export function registerPtyHandlers( let connectionId: string | null | undefined = ptyOwnership.get(ptyId) const parsedSshId = connectionId === undefined ? parseAppSshPtyId(ptyId) : null connectionId ??= parsedSshId?.connectionId - const options: PtyShutdownOptions = { immediate: false } + // Why: paired/mobile close has no renderer left to observe a later + // graceful fallback. Require the provider's synchronous force-close path. + const options: PtyShutdownOptions = { immediate: true } let provider: IPtyProvider | undefined try { provider = connectionId ? getProvider(connectionId) : getProviderForPty(ptyId) diff --git a/src/relay/pty-handler.test.ts b/src/relay/pty-handler.test.ts index 6957b48c832..2b7846bf93d 100644 --- a/src/relay/pty-handler.test.ts +++ b/src/relay/pty-handler.test.ts @@ -735,7 +735,7 @@ describe('PtyHandler', () => { expect(stderr).toHaveBeenCalledWith( expect.stringContaining('stale-spawn fallback kill failed') ) - await dispatcher.callRequest('pty.shutdown', { id: 'pty-1', immediate: true }) + expect(() => vi.advanceTimersByTime(5000)).not.toThrow() expect(killSpy).toHaveBeenNthCalledWith(3, 'SIGKILL') expect(handler.activePtyCount).toBe(0) } finally { diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index 62f1b24da20..2f5c35610f1 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -144,6 +144,8 @@ const INTERACTIVE_REDRAW_MAX_CHARS = PTY_OUTPUT_FLUSH_CHUNK_CHARS const INTERACTIVE_OUTPUT_BUDGET_CHARS = 32 * 1024 const STARTUP_COMMAND_WRITE_DELAY_MS = 50 const STARTUP_COMMAND_SHELL_READY_FALLBACK_MS = 1500 +const STALE_SPAWN_KILL_RETRY_MS = 5000 +const STALE_SPAWN_KILL_RETRY_LIMIT = 8 const ALLOWED_SIGNALS = new Set([ 'SIGINT', 'SIGTERM', @@ -728,27 +730,37 @@ 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) - managed.killTimer = setTimeout(() => { - const still = this.ptys.get(id) - if (still && !still.disposed) { - try { - killPtyForShutdown(still, 'SIGKILL') - } catch (error) { - process.stderr.write( - `[pty-handler] stale-spawn fallback kill failed: ${error instanceof Error ? error.message : String(error)}\n` - ) - // Why: a thrown native kill is not proof of close. Retain the only - // managed handle so relay disposal can retry instead of orphaning - // an untracked PTY/ConPTY process. - still.killTimer = undefined - return + let fallbackAttempts = 0 + const scheduleFallback = (): void => { + managed.killTimer = setTimeout(() => { + const still = this.ptys.get(id) + if (still && !still.disposed) { + try { + killPtyForShutdown(still, 'SIGKILL') + } catch (error) { + fallbackAttempts += 1 + if (fallbackAttempts <= 2 || fallbackAttempts === STALE_SPAWN_KILL_RETRY_LIMIT) { + process.stderr.write( + `[pty-handler] stale-spawn fallback kill failed: ${error instanceof Error ? error.message : String(error)}\n` + ) + } + // Why: the stale caller can never retry an id it did not receive. + // Keep bounded automatic ownership before relay disposal takes over. + if (fallbackAttempts < STALE_SPAWN_KILL_RETRY_LIMIT) { + scheduleFallback() + } else { + still.killTimer = undefined + } + return + } + this.notifyExitListener(still) + disposeManagedPty(still) + this.ptys.delete(id) + this.clearPtyFlowState(id) } - this.notifyExitListener(still) - disposeManagedPty(still) - this.ptys.delete(id) - this.clearPtyFlowState(id) - } - }, 5000) + }, STALE_SPAWN_KILL_RETRY_MS) + } + scheduleFallback() // Why: arm fallback ownership before native shutdown. node-pty can throw // synchronously, especially during ConPTY startup, and the orphan still // needs a later force-close owner even when this request rejects.