fix(core): harden text-measurement caches; surface snapshot failures by ChuckBuilds · Pull Request #400 · ChuckBuilds/LEDMatrix

ChuckBuilds · 2026-07-12T14:30:34Z

Summary

Three latent issues found in a codebase deep-dive, all live on every 24/7 install:

Unbounded text-keyed caches: font_manager.metrics_cache and display_manager._text_width_cache grow forever when plugins measure ever-changing strings (a clock, a live score, a ticker) — a slow leak on a Pi that never restarts. Now LRU-bounded at 1024 entries.
id(font) keying without holding a reference — a garbage-collected font's id can be recycled by a different font object, and the cache then silently returns the wrong width/metrics for it (classic plugins create fonts per render, so collection + reuse is reachable). Entries now pin the font. metrics_cache also keyed on the text itself rather than hash(text) (collision path).
Silent snapshot failure: _write_snapshot_if_due logged failures at DEBUG. The snapshot's mtime is the web UI's display mirror and its hardware-liveness proxy — a quiet failure freezes the mirror and makes health checks lie. We hit this in the field: a stale root-owned /tmp/led_matrix_preview.png froze the mirror for a day with nothing in the logs. Failures now WARN (rate-limited to once per 5 minutes).

Verification

New behavior verified directly: bounded growth at 1024, cached values stable, font reference held.
Existing font tests pass (17). Note: test_plugin_system.py::TestPluginHealth::test_circuit_breaker fails on pristine main too (pre-existing, unrelated).

🤖 Generated with Claude Code

https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam

Summary by CodeRabbit

Performance Improvements
- Improved text measurement responsiveness by limiting cached results and prioritizing recently used content.
- Reduced unnecessary recalculations when fonts or text are reused.
Bug Fixes
- Improved handling of web preview snapshot write failures.
- Snapshot issues now generate clearer, rate-limited warnings while avoiding excessive repeated alerts.

Deep-dive findings, all three latent on every 24/7 install: - font_manager.metrics_cache and display_manager._text_width_cache were unbounded dicts keyed by (text, id(font)). Two problems: keys embed the measured TEXT, so ever-changing strings (a clock, a live score, a ticker) grow them without limit; and id()-keying without holding a reference means a garbage-collected font's id can be recycled by a DIFFERENT font, silently returning wrong widths/metrics (classic plugins create fonts per render, so this is reachable). Both caches are now LRU-bounded (1024) and pin the font in the entry so its id stays valid. metrics_cache also keyed on the text itself instead of hash(text), removing a collision path. - _write_snapshot_if_due logged failures at DEBUG — invisible at the default level. The snapshot's mtime is the web UI's display mirror AND its hardware-liveness proxy, so a quiet failure freezes the mirror and makes health checks lie (seen in the field: a stale root-owned /tmp file froze it for a day). Failures now WARN, rate- limited to once per 5 minutes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam

coderabbitai · 2026-07-12T14:30:40Z

📝 Walkthrough

Walkthrough

DisplayManager and FontManager now use bounded LRU text-measurement caches that retain font references. Snapshot write failures use rate-limited warnings, and the cache-manager test mock accepts the expanded method signature.

Changes

Display caching and snapshot logging

Layer / File(s)	Summary
Bounded measurement caches `src/display_manager.py`, `src/font_manager.py`, `test/conftest.py`	Text-width and font-metrics caches now use `OrderedDict` LRU eviction, tuple keys, retained font references, and fixed capacity limits. The cache-manager mock accepts `memory_ttl`.
Rate-limited snapshot failures `src/display_manager.py`	Snapshot write failures produce warnings at most once per five minutes, with debug logs for suppressed retries.

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

Possibly related PRs

ChuckBuilds/LEDMatrix#358: Also updates DisplayManager.get_text_width caching around text and font identity.

🚥 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 summarizes the main changes: cache hardening and improved snapshot failure visibility.
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/core-cache-hardening

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.}

test_circuit_breaker has been failing on main: plugin_health passes memory_ttl= to cache_manager.get(), and the conftest mock's signature was never updated — the same component/double drift class as the monitored_update bug (#392). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam

codacy-production · 2026-07-12T14:34:28Z

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.}

ChuckBuilds · 2026-07-13T12:51:54Z

@coderabbitai review

coderabbitai · 2026-07-13T12:52:00Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

🧹 Nitpick comments (1)

src/display_manager.py (1)
709-740: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

LRU eviction loop could use if instead of while.

Only one entry is inserted per call, so the while loop executes at most once. An if would make the single-eviction intent clearer. This pattern is duplicated in font_manager.py measure_text.
♻️ Optional: use `if` for single-eviction clarity
-        while len(self._text_width_cache) > self._TEXT_WIDTH_CACHE_MAX:
+        if len(self._text_width_cache) > self._TEXT_WIDTH_CACHE_MAX:
             self._text_width_cache.popitem(last=False)
