test(server): add xfail repros for cancel leaving no terminal state - #1171
test(server): add xfail repros for cancel leaving no terminal state#1171technicalpickles 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
|
Cross-linking a measurement, since a repro with no fix attached is only worth what someone does with it. I ran these two scenarios verbatim against #1172, which fixes #1170 and #1159 together, using two On I have suggested on #1172 that these two be folded in there with the markers dropped, since they exercise the mid-run path through the client and transport rather than at the handler layer, which its own tests cover. Filing the repro separately and leaving the fix to someone else made the review of #1172 much cheaper than it would otherwise have been, so thank you for that. |
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