fix: retain shutdown proof ownership

This commit is contained in:
Brennan Benson
2026-07-13 03:02:15 -07:00
parent 3ded598253
commit 9f38d7da18
5 changed files with 41 additions and 26 deletions
+3 -2
View File
@@ -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"
+2 -2
View File
@@ -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',
+3 -1
View File
@@ -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)
+1 -1
View File
@@ -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 {
+32 -20
View File
@@ -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.