Stop the permanent ~7% core burn from re-fingerprinting state.db every 5s
and spinning on 0.1s stop-checks while nobody is listening. Block on
Event.wait for the poll interval, and sleep on _idle_wake until the first
subscriber arrives (maint #3035).
Co-authored-by: Cursor <cursoragent@cursor.com>
The #6366 suppression was too broad and could destroy data in two ways,
both verified with full `_apply_core_sync_or_error_marker()` probes:
1. A repeated prompt could be silently discarded. Checkpoint matching
truncates timestamps to integer seconds and ignored
`_active_turn_token`, so a prior identical prompt from a *different*
stream could match the new pending turn — and the suppression branch
then cleared the only durable copy of the prompt, with no recovered
row and no error marker.
Identity is now bound to the pending stream's exact active-turn token
(`_transcript_user_row_is_pending_turn`). Integer-second timestamp
equality never overrides a token mismatch, and a row that carries no
token cannot be proven to be this turn. Legacy sessions with no token
on either side fall back to the strict content + timestamp + source +
attachments checkpoint.
2. Interrupted tool-running turns were treated as completed. Any
unflagged assistant row counted as settled, including empty rows,
tool-call-only rows, interim-progress rows and compaction rows, so
`:3600` bypassed journal replay and the interruption marker.
Completion now reuses the established final-answer semantics
(`_pending_turn_has_final_assistant_answer` ->
`_assistant_message_has_final_visible_text`) inside the matched
user-turn boundary, rejecting tool-only, empty, interim, journal-
recovered and compaction tails.
The suppression is deliberately asymmetric: a false negative leaves a
visible cosmetic duplicate the user can ignore, while a false positive
silently destroys a prompt or a response. Whenever identity or
completion cannot be positively proven, recovery runs instead of
suppression.
Tests: the 11 predicate unit cases are re-derived against the new
identity/completion helpers, plus full-recovery-path cases that exercise
`_apply_core_sync_or_error_marker()` itself — distinct same-second
repeated turns (token-bearing and legacy) proving the prompt survives,
and empty / tool-call-only / interim / compaction tails proving journal
replay and the interruption marker still run — plus a positive control
for the suppression branch itself.
Metering telemetry is journaled at ~10 Hz during active turns, so marathon
reasoning runs balloon the durable run journal (12.4 MB of a single 18 MB run
was metering). On a mid-run refresh the reconnect path replays the entire
journal as one unpaced SSE burst, OOM-crashing the browser tab in a
crash-refresh loop while the server stays healthy.
- Write time: RunJournalWriter.append_sse_event() skips metering rows via
REPLAY_SKIPPED_SSE_EVENTS, returning None so caller journal-id plumbing is
untouched and remaining journaled seqs stay contiguous.
- Replay time: _replay_run_journal() and emit_replay() filter metering rows
through journal_replay_visible(), so legacy journals never stream the
backlog either.
- Readers (read_run_events / read_session_run_events) deliberately keep every
row: cursor math and the offline-gap coverage check count seqs.
Refs #7272
The ~10 Hz reasoning throttle clears the coalescing buffer before emitting
but left _reasoning_buffer_index holding offsets for text that was just
removed. Subsequent _strip_reasoning_output_echo cut_to() calls then used
raw offsets that no longer correspond to the buffer, so the live-echo strip
truncated the buffer at a phantom position and falsely marked the interim
as reasoning_echo: true — leaking a truncated copy into both the SSE stream
and the durable journal.
Fix: call _reasoning_buffer_index.reset() immediately after zeroing the
buffer in the timed-flush branch, matching the flush/_flush_reasoning_buffer
paths that already keep the folded index in step with the raw text.
Adds tests/test_7569_timed_flush_index_reset.py pinning the call site:
- revert this fix and both tests fail (verified via revert + re-run).
The journal replay matches through _CompactEchoIndex, so
_find_compact_echo_suffix_start and _strip_compact_echo_suffix are no
longer referenced in api/routes.py. The helpers stay in api/streaming.py
as the equivalence oracle the tests compare against.
Review follow-up for #7569: change 4 of the live-snapshot perf PR replaced
one quadratic with another. The window-free backward walk is correct, but it
re-walks the raw reasoning transcript's whitespace once per interim event —
on a whitespace-heavy transcript that span is megabytes, and the replay went
~55x slower than master on the production shape the gate measured
(1MB / 20-interim journal: 0.035s master → 1.922s branch).
## Thinking Path
The gate's finding was that the cost is structural, not a tweak: the matcher
consults the *whole* accumulated transcript on every interim event, so any
raw-text rescan of it is O(interim_count x whitespace_span). The fix is the
data-structure change the review asked for — maintain the folded view and the
raw cut offsets incrementally, and match/strip from that indexed state
instead of rescanning the raw buffer.
## What Changed
- `api/streaming.py`: new `_CompactEchoIndex` — an append-only folded index
that records, per non-whitespace character, its raw index. `append` folds a
chunk once (cost proportional to the chunk, never the buffer),
`matches_tail`/`cut_to` compare only the candidate against the folded tail
(O(len(candidate)), no raw walk), `reset` drops the state after the raw
text is truncated. The old raw helpers (`_find_compact_echo_suffix_start`,
`_strip_compact_echo_suffix`) stay as the equivalence oracle.
- `api/routes.py::_run_journal_live_snapshot`: reasoning chunks feed the
index as they are appended; `reasoning_echo_tail_matches` and
`strip_reasoning_echo_tail` consult the index. The strip re-indexes the
truncated transcript so later probes match against it.
- `api/streaming.py` live path: the same swap for the live-stream echo strip.
The three live reasoning buffers (STREAM_REASONING_TEXT, the coalescing
buffer, the per-message segments) each carry a mirrored index, fed on every
append and re-indexed after a strip. The buffer flush resets the buffer's
index with the text it drops, so the index never describes text that is no
longer there.
- `docs/rfcs/webui-run-state-consistency-contract.md`: the echo-match clause
now names the indexed representation and the scaling regression that pins
it.
## Why It Matters
Removing one quadratic by introducing another in the same hot path is the
exact risk the gate flagged — the headline benchmark used a journal shape
where the window-free walk's cost does not dominate, so the aggregate number
still looked like a win. The index keeps the original 8.9x and adds the
scaling guarantee on the shape that was regressing.
## Verification
- Measured on the reproduced shape (1MB whitespace span, 20 interim events):
0.972s → 0.049s. Doubling both span and interim count scales ~2x per step
(x1 0.041s / x2 0.090s / x4 0.175s) — linear, not quadratic.
- Behaviour equivalence: 300 randomized journals (whitespace-heavy,
Unicode, chunked, flagged/unflagged interims) rebuilt with the indexed
implementation and with the raw walk swapped in — sha256 of the canonical
snapshot JSON matched on all 300.
- `tests/test_compact_echo_index.py` (new, 6 tests): oracle equivalence over
2000+ randomized cases, chunked-append equivalence, matches_tail/cut_to
agreement, reset-after-truncation, and two cost probes that fail the raw
walk.
- `tests/test_live_snapshot_echo_scan_scaling.py` (new, 4 tests): the
proportional-scaling regression the review asked for — verified to FAIL on
the pre-fix matcher (14.6x time for 4x rows, the quadratic signature) and
pass on the indexed one.
- Existing echo/journal suites unchanged and green: 19 items across
`test_compact_echo_suffix_linear_scan` / `test_live_snapshot_echo_dedup` /
`test_live_snapshot_single_parse`, plus 66 neighbouring items
(`test_issue1361_cancel_data_loss`, `test_issue2565_reasoning_accumulation`,
`test_issue2713_streaming_segment_flush`, `test_cancel_interrupt`,
`test_run_journal`).
## Contract Change
None. The matcher's observable behaviour is unchanged (same cut points, same
whitespace-insensitive semantics); only its cost profile changes. No config
knobs, no API surface.
## Risks
- Index drift: if a future writer mutates one of the reasoning buffers
without feeding (or resetting) its index, the matcher would silently miss
echoes. Mitigated by keeping the index feed adjacent to every existing
write site and by the reset-on-flush/reset-on-strip symmetry; the
equivalence test would catch a dropped feed as a missed echo.
- Memory: the index stores one char plus one int per non-whitespace
character. On a whitespace-heavy transcript this is bounded by the folded
length, which is what the transcript would cost folded anyway; the raw
text is still the larger object.
## Model Used
Hermes Agent (step-5-preview) with direct file edits and pytest verification.
Cost properties were measured with a purpose-built scaling probe (proportional
span x interim count), not a fixed-size fixture; the probe is kept as a
regression test rather than a script.
## Related
#7569 (this PR), #7310 (slow /api/session under load), #7534 (incremental
journal read cache — complementary), #7273/#7291 (metering write-side skip).
Review follow-up: the bounded echo probe folded at most len(text)*3 raw characters, so a compact-equivalent suffix whose raw span was stretched by interior whitespace was missed - the echo strip was dropped and the interim text was duplicated in the restored transcript. The same retired window existed in the reverse helper (_strip_compact_echo_suffix), pinned by its own test.
Both now share _find_compact_echo_suffix_start: a backward walk over value/suffix that skips whitespace in value, inspects only the echo span, and allocates nothing. It restores the previous output semantics (origin/master folded the whole reasoning text) and is cheaper than the windowed version (no per-call tail fold).
Tests: new journal-reconstruction regression (a 5000-space-stretched echo must appear exactly once; genuine interim text must not be treated as an echo), the suffix-helper oracle updated to window-free semantics, and the window-contract test now pins that a stretched echo IS stripped.
`_run_journal_live_snapshot` rebuilds the live UI state for an active run
by replaying the whole durable journal. On a long run (22.7k events /
15MB) that rebuild took ~4.4s per session load and blocked the HTTP
thread - the dominant cost of "switching to a session with a task
running takes forever". Four output-preserving fixes:
1. Locate the journal without parsing it (`find_run_file`) and derive the
durable summary from the single `read_run_events` parse instead of
parsing the file twice (`find_run_summary` + `read_run_events`). The
historical `find_run_summary` lookup is kept as the fallback when no
journal file exists on disk.
2. Skip `metering` rows in the projection loop (~60% of a long live
journal's bytes; nothing in the snapshot is built from them), while
preserving the last_ts watermark they carry.
3. Collect reasoning deltas into a list and materialize lazily instead of
`reasoning_text += chunk` (quadratic inside these closures: ~2.3s of
the replay on a 9.7k-chunk run).
4. Fold only a bounded tail of the reasoning transcript for the
interim-echo suffix check instead of `re.sub` over the whole growing
text (~1.5s over 100+ probes).
Measured on a 15MB / 22.7k-row live journal: 4.34s -> 0.49s (8.9x) with
byte-identical snapshot output (sha256 of the canonical JSON unchanged).
Related work: #7310 (slow /api/session under load), #7534 (incremental
journal read cache - complementary: this removes one of the two parses
entirely and fixes the replay-loop hotspots), #7273/#7291 (metering
write-side skip - the read-side loop skip here complements them for
legacy journals that still carry metering rows).
The cache-only Steer path only special-cased backend == "gateway" and sent
every other value — including a missing, empty, or unknown backend — to the
cached local agent. Another runtime could own the run while guidance was
enqueued on a stale local agent.
Fail closed instead: the in-process worker now registers its active run with
the explicit local backend tag (WEBUI_LOCAL_CHAT_BACKEND, the same "legacy"
value webui_chat_backend_mode() returns), and cache-only Steer only enqueues
agent.steer() on that tag. Gateway keeps its gateway_steer_queued outcome; a
missing, empty, or foreign backend returns stream_dead without steering.
Extends the positive-ownership regression with backend cases and adds a
worker-side check that the local backend tag is actually registered.
The cache-only Steer branch (no AGENT_INSTANCES worker) only rejected
*conflicting* ownership, so a missing stream owner or a missing
active-run session_id was treated as non-conflicting and let
agent.steer() run and report accepted without proving the requesting
session owns the run.
Fail closed instead: both the stream owner and the active-run session
must be present and strictly equal to the requesting session (backend
and phase are still revalidated) under the same STREAMS_LOCK ->
ACTIVE_RUNS_LOCK edge; any absent identity returns stream_dead without
calling agent.steer(). No cache eviction, no agent closing, HTTP write
still outside the locks.
Regressions: missing owner, missing/empty run session, both missing,
missing run entry -> stream_dead + steer never called; both present and
equal -> accepted, cache untouched. Legacy happy-path fixtures now
register the ownership they rely on.
The cache-only compatibility path selected the matching cached agent,
checked stream liveness under STREAMS_LOCK, released the lock, and only
then called agent.steer(). A Stop running in that gap published
phase='cancelling' and detached STREAMS/AGENT_INSTANCES while the
earlier Steer still enqueued into the cached worker, so the HTTP
response reported accepted guidance that cancellation already owned.
Revalidate stream membership, owner, and active-run
session/backend/phase and call agent.steer() under the same
STREAMS_LOCK -> ACTIVE_RUNS_LOCK edge Stop uses; interrupt,
persistence, and HTTP writes stay outside the lock. Cache eviction and
agent closing remain absent.
Extend tests/test_steer_cancel_ordering.py with a cache-only reverse-
order test (AGENT_INSTANCES absent, matching cached worker, decoy cache
entry) gating both unlocked edges around the stream-lock reacquire.
Stop-first must return stream_dead without calling agent.steer(); the
selected cache object is asserted to be the intended worker. Proven
red on the pre-fix code (order ['cancel','steer']) and green after.
Stale-cancel recovery in _apply_core_sync_or_error_marker
(line ~3497+) only inspects the transcript tail
(`session.messages[-1]`) when checking whether the durable
transcript already contains the pending turn. When the pending
turn's user row is older than the tail, the tail is a *newer*
settled assistant row that can never match the pending **user**
checkpoint, and the recovery path falls through into the append
branch. The cancel/recovery tail then appends, after the valid
final answer, a recovered user row, a `_partial` clone of the
old journal, and a generic no-response error that survives reload
in the durable transcript.
The fix adds `_transcript_already_advanced_past_pending()`
which scans the messages for any strictly-matching user row
(content + timestamp + source + attachments) and, when one is
found, looks for a later settled assistant row (no
`_partial` / `_error` / `_recovered`). When that is the
case, the helper returns True and the non-completed-journal
branch clears only the stale pending fields and returns
without appending any rows.
Strict match is required so that a *new* user turn that happens
to repeat the same prompt text (e.g. user re-sends the same
question) is not misclassified as a stale advance. The
regression guard is the existing
`test_repeated_pending_prompt_uses_current_checkpoint_identity`
which now passes alongside the new helper unit cases.
The helper is a pure read; running recovery twice therefore
produces the same outcome, satisfying the maintainer's
idempotence requirement.
Validation: 11/11 new tests pass (covers the strict-match
distinction, the partial/error/recovered skip, and idempotence);
124/124 related stale-pending / sidecar-repair / recovery tests
pass; py_compile clean.
CLI_VISIBLE_SESSION_LIMIT is hardcoded to 20. That constant also bounds
how many delegated subagent children can render at once, since a child
only nests when its parent row is in the same payload, so operators
running wide delegate_task fan-outs cannot see all children without
editing the source.
Read it from HERMES_WEBUI_VISIBLE_SESSION_LIMIT via the existing
_env_int helper, keeping 20 as the default.
## Thinking Path
The Agent's OpenAI Codex Responses adapter persists user-visible
assistant progress in `codex_message_items` with `phase:
"commentary"` while the outer `content` field is empty. The Agent
already stamps these items with `role: assistant` and a normalized
phase (`agent/codex_responses_adapter.py::_message_item` and
`_OutputScan._message`), and the rendering lane is owned by #6828.
The WebUI's two consumers of those persisted rows, however, only
read the outer `content` field:
- `api/routes.py::_session_media_token_allows_path` flattens
`message.get("content")` through `_message_content_text()` and
scans it for `MEDIA:` tokens.
- `api/media_snapshots.py::annotate_media_snapshots` does the
same when annotating the settled transcript so previews can
survive an in-place overwrite of the source file.
For a safe image emitted only inside commentary, the browser
request hits the real `_handle_media` route, owns the correct
session_id and exact path, but is denied 403 by the auth predicate
because it cannot see the assistant's persisted `MEDIA:` token.
Symmetrically, the snapshot annotator never captures commentary
artifacts, so a subsequent in-place overwrite rewrites history
without a recoverable snapshot.
The fix is the maintainer's narrow recommendation: one shared
fail-closed public-assistant-text iterator in
`api/media_snapshots.py` (`codex_commentary_text`), consumed by
both the auth predicate and the snapshot annotator. Keeping the
extraction in one neutral helper means the auth and capture paths
cannot drift apart on a future change. The fail-closed predicate
is a strict chain — outer role is `assistant`; item type is
`message`; item role is `assistant`; item phase normalizes to
`commentary`; content is a list; the part is textual `output_text`
with a string `text` value. Every step that fails returns the safe
empty string, so the helper is safe to concatenate with the
existing top-level extraction. User rows, user sidecars, reasoning
items, analysis/final phases, malformed parts, non-string
`text` values, and arbitrary nested fields are all skipped.
## What Changed
- **`api/media_snapshots.py::codex_commentary_text`** — new
module-level helper that returns the concatenation of all
public-text `output_text` parts inside assistant-owned
`codex_message_items` with `phase: "commentary"`. The outer
row role, item type, item role, item phase, content shape, and
part type are all gated, so the helper is fail-closed and
safe to concatenate with the existing top-level text extraction.
- **`api/routes.py::_session_media_token_allows_path`** — inside
the existing per-message loop, the per-row `text` is now the
concatenation of `_message_content_text(message.get("content"))`
and the new `codex_commentary_text(message)`. The exact-path,
owning-session, safe-MIME, URL-rejection, hard-denied, and
symlink guards are unchanged. The commentary helper is imported
lazily inside the loop so this file does not pick up a new
top-level dependency.
- **`api/media_snapshots.py::annotate_media_snapshots`** — the
per-row `text` is now the concatenation of the existing
top-level string `content` and the new commentary helper
output. The existing `media_re.findall()` and resolve/allow
paths are unchanged, so otherwise-capture-eligible commentary
previews are now snapshotted at settle time. The string
pre-filter on `"MEDIA:" in text` is kept so the helper
short-circuits before regex extraction when neither source
contains a token.
- **`tests/test_issue7565_codex_commentary_media.py`** — new
regression module with eleven tests: a positive
valid-shape test, six fail-closed tests (user outer role,
wrong item type, wrong item role, wrong phase, non-output_text
parts, non-string output_text text), a case-insensitive
commentary-phase test, a malformed-input test, and two
integration static tests pinning that the auth predicate and
the snapshot annotator both consume the new helper.
## Why
- A user-visible artifact that the agent emitted only inside
commentary was returning 403 at the real `_handle_media` route
boundary even with the correct owning session_id, and the
artifact was never snapshotted at settle time. The fix restores
parity with top-level public assistant content for the
commentary phase specifically.
- Symmetry between auth and capture matters: a token that auth
admits must be one capture can stamp, otherwise the served
bytes can desynchronize from the snapshot pointer and the
in-place-overwrite survivor feature (#6922) silently breaks.
Sharing the helper makes drift impossible.
## Verification
```
$ .venv/bin/pytest \
tests/test_issue7565_codex_commentary_media.py \
tests/test_media_inline.py \
tests/test_media_message_snapshots.py \
-q --no-header
…
============================= 115 passed in 6.16s ==============================
```
Eleven new tests cover the helper contract and the integration
points. The 104 surrounding media_inline / media_message_snapshots
tests stay green.
## Contract
| Outer role | Item shape | Helper result | Auth result | Snapshot result |
| --- | --- | --- | --- | --- |
| `assistant` | `{type:"message", role:"assistant", phase:"commentary", content:[{type:"output_text", text:"…MEDIA:…"}]}` | text surfaced | `MEDIA:` token found; existing exact-path / safe-MIME / owning-session guards apply | `MEDIA:` token snapshotted at settle time |
| `assistant` | `{type:"message", role:"assistant", phase:"analysis", content:[{type:"output_text", text:"…"}]}` | empty | unchanged from existing top-level-only behavior | unchanged |
| `assistant` | `{type:"message", role:"assistant", phase:"final", content:[{type:"output_text", text:"…"}]}` | empty | unchanged | unchanged |
| `assistant` | `{type:"reasoning", …}` | empty | unchanged | unchanged |
| `assistant` | item role `user` | empty | unchanged | unchanged |
| `assistant` | item content not a list | empty | unchanged | unchanged |
| `assistant` | part type `tool_call` / `image` / `input_text` (even with text containing `MEDIA:…`) | empty | unchanged | unchanged |
| `assistant` | `output_text` with non-string `text` (dict / list / number) | empty | unchanged | unchanged |
| `user` (outer role) | any sidecar | empty | unchanged (user rows already skipped) | unchanged |
| `assistant` | no `codex_message_items` | empty | unchanged | unchanged |
| malformed (`None`, `""`, list, string, missing role) | n/a | empty | unchanged | unchanged |
The fix only widens the inspection surface for the one
public-text sidecar shape; everything else stays exactly as it
was, so existing safe-MIME, hard-denied, exact-path-equality,
owning-session, URL-rejection, and symlink guards remain intact.
## Risks
- **Commentary widening the inspection surface**: yes, by design.
The widening is narrowly scoped to the outer-assistant
/ item-message / item-role-assistant / phase-commentary /
part-output_text / string-text chain. The new tests pin every
step in that chain and every out-of-chain shape that must
remain rejected.
- **Existing safe-MIME / hard-denied / exact-path / symlink
guards weakened?** No. The `media_capture_allowed` predicate,
the `MIME_MAP` lookup, the `Path(ref).expanduser().resolve() ==
target_resolved` equality check, the `if "://" in ref` URL
rejection, the hard-denied state/secret paths, and the symlink
protections are all unchanged. The only change is what text
the regex sees.
- **Agent producer change**: the helper reads the same shape
the agent already stamps (`role: assistant`, `phase:
"commentary"`, `content: [{type: "output_text", text: "..."}]`).
A future producer change that adds a new phase or part type
requires an explicit follow-up PR; the helper's comments
document the precise chain.
- **Helper duplication drift risk avoided**: by placing the
helper in `api/media_snapshots.py` (the storage half) and
having `api/routes.py` import it lazily inside the loop, both
consumers go through one definition. A follow-up change to
the helper's contract automatically applies to both call sites.
## Model Used
- Generated with `MiniMax-M3` via `happy5318/hermes-webui` worktree
`/tmp/webui-worktrees/pr7565-codex-media`, branch
`fix/codex-commentary-media` (off `origin/master` via `ghfast`
mirror — `origin` was still TLS recv-failing at PR creation
time; the mirror was used for `git fetch` and `git worktree add`
only; push goes through the `fork` remote as usual).
## Related
- Closes#7565
- Surface code:
- `api/media_snapshots.py` (new) `codex_commentary_text` helper
- `api/routes.py::_session_media_token_allows_path` (modified
to concatenate the helper output)
- `api/media_snapshots.py::annotate_media_snapshots` (modified
to concatenate the helper output)
- Adjacent lanes (intentionally not folded in):
- #7294 / #7297 (safe-MIME, session_id, snapshot query
parameters) — independent contract
- #6828 (persisted-commentary rendering / recovery) — frontend
UI; the WebUI auth predicate does not assume or expand its
rendering scope
- Open PRs #7367 and #6923 — overlap the token-extraction sites
but neither implements this commentary ownership rule, per
the maintainer's open-PR search
- Test surface: new `tests/test_issue7565_codex_commentary_media.py`
## Thinking Path
The WebUI populates the editable schedule field from `schedule_display`,
which is the human-readable label `once at 2026-08-28 16:00` for one-shot
jobs. `saveCronForm()` then submits that exact string back to
`POST /api/crons/update`. The Agent's `cron.jobs.parse_schedule()`
rejects the display form with `ValueError` (it only accepts the canonical
ISO timestamp). The route's earlier `try/except ValueError` block
covered input normalization but not the `update_job()` call, so the
parser exception escaped as HTTP 500.
There are two independent failures to fix:
1. **Frontend** ships the unsafe display label as editable input. Both
`openCronEdit()` (`static/panels.js:1593`) and `duplicateCurrentCron()`
(`static/panels.js:1532`) use the same `schedule_display ||
schedule.expression` fallback. The cleanest fix is a shared helper
that returns the canonical Agent-parseable value by schedule kind,
matching the Agent's own schema:
- `once` → `schedule.run_at`
- recurring cron → `schedule.expr` (current schema; `expression`
accepted for legacy payloads)
- interval / legacy → the stored value, with `schedule_display`
kept as a final fallback only when it isn't the `once at …` label.
2. **Backend** converts `ValueError` from `update_job()` into HTTP 400
with the parser message, so the WebUI can show a real validation
toast instead of an opaque Internal Server Error. The 404 path
(job not found) is preserved.
## What Changed
- **`static/panels.js`** — add `_cronScheduleForEdit(job)` helper and
use it in both `openCronEdit()` and `duplicateCurrentCron()`. The
helper returns `schedule.run_at` for one-shot jobs,
`schedule.expr` (or legacy `schedule.expression`) for recurring
cron, and falls back to `schedule_display` only when it is not the
`once at …` presentation form. `saveCronForm()` is unchanged — it
still submits `schEl.value`, which is now the canonical field
instead of the display label.
- **`api/routes.py::_handle_cron_update`** — wrap the `update_job()`
call in `try/except ValueError` and return HTTP 400 with the parser
message. The earlier normalization try block stays untouched; the
404 check (`if not job`) sits outside the new try block so a missing
job still returns 404.
- **`tests/test_cron_edit_round_trip.py`** — new regression module
with seven tests covering the helper contract (helper exists, both
call sites use it, `saveCronForm` still submits the field by name),
the route's 400-on-`ValueError` behavior, the preserved 404 path,
and the happy-path 200.
## Why
- Editing a one-shot cron job's scheduled time has been returning
HTTP 500 since the WebUI's initial public release (per #7352
historical note), because the editable field round-trips the
`once at …` display label straight to the Agent parser.
- The backend regression is independent: even with a correct
frontend, any user-typed invalid schedule text (or a programmatic
caller) used to produce a 500 instead of a validation response.
Both halves are required to deliver a "no more 500" user
experience.
## Verification
```
$ .venv/bin/pytest \
tests/test_cron_edit_round_trip.py \
tests/test_cron_no_agent_edit.py \
tests/test_cron_model_override.py \
tests/test_cron_model_provider_picker.py \
tests/test_issue617_cron_profile_selector.py \
-v --no-header
…
============================== 27 passed in 4.84s ==============================
```
Seven new tests cover both layers; twenty surrounding cron tests
(no-agent edit, model override, model/provider picker, profile
selector) stay green.
## Contract
| Scenario | Before | After |
| --- | --- | --- |
| Open one-shot job for edit, change time, save | 500 (display label round-trips to parser) | 200, persisted with new `run_at` |
| Open recurring cron job for edit | 200 (legacy `schedule.expression` chain) | 200, unchanged canonical field (`schedule.expr` first, `expression` as legacy fallback) |
| Duplicate a one-shot job | 500 on first save | 200, populated with `schedule.run_at` from the source job |
| Submit a `once at …` schedule value (programmatic / pre-fix frontend) | 500 | 400 with the parser message |
| Submit any other unparseable schedule | 500 | 400 with the parser message |
| Update a job that does not exist | 404 | 404 (preserved) |
| Update a job with a valid canonical schedule | 200 | 200 (preserved) |
## Risks
- **Frontend helper change**: I switched the editable field source
for both edit and duplicate in one commit. `saveCronForm()` is
untouched, so anything already rendered with the legacy chain
(rare in practice — the helper is only called from these two
functions) keeps working. The regression `test_save_cron_form_still_submits_schedule_value`
pins the submit field name.
- **New `try/except ValueError` in `_handle_cron_update`**: the
earlier normalization try block (lines 24986–24998) is left
exactly as-is. The new try block sits directly around
`update_job()` only, so a `ValueError` from input parsing still
surfaces before any persistence side effect. The 404 check is
outside the new try, so a missing-job path cannot be misreported
as 400. Both `test_cron_update_preserves_404_when_job_missing` and
`test_cron_update_valid_schedule_still_succeeds` pin those
contracts.
- **Legacy `schedule.expression` payloads**: accepted as a
back-compat fallback in `_cronScheduleForEdit` and in the route
(the Agent schema is `expr`, but the parser still handles
`expression` per the existing contract). If you prefer to drop
the legacy fallback, the helper can be trimmed to `sched.expr`
only.
## Model Used
- Generated with `MiniMax-M3` via `happy5318/hermes-webui` worktree
`/tmp/webui-worktrees/pr7352-cron-edit`, branch
`fix/cron-edit-round-trip` (off `origin/master`).
## Related
- Closes#7352
- Surface code:
- `static/panels.js:1510` `duplicateCurrentCron` (uses helper)
- `static/panels.js:1585` `openCronEdit` (uses helper)
- `static/panels.js:1905` `saveCronForm` (submit path, unchanged)
- `api/routes.py:24979` `_handle_cron_update` (new try/except)
- `cron/jobs.py:785` `parse_schedule` (rejects the display form;
Agent-side, not modified in this PR)
- `cron/jobs.py:2199` `update_job` (the call that raises)
- Test surface: new `tests/test_cron_edit_round_trip.py`
Own a private stdout descriptor before agent imports and serialize request log writes. Keep logging failures isolated from HTTP responses. Add successful chat/start subprocess delivery regressions and document header-latency semantics and cron ownership.
_cleanup_chat_start_launch_failure runs inside the launch-failure handler,
but the session re-resolve only caught KeyError. An unreadable or
undeserializable session store raises something else, and that escaped the
helper, replacing the worker-launch error the caller is about to report and
aborting the reset after the registries were already cleared.
Contain the resolution-and-reset so the helper cannot raise. The KeyError
branch keeps its meaning (deleted session, return without saving); anything
else is logged at debug like the existing save() failure path.
The webhook second-pass loop in _load_cli_sessions_uncached called
get_last_workspace(profile=_cli_profile) directly per row instead of the
_cli_workspace() closure the main/cron/kanban passes already use, so it
paid its own redundant file reads/is_dir() probe on every webhook row even
though the loop already sits inside the profile-home resolve cache scope.
Flagged in PR #7636 review (greptile): this is the one concrete gap in an
otherwise-correct fix.
Also adds an integration-level regression test proving the actual N+1 this
PR targets -- Session.__init__ constructed via
_state_projection_sidecar_metadata's Session.load_metadata_only() fallback
on a cold sidecar-metadata cache -- is deduped by
@profile_home_resolve_cache_scope() on _load_cli_sessions_uncached, with a
companion test showing the same fixture scales linearly (one filesystem
resolve per row) when the scope is bypassed. This directly answers the
review claim that the cache scope doesn't reach any Session construction:
it does, verified by counting real _safe_resolve() filesystem calls rather
than call counts on the memoizing wrapper alone.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The process-lifetime cache from the previous commit was flagged in PR
review (nesquena-hermes): Path.resolve() is a filesystem call, not a
pure function of process-startup constants. A profile-home symlink can
be retargeted while the long-running webui process stays up, and
_safe_resolve() deliberately falls back to the unresolved input after a
transient OSError/RuntimeError/ValueError -- a process-lifetime cache
would serve either a stale symlink target or a stale fallback forever.
Redesign: replace the module-level dict with a contextvars.ContextVar
that defaults to None (no active scope -> resolve fresh, exactly like
pre-PR behavior). profile_home_resolve_cache_scope() sets it to a fresh
dict for the duration of one call and resets it in a finally block, so
no cache entry survives past that call. It now wraps only
_load_cli_sessions_uncached as a decorator -- the actual N+1 hot loop
(hundreds of Session.__init__ calls per sidebar build, each resolving
the same profile argument). All other call sites of
_resolve_profile_home_param (confirmed via repo-wide grep: all internal
to api/workspace.py, none elsewhere) get fresh resolution on every call,
same as before this PR existed.
Rewrote tests/test_profile_home_resolve_caching.py to prove: (a)
repeated resolves within one scope still hit the cache once, (b) two
separate scopes (simulating two requests) do not share state -- a
symlink change between them is observed on the second scope, and (c)
calls outside any scope never cache at all.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_resolve_profile_home_param() called Path.resolve() on the profile's home
directory with no caching. The webui session list builds one Session per
CLI session row, and Session.__init__ calls this function once per row, so
with ~484 sessions on the same profile it redundantly re-resolved the
identical path hundreds of times per request. Individually cheap (~1ms),
but under host load this compounded into multi-second sidebar hangs,
confirmed via journalctl showing repeated 'Slow WebUI request still
running' warnings (5-17s) pinned to this exact call site.
The mapping from a profile argument to its home path is a pure function of
process-startup constants (_DEFAULT_HERMES_HOME / _INITIAL_HERMES_HOME)
that are resolved once at import time and never mutated afterwards, so
memoizing per profile argument for the life of the process is safe. Profile
creation only creates the directory at the already-deterministic path; it
does not change what path a profile argument resolves to.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The session-field half of _cleanup_chat_start_launch_failure mutated and
saved the passed-in session object without holding the per-session lock and
without re-resolving the canonical session. A concurrent successor turn
(same session_id, new stream_id) or a delete could be clobbered: pending
fields wiped or a deleted session resurrected via save().
Mirror the established #1533 pattern (routes.py:3077) and anchor-scene
write guard (routes.py:5140): hold _get_session_agent_lock, re-resolve with
get_session(), early-return on KeyError (session deleted) or when the
canonical no longer points at the failed stream, then reset pending fields
on the canonical object only.
Add deterministic race coverage: successor admitted during the
check-then-clear window keeps its pending fields and is not saved away, and
a deleted session is not resurrected by save(). Both tests fail against the
previous implementation.
The all-profiles sidebar scan walks every profile's state.db in one
request. When it lazily minted a "Webhooks" / "Cron Jobs" project for a
profile that had webhook/cron rows, ensure_webhook_project() and
ensure_cron_project() tagged the new entry with get_active_profile_name()
-- the UI-selected profile -- instead of the profile whose rows triggered
it. A webhook-only profile therefore leaked a "Webhooks" project onto
whichever profile was selected; deleting it did nothing because the next
sidebar poll re-minted it, and switching profiles minted one per profile.
ensure_webhook_project / ensure_cron_project / _profile_has_user_projects
now take an explicit profile (default: active profile), and the scan
passes _cli_profile through, so the opt-in gate and the tag both key on
the scanned profile. Existing stubs in tests updated to accept the kwargs.
Fixes the #7310 stall class: six frontend recovery paths (offline/bfcache
refreshSession, stream-end settle, cancel sync, /compress preflight, /retry,
/undo) issued bare GET /api/session reads that re-walked, re-redacted and
re-serialized the ENTIRE transcript (4-15s / 28MB on a 5k-row session,
#7625), and the same shape shipped the full session-level tool_calls list
because windowing was gated on msg_limit being present.
Changes:
- Server: add explicit msg_limit=all escape hatch; window session-level
tool_calls by actual truncation (length comparison) instead of the
presence of a limit parameter. The bare no-limit shape keeps its
full-transcript contract (pinned by
test_state_db_reconciliation_preserves_sidecar_order_when_timestamps_collide
and the run-journal snapshot test).
- Client: refreshSession / _restoreSettledSession / cancel sync /
/compress preflight / /retry / /undo now request the bounded tail
(msg_limit=30) and restore the _messages_truncated/_messages_offset paging
gate instead of hardcoding 'not truncated'; jump-to-start
(_ensureAllMessagesLoaded) and outline jump opt in to the full transcript
via msg_limit=all.
Tests: test_session_default_window.py covers helper semantics, per-call-site
bounded/all shapes, truncation-signal restoration, and a regression guard
that no bare GET /api/session fetch remains in static/. 454 related tests
pass; reverse-verified (reverting the fixes turns the tests red).
Review follow-ups on #7445 (nesquena-hermes CHANGES_REQUESTED + Greptile).
api/agent_sessions.py
- The read-only pre-aggregated `latest_messages` candidate ordering now
applies to every missing-index projection (default sidebar, gateway
watcher, cron/webhook/kanban views), not only `include_sources ==
("cron",)`. Since the listing no longer self-heals the index, the
correlated `MAX(mx.timestamp)` scan re-ran on every load: 9.86 s on a
5,000-session / 20,000-message DB vs 0.044 s after this change, with
identical selected rows and order. The indexed seek is unchanged.
- `state_db_file_uri()` / `state_db_readonly_uri()` replace
`Path.as_uri()`: a UNC `HERMES_HOME` produced `file://server/share/...`
and CPython's bundled SQLite (3.11-3.13 Windows builds, no
SQLITE_ALLOW_URI_AUTHORITY) rejected the authority, so every agent-backed
session vanished. The builder emits the empty-authority
`file:////server/share/state.db?mode=ro`, keeps `file:///C:/...` for
drive letters, strips `\\?\` / `\\?\UNC\` prefixes, quotes `?`/`#`, and
leaves POSIX output byte-identical. Strict `mode=ro`, no writable
fallback.
scripts/ensure_state_db_read_indexes.py
- `fcntl` / `msvcrt` are imported conditionally; the module imports and
the no-`--lock-file` path runs on native Windows. `--lock-file` uses
`flock` on POSIX, `msvcrt.locking(LK_NBLCK)` on Windows, and fails with
a clear RuntimeError when neither exists. The rw open reuses the
UNC-aware URI builder.
docs/troubleshooting.md
- Operator section: read-only contract, degraded missing-index listing,
drained maintenance prerequisites, lock behaviour, failure modes, UNC.
Tests: tests/test_pr7445_readonly_review_followups.py (19 tests, red
before this change: correlated-subquery plan, missing URI builder,
ImportError on fcntl). Query-plan contract via EXPLAIN QUERY PLAN — no
CORRELATED SCALAR SUBQUERY on missing-index listings, identical rows to
the indexed listing; platform-parametrised URI cases; maintenance module
imported with `sys.modules["fcntl"] = None`. test_issue2628 no longer
pins the cron-only gate; the flock-held test skips without fcntl.
Readers never upgrade to writers. open_state_db_readonly() opens
file:...?mode=ro only; a read-only open failure propagates instead of
silently retrying with a write-capable handle on the multi-GiB WAL
database the agent is streaming into. The session listing no longer
self-heals idx_messages_session with CREATE INDEX (a writer lock held for
minutes on a large messages table) — a missing index degrades to the
pre-aggregated path. Cron sidebar info, insights and deep health checks in
api/routes.py use the same read-only opener.
Watcher (api/gateway_watcher.py): _poll_once returns without touching the
database when nobody is subscribed, and forces a fresh full projection on
the next subscription.
Maintenance (scripts/ensure_state_db_read_indexes.py): the covering read
indexes (idx_messages_session, idx_messages_session_role with NOCASE,
idx_sessions_webui_fingerprint) are created only under --confirm-drained,
optionally holding an exclusive flock on --lock-file for deployments that
serialise agent turns on a lock file; existing indexes are verified for
shape/collation and coverage (EXPLAIN QUERY PLAN), never silently
replaced.
Tests: tests/test_state_db_readonly_readers.py (read-only failure never
retries writable; no poll without subscribers; listing creates no index;
index tool requires drain, is idempotent, works without a lock file and
refuses a held lock; cron sidebar read is mode=ro). Neighbouring tests
that asserted the retired writable fallback / self-heal writer
(test_issue3887_index_prime, test_issue5455_listing_readonly_connection,
test_issue5455_lineage_readonly_reads, test_issue2628_cli_sessions_perf,
test_issue1494_state_db_fd_leak) now assert the read-only contract;
test_issue3506 subscribes before polling.
Gate found two blocking defects in the new caches:
1. The large-string LRU was keyed on input text only. The installed
agent.redact registry can extend the secret matcher at runtime via
register_redaction_patterns() and exposes no policy generation or
invalidation hook, so a >16k string primed before a registration was
served with the new secret intact (reproduced against the installed
registry). Drop the large-string cache: strings above 16,384 chars run
the redactor uncached again. Regression primes a large string, registers
a runtime pattern on the real registry and proves the next
redact_session_data() response is redacted.
2. The prefilter LRU was count-bounded (8192) but its per-entry gate was a
character cap, which is not a byte bound: 16k four-byte code points is
~64 KiB per key, ~512 MiB at capacity, and the workload died with
MemoryError under 96 MiB of headroom. Gate each entry on
sys.getsizeof(text) <= 2048 so the retained-key ceiling is a hard
16 MiB. Regressions cover max-width Unicode, sensitive verdict pairs,
eviction past the cap, and a low-memory RLIMIT_AS subprocess.
Michael Snoyman