fix(cron): kill process group on script timeout, fix mislabeled failure message (#59549) - #59574
fix(cron): kill process group on script timeout, fix mislabeled failure message (#59549)#59574webtecnica wants to merge 1 commit into
Conversation
…re message (NousResearch#59549) Two bugs: 1. **Mislabeled timeout.** The error-message classifier in ``_format_cron_failure`` matches "timed out" generically and reports every timeout as "provider timeout". Script timeouts produce "Script timed out after Ns: /path" which also contains "timed out", so they are mislabeled. Add a specific "script timed out" check before the generic provider-timeout branch. 2. **Orphaned child processes.** ``subprocess.run`` with ``timeout=`` only kills the direct child on timeout; grandchildren survive as orphans. Migrate to ``subprocess.Popen`` + ``preexec_fn=os.setsid`` (POSIX) to create a fresh process group, and on TimeoutExpired kill the entire group with ``os.killpg(pid, 9)``.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused cron fix. The two reported defects are present on current main: cron/scheduler.py:75-79 classifies the script runner's Script timed out ... result (cron/scheduler.py:2133-2134) as a provider timeout, and the runner uses subprocess.run(..., timeout=...) at cron/scheduler.py:2101-2109.
Problems
- In the PR timeout handler,
cron/scheduler.py:2048limits cleanup tosys.platform != "win32". The Windows branch returns afterproc.communicate(timeout=...)without terminating or reapingproc, so this migration drops direct-child timeout cleanup on a supported platform. Windows 10/11 is Tier 1 inwebsite/docs/getting-started/platform-support.md:20. - The PR adds no regression tests. Current
tests/cron/test_cron_script.py:184-196only checks that a timeout is reported; it does not cover descendant cleanup or the delivery classification.
Suggested changes
- Add a Windows direct-child terminate/kill-and-reap fallback while retaining the POSIX process-group cleanup.
- Add focused cleanup and classifier regression tests.
Automated hermes-sweeper review.
|
|
||
| except subprocess.TimeoutExpired: | ||
| # Kill the entire process group to clean up orphaned children (#59549) | ||
| if sys.platform != "win32": |
There was a problem hiding this comment.
This branch skips every termination action on Windows and immediately returns the timeout result. Please add a Windows direct-child terminate/kill-and-reap fallback here; otherwise the Popen migration leaves the timed-out script process unmanaged on that platform.
|
Closing as superseded — both bugs this PR targets are now fixed on main: (1) the mislabeled "provider timeout" for script failures was fixed by #85536 (commit d1fc204) — classification is gated on job mode ( |
Summary
Two bugs in
cron/scheduler.py's script execution path:Bug 1: Mislabeled timeout messages
The error classifier in
_format_cron_failurematches "timed out" generically before checking the specific source. Script timeouts (which produce "Script timed out after Ns: /path") contain "timed out" and are reported as provider timeout instead of script timeout. Fix: check for "script timed out" before the generic branch.Bug 2: Orphaned child processes on timeout
subprocess.run(timeout=N)only kills the direct child when the timeout fires; grandchildren spawned by the script survive as orphaned processes. Migrated tosubprocess.Popen+preexec_fn=os.setsid(POSIX) to create a fresh process group, and onTimeoutExpiredthe entire group is killed viaos.killpg(pid, 9).Changes
cron/scheduler.pyTests
tests/cron/test_cron_script.py: 38/38 passed ✅Closes #59549