From bb0f773b7d355d51f5e343fc3c76a90aaa15ff74 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Mon, 7 Sep 2026 16:06:11 +0200 Subject: [PATCH] Make sync_quietly's "never fail" contract true by construction sync_quietly exists so a background timer stays green when the pool cannot be synced. It caught only SyncUnavailableError, which meant the contract held only because of an upstream class hierarchy: today FirebaseAuthError happens to subclass RemoteSyncError, and requests' JSONDecodeError happens to subclass RequestException, so an expired refresh token or a malformed payload does get wrapped and caught. That is a coincidence, not a promise to this package -- and it is exactly the kind that goes quiet the day upstream reorganises its exceptions. The symptom would be a unit going red every 15 minutes, which is the outcome this function's own docstring says the design exists to prevent. It now also catches anything unexpected and logs it with a traceback, so a real bug is loud in the journal while the timer stays green. The narrow handler stays first and stays at debug level: offline is normal on a laptop and must not fill the journal. Two tests, neither of which the old suite could have failed on -- the existing one monkeypatched get_client to raise the one exception already known to be handled. One raises FirebaseAuthError *mid-sync* (a token expiring during the tick rather than at client construction, which is the realistic case per seed_session.py's own docstring), the other raises a RuntimeError that no hierarchy covers. 111 tests, 100% branch coverage. Co-Authored-By: Claude Opus 5 --- freedays/freedays/_sync.py | 19 +++++++++++-- freedays/freedays/tests/test_sync.py | 40 ++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/freedays/freedays/_sync.py b/freedays/freedays/_sync.py index 72d5b09..35a4f82 100644 --- a/freedays/freedays/_sync.py +++ b/freedays/freedays/_sync.py @@ -115,14 +115,29 @@ def sync( def sync_quietly(*, paths: Paths | None = None) -> bool: - """Sync, swallowing unavailability. Returns whether it actually ran. + """Sync, swallowing every failure. Returns whether it actually ran. For callers on a timer that want a fresher pool but must not fail, and - must not say anything, if the network is down. + must not say anything, when the sync cannot happen. + + The broad catch is deliberate and is the whole point of this function. + Today ``FirebaseAuthError`` happens to subclass ``RemoteSyncError`` and + ``requests``' ``JSONDecodeError`` happens to subclass + ``RequestException``, so the narrow handler above would in fact catch an + expired refresh token or a malformed payload. But that is an upstream + class hierarchy, not a promise to this package -- and a background sync + whose contract is "never fail" must not have that contract hold only by + coincidence. Anything unexpected is logged with its traceback, so a real + bug is loud in the journal while the timer stays green. """ try: sync(paths=paths) except SyncUnavailableError as exc: + # The expected case -- offline, or not configured yet. Debug level: + # this is normal on a laptop and must not fill the journal. _logger.debug("free-day sync skipped: %s", exc) return False + except Exception: + _logger.exception("free-day sync failed unexpectedly") + return False return True diff --git a/freedays/freedays/tests/test_sync.py b/freedays/freedays/tests/test_sync.py index e1cd60b..604a186 100644 --- a/freedays/freedays/tests/test_sync.py +++ b/freedays/freedays/tests/test_sync.py @@ -159,3 +159,43 @@ def test_the_pool_can_be_pointed_elsewhere_wholesale(tmp_path: Path) -> None: mark(LATER, now=TODAY, paths=paths) sync(client=FakeRemote(), paths=paths) assert is_free_day(LATER, log_path=paths.log, now=TODAY) + + +def test_an_expired_credential_mid_sync_does_not_fail_the_timer( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A refresh token expiring during the tick, not at client construction.""" + + class ExpiringRemote(FakeRemote): + def list_directory(self, path: str) -> list[str]: + msg = f"refresh token rejected while listing {path}" + raise FirebaseAuthError(msg) + + monkeypatch.setattr( + "freedays._sync.firebase_client_for", lambda _app: ExpiringRemote() + ) + assert sync_quietly() is False + + +def test_an_unexpected_error_is_logged_with_a_traceback_and_swallowed( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """The contract is "never fail", and it must not hold only by coincidence. + + FirebaseAuthError happens to subclass RemoteSyncError today. Something + that does not -- a bug, a changed upstream hierarchy -- must still leave + the timer green rather than turning it red every 15 minutes. + """ + + class BrokenRemote(FakeRemote): + def list_directory(self, path: str) -> list[str]: + msg = f"something nobody predicted, at {path}" + raise RuntimeError(msg) + + monkeypatch.setattr( + "freedays._sync.firebase_client_for", lambda _app: BrokenRemote() + ) + with caplog.at_level("ERROR"): + assert sync_quietly() is False + assert "failed unexpectedly" in caplog.text + assert "RuntimeError" in caplog.text