[PR] Fix database connection leaks in resetdb functions [airflow]

["magic-peach (via GitHub)" <gi...@apache.org>]

magic-peach opened a new pull request, #70011:
URL: https://github.com/apache/airflow/pull/70011

   ## Summary
   
   Two database connection leaks in the resetdb code path:
   
   ### 1. `_resetdb_default` (`airflow-core/src/airflow/utils/db.py:1328`)
   
   ```python
   connection = settings.get_engine().connect()
   with create_global_lock(session=session, lock=DBLocks.MIGRATIONS), connection.begin():
       ...
   ```
   
   A new database connection is opened but never closed. There is no `try/finally` with `connection.close()`, and the connection is not used as a context manager. Compare with the MySQL variant (`_resetdb_mysql`) at line 1307 which correctly uses `engine.connect() as connection`.
   
   ### 2. `BaseDBManager.resetdb` (`airflow-core/src/airflow/utils/db_manager.py:189`)
   
   ```python
   connection = settings.engine.connect()
   with create_global_lock(self.session, lock=DBLocks.MIGRATIONS), connection.begin():
       ...
   ```
   
   Identical pattern. The `with` statement manages a transaction (via `connection.begin()`) and a lock, but does not close the connection itself. After the `with` block exits, the connection remains open and the reference is lost.
   
   ### Fix
   
   Changed both functions to use `engine.connect() as connection` (context manager form), matching the pattern already used in `_resetdb_mysql`. This ensures connections are properly closed after use.
   
   ---
   
   ##### Was generative AI tooling used to co-author this PR?
   
   - [X] Yes — Claude Code (big-pickle)
   
   Generated-by: Claude Code (big-pickle) following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)


-- 
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

Re: [PR] Fix database connection leaks in resetdb functions [airflow]

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

potiuk closed pull request #70011: Fix database connection leaks in resetdb functions
URL: https://github.com/apache/airflow/pull/70011


-- 
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

Re: [PR] Fix database connection leaks in resetdb functions [airflow]

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

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: commits-unsubscribe@airflow.apache.org

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