Python: Don't emit response.completed after a cancelled Foundry hosting run - #8565
leilei3167 wants to merge 3 commits into
Conversation
…ng run Fixes microsoft#8564. When cancellation_signal fires, _handle_inner_workflow and _handle_inner_agent stop their _SignalledIterator loop and return normally instead of raising, so the post-loop code in _handle_prepared_response treated the cancelled run as a success and still emitted response.completed (or response.incomplete). Add an early return once the inner generator drains so a cancelled run emits no terminal event, letting the cancel-aware host layer synthesize the cancelled terminal instead.
agent-framework-automation
Bot
added
the
python
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Cancellation during tracker-closing events can still produce an incorrect terminal event.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Prevents cancelled Foundry-hosted Python runs from emitting successful terminal events.
Changes:
- Stops terminal emission when cancellation is detected.
- Updates agent and workflow cancellation tests.
| File | Description |
|---|---|
_responses.py |
Adds cancellation-aware terminal suppression. |
test_responses.py |
Updates cancellation expectations and coverage. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if cancellation_signal.is_set(): | ||
| # A cancelled run drains the inner generator without raising (both | ||
| # ``_handle_inner_workflow`` and ``_handle_inner_agent`` stop their | ||
| # ``_SignalledIterator`` loop and return normally once the signal fires). | ||
| # Emit nothing here so a caller cannot mistake this for a normal | ||
| # completion; the host server's cancel-aware layer synthesizes the | ||
| # cancelled terminal when the handler returns without one. | ||
| return |
…rminal event Copilot review on microsoft#8565 pointed out that the cancellation_signal check added there runs before tracker.close(), but each event that loop yields suspends the handler. A cancellation arriving while those trailing *.done events are being consumed still fell through to emit_completed/emit_incomplete, reproducing the microsoft#8564 race for a narrower window. Recheck the signal once more immediately after draining tracker.close(), before selecting the terminal event.
| if cancellation_signal.is_set(): | ||
| # A cancelled run drains the inner generator without raising (both | ||
| # ``_handle_inner_workflow`` and ``_handle_inner_agent`` stop their | ||
| # ``_SignalledIterator`` loop and return normally once the signal fires). | ||
| # Emit nothing here so a caller cannot mistake this for a normal | ||
| # completion; the host server's cancel-aware layer synthesizes the | ||
| # cancelled terminal when the handler returns without one. | ||
| return |
There was a problem hiding this comment.
Should these checks distinguish explicit cancellation from steering? cancellation_signal is also set when a steerable conversation supersedes a turn, but ResponseContext.client_cancelled remains false; returning without a terminal makes agentserver 2.2.0b1 synthesize response.failed for that turn instead of preserving its partial output as response.completed. Could we gate both early returns on context.client_cancelled so steering still drains tracker.close() and emits its normal terminal?
There was a problem hiding this comment.
Good catch, thanks. You're right that cancellation_signal also fires for steering with no cause flag, so the early return was dropping partial output for that case too. Pushed a fix that gates both checks on context.client_cancelled as well, so a steered turn still drains tracker.close() and gets its normal terminal; only an explicit cancel (or non-bg disconnect) now suppresses it. Added a regression test for the steering path and updated the existing cancel tests to set client_cancelled alongside the signal.
Only skip the response.completed/incomplete terminal when the cancellation_signal cause is an explicit /cancel (or non-bg disconnect), i.e. context.client_cancelled is True. cancellation_signal also fires for steering pressure with no cause flag set, so without this gate a steered turn's partial output would be silently dropped instead of completing normally. Update the existing explicit-cancel tests to set client_cancelled alongside the signal, and add a regression test for the steering case.
Motivation & Context
After a Foundry-hosted response is cancelled through
cancellation_signal,_handle_inner_workflowand_handle_inner_agentstop their_SignalledIteratorloop and return normally instead of raising. The post-loop code in_handle_prepared_responsetreats that normal return as a success and still yieldsresponse.completed(orresponse.incomplete).azure-ai-agentserver-responseshappens to rewrite the terminal tocancelledon the wire, but anything sitting between the hosting handler and that layer (for example a subclass wrapping_handle_responseto persist results) sees a successful terminal for a run that was actually cancelled.Description & Review Guide
_handle_prepared_response, return without yielding a terminal event oncecancellation_signalis observed set after the inner generator drains, instead of falling through totracker.close()/emit_completed/emit_incomplete.response.completed; callers consuming the handler stream can rely on the absence of a terminal event (or the cancel-aware host layer's synthesizedcancelledterminal) to distinguish a cancelled run from a finished one.tracker.close()on the cancelled path is safe (the underlying_SignalledIterator/ResponseStreamalready close the wrapped iterator in their ownfinallyblocks) and that this doesn't affect the shutdown/recovery path, which is gated separately.Related Issue
Fixes #8564
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.