From 0f6f61e89d20b8e0795cb3bc170a6acb3afa8417 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 2 Jul 2026 01:29:05 -0700 Subject: [PATCH] Harden checkpoint cooldown against crash-boundary and reuse edges From adversarial review of the cooldown commit: - Warm reattach by an adapter that was not already managing the session now forces a full-snapshot re-anchor: the previous adapter may have drained daemon records it never persisted (deferred hot-session tick), and appending past that unknown drain point would put a seq gap in the log, which the restore reader rejects wholesale. This also closes the pre-existing (previously ~ms-wide) drain-vs-persist crash window. - Cold-restore re-anchor clears any cooldown inherited from the pre-crash generation (daemon respawn within one adapter), so the revived session's first snapshot is never deferred. - Exit-event routing clears lastFullCheckpointAt (reused ptyIds must not inherit a dead session's cooldown); fanoutSyntheticExits now clears sessionsNeedingFullCheckpoint for consistency. - Backward wall-clock jumps count as expired instead of extending the deferral. Co-authored-by: Orca --- src/main/daemon/daemon-pty-adapter.test.ts | 56 ++++++++++++++++++++++ src/main/daemon/daemon-pty-adapter.ts | 26 +++++++++- src/main/daemon/history-reader.ts | 5 +- 3 files changed, 84 insertions(+), 3 deletions(-) diff --git a/src/main/daemon/daemon-pty-adapter.test.ts b/src/main/daemon/daemon-pty-adapter.test.ts index f2a41050b5e..e32415aa9a3 100644 --- a/src/main/daemon/daemon-pty-adapter.test.ts +++ b/src/main/daemon/daemon-pty-adapter.test.ts @@ -1183,6 +1183,62 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { } }) + it('clears a stale snapshot cooldown when the cold-restore re-anchor is flagged', async () => { + // Simulate a previous daemon crash with recoverable history on disk. + const sessionId = 'cold-restore-stale-cooldown' + const sessionDir = join(historyDir, getHistorySessionDirName(sessionId)) + mkdirSync(sessionDir, { recursive: true }) + writeFileSync( + join(sessionDir, 'meta.json'), + JSON.stringify({ + cwd: '/tmp', + cols: 80, + rows: 24, + startedAt: '2026-04-15T10:00:00Z', + endedAt: null, + exitCode: null + }) + ) + writeFileSync(join(sessionDir, 'scrollback.bin'), 'pre-crash output\r\n') + + historyAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, historyPath: historyDir }) + const internals = historyAdapter as unknown as { + lastFullCheckpointAt: Map + sessionsNeedingFullCheckpoint: Set + } + // A daemon respawn inside one adapter keeps this map: seed a fresh + // cooldown as if the pre-crash generation just snapshotted. + internals.lastFullCheckpointAt.set(sessionId, Date.now()) + + await historyAdapter.spawn({ cols: 80, rows: 24, sessionId }) + + // The revived generation has no checkpoint of its own — the re-anchor + // must not inherit the previous generation's cooldown. + expect(internals.sessionsNeedingFullCheckpoint.has(sessionId)).toBe(true) + expect(internals.lastFullCheckpointAt.has(sessionId)).toBe(false) + }) + + it('re-anchors a warm reattach the adapter was not already managing', async () => { + const sessionId = 'warm-reattach-reanchor' + const first = new DaemonPtyAdapter({ socketPath, tokenPath, historyPath: historyDir }) + await first.spawn({ cols: 80, rows: 24, sessionId }) + first.dispose() + + // A fresh adapter (app relaunch) attaches to the still-live daemon + // session. The old adapter may have drained records it never persisted + // (deferred hot-session tick), so appends must not resume until a full + // snapshot re-anchors the log. + historyAdapter = new DaemonPtyAdapter({ socketPath, tokenPath, historyPath: historyDir }) + const internals = historyAdapter as unknown as { + sessionsNeedingFullCheckpoint: Set + lastFullCheckpointAt: Map + } + const result = await historyAdapter.spawn({ cols: 80, rows: 24, sessionId }) + expect(result.isReattach).toBe(true) + expect(internals.sessionsNeedingFullCheckpoint.has(sessionId)).toBe(true) + expect(internals.lastFullCheckpointAt.has(sessionId)).toBe(false) + }) + it('returns same cold restore on StrictMode double-mount (sticky cache)', async () => { const sessionId = 'sticky-cache-test' const sessionDir = join(historyDir, getHistorySessionDirName(sessionId)) diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index 7a6c8436590..f4d717f1999 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -208,6 +208,7 @@ export class DaemonPtyAdapter implements IPtyProvider { } } + const wasAlreadyManaged = this.activeSessionIds.has(sessionId) this.activeSessionIds.add(sessionId) // Cold restore: daemon created a new session but disk history shows @@ -231,6 +232,10 @@ export class DaemonPtyAdapter implements IPtyProvider { if (this.historyManager) { this.historyManager.registerWriter(sessionId) this.sessionsNeedingFullCheckpoint.add(sessionId) + // Why: the revived generation has no valid checkpoint of its own; a + // cooldown inherited from the pre-crash generation (daemon respawn + // within one adapter) must not defer this re-anchor. + this.lastFullCheckpointAt.delete(sessionId) } if (scrollback) { const coldRestore = { scrollback, cwd: restoreInfo.cwd, oscLinks: restoreInfo.oscLinks } @@ -254,6 +259,15 @@ export class DaemonPtyAdapter implements IPtyProvider { // without overwriting meta.json or deleting the existing checkpoint // (which is the only valid recovery data until the next tick). this.historyManager.registerWriter(sessionId) + if (!wasAlreadyManaged) { + // Why: a previous adapter may have drained daemon records it never + // persisted (a deferred hot-session tick) before the app died. + // Appending increments past that unknown drain point would put a seq + // gap in the log, which the restore reader rejects wholesale. Force a + // full snapshot to re-anchor before any further appends. + this.sessionsNeedingFullCheckpoint.add(sessionId) + this.lastFullCheckpointAt.delete(sessionId) + } } const isReattach = !result.isNew @@ -514,6 +528,7 @@ export class DaemonPtyAdapter implements IPtyProvider { this.activeSessionIds.clear() this.dirtySessionVersions.clear() this.lastFullCheckpointAt.clear() + this.sessionsNeedingFullCheckpoint.clear() this.stopCheckpointTimer() for (const id of ids) { this.coldRestoreCache.delete(id) @@ -760,7 +775,13 @@ export class DaemonPtyAdapter implements IPtyProvider { // cold restore would find nothing. private isFullCheckpointCoolingDown(sessionId: string): boolean { const last = this.lastFullCheckpointAt.get(sessionId) - return last !== undefined && Date.now() - last < DaemonPtyAdapter.FULL_CHECKPOINT_COOLDOWN_MS + if (last === undefined) { + return false + } + const elapsed = Date.now() - last + // Why elapsed < 0 counts as expired: a backward wall-clock jump must not + // extend the deferral window. + return elapsed >= 0 && elapsed < DaemonPtyAdapter.FULL_CHECKPOINT_COOLDOWN_MS } // Why 'deferred' exists: a cap/overflow-triggered full snapshot inside the @@ -960,6 +981,9 @@ export class DaemonPtyAdapter implements IPtyProvider { // 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) + // Why: a reused sessionId (renderer respawns a persisted ptyId) must + // not inherit the dead session's snapshot cooldown. + this.lastFullCheckpointAt.delete(event.sessionId) this.stopCheckpointTimerIfIdle() if (this.historyManager) { void this.historyManager diff --git a/src/main/daemon/history-reader.ts b/src/main/daemon/history-reader.ts index 65e1afd8abe..2914ed21aa7 100644 --- a/src/main/daemon/history-reader.ts +++ b/src/main/daemon/history-reader.ts @@ -50,8 +50,9 @@ export class HistoryReader { } // Why log replay is preferred over the checkpoint alone: the log carries - // byte-exact output up to ~5s before the crash, while the checkpoint can - // be a full log-cap (~5MB of output) stale. + // byte-exact output up to ~5s before the crash (up to the full-snapshot + // cooldown, ~45s, for a streaming session mid-deferral), while the + // checkpoint can be a full log-cap (~5MB of output) stale. const logRestore = this.restoreFromIncrementalLog(sessionDir, meta, checkpoint) if (logRestore) { return logRestore