From 544c31b50b8d51eaaff4e95439ca88bc5e5f548f Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Tue, 19 May 2026 14:25:10 -0700 Subject: [PATCH] perf(agent-loop): cut 47% of per-conversation function calls via 3 targeted hot-path optimizations (#28866) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(config): add load_config_readonly() fast path for hot agent loop `load_config()` is called from the agent loop's per-API-call hot path via `get_provider_request_timeout()` and `get_provider_stale_timeout()` — both invoked once per turn from `_resolved_api_call_timeout()` in run_agent.py. Profiling a synthetic 20-tool-call agent run revealed: - 21 invocations of `load_config()` cumulating 56ms (~17% of agent loop) - 34,398 deepcopy calls totaling 37ms (config defensive deepcopy + chain) - 8,652 `_expand_env_vars` invocations (~412 per turn) Microbench (cache-hit, real config.yaml present): load_config() 265us/call (125us deepcopy + 140us infra) load_config_readonly() 138us/call (~48% faster) `load_config_readonly()` returns the cached dict directly without the defensive deepcopy. Documented contract: caller must not mutate. Returns plain dict (not MappingProxyType) so downstream `isinstance(x, dict)` guards keep working — caught during initial implementation when MappingProxyType broke get_provider_request_timeout's guard logic. Wired into hermes_cli/timeouts.py (the two functions called per agent turn). load_config() is unchanged for the 263 other call sites that mutate the result before save_config(), are not in the hot path, or where the safety guarantee matters more than the perf. Profile A/B (cached config, 21-turn agent loop): BEFORE AFTER delta get_provider_request_timeout 55ms 16ms -71% total function calls 399k 160k -60% deepcopy calls (in hotspots) 34,398 ~0 ~elim Verified: - isinstance(load_config_readonly(), dict) is True - timeout/stale resolutions correct - load_config() still returns isolated mutable deepcopies - tests/hermes_cli/test_config*.py / test_timeouts.py: 102/102 pass - tests/cli/ + tests/agent/test_auxiliary_client.py: 883/883 pass * perf(redact): substring pre-screens skip non-matching regex chains Every log record passes through `RedactingFormatter.format` which calls `redact_sensitive_text`, which historically ran ALL 13 secret-pattern regexes against every line — including DB connection strings, JWTs, Discord mentions, Signal phone numbers, etc. — even for typical clean log records like 'INFO run_agent: API call completed'. Add cheap substring pre-checks before each regex pass. False positives still run the regex (which then matches nothing); false negatives are impossible because every pattern requires the gated substring to match its leading anchor: - `_PREFIX_RE` gated on any of 33 known credential prefix substrings - `_ENV_ASSIGN_RE` gated on `=` in text - `_JSON_FIELD_RE` gated on `:` and `"` in text - `_AUTH_HEADER_RE` gated on `uthorization`/`UTHORIZATION` in text - `_TELEGRAM_RE` gated on `:` in text - `_PRIVATE_KEY_RE` gated on `BEGIN` and `-----` - `_DB_CONNSTR_RE` gated on `://` in text - `_JWT_RE` gated on `eyJ` in text - URL userinfo/query gated on `://` - `_redact_form_body` gated on `&` and `=` - `_DISCORD_MENTION_RE` gated on `<@` - `_SIGNAL_PHONE_RE` gated on `+` Microbench (5 typical log records, 20k iterations each): BEFORE AFTER delta redact_sensitive_text per call 5.63us 1.79us -68% Real-world impact: ~244 log records emitted in a 30-turn agent loop, so the chain saves ~1ms of CPU per conversation. Bigger win is the reduction in regex execution and GC pressure during heavy logging sessions (verbose logging, gateway message processing). Security regression test: 30 secret-containing inputs (sk-/ghp_/JWT/DB connstr/Auth-Bearer/private key/URL userinfo/Discord/Signal/etc.) verified to produce identical redacted output before/after. All 75 existing tests/agent/test_redact.py cases pass. The `?access_token=foo&code=bar` (bare query string, no scheme) case that 'leaks' is pre-existing behavior — the URL query redaction requires a well-formed URL with scheme+host. Not a regression. * perf(run_agent): cache _needs_thinking_reasoning_pad result per (provider, model, base_url) Profile of a 31-turn synthetic agent run shows `_needs_thinking_reasoning_pad` fires 495 times (~16 per turn) and each call ran 3 helper methods, each hitting `base_url_host_matches` 1-4 times via `urlparse`. Total cost: 3,342 base_url_host_matches calls + 3,373 urlparse calls accounting for ~36ms of agent-loop overhead (~7% of the entire post-network work). Provider / model / base_url don't change during a conversation except via `switch_model` and fallback activation — both of which already overwrite those attributes atomically. Cache the result on a tuple key; since the key is derived from the very fields that would change, the cache auto-invalidates on the next read after a switch. No manual invalidation needed in switch_model / _try_activate_fallback. Profile A/B (31-turn cached-config agent run): BEFORE AFTER delta _needs_thinking_reasoning_pad cum 18ms 1ms -94% _copy_reasoning_content_for_api cum 17ms 1ms -94% base_url_host_matches calls 3,342 372 -89% urlparse calls 3,373 403 -88% total function calls 296k 223k -25% Verified: - tests/run_agent/test_deepseek_reasoning_content_echo.py: 36/36 pass - tests/run_agent/ (full): 1383/1383 pass + 3 skipped --- agent/redact.py | 133 ++++++++++++++++++++++++++++++----------- hermes_cli/config.py | 50 +++++++++++++++- hermes_cli/timeouts.py | 8 +-- run_agent.py | 16 ++++- 4 files changed, 165 insertions(+), 42 deletions(-) diff --git a/agent/redact.py b/agent/redact.py index 4cafbaef7a2..1beb10450fd 100644 --- a/agent/redact.py +++ b/agent/redact.py @@ -321,6 +321,15 @@ def redact_sensitive_text(text: str, *, force: bool = False, code_file: bool = F patterns when the text is known to be source code (e.g. MAX_TOKENS=*** constants, "apiKey": "test" fixtures). Prefix patterns, auth headers, private keys, DB connstrings, JWTs, and URL secrets are still redacted. + + Performance: each regex pattern is gated behind a cheap substring + pre-check (e.g. ``"=" in text`` for ENV assignments, ``"://" in text`` + for URLs, ``"eyJ" in text`` for JWTs). On a typical hermes log line + (no secrets) this drops the 13-pattern scan from ~5.6us to ~1.8us per + record (-68%). The pre-checks are conservative — false positives + still run the full regex, which then doesn't match. False negatives + are impossible because every regex requires the gated substring to + match. """ if text is None: return None @@ -331,68 +340,122 @@ def redact_sensitive_text(text: str, *, force: bool = False, code_file: bool = F if not (force or _REDACT_ENABLED): return text - # Known prefixes (sk-, ghp_, etc.) - text = _PREFIX_RE.sub(lambda m: _mask_token(m.group(1)), text) + # Known prefixes (sk-, ghp_, etc.) — gate on substring presence + if _has_known_prefix_substring(text): + text = _PREFIX_RE.sub(lambda m: _mask_token(m.group(1)), text) # ENV assignments: OPENAI_API_KEY=*** (skip for code files — false positives) if not code_file: - def _redact_env(m): - name, quote, value = m.group(1), m.group(2), m.group(3) - return f"{name}={quote}{_mask_token(value)}{quote}" - text = _ENV_ASSIGN_RE.sub(_redact_env, text) + if "=" in text: + def _redact_env(m): + name, quote, value = m.group(1), m.group(2), m.group(3) + return f"{name}={quote}{_mask_token(value)}{quote}" + text = _ENV_ASSIGN_RE.sub(_redact_env, text) # JSON fields: "apiKey": "***" (skip for code files — false positives) - def _redact_json(m): - key, value = m.group(1), m.group(2) - return f'{key}: "{_mask_token(value)}"' - text = _JSON_FIELD_RE.sub(_redact_json, text) + if ":" in text and '"' in text: + def _redact_json(m): + key, value = m.group(1), m.group(2) + return f'{key}: "{_mask_token(value)}"' + text = _JSON_FIELD_RE.sub(_redact_json, text) - # Authorization headers - text = _AUTH_HEADER_RE.sub( - lambda m: m.group(1) + _mask_token(m.group(2)), - text, - ) + # Authorization headers — _AUTH_HEADER_RE is "Authorization: Bearer ..." + # case-insensitive, so "uthorization" is the cheapest substring gate that + # covers both "Authorization" and "authorization" without a casefold(). + if "uthorization" in text or "UTHORIZATION" in text: + text = _AUTH_HEADER_RE.sub( + lambda m: m.group(1) + _mask_token(m.group(2)), + text, + ) - # Telegram bot tokens - def _redact_telegram(m): - prefix = m.group(1) or "" - digits = m.group(2) - return f"{prefix}{digits}:***" - text = _TELEGRAM_RE.sub(_redact_telegram, text) + # Telegram bot tokens — pattern requires ":" with digits prefix + if ":" in text: + def _redact_telegram(m): + prefix = m.group(1) or "" + digits = m.group(2) + return f"{prefix}{digits}:***" + text = _TELEGRAM_RE.sub(_redact_telegram, text) # Private key blocks - text = _PRIVATE_KEY_RE.sub("[REDACTED PRIVATE KEY]", text) + if "BEGIN" in text and "-----" in text: + text = _PRIVATE_KEY_RE.sub("[REDACTED PRIVATE KEY]", text) # Database connection string passwords - text = _DB_CONNSTR_RE.sub(lambda m: f"{m.group(1)}***{m.group(3)}", text) + if "://" in text: + text = _DB_CONNSTR_RE.sub(lambda m: f"{m.group(1)}***{m.group(3)}", text) # JWT tokens (eyJ... — base64-encoded JSON headers) - text = _JWT_RE.sub(lambda m: _mask_token(m.group(0)), text) + if "eyJ" in text: + text = _JWT_RE.sub(lambda m: _mask_token(m.group(0)), text) # URL userinfo (http(s)://user:pass@host) — redact for non-DB schemes. # DB schemes are handled above by _DB_CONNSTR_RE. - text = _redact_url_userinfo(text) + if "://" in text: + text = _redact_url_userinfo(text) - # URL query params containing opaque tokens (?access_token=…&code=…) - text = _redact_url_query_params(text) + # URL query params containing opaque tokens (?access_token=…&code=…) + if "?" in text: + text = _redact_url_query_params(text) # Form-urlencoded bodies (only triggers on clean k=v&k=v inputs). - text = _redact_form_body(text) + if "&" in text and "=" in text: + text = _redact_form_body(text) # Discord user/role mentions (<@snowflake_id>) - text = _DISCORD_MENTION_RE.sub(lambda m: f"<@{'!' if '!' in m.group(0) else ''}***>", text) + if "<@" in text: + text = _DISCORD_MENTION_RE.sub(lambda m: f"<@{'!' if '!' in m.group(0) else ''}***>", text) # E.164 phone numbers (Signal, WhatsApp) - def _redact_phone(m): - phone = m.group(1) - if len(phone) <= 8: - return phone[:2] + "****" + phone[-2:] - return phone[:4] + "****" + phone[-4:] - text = _SIGNAL_PHONE_RE.sub(_redact_phone, text) + if "+" in text: + def _redact_phone(m): + phone = m.group(1) + if len(phone) <= 8: + return phone[:2] + "****" + phone[-2:] + return phone[:4] + "****" + phone[-4:] + text = _SIGNAL_PHONE_RE.sub(_redact_phone, text) return text +# Substrings used to gate ``_PREFIX_RE`` execution. If none of these appear in +# the input string, the prefix regex cannot match anything, so we skip it. +# False positives are fine (they just run the regex, which then matches +# nothing) — the bound is "no false negatives" and that holds because every +# pattern in ``_PREFIX_PATTERNS`` has at least one of these as a literal +# substring of its leading characters. +# +# Derived automatically from ``_PREFIX_PATTERNS`` at module load time so a +# future PR that adds a new prefix to the regex list can't silently break +# the screen. + +def _extract_literal_prefix(pattern: str) -> str: + """Return the leading literal characters of a regex pattern. + + Stops at the first regex metacharacter (``[``, ``(``, ``\\``, ``.``, + ``?``, ``*``, ``+``, ``|``, ``{``, ``^``, ``$``). Returns the literal + that any match of the pattern MUST contain as a substring, so the + pre-screen never produces false negatives. + """ + meta = "[(\\.?*+|{^$" + for i, ch in enumerate(pattern): + if ch in meta: + return pattern[:i] + return pattern + + +_PREFIX_SUBSTRINGS = tuple( + _extract_literal_prefix(p) for p in _PREFIX_PATTERNS +) + + +def _has_known_prefix_substring(text: str) -> bool: + """Return True if ``text`` contains any known credential prefix substring. + + Used as a cheap pre-check before invoking the expensive ``_PREFIX_RE``. + """ + return any(p in text for p in _PREFIX_SUBSTRINGS) + + class RedactingFormatter(logging.Formatter): """Log formatter that redacts secrets from all log messages.""" diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 9c4197c80fb..dd470bdbbf3 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -4331,7 +4331,38 @@ def load_config() -> Dict[str, Any]: The cache is keyed on ``str(config_path)`` so profile switches (which change ``HERMES_HOME`` and therefore ``get_config_path()``) don't collide. + + Read-only callers should use ``load_config_readonly()`` to skip the + defensive deepcopy — that path matters in agent-loop hot spots like + ``get_provider_request_timeout`` which is called once per API turn. """ + return _load_config_impl(want_deepcopy=True) + + +def load_config_readonly() -> Dict[str, Any]: + """Fast-path variant of ``load_config()`` for callers that ONLY READ. + + Returns the cached config dict directly without the defensive deepcopy + that ``load_config()`` applies. **Mutating the returned dict (or any + nested structure) corrupts the in-process cache for every subsequent + caller** — only use this when you are absolutely sure your code path + will not write to the result. If you need to mutate or pass to + ``save_config``, call ``load_config()`` instead. + + Why this exists: ``load_config()`` cache-hit cost is ~265us per call, + half of which (~135us) is the defensive deepcopy. The agent loop calls + into config reads (timeouts, thresholds, feature flags) ~20-50x per + conversation; skipping deepcopy here removes a measurable allocation + source and the GC pressure that comes with it. + + Note: this returns a plain ``dict`` (not ``MappingProxyType``) so + existing ``isinstance(x, dict)`` guards downstream keep working. The + safety guarantee is purely documented, not enforced — be careful. + """ + return _load_config_impl(want_deepcopy=False) + + +def _load_config_impl(*, want_deepcopy: bool) -> Dict[str, Any]: with _CONFIG_LOCK: ensure_hermes_home() config_path = get_config_path() @@ -4345,7 +4376,7 @@ def load_config() -> Dict[str, Any]: cached = _LOAD_CONFIG_CACHE.get(path_key) if cached is not None and cache_key is not None and cached[:2] == cache_key: - return copy.deepcopy(cached[2]) + return copy.deepcopy(cached[2]) if want_deepcopy else cached[2] config = copy.deepcopy(DEFAULT_CONFIG) @@ -4369,9 +4400,24 @@ def load_config() -> Dict[str, Any]: expanded = _expand_env_vars(normalized) _LAST_EXPANDED_CONFIG_BY_PATH[path_key] = copy.deepcopy(expanded) if cache_key is not None: - _LOAD_CONFIG_CACHE[path_key] = (cache_key[0], cache_key[1], copy.deepcopy(expanded)) + # Cache stores a separate deepcopy so subsequent ``load_config()`` + # (deepcopy=True) callers can mutate freely without affecting the + # cached value, and ``load_config_readonly()`` (deepcopy=False) + # callers all see the same stable cached object. + cached_copy = copy.deepcopy(expanded) + _LOAD_CONFIG_CACHE[path_key] = (cache_key[0], cache_key[1], cached_copy) + # On the readonly path return the same cached object subsequent + # calls will see — keeps "two readonly calls return the same + # object" invariant that callers may rely on for identity checks. + if not want_deepcopy: + return cached_copy else: _LOAD_CONFIG_CACHE.pop(path_key, None) + # First-load result is a fresh dict (not aliased to the cache); safe + # to return directly. For the deepcopy=True path this is the + # canonical "freshly-built mutable result" the function has always + # returned. For the deepcopy=False path with no cache (e.g. config + # file missing), it's also fine — callers get an isolated object. return expanded diff --git a/hermes_cli/timeouts.py b/hermes_cli/timeouts.py index 7bd40aaa1de..d4633fe2067 100644 --- a/hermes_cli/timeouts.py +++ b/hermes_cli/timeouts.py @@ -19,8 +19,8 @@ def get_provider_request_timeout( return None try: - from hermes_cli.config import load_config - config = load_config() + from hermes_cli.config import load_config_readonly + config = load_config_readonly() except Exception: return None @@ -48,8 +48,8 @@ def get_provider_stale_timeout( return None try: - from hermes_cli.config import load_config - config = load_config() + from hermes_cli.config import load_config_readonly + config = load_config_readonly() except Exception: return None diff --git a/run_agent.py b/run_agent.py index 6e39ccfbb56..f842ce6936c 100644 --- a/run_agent.py +++ b/run_agent.py @@ -3607,12 +3607,26 @@ class AIAgent: DeepSeek v4 thinking and Kimi / Moonshot thinking both reject replays of assistant tool-call messages that omit ``reasoning_content`` (refs #15250, #17400). Xiaomi MiMo thinking mode has the same requirement. + + Result cached on the AIAgent instance keyed by (provider, model, + base_url); invalidated whenever ``switch_model()`` / + ``_try_activate_fallback()`` mutate any of those. This is hot — the + agent loop hits ~16 invocations per turn, each of which would + otherwise re-run ~5 ``base_url_host_matches`` (and therefore + ``urlparse``) calls under it. Caching drops the per-turn cost from + ~5us × 16 = ~80us to <1us. """ - return ( + key = (self.provider, self.model, getattr(self, "_base_url_lower", self.base_url)) + cached = getattr(self, "_thinking_pad_cache", None) + if cached is not None and cached[0] == key: + return cached[1] + result = ( self._needs_deepseek_tool_reasoning() or self._needs_kimi_tool_reasoning() or self._needs_mimo_tool_reasoning() ) + self._thinking_pad_cache = (key, result) + return result def _needs_kimi_tool_reasoning(self) -> bool: """Return True when the current provider is Kimi / Moonshot thinking mode.