From 745d13a201ae12e16441e40a175fed6f99c6b96e Mon Sep 17 00:00:00 2001 From: Ron Date: Mon, 24 Aug 2026 19:01:10 -0700 Subject: [PATCH] fix(plugins): copy history entries out, lock clear_state Review follow-ups on the transition history. get_state_history() copied only the outer list, so a caller holding a returned transition could rewrite the manager's record of what happened -- which contradicted the defensive-copy guarantee in its own docstring. Copy each entry too. Every value in a transition is immutable, so a shallow copy per entry is enough. test_get_state_history_entries_are_copies pins it; without the change it fails with 'tampered' == 'enabled'. clear_state() mutated five shared dicts without holding _lock, while every other mutator takes it. A concurrent set_state() could interleave and leave a plugin with history but no state. Drop the five as one unit. This does not close the wider unload-vs-worker race, which lives in PluginManager.unload_plugin() and predates this change: an update worker still in flight can call set_state() after clear_state() returns and recreate the entry. Serialising that needs the per-plugin lock held across worker join in unload_plugin(), which is a separate change. Co-Authored-By: Claude Opus 5 --- src/plugin_system/plugin_state.py | 31 ++++++++++++++++++--------- test/test_plugin_state_history_cap.py | 18 ++++++++++++++++ 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/src/plugin_system/plugin_state.py b/src/plugin_system/plugin_state.py index d189d154..8b52823e 100644 --- a/src/plugin_system/plugin_state.py +++ b/src/plugin_system/plugin_state.py @@ -167,11 +167,16 @@ class PluginStateManager: plugin_id: Plugin identifier Returns: - List of recent state transitions, oldest first. A copy, so callers - cannot mutate the manager's own history. + List of recent state transitions, oldest first. Both the list and + the transition dicts are copies, so callers cannot mutate the + manager's own history. The values inside a transition are all + immutable, so a shallow copy per entry is enough. """ with self._lock: - return list(self._state_history.get(plugin_id, ())) + return [ + dict(transition) + for transition in self._state_history.get(plugin_id, ()) + ] def set_error_info(self, plugin_id: str, error_info: Dict[str, Any]) -> None: """ @@ -290,11 +295,17 @@ class PluginStateManager: return info def clear_state(self, plugin_id: str) -> None: - """Clear all state information for a plugin.""" - self._states.pop(plugin_id, None) - self._state_history.pop(plugin_id, None) - self._state_transition_counts.pop(plugin_id, None) - self._error_info.pop(plugin_id, None) - self._last_update.pop(plugin_id, None) - self._last_display.pop(plugin_id, None) + """Clear all state information for a plugin. + + Held under ``_lock`` so the five dicts are dropped as one unit: every + other mutator takes the lock, and without it a concurrent set_state() + could interleave and leave a plugin with history but no state. + """ + with self._lock: + self._states.pop(plugin_id, None) + self._state_history.pop(plugin_id, None) + self._state_transition_counts.pop(plugin_id, None) + self._error_info.pop(plugin_id, None) + self._last_update.pop(plugin_id, None) + self._last_display.pop(plugin_id, None) diff --git a/test/test_plugin_state_history_cap.py b/test/test_plugin_state_history_cap.py index ce1427cc..1d5e20d5 100644 --- a/test/test_plugin_state_history_cap.py +++ b/test/test_plugin_state_history_cap.py @@ -132,6 +132,24 @@ def test_get_state_history_returns_a_copy(): assert len(manager.get_state_history("clock")) == 1 +def test_get_state_history_entries_are_copies(): + """Copying the outer list is not enough -- the entries are handed out too. + + A caller holding a returned transition must not be able to rewrite the + manager's record of what happened. + """ + manager = PluginStateManager() + manager.set_state("clock", PluginState.ENABLED) + + entry = manager.get_state_history("clock")[0] + entry["to"] = "tampered" + entry["error"] = "injected" + + stored = manager.get_state_history("clock")[0] + assert stored["to"] == PluginState.ENABLED.value + assert stored["error"] is None + + def test_clear_state_drops_history(): """Unloading a plugin still releases everything it accumulated.""" manager = PluginStateManager()