Skip to content

fix(agent): reset flush cursor atomically during compression session rotation - #41057

Closed
rrevenanttt wants to merge 1 commit into
NousResearch:mainfrom
rrevenanttt:fix/compression-rotation-flush-cursor
Closed

fix(agent): reset flush cursor atomically during compression session rotation#41057
rrevenanttt wants to merge 1 commit into
NousResearch:mainfrom
rrevenanttt:fix/compression-rotation-flush-cursor

Conversation

@rrevenanttt

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a silent, permanent history-loss bug in the context-compression
session rotation. During a compaction rollover, compress_context
ends the old session, reassigns agent.session_id to a fresh id, and
only resets the SQLite flush cursor (_last_flushed_db_idx) at the very
end of the block — after the fallible create_session,
get_next_title_in_lineage, and update_system_prompt calls.

The severity here is high: this is data loss, not a cosmetic glitch.
If any of those DB steps raises (a transient SQLite lock or disk error —
precisely what happens when sessions are large and busy, which is exactly
when compaction fires), the bare except only logs a warning and falls
through. At that point session_id already points at the new session,
but the cursor still holds the pre-compression index (e.g. 50). The
compressed message list is far shorter (e.g. 3). The next
_persist_session passes conversation_history=None, so
flush_from = max(0, 50) = 50 and messages[50:] is empty — nothing is
written, and _ensure_db_session re-creates the new session empty. The
entire compressed conversation is gone from persisted/resumable state,
with no error surfaced to the user.

The fix makes the cursor reset atomic with the id rotation: it now runs
immediately after session_id is reassigned, before any fallible DB
call. Once the id rotates, 0 is the only correct cursor for the new
(empty) session, so even if a step below trips the handler, the next
persist recovers and writes the full compressed list. This is a safe,
narrowly-scoped change — it only moves an existing assignment earlier in
the same critical region and adds no new behavior on the success path.

Related Issue

N/A

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • agent/conversation_compression.py: move the agent._last_flushed_db_idx = 0
    reset to immediately after the session_id rotation (before the fallible
    create_session/title/update_system_prompt calls) so the flush cursor can
    never reference indices that no longer exist in the shortened compressed list.
  • tests/run_agent/test_compression_persistence.py: add
    TestRotationFlushCursorAtomicity, which drives the real compress_context
    with a transient create_session failure and asserts the cursor is reset to 0
    and the compressed messages are recovered on the next flush.

How to Test

  1. scripts/run_tests.sh tests/run_agent/test_compression_persistence.py — all 7 pass.
  2. Reproduce the bug: temporarily move the cursor reset back to the end of the
    rotation block and re-run the new test — it fails, showing the new session
    persists 0 of 3 compressed messages (session DB compression split failed … messages=50->3).
  3. Restore the fix: the new test passes and the new session persists all 3
    compressed messages.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (Darwin 25.5)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

…rotation

## What does this PR do?

Fixes a silent, permanent history-loss bug in the context-compression
session rotation. During a compaction rollover, `compress_context`
ends the old session, reassigns `agent.session_id` to a fresh id, and
only resets the SQLite flush cursor (`_last_flushed_db_idx`) at the very
end of the block — after the fallible `create_session`,
`get_next_title_in_lineage`, and `update_system_prompt` calls.

The severity here is high: this is data loss, not a cosmetic glitch.
If any of those DB steps raises (a transient SQLite lock or disk error —
precisely what happens when sessions are large and busy, which is exactly
when compaction fires), the bare `except` only logs a warning and falls
through. At that point `session_id` already points at the new session,
but the cursor still holds the *pre-compression* index (e.g. 50). The
compressed message list is far shorter (e.g. 3). The next
`_persist_session` passes `conversation_history=None`, so
`flush_from = max(0, 50) = 50` and `messages[50:]` is empty — nothing is
written, and `_ensure_db_session` re-creates the new session empty. The
entire compressed conversation is gone from persisted/resumable state,
with no error surfaced to the user.

The fix makes the cursor reset atomic with the id rotation: it now runs
immediately after `session_id` is reassigned, before any fallible DB
call. Once the id rotates, 0 is the only correct cursor for the new
(empty) session, so even if a step below trips the handler, the next
persist recovers and writes the full compressed list. This is a safe,
narrowly-scoped change — it only moves an existing assignment earlier in
the same critical region and adds no new behavior on the success path.

