Repository navigation
fix(cron): read the committed generation for Windows cron scripts #123668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
dd2b718
48686f8
43751c7
06a11f1
b86dffa
b3760ef
30c649f
ef7aaa9
f298859
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,171 @@ | ||||||||||||
| """Windows cron scripts must run on the ABI they were built for. | ||||||||||||
|
|
||||||||||||
| ``cron/scheduler_script.py::_windows_cron_python_invocation`` answers "which interpreter and | ||||||||||||
| which site-packages?" for a cron script child. On a PM-managed Windows install it read the | ||||||||||||
| dependency tree through ``pm.environments.selected_venv``, which answers with the leftover | ||||||||||||
| pre-PM ``<root>/venv`` when no generation is recorded (it falls back to ``base_venv``). That | ||||||||||||
| tree was built for whichever interpreter created it, so overlaying it on the managed store | ||||||||||||
| Python 3.14 loads a cp311 ``pydantic_core`` and every script dies with | ||||||||||||
| ``ModuleNotFoundError: No module named 'pydantic_core._pydantic_core'`` — the same class as | ||||||||||||
| the gateway crash in #122183 / #123650, in the cron sibling no PR covers. | ||||||||||||
|
|
||||||||||||
| ``committed_venv`` is the resolver that never answers with the in-tree venv. With none | ||||||||||||
| committed there is no tree this install ever provisioned, so the call keeps the child's own | ||||||||||||
| interpreter instead of borrowing the stale one. | ||||||||||||
| """ | ||||||||||||
|
|
||||||||||||
| import os | ||||||||||||
| import sys | ||||||||||||
| from pathlib import Path | ||||||||||||
|
|
||||||||||||
| import pytest | ||||||||||||
|
|
||||||||||||
| pytestmark = pytest.mark.skipif( | ||||||||||||
| sys.platform != "win32", | ||||||||||||
| reason="_windows_cron_python_invocation is the Windows cron spawn path (#122183/#123650)", | ||||||||||||
| ) | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nonblocking] This file runs on no CI lane. Risk/repro:
Suggested change
|
||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def _write_venv(venv: Path, base: Path, *, version: str | None = None, uv: bool = True) -> Path: | ||||||||||||
| """A fake Windows venv with the ``Scripts/python.exe`` shim a cron child is handed.""" | ||||||||||||
| site = venv / "Lib" / "site-packages" | ||||||||||||
| site.mkdir(parents=True, exist_ok=True) | ||||||||||||
| (venv / "Scripts").mkdir(parents=True, exist_ok=True) | ||||||||||||
| base.mkdir(parents=True, exist_ok=True) | ||||||||||||
| shim = venv / "Scripts" / "python.exe" | ||||||||||||
| shim.write_text("", encoding="utf-8") | ||||||||||||
| (base / "python.exe").write_text("", encoding="utf-8") | ||||||||||||
| lines = [f"home = {base}"] | ||||||||||||
| if uv: | ||||||||||||
| lines.append("uv = 0.11.14") | ||||||||||||
| if version: | ||||||||||||
| lines.append(f"version_info = {version}") | ||||||||||||
| (venv / "pyvenv.cfg").write_text("\n".join(lines) + "\n", encoding="utf-8") | ||||||||||||
| return venv | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def _site_packages_in(env_overlay: dict) -> Path | None: | ||||||||||||
| for entry in env_overlay.get("PYTHONPATH", "").split(os.pathsep): | ||||||||||||
| if Path(entry).name == "site-packages": | ||||||||||||
| return Path(entry) | ||||||||||||
| return None | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def test_managed_install_never_overlays_the_stale_in_tree_venv(tmp_path, monkeypatch): | ||||||||||||
| """A managed install with no committed generation keeps the child's own interpreter. | ||||||||||||
|
|
||||||||||||
| ``selected_venv`` answers the leftover pre-PM ``<root>/venv`` here, which is built for a | ||||||||||||
| different interpreter: its compiled extensions cannot load on the store Python. | ||||||||||||
| """ | ||||||||||||
| from cron import scheduler_script as sched_script | ||||||||||||
|
|
||||||||||||
| repo = tmp_path / "repo" | ||||||||||||
| stale = _write_venv(repo / "venv", tmp_path / "base") | ||||||||||||
| store = tmp_path / "store" / "python.exe" | ||||||||||||
| store.parent.mkdir(parents=True) | ||||||||||||
| store.write_text("", encoding="utf-8") | ||||||||||||
| child = _write_venv(tmp_path / "child", tmp_path / "childbase").parent | ||||||||||||
|
|
||||||||||||
| monkeypatch.setattr(sched_script, "_read_windows_pyvenv_cfg", lambda _dir: {}) | ||||||||||||
| monkeypatch.setattr( | ||||||||||||
| "hermes_cli._launchers.resolve_store_python", lambda _root: store | ||||||||||||
| ) | ||||||||||||
| monkeypatch.setattr("pm.environments.committed_venv", lambda _root: None) | ||||||||||||
| monkeypatch.setattr("pm.environments.selected_venv", lambda _root: stale) | ||||||||||||
|
|
||||||||||||
| interpreter, env_overlay = sched_script._windows_cron_python_invocation( | ||||||||||||
| str(child / "Scripts" / "python.exe") | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| overlay = _site_packages_in(env_overlay) | ||||||||||||
| assert overlay is None or not overlay.is_relative_to(stale), ( | ||||||||||||
| "a stale in-tree venv must never be overlaid on the managed store Python" | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def test_managed_install_uses_the_committed_generation(tmp_path, monkeypatch): | ||||||||||||
| """With a generation committed, the child's site-packages are that generation's.""" | ||||||||||||
| from cron import scheduler_script as sched_script | ||||||||||||
|
|
||||||||||||
| repo = tmp_path / "repo" | ||||||||||||
| stale = _write_venv(repo / "venv", tmp_path / "base") | ||||||||||||
| committed = _write_venv( | ||||||||||||
| tmp_path / "installs" / "gen" / "venv", tmp_path / "genbase", | ||||||||||||
| version=f"{sys.version_info[0]}.{sys.version_info[1]}.7", | ||||||||||||
| ) | ||||||||||||
| store = tmp_path / "store" / "python.exe" | ||||||||||||
| store.parent.mkdir(parents=True) | ||||||||||||
| store.write_text("", encoding="utf-8") | ||||||||||||
| child = _write_venv(tmp_path / "child", tmp_path / "childbase") | ||||||||||||
|
|
||||||||||||
| monkeypatch.setattr(sched_script, "_read_windows_pyvenv_cfg", lambda _dir: {}) | ||||||||||||
| monkeypatch.setattr("hermes_cli._launchers.resolve_store_python", lambda _root: store) | ||||||||||||
| monkeypatch.setattr("pm.environments.committed_venv", lambda _root: committed) | ||||||||||||
| monkeypatch.setattr("pm.environments.selected_venv", lambda _root: stale) | ||||||||||||
|
|
||||||||||||
| _, env_overlay = sched_script._windows_cron_python_invocation( | ||||||||||||
| str(child / "Scripts" / "python.exe") | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| overlay = _site_packages_in(env_overlay) | ||||||||||||
| assert overlay is not None | ||||||||||||
| assert overlay == committed / "Lib" / "site-packages" | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def test_corrupt_record_degrades_instead_of_aborting_the_spawn(tmp_path, monkeypatch): | ||||||||||||
| """A record that raises must not take every job down with it. | ||||||||||||
|
|
||||||||||||
| ``committed_venv`` raises (not returns None) on an unparseable or out-of-tree | ||||||||||||
| ``facts.json``, and ``_script_argv`` calls it unguarded — so the raise would turn | ||||||||||||
| every cron script into a spawn-time traceback. Degrade to the repo-only overlay. | ||||||||||||
| """ | ||||||||||||
| from cron import scheduler_script as sched_script | ||||||||||||
|
|
||||||||||||
| repo = tmp_path / "repo" | ||||||||||||
| stale = _write_venv(repo / "venv", tmp_path / "base") | ||||||||||||
| store = tmp_path / "store" / "python.exe" | ||||||||||||
| store.parent.mkdir(parents=True) | ||||||||||||
| store.write_text("", encoding="utf-8") | ||||||||||||
| child = _write_venv(tmp_path / "child", tmp_path / "childbase") | ||||||||||||
|
|
||||||||||||
| monkeypatch.setattr(sched_script, "_read_windows_pyvenv_cfg", lambda _dir: {}) | ||||||||||||
| monkeypatch.setattr("hermes_cli._launchers.resolve_store_python", lambda _root: store) | ||||||||||||
| monkeypatch.setattr("pm.environments.selected_venv", lambda _root: stale) | ||||||||||||
|
|
||||||||||||
| def _boom(_root): | ||||||||||||
| raise RuntimeError("cannot read dependency environment: facts.json") | ||||||||||||
|
|
||||||||||||
| monkeypatch.setattr("pm.environments.committed_venv", _boom) | ||||||||||||
|
|
||||||||||||
| interpreter, env_overlay = sched_script._windows_cron_python_invocation( | ||||||||||||
| str(child / "Scripts" / "python.exe") | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| # Spawn survives; the store interpreter is still the one handed to the child, and | ||||||||||||
| # the overlay carries the REAL checkout root (the resolver's own ``repo``), not the | ||||||||||||
| # tmp_path stand-in this test built. | ||||||||||||
| assert interpreter == str(store) | ||||||||||||
| overlay = _site_packages_in(env_overlay) | ||||||||||||
| assert overlay is None or not overlay.is_relative_to(stale) | ||||||||||||
| repo_root = str(Path(sched_script.__file__).resolve().parents[1]) | ||||||||||||
| assert repo_root in env_overlay["PYTHONPATH"] | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def test_non_managed_install_keeps_its_own_venv_overlay(tmp_path, monkeypatch): | ||||||||||||
| """With no store Python the uv base-interpreter overlay is unchanged: the legacy | ||||||||||||
| installs that depend on it must keep working.""" | ||||||||||||
| from cron import scheduler_script as sched_script | ||||||||||||
|
|
||||||||||||
| venv = _write_venv(tmp_path / "venv", tmp_path / "base") | ||||||||||||
| child = _write_venv(venv, tmp_path / "base2") | ||||||||||||
|
|
||||||||||||
| monkeypatch.setattr("hermes_cli._launchers.resolve_store_python", lambda _root: None) | ||||||||||||
|
|
||||||||||||
| interpreter, env_overlay = sched_script._windows_cron_python_invocation( | ||||||||||||
| str(child / "Scripts" / "python.exe") | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| assert interpreter == str(tmp_path / "base2" / "python.exe") | ||||||||||||
| overlay = _site_packages_in(env_overlay) | ||||||||||||
| assert overlay == venv / "Lib" / "site-packages" | ||||||||||||
| assert env_overlay["VIRTUAL_ENV"] == str(venv) | ||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[blocking] The no-generation arm discards the handed interpreter and gives the child a bare store Python.
Risk/repro: the probe imports the real module under a temp
HERMES_HOME, socommitted_venvreturnsNone. It stubs onlyresolve_store_pythonand setssys.platform = "win32"in the probe process only. The handed interpreter is a uv venv shim,<tmp>/ownvenv/Scripts/python.exe. On this head the function returnsinterpreter=<tmp>/store/python.exe,PYTHONPATH=<repo>._script_argvthen logsvenv site-packages Lib/site-packages not found; running without .pth processingon every spawn, because the repo-only overlay has nosite-packagesentry for_windows_cron_bootstrap_argvto find. When nothing is committed,_require_own_dependencies(pm/environments.py:280, called fromhermes_bootstrap.py:554) refuses a bare store Python at boot. So the process running cron here is a non-store interpreter (for a uv install, a venv that carries its own packages). The child gets none of them, and pure-Python packages that loaded on main now fail.Suggested fix: return from the managed branch only when a generation is committed, and otherwise fall through to the uv overlay below. The comment's "child keeps the interpreter it was handed" then becomes true: the same probe returns
<tmp>/uvbase/python.exewith<tmp>/ownvenv/Lib/site-packages, and there is no warning.