Skip to content

fix(memory): honor external prefetch timeout - #87028

Open
richardclawbot wants to merge 5 commits into
NousResearch:mainfrom
richardclawbot:fix/configurable-memory-prefetch-timeout
Open

richardclawbot wants to merge 5 commits into
NousResearch:mainfrom
richardclawbot:fix/configurable-memory-prefetch-timeout

Conversation

@richardclawbot

Copy link
Copy Markdown

Why

memory.external_prefetch_timeout existed in profile config but was never passed to the MemoryManager, so every external-memory provider used the hard-coded 8s join.

Change

  • Declare the setting in DEFAULT_CONFIG.
  • Parse a positive numeric value defensively and wire it into the external MemoryManager construction.
  • Preserve the 8s default for absent, invalid, and non-positive values.

Verification

  • uv run --extra dev pytest tests/agent/test_agent_init_memory_prefetch_timeout.py tests/agent/test_memory_provider.py -q — 68 passed
  • uv run ruff check agent/agent_init.py hermes_cli/config_defaults.py tests/agent/test_agent_init_memory_prefetch_timeout.py
  • HERMES_HOME=... uv run hermes config check — config recognizes the setting
  • Fresh-profile construction proof: configured 0.5s → manager 0.5s; Terra default/high read back.

Risk

Only external memory prefetch joins are bounded; recall that exceeds the configured bound is skipped for that turn, exactly as the existing timeout path specifies.

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers area/config Config system, migrations, profiles area/memory Memory subsystem: store, providers, sync, background reviews P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 15, 2026
@richardclawbot

Copy link
Copy Markdown
Author

Review remediation: the first adversarial review reproduced that YAML non-finite timeout values (.nan, ±infinity, overflow) could disable external recall. Commit 999db3bdfb0b2d4c5c7f5301b580b0e981368145 rejects those values in config parsing and defensively in MemoryManager; adds regressions.

Verification after remediation:

  • focused pytest: 80 passed
  • canonical clean-env runner: passed
  • Ruff + diff check: clean
  • fresh profile proof: configured 0.5 → manager 0.5; model gpt-5.6-terra, reasoning high.

GitHub exposes no fork CI checks for this branch; the gh pr checks --watch watcher exited with no checks reported.

@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

fix(memory): honor external prefetch timeout

  1. The hardening is correct and closes the real bug: math.nan previously slipped past the <= 0 check (NaN comparisons are False) and effectively disabled external recall; the explicit isfinite/positive/bounded checks in both agent_init._external_prefetch_timeout_from_config and MemoryManager.__init__ (agent/memory_manager.py) fix it, and bool is correctly rejected before the float() coercion. Good.
  2. Flake risk: test_configured_half_second_timeout_bounds_external_recall asserts 0.45 <= elapsed < 0.8 around a 0.5s timeout — a narrow wall-clock window on a loaded CI runner (repo flake policy prefers event-based sync or loose bounds). Consider widening to e.g. 0.4 <= elapsed < 2.0 plus a provider-side "still blocked" signal instead of the tight upper bound.
  3. Out-of-range values (> 60s) silently fall back to the 8s default with only a warning — a user setting external_prefetch_timeout: 120 gets a quarter of what they asked with no error surfaced. Consider clamping to the max with a warning (or failing loudly) so the intent is not silently reduced.
  4. mem_config was hoisted out of the skip_memory gate — behavior-neutral, and the try/except around MemoryManager construction covers the new path. Also note _config_version does not need a bump since the new key merges via deep-merge — correct as-is.

@rodrigogs

Copy link
Copy Markdown

Same failure mode from a different setup: Bedrock-only Docker gateway, memory.provider: holographic, dense retrieval via a local Ollama embedder (qwen3-embedding:4b) behind a single llama-server slot. Over about an hour the manager's 8s join fired 4 times ("prefetch timed out after 8.0s"), coinciding with Ollama embeds of 18.4s, 24.8s, 47.1s, or a 400 after 60s. Separately, 101 of 197 agent starts injected no external memory - I can't pin all of those on the 8s cap, since the stuck-thread skip only spans turns inside one cached session agent (gateway/run.py:3473). A configurable bound and embedder speed are the two levers, and this PR is the first.

On visibility: the gateway already surfaces the :426 WARNING on stderr by default (verbosity=0: gateway/run.py:4881-4890, :5159; hermes_cli/gateway.py:4513); only the plain CLI needs --verbose. The per-turn :419 DEBUG "still running" skip reaches nobody either way, which is why a bound the operator can tune matters more than more logging.

Two gaps on this head: it's dirty against main - construction moved into _init_memory (agent_init.py:1222-1290, call at :1271), mem_config already sourced at :1249, so the hoisted mem_config hunk isn't needed. cli-config.yaml.example's memory: block (ends line 908) is missing the new key.

Not opening a competing PR - this shape is right, just needs a rebase.

This branch has not been deployed

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

Labels

area/config Config system, migrations, profiles area/memory Memory subsystem: store, providers, sync, background reviews comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have 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.

5 participants