From 8aa4d3df19c80198949ef23c1ef63acc2cbfe5b4 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 21 May 2026 21:27:53 -0700 Subject: [PATCH] perf: checkpoint daemon PTYs only when dirty (#2593) --- src/main/daemon/daemon-pty-adapter.test.ts | 56 +++++++++++++++++++++ src/main/daemon/daemon-pty-adapter.ts | 57 ++++++++++++++++------ 2 files changed, 97 insertions(+), 16 deletions(-) diff --git a/src/main/daemon/daemon-pty-adapter.test.ts b/src/main/daemon/daemon-pty-adapter.test.ts index 85454407d..e201d466c 100644 --- a/src/main/daemon/daemon-pty-adapter.test.ts +++ b/src/main/daemon/daemon-pty-adapter.test.ts @@ -523,6 +523,62 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { } }) + it('does not schedule a checkpoint timer until a session is dirty', async () => { + const adapterClass = DaemonPtyAdapter as unknown as { CHECKPOINT_INTERVAL_MS: number } + const previousInterval = adapterClass.CHECKPOINT_INTERVAL_MS + const setTimeoutSpy = vi.spyOn(globalThis, 'setTimeout') + adapterClass.CHECKPOINT_INTERVAL_MS = 10_000 + + try { + historyAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, historyPath: historyDir }) + await historyAdapter.spawn({ + cols: 80, + rows: 24, + cwd: '/home/user', + sessionId: 'idle-checkpoint' + }) + + expect(setTimeoutSpy.mock.calls.some(([, delay]) => delay === 10_000)).toBe(false) + + lastSubprocess._simulateData('dirty after idle\r\n') + await waitFor(() => setTimeoutSpy.mock.calls.some(([, delay]) => delay === 10_000)) + } finally { + adapterClass.CHECKPOINT_INTERVAL_MS = previousInterval + setTimeoutSpy.mockRestore() + } + }) + + it('clears a pending checkpoint timer when the last dirty session closes', async () => { + const adapterClass = DaemonPtyAdapter as unknown as { CHECKPOINT_INTERVAL_MS: number } + const previousInterval = adapterClass.CHECKPOINT_INTERVAL_MS + const clearTimeoutSpy = vi.spyOn(globalThis, 'clearTimeout') + adapterClass.CHECKPOINT_INTERVAL_MS = 10_000 + + try { + historyAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, historyPath: historyDir }) + const { id } = await historyAdapter.spawn({ + cols: 80, + rows: 24, + cwd: '/home/user', + sessionId: 'close-dirty-checkpoint' + }) + const internals = historyAdapter as unknown as { + dirtySessionVersions: Map + } + + lastSubprocess._simulateData('dirty before close\r\n') + await waitFor(() => internals.dirtySessionVersions.has(id)) + const callsBeforeClose = clearTimeoutSpy.mock.calls.length + + await historyAdapter.shutdown(id, { immediate: true }) + + expect(clearTimeoutSpy.mock.calls.length).toBeGreaterThan(callsBeforeClose) + } finally { + adapterClass.CHECKPOINT_INTERVAL_MS = previousInterval + clearTimeoutSpy.mockRestore() + } + }) + it('writes meta.json with endedAt on exit', async () => { historyAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, historyPath: historyDir }) diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index ca492a953..f5c6cedb6 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -66,7 +66,7 @@ export class DaemonPtyAdapter implements IPtyProvider { private coldRestoreCache = new Map() private activeSessionIds = new Set() private dirtySessionVersions = new Map() - private checkpointInterval: ReturnType | null = null + private checkpointTimer: ReturnType | null = null private checkpointInFlight: Promise | null = null // Why: checkpoint-based persistence requires the getSnapshot RPC (v4+). // Legacy daemons reject it, causing noisy log spam every 5 seconds. @@ -246,6 +246,7 @@ export class DaemonPtyAdapter implements IPtyProvider { await this.client.request('kill', { sessionId: id }) this.activeSessionIds.delete(id) this.dirtySessionVersions.delete(id) + this.stopCheckpointTimerIfIdle() this.initialCwds.delete(id) // Why: history removal is for the "user explicitly closed this terminal" // path. Sleep also calls shutdown but expects scrollback to survive — wake @@ -407,6 +408,7 @@ export class DaemonPtyAdapter implements IPtyProvider { const ids = [...this.activeSessionIds] this.activeSessionIds.clear() this.dirtySessionVersions.clear() + this.stopCheckpointTimer() for (const id of ids) { // Why: listener throws are intentionally *not* caught — matches the // natural onExit fanout in setupEventRouting, so synthetic exits don't @@ -462,10 +464,7 @@ export class DaemonPtyAdapter implements IPtyProvider { } dispose(): void { - if (this.checkpointInterval) { - clearInterval(this.checkpointInterval) - this.checkpointInterval = null - } + this.stopCheckpointTimer() this.dirtySessionVersions.clear() this.removeEventListener?.() this.removeEventListener = null @@ -487,10 +486,7 @@ export class DaemonPtyAdapter implements IPtyProvider { // We write a final checkpoint before disconnecting so that if the daemon // later crashes while Orca is closed, checkpoint.json has recovery data. async disconnectOnly(): Promise { - if (this.checkpointInterval) { - clearInterval(this.checkpointInterval) - this.checkpointInterval = null - } + this.stopCheckpointTimer() // Why: wait for any in-flight timer pass to finish before starting // the final checkpoint. Otherwise both passes race on the shared tmp // file, risking ENOENT on rename and disabling future writes. @@ -512,23 +508,47 @@ export class DaemonPtyAdapter implements IPtyProvider { private async ensureConnected(): Promise { await this.client.ensureConnected() this.setupEventRouting() - this.startCheckpointTimer() + this.scheduleCheckpointTimer() } - private startCheckpointTimer(): void { - if (this.checkpointInterval || !this.historyManager || !this.supportsCheckpoints) { + private stopCheckpointTimer(): void { + if (!this.checkpointTimer) { return } - this.checkpointInterval = setInterval(() => { + clearTimeout(this.checkpointTimer) + this.checkpointTimer = null + } + + private stopCheckpointTimerIfIdle(): void { + if (this.dirtySessionVersions.size === 0) { + this.stopCheckpointTimer() + } + } + + private scheduleCheckpointTimer(): void { + if ( + this.checkpointTimer || + !this.historyManager || + !this.supportsCheckpoints || + this.dirtySessionVersions.size === 0 + ) { + return + } + // Why: checkpointing is only needed after terminal data/resize/write marks + // a session dirty. A permanent interval woke the main process every 5s for + // idle daemon-backed terminals just to discover there was nothing to write. + this.checkpointTimer = setTimeout(() => { + this.checkpointTimer = null // Why: if the previous pass is still in-flight (slow RPC or disk), - // skip this tick. Overlapping passes race on the shared tmp file - // in checkpoint(), and a lost rename triggers handleWriteError which - // permanently disables the session's history writes. + // retry later instead of overlapping checkpoint() writes to the same tmp + // file, which can lose a rename and disable future history writes. if (this.checkpointInFlight) { + this.scheduleCheckpointTimer() return } this.checkpointInFlight = this.checkpointDirtySessions().finally(() => { this.checkpointInFlight = null + this.scheduleCheckpointTimer() }) }, DaemonPtyAdapter.CHECKPOINT_INTERVAL_MS) } @@ -538,6 +558,7 @@ export class DaemonPtyAdapter implements IPtyProvider { return } this.dirtySessionVersions.set(sessionId, (this.dirtySessionVersions.get(sessionId) ?? 0) + 1) + this.scheduleCheckpointTimer() } private async checkpointDirtySessions(): Promise { @@ -552,6 +573,8 @@ export class DaemonPtyAdapter implements IPtyProvider { [...this.dirtySessionVersions].filter(([sessionId]) => this.activeSessionIds.has(sessionId)) ) if (versions.size === 0) { + this.dirtySessionVersions.clear() + this.stopCheckpointTimer() return } const completed = await this.checkpointSessions(versions.keys()) @@ -560,6 +583,7 @@ export class DaemonPtyAdapter implements IPtyProvider { this.dirtySessionVersions.delete(sessionId) } } + this.stopCheckpointTimerIfIdle() } // Why: the adapter runs in the Electron main process and does not have direct @@ -650,6 +674,7 @@ export class DaemonPtyAdapter implements IPtyProvider { } else if (event.event === 'exit') { this.activeSessionIds.delete(event.sessionId) this.dirtySessionVersions.delete(event.sessionId) + this.stopCheckpointTimerIfIdle() if (this.historyManager) { void this.historyManager .closeSession(event.sessionId, event.payload.code)