Background
Flagged by CodeRabbit during review of #398, deferred as out of scope for that PR (which only fixed a separate plugin_last_update dict-iteration race).
Problem
PluginManager.run_scheduled_updates() (src/plugin_system/plugin_manager.py:706-776) decides whether to update a plugin with a check-then-act sequence that isn't atomic:
if not self.state_manager.can_execute(plugin_id):
continue
...
self.state_manager.set_state(plugin_id, PluginState.RUNNING)
...
success = self.plugin_executor.execute_update(plugin_instance, plugin_id)can_execute() and set_state(..., RUNNING) are separate calls with no lock between them, so two threads can both observe ENABLED and both proceed to call plugin_instance.update() for the same plugin concurrently.
This is reachable in practice: Vegas mode (src/vegas_mode/coordinator.py) fires its own background vegas-plugin-tick thread (~every 4s while Vegas is active) that calls run_scheduled_updates() via DisplayController._tick_plugin_updates_for_vegas(). That thread is a daemon thread and is not joined when VegasModeCoordinator.play() returns — if it's still mid-flight (e.g. a slow plugin update() call) when the main render loop's next iteration calls the plain _tick_plugin_updates() (src/display_controller.py:1638), both threads can race into run_scheduled_updates() at once for the same plugin.
Impact
A plugin's update() could execute twice concurrently, which is unsafe for plugins that aren't reentrant (e.g. shared mutable state, non-thread-safe HTTP clients/caches). Low likelihood in practice (requires a slow plugin update overlapping the next tick) but a real correctness gap.
Suggested fix
Reserve the plugin under a per-plugin lock before can_execute()/set_state(RUNNING), holding it only across the eligibility check + state transition (not the full execute_update() call, to avoid serializing slow plugin updates), and roll the state back to ENABLED (or the existing failure state) if execute_update() raises or returns False. Needs the same reservation lock used by every caller of run_scheduled_updates()/update_all_plugins() to be effective.
This is a heavier change than a quick fix — worth its own design pass rather than folding into an unrelated PR.
Background
Flagged by CodeRabbit during review of #398, deferred as out of scope for that PR (which only fixed a separate
plugin_last_updatedict-iteration race).Problem
PluginManager.run_scheduled_updates()(src/plugin_system/plugin_manager.py:706-776) decides whether to update a plugin with a check-then-act sequence that isn't atomic:can_execute()andset_state(..., RUNNING)are separate calls with no lock between them, so two threads can both observeENABLEDand both proceed to callplugin_instance.update()for the same plugin concurrently.This is reachable in practice: Vegas mode (
src/vegas_mode/coordinator.py) fires its own backgroundvegas-plugin-tickthread (~every 4s while Vegas is active) that callsrun_scheduled_updates()viaDisplayController._tick_plugin_updates_for_vegas(). That thread is a daemon thread and is not joined whenVegasModeCoordinator.play()returns — if it's still mid-flight (e.g. a slow pluginupdate()call) when the main render loop's next iteration calls the plain_tick_plugin_updates()(src/display_controller.py:1638), both threads can race intorun_scheduled_updates()at once for the same plugin.Impact
A plugin's
update()could execute twice concurrently, which is unsafe for plugins that aren't reentrant (e.g. shared mutable state, non-thread-safe HTTP clients/caches). Low likelihood in practice (requires a slow plugin update overlapping the next tick) but a real correctness gap.Suggested fix
Reserve the plugin under a per-plugin lock before
can_execute()/set_state(RUNNING), holding it only across the eligibility check + state transition (not the fullexecute_update()call, to avoid serializing slow plugin updates), and roll the state back toENABLED(or the existing failure state) ifexecute_update()raises or returnsFalse. Needs the same reservation lock used by every caller ofrun_scheduled_updates()/update_all_plugins()to be effective.This is a heavier change than a quick fix — worth its own design pass rather than folding into an unrelated PR.