fix(gateway): resolve PairingStore's default pairing dir lazily, not at import time
PairingStore(profile=None) resolved its storage directory from the module-level PAIRING_DIR constant, which was computed exactly once, at module import time. A long-lived process (the gateway, started once at container/process boot) can import this module before HERMES_HOME or a profile's context is fully established, freezing PAIRING_DIR to a wrong value for the rest of that process's lifetime -- even though a freshly started, short-lived process (e.g. the `hermes pairing` CLI) re-imports the module later with the environment already correct. That asymmetry is exactly what made pending pairing codes issued by the gateway process unrecoverable (the pending-code write landed under the stale, wrong directory) while CLI-invoked writes to the same nominal directory kept working -- see #93449 for the full writeup and a live reproduction. tests/hermes_cli/test_dashboard_admin_endpoints.py already carried a comment acknowledging this exact staleness in passing ("the module-level PAIRING_DIR is bound at import"), and TestProfileScopedStorage::test_default_store_uses_global_dir's own comment describes working around it rather than it being intentional behavior -- this fixes the underlying cause both were compensating for. The profile-scoped branch already resolved its directory lazily inside __init__ (matching this docstring's claim that resolution is lazy); this brings the non-profile branch in line with it. Fix keeps PAIRING_DIR as the same test seam already used throughout the test suite (`patch("gateway.pairing.PAIRING_DIR", tmp_path)`, ~30 call sites) unchanged: it's now a None sentinel instead of an eagerly computed path, and a new _default_pairing_dir() helper resolves it fresh on every call, honoring a patched (non-None) value when one is set. No existing test needed to change. Added a regression test that does not patch PAIRING_DIR directly and instead exercises the real lazy-resolution path across two different HERMES_HOME values in the same process -- confirmed it fails on the pre-fix code (gets stuck with whatever the first PairingStore() call in the test session happened to see) and passes with the fix. Verified: tests/gateway/test_pairing.py (39, incl. the new one), tests/hermes_cli/test_pairing.py, and tests/tools/test_pr_6656_regressions.py all pass unmodified.
This commit is contained in:
committed by
Teknium
parent
905edf37a4
commit
e210fd8c1f
+33
-3
@@ -56,7 +56,37 @@ LOCKOUT_SECONDS = 3600 # Lockout duration after too many failures
|
||||
MAX_PENDING_PER_PLATFORM = 3 # Max pending codes per platform
|
||||
MAX_FAILED_ATTEMPTS = 5 # Failed approvals before lockout
|
||||
|
||||
PAIRING_DIR = get_hermes_dir("platforms/pairing", "pairing")
|
||||
# Default (non-profile-scoped) pairing directory. Left unresolved (``None``)
|
||||
# here rather than computed eagerly: this module is imported once by the
|
||||
# long-lived gateway process at container/process boot, and computing the
|
||||
# path eagerly freezes it to whatever HERMES_HOME/profile context existed
|
||||
# at that exact import moment for the rest of the process's lifetime --
|
||||
# even if a context-local override (see hermes_constants.set_hermes_home_override)
|
||||
# is established afterward. A freshly-started, short-lived process (e.g. the
|
||||
# ``hermes pairing`` CLI) re-imports this module later with the final
|
||||
# environment already in place, so it never observes the stale value -- the
|
||||
# resulting asymmetry is what made pending pairing codes issued by the
|
||||
# gateway unrecoverable while CLI-side writes to the same directory kept
|
||||
# working (NousResearch/hermes-agent#93449).
|
||||
#
|
||||
# ``_default_pairing_dir()`` below resolves this fresh on every call in
|
||||
# production. Tests patch this attribute directly to a concrete path for
|
||||
# isolation (e.g. ``patch("gateway.pairing.PAIRING_DIR", tmp_path)``); that
|
||||
# continues to work unchanged, since a patched (non-``None``) value takes
|
||||
# precedence over recomputing.
|
||||
PAIRING_DIR = None
|
||||
|
||||
|
||||
def _default_pairing_dir() -> Path:
|
||||
"""Resolve the default (non-profile-scoped) pairing directory.
|
||||
|
||||
Recomputed on every call rather than cached at import time -- see the
|
||||
``PAIRING_DIR`` comment above for why. Honors ``PAIRING_DIR`` when a
|
||||
caller (typically a test) has explicitly set it to a concrete path.
|
||||
"""
|
||||
if PAIRING_DIR is not None:
|
||||
return PAIRING_DIR
|
||||
return get_hermes_dir("platforms/pairing", "pairing")
|
||||
|
||||
|
||||
# Platform value -> its per-platform allowlist env var. When an operator has
|
||||
@@ -371,7 +401,7 @@ def _migrate_split_pairing_dirs(
|
||||
home = home or get_hermes_home()
|
||||
old_dir = home / "pairing"
|
||||
new_dir = home / "platforms" / "pairing"
|
||||
active = active or PAIRING_DIR
|
||||
active = active if active is not None else _default_pairing_dir()
|
||||
alternate = new_dir if active.resolve() == old_dir.resolve() else old_dir
|
||||
_merge_pairing_dir(active, alternate)
|
||||
|
||||
@@ -434,7 +464,7 @@ class PairingStore:
|
||||
home=profile_home,
|
||||
)
|
||||
else:
|
||||
self._dir = PAIRING_DIR
|
||||
self._dir = _default_pairing_dir()
|
||||
self._dir.mkdir(parents=True, exist_ok=True)
|
||||
if profile:
|
||||
# Explicit stores must resolve exactly as a standalone
|
||||
|
||||
@@ -574,6 +574,31 @@ class TestProfileScopedStorage:
|
||||
assert store._dir == tmp_path
|
||||
assert store._approved_path("weixin") == tmp_path / "weixin-approved.json"
|
||||
|
||||
def test_default_store_is_not_frozen_at_first_use(self, tmp_path, monkeypatch):
|
||||
"""Regression test for #93449.
|
||||
|
||||
PairingStore() (no profile) must not freeze its directory to
|
||||
whatever HERMES_HOME resolved to the first time this module's
|
||||
default path was computed. A long-lived process (the gateway,
|
||||
started once at container/process boot) can construct a
|
||||
PairingStore before HERMES_HOME/profile context is fully
|
||||
established; a later store in the same process must still pick up
|
||||
the real, current value instead of being stuck with a stale one.
|
||||
Deliberately does not patch PAIRING_DIR directly, unlike the sibling
|
||||
test above -- this exercises the real (unpatched) lazy-resolution
|
||||
path itself.
|
||||
"""
|
||||
first_home = tmp_path / "first"
|
||||
second_home = tmp_path / "second"
|
||||
|
||||
monkeypatch.setattr("hermes_constants.get_hermes_home", lambda: first_home)
|
||||
first_store = PairingStore()
|
||||
assert first_store._dir == first_home / "platforms" / "pairing"
|
||||
|
||||
monkeypatch.setattr("hermes_constants.get_hermes_home", lambda: second_home)
|
||||
second_store = PairingStore()
|
||||
assert second_store._dir == second_home / "platforms" / "pairing"
|
||||
|
||||
def test_profile_store_uses_profiles_subdir(self, tmp_path, monkeypatch):
|
||||
"""Explicit profile stores use that profile's normal Hermes layout."""
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
|
||||
Reference in New Issue
Block a user