Skip to content

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

Open
ChuckBuilds wants to merge 2 commits into
mainfrom
fix/core-cache-hardening
Open

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

Conversation

@ChuckBuilds

Copy link
Copy Markdown
Owner

Summary

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

  1. 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.
  2. 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).
  3. 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

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

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@ChuckBuilds, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 12 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c478dbd0-217d-4b45-9b3c-b56686f132d6

📥 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
✨ Finishing Touches
🧪 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

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