fix(honcho): timeout staleness check must resolve from the same sources as the build path - #66428
Merged
teknium1 merged 2 commits intoJul 17, 2026
Merged
Conversation
…e the build path The staleness check added in NousResearch#66052 resolved the timeout from env, config.yaml, and the default only, while the build path also reads the honcho.json host block (timeout/requestTimeout). With a timeout configured in honcho.json, the two permanently disagreed: every no-config get_honcho_client() call — i.e. every HonchoSessionManager .honcho property access — interpreted the mismatch as a config change and tore down and rebuilt the client, defeating the singleton on the hot path it was meant to protect. Teach the check to read honcho.json through the same host-aware chain as from_global_config, memoized on the file's mtime_ns so the per-call cost stays one stat(). A genuine honcho.json timeout change is now also detected, extending NousResearch#57437 to that config surface.
…donly The staleness check's bespoke mtime memo keyed only on the user config.yaml, but load_config() merges the managed-scope config (HERMES_MANAGED_DIR/config.yaml, /etc/hermes) whose leaf keys win. A managed honcho.timeout with no user config.yaml made the memo cache 'no timeout' while _build resolved the managed value — the same perpetual-rebuild mismatch this PR fixes for honcho.json. A managed timeout edit was likewise invisible while the user file's mtime stayed put. load_config_readonly() is already cached on both files' signatures plus the env-ref snapshot, so use it instead of duplicating that invalidation logic; the defensive deepcopy the old memo existed to avoid is skipped by the readonly variant. Drive the rebuild test through a real config.yaml and add a HERMES_MANAGED_DIR regression test covering stable reuse and managed-timeout edits.
erosika
force-pushed
the
fix/honcho-timeout-staleness
branch
from
July 17, 2026 19:38
a12d756 to
137ffb2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #66052. The timeout staleness check added there resolves the timeout from different config sources than the build path does, and when the timeout is configured in
honcho.jsonthe two permanently disagree — which the check interprets as a config change on every call, tearing down and rebuilding the client singleton each time.The bug
_resolve_timeout_from_sourceschecked: explicit config →HONCHO_TIMEOUTenv →config.yaml→ 30.0 default. It never readhoncho.json.from_global_config) resolves the timeout from thehoncho.jsonhost block (timeout/requestTimeout) and root-level keys — a supported surface (test_hermes_request_timeout_alias_usedasserts it).HonchoSessionManager.honchocallsget_honcho_client()with no config on every property access.So with e.g.
"hosts": {"hermes": {"requestTimeout": 120}}: the client is built with_cached_timeout = 120, the checker resolves 30.0, sees a "change", resets the singleton, and the rebuild lands back on 120 — forever. Every Honcho operation in a long-lived gateway pays a full client rebuild (config re-read, OAuth pre-refresh, new connection pool), which is exactly the hot path the mtime memo in #66052 was added to keep cheap.The fix
_resolve_timeout_from_sourcesnow mirrors the build path exactly: with an explicit config it matches_build(config → config.yaml → default); with no config it matchesfrom_global_config+_build(honcho.json host block/root → env → config.yaml → default). The honcho.json read is memoized on the file'sst_mtime_nsso the per-call cost stays onestat(). A genuine honcho.json timeout change is now also detected, extending fix(plugins): rebuild Honcho client when timeout config changes #57437 to that config surface.config.yamlread now goes throughload_config_readonly()instead of a bespoke user-file-mtime memo. The bespoke memo keyed only on the userconfig.yaml, butload_config()merges the managed-scope config (HERMES_MANAGED_DIR/config.yaml,/etc/hermes) whose leaf keys win — so a managedhoncho.timeoutwith no user file produced the same perpetual-rebuild mismatch, and a managed timeout edit was invisible while the user file's mtime stayed put.load_config_readonly()is already cached on both files' signatures plus the env-ref snapshot, and skips the defensive deepcopy the old memo existed to avoid — no invalidation logic to duplicate.Validation
test_honcho_json_timeout_does_not_thrash_singleton) fails on pre-fix client.py, passes with fixtest_managed_config_timeout_does_not_thrash_singleton, realHERMES_MANAGED_DIR): stable reuse with managed-only timeout + rebuild on managed edittest_timeout_change_triggers_client_rebuildrewritten to drive a realconfig.yaml(write → rewrite + mtime bump) instead of patchingload_configtests/honcho_plugin/+tests/test_honcho_client_config.py+ memory provider suites