Repository navigation
fix: propagate cancellation across active turns - #308
Conversation
ghfind-review
Bot
added
review: high
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
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. |
32421ce to
8941ed7
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32421ce1d8
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if context.cancelled is not None and context.cancelled.is_set(): | ||
| return context.cancelled |
There was a problem hiding this comment.
When this signal interrupts an active provider wait, bounded_call/bounded_stream raises ProviderError(CANCELLED), but every _get_llm_response path catches that exception and converts it to an [LLM Error] string. The exception therefore never reaches _chat_turn's new cancellation handler: accepted-plan tasks can be marked blocked rather than cancelled, no turn.effect_uncertain event is recorded, and chat() emits turn.failed instead of turn.cancelled. Propagate cancellation errors out of the provider response wrappers so the cancellation cleanup actually runs.
Useful? React with 👍 / 👎.
| async def api_cancel_chat(self, session_id: str, request: Request): | ||
| service = self | ||
| session_id = service._normalise_session_id(session_id) | ||
| user_id = service._actor_for_request(request) | ||
| return {"session_id": session_id, | ||
| "cancelled": service._agent.cancel_turn(session_id, user_id=user_id)} |
There was a problem hiding this comment.
For a turn started through POST /api/chat, api_chat is asynchronous but calls the synchronous _run_session_chat directly, blocking the process's event loop for the entire turn. Consequently this new cancellation endpoint cannot be scheduled on the same worker until the turn has already finished; sending it to another multi-process worker does not help because _active_turns is process-local. Dispatch the non-streaming chat work to a thread, as the streaming endpoint already does, so cancellation requests can run concurrently.
Useful? React with 👍 / 👎.
| {name: "agent", aliases: nil, description: "Switch auto, coder, or researcher mode"}, | ||
| {name: "permissions", aliases: []string{"permission"}, description: "Choose Ask, protected full access, or full access"}, | ||
| {name: "agents", description: "Inspect parallel agents, results, and independent reviews"}, | ||
| {name: "cancel", aliases: nil, description: "Cancel the active foreground turn"}, |
There was a problem hiding this comment.
The Go TUI registers /cancel, but model.submit() returns immediately whenever m.busy is true unless the input is exactly /agents (tui/input.go:604). Since an active foreground turn is precisely what sets the UI busy, users can discover and type this new command but it is never sent to the backend until there is no turn left to cancel. Add /cancel to the commands permitted during the busy state.
Useful? React with 👍 / 👎.
| if command in {"/cancel", "cancel"}: | ||
| session = runtime.current_session | ||
| cancelled = runtime.cancel_turn(session.session_id, user_id=session.memory.user_id) | ||
| self.emit("response", request_id, text="Cancellation requested." if cancelled else "No active turn to cancel.") |
There was a problem hiding this comment.
When a TUI turn is waiting at a high-impact tool approval, _approval() is blocked on its separate waiter for up to APPROVAL_TIMEOUT_SECONDS (15 minutes). This command only sets the turn cancellation event, so even a programmatic /cancel request leaves the worker and UI busy until the user answers the approval or the timeout expires; the cancellation check in _run_tool cannot run until that wait returns. Cancellation should also deny and signal approvals belonging to the active turn, or the approval wait should observe the turn signal.
Useful? React with 👍 / 👎.
| cancelled=CancellationEvents(self.execution_context.cancelled, | ||
| coordinator.cancelled.get(run["run_id"]))) |
There was a problem hiding this comment.
When a parent is cancelled while _complete_turn is waiting for delegated work, this combined signal can stop a child's provider/tool operation, but the delegation coordinator determines cancellation solely from its own per-run event. Because that event was not set, it records the child as failed, and _complete_turn treats the terminal failure as an ordinary unresolved result and returns normally, causing the cancelled parent turn to emit completion instead of entering _chat_turn's cancellation cleanup. Propagate inherited cancellation into the coordinator run state and make the waiting parent raise cancellation rather than synthesize a normal result.
Useful? React with 👍 / 👎.
| with self._delegation_tool_access(action, execution_args): | ||
| if self.execution_context.cancelled is not None and self.execution_context.cancelled.is_set(): | ||
| outcome = tool_failure("cancelled", "Tool call was cancelled before execution.") |
There was a problem hiding this comment.
This cancellation check runs only after _delegation_tool_access has been entered. If the foreground action is queued behind a sub-agent's overlapping workspace claim, WorkspaceAccess.acquire() may wait indefinitely relative to the cancellation request; for a parent action its callback is always false, and for a child it observes only the coordinator's event rather than the newly inherited parent event. Check the turn signal before entering and pass it into the access wait so cancellation can interrupt lock contention.
Useful? React with 👍 / 👎.
| return CommandResult("Error: command cancelled", False, process.returncode, | ||
| "cancelled", out, err, time.monotonic() - started, True, | ||
| sandbox_backend=invocation.backend, |
There was a problem hiding this comment.
The positional True passed here populates CommandResult.timed_out, so every command stopped by cancellation is serialized with both failure="cancelled" and timed_out=true. Consumers and receipts therefore cannot distinguish a user cancellation from deadline expiry even though the command was terminated before its timeout. Leave timed_out false for this path, preferably by using named arguments.
Useful? React with 👍 / 👎.
| if command in {"/cancel", "cancel"}: | ||
| session = runtime.current_session | ||
| cancelled = runtime.cancel_turn(session.session_id, user_id=session.memory.user_id) | ||
| self.emit("response", request_id, text="Cancellation requested." if cancelled else "No active turn to cancel.") |
There was a problem hiding this comment.
Once /cancel is allowed during a busy turn, emitting its acknowledgement as a normal response makes the Go client's response handler replace the current streaming assistant placeholder and immediately set m.busy=false (tui/events.go:138-162). The cancelled worker may still be unwinding, especially for cooperative or external operations, so the UI can accept another submission that the backend rejects as busy and the eventual terminal event can be attributed to an already-finalized placeholder. Send a nonterminal status/ack event here and let the original turn's terminal event clear the busy state.
Useful? React with 👍 / 👎.
| cancellation = threading.Event() | ||
| turn_context = copy.copy(self.execution_context) | ||
| from openkyrozen.agent.turn import CancellationEvents | ||
| turn_context.cancelled = CancellationEvents(turn_context.cancelled, cancellation) | ||
| turn_context_token = self._turn_context.set(turn_context) |
There was a problem hiding this comment.
The inner _chat_turn treats any exception raised while this cancellation signal is set as cancellation, but chat()'s outer terminal-event handler only recognizes KeyboardInterrupt, asyncio.CancelledError, and ProviderError(CANCELLED). Cancellation-aware paths such as delegation checks can raise an ordinary RuntimeError, producing a persisted cancelled outcome and uncertainty event inside _chat_turn followed by a contradictory turn.failed terminal event outside it. Include the active signal state in the outer classification so every cooperative cancellation has one consistent terminal event.
Useful? React with 👍 / 👎.
| if cancelled: | ||
| self._publish_runtime_event("turn.effect_uncertain", { | ||
| "reason": "cancelled_during_execution", | ||
| "message": "Remote or already-dispatched side effects may still complete.", | ||
| }) |
There was a problem hiding this comment.
_publish_runtime_event only notifies the current in-process observers; it does not append to the durable event store read by /api/v2/events. Consequently this newly emitted uncertainty notice disappears after a non-streaming turn or process restart, despite the documented promise that the session event journal records the possibility of already-dispatched side effects completing. Append a scoped durable event or conversation entry in addition to publishing the transient runtime event.
Useful? React with 👍 / 👎.
8941ed7 to
716d258
Compare
1 participant
Summary
Cancellation now flows from an active chat turn through provider waits, delegated child contexts, tool dispatch boundaries, and foreground shell process groups. Accepted plans and turn history record cancellation; an uncertainty event notes that already-dispatched external effects may still finish. Web clients can cancel with
POST /api/chat/{session_id}/cancel, and the TUI has/cancel. Intentional background jobs stay session-owned.Closes #258
Root cause and approach
An active turn had no shared cancellation signal. A request could not stop its provider wait, shell process tree, or subsequent tool dispatches. The runtime now registers one per-session active-turn signal and propagates it into inherited execution contexts. Shell cancellation terminates the process group/tree using existing platform mechanisms.
Tests
make lint,make check, andmake docs-check.make test-corewas interrupted after the local volume reached 99% capacity; its Chroma-backed tests hit SQLite disk I/O errors. The broad local run is inconclusive; PR CI is required before merge.Risks and follow-ups
Cancellation is cooperative for provider requests and tool boundaries. A tool already dispatched to a remote service may complete after cancellation; this is recorded as uncertain. Session background processes remain alive by design and retain their existing session stop controls.