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).
This commit is contained in:
@@ -12,15 +12,18 @@ import wrapper
|
|||||||
def reset_state():
|
def reset_state():
|
||||||
original_state = wrapper._auth_state
|
original_state = wrapper._auth_state
|
||||||
original_client = wrapper._client
|
original_client = wrapper._client
|
||||||
|
original_startup_attempted = wrapper._startup_login_attempted
|
||||||
for q in (wrapper._mfa_input_queue, wrapper._login_result_queue):
|
for q in (wrapper._mfa_input_queue, wrapper._login_result_queue):
|
||||||
while not q.empty():
|
while not q.empty():
|
||||||
try:
|
try:
|
||||||
q.get_nowait()
|
q.get_nowait()
|
||||||
except queue.Empty:
|
except queue.Empty:
|
||||||
break
|
break
|
||||||
|
wrapper._startup_login_attempted = False
|
||||||
yield
|
yield
|
||||||
wrapper._auth_state = original_state
|
wrapper._auth_state = original_state
|
||||||
wrapper._client = original_client
|
wrapper._client = original_client
|
||||||
|
wrapper._startup_login_attempted = original_startup_attempted
|
||||||
|
|
||||||
|
|
||||||
def test_authenticate_success():
|
def test_authenticate_success():
|
||||||
@@ -93,7 +96,15 @@ def test_call_dispatches_to_named_garminconnect_method():
|
|||||||
|
|
||||||
|
|
||||||
def test_call_unauthenticated_is_error():
|
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"
|
wrapper._auth_state = "unauthenticated"
|
||||||
|
with patch.dict("os.environ", {}, clear=False):
|
||||||
|
os.environ.pop("GARMIN_EMAIL", None)
|
||||||
|
os.environ.pop("GARMIN_PASSWORD", None)
|
||||||
resp = wrapper.dispatch({
|
resp = wrapper.dispatch({
|
||||||
"id": 7, "cmd": "call", "params": {"method": "get_activities_by_date", "args": {}},
|
"id": 7, "cmd": "call", "params": {"method": "get_activities_by_date", "args": {}},
|
||||||
})
|
})
|
||||||
@@ -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 resp == {"id": 20, "result": {"status": "success", "message": "Authenticated successfully."}}
|
||||||
assert wrapper._auth_state == "authenticated"
|
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()
|
||||||
|
|||||||
@@ -11,12 +11,17 @@ import traceback
|
|||||||
|
|
||||||
from garminconnect import Garmin
|
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
|
_client = None
|
||||||
_auth_state = "unauthenticated"
|
_auth_state = "unauthenticated"
|
||||||
_mfa_input_queue = queue.Queue()
|
_mfa_input_queue = queue.Queue()
|
||||||
_login_result_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):
|
def _debug(msg):
|
||||||
@@ -31,18 +36,28 @@ def _prompt_mfa():
|
|||||||
|
|
||||||
|
|
||||||
def _startup_login():
|
def _startup_login():
|
||||||
"""Silently resume a cached tokenstore session at process start, so a
|
"""Silently resume a cached tokenstore session, so a freshly
|
||||||
freshly (re)spawned subprocess is already authenticated for background
|
(re)spawned subprocess can already be authenticated for a data 'call'
|
||||||
syncs that never call the explicit authenticate command.
|
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
|
Bounded to the same 10s timeout as _handle_authenticate: the actual
|
||||||
login runs on a background daemon thread, and this function waits up
|
login runs on a background daemon thread, and this function waits up
|
||||||
to 10s for it before returning either way. This runs before main()
|
to 10s for it before returning either way, so a slow/rate-limited
|
||||||
enters its stdin dispatch loop, so a slow/rate-limited Garmin login
|
Garmin login can never block the caller indefinitely -- if it times
|
||||||
must never block indefinitely here -- if it times out, the thread
|
out, the thread keeps running and will update _auth_state whenever it
|
||||||
keeps running and will update _auth_state whenever it eventually
|
eventually finishes (success or failure), just without wedging the
|
||||||
finishes (success or failure), same as today, just without wedging
|
whole subprocess unresponsive in the meantime.
|
||||||
the whole subprocess unresponsive in the meantime.
|
|
||||||
|
|
||||||
Deliberately uses its own private, function-local result queue rather
|
Deliberately uses its own private, function-local result queue rather
|
||||||
than the module-level _login_result_queue that _handle_authenticate and
|
than the module-level _login_result_queue that _handle_authenticate and
|
||||||
@@ -87,7 +102,12 @@ def _startup_login():
|
|||||||
|
|
||||||
|
|
||||||
def _handle_authenticate(_params):
|
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", "")
|
email = os.environ.get("GARMIN_EMAIL", "")
|
||||||
password = os.environ.get("GARMIN_PASSWORD", "")
|
password = os.environ.get("GARMIN_PASSWORD", "")
|
||||||
@@ -159,6 +179,10 @@ def _handle_complete_mfa(params):
|
|||||||
|
|
||||||
|
|
||||||
def _handle_call(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":
|
if _auth_state != "authenticated":
|
||||||
raise RuntimeError("Not authenticated. Call authenticate first.")
|
raise RuntimeError("Not authenticated. Call authenticate first.")
|
||||||
method = params["method"]
|
method = params["method"]
|
||||||
@@ -188,7 +212,6 @@ def dispatch(req):
|
|||||||
|
|
||||||
|
|
||||||
def main():
|
def main():
|
||||||
_startup_login()
|
|
||||||
for line in sys.stdin:
|
for line in sys.stdin:
|
||||||
line = line.strip()
|
line = line.strip()
|
||||||
if not line:
|
if not line:
|
||||||
|
|||||||
Reference in New Issue
Block a user