mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-31 19:16:29 +00:00
refactor: fold simplify-code review findings
- extract _commit_registry/_note_refresh_failure shared by the background worker and foreground stage-4 (identical 4-step success + failure paths were duplicated); worker now commits under _models_dev_fetch_lock so a failing background refresh can never re-arm the backoff immediately after a successful force_refresh committed (unsynchronized-write race) - add should_clear_context_pin_async to hermes_cli/route_identity.py (matching the get_model_context_length_async precedent) and use it at the 4 async gateway sites instead of inline asyncio.to_thread wraps; the sync _format_session_info site keeps the sync call (already off-loop via its callers' to_thread) - test the background-refresh success path (the PR's primary new behavior): disk saved, mem cache swapped, backoff cleared, in_flight reset — mutation-checked - replace the race-prone spin-wait on _models_dev_refresh_in_flight with a named-thread join in the backoff test
This commit is contained in:
parent
ccf7129ed0
commit
222ea2b6c9
5 changed files with 109 additions and 52 deletions
|
|
@ -273,27 +273,52 @@ def _mark_stale_cache_grace() -> None:
|
|||
_models_dev_cache_time = grace_time
|
||||
|
||||
|
||||
def _commit_registry(data: Dict[str, Any], *, where: str) -> None:
|
||||
"""Persist a freshly fetched registry: disk + in-mem + clear backoff.
|
||||
|
||||
Callers must hold ``_models_dev_fetch_lock`` so a failing refresh on one
|
||||
path can never stomp the state a succeeding refresh on the other path
|
||||
just committed (e.g. a failing background worker re-arming the backoff
|
||||
immediately after a successful ``force_refresh``).
|
||||
"""
|
||||
global _models_dev_cache, _models_dev_cache_time, _models_dev_retry_after
|
||||
_save_disk_cache(data)
|
||||
_models_dev_cache = data
|
||||
_models_dev_cache_time = time.time()
|
||||
_models_dev_retry_after = 0
|
||||
logger.debug(
|
||||
"Refreshed models.dev registry (%s): %d providers, %d total models",
|
||||
where,
|
||||
len(data),
|
||||
sum(len(p.get("models", {})) for p in data.values() if isinstance(p, dict)),
|
||||
)
|
||||
|
||||
|
||||
def _note_refresh_failure(exc: Exception, *, where: str) -> None:
|
||||
"""Record a failed refresh: arm the process-wide 5-minute backoff.
|
||||
|
||||
Callers must hold ``_models_dev_fetch_lock`` (see ``_commit_registry``).
|
||||
"""
|
||||
global _models_dev_retry_after
|
||||
_models_dev_retry_after = time.time() + _MODELS_DEV_RETRY_DELAY
|
||||
logger.debug(
|
||||
"models.dev refresh failed (%s); retry suppressed for %ds: %s",
|
||||
where,
|
||||
_MODELS_DEV_RETRY_DELAY,
|
||||
exc,
|
||||
)
|
||||
|
||||
|
||||
def _background_refresh_models_dev() -> None:
|
||||
"""Best-effort refresh after serving stale cache data."""
|
||||
global _models_dev_cache, _models_dev_cache_time
|
||||
global _models_dev_retry_after, _models_dev_refresh_in_flight
|
||||
global _models_dev_refresh_in_flight
|
||||
try:
|
||||
data = _fetch_models_dev_from_network()
|
||||
_save_disk_cache(data)
|
||||
_models_dev_cache = data
|
||||
_models_dev_cache_time = time.time()
|
||||
_models_dev_retry_after = 0
|
||||
logger.debug(
|
||||
"Refreshed models.dev registry in background: %d providers",
|
||||
len(data),
|
||||
)
|
||||
with _models_dev_fetch_lock:
|
||||
_commit_registry(data, where="background")
|
||||
except Exception as e:
|
||||
_models_dev_retry_after = time.time() + _MODELS_DEV_RETRY_DELAY
|
||||
logger.debug(
|
||||
"Background models.dev refresh failed; retry suppressed for %ds: %s",
|
||||
_MODELS_DEV_RETRY_DELAY,
|
||||
e,
|
||||
)
|
||||
with _models_dev_fetch_lock:
|
||||
_note_refresh_failure(e, where="background")
|
||||
finally:
|
||||
with _models_dev_refresh_lock:
|
||||
_models_dev_refresh_in_flight = False
|
||||
|
|
@ -436,27 +461,10 @@ def fetch_models_dev(
|
|||
|
||||
try:
|
||||
data = _fetch_models_dev_from_network()
|
||||
_save_disk_cache(data)
|
||||
_models_dev_cache = data
|
||||
_models_dev_cache_time = time.time()
|
||||
_models_dev_retry_after = 0
|
||||
logger.debug(
|
||||
"Fetched models.dev registry: %d providers, %d total models",
|
||||
len(data),
|
||||
sum(
|
||||
len(p.get("models", {}))
|
||||
for p in data.values()
|
||||
if isinstance(p, dict)
|
||||
),
|
||||
)
|
||||
_commit_registry(data, where="foreground")
|
||||
return data
|
||||
except Exception as e:
|
||||
_models_dev_retry_after = time.time() + _MODELS_DEV_RETRY_DELAY
|
||||
logger.debug(
|
||||
"Failed to fetch models.dev; retry suppressed for %ds: %s",
|
||||
_MODELS_DEV_RETRY_DELAY,
|
||||
e,
|
||||
)
|
||||
_note_refresh_failure(e, where="foreground")
|
||||
|
||||
# Stage 5: network failed — return any stale memory/disk cache. Cache
|
||||
# freshness remains expired; the retry-after timestamp controls when
|
||||
|
|
|
|||
|
|
@ -13129,10 +13129,9 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew
|
|||
_msg_config_ctx = None
|
||||
if _msg_config_ctx is not None and isinstance(_msg_model_cfg, dict):
|
||||
try:
|
||||
from hermes_cli.route_identity import should_clear_context_pin
|
||||
from hermes_cli.route_identity import should_clear_context_pin_async
|
||||
|
||||
if await asyncio.to_thread(
|
||||
should_clear_context_pin,
|
||||
if await should_clear_context_pin_async(
|
||||
None, # model match already checked above
|
||||
None,
|
||||
_msg_model_cfg.get("base_url"),
|
||||
|
|
@ -13725,10 +13724,9 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew
|
|||
|
||||
if _hyg_config_context_length is not None:
|
||||
try:
|
||||
from hermes_cli.route_identity import should_clear_context_pin
|
||||
from hermes_cli.route_identity import should_clear_context_pin_async
|
||||
|
||||
if await asyncio.to_thread(
|
||||
should_clear_context_pin,
|
||||
if await should_clear_context_pin_async(
|
||||
_hyg_configured_model,
|
||||
_hyg_model,
|
||||
_hyg_configured_base_url,
|
||||
|
|
|
|||
|
|
@ -2008,10 +2008,9 @@ class GatewaySlashCommandsMixin:
|
|||
_persist_model_cfg = {}
|
||||
_persist_cfg["model"] = _persist_model_cfg
|
||||
try:
|
||||
from hermes_cli.route_identity import should_clear_context_pin
|
||||
from hermes_cli.route_identity import should_clear_context_pin_async
|
||||
|
||||
if await asyncio.to_thread(
|
||||
should_clear_context_pin,
|
||||
if await should_clear_context_pin_async(
|
||||
_persist_model_cfg.get("default")
|
||||
or _persist_model_cfg.get("model"),
|
||||
result.new_model,
|
||||
|
|
@ -2337,10 +2336,9 @@ class GatewaySlashCommandsMixin:
|
|||
model_cfg = {}
|
||||
cfg["model"] = model_cfg
|
||||
try:
|
||||
from hermes_cli.route_identity import should_clear_context_pin
|
||||
from hermes_cli.route_identity import should_clear_context_pin_async
|
||||
|
||||
if await asyncio.to_thread(
|
||||
should_clear_context_pin,
|
||||
if await should_clear_context_pin_async(
|
||||
model_cfg.get("default") or model_cfg.get("model"),
|
||||
result.new_model,
|
||||
model_cfg.get("base_url"),
|
||||
|
|
|
|||
|
|
@ -74,3 +74,31 @@ def should_clear_context_pin(
|
|||
)
|
||||
except Exception:
|
||||
return True
|
||||
|
||||
|
||||
async def should_clear_context_pin_async(
|
||||
configured_model: Any,
|
||||
active_model: Any,
|
||||
configured_base_url: Any,
|
||||
active_base_url: Any,
|
||||
configured_provider: Any,
|
||||
active_provider: Any,
|
||||
) -> bool:
|
||||
"""Async wrapper for ``should_clear_context_pin``.
|
||||
|
||||
Offloads the route comparison to a worker thread so async gateway
|
||||
handlers never run it on the event loop — the resolution chain is
|
||||
cache-only (``allow_network=False``) but can still do cold-start disk
|
||||
I/O. Shares all logic with the sync version — no code duplication.
|
||||
"""
|
||||
import asyncio
|
||||
|
||||
return await asyncio.to_thread(
|
||||
should_clear_context_pin,
|
||||
configured_model,
|
||||
active_model,
|
||||
configured_base_url,
|
||||
active_base_url,
|
||||
configured_provider,
|
||||
active_provider,
|
||||
)
|
||||
|
|
|
|||
|
|
@ -344,11 +344,12 @@ class TestFetchModelsDev:
|
|||
return_value=md._MODELS_DEV_CACHE_TTL + 60,
|
||||
), patch.object(md, "_load_disk_cache", return_value=SAMPLE_REGISTRY):
|
||||
first = fetch_models_dev()
|
||||
# Wait for the background refresh worker to finish so its
|
||||
# failure backoff is observable and requests.get stays patched.
|
||||
deadline = time.time() + 5
|
||||
while md._models_dev_refresh_in_flight and time.time() < deadline:
|
||||
time.sleep(0.01)
|
||||
# Join the background refresh worker so its failure backoff is
|
||||
# observable and requests.get stays patched for its lifetime.
|
||||
for worker in threading.enumerate():
|
||||
if worker.name == "models-dev-refresh":
|
||||
worker.join(timeout=5)
|
||||
assert not worker.is_alive()
|
||||
|
||||
assert first == SAMPLE_REGISTRY
|
||||
assert not md._models_dev_refresh_in_flight
|
||||
|
|
@ -364,6 +365,30 @@ class TestFetchModelsDev:
|
|||
assert not md._models_dev_refresh_in_flight
|
||||
mock_get.assert_called_once()
|
||||
|
||||
@patch("agent.models_dev.requests.get")
|
||||
def test_background_refresh_success_commits_registry(self, mock_get):
|
||||
"""The bg worker must save disk + swap mem cache + clear backoff."""
|
||||
import agent.models_dev as md
|
||||
|
||||
response = MagicMock()
|
||||
response.json.return_value = SAMPLE_REGISTRY
|
||||
mock_get.return_value = response
|
||||
|
||||
md._models_dev_cache = {"stale": {}}
|
||||
md._models_dev_cache_time = 0
|
||||
md._models_dev_retry_after = time.time() - 1
|
||||
|
||||
with patch.object(md, "_save_disk_cache") as mock_save:
|
||||
# Run the worker synchronously — deterministic, no thread.
|
||||
md._models_dev_refresh_in_flight = True
|
||||
md._background_refresh_models_dev()
|
||||
|
||||
mock_save.assert_called_once_with(SAMPLE_REGISTRY)
|
||||
assert md._models_dev_cache == SAMPLE_REGISTRY
|
||||
assert md._models_dev_cache_time > 0
|
||||
assert md._models_dev_retry_after == 0
|
||||
assert not md._models_dev_refresh_in_flight
|
||||
|
||||
@patch("agent.models_dev.requests.get")
|
||||
def test_missing_cache_failure_enters_backoff(self, mock_get):
|
||||
import agent.models_dev as md
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue