From 52c7de1adb051aae511b61fb014ec27b898a461c Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 9 Jul 2026 03:11:27 -0700 Subject: [PATCH] fix(terminal): guard PTY handler unregistration against detach/attach remount races (frozen pane) (#7894) Co-authored-by: Orca --- .../terminal-pane/pty-dispatcher.ts | 8 ++- .../terminal-pane/pty-pre-handler-buffer.ts | 15 +++++ .../terminal-pane/pty-transport.test.ts | 59 +++++++++++++++++++ .../components/terminal-pane/pty-transport.ts | 44 +++++++++++--- 4 files changed, 116 insertions(+), 10 deletions(-) diff --git a/src/renderer/src/components/terminal-pane/pty-dispatcher.ts b/src/renderer/src/components/terminal-pane/pty-dispatcher.ts index f27b23318..f8333edac 100644 --- a/src/renderer/src/components/terminal-pane/pty-dispatcher.ts +++ b/src/renderer/src/components/terminal-pane/pty-dispatcher.ts @@ -246,8 +246,12 @@ export function registerEagerPtyBuffer( 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(). - ptyDataHandlers.delete(ptyId) - ptyReplayHandlers.delete(ptyId) + // Identity-guarded like dispose(): never delete a handler a transport has + // since registered for this id. + if (ptyDataHandlers.get(ptyId) === dataHandler) { + ptyDataHandlers.delete(ptyId) + ptyReplayHandlers.delete(ptyId) + } ptyExitHandlers.delete(ptyId) eagerPtyHandles.delete(ptyId) onExit(ptyId, code) 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 2756f5fc1..23c4661ad 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 @@ -15,6 +15,11 @@ const preHandlerPtyExit = new Map() // and dropping the first setup-script bytes. const PRE_HANDLER_PTY_DATA_MAX_BYTES = 512 * 1024 const PRE_HANDLER_PTY_DATA_MAX_PTYS = 64 +// Why: legit pre-attach windows drain within milliseconds and hold little +// data. Sustained accumulation means a pane lost its data handler (the +// frozen-pane detach/attach race) — leave a breadcrumb for trace capture. +const PRE_HANDLER_PTY_DATA_WARN_BYTES = 64 * 1024 +const warnedLostHandlerPtyIds = new Set() export function bufferPreHandlerPtyData(ptyId: string, data: string, meta?: PtyDataMeta): void { const chunk = clampUtf8Tail(data, PRE_HANDLER_PTY_DATA_MAX_BYTES) @@ -42,6 +47,13 @@ export function bufferPreHandlerPtyData(ptyId: string, data: string, meta?: PtyD totalBytes -= chunks.shift()?.bytes ?? 0 } preHandlerPtyData.set(ptyId, chunks) + if (totalBytes > PRE_HANDLER_PTY_DATA_WARN_BYTES && !warnedLostHandlerPtyIds.has(ptyId)) { + warnedLostHandlerPtyIds.add(ptyId) + console.warn( + `[pty] ${ptyId}: ${totalBytes} bytes buffered with no registered data handler; ` + + 'the owning pane may have lost its handler to a detach/attach race' + ) + } } export function drainPreHandlerPtyData( @@ -49,6 +61,7 @@ export function drainPreHandlerPtyData( handler: (data: string, meta?: PtyDataMeta) => void ): void { const chunks = preHandlerPtyData.get(ptyId) + warnedLostHandlerPtyIds.delete(ptyId) if (!chunks) { return } @@ -73,9 +86,11 @@ export function drainPreHandlerPtyExit(ptyId: string, handler: (code: number) => export function clearPreHandlerPtyData(ptyId: string): void { preHandlerPtyData.delete(ptyId) + warnedLostHandlerPtyIds.delete(ptyId) } export function clearPreHandlerPtyState(ptyId: string): void { preHandlerPtyData.delete(ptyId) preHandlerPtyExit.delete(ptyId) + warnedLostHandlerPtyIds.delete(ptyId) } 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 a3c9f51ee..e68a5f9a2 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.test.ts @@ -137,6 +137,65 @@ describe('createIpcPtyTransport', () => { expect(onReplayData).toHaveBeenCalledWith('new replay') }) + it('keeps the live handler when detach() runs after a newer transport attached to the same PTY', async () => { + // Why: pane->tab detach and split-group moves rehome the React subtree, so + // the NEW TerminalPane can attach to the same ptyId BEFORE the old pane's + // unmount detach() runs. An unconditional unregister deletes the live + // handler and the pane freezes with the PTY still alive (frozen-pane bug). + const { createIpcPtyTransport } = await import('./pty-transport') + const receivedByNewPane = vi.fn() + const replayedToNewPane = vi.fn() + const exitSeenByNewPane = vi.fn() + const receivedByOldPane = vi.fn() + + const oldPane = createIpcPtyTransport({}) + await oldPane.connect({ url: '', callbacks: { onData: receivedByOldPane } }) + + const newPane = createIpcPtyTransport({}) + newPane.attach?.({ + existingPtyId: 'pty-1', + callbacks: { + onData: receivedByNewPane, + onReplayData: replayedToNewPane, + onExit: exitSeenByNewPane + } + }) + oldPane.detach?.() + + onData?.({ id: 'pty-1', data: 'live output' }) + onReplay?.({ id: 'pty-1', data: 'replay output' }) + + expect(receivedByNewPane).toHaveBeenCalledWith('live output') + expect(replayedToNewPane).toHaveBeenCalledWith('replay output') + expect(receivedByOldPane).not.toHaveBeenCalled() + + onExit?.({ id: 'pty-1', code: 0 }) + expect(exitSeenByNewPane).toHaveBeenCalledWith(0) + }) + + it('buffers data across a normal detach-then-attach gap and drains it to the next pane', async () => { + const { createIpcPtyTransport } = await import('./pty-transport') + const receivedByNewPane = vi.fn() + + const oldPane = createIpcPtyTransport({}) + await oldPane.connect({ url: '', callbacks: { onData: vi.fn() } }) + oldPane.detach?.() + + onData?.({ id: 'pty-1', data: 'buffered while detached' }) + expect(receivedByNewPane).not.toHaveBeenCalled() + + const newPane = createIpcPtyTransport({}) + newPane.attach?.({ + existingPtyId: 'pty-1', + callbacks: { onData: receivedByNewPane } + }) + + expect(receivedByNewPane).toHaveBeenCalledWith('buffered while detached') + + onData?.({ id: 'pty-1', data: 'live after reattach' }) + expect(receivedByNewPane).toHaveBeenCalledWith('live after reattach') + }) + it('exposes the connection identity captured at transport creation', async () => { const { createIpcPtyTransport } = await import('./pty-transport') diff --git a/src/renderer/src/components/terminal-pane/pty-transport.ts b/src/renderer/src/components/terminal-pane/pty-transport.ts index 71999b713..7df168d76 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.ts @@ -502,16 +502,41 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra }) let storedCallbacks: Parameters[0]['callbacks'] = {} + // Why: pane->tab detach / split-group moves rehome the React subtree, so a + // NEW TerminalPane can attach to the same ptyId before the OLD instance's + // detach() runs. Track the handlers THIS instance registered so unregister + // paths only delete map entries they still own — an unconditional delete + // destroys the live handler and the pane freezes (data diverts into the + // pre-handler buffer forever). + const ownedDataAndReplayHandlers = new Map< + string, + { data: (data: string, meta?: PtyDataMeta) => void; replay: (data: string) => void } + >() + const ownedExitHandlers = new Map void>() + function unregisterPtyHandlers(id: string): void { - ptyDataHandlers.delete(id) - ptyReplayHandlers.delete(id) - ptyExitHandlers.delete(id) - ptyTeardownHandlers.delete(id) + unregisterPtyDataAndStatusHandlers(id) + const ownedExit = ownedExitHandlers.get(id) + if (ownedExit && ptyExitHandlers.get(id) === ownedExit) { + ptyExitHandlers.delete(id) + } + ownedExitHandlers.delete(id) + if (ptyTeardownHandlers.get(id) === clearAccumulatedState) { + ptyTeardownHandlers.delete(id) + } } function unregisterPtyDataAndStatusHandlers(id: string): void { - ptyDataHandlers.delete(id) - ptyReplayHandlers.delete(id) + const owned = ownedDataAndReplayHandlers.get(id) + if (owned) { + if (ptyDataHandlers.get(id) === owned.data) { + ptyDataHandlers.delete(id) + } + if (ptyReplayHandlers.get(id) === owned.replay) { + ptyReplayHandlers.delete(id) + } + } + ownedDataAndReplayHandlers.delete(id) } // Why: shared by connect() and attach() to avoid duplicating title/bell/exit @@ -520,7 +545,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra // 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. - ptyReplayHandlers.set(id, (data) => { + const replayHandler = (data: string): void => { if (ptyId !== id) { return } @@ -529,7 +554,8 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra } else { storedCallbacks.onData?.(data) } - }) + } + ptyReplayHandlers.set(id, replayHandler) const dataHandler = (data: string, meta?: PtyDataMeta): void => { if (ptyId !== id) { return @@ -544,6 +570,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra ) } ptyDataHandlers.set(id, dataHandler) + ownedDataAndReplayHandlers.set(id, { data: dataHandler, replay: replayHandler }) drainPreHandlerPtyData(id, dataHandler) } @@ -599,6 +626,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra onPtyExit?.(id) } ptyExitHandlers.set(id, exitHandler) + ownedExitHandlers.set(id, exitHandler) // Why: shutdownWorktreeTerminals bypasses the transport layer — it // kills PTYs directly via IPC without calling disconnect()/destroy(). // This teardown callback lets unregisterPtyDataHandlers cancel