mirror of
https://github.com/stablyai/orca.git
synced 2026-10-24 00:04:01 +00:00
fix(terminal): a restarted relay must not hand a new tab a dead PTY's exit
A redeployed SSH relay renumbers its PTY ids from pty-1, so a freshly spawned PTY can be handed an id a dead one used to own. The renderer's pre-handler buffer is keyed on that id alone, so `registerExit` found the dead PTY's buffered exit and reported the brand-new shell as `exitedBeforeAttach` — the pane never bound a PTY and the tab came up blank forever. Date every buffered record with a monotonic sequence and fence a fresh spawn against it: state recorded before the spawn request left the renderer belongs to the id's earlier owner. State recorded after it is kept, so the real pre-attach race (a shell that dies instantly, or writes before the pane registers its handler) still works. Makes ssh-lost-kill-tab-resurrection.spec.ts:178 pass; it failed in SETUP.
This commit is contained in:
@@ -3,6 +3,8 @@ import { extractIpcErrorMessage } from '@/lib/ipc-error'
|
||||
import { ensurePtyDispatcher } from './pty-dispatcher'
|
||||
import {
|
||||
clearConsumedPreHandlerPtyExit,
|
||||
currentPreHandlerPtySequence,
|
||||
discardPreHandlerPtyStateFromPriorIncarnation,
|
||||
hasPreHandlerPtyExit,
|
||||
isPreHandlerPtyStateDiscarded
|
||||
} from './pty-pre-handler-buffer'
|
||||
@@ -68,6 +70,10 @@ export async function connectIpcPty(
|
||||
if (options.shouldContinue && !options.shouldContinue()) {
|
||||
return
|
||||
}
|
||||
// Why read it before the request and not after: a redeployed SSH relay renumbers from pty-1, so
|
||||
// this spawn can be handed an id a dead PTY used to own. State dated at or below this fence was
|
||||
// recorded before we asked for a PTY, so it belongs to that earlier owner, not to us.
|
||||
const priorIncarnationFence = currentPreHandlerPtySequence()
|
||||
const spawnResult = await spawnIpcPty(transportOptions, options, admittedSessionId)
|
||||
const retireFreshSpawn = async (): Promise<void> => {
|
||||
if (!spawnResult.isReattach && !spawnResult.coldRestore) {
|
||||
@@ -87,6 +93,11 @@ export async function connectIpcPty(
|
||||
context.getCallbacks().onReattachDetermined?.()
|
||||
}
|
||||
|
||||
if (!admittedSessionId && !spawnResult.isReattach && !spawnResult.coldRestore) {
|
||||
// Why only a fresh spawn: a reattach deliberately re-owns an id that already existed, so its
|
||||
// buffered exit is the real thing. A fresh spawn's PTY did not exist yet.
|
||||
discardPreHandlerPtyStateFromPriorIncarnation(spawnResult.id, priorIncarnationFence)
|
||||
}
|
||||
context.bind(spawnResult.id)
|
||||
if (!spawnResult.isReattach && !spawnResult.coldRestore) {
|
||||
onPtySpawn?.(spawnResult.id)
|
||||
|
||||
@@ -5,9 +5,11 @@ import {
|
||||
clearConsumedPreHandlerPtyExit,
|
||||
clearPreHandlerPtyState,
|
||||
consumePreHandlerPtyState,
|
||||
currentPreHandlerPtySequence,
|
||||
drainPreHandlerPtyData,
|
||||
drainPreHandlerPtyExit,
|
||||
discardPreHandlerPtyState,
|
||||
discardPreHandlerPtyStateFromPriorIncarnation,
|
||||
hasPreHandlerPtyExit,
|
||||
replayPreHandlerPtyData
|
||||
} from './pty-pre-handler-buffer'
|
||||
@@ -16,6 +18,7 @@ const RESCAN_PTY_ID = 'pty-pre-handler-rescan'
|
||||
const TRIM_PTY_ID = 'pty-pre-handler-trim'
|
||||
const EXIT_PTY_ID = 'pty-pre-handler-exit'
|
||||
const CAPPED_EXIT_PTY_IDS = Array.from({ length: 65 }, (_, index) => `pty-capped-exit-${index}`)
|
||||
const RECYCLED_PTY_ID = 'ssh:target@@pty-2'
|
||||
|
||||
describe('pre-handler PTY buffer', () => {
|
||||
afterEach(() => {
|
||||
@@ -25,6 +28,7 @@ describe('pre-handler PTY buffer', () => {
|
||||
for (const ptyId of CAPPED_EXIT_PTY_IDS) {
|
||||
clearPreHandlerPtyState(ptyId)
|
||||
}
|
||||
clearPreHandlerPtyState(RECYCLED_PTY_ID)
|
||||
})
|
||||
|
||||
it('does not rescan historical chunks while buffering small startup output', () => {
|
||||
@@ -187,4 +191,51 @@ describe('pre-handler PTY buffer', () => {
|
||||
expect(exits).not.toContain(0)
|
||||
expect(exits).toContain(64)
|
||||
})
|
||||
|
||||
// A redeployed SSH relay renumbers from pty-1, so a fresh spawn is handed an id whose dead owner
|
||||
// left an exit here. Applying it reports the brand-new shell as already exited and the pane never
|
||||
// binds a PTY at all — the tab comes up blank forever.
|
||||
it("drops a recycled id's exit recorded before the fresh spawn was requested", () => {
|
||||
bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 0)
|
||||
bufferPreHandlerPtyData(RECYCLED_PTY_ID, 'output from the dead shell')
|
||||
|
||||
const fence = currentPreHandlerPtySequence()
|
||||
discardPreHandlerPtyStateFromPriorIncarnation(RECYCLED_PTY_ID, fence)
|
||||
|
||||
const exit = vi.fn()
|
||||
const data = vi.fn()
|
||||
expect(hasPreHandlerPtyExit(RECYCLED_PTY_ID)).toBe(false)
|
||||
drainPreHandlerPtyExit(RECYCLED_PTY_ID, exit)
|
||||
drainPreHandlerPtyData(RECYCLED_PTY_ID, data)
|
||||
expect(exit).not.toHaveBeenCalled()
|
||||
expect(data).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('keeps state recorded after the fence, so a shell that dies instantly still reports its exit', () => {
|
||||
const fence = currentPreHandlerPtySequence()
|
||||
bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 1)
|
||||
bufferPreHandlerPtyData(RECYCLED_PTY_ID, 'startup bytes')
|
||||
|
||||
discardPreHandlerPtyStateFromPriorIncarnation(RECYCLED_PTY_ID, fence)
|
||||
|
||||
const exit = vi.fn()
|
||||
const data = vi.fn()
|
||||
expect(hasPreHandlerPtyExit(RECYCLED_PTY_ID)).toBe(true)
|
||||
// Data first: draining the exit transfers ownership and clears the buffered bytes with it.
|
||||
drainPreHandlerPtyData(RECYCLED_PTY_ID, data)
|
||||
drainPreHandlerPtyExit(RECYCLED_PTY_ID, exit)
|
||||
expect(exit).toHaveBeenCalledWith(1)
|
||||
expect(data).toHaveBeenCalledWith('startup bytes', undefined)
|
||||
})
|
||||
|
||||
it('re-admits exits for a recycled id whose prior incarnation was consumed', () => {
|
||||
consumePreHandlerPtyState(RECYCLED_PTY_ID)
|
||||
|
||||
discardPreHandlerPtyStateFromPriorIncarnation(RECYCLED_PTY_ID, currentPreHandlerPtySequence())
|
||||
bufferPreHandlerPtyExit(RECYCLED_PTY_ID, 3)
|
||||
|
||||
const exit = vi.fn()
|
||||
drainPreHandlerPtyExit(RECYCLED_PTY_ID, exit)
|
||||
expect(exit).toHaveBeenCalledWith(3)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -11,10 +11,17 @@ type BufferedPreHandlerPtyState = {
|
||||
chunks: BufferedPreHandlerPtyData[]
|
||||
head: number
|
||||
bytes: number
|
||||
/** Sequence of the newest chunk, used to fence state left by a prior incarnation of a reused id. */
|
||||
sequence: number
|
||||
}
|
||||
|
||||
type BufferedPreHandlerPtyExit = {
|
||||
code: number
|
||||
sequence: number
|
||||
}
|
||||
|
||||
const preHandlerPtyData = new Map<string, BufferedPreHandlerPtyState>()
|
||||
const preHandlerPtyExit = new Map<string, number>()
|
||||
const preHandlerPtyExit = new Map<string, BufferedPreHandlerPtyExit>()
|
||||
const consumedPreHandlerPtyExits = new Map<string, true>()
|
||||
const discardedPreHandlerPtyStates = new Map<string, ReturnType<typeof setTimeout>>()
|
||||
const DISCARDED_PRE_HANDLER_PTY_STATE_TTL_MS = 60_000
|
||||
@@ -31,6 +38,21 @@ const PRE_HANDLER_PTY_EXIT_MAX_PTYS = 64
|
||||
const PRE_HANDLER_PTY_DATA_WARN_BYTES = 64 * 1024
|
||||
const warnedLostHandlerPtyIds = new Set<string>()
|
||||
|
||||
// Why: pty ids are NOT unique over time — a redeployed SSH relay renumbers from pty-1, so a fresh
|
||||
// spawn can be handed an id whose previous incarnation left buffered state here. A monotonic
|
||||
// counter dates every record so a spawn can tell "arrived before I asked for this PTY" (someone
|
||||
// else's) from "arrived while it was starting" (mine).
|
||||
let preHandlerPtySequence = 0
|
||||
|
||||
function nextPreHandlerPtySequence(): number {
|
||||
preHandlerPtySequence += 1
|
||||
return preHandlerPtySequence
|
||||
}
|
||||
|
||||
export function currentPreHandlerPtySequence(): number {
|
||||
return preHandlerPtySequence
|
||||
}
|
||||
|
||||
export function bufferPreHandlerPtyData(ptyId: string, data: string, meta?: PtyDataMeta): void {
|
||||
if (discardedPreHandlerPtyStates.has(ptyId)) {
|
||||
return
|
||||
@@ -51,9 +73,10 @@ export function bufferPreHandlerPtyData(ptyId: string, data: string, meta?: PtyD
|
||||
: meta
|
||||
let state = preHandlerPtyData.get(ptyId)
|
||||
if (!state) {
|
||||
state = { chunks: [], head: 0, bytes: 0 }
|
||||
state = { chunks: [], head: 0, bytes: 0, sequence: 0 }
|
||||
preHandlerPtyData.set(ptyId, state)
|
||||
}
|
||||
state.sequence = nextPreHandlerPtySequence()
|
||||
state.chunks.push({
|
||||
data: chunk.data,
|
||||
bytes: chunk.bytes,
|
||||
@@ -117,7 +140,31 @@ export function bufferPreHandlerPtyExit(ptyId: string, code: number): void {
|
||||
preHandlerPtyExit.delete(oldestPtyId)
|
||||
}
|
||||
}
|
||||
preHandlerPtyExit.set(ptyId, code)
|
||||
preHandlerPtyExit.set(ptyId, { code, sequence: nextPreHandlerPtySequence() })
|
||||
}
|
||||
|
||||
/** Drop pre-handler state a freshly spawned PTY inherited from an earlier owner of its id.
|
||||
*
|
||||
* `fenceSequence` is read before the spawn request leaves the renderer, so anything at or below it
|
||||
* was recorded when this PTY did not yet exist and cannot describe it. Bytes and exits recorded
|
||||
* after the fence are kept: that is the real pre-attach race (a shell that dies instantly, or
|
||||
* writes before the pane registers its handler) and losing it would blank a legitimate pane. */
|
||||
export function discardPreHandlerPtyStateFromPriorIncarnation(
|
||||
ptyId: string,
|
||||
fenceSequence: number
|
||||
): void {
|
||||
const exit = preHandlerPtyExit.get(ptyId)
|
||||
if (exit && exit.sequence <= fenceSequence) {
|
||||
preHandlerPtyExit.delete(ptyId)
|
||||
}
|
||||
const data = preHandlerPtyData.get(ptyId)
|
||||
if (data && data.sequence <= fenceSequence) {
|
||||
preHandlerPtyData.delete(ptyId)
|
||||
warnedLostHandlerPtyIds.delete(ptyId)
|
||||
}
|
||||
// Why: the id now names a different, live PTY, so a prior incarnation's consumed/discarded marks
|
||||
// must not suppress this one's own exit — the same admission boundary a same-id reattach gets.
|
||||
clearConsumedPreHandlerPtyExit(ptyId)
|
||||
}
|
||||
|
||||
// Why: primary handlers and pane-less parked owners have fully handled this
|
||||
@@ -173,13 +220,13 @@ export function hasPreHandlerPtyExit(ptyId: string): boolean {
|
||||
}
|
||||
|
||||
export function drainPreHandlerPtyExit(ptyId: string, handler: (code: number) => void): void {
|
||||
const code = preHandlerPtyExit.get(ptyId)
|
||||
if (code === undefined) {
|
||||
const exit = preHandlerPtyExit.get(ptyId)
|
||||
if (exit === undefined) {
|
||||
return
|
||||
}
|
||||
preHandlerPtyExit.delete(ptyId)
|
||||
try {
|
||||
handler(code)
|
||||
handler(exit.code)
|
||||
} finally {
|
||||
// Why: draining transfers ownership to this handler. Even when it throws,
|
||||
// a duplicate exit must not become a new pre-handler event.
|
||||
|
||||
Reference in New Issue
Block a user