diff --git a/src/main/daemon/daemon-pty-adapter.test.ts b/src/main/daemon/daemon-pty-adapter.test.ts index 6ad7c739c..bf8948bb1 100644 --- a/src/main/daemon/daemon-pty-adapter.test.ts +++ b/src/main/daemon/daemon-pty-adapter.test.ts @@ -193,6 +193,24 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { }) }) + describe('sessionsNeedingFullCheckpoint cleanup (leak regression)', () => { + // Why: the cold-restore path flags a session for a full checkpoint. If the + // session exits before that checkpoint lands, the flag was never cleared and + // leaked a permanent Set entry for the daemon's lifetime. + it('clears the pending full-checkpoint flag when a session exits', async () => { + const { id } = await adapter.spawn({ cols: 80, rows: 24 }) + const internals = adapter as unknown as { sessionsNeedingFullCheckpoint: Set } + // Simulate the cold-restore reanchor path having flagged this session. + internals.sessionsNeedingFullCheckpoint.add(id) + expect(internals.sessionsNeedingFullCheckpoint.has(id)).toBe(true) + + lastSubprocess._simulateExit(0) + await new Promise((r) => setTimeout(r, 50)) + + expect(internals.sessionsNeedingFullCheckpoint.has(id)).toBe(false) + }) + }) + describe('sendSignal', () => { it('sends signal to the session', async () => { const { id } = await adapter.spawn({ cols: 80, rows: 24 }) diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index aa656851f..7357ecbd1 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -303,6 +303,11 @@ export class DaemonPtyAdapter implements IPtyProvider { this.activeSessionIds.delete(id) this.dirtySessionVersions.delete(id) this.coldRestoreCache.delete(id) + // Why: the !keepHistory close path doesn't take a final checkpoint, so a + // session stranded in sessionsNeedingFullCheckpoint would never be cleared. + // (Under keepHistory the final checkpoint above already cleared the flag, so + // this is a harmless no-op there — kept unconditional to cover both paths.) + this.sessionsNeedingFullCheckpoint.delete(id) this.stopCheckpointTimerIfIdle() this.initialCwds.delete(id) // Why: history removal is for the "user explicitly closed this terminal" @@ -873,6 +878,10 @@ export class DaemonPtyAdapter implements IPtyProvider { this.activeSessionIds.delete(event.sessionId) this.dirtySessionVersions.delete(event.sessionId) this.coldRestoreCache.delete(event.sessionId) + // Why: an exited session can never be checkpointed again, so its pending + // full-checkpoint flag is dead state. Without this, a cold-restored + // session that exits before its first checkpoint leaks a permanent entry. + this.sessionsNeedingFullCheckpoint.delete(event.sessionId) this.stopCheckpointTimerIfIdle() if (this.historyManager) { void this.historyManager diff --git a/src/main/daemon/session.ts b/src/main/daemon/session.ts index f3262b4ec..7f2dee741 100644 --- a/src/main/daemon/session.ts +++ b/src/main/daemon/session.ts @@ -55,6 +55,12 @@ export type SessionOptions = { shellReadySupported: boolean shellReadyTimeoutMs?: number scrollback?: number + // Why: fired once the session reaches a terminal state (natural exit or + // kill-timeout force-dispose) so the owner (TerminalHost) can reap it — + // dispose the headless emulator and drop it from its session map. Without a + // reaper, dead sessions (and their ~5000-row scrollback emulators) accumulate + // for the lifetime of the long-lived daemon process. + onExit?: (code: number) => void } type AttachedClient = { @@ -72,6 +78,7 @@ export class Session { private _disposed = false private emulator: HeadlessEmulator private subprocess: SubprocessHandle + private readonly onSessionExit?: (code: number) => void private attachedClients: AttachedClient[] = [] private preReadyStdinQueue: string[] = [] private markerBuffer = '' @@ -86,6 +93,7 @@ export class Session { constructor(opts: SessionOptions) { this.sessionId = opts.sessionId this.subprocess = opts.subprocess + this.onSessionExit = opts.onExit const size = normalizePtySize(opts.cols, opts.rows) this.emulator = new HeadlessEmulator({ cols: size.cols, @@ -324,6 +332,9 @@ export class Session { } this.#teardownSubprocess() this._state = 'exited' + // Why: free the headless emulator's scrollback here too (this path skips + // dispose()). Matches forceDispose(); reaping just drops the map entry. + this.emulator.dispose() } /** Private: shared teardown helper called by dispose(), forceDispose(), and @@ -418,10 +429,10 @@ export class Session { // Why: release the ptmx fd on the natural-exit path. Without this, the // node-pty wrapper's _socket stays alive until GC and the master fd leaks // (see docs/fix-pty-fd-leak.md). Do NOT route through #teardownSubprocess: - // that helper flips `_disposed = true`, which would short-circuit a later - // Session.dispose() call from TerminalHost's dead-session cleanup at - // terminal-host.ts:83 — skipping attachedClients/emulator/postReadyFlushGate - // cleanup. Call subprocess.dispose() directly inside try/catch. + // that helper flips `_disposed = true`, which would short-circuit the later + // Session.dispose() call from TerminalHost.reapSession (wired via onExit + // below) — skipping attachedClients/emulator/postReadyFlushGate cleanup. + // Call subprocess.dispose() directly inside try/catch. try { this.subprocess.dispose() } catch { @@ -431,6 +442,10 @@ export class Session { for (const client of this.attachedClients) { client.onExit(code) } + + // Why: hand off to the owner's reaper so the emulator is disposed and the + // session dropped from the host map; otherwise dead sessions accumulate. + this.onSessionExit?.(code) } private scanForShellMarker(data: string): void { @@ -510,5 +525,9 @@ export class Session { for (const client of clients) { client.onExit(-1) } + + // Why: reap from the host map on the kill-timeout path too (emulator already + // disposed above; reapSession's dispose() call is a no-op and just drops it). + this.onSessionExit?.(-1) } } diff --git a/src/main/daemon/terminal-host-session-reaping-leak.test.ts b/src/main/daemon/terminal-host-session-reaping-leak.test.ts new file mode 100644 index 000000000..30d1736e4 --- /dev/null +++ b/src/main/daemon/terminal-host-session-reaping-leak.test.ts @@ -0,0 +1,158 @@ +/** + * Memory-leak regression: TerminalHost must reap dead sessions. + * + * SessionIds are minted fresh per pane and never reused, so a `TerminalHost` + * that never removes exited sessions from its `sessions` map leaks one dead + * `Session` — each pinning a `@xterm/headless` emulator with ~5000 rows of + * scrollback — per terminal for the lifetime of the long-lived daemon process. + * + * The fix wires a Session `onExit` hook to `TerminalHost.reapSession`, which + * disposes the emulator and drops the entry from the map. These tests assert the + * emulator is disposed when a subprocess exits (before the fix it never was). + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { TerminalHost } from './terminal-host' +import type { SubprocessHandle } from './session' +import { HeadlessEmulator } from './headless-emulator' + +function createMockSubprocess(): SubprocessHandle & { + _onExitCb: ((code: number) => void) | null +} { + let onDataCb: ((data: string) => void) | null = null + let onExitCb: ((code: number) => void) | null = null + return { + pid: 99999, + getForegroundProcess: vi.fn(() => null), + write: vi.fn(), + resize: vi.fn(), + kill: vi.fn(() => { + setTimeout(() => onExitCb?.(0), 5) + }), + forceKill: vi.fn(), + signal: vi.fn(), + onData(cb) { + onDataCb = cb + }, + onExit(cb) { + onExitCb = cb + }, + dispose: vi.fn(), + get _onDataCb() { + return onDataCb + }, + get _onExitCb() { + return onExitCb + } + } as SubprocessHandle & { _onExitCb: ((code: number) => void) | null } +} + +describe('TerminalHost dead-session reaping (leak regression)', () => { + let host: TerminalHost + let lastSubprocess: ReturnType + let emulatorDispose: ReturnType + + beforeEach(() => { + emulatorDispose = vi.spyOn(HeadlessEmulator.prototype, 'dispose') + const spawnFn = vi.fn(() => { + lastSubprocess = createMockSubprocess() + return lastSubprocess + }) + host = new TerminalHost({ spawnSubprocess: spawnFn }) + }) + + afterEach(() => { + host.dispose() + emulatorDispose.mockRestore() + }) + + function streamClient() { + return { onData: vi.fn(), onExit: vi.fn() } + } + + it('disposes the emulator and reaps the session when its subprocess exits', async () => { + await host.createOrAttach({ + sessionId: 'session-1', + cols: 80, + rows: 24, + streamClient: streamClient() + }) + // Alive: emulator is held, not disposed. + expect(emulatorDispose).not.toHaveBeenCalled() + expect(host.listSessions()).toHaveLength(1) + + // Natural exit. + lastSubprocess._onExitCb?.(0) + + // The dead session's emulator (its scrollback buffer) is freed and the + // session is gone from the map — not merely skipped by listSessions. + expect(emulatorDispose).toHaveBeenCalledTimes(1) + expect(host.listSessions()).toHaveLength(0) + }) + + it('does not retain dead-session emulators across many create/exit cycles', async () => { + const CYCLES = 5 + for (let i = 0; i < CYCLES; i++) { + await host.createOrAttach({ + sessionId: `session-${i}`, + cols: 80, + rows: 24, + streamClient: streamClient() + }) + lastSubprocess._onExitCb?.(0) + } + + // Every dead session was reaped: one emulator disposed per cycle, none retained. + expect(emulatorDispose).toHaveBeenCalledTimes(CYCLES) + expect(host.listSessions()).toHaveLength(0) + }) + + it('reaps a session killed immediately (forceKill path)', async () => { + await host.createOrAttach({ + sessionId: 'session-1', + cols: 80, + rows: 24, + streamClient: streamClient() + }) + + host.kill('session-1', { immediate: true }) + + // Emulator freed and session dropped from the map (no lingering dead entry). + expect(emulatorDispose).toHaveBeenCalledTimes(1) + expect(host.listSessions()).toHaveLength(0) + }) + + it('reaps a session whose graceful kill times out (forceDispose path)', async () => { + vi.useFakeTimers() + try { + const stubbornHost = new TerminalHost({ + spawnSubprocess: () => { + const sub = createMockSubprocess() + // Stubborn child: ignores graceful kill, so the KILL_TIMEOUT_MS timer + // must force-dispose it. + sub.kill = vi.fn() + return sub + } + }) + await stubbornHost.createOrAttach({ + sessionId: 'stubborn', + cols: 80, + rows: 24, + streamClient: streamClient() + }) + + // Graceful kill — the no-op subprocess.kill never fires onExit. + stubbornHost.kill('stubborn') + expect(emulatorDispose).not.toHaveBeenCalled() + + // The 5s KILL_TIMEOUT_MS fallback fires forceDispose, which disposes the + // emulator and reaps the session via the onExit hook. + vi.advanceTimersByTime(5000) + + expect(emulatorDispose).toHaveBeenCalledTimes(1) + expect(stubbornHost.listSessions()).toHaveLength(0) + stubbornHost.dispose() + } finally { + vi.useRealTimers() + } + }) +}) diff --git a/src/main/daemon/terminal-host.test.ts b/src/main/daemon/terminal-host.test.ts index f26aa6817..bda5f4bed 100644 --- a/src/main/daemon/terminal-host.test.ts +++ b/src/main/daemon/terminal-host.test.ts @@ -398,12 +398,13 @@ describe('TerminalHost', () => { expect(host.listSessions()).toEqual([]) }) - it('skips forceKill on already-exited sessions to avoid recycled-pid SIGKILL', async () => { + it('never force-kills an exited session (recycled-pid SIGKILL safety)', async () => { // Why: after a session's subprocess has exited (onExit fired), proc.pid - // refers to a reaped child whose pid may have been recycled. Calling - // forceKillAndDisposeSubprocess() on an exited session would - // process.kill(recycled_pid, 'SIGKILL') — killing a stranger. Dispose - // must detect isAlive=false and use disposeSubprocess() (fd release only). + // refers to a reaped child whose pid may have been recycled. Force-killing + // it would process.kill(recycled_pid, 'SIGKILL') — killing a stranger. + // The exit now reaps the session via session.dispose(), which skips + // forceKill once _state==='exited' (only the fd is released). host.dispose + // then only ever sees live sessions. await host.createOrAttach({ sessionId: 'session-1', cols: 80, @@ -411,11 +412,14 @@ describe('TerminalHost', () => { streamClient: { onData: vi.fn(), onExit: vi.fn() } }) - // Simulate natural exit — session is retained in map until dispose. - lastSubprocess._onExitCb?.(0) - - // Re-create another session so the map has BOTH a live and dead entry. + // Natural exit reaps session-1 synchronously: its subprocess fd is + // released (dispose) but it is never force-killed, and it is dropped from + // the map (so it is not listed and not touched by host.dispose below). const exitedSub = lastSubprocess + lastSubprocess._onExitCb?.(0) + expect(host.listSessions()).toEqual([]) + + // A second, live session remains in the map for host.dispose to reap. await host.createOrAttach({ sessionId: 'session-2', cols: 80, diff --git a/src/main/daemon/terminal-host.ts b/src/main/daemon/terminal-host.ts index a772de6ef..6dd0909df 100644 --- a/src/main/daemon/terminal-host.ts +++ b/src/main/daemon/terminal-host.ts @@ -134,6 +134,11 @@ export class TerminalHost { rows: size.rows, subprocess, shellReadySupported: opts.shellReadySupported ?? false, + // Why: reap the dead session (dispose emulator + drop from the map) the + // moment its subprocess exits, instead of retaining it for the daemon's + // lifetime. Nothing reads a dead session's emulator (getSnapshot/ + // takePendingOutput/listSessions all skip !isAlive sessions). + onExit: () => this.reapSession(opts.sessionId), ...(opts.shellReadyTimeoutMs !== undefined ? { shellReadyTimeoutMs: opts.shellReadyTimeoutMs } : {}) @@ -179,11 +184,29 @@ export class TerminalHost { this.recordTombstone(sessionId) if (opts.immediate) { session.forceKillAndDisposeSubprocess() + // Why: the immediate path tears down synchronously without firing the + // session's onExit hook, so reap it here. The graceful path below funnels + // through Session.handleSubprocessExit -> onExit -> reapSession. + this.reapSession(sessionId) return } session.kill() } + // Why: dispose a dead session's headless emulator and drop it from the map so + // exited terminals don't pin ~5000 rows of scrollback for the daemon's life. + // No-ops on live sessions (a live session must never be disposed here) and on + // already-reaped/unknown ids. Wired as the Session onExit hook and also called + // on the immediate-kill path. + private reapSession(sessionId: string): void { + const session = this.sessions.get(sessionId) + if (!session || session.isAlive) { + return + } + session.dispose() + this.sessions.delete(sessionId) + } + signal(sessionId: string, sig: string): void { this.getAliveSession(sessionId).signal(sig) }