Steering a live gateway-owned run had no process-local agent in SESSION_AGENT_CACHE,
so it errored with "No agent cached" and dropped the typed text. Now detects the live
gateway ACTIVE_RUNS stream and queues the steer as the next turn (model/provider/profile
snapshotted first), updates the queue badge, clears the draft, shows a toast — without
cancelling the stream. Gateway sibling of the shipped #5597 fallback.
Codex SAFE + Fable SHIP-UX; byte-identical to PR head; 66 steer/busy-input tests pass;
Nathan visual-approved (desktop + mobile screenshots).
Co-authored-by: t <a@b>
Co-authored-by: rodboev <rodboev@users.noreply.github.com>
Phase 1 low-risk backend batch (on top of Batch A). Both byte-verified vs PR head,
Codex SAFE, 64 own + 749 regression tests pass, coexists with Batch A's #5833.
- #5828 (ai-ag2026): frame CORS OPTIONS preflight with Content-Length: 0 (keep-alive hang fix)
- #5834 (ai-ag2026): parse session _index.json from bytes not decoded str (all 10 sites)
Co-authored-by: t <a@b>
Co-authored-by: ai-ag2026 <ai-ag2026@users.noreply.github.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>
Three fixes for the CI failures (lint + test shard 1):
1. B009 lint fix (api/routes.py:6281): Replace getattr(session, 'profile')
with session.profile — the guard on line 6275 already ensures it exists.
2. B905 lint fix (api/routes.py:13843): Add strict=True to zip() of
_draft_stages slices. Both sides are always the same length (shifted
by one), so strict=True is correct.
3. _emit_slow format-arg fix (api/request_diagnostics.py:186-195):
Keep the prefix in the format string and pass the JSON payload as a
separate positional argument. The test at
test_issue1855_request_diagnostics.py:33 does
json.loads(caplog.records[0].args[0]) and the old code embedded the
prefix in the payload string, breaking the JSON parse. Now
self.logger.warning("%s %s", prefix, payload) — wait, actually
self.logger.warning(f"{prefix} %s", payload) — so args[0] is the
raw JSON string, same as the pre-_emit_slow code.
_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>
When cached session has no anchor_activity_scenes and the prefix reader
returns None (e.g. sidecar too large for prefix), the empty-scenes path
did not mark _scene_check_inconclusive = True. This meant an inactive
session with matching message counts would return False (cache fresh)
even though disk may have gained its first scene record.
Fix: the empty-scenes else-branch now sets _scene_check_inconclusive
when disk_meta_quick is None, matching the existing behavior in the
cached-scenes branch (line 3173). The code then falls through to the
full metadata comparison instead of serving a stale cache.
discussion_r3548650345
Two Greptile P1 comments from review of b5c0447a (tier3c):
1. Inconclusive Prefix Stays Fresh (discussion_r3548477859):
When _persisted_session_meta_prefix() returns None and cached has scene
records, the fast-path skipped scene comparison entirely and returned
False (cache fresh) for inactive equal-count sessions. Added
_scene_check_inconclusive flag that forces fall-through to the full
Session.load_metadata_only() comparison when the cheap prefix read
fails, preventing stale anchor_activity_scenes.
2. Prefix Check Misses Edits (discussion_r3548477915):
The 128-char content-prefix backstop in _read_profile_config_cached
only detected edits within the first 128 bytes of config.yaml. Replaced
with a full-content comparison that reads the entire file (~1-10KB)
and compares it to a copy stored in the cache entry. This detects
edits anywhere in the file at a fraction of the cost of yaml.safe_load().
Updated cache type hint from tuple[float, str|None, dict] to
tuple[float, str, dict].
Tests: 108 passed (41 reconciliation + 39 anchor-scene + 28 profile-config)
The fast-path scene comparison in _cached_session_lags_disk had two bugs
flagged by nesquena-hermes review:
1. Direction: used symmetric != instead of directional subset check.
When the cache had scene keys that disk lacked (cache-ahead), the old
code force-reloaded from disk, silently dropping un-persisted scene
data. Now mirrors master: reload only when disk is strictly ahead
(disk_keys.issubset(cached_keys)). The 'not disk_scenes' early-return
is also removed — empty disk scenes means disk is not ahead, so keep
the cache.
2. Key filtering mismatch: used raw set(disk_scenes.keys()) instead of
_anchor_scene_record_keys() helper which filters to truthy keys with
dict values. Now filters disk keys the same way as master, and uses
_anchor_scene_record_keys(cached) for the cached session object.
Also updated the tier3b content-prefix docstring to clarify it is a
best-effort backstop limited to the first 128 chars, not a full
integrity check.
Greptile P1 (discussion_r3548069113): the inode+mtime+size cache key
misses in-place rewrites on coarse filesystems. When a file is rewritten
in-place (same inode, same mtime tick, same byte size), the cache returns
the old parsed config for up to 60s.
Fix: on cache hit, read the first 128 characters of the file and compare
against the stored prefix from when the entry was populated. If they differ,
the content has changed — fall through to re-parse. Cost on hot path: one
128-char read + string comparison (negligible vs YAML parse).
_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>
Three unaddressed Greptile review comments fixed:
P1 - Empty scenes skip check (api/models.py):
When the cached session had no anchor_activity_scenes, the scene-comparison
branch was skipped entirely by the 'if cached_scenes:' guard. An inactive
session would return False (cache fresh) even when disk had gained its first
scene record. Add an else clause that checks disk for scene records when the
cache has none.
P2 - Draft slow log sink (api/routes.py):
The /api/session/draft handler's slow-path log (>200ms) used logger.warning,
which is silently dropped in the WebUI process (root logger has no handler).
Route through handler._safe_webui_print() instead, the same fix applied to
the /api/session slow log in tier2c.
P2 - Config cache same-timestamp stale (api/routes.py):
The profile config.yaml cache key (profile_name, mtime, size) could collide
on coarse-mtime filesystems (FAT, some network FS) when a config edit within
the same timestamp tick happened to produce the same byte count. Add st_ino
(inode) to the cache key — most editors use atomic-rename writes which assign
a new inode, so the key changes even when mtime+size are identical.
Greptile review caught that /api/session handler calls _diag.maybe_start()
at entry and _diag.finish() only on the success path. Three exit paths
leaked the watchdog entry:
1. early-return when sid is missing (400)
2. early-return when session is not visible to active profile (409)
3. early-return when session not found (404)
4. the existing 'except KeyError:' branch at the bottom of the handler
Without _diag.finish() the watchdog's _watchdog_pending dict held the request
until the 5s slow-request timeout fired _on_timeout and emitted a spurious
'Slow WebUI request still running' log. The pending dict also stayed
unbounded by in-flight count.
Fix: every early-return path now calls _diag.finish() before returning;
the except KeyError branch calls it once at entry so all paths through
it are covered. finish() is idempotent (no-ops if already called).
Greptile review caught that the Tier 1a fast-path returns 'cache fresh' when
disk_count == cached_count AND the session is inactive, but api/routes.py
saves the sidecar after editing only anchor_activity_scenes (no message
change). That path leaves the cached scene records stale until the cache is
otherwise invalidated.
Fix: when the cached session has scene records, also compare them against
the disk's scenes via the same metadata-prefix read that _persisted_message_count
already does. anchor_activity_scenes is in METADATA_FIELDS and serializes
before messages, so it lands in the prefix.
New _persisted_session_meta_prefix() helper exposes the parsed prefix dict;
_cached_session_lags_disk reads it and compares scene key sets + max
updated_at against the cached session.
Cost: one extra dict parse per cache hit on sessions that have scene
records, no cost on sessions without. Inactive sessions still take the
fast-path exit when scenes match.
Codex gate CORE: /api/updates/apply + /force ignored the request-body channel
and re-read the saved setting, so a channel switch whose debounced autosave
hadn't landed could apply the OLD channel. The banner now sends the channel the
CHECK reported for each target; the endpoints validate it against the enum and
thread it into apply_update/apply_force_update (None → saved-setting fallback,
preserving prior behavior). Agent stays channel-neutral server-side.
Tier 2c-followup added print_fn=handler._safe_webui_print to the
/api/session handler so its [SLOW] line and structured
'RequestDiagnostics' log actually land in the systemd journal (the
WebUI process boots with no root-logger handler, so logger.warning
calls are silently dropped — the per-request ms line has always
worked because it bypasses logging and writes via _safe_webui_print).
There are 4 OTHER call sites of RequestDiagnostics.maybe_start() in
api/routes.py (for /api/models, /api/sessions, /api/profiles, and the
generic POST dispatch). All four were wired with the same broken
logger.warning path, so their structured slow logs have been silent
in production as well.
This commit:
- passes print_fn=getattr(handler, '_safe_webui_print', None) to
the same 4 call sites
- keeps the existing stage() and finish() call shape intact
- the existing /api/sessions handler already has stage() markers
(load_settings, profile_lookups, response_write) so the [SLOW]
log on sidebar-list will now show real per-stage breakdown
No behavior change on the happy path. No public API change. The
allowlist still gates which paths get a diag at all; this only
fixes the sink for the structured log on the 4 already-instrumented
endpoints.
Tier 2c added per-stage instrumentation on /api/session and added
'GET /api/session' to the RequestDiagnostics allowlist, but the
structured log emitted by RequestDiagnostics.finish() and the
pre-existing [SLOW] line both call logger.warning() — which the
WebUI process silently drops because the root logger has no handler
configured at boot.
The per-request 'ms' line has worked all along because it goes
through handler._safe_webui_print(), which writes directly to the
systemd journal socket.
This commit:
- adds a print_fn= parameter to RequestDiagnostics.__init__ and
RequestDiagnostics.maybe_start. When provided, finish() and
_on_timeout() route through it instead of self.logger.warning.
- routes the [SLOW] line in routes.py through handler._safe_webui_print
instead of logger.warning.
- passes print_fn=handler._safe_webui_print from the /api/session
handler to RequestDiagnostics.maybe_start.
After this fix, every slow /api/session request (and the existing
[SLOW] line) actually lands in the journal — the very data Tier 2c
was supposed to produce. Without this, the allowlist + handler
instrumentation would be a no-op in production despite the code
being present.
Measured: the same 2.97s /api/session call (sid 295592fd560e,
m=1 ml=30) which produced a per-request ms line but no [SLOW] line
before this fix, now produces both lines with full per-stage breakdown.
No public API change. The new print_fn parameter is optional and
defaults to None, preserving the existing logger.warning behavior
for any caller that hasn't been updated.
_read_profile_model_config() fires on every chat-open that hits the
resolve_model=1 path (one of the 3 calls in the chat-open waterfall).
For sessions under a named profile (non-default), the function
performs:
- 1× os.path.isfile() call
- 1× open() of config.yaml
- 1× yaml.safe_load() of the full file
On a Celeron N3350 with eMMC the open+safe_load alone is single-digit
ms per call, repeated on every chat-open. The /api/session 24h log
showed ?messages=0&resolve_model=1 at p50 1514ms with this YAML
parse as one of the dominant costs.
This wraps the parse in a per-process cache:
- key = (profile_name, mtime, size)
- value = parsed dict + monotonic timestamp
- TTL 60s as a backstop for filesystem mtime resolution issues
- cap 32 entries (bounded; profiles are bounded in practice)
mtime+size auto-invalidates on edit. The TTL only fires when mtime
resolution is too coarse to detect a same-second write; in normal
operation, every config edit lands as a different mtime and the next
chat-open rebuilds the cache entry.
Threading: a single Lock guards the dict. yaml.safe_load is the slow
step, not the lock, so contention is bounded. The lock is held for
microseconds on hit and milliseconds on miss (parse).
Field-semantics check: the function's signature is unchanged
((provider, default, config_dict) tuple) and the only call sites
(_resolve_effective_session_model_for_display and its sibling)
treat it the same as before. The cache stores the parsed dict;
the function still does the per-call (provider, default) extraction
inline so the tuple's content is identical to the pre-patch path.
No public API change. No behavior change observable other than
faster chat-open for sessions under named profiles.
The /api/session handler already has _t0.._t6 local timers plus an
env-var-gated [SLOW] WARNING that logs the total wall time and the
_tN-to-_tN+1 deltas, but only when HERMES_DEBUG_SLOW is set or total
>= 2s. In production we cannot rely on the env var, and the 2s
threshold means the typical chat-open (sub-2s on Tier 1) never logs
at all — we only see the worst cases, never the marginal ones that
would tell us where the next bottleneck is.
This wires the handler into the existing RequestDiagnostics framework:
- add ('GET', '/api/session') to the allowlist
- call RequestDiagnostics.maybe_start() at the _t0 boundary
- call stage() at each of _t1.._t6
- call finish() before the response returns
The watchdog fires _on_timeout (with thread stack snapshot) when the
request crosses the per-path slow threshold, and finish() emits a
single structured log line on the same threshold crossing. The
existing env-var-gated [SLOW] log stays as a no-diag fallback.
Greptile P1 trap avoidance: previous allowlist additions for similar
endpoints were flagged as dead code because the handler was never
updated to call maybe_start/finish. Both halves are present in this
commit so the watchdog entry actually registers.
No behavior change observable on the happy path (diag is None for
non-allowlisted paths; the existing _tN log is the fallback for both
None-diag and slow cases). No public API change. The /api/sessions
and /api/profiles handlers already use this pattern.
Adds monotonic-clock stage markers around the existing
/api/session/draft hot path so we can decompose wall time by phase
(get_session, lock acquire, save, json emit). Logs a single WARNING
line only when total > 200ms — keeps the per-keystroke poll cheap
while still catching the slow cases that justify further work.
This is the source of the slow-path data needed to plan Tier 2:
without stage breakdowns on /api/session/draft, every subsequent
patch on this endpoint is a guess.
The existing _t0.._t6 pattern on /api/session (gated on
HERMES_DEBUG_SLOW) is intentionally NOT extended here — the env-var
gate requires a manual restart and is not useful for routine
production diagnosis. The new stage markers log via the standard
[webui] WARNING channel and require no env var.
No semantic change. No public API change. Logging only fires when
the request exceeds 200ms, so it is invisible on the happy path.
