From 41b7ae82e4055b8c4e6a3afd4c8dd7b902cf07c9 Mon Sep 17 00:00:00 2001 From: ericwyuan Date: Tue, 1 Sep 2026 08:11:24 +0800 Subject: [PATCH] =?UTF-8?q?fix(sync):=20=E3=80=8C=E5=85=A8=E9=83=A8?= =?UTF-8?q?=E5=8E=86=E5=8F=B2=E3=80=8D=E7=9C=9F=E7=9A=84=E6=8B=89=E5=85=A8?= =?UTF-8?q?=E9=83=A8=E5=8E=86=E5=8F=B2=EF=BC=8C=E8=87=AA=E5=8A=A8=E5=90=8C?= =?UTF-8?q?=E6=AD=A5=E4=B8=8D=E5=86=8D=E6=AF=8F=E6=AC=A1=E9=9D=99=E9=BB=98?= =?UTF-8?q?=E5=A4=B1=E8=B4=A5?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 四个独立的 bug 叠在一起，表现为「只同步两天、没有进度」： * 前端 `...(days ? { days } : {})` 把 days=0 当成未传。「全部历史」 存的就是 0，请求体里根本没有 days，后端退回 7 天默认值。 * scheduler 用 `s[0]` 读 query_one 返回的 dict，抛 KeyError 后被 per-account 的 except 吞掉。只要用户存过一次设置，每 30 分钟的 自动同步就一次都没成功过——库里那 2 天全是手动点出来的。 * 增量同步查 `health_daily`（表其实叫 health_data），后台线程直接 死掉，状态永远卡在 syncing，进度条不动。 * UI 完全不看 /sync 的返回值，rate_limited 时按钮点了没反应；轮询 结束时又把 rate_limited 归进 else 分支报「同步完成」。 顺带： * 日循环遇到 429 立即退避并保留已拉到的天数，而不是当成「跳过一天」 继续往下捶 700 天——这正是之前限流死循环的来源之一。 * 定时循环显式传 SYNC_DAYS。历史范围按 UI 文案只描述手动全量同步， 让半小时一次的 tick 重拉 730 天必然把限流撞得更深。 * 短同步逐天上报进度（原来每 5 天一次，7 天的同步全程停在 0）。 * /sync 路由重复解析 body，空 body 会 None.get 崩。 * 4 个 StubGarth 缺 configure()，7 个测试在此之前一直是红的。 新增 deploy/push.sh：NAS 只认密码，脚本开一个 ssh 复用连接，密码只 输一次，后面推送 / 重启 / 健康检查全走它。不碰 .env、.venv 和数据库。 Co-Authored-By: Claude Opus 5 --- backend/routes/garmin.py | 4 +- backend/services/garmin.py | 44 ++++++++++++++-- backend/services/scheduler.py | 20 +++++-- backend/tests/test_garmin_sync.py | 87 ++++++++++++++++++++++++++++++- backend/tests/test_scheduler.py | 55 +++++++++++++++++++ client/src/pages/SyncPage.tsx | 20 +++++-- client/src/services/api.ts | 13 +++-- deploy/push.sh | 60 +++++++++++++++++++++ 8 files changed, 283 insertions(+), 20 deletions(-) create mode 100755 deploy/push.sh diff --git a/backend/routes/garmin.py b/backend/routes/garmin.py index 56d079b..a146a47 100644 --- a/backend/routes/garmin.py +++ b/backend/routes/garmin.py @@ -43,7 +43,9 @@ def sync(): 400, ) - days = request.get_json(silent=True).get("days") if request.is_json else None + # `data` is the body already parsed above; re-parsing here used to blow up + # on a body-less POST (`None.get`). + days = data.get("days") try: if days is not None: days = int(days) diff --git a/backend/services/garmin.py b/backend/services/garmin.py index 30dc8ef..ec72673 100644 --- a/backend/services/garmin.py +++ b/backend/services/garmin.py @@ -1021,8 +1021,14 @@ def sync_data(user_id, creds, days=None, client=None): # -1 means "incremental sync": pick up from the latest date already in the # local database rather than pulling a fixed window. if days == -1: - row = query_one("SELECT MAX(date) FROM health_daily WHERE user_id = ?", (user_id,)) - latest = row[0] if row and row[0] else None + # The daily rows live in `health_data`; `health_daily` is the name of + # the *writer* (health.upsert_health_daily), not of any table. Reading + # it raised inside the background thread, so an incremental sync died + # silently and left the status stuck on "syncing". + row = query_one( + "SELECT MAX(date) AS latest FROM health_data WHERE user_id = ?", (user_id,) + ) + latest = row["latest"] if row and row.get("latest") else None if latest is None: days = DEFAULT_SYNC_DAYS # first sync → fall back to default window else: @@ -1038,12 +1044,21 @@ def sync_data(user_id, creds, days=None, client=None): days_synced = 0 day_errors = [] + rate_limited_mid_run = None for i in range(days): date_str = (today - datetime.timedelta(days=i)).isoformat() try: record = _extract_daily(client, date_str) record.update(extras.daily_extras(client, date_str)) except Exception as e: + # A 429 inside the day loop used to be filed as one more skipped + # day, so a 730-day backfill kept hammering Garmin for another + # 700 days and drove the throttle deeper. Stand down at the first + # one and keep whatever was already stored. + if isinstance(e, RateLimited) or _is_rate_limited(e): + _note_rate_limit(user_id) + rate_limited_mid_run = describe(e) + break day_errors.append(f"{date_str}: {describe(e)}") continue # A day Garmin has no data for comes back all-None; storing it would @@ -1060,15 +1075,34 @@ def sync_data(user_id, creds, days=None, client=None): except Exception as e: # noqa: BLE001 day_errors.append(f"{date_str} series: {describe(e)}") - # Reported every few days rather than every day: the write is cheap - # but not free, and the UI polls on a 2s cadence anyway. - if (i + 1) % 5 == 0 or i + 1 == days: + # A long backfill reports every fifth day — the write is cheap but not + # free. A short one reports every day: at 7 days a "every 5th" cadence + # meant the bar sat at 0 for most of the run and then vanished. + if days <= 30 or (i + 1) % 5 == 0 or i + 1 == days: _set_sync_status( user_id, "syncing", now, records_synced=days_synced, progress_current=i + 1, progress_total=days, stage=f"每日数据 {date_str}", ) + if rate_limited_mid_run: + until, msg = _rate_limit_block(user_id) + message = msg or "Garmin 限制了请求频率，已自动退避。" + _set_sync_status( + user_id, "rate_limited", now, records_synced=days_synced, + progress_current=days_synced, progress_total=days, stage=None, + last_error=message, + ) + return { + "status": "rate_limited", + "recordsSynced": days_synced, + "message": f"已同步 {days_synced} 天后被 Garmin 限流：{message}", + "retryAfterSeconds": ( + int((until - datetime.datetime.utcnow()).total_seconds()) if until else None + ), + "lastSyncTime": now, + } + activities_synced = 0 _set_sync_status(user_id, "syncing", now, records_synced=days_synced, progress_current=days, progress_total=days, diff --git a/backend/services/scheduler.py b/backend/services/scheduler.py index 192d92a..8473d53 100644 --- a/backend/services/scheduler.py +++ b/backend/services/scheduler.py @@ -130,7 +130,8 @@ def sync_all_accounts(days=None, respect_schedule=False): When `days` is not provided, each account's saved `history_days` from `user_settings` is used (the user's 历史范围 picker), falling back to - `SYNC_DAYS`. + `SYNC_DAYS`. The scheduled loop always passes `days` explicitly — 历史范围 + is the window for the manual 全量同步, not for a half-hourly tick. """ rows = query_all("SELECT user_id FROM garmin_tokens") results = [] @@ -150,8 +151,14 @@ def sync_all_accounts(days=None, respect_schedule=False): # Resolve the sync window: prefer the user's saved history_days, # then the caller override, then the global default. if days is None: - s = query_one("SELECT history_days FROM user_settings WHERE user_id = ?", (uid,)) - user_days = s[0] if s and s[0] is not None else None + # query_one returns a dict, so `s[0]` raised KeyError — caught + # by the per-account handler below, which meant every single + # scheduled tick failed for every account and nothing was ever + # synced automatically. + s = query_one( + "SELECT history_days FROM user_settings WHERE user_id = ?", (uid,) + ) + user_days = s.get("history_days") if s else None if user_days == 0: user_days = 730 # 全部历史 → 最大范围 d = user_days if user_days is not None else SYNC_DAYS @@ -180,7 +187,12 @@ def _loop(): try: if claim(interval=TICK_SECONDS): try: - sync_all_accounts(respect_schedule=True) + # Explicitly the recent window, never the user's 历史范围: + # that setting describes the manual 全量同步. Letting a + # half-hourly tick re-pull "全部历史" meant 730 days x ~7 + # Garmin calls every 30 minutes, which is precisely what + # kept the account in a 429 loop. + sync_all_accounts(days=SYNC_DAYS, respect_schedule=True) finally: release() except Exception as e: # noqa: BLE001 - the loop must outlive any single failure diff --git a/backend/tests/test_garmin_sync.py b/backend/tests/test_garmin_sync.py index 0e89087..36ac3d6 100644 --- a/backend/tests/test_garmin_sync.py +++ b/backend/tests/test_garmin_sync.py @@ -357,6 +357,8 @@ class TestMfaHandling: """garth's default MFA prompt calls input(); with no stdin that raises a bare EOFError, which says nothing about what to do about it.""" class StubGarth: + # _connect widens garth's 10s timeout before any call. + def configure(self, **kw): pass def loads(self, s): pass def refresh_oauth2(self): pass @@ -397,6 +399,9 @@ class TestMfaHandling: class StubGarth: profile = {"displayName": "Tester"} + # _connect widens garth's 10s timeout before any call. + def configure(self, **kw): pass + def loads(self, s): loaded["blob"] = s @@ -419,7 +424,7 @@ class TestMfaHandling: def test_no_token_and_no_password_is_refused_clearly(self, db, user, monkeypatch): class StubGarmin: def __init__(self, *a, **k): - self.garth = None + self.garth = type("G", (), {"configure": lambda *a, **k: None})() monkeypatch.setattr(garmin_svc, "_import_garmin", lambda: StubGarmin) with pytest.raises(RuntimeError, match="缺少 Garmin 密码"): @@ -464,6 +469,9 @@ class TestApiUserAgent: class StubGarth: profile = {"displayName": "Tester"} + + # _connect widens garth's 10s timeout before any call. + def configure(self, **kw): pass def __init__(self): self.sess = StubSess() def loads(self, s): pass def refresh_oauth2(self): pass @@ -491,6 +499,8 @@ class TestApiUserAgent: request to '.../None'.""" class StubGarth: profile = {"displayName": "Tester"} + # _connect widens garth's 10s timeout before any call. + def configure(self, **kw): pass sess = type("S", (), {"headers": {}})() def loads(self, s): pass def refresh_oauth2(self): pass @@ -689,6 +699,7 @@ class TestRateLimiting: 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")), @@ -710,6 +721,7 @@ class TestRateLimiting: 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, @@ -720,3 +732,76 @@ class TestRateLimiting: monkeypatch.setattr(garmin_svc, "_import_garmin", lambda: Stub) garmin_svc._connect({}, user_id=user["id"]) assert calls == [], "an unexpired token needs no refresh" + + +class TestSyncWindow: + """How many days a sync actually covers. + + The 历史范围 picker offers 全部历史 (0) and 自上次同步 (-1); both used to + end up pulling the 7-day default instead — the first because the client + dropped a falsy `days`, the second because the incremental lookup read a + table that does not exist and died inside the background thread. + """ + + def setup_method(self): + garmin_svc._rate_limited_until.clear() + + def teardown_method(self): + garmin_svc._rate_limited_until.clear() + + def test_incremental_resumes_from_the_latest_stored_day( + self, db, user, seed_health + ): + seed_health([{"date": day(5), "steps": 4000}]) + client = StubClient() + out = garmin_svc.sync_data(user["id"], CREDS, days=-1, client=client) + + pulled = {c[1] for c in client.calls if c[0] == "summary"} + assert out["status"] == "success" + assert pulled == {day(i) for i in range(5)} + + def test_incremental_without_history_falls_back_to_the_default(self, db, user): + client = StubClient() + garmin_svc.sync_data(user["id"], CREDS, days=-1, client=client) + pulled = {c[1] for c in client.calls if c[0] == "summary"} + assert len(pulled) == garmin_svc.DEFAULT_SYNC_DAYS + + def test_full_history_asks_for_the_maximum_window(self, db, user): + """`days=0` is 全部历史, not "unset".""" + client = StubClient() + garmin_svc.sync_data(user["id"], CREDS, days=0, client=client) + assert len([c for c in client.calls if c[0] == "summary"]) == 730 + + def test_the_sync_endpoint_passes_a_zero_through( + self, client, auth, user, db, monkeypatch + ): + garmin_svc.save_token(user["id"], "blob") + seen = {} + + def record(uid, creds, days=None): + seen["days"] = days + return {"status": "syncing", "days": days} + + monkeypatch.setattr(garmin_svc, "start_sync", record) + r = client.post("/api/garmin/sync", headers=auth, json={"days": 0}) + assert r.status_code == 202 + assert seen["days"] == 730, "全部历史 must not fall back to the default" + + def test_a_429_mid_run_stops_the_sync(self, db, user): + """Filing a 429 as one more skipped day meant a 730-day backfill kept + hammering Garmin for another 700 days and dug the throttle deeper.""" + + class Throttled(StubClient): + def get_user_summary(self, cdate): + self.calls.append(("summary", cdate)) + if cdate == day(2): + raise RuntimeError("too many 429 error responses") + return summary() + + client = Throttled() + out = garmin_svc.sync_data(user["id"], CREDS, days=365, client=client) + + assert out["status"] == "rate_limited" + assert out["recordsSynced"] == 2, "the days pulled before the 429 are kept" + assert len([c for c in client.calls if c[0] == "summary"]) == 3 + assert garmin_svc.rate_limited_until(user["id"]) is not None diff --git a/backend/tests/test_scheduler.py b/backend/tests/test_scheduler.py index 513799f..feb9e4e 100644 --- a/backend/tests/test_scheduler.py +++ b/backend/tests/test_scheduler.py @@ -184,3 +184,58 @@ class TestEndpoints: r = client.get("/api/garmin/auto-sync", headers=auth) assert r.status_code == 200 assert "intervalSeconds" in r.get_json() + + +class TestSyncWindowResolution: + """Which window each account is synced over. + + `query_one` returns a dict, so reading the saved 历史范围 as `s[0]` raised + KeyError — swallowed by the per-account handler, which meant every + scheduled tick failed for every account that had ever opened 设置, and + nothing was synced automatically at all. + """ + + def _account(self, user, monkeypatch): + from services import garmin as garmin_svc + + garmin_svc.save_token(user["id"], "t1") + seen = {} + + def record(uid, creds, days=None, client=None): + seen["days"] = days + return {"status": "success", "recordsSynced": 0} + + monkeypatch.setattr(garmin_svc, "sync_data", record) + return seen + + def test_saved_history_days_is_honoured(self, db, user, monkeypatch): + from services import settings as settings_svc + + seen = self._account(user, monkeypatch) + settings_svc.save_settings(user["id"], {"historyDays": 90}) + + results = scheduler.sync_all_accounts() + assert [r["status"] for r in results] == ["success"] + assert seen["days"] == 90 + + def test_full_history_becomes_the_maximum(self, db, user, monkeypatch): + from services import settings as settings_svc + + seen = self._account(user, monkeypatch) + settings_svc.save_settings(user["id"], {"historyDays": 0}) + + scheduler.sync_all_accounts() + assert seen["days"] == 730 + + def test_the_scheduled_tick_ignores_the_history_setting( + self, db, user, monkeypatch + ): + """历史范围 is the window for the manual 全量同步. A half-hourly tick + re-pulling 全部历史 is what kept the account in a 429 loop.""" + from services import settings as settings_svc + + seen = self._account(user, monkeypatch) + settings_svc.save_settings(user["id"], {"historyDays": 0, "autoSync": True}) + + scheduler.sync_all_accounts(days=scheduler.SYNC_DAYS, respect_schedule=True) + assert seen["days"] == scheduler.SYNC_DAYS diff --git a/client/src/pages/SyncPage.tsx b/client/src/pages/SyncPage.tsx index fd40888..d311858 100644 --- a/client/src/pages/SyncPage.tsx +++ b/client/src/pages/SyncPage.tsx @@ -218,8 +218,14 @@ function SyncPage() { setLoading(true); try { // 0 means "everything"; the backend caps it at what Garmin will serve. - await apiClient.syncGarminData(settings?.historyDays ?? 3650); - await refresh(); + const started = await apiClient.syncGarminData(settings?.historyDays ?? 365); + const s = await refresh(); + // A refused start (Garmin is throttling us) used to be swallowed: the + // button did nothing, no progress bar appeared, and no reason was shown. + if (started.status === 'rate_limited' || s?.status === 'rate_limited') { + setError(started.message || s?.lastError || 'Garmin 正在限流，稍后会自动恢复。'); + return; + } beginSyncPolling(); } catch (err: any) { setError(errorMessage(err, '同步失败')); @@ -265,8 +271,10 @@ function SyncPage() { if (!s) { stopPolling(); return; } if (s.status !== 'syncing') { stopPolling(); - if (s.status === 'error') setError(s.lastError || '同步失败'); - else setMessage(`同步完成，已更新 ${s.recordsSynced} 天数据`); + // Only 'idle' means it finished; anything else reported "同步完成" + // while nothing had been synced. + if (s.status === 'idle') setMessage(`同步完成，已更新 ${s.recordsSynced} 天数据`); + else setError(s.lastError || '同步失败'); } }, POLL_MS); }; @@ -374,7 +382,9 @@ function SyncPage() {
{syncStatus?.stage || '正在同步…'} - {current} / {total} 天 + + {total > 0 ? `${current} / ${total} 天` : `${current} 天`} +
( + const { data } = await this.client.post<{ + status: string; days?: number; message?: string; retryAfterSeconds?: number; + }>( '/garmin/sync', { - ...(days ? { days } : {}), + // `days` must survive being 0 — that is "全部历史", not "unset". + // `days ? …` dropped it, so the backend fell back to its 7-day + // default and a full backfill silently pulled a week. + ...(days === undefined || days === null ? {} : { days }), ...(garminPassword ? { garminPassword } : {}), } ); diff --git a/deploy/push.sh b/deploy/push.sh new file mode 100755 index 0000000..7c72eab --- /dev/null +++ b/deploy/push.sh @@ -0,0 +1,60 @@ +#!/bin/sh +# Push this working tree to the NAS and restart it. +# +# The NAS only accepts password auth, so one ssh master connection is opened +# up front and every later step rides on it: you type the password once, not +# five times. (macOS's bundled rsync 2.6.9 cannot carry a password through +# -e at all — hence tar over ssh.) +# +# ./deploy/push.sh [user@host] [port] +# +# Never touches .env, .venv or the database on the far side. +set -e + +HOST="${1:-ericwyuan@192.168.50.64}" +PORT="${2:-22}" +REPO="$(cd "$(dirname "$0")/.." && pwd)" +CTL="$(mktemp -u /tmp/garmin-deploy-XXXXXX)" + +sh_() { ssh -S "$CTL" -o BatchMode=yes "$HOST" "$@"; } + +cleanup() { ssh -S "$CTL" -O exit "$HOST" 2>/dev/null || true; } +trap cleanup EXIT + +echo "==> connecting to $HOST:$PORT (password prompt follows, once)" +ssh -M -S "$CTL" -fN -p "$PORT" -o ControlPersist=300 "$HOST" + +# The app dir has moved before; find it rather than assume it. +APP=$(sh_ 'for d in ~/apps/garmin-health-lab /volume1/web/garmin-health-lab; do + [ -d "$d/backend" ] && { echo "$d"; break; }; done') +[ -n "$APP" ] || { echo "cannot find the app dir on $HOST" >&2; exit 1; } +echo "==> app dir: $APP" + +# STATIC_DIR is ./static relative to backend/, which is where start.sh cds to. +STATIC="$APP/backend/static" +# `cmd && VAR=x` would trip `set -e` when cmd fails, so spell it out. +if sh_ "[ -f '$APP/static/index.html' ]" 2>/dev/null; then + STATIC="$APP/static" +fi +echo "==> static dir: $STATIC" + +echo "==> backend" +tar czf - --exclude .venv --exclude .env --exclude __pycache__ \ + --exclude '*.db' --exclude tests --exclude .pytest_cache \ + -C "$REPO/backend" . | sh_ "tar xzf - -C '$APP/backend'" + +echo "==> static (cleared first, so stale JS chunks do not pile up)" +if [ ! -f "$REPO/client/build/index.html" ]; then + echo "client/build is missing — run 'npm run build' first" >&2 + exit 1 +fi +sh_ "rm -rf '$STATIC' && mkdir -p '$STATIC'" +tar czf - -C "$REPO/client/build" . | sh_ "tar xzf - -C '$STATIC'" + +echo "==> restart" +sh_ "sh '$APP/deploy/stop.sh' >/dev/null 2>&1; sleep 2; sh '$APP/deploy/start.sh'" + +echo "==> health" +sh_ "curl -sf -m 5 -o /dev/null -w 'local api: %{http_code}\n' \ + http://127.0.0.1:8124/api/health/status" || echo "local api: unreachable" +echo "done. The public URL takes a few seconds longer (frp reconnecting)."