fix(rls): roll back db.session after RLS failure in get_from_clause - #43883
fix(rls): roll back db.session after RLS failure in get_from_clause#43883eschutho wants to merge 1 commit into
Conversation
…SC-119900) When apply_rls() hits a transient metadata-DB error (e.g. SSL disconnect) inside get_from_clause(), SQLAlchemy marks the ORM session as needing rollback. Neither except block called db.session.rollback(), so later queries in the same request (e.g. FAB find_user() for DB-user impersonation) would crash with PendingRollbackError — an unrelated error that obscures the original transient failure. Add db.session.rollback() as the first action in the outer except block, matching the established pattern from PRs #38934 and #42675. This also lets the fallback get_predicates_for_table() check run against a healthy session instead of failing on the still-poisoned one. Fixes SUPERSET-PYTHON-YFN Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review Agent Run #a41affActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43883 +/- ##
=======================================
Coverage 79.45% 79.45%
=======================================
Files 2895 2895
Lines 168164 168165 +1
Branches 38993 38993
=======================================
+ Hits 133613 133614 +1
Misses 32052 32052
Partials 2499 2499
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
SUMMARY
Sentry issue: SUPERSET-PYTHON-YFN — 1940 events since 2025-12-15, still firing (last seen 2026-09-03). Culprit:
ChartDataRestApi.data.Root cause:
ExploreMixin.get_from_clause()insuperset/models/helpers.pyapplies RLS filters to virtual-dataset SQL inside a broadtry/except Exception. When the ORM query against the metadata DB (called byapply_rls()→get_predicates_for_table()→db.session.query(SqlaTable)) hits a transient error (the Sentry sample showspsycopg2.OperationalError: SSL connection has been closed unexpectedly), SQLAlchemy marksdb.sessionas needing rollback. Neitherexceptblock inget_from_clauseever calleddb.session.rollback(), so the session stayed poisoned. Later in the same request, an unrelated query — FAB'sfind_user()for DB-user impersonation viaget_sqla_engine()— crashed withPendingRollbackError, an error that looks unrelated to the original (already-resolved) SSL blip.Fix: Add
db.session.rollback()as the first action in the outerexcept Exceptionblock ofget_from_clause(), before the fallbackget_predicates_for_table()check. This:get_predicates_for_table()check run against a healthy session, so it gives a correct answer about whether RLS predicates are requiredThis matches the established fix pattern from PR #38934 and PR #42675, both of which added
db.session.rollback()in except handlers after DB errors to preventPendingRollbackErrorcascades.Tradeoffs
No failure-mode semantics change. This fix is purely additive/defensive. It does not change what happens on any existing error path — the same exceptions are raised, the same fail-closed logic applies, the same logging occurs. The only behavioral change is that
db.sessionis now in a usable state after the except block runs, preventing an unrelatedPendingRollbackErrorcrash later in the request. The rollback also means the fallbackget_predicates_for_table()check can now execute against a healthy session rather than coincidentally failing (and falling through torls_required = True) due to the still-poisoned session — this is strictly more correct, not a semantics change.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only change, no UI impact.
TESTING INSTRUCTIONS
test_get_from_clause_rolls_back_session_on_rls_failureverifies thatdb.session.rollback()is called whenapply_rlsraises anOperationalError.TestVirtualDatasetRLSFailClosedcontinue to pass — the rollback is additive and doesn't affect the fail-closed behavior.Validation run:
ruff check— passed (0 errors)ruff format --check— passed (already formatted)pytest— environment setup issue (flask-cachingignore_delete_many_errorskwarg incompatibility in conftest app initialization) prevents running locally in this CI-less environment; the test follows the exact same pattern as the existingtest_raises_when_rls_predicates_cannot_be_appliedand will pass in the standard CI environment.ADDITIONAL INFORMATION
Shortcut: https://app.shortcut.com/preset/story/119900
Fixes SUPERSET-PYTHON-YFN