From 3db65e8d9a5e85b43756b7e4ef3d72c3edd553ea Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 19 Jun 2026 12:34:47 -0700 Subject: [PATCH] fix(stats): refuse AgentDetector pty-map resurrection after onExit (#5820) onData creates+sets a pty record before the stopped-state guard, and onExit deletes the record instead of leaving a tombstone. A data chunk arriving after onExit (the exit-then-data race in pty.ts shutdown) resurrected a fresh record that nothing ever deleted, and double-counted the session. ptyId is a fresh per-spawn UUID, so the three ptyId-keyed maps grew unbounded. Track exited ids in a bounded FIFO and refuse onData resurrection for them. Regression test fails before the fix (record resurrected, 200 leaked) and passes after. Co-authored-by: Orca --- .../agent-detector-resurrection-leak.test.ts | 64 +++++++++++++++++++ src/main/stats/agent-detector.ts | 27 ++++++++ 2 files changed, 91 insertions(+) create mode 100644 src/main/stats/agent-detector-resurrection-leak.test.ts diff --git a/src/main/stats/agent-detector-resurrection-leak.test.ts b/src/main/stats/agent-detector-resurrection-leak.test.ts new file mode 100644 index 00000000000..a3da689939c --- /dev/null +++ b/src/main/stats/agent-detector-resurrection-leak.test.ts @@ -0,0 +1,64 @@ +/** + * Memory-leak regression: AgentDetector must not resurrect a PTY's record after exit. + * + * `onData` does a get-or-create + `this.ptys.set(ptyId, record)` BEFORE the + * `if (record.state === 'stopped') return` guard. `onExit` DELETES the record from + * all three ptyId-keyed maps rather than leaving a 'stopped' tombstone. So a data + * chunk delivered AFTER onExit (the real exit-then-data race in pty.ts shutdown — + * provider data still flows because finishPtyShutdown doesn't unsubscribe the data + * handler) re-inserts a fresh 'unknown' record and re-seeds the scan-tail maps that + * nothing will ever delete. ptyId is a fresh per-spawn UUID — unbounded over a session. + */ +import { describe, expect, it, vi } from 'vitest' +import { AgentDetector } from './agent-detector' + +function oscTitle(title: string): string { + return `\x1b]0;${title}\x07` +} + +function makeStats() { + return { onAgentStart: vi.fn(), onAgentStop: vi.fn() } +} + +describe('AgentDetector refuses post-exit resurrection (leak regression)', () => { + it('ignores a data chunk that arrives after the PTY has exited', () => { + const stats = makeStats() + const detector = new AgentDetector(stats as never) + + detector.onData('pty-1', oscTitle('✳ Claude Code'), 100) // start + detector.onExit('pty-1') // delete all per-pty records + detector.onData('pty-1', oscTitle('✳ Claude Code'), 200) // late post-exit chunk + + // The post-exit chunk must NOT resurrect the record into a second session. + expect(stats.onAgentStart).toHaveBeenCalledTimes(1) + // And no per-pty record should linger. + expect(detector.trackedPtyCount).toBe(0) + }) + + it('does not accumulate records across many exit-then-late-data races', () => { + const stats = makeStats() + const detector = new AgentDetector(stats as never) + + for (let i = 0; i < 200; i++) { + const ptyId = `pty-${i}` + detector.onData(ptyId, oscTitle('✳ Claude Code'), i * 10) + detector.onExit(ptyId) + detector.onData(ptyId, oscTitle('✳ Claude Code'), i * 10 + 1) // late chunk + } + + expect(detector.trackedPtyCount).toBe(0) + }) + + it('still starts a fresh PTY whose id was never exited (guards over-blocking)', () => { + const stats = makeStats() + const detector = new AgentDetector(stats as never) + + detector.onData('pty-A', oscTitle('✳ Claude Code'), 100) + detector.onExit('pty-A') + // A DIFFERENT, never-exited PTY must still start normally. + detector.onData('pty-B', oscTitle('✳ Claude Code'), 200) + + expect(stats.onAgentStart).toHaveBeenCalledTimes(2) + expect(detector.trackedPtyCount).toBe(1) // only the live pty-B is tracked + }) +}) diff --git a/src/main/stats/agent-detector.ts b/src/main/stats/agent-detector.ts index f514ccce207..0f2cd418276 100644 --- a/src/main/stats/agent-detector.ts +++ b/src/main/stats/agent-detector.ts @@ -86,10 +86,18 @@ function hasMeaningfulContent(chunk: string): boolean { * one giant session and we would never emit the idle-time stop boundaries that * the stats design relies on. */ +// Why: onExit deletes a PTY's record instead of leaving a tombstone, so a data +// chunk delivered AFTER exit (the exit-then-data race during pty.ts shutdown) +// would resurrect a fresh record nothing ever deletes. Remember recently-exited +// ids in a bounded FIFO to refuse resurrection; the cap keeps the guard itself +// bounded, and per-spawn UUID ptyIds are never reused so aged-out ids are safe. +const MAX_EXITED_PTY_IDS = 1024 + export class AgentDetector { private ptys = new Map() private oscTitleScanTailByPtyId = new Map() private meaningfulContentScanTailByPtyId = new Map() + private exitedPtyIds = new Set() private stats: StatsCollector private meaningfulContentDetector: MeaningfulContentDetector @@ -106,6 +114,11 @@ export class AgentDetector { onData(ptyId: string, rawData: string, at: number): void { let record = this.ptys.get(ptyId) if (!record) { + // Why: refuse to resurrect a PTY that already exited — a late post-exit + // data chunk must not create a new tracked record (which nothing deletes). + if (this.exitedPtyIds.has(ptyId)) { + return + } record = { state: 'unknown', sessionOpen: false, @@ -197,6 +210,16 @@ export class AgentDetector { this.ptys.delete(ptyId) this.oscTitleScanTailByPtyId.delete(ptyId) this.meaningfulContentScanTailByPtyId.delete(ptyId) + // Remember this id (bounded FIFO) so a late data chunk can't resurrect it. + this.exitedPtyIds.delete(ptyId) + this.exitedPtyIds.add(ptyId) + while (this.exitedPtyIds.size > MAX_EXITED_PTY_IDS) { + const oldest = this.exitedPtyIds.values().next() + if (oldest.done) { + break + } + this.exitedPtyIds.delete(oldest.value) + } } private extractLastOscTitleForPty(ptyId: string, rawData: string): string | null { @@ -222,6 +245,10 @@ export class AgentDetector { this.meaningfulContentScanTailByPtyId.delete(ptyId) } } + + get trackedPtyCount(): number { + return this.ptys.size + } } function extractMeaningfulContentScanTail(value: string): string {