fix(memory/hindsight): flush buffered turns on session exit - #45
Open
hashbender wants to merge 1 commit into
Open
fix(memory/hindsight): flush buffered turns on session exit#45hashbender wants to merge 1 commit into
hashbender wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When
retain_every_n_turns > 1,sync_turn()only enqueues a retain every Nth turn. Turns 1..N-1 accumulate in_session_turnsand are silently discarded on exit (Ctrl-D,/quit,/exit).The plugin already handles this for session switches —
on_session_switch()flushes buffered turns before clearing (it explicitly calls out this data-loss class in its docstring). But the exit path goes throughon_session_end()+shutdown():on_session_end()— base class implementation is a no-op (Hindsight plugin does not override it)shutdown()— only drains already-queued retains from the writer thread; does not flush buffered-but-unqueued turns from_session_turnsFix
Adds
on_session_end()toHindsightMemoryProviderthat flushes the unretained delta through the same writer queue. Sinceon_session_endis called beforeshutdown_allin the exit sequence (cli.py:1049→run_agent.py:3141), the flush is enqueued first, thenshutdown()'s sentinel follows — the writer processes the flush before exiting (FIFO).Refactor
The flush logic is extracted into a shared
_enqueue_buffered_flush()helper. Bothon_session_endandon_session_switchnow call it, removing 36 lines of duplication.on_session_switchshrinks from 127 to 82 lines.The helper handles: metadata building, lineage tags, content serialization, the
_flushclosure, and writer-queue routing. The caller handles:on_session_endsends only the unretained delta (avoids append-mode duplicates);on_session_switchsends all turns (it clears the buffer anyway)on_session_switchmust resolve before session rotation;on_session_enduses current stateDesign choices
retain_every_n_turns=1(default):_session_turnsis empty at exit because every turn was already retained viasync_turn. The first guard (if not self._session_turns: return) makes the patch zero-risk for the default config.on_session_endslices from_last_retained_turn_countto avoid duplicating turns the server already has. In overwrite mode (legacy servers), it resends everything.on_session_switch. No crash or data corruption if the API is down._session_turns: the provider is being torn down, not rotated. No need to reset state that's about to be destroyed.Verification
All 116 existing Hindsight plugin tests pass, including all 5
TestSessionSwitchBufferFlushtests that verify the refactoredon_session_switchstill flushes correctly.Not covered by this patch
The
atexithook (_atexit_shutdown, line 1143) callsshutdown()directly, bypassingon_session_end. This is a pre-existing safety-net limitation for unclean interpreter exits — fixing it would require registering the flush in the atexit handler, which is a separate change.Mirror-of: NousResearch#55936
NousResearch#55936