Skip to content

fix(task-sdk): handle missing DAG in TriggerDagRunOperator gracefully - #63207

Closed
YoannAbriel wants to merge 2 commits into
apache:mainfrom
YoannAbriel:fix/issue-63089
Closed

fix(task-sdk): handle missing DAG in TriggerDagRunOperator gracefully#63207
YoannAbriel wants to merge 2 commits into
apache:mainfrom
YoannAbriel:fix/issue-63089

Conversation

@YoannAbriel

Copy link
Copy Markdown
Contributor

Problem

TriggerDagRunOperator crashes with UnboundLocalError: cannot access local variable 'state' when targeting a non-existent DAG. The API server returns 404, which SUPERVISOR_COMMS.send() re-raises as ServerResponseError — but this propagates past all exception handlers in run() into the finally block where state was declared but never assigned.

Root Cause

Two issues:

  1. state: TaskInstanceState in run() is declared without a default value. Any unhandled exception reaching the finally block causes UnboundLocalError when accessing state.value for stats.

  2. The DagRunOperations.trigger() client method only handles 409 (conflict) — a 404 from the API server is re-raised as ServerResponseError, which isn't caught by _handle_trigger_dag_run.

Fix

  • Initialize state = TaskInstanceState.FAILED as a safe default so the finally block always works, regardless of what exception propagates.
  • Add DAG_NOT_FOUND to ErrorType enum.
  • Handle 404 in DagRunOperations.trigger(), returning ErrorResponse(error=ErrorType.DAG_NOT_FOUND).
  • Handle DAG_NOT_FOUND in _handle_trigger_dag_run, failing the task gracefully instead of crashing.

Tests added for both the client-side 404 handling and the task runner DAG-not-found path.

Closes: #63089


Was generative AI tooling used to co-author this PR?
  • Yes — Claude Code

Generated-by: Claude Code following the guidelines


  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {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.

@YoannAbriel
YoannAbriel force-pushed the fix/issue-63089 branch 7 times, most recently from 7dac8a9 to 8b6e936 Compare March 16, 2026 16:09
@YoannAbriel
YoannAbriel force-pushed the fix/issue-63089 branch 2 times, most recently from 82c16cd to 898514d Compare March 23, 2026 19:07
@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label Apr 2, 2026
@YoannAbriel
YoannAbriel force-pushed the fix/issue-63089 branch 10 times, most recently from 175a64a to 9cd3765 Compare April 10, 2026 09:04
@kaxil
kaxil requested a review from Copilot April 10, 2026 19:55

Copilot AI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI 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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment on lines +4462 to +4465

log = mock.MagicMock()
mock_supervisor_comms.send.return_value = ErrorResponse(error=ErrorType.DAG_NOT_FOUND)
state, msg, _ = run(ti, ti.get_template_context(), log)

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

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

New test introduces an unspecced mock.MagicMock() for log. Airflow’s testing guidelines prefer mocks with spec/autospec to avoid silently accepting unexpected attributes; consider using a real structlog.get_logger(...) or mock.Mock(spec=...) for the logger instead.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +728 to +729
log.error("Dag not found.", dag_id=dag_id)
return ErrorResponse(error=ErrorType.DAG_NOT_FOUND)

Copilot AI Apr 10, 2026

Copy link

Choose a reason for hiding this comment

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

The new 404 handling logs only a generic "Dag not found" message and doesn’t include available response context (e.detail, status_code). Consider logging those fields (and/or returning them in ErrorResponse.detail) for easier debugging, consistent with other client methods’ 404 handling.

Suggested change
log.error("Dag not found.", dag_id=dag_id)
return ErrorResponse(error=ErrorType.DAG_NOT_FOUND)
log.error(
"Dag not found.",
dag_id=dag_id,
status_code=e.response.status_code,
detail=e.detail,
)
return ErrorResponse(error=ErrorType.DAG_NOT_FOUND, detail=e.detail)

Copilot uses AI. Check for mistakes.
@YoannAbriel
YoannAbriel force-pushed the fix/issue-63089 branch 2 times, most recently from 69533e8 to e8284fe Compare April 13, 2026 06:06
When TriggerDagRunOperator targets a non-existent DAG, the API server
returns 404 which was re-raised as ServerResponseError. This propagated
out of _handle_trigger_dag_run into the finally block of run() where
'state' was never assigned, causing UnboundLocalError.

Two fixes:
- Initialize state to FAILED before the try block so the finally block
  always has a valid value
- Handle 404 in the dag_runs client (returning DAG_NOT_FOUND error) and
  in _handle_trigger_dag_run (failing the task gracefully)

Closes: apache#63089
@potiuk potiuk removed the ready for maintainer review Set after triaging when all criteria pass. label May 18, 2026
@potiuk
potiuk marked this pull request as draft May 18, 2026 10:46
@potiuk

potiuk commented May 18, 2026

Copy link
Copy Markdown
Member

@YoannAbriel — Removing the ready for maintainer review label and converting back to draft. The branch now has merge conflicts with main that surfaced after the label was added.

The label's contract is that the PR is ready for maintainer review — a regression like this means the PR temporarily isn't. Rebase your branch onto the latest main, resolve conflicts, then mark "Ready for review" again to re-enter the queue.

git fetch upstream main && git rebase upstream/main, resolve, git push --force-with-lease. See the working-with-git docs.

No rush.


Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you.

@potiuk

potiuk commented Jun 1, 2026

Copy link
Copy Markdown
Member

@YoannAbriel This draft PR has had no activity for 2 weeks. Closing to keep the queue clean.

You are welcome to reopen and continue when you're ready. If you'd like to pick it back up, please rebase onto the current main branch first.


Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UnboundLocalError on TriggerDagRunOperator triggering a dag that doesn't exist

3 participants