FEAT: add optional RetryPolicy for transient failures on connect() (GH-682) - #751
om singhal (Om-singhaI) wants to merge 22 commits into
Conversation
…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: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🔵 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 inConnection.__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
|
Sumit Sarabhai (@sumitmsft) this is the 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: |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
🔵 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
Code Coverage Report
Files needing attentionmssql_python.pybind.performance_counter.hpp: 0.7% mssql_python.pybind.logger_bridge.cpp: 57.9% mssql_python.pybind.ddbc_bindings.h: 62.6% mssql_python.pybind.logger_bridge.hpp: 70.8% mssql_python.pybind.ddbc_bindings.cpp: 79.1% mssql_python.__init__.py: 81.2% mssql_python.pybind.connection.connection_pool.cpp: 82.3% mssql_python.pybind.connection.connection.cpp: 83.1% mssql_python.logging.py: 86.9% mssql_python.pooling.py: 90.1% |
Gaurav Sharma (bewithgaurav)
left a comment
Gaurav Sharma (bewithgaurav)
left a comment
There was a problem hiding this comment.
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.
| doublings -= 1 | ||
| delay = min(delay, self.max_delay) | ||
| if self.jitter: | ||
| delay = min(delay * (0.5 + _random()), self.max_delay) |
There was a problem hiding this comment.
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?
| 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.
| ) -> Dict[str, Any]: ... | ||
|
|
||
| # Retry Policy for transient failures at connect() time | ||
| class RetryPolicy: |
There was a problem hiding this comment.
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 RetryPolicythis 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.
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
🟢 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
|
All three done. Jitter's full now, The stub imports Took the caplog one too. |
There was a problem hiding this comment.
🟢 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
There was a problem hiding this comment.
🟡 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 includestoken_providerbeforeretry_policy(mssql_python/db_connection.py:13-21). A positional policy can type-check against this declaration but is bound totoken_providerat runtime, causing the connection to fail before retrying. Addtoken_providerbeforeretry_policyhere as well.
retry_policy: Optional[RetryPolicy] = None,
**kwargs: Any,
mssql_python/retry.py:35
- An arbitrarily large integer reaches
math.isfiniteand can raiseOverflowErrorduring float conversion instead of the documentedValueErrorfor an out-of-range delay. Catch this conversion overflow (or otherwise perform a safe finite-number check) so invalidbase_delay/max_delayvalues 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 incidentalTypeError: 'int' object is not iterable. The constructor and_normalize_sqlstatesdocumentValueErrorfor invalid setting types, so validate the iterable boundary and raise a deliberateValueErrorinstead.
for code in codes:
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
Sumit Sarabhai (sumitmsft)
left a comment
Sumit Sarabhai (sumitmsft)
left a comment
There was a problem hiding this comment.
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.
|
Pushed fixes for both of today's comments.
The Debian ARM64 failure is 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 |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
PR Performance Report✅ No regression detectedNo consistent slowdowns detected across all 2 environments. 0 IMPROVEMENTS 0 SLOWDOWNS 2/2 ENVIRONMENTS Coverage: 2 of 2 environments completed. Advisory result; does not block merging. Performance diagnosticsPhase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed. No affected phases or call-count changes were recorded. All database tasks and timingsUnix / SQL Server 2022
Unix / SQL Server 2025
Build and measurement detailsPR head:
A consistent change requires more than 20% median paired movement, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold in the same direction. A slowdown without enough pair agreement is reported as inconsistent. The displayed change is the median of paired before-and-after ratios. It is not recalculated from the two displayed median runtimes. Both revisions use profiling-enabled builds on the same agent and database, with alternating order and discarded warmups. Results are diagnostic and do not represent production-wheel latency. Raw samples and logs are attached to the ADO run as |
There was a problem hiding this comment.
🔵 Needs a closer look
The change affects connection establishment, authentication, pooling, and retry timing, and was not validated against a live SQL Server.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
Head branch was pushed to by a user without write access
7c285f8
|
Merged main again to clear the conflict from #613. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Work Item / Issue Reference
Summary
First of two PRs for #682,
connect()scope only; cursor andexecute()retry follow separately. Addsmssql_python.RetryPolicyandretry_policy=onconnect()/Connection(), with the constructor shape from the issue. The issue'sbackoff="none"is spelledbackoff="fixed", base_delay=0here. Without a policy nothing changes.How it works
Connection.__init__, below connection string parsing and any token acquired on the Python side, so every attempt reuses the same inputs.SQLSTATE:XXXXX:messagethe C++ layer already throws.exceptions.pyis untouched, so this does not preempt FEAT: Expose SQLSTATE (and native error number) as attributes on exception objects #581._raise_connection_errorruns 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.HYT00 HYT01 08001 08S01 08007 40001 40003.08004stays out, the page lists it under never retry.retriable_sqlstates=replaces the set.max_attemptsis total tries including the first, as in the issue. The Learn sample counts retries instead.ValueError.base_delayandmax_delaytop out at 86400 seconds, so a policy that validates can't fail insidetime.sleephalfway through a retry.token_providerahead ofretry_policyonconnect()andConnection(). It was missing upstream, which putretry_policyin 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 intest_006_exceptions.py, andretry._sleep/_randomare patched so delay sequences are asserted exactly. No policy is one attempt and the sameOperationalError; two transient failures then success is three calls with sleeps[1.0, 2.0]; exhaustion keeps the mapped type;28000,08004,42000and a message with no SQLSTATE fail once; a transient failure followed by28000or 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; atoken_providertoken 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, raiseValueError; log lines never contain the connection string.tests/test_028_stub_signature_parity.py: 3 passed. It parses the stub and the runtime withast, needs no native module, and fails ifconnect()orConnection.__init__drift in name, order, kind or default.test_006_exceptions.pyserver free tests: 20 passed.blackandflake8with the CI flags clean._raise_connection_error.