fix(native-chat): one path restarts a chat whose agent failed to start - #22741
brennanb2025 wants to merge 2 commits into
Conversation
A create whose agent died while being acquired used to leave a record and no journal: nothing to read, no tab, and nothing a send could be admitted into. The renderer covered it with a second restart path — a send into a failed launch re-ran the create under a new operation. The conversation now exists from its reservation. The attach founds the session's journal, and imports an adopted transcript into it, before it spawns anything. A create whose first child is proven gone answers that conversation as a readable chat whose first row says why the agent stopped; a send restarts the agent through the host's ensure-owner step like any chat whose child ended. The renderer's relaunch-on-send is deleted. Launch Retry stays for a create refused before anything was reserved, and for a host that predates this answer.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe attach flow records adopted conversation history before owner acquisition. A qualifying create failure can now return a readable session with a startup status row. The RPC create route calls the host’s create method. Renderer sends no longer trigger launch relaunch; users can retry the launch explicitly. Priority: ➖ Normal Merge Risk: 🔵 Low · up to Failed creates remain readable, but a repeated create reports the wrong replay state. Correct that response field; the remaining established risk is bounded. 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 15c87f25-9852-496d-b3dd-0fa28feb2cef
📒 Files selected for processing (18)
src/main/native-chat/agent-session-wire/structured-agent-session-adopted-import.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adopted-import.tssrc/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.tssrc/main/native-chat/agent-session-wire/structured-agent-session-conversation-founding.tssrc/main/native-chat/agent-session-wire/structured-agent-session-failed-create-readable.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-failed-create.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-mutations.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host.tssrc/main/native-chat/agent-session-wire/structured-agent-session-send-preparation.tssrc/main/native-chat/agent-session-wire/structured-agent-session-start-failure-row.tssrc/main/runtime/rpc/methods/structured-agent-session-create.tssrc/main/runtime/rpc/methods/structured-agent-session-precommit-refusal.test.tssrc/main/runtime/rpc/methods/structured-agent-session-rpc.test-fixture.tssrc/renderer/src/components/native-chat/NativeChatStructuredSession.launch-lifecycle.test.tsxsrc/renderer/src/components/native-chat/NativeChatStructuredSession.tsxsrc/renderer/src/components/native-chat/use-native-chat-provisional-launch.tssrc/renderer/src/lib/structured-agent-session-launch-exited-owner.test.tstests/e2e/cross-version-wire/structured-agent-session-host-fixture.ts
💤 Files with no reviewable changes (1)
- src/renderer/src/components/native-chat/use-native-chat-provisional-launch.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.
| const tabId = context.deps.store.getRecord(sessionId)?.surfaceTabId | ||
| return { | ||
| ok: true, | ||
| replayed: false, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
set -eu
printf '%s\n' '--- changed file diff ---'
git diff --unified=80 29480cd8cd35a1cf12f36bf056a7e280670c8c52 0e8477f126a20c9d1d89755dc8cfdf9823d15dd6 -- src/main/native-chat/agent-session-wire/structured-agent-session-failed-create.ts
printf '%s\n' '--- implementation ---'
cat -n src/main/native-chat/agent-session-wire/structured-agent-session-failed-create.ts
printf '%s\n' '--- related test files ---'
find . -name 'structured-agent-session-failed-create-readable.test.ts' -o -name '*agent-session*attach*' | sort
printf '%s\n' '--- test references ---'
rg -n -C 8 'replayed|repeated|attach|failed create|failedCreate' --glob '*structured-agent-session-failed-create-readable.test.ts' --glob '*.ts' src/main/native-chat/agent-session-wireRepository: stablyai/orca
Length of output: 45649
🏁 Script executed:
set -eu
printf '%s\n' '--- failed-create readable test ---'
wc -l src/main/native-chat/agent-session-wire/structured-agent-session-failed-create-readable.test.ts
cat -n src/main/native-chat/agent-session-wire/structured-agent-session-failed-create-readable.test.ts
printf '%s\n' '--- attach orchestration replay references ---'
rg -n -C 12 'replayed|clientOperationId|operationId|idempot|attach\(' src/main/native-chat/agent-session-wire/structured-agent-session-attach-orchestration.ts src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.ts src/main/native-chat/agent-session-wire/structured-agent-session-attach-failure.ts src/main/native-chat/agent-session-wire/structured-agent-session-attach.ts
printf '%s\n' '--- failure-row recorder ---'
cat -n src/main/native-chat/agent-session-wire/structured-agent-session-start-failure-row.ts
printf '%s\n' '--- shared result and operation types ---'
rg -n -C 12 'AgentSessionMutationResult|AgentSessionAttachResult|replayed|clientOperationId' src/shared/agent-session-wire.ts src/main/native-chat/agent-session-wireRepository: stablyai/orca
Length of output: 45661
🏁 Script executed:
set -eu
printf '%s\n' '--- attach flow bounded source ---'
sed -n '80,280p' src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.ts
printf '%s\n' '--- attach flow remaining result construction ---'
sed -n '280,520p' src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.ts
printf '%s\n' '--- reserveOwner definitions and dispositions ---'
rg -n -C 20 'reserveOwner|disposition:.*replayed|disposition.*replayed|type.*Reserve|replayed' src/main/runtime src/main/shared src/shared src/main/native-chat/agent-session-wire --glob '*.ts' | head -n 1200Repository: stablyai/orca
Length of output: 42440
🏁 Script executed:
set -eu
printf '%s\n' '--- direct reserveOwner files ---'
rg -l 'async reserveOwner|reserveOwner\s*=' src/main --glob '*.ts' | sort
printf '%s\n' '--- host create path ---'
rg -n -C 20 'answerStructuredAgentSessionCreate|async create|create\(' src/main/native-chat/agent-session-wire/structured-agent-session-host.ts src/main/native-chat/agent-session-wire/structured-agent-session-host-mutations.ts src/main/native-chat/agent-session-wire/structured-agent-session-start-failure-row.ts
printf '%s\n' '--- start-failure recorder ---'
cat -n src/main/native-chat/agent-session-wire/structured-agent-session-start-failure-row.ts
printf '%s\n' '--- reserve implementation excerpts ---'
for f in $(rg -l 'async reserveOwner|reserveOwner\s*=' src/main --glob '*.ts' | sort); do
echo "--- $f"
rg -n -C 35 'async reserveOwner|reserveOwner\s*=|disposition|operationRow' "$f" | head -n 500
doneRepository: stablyai/orca
Length of output: 16616
🏁 Script executed:
set -eu
printf '%s\n' '--- reservation commit implementation locations ---'
rg -l 'commitAgentSessionReservation' . --glob '*.ts' | sort
printf '%s\n' '--- reservation commit implementation ---'
for f in $(rg -l 'commitAgentSessionReservation' . --glob '*.ts' | sort); do
echo "--- $f"
rg -n -C 45 'commitAgentSessionReservation|disposition|operationRow|outcome' "$f" | head -n 1000
done
printf '%s\n' '--- host create delegation ---'
rg -n -C 15 'answerCreate|create\s*[:=]|create\(' src/main/native-chat/agent-session-wire/structured-agent-session-host.ts src/main/native-chat/agent-session-wire/structured-agent-session-host-mutations.tsRepository: stablyai/orca
Length of output: 32938
Preserve the replay state for failed creates.
A repeated create replays the stored failure, but answerStructuredAgentSessionCreate converts that refusal into a readable result with replayed: false. Propagate the attach operation's replay state into this conversion and assert replayed: true in the repeated-create test.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
pnpm tc:node is red on this branch — a typing error in the new test-fixture line blocks CI. Everything else reads sound; the only inline comment is the fix for that.
Reviewed changes
- Journal founded at reservation, before the spawn. New
structured-agent-session-conversation-founding.tsopens the journal and applies the adopted-transcript import right beforeacquireOwnerinstructured-agent-session-attach-flow.ts, then closes it in afinally; failures wrap inAgentSessionPreSpawnErrorso the reservation settlesprocessless. - Adopted import moved pre-spawn.
importAdoptedTranscriptnow takes thejournaldirectly and no longer closes it on failure; its sole caller (the founding step) owns the close. - Create answers a proven-exit failed start as a readable chat. New
structured-agent-session-failed-create.ts(answerStructuredAgentSessionCreate) writes a startup-failure row keyed by the create operation and returns{ok:true}with the hydration page;host.createis the new host entrypoint andcommitStructuredAgentSessionCreatecalls it. Replays re-derive the same chat with no second row. - Renderer relaunch-on-send deleted.
sendThroughRelaunchand its use inNativeChatStructuredSession.tsxare gone; a send into a still-failed launch parks in the outbox until launch Retry. - Refactor:
settleLateDispatch/releaseUnansweredDispatchesmoved onto the mutation delegates; host/test stubs gainedcreate.
I ran the new and adjacent suites: structured-agent-session-failed-create-readable.test.ts (4), structured-agent-session-adopted-import.test.ts (10), NativeChatStructuredSession.launch-lifecycle.test.tsx (11), and structured-agent-session-precommit-refusal.test.ts (9) all pass. The cross-version wire unit test fails only because the CI checkout is shallow with no release tags.
ℹ️ Nitpicks
- A failed create writes
failed-start:<createOpId>, and the next send that also fails writes an identicalfailed-restart:<sendOpId>row viarecordFailedRestart(structured-agent-session-send-preparation.ts), so the user can see two identical "The provider stopped before it finished starting" rows for one persistent failure. Worth confirming that is intended rather than reusing the create's key. readFailedCreate(structured-agent-session-failed-create.ts) always returnsreplayed: false, even when the answer comes from a replay of the create operation.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…nt fails to start A chat surface gets a readable conversation when its agent fails to start and restarts it by sending; an orchestration worker or agent.launch needs a running agent, so its create still answers the start's refusal with the reason after one spawn. Also types the RPC fixture's forwarding create.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the delta since the prior pullfrog review (197f72a on top of 0e8477f):
- Routed the create answer by caller.
commitStructuredAgentSessionCreatenow takesanswer: 'conversation' | 'running-agent': the chatagentSession.createroute passes'conversation', so a proven-exit failed start still answers a readable chat, whilecreateStructuredAgentSessionForWorktreehardcodes'running-agent'so orchestration workers andagent.launchkeep the start's refusal (with reason) after one spawn. - Covered the worker path with a real-host test.
orchestration-structured-worker-failed-start.test.tsproves a worker whose agent dies starting is refused with the reason, spawns once, publishes no tab, and leaves no session tab. It would fail against the priorhost.createpath, so it is genuine coverage rather than a tautology. - Cleared the typecheck failure from the prior review. The test-fixture
hostStubnow binds a well-typed localattachmock and forwardscreateto it, and the precommit-refusal stub returns{ attach, create: attach }.
I re-ran pnpm tc:node (green), oxlint on the changed files (0 warnings/0 errors), and the relevant suites — structured-agent-session.test.ts (58), NativeChatStructuredSession.launch-lifecycle.test.tsx (11), structured-agent-session-adopted-import.test.ts (10), orchestration-structured-worker-session.test.ts (12), structured-agent-session-failed-create-readable.test.ts (4), structured-agent-session-precommit-refusal.test.ts (9), and the two worker/start-failure tests (1 each) — all pass.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Closing without merging. It's superseded by the native-chat redesign: a failed start becoming a readable chat, and one restart path, are handled by #22821 (accept then deliver) and a later PR where creating a chat founds the conversation without starting the agent. |
1 participant
ELI5
When a new chat's agent (Claude or Codex) crashed while starting, Orca used to have two separate ways to try again, depending on exactly when it crashed. Now a chat whose agent failed to start is just an ordinary chat that says why. Sending a message restarts the agent the same way it does for any chat whose agent stopped.
What Changed
Before, for the user: if the agent died while Orca was still starting it (for example, the CLI isn't signed in), the chat showed "Chat could not be started. " with a Retry button. Sending a message quietly re-ran the whole "create chat" step. If the agent died a moment later, after the chat had opened, you got a normal chat with a status row, and sending a message restarted the agent through a different mechanism on the host. Same failure, two different screens and two restart paths depending on timing.
After, for the user: in both cases you get the chat with the row "The provider stopped before it finished starting: ." Sending a message restarts the agent on the host and delivers it. If it fails again (still signed out), the send is refused with the cause, and the next send tries again. Mobile gets the same chat. Before, it treated this failure as "unknown".
Mechanism:
structured-agent-session-conversation-founding.ts). Before, the journal was opened only after the agent process was up. A create whose agent died therefore left a session record with nothing to read, nothing to publish as a tab, and nothing a send could be admitted into.host.createruns the attach. When the attach is refused and the host proved the agent process is gone, it writes the startup-failure row, keyed by the create's operation id so a replay adds no second row. It then answers the readable chat, and the create route publishes its tab as usual. The row writer is the one the send path already used, moved out ofsend-preparationso both callers share it.sendThroughRelaunchinuse-native-chat-provisional-launch.tsand its use inNativeChatStructuredSession.tsx). The host's ensure-owner step insendis the only thing that restarts a chat now.What still answers a refusal: a definitive refusal (
structured_agent_session_unsupported), so the client still falls back to a terminal, and a failure where the host could not prove the process gone, so the client still reconciles instead of assuming.Why
Only one mechanism should restart a chat, and it should sit where the facts are: the execution host, which knows whether a process is alive. The old renderer path existed only because a failed create left nothing readable. Fixing that removes the reason for the second path, so it is deleted rather than kept alongside.
Alternatives considered:
Differences from the common pattern. In the usual design, the conversation (thread) is a record that exists independently of the agent process. Every turn start makes sure a process exists, the first turn included, and a failed start lands as an error entry in the conversation. This PR matches that ownership (the journal exists from reservation, independent of the child) and that delivery (a send goes through the one ensure-owner step). It deviates in lifecycle: Orca still starts the agent eagerly at create, and an open chat still takes a hold that starts one. So after a failed create, opening the chat makes one more start attempt before the user sends anything (one extra short-lived process for a persistent failure such as "not signed in"). That is covered by a follow-up that makes a chat surface's hold leave a failed start for the next send to restart; this PR adds no hold logic. A second design keeps "start" (which carries the first message) separate from "submit". Orca's create doesn't carry the first message, so this follows the first design.
Remote / mixed versions. No wire field was added or removed. What changed is content on an existing reply: a create that used to be refused now answers
okwith a chat.createas a member that forwards toattach, so both builds still reach the host throughattach.Other create callers: a chat surface gets a readable conversation, and a worker launch needs a running agent. So the chat
agentSession.createroute answers throughhost.create, while orchestration workers andagent.launch(the in-process create path) still answer throughhost.attachand keep the start's refusal, reason and all, after a single spawn.Known and left for later: a chat launched with a prompt whose agent keeps failing shows two rows: the startup row, then "couldn't restart: " from the prompt's send.
Persisted state: no new record field. The only new durable thing is a journal file created earlier in a session's life (at reservation, before the spawn). Journals are never enumerated for listings: history lists provider transcripts, tabs come from the host's open sessions, and startup restore uses the persisted visible-tab index. So a failed create that the user closes doesn't show up anywhere as a phantom chat.
Linked Issue
Follow-up to #22364. No separate issue.
Visual Proof
N/A: no layout change. The failed-start chat reuses the existing status row. Covered by host tests at the reply boundary.
Testing
New
structured-agent-session-failed-create-readable.test.ts(real host, real record store and journal):okwith the startup row and a published tab. A replay of the same operation answers the same chat with no second row. A send at the returned fence restarts the agent and is admitted.agent_session_owner_restart_failedwith the cause, and a send after signing in restarts and delivers.orchestration-structured-worker-failed-start.test.ts(real host): a worker whose agent fails to start is still refused with "…for this worker was refused: ", after one spawn, with no tab published.structured-agent-session-adopted-import.test.ts: an adopted history stays readable after the first start fails. An import write failure now fails before any spawn.NativeChatStructuredSession.launch-lifecycle.test.tsx: a send into a failed launch does not relaunch. It waits in the outbox and is delivered once launch Retry publishes.structured-agent-session-launch-exited-owner.test.tsstill covers the old-host refusal shape and Retry.Ablations against the committed tree: sending every create through
host.createreddens the worker test (it gets the bare codeagent_session_operation_invalid). Removing the founding call reddens the two failed-create tests and the adopt tests. Removing the create's failed-start answer reddens the two failed-create tests. The pre-change renderer reddens the new lifecycle test (relaunch called on send).tests/e2e/cross-version-wire/cross-version-agent-session-wire.unit.test.tspasses. Broad sweep ofsrc/main/native-chat,src/main/runtime/rpc/methods, structured runtime tests and the renderer native-chat/launch tests passes. The only failures under full parallel load were timeouts that pass when run alone.tsc --noEmit -p config/tsconfig.node.jsonpasses.Not run: the mobile suite (no mobile code changed) and Electron UI validation.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Author X handle: @BrennanKB5
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)