From 3b4dc246d80ea7e7fcb04c69e55e9c2335fd12db Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 19 Jun 2026 12:34:50 -0700 Subject: [PATCH] fix(terminal): evict lastCompletionIdentityByPaneKey on genuine pane teardown (#5822) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dispatchCompletion writes a per-paneKey entry into the module-scoped lastCompletionIdentityByPaneKey on every completion. paneKey is a never-reused ${tabId}:${leafUUID}. The map is module-scoped so it survives a live-stream remount (dispose-then-recreate, isLive() still true), but dispose never evicted it, so genuine teardown (PTY gone) leaked one entry per closed pane. Evict in dispose only when !options.isLive() — the same discriminator the cross-remount dedup tests rely on (remount disposes with isLive() true). Regression test fails before the fix (identity survives teardown, 200 leak) and passes after; the existing cross-remount dedup suite still passes. Co-authored-by: Orca --- ...ompletion-coordinator-dispose-leak.test.ts | 104 ++++++++++++++++++ .../agent-completion-coordinator.ts | 11 ++ 2 files changed, 115 insertions(+) create mode 100644 src/renderer/src/components/terminal-pane/agent-completion-coordinator-dispose-leak.test.ts diff --git a/src/renderer/src/components/terminal-pane/agent-completion-coordinator-dispose-leak.test.ts b/src/renderer/src/components/terminal-pane/agent-completion-coordinator-dispose-leak.test.ts new file mode 100644 index 00000000000..896ecc88548 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/agent-completion-coordinator-dispose-leak.test.ts @@ -0,0 +1,104 @@ +/** + * Memory-leak regression: lastCompletionIdentityByPaneKey must not retain an entry + * per genuinely-torn-down coordinator. + * + * `dispatchCompletion` does `lastCompletionIdentityByPaneKey.set(options.paneKey, …)` + * on every agent completion. `paneKey` is `${tabId}:${leafUUID}` — a never-reused + * per-pane UUID, so the key space is unbounded. The map is module-scoped on purpose + * so it survives a live-stream remount (dispose-then-recreate with the same paneKey + * while the PTY/hook stream stays live). But `dispose()` cleared only local timers + * and never the map, so on genuine teardown (PTY gone) the identity leaked — one + * entry per closed pane for the renderer session. The fix evicts on dispose only + * when the pane is no longer live, preserving the cross-remount dedup. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { + createAgentCompletionCoordinator, + resetAgentCompletionCoordinatorIdentitiesForTest, + getAgentCompletionCoordinatorIdentityCountForTest +} from './agent-completion-coordinator' +import type { + AgentCompletionCoordinatorOptions, + AgentCompletionStatusSnapshot +} from './agent-completion-coordinator-types' + +const HOOK_DONE_QUIET_MS = 1_500 + +function makeOptions(paneKey: string, live: { value: boolean }): AgentCompletionCoordinatorOptions { + return { + paneKey, + getPtyId: () => 'pty-1', + getSettings: () => null, + inspectProcess: vi.fn(), + dispatchCompletion: vi.fn(), + isLive: () => live.value + } +} + +// A working->done hook sequence with finite stateStartedAt makes the coordinator +// write a completion identity into the module map (the leaking write). +function driveCompletion( + paneKey: string, + live: { value: boolean } +): ReturnType { + const coordinator = createAgentCompletionCoordinator(makeOptions(paneKey, live)) + coordinator.observeHookStatus({ + state: 'working', + prompt: '', + agentType: 'codex', + stateStartedAt: 1000 + } as AgentCompletionStatusSnapshot) + coordinator.observeHookStatus({ + state: 'done', + prompt: '', + agentType: 'codex', + stateStartedAt: 2000 + } as AgentCompletionStatusSnapshot) + vi.advanceTimersByTime(HOOK_DONE_QUIET_MS) + return coordinator +} + +describe('agent completion coordinator identity map stays bounded (leak regression)', () => { + beforeEach(() => { + vi.useFakeTimers() + resetAgentCompletionCoordinatorIdentitiesForTest() + }) + + afterEach(() => { + resetAgentCompletionCoordinatorIdentitiesForTest() + vi.useRealTimers() + }) + + it('writes an identity on completion and clears it on genuine teardown', () => { + const live = { value: true } + const coordinator = driveCompletion('tab-1:leaf-x', live) + // The map is genuinely exercised by the completion. + expect(getAgentCompletionCoordinatorIdentityCountForTest()).toBe(1) + + // Genuine teardown: the PTY is gone. + live.value = false + coordinator.dispose() + expect(getAgentCompletionCoordinatorIdentityCountForTest()).toBe(0) + }) + + it('does not retain an identity per torn-down pane across many panes', () => { + for (let i = 0; i < 200; i++) { + const live = { value: true } + const coordinator = driveCompletion(`tab-1:leaf-${i}`, live) + live.value = false + coordinator.dispose() + } + expect(getAgentCompletionCoordinatorIdentityCountForTest()).toBe(0) + }) + + it('retains the identity across a live remount (dispose while still live)', () => { + // Cross-remount dedup: the pane remounts (dispose-then-recreate) while the + // stream stays live, so the identity must survive the dispose. + const live = { value: true } + const coordinator = driveCompletion('tab-1:leaf-remount', live) + expect(getAgentCompletionCoordinatorIdentityCountForTest()).toBe(1) + + coordinator.dispose() // isLive() still true -> remount, not teardown + expect(getAgentCompletionCoordinatorIdentityCountForTest()).toBe(1) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts b/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts index d17be49b736..085c45c8cad 100644 --- a/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts +++ b/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts @@ -717,6 +717,13 @@ export function createAgentCompletionCoordinator( clearPollTimer() clearPendingHookDone() dropPendingTitle() + // Why: the dedup identity is module-scoped so it survives a live-stream remount + // (dispose-then-recreate with the same paneKey while isLive() stays true). Only + // evict it on genuine teardown — when the PTY is gone (isLive() false) — so the + // never-reused ${tabId}:${leafUUID} key can't leak one identity per closed pane. + if (!options.isLive()) { + lastCompletionIdentityByPaneKey.delete(options.paneKey) + } } return { @@ -734,3 +741,7 @@ export function createAgentCompletionCoordinator( export function resetAgentCompletionCoordinatorIdentitiesForTest(): void { lastCompletionIdentityByPaneKey.clear() } + +export function getAgentCompletionCoordinatorIdentityCountForTest(): number { + return lastCompletionIdentityByPaneKey.size +}