From 7b50a2db59a42b5f9c36970381b96d86f4264949 Mon Sep 17 00:00:00 2001 From: Chuck Date: Wed, 29 Apr 2026 11:50:54 -0400 Subject: [PATCH] fix(plugin_state): lock _error_info accesses and store defensive copies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three verified issues: - set_error_info wrote _error_info without holding _lock and stored the caller's dict by reference, allowing races and post-write mutation - set_state_with_error stored error_info by reference (lock was already held) - get_error_info read _error_info without _lock and returned the live reference, letting callers mutate the stored snapshot Implicit fourth fix: set_state also wrote _error_info without _lock; locking get_error_info while leaving that writer unguarded would have created a new race, so set_state is now wrapped in _lock too for consistency. Changes: - set_state: wrap entire body in self._lock (covers _states, _state_history, and _error_info writes atomically; ERROR-path _error_info value was already a fresh dict literal so no copy needed) - set_error_info: acquire self._lock + store dict(error_info) shallow copy - set_state_with_error: store dict(error_info) shallow copy (lock already held) - get_error_info: acquire self._lock + return dict(info) copy or None All stored values are flat dicts of strings/floats/bools, so shallow copy is sufficient — deepcopy is not needed. Co-Authored-By: Claude Sonnet 4.6 --- src/plugin_system/plugin_state.py | 78 ++++++++++++++++--------------- 1 file changed, 41 insertions(+), 37 deletions(-) diff --git a/src/plugin_system/plugin_state.py b/src/plugin_system/plugin_state.py index 1d595d26..269bb423 100644 --- a/src/plugin_system/plugin_state.py +++ b/src/plugin_system/plugin_state.py @@ -50,44 +50,44 @@ class PluginStateManager: ) -> None: """ Set plugin state and record transition. - + Args: plugin_id: Plugin identifier state: New state error: Optional error if transitioning to ERROR state """ - old_state = self._states.get(plugin_id, PluginState.UNLOADED) - self._states[plugin_id] = state - - # Record state transition - if plugin_id not in self._state_history: - self._state_history[plugin_id] = [] - - transition = { - 'timestamp': datetime.now(), - 'from': old_state.value, - 'to': state.value, - 'error': str(error) if error else None - } - self._state_history[plugin_id].append(transition) - - # Store error info if transitioning to ERROR state - if state == PluginState.ERROR and error: - self._error_info[plugin_id] = { - 'error': str(error), - 'error_type': type(error).__name__, - 'timestamp': datetime.now() + with self._lock: + old_state = self._states.get(plugin_id, PluginState.UNLOADED) + self._states[plugin_id] = state + + if plugin_id not in self._state_history: + self._state_history[plugin_id] = [] + + transition = { + 'timestamp': datetime.now(), + 'from': old_state.value, + 'to': state.value, + 'error': str(error) if error else None } - elif state != PluginState.ERROR: - # Clear error info when leaving ERROR state - self._error_info.pop(plugin_id, None) - - self.logger.debug( - "Plugin %s state transition: %s → %s", - plugin_id, - old_state.value, - state.value - ) + self._state_history[plugin_id].append(transition) + + # Store error info if transitioning to ERROR state + if state == PluginState.ERROR and error: + self._error_info[plugin_id] = { + 'error': str(error), + 'error_type': type(error).__name__, + 'timestamp': datetime.now() + } + elif state != PluginState.ERROR: + # Clear error info when leaving ERROR state + self._error_info.pop(plugin_id, None) + + self.logger.debug( + "Plugin %s state transition: %s → %s", + plugin_id, + old_state.value, + state.value + ) def get_state(self, plugin_id: str) -> PluginState: """ @@ -149,7 +149,8 @@ class PluginStateManager: plugin_id: Plugin identifier error_info: Arbitrary dict describing the error """ - self._error_info[plugin_id] = error_info + with self._lock: + self._error_info[plugin_id] = dict(error_info) def set_state_with_error( self, @@ -187,7 +188,7 @@ class PluginStateManager: 'error': str(error) if error else None, }) - self._error_info[plugin_id] = error_info + self._error_info[plugin_id] = dict(error_info) self.logger.debug( "Plugin %s state transition: %s → %s (recoverable error stored)", @@ -201,15 +202,18 @@ class PluginStateManager: Get error information for a plugin. Returns the stored error dict whether the plugin is in ERROR state or - still ENABLED after a recoverable failure. + still ENABLED after a recoverable failure. Returns a shallow copy so + callers cannot mutate the stored snapshot. Args: plugin_id: Plugin identifier Returns: - Error information dict or None + Copy of the error information dict, or None """ - return self._error_info.get(plugin_id) + with self._lock: + info = self._error_info.get(plugin_id) + return dict(info) if info is not None else None def record_update(self, plugin_id: str) -> None: """Record that plugin update() was called."""