diff --git a/cron/scheduler.py b/cron/scheduler.py index b22cabe226..dc5a834f5b 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -35,6 +35,14 @@ from typing import Any, Callable, Dict, List, Optional, Protocol, Union # `hermes update`) otherwise fail with ModuleNotFoundError for hermes_time et al. sys.path.insert(0, str(Path(__file__).parent.parent)) +# Cron external worker: acquire the committed dependency generation's lifetime lease and +# activate its site-packages on this process's path, *before* any Hermes package import. +# Without this the collector may delete an unselected generation between the gateway's +# exit and the worker's next import; without site-packages the first dependency import +# fails (`No module named 'ruamel'`). No-op when the runner owns its dependencies. +from cron.worker_bootstrap import worker_bootstrap +worker_bootstrap() + from hermes_constants import get_hermes_home, hermes_home_key from hermes_cli.observability.shared_metrics_gateway import note_cron_execution, note_cron_skipped from cron.env_settings import cron_env_setting diff --git a/cron/worker_bootstrap.py b/cron/worker_bootstrap.py new file mode 100644 index 0000000000..a72f4a9e43 --- /dev/null +++ b/cron/worker_bootstrap.py @@ -0,0 +1,151 @@ +"""Cron external worker: lifetime protection and dependency activation at spawn. + +The restart-safe external worker (``sys.executable -m cron.scheduler``) is a fresh +process. Its own ``sys.path`` is the store Python's, which owns no third-party +dependencies; the gateway's dependency generation is not on that path. + +``pin_hermes_tree_on_pythonpath`` (``cron/scheduler_worker_env.py``) restores the +site-packages directory into the child's ``PYTHONPATH`` so the worker can import +Hermes dependencies. That directory alone, however, is not a retention +permit: the PM collector removes an unselected, lease-managed generation once it +is old enough and no process holds its lease. The gateway holds a lease on the +generation it is running; the freshly spawned worker does not, so between the +gateway's exit and the worker's next import the collector may delete the very +generation the worker needs (``#122290`` P2, ``andrexibiza`` review). + +This module closes that gap at the worker entry, *before* the first +application/dependency import: + +* ``lease_dependencies()`` acquires the generation's kernel lease (the same + primitive ``pm.environments.activate_dependencies`` uses) so the collector + cannot remove it while the worker imports or runs. +* ``activate_worker_dependencies()`` puts the committed generation's + ``site-packages`` on this process's ``sys.path`` (``site.addsitedir``), so + dependency imports resolve without waiting for the gateway's handoff. + +Both are no-ops (or degrade to a warning) when no dependency generation is +committed — a runner that owns its dependencies (wheel / pipx / test / +developer venv) needs nothing added. +""" + +from __future__ import annotations + +import logging +import os +import sys +from pathlib import Path + +logger = logging.getLogger(__name__) + +_root = Path(__file__).resolve().parent.parent +_LEASE_ACQUIRED = False +_ACTIVATED = False + + +def _activate_generation() -> Path | None: + """Locate the committed dependency venv, or ``None`` when there is none. + + Uses the same resolution ``pm.environments.activate_dependencies`` uses + (``committed_venv`` — never the in-tree ``venv``/``.venv``). Returns the + *venv* directory (the one containing ``pyvenv.cfg``) that + ``lease_generation`` expects; the function's ``.parent`` is the + lease-managed generation. + Stdlib-only reads: a missing facts file means "nothing committed", not an + error. + """ + try: + from pm.environments import committed_venv + except Exception: + # pm is not yet importable (pre-PM tree / sealed payload); nothing to lease. + return None + try: + venv = committed_venv(_root) + except Exception: + # Corrupt facts file: degrade to a warning, never brick the worker. + return None + if venv is None: + return None + # A committed venv must carry the marker; a directory that merely happens to + # be named like one is not a retention target. + if not (venv / "pyvenv.cfg").is_file(): + return None + return venv + + +def lease_dependencies() -> None: + """Hold the committed dependency generation's kernel lease for this worker's lifetime. + + Must be called *before* any application or dependency import, because the + collector's safety condition is "no process holds the lease"; a late lease + only protects imports that happen after it. ``lease_generation`` takes the + committed venv path and leases its parent (the generation directory), so a + generation that is no longer the selected one still survives the collector + while the worker holds the lease. The lease is released by ``atexit`` + (``hermes_cli.runtime_state.lease_directory``), so an early crash still + releases it on exit. Idempotent within a process. + """ + global _LEASE_ACQUIRED + if _LEASE_ACQUIRED: + return + venv = _activate_generation() + if venv is None: + return + try: + from hermes_cli.runtime_state import lease_generation + lease_generation(venv) + _LEASE_ACQUIRED = True + except Exception as exc: # pragma: no cover - degradation path + logger.warning("cron worker could not lease dependency generation: %s", exc) + + +def activate_worker_dependencies() -> None: + """Put the committed generation's ``site-packages`` on this process's path. + + Delegates to ``pm.environments.activate_dependencies`` — the same + resolution, ``site.addsitedir`` and ``sys.path`` reordering the gateway's + boot uses — so the worker imports exactly what the gateway was running. + No-op when there is no committed generation. Idempotent within a process. + """ + global _ACTIVATED + if _ACTIVATED: + return + try: + from pm.environments import activate_dependencies + activate_dependencies(_root) + _ACTIVATED = True + except Exception as exc: # pragma: no cover - degradation path + logger.warning("cron worker dependency activation failed: %s", exc) + + +def worker_bootstrap() -> None: + """Entry-point hook: lease then activate, in that order. + + Leasing first guarantees the generation survives even if activation is + later skipped; activating second puts ``site-packages`` on the path so the + first dependency import succeeds. Call this at the very top of + ``cron/scheduler.py`` (after the stdlib-only ``sys.path`` pin, before any + Hermes package import). + + Activation is gated on the process being a *restart-safe external worker* + (one spawned as ``python -m cron.scheduler --external-worker-file ...``): + such a worker is a fresh interpreter whose own ``sys.path`` is the store + Python's and carries no dependency packages, so it must activate. A + gateway process already runs on its activated dependency graph (booted + through ``hermes_bootstrap`` → ``activate_dependencies``); re-activating + there is a harmless no-op but is skipped so the gateway keeps its exact + launch contract. Leasing is unconditional: it is idempotent and protects + the generation the worker (or the gateway) will use regardless. + """ + lease_dependencies() + if _is_restart_safe_worker(): + activate_worker_dependencies() + + +def _is_restart_safe_worker() -> bool: + """True only when this process is the externally spawned restart-safe worker. + + The spawn site (``_launch_external_cron_worker``) passes + ``--external-worker-file``; the gateway runs in-process with no such flag, + and other CLI invocations of ``cron/scheduler`` do not use it either. + """ + return "--external-worker-file" in sys.argv diff --git a/tests/cron/test_restart_safe_worker.py b/tests/cron/test_restart_safe_worker.py index 8ceb6e9ffc..0af1197c9d 100644 --- a/tests/cron/test_restart_safe_worker.py +++ b/tests/cron/test_restart_safe_worker.py @@ -651,6 +651,7 @@ def test_launch_external_worker_pin_extends_the_sanitized_env_not_os_environ( assert "PYTHONPATH" not in worker_env_mod.pin_hermes_tree_on_pythonpath({}, repo_root) + def test_pin_restores_activated_dependency_site_packages(monkeypatch, tmp_path): """#122222: on a self-managed install the sanitizer drops the activated dependency site-packages and the store-Python worker cannot import them; the pin restores them @@ -727,6 +728,203 @@ def test_activated_dependency_site_packages_requires_real_venv(monkeypatch, tmp_ assert worker_env_mod._activated_dependency_site_packages() is None +def test_worker_bootstrap_lease_and_activation(tmp_path, monkeypatch): + """#122290 P2 (andrexibiza review): the externally spawned worker must hold the + committed generation's kernel lease *and* activate its site-packages on its own + sys.path, both *before* the first application/dependency import. Without the lease + the PM collector may delete an unselected generation between the gateway's exit and + the worker's next import; without activation the first import fails + (``No module named 'ruamel'``). + + The fixture arranges: + * a committed venv whose ``.parent`` carries ``.lease-managed`` (so + ``lease_directory`` takes a real kernel lock), + * a fake ``lease_generation`` that records the call, + * a fake ``activate_dependencies`` that records the call, + * the worker flag on ``sys.argv`` so ``worker_bootstrap`` performs activation. + """ + import cron.worker_bootstrap as wb + from pathlib import Path + + # Reset module-level idempotency flags so the assertions see fresh state. + wb._LEASE_ACQUIRED = False + wb._ACTIVATED = False + try: + # Build a committed venv layout under tmp_path: + # tmp_path/install/environments//venv -> pyvenv.cfg + # tmp_path/install/environments//.lease-managed + install_state = tmp_path / "install_state" + generation = install_state / "environments" / "g1" + venv = generation / "venv" + venv.mkdir(parents=True) + (venv / "pyvenv.cfg").write_text("home = /usr/bin\n", encoding="utf-8") + (generation / ".lease-managed").write_text("", encoding="utf-8") + + # Point _activate_generation at our fixture layout. + monkeypatch.setattr(wb, "_root", tmp_path) + + # Stub _activate_generation to return the venv path (the fixture). + monkeypatch.setattr(wb, "_activate_generation", lambda: venv) + + # Stub lease_generation to record the call without touching the real lock. + lease_calls = [] + def _fake_lease_generation(environment): + lease_calls.append(environment) + return lambda: None + monkeypatch.setattr( + "hermes_cli.runtime_state.lease_generation", _fake_lease_generation, + raising=False, + ) + # The real lease_generation is imported inside lease_dependencies via + # "from hermes_cli.runtime_state import lease_generation", so we must patch + # the module attribute before the import runs. + import hermes_cli.runtime_state as _rs + monkeypatch.setattr(_rs, "lease_generation", _fake_lease_generation) + + # Stub activate_dependencies to record the call. + activate_calls = [] + def _fake_activate(project_root): + activate_calls.append(project_root) + monkeypatch.setattr( + "pm.environments.activate_dependencies", _fake_activate, + raising=False, + ) + import pm.environments as _env + monkeypatch.setattr(_env, "activate_dependencies", _fake_activate) + + # Simulate the restart-safe worker flag. + old_argv = list(sys.argv) + sys.argv = old_argv + ["--external-worker-file", "x.json", "--ack-file", "a.ready"] + try: + wb.worker_bootstrap() + finally: + sys.argv = old_argv + + # Lease was taken on the venv path (lease_generation internally does .parent). + assert len(lease_calls) == 1 + assert lease_calls[0] == venv + # Activation was performed (worker flag was present). + assert len(activate_calls) == 1 + assert activate_calls[0] == tmp_path # _root passed to activate_dependencies + # Idempotency flags set. + assert wb._LEASE_ACQUIRED is True + assert wb._ACTIVATED is True + finally: + wb._LEASE_ACQUIRED = False + wb._ACTIVATED = False + + +def test_worker_bootstrap_leases_but_skips_activation_in_gateway(tmp_path, monkeypatch): + """The gateway process (no ``--external-worker-file`` flag) leases the generation + (unconditionally, idempotently) but does *not* re-activate dependencies, so the + gateway keeps its exact launch contract (already booted through hermes_bootstrap). + """ + import cron.worker_bootstrap as wb + + wb._LEASE_ACQUIRED = False + wb._ACTIVATED = False + try: + venv = tmp_path / "install" / "environments" / "g1" / "venv" + venv.mkdir(parents=True) + (venv / "pyvenv.cfg").write_text("home = /usr/bin\n", encoding="utf-8") + monkeypatch.setattr(wb, "_root", tmp_path) + monkeypatch.setattr(wb, "_activate_generation", lambda: venv) + + lease_calls = [] + def _fake_lease_generation(environment): + lease_calls.append(environment) + return lambda: None + import hermes_cli.runtime_state as _rs + monkeypatch.setattr(_rs, "lease_generation", _fake_lease_generation) + + activate_calls = [] + def _fake_activate(project_root): + activate_calls.append(project_root) + import pm.environments as _env + monkeypatch.setattr(_env, "activate_dependencies", _fake_activate) + + old_argv = list(sys.argv) + sys.argv = old_argv # no --external-worker-file flag + try: + wb.worker_bootstrap() + finally: + sys.argv = old_argv + + # Lease still taken (unconditional). + assert len(lease_calls) == 1 + # Activation NOT performed (gateway path). + assert activate_calls == [] + assert wb._ACTIVATED is False + finally: + wb._LEASE_ACQUIRED = False + wb._ACTIVATED = False + + +def test_worker_bootstrap_no_committed_venv_is_noop(tmp_path, monkeypatch): + """When there is no committed dependency generation (pre-PM install, wheel/pipx + runner that owns its own deps), both lease and activation are no-ops; the worker + proceeds on whatever its interpreter already carries.""" + import cron.worker_bootstrap as wb + + wb._LEASE_ACQUIRED = False + wb._ACTIVATED = False + try: + monkeypatch.setattr(wb, "_root", tmp_path) + # _activate_generation returns None -> no committed venv. + monkeypatch.setattr(wb, "_activate_generation", lambda: None) + + # Stub the real lease/activate to *fail* if called (they should not be). + def _boom_lease(environment): + raise AssertionError("lease_generation called with no committed venv") + import hermes_cli.runtime_state as _rs + monkeypatch.setattr(_rs, "lease_generation", _boom_lease) + + def _boom_activate(project_root): + raise AssertionError("activate_dependencies called with no committed venv") + import pm.environments as _env + monkeypatch.setattr(_env, "activate_dependencies", _boom_activate) + + old_argv = list(sys.argv) + sys.argv = old_argv + ["--external-worker-file", "x.json", "--ack-file", "a.ready"] + try: + wb.worker_bootstrap() # must not raise + finally: + sys.argv = old_argv + + assert wb._LEASE_ACQUIRED is False + assert wb._ACTIVATED is False + finally: + wb._LEASE_ACQUIRED = False + wb._ACTIVATED = False + + +def test_cron_scheduler_module_hooks_worker_bootstrap(tmp_path, monkeypatch): + """The scheduler module-level hook (``from cron.worker_bootstrap import + worker_bootstrap; worker_bootstrap()``) must run *before* the first Hermes + package import. Assert that importing ``cron.scheduler`` with the worker + flag set does not raise (the bootstrap is idempotent and degrades + gracefully when pm/environments is not yet reachable on a dev checkout).""" + import importlib + import sys + + # Remove any cached module so the import re-runs top-level code. + for mod in list(sys.modules): + if mod.startswith("cron.scheduler") or mod == "cron.worker_bootstrap": + del sys.modules[mod] + + old_argv = list(sys.argv) + sys.argv = old_argv + ["--external-worker-file", "x.json", "--ack-file", "a.ready"] + try: + # This import must succeed: worker_bootstrap degrades to a warning when + # pm.environments / hermes_cli.runtime_state are not importable in the + # dev checkout, and when _activate_generation returns None. + mod = importlib.import_module("cron.scheduler") + assert mod is not None + finally: + sys.argv = old_argv + + + def test_shared_run_path_hands_gateway_fire_to_external_worker(monkeypatch): import cron.scheduler as scheduler