fix(hindsight): pass retain_async=True to aretain_batch in tool handler

[yingliang-zhang]

Problem

The hindsight_retain tool handler calls client.aretain_batch() without retain_async, defaulting to synchronous mode. On banks with significant data, the LLM fact-extraction call can take 60–600+ seconds, far exceeding the 120s _DEFAULT_TIMEOUT. This produces an opaque TimeoutError with an empty message: "Failed to store memory: ".

The auto-retain path (sync_turn, ~L1685) correctly passes retain_async=retain_async_flag (True) to aretain_batch. The tool call path (~L1721) does not, despite the comment at ~L1715 stating "aretain_batch takes bank_id/retain_async as call args, not item keys".

Fix

Add retain_async=True to the aretain_batch call in the tool handler, consistent with the auto-retain path. The tool now returns immediately after the server accepts the request, matching sync_turn behavior.

Why not #37838?

PR #37838 adds retain_async=True to _build_retain_kwargs() instead. However, this is ineffective because item.pop("retain_async", None) at ~L1717 immediately removes it from the item dict before aretain_batch is called — the value never reaches the server.

This PR passes retain_async=True directly to aretain_batch as a call argument, which is the correct approach.

Closes #29079 (Embedded Hindsight retain reports failure while async retain later appears in recall).
Related: #14950 (per-operation timeout), #42466 (cron retain race).
Supersedes approach in #37838.

[alt-glitch]

This was generated by AI during triage.

Duplicate of #22836 (earliest open, canonical). Both route hindsight_retain through client.aretain_batch(bank_id=..., items=[...], retain_async=True) - the same fire-and-forget mechanism at the same call site. #44414 was already marked a duplicate of #22836 for the same reason. Related: #37838 (competing _build_retain_kwargs approach), #29079 (the bug this closes). A maintainer can pick between #22836 and #60648.

[yingliang-zhang]

Thanks for the triage @alt-glitch. I've analyzed all three competing PRs and want to clarify the differences:

#22836 (by @spirotot, May 9) — The earliest PR. It targets an older version of handle_tool_call that used client.aretain(**retain_kwargs). It refactors the call to client.aretain_batch(bank_id=..., items=[...], retain_async=True), pops bank_id/retain_async from kwargs, and changes the success message. Currently has merge conflicts (mergeable_state=dirty) because the upstream code has since been refactored — the tool handler already uses aretain_batch now.

#60648 (this PR, Jul 8) — Minimal fix against current main. The tool handler already calls client.aretain_batch(bank_id=..., items=[item]) — this PR simply adds retain_async=True to that existing call (+4/-2 lines). No refactoring needed because upstream already did the aretain → aretain_batch migration.

#37838 (by @kkangg1, Jul 8) — Adds retain_async=True to _build_retain_kwargs(). However, as documented in this PR's description, this is ineffective: the item dict gets retain_async popped via item.pop("retain_async", None) before being passed to aretain_batch, so the flag never reaches the client call.

Summary: #60648 is the correct minimal fix for current main. #22836 was the canonical fix but its refactoring has been partially superseded by upstream changes and now conflicts. #37838 doesn't actually work due to the pop. I'll close #37838 as superseded by this PR. A maintainer can pick between #22836 (needs rebase) and #60648 (clean, minimal).

[teknium1]

Thanks for targeting the current aretain_batch call site; the missing call-level flag is present on current main at plugins/memory/hindsight/__init__.py:1721.

Problems

• retain_async=True hard-codes behavior that is configurable today. The provider loads self._retain_async from config at plugins/memory/hindsight/__init__.py:1354, and sync_turn() forwards that value to aretain_batch() at plugins/memory/hindsight/__init__.py:1681-1686. A user with retain_async: false would have tool retains behave differently from auto-retains.
• The changed behavior lacks a tool-handler regression test. tests/plugins/memory/test_hindsight_provider.py:641-653 exercises this path but does not assert the call-level flag; the existing sync_turn test demonstrates both configuration values are meaningful at tests/plugins/memory/test_hindsight_provider.py:951-962.

Suggested changes

• Pass a captured self._retain_async value as the call-level argument instead of a literal.
• Add default-true and configured-false assertions for hindsight_retain.

Automated hermes-sweeper review.

[yingliang-zhang]

Thanks for the review @teknium1. Addressed the feedback: retain_async=True is now replaced with self._retain_async (read from provider config at init, same pattern as sync_turn at line 1664). This ensures users who set retain_async: false in config.yaml get consistent behavior across both code paths.

Pushed to the PR branch.

[yingliang-zhang]

Thanks for the triage note. We're keeping this PR open.

While #22836 targets the same aretain_batch call site, this PR's fix scope is narrower and the implementation diverges in approach. Both PRs can coexist — if the maintainers prefer to merge #22836 first, we'll rebase onto that and drop any overlapping changes. For now, keeping both open gives the maintainers flexibility on which approach to adopt.