Skip to content

fix: honor configured query retries and correct the retry debug event#730

Merged
axellpadilla merged 3 commits into
dbt-msft:masterfrom
joshmarkovic:fix/query-retry-loop
Jul 3, 2026
Merged

fix: honor configured query retries and correct the retry debug event#730
axellpadilla merged 3 commits into
dbt-msft:masterfrom
joshmarkovic:fix/query-retry-loop

Conversation

@joshmarkovic

@joshmarkovic joshmarkovic commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

What

Fixes two defects in the query-level retry path in SQLServerConnectionManager.add_query.

1. The retry-debug event was constructed with a nonexistent field. On a retryable error, the code fired AdapterEventDebug(message=...), but the event's field is base_msg. Construction does not crash: dbt-common logs an "Unable to parse logging event dictionary" warning and emits the event with an empty message (verified by constructing the event against the pinned dbt-adapters/dbt-common in this repo's environment). The retry itself still ran, but the debug line that should report the error and remaining attempts was blank. This now uses base_msg=, matching dbt-core's base add_query, which is where this method was originally copied from.

2. Configured retry counts of 1-3 were silently ignored. The retry limit was computed as credentials.retries if credentials.retries > 3 else retry_limit, where retry_limit is a hardcoded 2. Any configured retries value of 1, 2, or 3 was discarded in favor of 2, so even the default retries: 3 capped a query at two attempts. The configured value is now passed through directly.

Semantics

For queries, retries caps the total number of execute attempts, matching the base SQLConnectionManager.add_query contract (attempt >= retry_limit): retries: 3 means up to three attempts (two retries), and retries: 1 means a single attempt with no retry. Note this differs from connection opening, where retry_connection treats the same profile value as a retry count (retries: 3 allows up to four connect attempts). That inconsistency exists upstream between add_query and retry_connection in dbt-adapters; this PR standardizes query behavior on the base add_query semantics and documents it in the code.

Scope note

No in-tree caller currently passes retryable_exceptions to add_query: execute() calls self.add_query(sql, auto_begin), and the backend retryable-exception tuples (get_pyodbc_retryable_exceptions / get_mssql_python_retryable_exceptions) are only wired into retry_connection in open(). This PR therefore fixes the retry mechanism for callers that opt in, but does not by itself enable query retries on the standard execute path. Wiring the backend tuples into execute() is deliberately left as a follow-up, since auto-retrying arbitrary statements after e.g. a dropped connection can double-execute non-idempotent SQL and deserves its own discussion.

Tests

Adds unit tests for add_query's retry path:

  • retry until success on a retryable error (asserts attempt count, returned cursor, and sleep count),
  • a parametrized failure-path test covering the configured attempt cap (retries: 3 attempts three times), retries: 1 (single attempt, no retry), and non-retryable errors raising immediately with no retry.

Verification

  • Full unit suite: 272 passed.
  • ruff, black (line-length 99, py310), and mypy all clean on the changed files.

@joshmarkovic joshmarkovic marked this pull request as draft June 29, 2026 20:21
@axellpadilla axellpadilla added this to the v1.10.1 milestone Jun 30, 2026
@axellpadilla

Copy link
Copy Markdown
Collaborator

Just flagged this for final release 1.10.1

@joshmarkovic joshmarkovic changed the title fix: honor configured query retries and stop retry-event crash fix: honor configured query retries and correct the retry debug event Jul 2, 2026
@joshmarkovic joshmarkovic force-pushed the fix/query-retry-loop branch from 6933201 to 94dd8a0 Compare July 2, 2026 17:37
@joshmarkovic joshmarkovic marked this pull request as ready for review July 2, 2026 17:39
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.
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.
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.
@axellpadilla axellpadilla force-pushed the fix/query-retry-loop branch from 94dd8a0 to 4a1e661 Compare July 3, 2026 15:34
@axellpadilla axellpadilla merged commit a2e6196 into dbt-msft:master Jul 3, 2026
5 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants