Repository navigation
Skip on_kill() for triggers reassigned to another triggerer - #73454
Conversation
When a triggerer's heartbeat lapsed and its triggers were handed to another triggerer, the recovering triggerer treated every trigger missing from its assignment list as a user action and called on_kill(), cancelling remote work the new owner had just started polling. The supervisor now checks whether the trigger row still exists (and who owns it) before deciding: a surviving row is released locally without on_kill(); only a deleted row takes the user-action path.
amoghrajesh
left a comment
There was a problem hiding this comment.
Good fix, I have some nits, otherwise looks good.
|
Hello @kaxil , Thanks for this fix. Is a backport to the Airflow 3.3.x release line planned, and if so, which version would include it? We observed explicit BigQuery CancelJob calls while a BigQueryInsertJobOperator task was deferred on Airflow 3.3.1. We have not confirmed that trigger reassignment caused our incident, but the behavior described in this PR appears relevant. The fix is not present in the 3.3.1 or 3.3.2 tags. |
Backport failed to create: v3-4-test. View the failure log Run detailsNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
You can attempt to backport this manually by running: cherry_picker eebf253 v3-4-testThis should apply the commit to the v3-4-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continueIf you don't have cherry-picker installed, see the installation guide. |
Unfortunately, it will only be part of 3.4.0 since 3.4.0rc1 is about to be created |
|
Thanks for clarifying. The impact for us is broader than BigQuery. We have observed unexpected interruptions affecting multiple deferrable GCP operators, including BigQuery and Dataproc. In the BigQuery case, audit logs confirm explicit CancelJob calls while the task was deferred; two successive attempts of the same task ended in cancellation, so a retry did not reliably recover it. We cannot yet prove that trigger reassignment caused every incident. Nevertheless, the core failure mode fixed by this PR affects multiple remote-job triggers, and makes deferrable mode effectively unsafe for our affected workloads on Airflow 3.3.1. Falling back to non-deferrable execution consumes workers for the duration of these jobs. Given this operational impact, could a 3.3.x backport be reconsidered? If that is not possible, is there a supported mitigation for managed environments that cannot wait 3.4.X ? |
When a triggerer's heartbeat lapses for longer than
triggerer_health_check_threshold,Trigger.assign_unassignedhands its triggers to another triggerer. The old triggerer is often still alive: the runner-silence watchdog deliberately skips heartbeats when the runner subprocess stops responding, and a long GC pause, a stopped container or a network partition do the same. When it recovers,TriggerRunnerSupervisor.update_triggerssees those triggers missing from its assignment list and queues them for cancellation with the user-action message, sorun_triggercallson_kill()on every one of them. Nothing in that path distinguishes "the row was deleted because a user cleared the task" from "the row was reassigned".For any trigger that cancels remote work from
on_kill()this cancels a job the new owner has just started polling. The batch triggers in the Anthropic and Common AI providers, and the EMR, Redshift Data, Databricks, Dataproc and BigQuery triggers all do. TheBaseTrigger.on_killdocstring and the deferring docs both promise it does not fire on redistribution, so this is a contract violation, present sinceon_kill()shipped in 3.3.0 (#65590). Thecancel_triggersdocstring claimed redistribution "goes through a separate path"; there was no such path.Solution
Before queueing a cancel, the supervisor asks the DB which of the departed ids still have a trigger row and who owns them now (
Trigger.fetch_assignments, behind an overridablefetch_trigger_assignmentshook that owns its own session soupdate_triggersstays DB-free for subclasses without a metadata DB). A row that survives after leaving our list was reassigned, so those ids go to a newreleasing_triggersset and reach the runner asTriggerStateSync.to_release; the runner cancels those asyncio tasks with a distinct sentinel, andrun_triggerdrops the trigger withouton_kill(). Only a row that is gone, which is what a clear or mark-success/failed produces, takes the existing user-action path. The classification runs once per departed id, since ids already queued are excluded from the next loop's difference. The new field defaults to an empty set, so existingTriggerStateSyncproducers are unaffected.Why in the supervisor rather than the runner. The runner subprocess has no DB access; the supervisor already owns the assignment query and the per-loop
create_session()inbuild_trigger_workloads, so the one extraSELECT id ... WHERE id IN (...)sits next to it, and the runner keeps its existing message-based contract.Reproduction
Two triggerers started with
--queues afl297andtriggerer.queues_enabled=True(so the standalone triggerer stays out of it), a task on that queue deferring to a trigger whoseon_kill()appends a line to a file, thenSIGSTOPon the owning triggerer and its runner child until the trigger row moved, thenSIGCONT.SIGCONTon_kill()markerTrigger cancelled by user action, invoking on_killdeferredTriggers were reassigned to another triggerer, releasing them without on_kill new_owners={3: 8}and, in the runner,Trigger reassigned to another triggerer, dropping it without on_killWith this PR, marking the deferred task failed through the REST API still produced the marker from the new owner's runner within seconds, so the user-action path is intact.
Known gap
Row existence proves the row was reassigned, not that the new owner has started it. If a user clears the task in the window after the old owner classified the trigger as reassigned and before the new owner fetched its workload, neither side calls
on_kill(): the old owner released, and the new owner skips a trigger whose task instance is gone. This is the same outcome main has when the old owner has actually died, soon_kill()on a user action was already best-effort while a failover is in flight. The fix errs toward leaving remote work running rather than cancelling work another triggerer is polling.{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.