fix(memory): make prefetch timeout configurable and keep late results (rebase of #130788)

[liuhao1024]

What does this PR do?

A slow external memory provider no longer gets "parked" for the rest of the session after one prefetch timeout. This is a salvage rebase of #130788 by @RaviTharuma (itself a recovery of #104562 after an accidental empty force-push): the branch had fallen behind main and conflicted on agent/agent_init.py, so the net diff is replayed onto current main with all main-side evolution since (recall dedupe, provider telemetry, spill config wiring, the api_server memory_manager reuse path) preserved.

Three behaviors are delivered on top of current main:

1. Configurable timeout — memory.external_prefetch_timeout (config.yaml, default 8.0s, matching the historical hard cap) bounds an external provider's synchronous prefetch. agent_init builds the manager through the new memory_manager_from_config() so the configured value takes effect; invalid or non-positive values fall back to the 8s default so a bad config cannot break startup.
2. The stuck-prefetch guard is process-wide — the in-flight external prefetch registry is keyed by provider name at class level instead of per MemoryManager instance. A manager is rebuilt on each agent init, so a per-instance registry let every fresh manager whose prefetch timed out spawn one more stuck memory-prefetch-* thread against the same provider with no upper bound ([Bug]: Timed-out external memory prefetch leaks one thread per MemoryManager instance #134392); shared, a stuck call suppresses new spawns from any manager until it returns. The late-result cache stays instance-local.
3. Late results are kept, not dropped — when a prefetch exceeds the timeout, the abandoned call's eventual result is cached (bound to the originating session id and query, spill-capped before store) and returned on the next matching turn instead of the provider staying skipped until the stuck call returns. The cache is generation-invalidated on turn start, session rotate/rewind, and compression so a reused manager can never replay another session's recall.

Related Issue

Fixes #104405
Fixes #134392
Salvages #130788

Type of Change

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

Changes Made

• agent/memory_manager.py — add resolve_external_prefetch_timeout() + memory_manager_from_config(); add the _LatePrefetch cache (session/query-bound, generation-invalidated on on_turn_start, on_session_rotate, on_compress); spill-cap late values before store and return; timeout warning now names the config key and states the provider stays active.
• agent/agent_init.py — construct the manager via memory_manager_from_config(mem_config) inside the existing is_core_memory_provider gate (keeps main's memory_manager reuse parameter untouched).
• hermes_cli/config_defaults.py — memory.external_prefetch_timeout: 8.0 default with docs comment.
• cli-config.yaml.example — document the new key in the memory: block.
• agent/memory_manager.py — hoist _external_prefetch_threads/_external_prefetch_lock to class attributes (_EXTERNAL_PREFETCH_THREADS/_EXTERNAL_PREFETCH_LOCK) so the stuck-call guard spans managers ([Bug]: Timed-out external memory prefetch leaks one thread per MemoryManager instance #134392); the late-result cache and its generation stay per instance.
• tests/agent/test_memory_provider.py — regression tests: timeout resolution (valid/invalid/missing), late-result reuse for the same session+query, invalidation across session rotation/compression, and a cross-manager invariant test (5 fresh managers against one stuck provider → exactly 1 live memory-prefetch-* thread; reproduces as 5 on unpatched main).

How to Test

1. pytest tests/agent/test_memory_provider.py -q — Observed result: 68 passed (was 55 on main; the 13 new tests cover config resolution, late-result reuse and invalidation, and the cross-manager stuck-thread invariant).
2. pytest tests/agent/ -q -k "memory or agent_init or config_default" — Observed result: 243 passed, 6 failed, 1 skipped; the same 6 fail identically on a clean upstream/main checkout (batch-order interaction in test_memory_write_bridge.py / test_inline_tool_executors_memory_args.py, unrelated to this change).
3. Smoke: memory_manager_from_config({"external_prefetch_timeout": 2.5})._external_prefetch_timeout == 2.5; non-float and None configs fall back to 8.0.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run pytest tests/ -q and all tests pass
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: macOS 26 (arm64), Python 3.14

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — the key is documented in cli-config.yaml.example and config_defaults.py
• I've updated cli-config.yaml.example if I added/changed config keys
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide — N/A (pure-Python threading/config logic, no platform surface)
• I've updated tool descriptions/schemas if I changed tool behavior — N/A

[JoaoMarcos44]

DEFECT-ONLY adversarial review

Provenance / exact-head binding

• PR #131357 — fix(memory): make prefetch timeout configurable and keep late results
• base: 382c91204d1a2f0c54c778321482ae8f9371a273
• merge-base: 367441274c48a03d12ee9f8d3d9ccd9bc1585392
• head: fd164da23858d98ca32ded0e714d75a93e67c7df
• changed ownership inspected: config → resolve_external_prefetch_timeout() → MemoryManager._external_prefetch_timeout → external-prefetch Thread.join(timeout)
• prior-review reconciliation: zero reviews / zero review threads at the publication rebind, so this does not duplicate an existing finding.
• CI/Nix/Docker are action_required; I did not treat that metadata as either RED or GREEN.

Coverage matrix

• ordinary finite positive timeout: control
• zero / negative / non-numeric timeout: existing fallback control
• YAML/string NaN: RED
• YAML/string +Infinity: RED
• timeout consumer: actual stdlib Thread.join(timeout)
• late-result/session-generation logic: inspected; no second confirmed defect retained

[P2] Non-finite timeout values bypass validation and crash external memory prefetch

REPRODUCTION

The new resolver converts arbitrary config input with float(raw), then rejects only value <= 0. IEEE NaN and +Infinity both survive that predicate.

I executed the downstream consumer under CPython 3.13.5 with the values the changed resolver accepts:

• Thread.join(float("nan")) → ValueError: Invalid value NaN (not a number)
• Thread.join(float("inf")) → OverflowError: timestamp out of range for platform time_t
• normal finite positive timeouts remain valid.

The config path is reachable from ordinary config.yaml: Hermes uses YAML safe_load, and both YAML .nan / .inf and strings "nan" / "inf" become values for which float(raw) is non-finite. The PR explicitly says invalid values fall back so a bad config cannot break execution, but these invalid values escape that boundary.

DIAGNOSIS

CLAIMED INVARIANT: invalid, missing, or non-positive memory.external_prefetch_timeout values fall back to 8s, so malformed configuration cannot break the memory path.

ACTUAL PRODUCTION PATH: config.yaml memory.external_prefetch_timeout → memory_manager_from_config() → resolve_external_prefetch_timeout() → non-finite float stored in MemoryManager → first external prefetch_all() → thread.join(self._external_prefetch_timeout) → exception before the provider result/error handling can run.

PRECONDITIONS / REACHABILITY: external memory provider enabled plus a user-authored non-finite value such as .nan, .inf, nan, or inf. No corrupted runtime state or provider failure is required.

NEGATIVE CONTROL: finite positives (for example 8 or 30) reach join() normally; 0, negative, and ordinary non-numeric strings already take the intended 8s fallback.

FACTS vs INFERENCES: acceptance of NaN/+Inf follows directly from the changed predicate; the observed join() exceptions are runtime facts. The only bounded environmental note is that the exact exception text/range for +Inf can vary by platform, but non-finite values are not legal finite timeout durations.

STRONGEST COUNTER-HYPOTHESIS / REBUTTAL: a schema could theoretically reject non-finite numbers before this resolver. This function is itself the newly introduced defensive config boundary and accepts both mappings and string values; repository config readers use YAML safe loading, which admits non-finite YAML floats. The resolver's own contract therefore cannot rely on an unstated upstream finite-number guarantee.

OBSERVABLE CONSEQUENCE / BLAST RADIUS: the first external-memory prefetch can raise from Thread.join() instead of falling back to the configured default, breaking the turn's memory-prefetch path for an otherwise parseable config. Scope is users with an external provider and a non-finite timeout value.

CALLER / CONTROLLING BOUNDARY / CONSUMER: config → resolve_external_prefetch_timeout (controlling validation owner) → MemoryManager → Thread.join.

FIX: require a finite positive value at the resolver boundary, e.g. reject when not math.isfinite(value) or value <= 0, and keep the constructor invariant consistent so direct construction cannot admit the same state.

RED-before → GREEN-after acceptance contract: parameterize .nan, .inf, nan, inf, 0, negative and junk values; all invalid cases must resolve to _EXTERNAL_PREFETCH_TIMEOUT_S. Add a real external-prefetch call proving none can escape into Thread.join(). Keep finite-positive override controls.

LIMITS OF EVIDENCE

The connected native Hermes workstation is offline, so this is not a full repository-suite receipt. The failing consumer was executed directly on CPython 3.13.5 and the producer/validation path is bound to the exact GitHub head above. No CI metadata was used as defect evidence.

[JoaoMarcos44]

[P2] Reject non-finite timeout values here. float('nan') and float('inf') both pass value <= 0, then reach the external-prefetch Thread.join(timeout). I reproduced join(nan) raising ValueError and join(inf) raising OverflowError on CPython 3.13.5. YAML config can produce .nan / .inf, and string nan / inf also survive float(raw). Please require math.isfinite(value) as well as value > 0, with regressions that drive the actual prefetch join.

[RaviTharuma]

Thanks @JoaoMarcos44 for the P2 catch on non-finite timeouts.

Opened a tiny follow-up that requires math.isfinite in resolve_external_prefetch_timeout / MemoryManager.__init__ plus regressions:

• Issue: [Bug]: Non-finite memory.external_prefetch_timeout bypasses validation and crashes Thread.join #131446
• PR: fix(memory): reject non-finite external_prefetch_timeout #131448
• One-commit cherry-pick onto this branch if preferred: 281808ae843eb142c0ddf0fe9489436f744c1396 (fix(memory): reject non-finite external_prefetch_timeout)

Happy for you to land that commit here and close #131448 as redundant.

[liuhao1024]

Thanks @RaviTharuma for the follow-up! Agreed the non-finite guard belongs in its own change — keeping this PR scoped to the configurability + late-result semantics. #131448 is the right home for the math.isfinite validation, so no cherry-pick needed here.

[Enough1122]

AI code review — automated review for reference; please use your judgment.

Making the timeout configurable and keeping late results both check out — the late-value cache replays to the next turn with the same query+session, is dropped on a different query, and the three invalidation hooks (:802, :821, :853) all bump the generation. One gap in the config path: NaN passes straight through and permanently disables the guard.

_init_memory → memory_manager_from_config(mem_config) → MemoryManager(external_prefetch_timeout=...) → thread.join(self._external_prefetch_timeout) (:592). Nothing on that path rejects NaN:

• resolve_external_prefetch_timeout (:48-53) only checks value <= 0 after float(raw) succeeds. NaN compares False against everything, so value <= 0 is False and :53 returns it unchanged.
• MemoryManager.__init__ (:392) has the same <= 0 guard, so NaN does not raise and :394 stores it.
• inf also survives both (value <= 0 is False for inf) — there join(inf) simply never times out, the opposite of what cli-config.yaml.example:1018-1021 promises.

NaN is the sharper one: thread.join(nan) returns immediately, so :594 if thread.is_alive() is always true and every external prefetch is judged timed out and returns "" no matter how fast the provider is:

resolve_nan=nan isnan=True
stored=nan isnan=True
prefetch_returned='' elapsed=0.00
cache=[] registry=[True]

Same provider, same 0.3s delay, on base 382c9120 (hard-coded 8s): prefetch_returned='R' elapsed=4.32.

Not theoretical — hermes_yaml accepts YAML 1.1 .nan/.inf straight through, end to end:

'external_prefetch_timeout: .nan'   -> nan
'external_prefetch_timeout: 1e999'  -> inf
load_config_readonly() -> nan ; resolve -> nan ; stored -> nan

With that one line in config.yaml, external memory recall is silently dead on that host, and every turn logs prefetch timed out after nan; skipping this turn — hard to connect back to the number in the config.

math.isfinite(value) alongside the existing check would send both NaN and inf back to _EXTERNAL_PREFETCH_TIMEOUT_S, matching what the docstring at :41-43 already promises ("invalid, missing, or non-positive values fall back"). The same check at :392 covers direct construction. A case for float("nan") alongside the existing test_memory_manager_from_config_uses_resolved_timeout (which only covers 30 and {}) would lock it down.

[liuhao1024]

Thanks — good catch on the NaN path. Fixed in 30d0f63: resolve_external_prefetch_timeout now falls back to the default for non-finite values (math.isfinite alongside the existing <= 0 check), matching what the docstring already promised, and the MemoryManager constructor raises on non-finite timeouts alongside <= 0 (direct construction stays the strict path).

Verified end to end on the head tree: float("nan"), float("inf"), 1e999, and YAML external_prefetch_timeout: .nan all resolve to the 8s default, while 30 still resolves to 30.0. Tests: test_resolve_external_prefetch_timeout_rejects_nan_and_inf, test_memory_manager_constructor_rejects_nan_and_inf, plus a NaN case in test_memory_manager_from_config_uses_resolved_timeout (67 passed in the provider suite; the two test_memory_sync_interrupted failures reproduce identically on the pre-fix head, so they're pre-existing on this branch, not from this change).