fix(plugins): make update scheduling atomic so update() cannot run twice at once - #437
Merged
Merged
Conversation
…ice at once Closes #401. `run_scheduled_updates()` decided whether to update a plugin with a check-then-act sequence: `can_execute()` and `set_state(RUNNING)` were separate calls with nothing between them, so two scheduler threads could both observe ENABLED and both go on to call the same plugin's `update()`. `update_all_plugins()` had the identical pattern. Two schedulers really do run at once. The render loop calls `_tick_plugin_updates()`, and Vegas mode fires its own `vegas-plugin-tick` daemon thread that is never joined when `VegasModeCoordinator.play()` returns — a slow `update()` still in flight overlaps the next tick from the main loop. A plugin running `update()` twice concurrently is unsafe unless it happens to be reentrant; shared mutable state, a non-thread-safe HTTP session or cache all break. The async path was already covered by the `_pending_lock` dedup in `_enqueue_update`, so the live exposure was the synchronous kill-switch path and `update_all_plugins()`. Both now claim the plugin through `_reserve_for_update()`, which holds one lock across the eligibility check, the due-time check and the RUNNING transition — and nothing more. Holding it across `execute_update()` would serialize slow plugins behind each other and reintroduce the render stall the async worker exists to avoid. The due-time check moved inside the lock deliberately. Left outside, a thread that had already decided "due" could claim the plugin the instant the winner finished, running `update()` twice within one interval. Two supporting changes fall out of it: - `_enqueue_update()` no longer sets RUNNING (the reservation did), and hands the reservation back if the pending-dedup ever fires. Otherwise a reserved-but-unqueued plugin would sit in RUNNING with nothing left to release it, and `can_execute()` would refuse it forever. - `_finish()` now clears the pending entry *before* flipping the state back to ENABLED. The old order left a window where a scheduler saw ENABLED, reserved the plugin, then had its enqueue silently dropped by the dedup — harmless as a missed tick before, a stuck plugin once a reservation is involved. Regression suite added and enrolled in CI, along with test_async_plugin_updates.py which was not previously run there. The overlap tests delay `can_execute()` to hold every thread inside the check-then-act gap: the real window is a couple of bytecodes wide, so a plain hammering test passes against the unfixed scheduler and proves nothing. With that delay the suite reports `update() ran 8x concurrently` on both affected paths before the fix, and passes after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Contributor
📝 WalkthroughWalkthroughPlugin update scheduling now reserves eligible plugins atomically. Synchronous, asynchronous, and bulk paths use the reservation mechanism. Completion and dispatch failure paths clear pending bookkeeping and restore plugin state. New concurrency tests and workflow entries validate these paths. ChangesPlugin update concurrency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant PluginManager
participant UpdateQueue
participant PluginExecutor
Scheduler->>PluginManager: request plugin update
PluginManager->>PluginManager: reserve eligible plugin
PluginManager->>UpdateQueue: queue reserved update
UpdateQueue->>PluginExecutor: execute update
PluginExecutor-->>PluginManager: report completion or dispatch failure
PluginManager->>PluginManager: clear pending state and restore plugin state
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 9 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/plugin_system/plugin_manager.py (1)
876-882: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winOnly hand back a reservation that is still RUNNING.
_release_reservationwrites ENABLED unconditionally. Ifunload_pluginruns between the reservation and the release, it already set UNLOADED and calledclear_state. The release then recreates an ENABLED state entry for a plugin that is no longer inself.plugins, andget_plugin_statereports that stale entry to the web interface. Guard the transition with the reservation lock and a RUNNING check.♻️ Proposed guard
def _release_reservation(self, plugin_id: str) -> None: """Hand a claimed plugin back when it never got dispatched. Without this a plugin reserved but not queued would sit in RUNNING forever, and can_execute() would refuse it on every later tick. + + Only a plugin still in RUNNING is handed back; a plugin unloaded + while reserved keeps the state unload_plugin() left it in. """ - self.state_manager.set_state(plugin_id, PluginState.ENABLED) + with self._reservation_lock: + if self.state_manager.is_running(plugin_id): + self.state_manager.set_state(plugin_id, PluginState.ENABLED)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin_system/plugin_manager.py` around lines 876 - 882, Update _release_reservation to acquire the reservation lock and transition the plugin to ENABLED only when its current state is still RUNNING; otherwise leave the state untouched so unload_plugin cannot recreate a stale state entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugin_system/plugin_manager.py`:
- Around line 907-916: Update _enqueue_update to handle failures from
_ensure_update_worker or _update_queue.put by removing plugin_id from
_pending_updates, releasing its reservation via _release_reservation, and
logging the exception cause with self.logger; contain the failure so
run_scheduled_updates can continue processing remaining plugins.
In `@test/test_plugin_update_reservation.py`:
- Around line 203-209: Update the async wait loop around plugin._active to wait
until the update has started and the pending work has drained, rather than
exiting while _active is still false. Use the existing pending/update queue
state alongside plugin._active, retain the timeout, and assert that
plugin.max_concurrent confirms update() actually ran before checking its
concurrency value.
---
Nitpick comments:
In `@src/plugin_system/plugin_manager.py`:
- Around line 876-882: Update _release_reservation to acquire the reservation
lock and transition the plugin to ENABLED only when its current state is still
RUNNING; otherwise leave the state untouched so unload_plugin cannot recreate a
stale state entry.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b0e9f73c-2b4b-4551-a09b-851716898103
📒 Files selected for processing (3)
.github/workflows/test.ymlsrc/plugin_system/plugin_manager.pytest/test_plugin_update_reservation.py
`test_async_path_never_overlaps` failed in CI with "update() ran 0x concurrently" — the test's bug, not the scheduler's. It polled `plugin._active` to wait for the update to finish, but before the worker picks the item up nothing is active yet, so the loop fell straight through and asserted on a plugin that had never run. Both async waits now key on `update_calls >= 1` as well, so they wait for an update to have started *and* finished. The stranded-state test gets the same guard for a second reason: ENABLED is also the starting state, so without it that assertion passes vacuously on a plugin that was never scheduled. Verified over 12 consecutive local runs, 12 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
_enqueue_update() reserved the plugin and added it to the pending set, then started the worker and queued the item. Thread.start() raises RuntimeError when the OS refuses a new thread — not hypothetical on a Pi under memory or thread pressure — and nothing is queued at that point to release the plugin. It stayed RUNNING with a stale pending entry, so can_execute() refused it for the rest of the process, and the exception escaped run_scheduled_updates() and skipped every remaining plugin in that tick. That is the same stranded-RUNNING failure the reservation was introduced to prevent, just reached through the dispatch rather than the dedup, so it is handled the same way: discard the pending entry, hand the reservation back, log the cause. Swallowed rather than raised so one plugin failing to queue cannot abort the others' turn. Both new tests fail against the un-rolled-back version — the second on the escaping RuntimeError itself — and pass with it. 74 tests across the reservation, async-update, plugin-system, health, Vegas-adapter and controller-toggle suites still pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WexvwNDtWLVymGVqKD7BGk
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/test_plugin_update_reservation.py (1)
290-325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required type annotations.
Annotate
pmand-> Noneon both test methods. Add-> Nonetorefuse_to_startandfail_first_only. Confirm the availablePluginManagerimport or type alias before annotatingpm.As per coding guidelines, “Use type hints for function parameters and return values.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_plugin_update_reservation.py` around lines 290 - 325, Update the two test methods test_reservation_released_when_the_worker_cannot_start and test_dispatch_failure_does_not_abort_the_rest_of_the_tick with the appropriate PluginManager type annotation for pm and a -> None return annotation. Also annotate the nested refuse_to_start and fail_first_only helpers with -> None, reusing the available PluginManager import or type alias.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/test_plugin_update_reservation.py`:
- Around line 290-325: Update the two test methods
test_reservation_released_when_the_worker_cannot_start and
test_dispatch_failure_does_not_abort_the_rest_of_the_tick with the appropriate
PluginManager type annotation for pm and a -> None return annotation. Also
annotate the nested refuse_to_start and fail_first_only helpers with -> None,
reusing the available PluginManager import or type alias.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 64225d9c-1690-41b1-998f-71fc2b63aa86
📒 Files selected for processing (2)
src/plugin_system/plugin_manager.pytest/test_plugin_update_reservation.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugin_system/plugin_manager.py
Owner
Author
|
On the remaining nitpick (type annotations on the two new The convention here is split, and the numbers are unambiguous:
Not one test function in the suite carries The one written guideline I can find is Happy to annotate the whole Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #401.
run_scheduled_updates()decided whether to update a plugin with a check-then-act sequence —can_execute()andset_state(RUNNING)were separate calls with nothing between them, so two scheduler threads could both observeENABLEDand both go on to call the same plugin'supdate().update_all_plugins()had the identical pattern.Two schedulers really do run at once: the render loop calls
_tick_plugin_updates(), and Vegas mode fires its ownvegas-plugin-tickdaemon thread that is never joined whenVegasModeCoordinator.play()returns, so a slowupdate()still in flight overlaps the next tick from the main loop.What was actually still exposed
Worth stating, because the codebase has moved since the issue was filed. The async path is already safe —
_enqueue_update()dedups under_pending_lockand a single worker thread runs the updates. The live exposure was:plugin_system.synchronous_updates: true) —set_state(RUNNING)then inline_execute_update_now(), no dedup, no lockupdate_all_plugins()— same raw pattern, fully inlineBoth now claim the plugin through
_reserve_for_update(), which holds one lock across the eligibility check, the due-time check and the RUNNING transition — and nothing more. Holding it acrossexecute_update()would serialize slow plugins behind each other and reintroduce the render stall the async worker exists to avoid.The due-time check moved inside the lock deliberately: left outside, a thread that had already decided "due" could claim the plugin the instant the winner finished, running
update()twice within one interval.Two supporting changes
Both are consequences of introducing a reservation, not drive-by edits:
_enqueue_update()no longer sets RUNNING (the reservation did), and hands the reservation back if the pending-dedup ever fires. Otherwise a reserved-but-unqueued plugin sits in RUNNING with nothing left to release it, andcan_execute()refuses it forever._finish()clears the pending entry before flipping the state back to ENABLED. The old order left a window where a scheduler saw ENABLED, reserved the plugin, then had its enqueue silently dropped by the dedup. That was a harmless missed tick before; with a reservation involved it would strand the plugin.Tests
The overlap tests delay
can_execute()to hold every thread inside the check-then-act gap. This matters: the real window is a couple of bytecodes wide, so a plain hammering test passes against the unfixed scheduler and proves nothing — I confirmed that before adding the delay. With it, againstmain:and with the fix,
9 passed. Coverage: reservation atomicity (16 threads, exactly one winner), refusal while RUNNING, hand-back, the due check, no overlap on all three dispatch paths, no stranded RUNNING state, and the pending/state ordering invariant.Also enrolls
test_async_plugin_updates.pyin CI — it existed but was never run there, which is how a scheduler refactor could regress the async guarantees silently.63 existing tests across
test_async_plugin_updates,test_plugin_system,test_plugin_health,test_vegas_plugin_adapterandtest_display_controller_plugin_togglestill pass.Not addressed
The issue notes the Vegas tick thread is a daemon that is never joined. This PR makes the race harmless rather than removing the overlap; joining that thread on
play()return is a separate change with its own shutdown-latency trade-off.🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Summary by CodeRabbit
Bug Fixes
Tests