API: Return 503 when SQLite locks during backfill creation - #67900
Conversation
23tae
left a comment
There was a problem hiding this comment.
Thanks for working on this.
#66726 also reports that the backfill may still be created even though the request fails.
This PR seems to improve the error response from 500 to 503, but it is not clear whether it changes the partial-success behavior. Could we clarify this in the PR description?
d9ed03e to
8962d5e
Compare
8962d5e to
2c511e9
Compare
23tae
left a comment
There was a problem hiding this comment.
Thanks for the update. I left a few comments.
c1af990 to
cc00931
Compare
250fc2e to
3358470
Compare
23tae
left a comment
There was a problem hiding this comment.
I left a couple of comments.
Since implementing a retry loop in the API route introduces edge cases and makes the code complex, how about we just remove it entirely? Simply catching the OperationalError and raising an HTTP 503 on a database lock seems like a much cleaner approach.
3358470 to
8950a80
Compare
8950a80 to
dc36dae
Compare
4e546d7 to
67d50e1
Compare
a4cabdb to
2482bdd
Compare
|
Reproduced the 500 on macOS — 20 concurrent POST /backfills, 2/2 runs hit the lock + orphan. Repro steps and the full traceback posted on #66726. Also caught an edge case in the cleanup: SQLAlchemy deactivates the session on flush failure, so the first Ready for re-review. |
There was a problem hiding this comment.
Thanks for iterating on this. The direction is reasonable for the SQLite dev case, and I confirmed locally that the production path does return a clean 503 (Database is locked ... use PostgreSQL or MySQL) instead of the 500 — nice.
Two things block it though, both inline: the two ...database_locked tests actually fail as written, and the cleanup path that removes partial runs isn't really tested. A couple of smaller notes too.
d62d484 to
dab61df
Compare
346d1b4 to
3504a1b
Compare
|
Hey! A few CI checks were failing, so I pushed a patch to address them. |
SQLite backfill creation via the REST API could return HTTP 500 and leave an orphan Backfill row when the scheduler and API server contend for the database, blocking retries with 409. Map lock errors to HTTP 503 with a clear message and clean up partial DagRuns and TaskInstances so users can retry against a clean slate. Signed-off-by: Lohit Kolluri <lohitkolluri@gmail.com>
b99435e to
9354632
Compare
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
Backport successfully created: v3-3-testNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
|
#67900) (#69659) SQLite backfill creation via the REST API could return HTTP 500 and leave an orphan Backfill row when the scheduler and API server contend for the database, blocking retries with 409. Map lock errors to HTTP 503 with a clear message and clean up partial DagRuns and TaskInstances so users can retry against a clean slate. (cherry picked from commit 2fb2321) Signed-off-by: Lohit Kolluri <lohitkolluri@gmail.com> Co-authored-by: Lohit Kolluri <lohitkolluri@gmail.com>
#67900) (#69659) SQLite backfill creation via the REST API could return HTTP 500 and leave an orphan Backfill row when the scheduler and API server contend for the database, blocking retries with 409. Map lock errors to HTTP 503 with a clear message and clean up partial DagRuns and TaskInstances so users can retry against a clean slate. (cherry picked from commit 2fb2321) Signed-off-by: Lohit Kolluri <lohitkolluri@gmail.com> Co-authored-by: Lohit Kolluri <lohitkolluri@gmail.com>
Closes: #66726
On SQLite, creating a backfill through the API can fail with a "database is locked" error when the scheduler, dag-processor, and API server all share the same database.
Before this PR, this caused two problems:
After this PR:
Changes
Create route — When the database is locked, return 503 with a clear message instead of letting the error bubble up as a 500.
Dry-run route — Same 503 handling, so previewing a backfill doesn't return a 500 either.
Cleanup of partial state — If the lock happens partway through creating a backfill, the half-created backfill is removed so the user can retry:
SQLite detection — Teach the lock checker to recognize SQLite's "database is locked" message, since SQLite uses a human-readable string instead of a numeric error code like Postgres or MySQL.
OpenAPI / TypeScript — The 503 response is documented in the API spec and the generated UI client.
Tests
test_create_backfill_database_locked— checks the 503 mapping works for the create routetest_create_backfill_dry_run_database_locked— same check for the dry-run routetest_create_backfill_cleans_up_orphan_on_lock_error— checks the cleanup removes the half-created backfill from a real partial statetest_create_backfill_cleans_up_after_failed_transaction— checks the cleanup works when SQLAlchemy has already marked the session as needing a rollbacktest_create_backfill_cleanup_removes_partial_dag_runs— checks partial DagRuns, BackfillDagRun rows, and TaskInstances are all removedWas generative AI tooling used to co-author this PR?