Skip to content

fix(cli): finish stopped steers before releasing session ownership - #471

Open
Astro-Han wants to merge 2 commits into
LodyAI:mainfrom
Astro-Han:fix/451-stop-steer-handoff
Open

fix(cli): finish stopped steers before releasing session ownership#471
Astro-Han wants to merge 2 commits into
LodyAI:mainfrom
Astro-Han:fix/451-stop-steer-handoff

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

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

  • Stop cancels local steer waits before interrupting the owner. An already-applied handoff finishes first.
  • The existing owner drains raw ACP and configuration requests, or confirms termination, before releasing the session for the next input.
  • The original delivery result survives local cancellation. A late explicit refusal can still requeue the message; accepted or unknown delivery is not resent by Stop.
  • Failure to write the steer outcome is logged and does not skip the remaining cleanup.

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"]
Loading

The existing timeout handles an unresponsive execution. If termination fails, the owner continues waiting for the outstanding work.

Before / after

Before After
A pending steer can block the next message after Stop Local waits and their queue/lease unwind before owner interruption
Cancelling a local wait can hide a later refusal The delivery result remains available to decide whether to requeue
Raw configuration or steer work can outlive its local waiter The owner retains that work until completion or confirmed termination

Test plan

  • Extended the existing cancellation tests to cover preparation, configuration, application, late refusal and history-write failure, including delivery of the next message after cleanup.
  • Controlled AgentClient connections cover cancellation before and after acknowledgement, lease release and later refusal. Attachment and configuration tests check cancellation propagation.
  • Removing configuration draining causes premature owner release; ignoring a late refusal incorrectly marks the message failed. Both checks failed as expected, and the restored tests passed.
  • Local pnpm check, pnpm format and documentation checks passed. CI passed on e4ef2d29, including desktop smoke. An independent adversarial and simplification review found no remaining blocking issue.
  • Live-provider and Windows runtime acceptance were not rerun. The controlled tests do not establish cross-restart recovery guarantees.

Context handoff

Instructions for reviewing agents

  • Review focus: Trace Stop and natural prompt completion through the steer queue, applied handoff and existing turn finalizer. Check that the next message cannot overlap an unfinished old execution.
  • Decisions to challenge: Local cancellation must preserve the original delivery result; only definite refusal permits requeue. History-write failure must not skip cleanup.
  • Plausible failures / evidence gaps: Check late refusal, configuration still running after prompt completion, and failed termination. Live-provider and Windows runtime acceptance remain unverified.

Authoring context

  • User goal / directives: Keep fix(cli): finish stopped steers before releasing session ownership #471 focused on Stop blocking the next message, building on fix(cli): retain ACP prompt ownership after cancellation #571.
  • Constraints / non-goals: Reuse the existing owner, queue and termination timeout. Keep history storage, Operation claims and restart recovery unchanged.
  • Risk-bearing decisions: Applied handoffs finish before interruption. Accepted or unknown delivery is not resent by Stop; a failed outcome write is reported without blocking resource cleanup.
  • Destructive or irreversible behavior: The existing timeout may terminate an unresponsive execution; earlier tool effects cannot be undone. No session files or user history are deleted.
  • Deliberately not done or tested: No durable terminal receipts or restart recovery changes. Results held only in memory can still be lost if the daemon restarts before history arrives.
  • Unknowns / confidence: Tests cover the changed lifecycle paths with controlled responses; their frequency with live providers and Windows runtime behavior have not been rechecked.

Original user prompt

Show original prompt
可以的,那我们应该 force push 整个重写这个 PR 了。在 571 的基础上面去做改写和重构。包括 PR 正文也需要更新了。

@Astro-Han Astro-Han changed the title fix: preserve steer ownership while stopping a turn fix(cli): preserve steer ownership and terminal outcomes on Stop Sep 7, 2026
@github-actions github-actions Bot added status:needs-pr-attention External PR needs contributor attention before review and removed status:needs-pr-attention External PR needs contributor attention before review labels Sep 9, 2026
@Astro-Han
Astro-Han force-pushed the fix/451-stop-steer-handoff branch from 0b1762c to a0862e0 Compare September 12, 2026 09:43
@Astro-Han Astro-Han changed the title fix(cli): preserve steer ownership and terminal outcomes on Stop fix(cli): finish stopped steers before releasing session ownership Sep 12, 2026
@Astro-Han
Astro-Han force-pushed the fix/451-stop-steer-handoff branch from a0862e0 to b670377 Compare September 12, 2026 10:02
@Astro-Han
Astro-Han marked this pull request as ready for review September 12, 2026 10:03
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 12, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-12T14:45:32.471137Z e4ef2d2 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

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 wibus-wee left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>>();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Astro-Han
Astro-Han marked this pull request as draft September 12, 2026 10:55
@Astro-Han
Astro-Han force-pushed the fix/451-stop-steer-handoff branch from 59ceaa7 to 11d8c2a Compare September 12, 2026 11:41
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
@Astro-Han
Astro-Han force-pushed the fix/451-stop-steer-handoff branch from 11d8c2a to 1cf78fd Compare September 12, 2026 12:43
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
@Astro-Han
Astro-Han force-pushed the fix/451-stop-steer-handoff branch from 1cf78fd to e4ef2d2 Compare September 12, 2026 13:38
@Astro-Han
Astro-Han marked this pull request as ready for review September 12, 2026 14:40
@Astro-Han

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @wibus-wee! I’ve reworked this at e4ef2d29. Could you take another look when you have time?

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 invalid request still permits requeue. The shared request set only handles draining; the delivery verdict is preserved separately.

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"]
Loading

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 也通过了。方便时麻烦再帮忙看一下,谢谢!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Stop during steer can block the next user message

2 participants