diff --git a/src/renderer/src/components/terminal-pane/pty-data-sidecar-subscriptions.ts b/src/renderer/src/components/terminal-pane/pty-data-sidecar-subscriptions.ts index 3625a8e6007..a25a78c8350 100644 --- a/src/renderer/src/components/terminal-pane/pty-data-sidecar-subscriptions.ts +++ b/src/renderer/src/components/terminal-pane/pty-data-sidecar-subscriptions.ts @@ -1,5 +1,6 @@ import { acquirePtyDeliveryInterest } from './pty-delivery-interest' import { ensurePtyDispatcher, ptyDataSidecars } from './pty-dispatcher' +import { syncTerminalDeliveryWatchdogTimer } from './terminal-delivery-watchdog' /** Register a side-channel data watcher for a PTY without taking ownership * of the primary handler. Returns an unsubscribe fn. */ @@ -15,6 +16,8 @@ export function subscribeToPtyData(ptyId: string, watcher: (data: string) => voi ptyDataSidecars.set(ptyId, set) } set.add(watcher) + // Why: a sidecar receives pushed bytes with no primary handler, so it must arm the delivery watchdog. + syncTerminalDeliveryWatchdogTimer() return () => { releaseDeliveryInterest() const current = ptyDataSidecars.get(ptyId) @@ -25,5 +28,6 @@ export function subscribeToPtyData(ptyId: string, watcher: (data: string) => voi if (current.size === 0) { ptyDataSidecars.delete(ptyId) } + syncTerminalDeliveryWatchdogTimer() } } diff --git a/src/renderer/src/components/terminal-pane/pty-dispatcher-delivery-interest.test.ts b/src/renderer/src/components/terminal-pane/pty-dispatcher-delivery-interest.test.ts index c8f84865fed..19104ecdf63 100644 --- a/src/renderer/src/components/terminal-pane/pty-dispatcher-delivery-interest.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-dispatcher-delivery-interest.test.ts @@ -89,6 +89,31 @@ describe('pty dispatcher delivery interest', () => { expect(setPtyDeliveryInterest).not.toHaveBeenCalled() }) + it('arms the delivery watchdog for a sidecar-only PTY and disarms on the last unsubscribe', async () => { + // Why: a parked terminal (and draft-readiness / automation observers) receives pushed bytes + // through a sidecar with no primary handler — the watchdog must still cover that dead-channel case. + vi.useFakeTimers() + const reportMock = vi.fn(() => + Promise.resolve({ inFlightTotalChars: 0, inFlightPtyCount: 0, msSinceLastAck: null }) + ) + ;(window.api.pty as { reportRendererDeliveryState?: unknown }).reportRendererDeliveryState = + reportMock + try { + const { subscribeToPtyData } = await import('./pty-data-sidecar-subscriptions') + + const unsubscribe = subscribeToPtyData('pty-1', vi.fn()) + await vi.advanceTimersByTimeAsync(15_000) + expect(reportMock).toHaveBeenCalled() + + reportMock.mockClear() + unsubscribe() + await vi.advanceTimersByTimeAsync(15_000 * 2) + expect(reportMock).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + it('lets a sidecar exclusively own interest while an eager buffer overlaps', async () => { const { registerEagerPtyBuffer } = await import('./pty-dispatcher') const { subscribeToPtyData } = await import('./pty-data-sidecar-subscriptions') diff --git a/src/renderer/src/components/terminal-pane/pty-dispatcher-push-reattach.test.ts b/src/renderer/src/components/terminal-pane/pty-dispatcher-push-reattach.test.ts index 7430231bf04..499be934476 100644 --- a/src/renderer/src/components/terminal-pane/pty-dispatcher-push-reattach.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-dispatcher-push-reattach.test.ts @@ -64,6 +64,30 @@ describe('pty dispatcher push-listener reattach and delivery blackhole', () => { } }) + it('arms for eager PTYs and disarms only after the last eager PTY is disposed', async () => { + vi.useFakeTimers() + try { + const { ensurePtyDispatcher, registerEagerPtyBuffer } = await import('./pty-dispatcher') + ensurePtyDispatcher() + expect(vi.getTimerCount()).toBe(0) + + const first = registerEagerPtyBuffer('pty-eager-1', vi.fn()) + expect(vi.getTimerCount()).toBe(1) + const second = registerEagerPtyBuffer('pty-eager-2', vi.fn()) + expect(vi.getTimerCount()).toBe(1) + + await vi.advanceTimersByTimeAsync(30_000) + expect(reportMock).toHaveBeenCalledTimes(2) + + first.dispose() + expect(vi.getTimerCount()).toBe(1) + second.dispose() + expect(vi.getTimerCount()).toBe(0) + } finally { + vi.useRealTimers() + } + }) + it('blackholed delivery reproduces the wedge: no handler dispatch, no ACK ever', async () => { const { ensurePtyDispatcher, ptyDataHandlers } = await import('./pty-dispatcher') ensurePtyDispatcher() diff --git a/src/renderer/src/components/terminal-pane/pty-dispatcher.ts b/src/renderer/src/components/terminal-pane/pty-dispatcher.ts index 7386797b4bb..56f9df54930 100644 --- a/src/renderer/src/components/terminal-pane/pty-dispatcher.ts +++ b/src/renderer/src/components/terminal-pane/pty-dispatcher.ts @@ -18,7 +18,8 @@ import { clearReceivedPtyCharTotal, isPtyPushDeliveryBlackholed, recordPtyDataReceived, - startTerminalDeliveryWatchdog + startTerminalDeliveryWatchdog, + syncTerminalDeliveryWatchdogTimer } from './terminal-delivery-watchdog' import { recordTerminalFreezeBreadcrumb } from './terminal-freeze-breadcrumbs' import { installTerminalFreezeReport } from './terminal-freeze-report' @@ -76,6 +77,7 @@ export function unregisterPtyDataHandlers(ptyIds: string[]): PtyDataHandlerShutd ptyTeardownHandlers.delete(id) clearPreHandlerPtyState(id) } + syncTerminalDeliveryWatchdogTimer() return snapshots } @@ -93,6 +95,7 @@ export function restorePtyDataHandlersAfterFailedShutdown( ptyTeardownHandlers.set(snapshot.ptyId, snapshot.teardownHandler) } } + syncTerminalDeliveryWatchdogTimer() } let pushListenerUnsubscribes: (() => void)[] = [] @@ -120,7 +123,10 @@ export function ensurePtyDispatcher(): void { attachPtyPushListeners() startTerminalDeliveryWatchdog({ reattachPushListeners: reattachPtyDispatcherPushListeners, - hasAttachedPtys: () => ptyDataHandlers.size > 0 || eagerPtyHandles.size > 0 + // Why: sidecar consumers (parked terminals, draft-readiness, automation observers) hold main-side + // delivery interest and receive pushed bytes with no primary handler, so they need the watchdog too. + hasAttachedPtys: () => + ptyDataHandlers.size > 0 || eagerPtyHandles.size > 0 || ptyDataSidecars.size > 0 }) } @@ -296,13 +302,7 @@ export function registerEagerPtyBuffer( } } const exitHandler = (code: number): void => { - // Shell died before attach; identity-guard so we never evict a handler a transport re-registered for this id (#7894 detach/attach race). - if (ptyDataHandlers.get(ptyId) === dataHandler) { - ptyDataHandlers.delete(ptyId) - ptyReplayHandlers.delete(ptyId) - } - ptyExitHandlers.delete(ptyId) - eagerPtyHandles.delete(ptyId) + handle.dispose() onExit(ptyId, code) } @@ -330,10 +330,12 @@ export function registerEagerPtyBuffer( ptyExitHandlers.delete(ptyId) } eagerPtyHandles.delete(ptyId) + syncTerminalDeliveryWatchdogTimer() } } eagerPtyHandles.set(ptyId, handle) + syncTerminalDeliveryWatchdogTimer() drainPreHandlerPtyData(ptyId, dataHandler) // Why: defer the pre-handler exit one microtask so the caller receives the returned handle before onExit fires. queueMicrotask(() => { 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 34ec9ac7567..3e109a4216a 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.test.ts @@ -79,6 +79,41 @@ describe('createIpcPtyTransport', () => { transport.disconnect() }) + it('arms for mounted PTYs and disarms only after the last transport detaches', async () => { + vi.useFakeTimers() + const reportRendererDeliveryState = vi.fn().mockResolvedValue({ + inFlightTotalChars: 0, + inFlightPtyCount: 0, + msSinceLastAck: null + }) + window.api.pty.reportRendererDeliveryState = reportRendererDeliveryState + try { + const { createIpcPtyTransport, ensurePtyDispatcher } = await import('./pty-transport') + const spawn = window.api.pty.spawn as unknown as ReturnType + spawn.mockResolvedValueOnce({ id: 'pty-1' }).mockResolvedValueOnce({ id: 'pty-2' }) + + ensurePtyDispatcher() + expect(vi.getTimerCount()).toBe(0) + + const first = createIpcPtyTransport({}) + await first.connect({ url: '', callbacks: {} }) + expect(vi.getTimerCount()).toBe(1) + + const second = createIpcPtyTransport({}) + await second.connect({ url: '', callbacks: {} }) + expect(vi.getTimerCount()).toBe(1) + + first.detach?.() + expect(vi.getTimerCount()).toBe(1) + second.detach?.() + expect(vi.getTimerCount()).toBe(0) + } finally { + const { stopTerminalDeliveryWatchdog } = await import('./terminal-delivery-watchdog') + stopTerminalDeliveryWatchdog() + vi.useRealTimers() + } + }) + it('does not create a second kill authority when a mounted pane detaches', async () => { const { createIpcPtyTransport } = await import('./pty-transport') const kill = window.api.pty.kill 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 8e1a0380a58..6fa6595b594 100644 --- a/src/renderer/src/components/terminal-pane/pty-transport.ts +++ b/src/renderer/src/components/terminal-pane/pty-transport.ts @@ -11,6 +11,7 @@ import { iterateTerminalInputChunks } from '../../../../shared/terminal-input' import { isRuntimeOwnedSshTargetId } from '../../../../shared/execution-host' +import { syncTerminalDeliveryWatchdogTimer } from './terminal-delivery-watchdog' import { ptyDataHandlers, ptyReplayHandlers, @@ -542,6 +543,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra } } ownedDataAndReplayHandlers.delete(id) + syncTerminalDeliveryWatchdogTimer() } function registerPtyDataHandler(id: string): void { @@ -572,6 +574,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra } ptyDataHandlers.set(id, dataHandler) ownedDataAndReplayHandlers.set(id, { data: dataHandler, replay: replayHandler }) + syncTerminalDeliveryWatchdogTimer() drainPreHandlerPtyData(id, dataHandler) } diff --git a/src/renderer/src/components/terminal-pane/terminal-delivery-watchdog.test.ts b/src/renderer/src/components/terminal-pane/terminal-delivery-watchdog.test.ts index 8db2051fa35..0c822cce710 100644 --- a/src/renderer/src/components/terminal-pane/terminal-delivery-watchdog.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-delivery-watchdog.test.ts @@ -59,24 +59,26 @@ describe('terminal delivery watchdog', () => { } }) - async function startWatchdog(): Promise<{ + async function startWatchdog(hasAttachedPtys: () => boolean = () => true): Promise<{ recordPtyDataReceived: (ptyId: string, chars: number) => void registerRestoreHandler: ( ptyId: string, handler: (event: { id: string; reason: string; markerSeq?: number }) => void ) => void + syncTimer: () => void }> { const watchdog = await import('./terminal-delivery-watchdog') const restoreChannel = await import('./pty-model-restore-channel') watchdog.startTerminalDeliveryWatchdog({ reattachPushListeners: reattachMock, - hasAttachedPtys: () => true + hasAttachedPtys }) return { recordPtyDataReceived: watchdog.recordPtyDataReceived, registerRestoreHandler: (ptyId, handler) => { restoreChannel.registerPtyModelRestoreNeededHandler(ptyId, handler) - } + }, + syncTimer: watchdog.syncTerminalDeliveryWatchdogTimer } } @@ -182,6 +184,67 @@ describe('terminal delivery watchdog', () => { await vi.advanceTimersByTimeAsync(INTERVAL_MS * 3) expect(reportMock).not.toHaveBeenCalled() + expect(vi.getTimerCount()).toBe(0) + }) + + it('arms on the first attached PTY and disarms on the last detach', async () => { + reportMock.mockResolvedValue(HEALTHY) + let hasAttachedPty = false + const { syncTimer } = await startWatchdog(() => hasAttachedPty) + + expect(vi.getTimerCount()).toBe(0) + + hasAttachedPty = true + syncTimer() + expect(vi.getTimerCount()).toBe(1) + + await vi.advanceTimersByTimeAsync(INTERVAL_MS * 2) + expect(reportMock).toHaveBeenCalledTimes(2) + + hasAttachedPty = false + syncTimer() + expect(vi.getTimerCount()).toBe(0) + + await vi.advanceTimersByTimeAsync(INTERVAL_MS * 2) + expect(reportMock).toHaveBeenCalledTimes(2) + }) + + it('discards a stalled tick whose invoke was awaited across a last-PTY detach', async () => { + let hasAttachedPty = true + const { syncTimer } = await startWatchdog(() => hasAttachedPty) + + // One stalled tick brings the streak to one below the two-tick heal threshold. + reportMock.mockResolvedValue(STALLED) + await vi.advanceTimersByTimeAsync(INTERVAL_MS) + expect(reattachMock).not.toHaveBeenCalled() + + // The next tick's invoke is slow; while it is awaited, the last PTY detaches (disarm). + let resolveHealth: (health: PtyRendererDeliveryHealthReply) => void = () => {} + reportMock.mockReturnValueOnce( + new Promise((resolve) => { + resolveHealth = resolve + }) + ) + const tick = vi.advanceTimersByTimeAsync(INTERVAL_MS) + await Promise.resolve() + hasAttachedPty = false + syncTimer() + + // Even though the (now stale) reply is stalled, the tick must be discarded — no second + // confirmation, so no heal. Without the generation guard this would resurrect the streak to the + // threshold and heal spuriously while parked. + resolveHealth(STALLED) + await tick + + expect(reattachMock).not.toHaveBeenCalled() + + // The legitimate first-tick streak is preserved across the flicker (not reset), but the racing + // tick contributed nothing — so exactly ONE more real stalled confirmation after re-arm heals. + hasAttachedPty = true + syncTimer() + reportMock.mockResolvedValue(STALLED) + await vi.advanceTimersByTimeAsync(INTERVAL_MS) + expect(reattachMock).toHaveBeenCalledTimes(1) }) it('never starts without the invoke heal lane (web client, partial mocks)', async () => { diff --git a/src/renderer/src/components/terminal-pane/terminal-delivery-watchdog.ts b/src/renderer/src/components/terminal-pane/terminal-delivery-watchdog.ts index c9b0d8c29e5..4e6611d4607 100644 --- a/src/renderer/src/components/terminal-pane/terminal-delivery-watchdog.ts +++ b/src/renderer/src/components/terminal-pane/terminal-delivery-watchdog.ts @@ -59,6 +59,10 @@ let stallStreakTicks = 0 let lastHealAtMs: number | null = null let healCount = 0 let tickInFlight = false +// Why: bumped whenever a live timer is torn down (disarm / stop / reconfigure) so an in-flight tick +// whose health invoke was awaited across a last-PTY detach can discard its now-stale result instead +// of resurrecting the stall streak while parked. +let watchdogGeneration = 0 /** One Map upsert per received chunk — the watchdog's only hot-path cost. * Counted at dispatcher enqueue, BEFORE parse-deferred ACK crediting, so @@ -104,10 +108,17 @@ async function runWatchdogTick(): Promise { stallStreakTicks = 0 return } + const generationAtInvoke = watchdogGeneration const health = await report({ receivedCharsByPty: Object.fromEntries(receivedPtyCharTotals), processedCharsByPty: getProcessedPtyCharTotals() }) + // Why: a last-PTY detach (or reconfigure) during the await disarmed/re-armed the timer — this + // health reply is for the pre-detach state, so discard it WITHOUT touching the streak: mutating it + // now would either resurrect a stale streak while parked or reset progress a live reattach kept. + if (watchdogGeneration !== generationAtInvoke || !watchdogTimer) { + return + } if (!health || !isMainDeliveryStalled(health)) { stallStreakTicks = 0 return @@ -172,10 +183,16 @@ async function healDeadPushDelivery( }) } -function scheduleWatchdogTimer(): void { +function clearWatchdogTimer(): void { if (watchdogTimer) { clearInterval(watchdogTimer) + watchdogTimer = null + watchdogGeneration += 1 } +} + +function scheduleWatchdogTimer(): void { + clearWatchdogTimer() watchdogTimer = setInterval(() => { // Why serialized: a heal awaits two invokes; overlapping ticks could // double-heal inside one cooldown window. @@ -189,6 +206,22 @@ function scheduleWatchdogTimer(): void { }, watchdogConfig.intervalMs) } +export function syncTerminalDeliveryWatchdogTimer(): void { + const deps = watchdogDeps + if (!deps || !deps.hasAttachedPtys()) { + // Why: only park the timer — do NOT reset stallStreakTicks. A wedge mid-confirmation must not + // lose its progress across a brief zero-attachment detach/reattach flicker (matching the old + // always-armed timer); a healthy tick after re-arm clears the streak. No ticks fire while parked, + // so a stale streak can only ever heal one confirmation early, never permanently mis-detect. + clearWatchdogTimer() + return + } + if (!watchdogTimer) { + eventCountAtLastTick = receivedPtyDataEventCount + scheduleWatchdogTimer() + } +} + export function startTerminalDeliveryWatchdog(deps: TerminalDeliveryWatchdogDeps): void { if (watchdogDeps) { return @@ -200,17 +233,13 @@ export function startTerminalDeliveryWatchdog(deps: TerminalDeliveryWatchdogDeps return } watchdogDeps = deps - eventCountAtLastTick = receivedPtyDataEventCount - scheduleWatchdogTimer() + syncTerminalDeliveryWatchdogTimer() exposeE2eTerminalDeliveryWatchdog() } export function stopTerminalDeliveryWatchdog(): void { watchdogDeps = null - if (watchdogTimer) { - clearInterval(watchdogTimer) - watchdogTimer = null - } + clearWatchdogTimer() } /** Prod-reachable state for the one-paste freeze report (ids redacted). */ @@ -264,7 +293,7 @@ function exposeE2eTerminalDeliveryWatchdog(): void { }, configure: (config) => { watchdogConfig = { ...watchdogConfig, ...config } - if (watchdogDeps) { + if (watchdogTimer) { scheduleWatchdogTimer() } },