Skip to content

cron: record killed post-adopt worker on the job (incident, delivery, log) - #120360

Closed
Finn763 wants to merge 2 commits into
NousResearch:mainfrom
Finn763:fix/120328-cron-killed-worker-job-update
Closed

Finn763 wants to merge 2 commits into
NousResearch:mainfrom
Finn763:fix/120328-cron-killed-worker-job-update

Conversation

@Finn763

@Finn763 Finn763 commented Sep 23, 2026

Copy link
Copy Markdown

After adopt, a worker killed without terminalizing (e.g. systemd OOM-kill of its scope) was recovered to `unknown` while the job record kept the previous run's values: no incident, no delivery, no gateway log. Every job-level surface said the run never happened.

The external-worker waiter now does gateway-side bookkeeping on that path: gateway error log, failure incident (`_upsert_incident_for_failure`), failure delivery (`_deliver_crash_failure` with the run's adapters/loop, threaded through from `run_one_job`), and an owner-fenced `mark_job_run(False)` carrying the execution error plus the worker exit code.

Safety: only runs when the recovered record is `unknown` (a worker that terminalized itself keeps its own bookkeeping); fenced by the launch-time fire owner, so a replacement claim is never touched; the first mark consumes the claim, making repeats no-ops; the helper never raises, preserving the waiter's contract.

Tests: new `tests/cron/test_cron_killed_worker_job_update.py` failed on main (job `last_run_at` stayed None) and passes with the fix. Neighbor suites `test_restart_safe_worker` (2 assertions updated for the intentional `(job, adapters, loop)` handoff shape), `test_cron_incidents`, `test_cron_failure_deliver` green; the one remaining failure (`test_scoped_wrapper_exit...`, `os.getuid`) fails identically on pristine main on Windows.

Closes #120328.

… log)

After adopt, a worker killed without terminalizing (e.g. systemd OOM-kill) was recovered to unknown while the job record kept the previous run's values, no incident opened, nothing delivered, nothing logged. The waiter now does gateway-side bookkeeping: error log, failure incident, failure delivery, and an owner-fenced mark_job_run(False) carrying the exit code. Fenced by the launch-time fire owner, so replacement claims are never touched and repeats are no-ops. Closes NousResearch#120328.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Sep 23, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

Blocking

The bookkeeping sits at only one of the four exits a dead adopted worker can take — a competing reaper wins the unknown transition, and the helper never runs — cron/scheduler.py:3487.

_record_killed_worker_run has one call site: the branch after the waiter's own recover_interrupted_executions() (cron/scheduler.py:3487). Both earlier exits test _is_terminal(), and _TERMINAL_STATES includes "unknown" (cron/executions.py:33) — so a record a prior reaper already moved to unknown satisfies it: the timeout branch returns True at cron/scheduler.py:3467, the post-wait() branch at cron/scheduler.py:3476. Neither calls the helper, and both return the same "fine" answer the killed path gives.

That ordering is reachable in-process: recovery also runs from the tick's dead-owner reaper (cron/scheduler.py:4051, wired at cron/scheduler_tick.py:55) and on every manual run pre-dispatch (tools/cronjob_tools.py:401). The waiter polls every second, so either landing before the waiter's own recovery marks the row unknown first and the fix does nothing. That is exactly the post-adopt window: a running row with a dead owner is what recovery selects for.

Reproduced by pinning get_execution to the unknown record a prior reaper leaves and driving the real _wait_for_external_cron_worker: last_run_at=None last_status=None incidents=0 delivered=False — the pre-PR symptom on the path the PR claims to close. The new test cannot see it: it stubs recover_interrupted_executions (tests/cron/test_cron_killed_worker_job_update.py:34-45), so the waiter's own call always wins. Gating on status == "unknown" is not enough; the helper must be reachable from every exit that can see a dead worker

Minor

The exit code sits inside the incident-signature window, so a flapping kill mints an incident per code and resets the alert cooldown — cron/scheduler.py:3390.

_error_signature hashes the first 200 normalized chars (cron/incidents.py:126-129); _OWNER_GONE_REASON plus a code is 160 — so 137 and 143 yield different signatures and different incident rows, each fresh at detected. _repeat_alert_withheld (cron/scheduler.py:367-383) suppresses repeat pings per single signature, so an OOM-kill then a SIGTERM alerts twice despite the cooldown. _OWNER_WEDGED_REASON (209 chars) truncates past the code, so the two reasons disagree.

A competing dead-owner reaper (tick reap / manual pre-dispatch) can move the
killed worker's row to ``unknown`` before the waiter's own recovery runs; both
terminal exits then returned "fine" without the job bookkeeping (NousResearch#120328
follow-up). Hand every terminal record the waiter sees to
``_record_killed_worker_run`` (still a no-op unless the row is ``unknown`` and
the launch claim stands), with a None returncode for the wait-timeout exit
where the process has not exited yet.

Also exclude the exit code from the incident signature: a kill that flaps
between signals (137 vs 143) must refresh one incident per recovery reason
rather than mint a fresh row per code and reset the repeat-alert cooldown.
@Finn763

Finn763 commented Oct 6, 2026

Copy link
Copy Markdown
Author

Addressed the automated review (53b4f49):

Blocking — bookkeeping reachable from every exit. Both terminal exits now hand the record they see to _record_killed_worker_run before returning: the wait-timeout exit (no exit code yet — the process is still alive or in final teardown) and the post-wait() exit, on top of the waiter's own-recovery path. The helper still no-ops unless the row is unknown and the launch claim still stands, so a worker that terminalized itself is untouched and repeats stay idempotent. Two new tests drive the real _wait_for_external_cron_worker with get_execution pinned to the pre-recovered unknown record (both _OWNER_GONE_REASON and _OWNER_WEDGED_REASON): on the old head both fail with last_run_at=None last_status=None incidents=0 delivered=False; green now.

Minor — exit code out of the signature window. _error_signature strips the trailing (cron worker exited with code N) before hashing, consistently for both recovery reasons: 137 then 143 refreshes one incident per reason instead of minting a row per code and resetting the repeat-alert cooldown. New test pins one incident per reason across the flap (was 3 rows, now 2).

Tests: tests/cron/test_cron_killed_worker_job_update.py 4 passed (3 new). Adjacent subset (restart-safe worker, incidents, dead-owner reclaim, ledger, start-time drift, stale recovery, terminal rearm, warning outcome): 109 passed, 4 skipped, 1 failed — test_scoped_wrapper_exit_without_user_bus_names_the_cause_and_invalidates_probe fails identically on base 6b6c7f4 (AttributeError: module 'os' has no attribute 'getuid'; the scoped-dispatch branch runs only on Linux), so not from this change.

@Finn763 Finn763 closed this Oct 10, 2026
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 comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: cron worker killed after adoption (e.g. OOM) leaves the job record untouched — no last_status, no incident, no failure delivery

3 participants