fix(hindsight): flush buffered turns on session end - #36219
Conversation
Users with retain_every_n_turns > 1 silently lose conversation data when a session ends without hitting the modulo boundary. For example, with retain_every_n_turns=12, a session that lasts 8 turns gets zero facts extracted into Hindsight because the buffer is never flushed. The on_session_switch hook already handles mid-process rotations (/new, /resume, context compression), but on_session_end was inherited as a no-op from the base MemoryProvider class. This meant gateway session expiry, CLI exit, and /reset all dropped buffered turns on the floor. Implement on_session_end to flush the _session_turns buffer through the existing writer queue before shutdown() tears down the provider. The method is called by MemoryManager.on_session_end (via shutdown_memory_provider) which runs before shutdown_all(), so the writer thread is still alive to process the enqueued retain. Guard rails: - No-op when auto_retain is disabled - No-op when buffer is empty (avoids wasteful zero-content retains) - No-op when _shutting_down is set (writer is gone) - Clears buffer after enqueueing to prevent double-flush if shutdown() follows immediately Co-authored-by: Ra <agent-ra@users.noreply.github.com>
alt-glitch
added
type/bug
|
This is a high-impact bug that's affecting real users in production. On a self-hosted Hermes + Hindsight setup, no memories were recorded for ~7 days despite auto_retain: true and a healthy Hindsight backend. Recall worked perfectly the whole time, which masked the problem — it looks like memory is "working" until you check the bank and find zero new documents. Could this please be prioritized for merge soon? Silent, total memory loss with no error and a working recall path is about the worst failure mode a memory system can have for users who are trusting it to remember things. Happy to test against a build once it lands. |
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the missing session-end flush; the underlying loss case is still present on current main. sync_turn() buffers below-threshold turns in plugins/memory/hindsight/__init__.py:1621-1629, and shutdown calls the provider lifecycle hook before teardown (run_agent.py:3323-3329).
Problems
- The proposed flush serializes all
_session_turnsatplugins/memory/hindsight/__init__.py:1610while retaining with append mode at line 1637. Current main now sends only the suffix after_last_retained_turn_countfor append-capable APIs (plugins/memory/hindsight/__init__.py:1637-1696), so a session that already crossed a retain boundary would duplicate earlier turns on exit. - The new tests only assert queue occupancy (
tests/agent/test_memory_session_switch.py:351-358); they do not execute the queued job or verify append-mode payload contents.
Suggested changes
- Salvage this as an append-aware delta flush, preserving full-session payloads only for legacy overwrite mode.
- Cover a prior append retain followed by a partial final batch, and apply the same payload-selection rule to the existing
on_session_switch()flush (plugins/memory/hindsight/__init__.py:1822-1843).
Automated hermes-sweeper review.
| if self._shutting_down.is_set(): | ||
| return | ||
|
|
||
| old_turns = list(self._session_turns) |
There was a problem hiding this comment.
Current main now tracks _last_retained_turn_count for append-capable APIs (sync_turn() only submits the unretained suffix). When this session already crossed a retain boundary, flushing all _session_turns here with update_mode="append" duplicates the earlier turns. Select the unretained delta for append mode and retain the full buffer only for legacy overwrite mode.
| assert provider._session_turns == [] | ||
| assert provider._turn_counter == 0 | ||
| assert provider._turn_index == 0 | ||
| # A flush closure must have been enqueued on the writer queue. |
There was a problem hiding this comment.
This only proves that a closure was queued. Execute it against a fake client and assert the payload for an append-mode session that already retained a prior batch; otherwise the duplicate-retain regression is not covered.
teknium1
added
sweeper:risk-session-state
4 participants
What does this PR do?
Implements
on_session_end()onHindsightMemoryProviderto flush buffered turns through the existing writer queue when a session is torn down. Without this, users withretain_every_n_turns > 1silently lose whatever turns are buffered when the session ends before hitting the modulo boundary.Related Issue
Fixes #36216
Fixes #
Type of Change
Changes Made
plugins/memory/hindsight/__init__.py— Implementon_session_end()that snapshots_session_turns, builds retain kwargs, and enqueues a flush through the writer queue (same pattern as the existingon_session_switchflush). Clears the buffer afterward to prevent double-flush on subsequentshutdown().tests/agent/test_memory_session_switch.py— 5 new test cases covering: flush on session end, no-op when buffer empty, no-op when auto_retain disabled, no-op when shutting down, and no double-flush guarantee.How to Test
retain_every_n_turns: 12in~/.hermes/hindsight/config.json/reset, or let gateway idle expiry fire)async_operationstable for a retain op)Automated:
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