Compare commits

..
Author SHA1 Message Date
ChuckandClaude Opus 5.5 686b00d224 fix(fonts): call the font manager's forget methods without a None-able local
Pylint E1102 (Codacy) read getattr(..., None) as possibly not callable.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-04 17:11:09 -04:00
ChuckandClaude Opus 5.5 10ac608b9d fix(fonts): unloading a plugin forgets its manifest fonts
PluginManager.unload_plugin() and the failed-load cleanup only called
FontManager.forget_manager_fonts(), which drops usage data. The plugin's
manifest registrations stayed: plugin_fonts / plugin_font_catalogs, its
plugin_id::family entries in font_catalog, and cached font objects for them
-- so its fonts kept resolving after unload and a family a reinstalled
manifest dropped stayed registered. The deprecated unregister_plugin_fonts
did this cleanup but nothing called it; it was removed in #708.

Add FontManager.forget_plugin_fonts(plugin_id) and call it from both
paths alongside forget_manager_fonts (each guarded on its own, so a font
manager stub with only one still works). FontManager takes no locks, so
like forget_manager_fonts it uses atomic pops over snapshots. A reload
(unload + load) registers the manifest fonts again and they resolve.

Raised by CodeRabbit on #709.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-04 17:00:28 -04:00
8 changed files with 207 additions and 90 deletions
+8 -12
View File
@@ -21,6 +21,14 @@ accepts both, but the store flags the old spelling as deprecated
### Fixed ### Fixed
- Unloading a plugin now forgets the fonts its manifest registered, not only
the fonts it reported using. Its `plugin_id::family` entries kept resolving
and their cached font objects stayed alive until a restart, and a family a
reinstalled plugin's manifest dropped stayed registered. The new
`FontManager.forget_plugin_fonts(plugin_id)` does the cleanup;
`PluginManager.unload_plugin()` and a failed load call it alongside
`forget_manager_fonts()`, and a reload registers the manifest's fonts again.
- The web preview and `/api/v3/display/current` no longer stay black for a - The web preview and `/api/v3/display/current` no longer stay black for a
whole screen that draws its card once and then holds it. The snapshot is whole screen that draws its card once and then holds it. The snapshot is
written from `update_display()` at most once per write interval, so a frame written from `update_display()` at most once per write interval, so a frame
@@ -57,18 +65,6 @@ soccer-scoreboard 2.39.2, alternating runs: **~450 requests per start, peak
spends one doomed 400 per window at every start (eleven at once from a spends one doomed 400 per window at every start (eleven at once from a
soccer board); the range is still retried `RANGE_RETRY_SECONDS` in. soccer board); the range is still retried `RANGE_RETRY_SECONDS` in.
### Fetch stats: bytes on the wire, not just decoded
`GET /api/v3/plugins/fetch-stats` reported only `bytes`, the decoded body
size, and that read as the download volume. ESPN gzips every scoreboard, so
it overstated what crossed the network about 14x: a college football
Saturday's scoreboard is 865 KB decoded and 63 KB on the wire, and ledpi's
"643 MB in 6 hours" of football was ~47 MB of actual traffic. Every counter
set (totals, per plugin, per host) now has `wire_bytes` too, read from
urllib3's count of the raw bytes it took off the socket. A response with no
urllib3 response behind it is counted at its decoded size. `bytes` keeps its
meaning.
### Cheap per-frame and per-fetch savings ### Cheap per-frame and per-fetch savings
- `BaseOddsManager.get_odds()` no longer pretty-prints every odds response - `BaseOddsManager.get_odds()` no longer pretty-prints every odds response
+3 -1
View File
@@ -203,6 +203,7 @@ Current methods:
| `measure_text(text, font)` | `(width, height, baseline)` | | `measure_text(text, font)` | `(width, height, baseline)` |
| `get_font_height(font)` | Line height | | `get_font_height(font)` | Line height |
| `register_plugin_fonts(plugin_id, font_manifest)` | Register a plugin's fonts (core calls it at load) | | `register_plugin_fonts(plugin_id, font_manifest)` | Register a plugin's fonts (core calls it at load) |
| `forget_plugin_fonts(plugin_id)` | Drop a plugin's manifest fonts and their cached objects (core calls it when a plugin unloads) |
| `clear_cache()` | Drop cached fonts and metrics | | `clear_cache()` | Drop cached fonts and metrics |
| `font_catalog` (attribute) | Family name → file path | | `font_catalog` (attribute) | Family name → file path |
@@ -218,5 +219,6 @@ Removed in 3.8.0, after logging a deprecation warning on first call since
| `get_performance_stats()` | — | | `get_performance_stats()` | — |
| `set_override()`, `remove_override()`, `get_overrides()` | a font field in your plugin's config schema | | `set_override()`, `remove_override()`, `get_overrides()` | a font field in your plugin's config schema |
| `get_manager_fonts()`, `get_detected_fonts()` | — | | `get_manager_fonts()`, `get_detected_fonts()` | — |
| `get_plugin_fonts()`, `unregister_plugin_fonts()` | — | | `get_plugin_fonts()` | — |
| `unregister_plugin_fonts()` | `forget_plugin_fonts()` (core calls it on unload) |
| `add_font()`, `remove_font()`, `validate_font()` | the web UI's Fonts tab | | `add_font()`, `remove_font()`, `validate_font()` | the web UI's Fonts tab |
+1 -30
View File
@@ -59,10 +59,7 @@ says how old with ``cache_max_age`` (``fetch_get(..., cache_max_age=ttl)``;
Identical means what the validator store keys on: URL, query, effective Identical means what the validator store keys on: URL, query, effective
headers and, for a session with cookies or auth, the session. headers and, for a session with cookies or auth, the session.
**Counters.** Requests, merged requests, bytes (``bytes`` decoded, as the **Counters.** Requests, merged requests, bytes, 304s, errors, HTTP errors,
caller reads them; ``wire_bytes`` as they crossed the network, which is
what a metered connection pays for -- ESPN gzips, so the two differ ~14x),
304s, errors, HTTP errors,
adapter retries, throttled requests and seconds waited, plus requests adapter retries, throttled requests and seconds waited, plus requests
answered without the network: ``memo_hits`` (the response cache) and answered without the network: ``memo_hits`` (the response cache) and
``cache_hits`` / ``legacy_cache_hits`` (a shared ESPN scoreboard cache entry, ``cache_hits`` / ``legacy_cache_hits`` (a shared ESPN scoreboard cache entry,
@@ -204,7 +201,6 @@ _COUNTER_FIELDS = (
"throttled", # requests that waited for a host budget "throttled", # requests that waited for a host budget
"overruns", # requests that went after max_wait_seconds anyway "overruns", # requests that went after max_wait_seconds anyway
"bytes", # decoded response body bytes received "bytes", # decoded response body bytes received
"wire_bytes", # body bytes as they came off the socket (still compressed)
"wait_seconds", # time spent waiting for host budgets "wait_seconds", # time spent waiting for host budgets
"memo_hits", # answered from the response cache (max-age); nothing sent "memo_hits", # answered from the response cache (max-age); nothing sent
"cache_hits", # scoreboard fetches answered from a shared ESPN cache entry "cache_hits", # scoreboard fetches answered from a shared ESPN cache entry
@@ -620,30 +616,6 @@ def _body_of(response: Any) -> Optional[bytes]:
return content if isinstance(content, bytes) else None return content if isinstance(content, bytes) else None
def _wire_bytes_of(response: Any, body: Optional[bytes]) -> int:
"""How many body bytes came off the socket for ``response``: the
compressed size when the server sent gzip, which ESPN does for every
scoreboard (63 KB on the wire for an 865 KB college football Saturday).
urllib3's ``HTTPResponse.tell()`` counts the raw bytes read before
decoding. A response without one (a test double, an adapter that is not
urllib3) or one whose body was not read is counted at its decoded size,
or as 0, so the counter never claims less than it can prove.
"""
if body is None:
return 0
raw = getattr(response, "raw", None)
tell = getattr(raw, "tell", None)
if callable(tell):
try:
read = tell()
except Exception:
read = None
if isinstance(read, int) and not isinstance(read, bool) and read > 0:
return read
return len(body)
def _retries_of(response: Any) -> int: def _retries_of(response: Any) -> int:
raw = getattr(response, "raw", None) raw = getattr(response, "raw", None)
retries = getattr(raw, "retries", None) retries = getattr(raw, "retries", None)
@@ -1145,7 +1117,6 @@ class FetchService:
http_errors=int(status is not None and status >= 400), http_errors=int(status is not None and status >= 400),
retries=_retries_of(response), retries=_retries_of(response),
bytes=len(body) if body is not None else 0, bytes=len(body) if body is not None else 0,
wire_bytes=_wire_bytes_of(response, body),
throttled=int(waited > 0), overruns=int(overrun), throttled=int(waited > 0), overruns=int(overrun),
wait_seconds=waited) wait_seconds=waited)
except Exception: except Exception:
+33
View File
@@ -222,6 +222,39 @@ class FontManager:
logger.error(f"Error registering fonts for plugin {plugin_id}: {e}", exc_info=True) logger.error(f"Error registering fonts for plugin {plugin_id}: {e}", exc_info=True)
return False return False
def forget_plugin_fonts(self, plugin_id: str) -> bool:
"""Drop the fonts ``plugin_id``'s manifest registered: its manifest
and catalog, its ``plugin_id::family`` entries in font_catalog, and
cached font objects for those families. Called by core when a plugin
is unloaded, so a reload registers from its current manifest and a
removed plugin's fonts stop resolving.
FontManager takes no locks; like forget_manager_fonts this relies on
single dict operations being atomic and iterates snapshots, so a
render thread calling get_font() meanwhile cannot break it. Returns
True if the plugin had registered fonts.
"""
prefix = f"{plugin_id}::"
manifest = self.plugin_fonts.pop(plugin_id, None)
catalog = self.plugin_font_catalogs.pop(plugin_id, None)
# Every namespaced entry, not just the families in the catalog: one
# whose file failed to load never made it into the catalog, and a
# caller may have added one directly.
for family in list(self.font_catalog):
if family.startswith(prefix):
self.font_catalog.pop(family, None)
# get_font() keys the cache f"{family}_{size_px}".
dropped = [key for key in list(self.font_cache) if key.startswith(prefix)]
for key in dropped:
self.font_cache.pop(key, None)
if dropped:
# Font objects someone may hold were dropped; see cache_generation.
self.cache_generation += 1
if manifest is None and catalog is None:
return False
logger.info("Forgot fonts of plugin %s", plugin_id)
return True
def _validate_font_manifest(self, font_manifest: Dict[str, Any]) -> bool: def _validate_font_manifest(self, font_manifest: Dict[str, Any]) -> bool:
"""Validate the structure of a plugin's font manifest.""" """Validate the structure of a plugin's font manifest."""
required_fields = ["fonts"] required_fields = ["fonts"]
+18 -11
View File
@@ -597,11 +597,21 @@ class PluginManager:
self.plugin_loader.unregister_plugin_modules(plugin_id) self.plugin_loader.unregister_plugin_modules(plugin_id)
except Exception as e: # pragma: no cover - defensive except Exception as e: # pragma: no cover - defensive
self.logger.debug("Could not drop modules of %s: %s", plugin_id, e) self.logger.debug("Could not drop modules of %s: %s", plugin_id, e)
try: self._forget_plugin_fonts(plugin_id)
if self.font_manager is not None and hasattr(self.font_manager, 'forget_manager_fonts'):
self.font_manager.forget_manager_fonts(plugin_id) def _forget_plugin_fonts(self, plugin_id: str) -> None:
except Exception as e: """Drop what the FontManager holds for a plugin: the fonts its
self.logger.debug("Could not forget fonts of %s: %s", plugin_id, e) instance reported using (the Fonts tab's "Used by") and the fonts its
manifest registered. Never raises."""
if self.font_manager is None:
return
for name in ('forget_manager_fonts', 'forget_plugin_fonts'):
if not hasattr(self.font_manager, name):
continue
try:
getattr(self.font_manager, name)(plugin_id)
except Exception as e:
self.logger.debug("Could not forget fonts of %s (%s): %s", plugin_id, name, e)
#: Config keys the **core** reads out of a plugin's own config block. The #: Config keys the **core** reads out of a plugin's own config block. The
#: plugin never declares them, so a schema with #: plugin never declares them, so a schema with
@@ -846,12 +856,9 @@ class PluginManager:
# Delegate sub-module and cached-module cleanup to the loader # Delegate sub-module and cached-module cleanup to the loader
self.plugin_loader.unregister_plugin_modules(plugin_id) self.plugin_loader.unregister_plugin_modules(plugin_id)
# Its font registrations go with it (the Fonts tab's "Used by"). # Its font registrations go with it: the fonts it reported using
try: # and the ones its manifest registered.
if self.font_manager is not None and hasattr(self.font_manager, 'forget_manager_fonts'): self._forget_plugin_fonts(plugin_id)
self.font_manager.forget_manager_fonts(plugin_id)
except Exception as e:
self.logger.debug("Could not forget fonts of %s: %s", plugin_id, e)
# Update state # Update state
self.state_manager.set_state(plugin_id, PluginState.UNLOADED) self.state_manager.set_state(plugin_id, PluginState.UNLOADED)
-36
View File
@@ -576,42 +576,6 @@ class TestCounters:
assert snap["hosts"]["site.api.espn.com"]["requests"] == 1 assert snap["hosts"]["site.api.espn.com"]["requests"] == 1
assert snap["totals"]["bytes"] == 3 * len(b'{"ok": 1}') assert snap["totals"]["bytes"] == 3 * len(b'{"ok": 1}')
def test_wire_bytes_are_the_compressed_size(self, service):
# Built the way requests builds a real response: a urllib3
# HTTPResponse carrying a gzip body, decoded when .content is read.
import gzip
import io
from requests.adapters import HTTPAdapter
from urllib3.response import HTTPResponse
decoded = json.dumps({"events": [{"id": str(i), "name": "x" * 200}
for i in range(50)]}).encode()
wire = gzip.compress(decoded)
def handler(url, kwargs):
raw = HTTPResponse(body=io.BytesIO(wire), status=200,
headers={"Content-Encoding": "gzip",
"Content-Type": "application/json"},
preload_content=False, decode_content=True)
request = requests.Request("GET", url).prepare()
response = HTTPAdapter().build_response(request, raw)
response.content # what Session.get does for a non-streamed call
return response
response = service.get(FakeSession(handler), "https://site.api.espn.com/x")
assert response.content == decoded
totals = _counters(service)
assert totals["bytes"] == len(decoded)
assert totals["wire_bytes"] == len(wire) < len(decoded)
def test_wire_bytes_fall_back_to_the_decoded_size(self, service):
# No urllib3 response behind it (a test double, another adapter):
# count what is known rather than nothing.
service.get(FakeSession(), "https://api.test/x")
totals = _counters(service)
assert totals["wire_bytes"] == totals["bytes"] == len(b'{"ok": 1}')
def test_errors_and_http_errors(self, service): def test_errors_and_http_errors(self, service):
def handler(url, kwargs): def handler(url, kwargs):
if url.endswith("/down"): if url.endswith("/down"):
+143
View File
@@ -185,6 +185,149 @@ class TestPluginFonts:
assert fm.font_catalog["my-plugin::bundled"] == str(plugin_dir / "fonts" / "Bundled.ttf") assert fm.font_catalog["my-plugin::bundled"] == str(plugin_dir / "fonts" / "Bundled.ttf")
class TestForgetPluginFonts:
"""forget_plugin_fonts drops what a plugin's manifest registered. Before
it, unloading a plugin left its fonts resolvable and its cached font
objects alive until a restart."""
@staticmethod
def _register(fm, root, plugin_id, family="bundled"):
plugin_dir = root / plugin_id
(plugin_dir / "fonts").mkdir(parents=True, exist_ok=True)
font_file = plugin_dir / "fonts" / f"{family}.ttf"
if not font_file.exists(): # a loaded font may hold it open (Windows)
shutil.copy(resolve_asset_path("assets/fonts/PressStart2P-Regular.ttf"), font_file)
manifest = {"fonts": [{"family": family, "source": f"plugin://fonts/{family}.ttf"}]}
assert fm.register_plugin_fonts(plugin_id, manifest, plugin_dir=plugin_dir)
return plugin_dir
@staticmethod
def _entries_of(fm, plugin_id):
prefix = f"{plugin_id}::"
return {
"plugin_fonts": plugin_id in fm.plugin_fonts,
"plugin_font_catalogs": plugin_id in fm.plugin_font_catalogs,
"font_catalog": [k for k in fm.font_catalog if k.startswith(prefix)],
"font_cache": [k for k in fm.font_cache if k.startswith(prefix)],
}
NONE = {"plugin_fonts": False, "plugin_font_catalogs": False,
"font_catalog": [], "font_cache": []}
def test_unload_leaves_no_plugin_entries(self, fm, tmp_path):
self._register(fm, tmp_path, "alpha")
fm.resolve_font("alpha.title", "bundled", 8, plugin_id="alpha")
fm.get_font("alpha::bundled", 10)
assert self._entries_of(fm, "alpha")["font_cache"] # cached before
gen = fm.cache_generation
assert fm.forget_plugin_fonts("alpha") is True
assert self._entries_of(fm, "alpha") == self.NONE
assert fm.cache_generation == gen + 1
# The family no longer resolves to the plugin's file.
assert fm.font_catalog.get("alpha::bundled") is None
def test_other_plugins_and_core_fonts_are_untouched(self, fm, tmp_path):
self._register(fm, tmp_path, "alpha")
self._register(fm, tmp_path, "beta")
# A plugin whose id is a prefix of another's must not take it along.
self._register(fm, tmp_path, "alpha-two")
for pid in ("alpha", "beta", "alpha-two"):
fm.get_font(f"{pid}::bundled", 8)
core_font = fm.get_font("press_start", 8)
beta_before = self._entries_of(fm, "beta")
alpha_two_before = self._entries_of(fm, "alpha-two")
fm.forget_plugin_fonts("alpha")
assert self._entries_of(fm, "beta") == beta_before
assert self._entries_of(fm, "alpha-two") == alpha_two_before
assert fm.get_font("press_start", 8) is core_font
def test_reload_re_registers_cleanly(self, fm, tmp_path):
plugin_dir = self._register(fm, tmp_path, "alpha")
old = fm.get_font("alpha::bundled", 8)
fm.forget_plugin_fonts("alpha")
self._register(fm, tmp_path, "alpha")
assert fm.font_catalog["alpha::bundled"] == str(plugin_dir / "fonts" / "bundled.ttf")
font = fm.resolve_font("alpha.title", "bundled", 8, plugin_id="alpha")
assert isinstance(font, ImageFont.FreeTypeFont)
assert font is not old # loaded fresh, not the dropped cache entry
def test_a_family_the_new_manifest_drops_stops_resolving(self, fm, tmp_path):
self._register(fm, tmp_path, "alpha", family="old_face")
fm.forget_plugin_fonts("alpha")
self._register(fm, tmp_path, "alpha", family="new_face")
assert "alpha::old_face" not in fm.font_catalog
assert "alpha::new_face" in fm.font_catalog
def test_unknown_plugin_is_a_no_op(self, fm):
catalog = dict(fm.font_catalog)
gen = fm.cache_generation
assert fm.forget_plugin_fonts("never-registered") is False
assert fm.font_catalog == catalog
assert fm.cache_generation == gen
class TestPluginManagerReloadFonts:
"""Through PluginManager: unloading a plugin forgets its manifest fonts,
and reload_plugin (unload + load) registers them again so they resolve."""
PLUGIN_ID = "font-reload-demo"
MODULE = "plugin_font_reload_demo"
def test_unload_forgets_and_reload_resolves(self, tmp_path):
import sys
from src.plugin_system.plugin_manager import PluginManager
plugins_dir = tmp_path / "plugins"
plugin_dir = plugins_dir / self.PLUGIN_ID
(plugin_dir / "fonts").mkdir(parents=True)
shutil.copy(resolve_asset_path("assets/fonts/PressStart2P-Regular.ttf"),
plugin_dir / "fonts" / "Bundled.ttf")
manifest = {"id": self.PLUGIN_ID, "name": "Demo", "class_name": "Demo",
"entry_point": "manager.py",
"fonts": {"fonts": [{"family": "bundled",
"source": "plugin://fonts/Bundled.ttf"}]}}
(plugin_dir / "manifest.json").write_text(json.dumps(manifest), encoding="utf-8")
(plugin_dir / "manager.py").write_text(
"class Demo:\n"
" def __init__(self, plugin_id, config, display_manager, cache_manager, plugin_manager):\n"
" self.enabled = True\n", encoding="utf-8")
pm = PluginManager(plugins_dir=str(plugins_dir))
fm = FontManager({})
pm.font_manager = fm
pm.plugin_manifests[self.PLUGIN_ID] = manifest
key = f"{self.PLUGIN_ID}::bundled"
try:
assert pm.load_plugin(self.PLUGIN_ID) is True
assert key in fm.font_catalog
fm.register_manager_font(self.PLUGIN_ID, "demo.title", "bundled", 8)
old = fm.resolve_font("demo.title", "bundled", 8, plugin_id=self.PLUGIN_ID)
assert pm.unload_plugin(self.PLUGIN_ID) is True
assert self.PLUGIN_ID not in fm.plugin_fonts
assert self.PLUGIN_ID not in fm.plugin_font_catalogs
assert key not in fm.font_catalog
assert not [k for k in fm.font_cache if k.startswith(f"{self.PLUGIN_ID}::")]
assert self.PLUGIN_ID not in fm.manager_fonts
assert pm.reload_plugin(self.PLUGIN_ID) is True
assert fm.font_catalog[key] == str(plugin_dir / "fonts" / "Bundled.ttf")
font = fm.resolve_font("demo.title", "bundled", 8, plugin_id=self.PLUGIN_ID)
assert isinstance(font, ImageFont.FreeTypeFont)
assert font is not old
finally:
sys.modules.pop(self.MODULE, None)
class TestDownloadFont: class TestDownloadFont:
"""_download_font: plugin fonts declared by URL, cached in temp_font_dir.""" """_download_font: plugin fonts declared by URL, cached in temp_font_dir."""
@@ -69,6 +69,7 @@ def test_fixed_plugin_loads_new_code_after_failed_load(plugin_env, first_source)
assert MODULE_NAME not in sys.modules assert MODULE_NAME not in sys.modules
assert PLUGIN_ID not in pm.plugin_loader._loaded_modules assert PLUGIN_ID not in pm.plugin_loader._loaded_modules
pm.font_manager.forget_manager_fonts.assert_called_with(PLUGIN_ID) pm.font_manager.forget_manager_fonts.assert_called_with(PLUGIN_ID)
pm.font_manager.forget_plugin_fonts.assert_called_with(PLUGIN_ID)
(plugin_dir / "manager.py").write_text(_FIXED, encoding="utf-8") (plugin_dir / "manager.py").write_text(_FIXED, encoding="utf-8")
assert pm.load_plugin(PLUGIN_ID) is True assert pm.load_plugin(PLUGIN_ID) is True