mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-29 18:46:59 +00:00
refactor(photon): resolve sidecar dir lazily, not at import time
resolve_sidecar_dir() probes the filesystem (touch/unlink) and can mirror sidecar files to HERMES_HOME. Doing that as a module-import side effect meant plugin discovery, `hermes --help`, and test collection all paid a filesystem probe (and possibly a mirror copy) just for importing the photon adapter or CLI. Convert _SIDECAR_DIR/_NPM_ERROR_LOG in adapter.py and cli.py to lazy cached accessors (_sidecar_dir()/_npm_error_log()); resolution now happens on first actual use. Existing tests that monkeypatch the _SIDECAR_DIR module global keep working — the accessors honor a non-None value. Adds a regression test proving import performs no resolution.
This commit is contained in:
Teknium
parent
0dfd5546fc
commit
b62fc24dfa
3 changed files with 108 additions and 28 deletions
|
|
@ -184,16 +184,38 @@ _DEDUP_WINDOW_SECONDS = 48 * 3600
|
|||
|
||||
_FFFC_WAIT_SECONDS = 15.0 # Timeout for waiting on an attachment after a U+FFFC placeholder.
|
||||
|
||||
# Resolved once at import: the installed plugin tree when writable (dev
|
||||
# Resolved lazily on first use: the installed plugin tree when writable (dev
|
||||
# installs), or a mirror on the durable data volume when the install tree
|
||||
# is immutable and the baked deps are missing/stale (hosted images, NS-606).
|
||||
# See sidecar_paths.resolve_sidecar_dir for the full decision table.
|
||||
#
|
||||
# Resolution is deliberately NOT done at import time: resolve_sidecar_dir()
|
||||
# probes the filesystem (touch/unlink) and may mirror files to the data
|
||||
# volume — side effects that must not fire just because something imported
|
||||
# this module (hermes status, test collection, plugin discovery).
|
||||
from .sidecar_paths import dir_writable as _dir_writable, resolve_sidecar_dir
|
||||
|
||||
_SIDECAR_DIR = resolve_sidecar_dir()
|
||||
_NPM_ERROR_LOG = _SIDECAR_DIR / ".photon-npm-error.log"
|
||||
# Tests monkeypatch these module globals directly; the accessors below
|
||||
# honor a non-None value and only resolve/derive when unset.
|
||||
_SIDECAR_DIR: Optional[Path] = None
|
||||
_NPM_ERROR_LOG: Optional[Path] = None
|
||||
_NPM_ERROR_LOG_MAX_CHARS = 300
|
||||
|
||||
|
||||
def _sidecar_dir() -> Path:
|
||||
"""Sidecar runtime dir, resolved once on first use (never at import)."""
|
||||
global _SIDECAR_DIR
|
||||
if _SIDECAR_DIR is None:
|
||||
_SIDECAR_DIR = resolve_sidecar_dir()
|
||||
return _SIDECAR_DIR
|
||||
|
||||
|
||||
def _npm_error_log() -> Path:
|
||||
"""Path of the persisted npm-failure log (derived from the sidecar dir)."""
|
||||
if _NPM_ERROR_LOG is not None:
|
||||
return _NPM_ERROR_LOG
|
||||
return _sidecar_dir() / ".photon-npm-error.log"
|
||||
|
||||
# Cap on a self-heal `npm ci`/`npm install` of the sidecar deps. A cold
|
||||
# install of the pinned spectrum-ts tree normally takes well under a minute;
|
||||
# a wedged npm (dead registry, network blackhole) must not stall the photon
|
||||
|
|
@ -314,7 +336,7 @@ def sidecar_deps_installed() -> bool:
|
|||
_start_sidecar(), and `hermes photon status` so all three agree on
|
||||
what "installed" means.
|
||||
"""
|
||||
return (_SIDECAR_DIR / "node_modules" / "spectrum-ts").exists()
|
||||
return (_sidecar_dir() / "node_modules" / "spectrum-ts").exists()
|
||||
|
||||
|
||||
def _coerce_float(value: Any, default: float) -> float:
|
||||
|
|
@ -381,7 +403,7 @@ def check_requirements() -> bool:
|
|||
# user has no CLI to run `hermes photon setup`, so the connect path
|
||||
# must self-heal). Otherwise keep returning False so
|
||||
# `hermes setup` / status surface the missing-deps state.
|
||||
if bool(shutil.which("npm")) and _dir_writable(_SIDECAR_DIR):
|
||||
if bool(shutil.which("npm")) and _dir_writable(_sidecar_dir()):
|
||||
return True
|
||||
# DEBUG (not WARNING): this is the normal pre-setup state.
|
||||
# check_fn() is called from multiple hot paths in the core
|
||||
|
|
@ -389,21 +411,21 @@ def check_requirements() -> bool:
|
|||
# WARNING here would spam logs on every probe for unconfigured photon.
|
||||
npm_error = ""
|
||||
try:
|
||||
if _NPM_ERROR_LOG.exists():
|
||||
npm_error = _NPM_ERROR_LOG.read_text(encoding="utf-8").strip()[:_NPM_ERROR_LOG_MAX_CHARS]
|
||||
if _npm_error_log().exists():
|
||||
npm_error = _npm_error_log().read_text(encoding="utf-8").strip()[:_NPM_ERROR_LOG_MAX_CHARS]
|
||||
except OSError:
|
||||
pass
|
||||
if npm_error:
|
||||
logger.debug(
|
||||
"photon: spectrum-ts not installed at %s "
|
||||
"(last npm error: %s) — run: hermes photon setup",
|
||||
_SIDECAR_DIR,
|
||||
_sidecar_dir(),
|
||||
npm_error,
|
||||
)
|
||||
else:
|
||||
logger.debug(
|
||||
"photon: spectrum-ts not installed at %s — run: hermes photon setup",
|
||||
_SIDECAR_DIR,
|
||||
_sidecar_dir(),
|
||||
)
|
||||
return False
|
||||
return True
|
||||
|
|
@ -419,8 +441,8 @@ def _sidecar_deps_stale() -> bool:
|
|||
same signal ``npm ci`` uses. Returns False (do nothing) if either file is
|
||||
missing or unreadable, so a first-run or odd filesystem never blocks start.
|
||||
"""
|
||||
lockfile = _SIDECAR_DIR / "package-lock.json"
|
||||
marker = _SIDECAR_DIR / "node_modules" / ".package-lock.json"
|
||||
lockfile = _sidecar_dir() / "package-lock.json"
|
||||
marker = _sidecar_dir() / "node_modules" / ".package-lock.json"
|
||||
try:
|
||||
return lockfile.stat().st_mtime > marker.stat().st_mtime
|
||||
except OSError:
|
||||
|
|
@ -447,7 +469,7 @@ def _reinstall_sidecar_deps() -> None:
|
|||
try:
|
||||
result = subprocess.run( # noqa: S603
|
||||
[npm, "ci"],
|
||||
cwd=str(_SIDECAR_DIR),
|
||||
cwd=str(_sidecar_dir()),
|
||||
capture_output=True,
|
||||
text=True, encoding="utf-8", errors="replace",
|
||||
check=False,
|
||||
|
|
@ -460,7 +482,7 @@ def _reinstall_sidecar_deps() -> None:
|
|||
)
|
||||
result = subprocess.run( # noqa: S603
|
||||
[npm, "install"],
|
||||
cwd=str(_SIDECAR_DIR),
|
||||
cwd=str(_sidecar_dir()),
|
||||
capture_output=True,
|
||||
text=True, encoding="utf-8", errors="replace",
|
||||
check=False,
|
||||
|
|
@ -1538,7 +1560,7 @@ class PhotonAdapter(BasePlatformAdapter):
|
|||
# Cold install (NS-606): on hosted/managed images the install
|
||||
# tree is immutable and the user has no CLI to run
|
||||
# `hermes photon setup`, so the connect path must be able to
|
||||
# bootstrap the deps itself. _SIDECAR_DIR has already been
|
||||
# bootstrap the deps itself. _sidecar_dir() has already been
|
||||
# resolved to a writable location (or mirrored to the data
|
||||
# volume) by sidecar_paths.resolve_sidecar_dir; `npm ci` off
|
||||
# the committed lockfile is deterministic and bounded by
|
||||
|
|
@ -1547,14 +1569,14 @@ class PhotonAdapter(BasePlatformAdapter):
|
|||
# install (empty node_modules/) also triggers the reinstall.
|
||||
logger.info(
|
||||
"[photon] sidecar deps not installed; installing into %s",
|
||||
_SIDECAR_DIR,
|
||||
_sidecar_dir(),
|
||||
)
|
||||
await asyncio.to_thread(_reinstall_sidecar_deps)
|
||||
if not sidecar_deps_installed():
|
||||
raise RuntimeError(
|
||||
f"Photon sidecar deps could not be installed into "
|
||||
f"{_SIDECAR_DIR} (see log for the npm error). "
|
||||
f"Run: cd {_SIDECAR_DIR} && npm ci (or `hermes photon setup`)"
|
||||
f"{_sidecar_dir()} (see log for the npm error). "
|
||||
f"Run: cd {_sidecar_dir()} && npm ci (or `hermes photon setup`)"
|
||||
)
|
||||
# A `hermes update` that bumps the spectrum-ts pin rewrites
|
||||
# package-lock.json but never reinstalls node_modules, so the sidecar
|
||||
|
|
@ -1597,8 +1619,8 @@ class PhotonAdapter(BasePlatformAdapter):
|
|||
subprocess.run, # noqa: S603
|
||||
[
|
||||
self._node_bin,
|
||||
str(_SIDECAR_DIR / "patch-spectrum-mixed-attachments.mjs"),
|
||||
str(_SIDECAR_DIR),
|
||||
str(_sidecar_dir() / "patch-spectrum-mixed-attachments.mjs"),
|
||||
str(_sidecar_dir()),
|
||||
],
|
||||
capture_output=True,
|
||||
text=True, encoding='utf-8', errors='replace',
|
||||
|
|
@ -1619,7 +1641,7 @@ class PhotonAdapter(BasePlatformAdapter):
|
|||
)
|
||||
|
||||
self._sidecar_proc = subprocess.Popen( # noqa: S603
|
||||
[self._node_bin, str(_SIDECAR_DIR / "index.mjs")],
|
||||
[self._node_bin, str(_sidecar_dir() / "index.mjs")],
|
||||
stdin=subprocess.PIPE,
|
||||
stdout=subprocess.PIPE,
|
||||
stderr=subprocess.STDOUT,
|
||||
|
|
|
|||
|
|
@ -33,11 +33,31 @@ from .adapter import _NPM_ERROR_LOG_MAX_CHARS, sidecar_deps_installed
|
|||
from .sidecar_paths import resolve_sidecar_dir
|
||||
|
||||
# Writable sidecar runtime dir (mirrors to HERMES_HOME on immutable
|
||||
# installs — NS-606). All npm/setup work happens here.
|
||||
_SIDECAR_DIR = resolve_sidecar_dir()
|
||||
# installs — NS-606). All npm/setup work happens here. Resolved lazily on
|
||||
# first use — resolve_sidecar_dir() probes the filesystem and may mirror
|
||||
# files, side effects that must not fire at import time (e.g. when argparse
|
||||
# wiring imports this module for `hermes --help`).
|
||||
# Tests monkeypatch these module globals directly; the accessors honor a
|
||||
# non-None value and only resolve/derive when unset.
|
||||
_SIDECAR_DIR: Path | None = None
|
||||
# Written on npm failure so check_requirements() can surface the root cause
|
||||
# when called later (gateway start, hermes status). Cleared on success.
|
||||
_NPM_ERROR_LOG = _SIDECAR_DIR / ".photon-npm-error.log"
|
||||
_NPM_ERROR_LOG: Path | None = None
|
||||
|
||||
|
||||
def _sidecar_dir() -> Path:
|
||||
"""Sidecar runtime dir, resolved once on first use (never at import)."""
|
||||
global _SIDECAR_DIR
|
||||
if _SIDECAR_DIR is None:
|
||||
_SIDECAR_DIR = resolve_sidecar_dir()
|
||||
return _SIDECAR_DIR
|
||||
|
||||
|
||||
def _npm_error_log() -> Path:
|
||||
"""Path of the persisted npm-failure log (derived from the sidecar dir)."""
|
||||
if _NPM_ERROR_LOG is not None:
|
||||
return _NPM_ERROR_LOG
|
||||
return _sidecar_dir() / ".photon-npm-error.log"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
@ -438,13 +458,13 @@ def _install_sidecar() -> int:
|
|||
# `npm ci` installs the committed lockfile verbatim; fall back to
|
||||
# `npm install` when the lockfile is missing or drifted (e.g. a dev
|
||||
# checkout mid-upgrade).
|
||||
print(f" $ cd {_SIDECAR_DIR} && {npm} ci")
|
||||
print(f" $ cd {_sidecar_dir()} && {npm} ci")
|
||||
# stdout is not captured so npm progress prints to the terminal in real
|
||||
# time. stderr is captured so we can persist the failure reason for
|
||||
# check_requirements() to surface after the process exits.
|
||||
proc = subprocess.run( # noqa: S603
|
||||
[npm, "ci"],
|
||||
cwd=str(_SIDECAR_DIR),
|
||||
cwd=str(_sidecar_dir()),
|
||||
check=False,
|
||||
stderr=subprocess.PIPE,
|
||||
text=True,
|
||||
|
|
@ -455,7 +475,7 @@ def _install_sidecar() -> int:
|
|||
print(f" npm ci failed — falling back to: {npm} install")
|
||||
proc = subprocess.run( # noqa: S603
|
||||
[npm, "install"],
|
||||
cwd=str(_SIDECAR_DIR),
|
||||
cwd=str(_sidecar_dir()),
|
||||
check=False,
|
||||
stderr=subprocess.PIPE,
|
||||
text=True,
|
||||
|
|
@ -469,12 +489,12 @@ def _install_sidecar() -> int:
|
|||
error = (proc.stderr or "").strip()[:_NPM_ERROR_LOG_MAX_CHARS]
|
||||
if error:
|
||||
try:
|
||||
_NPM_ERROR_LOG.write_text(error, encoding="utf-8")
|
||||
_npm_error_log().write_text(error, encoding="utf-8")
|
||||
except OSError:
|
||||
pass
|
||||
else:
|
||||
try:
|
||||
_NPM_ERROR_LOG.unlink()
|
||||
_npm_error_log().unlink()
|
||||
except OSError:
|
||||
pass
|
||||
return proc.returncode
|
||||
|
|
|
|||
|
|
@ -147,3 +147,41 @@ def test_dir_writable_probe(tmp_path) -> None:
|
|||
assert sidecar_paths.dir_writable(ro) is False
|
||||
finally:
|
||||
ro.chmod(0o755)
|
||||
|
||||
|
||||
def test_adapter_import_does_not_resolve_sidecar_dir(monkeypatch) -> None:
|
||||
"""Importing the adapter must not probe the filesystem or mirror files.
|
||||
|
||||
resolve_sidecar_dir() touch/unlink-probes the source tree and may copy
|
||||
files to HERMES_HOME; the adapter and CLI resolve lazily on first use so
|
||||
a bare import (plugin discovery, `hermes --help`, test collection) has
|
||||
no filesystem side effects.
|
||||
"""
|
||||
import importlib
|
||||
|
||||
from plugins.platforms.photon import adapter as photon_adapter
|
||||
from plugins.platforms.photon import cli as photon_cli
|
||||
|
||||
def _boom(*args, **kwargs): # pragma: no cover - failure path
|
||||
raise AssertionError("resolve_sidecar_dir called at import time")
|
||||
|
||||
monkeypatch.setattr(sidecar_paths, "resolve_sidecar_dir", _boom)
|
||||
try:
|
||||
importlib.reload(photon_adapter)
|
||||
importlib.reload(photon_cli)
|
||||
# Nothing resolved yet.
|
||||
assert photon_adapter._SIDECAR_DIR is None
|
||||
assert photon_cli._SIDECAR_DIR is None
|
||||
# First real use resolves (and would call resolve_sidecar_dir).
|
||||
with pytest.raises(AssertionError, match="import time"):
|
||||
photon_adapter._sidecar_dir()
|
||||
# A monkeypatched _SIDECAR_DIR (the pattern existing tests use) is
|
||||
# honored without touching the resolver.
|
||||
monkeypatch.setattr(photon_adapter, "_SIDECAR_DIR", Path("/tmp/x"))
|
||||
assert photon_adapter._sidecar_dir() == Path("/tmp/x")
|
||||
assert photon_adapter._npm_error_log() == Path("/tmp/x/.photon-npm-error.log")
|
||||
finally:
|
||||
# Restore real bindings for any later test importing these modules.
|
||||
monkeypatch.undo()
|
||||
importlib.reload(photon_adapter)
|
||||
importlib.reload(photon_cli)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue