Skip to content

Commit 36804af

Browse files
caohy1988claude
andcommitted
fix(plugins): make notification dispatch best-effort on callback failure
_run_notification_callbacks() previously raised RuntimeError on the first plugin callback failure, which masked the original application exception and prevented later plugins from being notified. Now it logs the error and continues iterating all plugins. The caller's original exception (agent crash / runner crash) remains the primary error that propagates to the caller. Add regression tests: - test_plugin_callback_failure_does_not_mask_app_error: verifies p2 is still notified when p1's error callback raises - test_original_exception_propagates_despite_plugin_failure: end-to-end test verifying the caller sees the original agent exception, not the plugin's internal error Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent fd2a947 commit 36804af

2 files changed

Lines changed: 81 additions & 12 deletions

File tree

src/google/adk/plugins/plugin_manager.py

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -341,29 +341,29 @@ async def _run_notification_callbacks(
341341
) -> None:
342342
"""Executes a notification-only callback for all registered plugins.
343343
344-
Unlike ``_run_callbacks``, this method **always iterates all plugins**
345-
regardless of return values. It is intended for error callbacks where
346-
every plugin must be notified (the return value is discarded).
344+
Unlike ``_run_callbacks``, this method is **best-effort**: it always
345+
iterates all plugins regardless of return values or exceptions. If a
346+
plugin's callback raises, the error is logged and iteration continues
347+
so that every plugin gets notified. This is critical for error
348+
callbacks where the caller holds an original application exception
349+
that must remain the primary error.
347350
348351
Args:
349352
callback_name: The name of the callback method to execute.
350353
**kwargs: Keyword arguments to be passed to the callback method.
351-
352-
Raises:
353-
RuntimeError: If a plugin encounters an unhandled exception during
354-
execution. The original exception is chained.
355354
"""
356355
for plugin in self.plugins:
357356
callback_method = getattr(plugin, callback_name)
358357
try:
359358
await callback_method(**kwargs)
360359
except Exception as e:
361-
error_message = (
362-
f"Error in plugin '{plugin.name}' during '{callback_name}'"
363-
f" callback: {e}"
360+
logger.error(
361+
"Error in plugin '%s' during '%s' callback: %s",
362+
plugin.name,
363+
callback_name,
364+
e,
365+
exc_info=True,
364366
)
365-
logger.error(error_message, exc_info=True)
366-
raise RuntimeError(error_message) from e
367367

368368
async def close(self) -> None:
369369
"""Calls the close method on all registered plugins concurrently.

tests/unittests/plugins/test_error_callbacks.py

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -500,3 +500,72 @@ async def on_run_error_callback(self, **kwargs):
500500
# Both plugins must be called even though p1 returns non-None.
501501
assert p1.run_error_called
502502
assert p2.run_error_called
503+
504+
@pytest.mark.asyncio
505+
async def test_plugin_callback_failure_does_not_mask_app_error(self):
506+
"""When a plugin's error callback raises, iteration continues
507+
and the original application exception is what the caller sees."""
508+
509+
class _FailingPlugin(BasePlugin):
510+
__test__ = False
511+
512+
def __init__(self, name):
513+
super().__init__(name)
514+
self.agent_error_called = False
515+
self.run_error_called = False
516+
517+
async def on_agent_error_callback(self, **kwargs):
518+
self.agent_error_called = True
519+
raise ValueError("plugin boom")
520+
521+
async def on_run_error_callback(self, **kwargs):
522+
self.run_error_called = True
523+
raise ValueError("plugin boom")
524+
525+
p1 = _FailingPlugin(name="p1")
526+
p2 = _ErrorTrackingPlugin(name="p2")
527+
pm = PluginManager(plugins=[p1, p2])
528+
529+
# Agent error callback: p1 raises, p2 must still be notified.
530+
mock_agent = Mock(spec=BaseAgent)
531+
mock_agent.name = "test_agent"
532+
await pm.run_on_agent_error_callback(
533+
agent=mock_agent,
534+
callback_context=Mock(spec=CallbackContext),
535+
error=RuntimeError("app crash"),
536+
)
537+
assert p1.agent_error_called
538+
assert len(p2.agent_errors) == 1
539+
540+
# Run error callback: same behavior.
541+
await pm.run_on_run_error_callback(
542+
invocation_context=Mock(spec=InvocationContext),
543+
error=RuntimeError("app crash"),
544+
)
545+
assert p1.run_error_called
546+
assert len(p2.run_errors) == 1
547+
548+
@pytest.mark.asyncio
549+
async def test_original_exception_propagates_despite_plugin_failure(
550+
self,
551+
):
552+
"""End-to-end: a crashing plugin error callback does not mask
553+
the original agent exception seen by the caller."""
554+
555+
class _FailingPlugin(BasePlugin):
556+
__test__ = False
557+
558+
def __init__(self, name):
559+
super().__init__(name)
560+
561+
async def on_agent_error_callback(self, **kwargs):
562+
raise ValueError("plugin internal error")
563+
564+
plugin = _FailingPlugin(name="bad_plugin")
565+
agent = _CrashingAgent(name="crash_agent")
566+
ctx = await _create_ctx(agent, plugins=[plugin])
567+
568+
# The caller must see the original RuntimeError("agent crashed"),
569+
# NOT the plugin's ValueError.
570+
with pytest.raises(RuntimeError, match="agent crashed"):
571+
_ = [e async for e in agent.run_async(ctx)]

0 commit comments

Comments
 (0)