Skip to content

fix(hindsight): clear _session_turns after sync_turn to send only deltas on append mode - #23755

Closed
draix wants to merge 1 commit into
NousResearch:mainfrom
draix:fix/hindsight-sync-turn-delta
Closed

draix wants to merge 1 commit into
NousResearch:mainfrom
draix:fix/hindsight-sync-turn-delta

Conversation

@draix

@draix draix commented May 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #23724.

On the modern Hindsight API (≥ 0.5.0) sync_turn() uses update_mode='append'
against a stable session-scoped document_id, so the server preserves prior
document content across retains. However, _session_turns was never cleared
after a successful retain enqueue, so every subsequent retain re-shipped the
full growing transcript instead of just the new turns.

With retain_every_n_turns=N, retain #k sends turns 1..k·N, so turn i ends
up appended to the document ⌈(total_turns − i + 1)/N⌉ times. The downstream
chunker/extractor then produces 3-4× duplicate facts per session and burns
~80% extra extraction tokens (see issue for measurements on a live document).

Fix

After building content in sync_turn(), clear _session_turns iff we used
update_mode='append':

document_id, update_mode = self._resolve_retain_target(self._document_id)
...
if update_mode == "append":
    self._session_turns = []

That's the entire behavior change. The next retain starts with an empty buffer
and ships only the turns accumulated since the last successful enqueue.

Why conditional on update_mode == "append"

_resolve_retain_target returns update_mode=None on the legacy path
(Hindsight < 0.5.0). There the per-process document_id is unique per
process lifecycle and each retain replaces the document (preserving the
#6654 resume-overwrite fix). On that path we MUST keep resending the full
session — clearing the buffer would make the next retain overwrite the
document with only the new turns and silently lose earlier content.

The session-switch flush path (on_session_switch, c38dac74) already
snapshots _session_turns and clears it explicitly after enqueueing the
flush, so it's unaffected.

Relationship to #20664

I'm aware #20664 is open and addresses the same bug. Its approach is to
switch the modern path from update_mode='append' on the session document
back to update_mode='replace' on the per-process document.

This PR takes the opposite, minimal approach: keep append semantics (which
is the documented direction of travel — see 3082fa0 and
vectorize-io/hindsight#932 / #1303), and instead fix the client to actually
honor them by only sending deltas.

Reasons to prefer this approach:

  • One line of behavior change. No rollback of the append-mode work in
    3082fa0, no changes to capability probing, no changes to document_id
    selection.
  • Preserves the cross-process dedup property that update_mode='append'
    on a stable session-scoped document_id was introduced to enable. Going
    back to per-process replace documents loses that.
  • Smaller request payloads — each retain now ships only the new turns
    (retain_every_n_turns turns), not the entire growing session. This is
    the original motivation for batching.
  • Symmetric with the existing session-switch flush, which already does
    exactly this: build content from the buffered turns, then clear the
    buffer. sync_turn() was the asymmetric outlier.

Happy to defer to #20664 if maintainers prefer the replace-semantics
direction; flagging here so the choice is explicit.

Testing

$ scripts/run_tests.sh tests/plugins/memory/test_hindsight_provider.py
98 passed

Two new tests added under TestUpdateModeAppendCapability:

  • test_modern_api_sends_only_new_turns_on_subsequent_retain — with the
    /version probe stubbed to 0.5.6, verifies that the second retain batch
    contains turns 3-4 only, NOT turns 1-2 (which the server already has).
  • test_legacy_api_still_sends_full_session_on_each_retain — with /version
    stubbed to None (legacy server), verifies that the second retain still
    contains the full session 1-4 AND has no update_mode key, so the
    resume-overwrite fix keeps working on older Hindsight deployments.

The existing test_sync_turn_accumulates_full_session still passes — it
runs without monkeypatching the probe, falls back to legacy mode, and
exercises the same full-session-resend behavior the new legacy-API test
codifies explicitly.

Full memory plugin suite and session-switch tests also green:

$ pytest tests/plugins/memory/ tests/agent/test_memory_session_switch.py \
         tests/agent/test_memory_provider.py tests/run_agent/test_memory_sync_interrupted.py
252 passed

Tested on macOS 14 / Python 3.11.

Risk

  • The fix only affects the modern (update_mode='append') path. Legacy
    servers are untouched and covered by the new explicit test.
  • No public API or schema change.
  • on_session_switch already uses the same content/clear pattern, so the
    semantics are now symmetric across the two retain entry points instead of
    asymmetric.

…ousResearch#23724)

On the modern Hindsight API (>=0.5.0) sync_turn() uses update_mode='append'
against a stable session-scoped document_id, so the server preserves prior
content across retains. However, _session_turns was never cleared after a
successful enqueue, so every subsequent retain re-shipped the full growing
transcript. With retain_every_n_turns=N the server ended up with each turn
appended ceil(total_turns/N) times, producing duplicate chunks and ~80%
unnecessary extraction tokens.

Clear _session_turns after building content only when update_mode='append'.
On the legacy path (update_mode is None, per-process document_id, replace
semantics) the full session must keep being resent, so the buffer stays
intact in that case. The session-switch flush path (on_session_switch)
already snapshots and clears explicitly, so it is unaffected.
@draix

draix commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Friendly bump 🙂 — this one's been quiet for ~3 weeks but is still mergeable with no conflicts. The fix is small (+81 LoC, no deletions) and self-contained to the hindsight delta path. Happy to rebase or add tests if useful.

@draix

draix commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Closing to keep our open PR count manageable per the repo contribution guidelines. The underlying issue (#23686 / append-mode delta) remains open; happy to revive this PR if a maintainer wants to pick it up. Thanks!

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

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have 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.

Hindsight plugin: sync_turn resends full session transcript on every retain, duplicating content with update_mode=append

3 participants