Skip to content

fix(cron): run external workers and scripts on the committed environment interpreter (fixes #123400, #123440) - #123525

Closed
Sahilvishnaliya wants to merge 1 commit into
NousResearch:mainfrom
Sahilvishnaliya:fix-cron-managed-runtime-children
Closed

Sahilvishnaliya wants to merge 1 commit into
NousResearch:mainfrom
Sahilvishnaliya:fix-cron-managed-runtime-children

Conversation

@Sahilvishnaliya

Copy link
Copy Markdown

Summary

Under the PM runtime the gateway process runs the bare store Python with the committed environment's site-packages activated in-process — so children spawned with sys.executable see NEITHER tree: the subprocess sanitizer strips PYTHONPATH and pin_hermes_tree_on_pythonpath re-adds only the checkout root. Two failure classes, one root cause:

Fix

  • cron/scheduler_worker_env.py: new managed_runtime_python() — the committed dependency environment's own interpreter (POSIX; windows= data parameter, never sys.platform). Falls back to None on Windows, when no environment is committed, when the record is unreadable, or when the interpreter IS the running process (launch contract kept).
  • cron/scheduler.py::_launch_external_cron_worker: the worker command runs on the managed interpreter when one is committed.
  • cron/scheduler_script.py::_script_argv: on POSIX managed installs, .py scripts run on the managed interpreter with the repo pinned on PYTHONPATH (the worker's own [Bug]: Restart-safe external cron worker fails on dispatch due to symlink resolution in sys.executable #112729 remedy). A PYTHONPATH overlay of the dependency site was deliberately avoided — the variable is inherited by the script's children, and a helper on another interpreter would import this tree's compiled extensions first. Windows behavior is untouched (its own documented hidden-window overlay).

Tests

  • New tests/cron/test_managed_runtime_children.py (8 tests): resolution as data (committed gen resolved, Windows data → None, no environment/unreadable record/missing interpreter → None, running-interpreter guard), plus POSIX-marked argv assertions (managed interpreter + repo pin; unmanaged keeps sys.executable).
  • tests/cron/test_restart_safe_worker.py: drive-by fix — test_scoped_wrapper_exit_without_user_bus… is POSIX-only but was unmarked, so it failed on Windows under bare pytest (os.getuid); now @pytest.mark.platforms("linux"). Suite green: 52 passed, 6 skipped.

Fixes #123400, fixes #123440

@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/cron Cron scheduler and job management area/install-update Installer, updater, packaging, wheels, doctor needs-decision Awaiting maintainer decision before any implementation sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Sep 26, 2026
@kshitijk4poor

Copy link
Copy Markdown

@Sahilvishnaliya, a heads-up on scope: #124973 (armed) fixes the script half of this PR (#123440, POSIX cron .py scripts). It runs them on the selected venv's interpreter and puts the live checkout on sys.path in-process. It doesn't touch the restart-safe external worker (#123400, scheduler_worker_env.py / scheduler.py), so I'm leaving this open. After #124973 merges, rebasing onto main and dropping the scheduler_script.py changes would leave a worker-only PR.

@Enough1122

Copy link
Copy Markdown

AI code review — automated follow-up for reference; not a maintainer.

This PR runs cron children on the committed PM environment's own interpreter: managed_runtime_python() in cron/scheduler_worker_env.py, used for the restart-safe worker at cron/scheduler.py:3453 and for POSIX .py scripts at cron/scheduler_script.py:317. The fallbacks (Windows, no committed environment, missing interpreter) degrade to sys.executable, which I think is the right shape. One blocker in the new test file.

1. test_posix_script_argv_uses_managed_interpreter fails on every host (tests/cron/test_managed_runtime_children.py:108, blocker)

The test builds repo = tmp_path / "repo" (line 102) and expects overlay == {"PYTHONPATH": str(repo)}, but the code under test computes its own root: cron/scheduler_script.py:332 does repo = Path(__file__).resolve().parents[1] and line 338 returns {"PYTHONPATH": str(repo)}. The stub (lambda repo_root, **kw: stub, line 103) discards the repo_root the callee passes, so that tmp_path local can never reach the result. Reproduced against the post-image module, POSIX branch selected via a module-level platform shim:

probe repo_root received overlay
stub -> interpreter <checkout> {'PYTHONPATH': '<checkout>'}
assertion line 108 pytest-of-root*/repo mismatch

With the stub returning None the same probe gives {}, so the sibling test at line 125 passes and only the first is red. It is marked platforms("posix") (= linux/darwin, tests/_fixtures/platform_gating.py:53) and tests.yml runs ubuntu, so this is a red CI run. Cheapest fix: assert against str(Path(cron.scheduler_script.__file__).resolve().parents[1]), or capture the repo_root the callee passes.

Minor

  • tests/cron/test_restart_safe_worker.py:286 newly gets @pytest.mark.platforms("linux") with the body unchanged; it stubs restart_safe_gateway_child_argv, so nothing there needs systemd. Was it red on macOS, or scope-narrowing?

Unverified / please confirm:

  • Finding 1 is from running the code, on Windows with POSIX selected via a module-level shim rather than a real POSIX host: the table's paths are Windows values and will read differently on ubuntu, the mismatch does not.
  • No base/head pair for the test — the pre-image has no _posix_managed_python_invocation.
  • The platforms("linux") gate is from reading only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cron Cron scheduler and job management needs-decision Awaiting maintainer decision before any implementation P1 High — major feature broken, no workaround sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

5 participants