FEAT: add optional RetryPolicy for transient failures on connect() (GH-682) by Om-singhaI · Pull Request #751 · microsoft/mssql-python

om singhal (Om-singhaI) · 2026-09-03T22:56:18Z

Work Item / Issue Reference

GitHub Issue: #682

Summary

First of two PRs for #682, connect() scope only; cursor and execute() retry follow separately. Adds mssql_python.RetryPolicy and retry_policy= on connect() / Connection(), with the constructor shape from the issue. The issue's backoff="none" is spelled backoff="fixed", base_delay=0 here. Without a policy nothing changes.

How it works

The loop wraps only the native connect in Connection.__init__, below connection string parsing and any token acquired on the Python side, so every attempt reuses the same inputs.
The SQLSTATE is read from the SQLSTATE:XXXXX:message the C++ layer already throws. exceptions.py is untouched, so this does not preempt FEAT: Expose SQLSTATE (and native error number) as attributes on exception objects #581.
Retriable code with attempts left: a warning line (attempt, SQLSTATE, delay), sleep, retry. Otherwise _raise_connection_error runs exactly as today, same exception type, nothing rewrapped. If at least one retry happened, one more warning says which attempt failed last and with what SQLSTATE, so the give up shows in the logs too.
Default set is the seven transient SQLSTATEs from the Learn retry page: HYT00 HYT01 08001 08S01 08007 40001 40003. 08004 stays out, the page lists it under never retry. retriable_sqlstates= replaces the set.
max_attempts is total tries including the first, as in the issue. The Learn sample counts retries instead.
Every invalid setting raises ValueError. base_delay and max_delay top out at 86400 seconds, so a policy that validates can't fail inside time.sleep halfway through a retry.
The stub now declares token_provider ahead of retry_policy on connect() and Connection(). It was missing upstream, which put retry_policy in its positional slot.

Out of scope

Azure SQL throttling. Those are engine error numbers, and the native number is dropped in SQLCheckError_Wrap. Plumbing it out is #581's territory, so it goes with the second PR.

Validation

tests/test_027_retry_policy.py: 74 passed, no server. The native constructor is faked as in test_006_exceptions.py, and retry._sleep / _random are patched so delay sequences are asserted exactly. No policy is one attempt and the same OperationalError; two transient failures then success is three calls with sleeps [1.0, 2.0]; exhaustion keeps the mapped type; 28000, 08004, 42000 and a message with no SQLSTATE fail once; a transient failure followed by 28000 or no SQLSTATE logs one retry line and one give up line; an error of another type after a retry, like one from a deferred token factory, keeps its own type and still logs the give up; a token_provider token is acquired once across three attempts; bad settings, including huge ints, delays over a day and SQLSTATEs that aren't five ASCII letters or digits, raise ValueError; log lines never contain the connection string.
tests/test_028_stub_signature_parity.py: 3 passed. It parses the stub and the runtime with ast, needs no native module, and fails if connect() or Connection.__init__ drift in name, order, kind or default.
test_006_exceptions.py server free tests: 20 passed. black and flake8 with the CI flags clean.
Not run against a live server. The failure path is the unchanged _raise_connection_error.

…icrosoftGH-682) I added mssql_python.retry.RetryPolicy and retry_policy= on connect() and Connection(); cursor and execute() scope follow in a second PR. The loop wraps only the native connect, below connection string parsing and any token acquired on the Python side, so those run once; a deferred token factory is still invoked by native on each attempt. It retries the seven transient SQLSTATEs from the driver's retry logic page on Learn; without a policy nothing changes.

azure-pipelines · 2026-09-03T22:56:27Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

Copilot

🔵 Needs a closer look

It changes the core connection-establishment path (native connect invocation and retry timing/logging), which merits final human verification against real-world/native-layer behaviors beyond the included server-free tests.

Pull request overview

Adds an opt-in RetryPolicy API to the pure-Python DB-API surface so connect() / Connection(...) can automatically retry native connect failures classified as transient by SQLSTATE, without changing default behavior for existing callers.

Changes:

Introduces mssql_python.retry.RetryPolicy (configurable attempts, backoff, jitter, SQLSTATE allowlist) and exports it from the package.
Wraps the native ddbc_bindings.Connection(...) call in Connection.__init__ with retry + warning logs, leaving the existing exception mapping path intact on final failure.
Adds server-free tests covering retry behavior, delay sequences, non-retriable failures, token-acquisition reuse, and log redaction; updates stubs and changelog.

File summaries

