Repository navigation
perf(native-chat): the agent-session store stops re-reading its own writes - #23115
brennanb2025 wants to merge 12 commits into
Conversation
Appending a link and validating a persisted chain now share one transition check on (head, link, seen link ids). Validation no longer copies the chain per link or scans it for duplicate ids, and checks each link's shape once.
…ransaction Every transaction re-read, re-parsed and re-validated the whole agent-sessions.json to learn whether another writer changed it, then serialized it twice more on a write. - Keep the sha256 of the exact primary bytes last loaded or written. A refresh reads the file as a buffer and hashes it; on a match it skips parse, validation and revision. On a mismatch it decodes that same buffer and runs the full load as before. No hash is kept after a load that salvaged rows from the backup, after a backup recovery, or when the primary is absent. - The save returns the text it wrote; the revision and the byte hash both come from it, so a write serializes once. - Rows a transaction changed are checked against the same rules a load applies before they are saved. A load no longer re-checks our own bytes, so an invalid row is refused at write time instead of living in memory while every other reader quarantines it. - A transaction applies to a draft that is published only after the durable rename. Rollback is discarding the draft, and a reader can no longer see a change whose save later fails. A transaction is still O(file bytes): it reads and hashes the whole file.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe agent session store now validates persisted rows through shared rules, tracks primary and backup file inputs, and reports backup salvage. Transactions operate on drafts and publish changes only after saving. Provider handle chain validation and appending now share a transition function. Tests cover store recovery, transaction publication, transaction costs, and provider handle validation work. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to If the primary has an unreadable record and the backup cannot be read temporarily, a later store write can erase the backup copy needed to recover that record. Resolve or explicitly accept this data-loss risk before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The examined paths preserve the existing session-validation and ownership checks, and publish normal transactions only after a durable write. No security regression was established. Risk is not minimal because the new draft mechanism relies on callers replacing nested values rather than changing them in place, and coverage of all possible callers is incomplete. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description is detailed and covers the required change, rationale, testing, compatibility, and checklist items. However, it states that writes are refused when the backup cannot be read, while the latest commit reverts that behavior. It also provides no linked issue despite the template requiring one unless the author is a maintainer.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
The write-time check validated the in-memory row, which can hold members JSON drops (an enumerable `undefined` option, an own `launchEnv: undefined`). Main wrote and reloaded such rows fine; this branch refused the write. Each changed row is now checked in its written JSON form, so the check refuses exactly what a load would. The salvage test also now asserts the refresh stops re-reading once the salvaged row is written.
A transient read failure on the backup while salvaging a quarantined row left the load marked exact, so later transactions skipped the reload that would retry the salvage, and the next write copied the primary over the only valid copy of that chat. The load now stays non-exact until the backup is read or known absent.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one inline consistency note and one coverage gap worth a look.
Reviewed changes
Reviewed the full 12-file diff at 1c09e60 (base main @ 4cafa50). This replaces the store's unconditional read-parse-validate-serialize on every transaction with a byte-fingerprint fast path, and moves row validity checks to write time.
- Byte-fingerprint fast path —
refreshExternallyChangedStatereads the primary once and compares its sha256 to the hash of the bytes last loaded or written; a match skips the full load. A null fingerprint (no primary, or a load that salvaged rows from the backup) forces the slow path. - Serialize once per write —
saveAgentSessionStorereturns the exact text it published, and the queue derives both the revision and the byte fingerprint from it. - Write-time row validation — changed rows are checked before save against the same predicates a load applies, refusing
agent_session_store_write_invalid(closes the latent invalid-tab-id bug that used to wedge the next load onto the backup). - Draft-and-publish — a transaction applies to a shallow draft that is published only after the durable rename; the old snapshot/restore rollback is gone and the queue's
stateis now a getter over the published state. - Linear handle-chain validation — one
agentSessionProviderHandleTransitionis shared by append and persisted-chain validation in place of the O(n²) repeated append. I traced it as behaviorally equivalent to the old append/validate pair. - Tests — exact-change (9), transaction-cost (4), provider-handle (28), plus the surrounding backup-recovery, rollback-compatibility, tab-table, legacy-handoff and record-store suites all pass locally.
ℹ️ The fast path rests on a replace-never-mutate invariant nothing states or tests
agentSessionStoreDraftChanges detects a changed row only by object identity (published.get(key) !== value), and draftAgentSessionStoreState shallow-shares the record and operation objects between the draft and the published state. Every transaction-reachable transition I traced currently returns a fresh record, so this is sound today — but the invariant matters more now than it used to: an in-place mutation would change what readers see while changed stays false, so nothing would be written, assertAgentSessionStoreDraftReadable would never inspect the row, and the fingerprint fast path would never reload. That is a silent memory/disk divergence, and the new property test reads the file back from disk so it could not catch it. A one-line comment on draftAgentSessionStoreState/changedKeys asserting "transitions replace rows, never mutate them in place", or a focused test, would keep the guarantee from regressing.
Technical details
# Change detection relies on row replacement
## Affected sites
- `src/main/runtime/agent-session-store-draft.ts:15-24` — the draft shares `records`/`operations` values by reference with `published`.
- `src/main/runtime/agent-session-store-draft.ts:44` — change is detected by `published.get(key) !== value`.
- `src/main/runtime/agent-session-store-transaction-queue.ts:110-114` — a false `changed` skips both the save and `assertAgentSessionStoreDraftReadable`.
- `src/main/runtime/agent-session-store-transaction-queue.ts:137-142` — the fingerprint fast path then skips the reload.
## Required outcome
The "each transaction-reachable transition returns a new row object" invariant is documented where change detection lives, and ideally covered, so a future in-place mutation is caught rather than silently dropped.
## Suggested approach (optional)
Comment the invariant on `draftAgentSessionStoreState` and `changedKeys`; optionally add a test that exercises a representative set of transitions and asserts each replaced its row rather than mutating it.
## Open questions for the human (optional)
Is the round-trip property test intended to cover this, or would a dedicated test be preferred?ℹ️ Nitpicks
isReadableAgentSessionStoreTab's docstring says "a table is readable when every entry is and no id repeats", but the helper only ever sees one entry — uniqueness is enforced inparsePersistedTabs. Consider moving that clause to the caller so the comment matches what the function checks.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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: e8912db6-348f-436c-8c5f-de0a55861468
📒 Files selected for processing (2)
src/main/runtime/agent-session-record-store-file.tssrc/main/runtime/agent-session-store-exact-change.test.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.
There was a problem hiding this comment.
ℹ️ No new issues in this delta. The prior review's one minor, non-blocking note (record rows are validated through duplicated logic in
parseStaterather than the sharedisReadableAgentSessionStoreRecord) remains open; nothing here changes that.
Reviewed changes
Reviewed the delta from 1c09e60 to 0a494e6 — the one commit pushed during the prior review.
salvagedFromBackup→dependsOnBackup— the load result now also reports backup dependence when the backup read throws a non-ENOENT error, soagentSessionStoreExactPrimarySha256returns null and the byte-fingerprint fast path stays off.salvageUnreadableRecordsFromBackupreturns dependence, not only salvage — a transient backup read failure is treated as "the backup may still hold the row", so the next transaction re-reads it instead of trusting the primary's hash.- New regression test — seeds an unreadable primary row whose only valid copy lives in the backup, makes the
.bakread fail once withEIO, asserts the row is absent after open, then that a following write re-reads the backup, salvages the row and persists it.
This closes a real edge in the previous revision: a single transient backup read failure latched the primary's hash, and the next save's copy step (copyFileDurable(primary, backup)) then overwrote the only valid copy of the quarantined row. The new test asserts exactly that survival, so it fails against the prior revision rather than passing vacuously. I re-ran the exact-change, transaction-cost and backup-recovery suites and they are green.
One observation, not a request: a backup that reads successfully but fails parseState (corrupt or older-schema) still reports dependsOnBackup: false. The new comment "or it could not be read" reads as if it covers that case too. It is defensible — such a backup is proven unusable, and overwriting it loses nothing recoverable — but if the intent is that any backup the load could not use keeps the slow path on, that branch would return true.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
A load whose salvage reads the backup now records the backup's hash beside the primary's, so the skip holds only while both files still hold those bytes. A backup that gains a valid copy of a quarantined row is salvaged on the next transaction, as a full re-read would, instead of being overwritten by the next rotation. The load and the write check share one record rule.
…rotated, and the shared record rule at load
… a quarantined row A write used to copy the primary over the backup even when the backup held the only readable copy of a row the primary quarantines. Two cases lost that row: the backup could not be read, so its copy was never salvaged, or the write that would persist a salvaged row died after the rotation and before the primary landed. Now a write that would rotate an unreadable backup is refused and retried on the next transaction, which reads the backup again, and a store holding salvaged rows rotates in the state that holds them instead of the primary's bytes. A deferred rewrite at open tolerates the refusal.
…able backup still opens
…ead, as before This reverts commits b5e51ae and 68c354e. Refusing the write whenever the backup could not be read turned a backup read error into a failed send, reservation, rename or lease renewal, for as long as the error lasted, on any profile holding a quarantined row. Keeping the backup instead would let the primary move more than one generation past it, which the backup-recovery fence cannot cover. Both fixes address a gap that exists on main and belong in their own change.
…r the anti-slop lint
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the delta from 0a494e6 to b5e51ae (4 commits, 8 files) on top of the full PR. The prior pullfrog inline thread is addressed and resolved.
- The parse skip is keyed on every file the load read —
LoadedAgentSessionStorenow reports aninputKeyof the primary plus the backup ('unread'/'absent'/ sha256), andrefreshExternallyChangedStatere-reads both before skipping, so a backup that gains a quarantined row can no longer hide behind an unchanged primary. - A write refuses to rotate an unreadable backup — with a quarantined row outstanding and the backup unreadable, a writing transaction aborts with
agent_session_store_backup_unreadableinstead of copying the primary over the only readable copy; the open tolerates the refusal and defers the rewrite. - A salvaged write rotates the repaired state into the backup —
saveAgentSessionStoretakesbackupText, so a write holding salvaged rows backs upserialize(published)rather than the primary bytes that lack them; the post-write key matches whichever bytes were actually rotated in. - The record rule is shared —
parseStateaccepts record rows throughisReadableAgentSessionStoreRecord, keepingisPersistedAgentSessionRecordonly to choose the quarantine reason. - Tests — new cases cover a backup that gains the row while the primary is byte-identical, a write keyed on the backup it rotated, an unreadable backup holding the only copy, a refused open rewrite, and a crash after backup rotation; all pass.
Each new test fails against the pre-fix revision, and a data-loss-focused audit of the rotation/fingerprint path found no reachable sequence that destroys the only readable copy of a quarantined row.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the delta from b5e51aeff to e595c04045 (2 commits, 8 files).
- Reverted the backup-unreadable write refusal —
c7b027fb0ddropssaveAgentSessionStore'sbackupTextoption, thesalvagelatch, theagent_session_store_backup_unreadableerror and the open-time tolerance, so a validated write again rotates the backup unconditionally, as onmain; the three tests covering those paths are removed with it. - Kept the input key keyed on the rotated backup — a write still records
readPrimarySha256(the primary bytescopyFileDurablerotates in) while a load would read the backup, and'unread'otherwise, so a backup restored afterwards is still detected. That path is unchanged and stays exact. - Lint —
e595c04045marks the two test ProxyReflect.getforwards withanti-slop/no-reflect-getdisables matching the repo convention.
The revert widens main's existing crash window (a write can copy the primary over a backup holding the only readable copy of a quarantined row), but that matches pre-PR behavior and the commit subject states it plainly, so it reads as a deliberate scope decision rather than a regression this PR introduces.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Review status: ready for review at e595c04Result: all CI checks pass at e595c04. Five review rounds and an architecture review are done. The last round found nothing to fix, and QA on a real restart passed. The fix stays as designed: skip re-reading the store file when its bytes are unchanged, check each changed row when it is written, and show a change to readers only after it is saved. There is no file-format change and no UI change. Fixed during review:
Checked on every round:
Live QA: on a Mac, restarting with 247 restored native chats (1,301 records) restored all 247 tabs on every launch, each with its history. Closing 3 chats and reopening 1 from Session History survived a restart exactly as expected. The saved file matched main's, and no store errors were logged. Screenshots are in the PR body under Visual Proof. Follow-ups, not in this PR:
|
Ready to mergeChecked at head
|
2 participants
ELI5
Orca keeps one file that records every native chat and who owns it. Before this change, every small edit to that file first re-read, re-checked and re-encoded the whole thing, just to see whether another Orca process had changed it. With about 1,300 chats that is slow, and startup does it once per restored chat. Now Orca remembers a fingerprint of the exact bytes it last read or wrote, and it skips the re-read when the bytes on disk still match.
What Changed
Before: every agent-session store transaction called
refreshExternallyChangedStateunconditionally. That meant a full read and JSON parse ofagent-sessions.json, validating every record (including a provider-handle chain replay that grew with the square of the chain length), then stringifying and hashing the whole state. Each write stringified the whole store twice more. Restoring 247 chats from a 1,301-record store ran about 276 such transactions at startup, which measured about 3 s of main-process CPU on a 24-core Windows machine. Every attach runs about five, and so do reveals and mobile re-holds.After:
orcador an older build are still adopted exactly as before. When a load also had to read the backup file (to recover a row the main file marks unreadable), the backup's hash is part of the fingerprint too, so a change to either file triggers the full path. If the backup could not be read at all, no fingerprint is kept.:used to be written, after which every later load fell back to the backup. It is now refused withagent_session_store_write_invalid.Cost after the change: a transaction that changes nothing is O(file size) for reading and hashing plus some map copies. It no longer parses, validates or stringifies. While the main file holds a row marked unreadable that it has no valid copy of, the backup file is read and hashed too, which main already did on every transaction. A writing transaction serializes once. It is still not constant-time, and the commit messages say so.
Why
The expensive check answered a real question: did another process change the file? Other writers exist:
orcad, a second dev instance on the same profile, an older build during an upgrade. So the check can't just be dropped. Comparing the exact bytes answers the same question cheaply, and it stays content-exact across processes with no file-format change.Alternatives considered:
Linked Issue
No ticket. This was found by profiling native-chat startup with about 250 restored chats. It is one of two costs that grow with the number of chats; the other, re-encoding the status snapshot on every update, was already fixed on main by #22536.
Visual Proof
There is no UI change; the CPU numbers are under Testing. These hidden-window captures come from a restart check on a Mac with 247 restored native chats. They show nothing changes for the user.
Restored chat tabs after a restart (every launch showed 247 of 247, with history):
Close and reopen across a restart: 7 tabs, then 3 closed, then 1 reopened from Session History, then the result after a restart. The two closed chats stay closed, and the reopened chat keeps its history.
Testing
New tests, each paired with a deletion ablation that turns it red:
revision(load(serialize(s))) === revision(s);Unchanged and green: the rollback-compatibility, backup-recovery-fence and lease-renewer tests, plus the cross-version wire tests.
Measurement: same Windows machine (24 logical CPUs) and seed profile as the startup measurement: 247 restored chats, 1,301 records. Two runs per build, each confirmed to restore all 247 chats. Main is pinned at this PR's merge base
80ff6c4e3e.I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
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
orcadread and write it as before, and their writes are detected by bytes.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)