diff --git a/backend/services/garmin.py b/backend/services/garmin.py index 45edabb..b0b2daf 100644 --- a/backend/services/garmin.py +++ b/backend/services/garmin.py @@ -185,11 +185,15 @@ class RateLimited(RuntimeError): """ -# Retrying while rate limited is what deepens the limit, so once Garmin says -# 429 the whole process stands down until this passes. The account was stuck for -# days because the backoff kept expiring before Garmin's own (multi-hour) window -# closed, so every tick re-hit it and the limit never lifted. A 24h stand-down -# is what actually outlasts the throttle and lets the window close for good. +# Garmin answering 429 is what deepens the limit: the account once stayed +# stuck for days because every retry re-hit the throttle before its own +# (multi-hour) window closed. We therefore record a 24h cooldown when a 429 +# actually arrives, so the UI and the sync history can say when to expect +# recovery. The cooldown is *information*, not a gate: since 2026-09-02 the +# local estimate is no longer used to refuse a sync before it starts — every +# entry point (manual, 立即同步 and the automatic scheduler alike) issues a +# real request and trusts Garmin's live answer. Only a real 429 writes a new +# cooldown; a stale local estimate must never keep a healthy account idle. RATE_LIMIT_BACKOFF = datetime.timedelta(hours=24) # In-process cache of the cooldown, kept in sync with the DB copy below and # still the lever the tests reach for via _rate_limited_until.clear(). @@ -263,11 +267,29 @@ def _note_rate_limit(user_id): ) -def _rate_limit_block(user_id): - """If the account is cooling down, return (until, message); else (None, None). +def _clear_rate_limit(user_id): + """Drop a recorded cooldown after a sync that actually succeeded. - Central guard used by both the manual and scheduled sync entry points, so a - blocked account issues zero Garmin requests until the window closes. + A live success is proof Garmin stopped throttling, so the estimate has + served its purpose; leaving a future deadline behind would just mislead + the next diagnosis. + """ + _rate_limited_until.pop(user_id, None) + try: + execute( + "UPDATE sync_status SET rate_limited_until = NULL " + "WHERE user_id = ?", [user_id], + ) + except Exception: # noqa: BLE001 - clearing is best-effort + pass + + +def _rate_limit_block(user_id): + """Human-readable recovery estimate from the *recorded* cooldown. + + Only meaningful right after a real 429 wrote a fresh cooldown (mid-run + stand-down). It is not a pre-request gate — nothing consults the cooldown + before calling Garmin any more. """ until = rate_limited_until(user_id) if not until or datetime.datetime.utcnow() >= until: @@ -457,13 +479,6 @@ def _connect(creds, user_id=None): client.garth.loads(token) _use_api_user_agent(client) - blocked = rate_limited_until(user_id) - if blocked and datetime.datetime.utcnow() < blocked: - raise RateLimited( - "Garmin 暂时限制了请求频率，稍后会自动恢复（约 " - f"{max(1, int((blocked - datetime.datetime.utcnow()).total_seconds() // 60))} 分钟）。" - ) - # Only when it has actually expired. Refreshing on every connect spends # quota for nothing, and that is what walked the account into a 429. oauth2 = getattr(client.garth, "oauth2_token", None) @@ -1047,33 +1062,13 @@ def start_sync(user_id, creds, days=None): Progress lands in sync_status, which the UI polls; a full backfill runs far longer than any sensible HTTP timeout. + + No local rate-limit estimate is consulted here: every manual start issues + a real request and trusts Garmin's live answer. A real 429 writes the + cooldown and the run stands down inside sync_data. """ days = DEFAULT_SYNC_DAYS if days is None else days now = datetime.datetime.utcnow().isoformat(timespec="seconds") - until, msg = _rate_limit_block(user_id) - if until: - # A refused start synced nothing — keep the previous last_sync_time so - # the UI's 上次同步 keeps telling the truth (the data is stale since - # the last *successful* sync, and that date is what the user needs). - row = query_one( - "SELECT last_sync_time FROM sync_status WHERE user_id = ?", [user_id] - ) - _set_sync_status( - user_id, "rate_limited", now, - last_sync_time=row["last_sync_time"] if row else None, - records_synced=0, progress_current=0, progress_total=days, - started_at=now, last_error=msg, - ) - _log_sync_history(user_id, "manual", days, now, { - "status": "rate_limited", - "recordsSynced": 0, - "message": msg, - }) - return { - "status": "rate_limited", - "message": msg, - "retryAfterSeconds": int((until - datetime.datetime.utcnow()).total_seconds()), - } _set_sync_status( user_id, "syncing", now, records_synced=0, progress_current=0, progress_total=days, @@ -1104,25 +1099,10 @@ def sync_data(user_id, creds, days=None, client=None, trigger="manual"): _log_sync_history(user_id, trigger, days, started, result) return result - until, msg = _rate_limit_block(user_id) - if until: - # A blocked account must issue zero Garmin requests — that is the whole - # point. Return immediately without touching the network, and keep the - # previous last_sync_time (this was not a sync). - row = query_one( - "SELECT last_sync_time FROM sync_status WHERE user_id = ?", [user_id] - ) - _set_sync_status( - user_id, "rate_limited", now, - last_sync_time=row["last_sync_time"] if row else None, - records_synced=0, last_error=msg, - ) - return finish({ - "status": "rate_limited", - "recordsSynced": 0, - "message": msg, - "retryAfterSeconds": int((until - datetime.datetime.utcnow()).total_seconds()), - }) + # No local cooldown gate here (2026-09-02): the recorded rate_limited_until + # is an estimate, and refusing on it kept accounts idle after Garmin had + # already recovered. Every run issues a real request; only a real 429 + # stands the run down (mid-run, below) and writes a fresh cooldown. _set_sync_status( user_id, "syncing", now, records_synced=0, progress_current=0, progress_total=days, @@ -1331,6 +1311,9 @@ def sync_data(user_id, creds, days=None, client=None, trigger="manual"): progress_current=days, progress_total=days, stage=None, last_error="; ".join(day_errors[:3]) if day_errors else None, ) + # A live success proves Garmin stopped throttling — retire any recorded + # cooldown so it cannot mislead a later diagnosis. + _clear_rate_limit(user_id) message = ( f"同步完成，更新 {days_synced} 天数据、{activities_synced} 条运动记录" f"（含 {details_synced} 条详情）、" diff --git a/backend/services/scheduler.py b/backend/services/scheduler.py index b842c72..3fd68d8 100644 --- a/backend/services/scheduler.py +++ b/backend/services/scheduler.py @@ -151,16 +151,10 @@ def sync_all_accounts(days=None, respect_schedule=False): "reason": "not due"}) continue d = SYNC_DAYS if days is None else days - # Never poke Garmin while it is rate-limiting us — that is exactly - # what keeps the limit alive. Respect the persisted cooldown and sit - # this tick out. - blocked = garmin_svc.rate_limited_until(uid) - if blocked and blocked > _now(): - results.append({ - "user": uid, "status": "skipped", "reason": "rate-limited", - "retryAfterSeconds": int((blocked - _now()).total_seconds()), - }) - continue + # 2026-09-02: the recorded cooldown is no longer consulted before + # a sync — a stale estimate must not keep a healthy account idle, + # and only Garmin's live answer (a real 429, handled inside + # sync_data) decides whether the throttle is actually closed. out = garmin_svc.sync_data(uid, {}, days=d, trigger="auto") results.append({"user": uid, "status": out.get("status"), "records": out.get("recordsSynced")}) diff --git a/backend/tests/test_garmin_sync.py b/backend/tests/test_garmin_sync.py index b37ff1e..4a6065b 100644 --- a/backend/tests/test_garmin_sync.py +++ b/backend/tests/test_garmin_sync.py @@ -691,18 +691,27 @@ class TestRateLimiting: until = garmin_svc.rate_limited_until(user["id"]) assert until is not None and until > datetime.datetime.utcnow() - def test_a_blocked_account_does_not_call_garmin_again(self, db, user, monkeypatch): - """The retry is what deepens the limit, so it must not happen.""" + def test_a_recorded_cooldown_no_longer_blocks_connect(self, db, user, monkeypatch): + """Since 2026-09-02 the cooldown is information, not a gate: a stale + local estimate must not keep an account idle after Garmin recovered, + so _connect issues the refresh and trusts the live answer. Only a real + 429 (raised by refresh_oauth2) records a fresh cooldown and surfaces + as RateLimited.""" garmin_svc.save_token(user["id"], "token-blob") - garmin_svc._note_rate_limit(user["id"]) + garmin_svc._note_rate_limit(user["id"]) # a future deadline is recorded + + calls = [] + + def refresh(self): # bound by the stub instance -> receives self + calls.append(1) + raise RuntimeError("too many 429 error responses") class Stub: def __init__(self, *a, **k): self.garth = type("G", (), { "configure": lambda *a, **k: None, "loads": lambda *a: None, - "refresh_oauth2": lambda *a: (_ for _ in ()).throw( - AssertionError("must not reach Garmin while backing off")), + "refresh_oauth2": refresh, "oauth2_token": None, "sess": type("S", (), {"headers": {}})(), })() @@ -710,6 +719,31 @@ class TestRateLimiting: monkeypatch.setattr(garmin_svc, "_import_garmin", lambda: Stub) with pytest.raises(garmin_svc.RateLimited): garmin_svc._connect({}, user_id=user["id"]) + assert calls == [1], "the real request must still be attempted" + assert garmin_svc.rate_limited_until(user["id"]) is not None + + def test_a_stale_cooldown_does_not_stop_a_healthy_connect(self, db, user, monkeypatch): + """The mirror image: with a cooldown recorded but Garmin healthy, the + refresh succeeds and the connect proceeds (no local refusal).""" + garmin_svc.save_token(user["id"], "token-blob") + garmin_svc._note_rate_limit(user["id"]) + calls = [] + + class Stub: + def __init__(self, *a, **k): + unexpired = type("T", (), {"expired": False})() + self.garth = type("G", (), { + "configure": lambda *a, **k: None, + "loads": lambda *a: None, + "refresh_oauth2": lambda *a: calls.append(1), + "oauth2_token": unexpired, + "profile": {"displayName": "x"}, + "sess": type("S", (), {"headers": {}})(), + })() + + monkeypatch.setattr(garmin_svc, "_import_garmin", lambda: Stub) + garmin_svc._connect({}, user_id=user["id"]) + assert calls == [], "an unexpired token needs no refresh" def test_a_valid_token_is_not_refreshed(self, db, user, monkeypatch): """Refreshing on every connect spends quota for nothing — and that is @@ -902,13 +936,36 @@ class TestSyncHistory: assert garmin_svc.get_sync_history(user["id"])[0]["status"] == "rate_limited" garmin_svc._rate_limited_until.clear() - def test_a_refused_sync_is_still_a_record(self, db, user, monkeypatch): + def test_a_recorded_cooldown_does_not_refuse_a_sync(self, db, user, monkeypatch): + """The local cooldown is an estimate, not a gate (2026-09-02): a sync + with a recorded deadline still runs for real and records its actual + outcome — Garmin's live answer decides, not our guess.""" + future = datetime.datetime.utcnow() + datetime.timedelta(hours=1) + monkeypatch.setitem(garmin_svc._rate_limited_until, user["id"], future) + garmin_svc.save_token(user["id"], "token-blob") + + out = garmin_svc.sync_data(user["id"], CREDS, days=1, client=StubClient()) + assert out["status"] == "success", "a stale cooldown must not block the run" + row = garmin_svc.get_sync_history(user["id"])[0] + assert row["status"] == "success" + # A success retires the recorded cooldown. + assert garmin_svc.rate_limited_until(user["id"]) is None + + def test_a_real_429_mid_run_is_still_a_rate_limited_record(self, db, user, monkeypatch): + """Only a genuine 429 stands a run down — and it writes a fresh + cooldown instead of relying on whatever stale estimate was there.""" future = datetime.datetime.utcnow() + datetime.timedelta(hours=1) monkeypatch.setitem(garmin_svc._rate_limited_until, user["id"], future) - out = garmin_svc.sync_data(user["id"], CREDS, days=1, client=StubClient()) + class Throttled(StubClient): + def get_user_summary(self, cdate): + self.calls.append(("summary", cdate)) + raise RuntimeError("too many 429 error responses") + + out = garmin_svc.sync_data(user["id"], CREDS, days=2, client=Throttled()) assert out["status"] == "rate_limited" assert garmin_svc.get_sync_history(user["id"])[0]["status"] == "rate_limited" + assert garmin_svc.rate_limited_until(user["id"]) is not None def test_history_is_per_user(self, db, user, make_user): garmin_svc.sync_data(user["id"], CREDS, days=1, client=StubClient())