Repository navigation
fix(cron): alert on restart-safe worker dispatch failures (salvage #123432) - #124013
Merged
kshitijk4poor merged 6 commits intoSep 26, 2026
Conversation
…dent path A failed restart-safe handoff in run_one_job() recorded the failure on the job and in the executions ledger, then returned before any incident or delivery path ran: no cron_incidents row, no failure-lane notice. Route the dispatch-failure branch through _deliver_crash_failure() so it opens the same job+signature incident and delivers the same failure notice as any other job failure, with the existing alerted-cooldown withholding repeats. A notice-path exception no longer loses the bookkeeping: mark_job_run and finish_execution still run with a "failed" delivery outcome (NousResearch#123401). (cherry picked from commit e5b5969)
_deliver_crash_failure is already best-effort internally (incident upsert, _deliver_result and _mark_incident_alerted each swallow their own errors), and the sibling crash path in run_one_job calls it without a wrapper. The extra try/except was a defensive layer that could only mask a genuine bug in the notice helper; drop it so both call sites match. The existing try/finally still guarantees finish_execution runs after mark_job_run. webtecnica submitted an equivalent independent fix in NousResearch#123433. Fixes NousResearch#123401 Co-authored-by: webtecnica <webtecnica@gmail.com>
…ivery raises The dispatch-failure branch called _deliver_crash_failure before the try/finally that closes the execution row. If delivery ever raised, finish_execution was skipped and the row stayed claimed until dead-owner recovery. Run delivery inside the try so the pre-existing finally still terminalizes the attempt; delivery_error/outcome default to None when it raises.
_launch_external_cron_worker returns through _wait_for_external_cron_worker once the worker is spawned/acknowledged, and that waiter can raise (e.g. the adopted worker died and recovery could not terminalize the row). Such an error landed in the same run_one_job except branch as a genuine dispatch failure, so a job that actually ran got a false "Restart-safe cron worker dispatch failed" incident and failure ping on top of whatever the worker itself delivered. Wrap waiter failures in a dedicated _ExternalWorkerPostHandoffError and skip _deliver_crash_failure for it, keeping the base bookkeeping-only behaviour (mark_job_run + finish_execution, the latter a no-op on a worker-owned row). Pre-handoff dispatch failures still open an incident and notify.
run_one_job's dispatch-failure handler already routes an _ExternalWorkerPostHandoffError through bookkeeping only (no incident, no ping), but the recorded error still read "Restart-safe cron worker dispatch failed", which is misleading in last_error / executions.db for a worker that was spawned and may have run. Compute post_handoff first and label it "Restart-safe cron worker failed after handoff: ..."; the pre-handoff prefix is unchanged.
The fold that stops post-handoff waiter failures from raising a false "dispatch failed" incident/ping had no regression coverage. Drive run_one_job through a real _wait_for_external_cron_worker whose body raises and assert: no cron incident, no delivery, one failed mark_job_run carrying the post-handoff label, and the execution row terminalized as failed. Red with the post-handoff routing reverted (an incident is opened).
kshitijk4poor
enabled auto-merge (rebase)
September 26, 2026 14:36
This was referenced Sep 26, 2026
Closed
12 of 13 tasks
kshitijk4poor
pushed a commit
that referenced
this pull request
Oct 3, 2026
run_one_job's pre-handoff dispatch-failure branch (#124013) calls _deliver_crash_failure before _run_one_job_body, which is where the firing profile's secret scope is installed for delivery. Under a multiplexed gateway the notice then resolves the home channel through get_secret with no scope, raises UnscopedSecretError, and never leaves. Wrap the notice in _install_fire_secret_scope/_reset_fire_secret_scope, the pair _run_one_job_body and record_unknown_worker_outcome already use. (cherry picked from commit 5e0735b)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A failed handoff to the restart-safe cron worker now opens a
cron_incidentsrow and sends the job's failure notice, instead of failing silently. Fixes #123401. This salvages #123432 by @liuhao1024.Why
When the external-worker handoff failed,
run_one_job()(cron/scheduler.py) recorded the failure on the job and inexecutions.db, then returned before any operator-facing path ran. There was no incident and no notice. In #123401, a 9.5-hour outage produced 106 failed executions and zero notifications, including for jobs whose only purpose is alerting.Changes
_deliver_crash_failure, the same path a crashed run uses. That covers incident dedup, cooldown and the delivery outcome in the ledger.finish_executionstill runs if delivery raises. There's no new try/except swallowing. This is defensive and not pinned by a test (the stack keeps to two tests); the probe covers it.last_errorreadsfailed after handoff. A pre-handoff failure keeps the existingdispatch failedtext.Validation
failed/delivereddetected, delivery_error recordedtests/cron/test_restart_safe_worker.py+tests/cron/test_run_one_job.pypass on the rebased head.Known limit
The alert is not gated on the fire-claim owner. A non-owner that lost the claim can still alert on a pre-handoff failure. The triage rated this low severity, and it is left for a follow-up.
Credit