File	Description
`tests/test_027_retry_policy.py`	Adds server-free unit tests validating connect-scope retry behavior, delay computation, and logging expectations.
`mssql_python/retry.py`	Implements `RetryPolicy`, default transient SQLSTATE set, and deterministic seams for sleep/random in tests.
`mssql_python/mssql_python.pyi`	Extends public type stubs with `RetryPolicy` and the new `retry_policy` parameters.
`mssql_python/db_connection.py`	Plumbs `retry_policy` through the public `connect()` wrapper and documents the new parameter.
`mssql_python/connection.py`	Adds SQLSTATE extraction helper and wraps native connect with policy-driven retry + warning logs.
`mssql_python/__init__.py`	Exports `RetryPolicy` and includes it in `__all__`.
`CHANGELOG.md`	Documents the new opt-in retry policy feature and its default semantics.

Review details

Files reviewed: 7/7 changed files
Comments generated: 0
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

# Conflicts: # CHANGELOG.md # mssql_python/connection.py

om singhal (Om-singhaI) · 2026-09-07T00:08:56Z

Sumit Sarabhai (@sumitmsft) this is the connect() half of #682. Nothing has run on it beyond the CLA check, so I think it needs someone to kick off the pipelines.

The statement scope half is built on top of this branch and I've been holding it back rather than stacking two open PRs on the same issue. Happy to open it as soon as this one lands, or sooner if you'd rather review them together.

One thing I'd flag while you're in here: max_attempts counts total tries, so 1 means no retry. The docs page counts retries instead. I went with the issue, but say if you'd rather match the docs and I'll change it.

Gaurav Sharma (bewithgaurav) · 2026-09-08T17:29:33Z

/azp run

azure-pipelines · 2026-09-08T17:29:44Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Copilot

🔵 Needs a closer look

It changes the core connection-establishment path and depends on native error formatting behavior, but lacks live-server validation to confidently confirm real-world retry classification and timing.

Review details

Files reviewed: 7/7 changed files
Comments generated: 0 new
Review effort level: Lite

github-actions · 2026-09-08T18:00:08Z

Code Coverage Report

Diff coverage	Overall coverage	Lines covered
99%	83%	8546 of 10199

Files needing attention

mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.row.py: 77.6%
mssql_python.pybind.ddbc_bindings.cpp: 77.7%
mssql_python.__init__.py: 81.2%
mssql_python.pybind.connection.connection_pool.cpp: 81.8%
mssql_python.logging.py: 86.9%
mssql_python.pooling.py: 90.1%

View Azure DevOps build

Gaurav Sharma (bewithgaurav)

this addresses the connection-retry scope. I'd like capped retries to stay spread out before this lands; the other comments are cleanup suggestions. requesting changes.

Gaurav Sharma (bewithgaurav) · 2026-09-08T18:32:23Z

+                doublings -= 1
+        delay = min(delay, self.max_delay)
+        if self.jitter:
+            delay = min(delay * (0.5 + _random()), self.max_delay)


suggestion: clients retrying the same outage lose part of the intended spread once the delay reaches max_delay. every random draw at or above 0.5 becomes exactly the same wait because of the second cap.

in a controlled sweep of 10,000 draws at a 30-second cap, 5,000 returned exactly 30 seconds. this is a delay calculation result, not a concurrent load measurement.

can we use full jitter over the already-capped delay instead?

Suggested change

delay = min(delay * (0.5 + _random()), self.max_delay)

delay *= _random()

this deliberately changes the documented behavior and allows shorter waits, including zero. please update the jitter docstrings and assertions together, including a case where the backoff has reached the cap.

Gaurav Sharma (bewithgaurav) · 2026-09-08T18:32:23Z

    ) -> Dict[str, Any]: ...

+# Retry Policy for transient failures at connect() time
+class RetryPolicy:


suggestion: can we re-export the annotated RetryPolicy instead of maintaining a second copy of its constructor, properties and methods here?

from .retry import RetryPolicy as RetryPolicy

this keeps the public type and read-only properties without duplicating the policy API. the FrozenSet import can go once the copied class block is removed.

Gaurav Sharma (bewithgaurav) · 2026-09-08T18:32:23Z

+def driver_log():
+    """Attach a recording handler to the driver logger for the duration of a test.
+
+    The underlying stdlib logger sits at CRITICAL until setup_logging() is called, so its level
+    is lowered to WARNING here and restored afterwards; nothing else about logging is changed.
+    """
+    stdlib_logger = logging.getLogger("mssql_python")
+    previous_level = stdlib_logger.level
+    stdlib_logger.setLevel(logging.WARNING)
+    handler = RecordingHandler()
+    mssql_python.logging.logger.addHandler(handler)
+    try:
+        yield handler
+    finally:
+        mssql_python.logging.logger.removeHandler(handler)
+        stdlib_logger.setLevel(previous_level)


