Repository navigation
refactor(native-chat): keep agent-session records in the chat journal database - #24006
Conversation
…ailing app startup At startup the chat host re-checks every saved chat's lease and writes the result to agent-sessions.json. If that write failed (the file lock gave up, the file could not be written, or the file was written by a newer Orca and is read-only here), reconcileRestartLeases rejected, the startup IPC call rejected, and the renderer fell into its degraded "Session restore failed. Changes won't be saved until restart" mode. The reconcile is bookkeeping: a lease left unreconciled grants no writer, and every attach, send and read of a chat reconciles its own lease again. So the startup reconcile now reports its failure through a new optional host dependency, onStartupReconcileFailure, and resolves. The runtime routes it to its onError sink under the scope structured-agent-session-startup-reconcile, or logs it when no sink is installed (the desktop installs none).
…eping With native chat on and a chat tab open at quit, the renderer's startup also awaits the chat tab restore (session.tabs.listAll). That restore re-ran the lease reconcile before reading each chat and rethrew its store failure, then recorded each restored tab as visible through a store transaction that throws on a held lock or a read-only store. Either one failed the restore, so startup still fell into "Session restore failed". Reading a chat grants no writer, so the reconcile startup and the restore run is now a reader's: createReaderReconcile never throws, answers whether every lease is settled (recovery is resolved only then; the journal opens either way), and reports each distinct failure once until a reconcile settles. Attach and agent start keep the strict reconcile. The restore's tab republish logs a failed visibility write and still publishes the tab, since a client drops every unpublished chat tab; user-driven publishes still refuse. The host dependency is renamed onLeaseReconcileFailure (scope structured-agent-session-lease-reconcile), since it now also reports for reads.
…read path Round-2 review found two more writes on the startup chat restore that could still fail it and put the app into "Session restore failed": republishing a /clear replacement recorded its tab visibility strictly, and resolving a chat's recovery rethrew its store error. The restore also paid one lock wait per tab and per batch of chats while the lock stayed held. The restore now derives tabs from state it already holds: - publishStructuredAgentSessionTab splits into the strict write and projectStructuredAgentSessionTab, which only updates the runtime's snapshot. The restore and /clear replacements only project: a saved tab index already lists every restored chat, and a /clear moves the tab in the same write that commits it. visibilityWriteMayFail is gone. - Chats a legacy profile restores that the index does not list are recorded in one best-effort transaction (store.showSessionTabs), so a failure leaves the index absent to seed again rather than partial. - The read restore's recovery resolution is caught and reported through onLeaseReconcileFailure, deduplicated with the reconcile's reports. - Once lease bookkeeping fails in a restore pass, the rest of that pass skips it, so a held lock costs one wait for the startup reconcile and one for the restore, however many chats are open. User actions (create, reveal, attach, send, the /clear commit) keep their strict writes.
…harness Tests that open the durable agent-session record store, seed it, or read back what it persisted now go through agent-session-record-store-test-harness.ts instead of calling AgentSessionRecordStore.open or touching agent-sessions.json themselves. A later change that moves the store into the chat database then changes the harness instead of every test. No production code changes. Tests whose subject is the JSON file itself (its .bak recovery, salvage, schema versions, permissions, and what older builds read back) keep reading and writing the file directly; the storage move rewrites or deletes them.
…op its bookkeeping at the first failure The restore now runs one reader lease check for the pass and lets each chat re-check and resolve recovery only while the pass is still settled. The first refusal or failed write clears it for the rest of the pass, and every chat is still opened for reading. With another process holding the lock, startup waits on it once in prepare and once in the restore, however many chats are open; a legacy profile waits once more for its tab-index seed.
…artup' into brennanb2025/chat-records-in-journal-db
The harness took the store's own folder, so each caller picked one (join(root, 'store'), or 'agent-sessions' where a test read the store the runtime owns). A later change that moves the store into the state directory's journal database could not tell those apart, and would have had to edit every caller again. Every harness function now takes the state directory, the one the test's journal database and recovery capsule already live in, and keeps the store in the same subfolder the runtime uses. Callers pass that directory; store-only tests pass their temp directory unchanged. Format tests that share a directory with harness calls take the file path from testAgentSessionStoreFilePath. The folder name moves from a private constant in the runtime to AGENT_SESSION_STORE_DIR_NAME beside the store's file name, so the harness shares it without importing the runtime. Its value and every path built from it are unchanged.
… database The record store's records, operation ledger, retired claim keys and chat tab index become tables in agent-session-journal.db (user_version 4). The version-4 migration copies agent-sessions.json in its own transaction and never writes, renames or deletes that file or its .bak. Each store write is one journal transaction over exactly the rows it changed, checked with the load rules; the file lock, the external-change refresh and its hash, the .bak rotation, salvage and the hot-path recovery fence are gone from the store.
…elper' into brennanb2025/chat-records-in-journal-db # Conflicts: # src/main/runtime/agent-session-record-store-legacy-handoff-lease.test.ts # src/main/runtime/agent-session-record-store-tab-table.test.ts # src/main/runtime/agent-session-record-store-test-harness.ts # src/main/runtime/agent-session-record-store.test.ts # src/main/runtime/rpc/methods/agent-launch-replay.test.ts # src/main/runtime/rpc/methods/structured-chat-tab-table.test.ts # src/main/runtime/structured-agent-session-rollback-compatibility.test.ts # src/main/runtime/structured-agent-session-runtime.ts
…s and read-only records
…artup' into brennanb2025/chat-records-in-journal-db # Conflicts: # src/main/runtime/structured-agent-session-startup-tab-restore.test.ts
… and type the import fixture
…ords-in-journal-db # Conflicts: # src/main/native-chat/agent-session-wire/structured-agent-session-settled-attach-retry.test.ts # src/main/runtime/agent-session-record-store-file.ts # src/main/runtime/agent-session-record-store-legacy-handoff-lease.test.ts # src/main/runtime/agent-session-record-store-tab-table.test.ts # src/main/runtime/agent-session-record-store-test-harness.ts # src/main/runtime/agent-session-record-store.test.ts # src/main/runtime/rpc/methods/agent-launch-replay.test.ts # src/main/runtime/rpc/methods/structured-chat-tab-table.test.ts # src/main/runtime/structured-agent-session-integration.test.ts # src/main/runtime/structured-agent-session-rollback-compatibility.test.ts # src/main/runtime/structured-agent-session-runtime.ts
Brings in main at 75040eb. main's record-store test harness moved the host test harness's store from <root>/store to <root>/agent-sessions, so the startup reconcile failure test now opens, edits and reads the relaunched store through agent-session-record-store-test-harness instead of the old path.
… be written Stop's operation-ledger row now shares the database with the chat history, so damage, a full disk or a stranded transaction on that write refused the Stop before the interrupt. A cancel plan now takes its decision from the committed ledger in memory, runs without settling, and warns that the row was skipped. Other mutations answer proven damage with the typed "Unable to load this chat." refusal instead of the raw SQLite error.
…ase's rows Every host install creates agent-session-journal.db, chats or not, and the version probe created it too, so its mere existence made every profile that ever installed the host wait on host install and reconcile at startup. The check now opens the database read-only and looks for a record or tab row, lets the records file answer while its import is still owed, and counts an unreadable database as present. The version probe no longer creates the file.
…be written Over records a newer Orca wrote, every write is refused, so opening a closed chat from Agent Session History failed on the tab-visibility write and the chat read as unreachable. Like closing a tab, opening one now reports a failed restore-index write and still publishes the tab.
…ails transiently A torn records file whose .bak could not be read (EACCES, EIO) was reported as unusable, so the migration completed with nothing copied and never retried. A non-ENOENT read failure of either copy now carries its cause, which the importer classifies as a read that can clear.
…had no tab index A chat created while the import was owed recorded a tab index holding only itself. When the file it later imported had no index, that index still read as recorded, so the imported chats' tabs never came back. The import now clears the recorded marker in that case, and restore falls back to the profile's tabs.
Nothing called it, and it bypassed the write queue and the read-only refusal.
…d but never re-imported
…as it The chain-validation refactor has no measured need in this change.
…cords file as the live store
…ords-in-journal-db
…ide the profile's chats When the imported records file had no tab index, restore fell back to the profile's saved tabs, which never list a Claude chat, and the seed then rewrote the tab table without the chat opened while the import was owed. The tab rows that chat left are now loaded as unrecorded, restore takes them together with the profile's chats, and the seed keeps their tab ids.
…ords-in-journal-db
…ords-in-journal-db # Conflicts: # src/main/native-chat/agent-session-wire/structured-agent-session-host.ts # src/main/native-chat/agent-session-wire/structured-agent-session-restart-restore.test.ts # src/main/native-chat/agent-session-wire/structured-agent-session-startup-reconcile-failure.test.ts # src/main/runtime/agent-session-record-store.ts # src/main/runtime/agent-session-tab-table.ts # src/main/runtime/orca-runtime-restore-structured-agent-session-tabs-once.ts # src/main/runtime/structured-agent-session-startup-reconcile.test.ts # src/main/runtime/structured-agent-session-startup-tab-restore.test.ts # src/main/runtime/structured-agent-session-tab-index-seed.ts
…e in the Send-now rerun test The record store freezes published rows in tests, so setting a row's outcome in place threw; the test now swaps in a changed copy, as its sibling cases do.
Review status: ready for reviewThe problem. Native (structured) Claude and Codex chats keep an ownership record per chat (who owns it, which operations ran, which tabs were open). On main these live in What changes for the user. Records move into tables in the chat database Orca already has, and each change writes only its own rows. Measured on a Mac and on a Windows high-spec machine, each record write drops to about 0.3-1.3 ms. Steady-state startup is unchanged. The first launch after updating pays a one-time copy of about 0.8 s on Windows and about 45 ms on a Mac. A database written by a newer Orca opens read-only: history reads, and sends say "update Orca". Stop still reaches the agent when its bookkeeping write fails. Fixed during review
Deferred (low severity, not fixed here)
Already on main, found during QA, not changed here
Verified
Not verified
|
|
Comments Outside DiffThese findings could not be posted inline.
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Repository guideline files applied to this review (1)📝 WalkthroughPriority: ➖ Normal Merge Risk: 🔵 Low · up to This moves native chat records into the chat database, and startup imports the old records file. One narrow edge case remains. A set-aside chat record with a malformed field can leave the fence floor too low, so a later grant could reuse a fence that record may still hold. This is mergeable after that small fix or with owner awareness. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The change affects durable chat ownership and recovery. Migration is protected against partial completion, but a Stop completed during a storage failure can lose replay protection and affect later work. Recovery and caller-access behavior were only partially verified. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❓ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 93 functions across 50 files. (4 skipped: 4 over the file limit.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 598df067-c69e-4693-96bf-63081166f5a3
📒 Files selected for processing (68)
.github/workflows/pr.ymlsrc/main/native-chat/agent-session-journal/journal-database-records-migration.test.tssrc/main/native-chat/agent-session-journal/journal-database-schema.tssrc/main/native-chat/agent-session-journal/journal-database.test.tssrc/main/native-chat/agent-session-journal/journal-database.tssrc/main/native-chat/agent-session-journal/journal-host-database-test-support.tssrc/main/native-chat/agent-session-journal/journal-host-database.tssrc/main/native-chat/agent-session-journal/journal-open-failure.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-claude-root-exit.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-test-abandon.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-test-harness.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-test-recovery-capsule.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host.tssrc/main/native-chat/agent-session-wire/structured-agent-session-journal-corruption.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-lease-renewer.tssrc/main/native-chat/agent-session-wire/structured-agent-session-mutation-admission.tssrc/main/native-chat/agent-session-wire/structured-agent-session-mutation-plans.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-gate.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-reconcile.tssrc/main/native-chat/agent-session-wire/structured-agent-session-restart-restore.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-settled-attach-retry.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-startup-reconcile-failure.test.tssrc/main/runtime/agent-session-backup-recovery-fence.tssrc/main/runtime/agent-session-backup-recovery.test.tssrc/main/runtime/agent-session-lease-transitions.tssrc/main/runtime/agent-session-legacy-record-import.test.tssrc/main/runtime/agent-session-legacy-record-import.tssrc/main/runtime/agent-session-record-rows.tssrc/main/runtime/agent-session-record-store-file.tssrc/main/runtime/agent-session-record-store-legacy-handoff-lease.test.tssrc/main/runtime/agent-session-record-store-security.test.tssrc/main/runtime/agent-session-record-store-security.tssrc/main/runtime/agent-session-record-store-tab-table.test.tssrc/main/runtime/agent-session-record-store-test-harness.tssrc/main/runtime/agent-session-record-store-write.tssrc/main/runtime/agent-session-record-store.test.tssrc/main/runtime/agent-session-record-store.tssrc/main/runtime/agent-session-record-unsupported-schema.test.tssrc/main/runtime/agent-session-recovery-publish-fault.test.tssrc/main/runtime/agent-session-store-draft.tssrc/main/runtime/agent-session-store-row-rules.tssrc/main/runtime/agent-session-store-serialization.tssrc/main/runtime/agent-session-store-transaction-queue.tssrc/main/runtime/agent-session-store-transactions.test.tssrc/main/runtime/agent-session-store-transactions.tssrc/main/runtime/agent-session-tab-table.tssrc/main/runtime/agent-session-unreadable-record-salvage.test.tssrc/main/runtime/orca-runtime-close-structured-agent-session-tab.tssrc/main/runtime/orca-runtime-restore-structured-agent-session-tabs-once.tssrc/main/runtime/rpc/methods/agent-launch-replay.test.tssrc/main/runtime/rpc/methods/structured-chat-tab-table.test.tssrc/main/runtime/structured-agent-session-host-refusal.test.tssrc/main/runtime/structured-agent-session-install-failure.test.tssrc/main/runtime/structured-agent-session-integration.test.tssrc/main/runtime/structured-agent-session-journal-open.tssrc/main/runtime/structured-agent-session-record-import-install.test.tssrc/main/runtime/structured-agent-session-rollback-compatibility.test.tssrc/main/runtime/structured-agent-session-runtime.test.tssrc/main/runtime/structured-agent-session-runtime.tssrc/main/runtime/structured-agent-session-spawn-token-descendants.test.tssrc/main/runtime/structured-agent-session-startup-reconcile.test.tssrc/main/runtime/structured-agent-session-startup-tab-restore.test.tssrc/main/runtime/structured-agent-session-tab-index-seed.tssrc/shared/agent-session-next-fence.tssrc/shared/agent-session-operation-ledger.tssrc/shared/agent-session-record.tstests/e2e/cross-version-wire/agent-session-death-evidence-downgrade.unit.test.tstests/e2e/cross-version-wire/agent-session-unproven-release-downgrade.unit.test.ts
💤 Files with no reviewable changes (14)
- src/main/runtime/agent-session-recovery-publish-fault.test.ts
- src/main/runtime/agent-session-record-store-security.test.ts
- src/main/runtime/agent-session-record-store-security.ts
- .github/workflows/pr.yml
- src/main/runtime/agent-session-record-store-legacy-handoff-lease.test.ts
- tests/e2e/cross-version-wire/agent-session-death-evidence-downgrade.unit.test.ts
- src/main/runtime/agent-session-store-serialization.ts
- src/main/runtime/agent-session-lease-transitions.ts
- tests/e2e/cross-version-wire/agent-session-unproven-release-downgrade.unit.test.ts
- src/main/runtime/agent-session-backup-recovery.test.ts
- src/main/runtime/agent-session-store-transaction-queue.ts
- src/main/runtime/agent-session-record-unsupported-schema.test.ts
- src/main/runtime/agent-session-unreadable-record-salvage.test.ts
- src/main/runtime/agent-session-record-store-write.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| const rawLease = typeof raw === 'object' && raw !== null && 'lease' in raw ? raw.lease : undefined | ||
| if (typeof rawLease === 'object' && rawLease !== null && 'runtimeFence' in rawLease) { | ||
| const runtimeFence = safeFence(rawLease.runtimeFence) | ||
| const minimumNextFence = | ||
| 'minimumNextFence' in rawLease && rawLease.minimumNextFence !== undefined | ||
| ? safeFence(rawLease.minimumNextFence) | ||
| : undefined | ||
| if (runtimeFence !== null && minimumNextFence !== null) { | ||
| floor = Math.max(floor, nextAgentSessionFence({ runtimeFence, minimumNextFence })) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The set-aside copy's runtimeFence is ignored when its minimumNextFence is not a safe integer.
withFloorAboveSetAsideCopy raises the floor only when both runtimeFence and minimumNextFence pass safeFence. A set-aside copy is by definition a row this build cannot read. Its minimumNextFence can be null, negative, or a different type in a newer shape. In that case the floor ignores the set-aside runtimeFence, even when it is valid.
Example: the readable copy has fence 2 and the set-aside copy has runtimeFence: 7 with an unparseable minimumNextFence. The floor is 3. Later grants then climb through 3…7. A grant of 7 collides, under strict equality, with the lease the set-aside writer may still hold. This defeats the invariant that the comment on Line 72-76 describes ("The next grant must clear both").
Treat the two fields independently. Each valid value must raise the floor.
🛡️ Proposed fix
- if (typeof rawLease === 'object' && rawLease !== null && 'runtimeFence' in rawLease) {
- const runtimeFence = safeFence(rawLease.runtimeFence)
- const minimumNextFence =
- 'minimumNextFence' in rawLease && rawLease.minimumNextFence !== undefined
- ? safeFence(rawLease.minimumNextFence)
- : undefined
- if (runtimeFence !== null && minimumNextFence !== null) {
- floor = Math.max(floor, nextAgentSessionFence({ runtimeFence, minimumNextFence }))
- }
- }
+ if (typeof rawLease === 'object' && rawLease !== null) {
+ const runtimeFence =
+ 'runtimeFence' in rawLease ? safeFence(rawLease.runtimeFence) : null
+ const minimumNextFence =
+ 'minimumNextFence' in rawLease ? safeFence(rawLease.minimumNextFence) : null
+ if (runtimeFence !== null) {
+ floor = Math.max(floor, runtimeFence + 1)
+ }
+ if (minimumNextFence !== null) {
+ floor = Math.max(floor, minimumNextFence)
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const rawLease = typeof raw === 'object' && raw !== null && 'lease' in raw ? raw.lease : undefined | |
| if (typeof rawLease === 'object' && rawLease !== null && 'runtimeFence' in rawLease) { | |
| const runtimeFence = safeFence(rawLease.runtimeFence) | |
| const minimumNextFence = | |
| 'minimumNextFence' in rawLease && rawLease.minimumNextFence !== undefined | |
| ? safeFence(rawLease.minimumNextFence) | |
| : undefined | |
| if (runtimeFence !== null && minimumNextFence !== null) { | |
| floor = Math.max(floor, nextAgentSessionFence({ runtimeFence, minimumNextFence })) | |
| } | |
| } | |
| const rawLease = typeof raw === 'object' && raw !== null && 'lease' in raw ? raw.lease : undefined | |
| if (typeof rawLease === 'object' && rawLease !== null) { | |
| const runtimeFence = | |
| 'runtimeFence' in rawLease ? safeFence(rawLease.runtimeFence) : null | |
| const minimumNextFence = | |
| 'minimumNextFence' in rawLease ? safeFence(rawLease.minimumNextFence) : null | |
| if (runtimeFence !== null) { | |
| floor = Math.max(floor, runtimeFence + 1) | |
| } | |
| if (minimumNextFence !== null) { | |
| floor = Math.max(floor, minimumNextFence) | |
| } | |
| } |
There was a problem hiding this comment.
ℹ️ No critical issues — one behavior to confirm, plus a coverage note and two nits.
Reviewed changes — the full D-stage diff (68 files) moving native-chat agent-session records out of agent-sessions/agent-sessions.json and into the per-host chat journal database.
- Schema v3 → v4 — adds
agent_session_{records,operations,retired_claim_keys,tabs,store_meta}; the copy ofagent-sessions.jsonruns inside the same transaction as the table creation and theuser_versionbump, and "copied" is exactlyuser_version >= 4. An owed copy makes the tables, keeps version 3, and retries next launch. - Row store — the whole store is read once into memory; each mutation drafts, applies, diffs by identity, validates changed rows with the load rules, writes only those rows in one journal transaction, then publishes. Unreadable rows are derived at load and never rewritten.
- One-time import — reads the file with main's loader (
.bakfallback, salvage, fence floors), distinguishes a transient read (unavailable→ owed) from an unusable file (unusable→ reported, never re-imported), and inserts rows withINSERT OR IGNORE. - Read-only / damage behavior — a newer database opens read-only and writes return the
journalWrittenByNewerOrcarefusal; a Stop runs from the committed ledger when its row cannot be written. - Runtime wiring —
hasPersistedStructuredAgentSessionStoreanswers from database rows (or the legacy file while owed), and the host install reads the legacy file before opening.
I read the production paths and the new/changed tests, and ran three specialist passes (migration atomicity, presence-check reachability, row-diff persistence); none found a concrete bug. The intentional tradeoffs (one-time import, the downgrade window, shared SQLite damage) are documented and pinned by tests.
Technical details
# Verified as correct
- `migrateJournalSchema` cannot leave `user_version` disagreeing with the tables: table creation,
the records write and the bump are one `runJournalTransaction`; a throw rolls back to 3/0.
- `insertAgentSessionStoreRowsIfAbsent` is all `INSERT OR IGNORE`, so an owed-window chat keeps its
rows through the copy.
- `serializeChangedRows`'s `published.size + added !== next.size` shortcut is algebraically exact for
object-valued maps, and the tab check uses content equality on the clone.
- The `unavailable` vs `unusable` split keys off the `cause.code` on the `agent_session_store_corrupt`
error that `loadAgentSessionStore` throws for a non-ENOENT read failure.ℹ️ Removed cross-version downgrade coverage
The two tests/e2e/cross-version-wire/agent-session-*-downgrade tests and the pr.yml line that ran one are deleted; the replacement is openAsVersion3Build inside journal-database-records-migration.test.ts, a hand-written simulation of the older build's open. That pins this build's view of a v3 build, but it is not the real released binary, so a divergence between the simulation and the actual v1.4.x open would no longer be caught. Reasonable given this build never writes the JSON file — worth a deliberate note rather than a change.
Technical details
# Affected sites
- `tests/e2e/cross-version-wire/agent-session-unproven-release-downgrade.unit.test.ts` — deleted
- `tests/e2e/cross-version-wire/agent-session-death-evidence-downgrade.unit.test.ts` — deleted
- `.github/workflows/pr.yml:865` — the runner line for the first is removed
- `src/main/native-chat/agent-session-journal/journal-database-records-migration.test.ts` — `openAsVersion3Build` simulates the v3 latch
## Open questions for the human
- Is the byte-identity assertion on the untouched `agent-sessions.json` (already covered in
`agent-session-legacy-record-import.test.ts`) considered sufficient replacement for the deleted
real-binary downgrade tests?ℹ️ Nitpicks
src/main/runtime/agent-session-store-draft.ts:59—prior === undefined && !published.has(key); theprior === undefinedterm is implied by!published.has(key).src/main/native-chat/agent-session-wire/structured-agent-session-host.ts:220—this.deps.journalDatabase.legacyRecordImportOwed === trueon a boolean field.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
| try { | ||
| admitted = await request.store.admitMutationOperation(operation) | ||
| } catch (error) { | ||
| if (plan.runsWithoutLedgerRow) { |
There was a problem hiding this comment.
runsWithoutLedgerRow is caught for every thrown admission error, not only storage damage. On a database a newer Orca wrote, admitMutationOperation throws the read-only refusal — but cancelPlan also sets conversationWrite, which bypasses the lease check in admitAgentSessionMutation, so a Stop takes this branch, re-evaluates from memory, and runs plan.run instead of returning the journalWrittenByNewerOrca refusal. The PR description lists Stop among the actions that say "update Orca" on a newer database, and the structured-agent-session-queued-newer-orca.test.ts case covers send/Send-now/Delete/Resume but not Stop. Please confirm whether Stop should bypass a read-only store; if not, gate the fallback to damage (journalCorrupt / a full-disk error) and add a test.
Technical details
# Stop on a read-only store bypasses the write refusal
## Affected sites
- `src/main/native-chat/agent-session-wire/structured-agent-session-mutation-plans.ts:182` — `runsWithoutLedgerRow: true` is set for every cancel plan, and Stop additionally sets `conversationWrite` (:180)
- `src/main/native-chat/agent-session-wire/structured-agent-session-mutation-admission.ts:129-131` — the flag is checked before the refusal branch, so the read-only error never reaches `journalOpenRefusal(error)`
- `src/main/runtime/agent-session-store-transactions.ts:108-110` — a read-only database throws `journal_read_only` from `commit` for any non-`inMemoryWhenReadOnly` transaction
- `src/shared/agent-session-mutation-envelope.ts:118-120` — a `conversationWrite` returns `admit` without consulting the lease
## Required outcome
- Either Stop intentionally reaches the agent on a read-only store (then correct the PR description and pin it with a test), or the fallback only applies when the storage is damaged/unavailable and a read-only store still answers `journalWrittenByNewerOrca` to Stop.
## Open questions for the human
- Is `runsWithoutLedgerRow` meant to be narrower than "any admission error"?Main moved the agent-session record store into the chat journal database (#24006): a transaction now hands each mutation a draft and commits only that draft. The combined admit-and-claim still mutated the store's committed state, so after the merge neither the admission nor the claim was written and two concurrent launches could both run. It now applies both steps to the draft, like every other store mutation.
…ges crossed (#24231) * test: two tests catch up with the journal-database store and the close cause #24006 moved the session store into the chat journal database, and #23467 made close take a cause; #23684's and an older tab-table test still used the old calls, so main's typecheck fails. * test: the startup-reconcile close test passes the close cause too

What this fixes (scope)
Native (structured) Claude and Codex chats only. Terminal agents, orchestration terminals, chat history rows and the profile state are not touched.
The problem: each native chat has an ownership record (who owns the chat right now, which operations it already ran, which chat tabs were open). These lived in
agent-sessions.jsonbeside the chat database. Every change to any chat re-read and re-parsed the whole file, hashed it to detect other writers (which #23613's instance locks already rule out), took a cross-process file lock, and rewrote the whole file plus a.bakcopy. That happened for a lease renewal every 10 s per live chat, for every send's ledger row, and for every tab show or hide. #23115 measured about 2.8 s of Windows startup CPU from this path on a 247-chat, 1,301-record profile. On top of that sat.bakfallback, salvage, a recovery fence latch and a stored "unusable records" list, each with its own failure modes. A store that could not be opened refused every chat until restart.What changes for the user: each chat record write drops from about 34-40 ms to about 0.3-1.3 ms (measured on Mac and Windows), so chats that restore, renew or change tabs no longer pay a whole-file rewrite each time; steady-state startup is unchanged, and the first launch after updating pays a one-time copy (about 0.8 s on Windows). A records file that cannot be read no longer stops chats from working. A database written by a newer Orca opens read-only: history reads, the app starts, and sends, Stop, Delete and Resume say "update Orca". Stop still reaches the agent when the record of the Stop cannot be written.
Not affected: the RPC wire (no parameter or frame changes), chat history storage, SSH hosts (each host migrates its own database), folder workspaces (rows are keyed by session id).
This PR is step D of: R0 #23964 → L′-a #23978 → D0 #23986 → D (this). R0 #23964 and D0 #23986 are merged; this diff against main is D only.
getVisibleSessionTabIndex,listVisibleSessionIds,getSessionTabId,getRecord,listRecords,setSessionTabVisibility) with the same meaning.Supersedes #23115.
ELI5
Orca kept a small notebook per profile listing every native chat's owner and recent actions, in its own file. Every time anything changed it re-read, re-checked and rewrote the whole notebook. This PR moves that notebook into the chat database Orca already has, as ordinary rows, and each change writes only the lines that changed.
What Changed
Before: every record change paid a whole-file read, parse, hash and rewrite; a records file the build could not open refused every chat until restart. After: each change writes only its own rows in one database transaction; nothing re-reads the records after launch; a records file that cannot be read never stops chats from working.
Mechanism:
agent-session-journal.dbgoes fromuser_version3 to 4. Version 4 addsagent_session_records,agent_session_operations,agent_session_retired_claim_keys,agent_session_tabsand one meta row (session_tabs_recorded, which keeps "a tab index was never recorded" distinct from "recorded and now empty", as fix(native-chat): preserve structured chat across rollback #17439's legacy fallback needs).agent-sessions.jsonin, and setsuser_version = 4in one transaction. "Copied" isuser_version >= 4; nothing else records it. A crash inside the migration leaves version 3 and no rows, and the next launch copies once..bakare never written, renamed or deleted.EACCES,EIO,EMFILE, on the primary or on.bak) leaves the copy owed: the tables are made, the version stays 3, the host installs, and the chat list says "can't say" rather than "none" until a later launch copies it. Chats created meanwhile keep their rows through that copy, and their tabs come back beside the profile's..bakfence raise, salvage, and set-aside handling. A readable record that also has a set-aside raw copy gets its fence floor raised above both (fix(agent-session): restore a quarantined session record this build can read #23589's case). A set-aside record with no readable copy is stored verbatim, so a build that can read it gets it back.BEGINinside a caller's open journal transaction on the same connection.hostIdcomes from the open (always'local', which is also what every records file was created with). A file whosehostIddiffers is reported and imported; owners it names resolveunresolvedagainst this host, the conservative verdict..bakcopy-before-rename, salvage and the recovery fence on the ordinary path, the persisted unreadable list,needsRewrite,agent-session-store-transaction-queue.ts,agent-session-record-store-write.ts,agent-session-record-store-security.ts,agent-session-store-serialization.ts, and the unusedsetJournalCheckpoint(the persisted field stays).Behaviour under failure:
[agent-session] Stop's ledger row skippedis logged. A replayed Stop whose ledger row was written is still answered from memory and never interrupts twice; while record writes are failing, a repeat of the same Stop (same operation id) can interrupt the turn it names a second time, which is harmless for that turn and logged. This keeps main's contract (main pinned "Stop still reaches the agent on journal damage"), per the rule that bookkeeping never gates a user action; common designs refuse the interrupt when their write fails. Other actions on proven damage answer "Unable to load this chat."; a full disk or I/O error on those still throws, as on main.agent-sessions.jsonand its.bakcan't be read, the older native chats are no longer listed, new chats work, the file is kept untouched and is not re-imported, and the failure is reported through the host's error reporting path (scopestructured-agent-session-record-import). Today that path reaches only the log in production; the queued desktop host error sink change covers it. Common designs instead keep retrying a malformed source, either silently skipping it or blocking; we don't, because an owed import keeps the chat inventory at "can't say" every launch. A transient read error (primary or.bak) leaves the import owed and retried on the next launch.tabId.Mixed versions and remote hosts: no RPC parameter or frame changes. One published refusal changes: on main a read-only record store threw
agent_session_legacy_required, which an old client's remote-launch path treats as "fall back to a terminal launch". A read-only host here answersagent_session_journal_unreadable/journalWrittenByNewerOrca, which old clients already print as "update Orca", the same as a read-only journal does on main. No released build writes a newer JSON store, so the old path was unreachable in practice.Downgrade, per build:
agent-sessions.json, writable. Chats appear as they were at import.Deleted cross-version tests:
tests/e2e/cross-version-wire/agent-session-death-evidence-downgrade.unit.test.tsandagent-session-unproven-release-downgrade.unit.test.ts, and the.github/workflows/pr.ymlline that ran the second one. Both had a v1.4.211 release checkout read a JSON store this build wrote. This build never writesagent-sessions.json, so an older build only ever reads a file it or an earlier build wrote; the import is pinned instead by byte-identity tests (the file and.bakare unchanged after import).Test cleanup fix: restart tests replaced the test host without waiting for the old host's background restart-offer withdrawal, which held the recovery capsule's lock directory while cleanup removed the test directory (
ENOTEMPTY). The harness now waits for those operations before removing it, replacing the earlierrmretries. Production quit already awaits them (its teardown drains that lane).Size
D-only, against main (merge base
ba39c6d5fd6; heuristic: a path containing test/e2e/harness/fixture counts as test):The design estimated about +560 / -340 production. The actual is larger because of:
Cuts made during review: the unused in-transaction write hook was deleted, and the provider-handle chain-validation refactor was reverted to main's version.
Why
Records are coupled to journal rows (the operation ledger to submission rows, rewind records to journal replaces, conversation commands to their rows), so one database with one version latch is the right home: an older build becomes read-only on both at once, through a latch that already shipped. Keeping the JSON file but tuning it (#23115's approach: fingerprint the input to skip the re-parse, validate rows at write) keeps the lock, the
.bak, salvage and the recovery fence, all of which exist only because the file is a second store with its own failure model. This removes that model instead of making it cheaper. #23115's row rules and draft-then-adopt model are reused.Differences from the common pattern
The common pattern: session runtime state as rows in the app's one SQLite database, written per row, with the next in-memory model adopted after commit, numbered migrations run at startup, and a failed orphan reconcile logged while startup carries on. D matches that on where records live, per-row writes validated with the row schema, draft-then-adopt, a numbered migration with the copy inside it, single-writer serialization, and reconcile failures that never stop startup.
agent-session-reservation-admission.ts).session_tabs_recordedmeta row. fix(native-chat): preserve structured chat across rollback #17439's legacy profile fallback needs "never recorded", which no row count can express..bakfallback, salvage, fence raises, and the load-time legacy terminal-handoff lease mapping and latch drop). Follow-up F3 retires the importer and moves that normalization out of the load path with it.Deviations from the design (DESIGN-v2)
+1. The design's own expected result ("the next grant is 8" for copies at fence 2 and 7) needs this, and it is the rule fix(agent-session): restore a quarantined session record this build can read #23589 shipped. The readable copy's+1(the backup-recovery rule) is kept.agentSession.closeRPC still refuses when the index write fails, because an existing test pins that on purpose (does not stop the provider when durable tab retirement fails, from feat(mobile): structured native Codex chat #18074).openis split in two: a syncopenWith(stateDirectory, legacyRecords)beside the asyncopen(stateDirectory, readLegacyRecords), so the 88 synchronous test callers of the test database registry keep working.Known costs
Linked Issue
None. Planned refactor; supersedes #23115.
Visual Proof
Live QA on a separate Mac (M4 Air), background launches with isolated profiles, of this PR at
895b256c1bcagainst main atfceca5cecec(which has #23613). Later commits are a comment, main merges and a test fix.Upgrade: chats made on main come back after the one-time copy (database version 3 → 4,
agent-sessions.jsonbyte-identical):Quit and relaunch: a tab rename and a chat made on this build persist:
Stop mid-turn, then send again:
A database written by a newer Orca: send says "update Orca", the tab still closes, and history opens read-only:
Downgrade to main's build on the upgraded profile, then back:
Testing
Measurement: main + R0 (base) vs D, 7 paired and interleaved launches per set on each machine (6 for the Mac first launch), with a seeded profile of 1,300 records, 260 operations, 180 chat tabs, a 2.1 MB records file and a 492 MB chat database. Isolated profiles, background launches, production bundles. Windows low spec was not used (out of disk).
session.tabs.listAllat launch / on demand#23115 measured about 2.8 s of Windows startup CPU on a 247-chat profile. That came from restoring many chats, each paying a whole-file re-read per transaction. This seed restores no chat panes (no repositories), so it does not reproduce that number; it measures the per-transaction cost behind it directly: about 34 ms per record write on main + R0 on Windows versus 1.2-1.3 ms here. Side observation, not caused by this PR (no tray code changes): 3 Mac first launches of this build, each right after a main-build launch had quit, hung the main process in macOS menu-bar tray drawing. It did not reproduce in 10 back-to-back launches of this build alone.
QA (screenshots above): upgrade/copy, quit/relaunch, send/Stop, a newer-Orca database and a downgrade to main's build all behave as described. Found along the way, all already on main and not changed here: a message refused with "update Orca" is sent automatically on a later launch (follow-up owned separately); Claude chats show no "Cancellation requested." note and have no Rewind control.
New and changed tests (each run red with the mechanism it covers removed, unless it is a regression pin):
journal-database-records-migration.test.ts: v3 → v4 with rows and copy; never copies twice; a crash inside the migration leaves v3 and no record tables, then copies once; an owed copy makes the tables and keeps v3; a pinned copy of the version-3 open latches a v4 database read-only; the version probe never creates the file.agent-session-legacy-record-import.test.ts: every record, operation, key and tab copied, JSON and.bakbyte-identical; copies once; unusable file reported, nothing copied, file untouched; host-id mismatch; legacy terminal-handoff lease mapped; set-aside record stored verbatim and refused; both-lists floor (fence 2 beside 7 → next grant 8); salvage from.bak; torn primary with a transient.bakread error stays owed; an unreadable primary never falls back to a valid.bak(pin); a file with no tab index leaves the index unrecorded beside a chat created while owed; tab index seeding.structured-agent-session-startup-tab-restore.test.ts: a chat opened while the copy was owed comes back, with its tab id, beside the profile's chats.structured-agent-session-record-import-install.test.ts: an unreadable file → host installs, one report, v3, inventory "can't say", next launch copies; an unusable file → host installs, one report, v4, file untouched.agent-session-store-transactions.test.ts: one renewal of three chats changes one row; a no-op writes nothing; a store call inside an open journal transaction runs after it; invalid rows reject their own transaction with memory and rows unchanged; in-place mutation throws; a write after close rejects withjournal_closed.structured-agent-session-journal-corruption.test.ts: main's whole-database damage test restored (send refused "Unable to load this chat.", Stop reaches the agent once, then rejects on the note it cannot record); record-only damage and a full disk (Stop resolvescancelled: true, interrupt sent once, skip logged); a replayed Stop answered from memory without a second interrupt.structured-agent-session-runtime.test.ts: the startup presence check on an empty database, a database with a record, an owed database with and without the records file, and an unreadable database.structured-chat-tab-table.test.ts: opening a chat from history over a newer Orca's records; a create whose index write fails still opens the chat, logs, and returns notabId.structured-agent-session-startup-reconcile-failure.test.ts: R0's startup tests now inject write failures at the row writer; a failure sink that throws does not fail the startup read.structured-agent-session-host.test.tsran 10 times with 0ENOTEMPTY(with the wait removed, 2 of 5 runs failed).Ran locally, explicit files only: the affected-test batch (175 files, 1,364 tests), the record-store and host suites (32 files, 393 tests at the head), and the cross-version wire suite (all 9 files pass; under machine load two timed out on release-checkout setup and passed alone or with a longer timeout).
tc:nodethrough the typecheck gate: clean.oxlinton every changed file andcheck:code-quality:changed: clean. Mobile tests were not run locally; they ran in CI (the Mobile Checks workflow passed atadd94320eda), as did the full desktop unit suite, the cross-version wire suite and packaging.Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Security: the database files are hardened as #23613 already does; the records file's own permission hardening is gone with its writes. Cross-platform: no path or shell changes; the
chmod 000test skips on Windows and as root, and the directory-at-path tests run everywhere. SSH: each host migrates its own database; the presence check reads the local profile's database only. Mobile: unchanged wire. Backwards compatibility: see Downgrade above. Performance: see Measurement under Testing.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)