Skip to content

Add on_kill() to DatabricksTaskBaseOperator to cancel runs on task kill - #69442

Merged
eladkal merged 1 commit into
apache:mainfrom
victorymakes:fix/databricks-task-base-operator-on-kill
Aug 6, 2026
Merged

Add on_kill() to DatabricksTaskBaseOperator to cancel runs on task kill#69442
eladkal merged 1 commit into
apache:mainfrom
victorymakes:fix/databricks-task-base-operator-on-kill

Conversation

@victorymakes

Copy link
Copy Markdown
Contributor

Summary

DatabricksSubmitRunOperator and DatabricksRunNowOperator both implement on_kill() to cancel the Databricks run when an Airflow task is killed (SIGTERM or execution_timeout). DatabricksTaskBaseOperator — the base for DatabricksTaskOperator and DatabricksNotebookOperator — is missing the same implementation, so Databricks jobs continue running after the Airflow task is killed, orphaning compute resources and incurring unnecessary cloud spend.

DatabricksWorkflowTaskGroup received on_kill() in #42115; this PR closes the remaining gap for standalone task operators.

Changes

  • Adds on_kill() to DatabricksTaskBaseOperator using self.databricks_run_id, which is:
    • initialised to None in __init__ (no AttributeError risk)
    • set by _launch_job() the moment the run is submitted — earlier than any polling or permission calls
  • Adds unit tests covering both the cancel and no-op paths

Testing

pytest providers/databricks/tests/unit/databricks/operators/test_databricks.py -k "on_kill"
Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code (Sonnet 4.6)

@eladkal
eladkal requested a review from amoghrajesh July 6, 2026 15:45
@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jul 8, 2026
@Vamsi-klu

Vamsi-klu commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

databricks_run_id is the shared parent workflow-run ID for a member of DatabricksWorkflowTaskGroup. Cancelling it here would therefore stop sibling tasks as well.

Could on_kill mirror monitor_databricks_job: use _get_current_databricks_task()["run_id"] for workflow members, while retaining self.databricks_run_id for standalone operators? A regression test should set parent run ID 1, return child attempt ID 999, and assert cancel_run(999).

The branch also contains two fix: commit subjects, which current Airflow commit checks reject; those will need rewriting when the branch is rebased.


Drafted-by: Codex (GPT-5)

@victorymakes
victorymakes force-pushed the fix/databricks-task-base-operator-on-kill branch from 93e6f14 to 88089a2 Compare July 23, 2026 05:11
@victorymakes

Copy link
Copy Markdown
Contributor Author

Thanks for the review!

You're right — self.databricks_run_id is the shared parent workflow run ID for workflow members, so cancelling it would stop sibling tasks. Fixed in the latest commit.

on_kill() now mirrors monitor_databricks_job: for workflow members it calls _get_current_databricks_task()["run_id"] to get the child task's own run ID, while standalone operators continue to cancel via self.databricks_run_id directly.

Also added the regression test you suggested: parent run_id=1, _get_current_databricks_task returns child run_id=999, asserts cancel_run(999).

The branch has also been squashed to a single commit to fix the fix: subject check.

@victorymakes
victorymakes force-pushed the fix/databricks-task-base-operator-on-kill branch from 14783eb to 79168b2 Compare July 23, 2026 05:56
@victorymakes
victorymakes force-pushed the fix/databricks-task-base-operator-on-kill branch from 79168b2 to feb6783 Compare July 23, 2026 06:10
@Vamsi-klu

Copy link
Copy Markdown
Contributor

Thanks for the quick turnaround. I re-checked HEAD (feb6783) end-to-end.

Confirmed fixed

  • Workflow members cancel via _get_current_databricks_task()["run_id"] only (not the shared parent run id)
  • On child-run resolve failure: log + return (no parent fallback)
  • Unit coverage:
    • standalone cancel
    • no-op when databricks_run_id is None
    • workflow member child cancel (parent=1 -> cancel_run(999))
    • workflow member exception path (cancel_run not called)
  • Single commit; subject no longer uses a rejected fix: prefix

Residual before this is ready

Both touched files drop the first two lines of the standard ASF license header:

#
# Licensed to the Apache Software Foundation (ASF) under one

