potiuk commented on PR #70011:
URL: https://github.com/apache/airflow/pull/70011#issuecomment-5114953933
Thanks for digging into this — the observation underneath it is correct:
`connection = settings.get_engine().connect()` is never closed, and the
connection only returns to the pool when the fairy is finalized rather than at
end of use.
Unfortunately the fix as written is a regression, so I'm closing it rather
than merging.
Replacing `connection.begin()` with `with engine.connect() as connection:`
doesn't just add cleanup — it changes the transaction semantics.
`connection.begin()` as a context manager **commits** on clean exit.
`engine.connect()` is SQLAlchemy 2.0 "commit as you go": nothing calls
`.commit()`, so `Connection.close()` **rolls back** on block exit.
`drop_airflow_models()` and `drop_tables()` don't commit internally, so the
DROPs get reverted.
Against a real PostgreSQL 16 with SQLAlchemy 2.0.51:
```
current: connection.begin() -> table SURVIVED the drop: False
this PR: engine.connect() as conn -> table SURVIVED the drop: True
```
`_resetdb_default` is the default path for both PostgreSQL and SQLite, so on
Postgres this makes `airflow db reset` silently stop dropping anything. Both
hunks have it. It happens to be masked on SQLite because pysqlite doesn't open
a transaction for DDL, so DDL autocommits there — Postgres has real
transactional DDL, so it breaks.
On `_resetdb_mysql` as the precedent: that function is only safe with
`engine.connect() as connection` because the next line switches to
`connection.execution_options(isolation_level="AUTOCOMMIT")` and runs all DDL
on `ddl_conn`. The AUTOCOMMIT is what makes the pattern work there; without it
the same bug applies.
Green CI isn't evidence here either. The only test that actually calls
`resetdb()` is `test_resetdb_logging_level`, which asserts on the logging level
and nothing else, and `resetdb()` calls `initdb()` right afterwards — so the
tables come back and a failed drop is invisible. Nothing asserts the tables are
gone.
If you'd like to pursue the original cleanup, the version that keeps both
properties is:
```python
with (
create_global_lock(session=session, lock=DBLocks.MIGRATIONS),
settings.get_engine().connect() as connection,
connection.begin(),
):
```
or `engine.begin() as connection`, which is connect + begin + commit + close
in one. A new PR doing that, with a test that asserts the tables are actually
dropped, would be welcome. Worth knowing that the resource issue itself is
minor — `resetdb` is a one-shot CLI operation — so the test matters more than
the cleanup.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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]
