address review feedback: don't let a rollback failure escape retry contract

get_query's backoff decorator only retries on SqlLabException. If
db.session.rollback() itself raises (e.g. the connection is fully
dead), that new exception would replace the intended SqlLabException
and bypass the retry contract. Swallow rollback failures so the
original lookup error is always what gets raised.

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Elizabeth Thompson
2026-08-02 18:18:31 +00:00
parent 7d5e8ad785
commit 09c9410bdc
2 changed files with 33 additions and 2 deletions

View File

@@ -37,6 +37,7 @@ from superset.sql_lab import (
execute_sql_statements,
get_query,
get_sql_results,
SqlLabException,
)
from superset.utils.rls import apply_rls, get_predicates_for_table
from tests.conftest import with_config
@@ -100,6 +101,30 @@ def test_get_query_rolls_back_session_before_retrying(
mock_rollback.assert_called_once()
def test_get_query_swallows_rollback_failure(
mocker: MockerFixture, app: SupersetApp
) -> None:
"""
If the session/connection is too broken for `rollback()` itself to succeed,
that failure must not replace the original lookup error: `get_query` still
needs to raise `SqlLabException` so the `backoff` decorator's retry contract
(which only matches on `SqlLabException`) isn't bypassed.
"""
mocker.patch("backoff._sync.time.sleep")
mock_one = mocker.patch("superset.sql_lab.db.session.query")
mock_one.return_value.filter_by.return_value.one.side_effect = Exception(
"session is broken"
)
mocker.patch(
"superset.sql_lab.db.session.rollback",
side_effect=Exception("connection already closed"),
)
with pytest.raises(SqlLabException):
get_query(query_id=1)
@with_config(
{
"SQLLAB_PAYLOAD_MAX_MB": 50,