From b51dd802524bbeebbe60f66c3d560f7846497563 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 2 Jul 2026 01:56:25 -0700 Subject: [PATCH] Skip no-op state writes and bound save postponement in persistence (#7092) * Skip no-op state writes and bound save postponement in persistence orca-data.json (1.6MB live) was fully rewritten (pretty-print + tmp + rename) on every debounced save even when the state content had not changed - measured 3 rewrites/min (~5MB/min) on an idle production instance, and a sync-flush storm of identical multi-MB writes at every warm start via persistPtyBinding re-binds. - Content-hash guard in both writers: a save whose plaintext state hash matches the last successful write skips serialize+write+rename entirely. Hashing plaintext (not the payload) because encrypt() uses a random IV per call. Safe under flushOrThrow's durability contract: a matching hash means the file already holds exactly this state. - Debounce 300ms -> 1s trailing with a 5s max-wait. The old timer reset on every mutation with no bound, so sustained sub-interval mutation bursts could postpone the write indefinitely; now staleness is capped at 5s while bursts coalesce. Co-authored-by: Orca * Guard the state-hash against sync-flush interleaving mid-rename From adversarial review: an async writer that had already passed its generation check and dispatched the rename could have a sync flush interleave during the await, write fresher state, and record its hash - the async continuation then clobbered lastWrittenStateHash with a value describing content NOT on disk, making later saves (including the quit flush) silently skip. Re-check writeGeneration before recording. Twin edge: a hash-matching sync flush could skip its write while a stale dispatched rename lands afterwards, leaving stale content unchallenged. flushOrThrow now forces the sync write whenever an async write chain was in flight at entry, restoring the sync-write-last disk ordering. Co-authored-by: Orca --------- Co-authored-by: Orca --- src/main/persistence.test.ts | 130 ++++++++++++++++++++++++++++++++--- src/main/persistence.ts | 99 ++++++++++++++++++-------- 2 files changed, 190 insertions(+), 39 deletions(-) diff --git a/src/main/persistence.test.ts b/src/main/persistence.test.ts index d60033eac..b1856b1a8 100644 --- a/src/main/persistence.test.ts +++ b/src/main/persistence.test.ts @@ -10,6 +10,7 @@ import { mkdirSync, existsSync, realpathSync, + statSync, symlinkSync } from 'node:fs' import { join } from 'node:path' @@ -4442,7 +4443,7 @@ describe('Store', () => { const store = await createStore() store.addRepo(makeRepo()) store.flush() - vi.advanceTimersByTime(300) + vi.advanceTimersByTime(1000) const persisted = readDataFile() as { repos: Repo[] } expect(persisted.repos).toHaveLength(1) @@ -4462,9 +4463,9 @@ describe('Store', () => { // Before the debounce fires, file should not exist yet (or be stale) vi.advanceTimersByTime(100) - // The 300ms debounce hasn't elapsed yet + // The 1s debounce hasn't elapsed yet - vi.advanceTimersByTime(300) + vi.advanceTimersByTime(1000) // The timer fired; wait for the async disk write to complete await store.waitForPendingWrite() @@ -4476,6 +4477,113 @@ describe('Store', () => { } }) + // ── Content-hash write skipping ──────────────────────────────────── + // Why inode comparison: every real write is a tmp+rename, which allocates a + // new inode. An unchanged inode proves no write happened. + + it('skips the disk write when a mutation burst nets out to already-persisted state', async () => { + vi.useFakeTimers() + try { + const store = await createStore() + store.updateUI({ sidebarWidth: 400 }) + vi.advanceTimersByTime(1000) + await store.waitForPendingWrite() + const inoBefore = statSync(dataFile()).ino + + store.updateUI({ sidebarWidth: 500 }) + store.updateUI({ sidebarWidth: 400 }) + vi.advanceTimersByTime(2000) + await store.waitForPendingWrite() + + expect(statSync(dataFile()).ino).toBe(inoBefore) + } finally { + vi.useRealTimers() + } + }) + + it('skips the sync flush when state already matches the last write', async () => { + vi.useFakeTimers() + try { + const store = await createStore() + store.updateUI({ sidebarWidth: 420 }) + vi.advanceTimersByTime(1000) + await store.waitForPendingWrite() + const inoBefore = statSync(dataFile()).ino + + store.flush() + + expect(statSync(dataFile()).ino).toBe(inoBefore) + } finally { + vi.useRealTimers() + } + }) + + it('bounds save postponement under sustained mutation bursts (max-wait)', async () => { + vi.useFakeTimers() + try { + const store = await createStore() + // Mutations every 500ms keep resetting the 1s trailing debounce; the + // 5s max-wait must force a write anyway. + let width = 400 + for (let i = 0; i < 11; i++) { + store.updateUI({ sidebarWidth: width++ }) + vi.advanceTimersByTime(500) + } + await store.waitForPendingWrite() + + expect(existsSync(dataFile())).toBe(true) + const persisted = readDataFile() as { ui: { sidebarWidth: number } } + expect(persisted.ui.sidebarWidth).toBeGreaterThanOrEqual(400) + } finally { + vi.useRealTimers() + } + }) + + it('re-binding an already-persisted pty does not rewrite the state file', async () => { + const store = await createStore() + store.setWorkspaceSession({ + activeRepoId: 'r1', + activeWorktreeId: 'wt1', + activeTabId: 'tab1', + tabsByWorktree: { + wt1: [ + { + id: 'tab1', + worktreeId: 'wt1', + title: 'Terminal', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1, + ptyId: null + } + ] + }, + terminalLayoutsByTabId: { + tab1: { + root: null, + activeLeafId: null, + expandedLeafId: null + } + } + }) + + const binding = { + worktreeId: 'wt1', + tabId: 'tab1', + leafId: TEST_LEAF_1, + ptyId: 'daemon-pty' + } + store.persistPtyBinding(binding) + const inoBefore = statSync(dataFile()).ino + + // The warm-restart re-bind storm: every restored terminal re-asserts an + // identical binding with a sync flush. Identical state must not rewrite. + store.persistPtyBinding(binding) + + expect(statSync(dataFile()).ino).toBe(inoBefore) + }) + // ── UI state ─────────────────────────────────────────────────────── it('updateUI merges partial updates', async () => { @@ -4518,7 +4626,7 @@ describe('Store', () => { tasks: { firstInteractedAt: 100, interactionCount: 1 } } }) - vi.advanceTimersByTime(300) + vi.advanceTimersByTime(1000) await store.waitForPendingWrite() const persistedBefore = readFileSync(dataFile(), 'utf-8') store.onUIChanged((ui) => notifications.push(ui)) @@ -4532,7 +4640,7 @@ describe('Store', () => { tasks: { firstInteractedAt: 100, interactionCount: 1 } } }) - vi.advanceTimersByTime(300) + vi.advanceTimersByTime(1000) await store.waitForPendingWrite() expect(notifications).toEqual([]) @@ -8307,7 +8415,7 @@ describe('Store', () => { const store = await createStore() store.addRepo(makeRepo({ id: 'first-async' })) - vi.advanceTimersByTime(300) + vi.advanceTimersByTime(1000) await store.waitForPendingWrite() const bak0AfterFirst = readBackup(0) @@ -8315,7 +8423,7 @@ describe('Store', () => { vi.setSystemTime(new Date(Date.now() + 5 * 60 * 1000)) store.addRepo(makeRepo({ id: 'within-hour-async', path: '/within-async' })) - vi.advanceTimersByTime(300) + vi.advanceTimersByTime(1000) await store.waitForPendingWrite() const bak0AfterSecond = readBackup(0) @@ -8340,7 +8448,7 @@ describe('Store', () => { const store = await createStore() store.addRepo(makeRepo({ id: 'first-async' })) - vi.advanceTimersByTime(300) + vi.advanceTimersByTime(1000) await store.waitForPendingWrite() expect( @@ -8351,7 +8459,7 @@ describe('Store', () => { vi.setSystemTime(new Date(Date.now() + 61 * 60 * 1000)) store.addRepo(makeRepo({ id: 'after-hour-async', path: '/after-async' })) - vi.advanceTimersByTime(300) + vi.advanceTimersByTime(1000) await store.waitForPendingWrite() expect( @@ -8464,9 +8572,9 @@ describe('Store', () => { try { const store = await createStore() store.addRepo(makeRepo({ id: 'first' })) - vi.advanceTimersByTime(300) + vi.advanceTimersByTime(1000) store.addRepo(makeRepo({ id: 'second', path: '/second' })) - vi.advanceTimersByTime(300) + vi.advanceTimersByTime(1000) await store.waitForPendingWrite() const persisted = JSON.parse(readFileSync(dataFile(), 'utf-8')) as { repos: Repo[] } diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 21b1ff28d..5d9ef610f 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -16,7 +16,7 @@ import { import { writeFile, rename, mkdir, rm, copyFile } from 'node:fs/promises' import { join, dirname, isAbsolute, resolve, sep } from 'node:path' import { homedir } from 'node:os' -import { randomUUID } from 'node:crypto' +import { createHash, randomUUID } from 'node:crypto' import type { Automation, AutomationCreateInput, @@ -2444,6 +2444,13 @@ export class Store { private writeTimer: ReturnType | null = null private pendingWrite: Promise | null = null private writeGeneration = 0 + // Why: hash of the plaintext state as of the last successful write. Saves + // triggered by mutations that net out to identical state skip the full + // 1.6MB pretty-print + tmp write + rename. Hashing plaintext (not the + // written payload) because encrypt() uses a random IV per call, so the + // on-disk bytes differ even for identical state. + private lastWrittenStateHash: string | null = null + private firstPendingSaveAt: number | null = null private gitUsernameCache = new Map() private loadNeedsSave = false private settingsChangeListeners = new Set< @@ -3353,12 +3360,25 @@ export class Store { } } + // Why 1s trailing + 5s max-wait (previously 300ms trailing, unbounded): + // sustained sub-interval mutation bursts used to either rewrite the full + // multi-MB state ~3x/sec or postpone the write indefinitely by resetting + // the timer. The max-wait bounds crash staleness at 5s while bursts + // coalesce; the content-hash guard in the writers skips no-op payloads. + private static SAVE_DEBOUNCE_MS = 1_000 + private static SAVE_MAX_WAIT_MS = 5_000 + private scheduleSave(): void { + const now = Date.now() + this.firstPendingSaveAt ??= now if (this.writeTimer) { clearTimeout(this.writeTimer) } + const untilMaxWait = Math.max(0, this.firstPendingSaveAt + Store.SAVE_MAX_WAIT_MS - now) + const delay = Math.min(Store.SAVE_DEBOUNCE_MS, untilMaxWait) this.writeTimer = setTimeout(() => { this.writeTimer = null + this.firstPendingSaveAt = null // Why (issue #1158): serialize async writes so backup rotation never has // two callers racing over the same dataFile/tmp/.bak paths. const prev = this.pendingWrite ?? Promise.resolve() @@ -3373,7 +3393,7 @@ export class Store { } }) this.pendingWrite = next - }, 300) + }, delay) } /** Wait for any in-flight async disk write to complete. Used in tests. */ @@ -3383,15 +3403,14 @@ export class Store { } } - // Why: async writes avoid blocking the main Electron thread on every - // debounced save (every 300ms during active use). - private async writeToDiskAsync(): Promise { - const gen = this.writeGeneration - const dataFile = getDataFile() - const dir = dirname(dataFile) - await mkdir(dir, { recursive: true }).catch(() => {}) - const tmpFile = `${dataFile}.${process.pid}.${Date.now()}.${Math.random().toString(16).slice(2)}.tmp` + private computeStateHash(): string { + return createHash('sha1').update(JSON.stringify(this.state)).digest('hex') + } + // Why: builds the on-disk payload synchronously so the hash and the + // serialized bytes reflect the same state tick (no mutation can interleave + // before an await). + private buildStateToSave(): string { // Why: secrets must be encrypted on disk. Clone state so the in-memory // this.state stays plaintext for the rest of the app. const stateToSave = { @@ -3406,13 +3425,31 @@ export class Store { browserKagiSessionLink: encryptOptionalSecret(this.state.ui.browserKagiSessionLink) } } + return JSON.stringify(stateToSave, null, 2) + } + + // Why: async writes avoid blocking the main Electron thread on every + // debounced save during active use. + private async writeToDiskAsync(): Promise { + const gen = this.writeGeneration + const stateHash = this.computeStateHash() + // Why: a mutation burst that nets out to already-persisted state (or a + // flush that raced ahead) must not rewrite a byte-identical multi-MB file. + if (stateHash === this.lastWrittenStateHash) { + return + } + const payload = this.buildStateToSave() + const dataFile = getDataFile() + const dir = dirname(dataFile) + await mkdir(dir, { recursive: true }).catch(() => {}) + const tmpFile = `${dataFile}.${process.pid}.${Date.now()}.${Math.random().toString(16).slice(2)}.tmp` // Why: wrap write+rename in try/finally-on-error so any failure (ENOSPC, // ENFILE, EIO, permission) removes the tmp file rather than leaving a // multi-megabyte orphan behind. Successful rename consumes the tmp file. let renamed = false try { - await writeFile(tmpFile, JSON.stringify(stateToSave, null, 2), 'utf-8') + await writeFile(tmpFile, payload, 'utf-8') // Why: if flush() ran while this async write was in-flight, it bumped // writeGeneration and already wrote the latest state synchronously. // Renaming this stale tmp file would overwrite the fresh data. @@ -3421,6 +3458,13 @@ export class Store { } await rename(tmpFile, dataFile) renamed = true + // Why the gen re-check: a sync flush can interleave during the rename + // await, write fresher state, and record its own hash. Recording this + // stale hash over it would make later saves skip against content that + // is not what the file holds. + if (this.writeGeneration === gen) { + this.lastWrittenStateHash = stateHash + } } finally { if (!renamed) { await rm(tmpFile).catch(() => {}) @@ -3439,7 +3483,16 @@ export class Store { // Why: synchronous variant kept only for flush() at shutdown, where the // process may exit before an async write completes. - private writeToDiskSync(): void { + private writeToDiskSync(opts: { force?: boolean } = {}): void { + const stateHash = this.computeStateHash() + // Why: skipping is safe under flushOrThrow's durability contract — a + // matching hash means this exact state is already the file's content. + // Except when an async write was in flight at flush entry (force): its + // rename may already be dispatched past the generation check, and only + // an unconditional sync write afterwards reliably out-orders it. + if (!opts.force && stateHash === this.lastWrittenStateHash) { + return + } const dataFile = getDataFile() const dir = dirname(dataFile) if (!existsSync(dir)) { @@ -3447,29 +3500,17 @@ export class Store { } const tmpFile = `${dataFile}.${process.pid}.${Date.now()}.${Math.random().toString(16).slice(2)}.tmp` - // Why: secrets must be encrypted on disk. Clone state so the in-memory - // this.state stays plaintext for the rest of the app. - const stateToSave = { - ...this.state, - settings: { - ...this.state.settings, - opencodeSessionCookie: encrypt(this.state.settings.opencodeSessionCookie), - httpProxyUrl: encrypt(this.state.settings.httpProxyUrl ?? '') - }, - ui: { - ...this.state.ui, - browserKagiSessionLink: encryptOptionalSecret(this.state.ui.browserKagiSessionLink) - } - } + const payload = this.buildStateToSave() // Why: mirror the async path — on any failure between writeFileSync and // renameSync, remove the tmp file so crashes during shutdown don't leak // orphans into userData. let renamed = false try { - writeFileSync(tmpFile, JSON.stringify(stateToSave, null, 2), 'utf-8') + writeFileSync(tmpFile, payload, 'utf-8') renameSync(tmpFile, dataFile) renamed = true + this.lastWrittenStateHash = stateHash } finally { if (!renamed) { try { @@ -3490,11 +3531,13 @@ export class Store { clearTimeout(this.writeTimer) this.writeTimer = null } + this.firstPendingSaveAt = null + const asyncWriteWasInFlight = this.pendingWrite !== null // Why: bump writeGeneration so any in-flight async writeToDiskAsync skips // its rename, preventing a stale snapshot from overwriting this sync write. this.writeGeneration++ this.pendingWrite = null - this.writeToDiskSync() + this.writeToDiskSync({ force: asyncWriteWasInFlight }) } // ── Repos ──────────────────────────────────────────────────────────