fix(models): move anchor_activity_scenes out of the metadata prefix (#5854 backend / #4633) - #5858
nesquena-hermes wants to merge 1 commit into
Conversation
|
| Filename | Overview |
|---|---|
| api/models.py | Moves scene bodies out of the hot metadata prefix and updates freshness checks to use scene fingerprints without replacing cached full sessions on parity. |
| tests/test_issue5854_anchor_scene_split.py | Adds focused tests for serialization order, cheap metadata reads, metadata-only stubs, scene freshness checks, and legacy sidecar recovery. |
| CHANGELOG.md | Documents the session persistence memory-growth fix. |
Reviews (2): Last reviewed commit: "fix(models): move anchor_activity_scenes..." | Re-trigger Greptile
…5854, #4633) Reasoning-heavy sessions store 250-480KB of anchor_activity_scenes, which serialized BEFORE the messages array — so the cheap metadata-prefix read (load_metadata_only / _persisted_session_meta_prefix, 64KB cap) overflowed and forced a full multi-MB json.loads of the whole sidecar on every sidebar poll. That churn drove the glibc allocator high-water into the GB range (the #4633 RSS-climb-to-OOM crash). Persist a compact anchor_scene_index fingerprint ({scene_key: updated_at}) in the metadata prefix (after message_count, before messages) and the full scene bodies AFTER messages. The sidebar-poll freshness check reads the fingerprint; the full-session GET still reads full bodies. Cheap prefix drops from hundreds of KB to a few KB, eliminating the poll-path re-parse churn. Legacy sidecars (scenes-before-messages) are migrated lazily: rewritten in the modern layout on next save; a bounded stat-signature legacy-facts cache stores the authoritative message_count + fingerprint so an unchanged legacy file is full-parsed at most once (not every poll) and stays LRU-evictable. All cache writes are TOCTOU-guarded (expected_sig) so an atomic replace mid-parse can never stamp stale facts. Gated to convergence with Codex over 6 rounds (stale-fingerprint, legacy count-loss, re-parse churn, evictability, TOCTOU all found + fixed + regression- tested). Full suite 12522 passed; 17 targeted regression tests. Fixes the backend/server half of #5854. The browser render half (#5839) is a separate follow-up PR. Refs #4633, #5839
4a6dd13 to
e041659
Compare
nesquena
left a comment
There was a problem hiding this comment.
Review — end-to-end ✅ (clean approval; rebased onto master to clear a CHANGELOG conflict)
Independent review of the #4633 server-memory-growth root-cause fix (the backend half of #5854). This is on the crown-jewel session-serialization path, so I traced the on-disk layout change, round-trip fidelity, cross-tool safety, and every Codex-caught edge — and drove the real save/load with my own harness.
The bug
Reasoning-heavy sessions accumulate 250–480 KB of anchor_activity_scenes serialized before messages, so the 64 KB-capped metadata-prefix read overflowed and fell back to a full multi-MB json.loads on every sidebar poll → glibc high-water mark into the GB range → RSS 12–21 GB → OOM/segfault after hours.
What this ships
api/models.py (+~230), tests/test_issue5854_anchor_scene_split.py (+17 tests), CHANGELOG. Agent-authored (nesquena-hermes). Backend-only; scene render fidelity unchanged. The browser-freeze half (#5839) is a separate follow-up.
Housekeeping — rebased onto master
Branch was CONFLICTING on the CHANGELOG only; api/models.py merged cleanly. Force-pushed e041659e.
Cross-tool safety — verified
anchor_activity_scenes / anchor_scene_index / the metadata-prefix reader appear nowhere in the fresh hermes-agent tarball — this sidecar layout is WebUI-only. So reordering the JSON keys (scenes after messages) is safe: any full-parse reader is key-order-agnostic; only WebUI's prefix-cheap-read optimization cares, and it's updated in lockstep. ✓
The mechanism — traced + harnessed against real save/load
- Write (
Session.save): a compactanchor_scene_indexfingerprint ({scene_key: updated_at}) is placed aftermessage_count, beforemessages; full scene bodies serialize aftermessages/tool_calls. The_placedset prevents double-emit, and_anchor_scene_indexis_-prefixed so it's excluded from theextraspill. - Read (
_read_metadata_json_prefix): dual-stop atmessagesoranchor_activity_scenes, whichever first — modern files stop atmessages(small prefix), legacy files stop at the scenes key (still cheap, still captures the pre-scenesmessage_count).
My round-trip harness on a real 3-scene/80KB-body session (237 KB file):
| Check | Result |
|---|---|
Order: fingerprint before messages, bodies after |
✅ |
| Cheap prefix size | 237 KB → 1.86 KB (the #4633 churn eliminated) ✅ |
Prefix carries message_count + fingerprint keys, no bodies |
✅ |
| Metadata-only stub: fingerprint authoritative (keys + max updated_at) | ✅ |
| Full load: scene bodies byte-preserved | ✅ |
| Re-save: layout order stable (idempotent) | ✅ |
Correctness of the tricky invariants
- Fingerprint authority (Codex r1) is correctly scoped:
_session_scene_keys/_session_scene_updated_attrust_anchor_scene_indexonly when_loaded_metadata_onlyis True. A fully-loaded session reads real records, because in-place scene mutation staleness the load-time fingerprint._cached_session_lags_diskuses real records for the (always-full) cached side and the fingerprint for the disk prefix — the right asymmetry. ✓ - Legacy recovery (Codex r2/r3): a legacy file with no
message_countin the (scenes-stopped) prefix and no modernanchor_scene_indexrecovers authoritative facts via one full load, cached in a bounded stat-signature-keyed LRU so an unchanged legacy file full-parses at most once, not per poll. My legacy harness confirmed the dual-stop keeps the prefix tiny (0.05 KB) and the facts cache populates with the right count + scene keys; and for a realistic file (message_count == len(messages)) the cache-miss, cache-hit, and facts paths all agree. (The cache storeslen(messages)— the authoritative value if the persisted field ever lags.) ✓ - TOCTOU (Codex r4/r5):
_legacy_sidecar_facts_putrequires a mandatoryexpected_sigcaptured before the parse and commits only if the file's current signature still matches — an atomic replace mid-parse can never stamp stale facts. The metadata-only recovery deliberately does not re-cache after its own full-load (delegates toload()'s guarded write) to avoid the unguarded-second-write hazard. ✓
Edge-case matrix
| Scenario | Behavior |
|---|---|
| Modern large-scene session, sidebar poll | cheap 1.8 KB prefix, no body parse ✅ |
| Metadata-only stub freshness | fingerprint (keys + max updated_at) ✅ |
| Fully-loaded session, scenes mutated in place | real records, not stale fingerprint ✅ |
| Legacy large-scene sidecar, first poll | one full load → facts cached ✅ |
| Legacy sidecar, subsequent polls (unchanged) | facts cache hit, no re-parse ✅ |
| Legacy file atomically replaced mid-parse | facts not cached (sig mismatch) ✅ |
| Legacy → next save() | rewritten modern; fallback stops firing ✅ |
| Full load → re-save | scene bodies preserved, order stable ✅ |
| Genuine 0-message modern session | count stays 0 (prefix has it) ✅ |
Tests
tests/test_issue5854_anchor_scene_split.py— 17/17 (order, prefix size, stub correctness, full round-trip, all freshness directional cases, legacy count recovery, evictability, cache-miss, TOCTOU, modern/legacy fingerprint gating).- RED-on-master verified independently: 10 of 17 fail against master's
models.py. Load-bearing. - Independent round-trip + legacy harnesses (above) confirm the on-disk behavior directly.
- Full suite (post-rebase): 12368 passed / 0 failed in 282s (deselected the known darwin CRLF flake).
Minor observations (non-blocking)
- The legacy-facts cache stores
len(messages)while a first full-load reports the persistedmessage_countfield; these differ only for a corrupt file wheremessage_count ≠ len(messages)(never produced by any real writer).len(messages)is the safer of the two, so the resolution is correct; noting only for completeness.
Recommendation
✅ Approved. Parked at approval — ready for the release agent's merge/tag pipeline.
A genuinely well-engineered fix to the #4633 OOM: the hot metadata prefix drops ~130× (237 KB → 1.86 KB, harness-confirmed), scene bodies round-trip byte-for-byte, the fingerprint-vs-real-records authority is correctly gated on metadata-only stubs, and the legacy path is bounded, TOCTOU-guarded, and self-retiring on the next save. Cross-tool-safe (WebUI-only format). 17 tests, 10 RED on master; full suite clean. Ship.
…r freeze (#5839) A settled turn's activity worklog can carry 80+ rows (reasoning + tool steps). The compact-worklog render (_renderSettledAnchorSceneForMessage) built every row's DOM even when the worklog group was collapsed, so a long reasoning-heavy history accumulated tens of thousands of hidden nodes. A later synchronous layout read (e.g. opening the model/workspace dropdown calls getBoundingClientRect) forced a full layout over that oversized DOM and could push the tab into a multi-GB freeze — the #5839 report. Collapsed settled worklogs now defer building their row DOM until first expand: - _renderSettledAnchorSceneForMessage stashes the rows + marks the group data-worklog-rows-deferred and renders only the summary chip (which reads data-turn-duration, not the rows, so 'Processed Xm Ys' shows correctly). - _toggleActivityGroup materializes the rows once on expand. - The blank-turn safety reveal also materializes before force-expanding. - After an HTML-cache restore (innerHTML drops JS props), rows are recovered from the owning message via the disclosure key (anchor-scene:<rawIdx>); _rehydrateDeferredWorklogsFromCache re-stashes them on the fast-path restore. Verified in a browser against the reporter's real session: 3 collapsed worklogs build 0 rows (vs 2,143 nodes when expanded) — a 42% total-DOM reduction on that one session; expand materializes correctly (0 -> 52 nodes); switch-away-and-back (cache restore) still expands via message recovery. Desktop + mobile screenshots attached to the PR. 58 anchor-scene tests pass incl. 6 new regression tests. Fixes the browser render half of #5839. The backend server-crash half (#4633) is PR #5858. Transparent Stream mode (opt-in) already has per-row disclosure + expand/collapse-all; a node-count cap there is a possible follow-up. Refs #5839, #5854
Gate: SAFE TO SHIP ✅ (data-integrity verified by reproduction)Full authoritative gate (Codex reproduce + 2945-test session/serialization regression) on head Backward-compat + round-trip — the crux — verified with real data:
The fix works: the metadata prefix dropped from overflowing the 64 KB budget to 1,837 bytes for a reasoning-heavy session → modern reads need no full parse; a legacy file does one authoritative parse then serves from cache; an oversized header still correctly falls back to full parse. This is the #4633 server memory-growth root cause.
Gate-clean. Self-built backend crash/data-integrity fix on the session-persistence path → holding for independent review + maintainer sign-off before merge. |
…ver memory growth (#5858, #5854) (#5863) Reasoning-heavy sessions accumulate 250-480KB of anchor_activity_scenes serialized BEFORE the messages array, so the 64KB metadata-prefix read always overflowed -> full-parse fallback on every read -> memory growth. Move scene data AFTER messages; metadata prefix now ~2KB, modern reads skip the full parse. Old-layout files still load (one authoritative parse then cached); round-trip preserves scenes exactly. Independent review: nesquena APPROVED (end-to-end, on head). Gate-pass: Codex SAFE (933KB legacy sidecar loads clean + round-trip verified), 17 own + 2945 regression green. Co-authored-by: t <a@b> Co-authored-by: nesquena-hermes <nesquena-hermes@users.noreply.github.com>
|
Shipped in exp-v0.52.19 — independent review (nesquena) APPROVED + gate-pass (Codex reproduced a 933KB legacy sidecar loading clean + exact round-trip; metadata prefix now ~2KB), 17 own + 2945 regression tests green. Fixes the #4633 server memory-growth root cause. 🚀 |
…cap (#5974, #5966) (#5981) * fix(transparent-stream): defer settled tool-detail DOM + cap per-turn rows (#5966) Reported by @curtisszmania-cytocync. Transparent Stream built a DOM node for every activity event of every settled turn at load, and eagerly materialized each tool row's full detail body — a 15+ prompt reasoning-heavy history produced thousands of subtrees and tipped the tab into a multi-GB freeze. The #5860/#5839 lazy-worklog fix did not cover Transparent Stream (it never collapses). Two-part surgical fix in static/ui.js, mirroring #5860's proven machinery: - Row-detail deferral: a settled+collapsed transparent tool row renders header-only; its heavy .tool-card-detail body builds on first expand (_materializeTransparentToolDetail), recovering the tool call from the scene after an innerHTML cache round-trip. Same anchor-suppressed post-processing as the eager path, so an expanded row is byte-identical. - Per-turn row cap: a settled turn with >CAP+SLACK (30+10) rows renders the last 30 behind a clean 'Show earlier steps (N)' affordance that materializes the prefix in place with viewport compensation. Just-settled turn (keep-open token) and already-revealed turns are exempt; true Trace tool-count is stashed so the label stays honest; Expand-all reveals capped rows; cache-restore re-wires the affordance. Live rendering is untouched. Server-side #5858 (scene serialize-after-messages) already covers the container RSS half on the experimental channel; this is the browser-DOM half. * style(#5966): compact 'Show earlier steps' pill (hug content, left-aligned) Constrain the affordance to fit-content width + align-self:flex-start so it reads as a light inline control on the activity rail, not a full-width bar. * fix(#5966): address Codex gate findings + Fable UX fast-follows Codex SHIP-ONLY-WITH-FIXES (3 SILENT defects), all fixed + browser-verified: - F1: buildToolCard PRE-BUILDS .tool-card-detail when a tool has args/output, so the old `!detail` defer guard skipped exactly the heavy rows. Defer now fires on settled+collapsed+has-detail and STRIPS any prebuilt body (verified: 400 shell tool rows -> 0 detail bodies mounted; expand still materializes byte-identically). - F2: stamp owner rawIdx on rows + affordance; dataset recovery and cache-restore rebind resolve the scene-owning message (multi-segment turns own it on a later segment, not the first) with a scene-owning-segment fallback. - F3: persist revealed-turn state in a session/owner-keyed Set + invalidate the session HTML cache on reveal, so a rebuild/switch-away never re-caps a turn the user expanded (verified: reveal 30->400, full re-render stays 400). Fable SHIP-UX fast-follows: t()-with-fallback label (+ en locale keys), 640px touch breakpoint, stale comment fixed. Tests: 21 pass (added has-detail deferral+strip, detail-less no-defer, owner-idx recovery, persistent-reveal, i18n). * fix(#5966): address Codex re-gate findings (r2) Codex re-gate SHIP-ONLY-WITH-FIXES — 2 more SILENT defects, both fixed + browser-verified: - F1(r2): _materializeTransparentToolDetail rebuilt via the thinner _transparentToolDetailHtml(), dropping buildToolCard's richer detail (diff coloring / show-more / canonical shell-command lead). Now transplants buildToolCard(tc)'s own .tool-card-detail node so an expanded deferred row is byte-identical to the eager path (verified: canonical structure + 595-char body). - F2(r2): after an in-session renderMessages() rebuild (next send) an already-open settled tool row got re-deferred, and disclosure-restore only toggled .open → open-but-empty card. _setWorklogDetailDisclosureOpen now materializes a deferred transparent row BEFORE toggling open (verified: expand → rebuild → still open WITH content). Likely the same defect the RegressionGate caught intermittently. Tests: 22 pass (added canonical-detail + disclosure-restore-materialize). * fix(review): add show_earlier_steps i18n keys to all locales (#5966) The transparent-stream row-cap "Show N earlier steps" pill added two new i18n keys (show_earlier_steps, show_earlier_step_one) to the English block only, so the strict locale-parity tests (zh/ko/ru/zh-Hant/...) failed with missing-key assertions and CI shard 0 went red across 3.11/3.12/3.13. Add both keys to all 14 non-English locale blocks with the English string as fallback (the repo's established add-with-English-fallback convention for new UI keys). Inserted after each block's collapse_all anchor. All 197 locale tests + the #5966 suite green; node --check clean. * fix(review): extract _transparentToolRowHasDetail into the #5700 harness #5966 added a _transparentToolRowHasDetail() call into the settled transparent render (_anchorSceneTransparentNodeForRow) to decide whether to defer a collapsed tool row's detail body. The pre-existing #5700 timestamp test extracts that render fn into a node harness but didn't extract the new helper, so the extracted fn hit a ReferenceError → node exited non-zero → 3 #5700 tests failed (RED across CI shards; the PR updated its own #5966 harness but missed this one). Add `eval(extractFunc('_transparentToolRowHasDetail'))` to the harness. The helper self-guards its _toolActionKind/_toolCardAllowsDetail deps (returns true when absent), and the #5700 timestamp lives on the row HEADER (rendered even when detail is deferred), so all 20 #5700 assertions pass unchanged — confirming this was a harness gap, not a timestamp regression. * fix(#5974): hide Show-earlier-steps pill when a capped transparent turn is collapsed (Fable UX gate) Fable UX gate SHIP-WITH-UX-FIXES: the .transparent-earlier-steps pill wasn't in the .assistant-turn[data-transparent-turn-collapsed=1] hide rule, so collapsing a capped turn via the name-tag toggle left an orphan pill. Added it to the hide rule. (i18n finding was already resolved — both pill keys present in all 15 locales w/ English fallback + show_earlier_step_one is live at ui.js:12918, not dead.) * Release exp-v0.52.50: transparent-stream lazy settled DOM + per-turn cap (#5974, #5966) --------- Co-authored-by: nesquena-hermes <agent@nesquena-hermes> Co-authored-by: Nathan Esquenazi <nesquena@gmail.com>
Summary
Root-cause fix for the server memory-growth crash (#4633) — the backend half of #5854. Reasoning-heavy sessions accumulate 250–480 KB of
anchor_activity_scenes, which serialized before themessagesarray, so the cheap metadata-prefix read overflowed its 64 KB cap and fell back to a full multi-MBjson.loadsof the whole sidecar on every sidebar poll. That sustained allocation churn drove the glibc allocator high-water mark into the GB range and never returned it to the OS → RSS climbs to 12–21 GB → silent segfault/OOM after hours (@someaka confirmed the mechanism:MALLOC_ARENA_MAX=2cuts growth ~95%).Fix
Split the storage so scene bodies leave the hot metadata prefix:
Session.save): persist a compactanchor_scene_indexfingerprint ({scene_key: updated_at}) in the metadata prefix — placed aftermessage_count, beforemessages— and the fullanchor_activity_scenesbodies aftermessages/tool_calls.load_metadata_only/_persisted_session_meta_prefix): the cheap prefix now stays a few KB (fingerprint only), so the poll path never re-parses the bodies. The sidebar-poll freshness check (_cached_session_lags_disk) reads the fingerprint (it only ever needed scene keys + maxupdated_at); the full-session GET still reads full bodies.Measured: cheap prefix drops from ~740 KB → 1.8 KB on a 3-scene/80-row session; the poll-path re-parse churn is eliminated. Verified RED on master (its 64 KB-capped prefix read returns
None→ full parse).Legacy migration (lazy, safe)
Pre-#5854 sidecars serialize scenes before
messages. They're migrated lazily — rewritten in the modern layout on their nextsave(). In the meantime:_read_metadata_json_prefixgained a dual-stop (stop atmessagesoranchor_activity_scenes, whichever first) so a legacy large-scene file still yields a cheap prefix.message_count+ fingerprint after one full load, so an unchanged legacy file is full-parsed at most once (not every poll) and stays LRU-evictable.expected_sigcaptured before the parse) so an atomic sidecar replace mid-parse can never stamp stale facts under the new signature.Correctness / gating
Gated to convergence with Codex over 6 rounds — each round found a real, empirically-reproduced edge on this crown-jewel session-persistence path, each fixed with a regression test proven to fail against the buggy version:
_anchor_scene_indexon cached/fully-loaded sessions (spurious reload / cache-ahead data loss) → fingerprint used only on metadata-only stubs.message_countloss from a lagging_index.json→ authoritative full-load recovery.expected_sigguard + bounded stat-stable retry..venvstaging → removed;expected_sigmade mandatory.Final Codex verdict: SAFE TO SHIP — no regression risk found.
ruffclean,py_compileclean.tests/test_issue5854_anchor_scene_split.pycovering: serialization order, cheap-prefix size, stub correctness, full-load round-trip, all freshness directional cases, legacy count recovery, evictability, cache-miss, TOCTOU, and modern/legacy fingerprint gating.Backend-only; scene render fidelity is unchanged.
Scope note
This resolves the server crash (#4633). The browser freeze (#5839 — oversized settled-scene DOM) is a separate render-side workstream (lazy collapsed-worklog DOM + Transparent-Stream row expander) and will be a follow-up PR with screenshots.
This PR is self-built (nesquena-hermes) and touches the session-serialization path, so it should get an independent review before merge.
Refs #4633, #5839. Plan/investigation: #5854.