fix(daemon): reap dead terminal sessions and clear stale checkpoint flags (#5788)
* fix(daemon): reap dead terminal sessions and clear stale checkpoint flags TerminalHost.sessions never removed exited sessions: sessionIds are minted fresh per pane and never reused, so each dead Session pinned a @xterm/headless emulator (~5000 rows of scrollback) for the lifetime of the long-lived daemon process. Nothing reads a dead session's emulator (getSnapshot/takePendingOutput/ listSessions all skip !isAlive), so it was pure retained memory. Wire a Session onExit hook to TerminalHost.reapSession, which disposes the emulator and drops the entry from the map. Fires on natural exit and the kill-timeout force-dispose path; the immediate-kill path reaps inline. This is the 'TerminalHost dead-session cleanup' the handleSubprocessExit comment already anticipated. Also clear DaemonPtyAdapter.sessionsNeedingFullCheckpoint on session exit and non-keepHistory shutdown — a cold-restored session that exited before its first checkpoint stranded a permanent Set entry. Regression tests assert the emulator is disposed / the flag cleared on exit (both fail before the fix). Full daemon suite (557 tests) stays green. Co-authored-by: Orca <help@stably.ai> * test(daemon): cover forceDispose reaping; fix stale comments Review follow-up: - Update the 'already-exited session' test: with reaping, natural exit disposes + drops the session (never force-killed) at exit time via session.dispose, so host.dispose only sees live sessions. Comments now match; assert the exited session is gone from listSessions. - Add a forceDispose (graceful-kill-timeout) test: a stubborn child that ignores kill is force-disposed after KILL_TIMEOUT_MS, disposing its emulator and reaping. - Clarify the shutdown() comment that the unconditional checkpoint-flag delete is a harmless no-op under keepHistory. Co-authored-by: Orca <help@stably.ai> --------- Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Neil
GitHub
parent
c7e24059c7
commit
4dfc3608b7
|
|
@ -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<string> }
|
||||
// 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 })
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<typeof createMockSubprocess>
|
||||
let emulatorDispose: ReturnType<typeof vi.spyOn>
|
||||
|
||||
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()
|
||||
}
|
||||
})
|
||||
})
|
||||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue