Follow-up to the salvaged #93517. Two gaps found in review:
- After the last unregister the supervisor stayed resident for the life of
the process (~15 MB + a pipe) in any gateway/cron that ever connected a
stdio server; main's per-server watchdog exited with its server. Close
our write end when the supervised set empties: EOF with nothing
registered makes the supervisor exit without reaping, and the next
register already respawns and replays.
- A child that raced and exited before os.getpgid dropped its group from
coverage entirely. The SDK spawns stdio servers as session leaders
(pgid == pid), so fall back to the pid instead; the prune forgets the
group once nothing in it is alive.
Tests: fake-supervisor release/re-spawn sequence, and a real-process EOF
release (supervisor exits 0, unregistered child untouched). Mutation
checked: disabling the release branch fails both.
Review raised a real gap: we reap by pgid, so a registration is only as
meaningful as the group's identity. A group we deliberately keep registered --
an orphan teardown failed to kill, such as the `node` mcp-remote leaves behind
-- can later exit on its own, after which the kernel may hand that pgid to an
unrelated process owned by the same user. An ungraceful Hermes death while the
registration is stale would then signal a stranger. `_is_safe_target` cannot
catch it, because the value is stale rather than invalid.
Prune registrations whose group has no members left on every registration
change, and tell the supervisor to forget them. Signal 0 is a pure existence
question -- it cannot terminate anything -- so this is cheap and safe to run on
the hot path. An ambiguous answer (EPERM: exists but not ours) keeps the
registration, since dropping real coverage is the more expensive mistake.
This narrows the window rather than closing it: a group can still die and its
pgid be recycled between two probes. Closing it completely means proving group
identity at reap time, e.g. stamping children with a boot-unique env marker and
checking a member still carries it. That was judged not worth putting a `ps`
parse into the one process whose job is to stay simple enough to always work,
so the residual is now documented in the module docstring instead, along with
the note that Hermes's existing killpg-based orphan cleanup already carries the
same exposure (upstream #88350).
Also records why supervisor recovery is deliberately two-step: a failed write
drops the handle, and the next call rebuilds coverage from `_supervised_pgids`,
which -- not the pipe -- is the record of what needs reaping.
The protocol tests register synthetic pgids that were never real process
groups, so they now state that precondition through an `all_groups_alive`
fixture instead of depending on pid-space luck.
29 tests pass. Verified the change adds no failures: same selection, my changes
stashed vs applied, 42 failed either way (pre-existing in a locally rebuilt
venv) with passed going 617 -> 619 for the two new tests. Footgun linter clean.
(cherry picked from commit 3a7d219620a8cb57a7fe2c2346ce3f055320e743)
Every stdio MCP server was wrapped in its own CPython watchdog that polled
getppid() every 2s to notice an ungraceful Hermes exit (kill -9, OOM, crash,
force-quit), since macOS has no PR_SET_PDEATHSIG. That is a whole interpreter
per server for a job that does nothing until the moment Hermes dies: 10.1 MB
physical footprint each, measured on macOS/arm64.
Replace the fleet of pollers with a single supervisor per Hermes process
holding the read end of a pipe only Hermes writes to. Death detection becomes
EOF on that pipe -- exact and instant, rather than up to a poll interval late.
Hermes sends `register <pgid>` / `unregister <pgid>` as servers come and go;
on EOF the supervisor killpg's whatever is still registered, which is exactly
the set whose teardown never ran. A clean shutdown unregisters as it goes, so
EOF then finds nothing to kill.
Servers are now spawned unwrapped. The MCP SDK already starts each stdio child
in its own session, so the pgid recorded for killpg is the server's own group
and the existing cleanup paths reach it unchanged. That also deletes the
signal-forwarding layer the wrapper needed: wrapping had put the real server in
a different session from the pgid being tracked, so a graceful killpg would
have hit only the wrapper.
Measured on a 5-gateway host: 10 watchdogs (~98 MB) -> 5 supervisors (~49 MB).
One supervisor costs about what one watchdog did (9.9 vs 10.1 MB), so the win
is (servers_per_process - 1) x ~10 MB, and a process with no stdio servers now
spawns nothing at all.
The supervisor reads length-capped lines rather than iterating the stream: a
writer that never sends a newline would otherwise grow it without bound, which
it must not be vulnerable to when it is the last defense against leaked
servers. Found by feeding it /dev/zero, where it reached 15 GB.
Verified beyond unit coverage: a real stdio MCP server connects and its tools
are discovered on the unwrapped path; with a live Hermes holding a real
connection, kill -9 reaped the server, its grandchild (in the server's group,
the mcp-remote `node` case), and the supervisor exited on its own. The reap
tests were sabotage-checked in both directions -- a no-op reaper fails all
three, while the test pinning that a cleanly unregistered server survives keeps
passing -- and each wiring half fails independently when removed.
(cherry picked from commit a252d4ce7ff1722f687635fdbf0cff79f538c3f1)
tests/gateway/test_feishu_bot_admission.py builds FeishuAdapter via
object.__new__ and installs dedup state by hand (install_dedup_state), so
the asyncio.Lock added in __init__ was missing -> AttributeError in CI.
Resolve the lock through a getattr-with-create helper and add it to the
shared test helper.
Offloading the fsync to a worker thread (previous commit) lets two inbound
messages have two persists in flight. Without ordering, a slow first flush
lands after the second and the disk file loses the newer token/id/seen-id
(in-memory state stayed correct, so the loss only surfaced after restart).
The loop-side dict could also be mutated while the worker iterated it.
- weixin ContextTokenStore / discord non-conversational tracker: snapshot
the payload on the loop, hand it to the worker, serialize with a per-store
asyncio.Lock.
- feishu dedup: same asyncio.Lock around the offloaded persist (its snapshot
was already taken under _dedup_lock; the write was not).
- Regression tests: two concurrent persists with a slow first write must
leave the union on disk. All three fail on the previous commit.
atomic_json_write() calls os.fsync(), which blocks until the write
reaches stable storage. #83906 offloaded this in channel_directory.py,
gateway/run.py, and gateway/slash_commands.py, but the same blocking
pattern was still present on three platform-adapter message hot paths
that were never touched by that pass:
- plugins/platforms/feishu/adapter.py: _is_duplicate() persists the
dedup cache on every new inbound message (shared by both websocket
and webhook transports). The in-memory bookkeeping stays under
_dedup_lock; the fsync now runs off it via asyncio.to_thread so
concurrent callers on the ws-thread loop aren't blocked on disk I/O.
- gateway/platforms/weixin.py: ContextTokenStore.set() persists the
context-token cache on every inbound message carrying a
context_token, called from the async poll-loop handler
_process_message.
- plugins/platforms/discord/adapter.py: mark_many() persists the
non-conversational message-id tracker, called from both send() and
send_update_prompt() when a message is tagged nonconversational.
Each fix mirrors the existing asyncio.to_thread(atomic_json_write, ...)
pattern from #83906. Regression tests assert the persist call runs on
a different thread than the event loop, mirroring
test_directory_write_runs_off_event_loop_thread in
test_channel_directory.py.
Extract the signature-checking cleanup dispatch that cleanup_vm already had
into tools.terminal_tool._cleanup_env and call it from the prompt-time
backend probe instead of carrying a second copy. Comment on the probe
trimmed to the non-obvious part (why ssh is skipped).
Follow-up to the cherry-picked fix:
- decide force_remove by inspect.signature (the idiom cleanup_vm already
uses) instead of catching TypeError, which would misread a TypeError
raised inside cleanup() as a signature mismatch and call cleanup twice
- skip teardown for ssh: SSHEnvironment has no task-scoped sandbox and its
cleanup() runs sync_back + 'ssh -O exit' on a ControlMaster socket keyed
by user@host:port, shared with the agent's real environment
`_probe_remote_backend` spins up an environment with
`task_id="prompt-backend-probe"` purely to run one `uname` for the system
prompt, then drops the reference without cleaning up. Container backends
default to `container_persistent` / `docker_persist_across_processes`, so
that throwaway sandbox stays up for the entire process lifetime, sitting
idle next to the agent's own `default` sandbox.
Observed on a three-profile Docker-backend host: six running containers
where three were doing any work. Every profile permanently paid for one
extra container whose only job had been a single `uname` at startup.
Fix: wrap the probe in `try/finally` and call
`env.cleanup(force_remove=True)` — the explicit-teardown path that
`DockerEnvironment.cleanup` already documents ("No current caller passes
`force_remove=True`; the parameter is here so the explicit-teardown
semantics can be wired up later"). Backends that inherit the kwarg-less
base `cleanup()` fall back to the bare call.
Verified on a live aarch64 Docker backend: the probe container is now
created, used and reclaimed within the same call. Three regression tests
cover the success path, the exception path, and the kwarg-less fallback;
all three fail without the source change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PLbMzWqHVTPNBFisM8UiNB
Findings from the efficiency review pass on the two salvages, applied as one
small follow-up:
- copilot_auth: check the negative cache BEFORE taking the per-fingerprint
exchange lock. During the 60 s post-failure window, dashboard polls now
raise immediately instead of parking an executor thread behind the
in-flight holder (up to ~50 s) to learn the same answer. Test hangs
without the check (timeout 124), passes with it.
- buzz _localize_inbound_media: download_path.read_bytes() was still
evaluated on the loop as the argument to the offloaded cache call — up to
the 128 MiB inbound cap. Read off the loop too.
- test_list_credential_pool_keeps_loop_responsive: 0.5 s block / 0.25 s
threshold (2x margin) so runner descheduling cannot false-fail it while a
real regression still trips it.
`hermes_home_key` called `Path.resolve()` on every call, which is a
filesystem call. `ToolRegistry.current_scope_key()` calls it, and
`_merged_tools()` calls that, so every single registry lookup went to
the disk: `get_entry`, `get_toolset_for_tool`, `get_emoji`, and the rest.
Anything that looks up many tools in a row paid for it. Building the
deferred-tool catalog for `tool_search` calls `get_entry` once per tool,
so with 400 deferrable tools one search did 800 path resolutions. On
Windows that is `nt._getfinalpathname`, and it took 132ms of the 148ms
the catalog build spent. The tokenizing everyone would suspect first was
under 4ms of it.
The resolved value is now remembered per input path. A process only sees
a handful of home paths, so the dict stays tiny. Measured on the same
400-tool catalog build, the lookups drop from 132ms to 3.1ms with one
entry cached.
A path that does not exist yet is resolved but not remembered, because
the answer can change once it is created, for instance when part of the
path turns out to be a link. A different home path is a different key,
so switching profiles or `HERMES_HOME` gets its own entry rather than
reusing a stale one.
Same retry semantics (only a non-FileNotFoundError OSError leaves the latch
unset), one latch assignment instead of five; docstring now says the call
runs per get/put and does work once.
Follow-up to the cherry-picked change: reuse the repo's shared atomic writer
(adds the fsync the hand-rolled mkstemp+os.replace skipped, keeps 0600 on
create) and document the clean-verdict staleness trade-off the cross-process
cache introduces.
A busy or briefly unreadable cache file must not disable disk loads for
the rest of the process; only missing or permanently malformed files
should mark the cache as loaded.
The OSV preflight is a synchronous network POST to api.osv.dev (up to 10s timeout) on every MCP stdio server start. Tools like `hermes mcp test` and MCP reconnect ladders spawn the same package repeatedly, but the previous in-process cache was empty after every process restart, so each run re-queried OSV and added 5.91x variance to the connection-time span.
Persist the malware-check verdict cache to `<hermes_home>/cache/osv_check.json`. Cache expiry is stored as an absolute wall-clock timestamp, so it survives restarts and monotonic-clock skew. Loading only adds missing keys so an in-memory overwrite (e.g. a test forcing expiry) is not silently reversed by the disk copy. Writes are atomic (temp file + rename) and happen under the existing cache lock.
Fixes the Hermes-owned OSV preflight variance component of #68416. Server-side `initialize` time is outside Hermes' control.
- Adds `hermes_constants.get_hermes_home()` lazy import to keep `tools/osv_check.py` import-safe (stdlib + typing only at module scope).
- Switches cache timestamps from `time.monotonic()` to `time.time()` for persistence compatibility.
- Updates `tests/tools/test_osv_check.py` fixture to isolate disk cache per test via `HERMES_HOME` + `tmp_path`, and adds regression tests for persistence, reload, and disk format.
The background-process poller for non-local backends ran `cat` on the
whole log file every two seconds, then threw away everything except the
part it had not seen yet. The offset it needed was already tracked one
line below, so the full read was pure waste.
Cost of one poll grew with the total output so far, which makes the cost
of a run grow with the square of its length. A job writing 10 MB over an
hour moved about 9 GB across the docker or SSH channel to deliver 10 MB
of output.
The poller now asks the shell for the file size and the bytes after the
offset in one command. Reading the size first and cutting the tail at
that same size keeps the two in step, so a file that grows mid-command
never sends a byte twice. A file that shrank was rotated or truncated,
so the offset drops back to 0 and the buffer is dropped.
The output buffer is now appended to rather than replaced, matching the
local reader loops, and the offset is counted in bytes because the shell
counts bytes.
The previous commit removed cache_media_bytes from teams/adapter.py's import
block but left one bare call at the Bot-Framework image path — a NameError
that the surrounding 'except Exception' swallowed, so every BF image
attachment was silently dropped (ruff F821). Route it through
cache_media_bytes_async like the file's two other sites.
Same bug class at the sites the sweep did not reach:
- buzz _download_attachment / _localize_inbound_media: cache_media_bytes
called from async methods → cache_media_bytes_async.
- photon _dispatch_inbound: _normalize_binary_payload (base64 decode +
cache write of possibly multi-MB payloads) ran on the loop → closure made
async, helper offloaded via asyncio.to_thread.
Tests: cover cache_media_bytes_async (thread-id + kwarg forwarding);
teams/buzz tests re-pointed at the async seam.
Follow-up to the off-loop move: once the credential-pool handlers run on
worker threads, the dashboard's periodic /api/credentials/pool polls can
overlap, and during a DNS outage each poll would have started its own
exchange and abandoned its own hung resolver thread.
- Per-fingerprint threading.Lock around the exchange: concurrent callers
wait on the one in-flight attempt, then hit the positive or negative
cache (bounded worker count, no duplicate network calls).
- _urlopen_bounded: when the hard cap fires and the abandoned worker later
succeeds, close the HTTPResponse instead of leaking the socket.
- Tests (none shipped with the original PR): hard cap + late-close,
single-flight success and failure paths, and the pool endpoint running
off-loop / keeping the loop responsive under a 200 ms blocking read.
Network off (unplugged) froze the backend 17 minutes: the async
/api/credentials/pool endpoints (GET/POST/DELETE) called load_pool()
synchronously on the event-loop thread -> Copilot token exchange ->
blocking urlopen -> getaddrinfo stuck in C for 1016s, immune to
urlopen(timeout=10). WS dropped (1006), sessions detached, even log
writes stalled.
Fix 1 (web_server.py): move the three endpoint bodies into
asyncio.to_thread, matching the file's existing _run pattern.
Fix 2 (copilot_auth.py): _urlopen_bounded() runs the request in a
daemon thread with a wall-clock hard cap (timeout+5s) so DNS hangs
can no longer block any caller indefinitely.
Measured: simulated DNS hang now raises TimeoutError after 6.0s
instead of freezing the loop.
The module already binds run_in_threadpool (used by list_profiles_endpoint)
and every sibling router uses the same starlette helper; the nine new
loop.run_in_executor(None, _run) sites now go through that alias so the
file has one offload idiom. Behaviour-identical (both hand the callable to
a worker thread).
Also sweeps the one endpoint the PR left synchronous:
update_profile_model_endpoint's _write_profile_model reads and rewrites
the profile's config.yaml on the event loop.
Two assertions per offloaded site:
- a loop probe, where the stubbed callee records whether an event loop is
running in its own thread — the idiom already used by
tests/hermes_cli/test_cron_dashboard_off_loop.py; and
- a concurrency proof, where the stubbed callee blocks on a threading.Event
while an unrelated request is timed. On the unfixed handlers that request
waits out the whole block; served off the loop it returns in
milliseconds.
The concurrency proof needs a single event loop across requests, so the
client fixture enters the TestClient context manager: that pins one
blocking portal for the whole fixture, where a bare TestClient(app) would
spin up a fresh loop per request and pass even unfixed.
Also covers the status-code mapping through the executor hop (404 on a
missing profile, 400 on a rename collision, 404 from the resolve that stays
on the loop ahead of describe-auto) and the _MISSING sentinel cases: a
desktop.json holding `null` still reports exists=true, an absent one
reports exists=false, and an empty SOUL.md is still distinguishable from a
missing one.
The client fixtures read web_server._SESSION_TOKEN from the module rather
than pinning a literal. web_server resolves that token once at import, so
whichever test file imports it first fixes the value for the session and a
later monkeypatch.setenv is silently ignored — two files hardcoding
different tokens would 401 depending on collection order.
The remaining in-scope handlers in this router read and write profile
documents inline on the ASGI event loop:
- GET /api/profiles/{name}/soul reads SOUL.md
- PUT /api/profiles/{name}/soul atomic_write_text(SOUL.md)
- PUT /api/profiles/{name}/description write_profile_meta(profile.yaml)
- GET /api/profiles/{name}/desktop-overlay reads desktop.json
The persona save is the sharpest of the four: atomic_write_text() writes a
temp file, fsyncs it and replaces the original, so the loop is parked for
however long the filesystem takes to durably commit — unbounded on a slow
or contended disk, and paid on every Save in the editor.
Each handler keeps its existing status-code mapping. The reads probe and
load in a single executor hop rather than two, which also avoids widening
the gap between the existence check and the read.
Both readers return a _MISSING sentinel rather than None for an absent
file. desktop.json may legitimately contain the document `null`; collapsing
that onto None would newly report an existing-but-empty overlay as absent.
The same distinction is what the SOUL.md durability tests rely on, where
"file missing" and "file empty" must not both read as never-set.
_resolve_profile_dir() stays on the loop in all four, as it does in the
rest of this sweep: it is a name check plus one stat, and it owns the
400/404 responses.
Three more handlers in this router did filesystem work inline on the ASGI
event loop:
- PATCH /api/profiles/{name} calls rename_profile(), which stops a running
gateway through the same 10-second _stop_gateway_process() poll that
delete uses, then renames the profile directory, rewrites the Honcho
host blocks and regenerates the wrapper script.
- GET /api/profiles/active reads the active_profile state file and
resolves HERMES_HOME against the profiles root. The sidebar polls it.
- POST /api/profiles/active stats the target profile, creates the state
directory and writes active_profile via a temp file plus replace.
Rename carries the same worst case as delete and belongs off the loop for
the same reason. The two active-profile handlers are individually cheap,
but they are the routes the dashboard polls, so they are the ones most
likely to be queued behind something slower — and leaving them inline is
what made the router inconsistent with list_profiles_endpoint, which
already offloads a plain directory listing eight lines above.
The two reads in GET share one executor hop rather than taking one each.
POST /api/profiles/{name}/describe-auto called
profile_describer.describe_profile() inline. That function is a plain def;
it reaches agent.auxiliary_client.call_llm(), also a plain def, which makes
a synchronous provider request with a 60-second ceiling.
Held on the ASGI event loop that is six times the 10-second WebSocket
ready-probe threshold web_server.py records as the point where the desktop
app gives up (GH-73083). A single describe-auto on a slow or unreachable
auxiliary provider therefore takes the whole dashboard offline for up to a
minute, including the /api/ws and /api/pty sockets the desktop app and the
Chat tab run on.
Move the import and call into the default executor. _resolve_profile_dir()
deliberately stays on the loop ahead of the hop: it is a name validation
plus a single stat, and it owns the 400/404 responses that the handler's
`except Exception` would otherwise turn into a 500.
DELETE /api/profiles/{name} called profiles.delete_profile() inline on the
ASGI event loop. When the target profile has a gateway running, that call
stops it via _stop_gateway_process(), which polls the PID every 500 ms for
up to 10 s before escalating to a force kill, and then removes the profile
tree.
For the whole of that window the dashboard process serves nothing else.
web_server.py's own notes record what that costs: a stall of this length
"caus[ed] the Desktop's 10-second WebSocket ready-probe to time out
(GH-73083)", and both the desktop app and the dashboard's Chat tab drive the
agent over those WebSockets. Deleting a profile whose gateway is up is a
routine action that reliably reaches the full ten seconds — the handler's
own output announces "Gateway is running - it will be stopped".
Move the call into the default executor via run_in_executor, matching
list_profiles_endpoint, export_profile_endpoint and import_profile_endpoint
in this same module. The exception-to-status mapping is unchanged:
FileNotFoundError/ValueError are raised inside the worker and re-raised by
the await, so they still map to 404/400.
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.
`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 `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.
`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.
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.
`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.
Submodule and worktree checkouts store .git as a file (gitdir: pointer);
the carry-over only looked at directory names, so that form was still lost
on rollback. Handle files with the same guard. Docstring now states the
deliberate limit: an excluded entry whose skill dir the target snapshot
lacks is dropped with staging (no orphan .git) and is not undoable via the
safety snapshot, which excludes these paths as well.
Excluding nested .git from snapshots has a side effect on rollback: the
staging move takes the whole live skill dir (including its .git) into
.rollback-staging-*, the extract restores the snapshot without it, and the
staging dir is then deleted — so a skill that is itself a git checkout lost
its .git on any rollback. Reproduced: main preserves it, the exclusion-only
branch did not.
After a successful extract, move excluded subtrees from the staged copy back
under their restored skill dir (mirroring how a top-level .git survives by
never being staged). Regression test included.
Folds the incident explanation from #91458 (@liuhao1024) into the comment on
_EXCLUDE_TOP_LEVEL so the reason .git is excluded — compounding snapshot
growth, not just rollback safety — survives next to the set.
Co-authored-by: liuhao1024 <sunsky.lau@gmail.com>
_safe_skills_path() is the sibling of iter_skills_files(): when a symlink in
skills/ forces a sanitized copy for mount-based backends (Docker/Singularity),
it rglob-copied the whole tree — .hub, .curator_backups, node_modules and all.
Prune EXCLUDED_SKILL_DIRS before descending, same rule as the sync generator,
so the mounted copy never carries (or walks) the bookkeeping trees either.
Replaces the three hand-copied rglob loops + post-hoc parts check with one
os.walk generator that drops EXCLUDED_SKILL_DIRS from dirnames before
recursing. Same file set as the cherry-picked fix (the test binds it), but
the walk no longer stats every file under .hub/.curator_backups/node_modules
on each 5s FileSyncManager tick.
Bench (synthetic skills tree: 20 skills + 400 .hub files + 5x8MB curator
tarballs + 50 archived files): iter_skills_files() 35ms -> 2.4ms.
iter_skills_files() walked the skills tree with a bare rglob("*"), so the
.hub download cache, .archive, curator backups, and any node_modules/.git
under a skill package were uploaded to the sandbox on every sync. The
sandbox never reads them: skill content is resolved host-side.
EXCLUDED_SKILL_DIRS is already the canonical exclusion set, honoured by
discovery and backup. Apply it to the sync path too, across all three
roots iter_skills_files() walks (local, external, project-local), and add
.curator_backups to the set.
Measured on a local install: 900 files / 67.3 MB -> 771 files / 8.4 MB.
This is not just wasted bandwidth on the SSH backend, where the oversized
payload can exceed the 120s _ssh_bulk_upload deadline and surface as the
agent hanging on every tool call.
The filter intentionally does not reuse is_excluded_skill_path(), which
also prunes references/, templates/, assets/ and scripts/ -- those hold
support files and bundled scripts the sandbox does read and execute.
The sync and async retry sites each re-derived the same three-clause
decision (critical task + full-budget timeout + not a no-progress fail) with
their own copy of the rationale — which is exactly how the async site drifted
in the first place. _should_skip_same_provider_retry() now owns the rule and
its carve-out next to _TIMEOUT_NO_RETRY_TASKS; both sites call it.
Behavior-preserving: same clauses, same exception object, same outer guard.
f50b5bb0fa taught the sync retry site to keep the cheap same-provider retry
when a Codex stream dies inside the 60s no-progress window (zero output),
skipping straight to fallback only on a stall or hard-ceiling timeout. The
async site never got that carve-out, so after widening the skip to vision
(#97572) an async vision call on a stillborn stream would have jumped to
fallback where the sync path retries. Both sites now apply the same rule.
Adds the async twin of the vision-skip test and a no-progress-still-retries
guard for the async site.
Issue #54465 established that a same-provider retry after a full-budget
timeout costs a second whole `timeout` window before the fallback chain is
reached, doubling the user-visible stall, and that compression must not pay
it because it sits on a critical path. The guard added for that is spelled
`task == "compression"`, so vision — which sits on the interactive path —
still retries.
The cost is the same and the stall is more visible: the turn holding the
image cannot answer, and because turns are serialised the following user
messages queue behind it. Two sequential full-budget timeouts on an
unhealthy vision provider is a long stall for something the fallback chain
could have served immediately.
Replaces the string comparison at both retry sites (sync `call_llm` and
`async_call_llm`) with `_TIMEOUT_NO_RETRY_TASKS = {"compression", "vision"}`,
so the two paths cannot drift again. Behaviour is unchanged for every other
task: fast blips (a streaming-close or a 5xx) still retry, and only
full-budget timeouts on those two tasks skip straight to fallback.
Tests: vision now falls straight through to fallback with the primary tried
exactly once, and a non-critical task still gets its one same-provider
retry, so the change stays scoped. Reverting the source change fails the
vision test and leaves the scoping test green.
Not the same as #51513, which fixes five separate defects in the vision
fallback chain (capability detection, sync/async client misuse, geo-block
and RemoteProtocolError classification, and chain iteration). This is about
what happens before that chain is reached.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kshitijk4poor