Repository navigation
Revert "fix(native-chat): Claude, Pi and ACP chats are Working only while the agent holds the send" (#30134) - #30572
Conversation
…hile the agent holds the send (#30134)" This reverts commit fc925c3 until the real fix for STA-11160 lands. With it, `orca orchestration worker-start --agent claude` shuts a new Claude chat worker down right after Claude acknowledges its task: between the echo and the accepted row the send reads as no longer held, settlement reports it unknown, and worker-start tears the session down. Conflicts with #29150 were formatting only; resolved to the pre-#30134 code.
📝 Walkthrough
Priority: ⬆️ High Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 32.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 34 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the primary change as reverting #30134and is specific to the pull request.Description check The description is detailed and covers the user impact, mechanism, rationale, scope, linked tracking item, visual proof, testing, and remaining risk. Some checklist items and the formal issue-link fie… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is detailed and covers the user impact, mechanism, rationale, scope, linked tracking item, visual proof, testing, and remaining risk. Some checklist items and the formal issue-link field are not completed, but the description is otherwise substantially complete.
-
- 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
ce69e156-d096-4868-98f8-d52928dfe878
📒 Files selected for processing: 46
src/main/acp/acp-structured-acquire.tssrc/main/acp/acp-structured-host-lifecycle.test.tssrc/main/acp/acp-structured-host.test-support.tssrc/main/acp/acp-structured-session-adapter.tssrc/main/acp/acp-structured-session.tssrc/main/acp/acp-structured-turns.tssrc/main/claude/claude-command-lifecycle.test.tssrc/main/claude/claude-command-lifecycle.tssrc/main/claude/claude-structured-session-acquisition.tssrc/main/claude/claude-structured-session-adapter.tssrc/main/claude/claude-structured-session-close.tssrc/main/claude/claude-structured-session-exit-lifecycle.tssrc/main/claude/claude-structured-session-state.tssrc/main/codex/codex-structured-session-acquire.tssrc/main/codex/codex-structured-session-close.tssrc/main/codex/codex-structured-session-state.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adapter.tssrc/main/native-chat/agent-session-wire/structured-agent-session-attach-orchestration.tssrc/main/native-chat/agent-session-wire/structured-agent-session-child-exit.tssrc/main/native-chat/agent-session-wire/structured-agent-session-claude-stop-exit-ends-record.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-event-recovery.tssrc/main/native-chat/agent-session-wire/structured-agent-session-event-sink.tssrc/main/native-chat/agent-session-wire/structured-agent-session-exit-observation.tssrc/main/native-chat/agent-session-wire/structured-agent-session-handing-over.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-mutations.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-types.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host.tssrc/main/native-chat/agent-session-wire/structured-agent-session-late-dispatch.tssrc/main/native-chat/agent-session-wire/structured-agent-session-mutation-context.tssrc/main/native-chat/agent-session-wire/structured-agent-session-surface-lifetime.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-unanswered-dispatch-release.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-unanswered-dispatch-release.tssrc/main/native-chat/agent-session-wire/structured-agent-session-unexpected-exit.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-unfinished-work.tssrc/main/pi/rpc-configured-default.test.tssrc/main/pi/rpc-location.test.tssrc/main/pi/rpc-runtime-registration.tssrc/main/pi/rpc-session-adapter.test.tssrc/main/pi/rpc-session-adapter.tssrc/main/pi/rpc-session.tssrc/main/runtime/claude-structured-idle-releases-unknown.test.tssrc/main/runtime/structured-agent-runtime-registrations.tssrc/main/runtime/structured-agent-session-codex-held-sends.test.tssrc/main/runtime/structured-agent-session-dispatch-followups.tssrc/main/runtime/structured-agent-session-failure-logging.test.tssrc/main/runtime/structured-claude-runtime-adapter.ts
💤 Files with no reviewable changes: 12
- src/main/native-chat/agent-session-wire/structured-agent-session-mutation-context.ts
- src/main/acp/acp-structured-host-lifecycle.test.ts
- src/main/acp/acp-structured-turns.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-host-types.ts
- src/main/runtime/structured-agent-session-codex-held-sends.test.ts
- src/main/acp/acp-structured-session-adapter.ts
- src/main/codex/codex-structured-session-close.ts
- src/main/codex/codex-structured-session-state.ts
- src/main/acp/acp-structured-session.ts
- src/main/native-chat/agent-session-wire/structured-agent-session-exit-observation.ts
- src/main/claude/claude-command-lifecycle.ts
- src/main/codex/codex-structured-session-acquire.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.
| .catch((error: unknown) => | ||
| state.log.logger.warn('late settlement failed', { scope: 'late-settlement', error }) | ||
| ), | ||
| onDispatchSettledLate: (settlement) => void hosted.host?.settleLateDispatch(settlement), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle rejected late settlements in the test rig.
If settleLateDispatch rejects during a journal-failure test, this callback leaves an unhandled rejection. Vitest can then fail the test for the rejection rather than report it through the rig’s logger. Restore a rejection handler on the returned promise.
| fence: exit.session.fence, | ||
| acquisitionGeneration: exit.session.acquisitionGeneration, | ||
| observedAt: exit.observedAt | ||
| observedAt: deps.now?.() ?? Date.now() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the time when Claude’s exit was observed.
If persistClaudeSessionHandle is slow, this clock call timestamps the exit after persistence finishes. endExitedStructuredAgentSessionChildUnderSerialize uses that timestamp to complete interrupted turns, so the journal records a later end than the observed exit. Capture the timestamp when the exit callback runs and carry it into this event.
| attempt.candidate = { | ||
| sink: eventSink, | ||
| child: (owner === 'kept' && current) || { | ||
| child: { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Inspect kept-owner selection and close-state transitions without running repository code.
ast-grep outline src/main/native-chat/agent-session-wire/structured-agent-session-attach-orchestration.ts --items all --match 'runAttachUnderAbort|performAttach'
rg -n -C 5 'owner === .acquired.|owner: .kept.|child\.close|close\.recorded|performAttach\(' src/main/native-chat/agent-session-wireRepository: stablyai/orca
Length of output: 42034
🏁 Script executed:
set -eu
printf '%s\n' '--- attach orchestration ---'
nl -ba src/main/native-chat/agent-session-wire/structured-agent-session-attach-orchestration.ts | sed -n '1,285p'
printf '%s\n' '--- attach flow ---'
nl -ba src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.ts | sed -n '1,300p'
printf '%s\n' '--- close-related definitions and transitions ---'
rg -n -C 10 --glob '*.ts' -e 'function .*close' -e 'child\.close\s*=' -e 'close:\s*{' -e 'recorded' src/main/native-chat/agent-session-wire src/main/native-chat | sed -n '1,360p'Repository: stablyai/orca
Length of output: 30702
🏁 Script executed:
set -eu
nl -ba src/main/native-chat/agent-session-wire/structured-agent-session-attach-orchestration.ts | sed -n '110,270p'
nl -ba src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.ts | sed -n '90,250p'
rg -n -C 8 --glob '*.ts' -e 'child\.close' -e 'recorded' -e 'owner === .kept.' -e 'owner: .kept.' src/main/native-chat/agent-session-wireRepository: stablyai/orca
Length of output: 45665
🏁 Script executed:
set -eu
printf '%s\n' '--- close module ---'
nl -ba src/main/native-chat/agent-session-wire/structured-agent-session-child-close.ts | sed -n '1,220p'
printf '%s\n' '--- host lifetime close references ---'
rg -n -C 12 -F -- 'child.close' src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts src/main/native-chat/agent-session-wire/structured-agent-session-host-mutations.ts src/main/native-chat/agent-session-wire
printf '%s\n' '--- close entrypoints and serialization ---'
rg -n -C 10 --glob '*.ts' -e 'request.*close' -e 'close.*child' -e 'closeChild' -e 'runClose' -e 'serialize\(.*sessionId' src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts src/main/native-chat/agent-session-wire/structured-agent-session-host-mutations.ts src/main/native-chat/agent-session-wireRepository: stablyai/orca
Length of output: 45667
🏁 Script executed:
set -eu
rg -n -C 12 -F -- 'child.close' src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts src/main/native-chat/agent-session-wire/structured-agent-session-host-mutations.ts
rg -n -C 12 --glob '*.ts' -e 'closeChild' -e 'requestClose' -e 'runClose' -e 'serialize\(sessionId' src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts src/main/native-chat/agent-session-wire/structured-agent-session-host-mutations.ts src/main/native-chat/agent-session-wire/structured-agent-session-child-close.tsRepository: stablyai/orca
Length of output: 14866
🏁 Script executed:
set -eu
rg -n -C 8 -F -- 'stopStructuredAgentSessionAgentUnderSerialize' src/main/native-chat/agent-session-wire
rg -n -C 8 -F -- 'attachStructuredAgentSession(' src/main/native-chat/agent-session-wire
rg -n -C 8 -F -- 'attachStructuredAgentSessionUnderSerialize' src/main/native-chat/agent-session-wire
rg -n -C 8 -e 'serialize\s*[:(]' src/main/native-chat/agent-session-wire/structured-agent-session-host.ts src/main/native-chat/agent-session-wire/structured-agent-session-host-*.ts src/main/native-chat/agent-session-wire 2>/dev/null | sed -n '1,260p'Repository: stablyai/orca
Length of output: 42251
🏁 Script executed:
set -eu
nl -ba src/main/native-chat/agent-session-wire/structured-agent-session-host-lifetime.ts | sed -n '132,225p'
nl -ba src/main/native-chat/agent-session-wire/structured-agent-session-child-exit.ts | sed -n '1,180p'
nl -ba src/main/native-chat/agent-session-wire/structured-agent-session-provider-exit-proof.ts | sed -n '1,180p'Repository: stablyai/orca
Length of output: 18734
Preserve close state when an attach keeps the child.
A close that cannot prove the provider exit throws while the provider close continues. After the serializer is released, a kept-owner attach can replace the child and drop current.close. The exit handler then defaults the cause to evict and omits the requested timestamp.
🐛 Suggested fix
--- "a/src/main/native-chat/agent-session-wire/structured-agent-session-attach-orchestration.ts"
+++ "b/src/main/native-chat/agent-session-wire/structured-agent-session-attach-orchestration.ts"
@@ -247,15 +247,18 @@
)
attempt.candidate = {
sink: eventSink,
- child: {
+ child:
+ owner === 'kept' && current
+ ? current
+ : {
generation: acquisitionGeneration ?? current?.generation ?? null,
fence,
// An acquire reports its phase; a re-attach keeps what its child already proved.
phase: owner === 'acquired' ? providerChildPhase : (current?.phase ?? 'ready'),
...(startedFor === undefined ? {} : { startedFor }),
...(permissionMode === undefined ? {} : { permissionMode }),
heldSends: () => context.deps.adapter.heldSends?.(sessionId)
- }
+ }
}
// Only a proven child has the protocol session recovery asks; a starting one recovers in
// its `started` step, before it is handed anything.
Review loop status (checked after merge)This PR merged at 05:40 UTC, while the review loop was still running. The review and QA below ran on the merged head ( The problem. Since #30134, What changes for users. Claude chat workers started by worker-start stay up and complete their turns again. Pi, ACP and Codex workers are unchanged. As agreed, the bug #30134 fixed comes back: a Claude, Pi or ACP chat can keep showing "Working" after the agent finished, when saving the agent's answer about a message failed. That matches v1.4.229. What the review found (one round; no code changes were needed):
What was verified
Not verified: Windows and Linux (the change has no platform-specific code); Pi and ACP workers in a live run (covered by tests only). For Brennan's review
|
…ity, held-send revert Sleep (#28445) closes a workspace's chats through the same abort a close gives a registered start, re-aborts a start registered while it waited for the lane, and judges its receipt once the start it aborted has settled, so only a newer start reads as later work. Each adapter's owned-acquisition check covers the child it owns between acquire and open. Main's Sleep cases from the deleted surface lifetime suite move to the workspace Sleep suite, plus one for a starting agent. Codex (#30406): the split open sends the chat's command env overrides when it opens the thread; the adapter test keeps that assertion on the open's request. Revert of #30134 (#30572): the event sink, exit observation, Pi held-send wrapper and adapter heldSends overrides go with it; Pi dispatch submits through its turns again and its idle report is wired back.
… shutting down Claude chat workers (#30577) * fix(native-chat): a Claude chat worker keeps running after Claude acknowledges its task Claude's adapter dropped an echoed send from the sends it holds before reporting the send's answer to the host, and emitting the echo's turn can publish at once. That publish read the still-pending send as held by nobody, so it read as "couldn't confirm": orchestration worker-start then closed the new Claude worker, and every other answer wait and client saw a one-frame doubt. A frame's settled sends now stay held until their answers are reported, when the host's handover window takes them over; a Stop's rejected sends are released and reported one at a time, as Pi and ACP already do. Tracked in STA-11160. * test(native-chat): assert the settled send's dispatch state through the send result * Reapply "fix(native-chat): Claude, Pi and ACP chats are Working only while the agent holds the send (#30134)" (#30572) This reverts commit 3ef5a46. * fix(native-chat): a Claude send stays held until its settlement reaches the host, on the reapplied held-sends read * fix(native-chat): a Claude send stays Working, not in doubt, while a Stop is still ending its CLI * test(pi): drop the removed idle-release dependency from the adapter fixture
1 participant
ELI5
Since #30134 landed, Orca immediately shuts down any Claude chat it starts as an orchestration worker. The worker either never answers, or answers once and then ignores everything after. This PR puts back the code from before #30134 until the proper fix is ready.
What Changed
Before (main since #30134):
orca orchestration worker-start --agent claudeopens a Claude chat, sends the task, and as soon as Claude acknowledges it, Orca decides the task was never received and closes the chat. The dispatch endsstart_unknownwith "The dispatch preamble was submitted but not acknowledged (unknown): provider_ended_before_acknowledgement", and the session record says "closed by Orca". In Orca's own logs on Brennan's Mac, 90 of 92 Claude chat workers started since 10-18 17:00 UTC ended this way. Codex workers were unaffected.After: Claude, Pi and ACP chats go back to how they reported in-flight messages before #30134, which is also how v1.4.229 behaves. A worker's first message waits until Claude accepts it, and the worker keeps running.
Mechanism: #30134 made Claude, Pi and ACP chats report which messages the agent is still holding. For Claude, the hold is released when Claude echoes the message, a moment before Orca writes the "accepted" record. In that gap the send-settlement check (
publishedSubmission, added by #29922) sees a message that is neither held nor accepted and reports it as unknown.worker-starttreats that as a failed start and evicts the session, killing Claude mid-turn.What comes back with the revert: the bug #30134 fixed. A Claude, Pi or ACP chat can keep showing "Working" after the agent finished, when Orca received the agent's answer about a message but failed to save it. This is the same behavior as v1.4.229.
Also coming back, as part of the same whole revert: a Codex chat whose prompt a hook blocked, and that later crashes while idle, shows a false "response was in progress / exited unexpectedly" error. That is main's behavior before #30134 (and the 1.4.230 release branch's), not v1.4.229's. A few smaller pieces of #30134 go too, all matching v1.4.229: an interrupted turn's end time is recorded when shutdown finishes rather than when the exit was seen. The fix in STA-11160 restores #30134's behavior without the worker-start bug.
Why
This is a stopgap so main, and any release cut from main today, stops shipping broken Claude orchestration workers. The real fix is in progress: it closes the gap between "no longer held" and "accepted" for every agent instead of removing the held-send reporting. Tracked in STA-11160.
Revert conflicts with #29150 were formatting only (it reformatted lines #30134 had added); they are resolved to the pre-#30134 code, formatted with the current formatter. No other commit on main uses anything #30134 added.
Linked Issue
Tracked in STA-11160 (Linear). Reverts #30134.
Visual Proof
Built from source and run as isolated test copies on a macOS QA machine. A Claude chat worker started with
orca orchestration worker-start --agent claude, told to reply TURN1 OK, then TURN2 OK after a follow-up message.start_unknown(provider_ended_before_acknowledgement), session closed by Orca, follow-up never deliveredFirst turn runs a tool (Claude), and a Codex worker, on this PR:
A user-typed Claude chat on this PR: Working during a turn, Done after, and Interrupted after Stop.
Testing
/electronQA from this PR's source on macOS: Claude worker two turns, Claude worker whose first turn runs a tool, Codex worker two turns, a typed Claude chat's Working/Done/Interrupted, launch, Settings, terminal, quit and relaunch all pass. The same Claude worker on main fails as described above.Review
Agent skill upstream boundary
Notes
Remaining risk, not introduced here: a narrow Codex-only version of the same race still exists from #29922 (a Codex echo while a journal write is already queued). It has not been observed; the real fix covers it.