Skip to content

fix(cron): tree-kill script timeout descendants via agent.deadline.kill_process_tree (#85125 4a, salvage of #86791) - #93795

Merged
kshitijk4poor merged 1 commit into
NousResearch:mainfrom
kshitijk4poor:salvage/86791-cron-tree-kill
Aug 27, 2026
Merged

kshitijk4poor merged 1 commit into
NousResearch:mainfrom
kshitijk4poor:salvage/86791-cron-tree-kill

Conversation

@kshitijk4poor

Copy link
Copy Markdown
Contributor

#85125 Phase 4a. A cron script timeout used a site-local process-group kill that cannot reach a grandchild running in its OWN session (start_new_session background jobs, watchdogs) — descendants kept running after the job reported failure (#71148, #59549). The timeout path now goes through the unified deadline layer's agent.deadline.kill_process_tree (#85147): psutil snapshots the descendant set BEFORE signalling, so own-session grandchildren are reached too.

Based on #86791 by @ayushnangia — cherry-picked to preserve authorship (with his Co-authored-by credits to @dante32683 (#59379) and @supotato-ipj (#59549 evidence) kept). Salvage round folds four review findings into the same commit:

  • Sibling kill site migrated too (whole-bug-class rule): the cancel_event / "cron fire ownership was lost" path killed the script with the same killpg-only helper and orphaned setsid grandchildren identically. Both kill sites now tree-kill; pinned by test_cancel_path_also_tree_kills.
  • proc.poll() early-return in the new helper, mirroring _terminate_cron_script_process: a script that exits right at the deadline no longer produces a spurious "reported no signal" WARNING; pinned by test_already_exited_proc_is_left_alone.
  • Import split out of the kill try: a packaging/import problem with agent.deadline now logs as what it is instead of masquerading as a kill failure (import stays function-local so tests can monkeypatch).
  • Acceptance-test hardening: script timeout 1s → 2s (interpreter startup under CI load could eat the whole window before the spawner wrote its pid file); the cancel-path test's stub really kills so the drain doesn't stall 5s.

Behavior note: kill_process_tree hard-kills (SIGKILL) immediately, whereas the old timeout path gave a 1s SIGTERM grace window. For a deadline-expiry hard stop this is intended — both docstrings say "hard stop" — but it is a semantics change worth stating.

Verification

  • Red on main, green here: the acceptance test (spawner whose grandchild holds its own session; zero survivors asserted through the real cron path) FAILS on upstream/main with grandchild pid 62499 survived the script timeout and passes with the fix
  • tests/cron/: 900 passed, 5 pre-existing environmental failures (test_monitor_kind.py config-drift, unrelated); new class 3x flake-free; no leaked processes after runs
  • live_system_guard_bypass marker verified registered in tests/conftest.py; psutil verified a pinned core dependency
  • ruff clean; ty diff-clean (3 stub-typing errors fixed via cast)

Closes #86791. Fixes #71148. Fixes #59549.
Part of #85125 (Phase 4a — delegated item, implementation + acceptance cell by @ayushnangia).

…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.

Salvage additions on review (NousResearch#85125 Phase 4a):
- migrate the sibling kill site too — the cancel_event/"ownership was
  lost" path orphaned setsid grandchildren the same way (whole-bug-class
  rule); pinned by test_cancel_path_also_tree_kills
- proc.poll() early-return in _terminate_cron_script_tree so a script
  that exits right at the deadline doesn't log a spurious "no signal"
  warning (mirrors _terminate_cron_script_process); pinned by
  test_already_exited_proc_is_left_alone
- acceptance test's script timeout 1s -> 2s: interpreter startup under
  CI load could eat the whole 1s window before the spawner wrote its
  pid file
- note: kill_process_tree hard-kills (SIGKILL) immediately, whereas the
  old path gave a 1s SIGTERM grace window; intended for a deadline-
  expiry hard stop (both docstrings say "hard stop")

Based on NousResearch#86791 by @ayushnangia; cherry-picked to preserve authorship.

Co-authored-by: dante32683 <dante32683@users.noreply.github.com>
Co-authored-by: supotato-ipj <supotato-ipj@users.noreply.github.com>
@kshitijk4poor
kshitijk4poor enabled auto-merge (rebase) August 24, 2026 10:33
@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation labels Aug 24, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

Overall: fixes a real bug class (own-session grandchildren surviving cron script timeouts) by routing through agent.deadline.kill_process_tree, with layered fallbacks and a genuine end-to-end test using a start_new_session grandchild. Well-scoped. Observations:

  1. cron/scheduler.py:3846 — The race between proc.poll() is not None (early exit) and kill_process_tree(pid) is handled, but the window between "not exited" and the psutil snapshot inside kill_process_tree can still hit a pid that the OS just reaped. kill_process_tree is documented best-effort, so returning False triggers the fallback _terminate_cron_script_process on an already-dead process — which also handles it. Fine, but a one-line comment noting "double-kill of a reaped pid is harmless on both paths" would save the next reader a trace through both helpers.

  2. cron/scheduler.py:4120 — The cancel path and timeout path now both call _terminate_cron_script_tree, but the kill site at ownership loss (cancel_event.is_set()) runs _drain_script_pipes immediately after. If kill_process_tree has signalled but the OS hasn't reaped the grandchild yet, drain returns as soon as the direct child dies, potentially reporting back to the scheduler while the setsid grandchild is mid-teardown. Likely fine in practice (SIGKILL is fast), but if any caller treats "returned from _run_job_script" as "no descendants remain," document that it's eventually-consistent.

  3. cron/scheduler.py:3850 — The pid-validation branch catches pid <= 0, but subprocess.Popen.pid is always positive after successful spawn; this branch is realistically reachable only from tests passing SimpleNamespace. That is OK for defensive depth, but the warning "received invalid pid" will read strangely in production logs if it ever fires from a real Popen. Consider making the message name the source ("tree-kill called with mock/foreign proc object").

  4. tests/cron/test_cron_script.py:710 — test_timeout_leaves_no_setsid_grandchild spawns a real Python grandchild with sleep(30) and relies on the 2 s timeout kill. On Windows CI (no setsid, no killpg), this test's premise doesn't hold — start_new_session maps to CREATE_NEW_PROCESS_GROUP, and kill_process_tree uses taskkill /T, which does handle it, but the test specifically names "own-session" semantics. Confirm the test is expected to run on Windows; if it's POSIX-only in intent, add pytest.mark.skipif(sys.platform == "win32") rather than relying on behavior differences.

  5. The two contributors/emails/* files appear unrelated to the fix — presumably an artifact of the CLA assistant flow on the branch. Worth dropping from the PR to keep the diff purely about the tree-kill change.

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:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation type/bug Something isn't working

Projects

None yet

4 participants