From ffdb2e990005ce7800814df2c1c4ed0922fe77ed Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Mon, 7 Sep 2026 11:52:30 -0700 Subject: [PATCH] fix(terminal): classify a startup-carried provider session as a resume spawn The watchdog's resume exclusion keyed on the cold-restore override alone, but spawnIpcPty falls back to the transport options for resumeProviderSession. The sidebar 'resume a sleeping agent' flow clears its sleeping record at tab creation, so the pane mounts into a plain startFreshSpawn() with no override while main still re-issues --resume. A hung spawn there was remounted into a second --resume on one transcript. Carry the exclusion on the spawn promise rather than the arming call, so a remount that adopts a pending spawn cannot lose it. --- .../deferred-session-reattach-choice.ts | 5 +- .../pty-connection/fresh-spawn-start.test.ts | 84 +++++++++++++++++++ .../pty-connection/fresh-spawn-start.ts | 8 +- .../unbound-pane-spawn-recovery.test.ts | 16 ++++ .../unbound-pane-spawn-recovery.ts | 18 +++- 5 files changed, 124 insertions(+), 7 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-start.test.ts diff --git a/src/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-choice.ts b/src/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-choice.ts index 95e5dc01f31..069af396560 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-choice.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/deferred-session-reattach-choice.ts @@ -172,8 +172,9 @@ export function runDeferredSessionReattachChoice(session: ConnectPanePtySession) } recordPtyConnectDiagnostic(`pane=${session.pane.id} -> PENDING SPAWN`) session.armDirectSshPaneRetryTimeout(pendingSpawn, session.directSshRetryAttempt) - // Why re-arm: the adopting instance needs its own settlement clock, or a hung - // spawn it inherited would freeze this pane with no timer of its own. + // Why re-arm: a spawn whose arming pane was disposed before it could arm has + // no clock at all, and this pane would inherit the freeze. Already-armed + // spawns no-op, and the resume exclusion rides the promise, not this call. armSpawnSettlementWatchdog(session, pendingSpawn) void pendingSpawn .then((spawnedPtyId) => { diff --git a/src/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-start.test.ts b/src/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-start.test.ts new file mode 100644 index 00000000000..f8bdba12bec --- /dev/null +++ b/src/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-start.test.ts @@ -0,0 +1,84 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { bindStartFreshSpawn } from './fresh-spawn-start' +import { observeSpawnSettlement } from './unbound-pane-spawn-recovery' +import { pendingSpawnByPaneKey, pendingSpawnGenerationByPaneKey } from './pty-connect-limits' + +vi.mock('./unbound-pane-spawn-recovery', () => ({ observeSpawnSettlement: vi.fn() })) +vi.mock('@/lib/pane-manager/pane-terminal-output-scheduler', () => ({ + writeTerminalOutput: vi.fn() +})) +vi.mock('../pty-buffer-serializer', () => ({ hasPtySerializer: vi.fn(() => false) })) +vi.mock('@/store', () => ({ + useAppStore: { getState: () => ({ deleteStateByWorktreeId: {}, getTab: () => null }) } +})) + +function buildSession(overrides: Record = {}): never { + return { + deps: { tabId: 'tab-1', worktreeId: 'wt-1', cwd: '/w', isVisibleRef: { current: true } }, + pane: { id: 1, leafId: 'leaf-1', terminal: {} }, + // Non-null keeps the spawn off the serializer pre-signal, which is not under test. + runtimeEnvironmentId: 'rt-1', + transportOptions: {}, + disposed: false, + pendingSpawnKey: 'pane-key', + tabGeneration: 0, + cols: 80, + rows: 24, + authoritativeReattachGeneration: 0, + transportStreamGeneration: 0, + kittyKeyboardModes: { reset: vi.fn() }, + isLegacyWorkerAutomaticResumeBlocked: () => false, + clearPaneMode2031State: vi.fn(), + clearHiddenOutputRestoreState: vi.fn(), + resetFreshSpawnFollowOutput: vi.fn(), + prepareFreshShellViewportForSpawn: vi.fn(), + shouldDeclareHiddenAtSpawn: () => false, + captureTransportOutputCallbacks: () => ({ generation: 0, callbacks: {} }), + armDirectSshPaneRetryTimeout: vi.fn(), + reportError: vi.fn(), + finishReattachLiveDataDeferral: vi.fn(), + transport: { connect: vi.fn(() => new Promise(() => {})), getPtyId: () => null }, + ...overrides + } as never +} + +function resumesProviderSessionForSpawn(session: never): boolean { + bindStartFreshSpawn(session) + void (session as unknown as { startFreshSpawn: () => void }).startFreshSpawn() + return vi.mocked(observeSpawnSettlement).mock.calls[0]?.[2]?.resumesProviderSession === true +} + +describe('bindStartFreshSpawn resume classification', () => { + beforeEach(() => { + vi.clearAllMocks() + pendingSpawnByPaneKey.clear() + pendingSpawnGenerationByPaneKey.clear() + }) + + // The sidebar "resume a sleeping agent" flow clears its sleeping record at tab + // creation, so the pane mounts into a PLAIN startFreshSpawn() with no + // cold-restore override — while spawnIpcPty still replays the provider session + // off the transport options. Classifying that as non-resume lets a hung spawn + // be remounted into a SECOND --resume on one transcript. + it('treats a startup-carried provider session as a resume spawn', () => { + const session = buildSession({ + transportOptions: { resumeProviderSession: { key: 'session_id', id: 'sess-1' } } + }) + + expect(resumesProviderSessionForSpawn(session)).toBe(true) + }) + + it('treats a plain spawn as a non-resume spawn', () => { + expect(resumesProviderSessionForSpawn(buildSession())).toBe(false) + }) + + it('treats an explicit cold-restore override as a resume spawn', () => { + const session = buildSession() + bindStartFreshSpawn(session) + void (session as unknown as { startFreshSpawn: (startup: unknown) => void }).startFreshSpawn({ + launchConfig: { agent: 'claude' } + }) + + expect(vi.mocked(observeSpawnSettlement).mock.calls[0]?.[2]?.resumesProviderSession).toBe(true) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-start.ts b/src/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-start.ts index 04e8422b974..b4ed2df6fce 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-start.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/fresh-spawn-start.ts @@ -321,7 +321,13 @@ export function bindStartFreshSpawn(session: ConnectPanePtySession): void { }) session.armDirectSshPaneRetryTimeout(trackedPromise, session.directSshRetryAttempt) observeSpawnSettlement(session, trackedPromise, { - resumesProviderSession: Boolean(coldRestoreOverride) + // Why the transport options too: spawnIpcPty falls back to them for + // `resumeProviderSession`, so a pane whose startup carries a provider + // session (sidebar resume of a sleeping agent) re-issues --resume on + // EVERY fresh spawn — with no cold-restore override to mark it. + resumesProviderSession: Boolean( + coldRestoreOverride ?? session.transportOptions?.resumeProviderSession + ) }) // Why: split panes in the same tab can spawn concurrently. Key by pane // as well as tab so a remount cannot attach to a sibling setup pane's PTY. diff --git a/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.test.ts b/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.test.ts index a59c0928b61..78d7cc7e85c 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.test.ts @@ -261,6 +261,22 @@ describe('cold-restore resume spawns', () => { expect(pendingSpawnByPaneKey.has('resume-key')).toBe(false) }) + // The adopting remount arms with no options of its own. If the exclusion rode + // the arming call instead of the spawn, this pane would re-issue --resume. + it('never remounts a resume spawn adopted by a remount whose arming pane was disposed', () => { + const promise = new Promise(() => {}) + observeSpawnSettlement( + buildSession({ deps: { tabId: 'tab-disposed' }, disposed: true }), + promise, + { resumesProviderSession: true } + ) + + armSpawnSettlementWatchdog(buildSession({ deps: { tabId: 'tab-adopter' } }), promise) + vi.advanceTimersByTime(TRANSPORT_CONNECT_SETTLE_GRACE_MS) + + expect(requestTerminalPaneRecovery).not.toHaveBeenCalled() + }) + it('still remounts a non-resume spawn that hangs', () => { observeSpawnSettlement( buildSession({ deps: { tabId: 'tab-plain' }, pendingSpawnKey: 'plain-key' }), diff --git a/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.ts b/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.ts index 71eab2276b6..772d56c6af1 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection/unbound-pane-spawn-recovery.ts @@ -14,6 +14,11 @@ import type { ConnectPanePtySession } from './connect-pane-pty-session' // Why a module-level set: a promise is already a unique identity, so arming from // both the fresh spawn and a remount's pending-spawn adoption cannot double-time it. const spawnSettlementTimedPromises = new WeakSet>() +// Why keyed on the spawn and not on the arming call: a remount adopts a pending +// spawn it did not start, so a flag passed by the caller would be lost exactly +// when the arming pane was disposed before it could arm — the one case where the +// adopter's own arming is what runs. +const resumeShapedSpawns = new WeakSet>() function remountUnboundPane( session: ConnectPanePtySession, @@ -68,7 +73,12 @@ export function observeSpawnSettlement( trackedPromise: Promise, options: { resumesProviderSession?: boolean } = {} ): void { - armSpawnSettlementWatchdog(session, trackedPromise, options) + // Recorded before arming, so a pane disposed too early to arm still hands the + // flag to whichever remount adopts this spawn. + if (options.resumesProviderSession === true) { + resumeShapedSpawns.add(trackedPromise) + } + armSpawnSettlementWatchdog(session, trackedPromise) void trackedPromise.then((spawnedPtyId) => { if (spawnedPtyId) { return @@ -98,8 +108,7 @@ export function observeSpawnSettlement( * pane-key entry forever, which also freezes any remount that adopts it. */ export function armSpawnSettlementWatchdog( session: ConnectPanePtySession, - trackedPromise: Promise, - options: { resumesProviderSession?: boolean } = {} + trackedPromise: Promise ): void { if (session.disposed || spawnSettlementTimedPromises.has(trackedPromise)) { return @@ -121,10 +130,11 @@ export function armSpawnSettlementWatchdog( // (it may own a recycled id). Remounting a hung one would put a SECOND // --resume on the same transcript. A stuck pane is recoverable; two agents // writing one conversation is not. - if (options.resumesProviderSession === true) { + if (resumeShapedSpawns.has(trackedPromise)) { warnTerminalLifecycleAnomaly('resume spawn never settled; remount withheld', { tabId, worktreeId: session.deps.worktreeId, + leafId: session.deps.restoredLeafId ?? session.pane.leafId, paneId: session.pane.id, ptyId: null })