From 995ee888911fe15beb7140fb6a33cac00fb46dfa Mon Sep 17 00:00:00 2001
From: Neil <4138956+nwparker@users.noreply.github.com>
Date: Mon, 20 Jul 2026 19:47:58 -0700
Subject: [PATCH] perf(persistence): single full-state serialization per save
 (#9381)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## Description

Every durable save serialized the ~1.5 MB durable state **twice**, synchronously on the main Electron thread: once in `computeStateHash()` (the plaintext hash for the no-op-write guard) and again in `buildStateToSave()` (the encrypted on-disk payload). The guard couldn't hash the payload directly because `encrypt()` on random-IV platforms produces different bytes each save for identical state.

This PR collapses the two into **one** `JSON.stringify`. `buildStateToSave()` now returns `{ payload, stateHash }`: each encrypted secret slot is serialized as a **fresh per-slot random sentinel** (`orca-secret-slot-<uuid>`), and after the single stringify each sentinel is substituted exactly once — to its ciphertext for the on-disk `payload`, and to its plaintext for the guard `hashInput`. The guard hash is therefore a pure function of the plaintext state (`hashInput === JSON.stringify(plaintext durable state)`), and the on-disk `payload` is byte-identical to the previous two-stringify output.

No change to the save API, debounce timings, guard semantics, or on-disk format (compact JSON, same 3 encrypted fields).

### Why sentinels (not a string replace of the ciphertext)

The substitution must be **position-exact**. A naive `payload.replace(ciphertext, plaintext)` — or even a `"key":"ciphertext"`-anchored replace — can be **mimicked by user-controlled state**, which would substitute the wrong site and let two *distinct* states normalize to the same guard hash → a real change is silently **not written** (data loss). This is reachable on deterministic-IV platforms (macOS / legacy-Linux OSCrypt use a constant IV, so a user can read their own ciphertext out of `orca-data.json`):

- **Value vector**: a plaintext free-text field (e.g. `httpProxyBypassRules`) whose value equals a secret's ciphertext.
- **Key vector**: `agentDefaultEnv` lets the user name an env var exactly after a secret field (e.g. `browserKagiSessionLink`) with a ciphertext value, producing a `"browserKagiSessionLink":"<ciphertext>"` token inside `settings` (before `ui`).

A per-slot random sentinel is minted **after** all user data is captured, so it cannot occur anywhere else in the serialized state and matches only the intended secret slot — closing the entire class. (Both vectors were found by an adversarial fable→gpt-5.6-sol review loop; each has a store-level regression test that fails on the earlier approaches and passes now.)

The empty-secret / `safeStorage`-unavailable / encrypt-failure cases (`blob === plaintext`) get no sentinel and reproduce `main`'s behavior byte-for-byte.

## Evidence

- Full-state `JSON.stringify` calls per changed save: **2 → 1** (call-counting test).
- Frontier-review benchmark on an 8 MiB real-method state: **no perf regression** (median 12.73 ms sentinel vs 13.77 ms two-stringify baseline).
- Independent review modeled 625 adversarial durable states: every payload byte-identical to the pre-PR implementation, and every distinct state produced a distinct guard hash (no dropped-write collision).

## No-Regression Proof

`src/main/persistence-single-serialize.test.ts` (nondeterministic **and** deterministic cipher mocks):

- identical state with secrets set → no-op (inode unchanged); real change (incl. secret rotation) → writes
- on-disk payload is ciphertext (plaintext appears nowhere), round-trips through reload/decrypt
- empty secret + `isEncryptionAvailable() === false` → plaintext payload, guard still skips
- sync `flushOrThrow()` also skips on identical state
- exactly one full-state serialization per save
- **regression (value vector)**: a plaintext field equal to a secret's ciphertext, then swapped → write is not skipped, round-trips
- **regression (key vector)**: an `agentDefaultEnv` var named after a secret field holding its ciphertext, then swapped → write is not skipped, round-trips

Suites: `persistence-single-serialize.test.ts` + `persistence.test.ts` all green; `pnpm run typecheck:node` clean; oxlint clean.

## ELI5

Orca saves your app state whenever something changes, and it used to build the whole (large) state twice each time — once to check "did anything actually change?" and once to write the file. Now it builds it once and reuses that copy for both jobs. To compare states safely it swaps each encrypted secret for a random one-time marker while checking, so no value you type can ever be mistaken for a secret and trick it into skipping a real save. What lands on disk is exactly the same, and your secrets stay encrypted.

Made with [Orca](https://github.com/stablyai/orca) 🐋
---
 src/main/persistence-single-serialize.test.ts | 291 ++++++++++++++++++
 src/main/persistence.ts                       |  67 ++--
 2 files changed, 338 insertions(+), 20 deletions(-)
 create mode 100644 src/main/persistence-single-serialize.test.ts

diff --git a/src/main/persistence-single-serialize.test.ts b/src/main/persistence-single-serialize.test.ts
new file mode 100644
index 000000000..66c3b4ab0
--- /dev/null
+++ b/src/main/persistence-single-serialize.test.ts
@@ -0,0 +1,291 @@
+// Why this file exists: persistence.test.ts mocks safeStorage.encryptString
+// deterministically, which cannot catch the real-world hazard the single-
+// stringify save guard must survive — encrypt() uses a random IV, so identical
+// state produces different on-disk bytes each save. These tests mock a
+// nondeterministic cipher and pin the guard + payload invariants.
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
+import { readFileSync, rmSync, mkdtempSync, statSync } from 'node:fs'
+import { join } from 'node:path'
+import { tmpdir } from 'node:os'
+import { randomUUID } from 'node:crypto'
+
+const testState = { dir: '' }
+
+vi.mock('./ssh/ssh-config-parser', () => ({
+  loadUserSshConfig: vi.fn(),
+  sshConfigHostsToTargets: vi.fn()
+}))
+
+// Nondeterministic cipher: same plaintext → different ciphertext every call,
+// like safeStorage's random IV. encryptionAvailable is toggleable per test.
+// deterministic=true mimics macOS/legacy-Linux OSCrypt (constant IV → same
+// plaintext always yields the same ciphertext), which is what makes the
+// cross-field blob-collision reachable by a user.
+const cipherState = { encryptionAvailable: true, deterministic: false }
+const DETERMINISTIC_IV = 'd'.repeat(36)
+
+vi.mock('electron', () => ({
+  app: {
+    getPath: () => testState.dir
+  },
+  safeStorage: {
+    isEncryptionAvailable: () => cipherState.encryptionAvailable,
+    encryptString: (plaintext: string) =>
+      Buffer.from(
+        `enc:${cipherState.deterministic ? DETERMINISTIC_IV : randomUUID()}:${plaintext}`,
+        'utf-8'
+      ),
+    decryptString: (ciphertext: Buffer) => {
+      const decoded = ciphertext.toString('utf-8')
+      if (!decoded.startsWith('enc:')) {
+        throw new Error('invalid ciphertext')
+      }
+      return decoded.slice('enc:'.length + 36 + 1)
+    }
+  }
+}))
+
+vi.mock('./telemetry/client', () => ({
+  track: vi.fn()
+}))
+
+vi.mock('./telemetry/cohort-classifier', () => ({
+  getCohortAtEmit: vi.fn().mockReturnValue({ nth_repo_added: 2 })
+}))
+
+async function createStore() {
+  vi.resetModules()
+  const { Store, initDataPath } = await import('./persistence')
+  initDataPath()
+  return new Store()
+}
+
+function dataFile(): string {
+  return join(testState.dir, 'orca-data.json')
+}
+
+const SECRETS = {
+  opencodeSessionCookie: 'cookie-$&-value',
+  httpProxyUrl: 'http://user:p@ss@proxy.local:8080'
+} as const
+const KAGI_LINK = 'https://kagi.com/session?token=abc123'
+
+describe('persistence single-serialize save guard', () => {
+  beforeEach(() => {
+    testState.dir = mkdtempSync(join(tmpdir(), 'orca-test-'))
+    cipherState.encryptionAvailable = true
+    cipherState.deterministic = false
+    vi.useFakeTimers()
+  })
+
+  afterEach(() => {
+    vi.useRealTimers()
+    rmSync(testState.dir, { recursive: true, force: true })
+  })
+
+  async function seedStoreWithSecrets() {
+    const store = await createStore()
+    store.updateSettings({ ...SECRETS })
+    store.updateUI({ browserKagiSessionLink: KAGI_LINK })
+    vi.advanceTimersByTime(1000)
+    await store.waitForPendingWrite()
+    return store
+  }
+
+  it('skips the disk write when state is identical, even with secrets set (random-IV cipher)', async () => {
+    const store = await seedStoreWithSecrets()
+    const inoBefore = statSync(dataFile()).ino
+
+    // Net no-op mutation burst: the encrypted payload bytes would differ
+    // (random IV), but the normalized guard hash must not.
+    const originalWidth = store.getUI().sidebarWidth
+    store.updateUI({ sidebarWidth: 512 })
+    store.updateUI({ sidebarWidth: originalWidth })
+    store.updateSettings({ ...SECRETS })
+    vi.advanceTimersByTime(2000)
+    await store.waitForPendingWrite()
+
+    expect(statSync(dataFile()).ino).toBe(inoBefore)
+  })
+
+  it('still writes when state actually changes (including a secret change)', async () => {
+    const store = await seedStoreWithSecrets()
+    const inoBefore = statSync(dataFile()).ino
+
+    store.updateSettings({ opencodeSessionCookie: 'rotated-cookie' })
+    vi.advanceTimersByTime(2000)
+    await store.waitForPendingWrite()
+    const inoAfter = statSync(dataFile()).ino
+    expect(inoAfter).not.toBe(inoBefore)
+
+    store.updateUI({ sidebarWidth: 777 })
+    vi.advanceTimersByTime(2000)
+    await store.waitForPendingWrite()
+    expect(statSync(dataFile()).ino).not.toBe(inoAfter)
+  })
+
+  it('writes encrypted secrets to disk and round-trips them through a reload', async () => {
+    await seedStoreWithSecrets()
+
+    const raw = readFileSync(dataFile(), 'utf-8')
+    const persisted = JSON.parse(raw) as {
+      settings: { opencodeSessionCookie: string; httpProxyUrl: string }
+      ui: { browserKagiSessionLink: string }
+    }
+    // Secrets are ciphertext on disk, plaintext nowhere in the payload.
+    expect(persisted.settings.opencodeSessionCookie).not.toBe(SECRETS.opencodeSessionCookie)
+    expect(persisted.settings.httpProxyUrl).not.toBe(SECRETS.httpProxyUrl)
+    expect(persisted.ui.browserKagiSessionLink).not.toBe(KAGI_LINK)
+    expect(raw).not.toContain(SECRETS.opencodeSessionCookie)
+    expect(raw).not.toContain(KAGI_LINK)
+
+    const reloaded = await createStore()
+    expect(reloaded.getSettings().opencodeSessionCookie).toBe(SECRETS.opencodeSessionCookie)
+    expect(reloaded.getSettings().httpProxyUrl).toBe(SECRETS.httpProxyUrl)
+    expect(reloaded.getUI().browserKagiSessionLink).toBe(KAGI_LINK)
+  })
+
+  it('handles empty secrets and unavailable encryption (payload stays plaintext, guard still skips)', async () => {
+    cipherState.encryptionAvailable = false
+    const store = await createStore()
+    store.updateSettings({ ...SECRETS, opencodeSessionCookie: '' })
+    vi.advanceTimersByTime(1000)
+    await store.waitForPendingWrite()
+
+    const persisted = JSON.parse(readFileSync(dataFile(), 'utf-8')) as {
+      settings: { opencodeSessionCookie: string; httpProxyUrl: string }
+    }
+    expect(persisted.settings.opencodeSessionCookie).toBe('')
+    expect(persisted.settings.httpProxyUrl).toBe(SECRETS.httpProxyUrl)
+
+    const inoBefore = statSync(dataFile()).ino
+    store.updateSettings({ httpProxyUrl: SECRETS.httpProxyUrl })
+    vi.advanceTimersByTime(2000)
+    await store.waitForPendingWrite()
+    expect(statSync(dataFile()).ino).toBe(inoBefore)
+  })
+
+  it('sync flush also skips on identical state with secrets set', async () => {
+    const store = await seedStoreWithSecrets()
+    const inoBefore = statSync(dataFile()).ino
+
+    store.flushOrThrow()
+
+    expect(statSync(dataFile()).ino).toBe(inoBefore)
+  })
+
+  it('performs exactly one full-state JSON.stringify per save (was two)', async () => {
+    const store = await seedStoreWithSecrets()
+
+    // Count full-state serializations: only the durable-state payload is
+    // anywhere near this size in the save path (tiny stringifies elsewhere
+    // stay far below the threshold).
+    const original = JSON.stringify.bind(JSON)
+    let fullStateSerializations = 0
+    const spy = vi.spyOn(JSON, 'stringify').mockImplementation(((
+      value: unknown,
+      ...rest: unknown[]
+    ) => {
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      const out = original(value as any, ...(rest as [any?, any?]))
+      if (typeof out === 'string' && out.length > 1_000) {
+        fullStateSerializations++
+      }
+      return out
+    }) as typeof JSON.stringify)
+    try {
+      store.updateUI({ sidebarWidth: 640 })
+      vi.advanceTimersByTime(2000)
+      await store.waitForPendingWrite()
+    } finally {
+      spy.mockRestore()
+    }
+
+    expect(fullStateSerializations).toBe(1)
+  })
+
+  // Regression (adversarial review, gpt-5.6-sol round 1): the guard hash
+  // normalizes encrypted secrets back to plaintext. A user-controlled plaintext
+  // VALUE (httpProxyBypassRules) can equal a secret's ciphertext; on a
+  // deterministic cipher (macOS/legacy-Linux OSCrypt constant IV) a string
+  // search for the ciphertext would substitute the wrong site, letting two
+  // distinct states hash equal → a silently dropped write (data loss). The
+  // position-exact sentinel substitution must keep the hashes distinct.
+  it('persists a swap between a plaintext field and a secret when the plaintext equals the secret ciphertext (deterministic cipher)', async () => {
+    cipherState.deterministic = true
+    const store = await createStore()
+
+    const P = 'cookie-plaintext-value'
+    // Persist cookie=P, then read its on-disk ciphertext C (what a user could
+    // copy out of orca-data.json).
+    store.updateSettings({ opencodeSessionCookie: P })
+    vi.advanceTimersByTime(1000)
+    await store.waitForPendingWrite()
+    const C = (
+      JSON.parse(readFileSync(dataFile(), 'utf-8')) as {
+        settings: { opencodeSessionCookie: string }
+      }
+    ).settings.opencodeSessionCookie
+    expect(C).not.toBe(P) // C is ciphertext
+
+    // State 1: the plaintext bypass-rules field literally holds ciphertext C;
+    // cookie is still P (which also encrypts to C under the deterministic IV).
+    store.updateSettings({ httpProxyBypassRules: C, opencodeSessionCookie: P })
+    vi.advanceTimersByTime(2000)
+    await store.waitForPendingWrite()
+    const inoState1 = statSync(dataFile()).ino
+
+    // State 2 (distinct): swap the two values. Must be written, not skipped.
+    store.updateSettings({ httpProxyBypassRules: P, opencodeSessionCookie: C })
+    vi.advanceTimersByTime(2000)
+    await store.waitForPendingWrite()
+    expect(statSync(dataFile()).ino).not.toBe(inoState1)
+
+    // The swap round-trips through a reload — nothing was lost.
+    const reloaded = await createStore()
+    expect(reloaded.getSettings().httpProxyBypassRules).toBe(P)
+    expect(reloaded.getSettings().opencodeSessionCookie).toBe(C)
+  })
+
+  // Regression (adversarial review, gpt-5.6-sol round 2): the deeper variant of
+  // the same class — a user-controlled JSON KEY. agentDefaultEnv lets the user
+  // name an env var exactly after a secret field, so `"browserKagiSessionLink":
+  // "<ciphertext>"` can appear (inside settings, before ui) as a non-secret
+  // entry. A key-anchored string replace would substitute that site instead of
+  // the real ui field. The per-slot sentinel is unguessable, so it only ever
+  // matches the real secret slot and the two distinct states stay distinct.
+  it('persists a swap when an agentDefaultEnv var is named after a secret field and holds its ciphertext (deterministic cipher)', async () => {
+    cipherState.deterministic = true
+    const store = await createStore()
+
+    const K = 'https://kagi.com/session?token=SECRET'
+    store.updateUI({ browserKagiSessionLink: K })
+    vi.advanceTimersByTime(1000)
+    await store.waitForPendingWrite()
+    const C = (
+      JSON.parse(readFileSync(dataFile(), 'utf-8')) as {
+        ui: { browserKagiSessionLink: string }
+      }
+    ).ui.browserKagiSessionLink
+    expect(C).not.toBe(K) // C is ciphertext
+
+    // State 1: env var literally named after the secret field, value = C; the
+    // real ui secret is still K.
+    store.updateSettings({ agentDefaultEnv: { claude: { browserKagiSessionLink: C } } })
+    store.updateUI({ browserKagiSessionLink: K })
+    vi.advanceTimersByTime(2000)
+    await store.waitForPendingWrite()
+    const inoState1 = statSync(dataFile()).ino
+
+    // State 2 (distinct): swap — env var value = K, ui secret = C. Must write.
+    store.updateSettings({ agentDefaultEnv: { claude: { browserKagiSessionLink: K } } })
+    store.updateUI({ browserKagiSessionLink: C })
+    vi.advanceTimersByTime(2000)
+    await store.waitForPendingWrite()
+    expect(statSync(dataFile()).ino).not.toBe(inoState1)
+
+    const reloaded = await createStore()
+    expect(reloaded.getSettings().agentDefaultEnv?.claude?.browserKagiSessionLink).toBe(K)
+    expect(reloaded.getUI().browserKagiSessionLink).toBe(C)
+  })
+})
diff --git a/src/main/persistence.ts b/src/main/persistence.ts
index a1b3f3074..aba79a456 100644
--- a/src/main/persistence.ts
+++ b/src/main/persistence.ts
@@ -266,10 +266,6 @@ function decrypt(ciphertext: string): string {
   }
 }
 
-function encryptOptionalSecret(value: string | null | undefined): string | null {
-  return value ? encrypt(value) : null
-}
-
 function decryptOptionalSecret(value: string | null | undefined): string | null {
   return value ? decrypt(value) : null
 }
@@ -2536,7 +2532,7 @@ export class Store {
   private writeGeneration = 0
   // Why: after a profile transfer rewrites this file on disk, a late flush of stale in-memory state would resurrect the moved project.
   private writesFrozen = false
-  // Plaintext hash at last write, to skip no-op writes; not the payload, since encrypt() uses a random IV per call.
+  // Content hash at last write, to skip no-op writes; derived from the payload with encrypted blobs normalized back to plaintext (see buildStateToSave), since encrypt() uses a random IV per call.
   private lastWrittenStateHash: string | null = null
   private firstPendingSaveAt: number | null = null
   private githubCacheDirty = false
@@ -3506,27 +3502,61 @@ export class Store {
     return durable
   }
 
-  private computeStateHash(): string {
-    return createHash('sha1').update(JSON.stringify(this.getDurableState())).digest('hex')
-  }
-
-  // Why: build payload synchronously so hash and serialized bytes reflect the same state tick (no await interleave).
-  private buildStateToSave(): string {
+  // Why: build payload synchronously so hash and serialized bytes reflect the same state tick (no await interleave). One full-state stringify serves both the on-disk payload and the no-op-write guard hash: each secret slot is serialized as a fresh unguessable sentinel, then sentinels are substituted to ciphertext for the payload and to plaintext for the hash. The hash is thus a pure function of plaintext state (skips a byte-identical rewrite) without a second stringify.
+  private buildStateToSave(): { payload: string; stateHash: string } {
+    // Why sentinels (not a blob/key string match): the substitution must be
+    // position-exact. A plain search for the ciphertext — or even for a
+    // `"key":"blob"` token — can be mimicked by user-controlled state (e.g. an
+    // agentDefaultEnv var named after a secret field, or a value equal to a
+    // ciphertext), which would substitute the wrong site and let two DISTINCT
+    // states normalize equal → a silently dropped write (data loss), reachable
+    // on deterministic-IV platforms (macOS/legacy-Linux OSCrypt). A per-slot
+    // random UUID can't occur anywhere else in the serialized state (the user
+    // sets their data before it is minted), so it appears exactly once.
+    const secretSubs: { sentinel: string; blob: string; plaintext: string }[] = []
+    const encryptToSentinel = (plaintext: string): string => {
+      const blob = encrypt(plaintext)
+      // Deterministic already (empty secret / safeStorage unavailable / encrypt
+      // failure): blob === plaintext, so no normalization — and no sentinel,
+      // which also avoids substituting an empty or plaintext-shaped slot.
+      if (blob === plaintext) {
+        return blob
+      }
+      const sentinel = `orca-secret-slot-${randomUUID()}`
+      secretSubs.push({ sentinel, blob, plaintext })
+      return sentinel
+    }
     // Why: clone before encrypting secrets so in-memory this.state stays plaintext.
     const stateToSave = {
       ...this.getDurableState(),
       settings: {
         ...this.state.settings,
-        opencodeSessionCookie: encrypt(this.state.settings.opencodeSessionCookie),
-        httpProxyUrl: encrypt(this.state.settings.httpProxyUrl ?? '')
+        opencodeSessionCookie: encryptToSentinel(this.state.settings.opencodeSessionCookie),
+        httpProxyUrl: encryptToSentinel(this.state.settings.httpProxyUrl ?? '')
       },
       ui: {
         ...this.state.ui,
-        browserKagiSessionLink: encryptOptionalSecret(this.state.ui.browserKagiSessionLink)
+        browserKagiSessionLink: this.state.ui.browserKagiSessionLink
+          ? encryptToSentinel(this.state.ui.browserKagiSessionLink)
+          : null
       }
     }
     // Why compact: ~20% fewer bytes and less serialize time; all readers JSON.parse so formatting is irrelevant.
-    return JSON.stringify(stateToSave)
+    // One full-state stringify; secret slots currently hold sentinels.
+    const serialized = JSON.stringify(stateToSave)
+    // Substitute each unique sentinel exactly once: ciphertext for the on-disk
+    // payload, plaintext for the guard hash. Function-form replacement keeps
+    // `$` in blob/plaintext inert; both sides read the sentinel as JSON-escaped
+    // in `serialized`, so each replace is byte-for-byte position-exact.
+    let payload = serialized
+    let hashInput = serialized
+    for (const { sentinel, blob, plaintext } of secretSubs) {
+      const escapedSentinel = JSON.stringify(sentinel).slice(1, -1)
+      payload = payload.replace(escapedSentinel, () => blob)
+      hashInput = hashInput.replace(escapedSentinel, () => JSON.stringify(plaintext).slice(1, -1))
+    }
+    const stateHash = createHash('sha1').update(hashInput).digest('hex')
+    return { payload, stateHash }
   }
 
   // Why: async writes avoid blocking the main Electron thread on every debounced save.
@@ -3535,12 +3565,11 @@ export class Store {
       return
     }
     const gen = this.writeGeneration
-    const stateHash = this.computeStateHash()
+    const { payload, stateHash } = this.buildStateToSave()
     // Why: don't rewrite a byte-identical multi-MB file when state nets out to already-persisted.
     if (stateHash === this.lastWrittenStateHash) {
       return
     }
-    const payload = this.buildStateToSave()
     const dataFile = this.dataFile
     const dir = dirname(dataFile)
     await mkdir(dir, { recursive: true }).catch(() => {})
@@ -3580,7 +3609,7 @@ export class Store {
     if (this.writesFrozen) {
       return
     }
-    const stateHash = this.computeStateHash()
+    const { payload, stateHash } = this.buildStateToSave()
     // Why: matching hash means the file already holds this state; force overrides when an async rename may be racing past the gen check.
     if (!opts.force && stateHash === this.lastWrittenStateHash) {
       return
@@ -3592,8 +3621,6 @@ export class Store {
     }
     const tmpFile = `${dataFile}.${process.pid}.${Date.now()}.${Math.random().toString(16).slice(2)}.tmp`
 
-    const payload = this.buildStateToSave()
-
     // Why: on any write/rename failure, remove the tmp file so shutdown crashes don't leak orphans.
     let renamed = false
     try {