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