Skip to content

test(server): add xfail repros for cancel leaving no terminal state - #1171

Open
technicalpickles wants to merge 1 commit into
a2aproject:mainfrom
technicalpickles:cancel-no-terminal-state
Open

test(server): add xfail repros for cancel leaving no terminal state#1171
technicalpickles wants to merge 1 commit into
a2aproject:mainfrom
technicalpickles:cancel-no-terminal-state

Conversation

@technicalpickles

Copy link
Copy Markdown
Contributor

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 DefaultRequestHandlerV2 through the real client with an executor whose cancel() is empty:

  • test_scenario_19_mid_run_cancel_reaches_a_terminal_state cancels mid-run. The task stays working forever, so a caller has no way to know it can stop polling. The assertion is deliberately weak: any terminal state passes, not specifically canceled.
  • test_scenario_19_cancel_of_parked_task_does_not_silently_succeed cancels a task parked in input-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 raising TaskNotCancelableError would both pass.

Why an empty cancel()

test_scenario_cancel_working_task_empty_cancel already covers the mid-run case and passes, because its executor hand-enqueues the CANCELED event, 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, SlowAgent and DummyAgentExecutor in this file all define cancel() as pass already, 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, no use_legacy parametrize. The legacy handler hangs on both of these rather than failing, since on_cancel_task waits in consume_all for 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 check and ruff format --check are clean. tests/integration goes 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

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>
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

🧪 Code Coverage (vs main)

⬇️ Download Full Report

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

@astrogilda

Copy link
Copy Markdown
Contributor

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 git worktree checkouts at cff6727 and a382ad5 with this file dropped into both.

On main both report XFAIL (exit 0). On #1172 both report XPASS(strict), which pytest surfaces as a failure precisely because the bug they pin is gone, and with --runxfail both pass outright. So these hold as strict markers today and flip cleanly the moment the fix lands, which is exactly what you would want from them.

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.

chopmob-cloud added a commit to chopmob-cloud/a2a-python that referenced this pull request Aug 11, 2026
…cel scenarios

The two cancel-terminal-state scenarios were taken from @technicalpickles' strict-xfail repro in a2aproject#1171 and folded in here with the markers dropped; the code comment previously cited only issue a2aproject#1170, which lost that provenance. Restore the attribution in the comment and record it as a Co-authored-by trailer.

Signed-off-by: AlgoVoi <chopmob@gmail.com>
Co-authored-by: Josh Nichols <joshua.nichols@gmail.com>
@technicalpickles

Copy link
Copy Markdown
Contributor Author

#1172 already folds these two scenarios in directly (with the strict-xfail markers dropped, since it fixes what they pin), thanks for the credit there. I think this is superseded, happy to close in favor of #1172.

chopmob-cloud added a commit to chopmob-cloud/a2a-python that referenced this pull request Aug 19, 2026
…ess (a2aproject#1170)

DefaultRequestHandlerV2 could report a successful cancel while leaving the
task with no terminal state, so a client that cancelled had no way to know
it could stop polling.

Two paths reached that:

- Mid-run, ActiveTask.cancel cancelled the producer before awaiting the
  executor's cancel. The producer is the only component that writes the
  task's terminal state, so killing it first (and a cleanup-only
  executor.cancel() that writes nothing) left the task WORKING forever.
  Await the executor's cancel first, mirroring the V1 handler ordering, so
  a terminal event can still be written to the still-open event queue.

- A task parked in a non-terminal state (e.g. input-required) has no
  running producer, so cancel returned it untouched. After the task
  settles, close it out as CANCELED when it is still non-terminal, so the
  outcome is always visible to the caller, mirroring V1's guard that made
  a non-cancelled outcome visible instead of a silent no-op.

Adds two regression scenarios (converted from the xfail repros in a2aproject#1171)
covering both paths with an empty executor cancel().

Signed-off-by: AlgoVoi <chopmob@gmail.com>
chopmob-cloud added a commit to chopmob-cloud/a2a-python that referenced this pull request Aug 19, 2026
…cel scenarios

The two cancel-terminal-state scenarios were taken from @technicalpickles' strict-xfail repro in a2aproject#1171 and folded in here with the markers dropped; the code comment previously cited only issue a2aproject#1170, which lost that provenance. Restore the attribution in the comment and record it as a Co-authored-by trailer.

Signed-off-by: AlgoVoi <chopmob@gmail.com>
Co-authored-by: Josh Nichols <joshua.nichols@gmail.com>
Signed-off-by: AlgoVoi <chopmob@gmail.com>
@astrogilda

Copy link
Copy Markdown
Contributor

Checked the fold rather than taking the summary for it, and it is faithful. Both scenarios are in the integration scenarios file on #1172 as test_scenario_19_mid_run_cancel_reaches_a_terminal_state and test_scenario_19_cancel_of_parked_task_does_not_silently_succeed, with the markers dropped and a comment saying why. That is the whole of what this issue was pinning, so there is nothing left here that #1172 does not carry.
Close it in favour of #1172, and thanks for the credit. Worth saying that the split is what made this cheap: a repro filed on its own, with no fix attached and a strict marker holding it honest, meant the fix could be reviewed against something that already failed for the stated reason instead of against a description of the bug.

mykytanetipa pushed a commit that referenced this pull request Aug 21, 2026
…1159, #1170) (#1172)

## Summary

Two related fixes to `DefaultRequestHandlerV2`, shipped together because
the first is a prerequisite for the second to be safe.

**Fixes #1159 (owner-scoping, the security base).** `on_cancel_task` and
`on_subscribe_to_task` resolved a live task via
`ActiveTaskRegistry.get_or_create(task_id)` by `task_id` alone, skipping
the owner-aware `task_store.get(task_id, context)` that `on_get_task`
and the send path already perform. A caller who knows another tenant's
`task_id` could subscribe to its live stream and cancel it (CWE-639).
Both handlers now consult the owner-aware store first and fail closed
with `TaskNotFoundError` (masking existence, matching `on_get_task`).

**Fixes #1170 (cancel writes a terminal state).** `ActiveTask.cancel`
cancelled the producer before `AgentExecutor.cancel`, so the component
that owns the terminal state could not write it; and a task parked
non-terminal (e.g. `input-required`) reported cancel-success without
transitioning. The executor cancel now runs before the producer cancel
(producer still cancelled on the error path), and cancellation that
leaves the task non-terminal is closed out as `CANCELED`.

## Why one PR

#1170 alone makes #1159 worse: for a parked task, a cross-tenant cancel
via the #1159 bypass changes from a harmless no-op into an actual
cross-tenant `CANCELED` write. Landing #1159's owner check together (as
the base commit) removes that window. Merging them together, or #1159
before #1170, is safe; #1170 must not land first.

## Tests

- Owner-scope regressions: non-owner cancel and non-owner subscribe are
rejected (`TaskNotFoundError`), including a parked task; the owner still
succeeds.
- Cancel-terminal regressions: mid-run cancel reaches a terminal state;
cancel of a parked task does not silently succeed (the maintainer repros
from #1171, converted from xfail to passing).

## Validation

- Owner-scope tests: reverting the #1159 handler change fails all three;
restore passes. Full request-handler module: 62 passed.
- Cancel-terminal tests: reverting the #1170 change fails both
scenario_19 tests; restore passes. Full integration module: no
regression.
- Combined: with both applied, a cross-tenant cancel of a parked task is
rejected rather than writing `CANCELED`.
- `ruff` clean at the pinned version.

---------

Signed-off-by: AlgoVoi <chopmob@gmail.com>
Co-authored-by: Josh Nichols <joshua.nichols@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants