Skip to content

fix(hermes): pass retain_async=True to aretain_batch in tool handler - #4671

Closed
yingliang-zhang wants to merge 6 commits into
vectorize-io:mainfrom
yingliang-zhang:fix/hindsight-retain-async-tool-handler
Closed

yingliang-zhang wants to merge 6 commits into
vectorize-io:mainfrom
yingliang-zhang:fix/hindsight-retain-async-tool-handler

Conversation

@yingliang-zhang

@yingliang-zhang yingliang-zhang commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Problem

The hindsight_retain tool handler called self._retain_batch(item, bank_id=self._bank_id) without forwarding the configured self._retain_async, so tool-initiated retains silently dropped the provider's async/sync choice — aretain_batch fell back to its own server default. retain_async is a call-level arg (never an item key — _retain_batch's contract), so the tool handler must pass it explicitly; the auto-retain path (_make_turn_retain_job → _retain_batch) already does.

Concretely: a user with retain_async: false got synchronous semantics for auto-retains but server-default semantics for tool retains — and on banks with significant data the LLM fact-extraction call can take 60–600+ s, far exceeding the 120 s default timeout, surfacing as an opaque TimeoutError ("Failed to store memory: "). With the flag forwarded, retain_async: true tool retains return as soon as the server accepts the request, matching sync_turn.

Fix

Forward the configured mode as a call argument, matching the auto-retain path:

self._retain_batch(item, bank_id=self._bank_id, retain_async=self._retain_async)

This preserves the review feedback applied on the original PR (pass the captured self._retain_async, not a hard-coded True, so an explicit retain_async: false is honored on both paths).

Tests

tests/test_retain_async_forwarding.py — test_retain_tool_forwards_configured_retain_async, parametrized over True/False, asserting the call-level retain_async matches the provider config (port of the original parameterized test, adapted to this tree's recording FakeClient).

Verification

uv run pytest tests/test_retain_async_forwarding.py -v (only the touched test file):

collecting ... collected 2 items

tests/test_retain_async_forwarding.py::test_retain_tool_forwards_configured_retain_async[True] PASSED [ 50%]
tests/test_retain_async_forwarding.py::test_retain_tool_forwards_configured_retain_async[False] PASSED [100%]

============================== 2 passed in 0.05s ===============================

Mutation check — reverting the one-line fix makes both cases RED:

FAILED tests/test_retain_async_forwarding.py::test_retain_tool_forwards_configured_retain_async[True] - KeyError: 'retain_async'
FAILED tests/test_retain_async_forwarding.py::test_retain_tool_forwards_configured_retain_async[False] - KeyError: 'retain_async'
============================== 2 failed in 0.09s ===============================

uv run pytest tests/test_provider.py (existing suite exercising the touched path, unchanged): 15 passed in 0.28s.

Provenance

Ported from NousResearch/hermes-agent#60648 (closed in the hindsight-move close pass; the bundled provider moved to hindsight-integrations/hermes in NousResearch/hermes-agent#119888). Original fix and analysis by @yingliang-zhang; review feedback by @teknium1 (configured value instead of hard-coded True, parameterized coverage) is incorporated. Tracked in #4662.

Stacked PR (pre-chained 2026-09-30): the diff vs main is cumulative with #4674; this PR's own change is forwarding the configured retain_async to aretain_batch in the hindsight_retain tool handler. Merge top-down: 4674 -> 4671 -> 4672 -> 4673 -> 4675 -> 4676. Identical shared commits merge cleanly in any order (git dedups same-patch-both-sides).

_client is read/written from the retain writer, the prefetch worker and
the turn/tool thread with no lock. _get_client() was check-then-act and
the embedded constructor takes seconds: two threads hitting a cold or
just-nulled client both construct, and the loser's client is orphaned
with an aiohttp session nothing ever closes ("Unclosed client session"
noise). The stale-daemon retry's inline null-and-rebuild widened the
window to seconds and concurrent retries clobbered each other's
replacement.

- _client_lock, a LEAF lock: _get_client() takes it only on the
  construction/retire path; the fast path stays lock-free; never held
  across _run_sync/operation(client) or while taking _prefetch_lock /
  _pending_retain_ops_lock.
- _get_client(*, retire=<client>): the retry retires the EXACT client
  the operation ran with — identity passed as an argument, never shared
  broken-client state a concurrent retry could clobber — and rebuilds
  exactly once under the lock (identity CAS: a sibling's fresh rebuild
  is returned as-is instead of being orphaned).
- shutdown() retires the client under the lock BEFORE closing it
  (_close_client_of(client), parameterised); a concurrent _get_client()
  rebuilds instead of racing the close.

Ported from NousResearch/hermes-agent#117236 (closed in the hindsight-move
close pass). Tracked in vectorize-io#4662.
@yingliang-zhang

Copy link
Copy Markdown
Contributor Author

CI note: verify-generated-files fails here for a pre-existing reason — ./scripts/generate-docs-skill.sh on a clean upstream/main (7a593e8) reproduces the same skills/hindsight-docs/references/developer/oracle.md drift, so the gate is red on every PR including unmodified main. Fixed by #4678 (single-file regeneration); once it merges, this PR's gate goes green with no changes needed here.

@yingliang-zhang
yingliang-zhang force-pushed the fix/hindsight-retain-async-tool-handler branch from b177530 to 4d84463 Compare September 23, 2026 14:19
@yingliang-zhang

Copy link
Copy Markdown
Contributor Author

Generated-files sync commit added atop the substantive commit(s): the verify-generated-files gate runs its generators against the PR head (actions/checkout at ref: head.sha), so this branch now carries its own regenerated skills/hindsight-docs/references/developer/oracle.md plus the lint.sh ruff formatting, committed as chore: sync generated files for verify-generated-files. No substantive changes — the review surface is exactly as before.

Conflict resolution (both intents kept): hindsight-integrations/hermes/__init__.py merges the leaf-lock client lifecycle (_client_lock, _get_client(retire=...), _close_client_of(client), shutdown retire-then-close) with main's daemon-based embedded client (_embedded_url, HindsightEmbedded removal). _close_client_of keeps the PR's parameterized identity with main's simplified single-aclose body.

skills/hindsight-docs/references/developer/oracle.md auto-merged to main's regenerated blob (a8d62fc): the PR's vectorize-io#4678 mirror is subsumed by main's generate-docs-skill.sh output from the same source (byte-verified). Checks: 37 hermes tests passed incl. test_client_lifecycle.py; ruff check+format clean per scripts/hooks/lint.sh's hermes path.
The hindsight_retain tool handler called _retain_batch without forwarding
the configured self._retain_async, so tool retains silently dropped the
async/sync choice and aretain_batch fell back to its own server default.
retain_async is a call-level arg (never an item key), so the tool handler
must pass it explicitly. Forward the configured mode, matching the
auto-retain path.

Ported from NousResearch/hermes-agent#60648 (closed in the hindsight-move
close pass). Tracked in vectorize-io#4662.
@nicoloboschi

Copy link
Copy Markdown
Collaborator

Thanks for this! We're tracking this problem in #4662 and will fix it from there, so I'm closing this PR.

We've stopped accepting pull requests from outside the team (see CONTRIBUTING.md). Any extra detail or steps to reproduce on the issue are very welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants