fix(dashboard-auth): run every blocking IDP provider call off the dashboard event loop - #84891
Open
briandevans wants to merge 7 commits into
Open
briandevans wants to merge 7 commits into
briandevans wants to merge 7 commits into
Conversation
…event loop `gated_auth_middleware` is an `async def` running on the single uvicorn event loop, but `_verify_bearer` is synchronous and walks every registered session provider calling `verify_session`. The shipped providers verify a JWT against a `PyJWKClient` built with `lifespan=300` and no `timeout=` (`plugins/dashboard_auth/nous/__init__.py:419`, `plugins/dashboard_auth/self_hosted/__init__.py:600`), so it inherits PyJWKClient's 30s default and `fetch_data` does a blocking `urllib.request.urlopen`. Every 300 seconds the next bearer request to traverse the gate performs that fetch inline on the loop, and a slow or blackholed IDP stalls the entire dashboard — every other request, the live feed and the terminal stream — for up to 30 seconds. Offload at the helper rather than at the inner `verify_session` call so the whole provider loop, including its per-provider retries, moves to a worker in one `await` instead of ping-ponging the loop once per provider. The helper body is unchanged. `asyncio.to_thread` copies the caller's context and re-raises the worker's exception in the awaiting frame, so the `ProviderError` arm and the 503 it returns are untouched; this is a pure scheduling change. `hermes_cli/web_server.py`, the module that installs this middleware, already applies the same rule 51 times (rationale at `web_server.py:2373`); `hermes_cli/dashboard_auth/` had zero uses.
… loop The cookie branch of `gated_auth_middleware` runs its own verify loop inline, calling the synchronous `provider.verify_session` for each registered session provider directly on the event loop. This is the hot path: it executes on every gated request that presents an access-token cookie. In steady state the JWKS cache is warm and the verify is a local signature check, so most requests are cheap. But the cache expires every 300 seconds (`_JWKS_CACHE_SECONDS`), and the request unlucky enough to land after expiry pays a blocking `urlopen` to the IDP's JWKS endpoint, bounded only by PyJWKClient's 30s default. Stacked providers multiply this: each provider that cannot be reached raises only after its own timeout, and the loop tries the next one, so the gate can hold the event loop for a multiple of that ceiling while deciding a single request. Offload the individual `verify_session` call rather than the whole loop: the loop body between calls audits and logs against the request, and keeping that on the loop leaves the surrounding control flow — the `ProviderError` arm, the `unreachable_provider` bookkeeping and the 503 — byte-for-byte identical.
`_attempt_refresh` is the last synchronous provider call left in `gated_auth_middleware`, and it is the worst of the three. Unlike verify, refresh is never served from a cache: every call is a token-endpoint round trip at `_TOKEN_ENDPOINT_TIMEOUT_SEC = 10.0` (`plugins/dashboard_auth/nous/__init__.py:135`, `plugins/dashboard_auth/self_hosted/__init__.py:118`), and the helper walks every registered session provider until one accepts the refresh token. The reachability is routine rather than exceptional. The access-token cookie's Max-Age tracks the token's own lifetime, so the browser evicts it the moment the token lapses while the refresh-token cookie lives for 30 days. A dashboard with several tabs open therefore fires a burst of parallel requests that each carry only the RT — and on the event loop those refreshes serialise, so the freeze is the burst size times the round trip, not one round trip. Offload at the helper, not at the inner `refresh_session`: both the helper and its single caller sit in `gated_auth_middleware`, so one `await` moves the entire provider walk to a worker instead of returning to the loop between providers. The helper body is unchanged, and the audit trail is unaffected — `audit_log` is synchronous and appends under its own lock, and `_client_ip` only reads already-parsed headers, so neither needs the loop.
…e event loop `auth_login`, `auth_native_authorize` and `auth_callback` are all `async def` handlers that call synchronous provider methods directly. Unlike the gate's verify path, none of these has a cache in front of it — `start_login` may perform OIDC discovery and `complete_login` is unconditionally a token-endpoint round trip at `_TOKEN_ENDPOINT_TIMEOUT_SEC = 10.0`, with the self-hosted provider also fetching the discovery document (`plugins/dashboard_auth/self_hosted` `__init__.py:524`). Every one of these is a guaranteed network call on the event loop whenever the route runs. The blast radius is the whole dashboard, not the person signing in: while a login round trip is in flight the loop cannot serve any other request, so one user's slow IDP hop stalls every already-authenticated session on the instance. `_redirect_uri(request)` stays on the loop and its result is passed in as an argument — it is a pure header read, so there is nothing to gain from moving it, and evaluating it eagerly keeps the argument list identical to what the provider saw before.
…fresh off the event loop The last three synchronous provider calls in the router, all reached from `async def` handlers. `complete_password_login` verifies credentials against the provider's backing store; the route already rate-limits it per IP, which is only meaningful if the handler can keep serving requests while one credential check is outstanding. On the event loop the rate limiter and the freeze work against each other — a burst of login attempts blocks the dashboard for the duration of the attempts it is trying to throttle. `auth_logout` is the clearest case: the revoke is explicitly documented as best-effort, and failures are logged and never raised. Logging out is not supposed to be able to hurt anyone, but on the event loop an unreachable IDP makes an ignorable revoke stall the whole instance for the full timeout, once per registered provider, before the user is redirected. `auth_native_refresh` is the desktop's rotation endpoint and mirrors the middleware's `_attempt_refresh` provider stacking, with the same unconditional token-endpoint round trip per provider. Each `to_thread` wraps only the provider call itself; the surrounding `try`/`except` arms, audit calls and control flow are unchanged, and the worker's exception is re-raised in the awaiting frame so `InvalidCredentialsError`, `NotImplementedError`, `RefreshExpiredError` and `ProviderError` still land where they did.
…ker thread Nine production call sites invoke the synchronous DashboardAuthProvider protocol from `async def` handlers. This locks all nine to a worker thread so a future edit cannot quietly put one back on the event loop. The assertion is structural rather than timing-based: a recording provider calls `asyncio.get_running_loop()` at entry. A RuntimeError means it is on a worker thread (correct); a live loop means the dashboard's loop is being blocked. Each test reverts red with only its own production hunk undone. One timing test is included for the symptom itself — a provider that blocks for two seconds must not delay a concurrent request to a public, provider-free route. It is written around the trap that makes this class of test worthless: `TestClient(app)` outside a `with` block builds a fresh anyio portal, and therefore a fresh event loop, for every request, so two requests can never contend and the test passes even against unpatched code. Every client here is entered as a context manager, pinning one loop for the whole test exactly as uvicorn serves the dashboard. `test_provider_errors_still_surface_unchanged` is a behaviour-preservation guard, not regression coverage: it passes both before and after, and exists so a later refactor cannot swap the offload for something that drops the provider's exception instead of re-raising it in the awaiting frame. New file rather than an append to an existing `test_dashboard_auth_*` module, because it registers its own recording providers and needs its own gate fixture.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes dashboard lockups caused by synchronous DashboardAuthProvider implementations performing blocking network I/O when invoked directly from async def FastAPI handlers. It routes all dashboard-auth provider calls that can block (verify/refresh/login/logout flows) off the uvicorn event loop using asyncio.to_thread, preserving existing exception and control-flow behavior.
Changes:
- Offload provider calls in
dashboard_authroutes (start_login,complete_login,complete_password_login,refresh_session,revoke_session) viaawait asyncio.to_thread(...). - Offload provider calls in the auth middleware (
verify_sessionand the refresh chain) viaasyncio.to_thread, with docstrings explicitly documenting the blocking contract for sync helpers. - Add a dedicated regression test module that asserts each provider call site runs off-loop (plus a concurrency symptom test to ensure public routes aren’t stalled by a slow provider).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
hermes_cli/dashboard_auth/routes.py |
Offloads provider login/refresh/logout calls from async route handlers via asyncio.to_thread. |
hermes_cli/dashboard_auth/middleware.py |
Offloads bearer verify, cookie verify loop, and refresh chain from the async middleware; documents blocking sync helpers. |
tests/hermes_cli/test_dashboard_auth_off_loop.py |
Adds off-loop contract tests per call site and a concurrency regression to catch event-loop stalls. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
alt-glitch
added
type/perf
Contributor
fix(dashboard-auth): run every blocking IDP provider call off the dashboard event loop
|
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Symptom: on a non-loopback bind, the dashboard periodically locks up for
seconds at a time — pages stop loading, the live feed stalls, the terminal
stream freezes — and then everything arrives at once.
Cause: the
DashboardAuthProviderprotocol is entirely synchronous(
hermes_cli/dashboard_auth/base.py:188-209— every method is a plaindef),and the shipped providers implement it with blocking network I/O. All nine
call sites in
hermes_cli/dashboard_auth/invoke those methods directly fromasync defhandlers, so each one blocks the single uvicorn event loop forits full duration. This PR dispatches all nine through
asyncio.to_thread.Three provable ways this bites, in descending order of nastiness:
(a) A blocking JWKS re-fetch every 300 seconds, bounded at 30.
verify_session→_verify_jwt→PyJWKClient.get_signing_key_from_jwt. Theclient is constructed at
plugins/dashboard_auth/nous/__init__.py:419andplugins/dashboard_auth/self_hosted/__init__.py:600withlifespan=_JWKS_CACHE_SECONDS(=300, atnous:132/self_hosted:127) andno
timeout=, so it inherits PyJWKClient's defaulttimeout = 30. Itsfetch_dataisurllib.request.urlopen(r, timeout=self.timeout, ...)— ablocking synchronous socket call. So every 300 seconds the next request to
traverse the auth gate performs a 30s-bounded blocking fetch to the IDP's JWKS
endpoint on the event loop, and a slow or blackholed IDP freezes the whole
dashboard for up to half a minute. (In steady state, between those re-fetches,
verify_sessionis a local signature check against a warm cache — this is nota network round trip on every request.)
(b) A refresh burst at 10s per call, serialised.
refresh_sessionis nevercached; every call is a token-endpoint round trip at
_TOKEN_ENDPOINT_TIMEOUT_SEC = 10.0(nous:135,self_hosted:118). Theaccess-token cookie's
Max-Agetracks the token's own lifetime(
cookies.py:194passes the provider-suppliedaccess_token_expires_in) whilethe refresh-token cookie lives 30 days, so on expiry the browser evicts the AT
and a multi-tab dashboard fires a burst of parallel requests carrying only the
RT. On the event loop those refreshes serialise: the freeze is burst-size ×
round-trip, not one round trip.
(c) Login, callback, password login, logout revoke and OIDC discovery are
unconditionally blocking network calls whenever they run, with no cache in
front.
auth_logoutis the sharpest of these: the revoke is explicitlydocumented as best-effort with failures swallowed, yet on the loop an
unreachable IDP turns an ignorable call into a dashboard-wide stall for the
full timeout, once per registered provider.
Reachability.
hermes_cli/web_server.py:17957setsapp.state.auth_required = should_require_auth(host), true for anynon-loopback bind, and
web_server.py:659registers_dashboard_auth_gateas@app.middleware("http")— so the gate is on the path of every request to apublicly-bound dashboard.
In-file contrast.
hermes_cli/web_server.py— the same module thatinstalls this middleware — already applies this rule 51 times, with the
rationale spelled out at
web_server.py:2373("keep both off the eventloop").
hermes_cli/dashboard_auth/used it zero times.This is a pure scheduling change.
asyncio.to_threadcopies the caller'scontext and re-raises the worker's exception in the awaiting frame, so every
try/exceptarm, exception type, audit call and control-flow branch isunchanged —
ProviderError,RefreshExpiredError,InvalidCodeError,InvalidCredentialsErrorandNotImplementedErrorall still land exactlywhere they did.
Related Issue
No linked issue — this is an origination, found by sweeping the synchronous
provider protocol against its
async defcall sites.Type of Change
Changes Made
Sibling-site sweep — complete and closed
returns exactly 9 sites in 2 files, and nothing anywhere else in the
repository outside
plugins/(the provider implementations) andtests/.mainmiddleware.py:304provider.verify_session_verify_bearer(:290):359middleware.py:424provider.verify_sessionasync gated_auth_middlewareto_threadmiddleware.py:564provider.refresh_session_attempt_refresh(:547):457routes.py:205p.start_loginasync auth_loginroutes.py:370p.start_loginasync auth_native_authorizeroutes.py:470p.complete_loginasync auth_callbackroutes.py:710p.complete_password_loginasync auth_password_loginroutes.py:768provider.revoke_sessionasync auth_logoutroutes.py:939provider.refresh_sessionasync auth_native_refreshWhy sites 1 and 3 are offloaded at the helper rather than the inner call.
Both helpers are synchronous and each has exactly one caller
(
git grep -n "_verify_bearer\|_attempt_refresh"returns the definition, thesingle call, and two docstring mentions — nothing else), and both callers sit
inside
async gated_auth_middleware. Offloading the helper moves the entireprovider walk, including its per-provider retries, to a worker in one
await,instead of returning to the loop between providers. It also leaves both helper
bodies byte-identical to
main, which keeps this PR out of the regionsthree other open PRs are rewriting (see Related / Positioning).
audit_logand_client_ipthen run on the worker thread: both are synchronous,_client_iponly reads already-parsed headers, and
audit.pyappends under its own lock —no
awaitis introduced inside either helper.Both helper docstrings gain an explicit one-line blocking contract, which is
the durable half of the change: the next person to add a caller is told what
the requirement is.
Reasoned exclusion — one site, on purpose
hermes_cli/dashboard_auth/audit.py:92(with open(path, "a", ...)) isreached from the same async paths and is the same class of defect (blocking
I/O on the loop), but it has a different root cause and a different remedy: a
sub-millisecond local append with no network and no timeout, versus a 10-second
network round trip. Offloading it would require making
audit_log()async at~30 call sites, several inside synchronous helpers. That is a different change
of a different shape and does not belong in this PR.
Commits
Six atomic commits, one per independently-meaningful seam:
fix(dashboard-auth)— bearer verify (site 1, via:359)fix(dashboard-auth)— cookie-path verify loop (site 2)fix(dashboard-auth)— middleware refresh chain (site 3, via:457)fix(dashboard-auth)— login start ×2 + callback (sites 4, 5, 6)fix(dashboard-auth)— password login, logout revoke, native refresh (sites 7, 8, 9)test(dashboard-auth)—tests/hermes_cli/test_dashboard_auth_off_loop.pyEach of 1–5 was verified green on the full
dashboard_authsuite at its ownSHA (128 passed, 1 skipped at each), and none of them is a no-op — every one
has a test that goes red when only that commit's hunk is reverted (table
below).
How to Test
pytest tests/hermes_cli/test_dashboard_auth_off_loop.py -q→ 11 passed.pytest tests/hermes_cli/ -k dashboard_auth -q→ 139 passed, 1 skipped (128 passed, 1 skipped on
main; the 11 new).Test design
The assertion is structural, not timing-based. A recording provider calls
asyncio.get_running_loop()on entry to each protocol method:RuntimeErrormeans it is on a worker thread (correct), a live loop means the dashboard's
loop is being blocked (the bug). One assertion per production call site.
One timing test covers the user-visible symptom directly — a provider that
blocks for 2s must not delay a concurrent request to a public, provider-free
route. It is written around the trap that makes this class of test worthless:
TestClient(app)outside awithblock builds a fresh anyio portal, andtherefore a fresh event loop, per request, so two requests can never contend
and such a test passes even against unpatched code. Every client here is
entered as a context manager, pinning one loop for the whole test, exactly as
uvicorn serves the real dashboard.
New file rather than an append to an existing
test_dashboard_auth_*module:it registers its own recording providers and needs its own gate fixture.
Fails-before / passes-after, per reverted hunk
Each row: revert that production hunk alone against the rest of the branch,
run the named test, restore.
middleware.pybearer verify (:359)test_bearer_verify_runs_off_the_loop,test_a_slow_provider_does_not_stall_a_concurrent_requestmiddleware.pycookie verify (:424)test_cookie_verify_runs_off_the_loopmiddleware.pyrefresh chain (:457)test_middleware_refresh_runs_off_the_looproutes.pyauth_login(:205)test_auth_login_start_runs_off_the_looproutes.pyauth_native_authorize(:370)test_native_authorize_start_runs_off_the_looproutes.pyauth_callback(:470)test_auth_callback_complete_runs_off_the_looproutes.pyauth_password_login(:710)test_password_login_runs_off_the_looproutes.pyauth_logout(:768)test_logout_revoke_runs_off_the_looproutes.pyauth_native_refresh(:939)test_native_refresh_runs_off_the_looptest_provider_errors_still_surface_unchangedis deliberately not in thattable: it is a behaviour-preservation guard and passes both before and after.
It is here so a later refactor cannot swap the offload for something that drops
the provider's exception instead of re-raising it in the awaiting frame.
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — ran the fulldashboard_authsuite (139 passed, 1 skipped) plus per-commit runs, not the whole tree; relying on CI for thatDocumentation & Housekeeping
docs/, docstrings) — the_verify_bearerand_attempt_refreshdocstrings now state their blocking contractasyncio.to_threadis stdlib (3.9+) and platform-independent; no platform-specific code paths touchedRelated / Positioning
Ten open PRs touch these two files. None is a superset of this one, and only
one offloads anything. Line-level overlaps, by number and anchor (rival hunk
line numbers are against their own bases):
routes.py@@ -895,46 +1099,35 @@ async def auth_native_refreshawait run_in_threadpool(_refresh_native_session_sync, ...). That covers site 9 of our nine, and deletes the exact line we change. It does not touch the other eight.middleware.py@@ -454,7refreshed = _attempt_refresh(→_attempt_refresh_with_replay_grace(. Concern is desktop session persistence; the new wrapper is still synchronous, so nothing moves off the loop.middleware.py@@ -350,7,@@ -427,35 +450,75_attempt_refresh(→await _attempt_refresh() and rewrites the helper body. Concern is RT-reuse coalescing. Making the helperasync defdoes not take the blocking call off the loop — the innerprovider.refresh_session(...)stays synchronous inside it. Cold since 07-15 and already conflicts withmain.middleware.py@@ -341,16,@@ -361,7(inside_attempt_refresh)routes.py@@ -530,7,@@ -551,7,@@ -766,7@@ -766,7hunk is inauth_logoutbut covers its tail (clear_pkce_cookie), not ourrevoke_sessionline. Same function, disjoint lines.routes.py@@ -20,@@ -396,12auth_callback; disjoint from our:470.middleware.py, inside/neargated_auth_middleware/api/config/schemagating, native loopback login). None offloads.plugins/dashboard_auth/**onlyplugins/. Disjoint.Text searches for the offload concern itself —
dashboard_auth to_thread,asyncio.to_thread dashboard,run_in_executor dashboard_auth,run_in_threadpool dashboard_auth,blocking event loop dashboard auth—return zero open PRs.
Precedent for this exact shape (cited as precedent for the pattern, not as
anyone requesting this change): #83951, "fix(gateway): offload all blocking
atomic_json_writecalls from async paths", merged 2026-08-11, uses the sameawait asyncio.to_thread(fn, ...)at-the-call-site idiom. The class has a longmerge history: #55159, #53603, #56212 merged by @teknium1; #76972, #74048,
#51889, #51890, #48561 by @kshitijk4poor; #67283, #65893 by @OutThisLife.