fix: fence reentrant terminal lifecycle cleanup

This commit is contained in:
Brennan Benson
2026-07-13 16:33:21 -07:00
parent ec57a10e02
commit 555545b02a
7 changed files with 102 additions and 21 deletions
@@ -61,7 +61,9 @@ export function registerEagerPtyBufferState(
ptyExitHandlers.delete(ptyId)
revokePtyPrimaryDataHandlerOwner(ptyId, dataOwner)
revokePtyPrimaryExitHandlerOwner(ptyId, exitOwner)
eagerPtyHandles.delete(ptyId)
if (eagerPtyHandles.get(ptyId) === handle) {
eagerPtyHandles.delete(ptyId)
}
onExit(ptyId, code)
}
@@ -92,7 +94,9 @@ export function registerEagerPtyBufferState(
}
revokePtyPrimaryDataHandlerOwner(ptyId, dataOwner)
revokePtyPrimaryExitHandlerOwner(ptyId, exitOwner)
eagerPtyHandles.delete(ptyId)
if (eagerPtyHandles.get(ptyId) === handle) {
eagerPtyHandles.delete(ptyId)
}
}
}
@@ -115,6 +115,40 @@ describe('pre-handler PTY buffer', () => {
expect(handler).toHaveBeenCalledWith(-1)
})
it('retires an older overflow probe when a concrete exit is drained', async () => {
const target = 'pty-concrete-exit-replaces-overflow'
const fillerPrefix = 'pty-concrete-exit-filler-'
let resolveProbe: ((alive: boolean | null) => void) | undefined
const hasPty = vi.fn(
() =>
new Promise<boolean | null>((resolve) => {
resolveProbe = resolve
})
)
const handler = vi.fn()
try {
bufferPreHandlerPtyExit(target, 1)
for (let index = 0; index < 64; index += 1) {
bufferPreHandlerPtyExit(`${fillerPrefix}${index}`, index)
}
reconcilePreHandlerPtyExitAfterOverflow(target, hasPty, handler, () => true)
bufferPreHandlerPtyExit(target, 2)
expect(drainPreHandlerPtyExit(target, handler)).toBe(true)
resolveProbe?.(false)
await Promise.resolve()
await Promise.resolve()
await Promise.resolve()
expect(handler.mock.calls.map(([code]) => code)).toEqual([2])
expect(hasPty).toHaveBeenCalledOnce()
} finally {
clearPreHandlerPtyState(target)
for (let index = 0; index < 64; index += 1) {
clearPreHandlerPtyState(`${fillerPrefix}${index}`)
}
}
})
it('retains an evicted exit through unknown liveness and retries authoritative proof', async () => {
for (let index = 0; index <= 64; index += 1) {
bufferPreHandlerPtyExit(`pty-exit-${index}`, index)
@@ -140,6 +140,11 @@ export function drainPreHandlerPtyExit(ptyId: string, handler: (code: number) =>
return false
}
preHandlerPtyExit.delete(ptyId)
// Why: a concrete buffered exit supersedes any older overflow tombstone;
// leaving its probe alive can synthesize a duplicate -1 exit afterward.
preHandlerPtyEvictedExitIds.delete(ptyId)
preHandlerPtyEvictedExitProbes.delete(ptyId)
pendingPreHandlerPtyEvictedExitProbes.delete(ptyId)
handler(code)
return true
}
@@ -91,6 +91,13 @@ export function restorePtyPrimaryDataHandlerOwner(
return true
}
export function isPtyPrimaryDataHandlerOwnerCurrent(
ptyId: string,
owner: PtyPrimaryHandlerOwner | undefined
): boolean {
return Boolean(owner?.active && primaryDataHandlerOwners.get(ptyId) === owner)
}
export function publishPtyPrimaryExitHandlerOwner(
ptyId: string,
owner: PtyPrimaryHandlerOwner
@@ -1,6 +1,7 @@
import { clearPreHandlerPtyState } from './pty-pre-handler-buffer'
import { clearPreHandlerPtyState, drainPreHandlerPtyData } from './pty-pre-handler-buffer'
import {
getPtyPrimaryExitHandlerOwner,
isPtyPrimaryDataHandlerOwnerCurrent,
isPtyPrimaryExitHandlerOwnerCurrent,
restorePtyPrimaryDataHandlerOwner,
suspendPtyPrimaryDataHandlerOwner
@@ -67,6 +68,14 @@ export function restorePtyDataHandlersAfterFailedShutdown(
if (snapshot.replayHandler) {
ptyReplayHandlers.set(snapshot.ptyId, snapshot.replayHandler)
}
if (snapshot.dataHandler) {
drainPreHandlerPtyData(snapshot.ptyId, snapshot.dataHandler, () => {
return (
isPtyPrimaryDataHandlerOwnerCurrent(snapshot.ptyId, snapshot.dataOwner) &&
ptyDataHandlers.get(snapshot.ptyId) === snapshot.dataHandler
)
})
}
}
if (
snapshot.teardownHandler &&
@@ -463,14 +463,13 @@ describe('createIpcPtyTransport', () => {
const outerExit = vi.fn()
const replacementData = vi.fn()
const replacementExit = vi.fn()
const outerTransport = createIpcPtyTransport()
const replacementTransport = createIpcPtyTransport()
outerTransport.attach({
const transport = createIpcPtyTransport()
transport.attach({
existingPtyId: 'same-id',
callbacks: {
onData: (data) => {
outerData.push(data)
replacementTransport.attach({
transport.attach({
existingPtyId: 'same-id',
callbacks: { onData: replacementData, onExit: replacementExit }
})
@@ -480,15 +479,15 @@ describe('createIpcPtyTransport', () => {
})
onData?.({ id: 'same-id', data: 'live replacement data' })
onExit?.({ id: 'same-id', code: 7 })
expect(outerData).toEqual(['first buffered chunk'])
expect(outerTransport.isConnected()).toBe(false)
expect(replacementData).toHaveBeenCalledWith('second buffered chunk')
expect(replacementData).toHaveBeenCalledWith('live replacement data')
expect(outerExit).not.toHaveBeenCalled()
expect(transport.isConnected()).toBe(true)
onExit?.({ id: 'same-id', code: 7 })
expect(replacementExit).toHaveBeenCalledWith(7)
expect(replacementTransport.isConnected()).toBe(false)
expect(transport.isConnected()).toBe(false)
})
it('keeps reentrant replacement ownership atomic while connect drains buffered data', async () => {
@@ -502,15 +501,14 @@ describe('createIpcPtyTransport', () => {
const outerExit = vi.fn()
const replacementData = vi.fn()
const replacementExit = vi.fn()
const outerTransport = createIpcPtyTransport()
const replacementTransport = createIpcPtyTransport()
const transport = createIpcPtyTransport()
const result = await outerTransport.connect({
const result = await transport.connect({
url: '',
callbacks: {
onData: (data) => {
outerData.push(data)
replacementTransport.attach({
transport.attach({
existingPtyId: 'same-id',
callbacks: { onData: replacementData, onExit: replacementExit }
})
@@ -518,14 +516,15 @@ describe('createIpcPtyTransport', () => {
onExit: outerExit
}
})
onExit?.({ id: 'same-id', code: 8 })
expect(result).toBeUndefined()
expect(outerData).toEqual(['first buffered chunk'])
expect(outerTransport.isConnected()).toBe(false)
expect(replacementData).toHaveBeenCalledWith('second buffered chunk')
expect(outerExit).not.toHaveBeenCalled()
expect(transport.isConnected()).toBe(true)
onExit?.({ id: 'same-id', code: 8 })
expect(replacementExit).toHaveBeenCalledWith(8)
expect(transport.isConnected()).toBe(false)
})
it('ignores a stale exit for a previous PTY after reconnecting the same transport', async () => {
@@ -1332,6 +1331,19 @@ describe('createIpcPtyTransport', () => {
expect(flushed.endsWith('TAIL$')).toBe(true)
})
it('keeps a replacement eager handle when the stale handle disposes', async () => {
const { getEagerPtyBufferHandle, registerEagerPtyBuffer } = await import('./pty-transport')
const staleHandle = registerEagerPtyBuffer('pty-restored', vi.fn())
const replacementHandle = registerEagerPtyBuffer('pty-restored', vi.fn())
onData?.({ id: 'pty-restored', data: 'replacement startup output' })
staleHandle.dispose()
expect(getEagerPtyBufferHandle('pty-restored')).toBe(replacementHandle)
expect(replacementHandle.flush()).toBe('replacement startup output')
replacementHandle.dispose()
})
it('drains pre-handler data and exit into eager buffers for fast background PTYs', async () => {
const { ensurePtyDispatcher, registerEagerPtyBuffer } = await import('./pty-transport')
const onEagerExit = vi.fn()
@@ -2041,7 +2053,10 @@ describe('createIpcPtyTransport', () => {
restorePtyDataHandlersAfterFailedShutdown(snapshots)
onData?.({ id: 'pty-1', data: 'live again' })
expect(onDataCallback).toHaveBeenCalledWith('live again')
expect(onDataCallback.mock.calls.map(([data]) => data)).toEqual([
'final burst while detached',
'live again'
])
})
it('does not restore failed-shutdown handlers over a replacement pane', async () => {
@@ -546,6 +546,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra
let connected = false
let destroyed = false
let ptyId: string | null = null
let bindingGeneration = 0
// Why: eager PTY buffers contain output produced before the pane attached —
// often from the previous app session. We still replay that data so titles
// and scrollback restore correctly, but it must not produce fresh bells,
@@ -840,6 +841,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra
ptyId = spawnResult.id
connected = true
const currentBindingGeneration = ++bindingGeneration
// Why: for deferred reattach (Option 2), the daemon returns snapshot/
// coldRestore data from createOrAttach. Skip onPtySpawn for reattach —
@@ -856,8 +858,10 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra
exitRegistration
)
if (!handlersCurrent || !connected || ptyId !== spawnResult.id) {
connected = false
ptyId = null
if (bindingGeneration === currentBindingGeneration) {
connected = false
ptyId = null
}
return undefined
}
@@ -965,14 +969,17 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra
const id = options.existingPtyId
ptyId = id
connected = true
const currentBindingGeneration = ++bindingGeneration
// Why: skip onPtySpawn — it would reset lastActivityAt and destroy the
// recency sort order that reconnectPersistedTerminals preserved.
const dataRegistration = registerPtyDataHandler(id)
const exitRegistration = registerPtyExitHandler(id)
const handlersCurrent = drainRegisteredPtyHandlers(id, dataRegistration, exitRegistration)
if (!handlersCurrent || !connected || ptyId !== id) {
connected = false
ptyId = null
if (bindingGeneration === currentBindingGeneration) {
connected = false
ptyId = null
}
return
}