fix(mcp): reload OAuth provider on first sight when no tokens in memory (#39551)
Seeding the disk-watch baseline on first observation also swallowed the case where the process started before login: file absent, then an external `hermes mcp login` writes it, and the provider never reloaded. Only skip the reload when the provider already holds tokens in memory.
This commit is contained in:
@@ -136,6 +136,8 @@ async def test_first_observation_401_still_allows_in_place_refresh(tmp_path, mon
|
||||
(token_dir / "srv.json").write_text(json.dumps({"access_token": "OLD"}), encoding="utf-8")
|
||||
|
||||
class _Ctx:
|
||||
current_tokens = object() # tokens already in memory (in-process sign-in)
|
||||
|
||||
def can_refresh_token(self):
|
||||
return True
|
||||
|
||||
@@ -154,6 +156,33 @@ async def test_first_observation_401_still_allows_in_place_refresh(tmp_path, mon
|
||||
assert mgr._entries[mgr._key("srv")].last_mtime_ns != 0
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_external_login_after_absent_tokens_file_forces_reload(tmp_path, monkeypatch):
|
||||
"""Provider built before login (no file, no in-memory tokens) must reload
|
||||
once another process writes the tokens file (GH#39551 review)."""
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
from tools.mcp_oauth_manager import MCPOAuthManager, _ProviderEntry
|
||||
|
||||
class _Ctx:
|
||||
current_tokens = None
|
||||
|
||||
class _Provider:
|
||||
context = _Ctx()
|
||||
_initialized = True
|
||||
|
||||
mgr = MCPOAuthManager()
|
||||
provider = _Provider()
|
||||
mgr._entries[mgr._key("srv")] = _ProviderEntry(
|
||||
server_url="https://example.com/mcp", oauth_config=None, provider=provider,
|
||||
)
|
||||
assert await mgr.invalidate_if_disk_changed("srv") is False # file absent
|
||||
token_dir = tmp_path / "mcp-tokens"
|
||||
token_dir.mkdir(parents=True)
|
||||
(token_dir / "srv.json").write_text(json.dumps({"access_token": "NEW"}), encoding="utf-8")
|
||||
assert await mgr.invalidate_if_disk_changed("srv") is True
|
||||
assert provider._initialized is False
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_handle_401_dedup_survives_even_if_task_reference_dropped(tmp_path, monkeypatch):
|
||||
"""Concurrent 401s share one handler task and all callers resolve.
|
||||
|
||||
@@ -355,10 +355,12 @@ class MCPOAuthManager:
|
||||
if mtime_ns == entry.last_mtime_ns:
|
||||
return False
|
||||
old, entry.last_mtime_ns = entry.last_mtime_ns, mtime_ns
|
||||
if old == 0:
|
||||
# First observation only seeds the baseline. Forcing a
|
||||
# reload here can reset the provider in the middle of its
|
||||
# first auth handshake and tear down HTTP MCP discovery.
|
||||
if old == 0 and getattr(getattr(entry.provider, "context", None), "current_tokens", None) is not None:
|
||||
# First observation with tokens already in memory only seeds the
|
||||
# baseline: the file was written by this process's own first
|
||||
# sign-in, and reloading on the next request would tear down the
|
||||
# live HTTP MCP session. With no tokens in memory (started before
|
||||
# an external `hermes mcp login`), fall through and reload.
|
||||
return False
|
||||
# `_initialized` is private SDK API but stable across the pinned versions (>=1.26.0).
|
||||
if hasattr(entry.provider, "_initialized"):
|
||||
|
||||
Reference in New Issue
Block a user