fix(proxy): resolve upstream credentials off the event loop - #85017
fix(proxy): resolve upstream credentials off the event loop#85017briandevans wants to merge 6 commits into
Conversation
`create_app` registers `handle_proxy` as an `async def`, and it called `adapter.get_credential()` directly on the aiohttp event loop. `UpstreamAdapter` is a synchronous contract (`adapters/base.py` — every method is a plain `def`), and the shipped adapters implement it with blocking I/O. `NousPortalAdapter.get_credential` takes `_auth_store_lock()` — a cross-process advisory lock with `AUTH_LOCK_TIMEOUT_SECONDS = 15.0` (`hermes_cli/auth.py:110`) — reads `auth.json` off disk, and may issue a token-refresh POST; on a terminal `AuthError` it takes that lock a second time to persist the quarantined state. `XAIGrokAdapter.get_credential` reads its key pool off disk. The proxy is one process with one loop, and `handle_proxy` streams with `sock_read=300`, so long-lived completions are the normal case. Blocking inside credential resolution therefore freezes *every* concurrent in-flight stream mid-token for the duration — a concurrent `hermes auth` command holding the auth-store lock is enough to do it. Every proxied request goes through this path. Dispatch through `asyncio.to_thread` instead. This is a pure scheduling change: `to_thread` re-raises the worker's exception in the awaiting frame, so the existing `except Exception` -> 401 `upstream_auth_failed` mapping is unchanged, and the adapters' own `self._lock` still serialises concurrent resolutions exactly as before. Fixing it at the handler also leaves the synchronous `UpstreamAdapter` ABC untouched, so it covers every adapter without conflicting with in-flight work that subclasses it.
Adds `tests/hermes_cli/test_proxy_off_loop.py`, mirroring the harness in
`test_proxy.py`: the proxy and a fake upstream run as real aiohttp
servers on ephemeral ports under a single `asyncio.run`, guarded by
`pytest.importorskip("aiohttp")` — no pytest-aiohttp dependency.
The primary assertion is thread identity, not latency. A latency
assertion measured with an HTTP client on the blocked loop is vacuous:
the client's own timer cannot advance until the block ends, so it reports
a fast response on code that was provably frozen.
- `test_get_credential_runs_off_the_event_loop` records
`threading.get_ident()` inside the adapter and compares it to the loop
thread. Before the fix both are the same ident.
- `test_event_loop_keeps_running_while_credentials_resolve` runs a
heartbeat task on the loop and has the adapter sample its counter on
entry and exit, so the reading is taken from the loop rather than
through a client that shares it. Before the fix exactly 0 iterations
run across a 0.5s stall; after it, ~50.
- `test_credential_failure_still_maps_to_401` pins the error contract
across the change of call form. It is deliberately not in the
red-before set — it guards behaviour the fix must leave alone.
`handle_health` called `adapter.is_authenticated()` inline from an `async def`. `UpstreamAdapter.is_authenticated` is documented as "Should be cheap — no network calls. Used by `proxy start` for a clear up-front error before binding a port." (`adapters/base.py`), and that is true of the `proxy start` preflight, which runs in a plain synchronous CLI function. It is not true on the event loop: `NousPortalAdapter.is_authenticated` goes through `_read_state()`, which takes `_auth_store_lock()` — the same cross-process lock with a 15s timeout as credential resolution — and `XAIGrokAdapter` reads its key pool off disk. `/health` is precisely what a supervisor, systemd unit, container healthcheck or load balancer polls, on a fixed interval, so it is the endpoint least able to afford a lock wait; and a wait here freezes every concurrent proxied stream, not just the healthcheck. Offload it with `asyncio.to_thread`. The response body is byte-identical; only the scheduling changes.
Extends `test_proxy_off_loop.py` with the `/health` half, using the same two-assertion shape as the credential tests: - `test_is_authenticated_runs_off_the_event_loop` compares the thread the adapter's `is_authenticated` ran on against the loop thread. Before the fix they are the same ident. - `test_event_loop_keeps_running_while_health_resolves_auth_state` reads a loop-side heartbeat counter sampled by the adapter across its own stall. Before the fix exactly 0 iterations run across 0.5s. Both also assert the response is unchanged (`200`, `authenticated: true`), so the offload cannot quietly alter what `/health` reports.
There was a problem hiding this comment.
Pull request overview
This PR fixes hermes proxy event-loop starvation by offloading synchronous upstream adapter operations (is_authenticated() and get_credential()) onto worker threads, preventing streaming completions and /health checks from freezing all concurrent proxy traffic when adapters perform blocking disk/network I/O.
Changes:
- Offload
/healthauthentication checks viaasyncio.to_thread(...)to keep health polling from blocking the aiohttp loop. - Offload upstream credential resolution in the proxy forwarding path via
asyncio.to_thread(...)to prevent credential refresh / auth-store lock waits from freezing streaming. - Add a new regression test suite that asserts adapter calls run off the loop thread and that the loop continues to make progress during simulated blocking.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
hermes_cli/proxy/server.py |
Moves synchronous adapter calls (is_authenticated, get_credential) off the aiohttp event loop using asyncio.to_thread. |
tests/hermes_cli/test_proxy_off_loop.py |
Adds tests that verify thread identity (off-loop execution) and heartbeat-based loop liveness during blocking adapter work. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # it on a worker thread so a refresh or a contended lock cannot freeze | ||
| # every other in-flight streaming completion on this single loop. | ||
| try: | ||
| cred = adapter.get_credential() | ||
| cred = await asyncio.to_thread(adapter.get_credential) | ||
| except Exception as exc: |
There was a problem hiding this comment.
Shipped. get_retry_credential is now offloaded too, so all three blocking methods on the UpstreamAdapter contract run off the loop.
fix(proxy): resolve the 401/429 retry credential off the event loop (e02b447900d), current head 5d0e3a16249eed3fae6d8db66afd88ef68fe15a1 — hermes_cli/proxy/server.py, the upstream_resp.status in {401, 429} branch:
retry_cred = await asyncio.to_thread(
adapter.get_retry_credential,
failed_credential=cred,
status_code=upstream_resp.status,
)Worth noting this was the most expensive of the three rather than the cheapest. NousPortalAdapter.get_retry_credential routes into _get_credential(force_refresh=True), so the token-refresh POST that get_credential only performs near expiry is unconditional here — under the same 15s cross-process _auth_store_lock(). XAIGrokAdapter.get_retry_credential loads the key pool off disk and rotates it under self._lock. And a 429 is by definition the moment the proxy is busiest, so it was the worst point at which to freeze every other in-flight stream.
The error contract is unchanged: to_thread re-raises in the awaiting frame, so the branch's existing except Exception -> retry_cred = None still swallows a failed rotation and streams the upstream's own rejection back.
Pinned by three tests in tests/hermes_cli/test_proxy_off_loop.py (naming them since a SHA does not survive a rebase):
test_get_retry_credential_runs_off_the_event_loop— thread identity, not latency. A latency assertion measured by an HTTP client on the blocked loop is vacuous, since the client's own timer cannot advance until the block ends.test_event_loop_keeps_running_while_the_retry_credential_resolves— loop-side heartbeat sampled from inside the stalled adapter; records exactly 0 iterations across a 0.5s rotation before the fix.test_retry_credential_failure_still_returns_the_upstream_rejection— guards the error contract above.
12 passed across test_proxy_off_loop.py + test_proxy.py. The PR body's site-coverage table and the note on the textual overlap with #62297's @@ -198,23 +270,57 @@ hunk have been updated accordingly.
|
CI note — the single red is a third-party toolchain download, not this change.
That is a pinned |
`handle_proxy` already offloads the two credential reads on the happy path,
but the rotation inside the `upstream_resp.status in {401, 429}` branch still
called `adapter.get_retry_credential` inline on the event loop.
That is the most expensive of the three blocking methods on the
`UpstreamAdapter` contract, not the cheapest:
* `NousPortalAdapter.get_retry_credential` routes into
`_get_credential(force_refresh=True)`, so the token-refresh POST that
`get_credential` performs only near expiry is unconditional here — and it
runs under the same `_auth_store_lock()`, a cross-process advisory lock
with a 15s timeout.
* `XAIGrokAdapter.get_retry_credential` loads the key pool off disk and
calls `try_refresh_current` / `mark_exhausted_and_rotate` under its lock.
So every upstream 401 or 429 froze the proxy's single event loop — and with
it every other in-flight streaming completion — for the whole rotation. A 429
is exactly when the proxy is busiest, which is the worst moment to stall.
Wrap it in `asyncio.to_thread`, matching the two sites above. The error
contract is unchanged: `to_thread` re-raises the worker's exception in the
awaiting frame, so the existing `except Exception -> retry_cred = None` still
swallows a failed rotation and streams the upstream's own rejection back.
Extends the off-loop suite to the third and last blocking method on the
`UpstreamAdapter` contract, the 401/429 rotation.
As with the two existing pairs, the primary assertion is **thread identity**,
not latency: a latency assertion measured by an HTTP client on the blocked
loop is vacuous, because the client's own timer cannot advance until the
block ends and it therefore reports a fast response on provably frozen code.
* `test_get_retry_credential_runs_off_the_event_loop` records
`threading.get_ident()` inside the fake adapter and compares it to the
loop thread, and checks the rotation still works end to end (rejected
bearer forwarded first, rotated bearer second).
* `test_event_loop_keeps_running_while_the_retry_credential_resolves`
samples a loop-side heartbeat counter from inside the stalled adapter. On
the unfixed handler it records exactly 0 loop iterations across a 0.5s
rotation.
* `test_retry_credential_failure_still_returns_the_upstream_rejection`
guards the error contract the change must leave alone: a raising rotation
is still swallowed and the upstream's own 401 is streamed back, with no
second forward.
A new `_build_rejecting_upstream` harness drives the `status in {401, 429}`
branch by rejecting every bearer except the rotated one.
alt-glitch
added
type/perf
fix(proxy): resolve upstream credentials off the event loop
|
What does this PR do?
Symptom: with
hermes proxyrunning, an agent's streaming completion freezesmid-token for several seconds — and every other concurrent stream through the
same proxy freezes at the same moment, then everything resumes together. A
/healthpoll from a supervisor or container healthcheck can trigger it on itsown.
Cause:
create_appregisters exactly twoasync defhandlers, and betweenthem they called all three of the adapter contract's synchronous, hard-blocking
methods directly on the aiohttp event loop.
UpstreamAdapteris an entirely synchronous contract —hermes_cli/proxy/adapters/base.py, every method a plaindef— and the shippedadapters implement it with blocking file and network I/O:
handle_proxyadapter.get_credential()(server.py:125)nous_portal.py:76→_read_state()→_auth_store_lock()→_load_auth_store()→resolve_nous_runtime_credentials(), a token-refresh POST. On a terminalAuthError,_save_state()takes_auth_store_lock()a second time and rewrites the store.xai.py:56→_load_pool(), a disk read.handle_healthadapter.is_authenticated()(server.py:106)nous_portal.py:65→_read_state()→ the same_auth_store_lock().xai.py:52→_load_pool().handle_proxyadapter.get_retry_credential()(server.py:202)nous_portal.py:79→_get_credential(force_refresh=True)→ the same_auth_store_lock(), and the refresh POST above is now unconditional.xai.py:76→_load_pool()+try_refresh_current()/mark_exhausted_and_rotate()underself._lock._auth_store_lockis a cross-process advisory lock withAUTH_LOCK_TIMEOUT_SECONDS = 15.0(hermes_cli/auth.py:110, taken at:1225),so any concurrent
hermes auth …command in a different process can hold itwhile the proxy's event loop sits inside it.
Why this is severe rather than cosmetic.
handle_proxyforwards withaiohttp.ClientTimeout(total=None, sock_connect=15, sock_read=300)(
server.py:136) — long-lived streaming completions are the normal case, notthe exception. The proxy is one process with one loop, so blocking inside
credential resolution stops every in-flight stream, not just the request that
triggered it. Blast radius is every proxied request of every
hermes proxyuser; this is the subsystem's hot path, not a rare state.
For
/healththe repo already states the invariant against itself —adapters/base.py:63-68:That holds for the
proxy startpreflight, which runs in a plain synchronousCLI function (
proxy/cli.py:44). It does not hold on the event loop, where a15-second cross-process file lock is not cheap — and
/healthis exactly what asupervisor, systemd unit, container healthcheck or load balancer polls on a
fixed interval.
Fix: dispatch all three through
await asyncio.to_thread(...).asyncioisalready imported at
server.py:14;requires-python = ">=3.11,<3.14"(
pyproject.toml:15), andto_threadis the house idiom in this package (102matching lines vs 23 for
run_in_executoracrosshermes_cli/). It is also the idiom ofthe maintainer-side sweep this completes for the proxy —
965a5487884"serialize config mutations and finish the router off-loopsweep".
This is a pure scheduling change:
to_threadre-raises the worker's exception inthe awaiting frame, so
handle_proxy's existingexcept Exception→401 upstream_auth_failedmapping still fires identically, and the retry branch'sexcept Exception→retry_cred = Nonestill swallows a failed rotation andstreams the upstream's own rejection back. Both pinned by a test.
self._lock(nous_portal.py:51; xai mutatesself._poolunder the samelock), so concurrent offloaded calls serialise exactly as they do today. The
offload moves the wait off the loop; it does not remove the mutual
exclusion.
/healthreturns the same JSON.Why at the handler and not in the ABC. Making
UpstreamAdapterasync wouldbe the wrong layer and would collide with in-flight work: four open PRs are
currently adding brand-new upstream adapters that each implement
def get_credential/def is_authenticatedsynchronously (#58647anthropic.py,#54877
codex.py, #62510 and #62297openai_codex.py). Offloading once inserver.pyfixes every adapter — including those four — while leaving the ABCthey subclass completely untouched.
Related Issue
No filed issue. The invariant is stated in-repo at
hermes_cli/proxy/adapters/base.py:65-68("Should be cheap — no networkcalls"), and
965a5487884is the existing off-loop sweep this extends to theproxy.
Type of Change
Changes Made
Six independently cherry-pickable commits, one fix/test pair per site:
fix(proxy): resolve upstream credentials off the event loop—hermes_cli/proxy/server.py:125,handle_proxy.test(proxy): cover credential resolution blocking the proxy event loop—new file
tests/hermes_cli/test_proxy_off_loop.py.fix(proxy): keep the health endpoint off the auth-store lock—hermes_cli/proxy/server.py:106,handle_health.test(proxy): cover health responsiveness while the auth store is locked.fix(proxy): resolve the 401/429 retry credential off the event loop—hermes_cli/proxy/server.py:202,handle_proxy's rotation branch.test(proxy): cover the retry credential blocking the proxy event loop.Net diff:
hermes_cli/proxy/server.py+26/-3, plus one new test file.Site coverage (the full sweep of this root cause)
grepfor adapter calls outsidehermes_cli/proxy/adapters/returns exactlythree blocking sites, all in
server.py.adapter.display_nameandadapter.allowed_pathsare constant properties on both adapters and are notaffected.
proxy/cli.pyalso callsis_authenticated/get_credential, butfrom plain synchronous CLI functions with no event loop, so it does not share
the root cause.
handle_proxy→get_credential():125handle_health→is_authenticated():106handle_proxy→get_retry_credential():202All three blocking methods on the contract are now covered. This PR previously
shipped two of the three; that has been corrected — see the next section.
get_retry_credential(server.py:202) — added, with a known textual overlapAn earlier revision of this PR left this site alone because #62297 rewrites
that exact block, and noted it should be handled by whichever of the two lands
second. On reflection that was the wrong call, for three reasons:
is that the proxy's single loop must never sit inside adapter I/O. A partial
fix leaves the loop frozen on exactly the path where the harm is largest —
see below — while making the file look swept.
NousPortalAdapter.get_retry_credential(nous_portal.py:79) routes into_get_credential(force_refresh=True), so the token-refresh POST thatget_credentialperforms only near expiry is unconditional here — underthe same 15s cross-process
_auth_store_lock().XAIGrokAdapter.get_retry_credential(xai.py:76) loads the key pool offdisk and calls
try_refresh_current/mark_exhausted_and_rotateunderself._lock. And a 429 is by definition the moment the proxy is busiest, soit is the worst possible time to stall every other in-flight stream.
@@ -198,23 +270,57 @@(old lines 198–220, read from the diff itself)adds an
error_context=argument, convertsif upstream_resp.status in {401, 429}into awhile, and addsfail_closed_on_exhaustion. None ofthat conflicts with where the call runs: whichever lands second, the
reconciliation is wrapping the same call in
await asyncio.to_thread(...)inside the new loop structure. That PR has had no commit since 2026-07-11.
I have deliberately not touched the signature, the
if/whileshape, or theexhaustion handling, so the hunk stays as small and as cleanly re-appliable as
possible on top of either ordering.
Related / positioning
Eleven open PRs touch
hermes_cli/proxy/server.py. I read each diff and took thehunk anchors from the diffs themselves, not from PR titles:
server.pyhunks (old lines):106/:125?proxy/server.py, a separate credential-broker daemon#62297 in detail, since it is the only overlap. Its 198–220 hunk is the one
discussed above. Its 100–114 hunk does touch
handle_health, but onlyto restyle the
json_response(...)call from the expanded form to braces — itkeeps
adapter.is_authenticated()synchronous and on the loop, so it is aformatting collision, not a competing fix. I have kept the
/healthchangehere because it is the substantive one and because the invariant it restores is
the repo's own; rebasing either PR over the other in that block is a one-line
reconciliation.
I also grepped every one of those eleven diffs' added lines for all four
offload idioms (
to_thread,run_in_executor,run_in_threadpool,ThreadPoolExecutor) and forasync def get_credential/async def is_authenticated. Zero hits in all eleven — none of them offloads thesecalls, and none makes the adapter methods async. (The single corpus-wide hit,
run_in_executorin #4691, is in the unrelated root-levelproxy/daemon.) Sothis change is strictly additive to everyone else's work in this file.
The new tests live in
tests/hermes_cli/test_proxy_off_loop.pyrather thantests/hermes_cli/test_proxy.pyfor the same reason — six of those open PRstouch the existing file.
How to Test
Before/after, verified in both directions. With the three production hunks
reverted and the tests kept, on
origin/main:With the fix:
12 passedacross both proxy test files, and32 passedacross theadjacent proxy-touching suites (
tests/test_iron_proxy_cli.py,tests/hermes_cli/test_nous_inference_url_validation.py,tests/cli/test_cli_status_command.py,tests/gateway/test_unknown_command.py).Every one of the six commits is green on its own, checked out individually.
A note on how these tests are written
The primary assertion in each pair is thread identity, not latency, and that
is deliberate. A latency assertion measured with an HTTP client running on the
blocked loop is vacuous: the client's own timer cannot advance until the block
ends, so it reports a millisecond response time on code that was provably frozen
for a second. The secondary assertion avoids the same trap by reading a
heartbeat counter that ticks on the loop, sampled by the adapter itself on
entry and exit — 0 iterations before the fix, ~50 after.
The harness mirrors the one already in
tests/hermes_cli/test_proxy.py: theproxy and a fake upstream run as real aiohttp servers on ephemeral ports under a
single
asyncio.run, guarded bypytest.importorskip("aiohttp"). One realserver means exactly one loop, which is what makes the starvation observation
meaningful. No new test dependency — pytest-aiohttp is deliberately avoided,
as the existing file's own comment explains.
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/Aasyncio.to_threadis platform-neutral; the tests use ephemeral loopback ports and no platform-specific primitives