Skip to content

feat(memory): add sync_recall option for current-turn relevance - #5838

Closed
heathley wants to merge 1 commit into
NousResearch:mainfrom
heathley:feat/memory-sync-recall
Closed

feat(memory): add sync_recall option for current-turn relevance#5838
heathley wants to merge 1 commit into
NousResearch:mainfrom
heathley:feat/memory-sync-recall

Conversation

@heathley

@heathley heathley commented Apr 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #5820.

Adds a memory.sync_recall: true config option that switches memory providers from background prefetch to synchronous recall at turn start.

Problem

The current model queues a background recall at turn end using that turn's query. The next turn consumes this result — which is based on the previous message. When topics change between turns the injected context is irrelevant or misleading.

Solution

When sync_recall: true, each turn performs a live recall with the actual current message before the first LLM call. Background prefetch is skipped entirely at turn end. Default is false — no behavior change for existing users.

Implementation

MemoryProvider.recall_sync() default: fires queue_prefetch() then immediately calls prefetch(), which joins the background thread. This works for all providers that join inside prefetch() (honcho, hindsight, mem0, openviking). RetainDB overrides recall_sync() directly because its prefetch() reads shared state without joining threads.

4 files changed, 54 lines added.

Config

memory:         
  provider: hindsight  # or honcho, mem0, etc.
  sync_recall: true

ZaynJarvis

This comment was marked as off-topic.

@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels Apr 29, 2026
Adds memory.sync_recall config flag (default: false) that switches from
background prefetch to synchronous recall at the start of each turn.

The existing model queues a background recall at turn end using that turn's
query. The next turn consumes this result — which is stale when topics change.
With sync_recall=true, each turn does a live recall with the actual current
message before the first LLM call.

Base MemoryProvider.recall_sync() fires queue_prefetch() then prefetch(),
which is correct for all providers that join their thread inside prefetch()
(honcho, hindsight, mem0, openviking). RetainDB overrides recall_sync()
because its prefetch() reads shared state without joining threads.

queue_prefetch_all() is skipped at turn end when sync_recall is enabled.

Closes NousResearch#5820

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling the stale/empty first-turn recall problem. The premise remains present on current main: agent/turn_context.py:558-564 calls prefetch_all() before the tool loop, and Hindsight consumes a queued result in plugins/memory/hindsight/__init__.py:1466-1484.

Problems

  • The generic recall_sync() is not synchronous for current Honcho. plugins/memory/honcho/session.py:666-679 starts base-context work in a daemon thread, while plugins/memory/honcho/session.py:688-695 only returns a ready cached result. queue_prefetch() followed immediately by prefetch() can therefore still inject stale or empty base context.
  • Current main moved the pre-turn integration to agent/turn_context.py:558-564; the PR's agent/conversation_loop.py edit needs a current-tree port. The new manager path should also preserve prefetch_all()'s skill-scaffolding normalization at agent/memory_manager.py:501-503.
  • The PR changes no tests. Please add behavioral coverage for current-query recall, first-turn behavior, post-turn prefetch suppression, and provider-specific paths.

Suggested changes

  • Give Honcho an explicit synchronous recall implementation or a provider contract that can actually satisfy this guarantee.
  • Port the feature through turn_context.py, retain normalization, and add the config default/documentation.

Automated hermes-sweeper review.

