Skip to content

fix(plugins): report updates that completed, not ones that were queued - #460

Open
ChuckBuilds wants to merge 1 commit into
mainfrom
fix/plugin-update-change-reporting
Open

fix(plugins): report updates that completed, not ones that were queued#460
ChuckBuilds wants to merge 1 commit into
mainfrom
fix/plugin-update-change-reporting

Conversation

@ChuckBuilds

@ChuckBuilds ChuckBuilds commented Aug 14, 2026

Copy link
Copy Markdown
Owner

The bug

run_scheduled_updates_with_changes() answers "which plugins have fresh data". It did so by snapshotting plugin_last_update, calling run_scheduled_updates(), and diffing the two.

But run_scheduled_updates() only enqueues:

if last_update == 0.0 or (current_time - last_update) >= interval:
    if self._synchronous_updates:
        self._execute_update_now(plugin_id, plugin_instance, current_time)   # inline
    else:
        self._enqueue_update(plugin_id, current_time)                        # queued

_enqueue_update() sets state and pushes to a queue. The update runs on the worker thread, and plugin_last_update is stamped there — after this method has already returned. So the second snapshot was always identical to the first, and the method returned [] every time. Only the synchronous kill-switch path ever worked, which is why the wiring reads as correct.

Why it matters

Vegas is the caller. _tick_plugin_updates_for_vegas() feeds that list to mark_plugin_updated(), which drops the cached content for a plugin whose data moved. With the list permanently empty, a segment kept scrolling whatever it was first built from — exactly what coordinator.py warns about in its own comments:

Without this the pending-update flags are never consumed and a segment keeps rendering whatever it was first built from — last night's live game still shown as live the next morning.

Measured on a live 512×64 rig: zero update ticks in twenty minutes, with weather, stocks and news all updating on schedule.

The fix

Report what has completed since the last poll rather than what this call enqueued. The worker records each finished update in a small ledger; the call drains it.

That costs one tick of latency — Vegas polls roughly every 4 seconds — and is correct regardless of which side of the queue the work lands on, so it keeps working if the sync/async default ever changes.

Failure paths are deliberately excluded. They stamp plugin_last_update too, to space out retries, but no fresh data exists.

Verification

On the rig where it was found: 0 update ticks before, 208 in twenty-five minutes after, naming real plugins (ledmatrix-flights, stock-news, geochron, news…).

A note on the tests, because the first version of them was worthless. The behavioural tests drive the ledger directly, so they passed with both production call sites deleted — mutation testing caught that, not review. There is now also a structural test asserting the invariant at the source: wherever a successful update stamps plugin_last_update, it must record the completion. Writing that immediately caught something I would have missed — _record_update_failure stamps the same field and must not be included.

Mutation-checked: removing either call site, removing both, and dropping the drain's clear() are all caught.

Full suite: 2066 passed, with the same 4 failures present on main (unrelated — tmpdir, web API, state reconciliation).

Summary by CodeRabbit

  • Bug Fixes
    • Improved plugin update reporting so updates completed in the background are now detected reliably.
    • Synchronous and asynchronous updates are reported consistently.
    • Prevented duplicate update notifications while ensuring multiple completed updates are captured.
    • Improved handling of updates completed during ongoing checks and under concurrent activity.

run_scheduled_updates_with_changes() snapshotted plugin_last_update,
called run_scheduled_updates(), and diffed the two to answer "whose data
just changed".

But run_scheduled_updates() only enqueues. The work runs on the update
worker and stamps plugin_last_update there, after this method has already
returned, so the two snapshots were always identical and the result was
always an empty list. The only path that ever worked was the synchronous
kill-switch, where update() runs inline.

Vegas is the caller. That empty list is what feeds mark_plugin_updated(),
which drops the cached content for a plugin whose data moved -- so a
segment kept scrolling whatever it was first built from. It is the
failure the coordinator's own comments describe: last night's live game
still drawn as live the next morning. On a live rig: zero update ticks in
twenty minutes, with weather, stocks and news all updating on schedule.

The worker now records each completed update in a ledger and the call
drains it, reporting what has finished since the previous poll rather
than what this call enqueued. That costs one tick of latency -- Vegas
polls every ~4s -- and is correct whichever side of the queue the work
lands on. Failure paths are excluded: they stamp the timestamp too, to
space out retries, but no fresh data exists.

Verified on the rig it was found on: 0 update ticks before, 208 in
twenty-five minutes after, naming real plugins.

The behavioural tests here would pass with both production call sites
deleted, which mutation testing caught -- they drive the ledger directly.
So there is also a structural test asserting the invariant at the source:
wherever a successful update stamps plugin_last_update, it must record
the completion. Writing it immediately caught that _record_update_failure
stamps the same field and must not be included.

Mutation-checked: removing either call site, removing both, and dropping
the drain's clear are all caught.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3389fa91-6dc4-4abe-a5d5-ed9c6657c69d

📥 Commits

Reviewing files that changed from the base of the PR and between 08265c1 and 0339051.

📒 Files selected for processing (2)
  • src/plugin_system/plugin_manager.py
  • test/test_update_change_reporting.py

📝 Walkthrough

Walkthrough

PluginManager now records successful plugin updates in a thread-safe completion set. Scheduled reporting drains this set and returns sorted plugin IDs. Regression tests cover asynchronous, synchronous, concurrent, duplicate, and delayed completions.

Changes

Plugin update reporting

Layer / File(s) Summary
Completion ledger and update recording
src/plugin_system/plugin_manager.py
PluginManager stores completed plugin IDs in a thread-safe set. Successful asynchronous and synchronous updates record completion.
Scheduled reporting and validation
src/plugin_system/plugin_manager.py, test/test_update_change_reporting.py
run_scheduled_updates_with_changes() drains and clears completed IDs. Tests validate ordering, deduplication, concurrency, delayed completions, and all successful timestamping paths.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 03390

This change reports completed plugin updates so refreshed data can be recognized instead of remaining stale; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main fix: reporting completed plugin updates instead of queued updates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/plugin-update-change-reporting

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric Results
Complexity 0
Duplication 0

View in Codacy

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant