Skip to content

fix(native-chat): a failed startup chat-lease save no longer puts the app into "Session restore failed" - #23964

Merged
brennanb2025 merged 8 commits into
mainfrom
brennanb2025/records-readonly-startup
Sep 30, 2026
Merged

brennanb2025 merged 8 commits into
mainfrom
brennanb2025/records-readonly-startup

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 7 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​798 $\color{#cf222e}{\Huge{\mathbf{−}}}$​13 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​785
Prod 11 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​225 $\color{#cf222e}{\Huge{\mathbf{−}}}$​40 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​185

ELI5

If Orca could not save its chat bookkeeping file while starting up, the whole app went into "Session restore failed. Changes won't be saved until restart", even though only chat bookkeeping had failed. For example, the file was locked, could not be written, or was written by a newer Orca. Now the failure is logged, the app starts normally, and the chat tabs you had open come back with their history. Each chat redoes the bookkeeping the next time you send to it.

What Changed

The problem. Orca keeps a record file, agent-sessions.json, that says which process may write to each chat (its lease) and which chat tabs are open. Startup reads chats in two steps, and several bookkeeping writes to that file sat inside them. When one of those writes failed, the error reached the renderer's startup catch. That catch runs recoverFromDegradedStartup, which shows the sticky "Session restore failed. Changes won't be saved until restart" toast and saves no tabs, layout or workspace state for the rest of the session.

  • Step 1, always: app:prepareTerminalStartupRestoration re-checks every saved chat's lease and writes the result (reconcileRestartLeases). A failed write rejected the step.
  • Step 2, with native chat on: the renderer also awaits the chat tab restore (session.tabs.listAll). Before reading each open chat, it ran four more writes, and each one rethrew a failure:
    • the lease check again;
    • resolving a chat that was left mid-handoff after a crash ("recovery");
    • recording each restored tab as visible;
    • recording the tab of every chat you had cleared with /clear.

On main, these can make those writes fail:

  • the file lock gives up after its retries, about 3.1 seconds, because another process holds it;
  • EACCES or EIO when saving;
  • the file disappears between load and save;
  • on Windows, a rename held open by another process for too long;
  • another Orca process keeps rewriting the file, for example the outgoing one during an update restart, so the check gives up after 8 passes with execution_owner_reconciling (how often this happens has not been measured);
  • the file was written by a newer Orca. The store then opens read-only, and every write is refused with agent_session_legacy_required, even a write that changes nothing.

No user report ties the banner to this path yet; this was found by reading the code. The newer-Orca case is not reachable from any released build today, because no released build has written a record file newer than schema 2. It becomes reachable once a later change moves these records into the chat database, where a downgrade after that change would open read-only.

Before: any of the failures above put the whole app into "Session restore failed", and nothing you changed in that session was saved until you restarted.

After:

  • Startup finishes normally, including with native chat on, chat tabs open, cleared chats and chats left mid-handoff.
  • The chat tabs that were open at quit are restored and their history reads.
  • With a saved tab list, the startup restore writes no tab records to the record file. Lease checks, recovery and rewind settlement can still write, and each of those is best-effort.
  • Each distinct failure is logged once ([structured-agent-session] reconciling chat leases failed), or passed to the host's error sink when one is installed.
  • While another process holds the lock, startup costs about one lock wait (~3 s) per startup step, two in total, however many chats are open. A profile saved before tab lists existed waits once more, for the one write that records its tabs. Each chat whose Claude rewind was interrupted also adds one wait, because the restore settles that rewind with a best-effort write. That case is rare: it needs a crash in the middle of a rewind.
  • Until a check succeeds, no chat has a writer in this process, so a chat reads as not running.
  • A turn the previous run left running shows as unverifiable, not as ended, because nothing proves that process is gone. Once a later check proves it gone, the turn is revised to ended. As on any restart, running tool calls are marked failed and pending approvals are cancelled.
  • A send re-runs the check first. While the file still cannot be written, that send fails with the store's error. When another process holds the lock, that happens after about one lock wait (~3 s), the same as on main. Only that chat is affected.

Still not fixed (separate, deferred): a failure when the record file is first opened at startup still puts the app into "Session restore failed". That covers:

  • a lock still held right after a crash or an update restart;
  • EACCES/EIO while opening the file;
  • a file that cannot be parsed;
  • the one-time rewrite of an older file format at open.

Opening happens before the lease check, in the host install, and failing startup on an install error is deliberate existing behavior that a test pins. Changing it needs its own decision.

The mechanism. The rule: on the two startup read steps, no write to the record file can fail the read. Writes the user asks for keep failing loudly: creating, revealing, attaching to or sending to a chat, stopping it, and committing a /clear.

  • One place for the reader's lease bookkeeping. createStructuredAgentSessionHostRestore builds both bookkeeping calls, and both the startup check and the chat restore use them.
    • createReaderReconcile never throws and answers whether every lease is settled.
    • The restore's recovery resolution is caught the same way. It is reported through onLeaseReconcileFailure, and the next attach or send resolves it again, strictly.
    • reportEachFailureOnce logs each distinct failure once until a check succeeds, so a failing store does not log on every chat list.
    • Attach and agent start keep the strict check, which refuses or throws.
  • One bounded wait per restore. Each restore starts from one lease check for the whole pass. Each chat re-checks and resolves its recovery only while the pass is still settled. The first refusal or failed write stops bookkeeping for the rest of that pass, is reported once, and every chat is still opened. That stops a held lock from costing one wait per chat.
  • Tabs are projected, not written.
    • publishStructuredAgentSessionTab (the strict write, used by create and reveal) now calls a new projectStructuredAgentSessionTab, which only updates the runtime's in-memory tab snapshot.
    • The restore only projects. A saved tab list already lists every restored chat.
    • A /clear replacement only projects too, both at startup and in the live /clear call. The /clear commit already moved the tab in the same write, so the old write recorded nothing new. In the live call it could also fail after the clear had committed, which skipped closing the old chat.
    • visibilityWriteMayFail and its per-tab warning from the previous commit are gone.
  • The legacy seed is one write. A profile saved before tab lists existed restores its chats from the profile's own tabs. The chats the restore opened that the list does not hold are recorded in one best-effort transaction, store.showSessionTabs. It is never left half-written, which the next launch would read as complete and use to drop the other chats. If it fails, the list stays absent, and the next launch seeds it again unless another tab write lands first in the same session. That write can be opening, closing or clearing a chat, and it creates the list with only its own tab. The chats that were never seeded then do not reopen on the next launch, though History can still reopen them. This is a known, rare limitation that main shares: it needs a legacy profile, a failed seed and a later tab write in the same session.

Why

A lease check, a recovery resolution and a tab-visibility record are bookkeeping. They must never decide whether the app can save your tabs and layout, or whether you can read a chat. Reading grants no writer, so a read has nothing to wait for.

Alternatives considered:

  • Keep adding a best-effort flag at each write site. That was the previous commit's approach. It missed two sites, and nothing would stop a new one from appearing. Deriving the restored tabs from state already loaded removes the writes instead of guarding them.
  • Skip every write that would change nothing, at the store. That is unsafe store-wide: the store decides under the lock, after re-reading the file, and several writes are compare-and-swap. It would also not cover the lease check or recovery, which always change the file.
  • Wrap the store the restore uses so every write is swallowed. The bookkeeping would then carry on as if a failed write had succeeded.
  • Make only the renderer tolerate a failed chat restore. Desktop would show no chat tabs, and mobile or web clients listing tabs on this host would still fail.
  • Catch the error in the runtime's startup prepare step. That step also runs the host install, and an install failure that is not a known refusal should still fail there. It would also leave the chat tab restore failing.
  • Replace the per-chat lease check with a single startup reconcile phase. Deferred to the follow-up after refactor(native-chat): keep agent-session records in the chat journal database #24006 (see the Temporary item below), to keep this PR a focused fix. Until then, each restore pass checks the leases once at its start, checks again before each chat's recovery only while every check so far has held, and stops checking and resolving recovery at the first failure, so a store that keeps failing is not retried chat by chat.
  • Reuse the startup step's lease check in the restore. That would save one wait, but it is state that outlives a call, and a lock that cleared in between would not be retried.
  • Retry on a timer. Not needed: every attach and send re-runs the check before acting, and until then the lease lets nothing write.

How this compares with other agent servers. What matches: a failed startup bookkeeping phase is logged and startup continues, and listing and reading chats never fail because of that bookkeeping. The differences:

  • Temporary: before resolving each restored chat's recovery, the read restore checks that chat's lease again, instead of settling every lease once in a single startup phase. This behaviour predates this PR. The per-chat check guards against another process rewriting agent-sessions.json in the middle of a restore: resolving recovery can stop a process, so it must act on a current answer. That case no longer happens. Packaged Orca and the background daemon (orcad) each refuse a second process on the same profile or data folder, and since feat(native-chat): one structured-chat journal database per host, owned by one process #23613 dev builds do too. refactor(native-chat): keep agent-session records in the chat journal database #24006 (still open) then moves these records into the chat database, which one process owns, and stops the record store from using the file lock. The common pattern elsewhere settles ownership once, in one startup phase run by the single server process. One design holds incoming commands behind a readiness gate until that phase finishes. Another runs one recovery sweep at startup that logs its failures, plus periodic sweeps. Follow-up, after refactor(native-chat): keep agent-session records in the chat journal database #24006 lands: replace the per-chat check with one startup reconcile phase, and keep the per-chat check that already runs before a chat's agent starts (a send, for example, first checks that chat's lease, is refused if the check cannot settle, and resolves that chat's recovery) as the per-chat form of that readiness gate. No ticket is filed yet. This PR keeps the existing behaviour on purpose, to stay a focused fix. Until then, on a healthy startup the check needs no lock when nothing changed, and after a failure it is skipped for the rest of that restore.
  • Intended: no periodic retry. Every writer re-runs the check before it acts.
  • Out of scope, not yet tracked: one chat whose history cannot be opened still fails the whole restore, where the common design isolates each chat so one failure does not stop the others. That is a read failure, not bookkeeping. No ticket is filed yet.

Linked Issue

None. Found by code reading while planning the move of chat records into the chat database. This is the first, independent step of that plan: the later change makes a read-only record store reachable after a downgrade, and it relies on this step so that such a store never degrades startup.

Visual Proof

Live QA ran on a separate Mac (M4Air). Each launch used its own isolated profile whose agent-sessions.json had been written by a newer Orca (schema 3), so the store opens read-only and every write to it is refused.

Native chat on, with a chat tab open at quit.

Before, on main (7980ab9): the "Session restore failed" toast appears and the chat tab is missing. Only Terminal 1 comes back.

Before, main 7980ab9: the Session restore failed toast is shown and the chat tab is missing

After, on f3ec1a1 (behaviour identical to the current head; later commits change comments only), same profile plus a seeded chat history: no toast, and the Claude Chat tab is restored with its history.

After, f3ec1a1: no toast, and the Claude Chat tab is restored with its seeded question and answer

After, on f3ec1a1: a Terminal 2 tab opened during that session is back after quit and relaunch, so saving still works. The record file was byte-identical after both launches.

After, f3ec1a1: Terminal 1, Claude Chat and Terminal 2 all restored after quit and relaunch, no toast

Native chat off (record file only), empty profile.

Before, on main (7980ab9): the toast appears.

Before, main 7980ab9: the Session restore failed toast on an empty profile with native chat off

After, on 98a2e27 (the first commit of this PR; this off-mode pair was not re-run on later commits, but the f3ec1a1 run above goes through the same startup step and also shows no toast): no toast.

After, 98a2e27: the same empty profile with no toast

Testing

  • I manually tested these changes locally (live QA ran on a separate Mac, the M4Air, against seeded profiles; screenshots above)
  • Automated tests added/updated, or explained why not below

New tests:

  • structured-agent-session-startup-tab-restore.test.ts (runtime). These run over real agent-sessions.json files and real chat histories, through restoreStructuredAgentSessionTabs, the restore behind session.tabs.listAll:
    • Over a store a newer Orca wrote, and over a lock that keeps failing after the store opens: two open chats A and B, and a chat S cleared into A. The restore resolves and publishes A (marked as replacing S) and B, and both histories read. The failure is reported once. No tab write is attempted, and the newer store's file and directory are byte-identical afterwards.
    • With the lock held from just after the store opens and 1, 4 or 8 open chats, the startup step makes exactly 1 lock attempt and the restore exactly 1, counted rather than timed. On a legacy profile, the restore makes 2: its lease check and the one seed write. Against the previous commit's code the same tests count 2, 8 and 16 restore attempts, and 4 on the legacy profile.
    • On a legacy profile with no tab list, the restored chats are recorded in one write. When the lock allows only one more take, the list on disk holds both chats. When that write fails, the chats still publish and read, one warning is logged, and the list stays absent.
  • structured-agent-session-startup-reconcile-failure.test.ts (host), six tests:
    • the startup check under a failing lock, and the send that later reconciles;
    • a newer store left byte-identical;
    • the read restore under a failing lock, reported once across startup and the restore;
    • the read restore over a newer store;
    • new: a chat left mid-handoff whose recovery write fails. The restore resolves, the chat reads, the handoff stays pending for the next send, it is reported once, and no process is stopped.
  • structured-agent-session-restart-restore.test.ts, two new unit tests over 8 chats. When the pass-level lease check fails, no chat checks again or resolves recovery, and all 8 are opened. When recovery writes fail, no more start after the first failure: at most the 4 already running.
  • orca-runtime-structured-session-restore.test.ts, new test: replacing a cleared chat's tab projects it and never calls setSessionTabVisibility.
  • structured-agent-session-startup-reconcile.test.ts (runtime), unchanged in substance: startup prepare over a newer store resolves and reports; with no sink it logs.

Ablations, each run against those files, with the red logs kept:

  • /clear replacement back through the strict write: red in both restore variants and in the replace test.
  • Restored tabs back through the strict write: 5 red.
  • Restored tabs through a best-effort write that is then ignored: red on "no tab write attempted" in both variants, and the counted test sees 10 refused takes instead of 2.
  • Restore using the strict lease check: 5 red.
  • Recovery resolution not caught: the mid-handoff test is red.
  • Legacy seed written chat by chat: red, because the list on disk holds only A.
  • Restore without its pass-level check: the unit test sees 4 lease checks after the pass check failed, instead of 1.
  • No stop after the first bookkeeping failure: the unit test sees 8 recovery writes instead of at most 4.

After merging main, on head 4e5c839, a run of 52 named test files passes, 369 tests: the new tests above plus the existing suites. The existing suites cover the host, the restart check and restore, crash and stale-turn recovery, the conversation commands, the tab table, the session-tabs RPCs, the runtime restore, host refusal, and the tests/e2e unit suites for reopened tabs, owner status and cross-version wire. pnpm tc:node passes.

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Cross-platform: no platform-specific code. The Windows held-rename case is one of the causes this now contains.
  • SSH and remote: the check runs on the host that owns the record store. A failure there is now reported on that host instead of failing its startup call or its chat list. A chat is shown as unverifiable, never as ended, while the check has not settled.
  • Mixed client and host versions: no wire change: no request or response field or refusal code changes. One published-content change: a profile saved before tab lists existed, with a past /clear, now publishes the cleared chat's tab with replacesSessionId. Newer profiles already publish that on main, so clients already handle it.
  • Folder workspaces: unaffected. The lease check does not depend on the workspace kind.
  • Startup time while another live process holds the lock: about one lock wait (~3 s) per startup step. That is two in total (about 6.2 seconds) however many chats are open, plus one on a legacy profile, plus one per chat with an interrupted Claude rewind (rare: it needs a crash mid-rewind). The per-wait figure is derived from the retry settings (20 retries, 2.9 to 3.3 seconds) and matched a measured run. If the lock is lost in the gap between the startup check and the restore while chats are mid-handoff, the recoveries already running each wait once. That is at most four, because the restore opens four chats at a time, and at most five on a legacy profile, counting its seed write.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

…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.
…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.
brennanb2025 added a commit that referenced this pull request Sep 30, 2026
…e per-write gate

This PR had stopped startup waiting for the lease check (the pass that
settles each chat's previous-run owner) and instead made every write to
an unchecked chat wait for it. After #23964 that check can no longer fail
startup, and a review run measured it at about 30 ms on a 247-chat Mac profile, so the
gate bought nothing but a second mechanism. It also reordered two writes
that arrived while a chat's lease flipped from unchecked to checked.

Startup's structured step, the write path and worker release are back to
the base's shape: startup awaits the non-throwing check, writes enter the
chat's queue in arrival order, and an agent start still runs its own
strict check before it acts. What stays is the listing from records and
the tab table, and the chat history restore that starts after the list
has answered.
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.
@brennanb2025

brennanb2025 commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

Review status: ready for review (head 871f64b, updated after round 5).

The problem. At startup Orca checks the chat records file (agent-sessions.json) and writes back which chats are still owned by a running process. If that write failed, the whole app startup was treated as broken. Causes include another process holding the file lock, a permission error, or a records file saved by a newer Orca, which this version opens read-only. The user saw the sticky "Session restore failed. Changes won't be saved until restart" toast, open chat tabs didn't come back, and nothing else (tabs, layout) was saved for the rest of the session.

What changes for the user. When that bookkeeping write fails, Orca starts normally. There is no toast, open chat tabs come back with their history, and other changes save as usual. The failure is logged, and the records file is left untouched. Actions the user takes (sending, attaching, /clear) still report a failed write, because those can't succeed without it. With a lock held by another process, startup waits about one lock timeout (~3 s) per startup step no matter how many chats are open, where earlier drafts waited once per chat.

Fixed during review (4 rounds plus an architecture challenge; details in the PR body):

  • With native chat on and a chat tab open, the tab-restore step re-ran the failing check and still showed the toast. Now it reads and restores the chats and never lets a bookkeeping write block that.
  • Tab-visibility and /clear-replacement writes on the restore path could still fail startup. The /clear write never changed anything and is gone. The restore now writes only a one-time tab list for older profiles that lack one.
  • Recovering a chat whose process was still running after a crash could throw out of the restore. It is now reported and skipped.
  • With a held lock, startup time grew with the number of open chats (about 9 s for 1 chat, 47 s for 8). It is now bounded at about two lock waits in total.
  • PR-description accuracy: lock-wait counts, "send fails after one lock wait, as on main", and the legacy tab-list limitation.
  • Merged current main. Main moved where a test harness keeps its store, so one test's store path was updated.
  • Round 5 (review-bot finding): if the error reporter itself threw, the failure escaped and turned the recovered startup failure back into a failed startup. That can't happen today, because the only reporter is a console warning, but a planned desktop error reporter would make it reachable. The reporter call is now wrapped and falls back to a console warning. A new test covers both the startup check and the chat restore, and it fails without the fix.
  • PR description: the per-chat lease re-check during restore is now labelled Temporary. The follow-up, after refactor(native-chat): keep agent-session records in the chat journal database #24006 lands, replaces it with one startup check phase, keeping the per-chat check that runs before a chat's agent starts.

Deferred (not in this PR):

  • A records file that can't be opened at all (lock or permission failure on the very first open, or a corrupt primary and backup) still shows the toast. That is the install-refusal path, and changing it reverses an existing pinned test, so it is a separate decision.
  • Sending in a restored chat on a read-only records file shows the generic "Your message was not sent. Retry" without saying to update Orca. That state is unreachable in released builds today.
  • If one chat's history can't be opened, the rest of that restore pass is rejected. This is a read failure, not bookkeeping, and it behaves the same as on main.
  • On an older profile with no saved tab list, if the one-time tab-list write fails and a later tab write in the same session succeeds, a chat can drop out of the tab list on the next launch. It can still be reopened from History. Main has the same gap.
  • Stopping a leftover live chat process at startup can take about 6.5 s per chat, as on main.
  • Repeated warning lines when chat recovery keeps failing: up to 4 per restore pass (4 chats are restored at a time), then one per later pass. They never grow with the number of chats; this is log noise only.
  • Nits: a comment in structured-agent-session-tab-index-seed.ts describes the restored set too narrowly; two runtime tests build the store path by hand; failure warnings are deduplicated only per message.

Verified:

  • Live QA on a separate Mac (the M4Air, not the dev machine). Each launch used an isolated profile with a schema-3 records file that opens read-only.
  • On main: the toast shows and the chat tab is missing.
  • On this branch (f3ec1a1; later commits change comments and one test path only): no toast, the chat tab is restored with its history, one warning is logged, the records file stays byte-identical across launch and relaunch, and a new terminal tab survives quit and relaunch. Screenshots are in the PR body.
  • pnpm tc:node passes. 52 named test files (371 tests) pass after round 5. Removing either fix makes its new tests fail.
  • The readiness checklist passed with no P0–P2 findings.
  • CI on 871f64b (after merging current main): 21 checks pass, including all 5 test shards and both package jobs. 14 are skipped by path filters, and none failed. An earlier red on 1df09d5 came from a lint error on main (fix(tab-bar): keep the active tab visible when the tab strip scrolls #24010, fixed by test(e2e): stop main's static analysis failing on the tab strip test #24052), not from this PR.

Not verified:

  • Live QA was not re-run after f3ec1a1. The later commits are comments, test paths, main merges, and the reporter wrap from round 5, which only matters when a reporter throws.
  • Not tested on Windows, Linux or SSH hosts. The change is host-agnostic main-process logic with no change to what clients and hosts send each other.
  • Full pnpm lint and pnpm build were not run locally; CI's static analysis and package jobs passed.

@brennanb2025
brennanb2025 marked this pull request as ready for review September 30, 2026 07:00
@greptile-apps

greptile-apps Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Fixes startup error handling for chat session recovery.

The PR appears safe to merge; no outstanding blocking finding was identified.

Summary

The PR makes startup chat-lease reconciliation and read restoration tolerate bookkeeping-write failures without putting the app into degraded session restore mode.

  • Restored chat tabs are projected from opened sessions rather than written individually.
  • Older profiles seed their chat-tab index in one best-effort transaction.
  • Tests cover read-only stores, failed locks, recovery, and restored history.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Startup lease check] --> B{Bookkeeping succeeds?}
  B -->|Yes| C[Restore readable chats]
  B -->|No| D[Report failure]
  D --> C
  C --> E[Project restored tabs]
  E --> F{Older profile needs tab index?}
  F -->|Yes| G[Best-effort single seed]
  F -->|No| H[Finish startup]
  G --> H
Loading

Reviews (3) · Last reviewed commit: "Merge origin/main into brennanb2025/reco..."

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 16 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: b622cb06-e22e-42e7-a9c5-71e8cbf9862f

📥 Commits

Reviewing files that changed from the base of the PR and between 1df09d5 and 871f64b.

📒 Files selected for processing (18)
  • src/main/native-chat/agent-session-wire/structured-agent-session-host-types.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-host.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-readable-restorer.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-restart-reconcile.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-restart-restore.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-restore-without-import.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-reveal.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/orca-runtime-structured-session-restore.test.ts
  • src/main/runtime/rpc/methods/structured-agent-session.ts
  • src/main/runtime/structured-agent-session-runtime.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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 7bc82cae-fe70-43af-829c-0d7b0ef83a23

📥 Commits

Reviewing files that changed from the base of the PR and between 4e5c839 and 1df09d5.

📒 Files selected for processing (2)
  • src/main/native-chat/agent-session-wire/structured-agent-session-restart-reconcile.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-startup-reconcile-failure.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

Lease reconciliation failures are now reported while startup and readable-session restoration continue. Restart restoration skips later lease-settlement attempts after a failure but continues restoring session journals. Startup tab restoration selects saved or profile session IDs, seeds missing tab-index entries, and projects restored and replacement tabs.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 1df09

Startup can continue restoring readable chats when record-store bookkeeping fails. No concrete merge-blocking risk remains.

Architecture Summary

Architecture risk: 🔵 Low · up to 1df09

The change affects 1 system.

Changed systems: src

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — src (service) was modified; 18 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in src/main/native-chat/agent-session-wire/structured-agent-session-host-types.ts: StructuredAgentSessionHostDeps adds an optional callback receiving lease-reconciliation failures. Its comment describes reporting each distinct failure once; startup and reads continue, while the next attach or send retries reconciliation and recovery resolution.
  • observed — Modified behavior in src/main/native-chat/agent-session-wire/structured-agent-session-host.ts: The restore collaborator’s reconciliation callback is now passed as reconcileLeases instead of reconcile.
  • observed — Modified behavior in src/main/native-chat/agent-session-wire/structured-agent-session-host.ts: Adds showSessionTabs, forwarding the session ID list to the store’s showSessionTabs.
  • observed — Modified behavior in src/main/native-chat/agent-session-wire/structured-agent-session-host.ts: reconcileRestartLeases now delegates to restore.reconcileRestartLeases(); it no longer directly reconciles startup leases or throws when reconciliation returns a refusal.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 18 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description is detailed and covers the user impact, implementation, rationale, testing, visual proof, scope, and known limitations. However, it does not provide the required linked issue, and the … Add an actual issue reference after “Fixes #” or document the applicable maintainer exception. Confirm the required validation commands, or state that CI will cover the commands that were not run locally.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the primary change: startup continues instead of entering “Session restore failed” when chat-lease saving fails.
Full details: Description check

Explanation

The description is detailed and covers the user impact, implementation, rationale, testing, visual proof, scope, and known limitations. However, it does not provide the required linked issue, and the full lint, typecheck, test, and build checklist is not confirmed.

✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Reader lease bookkeeping can no longer fail a read. createReaderReconcile never throws and reconcileRestartLeases now delegates to it, reporting each distinct failure once (reportEachFailureOnce, keyed by error code/message) through onLeaseReconcileFailure or console.warn.
  • One bounded reconcile per restore pass. The restore now runs one pass-level lease check and re-checks each chat only while the pass is settled; the first refused/failed bookkeeping write stops bookkeeping for the remainder of the pass while every chat is still opened and read (structured-agent-session-restart-restore.ts).
  • Restored tabs are projected, not written. replaceStructuredAgentSessionTab is now synchronous and project-only (the /clear commit already moved the tab), and the runtime startup projects restored chats then seeds the tab index once, only when it is absent (seedStructuredAgentSessionTabIndex, showSessionTabs).
  • Tests. New host and runtime suites pin the reported-once behavior, the byte-identical read-only store, the per-startup-step lock-wait bound for 1/4/8 chats, and the legacy one-shot seed. Verified locally: pnpm tc:node passes and the four new suites pass (22 tests).

ℹ️ The legacy-seed limitation is slightly broader than the description states

The PR body says the deferred legacy limitation needs "a legacy profile, a failed seed and a later tab write in the same session," and that a later write "creates the list with only its own tab." A close is a counterexample: setSessionTabVisibility(..., false) on a store whose index was absent creates a non-null empty tab table, which serializes as sessionTabs: [] / visibleSessionIds: []. The next launch then sees present: true with sessionIds: [], sets targets = [], and restores none of the never-seeded chats — not just the closed one. The body also says main shares this limitation, but this one-shot seed is new here: on main a failed restored-tab write rejected startup outright.

Technical details
# Legacy seed failure followed by a close drops every unseeded chat

## Affected sites
- `src/main/runtime/agent-session-tab-table.ts:117-132` — a `hide` on a null table creates an empty, non-null `sessionTabs`.
- `src/main/runtime/agent-session-store-serialization.ts:17-19` — a non-null table serializes `sessionTabs: []` and `visibleSessionIds: []`.
- `src/main/runtime/agent-session-record-store.ts:132-135` + `orca-runtime-restore-structured-agent-session-tabs-once.ts:63` — next launch reports `present: true, sessionIds: []`, so `targets = []`.
- `src/main/runtime/structured-agent-session-tab-index-seed.ts:21-33` — the seed can only fire when the index is absent, which it no longer is.

## Required outcome
Only that the PR description be corrected (or a tracking note filed) so the acknowledged limitation is stated accurately: the triggering write may be a close, which drops all unseeded chats, and this behavior is new rather than shared with main.

## Suggested approach (optional)
No code change required for this PR; the seed design is deliberate and the fix is already deferred.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

…up chat read

The lease bookkeeping failure reporter called the host's failure sink
directly, so a sink that threw turned a reported, recoverable store failure
back into a rejected startup reconcile or read restore. The reporter now
catches a sink throw and logs both the original failure and the sink error
with console.warn.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • A throwing failure sink can no longer fail the startup read. reportEachFailureOnce now invokes deps.onLeaseReconcileFailure inside a try/catch; a throw falls back to console.warn carrying both the store failure and the sink error, so reconcileRestartLeases and restoreReadableSessions still resolve (structured-agent-session-restart-reconcile.ts:65-73).
  • Two host cases pin the invariant. A throwing sink over a held store lock is driven through both entrypoints, asserting each call resolves, the sink is called once, and the fallback warning is emitted (structured-agent-session-startup-reconcile-failure.test.ts:198-220).

This closes the earlier P1 finding that a throwing error callback could escape the reader reconcile and re-trigger the "Session restore failed" path. onLeaseReconcileFailure is only ever called through reportEachFailureOnce (verified by grep), so the guard covers every path, including the recovery-resolution rejection handler in structured-agent-session-reveal.ts.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Brings in main at 707d3dc, including #24052, which fixes main's
static-analysis failure in the tab strip e2e test. No conflicts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant