refactor(trigger_event): streamline event dispatching and improve plugin action handling

- Replaced the plugin listing mechanism with a more efficient handler iteration for event actions.
- Enhanced logging to provide clearer insights into the dispatching process and plugin action execution.
- Added error handling to ensure that failures in one plugin action do not block subsequent actions.
- Updated tests to cover new behavior and ensure proper handling of enabled and disabled plugins.

apps/connect/tests/test_trigger_event.py

@@ -35,43 +35,56 @@ def _empty_subscription_chain():
 
 
 class TriggerEventDispatchTests(SimpleTestCase):
-    def _run_trigger_event(self, plugins, event_name, payload):
+    def _run_trigger_event(self, handlers, event_name, payload, enabled_keys=None):
         pm = MagicMock()
-        pm.list_plugins.return_value = plugins
+        pm.iter_actions_for_event.return_value = handlers
+        if enabled_keys is None:
+            enabled_keys = [key for key, _ in handlers]
+
+        enabled_qs = MagicMock()
+        enabled_qs.values_list.return_value = enabled_keys
+
         with patch(
             "apps.connect.utils.PluginManager.get", return_value=pm
         ), patch(
             "apps.connect.utils.EventSubscription.objects.filter",
             return_value=_empty_subscription_chain(),
-        ):
+        ), patch(
+            "apps.plugins.models.PluginConfig"
+        ) as mock_cfg:
+            mock_cfg.objects.filter.return_value = enabled_qs
             from apps.connect.utils import trigger_event
 
             trigger_event(event_name, payload)
         return pm
 
     def test_disabled_plugin_does_not_abort_dispatch_for_later_enabled_plugin(self):
