fix(garmin): give startup login its own result queue, not the shared one
_startup_login's background thread and _handle_authenticate's shared the module-level _login_result_queue with no correlation. Since _startup_login can now time out at 10s while its thread keeps running (from the previous fix in this wave), a slow-cold-starting subprocess's first explicit "Connect to Garmin" call could accidentally dequeue the startup thread's stale result instead of its own fresh one, orphaning the loser's result to corrupt a later authenticate/complete_mfa call. _handle_authenticate and _handle_complete_mfa still correctly share _login_result_queue -- they're two halves of one explicit, MFA-capable login flow. _startup_login is a background tokenstore resume with no MFA involved, so it now uses its own private, function-local queue.Queue() instead, making cross-contamination structurally impossible. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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"
|
||||
|
||||
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user