Comment thread agent/memory_provider.py
mem0, openviking). Providers whose prefetch() reads shared state without
joining (retaindb) should override this method.
"""
self.queue_prefetch(query, session_id=session_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generic composition is not synchronous for current Honcho: its prefetch_context() starts a daemon thread and its prefetch() only consumes a ready cache (plugins/memory/honcho/session.py:666-695). Please provide an explicit synchronous Honcho path (or narrow this contract) before claiming current-query relevance.

Comment thread agent/memory_manager.py
parts = []
for provider in self._providers:
try:
result = provider.recall_sync(query, session_id=session_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When porting onto current main, normalize this query exactly as prefetch_all() does at agent/memory_manager.py:501-503; otherwise /skill and bundle-expanded turns send provider recall the full injected skill body rather than the user instruction.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/memory Memory subsystem: store, providers, sync, background reviews labels Jul 12, 2026
sbosshardt pushed a commit to sbosshardt/hermes-agent that referenced this pull request Aug 4, 2026
Rebased from PR NousResearch#5838 onto v2026.6.19 (turn_context.py refactor).

Adds memory.sync_recall config option (default: false). When enabled:
- Turn start calls recall_sync_all() which fires queue_prefetch() directly
  (bypassing the MemoryManager executor) then joins with prefetch_join_timeout
- End-of-turn skips queue_prefetch_all() since recall is done synchronously
- MemoryProvider ABC gains recall_sync() default impl: queue_prefetch + prefetch
- RetainDB overrides recall_sync() because its prefetch() reads shared state

This sidesteps the executor serialization bug where queue_prefetch_all() was
stuck behind sync_all() in the single-worker ThreadPoolExecutor, preventing
the prefetch thread from starting before the next turn.

Conflict resolution notes:
- PR targeted conversation_loop.py for prefetch call; upstream refactored
  this into agent/turn_context.py build_turn_context(). Applied there.
- PR targeted memory_provider.py sync_turn without messages kwarg; current
  upstream added messages param. No conflict in the recall_sync addition.
- run_agent.py: variable names unchanged, clean apply.

Cross-links: NousResearch#5838, NousResearch#5820, NousResearch#9973, NousResearch#44016
(cherry picked from commit b007ee7c57137df26722ce691fe513b34b8593ff)
(cherry picked from commit 8242cadfe1090d74bc3f12d2237e44273ed0b197)
(cherry picked from commit 416917bf1ad5bedabf7fdf3fca098456bf1491c1)
(cherry picked from commit 7857698aab7900a93091096e2d4f98e34c35433c)
(cherry picked from commit c7101b2cea1fe013db5c0abaf6e07af16f9d8a4a)
(cherry picked from commit 7ad8031cee2705e9fafb6fa17cecf28bd33f7076)
sbosshardt pushed a commit to sbosshardt/hermes-agent that referenced this pull request Aug 4, 2026
…ation

queue_prefetch_all() was submitted to the same single-worker
ThreadPoolExecutor as sync_all(), meaning a slow sync_all (~60s for
Hindsight LLM fact extraction) would prevent queue_prefetch from
starting before the next turn.

By the time the next turn called prefetch(), self._prefetch_thread was
still None because queue_prefetch() had not run yet. The
prefetch_join_timeout was irrelevant — there was no thread to wait for.
No <memory-context> block was ever injected.

Fix: add a separate single-worker prefetch executor (_prefetch_executor)
so sync and prefetch run in parallel. Both remain off-thread so a slow
or misbehaving provider cannot stall turn completion. flush_pending and
shutdown_all now drain both executors.

This is safer than calling queue_prefetch_all() directly (bypassing the
executor) because not all providers have non-blocking queue_prefetch()
implementations — RetainDB joins prior threads with timeout=2.0, and
third-party providers may block.

Root cause: executor serialization in MemoryManager._submit_background()
Cross-links: NousResearch#5838, NousResearch#9973, NousResearch#44016

(cherry picked from commit e0727080dd5c3760af8f12aafcc26b49ae23c0ad)
(cherry picked from commit bec31df8792f0096e3db182fbbb7d61c5706137b)
(cherry picked from commit 7b7954edb52d67be8b1d053bf1fa83508e8665e5)
(cherry picked from commit ddd0f95300929d75989480767246701465423b93)
(cherry picked from commit 9f9535c783b95b137ebc4d5e11646073f368ee15)
(cherry picked from commit 15f286c7953eff9a220f01fb5e481c998480b087)
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Merged via #85494 — the sync_recall concept from this PR was incorporated as recall_sync in the Hindsight provider config schema. Thanks for the original idea!

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 comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint 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-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/memory Memory tool and memory providers type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(memory): Allow synchronous recall for current turn as option for honcho and hindsight

5 participants