Skip to content

fix(cron): preserve venv worker interpreter - #112731

Closed
KoNit-K wants to merge 1 commit into
NousResearch:mainfrom
KoNit-K:fix/restart-safe-cron-venv
Closed

KoNit-K wants to merge 1 commit into
NousResearch:mainfrom
KoNit-K:fix/restart-safe-cron-venv

Conversation

@KoNit-K

@KoNit-K KoNit-K commented Sep 16, 2026

Copy link
Copy Markdown

What does this PR do?

Restart-safe cron workers now retain the virtualenv's lexical bin/python3 or
bin/python path when Hermes runs inside a virtualenv. Previously the worker
used symlink-resolved sys.executable, which could launch the base runtime
without Hermes packages and exit before acknowledging ownership. When no active
virtualenv interpreter is available, dispatch still uses sys.executable.

Related Issue

Fixes #112729

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Changes Made

  • cron/scheduler.py — select an executable venv bin/python3 or bin/python without resolving its symlink before constructing the external-worker command; retain the existing fallback.
  • tests/cron/test_restart_safe_worker.py — cover dispatch through a symlinked venv interpreter and the non-venv fallback.

How to Test

  • HERMES_PYTHON=/Users/blockkonit./Dev/hermes/hermes-agent/.venv/bin/python scripts/run_tests.sh tests/cron/test_restart_safe_worker.py -q — 20 passed, 5 Linux-only tests skipped on macOS.
  • HERMES_PYTHON=/Users/blockkonit./Dev/hermes/hermes-agent/.venv/bin/python ruff check cron/scheduler.py tests/cron/test_restart_safe_worker.py — all checks passed.
  • CONTROL: the test suite verifies dispatch falls back to sys.executable when no virtualenv is active.

Evidence

  • BEFORE RED: the focused pre-fix run failed with AttributeError: module 'cron.scheduler' has no attribute '_external_cron_worker_python' (1 failed, 1 Linux-only test skipped), demonstrating the missing venv-selection behavior.
  • AFTER GREEN: the focused cron worker suite passed 20 tests; the normal fallback assertion remains covered.
  • CONTROL: the new selector only uses VIRTUAL_ENV or a sys.prefix distinct from sys.base_prefix, and otherwise keeps the existing executable.

Review follow-up

The symlinked systemd-worker regression is marked Linux-only and was skipped on this macOS host; it will run in the Linux CI lane. The classified Docker and Nix CI lanes were not run locally.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run relevant tests locally (see How to Test)
  • I've added tests for my changes
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • Documentation update: N/A
  • cli-config.yaml.example: N/A
  • CONTRIBUTING.md or AGENTS.md: N/A
  • Cross-platform impact considered
  • Tool descriptions/schemas: N/A

@whyyagswhy

Copy link
Copy Markdown

Independent verification on the PR head (4dbe839): tests/cron/test_restart_safe_worker.py passes locally, 20 passed, 5 skipped (canonical runner, macOS). The new fallback test passes; the new venv-lexical test is linux_only so it skipped here, but I exercised _external_cron_worker_python directly with a symlinked fake venv and it returns the lexical venv path as claimed.

Premise holds on current main: _launch_external_cron_worker still spawns bare sys.executable (cron/scheduler.py:3105), so a symlinked interpreter loses the venv. The fix prefers VIRTUAL_ENV, then sys.prefix when it differs from base_prefix, checks the candidate is an executable file, and falls back to sys.executable. No shell, no injection surface.

One note: the probe only looks under bin/, so on Windows it always falls through to sys.executable. That is a safe no-op rather than a bug, but it makes the fix POSIX-only. Either probe Scripts/python.exe behind a win32 check or say so in the docstring.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists labels Sep 17, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Related: #113219 surfaces the pre-ack worker stderr but does not change the interpreter selection, so the two are complementary for #112729.

@KoNit-K
KoNit-K force-pushed the fix/restart-safe-cron-venv branch from 4dbe839 to 7bc95aa Compare September 17, 2026 13:55

KoNit-K commented Sep 20, 2026

Copy link
Copy Markdown
Author

Closing because the proposed sys.executable symlink mechanism was disproved: the spawned worker already reports the virtualenv interpreter and imports correctly in the normal launch environment, so selecting $VIRTUAL_ENV/bin/python is generally a no-op and can be wrong under systemd. Merged PR #113802 fixes the remaining real import-path atom by pinning the gateway's Hermes checkout into the restart-safe worker environment.

@KoNit-K KoNit-K closed this Sep 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Restart-safe external cron worker fails on dispatch due to symlink resolution in sys.executable

3 participants