From f204675e4c18c17f0776a5bd29ef06732eb1a3b2 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Fri, 10 Jul 2026 21:22:26 -0400 Subject: [PATCH] fix(plugin-manager): fix TypeError breaking every plugin's scheduled update run_scheduled_updates()'s resource-monitor branch wrapped the update call in a closure stored as a *class* attribute on a dynamically-built type (type('obj', (object,), {'update': monitored_update})()). The descriptor protocol turns a function found via class-attribute lookup into a bound method on instance access, silently prepending the synthetic instance as an implicit first argument -- but monitored_update() takes none, so every call raised "monitored_update() takes 0 positional arguments but 1 was given", was caught by run_scheduled_updates' try/except, and recorded as an update failure. self.resource_monitor is None by default and was dormant until PR #388 ("activate dormant plugin health/metrics subsystem") wired it up in both display_controller.py and web_interface/app.py -- meaning this bug went live in every real deployment as of that merge (2026-07-09) despite the buggy line itself dating back to 2025-12-27. In practice this means no plugin's update() has succeeded since upgrading past #388: circuit breakers cycle through half-open -> immediate failure -> reopened every health-check interval forever, and all plugin data (scores, odds, prices, etc.) goes stale from whatever was last fetched before the upgrade. Confirmed live on a running instance: odds-ticker (and stock-news, ledmatrix-stocks, baseball-scoreboard, ledmatrix-leaderboard, of-the-day) failing this exact way every 5-minute circuit-breaker retry. Fixed by using types.SimpleNamespace(update=monitored_update) instead of a dynamic class: SimpleNamespace stores attributes on the instance itself, so attribute lookup returns the plain function unchanged -- never routed through the class-attribute descriptor protocol that injects an implicit self. Added test_run_scheduled_updates_calls_update_with_resource_monitor to test/test_plugin_system.py using a real PluginResourceMonitor (not a mock of it), so the test exercises the actual descriptor-binding behavior that caused this. Verified the test fails with the exact reported error against the pre-fix code and passes against the fix. --- src/plugin_system/plugin_manager.py | 13 +++++++- test/test_plugin_system.py | 51 ++++++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/src/plugin_system/plugin_manager.py b/src/plugin_system/plugin_manager.py index 22f8b470e..9a2512dfc 100644 --- a/src/plugin_system/plugin_manager.py +++ b/src/plugin_system/plugin_manager.py @@ -12,6 +12,7 @@ import subprocess import time import threading +import types from pathlib import Path from typing import Dict, List, Optional, Any import logging @@ -828,8 +829,18 @@ def run_scheduled_updates(self, current_time: Optional[float] = None) -> None: # If resource monitor exists, wrap the call def monitored_update(): self.resource_monitor.monitor_call(plugin_id, plugin_instance.update) + # SimpleNamespace stores `update` as an *instance* + # attribute, so attribute lookup returns the plain + # function object as-is. A dynamically-built class + # (`type(..., {'update': monitored_update})`) instead + # stores it as a *class* attribute, which the + # descriptor protocol turns into a bound method on + # access -- silently prepending the instance as an + # implicit first argument to a function that takes + # none, raising "monitored_update() takes 0 + # positional arguments but 1 was given" on every call. success = self.plugin_executor.execute_update( - type('obj', (object,), {'update': monitored_update})(), + types.SimpleNamespace(update=monitored_update), plugin_id ) else: diff --git a/test/test_plugin_system.py b/test/test_plugin_system.py index b78a31565..ae44d7109 100644 --- a/test/test_plugin_system.py +++ b/test/test_plugin_system.py @@ -3,6 +3,7 @@ from pathlib import Path from src.plugin_system.plugin_manager import PluginManager from src.plugin_system.plugin_state import PluginState +from src.plugin_system.resource_monitor import PluginResourceMonitor class TestPluginManager: """Test PluginManager functionality.""" @@ -74,10 +75,58 @@ def test_load_plugin_missing_manifest(self, mock_config_manager, mock_display_ma # No manifest in pm.plugin_manifests result = pm.load_plugin("non_existent_plugin") - + assert result is False assert pm.state_manager.get_state("non_existent_plugin") == PluginState.ERROR + def test_run_scheduled_updates_calls_update_with_resource_monitor( + self, mock_config_manager, mock_display_manager, mock_cache_manager + ): + """Regression test: run_scheduled_updates() must actually call a + plugin's update() when self.resource_monitor is set (as it is in + every real deployment -- display_controller.py and web_interface/ + app.py both assign a real PluginResourceMonitor after construction). + + Previously, the resource_monitor branch wrapped the call in a + function stored as a *class* attribute on a dynamically-built type + (`type('obj', (object,), {'update': monitored_update})()`), which + the descriptor protocol turns into a bound method on access -- + silently passing the synthetic instance as an implicit first + argument to monitored_update(), which takes none. Every plugin's + scheduled update failed with "monitored_update() takes 0 positional + arguments but 1 was given" and was silently swallowed into a + circuit-breaker retry loop that never succeeded, so plugin data + (scores, odds, etc.) never refreshed. + """ + with patch('src.plugin_system.plugin_manager.ensure_directory_permissions'): + pm = PluginManager( + plugins_dir="plugins", + config_manager=mock_config_manager, + display_manager=mock_display_manager, + cache_manager=mock_cache_manager + ) + + plugin_instance = MagicMock() + plugin_instance.enabled = True + plugin_instance.update = MagicMock() + + pm.plugins["test_plugin"] = plugin_instance + pm.plugin_manifests["test_plugin"] = {"update_interval": 10} + pm.state_manager.set_state("test_plugin", PluginState.ENABLED) + # Plain MagicMock, not the mock_cache_manager fixture: this test + # is about run_scheduled_updates() actually invoking update() + # through the resource-monitor wrapper, not about + # PluginResourceMonitor's own cache-backed metrics persistence + # (which calls cache_manager.get(..., memory_ttl=...) -- + # a kwarg the fixture's mock_get() doesn't accept). + pm.resource_monitor = PluginResourceMonitor(MagicMock()) + + pm.run_scheduled_updates(current_time=time.time()) + + plugin_instance.update.assert_called_once() + assert "test_plugin" in pm.plugin_last_update + assert pm.state_manager.get_state("test_plugin") == PluginState.ENABLED + class TestPluginLoader: """Test PluginLoader functionality."""