fix: prevent OpenViking memory loss during context compression - #8362
Closed
fancydirty wants to merge 2 commits into
Closed
fix: prevent OpenViking memory loss during context compression#8362fancydirty wants to merge 2 commits into
fancydirty wants to merge 2 commits into
Conversation
- Call memory_manager.on_session_end() before session split in _compress_context so archived messages are committed to OpenViking instead of being dropped. - Re-initialize memory providers with the new session_id after compression. - Expose viking_remember to the summary model in flush_memories and execute the tool call through the memory manager, enabling OpenViking writes. Closes memory-loss issues during long-running sessions.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes loss of long-term memory for external memory providers (notably OpenViking) when context compression triggers a SQLite session split, by ensuring provider lifecycle hooks and flush tooling run across the split boundary.
Changes:
- Update
flush_memories()to surface bothmemoryandviking_remembertools to the summary/flush call and executeviking_remembertool calls. - Update
_compress_context()to notify memory providers before rotatingsession_idand re-initialize providers after the split. - Add regression tests covering provider lifecycle during compression and
viking_rememberexposure/execution in flush.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| run_agent.py | Adds session-end + re-init lifecycle calls around compression session split; expands flush tool exposure/execution to include viking_remember. |
| tests/run_agent/test_compression_memory_provider.py | New tests asserting memory provider lifecycle hooks are invoked correctly during compression-driven session splits. |
| tests/run_agent/test_flush_memories_codex.py | Adds tests ensuring viking_remember is included in flush tool lists and that returned tool calls are executed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+6468
to
+6472
| # Make one API call with memory-related tools available | ||
| memory_tool_defs = [] | ||
| for t in (self.tools or []): | ||
| if t.get("function", {}).get("name") == "memory": | ||
| memory_tool_def = t | ||
| break | ||
| name = t.get("function", {}).get("name") | ||
| if name in ("memory", "viking_remember"): |
Comment on lines
+6654
to
+6660
| # subsequent sync_turn calls target the new session | ||
| if self._memory_manager: | ||
| try: | ||
| self._memory_manager.initialize_all( | ||
| session_id=self.session_id, | ||
| platform=self.platform or os.environ.get("HERMES_SESSION_SOURCE", "cli"), | ||
| ) |
Comment on lines
+286
to
+296
| agent = run_agent.AIAgent( | ||
| api_key="test-key", | ||
| base_url="https://test.example.com/v1", | ||
| provider=provider, | ||
| api_mode=api_mode, | ||
| max_iterations=4, | ||
| quiet_mode=True, | ||
| skip_context_files=True, | ||
| skip_memory=True, | ||
| ) | ||
| agent._memory_store = MagicMock() |
ZaynJarvis
reviewed
Apr 14, 2026
ZaynJarvis
left a comment
ZaynJarvis
left a comment
Contributor
There was a problem hiding this comment.
Review: Compression Path Memory Commit + flush_memories Viking Support
Verdict: Approve — good fix with solid test coverage
Checklist Pass
-
_compress_context()callsmemory_manager.on_session_end(messages)before session ID rotation — correct, the old messages are committed under the old session_id -
_compress_context()callsmemory_manager.initialize_all(session_id=self.session_id)after new session_id is set — providers re-bind to the new session -
flush_memories()now collects both"memory"and"viking_remember"tools — the model can use either during the flush pass -
flush_memories()handlesviking_remembertool calls by dispatching tomemory_manager.handle_tool_call() - Tests: 100-line
test_compression_memory_provider.py+ 124-line extension totest_flush_memories_codex.py— covers both theon_session_endcall ordering and the tool inclusion - No new imports at module level, no circular dependencies
- AGENTS.md note about not bypassing abstraction layers is a good addition
One Minor Issue: initialize_all Call After Compression
The call to memory_manager.initialize_all() uses platform=self.platform or os.environ.get("HERMES_SESSION_SOURCE", "cli"). This is fine but note that initialize_all typically calls each provider's initialize() method which may reset prefetch state. Since we're mid-compression this is the correct behavior — new session, fresh state.
Merge Order
This PR touches run_agent.py which also has diffs in #8474. Merge this after #7762 and alongside or before #9167 to avoid conflicts. The flush_memories changes here are orthogonal to #9167's session management changes.
alt-glitch
added
type/bug
Contributor
|
would recommend to close because fix on #10463 is merged to commit long session. |
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.
Summary
Fixes two bugs that cause long-term memory to be lost when context compression triggers a session split while OpenViking is enabled.
Changes
run_agent.py
_compress_context– callself._memory_manager.on_session_end(messages)before rotating the session ID, andinitialize_all(session_id=...)after.flush_memories– expose bothmemoryandviking_remembertools to the summary model, and actually executeviking_remembertool calls.Tests
tests/run_agent/test_compression_memory_provider.py(new) – verifies memory-provider lifecycle hooks are called correctly during compression.tests/run_agent/test_flush_memories_codex.py– addedTestFlushMemoriesExposesVikingRememberto ensureviking_rememberis visible and executable.Verification
Related issues
Memory loss during long-running sessions with OpenViking enabled.