sadpandajoe opened a new pull request, #45861: URL: https://github.com/apache/superset/pull/45861
### SUMMARY Databases that allow only one configuration per database file per process, such as DuckDB, reject opening a file with different settings while any connection to it is still open in that process (`Connection Error: Can't open a connection to same database file with a different configuration than existing connections`). After a connection's settings are edited (for example a rotated access token), Superset could keep a connection with the old settings open in two ways, so the edited settings failed to open until the process released it: - `Database.has_view` opened a connection with `engine.connect()` and never released it. With the default `NullPool` a connection is only closed once it is released, so the database stayed open with its previous settings until the cyclic garbage collector reclaimed the leaked connection. Disposing or evicting the engine does not help, because the connection is checked out rather than pooled. `has_view` is reached from physical-table metadata discovery when `has_table` is false. - `_evict_engine_cache` dropped a database's cached engines on update or delete without disposing them, so an engine whose pool held an idle connection (for example when `DB_CONNECTION_MUTATOR` swaps in a pooled `poolclass`) kept that connection open after the edit. This PR: - releases the `has_view` connection with a context manager (same `get_view_names` call, same exception fallback, same return value); - disposes the engines evicted by `_evict_engine_cache`, after the cache lock is released. A failure to dispose is logged with a static message and never interrupts the update or delete. #### Root Cause Two independent ways a process leaves a connection with the old configuration open, reproduced against a real file-backed DuckDB database (DuckDB 1.5.6, duckdb-engine 0.17.0, SQLAlchemy 2.0.55): 1. `has_view` leaked its connection (default `NullPool`, no mutator needed). 2. Eviction on update never disposed pooled engines. #### Limits (stated plainly) This change does not claim to resolve every way these errors can appear: - eviction is per process, so engines pooled in other worker processes are not affected; - `Engine.dispose()` closes idle pooled connections; connections checked out at that moment are not released deterministically; - validating new settings while a saved pooled engine still holds the old ones (the pre-save validation / Test Connection path) is not addressed; - long-running statements that hold an old-configuration connection, and database-side instance lifetime, are not addressed. A cross-worker retirement of stale engines and disposal of unsaved-database engines are possible follow-ups; they need their own design (a settings generation that is ordered with the commit). ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF Not applicable (backend only). Regression guards, RED on the parent commit and GREEN with the fix: - `test_has_view_closes_its_connection` - `test_engine_evicted_on_update_releases_pooled_connection` Both fail on the parent commit with `sqlalchemy.exc.OperationalError: (_duckdb.ConnectionException) Connection Error: Can't open a connection to same database file with a different configuration than existing connections` and pass with the fix. `test_engine_cache_eviction_survives_dispose_failure` covers the dispose-failure handling. ### TESTING INSTRUCTIONS ``` pytest tests/unit_tests/models/core_test.py -p no:cacheprovider ``` The two regression guards alone: `pytest tests/unit_tests/models/core_test.py -k "pooled_connection or has_view_closes" -p no:cacheprovider`. ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API No new dependency, no migration, no change to the cache key or the `nullpool` default. For engines using `NullPool`, disposal is a no-op. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
