fix: openviking sync recall + increase API retry limit - #8474
fix: openviking sync recall + increase API retry limit#8474libinghui123 wants to merge 4 commits into
Conversation
- openviking: prefetch() now does a synchronous search with the current query instead of waiting for the previous turn's background search. Previously memories were always injected one turn late, and the first turn received no memories at all. - openviking: retain background prefetch as fallback if sync search fails. - run_agent: increase max_retries from 3 to 10 to improve resilience against transient provider errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
d1a83b5 to
c73f774
Compare
Add unit tests covering: - prefetch() performs sync search with current query - prefetch() uses current query, not stale cached background results - prefetch() returns empty on empty query / missing client - prefetch() falls back to cache on sync failure - queue_prefetch() starts background thread - sync_turn() increments turn count - on_session_end() commits session Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n recall Before: viking_remember() only added a message to the current session. The memory was only extracted and made searchable after the session ended or commitTokenThreshold was reached — meaning memories stored via viking_remember were NOT searchable in other channels/sessions. After: viking_remember() commits the session immediately after storing the message, so the memory is indexed and searchable across all sessions right away. Also updates the tool description to accurately reflect immediate indexing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- openviking: prefetch() now does a synchronous search with the current query instead of waiting for the previous turn's background search. Previously memories were always injected one turn late, and the first turn received no memories at all. - openviking: retain background prefetch as fallback if sync search fails. - openviking: viking_remember() now commits the session immediately so the memory is indexed and searchable across all sessions right away. - run_agent: increase max_retries from 3 to 10 to improve resilience against transient provider errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
b47ea38 to
690b0cb
Compare
ZaynJarvis
left a comment
There was a problem hiding this comment.
Review: Sync Recall + Retry Fixes
Verdict: Request Changes — please split into two PRs
This PR bundles two independent concerns: (1) sync recall implementation and (2) reliability fixes (max_retries, trust_env). The reliability fixes are good and mergeable today; the sync recall approach conflicts with #5838.
Blocking Issues
1. Sync recall in prefetch() conflicts with #5838's architecture
This PR implements sync recall by modifying prefetch() to do a synchronous search inline, overriding the background prefetch result. #5838 implements the same feature at the manager layer with a config gate, which is architecturally cleaner (the manager decides recall strategy, providers stay stateless). The two approaches conflict on both plugins/memory/openviking/__init__.py and run_agent.py. Since #5838 is the recommended merge, please extract the sync recall changes from this PR.
2. max_retries=10 changes the run loop retry count, not the HTTP retry count
The max_retries = 10 change is in the conversation loop's retry-on-API-error block — this is a good resilience improvement but should be in a separate PR so it doesn't get blocked by the sync recall conflict.
Cherry-Pick Candidates (extract into separate PR)
These two changes are clean, independent, and ready to merge:
# 1. In _VikingClient.__init__ — bypass corporate proxies for local endpoints
httpx_module = self._httpx
self._httpx = httpx_module.Client(trust_env=False)
# 2. In run_conversation() — increase API retry limit
max_retries = 10 # was 3Note on trust_env=False: This creates an httpx.Client instance stored as self._httpx. Since _VikingClient.get() calls self._httpx.get(), this correctly routes through the client's connection pool. However, the health() method also uses self._httpx.get() — verify that the timeout kwarg passes through cleanly on an httpx Client instance (it does, but worth a test).
What Is Good
- 206-line test suite is thorough and the
FakeHttpxfixture pattern (supporting both module-call and Client-call semantics) is well-designed. Please port the non-sync-recall tests into the cherry-pick PR. - Immediate commit after
viking_rememberis an interesting feature — but it belongs in a dedicated PR after the base session lifecycle is settled.
Summary
Three fixes for the OpenViking memory provider:
Changes
plugins/memory/openviking/__init__.pyprefetch()sync recall with current query;viking_remember()commits immediatelyrun_agent.pymax_retriesincreased from 3 to 10tests/plugins/memory/test_openviking_provider.pyTest Results
tests/plugins/memory/test_openviking_provider.py: 11/11 passedtests/agent/test_memory_provider.py: 54/54 passed (no regression)How to Test
viking_remember content="My favorite color is blue". Immediately switch to another channel (new session) and ask "What is my favorite color?". Verify it recalls correctly.Platforms Tested
Note
Ensure
OPENVIKING_ENDPOINTand related env vars are set in~/.hermes/.env(or the active dotenv) and thatno_proxyincludes the internal endpoint address to bypass corporate proxies.🤖 Generated with Claude Code