Two findings from the round-1 maintainer review were not addressed in round-2: the helper clamped an out-of-range value *up* to the ceiling, and the env var was exposed to per-profile ``.env`` overrides, which contradicts the "process-wide resource limit" intent and creates a first-load-wins asymmetry across profile switches. This round fixes both. ``_read_max_session_resolve_concurrent()`` previously treated out-of-range values like ``HERMES_WEBUI_MAX_SESSION_RESOLVE=999999`` as "the operator wants the maximum" and returned ``64`` — admitting 64 simultaneous unbounded transcript loads. The round-1 reviewer measured ~9.6 MB per parse on a synthetic 4.27 MB transcript, so the old ceiling permitted ~614 MB at 64-way concurrency before any other application memory. The cap is a *safety bound*, not a user preference. A typo must fall back to the safe default, never to the permissive extreme — matching the behavior of every other bad input in this helper (missing/empty/zero/negative/non-numeric all already fall back to 2). The 64 ceiling is reachable only via an explicit in-range operator value. server-wide key, not a per-profile .env override (SILENT) The cap controls a single process-wide ``BoundedSemaphore`` that gates full-transcript resolves. A profile's ``.env`` is per-profile by design; letting it set a server-wide limit lets a single profile drag the rest into a tighter or looser posture than the operator intended, with a first-load-wins asymmetry (a later profile switch cannot resize the already-constructed semaphore). The same shape as the isolated-profile key added in #4589: only the operator/launcher env at startup can set it. The cap is added to both ``_PROTECTED_ENV_KEYS`` (blocks the profile env at the load step) and ``_BLOCKED_RUNTIME_ENV_KEYS`` (filters the profile env in the runtime/gateway-parity path), so a profile cannot inject the env var at any layer. 1. ``api/models.py`` — ``_read_max_session_resolve_concurrent()``: - ``if n > 64: return 64`` → ``if n > 64: return 2`` - Docstring updated to spell out the "safety bound, not a user preference" rule and to point at ``_PROTECTED_ENV_KEYS`` as the place the 64 ceiling is enforced for profile env. 2. ``api/profiles.py`` — two frozensets: - ``_PROTECTED_ENV_KEYS`` adds ``'HERMES_WEBUI_MAX_SESSION_RESOLVE'`` (next to ``'HERMES_WEBUI_ISOLATED_PROFILE'``). - ``_BLOCKED_RUNTIME_ENV_KEYS`` adds the same key, with a comment explaining the first-load-wins asymmetry. 3. ``tests/test_issue7421_resolve_cap_config.py``: - The round-1 ``test_env_above_upper_bound_is_clamped_to_64`` is renamed to ``test_env_above_upper_bound_falls_back_to_default`` and the assertion + docstring reflect the new fallback contract. The maintainer's review said explicitly: "Update the clamp test accordingly." - Three new tests pin Finding 2: - ``test_protected_env_keys_includes_resolve_cap`` - ``test_blocked_runtime_env_keys_includes_resolve_cap`` - ``test_filter_runtime_env_strips_resolve_cap`` (end-to-end exercise: a profile env containing the cap is filtered out of the gateway-parity view, and an unrelated, non-blocked key like ``LANG`` still passes through as a sanity check) - ``tests/test_issue7421_resolve_cap_config.py``: 15/15 pass. - ``-k "profile or protect or isolated or HERMES_WEBUI_ISOLATED"``: 866 passed, 5 skipped, 1 xfailed, 2 xpassed — no regression. - The 5 ruff findings on the worktree are all pre-existing (3× F401 ``sqlite3`` + 2× B904 ``raise-with-from`` in ``api/profiles.py``); the curated gate is gated on newly-introduced findings only. Two. Both are the explicit fixes the maintainer requested. - ``_read_max_session_resolve_concurrent(999999)`` now returns ``2`` instead of ``64``. The 64 ceiling is still reachable via an explicit in-range operator value. - ``HERMES_WEBUI_MAX_SESSION_RESOLVE`` set in a profile's ``.env`` is now ignored: the operator/launcher env at startup is the only place the cap is configurable. Both changes are backward compatible at the operator level (an explicit ``=8`` still returns ``8``) and break the "typo → ceiling" failure mode and the "first-loaded-profile-wins" asymmetry the maintainer flagged. - An operator who set ``HERMES_WEBUI_MAX_SESSION_RESOLVE=64`` in their deployment env is unaffected (still returns 64, as before). - An operator who set ``HERMES_WEBUI_MAX_SESSION_RESOLVE=128`` in their deployment env now silently falls back to 2 instead of getting 64. This is the round-1 reviewer's intent: "Update the clamp test accordingly" + "the safe default, never the permissive extreme." A startup log warning could be added in a follow-up if a louder signal is wanted; this PR keeps the silent-fallback contract consistent with the other bad-input paths in the same helper. - A profile that *had* ``HERMES_WEBUI_MAX_SESSION_RESOLVE=64`` in its ``.env`` and was relying on it to set the cap now silently uses the operator-set value. This is the intended behavior: the cap is a process-wide resource limit, not a per-profile toggle. A profile-side log warning is a follow-up if needed; the runtime env path now drops the key, so the failure mode is "operator value wins" rather than "first profile to load wins". - #7656 — the original PR; this is round-3 of the same branch. - #7421 — the original env-override feature. - #4589 — the same shape ("operator-only, not per-profile") for the isolated-profile key; the comment block on the new protected key references the same pattern.
288 lines
12 KiB
Python
288 lines
12 KiB
Python
"""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``.
|
|
"""
|
|
from __future__ import annotations
|
|
|
|
import importlib
|
|
import os
|
|
import sys
|
|
from pathlib import Path
|
|
|
|
ROOT = Path(__file__).resolve().parents[1]
|
|
|
|
|
|
def _import_models_with_env(env_value):
|
|
"""Reload ``api.models`` with a chosen env value for
|
|
``HERMES_WEBUI_MAX_SESSION_RESOLVE``. The cap is a module-level
|
|
constant computed at import time, so the env var has to be
|
|
set before the module is first imported."""
|
|
if env_value is None:
|
|
os.environ.pop("HERMES_WEBUI_MAX_SESSION_RESOLVE", None)
|
|
else:
|
|
os.environ["HERMES_WEBUI_MAX_SESSION_RESOLVE"] = env_value
|
|
# Drop any cached import so the module-level constant is
|
|
# recomputed against the freshly-set env.
|
|
for mod_name in list(sys.modules):
|
|
if mod_name == "api.models" or mod_name.startswith("api.models."):
|
|
del sys.modules[mod_name]
|
|
return importlib.import_module("api.models")
|
|
|
|
|
|
# ── default behavior (env unset) ──────────────────────────────────────────────
|
|
|
|
|
|
def test_default_cap_is_two_when_env_unset():
|
|
"""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."""
|
|
models = _import_models_with_env(None)
|
|
assert models._FULL_SESSION_RESOLVE_MAX_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():
|
|
"""A user who sets the env var to an explicit empty string
|
|
(e.g. from a misconfigured .env file) is the same as unset
|
|
and must fall back to the default of 2 rather than 0."""
|
|
models = _import_models_with_env("")
|
|
assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2, (
|
|
"empty string must be treated as unset, not as the "
|
|
"literal value 0 (which would deadlock the semaphore)"
|
|
)
|
|
|
|
|
|
def test_default_cap_is_two_when_env_whitespace_only():
|
|
"""A user who copies a trailing space from a config file or
|
|
shell quoting must not accidentally deadlock the semaphore
|
|
by setting the cap to 0."""
|
|
models = _import_models_with_env(" ")
|
|
assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2
|
|
|
|
|
|
# ── env override success cases ───────────────────────────────────────────────
|
|
|
|
|
|
def test_env_override_takes_effect_within_upper_bound():
|
|
"""The headline use case: a multi-instance operator with 4
|
|
parallel active sessions sets the env to 4 and gets a
|
|
bounded semaphore with capacity 4 instead of 2. This
|
|
mirrors the maintainer's recommendation in the issue."""
|
|
models = _import_models_with_env("4")
|
|
assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 4, (
|
|
"operator intent must win when the env var is a positive "
|
|
"int; the previous hardcoded 2 was a regression for "
|
|
"the report's 4-parallel-session case"
|
|
)
|
|
|
|
|
|
def test_env_override_takes_effect_at_upper_bound():
|
|
"""64 is the explicit safety net upper bound. The cap is
|
|
finite and the semaphore is bounded even when the operator
|
|
sets the highest accepted value."""
|
|
models = _import_models_with_env("64")
|
|
assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 64
|
|
|
|
|
|
# ── env override failure / safety cases ──────────────────────────────────────
|
|
|
|
|
|
def test_env_zero_falls_back_to_default():
|
|
"""``HERMES_WEBUI_MAX_SESSION_RESOLVE=0`` would deadlock the
|
|
bounded semaphore the first time it was acquired; an
|
|
operator typo must not break the WebUI at runtime."""
|
|
models = _import_models_with_env("0")
|
|
assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2, (
|
|
"0 must fall back to the default; a bounded semaphore "
|
|
"with capacity 0 is a deadlock, not a concurrency cap"
|
|
)
|
|
|
|
|
|
def test_env_negative_falls_back_to_default():
|
|
"""``HERMES_WEBUI_MAX_SESSION_RESOLVE=-1`` is just as
|
|
dangerous as 0 (also a deadlock) and must be rejected
|
|
silently with the same fallback."""
|
|
models = _import_models_with_env("-1")
|
|
assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2
|
|
|
|
|
|
def test_env_above_upper_bound_falls_back_to_default():
|
|
"""``HERMES_WEBUI_MAX_SESSION_RESOLVE=999999`` is almost
|
|
certainly a typo and must NOT defeat the safety-bound
|
|
design by clamping up to the ceiling. The previous
|
|
clamp-to-64 behavior (round-1) admitted 64 simultaneous
|
|
unbounded transcript loads; a synthetic 4.27 MB transcript
|
|
consumed ~9.6 MB per parse, so the round-1 ceiling
|
|
permitted ~614 MB at 64-way concurrency before any other
|
|
application memory.
|
|
|
|
The cap is a *safety bound*, not a user preference — a
|
|
typo must fall back to the safe default (2), never to
|
|
the permissive extreme. The upper bound (64) is reachable
|
|
only via an explicit in-range operator value; out-of-range
|
|
values degrade silently to the default, matching the
|
|
behavior of every other bad input in this helper. See
|
|
#7656 round-3 finding 1.
|
|
"""
|
|
models = _import_models_with_env("999999")
|
|
assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2, (
|
|
"out-of-band values must fall back to the safe default, "
|
|
"not clamp up to the ceiling; a typo should never widen "
|
|
"the cap"
|
|
)
|
|
|
|
|
|
def test_env_non_numeric_falls_back_to_default():
|
|
"""``HERMES_WEBUI_MAX_SESSION_RESOLVE=many`` is a config
|
|
mistake and must not crash import. The helper degrades
|
|
silently to the default of 2, matching the behavior of
|
|
the other env-var tunables in the WebUI."""
|
|
models = _import_models_with_env("many")
|
|
assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 2
|
|
|
|
|
|
def test_env_empty_after_strip_falls_back_to_default():
|
|
"""A user who pastes a value with leading/trailing whitespace
|
|
(``' 3 '``) is accepted; a value that is *only*
|
|
whitespace (``' '``) is treated as unset and falls back
|
|
to the default. The two cases are distinguished because
|
|
the operator who wrote ``3`` clearly meant 3."""
|
|
models = _import_models_with_env(" 3 ")
|
|
assert models._FULL_SESSION_RESOLVE_MAX_CONCURRENT == 3
|
|
|
|
|
|
# ── bounded-semaphore capacity is consistent with the cap ────────────────────
|
|
|
|
|
|
def test_bounded_semaphore_capacity_matches_cap():
|
|
"""The semaphore is constructed with the same int that
|
|
``_FULL_SESSION_RESOLVE_MAX_CONCURRENT`` was assigned, so
|
|
raising the cap actually widens the concurrency budget.
|
|
Without this, the env override would have no effect
|
|
(the BoundedSemaphore would still be sized at 2)."""
|
|
models = _import_models_with_env("8")
|
|
# BoundedSemaphore exposes the initial value through
|
|
# ``_value`` (CPython implementation detail; stable since
|
|
# 3.x for the threading.BoundedSemaphore in the stdlib).
|
|
sem = models._FULL_SESSION_RESOLVE_SLOTS
|
|
capacity = getattr(sem, "_value", None)
|
|
assert capacity == 8, (
|
|
f"_FULL_SESSION_RESOLVE_SLOTS must be sized to the "
|
|
f"resolved cap (8), got {capacity!r}; otherwise the "
|
|
f"operator's env override has no effect"
|
|
)
|
|
|
|
|
|
# ── static wiring ───────────────────────────────────────────────────────────
|
|
|
|
|
|
def test_helper_defined_in_models():
|
|
"""The helper itself must live next to the cap it configures
|
|
so a future maintainer can find the precedence rules
|
|
without searching the codebase. The helper must also
|
|
accept the operator-facing env name verbatim — no alias
|
|
without a comment."""
|
|
src = (ROOT / "api" / "models.py").read_text(encoding="utf-8")
|
|
assert "def _read_max_session_resolve_concurrent" in src
|
|
assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" in src, (
|
|
"the env var name must appear in api/models.py so the "
|
|
"operator-facing contract is discoverable from this "
|
|
"module alone"
|
|
)
|
|
# The cap is now sourced from the helper rather than the
|
|
# bare literal.
|
|
assert "_FULL_SESSION_RESOLVE_MAX_CONCURRENT = _read_max_session_resolve_concurrent()" in src
|
|
# The bare literal-2 initializer line is gone; a stray
|
|
# duplicate would either keep the env-override silent or
|
|
# confuse the next reader about which value wins.
|
|
assert src.count("_FULL_SESSION_RESOLVE_MAX_CONCURRENT = 2\n") == 0, (
|
|
"the bare literal-2 initializer must be removed; the "
|
|
"cap is now sourced from the helper"
|
|
)
|
|
|
|
|
|
# ── #7656 round-3 finding 2: profile .env must not resize the cap ───────────
|
|
|
|
|
|
def test_protected_env_keys_includes_resolve_cap():
|
|
"""``HERMES_WEBUI_MAX_SESSION_RESOLVE`` is a process-wide
|
|
resource cap; a profile's ``.env`` must not override it.
|
|
|
|
The operator/launcher env at startup is the only place the
|
|
cap is configurable. Without this, a first-loaded profile
|
|
could set the cap to one value and a later profile-switch
|
|
would silently re-size the already-constructed
|
|
BoundedSemaphore — the same first-load-wins asymmetry
|
|
the maintainer flagged for the isolated-profile key in
|
|
#4589. Pin the contract.
|
|
"""
|
|
from api.profiles import _PROTECTED_ENV_KEYS
|
|
assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" in _PROTECTED_ENV_KEYS, (
|
|
"HERMES_WEBUI_MAX_SESSION_RESOLVE must be in "
|
|
"_PROTECTED_ENV_KEYS so a profile's .env cannot "
|
|
"override the process-wide cap"
|
|
)
|
|
|
|
|
|
def test_blocked_runtime_env_keys_includes_resolve_cap():
|
|
"""``filter_runtime_env_for_gateway_parity`` must also drop
|
|
the resolve cap from a profile's runtime env, so the
|
|
gateway/CLI/agent paths see the operator-set value rather
|
|
than whatever the profile tried to set.
|
|
|
|
This is the runtime counterpart of the protected-key
|
|
check. Together they pin the contract from both sides:
|
|
a profile cannot inject the env var at any layer
|
|
(startup reload, runtime env, gateway parity filter).
|
|
"""
|
|
from api.profiles import _BLOCKED_RUNTIME_ENV_KEYS
|
|
assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" in _BLOCKED_RUNTIME_ENV_KEYS, (
|
|
"HERMES_WEBUI_MAX_SESSION_RESOLVE must be in "
|
|
"_BLOCKED_RUNTIME_ENV_KEYS so a profile's runtime env "
|
|
"is filtered out before the gateway/CLI/agent paths "
|
|
"see it"
|
|
)
|
|
|
|
|
|
def test_filter_runtime_env_strips_resolve_cap():
|
|
"""A profile env containing the resolve cap must be filtered
|
|
out by ``filter_runtime_env_for_gateway_parity``.
|
|
|
|
End-to-end exercise of the SILENT finding: the
|
|
runtime-env filter is the one that actually prevents the
|
|
cap from being silently re-sized by a profile switch.
|
|
Without this, the cap the operator set at startup is
|
|
silently overridden the first time a profile with the
|
|
env var in its ``.env`` loads.
|
|
"""
|
|
from api.profiles import filter_runtime_env_for_gateway_parity
|
|
profile_env = {
|
|
"HERMES_WEBUI_MAX_SESSION_RESOLVE": "64",
|
|
"LANG": "en_US.UTF-8",
|
|
}
|
|
filtered = filter_runtime_env_for_gateway_parity(profile_env)
|
|
assert "HERMES_WEBUI_MAX_SESSION_RESOLVE" not in filtered, (
|
|
"filter_runtime_env_for_gateway_parity must strip "
|
|
"HERMES_WEBUI_MAX_SESSION_RESOLVE from a profile's "
|
|
"runtime env so the cap stays at the operator/launcher value"
|
|
)
|
|
# Sanity: an unrelated, non-blocked env key still passes through.
|
|
# (PATH, HOME, etc. are themselves in _BLOCKED_RUNTIME_ENV_KEYS,
|
|
# so pick something neutral like LANG.)
|
|
assert filtered.get("LANG") == "en_US.UTF-8"
|
|
|