fix(cron): hold the committed dependency generation's lease at worker entry (#122290 P2)
The pin in scheduler_worker_env.py restores the site-packages *directory* into the child's PYTHONPATH, but a directory name 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, so between the gateway's exit and the worker's next import the very generation the worker needs can be collected away. New cron/worker_bootstrap.py closes that gap at the worker entry, before the first application/dependency import: - lease_dependencies(): acquire the committed generation's kernel lease (the same hermes_cli.runtime_state.lease_generation primitive activate_dependencies uses), so the collector's 'no process holds the lease' safety condition keeps the generation alive for the worker's whole lifetime. Unconditional and idempotent. - activate_worker_dependencies(): delegate to pm.environments.activate_dependencies (the same resolution + site.addsitedir + sys.path reordering the gateway's boot uses) so the worker's own imports resolve without waiting for the gateway's handoff. Gated on --external-worker-file in argv, so the in-process gateway path keeps its exact launch contract and the runner-owns-its-own-deps case stays a no-op. Hooked into the top of cron/scheduler.py, after the stdlib-only sys.path pin and before any Hermes package import, so hermes_yaml -> ruamel.yaml succeeds. Both degrade to a logged no-op when no dependency generation is committed. Tests: test_worker_bootstrap_lease_and_activation, _leases_but_skips_activation_in_gateway, _no_committed_venv_is_noop, and a scheduler-import smoke test. The two home_io_guard failures in this suite are pre-existing on a clean checkout and unrelated. Fixes the P2 lifetime finding in @andrexibiza's review of #122290. (cherry picked from commit 429f0dd191dbf719d05cbbbe3cda6546893b97d0)
This commit is contained in:
123456hyu
kshitij
@@ -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
|
||||
|
||||
151
cron/worker_bootstrap.py
Normal file
151
cron/worker_bootstrap.py
Normal file
@@ -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
|
||||
@@ -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/<gen>/venv -> pyvenv.cfg
|
||||
# tmp_path/install/environments/<gen>/.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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user