Skip to content

fix(native-chat): acting on one chat no longer waits on checks of every other chat - #26555

Merged
brennanb2025 merged 3 commits into
mainfrom
brennanb2025/chat-reconcile-only-its-own
Oct 16, 2026
Merged

brennanb2025 merged 3 commits into
mainfrom
brennanb2025/chat-reconcile-only-its-own

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​284 $\color{#cf222e}{\Huge{\mathbf{−}}}$​12 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​272
Prod 7 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​69 $\color{#cf222e}{\Huge{\mathbf{−}}}$​44 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​25

ELI5

When Orca restarts, it double-checks every saved chat before letting any of them start or resume an agent, to make sure the agent from before the restart is really gone. Before this PR, starting, resuming or creating any one chat waited for that check on every saved chat. If one chat's check was slow or failed, a different chat (even a brand-new one) waited too, or was refused. Now each action checks only its own chat. A brand-new chat checks nothing.

What Changed

Problem. After a restart every saved chat's writer lease loads as "unreconciled": the agent process that held it may still be running, so no new writer is allowed until this Orca has checked it (process start time and spawn token for local owners; an owner on another host stays "unverifiable", never "exited"). All actions shared one host-wide check. Attaching (create/resume) or starting an agent in chat X ran or joined a pass that probed every unreconciled saved chat and committed them together. So X waited on every other chat's probe, and was refused if another chat's record kept changing until the pass gave up (execution_owner_reconciling).

Before (user): after a restart, starting or resuming a chat could be slow or refused because of a different chat. A brand-new chat ran the whole check too.
After (user): an action waits only on its own chat's check. A new chat starts right away. Nothing visible changes in the normal case. No new text, notices or settings.

Mechanism.

  • The restart reconciler takes an optional session. Per-chat calls select and probe only that chat's record, and concurrent calls share work only for the same chat. With no session (startup), it runs the whole-host pass on its own, and actions never join it.
  • Startup still runs one whole-host pass over every record.
  • The readable-chat restore at startup still opens with one whole-host check (it also retries a startup pass that failed). Like before, it stops per-chat bookkeeping after the first store failure, so a failing disk gets one write attempt per startup step, not one per chat.
  • Safety is unchanged. A stale probe result is discarded by the existing whole-record comparison when the startup pass and a per-chat pass race on the same chat. A chat whose ownership is unproven still stays in recovery and gets no writer, and acquisition still refuses an unreconciled lease inside the store transaction.
  • No persisted, wire or RPC fields change. The session selector is internal.

Why

Catching and ignoring other chats' failures would not remove the wait, and could hand out a writer without proof. Selecting the target before probing keeps unrelated chats out of both the probe and the commit. This extends the existing reconciliation rather than adding a second lease store or changing any recovery verdict.

The common pattern scopes a user action to the conversation it addresses. Differences:

  • Intended: Orca keeps a whole-host startup pass in addition to the per-chat check on the action path. Orca's provider processes can outlive the app, so a verdict needs process checks, and those must not sit on another chat's path. Concurrent runs are safe because the record comparison discards obsolete results.
  • Intended: ownership is proven by process identity (start time, spawn token), not by the owner vouching for itself. Owners on another host answer "unverifiable", never "exited".
  • Not changed here (follow-ups): the startup pass is still awaited before terminal restoration (it now carries no user action, so this costs only startup latency). Expired operation receipts are still pruned inside the same store write, which is the separate receipts work.

Linked Issue

Maintainer work from the chat-write admission audit; no pre-existing issue.

Visual Proof

Behaviour-only change with no UI difference, so there is no before/after pair. The live check ran on the PR build on Windows (low-spec box) with an isolated profile and a stand-in Claude agent (no real agent or credentials). It created 3 native chats, quit, relaunched, then created a 4th chat and sent in saved chats. Every reply came back in about 3 s with no notice. Zero lease-reconcile warnings in either run's main log.

Restored chat 1 after restart (history kept, no notice):
after3-run2-restored-chat1.png

Restored chat 2 after restart:
after3-run2-restored-chat2.png

Brand-new chat right after restart, answered:
after3-run2-newchat-4.png

Send in a saved chat after restart (agent resumed with the same session):
after3-run2-send-2.png

Testing

  • I manually tested these changes (live Windows run above)

  • Automated tests added/updated

  • New tests (real SQLite store):

    • a new chat is created while another saved chat's owner check fails;
    • an action does not wait on another chat's stalled check in the startup batch;
    • a chat settles while another chat's record changes under every check (this failed on the parent commit);
    • simultaneous checks of the same chat share one pass;
    • an unproven target stays in recovery;
    • the startup pass still reaches every record.
  • The restore tests pin that restore opens with one whole-host check.

  • Existing startup tab-restore, restart, recovery and reader-restore suites pass. CI is green on the final head, and the scoped Node typecheck passes.

  • Not verified live: a real SSH or WSL host, a real Claude/Codex agent, a paired older client, and a live reproduction of the slow-other-chat case. That case needs an injected slow or failing probe, so it is covered by the tests.

Review

Review summary is in a PR comment.

Agent skill upstream boundary

  • Not applicable: no upstream skill-installer material is copied.

Notes

Security: no new inputs, dependencies or network use. Cross-platform: no platform-specific code changed; the same owner probe runs on macOS, Linux and Windows. SSH/remote: owners on another host stay "unverifiable" as before. Mobile: no mobile-facing surface touched. Backward compatibility: no persisted or wire change. Performance: in the normal case actions do less work (one record instead of all). A few actions arriving while the startup pass is still running may probe their own chat a second time.

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)

Author: @BrennanKB5

brennanb2025 and others added 3 commits October 8, 2026 03:48
…ry other chat

Starting, resuming or switching a chat's agent first ran restart
reconciliation, which probed every unreconciled session and committed them
together, so one slow or failing probe for another chat delayed or failed
this chat's action. Acquisition now reconciles only the addressed session;
the explicit startup pass still covers all of them, and an owner that isn't
proven still can't be granted a writer.

Co-Authored-By: OpenAI Codex gpt-6.1-sol <noreply@openai.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sqlite tests

- Startup restore no longer lets one chat's failed lease check skip recovery for the others.
- Run the two new SQLite-backed tests in the Node runtime project.
- Startup reconcile runs the whole-host pass without a placeholder session id.
- Test names describe an injected failing/stalling owner check, not an SSH host.
The restore opens with the whole-host pass again and stops per-chat bookkeeping after the
first failure, as on main: on a failing store it makes one lease-check write, not one per
chat. User actions stay targeted to their own chat.
@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review summary

Problem. After Orca restarts, every saved chat's writer lease must be checked before that chat may start or resume an agent. All actions shared one host-wide check. Starting, resuming or creating one chat (even a brand-new one) therefore waited on every other saved chat's process check, and was refused if another chat's record kept changing until the check gave up.

User-facing change. After a restart, acting on a chat waits only on that chat's own check, and a new chat starts right away. In the normal case nothing looks different: no new text, notices or settings.

Review. The review ran in two rounds, plus a readiness pass over the final diff. Round 1 had three independent reviewers: ownership safety and concurrency, callers/performance/tests, and an architecture challenger that sketched its own design first. Their verdict was to keep the design; it matches the independent sketch.

Verified safe:

  • No writer is granted while ownership is unproven. The verdict is per record, and acquisition refuses an unreconciled lease inside the store transaction.
  • Nothing on the action path depends on other chats being checked first. That includes provider-session conflicts, tab ownership, and /clear replacements.
  • A startup pass and a per-chat pass racing on the same chat resolve correctly: the existing whole-record comparison discards the stale result. Both orders were proven with real-store tests.
  • Owners on another host stay "unverifiable", never "exited".

Fixed in this loop:

  • The two new SQLite-backed test files were not registered for the Node test runtime. They failed under the default runner, and so did the SQLite boundary guard.
  • The startup readable-chat restore had silently become a single-chat check, so a startup pass that failed was no longer retried host-wide. It is back to one whole-host check. A first attempt removed the restore's stop-after-first-failure latch instead, and CI showed that breaks the existing "one failed write per startup step" contract. It was reverted to the base latch.
  • Test names no longer claim to model an unreachable SSH host. The production owner probe never contacts remote hosts and never throws. A realistic coupling test was added (another chat's record changes under every check); it fails on the parent commit.
  • The startup path no longer passes a dummy 'startup' session id. The reader reconcile is typed for a zero-argument call.

Deferred (P3 or lower; none block):

  • A startup pass that fails, or never runs (for example, a headless server before any client lists tabs), is not retried host-wide until a restore or the next start. Untouched chats stay "unverifiable" rather than "exited" until their own action. This does not affect any writer.
  • The startup pass is still awaited before terminal restoration. That costs startup latency only, and it is a follow-up.
  • Expired operation receipts are pruned inside the same store write as a chat's verdict. That belongs to the separate receipts work.
  • An action arriving during the startup pass may probe its own chat a second time. A transient failure of that probe fails the action once; a retry succeeds.
  • One chat's failure in the restore still stops startup recovery bookkeeping for later chats. This behaviour is unchanged from main and kept so a failing disk is not hit once per chat; each chat settles on its own action.
  • Log noise: one chat's success clears the failure de-dup, so another chat's persistent failure can be logged again.

Verified:

  • CI is green on the final head.
  • The scoped Node typecheck passes, and the static checks pass 22/22.
  • The targeted suites pass: the 60 restart/restore/reconcile test files, 456 tests.
  • Live run on Windows (low-spec box), PR build, isolated profile, stand-in Claude agent. Three chats were created, then a quit and relaunch. All three reopened with history, a new chat and sends in two saved chats were answered in about 3 s with no notice, and the main logs had zero lease-reconcile warnings. The real-home config hashes were unchanged.

Not verified:

  • A real SSH or WSL host.
  • A real Claude or Codex agent.
  • A paired older client.
  • A live reproduction of the slow-other-chat case. It needs an injected slow or failing probe, so the tests cover it.

@brennanb2025
brennanb2025 marked this pull request as ready for review October 8, 2026 13:30
@coderabbitai

coderabbitai Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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: a94e083a-6db8-400a-becb-b027de183d7d
📥 Commits

Reviewing files that changed from the base of the PR and between e0a98a4 and d498578.

📒 Files selected for processing (12)
  • config/scripts/vitest-sqlite-runtime-files.mjs
  • src/main/native-chat/agent-session-wire/structured-agent-session-acquire-isolation.test.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.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-reveal.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-target-reconcile.test.ts
  • src/main/runtime/agent-session-record-store.ts
  • src/main/runtime/agent-session-restart-reconciliation.ts

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


📝 Walkthrough

Walkthrough

Restart lease reconciliation now supports checks for one session or all stored sessions. Restart restoration performs a host-wide lease check before per-session reconciliation and recovery resolution. The changes also add tests for targeted reconciliation, concurrent checks, owner-probe outcomes, and acquisition isolation.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to d4985

No actionable issue remains from this review; the change is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 12 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: actions on one chat no longer wait for reconciliation checks on other chats.
Description check ✅ Passed The description is complete and closely follows the repository template. It explains the user impact, implementation, rationale, testing, visual validation, compatibility considerations, and checklist…
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@brennanb2025
brennanb2025 merged commit 865cdaa into main Oct 16, 2026
61 checks passed
brennanb2025 added a commit that referenced this pull request Oct 16, 2026
… reconcile)

Part 4 landed as squash 7a78dbd, so this merge is resolved against that squash as the
base for its changes: Part 4's own lines come in once, and only what main changed after it
meets Part 5.

#26555 composed with Part 5's retry: one reconciler with an optional chat id. A chat's own
action (start, attach, send, a restored read) reconciles only that chat and never joins a
background run; a background run joins a person's run for its target; the every-chat
background pass (startup, the retry's store-wide step) lets chat checks in flight finish first
and holds no chat's lane. The restore keeps #26555's one every-chat check per pass.

After the squash, main kept the acquisition-time settle and open option that Part 5 removes;
those stay removed. A failed start records only this process's sends (Part 5) with main's
new start-failure causes.
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