diff --git a/backend/internal/garmin/pyscript/tests/test_wrapper.py b/backend/internal/garmin/pyscript/tests/test_wrapper.py index cb1ab51..8c1c32f 100644 --- a/backend/internal/garmin/pyscript/tests/test_wrapper.py +++ b/backend/internal/garmin/pyscript/tests/test_wrapper.py @@ -1,5 +1,6 @@ import os import queue +import threading from unittest.mock import MagicMock, patch import pytest @@ -146,13 +147,62 @@ def test_startup_login_missing_credentials_returns_immediately(): def test_startup_login_times_out_without_blocking(): + # _startup_login now waits on its own private, function-local queue + # (never the shared wrapper._login_result_queue -- see the + # no-shared-queue-leakage test below), so forcing its timeout path + # means intercepting the queue.Queue() constructor it calls internally + # rather than patching a module-level queue object. 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() - with patch.object(wrapper._login_result_queue, "get", side_effect=queue.Empty): + with patch("wrapper.queue.Queue") as mock_queue_cls: + mock_queue_cls.return_value.get.side_effect = queue.Empty # Should return promptly (bounded by the 10s timeout passed to # queue.get, which is mocked here to raise immediately) rather # than blocking main()'s stdin dispatch loop from starting. wrapper._startup_login() assert wrapper._auth_state == "unauthenticated" + + +def test_startup_login_does_not_leak_into_shared_authenticate_queue(): + """Regression test: _startup_login used to push its background thread's + result onto the same module-level _login_result_queue that + _handle_authenticate/_handle_complete_mfa share for the MFA handoff. A + startup login that was still in flight when a user's first explicit + 'authenticate' call came in could have that call accidentally dequeue + the startup thread's stale result instead of its own fresh one. + _startup_login now uses its own private queue, so a still-in-flight + startup attempt can never interfere with a later authenticate() call.""" + release = threading.Event() + + env = {"GARMIN_EMAIL": "test@example.com", "GARMIN_PASSWORD": "secret"} + with patch.dict("os.environ", env): + with patch("wrapper.Garmin") as mock_garmin_cls: + # A startup login whose underlying garminconnect call is still + # blocked (simulating "slow to resolve") when _startup_login's + # own bounded wait (mocked to time out immediately, so this test + # doesn't need to sleep the real 10s) returns. + slow_client = MagicMock() + slow_client.login.side_effect = lambda **kwargs: release.wait(5) + mock_garmin_cls.return_value = slow_client + with patch("wrapper.queue.Queue") as mock_queue_cls: + mock_queue_cls.return_value.get.side_effect = queue.Empty + wrapper._startup_login() + + assert wrapper._auth_state == "unauthenticated" + # Nothing from the still-in-flight startup attempt ever touches the + # shared queue that authenticate()/complete_mfa() rely on. + assert wrapper._login_result_queue.empty() + + release.set() # let the stale startup thread finish harmlessly in the background + + # A fresh, unrelated authenticate() call must get its own result, not + # anything left over from the startup attempt above. + with patch.dict("os.environ", env): + with patch("wrapper.Garmin") as mock_garmin_cls2: + mock_garmin_cls2.return_value = MagicMock() + resp = wrapper.dispatch({"id": 20, "cmd": "authenticate"}) + + assert resp == {"id": 20, "result": {"status": "success", "message": "Authenticated successfully."}} + assert wrapper._auth_state == "authenticated" diff --git a/backend/internal/garmin/pyscript/wrapper.py b/backend/internal/garmin/pyscript/wrapper.py index 6d786c1..533dab4 100644 --- a/backend/internal/garmin/pyscript/wrapper.py +++ b/backend/internal/garmin/pyscript/wrapper.py @@ -42,7 +42,17 @@ def _startup_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.""" + 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 + _handle_complete_mfa share -- those two are legitimately two halves of + one explicit, MFA-capable login flow and must share a queue for the MFA + handoff to work, but this is a plain background tokenstore resume with + no MFA involved. Sharing the queue here would let this thread's stale + result (arriving after the 10s timeout below) be dequeued by an + unrelated, later authenticate/complete_mfa call instead of that call's + own fresh result.""" global _client, _auth_state email = os.environ.get("GARMIN_EMAIL") password = os.environ.get("GARMIN_PASSWORD") @@ -50,18 +60,19 @@ def _startup_login(): return _client = Garmin(email, password) + result_queue = queue.Queue() # private to this call, never shared with authenticate/complete_mfa def _do_login(): try: _client.login(tokenstore=TOKENSTORE) - _login_result_queue.put(("success", None)) + result_queue.put(("success", None)) except Exception as exc: - _login_result_queue.put(("error", str(exc))) + result_queue.put(("error", str(exc))) threading.Thread(target=_do_login, daemon=True).start() try: - status, err = _login_result_queue.get(timeout=10) + status, err = result_queue.get(timeout=10) if status == "success": _auth_state = "authenticated" else: