From 0debc29ef76db3c8a5b4dae9d084fdc235f3fcb7 Mon Sep 17 00:00:00 2001 From: Josh Markovic Date: Mon, 29 Jun 2026 19:34:07 +0000 Subject: [PATCH 1/3] fix: honor configured query retries and stop retry-event crash The query-level retry in SQLServerConnectionManager.add_query had two defects: - A `credentials.retries > 3` guard silently ignored configured retry counts of 1-3 and fell back to a hardcoded limit of 2, so even the default `retries: 3` only attempted a query twice. Use `credentials.retries` directly, matching how open() drives connection retries. - The retry-debug event was built as `AdapterEventDebug(message=...)`, but the event field is `base_msg`; constructing it raised a protobuf ParseError on the first retryable error, so retries crashed instead of retrying. Use `base_msg=` as dbt-core's base add_query does. Add unit tests covering retry-until-success, the retries=3 regression, no-retry at retries=1, and non-retryable errors not being retried. --- .../sqlserver/sqlserver_connections.py | 14 +- .../test_sqlserver_connection_manager.py | 138 ++++++++++++++++++ 2 files changed, 150 insertions(+), 2 deletions(-) diff --git a/dbt/adapters/sqlserver/sqlserver_connections.py b/dbt/adapters/sqlserver/sqlserver_connections.py index 3291d6ca..057f6064 100644 --- a/dbt/adapters/sqlserver/sqlserver_connections.py +++ b/dbt/adapters/sqlserver/sqlserver_connections.py @@ -224,8 +224,12 @@ def _execute_query_with_retry( raise e fire_event( + # NB: the field is ``base_msg`` (as in dbt-core's base + # ``add_query``); ``message=`` raises a protobuf ParseError + # at event construction, which previously crashed the first + # retry instead of retrying. AdapterEventDebug( - message=( + base_msg=( f"Got a retryable error {type(e)}. {retry_limit - attempt} " "retries left. Retrying in 1 second.\n" f"Error:\n{e}" @@ -277,7 +281,13 @@ def _execute_query_with_retry( sql=sql, bindings=bindings, retryable_exceptions=retryable_exceptions, - retry_limit=(credentials.retries if credentials.retries > 3 else retry_limit), + # The connection's configured ``retries`` is authoritative for + # query retries, mirroring ``open()`` which passes + # ``credentials.retries`` to ``retry_connection``. A previous + # ``credentials.retries > 3`` guard silently ignored configured + # values of 1-3 and used the hardcoded ``retry_limit`` of 2, so + # even the default ``retries: 3`` only attempted a query twice. + retry_limit=credentials.retries, attempt=1, ) diff --git a/tests/unit/adapters/mssql/test_sqlserver_connection_manager.py b/tests/unit/adapters/mssql/test_sqlserver_connection_manager.py index fb61e875..da3ed3e8 100644 --- a/tests/unit/adapters/mssql/test_sqlserver_connection_manager.py +++ b/tests/unit/adapters/mssql/test_sqlserver_connection_manager.py @@ -1,5 +1,6 @@ import builtins import importlib +from contextlib import contextmanager from types import SimpleNamespace from typing import Any, Dict, List from unittest.mock import MagicMock, patch @@ -1594,3 +1595,140 @@ def test_rollback_handle_disabled_exception_fires_rollback_failed( SQLServerConnectionManager._rollback_handle(connection) mock_fire_event.assert_called_once() + + +class _FakeRetryableError(Exception): + """Stand-in for a backend retryable exception in add_query retry tests.""" + + +def _make_add_query_manager( + monkeypatch: pytest.MonkeyPatch, + *, + retries: int, + execute_side_effect: Any, +): + """Build a manager + thread connection wired for add_query retry tests. + + Uses ``object.__new__`` (the pattern used elsewhere in this module) so no + real connection pool is constructed; only the collaborators add_query + touches are stubbed. + """ + + manager = object.__new__(SQLServerConnectionManager) + + cursor = MagicMock() + cursor.execute.side_effect = execute_side_effect + cursor.rowcount = 0 + + handle = MagicMock() + handle.cursor.return_value = cursor + + credentials = MagicMock() + credentials.retries = retries + + connection = MagicMock() + connection.handle = handle + connection.credentials = credentials + connection.transaction_open = True + connection.name = "retry-test" + + monkeypatch.setattr(manager, "get_thread_connection", lambda: connection) + + # Isolate the retry loop from error translation / connection release, which + # have their own tests and otherwise depend on global runtime state. + @contextmanager + def _passthrough_exception_handler(_sql): + yield + + monkeypatch.setattr(manager, "exception_handler", _passthrough_exception_handler) + + return manager, connection, cursor + + +def test_add_query_retries_retryable_errors_until_success( + monkeypatch: pytest.MonkeyPatch, +) -> None: + manager, _connection, cursor = _make_add_query_manager( + monkeypatch, + retries=3, + execute_side_effect=[_FakeRetryableError(), _FakeRetryableError(), None], + ) + + with ( + patch("dbt.adapters.sqlserver.sqlserver_connections.fire_event"), + patch("dbt.adapters.sqlserver.sqlserver_connections.time.sleep") as mock_sleep, + ): + _conn, result_cursor = manager.add_query( + "select 1", auto_begin=False, retryable_exceptions=(_FakeRetryableError,) + ) + + assert cursor.execute.call_count == 3 + assert result_cursor is cursor + assert mock_sleep.call_count == 2 + + +def test_add_query_honors_configured_retries_over_method_default( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # Regression guard: a `credentials.retries > 3` branch used to ignore + # configured values of 1-3 and fall back to a hardcoded limit of 2, so the + # default `retries: 3` attempted a query only twice. It must now attempt + # the query three times before giving up. + manager, _connection, cursor = _make_add_query_manager( + monkeypatch, + retries=3, + execute_side_effect=_FakeRetryableError(), + ) + + with ( + patch("dbt.adapters.sqlserver.sqlserver_connections.fire_event"), + patch("dbt.adapters.sqlserver.sqlserver_connections.time.sleep"), + ): + with pytest.raises(_FakeRetryableError): + manager.add_query( + "select 1", auto_begin=False, retryable_exceptions=(_FakeRetryableError,) + ) + + assert cursor.execute.call_count == 3 + + +def test_add_query_does_not_retry_when_retries_is_one( + monkeypatch: pytest.MonkeyPatch, +) -> None: + manager, _connection, cursor = _make_add_query_manager( + monkeypatch, + retries=1, + execute_side_effect=_FakeRetryableError(), + ) + + with ( + patch("dbt.adapters.sqlserver.sqlserver_connections.fire_event"), + patch("dbt.adapters.sqlserver.sqlserver_connections.time.sleep"), + ): + with pytest.raises(_FakeRetryableError): + manager.add_query( + "select 1", auto_begin=False, retryable_exceptions=(_FakeRetryableError,) + ) + + assert cursor.execute.call_count == 1 + + +def test_add_query_does_not_retry_non_retryable_errors( + monkeypatch: pytest.MonkeyPatch, +) -> None: + manager, _connection, cursor = _make_add_query_manager( + monkeypatch, + retries=3, + execute_side_effect=ValueError("not retryable"), + ) + + with ( + patch("dbt.adapters.sqlserver.sqlserver_connections.fire_event"), + patch("dbt.adapters.sqlserver.sqlserver_connections.time.sleep"), + ): + with pytest.raises(ValueError): + manager.add_query( + "select 1", auto_begin=False, retryable_exceptions=(_FakeRetryableError,) + ) + + assert cursor.execute.call_count == 1 From 9af0bfd2f7be1d1696437934e453e4229e5c06c1 Mon Sep 17 00:00:00 2001 From: Josh Markovic Date: Thu, 2 Jul 2026 17:37:42 +0000 Subject: [PATCH 2/3] docs: keep only reader-facing retry comments Drop the event-construction comment narrating the old message= defect and reduce the retry_limit comment to the one fact a reader needs: retries caps total execute attempts, so retries: 1 disables retry. --- dbt/adapters/sqlserver/sqlserver_connections.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/dbt/adapters/sqlserver/sqlserver_connections.py b/dbt/adapters/sqlserver/sqlserver_connections.py index 057f6064..ad50e3b4 100644 --- a/dbt/adapters/sqlserver/sqlserver_connections.py +++ b/dbt/adapters/sqlserver/sqlserver_connections.py @@ -224,10 +224,6 @@ def _execute_query_with_retry( raise e fire_event( - # NB: the field is ``base_msg`` (as in dbt-core's base - # ``add_query``); ``message=`` raises a protobuf ParseError - # at event construction, which previously crashed the first - # retry instead of retrying. AdapterEventDebug( base_msg=( f"Got a retryable error {type(e)}. {retry_limit - attempt} " @@ -281,12 +277,8 @@ def _execute_query_with_retry( sql=sql, bindings=bindings, retryable_exceptions=retryable_exceptions, - # The connection's configured ``retries`` is authoritative for - # query retries, mirroring ``open()`` which passes - # ``credentials.retries`` to ``retry_connection``. A previous - # ``credentials.retries > 3`` guard silently ignored configured - # values of 1-3 and used the hardcoded ``retry_limit`` of 2, so - # even the default ``retries: 3`` only attempted a query twice. + # ``retries`` caps total execute attempts, so ``retries: 1`` + # means a single attempt with no retry. retry_limit=credentials.retries, attempt=1, ) From 94dd8a09061eb352df677802680efff809df5000 Mon Sep 17 00:00:00 2001 From: Josh Markovic Date: Thu, 2 Jul 2026 17:37:42 +0000 Subject: [PATCH 3/3] test: parametrize add_query retry failure cases The three failure-path tests differed only in retries, side effect, expected exception and expected attempt count; collapse them into one parametrized test. Also drop the unused connection element from the test helper's return value. --- .../test_sqlserver_connection_manager.py | 95 ++++++++----------- 1 file changed, 41 insertions(+), 54 deletions(-) diff --git a/tests/unit/adapters/mssql/test_sqlserver_connection_manager.py b/tests/unit/adapters/mssql/test_sqlserver_connection_manager.py index da3ed3e8..186f24d6 100644 --- a/tests/unit/adapters/mssql/test_sqlserver_connection_manager.py +++ b/tests/unit/adapters/mssql/test_sqlserver_connection_manager.py @@ -1642,13 +1642,13 @@ def _passthrough_exception_handler(_sql): monkeypatch.setattr(manager, "exception_handler", _passthrough_exception_handler) - return manager, connection, cursor + return manager, cursor def test_add_query_retries_retryable_errors_until_success( monkeypatch: pytest.MonkeyPatch, ) -> None: - manager, _connection, cursor = _make_add_query_manager( + manager, cursor = _make_add_query_manager( monkeypatch, retries=3, execute_side_effect=[_FakeRetryableError(), _FakeRetryableError(), None], @@ -1667,68 +1667,55 @@ def test_add_query_retries_retryable_errors_until_success( assert mock_sleep.call_count == 2 -def test_add_query_honors_configured_retries_over_method_default( - monkeypatch: pytest.MonkeyPatch, -) -> None: - # Regression guard: a `credentials.retries > 3` branch used to ignore - # configured values of 1-3 and fall back to a hardcoded limit of 2, so the - # default `retries: 3` attempted a query only twice. It must now attempt - # the query three times before giving up. - manager, _connection, cursor = _make_add_query_manager( - monkeypatch, - retries=3, - execute_side_effect=_FakeRetryableError(), - ) - - with ( - patch("dbt.adapters.sqlserver.sqlserver_connections.fire_event"), - patch("dbt.adapters.sqlserver.sqlserver_connections.time.sleep"), - ): - with pytest.raises(_FakeRetryableError): - manager.add_query( - "select 1", auto_begin=False, retryable_exceptions=(_FakeRetryableError,) - ) - - assert cursor.execute.call_count == 3 - - -def test_add_query_does_not_retry_when_retries_is_one( - monkeypatch: pytest.MonkeyPatch, -) -> None: - manager, _connection, cursor = _make_add_query_manager( - monkeypatch, - retries=1, - execute_side_effect=_FakeRetryableError(), - ) - - with ( - patch("dbt.adapters.sqlserver.sqlserver_connections.fire_event"), - patch("dbt.adapters.sqlserver.sqlserver_connections.time.sleep"), - ): - with pytest.raises(_FakeRetryableError): - manager.add_query( - "select 1", auto_begin=False, retryable_exceptions=(_FakeRetryableError,) - ) - - assert cursor.execute.call_count == 1 - - -def test_add_query_does_not_retry_non_retryable_errors( +@pytest.mark.parametrize( + ("retries", "execute_side_effect", "expected_exception", "expected_attempts"), + [ + pytest.param( + 3, + _FakeRetryableError(), + _FakeRetryableError, + 3, + id="retryable-error-exhausts-attempt-cap", + ), + pytest.param( + 1, + _FakeRetryableError(), + _FakeRetryableError, + 1, + id="retries-one-means-single-attempt", + ), + pytest.param( + 3, + ValueError("not retryable"), + ValueError, + 1, + id="non-retryable-error-not-retried", + ), + ], +) +def test_add_query_raises_after_expected_attempts( monkeypatch: pytest.MonkeyPatch, + retries: int, + execute_side_effect: Exception, + expected_exception: type, + expected_attempts: int, ) -> None: - manager, _connection, cursor = _make_add_query_manager( + # ``retries`` caps the total number of execute attempts: ``retries: 3`` + # tries a persistently failing query three times, ``retries: 1`` never + # retries, and errors outside ``retryable_exceptions`` raise immediately. + manager, cursor = _make_add_query_manager( monkeypatch, - retries=3, - execute_side_effect=ValueError("not retryable"), + retries=retries, + execute_side_effect=execute_side_effect, ) with ( patch("dbt.adapters.sqlserver.sqlserver_connections.fire_event"), patch("dbt.adapters.sqlserver.sqlserver_connections.time.sleep"), ): - with pytest.raises(ValueError): + with pytest.raises(expected_exception): manager.add_query( "select 1", auto_begin=False, retryable_exceptions=(_FakeRetryableError,) ) - assert cursor.execute.call_count == 1 + assert cursor.execute.call_count == expected_attempts