fix(cli): run /new session-boundary memory extraction off the command path (#16454 salvage) - #61139
Merged
kshitijk4poor merged 3 commits intoJul 8, 2026
Conversation
(cherry picked from commit 3e82a86)
Deep review of the cherry-picked NousResearch#16454 found the ad-hoc flush thread raced new_session()'s inline on_session_switch(reset=True): memory providers key off internal _session_id state (MemoryManager.on_session_end takes no session id), so a late off-thread extraction ran against post-rotation bindings — misattributing the old transcript to the new session id, double-ingesting the old turn buffer (supermemory), or double-committing (openviking already async-finalizes in on_session_switch). Redesign: new MemoryManager.commit_session_boundary_async queues on_session_end + on_session_switch as ONE task on the manager's existing single-worker background executor (the same worker sync_all already uses). This preserves the strict end→switch ordering providers depend on, serializes against per-turn syncs FIFO, keeps /new non-blocking, and degrades to inline (pre-NousResearch#16454 behavior) when the executor is unavailable. No ad-hoc threads; no per-provider changes needed. The context-engine on_session_end half stays synchronous in _launch_session_boundary_memory_flush (cheap, must land before reset_session_state rebinds the engine). Exit durability: _run_cleanup calls the manager's existing flush_pending(timeout=10) barrier before shutdown, so '/new then quit' doesn't drop the queued extraction (shutdown_all's own drain is ~5s and cancels queued tasks). Bounded well inside the 30s exit watchdog. Tests: ordering invariant with slow (LLM-like) extraction, FIFO serialization vs sync_all, switch-fires-even-if-end-raises, no-provider no-op, CLI snapshot handoff + inline-switch fallback, sync engine boundary, cleanup flush_pending.
alt-glitch
added
type/bug
- Return the boundary snapshot from _launch_session_boundary_memory_flush as a local value instead of staging it on self._session_boundary_snapshot. The instance-attr handoff could leak (no memory manager configured) or mis-fire a stale snapshot on a later /new if an exception hit between staging and consumption. A local variable eliminates the class; the helper also returns None when no memory manager is configured so new_session takes the inline-switch path. - Drop the now-dead session_id kwarg from commit_memory_session: after the redesign no production caller passes it (gateway, TUI, compression all use the default), and speculative params are rejected per AGENTS.md. The explicit-old-session need is served by cli.py's direct engine call + commit_session_boundary_async. - Drop the dead providers snapshot in commit_session_boundary_async (only the emptiness check used it). - Tests updated accordingly (dead-kwarg test removed, snapshot assertion now covered by return-value contract). Phase-2 gates: 2a tests/cli 1048 passed + 6 memory files 137 passed; 2b programmatic live smoke 0.38ms non-blocking caller, end→switch→sync ordering verified; 2c structured 4-angle review — no Criticals, these warnings fixed.
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
3 participants
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
/newno longer blocks for seconds on end-of-session memory extraction — the LLM-bound provider flush is queued on the memory manager's serialized background worker, with the end→switch ordering providers depend on preserved and exit-time durability guaranteed.Salvages #16454 by @Tosko4 (cherry-picked, authorship preserved) onto current main, with a review-driven redesign of the offload mechanism on top.
Supersedes #16454 — can be closed in favor of this PR.
Root cause & why not a bare thread
The original PR's ad-hoc daemon thread raced
new_session()'s inlineon_session_switch(reset=True):MemoryManager.on_session_endtakes no session id, providers key off internal_session_idstate, and the switch (instant) beats the extraction (LLM-bound, seconds) essentially every time. A lateon_session_endthen ran against post-rotation bindings — old transcript misattributed to the NEW session id, double-ingest of the old turn buffer (supermemory), or double-commit (openviking already async-finalizes old sessions insideon_session_switch). Caught in deep review (3-angle, all converged on this).Changes
run_agent.py/cli.py(contributor, cherry-picked): move/new's blockingcommit_memory_sessionoff the command path;commit_memory_sessionaccepts an explicitsession_id.agent/memory_manager.py(follow-up): newcommit_session_boundary_async()— queueson_session_end+on_session_switch(reset=True)as ONE task on the manager's existing single-worker executor (the same workersync_alluses). Strict end→switch ordering, FIFO serialization against per-turn syncs, non-blocking caller, inline degradation when the executor is unavailable. No ad-hoc threads, no per-provider changes.cli.py(follow-up):new_session()snapshots history and routes the boundary throughcommit_session_boundary_async(inline switch preserved for the no-history case); the context-engineon_session_endhalf stays synchronous (cheap, must land beforereset_session_state()rebinds the engine);_run_cleanupdrains pending boundary work via the manager's existingflush_pending(timeout=10)barrier so "/new then quit" can't drop extraction (shutdown_all's own drain is ~5s and cancels queued tasks; bounded well inside the 30s exit watchdog).Validation
New regression tests: ordering invariant under slow extraction, FIFO vs
sync_all, switch-fires-even-if-end-raises, no-provider no-op, CLI snapshot handoff + inline-switch fallback, synchronous engine boundary, cleanupflush_pending.Credit: @Tosko4 for the diagnosis and the off-thread design direction (#16454).
Phase-2 review gates (follow-up commit 3)
Ran the structured hermes-pr-review Phase 2 (2a/2b/2c) + hermes-agent-dev gates against the branch; findings folded in as
refactor: address Phase-2 review findings:new_session).session_idkwarg fromcommit_memory_session(no caller passed it after the redesign; speculative params rejected per AGENTS.md) and a dead local incommit_session_boundary_async.tests/cli/1048 passed + 6 memory suites 136 passed, 0 failed; 2b programmatic live smoke — caller returns in <1ms, end→switch→sync ordering verified against a real executor; 2c 4-angle review — 0 Criticals (compressor durable-cooldown round-trip and end→switch vs DB-flush ordering both verified safe against source), warnings fixed. Known scoping note: TUI/gateway finalize paths still run extraction inline — pre-existing behavior, out of scope for thisfix(cli)PR.