Skip to content

Commit 662fd92

Browse files
committed
fix: observe unexpected background task completion during shutdown wait
Wait on both the shutdown event and background tasks so failures like __command_dispatch() are retrieved and logged instead of being ignored. Retrieve outcomes from already-done tasks during shutdown cancellation.
1 parent 77bbfb9 commit 662fd92

2 files changed

Lines changed: 120 additions & 3 deletions

File tree

skywalking/agent/__init__.py

Lines changed: 71 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,66 @@ async def _shutdown_async_queue(q: asyncio.Queue, label: str) -> None:
147147
)
148148

149149

150+
def _retrieve_background_task_outcome(task: asyncio.Task):
151+
"""
152+
Return a completed background task's exception, or a sentinel for unexpected success.
153+
154+
Retrieves the exception so asyncio does not emit "never retrieved" warnings.
155+
"""
156+
if task is None or not task.done() or task.cancelled():
157+
return None
158+
exc = task.exception()
159+
if exc is not None:
160+
return exc
161+
return RuntimeError('Python agent asyncio background task finished unexpectedly')
162+
163+
164+
def _log_background_task_outcome(task: asyncio.Task) -> bool:
165+
"""Log and retrieve a completed background task outcome. Returns True if logged."""
166+
exc = _retrieve_background_task_outcome(task)
167+
if exc is None:
168+
return False
169+
logger.error('Error in Python agent asyncio event loop: %s', exc, exc_info=exc)
170+
return True
171+
172+
173+
async def _await_shutdown_or_background_failure(
174+
finished: asyncio.Event,
175+
background_tasks,
176+
) -> None:
177+
"""
178+
Wait for shutdown or the first unexpected background-task completion.
179+
180+
Background tasks are expected to run until shutdown. If one finishes early,
181+
retrieve/log its outcome and signal shutdown so the root can clean up.
182+
"""
183+
shutdown_waiter = asyncio.create_task(finished.wait())
184+
pending = {task for task in background_tasks if task is not None}
185+
try:
186+
while not finished.is_set():
187+
if not pending:
188+
await finished.wait()
189+
return
190+
done, _ = await asyncio.wait(
191+
pending | {shutdown_waiter},
192+
return_when=asyncio.FIRST_COMPLETED,
193+
)
194+
if shutdown_waiter in done or finished.is_set():
195+
return
196+
for task in done:
197+
pending.discard(task)
198+
if _log_background_task_outcome(task):
199+
finished.set()
200+
return
201+
finally:
202+
if not shutdown_waiter.done():
203+
shutdown_waiter.cancel()
204+
try:
205+
await shutdown_waiter
206+
except asyncio.CancelledError:
207+
pass
208+
209+
150210
async def _cancel_pending_tasks(tasks) -> None:
151211
"""
152212
Cancel agent-owned reporter / connectivity-watch tasks only.
@@ -155,6 +215,11 @@ async def _cancel_pending_tasks(tasks) -> None:
155215
The current task is excluded so we never await ourselves.
156216
"""
157217
current = asyncio.current_task()
218+
for task in tasks:
219+
if task is None or task is current:
220+
continue
221+
if task.done():
222+
_log_background_task_outcome(task)
158223
pending = [
159224
task for task in tasks
160225
if task is not None and task is not current and not task.done()
@@ -174,6 +239,8 @@ async def _cancel_pending_tasks(tasks) -> None:
174239
_SHUTDOWN_JOIN_TIMEOUT_SEC,
175240
sum(1 for task in pending if not task.done()),
176241
)
242+
for task in pending:
243+
_log_background_task_outcome(task)
177244

178245

179246
def _close_previous_protocol(protocol) -> None:
@@ -821,9 +888,10 @@ async def __start_event_loop_async(self) -> None:
821888

822889
self.background_tasks = {asyncio.create_task(coro) for coro in self.background_coroutines}
823890
logger.debug('All background coroutines started')
824-
# Wait for shutdown inside the asyncio.run root, then clean up here so the
825-
# Runner stays alive through protocol aclose() before asyncio.run returns.
826-
await self._finished.wait()
891+
# Wait for shutdown or unexpected background-task completion inside the
892+
# asyncio.run root, then clean up here so the Runner stays alive through
893+
# protocol aclose() before asyncio.run returns.
894+
await _await_shutdown_or_background_failure(self._finished, self.background_tasks)
827895
await self.__async_shutdown_cleanup()
828896

829897
def __start_event_loop(self) -> None:

tests/unit/test_shutdown_queue.py

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#
1717

1818
import asyncio
19+
import logging
1920
import time
2021
import unittest
2122
from queue import Queue
@@ -24,6 +25,7 @@
2425
from skywalking.agent import (
2526
_abandon_async_queue,
2627
_abandon_sync_queue,
28+
_await_shutdown_or_background_failure,
2729
_cancel_pending_tasks,
2830
_join_sync_queue,
2931
_shutdown_async_queue,
@@ -176,6 +178,53 @@ async def reporter():
176178
self.assertTrue(aclose_done.wait(2.0))
177179
self.assertTrue(holder.get('root_finished', False))
178180

181+
def test_background_task_failure_is_logged_not_silenced(self):
182+
"""Regression: failing background tasks must be observed and logged."""
183+
holder = {}
184+
error_logged = Event()
185+
186+
class _Handler(logging.Handler):
187+
def emit(self, record):
188+
if 'Error in Python agent asyncio event loop' in record.getMessage():
189+
holder['logged'] = record.getMessage()
190+
error_logged.set()
191+
192+
agent_logger = logging.getLogger('skywalking')
193+
handler = _Handler()
194+
agent_logger.addHandler(handler)
195+
previous_level = agent_logger.level
196+
agent_logger.setLevel(logging.ERROR)
197+
198+
async def failing_background():
199+
await asyncio.sleep(0.02)
200+
raise ValueError('command dispatch failed')
201+
202+
async def root():
203+
finished = asyncio.Event()
204+
holder['finished'] = finished
205+
failing_task = asyncio.create_task(failing_background())
206+
207+
async def reporter():
208+
while not finished.is_set():
209+
await asyncio.sleep(0.05)
210+
211+
reporter_task = asyncio.create_task(reporter())
212+
tasks = {failing_task, reporter_task}
213+
await _await_shutdown_or_background_failure(finished, tasks)
214+
holder['after_wait'] = True
215+
holder['failing_task'] = failing_task
216+
217+
try:
218+
asyncio.run(root())
219+
self.assertTrue(holder.get('after_wait'))
220+
self.assertTrue(error_logged.wait(2.0))
221+
self.assertIn('command dispatch failed', holder.get('logged', ''))
222+
self.assertTrue(holder['finished'].is_set())
223+
self.assertTrue(holder['failing_task'].done())
224+
self.assertIsInstance(holder['failing_task'].exception(), ValueError)
225+
finally:
226+
agent_logger.removeHandler(handler)
227+
agent_logger.setLevel(previous_level)
179228

180229
def test_shutdown_async_queue_bounded(self):
181230
async def _run():

0 commit comments

Comments
 (0)