Skip to content

fix(hindsight): flush buffered turns on session end - #55046

Open
kumaxs wants to merge 3 commits into
NousResearch:mainfrom
kumaxs:pr/hindsight-on-session-end
Open

fix(hindsight): flush buffered turns on session end#55046
kumaxs wants to merge 3 commits into
NousResearch:mainfrom
kumaxs:pr/hindsight-on-session-end

Conversation

@kumaxs

@kumaxs kumaxs commented Jun 29, 2026

Copy link
Copy Markdown

Problem

HindsightMemoryProvider accumulates conversation turns in _session_turns and only flushes them to the Hindsight API every _retain_every_n_turns turns (default 5). If a session ends (idle TTL eviction, /reset, cap enforcement) before reaching the threshold, the buffered turns are silently discarded — the message content is lost forever.

The base MemoryProvider.on_session_end() is a no-op, and Hindsight does not override it, so session boundaries never trigger a flush.

Fix

Override on_session_end(messages) to flush any remaining _session_turns into the Hindsight API via the same aretain_batch + writer queue path used by the periodic flush and on_session_switch flush.

The implementation:

  • Snapshots current _session_turns, then clears the buffer (re-entrancy safe)
  • Resolves the document target (append-mode for v0.5.0+, fallback to per-process document for legacy)
  • Enqueues a _flush job to the existing serial writer queue, so it's ordered behind any in-flight retains
  • Logs success/failure at INFO/WARNING level

Related

Depends on the gateway-side change in PR #55045 which calls on_session_end before evicting cached agents. Without that PR, this hook would never be reached on idle TTL eviction (only on explicit /reset and the existing shutdown path).

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have tool/memory Memory tool and memory providers comp/plugins Plugin system and bundled plugins duplicate This issue or pull request already exists labels Jun 29, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #36219 — same fix (HindsightMemoryProvider.on_session_end() flushing buffered _session_turns through the writer queue, both Fixes #36216) with the identical title. #36219 is the earlier open canonical. Companion to your #55045 (the gateway-side hook this depends on). Flagging the cluster so a maintainer can pick one of the on_session_end flush PRs.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for targeting a real data-loss path. Current main buffers sub-threshold turns in plugins/memory/hindsight/__init__.py:1621-1629, and shutdown only drains work already enqueued (plugins/memory/hindsight/__init__.py:1905-1925), so an end-of-session flush is needed.

Problems

  • The proposed flush sends all _session_turns after resolving update_mode. On append-capable APIs, current periodic retention deliberately sends only _session_turns[_last_retained_turn_count:] and advances that watermark (plugins/memory/hindsight/__init__.py:1631-1696). A completed batch followed by a partial tail would therefore append the completed batch a second time.
  • tests/plugins/memory/test_hindsight_provider.py has switch-flush coverage (:1196-1437) but no Hindsight on_session_end() coverage for this new lifecycle path.

Suggested changes

  • Send only the unretained watermark delta in append mode; retain the full-buffer behavior for legacy overwrite mode.
  • Add session-end tests for sub-threshold flushing, append-mode tail-only flushing after a prior batch, no-op guards, and queue-before-shutdown ordering.
  • Consider sharing the corrected flush helper with on_session_switch() (plugins/memory/hindsight/__init__.py:1822-1867), which currently also flushes its full buffer.

This is an automated hermes-sweeper review.

@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 area/sessions Session lifecycle, resume, persistence, history area/memory Memory subsystem: store, providers, sync, background reviews labels Jul 15, 2026
HindsightMemoryProvider accumulates turns in _session_turns and only
flushes them to the API every _retain_every_n_turns turns (default 5).
If the session ends (idle TTL eviction, /reset, cache eviction) before
reaching the threshold, the buffered turns are silently lost.

Override on_session_end() to flush any remaining _session_turns via the
writer queue, using the same aretain_batch path as the periodic flush
and on_session_switch flush.  This ensures no conversation data is lost
at session boundaries.

The flush is idempotent: if _session_turns is empty, it's a no-op.
The queue serializes behind any in-flight retains from sync_turn.
@kumaxs
kumaxs force-pushed the pr/hindsight-on-session-end branch from 035baa9 to 07698a7 Compare July 21, 2026 06:06
The periodic retain in sync_turn only sends the unretained watermark
delta (_session_turns[_last_retained_turn_count:]) in append mode.
The on_session_end flush was sending ALL _session_turns, which would
re-ship turns already retained by the periodic flush, causing duplicate
content in append-mode documents.

Reorder to resolve update_mode first, then use watermark slicing for
append mode. Also add early return when no new turns exist (all already
retained by periodic flush).

Refs: NousResearch#55046 review by teknium1
@kumaxs

kumaxs commented Jul 21, 2026

Copy link
Copy Markdown
Author

@teknium1 已修复 append-mode 重复 flush 问题:

修复内容 (5fc7ff2):

  • on_session_end 现在先 resolve update_mode,append 模式下使用 watermark delta _session_turns[_last_retained_turn_count:],只发送未被 periodic retain 覆盖的新 turns
  • 新增 early return:如果所有 turns 已被 periodic flush 覆盖,跳过 flush
  • overwrite 模式保持全量发送(legacy 兼容)

还需要补充测试覆盖(sub-threshold flushing、append-mode tail-only、no-op guard)——我接下来处理。

@alt-glitch alt-glitch removed the duplicate This issue or pull request already exists label Jul 21, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #36219 and #55936 rather than a duplicate: the current head adds the append-mode watermark delta that the earlier duplicate review found missing. The remaining review gap is dedicated on_session_end regression coverage.

Cover the session-end flush lifecycle:
- sub-threshold turns flushed on session end
- no-op when buffer empty
- no-op when auto_retain disabled
- append mode: only unretained watermark delta sent
- overwrite mode: full buffer sent
- flush serializes behind pending periodic retains via writer queue

Refs: NousResearch#55046 review by teknium1
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 area/sessions Session lifecycle, resume, persistence, history comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have 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 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.

3 participants