Please restore the full header in:

  1. providers/databricks/src/airflow/providers/databricks/operators/databricks.py
  2. providers/databricks/tests/unit/databricks/operators/test_databricks.py

Testing bar

No live Databricks credentials/UI testing needed for merge. This matches how DatabricksSubmitRunOperator / DatabricksRunNowOperator on_kill is validated: mock cancel_run and assert the correct run id.

A rebase onto current main will still be needed before merge (branch is quite behind). That can land together with the header fix.

Logic otherwise looks good from my side.

@victorymakes
victorymakes force-pushed the fix/databricks-task-base-operator-on-kill branch 3 times, most recently from 9af8024 to 7790249 Compare July 23, 2026 06:23
@victorymakes

Copy link
Copy Markdown
Contributor Author

Rebased onto current main and fixed the missing license header. Branch is now a single commit on top of 84e520a.

DatabricksSubmitRunOperator and DatabricksRunNowOperator both implement
on_kill() to cancel the Databricks run when an Airflow task is killed
(SIGTERM or execution_timeout). DatabricksTaskBaseOperator — the base for
DatabricksTaskOperator and DatabricksNotebookOperator — was missing the
same implementation, leaving Databricks jobs running after the Airflow task
was killed and orphaning compute resources.

DatabricksWorkflowTaskGroup received on_kill() in apache#42115; this PR closes
the remaining gap for standalone task operators.

For workflow members self.databricks_run_id is the shared parent run ID;
cancelling it would stop all sibling tasks. on_kill() therefore calls
_get_current_databricks_task()["run_id"] to target only the current task's
own child run, mirroring monitor_databricks_job. Standalone operators
continue to cancel via self.databricks_run_id directly.

If resolving the child run_id fails (API error, task_key mismatch), on_kill
logs the exception and returns without cancelling anything — falling back to
the parent run_id would stop sibling tasks, defeating the purpose.

Unit tests cover: cancel called for standalone operator, no-op when
databricks_run_id is None, workflow-member cancels child run (parent=1,
child=999, asserts cancel_run(999)), and workflow-member where
_get_current_databricks_task raises asserts cancel_run not called.
@victorymakes
victorymakes force-pushed the fix/databricks-task-base-operator-on-kill branch from 7790249 to 9bbe8d0 Compare July 23, 2026 06:28
@eladkal

eladkal commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

cc @moomindani for Databricks team review

@moomindani moomindani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I verified the run-id semantics both against internal Databricks documentation and empirically on a live workspace, because that is the crux of the original objection about cancelling the shared parent run.

Docs: the Jobs CLI/API reference states runs/cancel "Cancels a job run or a task run", so passing a child task run id is a supported operation, not an accident that happens to work. The internal Jobs Task API design doc confirms the id model — job_run_id is the parent, and multitask runs have multiple task runs grouped under it.

Empirically, with a two-task job running both tasks concurrently:

Action task_a task_b parent run
cancel task_a's child run (post-fix) TERMINATED/CANCELED still RUNNING RUNNING
cancel the parent run (pre-fix) CANCELED CANCELED CANCELED

So that concern was exactly right, and the fix does what it claims: a killed Airflow task now cancels only its own Databricks task run and leaves siblings alone. Cancelling the parent really does take the siblings down with it.

The rest checks out: on_kill sits on DatabricksTaskBaseOperator so both DatabricksTaskOperator and DatabricksNotebookOperator inherit it, it mirrors monitor_databricks_job's _get_current_databricks_task()["run_id"] pattern, and refusing to fall back to the parent id on resolution failure is the right call — that fallback is precisely the sibling-cancellation I measured. 204 tests pass in the file, prek --stage pre-commit clean.

One thing worth recording rather than changing: in deferrable mode this on_kill does not fire, since the operator has left the worker by then. That is not a gap — DatabricksExecutionTrigger has its own async on_kill, and monitor_databricks_job defers with run_id=current_task_run_id, i.e. the child run, so the deferred path cancels the correct run too. Both paths are covered.


Drafted-by: Claude Code (Opus 5)

@eladkal
eladkal merged commit dc0e0bb into apache:main Aug 6, 2026
83 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants