From 45f29219714eabedad669c5645484af2efb07ec9 Mon Sep 17 00:00:00 2001 From: Christophe Vila Date: Sun, 26 Jul 2026 21:25:53 +0200 Subject: [PATCH] fix(garmin): make wrapper.py's startup tokenstore login lazy _startup_login() used to run unconditionally at process boot, before main()'s stdin dispatch loop started. Whenever the very first real command turned out to be an explicit authenticate, this was actively counterproductive: on success it duplicated a login authenticate was about to redo anyway, and on failure it was a wasted, unauthenticated hit against Garmin's servers moments before the real attempt -- exactly the kind of extra load that worsens rate-limiting risk. It also never handled MFA, so it couldn't stand in for authenticate regardless. Make it lazy instead: only _handle_call falls back to it, at most once per subprocess lifetime, and only if authenticate was never explicitly attempted first. This is what it was actually for -- silently resuming a cached tokenstore session for a data call that never goes through the explicit authenticate command (e.g. "Sync now" reaching an already-connected user's client right after a backend restart cleared the in-memory client cache). --- .../garmin/pyscript/tests/test_wrapper.py | 69 ++++++++++++++++++- backend/internal/garmin/pyscript/wrapper.py | 47 +++++++++---- 2 files changed, 101 insertions(+), 15 deletions(-) diff --git a/backend/internal/garmin/pyscript/tests/test_wrapper.py b/backend/internal/garmin/pyscript/tests/test_wrapper.py index 8c1c32f..6e3fe67 100644 --- a/backend/internal/garmin/pyscript/tests/test_wrapper.py +++ b/backend/internal/garmin/pyscript/tests/test_wrapper.py @@ -12,15 +12,18 @@ import wrapper def reset_state(): original_state = wrapper._auth_state original_client = wrapper._client + original_startup_attempted = wrapper._startup_login_attempted for q in (wrapper._mfa_input_queue, wrapper._login_result_queue): while not q.empty(): try: q.get_nowait() except queue.Empty: break + wrapper._startup_login_attempted = False yield wrapper._auth_state = original_state wrapper._client = original_client + wrapper._startup_login_attempted = original_startup_attempted def test_authenticate_success(): @@ -93,10 +96,18 @@ def test_call_dispatches_to_named_garminconnect_method(): def test_call_unauthenticated_is_error(): + # Explicitly absent GARMIN_EMAIL/PASSWORD: this now indirectly triggers + # the lazy _startup_login fallback (see _handle_call), which must + # return immediately with nothing to resume, leaving this the same + # "Not authenticated" error as before -- not dependent on whatever + # happens to be in the ambient shell environment. wrapper._auth_state = "unauthenticated" - resp = wrapper.dispatch({ - "id": 7, "cmd": "call", "params": {"method": "get_activities_by_date", "args": {}}, - }) + with patch.dict("os.environ", {}, clear=False): + os.environ.pop("GARMIN_EMAIL", None) + os.environ.pop("GARMIN_PASSWORD", None) + resp = wrapper.dispatch({ + "id": 7, "cmd": "call", "params": {"method": "get_activities_by_date", "args": {}}, + }) assert "error" in resp assert "Not authenticated" in resp["error"] @@ -206,3 +217,55 @@ def test_startup_login_does_not_leak_into_shared_authenticate_queue(): assert resp == {"id": 20, "result": {"status": "success", "message": "Authenticated successfully."}} assert wrapper._auth_state == "authenticated" + + +def test_call_triggers_startup_login_lazily_on_first_call(): + """A data 'call' with no prior explicit authenticate on this subprocess + -- e.g. "Sync now" reaching an already-connected user's client right + after a backend restart -- gets exactly one lazy attempt to silently + resume a cached tokenstore session before giving up.""" + wrapper._auth_state = "unauthenticated" + wrapper._client = MagicMock() + wrapper._client.get_activities_by_date.return_value = [] + + def fake_startup_login(): + wrapper._auth_state = "authenticated" + + with patch("wrapper._startup_login", side_effect=fake_startup_login) as mock_startup: + resp = wrapper.dispatch({ + "id": 30, "cmd": "call", "params": {"method": "get_activities_by_date", "args": {}}, + }) + + mock_startup.assert_called_once() + assert resp == {"id": 30, "result": []} + + +def test_call_does_not_retry_startup_login_after_first_attempt(): + """If the lazy startup-login attempt doesn't actually authenticate, a + second 'call' must not try it again -- one attempt per subprocess + lifetime, not once per call.""" + wrapper._auth_state = "unauthenticated" + + with patch("wrapper._startup_login") as mock_startup: # no-op: leaves state unauthenticated + wrapper.dispatch({"id": 31, "cmd": "call", "params": {"method": "x", "args": {}}}) + wrapper.dispatch({"id": 32, "cmd": "call", "params": {"method": "x", "args": {}}}) + + mock_startup.assert_called_once() + + +def test_authenticate_prevents_later_lazy_startup_login(): + """Once authenticate has been explicitly called (regardless of + outcome), a later 'call' must never fall back to _startup_login -- that + would either duplicate a login authenticate already did, or waste an + extra hit against Garmin after a failure.""" + env = {"GARMIN_EMAIL": "test@example.com", "GARMIN_PASSWORD": "secret"} + with patch.dict("os.environ", env): + with patch("wrapper.Garmin") as mock_garmin_cls: + mock_garmin_cls.return_value = MagicMock() + wrapper.dispatch({"id": 33, "cmd": "authenticate"}) + + wrapper._auth_state = "unauthenticated" # simulate a later, separate call finding no session + with patch("wrapper._startup_login") as mock_startup: + wrapper.dispatch({"id": 34, "cmd": "call", "params": {"method": "x", "args": {}}}) + + mock_startup.assert_not_called() diff --git a/backend/internal/garmin/pyscript/wrapper.py b/backend/internal/garmin/pyscript/wrapper.py index 533dab4..fc0e823 100644 --- a/backend/internal/garmin/pyscript/wrapper.py +++ b/backend/internal/garmin/pyscript/wrapper.py @@ -11,12 +11,17 @@ import traceback from garminconnect import Garmin -TOKENSTORE = os.path.expanduser(os.environ.get("GARMIN_TOKENSTORE", "~/.garth")) +TOKENSTORE = os.path.expanduser(os.environ.get("GARMIN_TOKENSTORE", "~/.garmin")) _client = None _auth_state = "unauthenticated" _mfa_input_queue = queue.Queue() _login_result_queue = queue.Queue() +# Set the first time this subprocess attempts any login, whichever path +# gets there first (see _handle_call's lazy call and _handle_authenticate) -- +# guarantees _startup_login runs at most once per subprocess lifetime, and +# never at all once an explicit authenticate has been attempted. +_startup_login_attempted = False def _debug(msg): @@ -31,18 +36,28 @@ def _prompt_mfa(): def _startup_login(): - """Silently resume a cached tokenstore session at process start, so a - freshly (re)spawned subprocess is already authenticated for background - syncs that never call the explicit authenticate command. + """Silently resume a cached tokenstore session, so a freshly + (re)spawned subprocess can already be authenticated for a data 'call' + that never goes through the explicit authenticate command -- e.g. + "Sync now" reaching an already-connected user's client right after a + backend restart cleared the in-memory cache. + + Called lazily (see _handle_call), at most once per subprocess lifetime, + and only if nothing has explicitly called authenticate first. Running + it unconditionally at process start used to be actively counterproductive + whenever the very first command actually was authenticate: on success it + just duplicated a login _handle_authenticate was about to redo anyway + (it always rebuilds _client from scratch), and on failure it was a + wasted, unauthenticated hit against Garmin's servers moments before the + real attempt -- extra load that only makes rate-limiting worse. Bounded to the same 10s timeout as _handle_authenticate: the actual login runs on a background daemon thread, and this function waits up - to 10s for it before returning either way. This runs before main() - enters its stdin dispatch loop, so a slow/rate-limited Garmin login - must never block indefinitely here -- if it times out, the thread - keeps running and will update _auth_state whenever it eventually - finishes (success or failure), same as today, just without wedging - the whole subprocess unresponsive in the meantime. + to 10s for it before returning either way, so a slow/rate-limited + Garmin login can never block the caller indefinitely -- if it times + out, the thread keeps running and will update _auth_state whenever it + eventually finishes (success or failure), just without wedging the + whole subprocess unresponsive in the meantime. Deliberately uses its own private, function-local result queue rather than the module-level _login_result_queue that _handle_authenticate and @@ -87,7 +102,12 @@ def _startup_login(): def _handle_authenticate(_params): - global _client, _auth_state + global _client, _auth_state, _startup_login_attempted + # An explicit authenticate is happening (successful or not) -- the lazy + # startup-login fallback in _handle_call must never fire after this, it + # would be redundant at best and a wasted extra hit against Garmin at + # worst. + _startup_login_attempted = True email = os.environ.get("GARMIN_EMAIL", "") password = os.environ.get("GARMIN_PASSWORD", "") @@ -159,6 +179,10 @@ def _handle_complete_mfa(params): def _handle_call(params): + global _startup_login_attempted + if _auth_state != "authenticated" and not _startup_login_attempted: + _startup_login_attempted = True + _startup_login() if _auth_state != "authenticated": raise RuntimeError("Not authenticated. Call authenticate first.") method = params["method"] @@ -188,7 +212,6 @@ def dispatch(req): def main(): - _startup_login() for line in sys.stdin: line = line.strip() if not line: