Re: [PR] fix(backfill): Handle SQLite lock errors with atomic creation and graceful 503 [airflow]

["23tae (via GitHub)" <gi...@apache.org>]

23tae commented on code in PR #67900:
URL: https://github.com/apache/airflow/pull/67900#discussion_r3349369321


##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/backfills.py:
##########
@@ -269,6 +284,14 @@ def create_backfill(
             status_code=status.HTTP_400_BAD_REQUEST,
             detail=str(e),
         )
+    except OperationalError as e:
+        if "database is locked" in str(e):
+            raise HTTPException(
+                status_code=status.HTTP_503_SERVICE_UNAVAILABLE,
+                detail="Backfill creation failed because the database is locked. "
+                "This is a transient error — please retry the request.",
+            )
+        raise

Review Comment:
   This looks duplicated. Since the first `except OperationalError` already catches this exception type, the later handler appears unreachable. Could we consolidate this into a single handler?



##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_backfills.py:
##########
@@ -352,6 +353,34 @@ def test_create_backfill_with_depends_on_past(
                     == "Dag has tasks for which depends_on_past=True. You must set reprocess behavior to reprocess completed or reprocess failed."
                 )
 
+    def test_create_backfill_database_locked(self, session, dag_maker, test_client):
+        """Regression test for issue #66726: SQLite 'database is locked' returns 503."""
+        with dag_maker(session=session, dag_id="TEST_DAG_1", schedule="0 * * * *") as dag:
+            EmptyOperator(task_id="mytask")
+        session.scalars(select(DagModel)).all()
+        session.commit()
+
+        from_date = pendulum.parse("2024-01-01")
+        to_date = pendulum.parse("2024-02-01")
+
+        data = {
+            "dag_id": dag.dag_id,
+            "from_date": to_iso(from_date),
+            "to_date": to_iso(to_date),
+            "max_active_runs": 5,
+            "run_backwards": False,
+            "dag_run_conf": {},
+        }
+
+        with mock.patch(
+            "airflow.api_fastapi.core_api.routes.public.backfills._create_backfill",
+            side_effect=OperationalError("statement", "params", "database is locked"),

Review Comment:
   I ran this test locally and it fails — the `OperationalError` propagates unhandled instead of returning 503.
   
   The mock passes `"database is locked"` as a plain string for `e.orig`, but the implementation checks `e.orig.args[0]`. Since strings don't have an `args` attribute, the condition is never met and the exception is re-raised as-is.
   
   To fix this, either:
   1. Use a real exception object in the mock: `sqlite3.OperationalError("database is locked")`
   2. Or simplify the implementation to check `"database is locked" in str(e)` instead



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org