pierrejeambrun commented on code in PR #67900:
URL: https://github.com/apache/airflow/pull/67900#discussion_r3506466191
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_backfills.py:
##########
@@ -353,6 +354,135 @@ 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):
+ """SQLite 'database is locked' during backfill creation returns HTTP
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:
When I run these two `...database_locked` tests locally they fail: the mock
passes a plain string as the DBAPI `orig`, but SQLAlchemy wraps an *exception*,
so `is_lock_not_available_error` hits `error.orig.args[0]` and raises
`AttributeError: 'str' object has no attribute 'args'` before it can classify
the lock — the endpoint then 500s instead of 503. I think this is the same
failure raised earlier in review that got marked resolved. Could you pass a
real `orig`? With this it passes and I confirmed the route returns 503:
```suggestion
side_effect=OperationalError("statement", "params",
sqlite3.OperationalError("database is locked")),
```
(needs `import sqlite3`). Same fix applies to the dry-run test just below
(line ~410).
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_backfills.py:
##########
@@ -353,6 +354,135 @@ 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):
+ """SQLite 'database is locked' during backfill creation returns HTTP
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"),
+ ):
+ response = test_client.post("/backfills", json=data)
+
+ # OperationalError with "database is locked" should return 503
+ assert response.status_code == 503
+ assert "database is locked" in response.json()["detail"].lower()
+
+ class TestCreateBackfillDryRun:
+ """Tests for the create_backfill dry_run endpoint."""
+
+ def test_create_backfill_dry_run_database_locked(self, session,
dag_maker, test_client):
+ """SQLite 'database is locked' during dry-run 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._do_dry_run",
+ side_effect=OperationalError("statement", "params", "database
is locked"),
+ ):
+ response = test_client.post("/backfills/dry_run", json=data)
+
+ # OperationalError with "database is locked" should return 503
+ assert response.status_code == 503
+ assert "database is locked" in response.json()["detail"].lower()
+
+ def test_create_backfill_cleans_up_orphan_on_lock_error(self, session,
dag_maker):
Review Comment:
This is the only cleanup test, and it removes a bare `Backfill` row with no
runs — so the actual bug ("removes partial dag-run rows") isn't exercised. In
the real flow those partial DagRuns also carry TaskInstances (I checked a
repro: 2 TIs on the first backfill run), and the cleanup depends on `dag_run`'s
`ON DELETE CASCADE` firing. Could we add a case that creates a couple of
partial DagRuns and asserts both they and their TIs are gone after
`_cleanup_partial_backfill`? That's what proves the fix on the backend under
test.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
