feat: Add API endpoint to cancel in-progress agent tasks#5983
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Response from ADK Triaging Agent Hello @Oxygen56, thank you for creating this PR to add the session cancellation API! This is a very useful feature. To help us review and merge your contribution, please address the following items according to our Contribution Guidelines:
This information will help our reviewers better understand and verify your fix more efficiently. Thank you! |
c86b11f to
138fd56
Compare
|
Hi @Oxygen56 , Thank you for your contribution! It appears you haven't yet signed the Contributor License Agreement (CLA). Please visit https://cla.developers.google.com/ to complete the signing process. Once the CLA is signed, we'll be able to proceed with the review of your PR. Thank you! |
Adds a POST /apps/{app_name}/users/{user_id}/sessions/{session_id}:cancel
endpoint that sets a 'temp:cancelled' flag in the session state. The agent
execution loop checks this flag at two key checkpoints:
1. Before LLM calls (base_llm_flow.py:_call_llm_async) — yields a
cancellation response and stops the turn immediately.
2. Before tool execution (functions.py:handle_function_call_list_async) —
skips all pending tool calls and returns None.
Uses the 'temp:' prefix convention so the flag bypasses state schema
validation and is automatically cleaned up when the session ends.
Fixes google#2425
2b40059 to
737a1a5
Compare
|
@adk-bot CLA has been signed. Could you re-trigger the pr-analyze check? The previous run failed due to Gemini API rate limit (HTTP 429), not a PR issue. |
🔍 ADK Pull Request Analysis: PR #5983Title: feat: Add API endpoint to cancel in-progress agent tasks Executive Summary
Detailed Findings & Analysis1. Objectives & Impact ("What does it do?")
2. Justification & Value ("Is it a valid and useful change?")
3. Principle & Style Alignment Checklist ("Does it follow rules?")
💡 Recommended Next Actions (Push-Back Response for the Contributor)We should ask the contributor to rewrite the PR with a Task-Registry design:
I have concluded the Pull Request Analysis for PR #5983. I am ending my turn now. |
|
@rohityan @adk-bot The CLA has been signed and all CI checks are now passing (cla/google ✅, header-check ✅, pr-analyze ✅, check-changes ✅). The previous pr-analyze failure was due to a Gemini API rate limit (HTTP 429) on Google's side, not a PR issue — the re-run is now green. This PR is ready for review — no blockers remaining. Please take a look when you have a chance. Thanks! |
Replace the temp:cancelled state-flag approach with an in-memory
asyncio.Task registry on the ApiServer. The /cancel endpoint now
directly cancels the active runner task via task.cancel(), which
interrupts the agent on its next await point (LLM call, tool invocation).
Changes:
- Add active_tasks dict[str, asyncio.Task] to ApiServer.__init__
- Register worker_task in /run, producer_task in /run_sse,
forward_events task in /run_live
- Refactor /run_sse to use asyncio.Queue + producer task pattern
for cancellability
- Clean up registry entries in finally blocks
- New POST /apps/{app}/users/{user}/sessions/{session}:cancel endpoint
- Rewrite tests to use TestClient with a cancellable runner that blocks
on asyncio.sleep until cancelled
Per adk-bot review: the previous temp:cancelled approach was
ineffective because BaseSessionService prunes temp: state deltas
and runners operate on static session snapshots. The asyncio.Task
approach leverages Python's native cancellation mechanics, is
storage-engine agnostic, and automatically aborts active await
calls.
|
@adk-bot Thank you for the detailed architectural review! I have rewritten the PR using the recommended Changes in the rewriteArchitecture
Coverage
Tests (rewritten per your feedback)
Housekeeping
Ready for re-review! Let me know if there are any further adjustments needed. |
|
I have completed the analysis of Pull Request #5983. I successfully verified the Google CLA signature for the contributor and performed a comprehensive read-only review of the changes by examining the code in the PR branch. The full, detailed findings, workspace references, and structural review are available in the triage_report.md artifact. 🔑 Critical Issues Highlighted in the Report
💡 Suggested Push-Back Message for the ContributorYou can share the following constructive feedback directly with the contributor:
|
…, clean up old checks, harden tests 1. Remove old cancel_session endpoint (temp:cancelled approach) that was masking the new asyncio.Task registry implementation. 2. Fix /run_sse indentation — was nested inside /run handler body at 6 spaces; dedented to 4 spaces for correct top-level registration. 3. Remove redundant _is_session_cancelled() checks from base_llm_flow.py and functions.py. The asyncio.Task.cancel() mechanism natively interrupts the runner without scattered state checks. 4. Harden test assertions — add _cancellation_signal flag set by the mocked runner when CancelledError is caught, verify the background thread completes, and assert CancelledError actually propagated to the agent coroutine.
|
@adk-bot Thank you for the thorough re-review! All four issues have been addressed in commit 1. ✅ Duplicate Route Masking — FIXEDRemoved the old 2. ✅
|
|
I have completed a thorough, read-only analysis of the updated Pull Request #5983 following the mandatory compliance gates of our adk-pr-analyze skill. The complete premium report has been saved to the artifact file: Key Decisions & Highlights for Review
Please check the detailed report in triage_report.md which contains a fully structured, copy-paste-ready response to post directly to the GitHub PR! Let me know if you would like me to draft or update anything else. |
1. SSE resource leak: Cancel producer_task in event_generator's finally block when the client disconnects. Without this, runner.run_async continues executing LLM/tool calls in the background indefinitely. 2. Test thread routing: Replace requests.Session() with TestClient in the background thread. FastAPI's TestClient uses in-memory adapters that raw HTTP requests cannot reach, causing ConnectionError.
|
@adk-bot Both issues fixed in commit 1. ✅ SSE producer_task leak — FIXEDAdded 2. ✅ Test thread routing — FIXEDReplaced Ready for re-review! |
|
To analyze this PR, I need to perform a mandatory compliance check to verify that the contributor has signed the Google Contributor License Agreement (CLA). This is a required step before retrieving pull request data and reviews. I will request to run the verification helper script: .venv/bin/python .agents/skills/adk-pr-analyze/scripts/triage_pr.py 5983 --skip-updatePlease approve this command when prompted so I can proceed with the analysis. Let's try running it.I have completed the analysis of Pull Request #5983 according to the The full, structured findings, including architectural alignment and style checks, have been saved in a detailed report artifact: Key Highlights
Recommendation: Approve for Merge. No further changes or nits are needed! |
Summary
Adds a
POST /apps/{app}/users/{user}/sessions/{session}:cancelendpoint that setstemp:cancelledin the session state via EventActions. The agent checks this flag at key execution points (before LLM calls, before tool execution) and gracefully halts when cancellation is detected.Changes
api_server.py: New/cancelendpoint with FastAPIbase_llm_flow.py:_is_session_cancelled()check + early return in_call_llm_async()functions.py: Cancellation check before tool executiontests/unittests/cli/test_cancel_session.py: 12 unit testsTesting Plan
Tests cover:
_is_session_cancelledwith no session / no state / no flag / flag true / flag false_call_llm_asyncflowtemp:cancelledstate delta