fix(db): use a private engine for prequery connections to avoid listener race

[mikebridge]

SUMMARY

Fixes an intermittent RuntimeError: deque mutated during iteration (surfacing as HTTP 500s and E2E flakiness) caused by an interaction between two changes:

• fix(sqllab): execute prequeries on streaming connection to fix PostgreSQL CSV export #40194 made Database.get_sqla_engine attach a per-call SQLAlchemy connect listener when prequeries exist (e.g. SET search_path on PostgreSQL), removing it again on context exit — two mutations of the engine's connect-listener deque per call.
• test(database): regression test for sqla engine creation (#27897) #40237 introduced _ENGINE_CACHE, making Engine objects shared across threads per (database_id, url, engine_kwargs).

SQLAlchemy stores event listeners in a plain, unlocked collections.deque, and dispatch iterates it unguarded (for fn in self.listeners). With NullPool, every raw_connection() creates a fresh DBAPI connection and fires that dispatch. So one thread's connection checkout can iterate the shared engine's deque while another thread's get_sqla_engine enter/exit mutates it — a classic race that reproduces readily under concurrent load (observed as consistent create-dataset 500s on one Playwright run and flaked-but-recovered failures on another).

Fix: compute prequeries before fetching the engine, and request a private, uncached engine (cacheable=False) whenever prequeries are present. The per-call listener add/remove then only ever touches an engine no other thread can see. No-prequery paths keep the shared cache and its #27897 semantics. The listener also closes over per-call, schema-specific prequeries and must not leak to other requests sharing a cached engine — which is why a register-once approach was rejected.

This restores the pre-#40237 behaviour for prequery engines only (fresh engine per call); prequery-less databases keep full engine caching. Private engines are explicitly dispose()d on context exit — a no-op under the default nullpool=True, and a pool-leak guard if a caller ever passes nullpool=False.

Independent validation: the diagnosis and fix were adversarially reviewed by a second model (verdict: merge as-is), which independently reproduced the exact RuntimeError with standalone SQLAlchemy-only code; both of its hardening suggestions (the dispose() above and a deterministic regression test below) are applied. Details: #41642 (comment)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A (backend concurrency fix)

TESTING INSTRUCTIONS

Unit tests in tests/unit_tests/models/core_test.py:

• test_prequery_engine_bypasses_shared_cache — deterministic regression: with prequeries the yielded engine is never the cached instance and _ENGINE_CACHE is untouched; without prequeries caching behaviour is unchanged.
• test_prequeries_execute_on_real_connections — behavioural guard for fix(sqllab): execute prequeries on streaming connection to fix PostgreSQL CSV export #40194: prequeries still execute on every new DBAPI connection (file-backed SQLite, table-creating prequery, observed on the same connection).
• test_prequery_listener_mutation_race_deterministic — deterministic regression (no timing dependence): a patched engine creator parks one thread provably mid-connect-dispatch (inside the prequery listener's first cursor() call) while a second thread enters the prequery path and mutates the listener collection, then releases the first. A warm-up connection first consumes the dialect's one-time first_connect isolation probe, which would otherwise hold the engine's once-mutex and deadlock the sibling thread instead of exercising the race. On pre-fix code this fails every run (6/6 observed) with the exact deque mutated during iteration error in under a second; with the fix it passes standalone and in-suite.
• test_concurrent_prequery_connections_do_not_race — stress companion: 4 threads × 50 get_raw_connection calls with prequeries. Against the previous code this reproduced the production error readily (probabilistic by nature — the deterministic test above is the authoritative pin); with this fix it passes.

pytest tests/unit_tests/models/core_test.py -q

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • 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

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.55%. Comparing base (393adc4) to head (22b5fae).
⚠️ Report is 50 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #41642      +/-   ##
==========================================
+ Coverage   64.52%   64.55%   +0.03%     
==========================================
  Files        2673     2684      +11     
  Lines      147639   148274     +635     
  Branches    34093    34153      +60     
==========================================
+ Hits        95260    95725     +465     
- Misses      50650    50804     +154     
- Partials     1729     1745      +16     

Flag	Coverage Δ
hive	39.23% <33.33%> (+0.16%)	⬆️
mysql	57.83% <100.00%> (+0.15%)	⬆️
postgres	57.89% <100.00%> (+0.14%)	⬆️
presto	40.76% <33.33%> (+0.15%)	⬆️
python	59.28% <100.00%> (+0.13%)	⬆️
sqlite	57.47% <100.00%> (+0.14%)	⬆️
unit	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	22b5fae
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6a46e2ba2cc5400008246826
😎 Deploy Preview	https://deploy-preview-41642--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[mikebridge]

Independent validation summary

Since a concurrency claim is hard to review by inspection, we ran this PR through an independent second-model review (OpenAI Codex, adversarial brief: verify both the diagnosis and the fix; say plainly if either is wrong). Summary of its findings:

Diagnosis confirmed — including an independent reproduction. It traced the same mechanism (shared _ENGINE_CACHE engine → per-call event.listen/event.remove → SQLAlchemy 1.4.54's listener deque iterated during dispatch and mutated on remove, sqlalchemy/event/attr.py), noted SQLAlchemy's own event.remove() docs forbid removal during dispatch, and reproduced the exact RuntimeError: deque mutated during iteration with standalone SQLAlchemy-only code. It also confirmed the second failure mode: two threads' prequeries (e.g. different SET search_path) both dispatch on each new connection of the shared engine, last listener winning — wrong-results, not just crashes.

Fix confirmed sound and complete. It searched for other attach/remove paths on cached engines and found none; non-prequery databases keep caching exactly as before. Verdict: "Merge as-is."

Both of its improvement suggestions are now applied (22b5fae293):

1. engine.dispose() for private prequery engines in the finally — a no-op safety net under the default nullpool=True, load-bearing if a caller ever passes nullpool=False (otherwise each private engine would keep a short-lived pool alive until GC).
2. A deterministic race regression test to complement the timing-based 4-thread stress test. A patched engine creator parks thread A provably mid-connect-dispatch (inside the prequery listener's first cursor() call) while thread B enters the prequery path and mutates the listener collection, then releases A — pure event ordering, no timing dependence. One subtlety, found via faulthandler stack dumps: the park must be armed only after a warm-up connection consumes the dialect's one-time first_connect isolation probe, which otherwise holds the engine's once-mutex and deadlocks the sibling thread instead of exercising the race. Validated bidirectionally: on pre-fix code the test fails every run (6/6) with the exact deque error in under a second; on this branch it passes standalone and in-suite (70/70).

Between the original stress repro, the second model's standalone reproduction, and the deterministic test, the diagnosis and fix are validated three independent ways.

[mikebridge]

cc @fitzee and @rusackas as the authors of the two PRs whose interaction produces this race — to be clear, neither change is at fault on its own:

• fix(sqllab): execute prequeries on streaming connection to fix PostgreSQL CSV export #40194 (@fitzee) added the per-call prequery connect listener with a matching remove() in a finally — exactly right at the time, when engines were created fresh per call, and the remove is what keeps one request's SET search_path from leaking to others.
• test(database): regression test for sqla engine creation (#27897) #40237 (@rusackas) added _ENGINE_CACHE per sqlalchemy engine should be created once per process #27897 — a legitimate perf win that only later armed the listener add/remove into a cross-thread mutation of a shared engine's listener collection.

This fix keeps both behaviors intact: prequeries still execute on every new DBAPI connection (behavioral guard test included), and non-prequery databases keep full engine caching. Only prequery-bearing calls opt out of the cache, restoring their pre-#40237 private-engine behavior.

Reviews from either of you would be especially valuable since you know these two code paths best — the validation summary above (independent second-model review + deterministic regression test) should make the concurrency claim easy to check without re-deriving it.

[rusackas]

Really nice work @mikebridge, LGTM. The diagnosis holds up... a shared _ENGINE_CACHE engine plus per-call listener add/remove on an unlocked deque is exactly the race, and gating the cache with cacheable=False for prequery engines is the right fix (keeps the #27897 caching everywhere else, restores fresh-per-call only where the listener needs to mutate). The deterministic parking-harness test is a thing of beauty, reproduces the deque mutated during iteration crash on the old code and passes clean on the new. Since #40237 was mine, extra glad to see it pinned down properly. Approving, CI is green.

[bito-code-review]

Bito Automatic Review Skipped – PR Already Merged

Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.
No action is needed if you didn't intend to review it. To get a review, you can type /review in a comment and save it