FEAT: add optional RetryPolicy for transient failures on connect() (GH-682) - #751
om singhal (Om-singhaI) wants to merge 15 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.logger_bridge.cpp: 57.9% mssql_python.pybind.ddbc_bindings.h: 61.5% mssql_python.pybind.logger_bridge.hpp: 70.8% mssql_python.pybind.ddbc_bindings.cpp: 77.5% mssql_python.row.py: 77.6% mssql_python.__init__.py: 81.2% mssql_python.pybind.connection.connection_pool.cpp: 82.2% mssql_python.pybind.connection.connection.cpp: 84.4% mssql_python.connection.py: 86.4% mssql_python.logging.py: 86.9% |
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.
base_delay and max_delay are now limited to one day. A huge int used to escape as OverflowError from math.isfinite, and a delay above what time.sleep accepts passed validation and then failed inside the retry loop. A retriable_sqlstates value that is not iterable raises ValueError instead of TypeError, and each code must be five ASCII letters or digits, so upper() cannot change its length and every accepted code can match a driver SQLSTATE. Also corrects the jitter bound in the compute_delay docstring, seeds the jitter spread test, and says in the CHANGELOG that full jitter is on by default.
The stub imported TokenProvider without the redundant alias that marks a stub import as public, under a comment that only described RetryPolicy. test_028 parses connection.py, db_connection.py and the stub with ast and fails if the kind, name, order or default of any parameter of Connection.__init__ or connect() drifts between them. That drift is how retry_policy ended up in token_provider's positional slot.
Adds three tests, each of which fails if the matching part of the loop regresses: - a wrong retry_policy type raises TypeError before a token_provider is asked for a token - an exception from the native constructor that is not a RuntimeError, such as an InterfaceError from a deferred token factory, is raised once as its own type and not retried - with Authentication=ActiveDirectoryMsi, every attempt gets the same token factory and the same non-empty pool key The token_provider test now also asserts that every attempt gets identical arguments. The comment on the first retry test no longer claims it covers a pool key and factory that its connection string does not have.
The comment said the stored policy would later be picked up by cursor level retries, which this change does not ship. The attribute stays; only the forward looking sentence goes.
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
|
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. |
There was a problem hiding this comment.
🔵 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-RuntimeErrorafter an earlier transient attempt has already been retried, that exception bypasses theattempt > 1warning and is re-raised without the documented final give-up log. Preserve the original exception type, but emit the final warning (with SQLSTATE reported asnone) 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
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.