2 commits

Author	SHA1	Message	Date
joaomarcos	8a573bb6e7	fix(cron): stop interrupted jobs from delivering their pre-kill output ... Follow-up to the previous commit on #60432. The status-write guard (_consume_interrupted_flag, checked right before mark_job_run) closes the false-success bookkeeping gap, but run_one_job delivers its result BEFORE that check: delivery happens right after run_job() returns, mark_job_run happens at the very end. A job whose tool subprocess was killed mid-flight can still produce a plausible-looking final_response from the truncated output, and that response would reach the user via _deliver_result before the interrupted flag was ever consulted -- correct status in jobs.json, wrong message already sent. Adds _is_interrupted(), a non-destructive peek at the same _interrupted_job_ids set (_consume_interrupted_flag stays as the consuming, authoritative check right before the status write -- this needed a peek instead since the flag has to still be visible there). Checked right after save_job_output, before the deliver_content decision: if the run looked successful but was flagged interrupted, force success=False with an explicit interruption message. This routes delivery through the existing _summarize_cron_failure_for_delivery path (the same one a real failure already uses) instead of the raw final_response, so the user gets an honest "this run was interrupted" instead of a truncated/misleading result. Testing: 4 new tests in tests/cron/test_shutdown_interrupt.py -- _is_interrupted peek semantics (false/true/does-not-clear, as opposed to the consuming _consume_interrupted_flag), and the delivery-gate test itself, which mocks run_job to return a normal-looking success with a "plausible final response" while the job is pre-marked interrupted, and asserts _deliver_result receives the failure summary ("This run was interrupted.") instead, with the summarizer's error argument confirmed to mention the interruption. Fail-then-pass: reverted cron/scheduler.py only, the 4 new tests fail (3 on the missing _is_interrupted attribute, 1 -- the delivery-gate test -- on _summarize_cron_failure_for_delivery never being called, i.e. the raw response would have gone out); restored, all 16 tests in the file pass. Regression: tests/cron/ (683 tests) + test_cron_active_work_drain.py + test_gateway_shutdown.py + test_shutdown_cache_cleanup.py -- 11 pre-existing failures (Unix file-permission-bit and path-tilde assertions that don't apply on this Windows dev box), matching the same set already established as pre-existing in the prior commit's regression check. Zero new failures. Continues #60432	2026-07-07 22:15:04 -07:00
joaomarcos	24e9ed73c2	fix(gateway,cron): make shutdown drain visible to in-flight cron work ... Cron jobs run through cron/scheduler.py's own ThreadPoolExecutor via a standalone AIAgent (run_job/run_one_job), entirely outside GatewayRunner._running_agents -- the dict _drain_active_agents() and every other active-work check on that class reads. A gateway shutdown (/update, /restart, and SIGUSR1 all funnel through the same stop()) could log active_at_start=0 and immediately kill tool subprocesses while a cron job's terminal command was still running, with no wait and no indication anything was interrupted. Real-world impact (from the issue): a scheduled daily briefing cron job was in flight during /update, its tool subprocess got killed by the unconditional shutdown cleanup, and the job was never marked failed -- it simply never completed or delivered, with no error surfaced anywhere. A repro with a 30-minute `sleep` cron job in flight during /update reproduced the same pattern: subprocess killed at +0.22s of drain (active_at_start=0), the job's agent thread continued in-process and produced a plausible-looking final response from the truncated tool output, and the scheduler marked the run successful. Root cause is layered, not a single line: 1. GatewayRunner._drain_active_agents() only waits on _running_agents. Cron work was invisible to it, so drain returned instantly whenever the only active work was a cron job. 2. Even with visibility, the shutdown's final tool-subprocess kill (process_registry.kill_all()) is a global, unconditional sweep with no per-job targeting -- a long-running cron job that outlives the drain timeout still gets its subprocess killed. 3. cron/scheduler.py had no way to detect that a job's tool subprocess was killed out from under it mid-run; the agent thread kept going and its eventual (often degraded but plausible-looking) response got reported as a normal successful completion. Fix, three parts: - cron/scheduler.py: expose get_running_job_ids() (thread-safe snapshot of the existing _running_job_ids set, already used to prevent double-dispatch) so the gateway can read cron's in-flight state without reaching into private module internals. - gateway/run.py: GatewayRunner._active_cron_job_count() reads that snapshot. _drain_active_agents() now waits on (_running_agents OR active cron jobs), so a cron-only workload gets the same bounded wait chat sessions already get instead of an instant active_at_start=0. Shutdown drain logging gains cron_active_at_start/cron_active_now fields alongside the existing ones (unchanged, for compat). - cron/scheduler.py: mark_running_jobs_interrupted(reason), called by gateway/run.py's _kill_tool_subprocesses() right after process_registry.kill_all(), marks every job still in _running_job_ids at that instant as failed/interrupted via the existing mark_job_run() -- and records the job IDs in _interrupted_job_ids BEFORE writing, so run_one_job()'s own eventual completion for the same run (racing in its own thread) checks that flag and skips its normal write instead of clobbering the interrupted status with a false "ok" produced from the now-truncated tool output. This does not attempt to correlate a killed PID to a specific job ID (process_registry tracks PIDs, not job IDs) -- any job still dispatched at the moment of a forced kill is treated as interrupted, matching the existing coarser precedent set by _interrupt_running_agents(), which interrupts every entry in _running_agents on a drain timeout without per-agent correlation either. Deliberately out of scope (flagged in the issue as a separate, lower-priority concern): startup-time reconciliation of cron runs that started but never reached a terminal status. Testing: - tests/cron/test_shutdown_interrupt.py (12 tests): get_running_job_ids snapshot semantics, mark_running_jobs_interrupted marking/no-op/ partial-failure behavior, and -- the core race guard -- run_one_job skipping its own last_status write (both the success path and the exception path) when the shutdown path already marked the run interrupted, with a control test proving ordinary un-interrupted completions are unaffected. - tests/gateway/test_cron_active_work_drain.py (9 tests): _active_cron_job_count reading cron state and failing closed (0) if the cron module is unavailable; _drain_active_agents waiting for an in-flight cron job the same way it waits for chat sessions, timing out if the job outruns the window, and leaving existing chat-session drain behavior unchanged; a full runner.stop() integration test (drain-timeout path) proving mark_running_jobs_interrupted actually fires with the right job ID when a tool subprocess is force-killed, plus a no-op control when nothing cron-related is in flight. - tests/gateway/test_shutdown_cache_cleanup.py: added _active_cron_job_count() to that file's hand-rolled _FakeGateway test double, which stop() now calls -- without it those 8 pre-existing tests AttributeError (caught by fail-then-pass below, not a production bug). Fail-then-pass: reverted gateway/run.py + cron/scheduler.py, all 21 new tests fail (fixture/attribute errors -- the feature doesn't exist yet); restored, all 21 pass. Regression check: ran the full plausibly-affected surface -- tests/gateway/{test_gateway_shutdown,test_restart_drain, test_restart_notification,test_restart_redelivery_dedup, test_restart_resume_pending,test_restart_service_detection, test_shutdown_cache_cleanup,test_stuck_loop,test_clean_shutdown_marker, test_external_drain_control,test_session_state_cleanup, test_update_command,test_update_streaming}.py plus tests/cron/ (944 tests) -- against a clean upstream/main checkout and against this branch. Diffed the two FAILED lists: identical, 20 pre-existing failures on both sides (Windows-locale/cp1252 file-encoding issues and Unix-permission-bit assertions that don't apply on this Windows dev box), zero new failures, zero fixed-by-accident. The 8 test_shutdown_cache_cleanup.py failures found mid-development were from the _FakeGateway gap above, fixed in the same commit and confirmed clean on the final rerun (diff against baseline: exit 0). Fixes #60432	2026-07-07 22:15:04 -07:00