fix(plugin-manager): fix TypeError breaking every plugin's scheduled update (#392)

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.
This commit is contained in:
Chuck
2026-07-11 08:54:30 -04:00
committed by GitHub
parent 978a03b42d
commit aab0e9ade0
2 changed files with 62 additions and 2 deletions
+12 -1
View File
@@ -12,6 +12,7 @@ import sys
import subprocess import subprocess
import time import time
import threading import threading
import types
from pathlib import Path from pathlib import Path
from typing import Dict, List, Optional, Any from typing import Dict, List, Optional, Any
import logging import logging
@@ -828,8 +829,18 @@ class PluginManager:
# If resource monitor exists, wrap the call # If resource monitor exists, wrap the call
def monitored_update(): def monitored_update():
self.resource_monitor.monitor_call(plugin_id, plugin_instance.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( success = self.plugin_executor.execute_update(
type('obj', (object,), {'update': monitored_update})(), types.SimpleNamespace(update=monitored_update),
plugin_id plugin_id
) )
else: else:
+50 -1
View File
@@ -3,6 +3,7 @@ from unittest.mock import MagicMock, patch
from pathlib import Path from pathlib import Path
from src.plugin_system.plugin_manager import PluginManager from src.plugin_system.plugin_manager import PluginManager
from src.plugin_system.plugin_state import PluginState from src.plugin_system.plugin_state import PluginState
from src.plugin_system.resource_monitor import PluginResourceMonitor
class TestPluginManager: class TestPluginManager:
"""Test PluginManager functionality.""" """Test PluginManager functionality."""
@@ -74,10 +75,58 @@ class TestPluginManager:
# No manifest in pm.plugin_manifests # No manifest in pm.plugin_manifests
result = pm.load_plugin("non_existent_plugin") result = pm.load_plugin("non_existent_plugin")
assert result is False assert result is False
assert pm.state_manager.get_state("non_existent_plugin") == PluginState.ERROR 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: class TestPluginLoader:
"""Test PluginLoader functionality.""" """Test PluginLoader functionality."""