fix(hindsight): flush only the un-retained delta on session switch - #41911
fix(hindsight): flush only the un-retained delta on session switch#41911Vesna-9 wants to merge 1 commit into
Conversation
In append-capable mode (Hindsight >= 0.5.0) `sync_turn` ships only the turns past `_last_retained_turn_count` on each retain and advances that watermark, so already-appended turns are never re-sent. The flush-on-switch path in `on_session_switch`, however, snapshotted the full `_session_turns` (`old_turns = list(self._session_turns)`) and re-appended every turn under `update_mode='append'`, ignoring the watermark. With the default `retain_every_n_turns=1` every turn has already been appended by the time the session rotates, so each `/resume`, `/branch`, `/new`, `/reset`, and context compression appended a duplicate copy of the entire session to the stored document — silent memory-store corruption that skews recall and reflect. The flush now mirrors `sync_turn`: in append mode it sends only `self._session_turns[self._last_retained_turn_count:]` and skips the retain entirely when that delta is empty, while the legacy/overwrite path still resends the whole session (each retain replaces the document). `message_count` metadata is derived from the trimmed delta. ## What does this PR do? Fixes duplicate-turn accumulation in the Hindsight memory provider. The flush that runs when the agent rotates its `session_id` was re-appending the whole buffered session even though `sync_turn` had already appended each turn incrementally, so the backing document grew a fresh duplicate of the conversation on every session switch. Before: switching sessions in append mode re-appended every turn already written by `sync_turn`; with `retain_every_n_turns=1` that meant a full duplicate of the session per `/resume`, `/branch`, `/new`, `/reset`, and compression. After: the flush sends only the turns past `_last_retained_turn_count`, and fires no retain at all when the watermark has caught up to the buffer. The legacy/overwrite path is unchanged. ## Related Issue N/A ## Type of Change - [x] 🐛 Bug fix (non-breaking change that fixes an issue) ## Changes Made - `plugins/memory/hindsight/__init__.py`: in `on_session_switch`, resolve the old session's `update_mode` before computing the flush payload; in append mode slice `_session_turns[self._last_retained_turn_count:]` instead of copying the full buffer, skip the retain when the delta is empty, and size `message_count` from the trimmed delta. The legacy/overwrite branch still flushes the entire session. - `tests/plugins/memory/test_hindsight_provider.py`: add `test_session_switch_does_not_reflush_already_appended_turns` (every turn already retained -> switch fires no flush) and `test_session_switch_flush_ships_only_unretained_delta_in_append_mode` (partially-buffered -> flush carries only the un-retained tail, never the already-appended turns). ## How to Test 1. Run the targeted regression tests: `scripts/run_tests.sh tests/plugins/memory/test_hindsight_provider.py` 2. Both new tests pass against the fix; reverting the slice back to `old_turns = list(self._session_turns)` makes them fail because the flush re-ships `turn1`/`turn2` under `update_mode='append'`. 3. Full file is green: 104 passed. ## Checklist ### Code - [x] I've read the [Contributing Guide](https://github.com/NousResearch/hermes-agent/blob/main/CONTRIBUTING.md) - [x] My commit messages follow [Conventional Commits](https://www.conventionalcommits.org/) (`fix(scope):`, `feat(scope):`, etc.) - [x] I searched for [existing PRs](https://github.com/NousResearch/hermes-agent/pulls) 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](https://github.com/NousResearch/hermes-agent/blob/main/CONTRIBUTING.md#cross-platform-compatibility) — or N/A - [x] I've updated tool descriptions/schemas if I changed tool behavior — or N/A
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Analysis
Correctness ✅
- Root cause clearly identified:
sync_turnin append mode ships each turn as a delta and advances_last_retained_turn_count, buton_session_switchwas flushing the entire_session_turnsbuffer — causing silent duplicate appends on every switch, resume, branch, reset, and context compression. - Fix correctly slices
self._session_turns[self._last_retained_turn_count:]to get only the un-retained delta. - The
if old_turns:guard correctly skips the flush entirely when the watermark has caught up (all turns already retained).
Testing ✅
test_session_switch_does_not_reflush_already_appended_turns: verifies that whenretain_every_n_turns=1, no retain call is made at all (watermark == buffer len).test_session_switch_flush_ships_only_unretained_delta_in_append_mode: verifies that whenretain_every_n_turns=2, only turn 3 is flushed (turns 1+2 were already appended).
Code Quality ✅
- The restructured code is clearer — the delta-slicing logic is upfront and obvious.
Recommendation
Approve — well-scoped fix that eliminates the duplicate data corruption with thorough tests.
|
Thanks for the focused regression fix. The premise holds on current main: The proposed branch mirrors the existing append/legacy split, skips an empty append delta, retains the legacy full-buffer overwrite behavior, and tests both the fully retained and partially buffered cases. GitHub currently reports the PR as mergeable. Automated hermes-sweeper review. |
|
Independent reproduction on a separate deployment, plus one adjacent gap worth flagging. I ran into this while investigating duplicate facts accumulating in a Hindsight bank, traced it to the same root cause, and wrote essentially the same patch before finding this PR — same delta slice, same skip-when-empty, same "legacy/overwrite still resends the whole session", same Reproduction, driving turns through Turns 1–20 committed twice — every turn up to the last I also checked the legacy path doesn't regress, since that's the easy thing to break here: Repeated full-buffer sends, which is correct under overwrite semantics — each retain replaces the document. Still applies to current main. Adjacent gap: #55936 adds an |
In append-capable mode (Hindsight >= 0.5.0)
sync_turnships only theturns past
_last_retained_turn_counton each retain and advances thatwatermark, so already-appended turns are never re-sent. The
flush-on-switch path in
on_session_switch, however, snapshotted thefull
_session_turns(old_turns = list(self._session_turns)) andre-appended every turn under
update_mode='append', ignoring thewatermark. With the default
retain_every_n_turns=1every turn hasalready been appended by the time the session rotates, so each
/resume,/branch,/new,/reset, and context compression appendeda duplicate copy of the entire session to the stored document — silent
memory-store corruption that skews recall and reflect.
The flush now mirrors
sync_turn: in append mode it sends onlyself._session_turns[self._last_retained_turn_count:]and skips theretain entirely when that delta is empty, while the legacy/overwrite
path still resends the whole session (each retain replaces the
document).
message_countmetadata is derived from the trimmed delta.What does this PR do?
Fixes duplicate-turn accumulation in the Hindsight memory provider. The
flush that runs when the agent rotates its
session_idwas re-appendingthe whole buffered session even though
sync_turnhad already appendedeach turn incrementally, so the backing document grew a fresh duplicate
of the conversation on every session switch.
Before: switching sessions in append mode re-appended every turn already
written by
sync_turn; withretain_every_n_turns=1that meant a fullduplicate of the session per
/resume,/branch,/new,/reset, andcompression.
After: the flush sends only the turns past
_last_retained_turn_count,and fires no retain at all when the watermark has caught up to the
buffer. The legacy/overwrite path is unchanged.
Related Issue
N/A
Type of Change
Changes Made
plugins/memory/hindsight/__init__.py: inon_session_switch, resolvethe old session's
update_modebefore computing the flush payload; inappend mode slice
_session_turns[self._last_retained_turn_count:]instead of copying the full buffer, skip the retain when the delta is
empty, and size
message_countfrom the trimmed delta. Thelegacy/overwrite branch still flushes the entire session.
tests/plugins/memory/test_hindsight_provider.py: addtest_session_switch_does_not_reflush_already_appended_turns(everyturn already retained -> switch fires no flush) and
test_session_switch_flush_ships_only_unretained_delta_in_append_mode(partially-buffered -> flush carries only the un-retained tail, never
the already-appended turns).
How to Test
scripts/run_tests.sh tests/plugins/memory/test_hindsight_provider.pyold_turns = list(self._session_turns)makes them fail because theflush re-ships
turn1/turn2underupdate_mode='append'.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A