From 6b8f79dd9ab6d6977a19ce3dde68bc33e09c5f5e Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Sun, 12 Jul 2026 01:25:08 +0200 Subject: [PATCH] fix(server): a raising tuning provider no longer takes down get_merged() for everyone (#899) (#904) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One word. server.py's TuningProviderRegistry.get_merged(): except Exception: - logger.exception("tuning provider %r raised during get_merged()", provider_id) + log.exception("tuning provider %r raised during get_merged()", provider_id) There is no `logger` in server.py — the module logger is `log`. So the handler written to swallow-and-report a bad provider instead raised NameError from inside the except, and that NameError propagated out of get_merged(). The effect was the exact OPPOSITE of what the handler is for: one misbehaving plugin took the whole merged-tunings call down for every other provider, AND the traceback named the wrong problem ("name 'logger' is not defined" rather than the provider that actually blew up). Doubly silent: nothing was ever logged either, because the logging call was the thing that crashed. Found by pyflakes while carving server.py (R3b). It survived because NOTHING exercised the failure path — no test ever had a provider raise. That is the whole reason this class of bug is invisible: it lives only on error paths, so the suite is green and the feature is broken exactly when it matters. tests/test_tuning_provider_isolation.py is that path: * a raising provider must not lose the HEALTHY providers' tunings, nor the defaults * and the failure must actually be LOGGED — swallowing is only acceptable if it reports Bite-tested: restoring `logger` fails both. (The log assertion attaches caplog's handler to the feedBack logger directly. It sets propagate=False, so pytest's root capture sees nothing from it — test_plugins.py has a capture_logger() for this, but it is not importable here: pyproject pins pythonpath to [".", "lib"], so `tests` is not a package. Three lines beat churning 21 call sites in an unrelated file to convert that helper into a fixture.) pytest 2410, pyflakes 0 undefined names in server.py, Codex 0. Closes #899 Co-authored-by: Claude Opus 4.8 (1M context) --- server.py | 7 ++- tests/test_tuning_provider_isolation.py | 72 +++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 tests/test_tuning_provider_isolation.py diff --git a/server.py b/server.py index 155007e..7e2800f 100644 --- a/server.py +++ b/server.py @@ -427,7 +427,12 @@ class TuningProviderRegistry: for name, freqs in names.items(): result[instrument][name] = [round(f * scale, 4) for f in freqs] except Exception: - logger.exception("tuning provider %r raised during get_merged()", provider_id) + # `log`, not `logger` — there is no `logger` in this module. This handler + # exists so ONE bad provider cannot break tunings for everyone; with the + # wrong name it raised NameError from inside the except and did precisely + # what it was written to prevent. See #899 and + # tests/test_tuning_provider_isolation.py. + log.exception("tuning provider %r raised during get_merged()", provider_id) return result diff --git a/tests/test_tuning_provider_isolation.py b/tests/test_tuning_provider_isolation.py new file mode 100644 index 0000000..ff8149f --- /dev/null +++ b/tests/test_tuning_provider_isolation.py @@ -0,0 +1,72 @@ +"""A raising tuning provider must not take down get_merged() for everyone. (#899) + +`TuningProviderRegistry.get_merged()` wraps each provider in a try/except precisely so one +misbehaving plugin cannot break tunings for the rest. The handler called `logger.exception` +— and there is no `logger` in server.py; the module logger is `log`. So the handler MEANT +to swallow-and-report instead raised NameError, which propagated out of get_merged(). + +The net effect was the exact opposite of the handler's purpose: one bad provider took the +whole merged-tunings call down, and the traceback named the wrong problem. + +Nothing exercised the failure path, which is why it survived. This is that path. +""" + +import importlib +import logging +import sys + +import pytest + + +@pytest.fixture() +def registry(monkeypatch, tmp_path): + monkeypatch.setenv("CONFIG_DIR", str(tmp_path)) + sys.modules.pop("server", None) + mod = importlib.import_module("server") + yield mod.TuningProviderRegistry() + + +def test_a_raising_provider_does_not_break_the_others(registry, caplog): + """The whole point of the try/except. Before the fix this raised NameError.""" + def boom(): + raise RuntimeError("provider exploded") + + def good(): + return {"guitar": {"My Tuning": [82.41, 110.0, 146.83, 196.0, 246.94, 329.63]}} + + registry.register("bad-plugin", boom) + registry.register("good-plugin", good) + + merged = registry.get_merged() # must NOT raise + + assert "My Tuning" in merged["guitar"], ( + "the healthy provider's tuning is missing — one raising provider took down the " + "merged result for everyone" + ) + # and the default tunings survive + assert merged["guitar"], "default tunings were lost" + + +def test_the_failure_is_actually_logged(registry, caplog): + """Swallowing is only acceptable if it is reported. A NameError in the handler meant + nothing was ever logged — the failure was both fatal AND silent about its real cause.""" + def boom(): + raise RuntimeError("provider exploded") + + registry.register("bad-plugin", boom) + + # The feedBack logger sets propagate=False, so pytest's root-logger capture sees + # NOTHING from it. Attach caplog's handler directly. (test_plugins.py has a + # capture_logger() context manager for this, but it is not importable from here: + # pyproject pins pythonpath to [".", "lib"], so `tests` is not a package.) + lg = logging.getLogger("feedBack") + lg.addHandler(caplog.handler) + lg.setLevel(logging.ERROR) + try: + registry.get_merged() + finally: + lg.removeHandler(caplog.handler) + + assert any("bad-plugin" in r.getMessage() for r in caplog.records), ( + "the raising provider was never named in the logs" + )