fix(desktop): bound the in-flight turn journal and stop rewriting it every 400ms - #80872
Closed
royalaid wants to merge 9 commits into
Closed
fix(desktop): bound the in-flight turn journal and stop rewriting it every 400ms#80872royalaid wants to merge 9 commits into
royalaid wants to merge 9 commits into
Conversation
Production-buildable ring-buffer capture: LoAF observer core extracted from the dev-only profiler, live IPC arm/disarm, per-flush stream_delta_applied events reusing the existing commit-cost rAF measurement, record-time sanitization (numbers/counts/durations/IDs only). Zero observers when disarmed. 14 new tests; 94 total green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Root cause: a desktop app launched from inside a delegate_task child's terminal inherits HERMES_DELEGATED_CHILD_CONTEXT=1, poisoning the long-lived serve process — connect()'s migration pass hits the mutation guard and the dashboard event stream dies and reconnects every few seconds forever. Fix: server-role entrypoints (serve/dashboard/gui/desktop/gateway run) drop the inherited marker at startup; agent-role argv keeps it and real child and grandchild lineage is still rejected (guard assertion unchanged). The event stream now closes terminally (4004, once-per-process log) on PermissionError, and the dashboard client stops reconnecting on that code with a non-auth error string. Tests: 43 new across dashboard-plugin and delegate-isolation suites; 185-test regression sweep green. One pre-existing unrelated failure in test_kanban_attachments (Windows path mangling), verified identical with the change stashed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Capture controller with bounded main ring (unresponsive/responsive, app.getAppMetrics sampling while armed, sanitized hermes:api transport errors), renderer arm/disarm/collect IPC over the preload bridge with per-window timeout, and an authenticated gateway pull client (POST /api/diagnostics/arm|collect|disarm) that degrades to an absent stream on remote-gateway/unsupported/unauthenticated. 27 tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
While a capture is armed the CF-1 heartbeat tightens to 20Hz and records every loop-drift sample >=250ms into a bounded in-memory ring, with an off-loop watchdog thread attaching a sanitized frame summary (module/ function/line) once per stall episode — closing the 5s blind spot that made the complaint-day hitch unattributable. ws-write-slow events join the ring with per-capture HMAC'd peer ids. Served over the existing authenticated /api/diagnostics/arm|disarm|collect routes (401 unauthenticated; 409 on mis-sequenced collect). Zero overhead disarmed. 23 new tests; 250-test regression sweep green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nose (U4) Stop & export writes a local bundle under userData: per-stream JSONL, manifest with basename-only process tree (rebuilt fields — argv can never pass through), and classification.json labeling the capture renderer/ gateway/IPC/memory/history-bound via threshold heuristics. Minimal Diagnostics settings section (start/stop/status/bundle path). New 'hermes debug diagnose' subcommand records the PID tree and an optional time-bounded WPR trace into a separate unsanitized directory labeled unsafe-to-share. Docs entry added. 48 electron + 18 python tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Real harness-launched run injects renderer long tasks (CDP busy loop) and gateway loop blocks (env-guarded test route, absent unless HERMES_DIAGNOSTICS_TEST_HOOKS=1 at registration) and asserts the exported bundle's classification: renderer-bound, gateway-bound at 6s AND at 3x ~0.6s sub-threshold blocks (CF-1 blind spot closed), and combined multi-label. 5/5 cases pass; 33 gateway tests green. Scenario pins the spawned backend to this checkout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ures
A two-thread capture showed sessions:1 on every stream_delta_applied event
while the long-frame rate doubled: ordering-sensitive gateway events
(tool.start/complete, message.interim/complete, moa, error) drain the delta
queue eagerly, outside the instrumented timer flush, so the second thread's
apply cost was invisible — and per-event commits (tool-row upserts, subagent
progress, session.info patches, terminal chunks) never touch the queue at
all.
Instrument both gaps:
- stream_delta_applied gains path ('eager'|'timer') and busySessions (turns
in flight at record time — counts tool-only turns the queue never sees);
eager drains are now recorded with their drain cost.
- new gateway_event_applied event times every gateway-event dispatch and
records type tag + duration (>=4ms floor) + busySessions, covering the
unqueued commit families by construction.
Sanitization unchanged: counts, durations, and type tags only. Disarmed
cost stays one boolean test per flush/event. Classification heuristics
untouched — eager rows flow into the existing rendererStreamDeltas count.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…every 400ms A DevTools trace of live hitching showed ~2s of a 10s window inside the journal's timer: every 400ms per busy session it re-read and JSON-parsed the entire single-key store (~12MB on this profile — over localStorage quota, so writes were likely failing silently after paying full serialization cost), deep-cloned the unbounded turn tail with full tool bodies, re-stringified everything, and setItem'd it back — 250-400ms synchronous main-thread stalls, multiplied by concurrent streaming threads. Cross-checked by two independent external reviews; both confirmed the mechanism. Rewrite the storage layer, preserving recovery semantics: - Bounded schema-aware projection built before any clone: full user prompt (recovery matching needs it) + attachment refs, assistant text <=64KB/part, reasoning <=16KB/part, tool calls reduced to id/name/error + 2KB previews; 256KB hard entry budget enforced structurally, single stringify. - Write-only hot path to per-session keys (v2.<sessionId>): no read before write, no aggregate cache (multi-window coherence), per-session timers kept. - One-shot boot migration: project v1 entries onto v2 keys (new-key-wins), then delete the v1 blob unconditionally — including on parse failure. - Startup sweep replaces the per-write filter/sort/slice for the 7-day / 24-entry global bound. - Quota failures warned once per session and recorded, never thrown into streaming; kill switch key ...inflightTurnJournal.disabled=1 for A/B. - New journal_write diagnostics ring event (durationMs/bytes/outcome/ busySessions) so captures attribute journal cost directly. Tests: 5MB tool-result fixture bounds under budget with recovery intact, 100KB prompt kept whole, migration (normal/oversized/expired/garbage), new-key-wins, quota-throw resilience; all pre-existing recovery semantics tests unchanged and passing (47 journal+diagnostics, 442 session-hook). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alt-glitch
added
type/perf
Five review findings, each verified then fixed:
- previewOf ran JSON.stringify on object-valued tool args/results before
truncating, fully serializing multi-MB structures every tick. Replaced
with boundedJsonPreview: traversal and emission share one 2KB budget
(stops at the cap, slices giant strings pre-escape, depth 6, cycle-safe,
getter-throw-safe). Preview output was never parsed back — consumers
already try/catch with {} fallback.
- The 256KB entry budget was not re-checked after the tight re-projection;
a giant user prompt could still produce multi-MB writes. New 512KB hard
ceiling: still-over writes are skipped with outcome 'oversize' in the
journal_write ring event (prompt stays untruncated per the recovery-
matching contract).
- Recovery's getItem was outside any failure boundary; a storage read
error now degrades to "no journal" instead of breaking session resume.
- The 24-entry global cap was boot-only; a long-lived renderer can now
trigger one extra sweep when a new session key pushes the distinct
count past the cap (never on the per-tick hot path).
- The kill switch ran migration + sweep before disabling; ensureBooted now
returns right after the flag probe, making it the subsystem's only
storage access when disabled (legacy blob preserved for revert).
Tests: Proxy-counted 200k-element result proves bounded traversal (<1000
element reads); oversize-skip, guarded-read, runtime-cap, and kill-switch
no-op coverage. 663 tests across src/lib + src/diagnostics passing.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Follow-up from an external adversarial review — all five findings verified and fixed in the new head commit (
663 tests passing across |
Contributor
Author
|
Closing in favor of #82832. Both PRs address the same in-flight journal hot path, but #82832 provides the narrower and cleaner per-session persistence and recovery implementation. The broader diagnostics work from this branch can remain independent and build on top of #82832 rather than carrying a competing journal implementation. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The crash-recovery in-flight turn journal (
inflight-turn-journal.ts) was the dominant source of desktop UI hitching under streaming, especially with concurrent threads. While any session is busy, every 400ms per busy session it synchronously, on the renderer main thread:getItem+JSON.parseof the entire single-key store (all sessions' journaled turns, up to 24 entries kept 7 days),JSON.parse(JSON.stringify(...))— full tool-call bodies included,JSON.stringifyed everything andsetItemed the whole blob back.On a real profile the blob had grown to ~12MB (agentic turns journal full tool results; old sessions ride along for a week). A DevTools Performance trace of live hitching showed ~2s of a 10s window inside this timer: repeating ~260–310ms tasks ending in
setItem, ~1.9s of JSON serialization self-time. Cost multiplies per concurrent busy session — which is why multi-thread streaming felt so much worse. A 12MB value also exceeds typical localStorage quota, so writes were likely failing silently (errors are swallowed) after paying full serialization cost: all the jank, possibly no actual recovery.The rewrite (recovery/merge semantics preserved verbatim, all pre-existing tests unchanged and passing):
…journal.v2.<sessionId>): no read-before-write, no aggregate in-memory cache (multi-window coherence — secondary/peer windows share the storage partition). Reads happen only during recovery and the boot sweep.…inflightTurnJournal.disabled = '1') for A/B diagnosis; newjournal_writediagnostics ring event (durationMs/bytes/outcome/busySessions).Result
Verified before/after on the same machine and workload (two threads streaming):
localStorage.setItemself-timeThe design was cross-checked by two independent external model reviews before implementation (both confirmed the mechanism; the write-only no-cache design and the explicit legacy-blob policy come out of that review).
Related issues
Testing
vitest 24 journal tests (all pre-existing recovery-semantics tests unchanged) + new: 5MB tool-result fixture bounds under budget with tool identity and recovery merge intact; 100KB user prompt kept whole; migration (normal/oversized/expired/garbage v1 blobs, new-key-wins, unconditional v1 deletion); quota-throw resilience (streaming unaffected, warn once, later writes recover). Full session-hooks suite: 442 tests passing.
tscand eslint clean.