fix(honcho): delegate the config.yaml timeout read to load_config_readonly
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.
This commit is contained in:
parent
9769facae1
commit
e4cdd8d9ad
2 changed files with 66 additions and 30 deletions
|
|
@ -856,39 +856,27 @@ class HonchoClientConfig:
|
|||
|
||||
_honcho_client_slot: SingletonSlot = SingletonSlot()
|
||||
_cached_timeout: float | None = None
|
||||
# Memo for the config.yaml-derived timeout, keyed on the file's mtime_ns so
|
||||
# Memo for the honcho.json-derived timeout, keyed on the file's mtime_ns so
|
||||
# the staleness check on every get_honcho_client() call costs one stat()
|
||||
# instead of a full YAML load. (None, None) = not yet populated.
|
||||
_config_timeout_memo: tuple[int | None, float | None] = (None, None)
|
||||
# Same memoization for the honcho.json-derived timeout; mtime -1 = file absent.
|
||||
# instead of a JSON parse. mtime -1 = file absent; (None, None) = not yet
|
||||
# populated. config.yaml needs no such memo: load_config_readonly() is
|
||||
# internally cached on both the user and managed files' signatures, and a
|
||||
# bespoke key here would have to duplicate that invalidation logic.
|
||||
_honcho_json_timeout_memo: tuple[int | None, float | None] = (None, None)
|
||||
|
||||
|
||||
def _config_yaml_timeout() -> float | None:
|
||||
"""Read honcho.timeout / honcho.request_timeout from config.yaml, memoized on mtime."""
|
||||
global _config_timeout_memo
|
||||
"""Read honcho.timeout / honcho.request_timeout via the cached config loader."""
|
||||
try:
|
||||
from hermes_constants import get_hermes_home
|
||||
from hermes_cli.config import load_config_readonly
|
||||
|
||||
cfg_path = get_hermes_home() / "config.yaml"
|
||||
try:
|
||||
mtime_ns: int | None = cfg_path.stat().st_mtime_ns
|
||||
except OSError:
|
||||
mtime_ns = None
|
||||
if _config_timeout_memo[0] is not None and _config_timeout_memo[0] == mtime_ns:
|
||||
return _config_timeout_memo[1]
|
||||
|
||||
from hermes_cli.config import load_config
|
||||
|
||||
honcho_cfg = load_config().get("honcho", {})
|
||||
timeout = None
|
||||
honcho_cfg = load_config_readonly().get("honcho", {})
|
||||
if isinstance(honcho_cfg, dict):
|
||||
timeout = _resolve_optional_float(
|
||||
return _resolve_optional_float(
|
||||
honcho_cfg.get("timeout"),
|
||||
honcho_cfg.get("request_timeout"),
|
||||
)
|
||||
_config_timeout_memo = (mtime_ns, timeout)
|
||||
return timeout
|
||||
return None
|
||||
except Exception:
|
||||
return None
|
||||
|
||||
|
|
@ -1118,8 +1106,7 @@ def get_honcho_client(config: HonchoClientConfig | None = None) -> Honcho:
|
|||
|
||||
def reset_honcho_client() -> None:
|
||||
"""Reset the Honcho client singleton (useful for testing)."""
|
||||
global _cached_timeout, _config_timeout_memo, _honcho_json_timeout_memo
|
||||
global _cached_timeout, _honcho_json_timeout_memo
|
||||
_honcho_client_slot.reset()
|
||||
_cached_timeout = None
|
||||
_config_timeout_memo = (None, None)
|
||||
_honcho_json_timeout_memo = (None, None)
|
||||
|
|
|
|||
|
|
@ -741,6 +741,11 @@ class TestGetHonchoClient:
|
|||
)
|
||||
def test_timeout_change_triggers_client_rebuild(self):
|
||||
"""Changing timeout config must rebuild the cached client."""
|
||||
from hermes_constants import get_hermes_home
|
||||
|
||||
cfg_yaml = get_hermes_home() / "config.yaml"
|
||||
cfg_yaml.write_text("honcho:\n timeout: 30\n")
|
||||
|
||||
fake_honcho_1 = MagicMock(name="Honcho_v1")
|
||||
fake_honcho_2 = MagicMock(name="Honcho_v2")
|
||||
cfg = HonchoClientConfig(
|
||||
|
|
@ -749,30 +754,74 @@ class TestGetHonchoClient:
|
|||
environment="production",
|
||||
)
|
||||
|
||||
with patch("honcho.Honcho", return_value=fake_honcho_1) as mock_h1, \
|
||||
patch("hermes_cli.config.load_config", return_value={"honcho": {"timeout": 30}}):
|
||||
with patch("honcho.Honcho", return_value=fake_honcho_1) as mock_h1:
|
||||
client1 = get_honcho_client(cfg)
|
||||
|
||||
assert client1 is fake_honcho_1
|
||||
assert mock_h1.call_args.kwargs["timeout"] == 30.0
|
||||
|
||||
# Same config — should return cached client (no rebuild)
|
||||
with patch("honcho.Honcho", return_value=fake_honcho_2) as mock_h2, \
|
||||
patch("hermes_cli.config.load_config", return_value={"honcho": {"timeout": 30}}):
|
||||
with patch("honcho.Honcho", return_value=fake_honcho_2) as mock_h2:
|
||||
client2 = get_honcho_client(cfg)
|
||||
|
||||
assert client2 is fake_honcho_1 # still cached
|
||||
mock_h2.assert_not_called()
|
||||
|
||||
# Changed timeout — must rebuild
|
||||
with patch("honcho.Honcho", return_value=fake_honcho_2) as mock_h3, \
|
||||
patch("hermes_cli.config.load_config", return_value={"honcho": {"timeout": 300}}):
|
||||
cfg_yaml.write_text("honcho:\n timeout: 300\n")
|
||||
st = cfg_yaml.stat()
|
||||
os.utime(cfg_yaml, ns=(st.st_atime_ns, st.st_mtime_ns + 1_000_000))
|
||||
|
||||
with patch("honcho.Honcho", return_value=fake_honcho_2) as mock_h3:
|
||||
client3 = get_honcho_client(cfg)
|
||||
|
||||
assert client3 is fake_honcho_2 # rebuilt
|
||||
mock_h3.assert_called_once()
|
||||
assert mock_h3.call_args.kwargs["timeout"] == 300.0
|
||||
|
||||
@pytest.mark.skipif(
|
||||
not importlib.util.find_spec("honcho"),
|
||||
reason="honcho SDK not installed"
|
||||
)
|
||||
def test_managed_config_timeout_does_not_thrash_singleton(self, tmp_path, monkeypatch):
|
||||
"""A managed-scope honcho.timeout with no user config.yaml must be seen
|
||||
by the staleness check (stable reuse), and a managed edit must trigger
|
||||
a rebuild. Regression for a memo that keyed only on the user file."""
|
||||
managed_dir = tmp_path / "managed"
|
||||
managed_dir.mkdir()
|
||||
managed_cfg = managed_dir / "config.yaml"
|
||||
managed_cfg.write_text("honcho:\n timeout: 88\n")
|
||||
monkeypatch.setenv("HERMES_MANAGED_DIR", str(managed_dir))
|
||||
|
||||
fake_honcho_1 = MagicMock(name="Honcho_v1")
|
||||
fake_honcho_2 = MagicMock(name="Honcho_v2")
|
||||
cfg = HonchoClientConfig(
|
||||
api_key="test-key",
|
||||
workspace_id="hermes",
|
||||
environment="production",
|
||||
)
|
||||
|
||||
with patch("honcho.Honcho", return_value=fake_honcho_1) as mock_h1:
|
||||
client1 = get_honcho_client(cfg)
|
||||
client2 = get_honcho_client(cfg)
|
||||
|
||||
assert client1 is fake_honcho_1
|
||||
assert client2 is fake_honcho_1
|
||||
assert mock_h1.call_count == 1
|
||||
assert mock_h1.call_args.kwargs["timeout"] == 88.0
|
||||
|
||||
# A managed-timeout edit is detected (same-size write, so bump mtime).
|
||||
managed_cfg.write_text("honcho:\n timeout: 99\n")
|
||||
st = managed_cfg.stat()
|
||||
os.utime(managed_cfg, ns=(st.st_atime_ns, st.st_mtime_ns + 1_000_000))
|
||||
|
||||
with patch("honcho.Honcho", return_value=fake_honcho_2) as mock_h2:
|
||||
client3 = get_honcho_client(cfg)
|
||||
|
||||
assert client3 is fake_honcho_2
|
||||
mock_h2.assert_called_once()
|
||||
assert mock_h2.call_args.kwargs["timeout"] == 99.0
|
||||
|
||||
@pytest.mark.skipif(
|
||||
not importlib.util.find_spec("honcho"),
|
||||
reason="honcho SDK not installed"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue