test(server): add xfail repros for cancel leaving no terminal state - #1171
Open
technicalpickles wants to merge 1 commit into
Open
test(server): add xfail repros for cancel leaving no terminal state#1171technicalpickles wants to merge 1 commit into
technicalpickles wants to merge 1 commit into
Conversation
Two integration scenarios against DefaultRequestHandlerV2, both using an
executor whose cancel() is empty. That is the shape InputRequiredAgent,
SlowAgent and DummyAgentExecutor already use in this file.
test_scenario_cancel_working_task_empty_cancel passes today only because
its executor hand-enqueues a CANCELED event, next to a "TODO: this should
be done automatically by the framework ?" comment. With that removed:
- a mid-run cancel strands the task in `working` with no terminal state
ever written, so a caller has no way to know it can stop polling
- cancelling a task parked in `input-required` returns success with the
task still `input_required`
Both marked xfail(strict=True) so they flip loudly when fixed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🧪 Code Coverage (vs
|
| Base | PR | Delta | |
|---|---|---|---|
| src/a2a/server/events/event_queue_v2.py | 91.79% | 91.28% | 🔴 -0.51% |
| src/a2a/utils/telemetry.py | 91.47% | 90.70% | 🔴 -0.78% |
| Total | 93.00% | 92.97% | 🔴 -0.02% |
Generated by coverage-comment.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds two failing scenarios for #1170, both
xfail(strict=True)so they sit green in CI today and flip loudly the moment someone fixes the underlying bug.No fix here, just the repro. #1170 has the analysis and two possible fix shapes, and which one you want depends on calls that are yours to make (particularly whether the framework should close a task out when the executor didn't, which an existing TODO in this file already raises).
What they cover
Both drive
DefaultRequestHandlerV2through the real client with an executor whosecancel()is empty:test_scenario_19_mid_run_cancel_reaches_a_terminal_statecancels mid-run. The task staysworkingforever, so a caller has no way to know it can stop polling. The assertion is deliberately weak: any terminal state passes, not specificallycanceled.test_scenario_19_cancel_of_parked_task_does_not_silently_succeedcancels a task parked ininput-required. Cancel returns success with the task unchanged. This one asserts only that the state must not come back unchanged, so it stays agnostic on whether a parked task should be cancelable. Cancelling it or raisingTaskNotCancelableErrorwould both pass.Why an empty
cancel()test_scenario_cancel_working_task_empty_cancelalready covers the mid-run case and passes, because its executor hand-enqueues theCANCELEDevent, right below a# TODO: this should be done automatically by the framework ?comment. These two are that same scenario with the hand-written event removed.InputRequiredAgent,SlowAgentandDummyAgentExecutorin this file all definecancel()aspassalready, so the shape isn't unusual. It's also what a real executor looks like when its cancel path only does teardown.Notes
Only
DefaultRequestHandlerV2, nouse_legacyparametrize. The legacy handler hangs on both of these rather than failing, sinceon_cancel_taskwaits inconsume_allfor an event the empty executor never sends. That seemed like a separate thing and not worth entangling with this.Pure addition, 117 lines, no existing test touched and no new dependencies.
ruff checkandruff format --checkare clean.tests/integrationgoes from 427 passed / 1 xpassed to 427 passed / 2 xfailed / 1 xpassed, and the two new ones run in about 0.2s.🤖 Generated with Claude Code