Skip to content

Commit ac48ce1

Browse files
authored
fix(server): prevent failed cancel from overwriting terminal task state (#1405)
closes #1397 ## Issue Cancel event could overwrite a `task` that was already in terminated state ## Change On producer site: Ensured that `task` is not in terminal state when enqueueing On consumer site: If `task` is in terminal state, drop the current processing event (and `.taskdone()` on queue) ## Test Added tests that ensure that cancellation events do not overwrite terminal state Also, re-runned `repro` provided in issue desc to validate that after the changes, flow works as expected
1 parent 56ec855 commit ac48ce1

2 files changed

Lines changed: 130 additions & 2 deletions

File tree

‎src/a2a/server/agent_execution/active_task.py‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,22 @@ async def _process_event_inner(self, event: Event) -> None:
260260
| PushNotificationEvent
261261
| Task,
262262
):
263+
if isinstance(
264+
event, TaskStatusUpdateEvent | TaskArtifactUpdateEvent
265+
):
266+
current = await self.active_task._task_manager.get_task()
267+
if (
268+
current is not None
269+
and current.status.state in TERMINAL_TASK_STATES
270+
):
271+
# A terminal task's status is final. New status updates should be dropped
272+
logger.debug(
273+
'Consumer[%s]: dropping %s for an already-terminal task',
274+
self.active_task._task_id,
275+
type(event).__name__,
276+
)
277+
self.active_task._event_queue_agent.task_done()
278+
return
263279
updated_task = await self._handle_task_event(event)
264280
handled_event = updated_task if isinstance(event, Task) else event
265281

@@ -1079,7 +1095,10 @@ async def _mark_task_as_failed(self, exception: Exception) -> Task | None:
10791095
if self._task_created.is_set():
10801096
try:
10811097
task = await self._task_manager.get_task()
1082-
if task is not None:
1098+
if (
1099+
task is not None
1100+
and task.status.state not in TERMINAL_TASK_STATES
1101+
):
10831102
await self._event_queue_agent.enqueue_event(
10841103
TaskStatusUpdateEvent(
10851104
task_id=task.id,

‎tests/server/agent_execution/test_active_task.py‎

Lines changed: 110 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
import pytest
88
import pytest_asyncio
99

10-
from a2a.server.agent_execution.active_task import ActiveTask
10+
from a2a.server.agent_execution.active_task import ActiveTask, EventConsumer
1111
from a2a.server.agent_execution.agent_executor import AgentExecutor
1212
from a2a.server.agent_execution.context import RequestContext
1313
from a2a.server.context import ServerCallContext
@@ -234,6 +234,115 @@ async def execute_mock(req, q):
234234
assert shared.status.state == TaskState.TASK_STATE_WORKING
235235
stop_event.set()
236236

237+
@pytest.mark.asyncio
238+
@pytest.mark.parametrize(
239+
('final_state', 'cancel_error_message', 'expected_failed_count'),
240+
[
241+
(TaskState.TASK_STATE_COMPLETED, 'cancel lost the race', 0),
242+
(TaskState.TASK_STATE_WORKING, 'agent cancel blew up', 1),
243+
],
244+
ids=['terminal-task-not-overwritten', 'active-task-still-marks-failed'],
245+
)
246+
async def test_active_task_cancel_exception_marks_failed_only_when_not_terminal(
247+
self,
248+
active_task: ActiveTask,
249+
agent_executor: Mock,
250+
request_context: Mock,
251+
task_manager: Mock,
252+
final_state: TaskState,
253+
cancel_error_message: str,
254+
expected_failed_count: int,
255+
) -> None:
256+
"""Regression test: FAILED is enqueued only if the task was not already terminal when cancel() failed."""
257+
stop_event = asyncio.Event()
258+
259+
async def execute_mock(req: RequestContext, q: EventQueue) -> None:
260+
await stop_event.wait()
261+
262+
get_task_calls = 0
263+
264+
async def get_task_side_effect() -> Task | None:
265+
nonlocal get_task_calls
266+
get_task_calls += 1
267+
# start()'s existence check needs a non-terminal task so it sets _task_created.
268+
if get_task_calls == 1:
269+
return Task(
270+
id='test-task-id',
271+
status=TaskStatus(state=TaskState.TASK_STATE_WORKING),
272+
)
273+
return Task(id='test-task-id', status=TaskStatus(state=final_state))
274+
275+
agent_executor.execute = AsyncMock(side_effect=execute_mock)
276+
agent_executor.cancel = AsyncMock(
277+
side_effect=RuntimeError(cancel_error_message)
278+
)
279+
task_manager.get_task = AsyncMock(side_effect=get_task_side_effect)
280+
281+
await active_task.enqueue_request(request_context)
282+
await active_task.start(
283+
call_context=ServerCallContext(), create_task_if_missing=True
284+
)
285+
await asyncio.sleep(0.1)
286+
# Precondition: the guard under test only applies once this is set.
287+
assert active_task._task_created.is_set()
288+
289+
with patch.object(
290+
active_task._event_queue_agent,
291+
'enqueue_event',
292+
wraps=active_task._event_queue_agent.enqueue_event,
293+
) as enqueue_spy:
294+
with pytest.raises(RuntimeError, match=cancel_error_message):
295+
await active_task.cancel(request_context)
296+
297+
failed_writes = [
298+
call.args[0]
299+
for call in enqueue_spy.await_args_list
300+
if isinstance(call.args[0], TaskStatusUpdateEvent)
301+
and call.args[0].status.state == TaskState.TASK_STATE_FAILED
302+
]
303+
assert len(failed_writes) == expected_failed_count
304+
assert all(write.task_id == 'test-task-id' for write in failed_writes)
305+
306+
stop_event.set()
307+
await active_task.aclose()
308+
309+
@pytest.mark.asyncio
310+
async def test_event_consumer_drops_status_update_for_terminal_task(
311+
self,
312+
active_task: ActiveTask,
313+
task_manager: Mock,
314+
) -> None:
315+
"""Regression test: FAILED update enqueued before the task turned terminal must be dropped at apply-time too."""
316+
task_manager.get_task = AsyncMock(
317+
return_value=Task(
318+
id='test-task-id',
319+
status=TaskStatus(state=TaskState.TASK_STATE_COMPLETED),
320+
)
321+
)
322+
323+
stale_failed_event = TaskStatusUpdateEvent(
324+
task_id='test-task-id',
325+
status=TaskStatus(state=TaskState.TASK_STATE_FAILED),
326+
)
327+
await active_task._event_queue_agent.enqueue_event(stale_failed_event)
328+
# Round-trip through the queue so task_done() below has a matching get().
329+
dequeued = await active_task._event_queue_agent.dequeue_event()
330+
331+
# Not started: exercising the consumer's guard directly needs no
332+
# producer/consumer loop.
333+
consumer = EventConsumer(active_task)
334+
with patch.object(
335+
active_task._event_queue_subscribers,
336+
'enqueue_event',
337+
wraps=active_task._event_queue_subscribers.enqueue_event,
338+
) as subscriber_spy:
339+
await consumer._process_event(dequeued)
340+
341+
# Dropped event must reach neither TaskManager nor subscribers.
342+
task_manager.ensure_task_id.assert_not_awaited()
343+
task_manager.process.assert_not_awaited()
344+
subscriber_spy.assert_not_awaited()
345+
237346
@pytest.mark.asyncio
238347
async def test_active_task_interrupted_auth(
239348
self,

0 commit comments

Comments
 (0)