Repository navigation
Native chat: a failed save of an earlier session's unsent message no longer fails your new messages - #30405
Native chat: a failed save of an earlier session's unsent message no longer fails your new messages#30405brennanb2025 wants to merge 5 commits into
Conversation
…nger rejects this process's queued sends The delivery loop and its ordering now read the derived handsOver rule, so a message an earlier Orca process left queued is never picked whether or not the write that keeps it as a card succeeded. That write is bookkeeping again: its failure is logged and retried at the next delivery step instead of failing the run and rejecting every queued message.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 62.96% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 34 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Issue #28122is closed and supplies historical context only. No active directly linked coding target remains. This PR has no linked-issue coding requirements.Out of Scope Changes check The production changes address restart delivery ownership, failed settlement, message identity after history replacement, and related delivery readers. The test changes support these behaviors and upd… Title check The title clearly states the main user-facing change: failures while saving an earlier session's unsent message no longer fail new messages. Description check The description is detailed and covers ELI5, changes, rationale, alternatives, issue context, visual proof, testing, platform considerations, tradeoffs, and checklist items. Minor inconsistencies rema… Full details: Out of Scope Changes check
-
Explanation
The production changes address restart delivery ownership, failed settlement, message identity after history replacement, and related delivery readers. The test changes support these behaviors and update affected fixtures. No unrelated change is demonstrated.
Full details: Description check
-
Explanation
The description is detailed and covers ELI5, changes, rationale, alternatives, issue context, visual proof, testing, platform considerations, tradeoffs, and checklist items. Minor inconsistencies remain: the manual-testing checkbox is unchecked despite reported Electron QA, the visual-proof checklist is unchecked despite attached evidence, and the build result is not explicitly reported.
-
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · 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.
Comment @coderabbitai help to get the list of available commands.
A send an earlier Orca process left queued is now named by its id, captured when the journal opens, so a provider history replacement can no longer make it look like this process's send. Every host reader that means this handle's own deliverable work asks the same rule: delivery picking and order, the idle sweep, the failed-start row, Stop's target and withdrawal, the start-failure rejection, and whether the handle can be released. Raw queued checks stay where they mean never handed to the agent.
|
An unsent message from an earlier Orca process could prevent new messages from running: if saving the old message as a card failed at open and again at delivery, the delivery loop failed and rejected this process’s queued messages too. This change keeps new messages deliverable when that recovery write fails. It identifies messages already queued when the journal opened, so every host operation uses the same rule for which messages this process owns. It preserves the existing restart-card behavior and adds no saved field or wire change. The review fixed an old, never-delivered command being used to explain a new message’s failure. It also added one best-effort attempt to save the existing restart rejection for old messages when keeping them as cards fails; failure of that write cannot fail the new run. Three review rounds, an independent architecture challenge, a bounded design ruling, and final readiness and performance reviews covered head One merge gate remains (QA has since passed):
For Brennan’s review Main-versus-branch screenshots are in the PR description's Visual Proof section. This comment does not certify completion or merge readiness while the product decision above is open.
|
ELI5
When Orca restarts, a chat can still hold a message the earlier Orca accepted but never sent. Orca keeps that message as a waiting card instead of sending it on its own. Saving that card is a database write. If that write failed, Orca gave up on the chat's whole queue and marked every waiting message as failed, including the ones you had just typed. Now Orca tells "the earlier Orca's message" apart from "your new messages" without needing that write to succeed: your new messages can proceed when their own required writes succeed, and the old one is never sent automatically. If keeping it as a card still fails during delivery, Orca makes one more best-effort write to mark only that earlier message as not sent, with the existing restart explanation. Failure of that write does not fail your new message.
What Changed
Before: after a restart, if keeping the earlier Orca's unsent message as a card failed both when the chat opened and again just before the next handover, the delivery run failed and every queued message in the chat was rejected with "Orca failed", including messages sent in this session that were perfectly safe to send.
After: your own queued messages can proceed when their own required writes succeed. If both attempts to settle the earlier message during delivery fail, one further best-effort write rejects only that earlier message with “Orca restarted before this message was sent.” This is a saved rejection, so later recovery cannot also create a card for it. If that write fails too, it is logged and your new messages still proceed. The earlier message is never sent automatically, does not count as current host work, and does not prevent the chat from closing. The existing successful recovery-card path is unchanged; the remaining failure-state tradeoff is described below.
Mechanism: the problem was structural. "This queued message belongs to this Orca, so it may be delivered, withdrawn or waited on" was read straight off the stored queued state, which is only right once the restart cleanup has succeeded. So a failed cleanup write had to fail the whole run to keep the earlier message from being sent.
structuredAgentSessionHandsOver(queued and not accepted before open), which the Working status already used, is now what every host reader that means "this chat's own deliverable work" asks: choosing and ordering the next message to send, the wake after a journal commit, the idle sweep, the failed-start error row and its command retry advice, Stop's target and its withdrawal, the start-failure rejection, and whether a chat can be released.Why
Bookkeeping should never decide whether the person's own message is sent. The common pattern works out which queued messages are eligible from facts already held (saved rows plus what this process owns), instead of from a flag a recovery step must write first; a design that reads a written flag is only safe when a failed write stops the server from starting, which Orca deliberately never does.
Alternatives considered:
Linked Issue
Tracked in #28122 (closed unmerged; this is its one remaining fix).
Visual Proof
Real Electron QA on a separate Mac (the M4Air), main at 36913fd versus this branch at 64a7055. Same inputs on both: a real signed-in Claude chat; a message sent while
/compactruns (so it's accepted but not yet handed over); then Orca quit and relaunched. Both builds behaved the same and as intended: after the restart the message waits as a card and is never sent on its own; a new message runs first and the card follows it; Stop shows Paused with Resume and keeps the card; Resume sends it. Codex was also checked as a second agent (it answered in the branch build). The failed-save path this PR fixes has no way to trigger it from the app, so this QA covers the surrounding normal behavior only; the failure path is covered by the automated tests below.Testing
structured-agent-session-unsent-send-hold.test.ts: injects failed settlement for earlier messages, including/compact. If a new ordinary message cannot start the agent, its error says to send the message again, while the earlier command stays untouched. A subsequent new message is still sent, Stop preserves the earlier messages, and a later successful settlement keeps the ordinary earlier message as a card. This tests ownership under injected failures; it does not reproduce a persistent database outage.journal-store.spec.ts: an earlier handle's queued message is still recognised after a provider history replacement re-numbers it; a message accepted after it is not.The existing recovery test now covers two cases: after the open’s two writes fail, the delivery step keeps the card; after both the open’s and delivery step’s writes fail, the extra rejection succeeds with restart wording and no card. The new message is delivered in both cases.
CI runs the full suite, lint and typecheck.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Review
Agent skill upstream boundary
Notes
Author: Brennan Benson (X: @BrennanKB5)
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)