## Related Issue

N/A

## Type of Change

- [x] 🐛 Bug fix (non-breaking change that fixes an issue)

## Changes Made

- `agent/conversation_compression.py`: move the `agent._last_flushed_db_idx = 0`
  reset to immediately after the `session_id` rotation (before the fallible
  `create_session`/title/`update_system_prompt` calls) so the flush cursor can
  never reference indices that no longer exist in the shortened compressed list.
- `tests/run_agent/test_compression_persistence.py`: add
  `TestRotationFlushCursorAtomicity`, which drives the real `compress_context`
  with a transient `create_session` failure and asserts the cursor is reset to 0
  and the compressed messages are recovered on the next flush.

## How to Test

1. `scripts/run_tests.sh tests/run_agent/test_compression_persistence.py` — all 7 pass.
2. Reproduce the bug: temporarily move the cursor reset back to the end of the
   rotation block and re-run the new test — it fails, showing the new session
   persists 0 of 3 compressed messages (`session DB compression split failed …
   messages=50->3`).
3. Restore the fix: the new test passes and the new session persists all 3
   compressed messages.

## Checklist

### Code

- [x] I've read the Contributing Guide
- [x] My commit messages follow Conventional Commits (`fix(scope):`, `feat(scope):`, etc.)
- [x] I searched for existing PRs to make sure this isn't a duplicate
- [x] My PR contains **only** changes related to this fix/feature (no unrelated commits)
- [x] I've run `pytest tests/ -q` and all tests pass
- [x] I've added tests for my changes (required for bug fixes, strongly encouraged for features)
- [x] I've tested on my platform: macOS 15 (Darwin 25.5)

### Documentation & Housekeeping

- [x] I've updated relevant documentation (README, `docs/`, docstrings) — or N/A
- [x] I've updated `cli-config.yaml.example` if I added/changed config keys — or N/A
- [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture or workflows — or N/A
- [x] I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
- [x] I've updated tool descriptions/schemas if I changed tool behavior — or N/A

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Fixes data-loss bug in compression session rotation.

What changed

  • Moved _last_flushed_db_idx = 0 to before the fallible DB calls in _release_lock()
  • Previously: if create_session/update_system_prompt raised after session_id was rotated, the flush cursor stayed at the pre-compression value, causing all compressed messages to be silently dropped on next persist

Looks Good

  • Root cause is well-understood and the fix is minimal and correct
  • Regression test (test_flush_cursor_reset_when_rotation_db_call_fails) is excellent — simulates flaky SQLite, verifies cursor is 0 after partial failure, and confirms full recovery on retry
  • Comment in code explains the failure mode clearly

Reviewed by Hermes Agent

@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels Jun 7, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Looks Good

  • Moves agent._last_flushed_db_idx = 0 to immediately after the agent.session_id rotation, so a transient SQLite failure in create_session / update_system_prompt cannot leave a stale cursor pointing past the end of the compressed message list.
  • New test class TestRotationFlushCursorAtomicity drives exactly the failure mode: a flaky create_session that raises on the first attempt, then asserts the cursor is reset at 0 and the compressed messages are fully recovered on the next persist.
  • Regression documentation in the test docstring is clear: it explains the data-loss scenario, the trigger condition, and the post-fix recovery path.

Reviewed by Hermes Agent (batch cron)

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jun 21, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for this — the diagnosis and fix are correct for the rotation path.

Context on why we're closing it: as of #52658 (#38763), compression.in_place now defaults to True. Compaction keeps ONE durable session id for the conversation's whole life — it no longer ends the session and forks a continuation id. Because the session id is unchanged across compaction, the flush cursor / user_id stay attached to the same session — there is no rotation to reset or re-propagate them across.

Rotation now only runs as an explicit opt-out (compression.in_place: false), and the direction is to treat that path as legacy. So this fix hardens a code path that no longer executes by default and that we're not investing further in. Closing as superseded by the in-place default — not a reflection on the code, which was a correct fix for the behavior at the time. Credit preserved in the issue/PR history. Appreciate the contribution.

@teknium1 teknium1 closed this Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P1 High — major feature broken, no workaround sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

4 participants