fix(cli): finish stopped steers before releasing session ownership - #471
fix(cli): finish stopped steers before releasing session ownership#471Astro-Han wants to merge 2 commits into
Conversation
0b1762c to
a0862e0
Compare
a0862e0 to
b670377
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
wibus-wee
left a comment
There was a problem hiding this comment.
Hi @Astro-Han! I found two correctness gaps, plus one related design concern.
First, I think Stop can currently erase a definitive steer refusal.
After a steer has been submitted, Stop aborts the local application waiter. The raw steer RPC is still retained in the cancellation drain, which is good, but its eventual result is no longer used to classify delivery.
So this sequence appears possible:
steer B submitted
→ Stop aborts local application wait
→ B is recorded as delivery-unknown / failed
→ raw steer RPC later returns invalid request
invalid request is normally the definitive signal that the provider did not accept the steer, so B should remain eligible for the existing follow-up/requeue behavior. Instead, the late refusal seems to be discarded after the local abort has already won.
I think local cancellation should only release the application wait/lease. The raw RPC should remain the delivery verdict source:
invalid request → definitely refused → requeue
injected ACK → delivered / do not resend
transport error → unknown / do not resend
Second, I think the new durable receipt path still has a write-failure hole.
setTerminalUserTurnStatus() currently writes pendingTerminalUserTurnReceipts only when the history entry was not found. But if the entry exists and updateHistory() itself throws, the function exits before writing a receipt.
That leaves:
history terminal write fails
→ no history outcome
→ no durable receipt
→ restart can still lose the exact terminal outcome
It seems safer for the receipt to act as the durable fallback/WAL for the terminal decision, including failures of the initial history write, not only missing-history cases.
| private readonly steerApplicationWaiters = new Map<string, SteerApplicationWaiter>(); | ||
| private steerApplicationBarrier: Promise<void> | null = null; | ||
| private activePromptCompletion: ActivePromptCompletion | null = null; | ||
| private readonly pendingPrompts = new Set<Promise<acp.PromptResponse>>(); |
There was a problem hiding this comment.
One related design concern: pendingPrompts was widened from:
Set<Promise<acp.PromptResponse>>to:
Set<Promise<unknown>>so prompt completions and raw steer RPCs can share the same drain.
I don't think unknown itself is the bug because the drain only needs settlement, but it seems to expose the underlying issue: the raw steer request is now retained mostly as a “pending/settled” token, while its eventual typed delivery verdict can be lost.
Would it be cleaner to preserve the original typed request and put only a Promise<void> settlement wrapper into the cancellation drain? That would keep request draining separate from delivery classification instead of erasing the distinction at this boundary.
There was a problem hiding this comment.
Thanks, the earlier revision did lose the verdict after local cancellation. At e4ef2d29, SteerPromptRun.delivery now preserves it independently of the application wait, so a late invalid request still reaches the requeue logic.
I’ve kept pendingPrompts as a settlement-only collection because it never reads the response. requestSteeringExtension validates the response and classifies refusals before delivery settles. The finalizer uses that result to decide what happens to the message.
59ceaa7 to
11d8c2a
Compare
Pass an optional cancellation signal through attachment downloads and run configuration so a stopped steer cannot keep preparing or apply later configuration steps. Model: gpt-6-astra
11d8c2a to
1cf78fd
Compare
Unwind local steer waits before interrupting the existing owner, retain raw delivery and configuration work through its drain, and preserve definite refusal without adding restart recovery state. Model: gpt-6-astra
1cf78fd to
e4ef2d2
Compare
|
Thanks for the review, @wibus-wee! I’ve reworked this at Your late-refusal example helped clarify the boundary: cancelling the local wait must not decide whether the steer was delivered. The original delivery promise now survives cancellation, so a later I also narrowed #477 and this PR to Stop cleanup and next-message execution. The durable receipts, persistence barriers and restart recovery changes are removed. The history-write failure you identified remains a separate recovery problem that I don’t plan to solve in this PR. A failed outcome write does, however, allow the remaining cancellation cleanup to finish. The existing turn owner remains responsible throughout: flowchart TD
A["Stop"] --> B["Cancel local steer wait"]
B --> C["Finish applied handoff; release queue and lease"]
C --> D["Existing owner waits for raw ACP/config work or confirmed termination"]
D --> E["Use delivery verdict and finish cleanup"]
E --> F["Release owner; next message can run"]
Only a definite refusal permits requeue. Accepted or unknown delivery is not resent by Stop. I’ve run the tests and a separate adversarial review locally, and CI is green, including desktop smoke. I haven’t rerun live-provider or Windows acceptance. Thanks again for helping catch the ownership issue. 中文说明谢谢之前指出的问题!这次调整了 Stop 的收尾顺序,也保留了取消后晚到的投递结果,避免把明确拒绝的消息误判成失败。 跨重启的结果保护暂时不打算在这个 PR 里解决,Issue 和 PR 的范围也一起收窄了。我在本地跑了测试和独立的对抗性评审,CI 也通过了。方便时麻烦再帮忙看一下,谢谢! |
Related issue
Closes #477
Refs #451
Problem / pressure
Pressing Stop while a steer is pending can leave the session queue and history-rewrite lease occupied. The interface looks stopped, but the next message never reaches the agent.
#571 already keeps the turn owner until the old ACP execution finishes or is confirmed terminated. This fix makes pending steer preparation, configuration and application waits follow that same ownership boundary.
Summary
This PR only covers Stop cleanup and next-message execution. Missing-history results keep the existing in-memory handling; preserving those results across a daemon restart is outside #477 and this PR.
Visual explanation
Cancelling a local wait does not mean the old execution has ended:
flowchart TD A["Stop"] --> B["Cancel local steer wait"] B --> C["Finish applied handoff; release queue and lease"] C --> D["Existing owner waits for raw ACP/config work or confirmed termination"] D --> E["Use delivery result and finish cleanup"] E --> F["Release owner; next message can run"]The existing timeout handles an unresponsive execution. If termination fails, the owner continues waiting for the outstanding work.
Before / after
Test plan
pnpm check,pnpm formatand documentation checks passed. CI passed one4ef2d29, including desktop smoke. An independent adversarial and simplification review found no remaining blocking issue.Context handoff
Instructions for reviewing agents
Authoring context
Original user prompt
Show original prompt