From a088f481babaf56371da3c1b609e34ddfdf55fc7 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Sun, 3 May 2026 16:56:11 -0400 Subject: [PATCH] feat(pty): post-release monitoring + Issue #217 e2e regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why (design doc §9): surface the signal that matters after shipping the deterministic-sessionId fix. Two log sites added: - daemon-pty-adapter.ts: warn when a spawn with an explicit sessionId produced a new daemon session AND no disk history was found — that means either the reaper ate the dir, or the persisted mapping pointed at a session that never wrote a checkpoint. Shouldn't happen under the defensive keep-set derivation; worth watching. - main/index.ts: log the reaper's ReapStats at each startup pass so we can tell whether it's firing at all and whether the three gates (keep set, mtime grace, quarantine) are in expected ratios. E2E test added (§7.2): Issue #217's load-bearing scenario — scrollback survives a quit where the persisted orca-data.json lacks `ptyId`. We simulate the SIGKILL-before-persist race by mutating orca-data.json between launches (delete ptyId, leave terminal-history/ dir intact). Pre-fix code couldn't reach the dir; post-fix code re-derives the id via mintPtySessionId(worktreeId, tabId, leafId) and cold-restores. Co-authored-by: Orca --- src/main/daemon/daemon-pty-adapter.ts | 15 ++++ src/main/index.ts | 8 +- .../e2e/terminal-restart-persistence.spec.ts | 80 ++++++++++++++++++- 3 files changed, 101 insertions(+), 2 deletions(-) diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index cd73232acf7..7a962caff9c 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -186,6 +186,21 @@ export class DaemonPtyAdapter implements IPtyProvider { } if (this.historyManager && result.isNew) { + // Why (design doc §9 post-release monitoring): a spawn with an explicit + // sessionId that produced a brand-new daemon session AND has no disk + // history is the signal that either (a) the reaper ate a dir the user + // still wanted, or (b) the persisted mapping pointed at a session that + // never wrote a checkpoint. This shouldn't happen under the defensive + // keep-set derivation — if it starts showing up in logs, investigate + // before the next release. + if (opts.sessionId !== undefined && !restoreInfo) { + console.warn('[pty] cold-restore miss for explicit sessionId — no disk history found', { + sessionId, + worktreeId: opts.worktreeId, + tabId: opts.tabId, + leafId: opts.leafId + }) + } void this.historyManager .openSession(sessionId, { cwd: effectiveCwd ?? '', diff --git a/src/main/index.ts b/src/main/index.ts index d351fa2395e..fa8d9d824cb 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -550,12 +550,18 @@ app.whenReady().then(async () => { liveDaemonSessionIds.add(id) } } - await runOrphanHistoryReaper({ + const reapStats = await runOrphanHistoryReaper({ historyBasePath: getHistoryDir(), sidecarDir: app.getPath('userData'), workspaceSession: store.getWorkspaceSession(), liveDaemonSessionIds }) + // Why (design doc §9): log counts so post-release we can tell whether + // the reaper is firing at all and whether the three gates (keep set, + // mtime grace, quarantine) are in their expected ratios. Persistent + // non-zero `reaped` without corresponding shrinkage of history dirs in + // the wild would indicate a logic bug worth investigating. + console.log('[history-reaper] startup pass stats:', reapStats) } catch (error) { console.warn('[history-reaper] startup pass failed:', error) } diff --git a/tests/e2e/terminal-restart-persistence.spec.ts b/tests/e2e/terminal-restart-persistence.spec.ts index 87da9ab28d2..d85d9b0ca12 100644 --- a/tests/e2e/terminal-restart-persistence.spec.ts +++ b/tests/e2e/terminal-restart-persistence.spec.ts @@ -27,7 +27,8 @@ * removed periodic save represented. */ -import { readFileSync, existsSync } from 'fs' +import { readFileSync, writeFileSync, existsSync } from 'fs' +import { join } from 'path' import type { ElectronApplication, Page } from '@stablyai/playwright-test' import { test, expect } from './helpers/orca-app' import { TEST_REPO_PATH_FILE } from './global-setup' @@ -227,6 +228,83 @@ test.describe('Terminal restart persistence', () => { } }) + test('scrollback survives quit with missing ptyId (Issue #217 regression)', async (// oxlint-disable-next-line no-empty-pattern -- Playwright's second fixture arg is testInfo; the first must be an object destructure to opt out of the default fixture set. + {}, testInfo) => { + // Why (design doc §7.2): this is the load-bearing test for the + // deterministic-sessionId fix. Issue #217 failed specifically when + // orca-data.json's `ptyId` field was empty at restore time (the + // SIGKILL-between-spawn-and-persist window). Before the fix, the + // renderer had no way to reach the on-disk history dir without that + // field. After the fix, mintPtySessionId(worktreeId, tabId, leafId) + // reproduces the same id, so the cold-restore reader finds the dir + // anyway. We simulate the race here by mutating the persisted + // orca-data.json between launches — deleting the ptyId field gives + // us the exact state the old code couldn't recover from, without + // the flakiness of racing a real SIGKILL. + const repoPath = readFileSync(TEST_REPO_PATH_FILE, 'utf-8').trim() + if (!repoPath || !existsSync(repoPath)) { + test.skip(true, 'Global setup did not produce a seeded test repo') + return + } + + const session = createRestartSession(testInfo) + let firstApp: ElectronApplication | null = null + let secondApp: ElectronApplication | null = null + + try { + // ── First launch: produce scrollback, let clean quit flush state ── + const firstLaunch = await session.launch() + firstApp = firstLaunch.app + const { worktreeId, ptyId } = await bootstrapFirstLaunch(firstLaunch.page, repoPath) + + const marker = `DETERMINISTIC_RESTORE_${Date.now()}` + await execInTerminal(firstLaunch.page, ptyId, `echo ${marker}`) + await waitForTerminalOutput(firstLaunch.page, marker) + + await session.close(firstApp) + firstApp = null + + // Simulate the SIGKILL-before-persist race: strip ptyId from every + // tab in the persisted session. The terminal-history/ dir stays on + // disk (it was written by the daemon's checkpoint tick, not by + // orca-data.json). Pre-fix code would fail to find it; post-fix + // code re-derives the id from (worktreeId, tabId, leafId) and + // hits the same directory. + const dataFile = join(session.userDataDir, 'orca-data.json') + const data = JSON.parse(readFileSync(dataFile, 'utf-8')) as { + workspaceSession?: { + tabsByWorktree?: Record + } + } + const tabs = data.workspaceSession?.tabsByWorktree?.[worktreeId] ?? [] + for (const tab of tabs) { + tab.ptyId = null + } + writeFileSync(dataFile, JSON.stringify(data)) + + // ── Second launch: deterministic path must recover scrollback ──── + const secondLaunch = await session.launch() + secondApp = secondLaunch.app + await bootstrapRestoredLaunch(secondLaunch.page, worktreeId) + + await expect + .poll(async () => (await getTerminalContent(secondLaunch.page)).includes(marker), { + timeout: 15_000, + message: + 'Deterministic-sessionId restore did not recover scrollback when ptyId was missing' + }) + .toBe(true) + } finally { + if (secondApp) { + await session.close(secondApp) + } + if (firstApp) { + await session.close(firstApp) + } + session.dispose() + } + }) + test('idle session does not spam session.set writes', async (// oxlint-disable-next-line no-empty-pattern -- Playwright's second fixture arg is testInfo; the first must be an object destructure to opt out of the default fixture set. {}, testInfo) => { const repoPath = readFileSync(TEST_REPO_PATH_FILE, 'utf-8').trim()