Roll back orphaned backfill when run creation fails

[Abdulrehman-PIAIC80387]

When a backfill is created via POST /api/v2/backfills, _create_backfill committed the Backfill row before creating its dag runs. If run creation then failed — the reported case is sqlite3.OperationalError: database is locked under concurrent requests, but any error would do — the already-committed Backfill row survived with no/partial runs. The num_active > 0 check then treated it as an in-progress backfill and blocked all future backfills for that dag with "already running backfill".

So this is an atomicity problem, not really SQLite-specific: any failure mid-creation left an orphaned, un-removable backfill.

Fix

Keep the whole creation in a single transaction: session.flush() instead of the early session.commit() populates backfill.id for the run-creation code, and create_session already rolls back on exception. A failed creation therefore leaves no rows behind — Backfill, DagRun, TaskInstance and BackfillDagRun all go away together — so the dag is not blocked.

Design notes

An earlier revision of this PR did the cleanup manually (try/except around run creation, then DELETE the orphan row). Per review feedback that was the wrong shape — a plain rollback is enough once nothing is committed early, so this revision deletes that machinery instead of adding to it.

This also supersedes the best-effort _cleanup_partial_backfill helper added in #67900 for the lock-error path: with one transaction there is no partial state to clean up, so the helper and its three unit tests are removed (~40 lines of production code). The 503 mapping that PR added at the route boundary is untouched — a lock error still surfaces as 503 Service Unavailable, it just no longer needs a compensating delete behind it.

Gotchas

The removed comment on the early commit claimed it made the backfill visible to concurrent requests checking num_active, "preventing duplicate active backfills". It never actually prevented that: two requests can both read num_active == 0 before either commits, so the race predates this PR either way. Flushing rather than committing does widen that window from "until the row is inserted" to "until the whole creation commits". Genuinely closing it needs a DB-level guard (a partial unique index on (dag_id) where completed_at IS NULL, or a row lock on the dag), which I'd rather do as a separate, focused change than smuggle in here. Happy to follow up with it if you'd like.

Tests

• test_create_backfill_no_orphan_on_run_creation_failure (model): a generic failure during run creation leaves no Backfill row, and a subsequent backfill for the same dag succeeds.
• test_create_backfill_lock_error_rolls_back_partial_state (route): a lock error on the third run — i.e. after two runs already exist in the transaction — returns 503 and leaves zero Backfill / DagRun / TaskInstance / BackfillDagRun rows. This replaces the three helper-level tests from API: Return 503 when SQLite locks during backfill creation #67900 with one behavioural test through the endpoint.

Both fail without the change and pass with it.

closes: #68699

[mwisnicki]

Does the test trigger the bug before fix is applied?

[Abdulrehman-PIAIC80387]

Yes — confirmed both directions:

• Without the fix (reverting just the _create_backfill change, keeping the test): it fails — the orphaned Backfill row survives the run-creation failure, so the count == 0 assertion fails (and a retry would hit AlreadyRunningBackfill).
• With the fix: it passes.

So it's a genuine regression test for the orphaned-backfill behaviour. The full test_backfill.py suite also passes (84 passed).

[ashb]

This doesn't feel like the right fix. Please change things so that it is all in a single transaction so that "simple rollback" is enough to remove the row. A session.flush() instead of a session.commit further up is I think enough go get br.id populated.

[Abdulrehman-PIAIC80387]

Agreed, and done — session.flush() populates backfill.id, create_session rolls back on exception, and the whole try/except plus the manual delete are gone.

While rebasing onto latest main I found this now also supersedes the best-effort _cleanup_partial_backfill helper added in #67900: with a single transaction there is no partial state to compensate for, so the helper is removed too (net −85 lines). The 503 mapping at the route boundary is untouched. Its three helper-level unit tests are replaced by one behavioural test that drives a lock error through the endpoint on the third run — after two runs already exist in the transaction — and asserts the Backfill, DagRun, TaskInstance and BackfillDagRun rows are all gone.

One thing worth flagging: the comment on the early commit claimed it prevented duplicate active backfills for a dag. It didn't really — two requests can both read num_active == 0 before either commits — but flushing does widen that window. Closing it properly needs a DB-level guard (partial unique index on dag_id where completed_at IS NULL, or a row lock), which I've left out of this PR to keep it focused. Happy to do it as a follow-up if you think it's worth it.