Skip to content

fix(memory): avoid re-shipping retained turns on session switch - #88358

Closed
rafpigna wants to merge 1 commit into
NousResearch:mainfrom
rafpigna:fix/hindsight-retain-flush-on-shutdown
Closed

fix(memory): avoid re-shipping retained turns on session switch#88358
rafpigna wants to merge 1 commit into
NousResearch:mainfrom
rafpigna:fix/hindsight-retain-flush-on-shutdown

Conversation

@rafpigna

Copy link
Copy Markdown

What does this PR do?

Fixes a silent data-duplication bug in the Hindsight memory provider's
on_session_switch() flush path.

With retain_every_n_turns > 1 and an append-capable Hindsight API
(≥ 0.5.0), sync_turn() persists a delta at every Nth-turn boundary and
advances the append watermark. The on_session_switch() flush, however,
re-shipped the entire _session_turns buffer regardless of that
watermark. So once turns 1..N had already been retained at a boundary, a
later session switch (/new, /reset, /undo, context compression)
would append the whole buffer again into the same document — silently
duplicating already-retained turns.

The fix makes the switch flush respect the watermark: on append-capable
APIs it ships only the delta since the last boundary, on overwrite APIs it
re-sends the full session (each retain replaces the document there), and it
skips entirely when nothing new is buffered — so no already-retained turn
is ever re-shipped at switch time.

Related Issue

N/A — reported in this PR. (Note: several sibling PRs on the general
Hindsight turn-flush topic are open upstream — e.g. #77454, #55936,
#55046, #80503. Those target the shutdown/session-exit data-loss class;
this PR targets a distinct defect: the session-switch flush re-shipping
already-retained turns. None of the open PRs address this duplication, and
one (#55936) explicitly preserves the "switch sends all turns" behavior
that this PR corrects.)

Type of Change

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

Changes Made

  • plugins/memory/hindsight/__init__.py: in on_session_switch(), the
    buffered-turn flush now computes turns_since_retain from the append
    watermark, ships only the delta on append-capable APIs (full session on
    overwrite APIs), and no-ops when nothing new is buffered — instead of
    always re-shipping every buffered turn.
  • tests/plugins/memory/test_hindsight_provider.py: new
    TestSessionSwitchBufferFlushNoReship test — after a boundary retain
    persisted turns 1..N, a switch must flush only the partial block (N+1..),
    never re-shipping already-retained turns.
  • tests/agent/test_memory_session_switch.py: seeds the retain watermark
    (_last_retained_turn_count) in the bare-provider test helper, which
    on_session_switch() now reads.

How to Test

  1. Unit test (CI parity — do not call plain pytest):
    cd <repo> && source .venv/bin/activate
    scripts/run_tests.sh tests/plugins/memory/test_hindsight_provider.py -q
  2. To reproduce the bug directly: configure a Hindsight provider with
    retain_every_n_turns > 1 on an append-capable server, run past one
    retain boundary (so the watermark advances), buffer a few more turns,
    then call on_session_switch(). Before the fix the flush carries the
    whole buffer (duplicating turns 1..N in the document); after the fix it
    carries only the partial block.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix (no unrelated commits)
  • I've run the tests via scripts/run_tests.sh (whole tests/plugins/memory/ + tests/agent/test_memory_session_switch.py): 343 passed, 0 failed
  • I've added tests for my changes
  • I've tested on my platform: Arch Linux (CachyOS), Linux

Documentation & Housekeeping

  • I've updated relevant documentation — N/A (code-level fix, docstring updated in on_session_switch())
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) — N/A (pure Python, no platform-specific code)
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

Test run (canonical runner, reduced scope — the three prior shutdown tests
were intentionally dropped from this PR since that work is covered by
sibling open PRs):

=== Summary: 25 files, 343 tests passed, 0 failed, 1 skipped ===

AI Tools were used to speed-up the development. Tools used are 100%
compatible with the project license.

Hindsight's on_session_switch() flush re-shipped the entire _session_turns
buffer regardless of the append watermark. On append-capable APIs, once a
boundary retain persisted turns 1..N, a later session switch (/new, /reset,
/undo, context compression) appended the whole buffer again — duplicating
already-retained turns into the same document.

The flush now only ships the delta since the last retain boundary on
append-capable APIs, re-sends the full session on overwrite APIs (where each
retain replaces the document), and skips entirely when nothing new is
buffered — so no already-retained turn is ever re-shipped at switch time.

Adds a test asserting a switch after a boundary retain ships only the
partial block, and seeds the retain watermark in the session-switch test
helper.
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers area/memory Memory subsystem: store, providers, sync, background reviews P3 Low — cosmetic, nice to have 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 17, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #41911: both fix the same Hindsight session-switch append-watermark flush, sending only the unretained delta.

@rafpigna

Copy link
Copy Markdown
Author

Thank you for the triage note. You are right — I missed #41911 when I searched
for existing coverage before opening this PR. It fixes the exact same
on_session_switch() append-watermark issue with the same delta-only approach
and it predates mine, so this PR is a genuine duplicate. My apologies for the
noise, and in particular for the incorrect claim in my PR description that no
open PR addressed this duplication.

I'll close this PR. Please go ahead with #41911. If it is useful, I'm happy to
review it or help with anything still outstanding on that front.

@rafpigna rafpigna closed this Aug 17, 2026
@rafpigna
rafpigna deleted the fix/hindsight-retain-flush-on-shutdown branch August 17, 2026 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/memory Memory subsystem: store, providers, sync, background reviews comp/plugins Plugin system and bundled plugins duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants