From d30f59eefc3f3312e323273db2dde2ff2566bbf4 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 13 Jul 2026 13:44:26 -0700 Subject: [PATCH] fix(terminal): serialize renderer session admission --- .../pty-session-connect-admission.test.ts | 39 ++++++++++++++++ .../pty-session-connect-admission.ts | 29 ++++++++++++ .../terminal-pane/pty-transport.test.ts | 46 +++++++++++++++++++ .../components/terminal-pane/pty-transport.ts | 19 +++++--- 4 files changed, 127 insertions(+), 6 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/pty-session-connect-admission.test.ts create mode 100644 src/renderer/src/components/terminal-pane/pty-session-connect-admission.ts diff --git a/src/renderer/src/components/terminal-pane/pty-session-connect-admission.test.ts b/src/renderer/src/components/terminal-pane/pty-session-connect-admission.test.ts new file mode 100644 index 00000000000..d9aaf9ba50b --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-session-connect-admission.test.ts @@ -0,0 +1,39 @@ +import { afterEach, describe, expect, it } from 'vitest' +import { + acquirePtySessionConnectAdmission, + getPtySessionConnectAdmissionCountForTest +} from './pty-session-connect-admission' + +describe('PTY session connect admission', () => { + afterEach(() => { + expect(getPtySessionConnectAdmissionCountForTest()).toBe(0) + }) + + it('serializes one session without blocking a different session', async () => { + const releaseFirst = await acquirePtySessionConnectAdmission('same-id') + let secondAdmitted = false + const secondAdmission = acquirePtySessionConnectAdmission('same-id').then((release) => { + secondAdmitted = true + return release + }) + const releaseDifferent = await acquirePtySessionConnectAdmission('different-id') + + await Promise.resolve() + expect(secondAdmitted).toBe(false) + expect(getPtySessionConnectAdmissionCountForTest()).toBe(2) + + releaseFirst() + const releaseSecond = await secondAdmission + releaseSecond() + releaseDifferent() + }) + + it('releases every retained identity after repeated admissions', async () => { + for (let index = 0; index < 100; index += 1) { + const release = await acquirePtySessionConnectAdmission(`session-${index}`) + release() + } + + expect(getPtySessionConnectAdmissionCountForTest()).toBe(0) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/pty-session-connect-admission.ts b/src/renderer/src/components/terminal-pane/pty-session-connect-admission.ts new file mode 100644 index 00000000000..65ca763904d --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-session-connect-admission.ts @@ -0,0 +1,29 @@ +const sessionAdmissionTails = new Map>() + +export async function acquirePtySessionConnectAdmission(sessionId: string): Promise<() => void> { + const predecessor = sessionAdmissionTails.get(sessionId) ?? Promise.resolve() + let releaseCurrent!: () => void + const current = new Promise((resolve) => { + releaseCurrent = resolve + }) + const tail = predecessor.then(() => current) + sessionAdmissionTails.set(sessionId, tail) + + await predecessor + let released = false + return () => { + if (released) { + return + } + released = true + releaseCurrent() + // Why: a settled predecessor must not delete a newer queued owner. + if (sessionAdmissionTails.get(sessionId) === tail) { + sessionAdmissionTails.delete(sessionId) + } + } +} + +export function getPtySessionConnectAdmissionCountForTest(): number { + return sessionAdmissionTails.size +} 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 97596bfba84..e292a4c8f0c 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.test.ts @@ -147,6 +147,52 @@ describe('createIpcPtyTransport', () => { expect(onDataCallback).not.toHaveBeenCalledWith('dead generation output') }) + it('preserves replacement startup events while an older same-id admission rejects', async () => { + const { createIpcPtyTransport } = await import('./pty-transport') + const spawn = window.api.pty.spawn as unknown as ReturnType + let rejectFirstSpawn: ((error: Error) => void) | undefined + let resolveReplacementSpawn: ((result: { id: string }) => void) | undefined + spawn + .mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectFirstSpawn = reject + }) + ) + .mockImplementationOnce(() => { + onData?.({ id: 'same-id', data: 'replacement startup output' }) + onExit?.({ id: 'same-id', code: 23 }) + return new Promise<{ id: string }>((resolve) => { + resolveReplacementSpawn = resolve + }) + }) + const firstTransport = createIpcPtyTransport({}) + const replacementData = vi.fn() + const replacementExit = vi.fn() + const replacementTransport = createIpcPtyTransport({}) + + const firstConnect = firstTransport.connect({ + url: '', + sessionId: 'same-id', + callbacks: {} + }) + const replacementConnect = replacementTransport.connect({ + url: '', + sessionId: 'same-id', + callbacks: { onData: replacementData, onExit: replacementExit } + }) + await vi.waitFor(() => expect(rejectFirstSpawn).toBeTypeOf('function')) + + rejectFirstSpawn?.(new Error('Daemon PTY exited during admission for "same-id"')) + await firstConnect + await vi.waitFor(() => expect(spawn).toHaveBeenCalledTimes(2)) + resolveReplacementSpawn?.({ id: 'same-id' }) + await replacementConnect + + expect(replacementData).toHaveBeenCalledWith('replacement startup output') + expect(replacementExit).toHaveBeenCalledWith(23) + }) + 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 diff --git a/src/renderer/src/components/terminal-pane/pty-transport.ts b/src/renderer/src/components/terminal-pane/pty-transport.ts index 790ece864eb..e42fb36fc2b 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.ts @@ -29,6 +29,7 @@ import { reconcilePreHandlerPtyExitAfterOverflow } from './pty-pre-handler-buffer' import { createPtyInputWriteQueue } from './pty-input-write-queue' +import { acquirePtySessionConnectAdmission } from './pty-session-connect-admission' import type { PtyDataMeta } from './pty-dispatcher' import type { IpcPtyTransportOptions, PtyConnectResult, PtyTransport } from './pty-transport-types' import { createBellDetector } from '../../../../shared/terminal-bell-detector' @@ -700,14 +701,18 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra return { async connect(options) { - storedCallbacks = options.callbacks ensurePtyDispatcher() - - if (destroyed) { - return - } - + // Why: rejected-generation cleanup is ID-scoped, so a replacement with + // the same session ID must not publish until its predecessor releases. + const releaseSessionAdmission = options.sessionId + ? await acquirePtySessionConnectAdmission(options.sessionId) + : undefined try { + storedCallbacks = options.callbacks + if (destroyed) { + return + } + // Why: missing-cwd recovery is only valid for fresh local spawns — // reattach must keep the session's exact cwd and SSH-tagged transports // resolve cwd on the remote host. @@ -856,6 +861,8 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra storedCallbacks.onError?.(msg) } return undefined + } finally { + releaseSessionAdmission?.() } },