From eddddc06826433d06979848b56fefa6444e6a073 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 13 Jul 2026 16:01:15 -0700 Subject: [PATCH] fix: publish terminal handler groups atomically --- .../terminal-pane/pty-dispatcher.ts | 206 ++---------------- .../terminal-pane/pty-eager-buffer.ts | 117 ++++++++++ .../pty-primary-handler-admission.ts | 35 ++- .../pty-primary-handler-registry.ts | 79 +++++++ .../terminal-pane/pty-transport.test.ts | 111 ++++++++++ .../components/terminal-pane/pty-transport.ts | 64 ++++-- 6 files changed, 405 insertions(+), 207 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/pty-eager-buffer.ts create mode 100644 src/renderer/src/components/terminal-pane/pty-primary-handler-registry.ts diff --git a/src/renderer/src/components/terminal-pane/pty-dispatcher.ts b/src/renderer/src/components/terminal-pane/pty-dispatcher.ts index 552ca9641c1..43009629145 100644 --- a/src/renderer/src/components/terminal-pane/pty-dispatcher.ts +++ b/src/renderer/src/components/terminal-pane/pty-dispatcher.ts @@ -5,21 +5,16 @@ * co-locating the global handler maps that both the transport factory * and the eager-buffer reconnection logic share. */ -import { TERMINAL_SCROLLBACK_SESSION_BUFFER_BYTE_LIMIT } from '../../../../shared/terminal-scrollback-limits' import { clearProcessedPtyCharTotal, deliverPtyDataWithDeferredAck, exposeE2eTerminalPtyAckGate, getProcessedPtyCharTotals } from './terminal-pty-ack-gate' -import { clampUtf8Tail, type EagerBufferChunk } from './pty-eager-buffer-clamp' import { bufferPreHandlerPtyData, bufferPreHandlerPtyExit, - clearPreHandlerPtyState, - drainPreHandlerPtyData, - drainPreHandlerPtyExit, - reconcilePreHandlerPtyExitAfterOverflow + clearPreHandlerPtyState } from './pty-pre-handler-buffer' import { clearReceivedPtyCharTotal, @@ -29,22 +24,32 @@ import { } from './terminal-delivery-watchdog' import { recordTerminalFreezeBreadcrumb } from './terminal-freeze-breadcrumbs' import { installTerminalFreezeReport } from './terminal-freeze-report' +import { ptyDataHandlers, ptyExitHandlers, ptyReplayHandlers } from './pty-primary-handler-registry' +import { + getEagerPtyBufferHandle, + hasEagerPtyBufferHandles, + registerEagerPtyBufferState +} from './pty-eager-buffer' +import type { EagerPtyHandle } from './pty-eager-buffer' +import type { PtyDataMeta } from './pty-primary-handler-registry' + +export { + ptyDataHandlers, + ptyExitHandlers, + ptyReplayHandlers, + ptyTeardownHandlers, + restorePtyDataHandlersAfterFailedShutdown, + unregisterPtyDataHandlers +} from './pty-primary-handler-registry' +export type { PtyDataHandlerShutdownSnapshot, PtyDataMeta } from './pty-primary-handler-registry' +export { getEagerPtyBufferHandle } +export type { EagerPtyHandle } // ── Singleton PTY event dispatcher ─────────────────────────────────── // One global IPC listener per channel, routes events to transports by // PTY ID. Eliminates the N-listener problem that triggers // MaxListenersExceededWarning with many panes/tabs. -export type PtyDataMeta = { - seq?: number - rawLength?: number - background?: boolean - /** Main dropped this PTY's buffered output at the pending cap; the pane - * must repaint from the main-owned snapshot instead of the live stream. */ - droppedOutput?: boolean -} - -export const ptyDataHandlers = new Map void>() /** Sidecar subscriptions that observe PTY data without owning the primary * handler. Used by features that need to react to the live byte stream * (e.g. agent-paste-draft watching for DECSET 2004 / bracketed-paste- @@ -52,70 +57,9 @@ export const ptyDataHandlers = new Map void>>() - -/** Per-PTY replay handlers for relay pty.attach replay data. Routed through - * a dedicated pty:replay IPC channel so the renderer can engage the replay - * guard and suppress xterm auto-replies during replay. */ -export const ptyReplayHandlers = new Map void>() -export const ptyExitHandlers = new Map void>() const ptyExitSidecars = new Map void>>() -/** Per-PTY teardown callbacks registered by each transport to clear closure - * state (stale-title timer, agent tracker) that would otherwise fire after - * the data handler is removed. */ -export const ptyTeardownHandlers = new Map void>() let ptyDispatcherAttached = false -export type PtyDataHandlerShutdownSnapshot = { - ptyId: string - dataHandler?: (data: string, meta?: PtyDataMeta) => void - replayHandler?: (data: string) => void - teardownHandler?: () => void -} - -/** - * Remove data and status handlers for the given PTY IDs so that any final - * data flushed by the main process during PTY teardown cannot trigger - * bell / agent-status notifications from a worktree that is being shut down. - * Also invokes per-transport teardown callbacks to cancel accumulated closure - * state (e.g. staleTitleTimer, agent tracker) that could independently fire - * stale notifications. - * Exit handlers are intentionally kept alive so the normal exit-cleanup - * path (unregister, clear stale timers, update store) still runs. - */ -export function unregisterPtyDataHandlers(ptyIds: string[]): PtyDataHandlerShutdownSnapshot[] { - const snapshots: PtyDataHandlerShutdownSnapshot[] = [] - for (const id of ptyIds) { - snapshots.push({ - ptyId: id, - dataHandler: ptyDataHandlers.get(id), - replayHandler: ptyReplayHandlers.get(id), - teardownHandler: ptyTeardownHandlers.get(id) - }) - ptyDataHandlers.delete(id) - ptyReplayHandlers.delete(id) - ptyTeardownHandlers.get(id)?.() - ptyTeardownHandlers.delete(id) - clearPreHandlerPtyState(id) - } - return snapshots -} - -export function restorePtyDataHandlersAfterFailedShutdown( - snapshots: readonly PtyDataHandlerShutdownSnapshot[] -): void { - for (const snapshot of snapshots) { - if (snapshot.dataHandler) { - ptyDataHandlers.set(snapshot.ptyId, snapshot.dataHandler) - } - if (snapshot.replayHandler) { - ptyReplayHandlers.set(snapshot.ptyId, snapshot.replayHandler) - } - if (snapshot.teardownHandler) { - ptyTeardownHandlers.set(snapshot.ptyId, snapshot.teardownHandler) - } - } -} - let pushListenerUnsubscribes: (() => void)[] = [] /** Detach and freshly re-subscribe every push-channel listener. Called by the @@ -144,7 +88,7 @@ export function ensurePtyDispatcher(): void { attachPtyPushListeners() startTerminalDeliveryWatchdog({ reattachPushListeners: reattachPtyDispatcherPushListeners, - hasAttachedPtys: () => ptyDataHandlers.size > 0 || eagerPtyHandles.size > 0 + hasAttachedPtys: () => ptyDataHandlers.size > 0 || hasEagerPtyBufferHandles() }) } @@ -288,114 +232,10 @@ export function subscribeToPtyExit(ptyId: string, watcher: (code: number) => voi } } -// ─── Eager PTY buffer for reconnection on restart ──────────────────── -// Why: On startup, PTYs are spawned before TerminalPane mounts. Shell output -// (prompt, MOTD) arrives via pty:data before xterm exists. These helpers buffer -// that output so transport.attach() can replay it when the pane finally mounts. - -export type EagerPtyHandle = { flush: () => string; dispose: () => void } -const eagerPtyHandles = new Map() - -export function getEagerPtyBufferHandle(ptyId: string): EagerPtyHandle | undefined { - return eagerPtyHandles.get(ptyId) -} - -// Why: 512 KB matches the scrollback buffer cap used by TerminalPane's -// serialization. Prevents unbounded memory growth if a restored shell -// runs a long-lived command (e.g. tail -f) in a worktree the user never opens. -const EAGER_BUFFER_MAX_BYTES = TERMINAL_SCROLLBACK_SESSION_BUFFER_BYTE_LIMIT - export function registerEagerPtyBuffer( ptyId: string, onExit: (ptyId: string, code: number) => void ): EagerPtyHandle { ensurePtyDispatcher() - // Why: a head index instead of Array.shift() — shift() is O(n), making - // pre-attach buffering quadratic under many small chunks. Compaction is deferred. - const chunks: EagerBufferChunk[] = [] - let head = 0 - let bufferBytes = 0 - - const dataHandler = (data: string): void => { - // A single chunk larger than the cap would otherwise bypass trimming and - // store the whole payload; keep only its most-recent tail. - const chunk = clampUtf8Tail(data, EAGER_BUFFER_MAX_BYTES) - chunks.push(chunk) - bufferBytes += chunk.bytes - // Drop whole leading chunks (keeping the prompt-bearing tail) until within cap. - while (bufferBytes > EAGER_BUFFER_MAX_BYTES && head < chunks.length - 1) { - bufferBytes -= chunks[head].bytes - chunks[head] = { data: '', bytes: 0 } - head += 1 - } - // Compact when dead slots reach half the array so it can't grow unbounded. - if (head > 0 && head * 2 >= chunks.length) { - chunks.splice(0, head) - head = 0 - } - } - const exitHandler = (code: number): void => { - // Shell died before TerminalPane attached — clean up and notify the store - // so the tab's ptyId is cleared and connectPanePty falls through to connect(). - // Identity-guarded like dispose(): never delete a handler a transport has - // since registered for this id (#7894 detach/attach remount race). - if (ptyDataHandlers.get(ptyId) === dataHandler) { - ptyDataHandlers.delete(ptyId) - ptyReplayHandlers.delete(ptyId) - } - ptyExitHandlers.delete(ptyId) - eagerPtyHandles.delete(ptyId) - onExit(ptyId, code) - } - - ptyDataHandlers.set(ptyId, dataHandler) - ptyExitHandlers.set(ptyId, exitHandler) - - const handle: EagerPtyHandle = { - flush() { - const data = chunks - .slice(head) - .map((chunk) => chunk.data) - .join('') - chunks.length = 0 - head = 0 - bufferBytes = 0 - return data - }, - dispose() { - // Why: dispose runs at pane attach (mount completed) — the pane's own - // visibility sync now owns the hidden-delivery decision for this PTY. - // Only remove if the current handler is still the temp one (compare by - // reference). After attach() replaces the handler this becomes a no-op. - if (ptyDataHandlers.get(ptyId) === dataHandler) { - ptyDataHandlers.delete(ptyId) - ptyReplayHandlers.delete(ptyId) - } - if (ptyExitHandlers.get(ptyId) === exitHandler) { - ptyExitHandlers.delete(ptyId) - } - eagerPtyHandles.delete(ptyId) - } - } - - eagerPtyHandles.set(ptyId, handle) - drainPreHandlerPtyData(ptyId, dataHandler) - // Why: launcher callbacks often capture the returned handle so they can - // flush output on exit. Defer a pre-handler exit by one microtask so the - // caller receives that handle before onExit fires. - queueMicrotask(() => { - if (ptyExitHandlers.get(ptyId) === exitHandler) { - if (!drainPreHandlerPtyExit(ptyId, exitHandler)) { - reconcilePreHandlerPtyExitAfterOverflow( - ptyId, - window.api.pty.hasPty, - exitHandler, - () => ptyExitHandlers.get(ptyId) === exitHandler - ) - } - } else { - clearPreHandlerPtyState(ptyId) - } - }) - return handle + return registerEagerPtyBufferState(ptyId, onExit) } diff --git a/src/renderer/src/components/terminal-pane/pty-eager-buffer.ts b/src/renderer/src/components/terminal-pane/pty-eager-buffer.ts new file mode 100644 index 00000000000..e20a00dac50 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-eager-buffer.ts @@ -0,0 +1,117 @@ +import { TERMINAL_SCROLLBACK_SESSION_BUFFER_BYTE_LIMIT } from '../../../../shared/terminal-scrollback-limits' +import { clampUtf8Tail, type EagerBufferChunk } from './pty-eager-buffer-clamp' +import { ptyDataHandlers, ptyExitHandlers, ptyReplayHandlers } from './pty-primary-handler-registry' +import { + createPtyPrimaryHandlerOwner, + publishPtyPrimaryDataHandlerOwner, + publishPtyPrimaryExitHandlerOwner, + revokePtyPrimaryDataHandlerOwner, + revokePtyPrimaryExitHandlerOwner +} from './pty-primary-handler-admission' +import { + clearPreHandlerPtyState, + drainPreHandlerPtyData, + drainPreHandlerPtyExit, + reconcilePreHandlerPtyExitAfterOverflow +} from './pty-pre-handler-buffer' + +export type EagerPtyHandle = { flush: () => string; dispose: () => void } +const eagerPtyHandles = new Map() + +export function getEagerPtyBufferHandle(ptyId: string): EagerPtyHandle | undefined { + return eagerPtyHandles.get(ptyId) +} + +export function hasEagerPtyBufferHandles(): boolean { + return eagerPtyHandles.size > 0 +} + +const EAGER_BUFFER_MAX_BYTES = TERMINAL_SCROLLBACK_SESSION_BUFFER_BYTE_LIMIT + +export function registerEagerPtyBufferState( + ptyId: string, + onExit: (ptyId: string, code: number) => void +): EagerPtyHandle { + // Why: a head index avoids quadratic Array.shift() behavior under many chunks. + const chunks: EagerBufferChunk[] = [] + let head = 0 + let bufferBytes = 0 + const dataOwner = createPtyPrimaryHandlerOwner() + const exitOwner = createPtyPrimaryHandlerOwner() + + const dataHandler = (data: string): void => { + const chunk = clampUtf8Tail(data, EAGER_BUFFER_MAX_BYTES) + chunks.push(chunk) + bufferBytes += chunk.bytes + while (bufferBytes > EAGER_BUFFER_MAX_BYTES && head < chunks.length - 1) { + bufferBytes -= chunks[head].bytes + chunks[head] = { data: '', bytes: 0 } + head += 1 + } + if (head > 0 && head * 2 >= chunks.length) { + chunks.splice(0, head) + head = 0 + } + } + const exitHandler = (code: number): void => { + if (ptyDataHandlers.get(ptyId) === dataHandler) { + ptyDataHandlers.delete(ptyId) + ptyReplayHandlers.delete(ptyId) + } + ptyExitHandlers.delete(ptyId) + revokePtyPrimaryDataHandlerOwner(ptyId, dataOwner) + revokePtyPrimaryExitHandlerOwner(ptyId, exitOwner) + eagerPtyHandles.delete(ptyId) + onExit(ptyId, code) + } + + ptyDataHandlers.set(ptyId, dataHandler) + ptyExitHandlers.set(ptyId, exitHandler) + publishPtyPrimaryDataHandlerOwner(ptyId, dataOwner) + publishPtyPrimaryExitHandlerOwner(ptyId, exitOwner) + + const handle: EagerPtyHandle = { + flush() { + const data = chunks + .slice(head) + .map((chunk) => chunk.data) + .join('') + chunks.length = 0 + head = 0 + bufferBytes = 0 + return data + }, + dispose() { + // Why: a mounted pane may already have replaced these temporary handlers. + if (ptyDataHandlers.get(ptyId) === dataHandler) { + ptyDataHandlers.delete(ptyId) + ptyReplayHandlers.delete(ptyId) + } + if (ptyExitHandlers.get(ptyId) === exitHandler) { + ptyExitHandlers.delete(ptyId) + } + revokePtyPrimaryDataHandlerOwner(ptyId, dataOwner) + revokePtyPrimaryExitHandlerOwner(ptyId, exitOwner) + eagerPtyHandles.delete(ptyId) + } + } + + eagerPtyHandles.set(ptyId, handle) + drainPreHandlerPtyData(ptyId, dataHandler) + // Why: let callers receive the handle before a buffered exit invokes onExit. + queueMicrotask(() => { + if (ptyExitHandlers.get(ptyId) === exitHandler) { + if (!drainPreHandlerPtyExit(ptyId, exitHandler)) { + reconcilePreHandlerPtyExitAfterOverflow( + ptyId, + window.api.pty.hasPty, + exitHandler, + () => ptyExitHandlers.get(ptyId) === exitHandler + ) + } + } else { + clearPreHandlerPtyState(ptyId) + } + }) + return handle +} 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 24db82bb01b..c84b95149be 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 @@ -1,4 +1,4 @@ -import type { PtyDataMeta } from './pty-dispatcher' +import type { PtyDataMeta } from './pty-primary-handler-registry' type PtyPrimaryHandlerAdmissionRegistry = { dataHandlers: Map void> @@ -72,6 +72,25 @@ export function revokePtyPrimaryDataHandlerOwner( revokeOwner(primaryDataHandlerOwners, ptyId, owner) } +export function suspendPtyPrimaryDataHandlerOwner( + ptyId: string +): PtyPrimaryHandlerOwner | undefined { + const owner = primaryDataHandlerOwners.get(ptyId) + primaryDataHandlerOwners.delete(ptyId) + return owner +} + +export function restorePtyPrimaryDataHandlerOwner( + ptyId: string, + owner: PtyPrimaryHandlerOwner | undefined +): boolean { + if (!isOwnerCurrent(primaryDataHandlerOwners, ptyId, owner)) { + return false + } + primaryDataHandlerOwners.set(ptyId, owner) + return true +} + export function publishPtyPrimaryExitHandlerOwner( ptyId: string, owner: PtyPrimaryHandlerOwner @@ -86,6 +105,17 @@ export function revokePtyPrimaryExitHandlerOwner( revokeOwner(primaryExitHandlerOwners, ptyId, owner) } +export function getPtyPrimaryExitHandlerOwner(ptyId: string): PtyPrimaryHandlerOwner | undefined { + return primaryExitHandlerOwners.get(ptyId) +} + +export function isPtyPrimaryExitHandlerOwnerCurrent( + ptyId: string, + owner: PtyPrimaryHandlerOwner | undefined +): boolean { + return Boolean(owner?.active && primaryExitHandlerOwners.get(ptyId) === owner) +} + function isOwnerCurrent( owners: Map, ptyId: string, @@ -101,7 +131,7 @@ export function suspendPtyPrimaryHandlersForAdmission( ): PtyPrimaryHandlerAdmissionSnapshot { const snapshot: PtyPrimaryHandlerAdmissionSnapshot = { ptyId, - dataOwner: primaryDataHandlerOwners.get(ptyId), + dataOwner: suspendPtyPrimaryDataHandlerOwner(ptyId), exitOwner: primaryExitHandlerOwners.get(ptyId), dataHandler: registry.dataHandlers.get(ptyId), replayHandler: registry.replayHandlers.get(ptyId), @@ -112,7 +142,6 @@ export function suspendPtyPrimaryHandlersForAdmission( registry.replayHandlers.delete(ptyId) registry.exitHandlers.delete(ptyId) registry.teardownHandlers.delete(ptyId) - primaryDataHandlerOwners.delete(ptyId) primaryExitHandlerOwners.delete(ptyId) return snapshot } 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 new file mode 100644 index 00000000000..1f54eb5003b --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-primary-handler-registry.ts @@ -0,0 +1,79 @@ +import { clearPreHandlerPtyState } from './pty-pre-handler-buffer' +import { + getPtyPrimaryExitHandlerOwner, + isPtyPrimaryExitHandlerOwnerCurrent, + restorePtyPrimaryDataHandlerOwner, + suspendPtyPrimaryDataHandlerOwner +} from './pty-primary-handler-admission' +import type { PtyPrimaryHandlerOwner } from './pty-primary-handler-admission' + +export type PtyDataMeta = { + seq?: number + rawLength?: number + background?: boolean + /** Main dropped buffered output; the pane must repaint from main's snapshot. */ + droppedOutput?: boolean +} + +export const ptyDataHandlers = new Map void>() +export const ptyReplayHandlers = new Map void>() +export const ptyExitHandlers = new Map void>() +export const ptyTeardownHandlers = new Map void>() + +export type PtyDataHandlerShutdownSnapshot = { + ptyId: string + dataOwner?: PtyPrimaryHandlerOwner + exitOwner?: PtyPrimaryHandlerOwner + dataHandler?: (data: string, meta?: PtyDataMeta) => void + replayHandler?: (data: string) => void + teardownHandler?: () => void +} + +export function unregisterPtyDataHandlers(ptyIds: string[]): PtyDataHandlerShutdownSnapshot[] { + const snapshots: PtyDataHandlerShutdownSnapshot[] = [] + for (const id of ptyIds) { + snapshots.push({ + ptyId: id, + dataOwner: suspendPtyPrimaryDataHandlerOwner(id), + exitOwner: getPtyPrimaryExitHandlerOwner(id), + dataHandler: ptyDataHandlers.get(id), + replayHandler: ptyReplayHandlers.get(id), + teardownHandler: ptyTeardownHandlers.get(id) + }) + ptyDataHandlers.delete(id) + ptyReplayHandlers.delete(id) + ptyTeardownHandlers.get(id)?.() + ptyTeardownHandlers.delete(id) + clearPreHandlerPtyState(id) + } + return snapshots +} + +export function restorePtyDataHandlersAfterFailedShutdown( + snapshots: readonly PtyDataHandlerShutdownSnapshot[] +): void { + for (const snapshot of snapshots) { + const dataSlotsAvailable = + (!snapshot.dataHandler || !ptyDataHandlers.has(snapshot.ptyId)) && + (!snapshot.replayHandler || !ptyReplayHandlers.has(snapshot.ptyId)) + if ( + dataSlotsAvailable && + (snapshot.dataHandler || snapshot.replayHandler) && + restorePtyPrimaryDataHandlerOwner(snapshot.ptyId, snapshot.dataOwner) + ) { + if (snapshot.dataHandler) { + ptyDataHandlers.set(snapshot.ptyId, snapshot.dataHandler) + } + if (snapshot.replayHandler) { + ptyReplayHandlers.set(snapshot.ptyId, snapshot.replayHandler) + } + } + if ( + snapshot.teardownHandler && + !ptyTeardownHandlers.has(snapshot.ptyId) && + isPtyPrimaryExitHandlerOwnerCurrent(snapshot.ptyId, snapshot.exitOwner) + ) { + ptyTeardownHandlers.set(snapshot.ptyId, 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 4d6d72bda97..9f08086b5e5 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.test.ts @@ -454,6 +454,80 @@ describe('createIpcPtyTransport', () => { expect(nextTransport.isConnected()).toBe(false) }) + it('keeps reentrant replacement ownership atomic while attach drains buffered data', async () => { + const { createIpcPtyTransport, ensurePtyDispatcher } = await import('./pty-transport') + ensurePtyDispatcher() + onData?.({ id: 'same-id', data: 'first buffered chunk' }) + onData?.({ id: 'same-id', data: 'second buffered chunk' }) + const outerData: string[] = [] + const outerExit = vi.fn() + const replacementData = vi.fn() + const replacementExit = vi.fn() + const outerTransport = createIpcPtyTransport() + const replacementTransport = createIpcPtyTransport() + outerTransport.attach({ + existingPtyId: 'same-id', + callbacks: { + onData: (data) => { + outerData.push(data) + replacementTransport.attach({ + existingPtyId: 'same-id', + callbacks: { onData: replacementData, onExit: replacementExit } + }) + }, + onExit: outerExit + } + }) + + 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(replacementExit).toHaveBeenCalledWith(7) + expect(replacementTransport.isConnected()).toBe(false) + }) + + it('keeps reentrant replacement ownership atomic while connect drains buffered data', async () => { + const { createIpcPtyTransport, ensurePtyDispatcher } = await import('./pty-transport') + const spawn = window.api.pty.spawn as unknown as ReturnType + spawn.mockResolvedValueOnce({ id: 'same-id' }) + ensurePtyDispatcher() + onData?.({ id: 'same-id', data: 'first buffered chunk' }) + onData?.({ id: 'same-id', data: 'second buffered chunk' }) + const outerData: string[] = [] + const outerExit = vi.fn() + const replacementData = vi.fn() + const replacementExit = vi.fn() + const outerTransport = createIpcPtyTransport() + const replacementTransport = createIpcPtyTransport() + + const result = await outerTransport.connect({ + url: '', + callbacks: { + onData: (data) => { + outerData.push(data) + replacementTransport.attach({ + existingPtyId: 'same-id', + callbacks: { onData: replacementData, onExit: replacementExit } + }) + }, + 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(replacementExit).toHaveBeenCalledWith(8) + }) + it('ignores a stale exit for a previous PTY after reconnecting the same transport', async () => { const { createIpcPtyTransport } = await import('./pty-transport') const spawn = window.api.pty.spawn as unknown as ReturnType @@ -1970,6 +2044,43 @@ describe('createIpcPtyTransport', () => { expect(onDataCallback).toHaveBeenCalledWith('live again') }) + it('does not restore failed-shutdown handlers over a replacement pane', async () => { + const { + createIpcPtyTransport, + restorePtyDataHandlersAfterFailedShutdown, + unregisterPtyDataHandlers + } = await import('./pty-transport') + const { ptyDataHandlers, ptyReplayHandlers, ptyTeardownHandlers } = + await import('./pty-dispatcher') + const oldTransport = createIpcPtyTransport() + oldTransport.attach({ existingPtyId: 'same-id', callbacks: {} }) + const snapshots = unregisterPtyDataHandlers(['same-id']) + const replacementData = vi.fn() + const replacementReplay = vi.fn() + const replacementTransport = createIpcPtyTransport() + replacementTransport.attach({ + existingPtyId: 'same-id', + callbacks: { onData: replacementData, onReplayData: replacementReplay } + }) + const replacementHandlers = { + data: ptyDataHandlers.get('same-id'), + replay: ptyReplayHandlers.get('same-id'), + teardown: ptyTeardownHandlers.get('same-id') + } + + restorePtyDataHandlersAfterFailedShutdown(snapshots) + onData?.({ id: 'same-id', data: 'replacement live data' }) + onReplay?.({ id: 'same-id', data: 'replacement replay data' }) + + expect(ptyDataHandlers.get('same-id')).toBe(replacementHandlers.data) + expect(ptyReplayHandlers.get('same-id')).toBe(replacementHandlers.replay) + expect(ptyTeardownHandlers.get('same-id')).toBe(replacementHandlers.teardown) + expect(replacementData).toHaveBeenCalledWith('replacement live data') + expect(replacementReplay).toHaveBeenCalledWith('replacement replay data') + oldTransport.detach?.() + replacementTransport.disconnect() + }) + it('unregisterPtyDataHandlers cancels staleTitleTimer so it cannot fire stale idle transition', async () => { vi.useFakeTimers() try { diff --git a/src/renderer/src/components/terminal-pane/pty-transport.ts b/src/renderer/src/components/terminal-pane/pty-transport.ts index abe87ce8df6..fee7281dbb6 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.ts @@ -628,7 +628,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra // Why: shared by connect() and attach() to avoid duplicating title/bell/exit // logic across the two code paths that register a PTY. - function registerPtyDataHandler(id: string): void { + function registerPtyDataHandler(id: string) { // Why: relay pty.attach sends replay data via a dedicated pty:replay IPC // channel. Route it through onReplayData so the renderer engages the // replay guard and xterm auto-replies do not leak into the shell. @@ -660,14 +660,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra ptyDataHandlers.set(id, dataHandler) ownedDataAndReplayHandlers.set(id, { data: dataHandler, replay: replayHandler, owner }) publishPtyPrimaryDataHandlerOwner(id, owner) - drainPreHandlerPtyData( - id, - dataHandler, - () => - owner.active && - ownedDataAndReplayHandlers.get(id)?.owner === owner && - ptyDataHandlers.get(id) === dataHandler - ) + return { dataHandler, owner } } function clearAccumulatedState(): void { @@ -705,7 +698,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra } } - function registerPtyExitHandler(id: string): void { + function registerPtyExitHandler(id: string) { const exitHandler = (code: number): void => { if (ptyId !== null && ptyId !== id) { // Why: a preserved sleep/reconnect session can report its old exit @@ -732,16 +725,35 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra // would otherwise fire stale notifications after the data handler // is removed but before the exit event arrives. ptyTeardownHandlers.set(id, clearAccumulatedState) - if (!drainPreHandlerPtyExit(id, exitHandler)) { + return { exitHandler, owner } + } + + function drainRegisteredPtyHandlers( + id: string, + dataRegistration: ReturnType, + exitRegistration: ReturnType + ): boolean { + const isDataCurrent = (): boolean => + dataRegistration.owner.active && + ownedDataAndReplayHandlers.get(id)?.owner === dataRegistration.owner && + ptyDataHandlers.get(id) === dataRegistration.dataHandler + const isExitCurrent = (): boolean => + exitRegistration.owner.active && + ownedExitHandlers.get(id)?.owner === exitRegistration.owner && + ptyExitHandlers.get(id) === exitRegistration.exitHandler + drainPreHandlerPtyData(id, dataRegistration.dataHandler, isDataCurrent) + if (!isExitCurrent()) { + return false + } + if (!drainPreHandlerPtyExit(id, exitRegistration.exitHandler)) { reconcilePreHandlerPtyExitAfterOverflow( id, window.api.pty.hasPty, - exitHandler, - () => - ownedExitHandlers.get(id)?.handler === exitHandler && - ptyExitHandlers.get(id) === exitHandler + exitRegistration.exitHandler, + isExitCurrent ) } + return isDataCurrent() && isExitCurrent() } return { @@ -836,9 +848,16 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra onPtySpawn?.(spawnResult.id) } - registerPtyDataHandler(spawnResult.id) - registerPtyExitHandler(spawnResult.id) - if (!connected || ptyId !== spawnResult.id) { + const dataRegistration = registerPtyDataHandler(spawnResult.id) + const exitRegistration = registerPtyExitHandler(spawnResult.id) + const handlersCurrent = drainRegisteredPtyHandlers( + spawnResult.id, + dataRegistration, + exitRegistration + ) + if (!handlersCurrent || !connected || ptyId !== spawnResult.id) { + connected = false + ptyId = null return undefined } @@ -948,9 +967,12 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra connected = true // Why: skip onPtySpawn — it would reset lastActivityAt and destroy the // recency sort order that reconnectPersistedTerminals preserved. - registerPtyDataHandler(id) - registerPtyExitHandler(id) - if (!connected || ptyId !== id) { + const dataRegistration = registerPtyDataHandler(id) + const exitRegistration = registerPtyExitHandler(id) + const handlersCurrent = drainRegisteredPtyHandlers(id, dataRegistration, exitRegistration) + if (!handlersCurrent || !connected || ptyId !== id) { + connected = false + ptyId = null return }