Repository navigation
fix(a2a): cooperative cancellation checkpoints in execute_a2a_task (#16174) - #16498
Conversation
…16174) Cancelling an A2A task previously only flipped a Redis state -- the executor kept running to completion and its work-claim could only lapse at TTL, never release, because releasing early would free a scope while the work holding it was still writing. execute_a2a_task now checks for cancellation at three defined points (before scrub, before orchestration, after orchestration) and stops there instead of continuing, so hold_scopes' `finally` releases the claim as soon as the next checkpoint sees the cancellation. The stop-predicate renewal path in scope_enforcement.py stays as the backstop for an executor wedged before any checkpoint.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 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 |
AutoBot Phase Validation ResultsSystem Maturity: 96.5% Phase Status:PASS Phase 1: Core Infrastructure: 100.0% Recommendations:
|
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
|
Independent review: changes requested, one blocking gap.
Verified sound:
Non-blocking:
|
Review finding: every existing test cancels mid-run, inside scrub_outbound or process_request's side effect, so the first checkpoint (before either runs) could be deleted and all four would stay green. Adds a case starting _FakeTaskManager already CANCELLED, asserting scrub_outbound and process_request are never called, no artifact is stored, and the claim is released.
AutoBot Phase Validation ResultsSystem Maturity: 96.5% Phase Status:PASS Phase 1: Core Infrastructure: 100.0% Recommendations:
|
|
Delta |
1 participant
Thinking Path
TaskManager.cancel_taskis synchronous and advisory — it flips a state in Redis and returns;execute_a2a_taskruns as a fire-and-forgetBackgroundTaskscoroutine that never observed this, so a cancelled task's executor kept running to completion and its work-claim could only lapse at TTL, never release. Releasing the claim immediately on cancel was considered and rejected: it would free a scope while the work holding it was still writing, which is worse than not releasing at all. The fix has to be cooperative — the executor itself has to check for cancellation and stop — and since a single orchestrator call in flight can't be interrupted, the checkpoints have to sit at the boundaries between steps, not inside one.What Changed
_abort_if_cancelledand three checkpoints inside_execute_claimed(autobot-backend/a2a/task_executor.py) — before scrub, before calling the orchestrator, and after it returns — each checking whether the task has moved toCANCELLED.hold_scopes'sfinallyreleases the claim as soon as the function returns, at whichever checkpoint that is.scope_enforcement.py'sstop-predicate renewal path is untouched except a docstring update reflecting the new state; it remains the backstop for an executor wedged before any checkpoint.test_already_cancelled_before_the_first_checkpoint_does_nothing,test_cancelled_before_orchestration_skips_the_orchestrator_call,test_scope_stays_held_while_the_orchestrator_call_is_still_running,test_cancelled_during_orchestration_performs_no_further_work_and_releases,test_never_cancelled_still_completes_normally(autobot-backend/a2a/task_executor_cancellation_test.py).Verification
Static only, per the owner's no-local-execution rule — no test suite was run against this branch locally; CI and the pre-push hook are the authority.
flake8 --select=F401,F821,E501over every touched file — clean.if await _abort_if_cancelled(...): returnblocks removes exactly the behaviour the paired test checks. Removing the first ("before scrub") checkpoint is the one the other three tests cannot catch — they all cancel inside a mockedscrub_outbound/process_requestside effect, i.e. after the first checkpoint has already passed, so deleting it would leave all three green.test_already_cancelled_before_the_first_checkpoint_does_nothingcloses that gap: it flips the fake manager toCANCELLEDfrom insideupdate_state's first call (theWORKINGtransitionexecute_a2a_taskmakes unconditionally, before any checkpoint), then assertsscrub_outboundandorchestrator.process_requestare never called, no artifact is stored, and the claim is released. Removing the "before orchestration" checkpoint would letorchestrator.process_requestrun even thoughtm.cancel()already fired insidescrub_outbound's mock, failingorchestrator.process_request.assert_not_called()in the second test; removing "after orchestration" would letevaluate_task_output/_store_response_artifactsrun after cancellation, failing the empty-artifacts assertion in the third.test_scope_stays_held_while_the_orchestrator_call_is_still_runningandtest_never_cancelled_still_completes_normallyare checkpoint-independent by design (a true-either-way property and the no-cancellation contrast).[pre-push OK] pytest: all relevant tests pass, 5/5).Risks
Low. The checkpoints only ever stop the next step from starting — they cannot interrupt work already in flight, so no behaviour changes for a task that is never cancelled (proven by the contrast test). #15950's cancellation criterion is left unticked here deliberately; a PR cannot verify itself against merged code.
Considered and declined the optional step-list refactor: the three checkpoints interleave with three structurally different operations (scrub, an orchestrator call, artifact storage/eval), not a uniform sequence — folding them into a list would add a layer of indirection without actually shrinking what each step does, for a function that already passes the repo's function-length hook at its current size.
Model Used
Claude Sonnet 5 (claude-sonnet-5)
Issue Link
Closes #16174
Single-issue rationale
Split from #16173 deliberately: different files, no overlap, and materially different risk (a mechanical extraction vs. a lifecycle-semantics change affecting every A2A task) — the batching rule's own exception for independent-or-different-risk changes.
Changelog fragment
changelog/unreleased/16174-a2a-cancel-cooperative.mdChecklist
CLAUDE.mdmain