-        """The original bug: AttributeError on a disabled plugin's debug log
-        aborted the loop, so any plugin iterated AFTER a disabled one never
-        received events."""
-        plugins = [
-            {
-                "key": "disabled-plugin",
-                "name": "Disabled Plugin",
-                "enabled": False,
-                "actions": [],
-            },
-            {
-                "key": "enabled-plugin",
-                "name": "Enabled Plugin",
-                "enabled": True,
-                "actions": [
-                    {"id": "on_event", "events": ["channel_start"]},
-                ],
-            },
+        """Enabled handlers still run when other plugins are disabled in DB."""
+        handlers = [
+            ("enabled-plugin", "on_event"),
         ]
 
         pm = self._run_trigger_event(
-            plugins, "channel_start", {"channel_name": "TEST"}
+            handlers, "channel_start", {"channel_name": "TEST"}
+        )
+
+        pm.run_action.assert_called_once_with(
+            "enabled-plugin",
+            "on_event",
+            {"event": "channel_start", "payload": {"channel_name": "TEST"}},
+        )
+
+    def test_skips_handlers_for_disabled_plugins(self):
+        handlers = [
+            ("disabled-plugin", "on_event"),
+            ("enabled-plugin", "on_event"),
+        ]
+
+        pm = self._run_trigger_event(
+            handlers,
+            "channel_start",
+            {"channel_name": "TEST"},
+            enabled_keys=["enabled-plugin"],
         )
 
         pm.run_action.assert_called_once_with(
@@ -81,22 +94,64 @@ class TriggerEventDispatchTests(SimpleTestCase):
         )
 
     def test_action_without_matching_event_is_not_dispatched(self):
-        """Sanity check: actions whose `events` list doesn't include the
-        fired event, or which have no `events` key at all, are skipped."""
-        plugins = [
-            {
-                "key": "plugin",
-                "name": "Plugin",
-                "enabled": True,
-                "actions": [
-                    {"id": "on_event", "events": ["channel_stop"]},
-                    {"id": "manual_button"},  # no events key
-                ],
-            },
-        ]
-
+        """When no handlers are registered for the event, run_action is not called."""
         pm = self._run_trigger_event(
-            plugins, "channel_start", {"channel_name": "TEST"}
+            [], "channel_start", {"channel_name": "TEST"}
         )
 
         pm.run_action.assert_not_called()
+
+    def test_no_plugin_config_query_when_no_handlers(self):
+        pm = MagicMock()
+        pm.iter_actions_for_event.return_value = []
+        with patch(
+            "apps.connect.utils.PluginManager.get", return_value=pm
+        ), patch(
+            "apps.connect.utils.EventSubscription.objects.filter",
+            return_value=_empty_subscription_chain(),
+        ), patch(
+            "apps.plugins.models.PluginConfig"
+        ) as mock_cfg:
+            from apps.connect.utils import trigger_event
+
+            trigger_event("channel_start", {"channel_name": "TEST"})
+
+        mock_cfg.objects.filter.assert_not_called()
+
+    def test_plugin_action_failure_does_not_block_sibling_handlers(self):
+        handlers = [
+            ("failing-plugin", "on_event"),
+            ("working-plugin", "on_event"),
+        ]
+
+        pm = MagicMock()
+        pm.iter_actions_for_event.return_value = handlers
+        pm.run_action.side_effect = [RuntimeError("boom"), {"status": "ok"}]
+
+        enabled_qs = MagicMock()
+        enabled_qs.values_list.return_value = ["failing-plugin", "working-plugin"]
+
+        with patch(
+            "apps.connect.utils.PluginManager.get", return_value=pm
+        ), patch(
+            "apps.connect.utils.EventSubscription.objects.filter",
+            return_value=_empty_subscription_chain(),
+        ), patch(
+            "apps.plugins.models.PluginConfig"
+        ) as mock_cfg:
+            mock_cfg.objects.filter.return_value = enabled_qs
+            from apps.connect.utils import trigger_event
+
+            trigger_event("channel_start", {"channel_name": "TEST"})
+
+        self.assertEqual(pm.run_action.call_count, 2)
+        pm.run_action.assert_any_call(
+            "failing-plugin",
+            "on_event",
+            {"event": "channel_start", "payload": {"channel_name": "TEST"}},
+        )
+        pm.run_action.assert_any_call(
+            "working-plugin",
+            "on_event",
+            {"event": "channel_start", "payload": {"channel_name": "TEST"}},
+        )

apps/connect/utils.py

@@ -1,5 +1,5 @@
 # connect/utils.py
-import logging, json
+import logging
 from django.template import Template, Context
 from .models import EventSubscription, DeliveryLog, SUPPORTED_EVENTS
 from .handlers.webhook import WebhookHandler
@@ -94,23 +94,44 @@ def trigger_event(event_name, payload):
 
     pm = PluginManager.get()
     pm.discover_plugins(sync_db=False, use_cache=True)
-    plugins = pm.list_plugins()
+    handlers = list(pm.iter_actions_for_event(event_name))
+    if not handlers:
+        return
 
-    logger.debug(f"Checking {len(plugins)} plugins for event '{event_name}'")
-    for plugin in plugins:
-        if not plugin["enabled"]:
-            logger.debug(f"Skipping disabled plugin id={plugin['key']} name={plugin['name']}")
-            continue
+    from apps.plugins.models import PluginConfig
 
-        logger.debug(json.dumps(plugin))
-        for action in plugin["actions"]:
-            if "events" in action and event_name in action["events"]:
-                key = plugin["key"]
-                params = {"event": event_name, "payload": payload}
-                action_name = action.get("label") or action.get("id")
-                action_id = action.get("id")
-                logger.debug(
-                    f"Triggering plugin action for event '{event_name}' on plugin id={key} action={action_name}"
+    handler_keys = {key for key, _ in handlers}
+    enabled_keys = set(
+        PluginConfig.objects.filter(enabled=True, key__in=handler_keys).values_list(
+            "key", flat=True
         )
-                if action_id:
+    )
+
+    logger.debug(
+        "Dispatching event '%s' to %d plugin action(s) (%d enabled)",
+        event_name,
+        len(handlers),
+        len(enabled_keys),
+    )
+    params = {"event": event_name, "payload": payload}
+    for key, action_id in handlers:
+        if key not in enabled_keys:
+            logger.debug(
+                "Skipping disabled plugin id=%s for event '%s'", key, event_name
+            )
+            continue
+        logger.debug(
+            "Triggering plugin action for event '%s' on plugin id=%s action=%s",
+            event_name,
+            key,
+            action_id,
+        )
+        try:
             pm.run_action(key, action_id, params)
+        except Exception:
+            logger.exception(
+                "Plugin action failed for event '%s' on plugin id=%s action=%s",
+                event_name,
+                key,
+                action_id,
+            )

apps/plugins/loader.py

@@ -8,7 +8,7 @@ import sys
 import threading
 import types
 from dataclasses import dataclass, field
-from typing import Any, Dict, List, Optional
+from typing import Any, Dict, Iterator, List, Optional, Tuple
 
 from django.db import close_old_connections, transaction
 
@@ -92,6 +92,29 @@ class PluginManager:
                 key: lp.path for key, lp in self._registry.items() if lp and lp.path
             }
 
+        try:
+            return self._discover_plugins_impl(
+                sync_db=sync_db,
+                force_reload=force_reload,
+                previous_packages=previous_packages,
+                previous_aliases=previous_aliases,
+                previous_paths=previous_paths,
+                token=token,
+            )
+        finally:
+            # Discovery runs outside Django's request/task cycle (boot, worker_ready).
+            close_old_connections()
+
+    def _discover_plugins_impl(
+        self,
+        *,
+        sync_db: bool,
+        force_reload: bool,
+        previous_packages: Dict[str, str],
+        previous_aliases: Dict[str, str],
+        previous_paths: Dict[str, str],
+        token: int,
+    ) -> Dict[str, LoadedPlugin]:
         try:
             configs: Optional[Dict[str, PluginConfig]] = None
             try:
@@ -247,6 +270,23 @@ class PluginManager:
                 logger.exception("Deferring plugin DB sync; database not ready yet")
         return self._registry
 
+    def iter_actions_for_event(self, event_name: str) -> Iterator[Tuple[str, str]]:
+        """Yield (plugin_key, action_id) pairs from the in-memory registry."""
+        with self._lock:
+            registry = list(self._registry.items())
+        for key, lp in registry:
+            for action in lp.actions or []:
+                if not isinstance(action, dict):
+                    continue
+                action_id = action.get("id")
+                events = action.get("events")
+                if (
+                    action_id
+                    and isinstance(events, (list, tuple))
+                    and event_name in events
+                ):
+                    yield key, action_id
+
     def _load_plugin(
         self,
         key: str,

apps/plugins/tests/test_iter_actions_for_event.py

@@ -0,0 +1,51 @@
+from django.test import SimpleTestCase
+
+from apps.plugins.loader import LoadedPlugin, PluginManager
+
+
+class IterActionsForEventTests(SimpleTestCase):
+    def test_yields_matching_handlers(self):
+        pm = PluginManager()
+        pm._registry = {
+            "alpha": LoadedPlugin(
+                key="alpha",
+                name="Alpha",
+                actions=[
+                    {"id": "on_start", "events": ["channel_start"]},
+                    {"id": "manual"},
+                ],
+            ),
+            "beta": LoadedPlugin(
+                key="beta",
+                name="Beta",
+                actions=[
+                    {"id": "on_both", "events": ["channel_start", "channel_stop"]},
+                ],
+            ),
+        }
+
+        self.assertEqual(
+            list(pm.iter_actions_for_event("channel_start")),
+            [("alpha", "on_start"), ("beta", "on_both")],
+        )
+        self.assertEqual(list(pm.iter_actions_for_event("channel_stop")), [("beta", "on_both")])
+
+    def test_ignores_string_events_value(self):
+        pm = PluginManager()
+        pm._registry = {
+            "bad": LoadedPlugin(
+                key="bad",
+                name="Bad",
+                actions=[{"id": "hook", "events": "client_connect"}],
+            ),
+            "good": LoadedPlugin(
+                key="good",
+                name="Good",
+                actions=[{"id": "hook", "events": ["client_connect"]}],
+            ),
+        }
+
+        self.assertEqual(
+            list(pm.iter_actions_for_event("client_connect")),
+            [("good", "hook")],
+        )

apps/plugins/tests/test_run_action_db_cleanup.py

@@ -62,3 +62,20 @@ class PluginRunActionDbCleanupTests(SimpleTestCase):
             self.assertTrue(pm.stop_plugin("test_plugin", reason="shutdown"))
 
         mock_close.assert_called_once()
+
+
+class PluginDiscoverDbCleanupTests(SimpleTestCase):
+    @patch("apps.plugins.loader.close_old_connections")
+    @patch.object(PluginManager, "_discover_plugins_impl", return_value={})
+    def test_discover_plugins_closes_connections(self, _mock_impl, mock_close):
+        pm = PluginManager()
+        pm.discover_plugins(sync_db=False)
+        mock_close.assert_called_once()
+
+    @patch("apps.plugins.loader.close_old_connections")
+    def test_discover_plugins_cache_hit_skips_close(self, mock_close):
+        pm = PluginManager()
+        pm._discovery_completed = True
+        pm._last_reload_token = pm._get_reload_token()
+        pm.discover_plugins(sync_db=False, use_cache=True)
+        mock_close.assert_not_called()