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 <help@stably.ai> * 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 <help@stably.ai> --------- Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Neil
GitHub
parent
6142ec1a06
commit
b51dd80252
|
|
@ -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[] }
|
||||
|
|
|
|||
|
|
@ -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<typeof setTimeout> | null = null
|
||||
private pendingWrite: Promise<void> | 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<string, string>()
|
||||
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<void> {
|
||||
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<void> {
|
||||
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 ──────────────────────────────────────────────────────────
|
||||
|
|
|
|||
Loading…
Reference in New Issue