Repository navigation
fix: keep chat Stop and Close independent of receipt bookkeeping - #26461
brennanb2025 wants to merge 17 commits into
Conversation
… pre-turn A Stop that names its target can never run work twice, so it no longer expires with delivery receipts; a Stop for a background task that already ended answers as a no-op without needing a live provider; and the phone can stop a pending submission before its turn opens.
Keep one target observation through admission and provider dispatch. Require execution ownership for unavailable roster recovery, surface unconfirmed Stops as failures, and preserve ended-target retries after release and restart.
…-stop-close-never-gated # Conflicts: # src/main/runtime/rpc/methods/agent-launch-restart-replay.test.ts # src/shared/agent-session-operation-ledger.test.ts # src/shared/agent-session-operation-ledger.ts
A Stop captured before a turn opens could be discarded before its user echo linked the submission. Resolve the submission through the execution host's existing dispatch binding and interrupt only that admitted turn. Report the existing unknown-outcome failure when identity or cancellation is unconfirmed; keep ended and superseded targets quiet so retries cannot stop later work. Add real-host and production-adapter regressions for the opening/echo race, admitted turns, missing binding, ended targets, and later-turn protection.
Closing a chat settled its waiting messages and wrote the queue's reopen mark before it stopped the agent, so a slow or pending journal write kept the agent running until that bookkeeping finished. Close now stops the agent first and settles the waiting messages after, even when the stop fails. Nothing queued can be handed over in between: every send and start waits behind the Close step, and a settlement that fails after a person's close is redone before the next start. The stop's check for work being cut short ignores queued sends during a close, so Close records the same Stop row as before. Unsent messages still become cards that wait for the chat's next turn.
…-stop-close-never-gated # Conflicts: # mobile/src/session/use-mobile-structured-agent-session-send.test.tsx # src/main/native-chat/agent-session-wire/structured-agent-session-mutation-plans.ts # src/renderer/src/components/native-chat/use-structured-agent-session.ts
A Stop naming a waiting send became a silent no-op when a later send was queued first, because the send counted as live only while it was the newest unanswered one. The host now reads the named send itself: still waiting means live, a Stop recorded after it reached the agent or an answer with no turn running means ended. Clients never name a queued send. A card's Cancel again settles the card when the turn its Stop named already ended; the target bounds only the Stop route.
A targeted Stop treated any recorded Stop after the send reached the agent as proof the send had ended, including a Stop whose interrupt and child end both failed. A repeat Stop then did nothing and answered success, and the failed Stop itself answered success instead of the unconfirmed failure. The Stop now counts only once it took effect: settled, and not failed, the same settle state that keeps the chat reading Stopping.
Review summary (final review, head a9a1bd7)Problem: on the SSH server, pressing Stop or closing a chat tab could fail to stop the agent. After enough chat actions, Stop could be refused because the server's saved request records filled up. Close could leave the agent running when saving the hidden-tab state failed. And a Stop pressed right as a message was handed to the agent could report success while the agent kept working. User-facing change:
How: a desktop or phone that supports it names the host-issued turn, message or task it wants stopped. The SSH server checks that against its own state: live work is interrupted, work that has positively ended is a quiet no-op, and anything it can't verify is retried or reported. Older clients and older servers keep the previous request shape. Review rounds: eight independent rounds. Round 7 found that a Stop that failed was itself being counted as proof its message had ended, so the failed Stop answered "ok" and every later Stop of that message did nothing. Round 7's fix counts a Stop only once it finished without failing.
Live SSH QA (integration build with all the SSH chat fixes, Docker SSH host, stand-in Claude): Stop mid-reply stopped the agent in about 2 s and showed "Interrupted after 13s"; closing the tab mid-reply stopped it in under 2 s; Stop pressed 27 ms after sending, before any reply text, still stopped it. Screenshots go in the PR body before this is marked ready. Known gaps (in the PR body): unbounded growth of recorded requests comes from main #26543 and needs its own follow-up; older clients' unnamed Stop keeps the best-effort path; Close stopping the agent for everyone watching is an open product decision. Not verified: WSL, native Windows or Linux hosts, a real Claude login, the rendered phone, and mixed old/new desktop and server pairs end to end (covered by compatibility tests only). |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to Two narrow Stop edge cases remain: a reused operation ID can let a targeted Stop cancel work despite a conflict, and a delayed turn Stop can interrupt later work. The test-runner change is correctly routed; merge with awareness of these remaining chat risks. Pre-merge checks |
|
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
2a9651e3-7c25-42fc-881b-9aecaabfe06e
📒 Files selected for processing (62)
mobile/src/session/mobile-structured-agent-session-cancel.tsmobile/src/session/mobile-structured-agent-session-host-support.test.tsmobile/src/session/mobile-structured-agent-session-host-support.tsmobile/src/session/mobile-structured-agent-session-stop-target.test.tsmobile/src/session/mobile-structured-operation-id-retirement.test.tsmobile/src/session/use-mobile-structured-agent-session-background-tasks.test.tsxmobile/src/session/use-mobile-structured-agent-session-send.test.tsxmobile/src/session/use-mobile-structured-agent-session.tsmobile/src/session/use-mobile-structured-background-tasks.tssrc/main/codex/codex-structured-dispatch-echo.test.tssrc/main/codex/codex-structured-dispatch-echo.tssrc/main/codex/codex-structured-session-adapter.tssrc/main/codex/codex-structured-submission-turn.tssrc/main/native-chat/agent-session-journal/journal-stop-marks.tssrc/main/native-chat/agent-session-journal/queued-message-pause.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adapter.tssrc/main/native-chat/agent-session-wire/structured-agent-session-background-target-lifetime.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-background-target-unverifiable.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-cancel-plan.tssrc/main/native-chat/agent-session-wire/structured-agent-session-chat-stop.tssrc/main/native-chat/agent-session-wire/structured-agent-session-close-storage.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-codex-submission-stop.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-conversation-lifetime.tssrc/main/native-chat/agent-session-wire/structured-agent-session-failed-stop-repeat.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-mutations.tssrc/main/native-chat/agent-session-wire/structured-agent-session-host-stop-event.tssrc/main/native-chat/agent-session-wire/structured-agent-session-mutation-admission.tssrc/main/native-chat/agent-session-wire/structured-agent-session-mutation-admits-now.tssrc/main/native-chat/agent-session-wire/structured-agent-session-mutation-plans.tssrc/main/native-chat/agent-session-wire/structured-agent-session-prompt-cancel-ended-target.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-message-rig.test-fixture.tssrc/main/native-chat/agent-session-wire/structured-agent-session-queued-mutations.tssrc/main/native-chat/agent-session-wire/structured-agent-session-status-feed.tssrc/main/native-chat/agent-session-wire/structured-agent-session-stop-capacity.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-stop-target-observation.tssrc/main/native-chat/agent-session-wire/structured-agent-session-stopping.tssrc/main/native-chat/agent-session-wire/structured-agent-session-submission-stop-opening.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-target-expiry.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-targeted-stop.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-turns-cancel.tssrc/main/native-chat/agent-session-wire/structured-agent-session-work-receipt-window.test.tssrc/main/runtime/agent-session-operation-admission.tssrc/main/runtime/agent-session-operation-retention.test.tssrc/main/runtime/rpc/methods/agent-launch-restart-replay.test.tssrc/main/runtime/rpc/methods/structured-agent-session-close.tssrc/main/runtime/rpc/methods/structured-agent-session.test.tssrc/main/runtime/rpc/methods/structured-agent-session.tssrc/renderer/src/components/native-chat/use-structured-agent-session-cancel.tssrc/renderer/src/components/native-chat/use-structured-agent-session-stop.test.tsxsrc/renderer/src/components/native-chat/use-structured-agent-session-stop.tssrc/renderer/src/components/native-chat/use-structured-agent-session.tssrc/shared/agent-child-work-invocation.tssrc/shared/agent-session-background-stop-target.tssrc/shared/agent-session-operation-ledger.tssrc/shared/agent-session-stop-capabilities.tssrc/shared/agent-session-stop-target.test.tssrc/shared/agent-session-stop-target.tssrc/shared/agent-status-child-work.tssrc/shared/rpc-contract/agent-session-stop-target-params.test.tssrc/shared/rpc-contract/agent-session-stop-target-params.tssrc/shared/rpc-contract/structured-agent-session-params.ts
💤 Files with no reviewable changes (1)
- src/main/native-chat/agent-session-wire/structured-agent-session-queued-mutations.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| if (target.kind !== 'submission') { | ||
| return agentSessionStopTargetIsLive(target, activeTurnId, submissions, ctx.fence) | ||
| ? { verdict: 'live' } | ||
| : { verdict: 'exited' } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return the target's turn ID for a live turn target, so the host runs the Stop as a named Stop.
For a turn target, the observation returns { verdict: 'live' } with no turnId. The desktop Stop hook (use-structured-agent-session-stop.ts, Lines 60-66) sends only { stopTarget } and omits the top-level turnId. As a result, structured-agent-session-host-mutations.ts Line 159 calls stop(undefined), and the host runs an unnamed "stop whatever is in flight" Stop.
mutateWithChatStop awaits several steps between this observation and the interrupt, such as the queued-withdrawal and the Stop event append. cancelTurn then gets no turnId and reads the live turn through resolveLiveTurnId when it sends the interrupt. Suppose the target turn ends and a queued or later send opens a new turn inside that window. The interrupt then reaches the new turn. The target was meant to prevent exactly this case.
The submission branch already returns turnId: activeTurnId for this reason. Mobile avoids the window because it also sends the top-level turnId.
Proposed fix
--- "a/src/main/native-chat/agent-session-wire/structured-agent-session-stop-target-observation.ts"
+++ "b/src/main/native-chat/agent-session-wire/structured-agent-session-stop-target-observation.ts"
@@ -22,11 +22,12 @@
): StopTargetObservation {
const activeTurnId = ctx.journal.activeTurnId()
const submissions = ctx.journal.submissions()
if (target.kind !== 'submission') {
- return agentSessionStopTargetIsLive(target, activeTurnId, submissions, ctx.fence)
- ? { verdict: 'live' }
- : { verdict: 'exited' }
+ if (!agentSessionStopTargetIsLive(target, activeTurnId, submissions, ctx.fence)) {
+ return { verdict: 'exited' }
+ }
+ return target.kind === 'turn' ? { verdict: 'live', turnId: target.turnId } : { verdict: 'live' }
}
const submission = submissions.find((entry) => entry.clientMessageId === target.clientMessageId)
if (!submission) {
return { verdict: 'unverifiable' }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (target.kind !== 'submission') { | |
| return agentSessionStopTargetIsLive(target, activeTurnId, submissions, ctx.fence) | |
| ? { verdict: 'live' } | |
| : { verdict: 'exited' } | |
| } | |
| if (target.kind !== 'submission') { | |
| if (!agentSessionStopTargetIsLive(target, activeTurnId, submissions, ctx.fence)) { | |
| return { verdict: 'exited' } | |
| } | |
| return target.kind === 'turn' ? { verdict: 'live', turnId: target.turnId } : { verdict: 'live' } | |
| } |
| if (args.skipReceipt) { | ||
| return { rows, decision: evaluateAgentSessionOperation({ ...args, rows: new Map() }) } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Check existing receipts before admitting a receipt-free Stop.
If a targeted Stop reuses a clientOperationId recorded for a Send, the empty map hides that receipt. The host can then admit the Stop and cancel a live target instead of refusing the fingerprint conflict. Keep the existing-row replay and conflict checks; skip only creation of the new receipt.
…-stop-close-never-gated # Conflicts: # src/main/native-chat/agent-session-wire/structured-agent-session-turns-cancel.ts # src/renderer/src/components/native-chat/use-structured-agent-session.ts
Review summary addendum (final main sync, head 0cf04b5)What changed since the last review: a sync with main (151 commits) and one CI fix.
Independent review of the merge: no P0–P2.
Optional test and comment tidy-ups (not fixed):
CI is green and the PR merges cleanly. |
|
Replaced by #28243, which keeps main's untargeted Stop (the common pattern) and fixes the remaining phone and background-task gaps in about 30 lines. |
ELI5
Stop and Close have to work every time. Over an SSH host they could fail in a few ways: Stop could be refused after a heavy day of chat actions, a Stop pressed just as a turn started (or while the agent's background task list was briefly unreadable) could report success without stopping anything, and Close could leave the agent running if saving the hidden-tab state failed. Main's #26543 has since removed the request limit that caused the refusal; this PR fixes the rest.
What Changed
Why
Naming the work removes the need for Stop to depend on request bookkeeping, which is what made it refusable. Checking the name against the execution host's own state keeps the stop correct across reconnects and retries, without a saved "Stop still owed" flag.
The common pattern has the execution host resolve which work it owns before acting on a control request; this PR matches that part. Differences:
The stop target is only sent to a server that advertises
agent-session.targeted-stop.v1, and older servers get their existing request shape. No new stream message type.Linked Issue
Coordinator's structured SSH chat fix plan, U21 and U23. No separate public issue.
Visual Proof
The controls, layout and copy are unchanged. Live run on a managed SSH host (Docker Linux container, desktop on macOS, a credential-free stand-in Claude), with this PR combined with the other SSH chat fixes:
Testing
On macOS, each review round's failure was reproduced as a regression that failed before its fix: the Stop pressed while a message is starting (with the real Codex adapter), the unreadable background-task list, ended targets after the agent was released or the server restarted, and Close with a failed or stuck tab-hide save. After the final sync with main (which brought in #26780's background-task Stop and #26672's Send replay), the PR's 18 desktop test files (194 tests), 10 neighbouring main test files and its 5 mobile test files pass; the node, web and mobile typechecks pass; and CI is green.
orca-ci-checkspasses except main's known mobile import-cycle warnings in files this PR doesn't touch.Live managed-SSH run (desktop on macOS, Linux container host, stand-in Claude, combined with the other SSH chat fixes): Stop mid-reply, Stop 31 ms after sending, and closing the tab mid-reply all stopped the agent; see Visual Proof. Not run: WSL, native Windows or Linux desktops, the phone, a real Claude login, or old/new client and server pairs end to end (covered by compatibility tests).
Targeted Stop follow-up
The problem. Stop could report success without stopping anything in two moments:
In both cases the agent kept working on the message, and the Stop button reported success.
What changes for you. Pressing Stop while a message is waiting to start now stops it, whatever arrives after the press. If the server can't confirm what happened, Stop reports it as not confirmed instead of quietly succeeding. A Stop that is retried after the reply was lost still never stops a later message or a later turn. A permission or question card's Cancel always settles that card, even when the turn you were looking at ended just before the server processed it.
How it works.
Other notes.
Request limits. The limits come from main's #26543. This PR adopts that policy and no longer changes operation limits: no request is refused for having too many recorded actions, in flight or settled. Delivery-ID validation, duplicate replay and conflict checks stay as they are on main; since main's #26672 a retried Send replays from its own receipt instead of expiring, and this PR follows that.
Testing. The tests run the production server code over scripted agent traffic, including the real Codex adapter. They cover:
The live managed-SSH run above covers Stop and Close on the desktop; WSL, Windows and the phone were not run live.
Known difference, temporary. On a phone, Stop pressed while the only waiting message is still held by the server (for example while the agent starts) shows "Stop not sent", the same as main. The desktop stops it. This follows main's current behavior and is tracked with the uncertain-send follow-up (U10/U22), which decides whether the phone may send a plain Stop there.
Review
Draft; independent reviews are run on every round, and the final one runs on the final head. Known gaps, stated plainly:
Agent skill upstream boundary
Notes
The execution host owns the behavior for local, SSH, WSL, folder, and Git workspaces. No platform-specific process handling or path behavior was added. Fingerprint and authority checks remain. There are no capacity limits or capacity exemptions in this PR; request limits are main's #26543 policy.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)