fix(cron): contain timed-out cron workers (#18004) - #39782
Conversation
2bea909 to
0cb2f03
Compare
|
I compared the competing #18011 branch against this PR and pulled in the part it handled better: a cron timeout must not allow the same job to overlap a still-running worker when This branch is now a hybrid of the original PR and that non-overlap behavior:
I also fixed two edge cases from review:
Validation on Windows:
|
0cb2f03 to
0f9360f
Compare
625dfeb to
1918447
Compare
|
Thanks for pushing this forward. I tested this branch against the #18004 shape with a non-cooperative agent, and it looks like the PR fixes the important same-job overlap/state problem: after timeout it interrupts, records the still-running future, and later runs of the same job are skipped while that worker remains active. One residual behavior seems worth calling out so maintainers can decide whether it belongs here or in a follow-up: for non-workdir jobs, the worker thread can still be alive after cron has returned the timeout failure. In my synthetic probe, For workdir/profile jobs, this branch takes the opposite safe tradeoff and waits for the worker to finish before restoring process-global context, which avoids context corruption but can block indefinitely if the worker never exits. That may be the right pragmatic scope for #18004. If maintainers want “timeout means no further side effects are possible” as the stronger contract, I’m happy to open a focused follow-up issue/PR around process isolation or tool/subprocess-level cancellation rather than expanding this PR. |
1918447 to
4be0460
Compare
|
Thanks for testing the non-cooperative case. I agree with your read: this PR gives cron containment and same-job non-overlap, not a hard cancellation boundary for arbitrary worker side effects. I rebased the branch on current
I also dropped the old Docker workflow workaround while rebasing because current |
4be0460 to
74f7b33
Compare
|
Rebased this branch onto current The diff is still scoped to Validation rerun on the rebased branch:
|
|
Thanks for addressing a verified cron timeout-containment gap. Current The PR's timed-out-future registry, cooperative interrupt grace, and workdir/profile wait directly cover those paths. The added tests cover cooperative interruption, non-cooperative same-job containment, no-interrupt behavior, and workdir/profile context handling. GitHub reports the rebased PR as mergeable clean. Automated hermes-sweeper review. |
74f7b33 to
11e0ef0
Compare
|
Rebuilt this against current The same local admission path now covers ticks, external fires, manual runs, and direct calls. Matching external fire claims stay live for long-running agent and script work, and gateway shutdown leaves a timed-out worker's claims alone until it actually stops. This remains containment rather than hard cancellation: an ignored interrupt keeps that one attempt live and blocks another run of the same job. |
Related: #18011 is the open alternate timeout-semantics repair, while this current head now retains admission, attempt state, claims, and process-global context until the worker exits. The remaining maintainer decision is whether containment is sufficient or a stronger hard-cancellation boundary is required. |
Summary
Cron inactivity could mark an attempt failed while its nested agent worker still executed. That allowed output, job state, durable execution history, local admission, and process-global cwd cleanup to describe a completed attempt before the worker had stopped.
The timeout path now requests cooperative interruption and retains the active attempt until the submitted worker exits. The existing terminal lifecycle then records the saved timeout once. Built-in ticks, external fires, manual runs, and direct callers share one local admission lease, while matching external fire claims remain live for long-running agent and script work.
Changes
cron/scheduler.py: joins timed-out workers before terminal handling, preserves the active lease through cleanup, protects shutdown behavior, and snapshotsTERMINAL_CWDunder its lock.cron/jobs.py: adds matching-incarnation fire-claim refresh for live externally fired work.cron/scheduler_provider.pyandtools/cronjob_tools.py: acquire local admission before durable fire claims.Validation
Test plan
python -m py_compile cron/scheduler.py cron/jobs.py cron/scheduler_provider.py tools/cronjob_tools.pypython -m pytest tests/cron/test_claim_job_for_fire.py tests/cron/test_cron_inactivity_timeout.py tests/cron/test_execution_ledger.py tests/cron/test_run_one_job.py tests/cron/test_scheduler.py tests/cron/test_scheduler_provider.py tests/cron/test_script_claim_heartbeat.py tests/cron/test_shutdown_interrupt.py tests/cron/test_terminal_cwd_lock.py tests/tools/test_cronjob_run_immediate.py -v --timeout=0Not in scope
Python cannot forcibly terminate an arbitrary running thread or external side effect. A supervised process boundary would be required for immediate hard cancellation.
Upstream
Closes #18004.