Skip to content

fix(cron): prevent script pipe timeouts - #60490

Closed
noamsiegel wants to merge 1 commit into
NousResearch:mainfrom
noamsiegel:noam/fix-cron-script-pipe-timeouts
Closed

noamsiegel wants to merge 1 commit into
NousResearch:mainfrom
noamsiegel:noam/fix-cron-script-pipe-timeouts

Conversation

@noamsiegel

Copy link
Copy Markdown

Fixes #60485.

Summary

  • capture cron script stdout/stderr via temp files instead of pipes
  • run POSIX scripts in a process group and kill that group on timeout
  • clean leftover script children after successful parent exit
  • preserve per-job script_timeout_seconds, redaction, and error formatting

Validation

  • python3 -m py_compile cron/scheduler.py
  • regression probe: parent script exits while child keeps stdout open; _run_job_script("fd-leak-probe.sh", 5) returns immediately with parent-done and leaves no sleep 30 process

Capture script output through temp files and clean POSIX process groups so orphaned children cannot keep stdout/stderr pipes open after the parent script exits.
@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 Jul 7, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the inherited-pipe failure in cron scripts. The premise remains valid on current main: cron/scheduler.py:2101-2109 still uses subprocess.run(..., capture_output=True) for script execution.

Problems

  • Current main added _run_job_script_with_claim_heartbeat() in cron/scheduler.py:2139-2201 via cd5371876; no-agent and pre-run jobs now route through it at cron/scheduler.py:2611 and 2758. The conflicting patch needs to preserve this one-shot claim protection.
  • The new job.get("script_timeout_seconds") calls do not match a current public cron-job field. The documented timeout contract is global config/environment only (website/docs/user-guide/features/cron.md:404-414), and the heartbeat wrapper currently calls _run_job_script() without this argument (cron/scheduler.py:2162, 2193, 2196).
  • The diff adds no regression test. tests/cron/test_cron_script.py:184-196 covers a direct timeout, not the parent-exits/child-keeps-fd-open case.

Suggested changes

  • Reapply the process-group/temp-file fix inside current _run_job_script() while retaining the heartbeat wrapper.
  • Add a POSIX regression for an inherited stdout/stderr descriptor and child cleanup.
  • Either remove the per-job timeout change or wire and document it consistently.

Automated hermes-sweeper review.

Comment thread cron/scheduler.py
return False, f"Script path is not a file: {path}"

script_timeout = _get_script_timeout()
script_timeout = int(timeout_seconds) if timeout_seconds else _get_script_timeout()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current cron jobs do not expose script_timeout_seconds as a stored or schema-supported per-job field, and current main routes real script runs through _run_job_script_with_claim_heartbeat(), whose calls do not pass this argument. Please either remove this dead override or wire it through the public job API, documentation, and heartbeat wrapper.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@kshitijk4poor

Copy link
Copy Markdown
Contributor

Closing per #85125 Phase 4a: cron scripts now run in their own process group and are tree-killed on timeout (PR #93795, merged). The post-kill pipe drain is bounded by _drain_script_pipes (5s communicate → kill → 5s wait → abandon). @noamsiegel's temp-file capture approach was an alternative path; the functional requirements (bounded drain, group kill, child cleanup) are met by the tree-kill + bounded-drain implementation.

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 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-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cron scripts can falsely time out when children keep stdout pipes open

4 participants