fix(mcp): clamp expired-token expires_in to -1 and add proactive refresh timer - #62309
diegokolling wants to merge 1 commit into
Conversation
|
Thanks for the focused MCP OAuth work. Current main already has a cold-load expiry path: Problems
Suggested changes
Automated hermes-sweeper review. |
Addresses teknium1 review on PR NousResearch#62309: 1. Timer lifecycle: cancel before schedule/re-init/remove so orphaned callbacks cannot race refresh_token rotation after provider eviction or disk-invalidation reinitialize. 2. Retry only on transient failures (timeouts, network errors, 408/425/429/5xx) and only while can_refresh_token() still holds. Permanent 4xx / invalid payload stop without calling the SDK non-200 path that clears current_tokens. 3. Deterministic tests for duplicate schedule cancel, re-init cancel, manager eviction, success chaining, permanent vs transient retry, and expires_in=-1 clamp (SDK is_token_valid treats 0 as valid).
86ca786 to
8d2032c
Compare
|
Thanks for the review — all three points are addressed in the force-pushed tip ( 1. Timer cancellation / lifecycle
2. Retry only for transient failuresMCP 1.26.0 clears New policy in
3. Deterministic testsNew file
Also updated VerificationLocal: —Diego Kolling (@diegokolling) + Hermes Agent (team) |
Summary
MCP OAuth servers (Notion, Todoist, etc.) force a full browser re-authentication on every Hermes restart. Two independent latent bugs combine to cause this:
Both are fixed without new environment variables, without new model tools, and without touching the core tool schema. Fix B reuses the SDK's existing
_refresh_token()/_handle_refresh_response()internal methods.Root Cause Analysis
Fix A — Dead-token misdetection (
tools/mcp_oauth.py)HermesTokenStorage.get_tokens()clamps the recomputedexpires_into a minimum of0usingint(max(..., 0)). For an expired token this producesexpires_in = 0. The SDK storestoken_expiry_time = time.time() + 0, makingis_token_valid()evaluatetime.time() <= time.time()— True due to float granularity. The SDK ships the stale token, gets a 401, and falls through to browser OAuth instead of silent refresh.Fix: clamp to
-1instead of0—calculate_token_expiry(-1)yieldstime.time() - 1, makingis_token_valid()return False, routing to therefresh_tokengrant.Fix B — No proactive refresh (
tools/mcp_oauth_manager.py)The SDK only refreshes reactively (on 401). If the token expires 5 min after startup and the next call is background discovery, the SDK opens a browser in a non-interactive context. Fix: schedule a
loop.call_latertimer that silently refreshes 5 min before expiry (capped at 55 min), chains on success, retries in 60s on failure. Reuses the SDK's existing_refresh_token()+_handle_refresh_response()— no hand-rolled HTTP.Reproduction Steps
expires_atin~/.hermes/mcp-tokens/<server>.jsonto a past timestamp.Production evidence:
Test Plan
Fix A:
test_get_tokens_expired_clamped_to_neg1,test_get_tokens_expired_implied_expiry,test_get_tokens_valid_token_unchangedFix B:
test_proactive_refresh_scheduled_on_init,test_proactive_refresh_skip_if_no_refresh_token,test_proactive_refresh_skip_if_no_expiry,test_proactive_refresh_delay_capped_at_55min,test_proactive_refresh_retries_on_failure,test_proactive_refresh_chains_on_successAll follow existing patterns:
tmp_path + monkeypatch,MagicMock, no network I/O.Relationship to existing issues
Attributed to: Diego Kolling + Hermes