mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-15 14:22:43 +00:00
fix: return per-call copies from the skill-discovery cache
Review finding: callers mutate the returned dicts in place — hermes_cli/web_server.py annotates s['enabled']/s['usage'] on the skills list — so handing out the cached objects poisons the cache for every subsequent caller (and is a cross-thread shared-mutable hazard in the gateway). Return [dict(s) for s in cached] on both hit and miss paths; warm-path cost is negligible (241x speedup retained on a 300-skill fixture). Regression test mutates a returned list/dict and asserts the next cached call is clean.
This commit is contained in:
parent
9e9608ecc3
commit
cbdf87b21f
2 changed files with 17 additions and 5 deletions
|
|
@ -40,13 +40,21 @@ def _write_skill(root, category, name, description="a skill"):
|
|||
return d
|
||||
|
||||
|
||||
def test_cache_hit_serves_same_result_without_rescan(tmp_path):
|
||||
def test_cache_hit_serves_copies_not_cache_objects(tmp_path):
|
||||
"""Callers mutate the returned dicts (web_server annotates
|
||||
s['enabled']/s['usage']) — the cache must hand out per-call copies."""
|
||||
_write_skill(tmp_path, "cat-a", "skill-one")
|
||||
first = st._find_all_skills()
|
||||
assert [s["name"] for s in first] == ["skill-one"]
|
||||
|
||||
# Mutate what the first caller got; the next (cached) call must be clean.
|
||||
first[0]["enabled"] = False
|
||||
first.append({"name": "junk"})
|
||||
|
||||
second = st._find_all_skills()
|
||||
assert second is first # cache hit returns the SAME object, no rescan
|
||||
assert [s["name"] for s in second] == ["skill-one"]
|
||||
assert "enabled" not in second[0], "cache poisoned by caller mutation"
|
||||
assert second is not first
|
||||
|
||||
|
||||
def test_nested_category_skill_add_invalidates(tmp_path):
|
||||
|
|
|
|||
|
|
@ -707,7 +707,10 @@ def _find_all_skills(*, skip_disabled: bool = False) -> List[Dict[str, Any]]:
|
|||
and cached[0] == signature
|
||||
and (now - cached[1]) < _SKILLS_CACHE_TTL_SECONDS
|
||||
):
|
||||
return cached[2]
|
||||
# Per-call shallow copies: callers mutate the returned dicts
|
||||
# (e.g. web_server annotates s["enabled"]/s["usage"]) — handing
|
||||
# out the cached objects would poison the cache for everyone else.
|
||||
return [dict(s) for s in cached[2]]
|
||||
|
||||
skills = []
|
||||
seen_names: set = set()
|
||||
|
|
@ -768,9 +771,10 @@ def _find_all_skills(*, skip_disabled: bool = False) -> List[Dict[str, Any]]:
|
|||
|
||||
# Store in cache keyed by the scan signature computed BEFORE the scan
|
||||
# (a write racing the scan changes the signature, so the next call
|
||||
# re-scans rather than serving the torn result past the TTL).
|
||||
# re-scans rather than serving the torn result past the TTL). Same
|
||||
# shallow-copy contract as the hit path — the caller may mutate.
|
||||
_SKILLS_CACHE[cache_key] = (signature, now, skills)
|
||||
return skills
|
||||
return [dict(s) for s in skills]
|
||||
|
||||
|
||||
def _sort_skills(skills: List[Dict[str, Any]]) -> List[Dict[str, Any]]:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue