Fail queued DagRuns with an unresolvable pinned dag version

[RehanAhmad25]

Fail queued DagRuns with an unresolvable pinned dag version

closes: #70056

A DagRun pinned to a specific dag version (bundle_version + created_dag_version_id) can get stuck in QUEUED forever if the pinned version row is later deleted. The FK is ON DELETE SET NULL, so created_dag_version_id becomes NULL while bundle_version stays set, and _version_from_dag_run only falls back to the latest versionwhen bundle_version is unset. The run is neither started nor failed, logged and skipped on every scheduler loop indefinitely.

This detects that specific, permanently-unresolvable case (pinned version deleted) and fails the run explicitly via set_state(), which correctly updates end_date along with the state transition. Other reasons get_dag_for_run can return None (e.g. an unpinned run whose version hasn't parsed yet) are left untouched, since those can still resolve on a later loop and shouldn't be failed outright.

A second test confirms that scoping: an unpinned run with no resolvable version yet stays QUEUED rather than being failed.

---

Was generative AI tooling used to co-author this PR?

• Yes (Claude)

Generated-by: Claude (Anthropic) following the guidelines

[ephraimbuddy]

How about changing the dagrun column to restrict like TI dag version column? Can you find out why we used set null. I can’t remember now but it seems wrong now

[RehanAhmad25]

Yeah, I think we can change it, RESTRICT looks like the right call here.

Dug through git blame on this. created_dag_version_id was added in #49097 back in April 2025 (the "use best available serdag" PR), and it went with SET NULL from the start. What's interesting is that at that same time, task_instance.dag_version_id was actually on CASCADE, not RESTRICT, so deleting a dag_version would've silently wiped out any TI rows pointing at it. That got caught and fixed a month later in migration 0072 (3.1.0), moving TI over to RESTRICT specifically to stop that data loss.

DagRun just never got the same follow-up. Looks like an oversight from that period rather than a deliberate choice, nothing in the PR discussion explains why DagRun should behave differently from TI here.

I checked whether RESTRICT would actually be safe to add: the only place dag_version rows get deleted at all is delete_dag(), and it already deletes TaskInstance and DagRun rows before the DagModel cascade touches DagVersion (there's a comment in that function about doing this on purpose for TI's RESTRICT constraint). So DagRun would already be gone by the time the cascade fires, switching to RESTRICT shouldn't affect normal dag deletion at all, it'd only ever trigger in exactly the broken scenario this issue describes.

I can add a migration for it, basically mirroring 0072 but for dag_run.created_dag_version_id, want me to add that to this PR or should it go separately?

[ephraimbuddy]

Yeah, I think we can change it, RESTRICT looks like the right call here.

Dug through git blame on this. created_dag_version_id was added in #49097 back in April 2025 (the "use best available serdag" PR), and it went with SET NULL from the start. What's interesting is that at that same time, task_instance.dag_version_id was actually on CASCADE, not RESTRICT, so deleting a dag_version would've silently wiped out any TI rows pointing at it. That got caught and fixed a month later in migration 0072 (3.1.0), moving TI over to RESTRICT specifically to stop that data loss.

DagRun just never got the same follow-up. Looks like an oversight from that period rather than a deliberate choice, nothing in the PR discussion explains why DagRun should behave differently from TI here.

I checked whether RESTRICT would actually be safe to add: the only place dag_version rows get deleted at all is delete_dag(), and it already deletes TaskInstance and DagRun rows before the DagModel cascade touches DagVersion (there's a comment in that function about doing this on purpose for TI's RESTRICT constraint). So DagRun would already be gone by the time the cascade fires, switching to RESTRICT shouldn't affect normal dag deletion at all, it'd only ever trigger in exactly the broken scenario this issue describes.

I can add a migration for it, basically mirroring 0072 but for dag_run.created_dag_version_id, want me to add that to this PR or should it go separately?

Yes. Let's add it to this PR

[RehanAhmad25]

@ephraimbuddy Completed the migration work discussed above and force-pushed this branch. A couple of local pre-push checks blocked the push at first, both unrelated to this PR, more on that below, so I bypassed them locally to get this up.

What changed: added the migration, changing dag_run.created_dag_version_id's FK from ON DELETE SET NULL to ON DELETE RESTRICT (mirrors migration 0072, which did the same for task_instance.dag_version_id). Updated the model and REVISION_HEADS_MAP to match. Verified locally with a full db migrate → db downgrade → db migrate round-trip, and the existing tests for this fix still pass.

Why the force-push: I rebased onto current main to pick up the latest history and get a clean base for the new migration, rather than merging.

The local checks that blocked me:check-provider-yaml-valid failing on providers/common/ai/provider.yaml (a missing AWSToolset registration), and ktlint failing because I don't have a local Java runtime for the JVM/TS SDK files that came in via the rebase. Neither touches anything in this diff, both are pre-existing on main. If CI shows either red, that's why.

Could someone approve the workflows so CI can run on this? Happy to address anything it turns up.

[RehanAhmad25]

@ephraimbuddy Following up here since it's been about a month with no activity, apologies for the gap on my end too, had to sync up with a bunch of upstream changes.

Since that last comment, I rebased again to catch up with main (a good chunk had landed since, including some new migrations), fixed the resulting conflicts, and this time everything came through clean, no CI-blocking issues locally, no need to bypass any checks. While I was at it, I also caught and fixed a real bug that surfaced from the rebase: turned out cached_get_dag() has a fallback that returns a truthy base DAG even when the pinned version is unresolvable, so my original check was getting skipped. Moved it to run before that call instead, verified with the existing tests plus a full migration round-trip.

Whenever you get a chance, would appreciate a look, and if it needs another round of workflow approval to get CI running, happy to poke that too. No rush, just wanted to bump this since it's been sitting a while.