From b5d382a3bba0823a44563d8a553a33fc646a7bc4 Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Thu, 11 Jun 2026 01:41:24 +0000 Subject: [PATCH] fix(profiles): scope the detached models-rebuild worker to the active profile (#3957) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex + Opus both independently caught a CORE gap in the first cut: the bounded /api/models rebuild runs on a detached 'models-catalog-rebuild' daemon thread that inherits neither the request-profile thread-local (#798) nor os.environ. So on a non-default profile the worker probed the DEFAULT profile's credentials and, when the 4s budget was exceeded, published the rebuilt catalog to the DEFAULT profile's disk cache (cross-contamination) — exactly the slow path a non-default cold rebuild takes. Fix: - profile_scope_for_detached_worker(profile_name): sets the request-profile TLS AND applies the profile .env on the worker thread, restoring both on exit (no-op for default). Distinct from profile_env_for_active_request (which reads the current thread's TLS and must not clear it). - get_available_models() captures the active profile on the request thread and wraps the rebuild worker body (probe + over-budget publish + disk save + fingerprint) in that scope; the legacy synchronous rebuild applies the profile env on the foreground. /api/models route no longer wraps (the work moved into get_available_models so ALL callers — chat/start, resolution — are fixed). - 2 new regression tests incl. the worker-thread before/inside/after assertion. Empirically verified: a fresh worker thread resolved models_cache.json/default WITHOUT the scope (the bug) and models_cache.work.json + the work .env + the work auth.json WITH it. Co-authored-by: nesquena-hermes --- api/config.py | 74 +++++++++++++---- api/profiles.py | 43 ++++++++++ api/routes.py | 12 +-- ...test_issue3957_profile_providers_models.py | 81 +++++++++++++++++-- 4 files changed, 183 insertions(+), 27 deletions(-) diff --git a/api/config.py b/api/config.py index 31b1a9bd..5b65a247 100644 --- a/api/config.py +++ b/api/config.py @@ -5250,10 +5250,40 @@ def get_available_models(*, prefer_cache: bool = False) -> dict: with _cache_build_cv: _cache_build_in_progress = True + # Capture the active per-request profile (#3957). The live provider + # probe inside the rebuild resolves credentials from os.environ / + # HERMES_HOME and the disk-cache path/fingerprint from the profile TLS; + # the detached worker thread below inherits NEITHER, so it must be + # captured here (on the request thread, where the TLS is valid) and + # re-bound on the worker. Empty / default for single-profile installs. + from contextlib import nullcontext as _nullcontext + + _active_profile_name = "" + _prof_env_request = None + _prof_scope_worker = None + try: + from api.profiles import ( + get_active_profile_name as _gapn, + profile_env_for_active_request as _prof_env_request, + profile_scope_for_detached_worker as _prof_scope_worker, + ) + _active_profile_name = (_gapn() or "").strip() + except Exception: + _prof_env_request = None + _prof_scope_worker = None + # Legacy synchronous (unbounded) rebuild — opt-in via budget<=0. if _LIVE_REBUILD_BUDGET_SECONDS <= 0: try: - result = _invoke_models_rebuild(_build_available_models_uncached) + # Foreground thread already carries the request-profile TLS; + # apply the profile env (no-op for default) for the live probe. + _sync_scope = ( + _prof_env_request("models rebuild (sync)") + if _prof_env_request is not None + else _nullcontext() + ) + with _sync_scope: + result = _invoke_models_rebuild(_build_available_models_uncached) except BaseException: # Always reset the flag so waiting threads don't block for 60s with _cache_build_cv: @@ -5329,20 +5359,34 @@ def get_available_models(*, prefer_cache: bool = False) -> dict: return True def _rebuild_worker(): - try: - box["result"] = _invoke_models_rebuild(_build_available_models_uncached) - except Exception as exc: # noqa: BLE001 — propagated to caller - box["error"] = exc - finally: - build_done.set() - # Only publish out-of-band if the foreground already gave up - # (over budget). Within budget the foreground publishes - # synchronously, so the worker must NOT touch the cache. - if budget_exceeded.is_set() and _claim_publish(): - if "result" in box: - _publish_models_result(box["result"]) - else: - _clear_build_in_progress() + # Re-bind the captured per-request profile on THIS worker thread + # (#3957): the daemon inherits neither the request-profile TLS nor + # os.environ, so without this it would probe the default profile's + # credentials and, over budget, publish the rebuilt catalog to the + # DEFAULT profile's disk cache. No-op for the default profile. + _worker_scope = ( + _prof_scope_worker(_active_profile_name, "models rebuild (worker)") + if _prof_scope_worker is not None + else _nullcontext() + ) + with _worker_scope: + try: + box["result"] = _invoke_models_rebuild(_build_available_models_uncached) + except Exception as exc: # noqa: BLE001 — propagated to caller + box["error"] = exc + finally: + build_done.set() + # Only publish out-of-band if the foreground already gave up + # (over budget). Within budget the foreground publishes + # synchronously, so the worker must NOT touch the cache. + # NOTE: the publish (and its disk write + fingerprint) runs + # INSIDE this profile scope so the over-budget path writes + # the correct profile's cache file. + if budget_exceeded.is_set() and _claim_publish(): + if "result" in box: + _publish_models_result(box["result"]) + else: + _clear_build_in_progress() _worker = threading.Thread( target=_rebuild_worker, diff --git a/api/profiles.py b/api/profiles.py index 2969fafb..e4050edd 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -821,6 +821,49 @@ def profile_env_for_active_request( yield +@contextmanager +def profile_scope_for_detached_worker( + profile_name, + purpose: str = "detached worker", + logger_override: Optional[logging.Logger] = None, +): + """Bind BOTH the per-request profile TLS and the profile env on a NEW thread (#3957). + + A detached worker thread (e.g. the ``models-catalog-rebuild`` daemon that + ``get_available_models`` spawns for a bounded rebuild) inherits neither the + spawning request's profile thread-local (issue #798) nor its ``os.environ``. + Without re-establishing both, the worker resolves the *default* profile: + - profile-keyed paths (``_get_models_cache_path`` / ``_get_config_path`` / + ``_get_auth_store_path`` / ``_models_cache_source_fingerprint``) read the + per-request profile via ``get_active_profile_name()`` — needs the TLS; + - credential lookups (``provider_model_ids`` / ``_lookup_custom_api_key_env``) + read ``os.environ`` — needs the profile ``.env`` applied. + + Pass the profile name CAPTURED on the spawning thread (where the TLS is + valid) into the worker, then enter this scope at the top of the worker body. + It sets the request-profile TLS for this (worker) thread and applies the + profile env via ``profile_env_for_background_worker``, restoring both on exit. + No-op for the default/root profile. + + Unlike ``profile_env_for_active_request`` (which reads the *current* thread's + TLS and must NOT clear it — the request thread keeps using it after the call), + this sets and then CLEARS the TLS, which is correct for a dedicated worker + thread that has no other use for it. + """ + name = (profile_name or "").strip() + if not name or _is_root_profile(name): + yield + return + set_request_profile(name) + try: + with profile_env_for_background_worker( + name, purpose, logger_override=logger_override + ): + yield + finally: + clear_request_profile() + + def _set_hermes_home(home: Path): """Set HERMES_HOME env var and monkey-patch cached module-level paths.""" os.environ['HERMES_HOME'] = str(home) diff --git a/api/routes.py b/api/routes.py index a54e4b5e..f7833569 100644 --- a/api/routes.py +++ b/api/routes.py @@ -5620,12 +5620,12 @@ def handle_get(handler, parsed) -> bool: return True if parsed.path == "/api/models": - # Apply the active per-request profile's env so the model catalog - # resolves against that profile's credentials, not the process-default - # profile's (#3957). No-op for the default profile. - from api.profiles import profile_env_for_active_request - with profile_env_for_active_request("/api/models", logger_override=logger): - return j(handler, get_available_models()) + # Profile-scoping for non-default profiles (#3957) is handled INSIDE + # get_available_models() — it binds the active profile's env + TLS on + # the detached rebuild worker (and the legacy synchronous rebuild), + # which the request-thread wrapper could not reach. See + # api.config.get_available_models cold path + profile_scope_for_detached_worker. + return j(handler, get_available_models()) if parsed.path == "/api/models/live": return _handle_live_models(handler, parsed) diff --git a/tests/test_issue3957_profile_providers_models.py b/tests/test_issue3957_profile_providers_models.py index 176b3a03..2fb66a66 100644 --- a/tests/test_issue3957_profile_providers_models.py +++ b/tests/test_issue3957_profile_providers_models.py @@ -168,15 +168,84 @@ def test_active_request_env_restores_on_exception(monkeypatch, tmp_path): def test_providers_and_models_routes_wrap_in_profile_env(): - """The two read routes invoke the profile-env context manager (#3957). + """The two read routes are profile-scoped for non-default profiles (#3957). - Structural guard: a future refactor that drops the wrapper would silently - reintroduce the bug, so pin the wiring at the source level. + Structural guard: a future refactor that drops the wiring would silently + reintroduce the bug, so pin it at the source level. + - /api/providers wraps the synchronous read in profile_env_for_active_request. + - /api/models relies on get_available_models() scoping its detached + rebuild worker via profile_scope_for_detached_worker (the request-thread + wrapper cannot reach the worker thread — Codex CORE finding). """ routes_src = Path(profiles.__file__).resolve().parent.joinpath("routes.py").read_text( encoding="utf-8" ) - # Both endpoint blocks must reference the active-request profile-env wrapper. assert "profile_env_for_active_request" in routes_src - # And it must be imported from the profiles module in routes.py. - assert routes_src.count("profile_env_for_active_request") >= 3 # 2 calls + >=1 import + config_src = Path(config.__file__).resolve().read_text(encoding="utf-8") + assert "profile_scope_for_detached_worker" in config_src + assert "_get_models_cache_path" in config_src + + +# ───────────────────────────────────────────────────────────────────────────── +# Facet A (worker thread) — the detached models-rebuild worker is profile-scoped +# (Codex CORE finding: the request-thread wrapper cannot reach the worker thread) +# ───────────────────────────────────────────────────────────────────────────── + + +def test_detached_worker_scope_noop_for_default_profile(monkeypatch): + """profile_scope_for_detached_worker is a no-op for the default profile.""" + monkeypatch.setattr(profiles, "_is_root_profile", lambda n: n in ("", "default")) + monkeypatch.delenv("ISSUE_3957_WPROBE", raising=False) + # Default/empty name → no TLS set, no env applied. + with profiles.profile_scope_for_detached_worker("default", "test"): + assert profiles.get_active_profile_name() in ("", "default") + assert os.environ.get("ISSUE_3957_WPROBE") is None + with profiles.profile_scope_for_detached_worker("", "test"): + assert os.environ.get("ISSUE_3957_WPROBE") is None + + +def test_detached_worker_scope_binds_profile_on_new_thread(monkeypatch, tmp_path): + """A worker thread re-binds the captured profile's TLS + env + cache path. + + Reproduces the Codex CORE finding: WITHOUT the scope a new thread resolves + the default profile (cache path models_cache.json, no profile env); WITH it + the thread resolves the captured profile's cache file + .env. + """ + import threading + + base = tmp_path / ".hermes" + (base / "profiles" / "work").mkdir(parents=True) + (base / "profiles" / "work" / ".env").write_text( + "ISSUE_3957_WPROBE=worker-env\n", encoding="utf-8" + ) + monkeypatch.setattr(profiles, "_DEFAULT_HERMES_HOME", base) + # Point the default models cache at an isolated tmp file so the named path + # derives from it (models_cache.work.json under the same dir). + default_cache = tmp_path / "state" / "models_cache.json" + default_cache.parent.mkdir(parents=True, exist_ok=True) + monkeypatch.setattr(config, "_models_cache_path", default_cache) + monkeypatch.delenv("ISSUE_3957_WPROBE", raising=False) + + out = {} + + def worker(): + # No TLS on this fresh thread yet → default profile resolution (the bug). + out["before_name"] = config._get_models_cache_path().name + out["before_env"] = os.environ.get("ISSUE_3957_WPROBE") + with profiles.profile_scope_for_detached_worker("work", "test-worker"): + out["inside_name"] = config._get_models_cache_path().name + out["inside_env"] = os.environ.get("ISSUE_3957_WPROBE") + out["after_name"] = config._get_models_cache_path().name + out["after_env"] = os.environ.get("ISSUE_3957_WPROBE") + + t = threading.Thread(target=worker) + t.start() + t.join() + + assert out["before_name"] == "models_cache.json" # default (no scope) + assert out["before_env"] is None + assert out["inside_name"] == "models_cache.work.json" # profile-scoped + assert out["inside_env"] == "worker-env" + assert out["after_name"] == "models_cache.json" # restored + assert out["after_env"] is None +