From 9074f4fecab3b68ad09f9d21dacb1d5fea84644e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Veyssi=C3=A8re?= Date: Thu, 30 Jul 2026 12:06:50 -0500 Subject: [PATCH] report a switch name that matches no registration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Redis accepts any field, so a mistyped switch name was written, reported as applied, and never consulted — indistinguishable from a working pause until someone noticed the flow had never stopped. App now tells its SwitchBoard which names this process actually checks, and anything else is logged at ERROR. Checked both on set() and on every re-read: set() catches this process's own callers, the re-read catches whatever switches.disable() wrote from outside, which holds a short-lived connection, knows no topology, and so cannot check itself. Logged rather than raised — a process knows only the names it mounted, and in a split deployment a name may belong to a service running elsewhere. Reported once per name so a 2s refresh cannot turn one typo into a flood. Also fixes pytest collection: tests/ has no __init__.py, so pytest put that directory on sys.path instead of the project root and `from tests.helpers import ...` only resolved under `python -m pytest`. Co-Authored-By: Claude --- CHANGELOG.md | 35 +++++++++++++++++++++++ pyproject.toml | 5 ++++ runtime/_version.py | 2 +- runtime/app.py | 34 ++++++++++++++-------- runtime/switches.py | 39 ++++++++++++++++++++++++- tests/test_app.py | 69 +++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 170 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 210dd85..4b28795 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,40 @@ # Changelog +## 0.3.2 + +### Fixed + +- **A switch name that matches no registration is now reported.** Redis accepts + any field, so a mistyped name was written, reported as applied, and then never + consulted — indistinguishable from a working pause until someone noticed the + flow had never stopped. + + `App` now tells its `SwitchBoard` which names this process actually checks + (service names, registration names, producer ids), and anything else is logged: + + ``` + ERROR switch 'catalogue' matches no registration here, so it changes nothing + in this process. Known: catalog, catalog.sync, doctor + ``` + + Checked both when a switch is set and on every re-read, because the two catch + different mistakes: `set` catches this process's own callers, the re-read + catches whatever `switches.disable(url, name)` wrote from outside — that helper + holds a short-lived connection and knows no topology, so it cannot check + itself. + + Logged, never raised: a process knows only the names *it* mounted, and in a + split deployment a name may belong quite legitimately to a service running + elsewhere. Reported once per name, so a 2 s refresh cannot turn one typo into a + flood. A `SwitchBoard` built by hand, with no topology, polices nothing. + +### Development + +- `pytest` failed to collect four test modules unless invoked as + `python -m pytest`: `tests/` has no `__init__.py`, so pytest put that directory + on `sys.path` instead of the project root and `from tests.helpers import ...` + did not resolve. Fixed with `pythonpath = ["."]`. + ## 0.3.1 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index e8c80b4..f95e64f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -67,6 +67,11 @@ runtime = ["py.typed"] [tool.pytest.ini_options] asyncio_mode = "auto" timeout = 30 +# `tests/` has no __init__.py, so pytest puts that directory on sys.path rather +# than the project root — and `from tests.helpers import serving` then fails to +# import under a plain `pytest`. It only worked via `python -m pytest`, which +# prepends the cwd. +pythonpath = ["."] [tool.mypy] python_version = "3.12" diff --git a/runtime/_version.py b/runtime/_version.py index 260c070..f9aa3e1 100644 --- a/runtime/_version.py +++ b/runtime/_version.py @@ -1 +1 @@ -__version__ = "0.3.1" +__version__ = "0.3.2" diff --git a/runtime/app.py b/runtime/app.py index 2ca3c26..8ad5c89 100644 --- a/runtime/app.py +++ b/runtime/app.py @@ -213,6 +213,24 @@ def _slot_count(self) -> int: for inc in self._services ) + def _switch_names(self) -> frozenset[str]: + """Every name a switch can meaningfully address in this process. + + A service name is a master switch over what it registers; each + registration also has its own. Anything outside this set is a switch that + gets written and never consulted. + """ + return frozenset( + name + for inc in self._services + for name in ( + inc.service.name, + *(reg.name for reg in inc.service.consumers), + *(reg.id for reg in inc.service._every), + *(reg.id for reg in inc.service._once), + ) + ) + def _warn_if_paused(self, switches: SwitchBoard) -> None: """Name anything this process is about to start that is already paused. @@ -223,17 +241,7 @@ def _warn_if_paused(self, switches: SwitchBoard) -> None: else entirely. """ paused = sorted( - { - name - for inc in self._services - for name in ( - inc.service.name, - *(reg.name for reg in inc.service.consumers), - *(reg.id for reg in inc.service._every), - *(reg.id for reg in inc.service._once), - ) - if not switches.is_enabled(name) - } + name for name in self._switch_names() if not switches.is_enabled(name) ) if paused: log.warning( @@ -311,7 +319,9 @@ async def _serve(self, broker: Broker | None = None) -> None: ) node = Node(bk) - switches = SwitchBoard(bk, interval_s=self.switch_interval) + switches = SwitchBoard( + bk, interval_s=self.switch_interval, known=self._switch_names() + ) await switches.refresh() # start with the truth, not with "all enabled" self._warn_if_paused(switches) diff --git a/runtime/switches.py b/runtime/switches.py index 34e2197..1848499 100644 --- a/runtime/switches.py +++ b/runtime/switches.py @@ -39,10 +39,19 @@ class SwitchBoard: """An in-process snapshot of the switch registry, refreshed on a timer.""" def __init__( - self, broker: Broker, *, interval_s: float = DEFAULT_INTERVAL_S + self, + broker: Broker, + *, + interval_s: float = DEFAULT_INTERVAL_S, + known: frozenset[str] = frozenset(), ) -> None: self._broker = broker self._interval_s = interval_s + self._known = known + """Every name this process actually checks — service names, registration + names and producer ids. Empty means "do not police names", which is what + a board built by hand in a test gets.""" + self._reported_unknown: set[str] = set() self._paused: frozenset[str] = frozenset() self._generation = 0 """Bumped by every local ``set``. A refresh that started before one @@ -72,10 +81,37 @@ async def refresh(self) -> None: # Redis and is strictly newer than what we are holding, so publishing # this would silently undo it. return + for name in switches: + self._report_if_unknown(name) paused = frozenset(name for name, enabled in switches.items() if not enabled) self._log_changes(paused) self._paused = paused + def _report_if_unknown(self, name: str) -> None: + """Say so when a switch matches nothing this process checks. + + Redis accepts any field, so a mistyped name is written, reported as + applied, and then never consulted — indistinguishable from a working + pause until someone notices the flow never stopped. Checked on the way in + AND on every re-read, because the two catch different mistakes: `set` + catches this process's own callers, the re-read catches whatever + ``switches.disable`` wrote from outside. + + Logged, never raised. A process knows only the names IT mounted, and in a + split deployment the name may belong perfectly legitimately to a service + running somewhere else. Once per name, or a 2 s timer turns one typo into + a flood. + """ + if not self._known or name in self._known or name in self._reported_unknown: + return + self._reported_unknown.add(name) + log.error( + "switch %r matches no registration here, so it changes nothing in " + "this process. Known: %s", + name, + ", ".join(sorted(self._known)), + ) + def _log_changes(self, paused: frozenset[str]) -> None: """Say so when a switch flips. @@ -97,6 +133,7 @@ async def run(self) -> None: async def set(self, name: str, enabled: bool) -> None: """Flip a switch and apply it locally at once, so the caller's own next check reflects what it just did instead of waiting for the timer.""" + self._report_if_unknown(name) await self._broker.switch_set(name, enabled) paused = self._paused - {name} if enabled else self._paused | {name} # Logged here as well as in refresh(): a flip made from inside this diff --git a/tests/test_app.py b/tests/test_app.py index d66c147..9deda25 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -315,6 +315,75 @@ async def test_a_flip_made_elsewhere_is_logged_when_it_is_noticed(broker, caplog assert "resumed catalog" in caplog.text +async def test_setting_a_switch_no_registration_answers_to_is_an_error(broker, caplog): + """Redis accepts any field, so a mistyped name is written, reported as + applied, and never consulted — indistinguishable from a working pause until + someone notices the flow never stopped.""" + board = SwitchBoard(broker, known=frozenset({"catalog", "catalog.sync"})) + + with caplog.at_level("ERROR", logger="runtime"): + await board.set("catalogue", False) # one letter off + + assert "matches no registration here" in caplog.text + assert "catalogue" in caplog.text + assert "catalog.sync" in caplog.text # the message lists what would have worked + + +async def test_a_switch_written_from_outside_is_reported_on_the_next_read( + broker, caplog +): + """The case `set` cannot see: `switches.disable(url, name)` holds a + short-lived connection and knows no topology, so a typo there is only + catchable when a board that does know re-reads the registry.""" + board = SwitchBoard(broker, known=frozenset({"catalog"})) + await broker.switch_set("catlog", False) + + with caplog.at_level("ERROR", logger="runtime"): + await board.refresh() + + assert "matches no registration here" in caplog.text + + +async def test_an_unknown_switch_is_reported_once_not_every_refresh(broker, caplog): + """The board re-reads every couple of seconds; one typo must not become a + flood that buries the rest of the log.""" + board = SwitchBoard(broker, known=frozenset({"catalog"})) + await broker.switch_set("catlog", False) + + with caplog.at_level("ERROR", logger="runtime"): + for _ in range(5): + await board.refresh() + + assert caplog.text.count("matches no registration here") == 1 + + +async def test_a_board_with_no_topology_polices_nothing(broker, caplog): + """A board built by hand — a test, a tool — knows no names. Complaining about + every one of them would make the check worse than useless.""" + board = SwitchBoard(broker) + + with caplog.at_level("ERROR", logger="runtime"): + await board.set("anything.at.all", False) + + assert "matches no registration" not in caplog.text + + +async def test_a_real_registration_name_is_never_reported(broker, caplog): + svc = Service("catalog", max_slots=1) + svc.register_action(noop, name="catalog.sync") + svc.register_every(lambda ctx: asyncio.sleep(0), interval="1s", id="doctor") + app = App(redis="unused://") + app.include(svc) + + board = SwitchBoard(broker, known=app._switch_names()) + + with caplog.at_level("ERROR", logger="runtime"): + for name in ("catalog", "catalog.sync", "doctor"): + await board.set(name, False) + + assert "matches no registration" not in caplog.text + + async def test_a_failed_refresh_keeps_the_previous_snapshot(broker): """Losing Redis must not silently resume everything that was paused."""