🤖 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/display_manager.py` around lines 709 - 740, Replace the `while` eviction
loop in `get_text_width` with a single conditional eviction, since each call
inserts at most one cache entry. Apply the same `if`-based eviction change to
`font_manager.py`’s `measure_text` cache logic.

🤖 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 `@src/display_manager.py`:
- Around line 709-740: Replace the `while` eviction loop in `get_text_width`
with a single conditional eviction, since each call inserts at most one cache
entry. Apply the same `if`-based eviction change to `font_manager.py`’s
`measure_text` cache logic.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 561191d4-f9d1-40e1-81b9-b1249daf6895

📥 Commits

Reviewing files that changed from the base of the PR and between 05e7c43 and ced173e.

📒 Files selected for processing (3)

src/display_manager.py
src/font_manager.py
test/conftest.py

…log fixes CodeRabbit review on #406, verified against current code: - update_display() can genuinely be called from background threads (some sports base classes call it directly from inside update() for an immediate "live" refresh), not just the render loop — confirmed via the existing follower-mode gating wrapper in display_controller.py, which exists specifically because "background plugin threads" can reach it. Without a lock, two callers could both pass the digest check before either writes _last_pushed_digest back, causing a redundant push, or interleave the offscreen/current canvas swap. Added self._update_lock (RLock, in case of re-entrant callers) around the full method body so every call site is automatically covered — no caller changes needed. (No prior lock existed to reuse on DisplayManager; this adds one.) - Narrowed the brightness-read exception handler to AttributeError, matching the established pattern in get_brightness()/set_brightness() — a getattr() with a default already swallows AttributeError, so the only case this guards is the property getter itself raising, and the established pattern treats that as an expected, specific failure mode rather than something to blanket-catch. - FPS-check debug log now includes the plugin_id already in scope (previously only active_mode) and a "[DisplayController]" prefix for grep-ability, matching the sibling log two lines below it. - test_display_dirty_tracking.py: dm fixture and test_config_flag_wires_through now reset the DisplayManager singleton on teardown, matching the pattern test_display_manager.py already uses elsewhere in the same file family. - test_snapshot_still_written_on_skip previously only exercised the non-skip (push) path despite its name; now performs a second update that meets the skip conditions (identical frame) and asserts the snapshot is still written even though the panel push itself is skipped. All 7 dirty-tracking tests pass, plus the full display_manager/ display_controller/vegas suite (140 passed). Full repo suite has only the 5 known pre-existing failures (double-sided config x2, state_reconciliation x2, and test_circuit_breaker's conftest.py mock signature drift — the latter fixed in #400, which this branch's base predates).

…ion (#406) * perf(display): dirty tracking in update_display + plugin FPS declaration update_display now skips SetImage+SwapOnVSync when the frame is byte-identical to the last pushed one (adler32 digest) AND brightness is unchanged — brightness is part of the digest, and set_brightness additionally resets it, so a dim-schedule change can never be skipped. clear() resets the digest (it writes to the matrix directly). Skipping a swap is hardware-safe: the panel refreshes the current frame from the driver's own thread; swaps only change content. Kill switch: display.dirty_tracking: false restores always-push. display_controller's high-FPS decision gains a precedence step: a plugin exposing needs_high_fps is honored first (so static-image can declare False for still PNGs and stop burning a 125fps loop on them); static-image without the attribute keeps its historical forced high-FPS (GIF back-compat); scrolling logic is otherwise unchanged. Verified with 7 tests against the real DisplayManager on RGBMatrixEmulator (identical-frame skip, pixel-change push, clear and brightness invalidation, snapshot-through-skip, kill switch) plus the 202-test display/controller/vegas suites, and a clean devpi deploy. Audit: every SetImage/SwapOnVSync/Clear/brightness call site is inside display_manager — no external writer can bypass the digest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam * fix(display): serialize update_display, narrow brightness exception, log fixes CodeRabbit review on #406, verified against current code: - update_display() can genuinely be called from background threads (some sports base classes call it directly from inside update() for an immediate "live" refresh), not just the render loop — confirmed via the existing follower-mode gating wrapper in display_controller.py, which exists specifically because "background plugin threads" can reach it. Without a lock, two callers could both pass the digest check before either writes _last_pushed_digest back, causing a redundant push, or interleave the offscreen/current canvas swap. Added self._update_lock (RLock, in case of re-entrant callers) around the full method body so every call site is automatically covered — no caller changes needed. (No prior lock existed to reuse on DisplayManager; this adds one.) - Narrowed the brightness-read exception handler to AttributeError, matching the established pattern in get_brightness()/set_brightness() — a getattr() with a default already swallows AttributeError, so the only case this guards is the property getter itself raising, and the established pattern treats that as an expected, specific failure mode rather than something to blanket-catch. - FPS-check debug log now includes the plugin_id already in scope (previously only active_mode) and a "[DisplayController]" prefix for grep-ability, matching the sibling log two lines below it. - test_display_dirty_tracking.py: dm fixture and test_config_flag_wires_through now reset the DisplayManager singleton on teardown, matching the pattern test_display_manager.py already uses elsewhere in the same file family. - test_snapshot_still_written_on_skip previously only exercised the non-skip (push) path despite its name; now performs a second update that meets the skip conditions (identical frame) and asserts the snapshot is still written even though the panel push itself is skipped. All 7 dirty-tracking tests pass, plus the full display_manager/ display_controller/vegas suite (140 passed). Full repo suite has only the 5 known pre-existing failures (double-sided config x2, state_reconciliation x2, and test_circuit_breaker's conftest.py mock signature drift — the latter fixed in #400, which this branch's base predates). --------- Co-authored-by: Chuck <chuck@example.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

* chore: remove dead modules and unused dependencies (~1,180 LOC) Deletions, each re-verified with a fresh repo-wide grep (core, web, scripts, docs, plugin monorepo) immediately before removal: Modules with zero live importers: - src/background_cache_mixin.py + src/generic_cache_mixin.py (134+150 LOC — referenced only by each other) - src/font_test_manager.py (134 LOC) - src/image_utils.py (22 LOC, self-documented deprecated) - src/layout_manager.py (408 LOC — only its own test imported it) + test/test_layout_manager.py - src/common/basketball_plugin_example.py (328 LOC sample) requirements.txt entries with zero importers in core (pre-plugin-era manager deps): icalevents, geopy, timezonefinder, unidecode. Plus the google-auth trio (google-auth-oauthlib, google-auth-httplib2, google-api-python-client): their only importer is the calendar PLUGIN, which declares all three in its own requirements.txt (verified in the monorepo and on an installed copy) — the plugin dependency installer owns them. Existing venvs are unaffected (removal doesn't uninstall); fresh installs get them when calendar is installed. Two stale references cleaned (a comment in test_pillow_compat.py, a directory listing in HOW_TO_RUN_TESTS.md). Full suite green except the two documented pre-existing failures (circuit_breaker mock drift, fixed in #400; clock-simple 64x32 overflow, pre-dates this series); all core entry modules verified importing cleanly under the emulator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam * fix: remove content accidentally bundled into the dead-code-removal commit The previous commit's git add/commit swept in a lot of unrelated, unreviewed work alongside the intended dead-code deletions: a new Plugin Composer web UI, new security-audit/CI tooling, a new march-madness plugin, and 23 local-development-only symlinks under plugin-repos/ (per scripts/setup_plugin_repos.py's own docstring, these are meant to be generated locally, never committed -- .gitignore has no entry for them, which is how they slipped in). Removed here, split into their own PRs instead (except plugin-repos/* symlinks and march-madness/ncaa_logos, which are dropped rather than carried forward -- see PR discussion): - web_interface/blueprints/composer.py + composer-app.js + composer-canvas.js + composer.html + manager.py.j2 - scripts/prove_security.py, audit_plugins.py, generate_report.py - .github/workflows/security-audit.yml, .github/workflows/tests.yml, bandit.yaml - All plugin-repos/* symlinks (local dev artifacts, not meant to be committed at all) - plugin-repos/march-madness/* and the 4 new assets/sports/ncaa_logos/* PNGs it needed (left out of every split PR pending a decision on whether march-madness belongs in the core repo or the plugin monorepo) This PR now contains only what its title describes: the dead-code removal from the previous commit, untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ --------- Co-authored-by: Chuck <chuck@example.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

ChuckBuilds mentioned this pull request Jul 12, 2026

perf(plugins): run scheduled updates off the render thread #407

Merged

coderabbitai Bot reviewed Jul 13, 2026

View reviewed changes

ChuckBuilds merged commit 9e3b5f3 into main Jul 13, 2026
9 checks passed

Navigation Menu

Uh oh!

fix(core): harden text-measurement caches; surface snapshot failures - #400

fix(core): harden text-measurement caches; surface snapshot failures#400
ChuckBuilds merged 2 commits into
mainfrom
fix/core-cache-hardening

ChuckBuilds commented Jul 12, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 12, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

codacy-production Bot commented Jul 12, 2026

Uh oh!

ChuckBuilds commented Jul 13, 2026

Uh oh!

coderabbitai Bot commented Jul 13, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

ChuckBuilds commented Jul 12, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

codacy-production Bot commented Jul 12, 2026

Up to standards ✅

Uh oh!

ChuckBuilds commented Jul 13, 2026

Uh oh!

coderabbitai Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ChuckBuilds commented Jul 12, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 12, 2026 •

edited

Loading

coderabbitai Bot commented Jul 13, 2026 •

edited

Loading