rick/LEDMatrix
1
0
Fork 0
mirror of https://github.com/ChuckBuilds/LEDMatrix.git synced 2026-08-07 03:38:06 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(plugins): make update scheduling atomic so update() cannot run twice at once (#437)

Browse Source 
* fix(plugins): make update scheduling atomic so update() cannot run twice 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

* test: fix a race in the reservation suite's own wait loops

`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

* fix(plugins): roll back the claim when dispatch fails

_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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chuck
2026-08-05 19:38:11 -04:00
committed by GitHub
co-authored by Claude Opus 5
parent 5b81cca684
commit 2af41c561b
3 changed files with 448 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/plugin_system/plugin_manager.py
+110 -28
View File
@@ -113,6 +113,15 @@ class PluginManager:
self._update_queue: "queue.Queue[Optional[Tuple[str, float]]]" = queue.Queue()
self._pending_updates: set = set()
self._pending_lock = threading.Lock()
# Serializes the "is this plugin eligible?" -> "claim it (RUNNING)"
# transition. Two schedulers run concurrently in practice — the render
# loop's _tick_plugin_updates() and Vegas mode's vegas-plugin-tick
# daemon thread, which is never joined — so without this both can
# observe ENABLED and both call update() on the same plugin. Held only
# across the check and the state transition, never across update()
# itself: that would serialize slow plugins behind each other and
# reintroduce the stall the async worker exists to avoid.
self._reservation_lock = threading.Lock()
self._plugin_locks: Dict[str, threading.Lock] = {}
self._plugin_locks_guard = threading.Lock()
self._update_worker: Optional[threading.Thread] = None
@@ -808,25 +817,69 @@ class PluginManager:
if self.health_tracker and self.health_tracker.should_skip_plugin(plugin_id):
continue
# Check if plugin can execute
if not self.state_manager.can_execute(plugin_id):
continue
interval = self._get_plugin_update_interval(plugin_id, plugin_instance)
if interval is None:
continue
with self._plugin_last_update_lock:
last_update = self.plugin_last_update.get(plugin_id, 0.0)
# Eligibility check, due check and the RUNNING transition happen
# together, so a concurrent scheduler cannot claim the same plugin.
if not self._reserve_for_update(plugin_id, current_time, interval):
continue
if last_update == 0.0 or (current_time - last_update) >= interval:
if self._synchronous_updates:
# Kill-switch path: the original inline execution
# (blocks the caller until update() completes/times out)
self.state_manager.set_state(plugin_id, PluginState.RUNNING)
self._execute_update_now(plugin_id, plugin_instance, current_time)
else:
self._enqueue_update(plugin_id, current_time)
if self._synchronous_updates:
# Kill-switch path: the original inline execution
# (blocks the caller until update() completes/times out)
self._execute_update_now(plugin_id, plugin_instance, current_time)
else:
self._enqueue_update(plugin_id, current_time)
def _reserve_for_update(
self,
plugin_id: str,
current_time: Optional[float] = None,
interval: Optional[float] = None,
) -> bool:
"""Atomically claim a plugin for update, returning True if we won it.
can_execute() and the RUNNING transition have to happen under one lock.
As two separate calls, two scheduler threads can both see ENABLED and
both go on to run the same plugin's update() concurrently — unsafe for
any plugin that isn't reentrant (shared mutable state, a non-thread-safe
HTTP session or cache).
The due-time check is inside the lock too. Leaving it outside would let
a second thread that had already decided "due" claim the plugin the
instant the first finished, running update() twice in one interval.
Args:
plugin_id: Plugin to claim.
current_time: Now, for the due check. Omit to skip that check.
interval: Seconds between updates. Omit to skip the due check.
Returns:
True if this caller reserved the plugin and must dispatch it,
False if it is ineligible, not yet due, or already claimed.
"""
with self._reservation_lock:
if not self.state_manager.can_execute(plugin_id):
return False
if current_time is not None and interval is not None:
with self._plugin_last_update_lock:
last_update = self.plugin_last_update.get(plugin_id, 0.0)
if last_update != 0.0 and (current_time - last_update) < interval:
return False
self.state_manager.set_state(plugin_id, PluginState.RUNNING)
return True
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.
"""
self.state_manager.set_state(plugin_id, PluginState.ENABLED)
def get_plugin_lock(self, plugin_id: str) -> threading.Lock:
"""Per-plugin lock keeping update() and display() mutually exclusive.
@@ -843,16 +896,40 @@ class PluginManager:
return lock
def _enqueue_update(self, plugin_id: str, scheduled_time: float) -> None:
"""Queue a due update for the background worker (dedup while pending)."""
"""Queue an already-reserved update for the background worker.
The caller has reserved the plugin (RUNNING), which is what blocks
re-entry and shows the truthful state in the web UI while the item
waits its turn. The pending set stays as a second line of defence; if
it ever fires the reservation has to be handed back, or the plugin
would sit in RUNNING with nothing queued to release it.
"""
with self._pending_lock:
if plugin_id in self._pending_updates:
self.logger.warning(
"Plugin %s reserved for update but already queued; "
"releasing the reservation", plugin_id)
self._release_reservation(plugin_id)
return
self._pending_updates.add(plugin_id)
# RUNNING is set at enqueue time so can_execute() blocks re-entry and
# the web UI shows the truthful state while the item waits its turn.
self.state_manager.set_state(plugin_id, PluginState.RUNNING)
self._ensure_update_worker()
self._update_queue.put((plugin_id, scheduled_time))
try:
self._ensure_update_worker()
self._update_queue.put((plugin_id, scheduled_time))
except Exception as exc: # pylint: disable=broad-except
# Thread.start() raises RuntimeError when the OS refuses a new
# thread — a real condition on a Pi under memory pressure. Nothing
# is queued to release the plugin at that point, so the claim has to
# be undone here, or it sits in RUNNING with nothing to clear it and
# can_execute() refuses it for the rest of the process. Swallowed
# rather than raised so the remaining plugins in this tick still get
# their turn.
self.logger.error(
"Could not queue update for plugin %s (%s: %s); releasing the "
"reservation so the next tick can retry",
plugin_id, type(exc).__name__, exc, exc_info=True)
with self._pending_lock:
self._pending_updates.discard(plugin_id)
self._release_reservation(plugin_id)
def _ensure_update_worker(self) -> None:
if self._update_worker is not None and self._update_worker.is_alive():
@@ -937,6 +1014,14 @@ class PluginManager:
return
finished['done'] = True
try:
# Drop the queue reservation *before* the state goes back to
# ENABLED. The other order leaves a window where a scheduler
# sees ENABLED, reserves the plugin, then finds it still in
# _pending_updates -- the enqueue is dropped and the plugin
# would sit in RUNNING with nothing left to release it.
if lock is not None:
with self._pending_lock:
self._pending_updates.discard(plugin_id)
if success:
with self._plugin_last_update_lock:
self.plugin_last_update[plugin_id] = scheduled_time
@@ -949,8 +1034,6 @@ class PluginManager:
finally:
if lock is not None:
lock.release()
with self._pending_lock:
self._pending_updates.discard(plugin_id)
if lock is None:
# Synchronous / no-lock path: unchanged behavior.
@@ -1041,13 +1124,12 @@ class PluginManager:
if not hasattr(plugin_instance, "update"):
continue
# Check if plugin can execute
if not self.state_manager.can_execute(plugin_id):
# Eligibility check and the RUNNING transition together, so a
# concurrent scheduler cannot claim the same plugin (see
# _reserve_for_update).
if not self._reserve_for_update(plugin_id):
continue
# Update state to RUNNING
self.state_manager.set_state(plugin_id, PluginState.RUNNING)
try:
success = self.plugin_executor.execute_update(plugin_instance, plugin_id)
if success: