Skip to content

Commit deb6a5f

Browse files
Zivxxclaude
andcommitted
fix(opal-server): retrieve webhook task exceptions before dropping them
The webhook task sweep dropped finished tasks without inspecting t.exception(), and stop() only gathered self._tasks — so an error raised inside trigger() never reached the app logger, surfacing only as asyncio's generic "Task exception was never retrieved" at GC time. Log failed trigger tasks at ERROR during the sweep and include webhook tasks in the stop() gather. Also de-flake the cleanup test: await the freshly scheduled trigger task directly instead of relying on an asyncio.sleep(0) scheduler tick, and add a test asserting the failure of a trigger task is retrieved and logged. Addresses review comments: - #923 (comment) (@zeevmoney) - #923 (comment) (@zeevmoney) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 1e7df1c commit deb6a5f

2 files changed

Lines changed: 42 additions & 4 deletions

File tree

packages/opal-server/opal_server/policy/watcher/task.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,12 @@ async def _on_webhook(self, topic: Topic, data: Any):
3030
logger.info(f"Webhook listener triggered ({len(self._webhook_tasks)})")
3131
# Rebuild rather than remove-while-iterating: list.remove() inside a
3232
# `for t in self._webhook_tasks` loop skips the element after each removal,
33-
# so finished tasks accumulate.
33+
# so finished tasks accumulate. Retrieve exceptions before dropping the
34+
# references — otherwise a failed trigger() is only reported by asyncio's
35+
# generic "Task exception was never retrieved" at GC time.
36+
for t in self._webhook_tasks:
37+
if t.done() and not t.cancelled() and t.exception() is not None:
38+
logger.error(f"Webhook trigger task failed: {t.exception()!r}")
3439
self._webhook_tasks = [t for t in self._webhook_tasks if not t.done()]
3540
self._webhook_tasks.append(asyncio.create_task(self.trigger(topic, data)))
3641

@@ -72,7 +77,7 @@ async def stop(self):
7277
for task in self._tasks + self._webhook_tasks:
7378
if not task.done():
7479
task.cancel()
75-
await asyncio.gather(*self._tasks, return_exceptions=True)
80+
await asyncio.gather(*self._tasks, *self._webhook_tasks, return_exceptions=True)
7681

7782
async def trigger(self, topic: Topic, data: Any):
7883
"""Triggers the policy watcher from outside to check for changes (git

packages/opal-server/opal_server/tests/webhook_task_cleanup_test.py

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ async def _done():
2222
w._webhook_tasks = list(finished)
2323

2424
await w._on_webhook("webhook", None)
25-
await asyncio.sleep(0) # let the newly created trigger task finish
2625

2726
# all 3 done ones removed...
2827
remaining_done = [t for t in w._webhook_tasks if t in finished]
@@ -32,4 +31,38 @@ async def _done():
3231
assert len(w._webhook_tasks) == 1
3332
assert len(survivors) == 1, f"new trigger task not scheduled: {w._webhook_tasks}"
3433

35-
await asyncio.gather(*survivors) # drain the dangling task
34+
# Await the trigger task directly (not sleep(0)) so the test doesn't depend
35+
# on scheduler tick ordering.
36+
await asyncio.gather(*survivors)
37+
38+
39+
class _FailingWatcher(BasePolicyWatcherTask):
40+
async def trigger(self, topic, data):
41+
raise RuntimeError("trigger blew up")
42+
43+
44+
@pytest.mark.asyncio
45+
async def test_failed_trigger_exception_is_retrieved_and_logged():
46+
"""Sweeping a failed trigger task must retrieve and log its exception, not
47+
silently drop the reference (asyncio's 'exception was never retrieved')."""
48+
from opal_common.logger import logger as opal_logger
49+
50+
w = _FailingWatcher(pubsub_endpoint=None)
51+
52+
await w._on_webhook("webhook", None) # schedules a trigger that raises
53+
failed = list(w._webhook_tasks)
54+
await asyncio.gather(*failed, return_exceptions=True)
55+
56+
records = []
57+
sink_id = opal_logger.add(lambda m: records.append(str(m)), level="ERROR")
58+
try:
59+
await w._on_webhook("webhook", None) # sweep must log the failure
60+
finally:
61+
opal_logger.remove(sink_id)
62+
63+
assert failed[0] not in w._webhook_tasks, "failed task not swept"
64+
assert any(
65+
"Webhook trigger task failed" in r and "trigger blew up" in r for r in records
66+
), f"failure not logged: {records}"
67+
68+
await asyncio.gather(*w._webhook_tasks, return_exceptions=True)

0 commit comments

Comments
 (0)