From c9305753b75964747cb71f429147420220573dec Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Sat, 7 Nov 2026 23:11:48 +0100 Subject: [PATCH 1/2] fix: retry a refused release check within the hour, so a rate-limited startup no longer hides updates for a day --- app/main.py | 14 ++- app/update_check.py | 37 ++++-- tests/test_main_routes.py | 5 + .../test_update_check_retry_after_failure.py | 112 ++++++++++++++++++ 4 files changed, 155 insertions(+), 13 deletions(-) create mode 100644 tests/test_update_check_retry_after_failure.py diff --git a/app/main.py b/app/main.py index eb070639..008fe2ef 100644 --- a/app/main.py +++ b/app/main.py @@ -2072,13 +2072,15 @@ def _on_job_event(event): coalesce=True, misfire_grace_time=300, ) - # Once a day. A release is not urgent, and a self-hosted app polling a third - # party more often than that is rude. Every failure mode inside refresh() - # lands on UNKNOWN, so a firewalled install produces no noise at all. + # Hourly tick, daily fetch: refresh() asks GitHub once a day after a check + # that worked and an hour after one that failed, so a rate-limited startup + # check no longer leaves the release unknown for a whole day. Every failure + # mode inside refresh() lands on UNKNOWN, so a firewalled install produces + # no noise at all. scheduler.add_job( update_check.refresh, "interval", - hours=24, + hours=1, id="update_check", max_instances=1, coalesce=True, @@ -2087,13 +2089,13 @@ def _on_job_event(event): # convention a test enforces uniformly. misfire_grace_time=300, ) - # The Android client's own release track, on the same daily cadence and with + # The Android client's own release track, on the same cadence and with # the same failure contract. Separate job rather than folded into the one # above so a GitHub hiccup fetching one cannot suppress the other. scheduler.add_job( update_check.refresh_android, "interval", - hours=24, + hours=1, id="update_check_android", max_instances=1, coalesce=True, diff --git a/app/update_check.py b/app/update_check.py index 13bd48af..883bedb8 100644 --- a/app/update_check.py +++ b/app/update_check.py @@ -55,18 +55,37 @@ #: party more often than that is rude. CHECK_INTERVAL_SECONDS = 24 * 60 * 60 +#: After a FAILED check, the next try comes an hour later instead of a day. A +#: home connection shares GitHub's unauthenticated limit (60 requests an hour +#: per public IP) with everything else in the house, so the one check made at +#: startup is often refused with a 403; waiting a full day after that left the +#: release unknown, and every phone unjudged, for 24 hours after each restart. +#: The scheduler calls refresh() hourly; refresh() decides whether a fetch is +#: due, so a working install still asks GitHub once a day. +RETRY_AFTER_FAILURE_SECONDS = 60 * 60 + _HTTP_TIMEOUT = 10.0 #: Cached result. `tag` is None for UNKNOWN -- never for "up to date", which is a #: different statement and is carried by `known`. _latest_tag: str | None = None _checked_at: float = 0.0 +_last_failed: bool = False #: Same shape, for the Android client. Kept separate rather than folded into a #: dict so the existing server path cannot be perturbed by a failure fetching #: the Android one. _android_tag: str | None = None _android_checked_at: float = 0.0 +_android_last_failed: bool = False + + +def _due(checked_at: float, last_failed: bool) -> bool: + """Whether the cooldown since the last attempt has run out.""" + if not checked_at: + return True + wait = RETRY_AFTER_FAILURE_SECONDS if last_failed else CHECK_INTERVAL_SECONDS + return (time.time() - checked_at) >= wait def enabled() -> bool: @@ -99,11 +118,11 @@ async def refresh(*, force: bool = False) -> str | None: that is not JSON, a tag that is not a version -- lands on the same answer: UNKNOWN. That is what makes the banner safe on an air-gapped install. """ - global _latest_tag, _checked_at + global _latest_tag, _checked_at, _last_failed if not enabled(): return None - if not force and _checked_at and (time.time() - _checked_at) < CHECK_INTERVAL_SECONDS: + if not force and not _due(_checked_at, _last_failed): return _latest_tag try: @@ -117,10 +136,12 @@ async def refresh(*, force: bool = False) -> str | None: # log full of harmless warnings is a log nobody reads. logger.debug("Update check could not reach %s: %s", LATEST_URL, exc) _checked_at = time.time() # do not hammer a host that is refusing us + _last_failed = True return _latest_tag _latest_tag = tag _checked_at = time.time() + _last_failed = False if tag: logger.debug("Newest published release is %s", tag) return tag @@ -134,11 +155,11 @@ async def refresh_android(*, force: bool = False) -> str | None: value feeds a WARNING on the fleet page -- an unreachable GitHub must make the warning disappear, never make every phone look out of date. """ - global _android_tag, _android_checked_at + global _android_tag, _android_checked_at, _android_last_failed if not enabled(): return None - if not force and _android_checked_at and (time.time() - _android_checked_at) < CHECK_INTERVAL_SECONDS: + if not force and not _due(_android_checked_at, _android_last_failed): return _android_tag try: @@ -149,10 +170,12 @@ async def refresh_android(*, force: bool = False) -> str | None: except Exception as exc: # noqa: BLE001 - every failure means the same thing logger.debug("Android update check could not reach %s: %s", ANDROID_LATEST_URL, exc) _android_checked_at = time.time() + _android_last_failed = True return _android_tag _android_tag = tag _android_checked_at = time.time() + _android_last_failed = False return _android_tag @@ -202,6 +225,6 @@ def _tuple(series_text: str) -> tuple[int, ...]: def _reset_for_tests() -> None: """Clear the module cache. Tests only.""" - global _latest_tag, _checked_at, _android_tag, _android_checked_at - _latest_tag, _checked_at = None, 0.0 - _android_tag, _android_checked_at = None, 0.0 + global _latest_tag, _checked_at, _last_failed, _android_tag, _android_checked_at, _android_last_failed + _latest_tag, _checked_at, _last_failed = None, 0.0, False + _android_tag, _android_checked_at, _android_last_failed = None, 0.0, False diff --git a/tests/test_main_routes.py b/tests/test_main_routes.py index e48cce96..6569fef4 100644 --- a/tests/test_main_routes.py +++ b/tests/test_main_routes.py @@ -2269,6 +2269,11 @@ async def _run(): assert job.max_instances == 1, f"{job.id} max_instances" assert job.coalesce is True, f"{job.id} coalesce" assert job.misfire_grace_time == 300, f"{job.id} misfire_grace_time" + # The release checks tick hourly so a refused check is + # retried within the hour; refresh() itself keeps a + # working install to one GitHub call a day. + for job_id in ("update_check", "update_check_android"): + assert jobs[job_id].trigger.interval.total_seconds() == 3600, job_id # The error/missed listener is attached. assert len(sched._listeners) >= 1 diff --git a/tests/test_update_check_retry_after_failure.py b/tests/test_update_check_retry_after_failure.py new file mode 100644 index 00000000..78903329 --- /dev/null +++ b/tests/test_update_check_retry_after_failure.py @@ -0,0 +1,112 @@ +"""A refused release check is retried within the hour, not the next day. + +A home connection shares GitHub's unauthenticated limit (60 requests an hour per +public IP) with everything else in the house. The one check CashPilot makes at +startup was refused with a 403 on a live dashboard, and the old cooldown treated +that failure like a success: nothing asked again for 24 hours, so the update +card said "unknown" and no phone was judged against the Android release for a +whole day after every restart. + +The contract now: a check that worked is not repeated for a day; one that failed +is repeated after RETRY_AFTER_FAILURE_SECONDS, and not before. +""" + +from __future__ import annotations + +import asyncio +from unittest.mock import AsyncMock, patch + +import httpx +import pytest + +from app import update_check + + +@pytest.fixture(autouse=True) +def _clean(): + update_check._reset_for_tests() + yield + update_check._reset_for_tests() + + +def _client(*outcomes): + """An AsyncClient stand-in whose get() yields each outcome in turn.""" + client = AsyncMock() + client.get.side_effect = list(outcomes) + client.__aenter__.return_value = client + return client + + +def _ok(url, tag): + return httpx.Response(200, json={"tag_name": tag}, request=httpx.Request("GET", url)) + + +def _rate_limited(url): + response = httpx.Response(403, json={"message": "API rate limit exceeded"}, request=httpx.Request("GET", url)) + return response + + +class _Clock: + def __init__(self, start: float = 1_000_000.0): + self.now = start + + def __call__(self) -> float: + return self.now + + +@pytest.mark.parametrize( + ("refresh_name", "url", "tag"), + [ + ("refresh", update_check.LATEST_URL, "v2.0.1"), + ("refresh_android", update_check.ANDROID_LATEST_URL, "v1.0.1"), + ], +) +class TestARefusedCheckIsRetriedSoon: + def _run(self, refresh_name, client, clock): + with ( + patch.dict("os.environ", {"CASHPILOT_VERSION": "2.0.0"}, clear=False), + patch("app.update_check.httpx.AsyncClient", return_value=client), + patch("app.update_check.time.time", clock), + ): + return asyncio.run(getattr(update_check, refresh_name)()) + + def test_a_403_at_startup_is_retried_after_an_hour_not_a_day(self, refresh_name, url, tag): + clock = _Clock() + client = _client(_rate_limited(url), _ok(url, tag)) + + assert self._run(refresh_name, client, clock) is None # startup: refused + + clock.now += update_check.RETRY_AFTER_FAILURE_SECONDS - 1 + assert self._run(refresh_name, client, clock) is None + assert client.get.await_count == 1, "retried before the failure cooldown ran out" + + clock.now += 1 + assert self._run(refresh_name, client, clock) == tag + assert client.get.await_count == 2, "a refused check waited longer than the retry window" + + def test_a_check_that_worked_still_waits_a_day(self, refresh_name, url, tag): + clock = _Clock() + client = _client(_ok(url, tag), _ok(url, "v9.9.9")) + + assert self._run(refresh_name, client, clock) == tag + clock.now += update_check.RETRY_AFTER_FAILURE_SECONDS * 5 + assert self._run(refresh_name, client, clock) == tag + assert client.get.await_count == 1, "a working install asked GitHub more than once a day" + + clock.now = 1_000_000.0 + update_check.CHECK_INTERVAL_SECONDS + assert self._run(refresh_name, client, clock) == "v9.9.9" + assert client.get.await_count == 2 + + def test_a_failure_after_a_success_keeps_the_known_tag(self, refresh_name, url, tag): + """A refused retry must not blank what was already known.""" + clock = _Clock() + client = _client(_ok(url, tag), _rate_limited(url)) + + assert self._run(refresh_name, client, clock) == tag + clock.now += update_check.CHECK_INTERVAL_SECONDS + assert self._run(refresh_name, client, clock) == tag + assert client.get.await_count == 2 + + +def test_the_retry_window_is_shorter_than_a_day_and_not_a_hammer(): + assert 15 * 60 <= update_check.RETRY_AFTER_FAILURE_SECONDS < update_check.CHECK_INTERVAL_SECONDS From 93e5a7dfd7c67b3824a959e6a9b6992a150ddaf0 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Sat, 7 Nov 2026 23:26:03 +0100 Subject: [PATCH 2/2] fix: retry on a tag-less reply too, and tick often enough that the retry never slips a window --- app/main.py | 12 ++--- app/update_check.py | 18 +++++-- tests/test_main_routes.py | 10 ++-- .../test_update_check_retry_after_failure.py | 50 +++++++++++++++++++ 4 files changed, 76 insertions(+), 14 deletions(-) diff --git a/app/main.py b/app/main.py index 008fe2ef..218a1962 100644 --- a/app/main.py +++ b/app/main.py @@ -2072,15 +2072,15 @@ def _on_job_event(event): coalesce=True, misfire_grace_time=300, ) - # Hourly tick, daily fetch: refresh() asks GitHub once a day after a check + # Frequent tick, daily fetch: refresh() asks GitHub once a day after a check # that worked and an hour after one that failed, so a rate-limited startup - # check no longer leaves the release unknown for a whole day. Every failure - # mode inside refresh() lands on UNKNOWN, so a firewalled install produces - # no noise at all. + # check no longer leaves the release unknown for a whole day. A tick that + # is not due fetches nothing. Every failure mode inside refresh() lands on + # UNKNOWN, so a firewalled install produces no noise at all. scheduler.add_job( update_check.refresh, "interval", - hours=1, + seconds=update_check.SCHEDULER_TICK_SECONDS, id="update_check", max_instances=1, coalesce=True, @@ -2095,7 +2095,7 @@ def _on_job_event(event): scheduler.add_job( update_check.refresh_android, "interval", - hours=1, + seconds=update_check.SCHEDULER_TICK_SECONDS, id="update_check_android", max_instances=1, coalesce=True, diff --git a/app/update_check.py b/app/update_check.py index 883bedb8..ba79d2cf 100644 --- a/app/update_check.py +++ b/app/update_check.py @@ -60,10 +60,17 @@ #: per public IP) with everything else in the house, so the one check made at #: startup is often refused with a 403; waiting a full day after that left the #: release unknown, and every phone unjudged, for 24 hours after each restart. -#: The scheduler calls refresh() hourly; refresh() decides whether a fetch is -#: due, so a working install still asks GitHub once a day. +#: A reply with no usable release tag counts as a failure too, and keeps the +#: tag already known. The scheduler calls refresh() every +#: SCHEDULER_TICK_SECONDS; refresh() decides whether a fetch is due, so a +#: working install still asks GitHub once a day. RETRY_AFTER_FAILURE_SECONDS = 60 * 60 +#: How often the scheduler calls refresh(). Well under the retry window, so a +#: tick that lands a few seconds before a cooldown ends does not push the retry +#: back by a whole extra window. A tick that is not due fetches nothing. +SCHEDULER_TICK_SECONDS = 15 * 60 + _HTTP_TIMEOUT = 10.0 #: Cached result. `tag` is None for UNKNOWN -- never for "up to date", which is a @@ -130,6 +137,8 @@ async def refresh(*, force: bool = False) -> str | None: response = await client.get(LATEST_URL, headers={"Accept": "application/vnd.github+json"}) response.raise_for_status() tag = _parse_tag(response.json()) + if tag is None: + raise ValueError("no release tag in the reply") except Exception as exc: # noqa: BLE001 - every failure means the same thing # DEBUG, not WARNING. A firewalled install would otherwise log a warning # every day forever about a feature its operator never asked for, and a @@ -142,8 +151,7 @@ async def refresh(*, force: bool = False) -> str | None: _latest_tag = tag _checked_at = time.time() _last_failed = False - if tag: - logger.debug("Newest published release is %s", tag) + logger.debug("Newest published release is %s", tag) return tag @@ -167,6 +175,8 @@ async def refresh_android(*, force: bool = False) -> str | None: response = await client.get(ANDROID_LATEST_URL, headers={"Accept": "application/vnd.github+json"}) response.raise_for_status() tag = _parse_tag(response.json()) + if tag is None: + raise ValueError("no release tag in the reply") except Exception as exc: # noqa: BLE001 - every failure means the same thing logger.debug("Android update check could not reach %s: %s", ANDROID_LATEST_URL, exc) _android_checked_at = time.time() diff --git a/tests/test_main_routes.py b/tests/test_main_routes.py index 6569fef4..1e663c6c 100644 --- a/tests/test_main_routes.py +++ b/tests/test_main_routes.py @@ -2269,11 +2269,13 @@ async def _run(): assert job.max_instances == 1, f"{job.id} max_instances" assert job.coalesce is True, f"{job.id} coalesce" assert job.misfire_grace_time == 300, f"{job.id} misfire_grace_time" - # The release checks tick hourly so a refused check is - # retried within the hour; refresh() itself keeps a - # working install to one GitHub call a day. + # The release checks tick well inside the retry window so + # a refused check is retried about an hour later; + # refresh() itself keeps a working install to one GitHub + # call a day. for job_id in ("update_check", "update_check_android"): - assert jobs[job_id].trigger.interval.total_seconds() == 3600, job_id + tick = jobs[job_id].trigger.interval.total_seconds() + assert tick <= main_mod.update_check.RETRY_AFTER_FAILURE_SECONDS / 2, job_id # The error/missed listener is attached. assert len(sched._listeners) >= 1 diff --git a/tests/test_update_check_retry_after_failure.py b/tests/test_update_check_retry_after_failure.py index 78903329..6ed761a0 100644 --- a/tests/test_update_check_retry_after_failure.py +++ b/tests/test_update_check_retry_after_failure.py @@ -110,3 +110,53 @@ def test_a_failure_after_a_success_keeps_the_known_tag(self, refresh_name, url, def test_the_retry_window_is_shorter_than_a_day_and_not_a_hammer(): assert 15 * 60 <= update_check.RETRY_AFTER_FAILURE_SECONDS < update_check.CHECK_INTERVAL_SECONDS + + +@pytest.mark.parametrize( + ("refresh_name", "url"), + [ + ("refresh", update_check.LATEST_URL), + ("refresh_android", update_check.ANDROID_LATEST_URL), + ], +) +def test_a_reply_without_a_usable_tag_keeps_the_known_tag_and_retries_soon(refresh_name, url): + """A 200 whose tag is not a version is a failed check, not "no release".""" + clock = _Clock() + bad = httpx.Response(200, json={"tag_name": "nightly"}, request=httpx.Request("GET", url)) + client = _client(_ok(url, "v1.2.3"), bad, _ok(url, "v1.2.4")) + with ( + patch.dict("os.environ", {"CASHPILOT_VERSION": "2.0.0"}, clear=False), + patch("app.update_check.httpx.AsyncClient", return_value=client), + patch("app.update_check.time.time", clock), + ): + refresh = getattr(update_check, refresh_name) + assert asyncio.run(refresh()) == "v1.2.3" + clock.now += update_check.CHECK_INTERVAL_SECONDS + assert asyncio.run(refresh()) == "v1.2.3", "an unusable reply blanked the known tag" + clock.now += update_check.RETRY_AFTER_FAILURE_SECONDS + assert asyncio.run(refresh()) == "v1.2.4", "an unusable reply waited a day before the next try" + assert client.get.await_count == 3 + + +def test_a_tick_just_before_the_cooldown_ends_does_not_cost_a_whole_extra_window(): + """Ticks run every SCHEDULER_TICK_SECONDS from scheduling time; the startup + check fails a few seconds later, so the first tick at the hour mark is just + early. The retry must not slip by a whole extra window.""" + clock = _Clock() + url = update_check.LATEST_URL + client = _client(_rate_limited(url), _ok(url, "v2.0.1")) + scheduled_at = clock.now + with ( + patch.dict("os.environ", {"CASHPILOT_VERSION": "2.0.0"}, clear=False), + patch("app.update_check.httpx.AsyncClient", return_value=client), + patch("app.update_check.time.time", clock), + ): + clock.now += 5 # the startup check runs a few seconds after the jobs are added + assert asyncio.run(update_check.refresh()) is None + tick = 0 + while client.get.await_count < 2: + tick += 1 + clock.now = scheduled_at + tick * update_check.SCHEDULER_TICK_SECONDS + asyncio.run(update_check.refresh()) + waited = clock.now - (scheduled_at + 5) + assert waited < update_check.RETRY_AFTER_FAILURE_SECONDS * 1.5, f"retry came {waited:.0f} s after the failure"