Adversarial review of the previous commit found a leak it introduced.
`attach_terminal()` subscribes and only then are the response headers written.
A client that dropped in that instant makes `end_headers()` raise
BrokenPipeError/ConnectionResetError, and because the `try/finally` started
*after* those writes, the exception escaped `_handle_terminal_output` with the
queue still in `_subscribers`. That pins `unwatched_since` at None — only
`unsubscribe()` on an empty subscriber list resets it — so the terminal became
permanently unreapable: the exact fd/thread leak (#4633) this reaper exists to
prevent, and strictly worse than the TOCTOU the previous commit fixed, since
that at least still allowed cleanup.
The try now starts immediately after the attach, so every exit path
unsubscribes. Regression test drives a handler whose `end_headers()` raises and
asserts the subscriber list is empty, `unwatched_since` is restored, and the
terminal is reapable again once the grace elapses. It fails against the
previous ordering.
`_terminals_to_reap()` snapshots victims under `_LOCK` and then releases it,
but `subscribe()` clears `unwatched_since` and appends to `_subscribers` under
the terminal's own `_sub_lock`, which the selection pass never held. A viewer
reconnecting in that gap left object identity — all `close_terminal(expected=…)`
ever checked — still true, so the reaper killed a terminal that now had a live
viewer.
- `_claim_reap_victim()` re-establishes the *whole* selection predicate while
holding `_LOCK` and the terminal's `_sub_lock`, and removes the registry
entry in that same critical section: same object, no subscribers, and
`unwatched_since` still set and still expired. A dead process is still
reaped unconditionally — it cannot come back, and a viewer watching a corpse
must not pin the entry forever.
- `attach_terminal()` does the registry lookup and the subscribe atomically
under `_LOCK`, replacing `get_terminal()` + `term.subscribe()` in the SSE
route. It takes the same two locks in the same order as the claim, so
exactly one side wins: the viewer attaches (and the reap is refused), or the
entry is already gone (and the route answers "terminal not running" before
committing a 200, instead of streaming from a corpse). Registration is the
authority — a registered-but-`closed` terminal still attaches so the viewer
receives `terminal_closed`.
- Teardown moved to `_teardown_terminal()` and runs after both locks are
released; `killpg`+`wait` can block ~2.5s and must not pin the registry.
- Dropped the duplicated `self.last_activity = time.time()` the rebase left in
`put_output()`.
Tests: a barrier regression that pauses the reaper after selection, attaches a
viewer through the production path, resumes, and asserts the terminal stays
registered/live and still receives output; the inverse where the reap claims
first and the attach reports not-running; refreshed-grace and replaced-entry
claims; dead-with-viewer; and a probe that `_LOCK` is free during teardown.
Three of them fail against an identity-only claim.
Render data:image URIs as inline images (raster + base64 SVG) instead of raw base64 text, route file:// images through the media pipeline, with a strict allowlist + 2MB cap and img-only data: sanitizer. Thanks @ai-ag2026.
Co-authored-by: ai-ag2026 <ai-ag2026@users.noreply.github.com>
1. inherit_errexit: the listener-diagnostic assignments in
_port_listener_diag abort status/stop when errexit is inherited into
command substitutions (shopt inherit_errexit or a BASHOPTS env from the
invoking shell). Guard both assignments with || true; regression test
runs status/stop under BASHOPTS=inherit_errexit.
2. PID/port AND: _pid_listens_on_port called lsof -p PID -iTCP:PORT
without -a, which OR-combines the selectors — any socket of the PID or
any listener on the port matched, so an active unit could be blamed for
a port its MainPID does not listen on. Add -a; the new fake lsof
mimics real OR/AND semantics so a missing -a fails the test.
3. Binding-aware collision: an active/activating unit whose ownership
could not be attributed via MainPID was assumed to own port 8787
unconditionally. Resolve the unit's configured binding first
(HERMES_WEBUI_PORT from Environment=, then --port from ExecStart=) and
refuse only on actual overlap; the default-port guard remains solely
for undeterminable bindings (#3291 semantics). Tests cover the reverse
alternate-port case (unit on 9999, start on 8787 proceeds) and the
overlap case (unit on the requested port refuses).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding 2 (10.07 gate): a deliberately locked config (0444) in a
writable directory was silently overwritten — atomic replace creates a
fresh temp inode and renames over the read-only file, defeating the old
in-place Path.write_text PermissionError contract.
Probe the existing target with a non-truncating O_WRONLY open before any
replacement work and let the PermissionError propagate. The probe fstat()s
the fd it actually opened and hands that stat to the rest of the write, so
a concurrent writer replacing the inode between stat and probe refreshes
the metadata instead of failing (keeps concurrent-writer semantics).
Regression: writable parent + 0444 target now raises and keeps the
original bytes and mode.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Four fixes from the Greptile review on #5944:
- Bracket IPv6 literals in the probe target ('::1' -> '[::1]') so the
URL-based responder checks don't silently miss a running instance.
- Force direct connections in the local ownership probes: --noproxy '*'
(curl) / --no-proxy (wget) in _port_answers_http, and neutralized
proxy env around the startup-grace health probe — a configured
http(s)_proxy would report the proxy instead of the port.
- Clamp HERMES_WEBUI_START_GRACE=0 to the default: a zero window would
skip startup monitoring entirely and restore the stale-PID behavior
the window exists to prevent.
- stop: warn about an unmanaged instance BEFORE deleting the state
file — it carries the saved host/port binding the probe needs when
the instance was started off-default.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ctl.sh start only guarded against launchd (macOS). On Linux, with a
systemd-supervised WebUI serving the port and a stale PID file, stop
reported 'stopped', start spawned a bootstrap that died ~2s later on
server.py's 'already responding' check — after the 0.15s aliveness gate
had already printed 'Started' and recorded the doomed PID. Killing the
foreign server by hand then put its supervisor's auto-restart into a
race with ctl.sh's start, ending in a permanent RestartSec crash loop.
- start: refuse when anything answers HTTP(S) on the target port (any
response bytes, matching server.py's abort semantics — a 404 squatter
still dooms our server), and when the hermes-webui systemd unit is
active on our port or mid-auto-restart (activating). Port scoping
mirrors the launchd #3291 over-block fix; overrides:
HERMES_WEBUI_CTL_ALLOW_SYSTEMD_CONFLICT / _ALLOW_PORT_CONFLICT,
unit name via HERMES_WEBUI_SYSTEMD_UNIT.
- start: watch the child through a startup grace window
(HERMES_WEBUI_START_GRACE, default 3s) — report failure and clean the
PID file when it dies during startup; break early once /health answers.
- status/stop: when ctl.sh owns no PID but the port answers, say
'running (not managed by ctl.sh)' with listener diagnostics instead of
'stopped', and never touch the foreign process.
- _pid_listens_on_port: ss fallback for Linux hosts without lsof.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
config.yaml and profile config.yaml were persisted with a plain
Path.write_text(), which truncates the target before writing. A crash
(or exception) after the truncate but before the full payload was
flushed left the live config truncated/corrupt, so the next agent/WebUI
start failed to parse it (availability regression).
Extract a shared api.paths._atomic_write_text() helper (tempfile in the
same dir -> write -> flush + os.fsync -> os.replace; unlink tmp on
error), mirroring the existing .env / cost-snapshot atomic pattern in
api.providers, and apply it to _save_yaml_config_file (api.config) and
the two profile model-config writers (api.profiles). On any mid-write
failure os.replace never runs, so the original file stays byte-for-byte
intact.
Preserve the target's permissions: tempfile.mkstemp() hard-codes 0600
and os.replace carries the temp file's mode onto the target, so without
an explicit chmod every save would silently tighten a group/other-
readable config.yaml (the homelab install ships 0644, profiles 0664)
down to owner-only. Copy the existing file's mode before the replace,
falling back to the umask-adjusted 0666 for a new file. config.yaml
holds no secrets, so that tightening would be a regression, not
hardening (unlike .env, which stays 0600 in api.providers).
settings.json is intentionally left untouched here.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both gateway SSE read loops used urlopen(timeout=600) and re-checked cancel only
BETWEEN lines, so a half-open gateway (TCP open, zero bytes) pinned the worker
for the full 10 minutes and ignored Stop.
Route raw-line iteration through _iter_sse_lines_cancellable and bound the socket
to a byte-silence budget (_gateway_read_timeout_secs, default 120s, env
HERMES_WEBUI_GATEWAY_READ_TIMEOUT). A read timeout is TERMINAL — CPython's
socket.makefile latches _timeout_occurred on the first socket.timeout, after
which every read raises a bare OSError, so the connection can't be resumed. On a
read timeout (or that poisoned-socket OSError) the generator surfaces the user's
Stop (yield b"" -> caller emits cancel) or re-raises so the caller reports the
stall. A stream that keeps emitting bytes within the window never trips it, so a
genuinely alive (if slow) turn is untouched; only >budget of total byte-silence
is treated as a dead gateway. Both loops (runs-API + legacy) changed identically.
The test fake now models the timeout-poison latch (a bare OSError after the first
timeout) so it reflects real socket semantics; the previous stateless re-raise
had made a multi-window "recovery" path look green that cannot occur in
production.
Refs #2476
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The per-token hot path mirrors partial text, reasoning, and live tool
calls into STREAM_PARTIAL_TEXT / STREAM_REASONING_TEXT /
STREAM_LIVE_TOOL_CALLS without holding STREAMS_LOCK, while snapshot
readers (_snapshot_and_append_partial_on_error, cancel_stream) read them
under STREAMS_LOCK. This is safe under the CPython GIL for two reasons —
NOT because `+=` is one bytecode (it is not: `d[k] += s` compiles to
load / add / STORE_SUBSCR):
1. Single writer — only the streaming thread ever writes a given
stream_id's buffers, so there is no writer/writer race to tear.
2. Reader/writer atomicity — `+=` builds a complete new immutable str,
then the final STORE_SUBSCR binding it into the dict is a single
atomic bytecode, so a concurrent reader's dict.get returns either
the old or new *complete* string (strings are immutable), never a
half-built one. list.append / single-key dict writes are atomic too.
So a reader sees a complete-but-possibly-stale value, never a torn one,
and the snapshot is a best-effort partial reconciled by later journal/SSE
events.
Chose documentation over locking: pulling the writes under STREAMS_LOCK
would add per-token contention against readers copying large buffers and
entangle the documented LOCK -> STREAMS_LOCK ordering (deadlock risk) for
only a recoverable staleness window. Make the assumption explicit at the
write sites and the reader instead. No behaviour change.
Rollback: revert this commit (comments only).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_probe_remote_gateway held _remote_probe_lock only around the cache
read and cache write; the network probe itself (_http_probe per path,
each up to ~2s) ran unlocked. On a cold cache a dashboard that fans out
to N panels fired N concurrent probe sets at the (possibly dead)
gateway, each blocking up to ~6s (#5455, #2476).
Wrap the existing lock in a Condition and add an in-flight set: the
first 'leader' thread marks the base_url in-flight and runs the probe
(via extracted _run_remote_probe), while latecomers wait on the
Condition for the leader's cached result instead of probing themselves.
The leader clears the marker and notify_all()s in a finally block, so
waiters are woken even if the probe raises (no deadlock); a bounded wait
budget lets a waiter fall back to probing itself if a leader hangs.
_reset_remote_probe_cache_for_tests also clears the in-flight set.
Expire the cache from the probe's COMPLETION time, not the leader's
entry time: walking every path of a hung gateway takes
len(paths) * timeout (~6s), which exceeds the 5s TTL, so
`entry_time + TTL` would store an already-expired cache line. Waiters
woken right after would then miss the cache and each re-probe the dead
gateway — collapsing single-flight and regressing latency to worse-than-
serial. A regression test drives a walk longer than the TTL and asserts
still exactly one probe set (the shipped happy-path test kept the walk
well under the TTL and so never exercised this).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
append_run_event() without an explicit seq called _next_seq(path), which
re-read and re-parsed the entire journal file on every append — O(n) per
append, O(n^2) over a run. Direct callers therefore paid a full file
reparse per event; only RunJournalWriter avoided it via its own in-memory
counter.
Introduce a module-level _SEQ_CACHE keyed by run-journal file path. The
per-path _lock_for(path) serializes same-path reserve->append so seqs
stay monotonic and file order matches; both RunJournalWriter and the free
append_run_event now draw from this one cache (_reserve_next_seq), so
seqs stay gapless even when both write the same path — the writer's
private counter is removed to keep a single source of truth. An explicit
caller-supplied seq advances the cache (_note_assigned_seq) so it can
never be re-issued. delete_run_journal() now also evicts cache entries
for the removed session so a run re-created at the same path restarts
numbering at 1.
Guard every structural _SEQ_CACHE access (reserve/note/evict) with a
dedicated _SEQ_CACHE_LOCK. The eviction sweep in delete_run_journal
iterates the whole dict; the per-path locks the append path holds do NOT
serialize against it, so a concurrent append on another session inserted
a fresh key mid-iteration and raised "dictionary changed size during
iteration" (the caller swallows it, but the eviction then aborts and
leaks stale seqs, so a re-created run resumed a stale number instead of
restarting at 1). A stress test races deletes against cross-session
appends and asserts no such error.
Rollback: revert this commit; _next_seq(path) re-read behaviour returns.
No on-disk format change, so existing journals stay compatible.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The connectivity probe schedules retries with setInterval(probe, 3000)
but tore the timer down with clearTimeout(retryTimer). Browsers share a
single numeric handle space between the timer families, so clearing an
interval handle via clearTimeout only worked by coincidence and is
semantically wrong. Match the teardown to how the timer was created.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The composer terminal's output EventSource registered handlers for
output/terminal_closed/terminal_error but none for the transport-level 'error'
event, so a session expiry, killed gateway, or network drop left the terminal
frozen with no feedback and no recordClientSSEError telemetry.
Add an 'error' handler behind the existing identity guard: let the browser
auto-reconnect a CONNECTING source (one-time "reconnecting" note, no manual
loop/backoff), and on a permanently CLOSED source surface a "disconnected" line
and dispose it (null TERMINAL_UI.source) so a restart can reconnect. Notify once
per outage to avoid flooding the pane/telemetry on a flapping connection. Emits
recordClientSSEError('terminal', {ready_state, reason}) like the chat/session
consumers.
UI-evidence (screenshot of terminal on gateway-kill) still needed before merge.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_drain_loop read process_registry.completion_queue directly under a bare
`except Exception: continue`. A registry missing that attribute raised
AttributeError that was swallowed and retried with no backoff — a
100%-CPU tight loop with no log line. The loop now reads the queue via
getattr(..., None) and backs off on the stop event when it is absent
(mirroring streaming.py), catches queue.Empty explicitly for the normal
idle path, and logs a warning + backs off (_DRAIN_STOP.wait(1.0)) on any
other queue error so a persistent fault can neither spin the CPU nor stay
invisible.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The passkey-login success handler wrote its 200 JSON body without a
Content-Length header, unlike the password-login and logout handlers. Under
HTTP/1.1 keep-alive that response is unframed, so the browser's fetch().json()
waits for connection close and appears to hang until it times out.
Encode the body once and send Content-Length before end_headers(), mirroring
the /api/auth/login block. Header order is unchanged so set_auth_cookie's
Set-Cookie still precedes end_headers().
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The streaming worker called meter().begin_session(stream_id) but never a
paired end_session. A turn that produced zero output tokens (pre-flight
cancel, or a setup exception before the first token) left a _SessionMeter
in GlobalMeter._sessions forever: get_stats() only prunes sessions with
first_token_ts > 0, so a zero-token session is never reclaimed, and each
leaked entry inflates the `active` count sent over the SSE metering event.
begin_session() and the metering ticker's .start() are now registered as
the first statements inside the worker's outer `try`, and paired with an
idempotent meter().end_session(stream_id, 0) plus a deterministic
_metering_stop.set() in that try's outer `finally` (the same block that
pops STREAMS/CANCEL_FLAGS) — so every exit path tears the session down.
Deferring .start() until after put() is defined also closes a latent
start-before-put ordering window in the ticker closure.
The metering payload is unchanged; end_session only pops the session.
Verification: tests/test_metering_session_lifecycle.py (leak reproduction,
end_session reclaim, idempotency, begin->cancel-before-token->empty) plus a
plain-assert run on python3.11 (conftest gates pytest to 3.11-3.13; system
python is 3.14). py_compile clean.
WARNING streaming-contract: adjusts the streaming worker's begin/end
teardown ordering. Needs RFC/contract review against docs/rfcs/
(session-sse-contract-v1.md) before merge — draft PR.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`_lock_for` caches one threading.Lock per (dir, file, pid) in the module-global
`_WRITER_LOCKS`, but nothing evicted those entries: `delete_run_journal` rmtree'd
the on-disk `_run_journal/{sid}/` directory yet left the cached lock objects
behind, so a long-lived gateway leaked one entry per deleted run forever.
Delete now evicts every cached lock whose parent directory matches the removed
session (pid-independent) under `_WRITER_LOCKS_GUARD`, leaving unrelated
sessions' locks intact.
Refs #4633, #2097.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
start_drain_thread and start_session_channel_reaper checked is_alive()
and then created + started the daemon thread without holding a lock, a
check-then-act race: two concurrent callers could both see "not alive"
and each spawn a thread. The loser's thread was never stored in the
module global and ran forever, un-joinable by the matching stop_*. Both
check-then-start sequences now run under a dedicated
_THREAD_LIFECYCLE_LOCK (kept separate from the purpose-bound
SESSION_CHANNELS_LOCK / _EMIT_COALESCE_LOCK), so exactly one thread is
ever created.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BG_TASK_COMPLETE_EVENTS_SEEN gained a session_id -> set[process_id] entry the
first time a bg task completed for a session and was never deleted anywhere, so
it grew unbounded for the server lifetime.
The entry is created in _process_one for EVERY completion, whether or not any
SSE channel/tab exists, so pruning it only when the SessionChannel is reaped
would miss the dominant headless case (task fires, tab closed or never opened —
no channel to collect). Instead the reaper now sweeps the map by DELIVERY: once
a completion is drained (its session_id removed from PENDING_BG_TASK_COMPLETIONS)
the short _move_to_finished dedup window is closed and the entry is swept, every
tick, regardless of any channel. The registry's per-process_id
_completion_consumed gate remains the primary idempotency backstop, so sweeping a
delivered session's set can never resurrect an already-delivered completion.
Session deletion also prunes the entry (new forget_bg_task_completion_dedup),
covering a session deleted while a completion is still pending (which the
delivery-gated sweep deliberately retains).
Refs #4633
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds a distinct two-tone attention sound (880->660Hz) for approval and
clarify prompts so they are not confused with the existing completion sound,
plus sidebar attention badges + colored rails driven by `attention` metadata
on /api/sessions. Includes lock-safety note: the in-lock
publish_session_list_changed() calls in clarify.py are safe because publish()
only takes the leaf _SESSION_EVENTS_LOCK and never re-acquires clarify._lock
(verified by Opus advisor review).
Co-authored-by: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com>
Cherry-pick of PR #2796 by @ai-ag2026, squashed from 5 author commits onto current master:
- dcee0563 fix: drop stale optimistic sidebar rows
- 3a73400d fix: clear stale busy state before send
- 46c3b902 fix: preserve server idle rows during optimistic merge
- de51d271 fix: let chat start survive pre-start UI errors
- d2f5c906 fix: hide nonfatal pre-start send warnings
Authorship preserved via --author. Code-only squash (no CHANGELOG).
ai-ag2026