fix(cron): tree-kill script timeout descendants via agent.deadline.kill_process_tree - #86791
fix(cron): tree-kill script timeout descendants via agent.deadline.kill_process_tree#86791ayushnangia wants to merge 1 commit into
Conversation
|
CI note on the slice-11 failure: the slice log shows |
PR: fix(cron): tree-kill script timeout descendants via agent.deadline.kill_process_tree
|
fbd9cdb to
06b7498
Compare
|
Thanks for the review — point 1 is right and it's now fixed on the current head: the cell is promoted to a hard assertion (the xfail + stale docstring are gone), per its own acceptance conditions. The promotion also surfaced two real gaps which the head addresses: the cell now skips when psutil is absent (kill_process_tree's descendant snapshot depends on it — without it the path silently degraded to the group kill, which is exactly how own-session grandchildren get orphaned), and the timeout fallback now logs instead of degrading silently. The whole branch is also rebased onto current main (~500 commits behind before). Other points from the review: happy to look at the rest if any still apply after the rebase. |
ffc1b3d to
8381d37
Compare
…ll_process_tree The script-timeout path used a site-local process-group kill, which cannot reach a grandchild that created its OWN session (start_new_session background jobs, watchdogs). Such descendants kept running after the job reported failure (NousResearch#71148, NousResearch#59549). Migrate the timeout handler to the unified deadline layer's kill_process_tree (NousResearch#85147, d6a5cb9): psutil snapshots the descendant set before signalling, so own-session grandchildren are reached too. Fallback to the site-local group kill if the import ever fails, so the path cannot re-wedge. The explicit script-timeout message stays the classification anchor (NousResearch#85536's contract), keeping cron timeouts distinct from provider timeouts. Co-authored-by: dante32683 <dante32683@users.noreply.github.com> Co-authored-by: supotato-ipj <supotato-ipj@users.noreply.github.com>
8381d37 to
705a07e
Compare
|
Salvaged into #93795 — your commit cherry-picked with authorship + both Co-authored-by trailers preserved. Verified red-on-main empirically (grandchild pid survived on upstream/main, zero survivors with the fix). Review round folded in: the cancel_event/'ownership was lost' kill site was the same bug class (killpg-only, orphans setsid grandchildren) so it now tree-kills too; proc.poll() early-return kills a spurious warning when the script exits right at the deadline; the import moved out of the kill try so packaging errors log honestly; acceptance-test timeout 1s→2s for CI headroom. One semantics note documented in the PR: kill_process_tree SIGKILLs immediately vs the old 1s SIGTERM grace — intended for a hard stop. Closing in favor of #93795. Phase 4a delivered — nice work on the acceptance cell. |
Summary
Phase 4a of #85125 (delegated): a cron script timeout must leave zero living descendants. The previous timeout handler used a site-local process-group kill (
_terminate_cron_script_process), which cannot reach a grandchild that created its own session —start_new_sessionbackground jobs, watchdog chains. Those survived the reported failure and kept running (#71148, #59549 — @supotato-ipj's production trace of the 3600s-timeout mislabeling is the same class).Changes
cron/scheduler.py::_run_job_script: the timeout branch now callsagent.deadline.kill_process_tree(proc.pid)(landed with feat(agent): unified deadline layer — bounded execution primitive + timeout resolver (#85125 Phase 1) #85147) — psutil snapshots the descendant set before signalling, so own-session grandchildren are reached, with PID-recycling safety. Falls back to the site-local group kill if the import ever fails, so the path cannot re-wedge.Script timed out after Nsmessage stays the classification anchor (the fix(cron): no_agent and script failures stop masquerading as provider errors #85536 contract), keeping script timeouts distinct from provider timeouts.tests/cron/test_cron_script.py: the script spawns astart_new_sessiongrandchild (DEVNULL stdio so it can't hold the runner's pipes), the timeout fires at 1s, and the test asserts the grandchild pid is dead. Red on main before this change, green with it.Validation
scripts/run_tests.sh tests/cron/→ 684 passed, 0 failed, 1 skippedCredit
start_new_session=Truemechanism: @dante32683 (fix(cron): script timeouts leave orphaned subprocess groups and get mislabeled as provider timeouts #59379) — Co-authored-byCloses #71148 and #59549's orphan half. The Phase 4 conformance-harness copy of the acceptance cell stacks behind #83964 per the #85125 ordering condition.