diff --git a/.github/actions/spelling/allow.txt b/.github/actions/spelling/allow.txt index 1701f14ef..399075923 100644 --- a/.github/actions/spelling/allow.txt +++ b/.github/actions/spelling/allow.txt @@ -150,4 +150,5 @@ typ typeerror UIDs vulnz +GC whl diff --git a/src/a2a/server/agent_execution/active_task.py b/src/a2a/server/agent_execution/active_task.py index b0154c8d6..0633a4415 100644 --- a/src/a2a/server/agent_execution/active_task.py +++ b/src/a2a/server/agent_execution/active_task.py @@ -36,6 +36,7 @@ from __future__ import annotations import asyncio +import contextlib import logging import uuid @@ -577,6 +578,18 @@ async def _run_consumer(self) -> None: async with self._lock: self._reference_count -= 1 logger.debug('Consumer[%s]: Finishing', self._task_id) + + # Cancel and await the producer before cleanup to prevent + # asyncio "Task was destroyed but it is pending!" warnings. + # Without this, the producer could still be parked on + # _request_queue.get() or inside its finally block when + # _maybe_cleanup() releases the ActiveTask reference, + # causing the still-pending task to be garbage-collected. + if self._producer_task and not self._producer_task.done(): + self._producer_task.cancel() + with contextlib.suppress(asyncio.CancelledError): + await self._producer_task + await self._maybe_cleanup() async def subscribe( diff --git a/tests/server/agent_execution/test_active_task.py b/tests/server/agent_execution/test_active_task.py index ce9e2c068..ca6f89ff9 100644 --- a/tests/server/agent_execution/test_active_task.py +++ b/tests/server/agent_execution/test_active_task.py @@ -895,3 +895,95 @@ async def execute_mock(req, q): assert len(events) == 0 await active_task.cancel(request_context) + + +@pytest.mark.timeout(5) +@pytest.mark.asyncio +async def test_producer_cancelled_on_normal_completion(): + """Verify producer is cancelled and awaited before cleanup to prevent GC warnings. + + Regression test for #1121: when the consumer finishes first (normal + completion path), the producer must be cancelled and awaited before + _maybe_cleanup() releases the ActiveTask reference, otherwise asyncio + logs "Task was destroyed but it is pending!". + """ + agent_executor = Mock() + task_manager = Mock() + cleanup_called = False + + def on_cleanup(_task: ActiveTask) -> None: + nonlocal cleanup_called + cleanup_called = True + + active_task = ActiveTask( + agent_executor=agent_executor, + task_id='test-task-id', + task_manager=task_manager, + on_cleanup=on_cleanup, + ) + + # Simulate a producer that blocks waiting for a request. + # The consumer will finish first (after processing all events), + # triggering normal teardown. + execute_started = asyncio.Event() + execute_barrier = asyncio.Event() + + async def execute_mock(req, q): + execute_started.set() + # Block until released — simulates producer waiting for next request + await execute_barrier.wait() + + agent_executor.execute = AsyncMock(side_effect=execute_mock) + agent_executor.cancel = AsyncMock() + task_manager.get_task = AsyncMock( + return_value=Task( + id='test-task-id', + status=TaskStatus(state=TaskState.TASK_STATE_WORKING), + ) + ) + task_manager.save_task_event = AsyncMock() + task_manager.ensure_task_id = AsyncMock( + return_value=Task( + id='test-task-id', + status=TaskStatus(state=TaskState.TASK_STATE_WORKING), + ) + ) + task_manager.process = AsyncMock(side_effect=lambda x: x) + + request_context = Mock(spec=RequestContext) + request_context.call_context = ServerCallContext() + request_context.context_id = 'test-context' + request_context.message = None + + await active_task.enqueue_request(request_context) + await active_task.start( + call_context=ServerCallContext(), create_task_if_missing=True + ) + + # Wait for producer to be executing + await execute_started.wait() + + # Cancel the consumer to force consumer teardown while producer is pending. + # This simulates the normal-completion race: consumer finishes first, + # triggering _maybe_cleanup while producer is still parked. + if active_task._consumer_task: + active_task._consumer_task.cancel() + try: + await active_task._consumer_task + except asyncio.CancelledError: + pass + + # Release the producer so it can complete its CancelledError/finally path + execute_barrier.set() + + # Wait for producer to finish + await active_task._is_finished.wait() + + # Verify producer was properly completed, not left pending + assert active_task._producer_task is not None + assert active_task._producer_task.done(), ( + 'Producer task should be done after consumer teardown' + ) + assert cleanup_called, ( + 'on_cleanup should be called after producer is drained' + )