From 555545b02a86ecf389dc373e9da01a8d47084f1a Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 13 Jul 2026 16:33:21 -0700 Subject: [PATCH] fix: fence reentrant terminal lifecycle cleanup --- .../terminal-pane/pty-eager-buffer.ts | 8 +++- .../pty-pre-handler-buffer.test.ts | 34 +++++++++++++++ .../terminal-pane/pty-pre-handler-buffer.ts | 5 +++ .../pty-primary-handler-admission.ts | 7 +++ .../pty-primary-handler-registry.ts | 11 ++++- .../terminal-pane/pty-transport.test.ts | 43 +++++++++++++------ .../components/terminal-pane/pty-transport.ts | 15 +++++-- 7 files changed, 102 insertions(+), 21 deletions(-) diff --git a/src/renderer/src/components/terminal-pane/pty-eager-buffer.ts b/src/renderer/src/components/terminal-pane/pty-eager-buffer.ts index e20a00dac50..c78eb78cf36 100644 --- a/src/renderer/src/components/terminal-pane/pty-eager-buffer.ts +++ b/src/renderer/src/components/terminal-pane/pty-eager-buffer.ts @@ -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) + } } } diff --git a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts index 3bd7a323532..571cb32316f 100644 --- a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.test.ts @@ -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((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) diff --git a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts index 66be926d1e5..a6afee540d6 100644 --- a/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts +++ b/src/renderer/src/components/terminal-pane/pty-pre-handler-buffer.ts @@ -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 } diff --git a/src/renderer/src/components/terminal-pane/pty-primary-handler-admission.ts b/src/renderer/src/components/terminal-pane/pty-primary-handler-admission.ts index c84b95149be..818b6e900aa 100644 --- a/src/renderer/src/components/terminal-pane/pty-primary-handler-admission.ts +++ b/src/renderer/src/components/terminal-pane/pty-primary-handler-admission.ts @@ -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 diff --git a/src/renderer/src/components/terminal-pane/pty-primary-handler-registry.ts b/src/renderer/src/components/terminal-pane/pty-primary-handler-registry.ts index 1f54eb5003b..cbe163e58dc 100644 --- a/src/renderer/src/components/terminal-pane/pty-primary-handler-registry.ts +++ b/src/renderer/src/components/terminal-pane/pty-primary-handler-registry.ts @@ -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 && diff --git a/src/renderer/src/components/terminal-pane/pty-transport.test.ts b/src/renderer/src/components/terminal-pane/pty-transport.test.ts index 9f08086b5e5..1014f5f7852 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.test.ts @@ -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 () => { diff --git a/src/renderer/src/components/terminal-pane/pty-transport.ts b/src/renderer/src/components/terminal-pane/pty-transport.ts index fee7281dbb6..78eabc3fc96 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.ts @@ -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 }