optional: can we reuse caplog here and drop RecordingHandler plus the manual log-level restoration?

the driver logger doesn't propagate, so attach caplog.handler directly, use caplog.at_level(logging.WARNING, logger="mssql_python"), and remove the handler in finally. the assertions can use caplog.records.

…dler Jitter scaled the delay by a factor in [0.5, 1.5) and then clamped to max_delay, so once backoff reached the cap every draw at or above the midpoint produced exactly max_delay. At a 30 second cap that was 49.7 percent of draws landing on the same number, which is the point at which spreading clients out matters most. It now scales down by a factor in [0, 1), so a capped delay lands anywhere in [0, max_delay). Waits can be shorter than base_delay and can be zero, and the docstrings and assertions say so. Added a test that a capped delay never returns max_delay and does not pile up in any tenth of the range. The type stub kept a hand written copy of the RetryPolicy constructor, properties and methods. It re-exports the annotated class instead, so there is one source of truth, and the FrozenSet import goes with it. The logging test used a hand rolled handler and restored the logger level by hand. It uses caplog with at_level now, attaching caplog.handler directly because the driver logger does not propagate.

Copilot

🟢 Approval recommended

The change is opt-in, localized to connect-time behavior, and is backed by thorough server-free unit tests asserting retries, delays, and logging.

Review details

Files reviewed: 7/7 changed files
Comments generated: 0 new
Review effort level: Lite

om singhal (Om-singhaI) · 2026-09-09T18:27:07Z

All three done.

Jitter's full now, delay *= _random(). I ran your case before touching it and got 49.7% landing on exactly the cap over 100k draws at 30 seconds, so your 5000 in 10000 holds. Docstrings say the wait can be shorter than base_delay and can be zero, and there's a new test that a capped delay never comes back as max_delay and doesn't bunch up in any tenth of the range.

The stub imports RetryPolicy from retry now instead of keeping a copy. FrozenSet went with it.

Took the caplog one too. RecordingHandler and the manual level restore are gone.

Copilot

🟢 Approval recommended

The retry behavior is opt-in, narrowly scoped to native connect, and is covered by comprehensive server-free tests that validate correctness and logging expectations.

Review details

Files reviewed: 7/7 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot

🟡 Changes recommended

The type-stub parameter mismatch and retry-policy validation issues must be corrected before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

mssql_python/mssql_python.pyi:366

The top-level stub has the same positional mismatch with db_connection.connect, whose runtime signature includes token_provider before retry_policy (mssql_python/db_connection.py:13-21). A positional policy can type-check against this declaration but is bound to token_provider at runtime, causing the connection to fail before retrying. Add token_provider before retry_policy here as well.

    retry_policy: Optional[RetryPolicy] = None,
    **kwargs: Any,

mssql_python/retry.py:35

An arbitrarily large integer reaches math.isfinite and can raise OverflowError during float conversion instead of the documented ValueError for an out-of-range delay. Catch this conversion overflow (or otherwise perform a safe finite-number check) so invalid base_delay/max_delay values consistently use the constructor's documented exception type.

def _is_finite_number(value: object) -> bool:
    """Return True for a finite int or float that is not a bool."""
    return isinstance(value, (int, float)) and not isinstance(value, bool) and math.isfinite(value)

mssql_python/retry.py:58

A non-iterable value such as RetryPolicy(retriable_sqlstates=123) reaches this loop and leaks the incidental TypeError: 'int' object is not iterable. The constructor and _normalize_sqlstates document ValueError for invalid setting types, so validate the iterable boundary and raise a deliberate ValueError instead.

    for code in codes:

Files reviewed: 7/7 changed files
Comments generated: 1
Review effort level: Lite

Sumit Sarabhai (sumitmsft)

PR #751: No actionable findings in the reviewed changes. Retries remain opt-in and limited to connection establishment, with bounded attempts and capped jitter. Statements are not replayed.

…tion

Issue microsoft#682 asks for a log record on each retry and on the final give up. Each retry already logged a warning; now, when a retried connect still fails, one more warning names the attempt that failed last, the attempt limit and its SQLSTATE before _raise_connection_error runs as before. A first try failure, with or without a policy, logs nothing new, and the exception is unchanged.

…delay is before jitter

om singhal (Om-singhaI) · 2026-09-12T22:32:13Z

Gaurav Sharma (@bewithgaurav) Sumit Sarabhai (@sumitmsft) I've addressed everything from the earlier rounds, including the stub order Copilot caught. Could you take another look when you get a chance? And could you approve the workflow runs too? They're stuck on approval since this comes from a fork.

Copilot

🔵 Needs a closer look

Final retry-failure logging must also handle non-RuntimeError exceptions after a retry.

Review details

Suppressed comments (1)

mssql_python/connection.py:922

The loop only handles RuntimeError. If a deferred token factory (or another pybind callback) raises a non-RuntimeError after an earlier transient attempt has already been retried, that exception bypasses the attempt > 1 warning and is re-raised without the documented final give-up log. Preserve the original exception type, but emit the final warning (with SQLSTATE reported as none) for any failed attempt after a retry.

            except RuntimeError as e:

Files reviewed: 8/8 changed files
Comments generated: 0 new
Review effort level: Lite

…ect errors

Copilot

🔵 Needs a closer look

The connection retry path and public API changes warrant final human review.

Review details

Files reviewed: 8/8 changed files
Comments generated: 0 new
Review effort level: Lite

Sumit Sarabhai (sumitmsft) · 2026-09-17T07:28:50Z

/azp run

azure-pipelines · 2026-09-17T07:29:01Z

Azure Pipelines: Successfully started running 1 pipeline(s).

Gaurav Sharma (bewithgaurav)

lgtm

Gaurav Sharma (bewithgaurav) · 2026-09-17T07:52:28Z

/azp run

azure-pipelines · 2026-09-17T07:52:39Z

Azure Pipelines: Successfully started running 1 pipeline(s).

+    # Once backoff reaches max_delay every client is asking for the same number, so the jitter is
+    # the only thing keeping them apart. Scaling around the delay used to clamp roughly half of
+    # the draws to exactly max_delay.
+    monkeypatch.setattr(mssql_python.retry, "_random", random.Random(682).random)


Copilot

🟡 Changes recommended

The signature-parity test does not distinguish positional-only from positional-or-keyword parameters and can miss an API mismatch.

Get a fresh assessment by requesting another Copilot review.

Review details

Files reviewed: 8/8 changed files
Comments generated: 1
Review effort level: Lite

…y helper The helper tagged posonlyargs and args alike, so a stub that drops the slash compared equal to a runtime that keeps it, which is the positional drift this file exists to catch. Tags now follow the group each parameter came from, while the defaults list is still built across the combined sequence because a.defaults spans both groups jointly. Adds a case that fails under the old single tag helper and passes under this one.

…seeded draw The test seeded random.Random only to make the spread assertion reproducible, which the security scanner reports as a weak random number generator. The _random seam takes any callable, so it now receives an evenly spaced sweep of [0, 1). That covers the interval the same way, holds on every run rather than for one seed, and leaves no random source in the file. The assertions keep their power: under the same sweep the previous clamping jitter puts 1000 of 2000 draws on the cap and fails both of them.

Copilot

🟢 Approval recommended

The changed code and supporting tests consistently implement the documented opt-in connect-only retry behavior.

Review details

Files reviewed: 8/8 changed files
Comments generated: 0 new
Review effort level: Lite

om singhal (Om-singhaI) · 2026-09-17T17:20:57Z

Pushed fixes for both of today's comments.

test_028: _signature now tags posonlyargs and args separately, so a stub that drops the / no longer matches the runtime. Defaults still line up across both groups. The new case compares def f(a, /, b=1) with def f(a, b=1): the old helper reported them as matching, the new one doesn't.

test_027: the spread test fed _random from a seeded random.Random, which is what devskim flagged. It now gets an evenly spaced sweep over [0, 1), so the test has no random source and doesn't depend on a seed. The old clamping jitter still fails both assertions under the sweep.

The Debian ARM64 failure is test_concurrent_connections_with_same_token_provider in test_008_auth.py. It fails inside conn.close() with the native connect mocked, and without a retry_policy the connect loop makes a single attempt. This branch doesn't touch close() or that file, so I don't think it comes from this change, but a rerun would show whether it repeats.

Sumit Sarabhai (@sumitmsft) Gaurav Sharma (@bewithgaurav) the workflow runs on the new commits are waiting for approval again. Could one of you approve them and /azp run?

Copilot AI lite review requested due to automatic review settings September 3, 2026 22:56

Copilot started reviewing on behalf of om singhal (Om-singhaI) September 3, 2026 22:57 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into om/feat/retry-policy

2d04753

# Conflicts: # CHANGELOG.md # mssql_python/connection.py

Merge branch 'main' into om/feat/retry-policy

9e552d2

Copilot AI review requested due to automatic review settings September 8, 2026 17:29

Copilot started reviewing on behalf of Gaurav Sharma (bewithgaurav) September 8, 2026 17:32 View session

Copilot AI reviewed Sep 8, 2026

View reviewed changes

Gaurav Sharma (bewithgaurav) requested changes Sep 8, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 9, 2026 04:10

Copilot started reviewing on behalf of om singhal (Om-singhaI) September 9, 2026 04:10 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into om/feat/retry-policy

b8eb218

Copilot AI review requested due to automatic review settings September 10, 2026 00:46

Copilot started reviewing on behalf of om singhal (Om-singhaI) September 10, 2026 00:46 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into om/feat/retry-policy

45b6af3

Copilot AI review requested due to automatic review settings September 11, 2026 07:43

Copilot started reviewing on behalf of om singhal (Om-singhaI) September 11, 2026 07:44 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Comment thread mssql_python/mssql_python.pyi

Sumit Sarabhai (sumitmsft) previously approved these changes Sep 11, 2026

View reviewed changes

Add token_provider to the stub so retry_policy keeps its runtime posi…

126e85d

…tion

om singhal (Om-singhaI) added 2 commits September 12, 2026 18:09

Collect leftover connections before capturing driver logs; note base_…

bd801db

…delay is before jitter

Copilot AI review requested due to automatic review settings September 12, 2026 22:30

om singhal (Om-singhaI) dismissed Sumit Sarabhai (sumitmsft)’s stale review via bd801db September 12, 2026 22:30

Copilot started reviewing on behalf of om singhal (Om-singhaI) September 12, 2026 22:31 View session

Copilot AI reviewed Sep 12, 2026

View reviewed changes

Log the give up after a retry for any exception, not only native conn…

bdb8911

…ect errors

Copilot AI review requested due to automatic review settings September 12, 2026 23:14

Copilot started reviewing on behalf of om singhal (Om-singhaI) September 12, 2026 23:14 View session

Copilot AI reviewed Sep 12, 2026

View reviewed changes

Sumit Sarabhai (sumitmsft) previously approved these changes Sep 17, 2026

View reviewed changes

Gaurav Sharma (bewithgaurav) previously approved these changes Sep 17, 2026

View reviewed changes

Merge branch 'main' into om/feat/retry-policy

fe7a485

Copilot AI review requested due to automatic review settings September 17, 2026 07:51

Copilot started reviewing on behalf of om singhal (Om-singhaI) September 17, 2026 07:52 View session

github-advanced-security Bot found potential problems Sep 17, 2026

View reviewed changes

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread tests/test_028_stub_signature_parity.py

om singhal (Om-singhaI) added 2 commits September 17, 2026 12:17

Copilot AI review requested due to automatic review settings September 17, 2026 16:17

om singhal (Om-singhaI) dismissed stale reviews from Gaurav Sharma (bewithgaurav) and Sumit Sarabhai (sumitmsft) via 34404af September 17, 2026 16:17

Copilot started reviewing on behalf of om singhal (Om-singhaI) September 17, 2026 16:18 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

	delay = min(delay * (0.5 + _random()), self.max_delay)
	delay *= _random()

Conversation

om singhal (Om-singhaI) commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Work Item / Issue Reference

Summary

How it works

Out of scope

Validation

Uh oh!

azure-pipelines Bot commented Sep 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

om singhal (Om-singhaI) commented Sep 7, 2026

Uh oh!

Gaurav Sharma (bewithgaurav) commented Sep 8, 2026

Uh oh!

azure-pipelines Bot commented Sep 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

github-actions Bot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Report

Files needing attention

Uh oh!

Gaurav Sharma (bewithgaurav) left a comment

Choose a reason for hiding this comment

Uh oh!

Gaurav Sharma (bewithgaurav) Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Gaurav Sharma (bewithgaurav) Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Gaurav Sharma (bewithgaurav) Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

om singhal (Om-singhaI) commented Sep 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

Uh oh!

Sumit Sarabhai (sumitmsft) left a comment

Choose a reason for hiding this comment

Uh oh!

om singhal (Om-singhaI) commented Sep 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (1)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Sumit Sarabhai (sumitmsft) commented Sep 17, 2026

Uh oh!

azure-pipelines Bot commented Sep 17, 2026

Uh oh!

Gaurav Sharma (bewithgaurav) left a comment

Choose a reason for hiding this comment

Uh oh!

Gaurav Sharma (bewithgaurav) commented Sep 17, 2026

om singhal (Om-singhaI) commented Sep 3, 2026 •

edited

Loading

github-actions Bot commented Sep 8, 2026 •

edited

Loading