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 <help@stably.ai>
This commit is contained in:
Neil
2026-06-19 12:34:47 -07:00
committed by GitHub
co-authored by Orca
parent 77633809c6
commit 3db65e8d9a
2 changed files with 91 additions and 0 deletions
@@ -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
})
})
+27
View File
@@ -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<string, PtyRecord>()
private oscTitleScanTailByPtyId = new Map<string, string>()
private meaningfulContentScanTailByPtyId = new Map<string, string>()
private exitedPtyIds = new Set<string>()
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 {