fix(native-chat): a chat whose start never lands is released after the idle window - #22733
brennanb2025 wants to merge 5 commits into
Conversation
…e idle window The release clock treated a message held for a Claude start as owed work and re-armed forever, so an unheld start that never answered initialize and never exited kept its child for the host's lifetime. A pending send is no longer owed work: after the normal 30-minute idle window the child is closed, which fails the startup gate and rejects the held message with the reason. A start that lands inside the window renews it, as before.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Release a chat whose start never lands.
hasOwedWorkinstructured-agent-session-host-lifetime.tsdrops theproviderChildPhase === 'starting' && pendingSubmissions().length > 0clause, so an unviewed session holding a prompt for a start that never answersinitializeis evicted after the 30-minute idle window instead of being kept for the host's lifetime. Eviction closes the child, which rejects the held prompt asrejected(with the startup-failure reason) through the existing close path. - Docs updated to match. Comments in both
structured-agent-session-host-lifetime.tsandstructured-agent-session-release-clock.tsnow describe owed work as "a turn running" only. - Test rewritten to the new contract.
structured-agent-session-starting-release.test.tsnow uses fake timers and asserts that at the window's edge the child is closed, nothing is written to the CLI, and the pane's subscriber sees the held message asrejectedwith the "before it finished starting" reason. The slow-start test now lands init just inside the window and keeps asserting the turn is protected.
I verified the change directly: the new first test fails with the old clause restored (starting-release.test.ts:162), and the whole src/main/native-chat/agent-session-wire/ suite (106 files / 839 tests) passes. providerChildPhase: 'starting' is set only by the Claude adapter, no wire surface changes, and a held session is never touched by the clock (isHeld is re-checked at fire time), so the blast radius matches the described scope.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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 (9)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe host now treats only an active session turn as owed work. A pending submission during provider startup does not count. When a session closes while startup is pending, the startup gate fails and held writes receive the startup-abandoned rejection reason. Tests verify release at the grace-window boundary when startup does not land and session availability when startup lands before the boundary. Shared send-disposition logic reconciles journal submissions with renderer outbox entries and mobile sends, including late rejections, retries, and cancellations. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The session-release and late-rejection changes are mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change affects when an abandoned chat process stops and how unsent messages can be retried. The reviewed paths preserve host ownership of accepted messages and do not show a new access path. Some startup and recovery behavior remains incompletely covered. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
…I failed Closing a Claude child that had not finished starting rejected its held messages with "The provider stopped before it finished starting: claude stream-json ended before startup completed", which reads as a crash. The deliberate close now fails the startup gate with a plain reason first: Claude never finished starting, so Orca stopped it; send again to retry. A child that exits on its own keeps the diagnostic from its exit. Also imports the subscribe event type from the wire module in the test.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- A start Orca stops now says Orca stopped it.
finalizeClaudePublishedSessioninclaude-structured-session-close.tsfails a still-pendingstartup gate withCLAUDE_STARTUP_ABANDONED_REJECTION("Claude never finished starting, so Orca stopped it. Send again to retry.") beforeretireClaudeDispatchWaiters, so a prompt held by a chat Orca deliberately releases is rejected with that plain reason instead of the crash-sounding "claude stream-json ended before startup completed". The gate ordering matters: the close-path failure splices the held prompts first, so the laterretireClaudeDispatchWaiterscall finds nothing to reject. failClaudeStartupGategained an explicit rejection. The new optional third parameter defaults toproviderStartupFailureRejection(error), so the exit path (observeClaudeSessionExit), the startup-fault path, andretireClaudeDispatchWaiterskeep their existing wording unchanged.- Test pins the new copy. The idle-release case now asserts the held message is rejected with exactly
CLAUDE_STARTUP_ABANDONED_REJECTIONrather than a substring of the old reason, and the prior ablation coverage confirms the assertion is load-bearing.
This addresses the risk flagged in the previous review — that an intentional idle eviction surfaced the internal "claude stream-json ended before startup completed" detail. A child that exits on its own still keeps its own diagnostic because observeClaudeSessionExit marks the gate failed first.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…desktop and mobile A send the host answered pending could still be rejected later, when the child holding it was stopped before writing it. The desktop outbox left the entry as if it were still in flight, or showed "delivery is unconfirmed" after a reload, and never the reason. Mobile had already cleared the draft, and the message just disappeared from the chat. The journal side of the outbox now folds a rejected submission through the same disposition a rejected send result gets: the reason under the composer, the queue stopped on that message, and Retry sending it under a new id. The fold covers a live update, a reopened chat, and a rejection that beats its own send answer. Cancelled sends still drop silently. Mobile remembers the sends the host answered pending and reports one the stream later rejects, once, with the same notice an immediate rejection gets.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Late rejection on desktop now gets the same disposition as an immediate one.
foldStructuredAgentSessionJournalfolds arejectedjournal submission throughdisposeStructuredAgentSessionLateRejection, so a send the host answeredpendingand later rejected gets the reason under the composer, the queue blocked on it, and Retry under a new id — whether the chat is open, reopened with the rejection already recorded, or the rejection beats its own send answer. - The fold releases single-flight when the journal answers the in-flight send.
answeredInFlightnow also covers the case where the late rejection requeued the in-flight entry, so the still-pending send promise cannot overwrite the rejection. - Mobile reports a late rejection once.
useMobileStructuredSendOperationReconciliationremembers ids the host answeredpendingand reports a later stream rejection throughsettleAwaitedStructuredAgentSessionSends, using the same copy rule as an immediate rejection; cancelled and accepted sends stay silent and a resend is a new message. - Shared disposition extracted.
rejectedSubmissionDispositioncentralizes the reason/block/Retry mapping now used by both the send-result path and the new late path;reconcileStructuredAgentSessionOutboxis called from inside the fold.
I read the full diff end-to-end and traced the new fold against the inline logic it replaces. answeredInFlight and clearsError are supersets/equivalents of the old admittedInFlight and error-clearing branches, and the single-flight guarantee (at most one live send promise) makes the multi-entry requeue orderings unreachable. No new issues.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…ejection's reason on screen A journal rejection can block an earlier entry while a later send is still in flight. That send's clean answer preserved the block but cleared the error, leaving "Message was not sent." with no reason. A clean answer that leaves another entry blocked now keeps that entry's reason. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- A clean send answer no longer wipes another entry's rejection reason.
applyDispositioninuse-structured-agent-session-outbox.tsnow only callssetErrorwhen the result itself carries an error, so apending/accepted answer for one in-flight entry leaves the reason recorded for a still-blocked earlier entry (blockedClientMessageIdunchanged) on screen instead of replacing it with an empty "Message was not sent." row. - The guard is load-bearing and covered. The new case in
use-structured-agent-session-outbox-late-rejection.test.tsxsends a second message while the first is held, rejects the first through the journal, then lets the second answerpending; it asserts both the reason and the blocked id survive. I ran the file — 6/6 pass — and the assertion would go red against the previous unconditionalsetError(disposition.error).
The existing answeredInFlight generation guard only neutralizes the promise for the rejected entry, so it could not cover this case: here the in-flight id is a different entry, and its clean answer is applied. The fix belongs at that application point, which is the single owner of the error state, so the two rejection paths cannot disagree.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…er to send again On desktop the rejected message stays blocked in the queue, so a new send queues behind it and Retry then sends both. State what happened instead; desktop offers Retry beside it and mobile resends.
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes
- Reworded the abandoned-start reason.
CLAUDE_STARTUP_ABANDONED_REJECTIONno longer says "Send again to retry"; it now says "Your message was not sent." The desktop and mobile test fixtures that hardcode the string were updated to match, and no "Send again to retry" references remain in the tree.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
|
||
| /** Why a held prompt was rejected when Orca itself stopped a child still starting. */ | ||
| export const CLAUDE_STARTUP_ABANDONED_REJECTION = | ||
| 'Claude never finished starting, so Orca stopped it. Your message was not sent.' |
There was a problem hiding this comment.
The reason now ends with "Your message was not sent.", but on desktop that sentence is already the label rendered beside Retry (NativeChatDeliveryRetry shows Message was not sent.), so an abandoned start reads as "not sent" twice in the same stack. Mobile does need the clause because it shows the reason alone — worth deciding which surface should carry it.
Technical details
# Desktop renders the reason and the generic label side by side
## Affected sites
- `src/main/claude/claude-structured-session-startup-gate.ts:149` — the reason now ends with "Your message was not sent."
- `src/renderer/src/components/native-chat/NativeChatDeliveryRetry.tsx:33-36` — renders the generic `Message was not sent.` label plus Retry whenever an entry is blocked (`admitStructuredAgentSessionOutboxEntry` returns `blocked` for the entry named by `blockedClientMessageId`, including a requeued one).
- `src/renderer/src/components/native-chat/NativeChatStructuredSessionStatus.tsx:71-75` — renders `controller.error` (this reason) in red, in the same stack.
- `mobile/src/session/use-mobile-structured-send-operation-reconciliation.ts` — mobile surfaces the reason alone through `onSendError`, with no label or Retry.
## Required outcome
- Desktop should not state "message was not sent" twice in the same error stack; mobile must keep a self-contained statement because it has no label/Retry.
## Open questions for the human
- Should the desktop label drop its copy when a reason is present, or should the shared constant stop carrying the clause (accepting that mobile then reads only "Claude never finished starting, so Orca stopped it.")?
Hands-on check in the running app (commit db8f4ba)I ran this branch as a hidden dev build with a fresh profile and a scratch repo. A small wrapper stood in for the Claude command line so I could make it hang, start slowly, or fail on purpose. To make the test practical, I shortened the 30-minute idle window to 45 seconds in my local copy only. That change was never committed. Everything else is the branch as pushed. The main case does not happen for a desktop user yetYou send a message while Claude is still starting, then switch to another worktree or tab. With the branch as is, the starting chat is never released. Claude was still running and the message was still waiting well over twice the window later. Main behaves the same way, so in this situation the branch doesn't change what a desktop user sees. Before leaving, and after coming back 100 seconds later (still starting):
Two things keep the chat held, and I confirmed both inside the running app:
So on desktop, the only things that end a hung start today are closing the chat (the child exited about 1.6 seconds after the close) or quitting. When nothing holds the chat, the new behavior is rightTo test the change itself, I removed the host's record of who holds one hidden chat, using the app's debugger (a test step, not something a user can do). The idle timer, stopping Claude and the chat screen all ran unmodified.
Two things to look at, both seen only after this test step:
Typing a new message instead of pressing Retry (the known issue you're fixing separately) behaved as described. The new message waits behind the rejected one, the red reason disappears, and later both are sent in order.
Other checks: all as expected
Mobile (iPhone simulator paired to this dev build)The mobile app ran this branch's code. With the mobile chat open and the same test step used to release the chat, mobile showed the new reason above the message box, in red, once. I took screenshots every 0.3 seconds, and it appeared in one continuous stretch of about 4 seconds. The waiting message was removed.
Notes:
Not covered: quitting Orca while a message is waiting, and a rejection that arrives while the mobile chat is closed. |
|
Closing without merging: this is folded into a larger native-chat redesign. QA showed the host change works once nothing holds the chat, but on desktop an open chat keeps holding it. Reading a hidden chat with an unsent message counts as holding, so the hung start is never released. There is also a second problem: after a release, Retry hits a fence mismatch until reload. Fixing those means changing what keeps a chat's agent alive, which is the redesign's scope. This PR's host change, its test and its 'Claude never finished starting, so Orca stopped it' message move into that work. |
1 participant
ELI5
When you send a message to a Claude chat that is still starting and then switch away, Orca keeps the Claude process until you come back. Before this change, a start that never finished and never crashed was kept forever. Now it gets the same 30-minute idle window as any other chat you've left. If the start still hasn't finished by then, Orca stops it and marks the message it was holding as not sent. The chat shows why ("Claude never finished starting, so Orca stopped it. Your message was not sent."), and Retry sends the message again, which starts Claude fresh. More generally, any message Orca first said was on its way and then couldn't deliver now shows the reason. On desktop it also gets Retry. On mobile the reason appears once, while the chat screen is open. Before, it looked sent on desktop and quietly vanished on mobile. A rejected message still blocks the messages sent after it until you press Retry, as it already did for messages refused right away. A follow-up will change that (see "Not changed").
What Changed
Before: Send a message to a Claude chat whose CLI is still starting, then leave the chat. If the CLI hangs before it answers
initializeand never exits, Orca keeps that child process until the app quits. The message shows as pending the whole time. On desktop the pane says "still starting" while you are looking at it, but nothing ever ends the start unless you close the tab.After: The chat is released after the normal 30-minute idle window with no viewer. Releasing it closes the child. That fails the startup gate, so the held message settles as
rejectedwith the reason "Claude never finished starting, so Orca stopped it. Your message was not sent." It never reached Claude, so resending it is safe, and a send to the released chat starts a new Claude process. A slow start that lands inside the window is unaffected: the start landing restarts the window, and so does any journal write.What the chat shows for that message, and for any send the host answered
pendingand rejected later (a close, a quit, a relaunch, or a handoff while it was held):Mechanism: The release clock re-arms whenever
hasOwedWorkanswers true (structured-agent-session-host-lifetime.ts). Until nowhasOwedWorkcounted two things as owed work: a running turn, and "the child is starting and has a pending submission". This PR deletes the second clause. A running turn is still owed work. A pending message is not, because nothing guaranteed the start would ever land, so the clock re-armed forever. No new rule takes its place. The existing close path already settles the held message: eviction stops the child, the Claude adapter retires its dispatch waiters,failClaudeStartupGaterejects the held prompts, and that late settlement lands in the journal before the session is forgotten.The late rejection in the chat: the desktop outbox now handles a journal update in
foldStructuredAgentSessionJournal(structured-agent-session-send-disposition.ts), next to the function that handles a send's own answer. When the journal says a message the outbox still holds was rejected, the fold gives it the same result a rejected send answer gets (rejectedSubmissionDisposition): reason, blocked, and Retry under a new id. So a rejection shows the same way whenever it arrives. If the rejection arrives before the send's own answer, the fold also releases that send, so its latependinganswer can't overwrite the rejection. A rejection that was already shown when the send answered is left alone, and a cancelled send is still dropped silently. Mobile has no outbox, so it keeps an in-memory list of the sends the host answeredpending. When the stream settles one as rejected, it shows the same notice throughsettleAwaitedStructuredAgentSessionSends, which uses the same copy rule as an immediate rejection. The wire doesn't change: the reason was already in the submission.The reason wording: before this PR, every deliberate close of a still-starting child rejected held messages with "The provider stopped before it finished starting: claude stream-json ended before startup completed", which reads as a crash. The Claude adapter's close (
finalizeClaudePublishedSession) now fails a still-pending startup gate with a plain reason before it retires the waiters. The reason is decided where Orca decides to stop the child, not by matching text afterwards. A child that exits on its own keeps the diagnostic from its exit (for example "not signed in"). The wording deliberately doesn't say "after 30 idle minutes": the same close also runs when the user closes the chat, when Orca quits, and on a handoff to the terminal, and it is true in every one of those. It also deliberately doesn't say "send again": on desktop the rejected message stays blocked in the queue with Retry beside it, so a fresh send would queue behind it and Retry would then send both.Why
claude-structured-session-startup-gate.ts), so a child that never started accepted nothing. The rejection is a proven fact and keeps Retry available.Differences from the common pattern
hasOwedWorkdoesn't consider background tasks. This PR leaves that alone because it is a separate question.Not changed
evictHeldStructuredAgentSession, called for each session byevictOwnedStructuredAgentSessions) that the new test instructured-agent-session-starting-release.test.tsexercises through the idle release. I did not run a separate quit-specific test.MobileSessionSheets.tsx, the agent-session action sheet built bycloseWithBulkActions). It sendssession.tabs.closewith reasonuser. The host closes an agent-session tab throughcloseStructuredAgentSessionTab, which calls the structured host'sclose. That is the same eviction this PR relies on, so the child stops and the held message is rejected with the reason above. I traced this in code and did not run it on a device. Leaving the chat without closing it drops mobile's hold, so the 30-minute idle window starts once no other screen has the chat open.Linked Issue
Follow-up to #22364.
Visual Proof
N/A. No new UI. Existing UI now shows up where it didn't: a message rejected after it went pending gets the existing "Message was not sent." row with the reason and Retry on desktop, and the existing send error on mobile while the chat screen is open, instead of looking sent or disappearing with no explanation. The tests below assert the exact reason text at the outbox hook and at mobile's error callback. I didn't record it in the running app.
Testing
structured-agent-session-starting-release.test.tsruns a real Claude adapter over a fake CLI, uses fake timers, and uses a 5 ms grace in place of 30 minutes:rejectedwith the exact plain reason. Nothing was written to the CLI.host-lifetime.ts), the test fails because the session is never released.failClaudeStartupGatedeleted from the close path, it fails withunknown/provider_closed_before_acknowledgement. That shows the rejection assertion depends on the close path.finalizeClaudePublishedSession, it fails and receives the old "claude stream-json ended before startup completed" wording.rejectedwith the plain reason, so reopening the chat finds the rejection. The "start landing restarts the window" case goes red only when both renewals are removed (the start-landed restart and the journal-write restart). Either one alone keeps it green.use-structured-agent-session-outbox-late-rejection.test.tsx(desktop outbox hook):retryUnknown;pendinganswer: the rejection stays;Deleting the late-rejection step from the fold makes the first three fail. Deleting only the in-flight release makes the third fail. The last two pass on main too; they guard against double handling.
use-mobile-structured-agent-session-late-rejection.test.tsx(mobile session hook):Reverting the mobile change makes the first two fail. Deleting only the check made when a send is registered makes the second fail.
Desktop renderer and shared suites pass (10954 tests). The mobile suite passes (9524 tests) and the mobile app typecheck passes, with mobile dependencies installed locally.
pnpm tc,check:code-quality:changed,check:react-doctor:changedandoxlinton the changed files all pass.pnpm test src/main/native-chat src/main/claude src/main/runtimepasses except these, all local-only:tsc --noEmit -p config/tsconfig.node.jsonpasses. macOS only. SSH: the release clock and eviction run on the execution host, so a remote host behaves the same way. There is no wire change.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
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)