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

[kkangg1]

The hindsight_retain tool calls client.aretain() without retain_async, defaulting to False (synchronous mode). On banks with significant data, synchronous processing exceeds the 120s timeout, producing:

Tool hindsight_retain returned error (120.01s): {"error": "Failed to store memory: "}

The auto-retain path (sync_turn) works correctly because it passes retain_async=self._retain_async (True) to aretain_batch.

Fix add retain_async=True to _build_retain_kwargs in the tool handler (1 line).

What does this PR do?

Add retain_async=True to the _build_retain_kwargs call in the hindsight_retain tool handler, making the tool use asynchronous processing consistent with the auto-retain path (sync_turn). Previously the tool defaulted to retain_async=False (synchronous mode), causing timeouts on banks with significant data when the Hindsight APIs LLM extraction took longer than 120 seconds.

Related Issues

• Closes Embedded Hindsight retain reports failure while async retain later appears in recall #29079 (Embedded Hindsight retain reports failure while async retain later appears in recall)
• References Bug: hindsight_retain tool fails with 'Connection refused' while hindsight_recall works #7974 (Bug hindsight_retain tool fails with Connection refused while hindsight_recall works)
• Follow-up to PR feat(hindsight): richer session-scoped retain metadata (salvage of #6290) #13987 (feat richer session-scoped retain metadata introduced _build_retain_kwargs without retain_async)
• Similar to PR [Bug]: Hindsight plugin hardcoded 30s timeout causes hindsight_reflect to fail #9869 (Hindsight plugin hardcoded 30s timeout causes hindsight_reflect to fail same timeout pattern)

Type of Change

• Bug fix (non-breaking change that fixes an issue)

Changes Made

• plugins/memory/hindsight/init.py Add retain_async=True parameter to _build_retain_kwargs() call in handle_tool_call for hindsight_retain (line 1508)

How to Test

1. Start Hermes with memory provider hindsight (local or cloud)
2. Call hindsight_retain with any content via CLI or gateway session
3. Observe the tool returns success within seconds instead of timing out at 120s
4. Verify retained content appears in hindsight_recall

Checklist

Code

• Ive read the Contributing Guide
• My commit messages follow Conventional Commits
• I searched for existing PRs to make sure this isnt a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)

Documentation

• N/A Documentation update not needed (one-line fix

[yingliang-zhang]

Independent reproduction + a note on the fix approach.

Reproduction

Environment: Hermes Agent, Hindsight local_embedded mode, GLM model as extraction LLM, idle_timeout: 0 (default at setup time).

Symptom: hindsight_retain tool consistently times out at 120s with an empty error message:

Tool hindsight_retain returned error (120.02s): {"error": "Failed to store memory: "}

Daemon logs show worker tasks stuck at llm.openai.retain_extract_facts+structured for 300-691s, occupying all worker slots. New retain requests queue but never get processed, hitting the 120s _DEFAULT_TIMEOUT.

The auto-retain path (sync_turn) works fine because it passes retain_async=True to aretain_batch. The tool call path does not.

Note on the fix approach

This PR adds retain_async=True to _build_retain_kwargs(). However, in the current code, handle_tool_call does item.pop("retain_async", None) (line ~1717) immediately after _build_retain_kwargs returns, which would remove retain_async from the item dict before aretain_batch is called.

The working fix is to pass retain_async=True directly to aretain_batch as a call argument, consistent with how sync_turn does it (line ~1685):

client.aretain_batch(bank_id=self._bank_id, items=[item], retain_async=True)

This matches the existing comment at line ~1715: "aretain_batch takes bank_id/retain_async as call args, not item keys."

Would the maintainer prefer this PR updated, or should I submit a separate one with the corrected approach? Happy to help either way — just want to see this land.

[yingliang-zhang]

Thanks for this PR! I was investigating the same issue (#29079) and noticed that this approach may not be effective.

The issue

The retain_async=True is added to _build_retain_kwargs(), which stores it in the returned kwargs dict:

if retain_async is not None:
    kwargs["retain_async"] = retain_async

However, immediately after _build_retain_kwargs() returns, the calling code pops it out:

item = self._build_retain_kwargs(
    content,
    context=context,
    tags=args.get("tags"),
    retain_async=True,  # ← this PR adds it here
)
# aretain_batch takes bank_id/retain_async as call args, not item keys.
item.pop("bank_id", None)
item.pop("retain_async", None)  # ← immediately removed, making the change a no-op

So retain_async=True never reaches aretain_batch — the behavior remains synchronous.

Suggested fix

Pass retain_async=True directly to aretain_batch as a call argument (consistent with the auto-retain path in sync_turn at ~L1685):

self._run_hindsight_operation(
    lambda client: client.aretain_batch(
        bank_id=self._bank_id, items=[item], retain_async=True,
    )
)

I have opened #60648 with this approach — happy to close it if you'd like to update this PR instead. Either way, wanted to flag the issue so the fix actually takes effect.

[yingliang-zhang]

Closing in favor of #60648, which is the correct minimal fix for current main.

As documented in #60648's description: adding retain_async=True to _build_retain_kwargs() is ineffective because item.pop("retain_async", None) strips the flag before it reaches client.aretain_batch(). The fix must be at the aretain_batch call site itself, which is what #60648 does.

[yingliang-zhang]

Marking this as superseded by #60648, which is the correct minimal fix for current main.

As documented in #60648's description: adding retain_async=True to _build_retain_kwargs() is ineffective because item.pop("retain_async", None) strips the flag before it reaches client.aretain_batch(). The fix must be at the aretain_batch call site itself, which is what #60648 does.

A maintainer can close this PR.

[teknium1]

Thanks for isolating the tool-path timeout issue. The underlying behavior is still present on current main, but this specific placement does not reach the Hindsight client.

Problems

• plugins/memory/hindsight/__init__.py:1715-1721 removes item["retain_async"] before calling aretain_batch, so adding it to _build_retain_kwargs() is a no-op.
• plugins/memory/hindsight/__init__.py:1354 keeps the configured value in self._retain_async; the auto-retain path forwards that value at :1681-1686. Hard-coding True would diverge from configured retain_async: false behavior.
• tests/plugins/memory/test_hindsight_provider.py:641-653 has no assertion for the tool call's call-level retain_async keyword.

Suggested changes

• Pass retain_async=self._retain_async directly to client.aretain_batch(...) in the tool handler after item cleanup.
• Add regression coverage for default-true and configured-false tool retains.

Automated hermes-sweeper review.

[teknium1]

handle_tool_call removes item["retain_async"] immediately before aretain_batch() (current main: :1715-1721), so this item-level value never reaches Hindsight. Pass retain_async=self._retain_async to aretain_batch(...) instead, matching the auto-retain path and preserving configured retain_async: false.

[kkangg1]

Closing in favor of #60648, which is the correct minimal fix.

As @yingliang-zhang documented: adding retain_async=True to _build_retain_kwargs() is ineffective because item.pop("retain_async", None) at ~L1717 strips the flag before client.aretain_batch() is called. The fix must be at the aretain_batch call site itself, which is what #60648 does.

Thanks @yingliang-zhang for the careful reproduction and @teknium1 for the review.