Skip to content

fix(memory/hindsight): honor configured retain_async on manual hindsight_retain - #34529

Open
spiky02plateau wants to merge 1 commit into
NousResearch:mainfrom
spiky02plateau:fix/hindsight-manual-retain-async
Open

fix(memory/hindsight): honor configured retain_async on manual hindsight_retain#34529
spiky02plateau wants to merge 1 commit into
NousResearch:mainfrom
spiky02plateau:fix/hindsight-manual-retain-async

Conversation

@spiky02plateau

Copy link
Copy Markdown
Contributor

Problem

The manual hindsight_retain tool path in HindsightMemoryProvider.handle_tool_call builds its retain kwargs without passing retain_async:

retain_kwargs = self._build_retain_kwargs(
    content,
    context=context,
    tags=args.get("tags"),
)
...
self._run_hindsight_operation(lambda client: client.aretain(**retain_kwargs))

_build_retain_kwargs only inserts retain_async into the dict when the arg is not None (it defaults to None), so the key is omitted and client.aretain(**kwargs) falls back to the client default retain_async=False — running the retain server-sync.

That's inconsistent with the rest of the provider:

  • the provider default is self._retain_async = config.get("retain_async", True) (i.e. True), and
  • the gateway auto-retain paths (_do_retain and the flush-on-switch path) both pass the configured flag to aretain_batch(..., retain_async=retain_async_flag).

So only the manual tool runs synchronously.

Impact

  • A manual hindsight_retain blocks the turn while the server performs synchronous fact extraction (~15-40s under a slow/loaded extraction LLM).

  • It's fragile to daemon lifecycle: an in-flight sync retain can be cancelled if the daemon is torn down, surfacing to the client as a bare 500. The server-side traceback in that case is:

    ERROR: Cancel 1 running task(s), timeout graceful shutdown exceeded
    ERROR: Exception in ASGI application
    asyncio.exceptions.CancelledError: Task cancelled, timeout graceful shutdown exceeded
    

    In a long-lived gateway the same defect manifests as "the manual tool blocks the turn / occasionally errors," rather than a 500.

Fix

Pass retain_async=self._retain_async from the tool path, mirroring the auto-retain path. The tool response is unchanged. Applies to all profiles.

Test

Adds test_manual_retain_uses_configured_async_mode, asserting the configured async flag reaches aretain. Full file suite passes:

python -m pytest tests/plugins/memory/test_hindsight_provider.py -q -o 'addopts='
101 passed

…ght_retain

The manual `hindsight_retain` tool path in
`HindsightMemoryProvider.handle_tool_call` built its retain kwargs without
passing `retain_async`, so `_build_retain_kwargs` left it out and
`client.aretain(**kwargs)` fell back to the client default
(`retain_async=False`). The tool therefore ran the retain *server-sync* —
inconsistent with both the provider default (`retain_async=True`) and the
gateway auto-retain path (`_do_retain` / flush-on-switch), which pass the
configured flag to `aretain_batch`.

Practical effect: a manual `hindsight_retain` call blocks the turn while the
server performs synchronous extraction (~15-40s under a slow extraction LLM),
and is fragile to daemon lifecycle — an in-flight sync retain can be cancelled
if the daemon is torn down, surfacing as a 500
(`CancelledError: Task cancelled, timeout graceful shutdown exceeded`).

Fix: pass `retain_async=self._retain_async` from the tool path, mirroring the
auto-retain path. Covers all profiles. Tool response unchanged.

Adds a regression test asserting the configured async flag reaches `aretain`.

@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 identifying the manual-retain configuration gap. The defect remains on current main, but the provider was refactored after this branch.

Problems

  • Current hindsight_retain calls aretain_batch, not aretain, and removes retain_async from the item before the call (plugins/memory/hindsight/__init__.py:1715-1722). The configured value must instead be passed as a call-level aretain_batch(..., retain_async=self._retain_async) argument, as automatic retain does at plugins/memory/hindsight/__init__.py:1681-1686.
  • The added test asserts against p._client.aretain (tests/plugins/memory/test_hindsight_provider.py:480 in this PR), while current main's manual path uses p._client.aretain_batch.

Suggested changes

  • Rework the patch and test for the current aretain_batch contract; test both retain_async=True and False on the manual path.

Automated hermes-sweeper review.

content,
context=context,
tags=args.get("tags"),
retain_async=self._retain_async,

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 branch predates the aretain_batch refactor. On current main, the manual path removes retain_async from the item and calls aretain_batch; pass retain_async=self._retain_async to that call instead of adding it to _build_retain_kwargs.

"hindsight_retain", {"content": "user likes dark mode"}
))
assert result["result"] == "Memory stored successfully."
assert p._client.aretain.call_args.kwargs["retain_async"] is True

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.

Current main's manual handler calls aretain_batch, so this regression should assert p._client.aretain_batch.call_args.kwargs["retain_async"] instead.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers duplicate This issue or pull request already exists labels Jul 13, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #22836 (earliest-open canonical of the hindsight retain_async cluster) — same mechanism: thread the configured retain_async flag through the manual hindsight_retain tool path. Competing open alternatives: #37838 (hardcodes retain_async=True) and #52929 (routes the configured flag via aretain_batch). Cross-linked for the maintainer.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users area/memory Memory subsystem: store, providers, sync, background reviews labels Jul 13, 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 duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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