fix(vegas): lock plugin_last_update snapshot/diff against concurrent mutation#398
Merged
Merged
Conversation
Investigating a user report that Vegas scroll mode doesn't update scores or game status. Root cause: PR #299 (Mar 28) added a mechanism so a live score change reached the ticker within a few seconds instead of waiting for a full scroll cycle -- _tick_plugin_updates_for_vegas() diffed plugin_last_update timestamps to detect which plugins got fresh data and called coordinator.mark_plugin_updated() for each, and should_recompose() checked has_pending_updates_for_visible_segments() to trigger an immediate hot-swap. PR #330 (May 14, multi-display wireless sync) refactored both call sites while adding sync support and silently deleted this entire mechanism -- not just gated it behind the new sync-mode deferral it legitimately needed, but removed it outright. The result: VegasModeCoordinator. mark_plugin_updated() and StreamManager.has_pending_updates_for_visible_ segments() have been fully implemented but never called from anywhere since. Vegas mode's only remaining freshness sources are a 5s content cache TTL (fine) and full recompose at cycle boundaries, which depending on min/max_cycle_duration can be minutes away -- so live scores/status can sit stale far longer than a user would expect from a "live" ticker. Fix: - Restored _tick_plugin_updates_for_vegas() in display_controller.py, wired as the Vegas coordinator's update callback in place of the plain _tick_plugin_updates(). Diffs plugin_last_update before/after the tick and calls vegas_coordinator.mark_plugin_updated(plugin_id) for each plugin that actually got new data (rather than returning the list, since the callback interface no longer consumes a return value). - Restored the has_pending_updates_for_visible_segments() check in render_pipeline.should_recompose(), positioned after (not instead of) the sync-mode early return PR #330 added, so standalone installations regain immediate refresh while synced leader/follower pairs correctly keep deferring hot-swaps to cycle boundaries as PR #330 intended. Test plan: - Added test_display_controller_vegas_tick.py and test_vegas_render_pipeline_recompose.py -- neither area had any prior test coverage, which is very likely why this regression went unnoticed for ~2.5 months. - Verified both new test files fail against the pre-fix code (swapped in the current main versions of both files) with exactly the expected errors -- AttributeError for the deleted method, and the recompose assertion returning False instead of True -- then pass against the fix. - Confirmed the sync-mode deferral this restoration must not break still holds: test_sync_active_defers_pending_updates_to_cycle_boundary. - Full related suite (test_vegas_plugin_adapter, test_vegas_config, test_display_controller_plugin_toggle, test_display_controller_ optimizations, test_plugin_system): 108 passed, 1 pre-existing failure unrelated to this change (test_circuit_breaker, stale mock signature). - Full CI plugin-safety suite (test_harness, test_visual_rendering, test_plugin_matrix): 52 passed, 2 pre-existing skips.
…mutation _tick_plugin_updates_for_vegas() snapshotted and later re-iterated plugin_manager.plugin_last_update from the Vegas background update-tick thread while the main render loop (or other callers) could mutate the same dict concurrently — a real race (unprotected dict iteration/mutation across threads), not just a style nit. Move the snapshot/update/diff into a new locked PluginManager.run_scheduled_updates_with_changes() so all reads and mutations of plugin_last_update happen under one lock, and update DisplayController to use it. The lock is only held around the dict accesses, not the update pass itself, so slow plugin update() calls don't serialize against other callers. Also add a regression test covering that the Vegas coordinator is wired to the Vegas-aware tick callback rather than the plain one. Skipped as not worth the change: - Narrowing the broad `except Exception` around vc.mark_plugin_updated(plugin_id) to specific types: it's a deliberate per-plugin isolation boundary (matches the same pattern used elsewhere in this file for plugin/coordinator calls) and there's no documented, stable set of exceptions that call can raise to narrow to. - Adding an inactive-DisplaySyncManager test to test_vegas_render_pipeline_recompose.py: verified VegasModeCoordinator.set_sync_manager() already normalizes a SyncRole.STANDALONE manager to None before handing it to the render pipeline (src/vegas_mode/coordinator.py:152-156), so should_recompose()'s `is not None` check is correct in practice; the suggested case is already covered by that normalization.
📝 WalkthroughWalkthroughPluginManager now synchronizes ChangesVegas plugin update tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant VegasModeCoordinator
participant DisplayController
participant PluginManager
VegasModeCoordinator->>DisplayController: trigger update callback
DisplayController->>PluginManager: run_scheduled_updates_with_changes()
PluginManager-->>DisplayController: updated plugin IDs
DisplayController->>VegasModeCoordinator: mark_plugin_updated(plugin ID)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 | 0 |
| 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.
# Conflicts: # src/display_controller.py # test/test_display_controller_vegas_tick.py
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugin_system/plugin_manager.py (1)
715-787: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake the update reservation atomic in
run_scheduled_updates()can_execute()andset_state(..., RUNNING)are separate calls, so the background tick and render loop can both seeENABLEDand enterupdate()for the same plugin at once. Reserve the plugin under one per-plugin lock beforeexecute_update(), and roll back on failure.🤖 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 715 - 787, Make the eligibility check and RUNNING transition in run_scheduled_updates atomic by acquiring the existing per-plugin lock before can_execute(), rechecking eligibility while holding it, and setting PluginState.RUNNING before releasing or executing. Ensure competing scheduler/render paths use the same reservation lock, and roll the reservation back to ENABLED (or the established failure state) when execute_update() fails or raises, while preserving successful update bookkeeping.
🤖 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.
Outside diff comments:
In `@src/plugin_system/plugin_manager.py`:
- Around line 715-787: Make the eligibility check and RUNNING transition in
run_scheduled_updates atomic by acquiring the existing per-plugin lock before
can_execute(), rechecking eligibility while holding it, and setting
PluginState.RUNNING before releasing or executing. Ensure competing
scheduler/render paths use the same reservation lock, and roll the reservation
back to ENABLED (or the established failure state) when execute_update() fails
or raises, while preserving successful update bookkeeping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d470f057-6c61-43fa-b1fa-caec541130f1
📒 Files selected for processing (3)
src/display_controller.pysrc/plugin_system/plugin_manager.pytest/test_display_controller_vegas_tick.py
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
2 participants
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.
Summary
Follow-up to review feedback on #395 (
fix/vegas-live-refresh). This branch is based on that PR's tip and addresses the actionable and nitpick comments left on it._tick_plugin_updates_for_vegas()snapshotted and later re-iteratedplugin_manager.plugin_last_updatefrom Vegas mode's background update-tick thread (vegas-plugin-tick) while the main render loop / other callers can mutate the same dict concurrently. That's a real unprotected-iteration race (RuntimeError: dictionary changed size during iterationor lost updates), not just style.PluginManager.run_scheduled_updates_with_changes(): snapshotsplugin_last_update, runs the scheduled update pass, and diffs it — with the before/after dict accesses each individually lock-protected via a new_plugin_last_update_lock. The lock is only held around the dict snapshot/diff, not across the update pass itself, so slow/blocking pluginupdate()calls don't serialize against otherplugin_last_updatereaders.plugin_last_updatemutation sites inPluginManager(load/unload,run_scheduled_updates,update_all_plugins, failure recording) now go through the same lock.DisplayController._tick_plugin_updates_for_vegas()now delegates to the new helper instead of doing its own snapshot/diff, and got the missing-> Nonereturn annotation._tick_plugin_updates_for_vegas(the Vegas-aware callback) rather than the plain_tick_plugin_updates— this is the exact wiring the original regression (feat(sync): multi-display wireless sync — extend scrolling across two LED matrices #330) silently dropped.test_display_controller_vegas_tick.pytests to match the newrun_scheduled_updates_with_changes()-based flow, with type annotations on the test helper.Skipped (with reason)
except Exceptionaroundvc.mark_plugin_updated(plugin_id)to specific exception types: this is a deliberate per-plugin isolation boundary (same pattern used elsewhere in this file for plugin/coordinator calls), and there's no documented, stable set of exceptions that call can raise to narrow to safely.DisplaySyncManagercase totest_vegas_render_pipeline_recompose.py: verifiedVegasModeCoordinator.set_sync_manager()already normalizes aSyncRole.STANDALONEmanager toNonebefore handing it to the render pipeline (src/vegas_mode/coordinator.py:152-156), soshould_recompose()'sis not Nonecheck is correct in practice — the suggested case is already covered by that normalization, so a dedicated test adds little value.Test plan
EMULATOR=true pytest test/test_display_controller_vegas_tick.py test/test_vegas_render_pipeline_recompose.py -v --no-cov: 11 passed.EMULATOR=true pytest test/test_vegas_plugin_adapter.py test/test_vegas_config.py test/test_display_controller_plugin_toggle.py test/test_display_controller_optimizations.py test/test_plugin_system.py --no-cov: 108 passed, 1 pre-existing unrelated failure (test_circuit_breaker, stale mock signature — already documented in prior PRs).EMULATOR=true pytest test/plugins/test_harness.py test/plugins/test_visual_rendering.py test/plugins/test_plugin_matrix.py --no-cov: 56 passed.Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Tests