From b62fc24dfa64a33a4693f7df9336630d38ebd431 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Tue, 28 Jul 2026 21:53:40 -0700 Subject: [PATCH] refactor(photon): resolve sidecar dir lazily, not at import time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- plugins/platforms/photon/adapter.py | 62 +++++++++++++------ plugins/platforms/photon/cli.py | 36 ++++++++--- .../platforms/photon/test_sidecar_paths.py | 38 ++++++++++++ 3 files changed, 108 insertions(+), 28 deletions(-) diff --git a/plugins/platforms/photon/adapter.py b/plugins/platforms/photon/adapter.py index 6a4910ca0f4b..6b9fa2461310 100644 --- a/plugins/platforms/photon/adapter.py +++ b/plugins/platforms/photon/adapter.py @@ -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, diff --git a/plugins/platforms/photon/cli.py b/plugins/platforms/photon/cli.py index da64912685d9..1cc19f6e82bf 100644 --- a/plugins/platforms/photon/cli.py +++ b/plugins/platforms/photon/cli.py @@ -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 diff --git a/tests/plugins/platforms/photon/test_sidecar_paths.py b/tests/plugins/platforms/photon/test_sidecar_paths.py index d9791e134f02..1a744e3a3292 100644 --- a/tests/plugins/platforms/photon/test_sidecar_paths.py +++ b/tests/plugins/platforms/photon/test_sidecar_paths.py @@ -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)