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 <noreply@anthropic.com>
This commit is contained in:
Ron
2026-08-24 19:01:10 -07:00
co-authored by Claude Opus 5
parent f66e059fca
commit 745d13a201
2 changed files with 39 additions and 10 deletions
+15 -4
View File
@@ -167,11 +167,16 @@ class PluginStateManager:
plugin_id: Plugin identifier plugin_id: Plugin identifier
Returns: Returns:
List of recent state transitions, oldest first. A copy, so callers List of recent state transitions, oldest first. Both the list and
cannot mutate the manager's own history. 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: 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: def set_error_info(self, plugin_id: str, error_info: Dict[str, Any]) -> None:
""" """
@@ -290,7 +295,13 @@ class PluginStateManager:
return info return info
def clear_state(self, plugin_id: str) -> None: def clear_state(self, plugin_id: str) -> None:
"""Clear all state information for a plugin.""" """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._states.pop(plugin_id, None)
self._state_history.pop(plugin_id, None) self._state_history.pop(plugin_id, None)
self._state_transition_counts.pop(plugin_id, None) self._state_transition_counts.pop(plugin_id, None)
+18
View File
@@ -132,6 +132,24 @@ def test_get_state_history_returns_a_copy():
assert len(manager.get_state_history("clock")) == 1 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(): def test_clear_state_drops_history():
"""Unloading a plugin still releases everything it accumulated.""" """Unloading a plugin still releases everything it accumulated."""
manager = PluginStateManager() manager = PluginStateManager()