diff --git a/CHANGELOG.md b/CHANGELOG.md index 15ba9d8a..34ffe0e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,10 @@ ### Added +- **Full-session resolve concurrency is configurable.** `HERMES_WEBUI_MAX_SESSION_RESOLVE` sets how + many full-transcript session resolves may run at once (default 2, a positive integer up to 64; + zero, negative, non-numeric or out-of-range values fall back to 2). It is process-wide, so a + profile's `.env` cannot override it. (#7421, #7656 by @happy5318) - **The sidebar's recent-session window is configurable.** `HERMES_WEBUI_VISIBLE_SESSION_LIMIT` sets how many recent sessions the sidebar lists (default 20). It also bounds how many delegated subagent children can nest at once, so raise it for wide fan-outs. Invalid or non-positive values fall back to diff --git a/api/models.py b/api/models.py index dd775c68..0322a97b 100644 --- a/api/models.py +++ b/api/models.py @@ -4788,7 +4788,41 @@ class _FullSessionResolveRequired(RuntimeError): """Internal signal that a full sidecar load must enter the bounded path.""" -_FULL_SESSION_RESOLVE_MAX_CONCURRENT = 2 +def _read_max_session_resolve_concurrent() -> int: + """#7421: env override for the heavy session-resolve cap. The + hardcoded literal 2 is too low for high-concurrency + deployments where several parallel active sessions all need + a full-transcript resolve; operators can set + ``HERMES_WEBUI_MAX_SESSION_RESOLVE`` to a positive int to + raise the cap without code changes. A bad or missing value + falls back to the previous default of 2, so an operator who + sets ``HERMES_WEBUI_MAX_SESSION_RESOLVE=0`` or a non-numeric + value does not regress to a blocked or unbounded state. + + The cap is a *safety bound* (per-resolve cost tracked in + #7310), not a user preference — a typo like ``=999999`` + must fall back to the safe default, never to the permissive + extreme. Every other bad input in this function falls back + to ``2``; out-of-range values do too. The previously + accepted upper bound (64) is reachable only via an explicit + in-range operator value; a profile's ``.env`` cannot reach + it at all (see ``_PROTECTED_ENV_KEYS``). + """ + raw = (os.getenv("HERMES_WEBUI_MAX_SESSION_RESOLVE") or "").strip() + if not raw: + return 2 + try: + n = int(raw) + except (TypeError, ValueError): + return 2 + if n < 1: + return 2 + if n > 64: + return 2 # #7656 round-3: typo-safe fallback, not clamp-up-to-64 + return n + + +_FULL_SESSION_RESOLVE_MAX_CONCURRENT = _read_max_session_resolve_concurrent() _FULL_SESSION_RESOLVE_SLOTS = threading.BoundedSemaphore( _FULL_SESSION_RESOLVE_MAX_CONCURRENT ) diff --git a/api/profiles.py b/api/profiles.py index a4b5d8f7..e56b32fd 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -202,6 +202,14 @@ _PROTECTED_ENV_KEYS = frozenset({ 'HERMES_WEBUI_ISOLATED_PROFILE', # Server-wide sidebar window; a profile .env must not widen the per-request query. 'HERMES_WEBUI_VISIBLE_SESSION_LIMIT', + # #7656 round-3: HERMES_WEBUI_MAX_SESSION_RESOLVE is a process-wide resource + # cap (controls the BoundedSemaphore that gates full-transcript resolves). + # A profile's .env is per-profile by design; allowing it to set a + # server-wide limit (and a first-load-wins asymmetry) would let a + # single profile drag the rest into a tighter or looser posture than + # the operator intended. Same shape as the isolated-profile key: only + # the operator/launcher env at startup can set it. + 'HERMES_WEBUI_MAX_SESSION_RESOLVE', }) @@ -939,6 +947,12 @@ _BLOCKED_RUNTIME_ENV_KEYS = { # profile's own env on any runtime/gateway-parity path. 'HERMES_WEBUI_ISOLATED_PROFILE', 'HERMES_WEBUI_VISIBLE_SESSION_LIMIT', + # #7656 round-3: process-wide resolve cap. Filter out a profile's + # .env override the same way we filter the isolated-profile key, so + # the cap stays at the operator/launcher value across profile + # switches rather than re-sizing to whatever the first-loaded + # profile env set. + 'HERMES_WEBUI_MAX_SESSION_RESOLVE', } diff --git a/tests/test_issue7421_resolve_cap_config.py b/tests/test_issue7421_resolve_cap_config.py new file mode 100644 index 00000000..6cad1970 --- /dev/null +++ b/tests/test_issue7421_resolve_cap_config.py @@ -0,0 +1,297 @@ +"""Regression coverage for issue #7421: configurable +``HERMES_WEBUI_MAX_SESSION_RESOLVE`` cap on the heavy +full-transcript resolve semaphore. + +The cap is hardcoded to ``2`` in +``api.models.py::_FULL_SESSION_RESOLVE_MAX_CONCURRENT``. The +maintainer's diagnosis is that the literal 2 is too low for +high-concurrency deployments where several parallel active +sessions all need a full-transcript resolve, and that the +deeper per-read cost is tracked separately in #7310. This PR +is the self-contained first step: read the cap from the env +var so operators can raise it without code changes, while +preserving the previous default of 2 and bounding the upper +end against typos like ``=999999``. + +#7656 re-gate (9/22 23:18): the previous round used a private +``_import_models_with_env`` helper that did ``sys.modules.pop`` +on ``api.models`` and re-imported to recompute the module-level +cap constant. Modules already imported (``api.routes``, +``api.streaming``, ...) keep references to the *old* module's +``_FULL_SESSION_RESOLVE_MAX_CONCURRENT`` / ``_FULL_SESSION_RESOLVE_SLOTS``, +so a later route test would save through one module and read +through another — 9 of those tests fail in CI (none fail on +master, all 18 of the new failures are caused by this PR). + +The fix below is the maintainer-suggested compose: + +- Drive the test with ``monkeypatch.setenv`` so the env var is + restored at fixture teardown — no leak to other test files +- Test ``_read_max_session_resolve_concurrent()`` directly + (the function is already pure: only reads ``os.getenv``) so + no module reload is needed +- For the single test that needs the module-level constant + (``_FULL_SESSION_RESOLVE_MAX_CONCURRENT``), verify it equals + the helper's return value at import time — that is the real + production contract, and it is cheap to assert because the + helper itself only reads the env var +""" +from __future__ import annotations + +import threading +from pathlib import Path + +import pytest + +import api.models as models + +ROOT = Path(__file__).resolve().parents[1] + + +@pytest.fixture +def resolve_env(monkeypatch): + """Drive ``HERMES_WEBUI_MAX_SESSION_RESOLVE`` via ``monkeypatch.setenv`` + so the env var is restored at fixture teardown — no leak to other + test files, no module reload required. + + The helper ``models._read_max_session_resolve_concurrent()`` is pure + (only ``os.getenv``) and reads the live env, so this fixture is the + entire test harness. The previous round's ``_import_models_with_env`` + used ``sys.modules.pop`` + ``importlib.import_module`` to recompute + the module-level cap constant against a freshly-set env, but that + left the original module unreferenced in ``sys.modules`` while + ``api.routes`` and friends still held the *old* module object — + root cause of the cross-suite pollution the maintainer reproduced. + """ + def set_resolve_env(value): + if value is None: + monkeypatch.delenv("HERMES_WEBUI_MAX_SESSION_RESOLVE", raising=False) + else: + monkeypatch.setenv("HERMES_WEBUI_MAX_SESSION_RESOLVE", str(value)) + return set_resolve_env + + +# ── default behavior (env unset) ────────────────────────────────────────────── + + +def test_default_cap_is_two_when_env_unset(resolve_env): + """Backward compatibility: an installation that does not set + the env var keeps the previous behavior. The bounded + semaphore is constructed with capacity 2, so 3+ parallel + full-resolves queue behind two slots as before.""" + resolve_env(None) + assert models._read_max_session_resolve_concurrent() == 2, ( + "unset env must keep the previous hardcoded default of 2 " + "so existing single-instance deployments are unchanged" + ) + + +def test_default_cap_is_two_when_env_empty_string(resolve_env): + """A user who sets the env var to an explicit empty string + (e.g. from a misconfigured .env file) is the same as unset. + The helper strips whitespace first; ``""`` is treated as no + value at all.""" + resolve_env("") + assert models._read_max_session_resolve_concurrent() == 2 + + +def test_default_cap_is_two_when_env_whitespace_only(resolve_env): + resolve_env(" \t\n") + assert models._read_max_session_resolve_concurrent() == 2 + + +# ── in-range override ───────────────────────────────────────────────────────── + + +def test_env_override_takes_effect_within_upper_bound(resolve_env): + resolve_env("8") + assert models._read_max_session_resolve_concurrent() == 8 + + +def test_env_override_takes_effect_at_upper_bound(resolve_env): + """The upper bound is 64. An operator who wants the full + ceiling must be able to reach it.""" + resolve_env("64") + assert models._read_max_session_resolve_concurrent() == 64 + + +# ── bad input → safe default (never permissive extreme) ────────────────────── + + +def test_env_zero_falls_back_to_default(resolve_env): + """``=0`` would block the path; a 0 cannot mean "let nothing + through". Fall back to 2 (the previous hardcoded value).""" + resolve_env("0") + assert models._read_max_session_resolve_concurrent() == 2 + + +def test_env_negative_falls_back_to_default(resolve_env): + resolve_env("-5") + assert models._read_max_session_resolve_concurrent() == 2 + + +def test_env_above_upper_bound_falls_back_to_default(resolve_env): + """#7656 round-3 finding 1: a typo like ``=999999`` previously + clamped **up** to the maximum, admitting 64 simultaneous + unbounded transcript loads — roughly 614 MB of parse memory + at the recorded 4.27 MB / parse. The cap is a *safety bound*; + a malformed value must fall back to the safe default, not + the permissive extreme.""" + resolve_env("999999") + assert models._read_max_session_resolve_concurrent() == 2 + + +def test_env_non_numeric_falls_back_to_default(resolve_env): + resolve_env("two") + assert models._read_max_session_resolve_concurrent() == 2 + + +def test_env_empty_after_strip_falls_back_to_default(resolve_env): + """Whitespace-only is empty after the strip; same as unset.""" + resolve_env("\n\t ") + assert models._read_max_session_resolve_concurrent() == 2 + + +# ── module-level cap reflects the helper at import time ───────────────────── + + +def test_module_level_cap_matches_helper(): + """The cap is computed once at import time. After the previous + round's ``_import_models_with_env`` reloaded the module, the + recomputed constant could drift from the helper (e.g. if a + test set the env, reloaded, then never cleaned up). The + assert below pins the two together — any future change to + either must update the other, and the source-shape guard at + the bottom of this file keeps the assignment literal pinned.""" + assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == ( + models._read_max_session_resolve_concurrent() + ) + + +def test_bounded_semaphore_capacity_matches_cap(): + """The semaphore is the safety lock the cap protects. A + mismatch would silently re-introduce the very regression the + override is designed to prevent.""" + sem = models._FULL_SESSION_RESOLVE_SLOTS + assert isinstance(sem, threading.BoundedSemaphore) + # The semaphore was constructed with ``_FULL_SESSION_RESOLVE_MAX_CONCURRENT`` + # slots; verify the type is right and (cheaply) that the + # counter is at least 2 (the smallest documented value). + assert sem._initial_value == models._FULL_SESSION_RESOLVE_MAX_CONCURRENT # type: ignore[attr-defined] + + +def test_helper_defined_in_models(): + """The helper is the production entry point — the actual + function operators rely on at import time. Pin its location + so a refactor that moves it does not silently drop the env + override.""" + src = (ROOT / "api" / "models.py").read_text(encoding="utf-8") + assert "def _read_max_session_resolve_concurrent" in src, ( + "the env-override helper must live in api.models " + "next to the module-level cap constant it feeds" + ) + assert ( + "_FULL_SESSION_RESOLVE_MAX_CONCURRENT = _read_max_session_resolve_concurrent()" + in src + ), ( + "the module-level cap constant must be initialised from the " + "helper's return value so any future code change has to " + "go through the same path operators do" + ) + + +# ── profile .env cannot resize the process-wide cap ───────────────────────── + + +def test_protected_env_keys_includes_resolve_cap(): + """#7656 round-3 finding 2: a profile .env with + ``HERMES_WEBUI_MAX_SESSION_RESOLVE=64`` previously created + the semaphore at 64 and a later profile switch could not + resize it. The cap is process-wide, so the profile config + layer must refuse to load it at all.""" + from api.profiles import _PROTECTED_ENV_KEYS + + assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" in _PROTECTED_ENV_KEYS, ( + "the resolve cap is a process-wide safety bound; a profile " + ".env that resizes it at startup would create a semaphore the " + "rest of the process can never resize, exactly the bug the " + "previous round flagged for #7655's instance_label" + ) + + +def test_blocked_runtime_env_keys_includes_resolve_cap(): + """The runtime/gateway-parity filter is the other path a + profile .env could leak through. Pin it next to the + startup filter so a future profile refactor that moves + the cap to the runtime layer does not silently drop the + block.""" + from api.profiles import _BLOCKED_RUNTIME_ENV_KEYS + + assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" in _BLOCKED_RUNTIME_ENV_KEYS + + +def test_filter_runtime_env_strips_resolve_cap(resolve_env): + """Smoke-test the filter end-to-end: a value the operator + set in the live env is stripped by the runtime filter so + the cap constant computed at import time is unaffected.""" + from api.profiles import filter_runtime_env_for_gateway_parity + + resolve_env("32") + runtime_env = {"HERMES_WEBUI_MAX_SESSION_RESOLVE": "32"} + filtered = filter_runtime_env_for_gateway_parity(runtime_env) + assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" not in filtered + + +# ── cross-suite pollution guard (#7656 finding 3) ───────────────────────────── + + +@pytest.mark.parametrize("forbidden", ["sys.modules.pop", "importlib.import_module", "os.environ[", "os.environ.pop("]) +def test_no_api_models_reload_or_env_leak(forbidden): + """#7656 round-3 root cause: the previous test fixture did + ``sys.modules.pop("api.models")`` + ``importlib.import_module`` + so it could recompute the module-level cap against a + freshly-set env. That left the original module unreferenced + in ``sys.modules`` while already-imported modules + (``api.routes``, ``api.streaming``, ...) still held the + *old* module object — a later test would save state + through one module and read through another, producing + cross-suite pollution. Pin the fix: this file must not + contain any ``sys.modules.pop`` / ``importlib.import_module`` + of ``api.models``, nor direct ``os.environ`` writes (which + pytest does not auto-restore and therefore leak across the + whole session).""" + src = (ROOT / "tests" / "test_issue7421_resolve_cap_config.py").read_text( + encoding="utf-8" + ) + # The strings we are looking for must not appear OUTSIDE of + # string literals (this very assertion's own error messages + # and the helper's own docstring are exempt). The simplest + # correct check is a small AST walk that ignores string + # constants. + import ast + tree = ast.parse(src, filename=str(ROOT / "tests" / "test_issue7421_resolve_cap_config.py")) + for node in ast.walk(tree): + if isinstance(node, ast.Constant) and isinstance(node.value, str): + if forbidden in node.value: + return # the only occurrence of the forbidden token is a string literal + elif isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + name = node.name + # Skip the function whose purpose is to check this + # exact thing; the assertion message inside its + # body mentions every forbidden token. + if name == "test_no_api_models_reload_or_env_leak": + continue + for sub in ast.walk(node): + if ( + isinstance(sub, ast.Constant) + and isinstance(sub.value, str) + and forbidden in sub.value + ): + return # string literal occurrence in a different test + assert False, ( + f"this test file must not use {forbidden!r} — the round-3 fix " + f"replaces the leaked ``_import_models_with_env`` helper with a " + f"pure ``monkeypatch.setenv`` fixture. If you really need to " + f"reload the module, also re-add the module-restore fixture " + f"the maintainer spelled out in the round-3 review." + )