Skip to content

fix(agent): adopt live compression continuation when flushing to a closed session - #86409

Open
kshitijk4poor wants to merge 3 commits into
NousResearch:mainfrom
kshitijk4poor:fix/82001-compression-adoption
Open

fix(agent): adopt live compression continuation when flushing to a closed session#86409
kshitijk4poor wants to merge 3 commits into
NousResearch:mainfrom
kshitijk4poor:fix/82001-compression-adoption

Conversation

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Summary

An agent turn that flushes its transcript against a session id that compression already closed now adopts the unique live continuation and persists there, instead of dying with session_persistence_failed and misleading "this is often a full disk" advice.

Root cause (#82001): WebUI/streaming clients resend the pre-compression session id every turn. SessionDB.append_messages_batch correctly fails closed with CompressionSessionClosedError, but the flush chokepoint in run_agent.py swallowed it as a generic write failure — so every follow-up turn died until the client reloaded, and the error copy blamed the disk.

Changes

  • run_agent.py _flush_messages_to_session_db_unlocked: catch CompressionSessionClosedError, resolve the unique live continuation via the existing find_live_compression_child() (at most one probe per flush), rebind session_id, replay the failed batch once. 0 children (orphan window), >1 children (ambiguous lineage), or a second Closed error on the adopted child all fail closed exactly as before.
  • hermes_state.py classify_persistence_error: map CompressionSessionClosedError (by type and by RPC-wrapped string) to the compression cause, so the fail-closed path's turn-end explanation names compression rotation instead of disk advice. The existing compression wording already fits ("send it again after compression completes").
  • tests/run_agent/test_flush_compression_adoption.py: 7 tests — adopt-unique-child, fail-closed no-child, fail-closed ambiguous, adoption budget bounded to one probe (child closed mid-replay), next-flush-writes-directly-to-child, normal-flush untouched, cause classification.

Validation

Scenario (real AIAgent + real SessionDB, temp HERMES_HOME) main this PR
Stale-id flush after compression rotation False, cause unknown, rows lost from turn True, rows on live child
Follow-up turn on same agent fails every turn writes directly to adopted child
No/ambiguous continuation False, "full disk" advice False, honest compression wording

Targeted suites: tests/run_agent/ persistence/flush/compression selection 189 passed; tests/state/test_compression_lineage_guard.py 16 passed; new file 7 passed.

Scope note: this is part 1 from the issue (agent-side adoption). Part 2 (returning the adopted id through the SSE stream so the WebUI session index converges) is client-facing and remains a separate change, as the issue itself proposes.

Fixes #82001

Credit

Diagnosis, seam taxonomy, and the proposed fix design are from @Al3xand3r1987's exceptionally thorough report in #82001. PR #86099 attempted this area but was closed (see review there); this implements the minimal agent-side fix the issue describes.

…osed session

When context compression closes a session while a client still writes
against the old id (WebUI/streaming clients resend the pre-compression
session id every turn), the transcript flush swallowed
CompressionSessionClosedError as a generic write failure: the turn died
with session_persistence_failed and the user was told 'this is often a
full disk' even though the DB was healthy. The stale id reproduced the
failure on every follow-up turn until the client reloaded.

The flush chokepoint now catches CompressionSessionClosedError, resolves
the unique live continuation via find_live_compression_child (at most one
probe per flush), rebinds session_id, and replays the failed batch once.
Zero or multiple children — and a second Closed error on the adopted
child — still fail closed, but classify_persistence_error now maps the
closed-parent error to the 'compression' cause so the turn-end
explanation names compression rotation instead of disk advice.

Fixes NousResearch#82001

Reported-by: @Al3xand3r1987
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint area/compression Context compression and continuation sessions P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 14, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #79763: both repair direct compression-closed flushes by adopting a live child. #79763 performs the selection and append atomically, so it is the safer canonical implementation.

@Al3xand3r1987

Al3xand3r1987 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Quick data point from our side — no action needed for this PR.

We've been running the transitive variant (get_compression_tip, 100-hop bound, excluding branch/delegate/tool children) on both write paths — agent flush and gateway transcript flush — since Aug 8, without a recurrence. We retained the lineage walk over the depth-1 probe because there are now four independent multi-hop cases: our 2-hop repro, juliedarkheart's 2-hop WebUI-restart case, and 3-hop chains reported by gyu-bot and ayushnangia. In those cases, a depth-1 find_live_compression_child finds no live direct child and fails closed — honest error copy, but the turn still dies.

Not a criticism of this PR — the minimal fail-closed fix with honest copy is the right scope. This follow-up applies regardless of whether #86409 or #79763 ultimately lands: if multi-hop turns out to be common, promoting the resolver to the transitive tip would cover those chain cases too. I can share the concrete 3-hop regression test and repro if useful.

…id writers

Review follow-up on the NousResearch#82001 fix:

- Extract rebind_agent_compression_side_state() from
  _adopt_live_compression_child() and use it in the flush-time adoption,
  so both adoption paths move the session ContextVar/env, logging
  context, _flushed_db_message_session_id guard, and compressor/
  memory-manager boundary notifications together instead of the flush
  path rebinding only session_id. Load-specific state stays in the
  turn-start path, which is the only one that reloads messages.
- Dedupe the flush's two verbatim append_messages_batch calls into a
  local _append_batch(sid) closure so the retry can't drift from the
  primary call.
- Fix two sibling writers in the same bug class:
  - tui_gateway/server.py model-switch marker persisted to the stale
    session_key after rotation (silently dropped via debug-logged
    except); use agent.session_id like the NousResearch#20001 fix established.
  - gateway/shutdown_flush.recover_pending_to_db replayed spooled rows
    to the recorded id; a compression-closed id raised on every startup
    and retried the same file forever. New
    _append_with_compression_adoption() probes
    find_live_compression_child once and preserves the file on 0/>1
    children.
- Tests: 3 new spool-recovery/helper tests; turn-start adoption suites
  (233 selected) all green against the shared helper.
…on id

Updates the assertion pinned to the old stale-key behavior changed in the
previous commit (session_key -> agent.session_id, the NousResearch#20001/NousResearch#82001
bug class).
@ayushnangia

Copy link
Copy Markdown
Contributor

Verified against the #82001 contract on your branch: the adoption goes through _adopt_live_compression_child in hermes_state (which wraps the canonical chain walk — get_compression_tip appears 6x there), fails closed on ambiguous lineage, and covers the flush chokepoint plus the spool-replay and shutdown-flush surfaces — which is broader than the thread's original two call sites, good. Ran the new suites: test_flush_compression_adoption + test_spool_recovery_compression_adoption11 passed, 0 failed. One lineage note for the record: teknium1's #86666 already closed the read path with the same resolver; this closes the write/flush side, which was the last non-chain-aware consumer from the #82001 forensics. The verified thread patch (@Al3xand3r1987 + @yflmq001's repro) matches the semantics here — happy to cross-link it for credit if useful.

@alt-glitch alt-glitch added comp/gateway Gateway runner, session dispatch, delivery and removed duplicate This issue or pull request already exists labels Aug 15, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #86666 and #79763 rather than a duplicate: this diff adds compression-closed adoption for shutdown-spool replay, while its direct-child agent-flush path is narrower than #86666's merged transitive resolver.

@alt-glitch alt-glitch added comp/tui Terminal UI (ui-tui/ + tui_gateway/) area/sessions Session lifecycle, resume, persistence, history labels Aug 15, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(agent): adopt live compression continuation when flushing to a closed session

Good fix: extracting rebind_agent_compression_side_state so turn-start adoption and flush-time adoption share one code path is the right structure, and the fail-closed behavior on 0/>1 children is well tested. A few observations:

  1. run_agent.py — the replay-once adoption assumes append_messages_batch is all-or-nothing: if the first _append_batch(self.session_id) partially persisted rows before raising CompressionSessionClosedError, the replay against the child would duplicate those rows. The comment implies the batch is atomic, but nothing in the tests asserts the invariant. Worth confirming append_messages_batch commits atomically (or adding a test that a mid-batch failure leaves no partial rows).

  2. rebind_agent_compression_side_state is shared by two paths with different contracts — the turn-start path reloads messages and sets _last_flushed_db_idx/_flushed_db_message_ids, the flush path deliberately does not (documented in the docstring). There's no test pinning the shared helper's side-effect contract (only _adopt_live_compression_child is exercised), so the two paths could drift silently. A small test asserting rebind does not touch the flush counters/last-flushed state would lock the contract.

  3. tui_gateway/server.py:4088getattr(agent, "session_id", None) or session_key silently falls back to the stale session_key if agent.session_id is ever unset/empty at that point, which is exactly the bug class this PR fixes. Safe, but consider logging when the fallback is taken so a regression in the ordering (marker written before agent.session_id is updated) is visible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/compression Context compression and continuation sessions area/sessions Session lifecycle, resume, persistence, history comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/gateway Gateway runner, session dispatch, delivery comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Agent flush does not adopt live continuation after compression — turns die with misleading "full disk" dialog

5 participants