Skip to content

fix(hindsight): make hindsight_retain tool fire-and-forget - #22836

Draft
bitfieldz wants to merge 1 commit into
NousResearch:mainfrom
bitfieldz:fix/hindsight-retain-async
Draft

fix(hindsight): make hindsight_retain tool fire-and-forget#22836
bitfieldz wants to merge 1 commit into
NousResearch:mainfrom
bitfieldz:fix/hindsight-retain-async

Conversation

@bitfieldz

Copy link
Copy Markdown

What does this PR do?

hindsight_retain was calling client.aretain(), a thin SDK wrapper that hardcodes
retain_async=False. The tool blocks until the server finishes LLM extraction. Under load,
extraction exceeds _DEFAULT_TIMEOUT (120 s); the SDK raises a bare TimeoutError; the tool
returns {"error": "Failed to store memory: "} with an empty exception string.

_do_retain (the auto-retain path) has used aretain_batch(retain_async=True) since
a1b6d9e — the tool just never
got the same treatment. This PR brings them in line: route hindsight_retain through
aretain_batch(retain_async=True), eliminating the timeout coupling entirely. Extraction runs
server-side; results surface via recall once Reflect runs.

Success message changes from "Memory stored successfully." to "Memory queued for storage."
to reflect the queued semantics.

Related Issue

Partially addresses #14950 — resolves the retain case by eliminating the timeout dependency.
The reflect timeout described in that issue is out of scope here.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • plugins/memory/hindsight/__init__.py: replace client.aretain(**retain_kwargs) with
    client.aretain_batch(bank_id=..., items=[retain_kwargs], retain_async=True) in
    handle_tool_call("hindsight_retain"). Update success message.
  • tests/plugins/memory/test_hindsight_provider.py: update five TestToolHandlers retain tests
    to assert aretain_batch.call_args.kwargs instead of aretain.call_args.kwargs.

How to Test

  1. pytest tests/plugins/memory/test_hindsight_provider.py -q — 96 passed.
  2. pytest tests/plugins/ -q — 539 passed. (One pre-existing flake:
    test_achievements_plugin.py::test_evaluate_all_stale_cache_serves_stale_and_refreshes_in_background
    — timing race, reproducible on clean main, unrelated.)
  3. Manual: against a slow extraction backend, hindsight_retain returns
    {"result": "Memory queued for storage."} immediately; the operation appears at
    GET /v1/default/banks/<bank>/operations within ~200 ms.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs — no duplicate
  • My PR contains only changes related to this fix
  • I've run pytest tests/plugins/ -q — 539 passed
  • I've added/updated tests for my changes
  • I've tested on my platform: macOS 15 (Apple Silicon)

Documentation & Housekeeping

  • Docs — N/A
  • cli-config.yaml.example — N/A
  • CONTRIBUTING.md / AGENTS.md — N/A
  • Cross-platform — pure Python async, no platform-specific paths
  • Tool schema (content, context, tags) unchanged; success string updated

Screenshots / Logs

Before:

{"error": "Failed to store memory: "}

After:

{"result": "Memory queued for storage."}

The user-facing hindsight_retain tool was calling client.aretain()
which blocks server-side until the embedding/extraction LLM completes.
When the inference backend is under load, extraction routinely exceeds
HINDSIGHT_TIMEOUT (default 120s), the SDK raises a bare timeout, and
the tool returns '{"error": "Failed to store memory: "}' (note empty
inner exception) to the model.

This is asymmetric with the auto-retain (sync_turn -> _do_retain) path,
which already uses aretain_batch(retain_async=True) for instant return
after server-side queuing. The user-callable tool just never got the
same treatment. The SDK's aretain() is a thin wrapper over aretain_batch
but does not expose retain_async, so it's hardcoded blocking.

Switch the tool path to aretain_batch(retain_async=True). Server-side
extraction still happens asynchronously and is observable via
/v1/default/banks/<bank>/operations; recall surfaces the extracted
memory units once Reflect runs. No client-side timeout coupling to
LLM load.

The success message changes from 'Memory stored successfully.' to
'Memory queued for storage.' to make the fire-and-forget semantics
explicit to the model.
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying the missing call-level retain_async propagation: current main still omits it in the tool path at plugins/memory/hindsight/__init__.py:1715-1724.

Problems

  • The patch hard-codes retain_async=True, but retain_async is a documented user setting (plugins/memory/hindsight/README.md:91-100) and the automatic retain path preserves self._retain_async (plugins/memory/hindsight/__init__.py:1664-1686). This would make tool retains asynchronous even when the provider is configured otherwise.
  • The new call remains inline. _run_hindsight_operation() calls _run_sync(...).result(...) (plugins/memory/hindsight/__init__.py:1151-1155), so it can still wait for the HTTP acknowledgement. The existing writer queue is the established in-process non-blocking mechanism (plugins/memory/hindsight/__init__.py:1690-1692).

Suggested changes

  • Propagate self._retain_async; enqueue through the writer only when it is enabled, and preserve the direct synchronous/error-reporting path when disabled.
  • Add tests for queued async behavior and configured synchronous behavior.

Automated hermes-sweeper review.

@teknium1 teknium1 added 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 labels Jul 13, 2026
@teknium1 teknium1 added the area/memory Memory subsystem: store, providers, sync, background reviews label Jul 19, 2026
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/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 tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants