Cancel the Redshift statement when a user kills the deferred task - #69676
Conversation
A deferred RedshiftDataOperator parks its query in the triggerer, so the operator's own on_kill no longer runs once the task is deferred. When a user marks that task failed, clears it, or marks it success, the trigger had no on_kill hook, so the Redshift statement kept running against the cluster or workgroup even though the operator already cancels the statement on kill in the non-deferred path. On Redshift Serverless a runaway statement keeps billing RPU-hours, and on a provisioned cluster it holds a WLM slot, until it finishes on its own. This adds on_kill to RedshiftDataTrigger to cancel the running statement when the user acts on the deferred task, matching the behaviour already shipped for the EMR, Dataproc, BigQuery, and Dataflow triggers. A cancel_on_kill flag on both the operator and the trigger lets users opt out.
02ddfa7 to
af96bfc
Compare
potiuk
left a comment
There was a problem hiding this comment.
Good catch on a gap with a direct running cost — after defer() the worker is gone, so the operator's on_kill could never fire, and an orphaned statement keeps billing RPU hours on Serverless or holds a WLM slot on a provisioned cluster.
The part I care most about is where the cancellation lives, and it is in the right place. BaseTrigger's own docstring warns that putting external job cancellation in cleanup() would cancel in-flight work on every triggerer restart or rolling deploy; this uses on_kill(), and the triggerer only invokes that on an explicit user action — the timeout path raises without calling it. So a deploy will not take out live queries. It is also bounded by _ON_CANCEL_TIMEOUT, so a slow cancel cannot wedge the triggerer.
Adding cancel_on_kill to serialize() matters more than it looks: without it the flag would be silently dropped crossing into the triggerer, and the opt-out would appear to work while doing nothing.
Handling CancelStatement returning Status=False is a nice detail — reporting "declined, may have already finished" rather than claiming a successful cancel is the difference between a log you can trust and one you cannot. Swallowing cancel errors with log.exception is right too; a failed cancel must not block the kill.
Worth being explicit in the release notes that killing a deferred task now cancels the statement where it previously orphaned it. That is the intended fix, and cancel_on_kill=False is the escape hatch, but it does change what a kill does for anyone who had built expectations around the old behaviour.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
Problem
RedshiftDataOperator.on_killcancels the running statement, but afterdefer()the worker is gone and it can never run.RedshiftDataTriggerhad noon_kill(base default is a no op), so killing a deferred task orphans the statement: RPU hours keep billing on Serverless, a WLM slot stays held on a provisioned cluster.Change
on_killtoRedshiftDataTrigger: cancels via the hook's async connection (cancel_statement).CancelStatementreturns{"Status": bool}; a declined cancel (statement already finished) logs a warning instead of claiming success.cancel_on_killflag (defaultTrue) on operator and trigger; operator threads it into the trigger at the defer site.Live verification
STARTED); taskdeferred, trigger polling via the real aiobotocore client.DescribeStatementreturnedStatus: ABORTED. Without the change the statement staysSTARTED(baseon_killis a no op).Tests
StatusTrue/Falsewith the real response shape), disabled/missing id no ops, error swallowing, serialization of the flag.cancel_on_kill=Falseguard, flag threaded into the trigger on defer (bothTrueandFalse).Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Fable 5) following the guidelines