fix(dashboard-auth): coalesce concurrent refresh requests to prevent RT reuse detection - #55717
Conversation
…RT reuse detection When the browser discovers an expired access-token cookie it can fire a burst of parallel fetch() calls (session, profile, status, ws-ticket …). Each carries the same old hermes_session_rt cookie. The first request rotates the RT and gets new cookies via Set-Cookie, but sibling in-flight requests still carry the old RT. Without coalescing, each of those replays the now-stale RT and the provider's reuse-detection revokes the whole session — kicking the remote UI back to login. Fix: add an in-process replay cache (_refresh_cache) that maps sha256(old_refresh_token) to (new_session, provider_name, timestamp). An asyncio lock serializes concurrent misses so at most one request per old-RT ever reaches the provider. Cache entries expire after 120 seconds. Regression test uses a ReuseDetectingProvider that raises RefreshExpiredError on the second call with the same RT (simulating real rotating-RT provider reuse detection).
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the rotating-refresh-token path. The current-main premise remains: hermes_cli/dashboard_auth/middleware.py:352-388 refreshes each failed verification independently, while the Nous provider rotates RTs (plugins/dashboard_auth/nous/__init__.py:253-260).
Problems
- Blocking:
hermes_cli/dashboard_auth/middleware.py:493calls the synchronous provider refresh while holding the async lock. The Nous provider executes synchronoushttpx.postatplugins/dashboard_auth/nous/__init__.py:269, so this blocks the ASGI event loop. This matches the #55712 follow-up, which specifically calls for threadpool execution. - Blocking:
tests/hermes_cli/test_dashboard_auth_middleware.py:671-691capturesrt_valbut never uses it.r4runs afterr3in the sameTestClient, so this is neither concurrent nor an independently preserved stale-cookie request. hermes_cli/dashboard_auth/middleware.py:519stores every successful result in a process-global dict, but expired entries are never removed.
Suggested changes
- Use per-token single-flight plus
run_in_threadpoolfor synchronous provider refreshes. - Test two isolated clients carrying the captured old RT concurrently, with a provider barrier/delay; also verify an unrelated endpoint remains responsive.
- Add bounded cache eviction and revisit the replay grace window.
Automated hermes-sweeper review.
|
|
||
| for provider in list_session_providers(): | ||
| try: | ||
| new_session = provider.refresh_session(refresh_token=refresh_token) |
There was a problem hiding this comment.
refresh_session is synchronous; the bundled Nous provider reaches synchronous httpx.post (plugins/dashboard_auth/nous/__init__.py:269). Calling it here blocks the ASGI event loop while this global lock is held, matching the #55712 recurrence report. Run it through starlette.concurrency.run_in_threadpool and coordinate only requests for this RT.
| if new_session is not None: | ||
| # Cache the rotated session so concurrent siblings don't | ||
| # replay the old RT and trip reuse detection. | ||
| _refresh_cache[rt_hash] = ( |
There was a problem hiding this comment.
Entries only become logically expired during lookup; none are removed from _refresh_cache. A long-running dashboard retains one Session and RT hash for every successful refresh until restart. Evict expired entries during insertion/access and bound the cache.
| # middleware will try to refresh again. The replay cache must serve | ||
| # the previously-rotated session without calling the provider a second | ||
| # time (which would trip reuse detection). | ||
| r4 = client.get("/api/auth/me") |
There was a problem hiding this comment.
This request is sequential and uses the same TestClient after r3 has processed Set-Cookie; the captured rt_val is never restored or supplied here. Use isolated clients/cookie snapshots and launch both requests concurrently against a delayed/barrier provider so the test exercises the stale-RT race.
What does this PR do?
Fixes a race condition in the dashboard auth middleware where concurrent requests after access-token expiry can trip refresh-token reuse detection, killing the remote dashboard session.
When the browser discovers an expired access-token cookie it fires a burst of parallel fetch() calls (session, profile, status, ws-ticket, etc.). Each carries the same old
hermes_session_rtcookie. The first request rotates the RT and returns new cookies viaSet-Cookie, but sibling in-flight requests still carry the old RT. Without coalescing, each replay triggers the provider's reuse-detection →RefreshExpiredError→ session revoked → user kicked back to login.Related Issue
Fixes #55712
Type of Change
Changes Made
hermes_cli/dashboard_auth/middleware.py— Add in-process replay cache (_refresh_cache) mappingsha256(old_refresh_token)to(new_session, provider_name, timestamp). Anasyncio.Lockserializes concurrent refresh misses so at most one request per old-RT reaches the provider. Cache entries expire after 120 seconds._attempt_refreshis now async to support the lock.tests/hermes_cli/test_dashboard_auth_middleware.py— AddReuseDetectingProvider(raisesRefreshExpiredErroron second call with same RT) andtest_refresh_token_replay_cache_prevents_reuse_detectionregression test proving concurrent requests don't trip reuse detection.How to Test
pytest tests/hermes_cli/test_dashboard_auth_middleware.py -x -q— all 34 tests should pass (33 existing + 1 new regression test).test_refresh_token_replay_cache_prevents_reuse_detectionspecifically validates that two sequential requests with an expired AT and the same RT both succeed, and the provider'srefresh_sessionis called exactly once (the second request hits the cache).Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
Log evidence from the issue reporter showing the problem:
{"event":"login_success","provider":"nous","ip":"192.168.64.117"} {"event":"ws_ticket_minted","provider":"nous","ip":"192.168.64.117"} ... {"event":"refresh_failure","provider":"nous","reason":"refresh_expired","ip":"192.168.64.117"} {"event":"session_verify_failure","reason":"no_provider_recognises","ip":"192.168.64.117"}The burst pattern (login_success → immediate refresh_failure) is the signature of concurrent requests replaying the same stale RT.