Skip to content

fix(cron): run no_agent scripts in workdir without process-global chdir - #40964

Open
xy200303 wants to merge 1 commit into
NousResearch:mainfrom
xy200303:fix/cron-profile-home-isolation
Open

fix(cron): run no_agent scripts in workdir without process-global chdir#40964
xy200303 wants to merge 1 commit into
NousResearch:mainfrom
xy200303:fix/cron-profile-home-isolation

Conversation

@xy200303

@xy200303 xy200303 commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Re-scoped per the review on the previous revision: per-job cron profile support was deliberately removed in revert(cron): remove per-job profile support (#28124) #43956, so the original profile-home race (cron scheduler: profile-job context bleeds into concurrent non-profile job (script not found) #39886) no longer exists on main_hermes_home is read-only now and cron runs per-profile inside a profile-scoped gateway.
  • This revision fixes the remaining process-global bleed of the same bug class that does exist on current main: the no_agent + workdir path in run_job applied the workdir via a process-global os.chdir() outside _terminal_cwd_lock. A sequential-pool workdir job can overlap parallel-pool jobs, so the chdir bled the workdir into concurrently running workdir-less jobs for the whole script run.
  • Additionally, _run_job_script hardcoded subprocess.run(cwd=<script dir>), so the chdir never even reached the script — workdir silently had no effect for no_agent jobs despite the comment claiming it set the subprocess cwd.

Fix

  • _run_job_script gains a cwd parameter, passed to subprocess.run(); when omitted the script runs in its own directory, preserving prior behavior for the agent pre-run / wake-gate call sites.
  • run_job's no_agent path passes the job's workdir as the script subprocess cwd and drops os.chdir entirely — zero process-global state.
  • A workdir that vanished on disk now logs a warning and falls back to the script directory, matching the agent path's behavior.

Tests

  • New TestNoAgentWorkdir in tests/cron/test_cron_workdir.py (4 tests):
    • workdir reaches the script subprocess as its cwd;
    • process cwd is not mutated while a workdir job's script runs concurrently (reproduces the bleed — fails on the old code);
    • missing workdir warns and falls back to the script dir;
    • no-workdir default is unchanged.
  • Verified the new tests fail against the pre-fix implementation; full tests/cron/ suite passes.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the original module-global race. This patch cannot be applied to current main as a standalone correctness fix.

Problems

  • Merged commit 7d8d000b1921cb1c14aa137a41bd96203992e2c0 (revert(cron): remove per-job profile support (#28124) #43956) deliberately removed per-job cron profile support, including _job_profile_context, the scheduler profile override, job-record handling, CLI/tool inputs, and tests/cron/test_cron_profile.py.
  • Current main documents the different model in cron/jobs.py:54-63: cron storage is per-profile, and a profile-scoped gateway executes that profile's jobs under its active HERMES_HOME. Current cron/scheduler.py:543-552 has no per-job profile context.
  • Consequently, this scheduler-only change would revive behavior for legacy profile fields that current interfaces intentionally ignore; its test target no longer exists on main.

Suggested changes

  • If per-job cross-profile execution is still desired, please re-scope it against the current per-profile scheduler architecture and restore the full API/storage/validation contract with end-to-end coverage rather than cherry-picking this old slice.

Automated hermes-sweeper review.

The no_agent + workdir path in run_job applied the workdir via a
process-global os.chdir() outside _terminal_cwd_lock. That had two
problems:

- _run_job_script hardcoded subprocess.run(cwd=<script dir>), so the
  chdir never reached the script — workdir silently had no effect for
  no_agent jobs despite the comment claiming it set the subprocess cwd.
- A sequential-pool workdir job can overlap parallel-pool jobs, so the
  process-wide chdir bled the workdir into concurrently running
  workdir-less jobs for the whole script run — the same bug class as
  the TERMINAL_CWD override that _terminal_cwd_lock guards.

Pass the workdir through to _run_job_script(cwd=...) instead, so the
script subprocess gets the workdir as its cwd with zero process-global
state. A workdir that vanished on disk now warns and falls back to the
script directory, matching the agent path's behavior.

Regression coverage in TestNoAgentWorkdir: workdir reaches the script
subprocess, the process cwd is not mutated while a workdir job's script
runs concurrently, missing workdir warns and falls back, and the
no-workdir default is unchanged.
@xy200303
xy200303 force-pushed the fix/cron-profile-home-isolation branch from a95faab to 5975828 Compare July 14, 2026 04:51
@xy200303 xy200303 changed the title fix(cron): isolate profile home per job context fix(cron): run no_agent scripts in workdir without process-global chdir Jul 14, 2026
@xy200303

Copy link
Copy Markdown
Contributor Author

Thanks for the review — agreed that the previous revision targeted machinery that #43956 deliberately removed. I've re-scoped the PR against the current per-profile scheduler architecture instead of restoring any per-job profile contract.

While auditing cron/scheduler.py on current main, I found the remaining process-global bleed of the same bug class: the no_agent + workdir path in run_job applied the workdir via os.chdir() outside _terminal_cwd_lock. Since sequential-pool workdir jobs can overlap parallel-pool jobs, that process-wide chdir bled the workdir into concurrently running workdir-less jobs — and because _run_job_script hardcoded subprocess.run(cwd=<script dir>), the chdir never even reached the script, so workdir silently had no effect for no_agent jobs.

The revised patch passes the workdir through to _run_job_script(cwd=...) so the script subprocess gets it as its cwd with zero process-global state, and adds regression coverage (including a concurrency test that fails on the old implementation). Happy to adjust if you'd prefer a different shape.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@teknium1 teknium1 added the area/profiles Multi-profile isolation, HERMES_HOME scoping label Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants