FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Retry: sleep_for_retry uses max(wait, delay_max) — inflates small Retry-After to delay_max (60s floor) (#860) by peco-engineer-bot[bot] · Pull Request #868 · databricks/databricks-sql-python · GitHub

/ databricks-sql-python Public

Retry: sleep_for_retry uses max(wait, delay_max) — inflates small Retry-After to delay_max (60s floor) (#860) - #868

Open
peco-engineer-bot[bot] wants to merge 3 commits into
mainfrom
ai/issue-860
ConversationCommits (3)ChecksFiles changed
Open

Retry: sleep_for_retry uses max(wait, delay_max) — inflates small Retry-After to delay_max (60s floor) (#860)#868
peco-engineer-bot[bot] wants to merge 3 commits into
mainfrom
ai/issue-860

Conversation

peco-engineer-bot Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Automated fix for #860 — Retry: sleep_for_retry uses max(wait, delay_max) — inflates small Retry-After to delay_max (60s floor).

In sleep_for_retry (src/databricks/sql/auth/retry.py) changed proposed_wait = max(proposed_wait, self.delay_max) to min(...), so delay_max acts as a ceiling (matching get_backoff_time) instead of a floor that inflated small Retry-After values to 60s. Verified with the three reproducing tests plus the full unit suite (493 passed).

Root cause & plan

Root cause: In src/databricks/sql/auth/retry.py, sleep_for_retry (line 300) applies proposed_wait = max(proposed_wait, self.delay_max). delay_max is meant as a CEILING on the wait (as the sibling get_backoff_time correctly uses min(proposed_backoff, self.delay_max) with the docstring "Never returns a value larger than self.delay_max"). Using max makes delay_max a FLOOR: a small server Retry-After (e.g. 2s) is inflated to the full delay_max (default 60s in prod config; 30s in unit test fixtures). It also forces the no-Retry-After backoff path to always sleep delay_max since get_backoff_time() already returns a value <= delay_max. Fix: change max to min so delay_max caps (not floors) the wait, matching the suggested fix in the issue and the behavior of get_backoff_time.
Files: src/databricks/sql/auth/retry.py, tests/unit/test_retry.py
Planned coverage:

  • New test: policy with a small server Retry-After header (e.g. Retry-After: 2, delay_max=30) must call time.sleep(2), not delay_max. This is the primary reported divergence (small Retry-After inflated to the delay_max floor). (Small Retry-After inflated to delay_max floor)
  • Correct the two existing tests (test_sleep__no_retry_after and test_sleep__no_retry_after_header__multiple_retries) which currently assert the buggy floor behavior via an outer max(..., delay_max). After the fix, the no-Retry-After backoff path must sleep the exponential backoff value (clamped by min in get_backoff_time), not delay_max. These tests encode the bug and must be updated to assert the correct (min-clamped) value. (No-Retry-After backoff path forced up to delay_max floor)

Files changed

  • src/databricks/sql/auth/retry.py
  • tests/unit/test_retry.py

Test plan

  • tests/unit/test_retry.py::TestRetry::test_sleep__short_retry_after_is_not_inflated — fails (red) against the original code, passes (green) after the fix
  • tests/unit/test_retry.py::TestRetry::test_sleep__no_retry_after — fails (red) against the original code, passes (green) after the fix
  • tests/unit/test_retry.py::TestRetry::test_sleep__no_retry_after_header__multiple_retries — fails (red) against the original code, passes (green) after the fix

🤖 Generated by engineer-bot (bug-fix flow) — review before merge.

Retry: sleep_for_retry uses max(wait, delay_max) — inflates small Ret…
9f9ec88 
…ry-After to delay_max (60s floor) (#860)

Signed-off-by: peco-engineer-bot[bot] <3815206+peco-engineer-bot[bot]@users.noreply.github.com>
peco-engineer-bot Bot added the engineer-bot Maintainer-applied gate: triggers engineer-bot (bug-fix on issue / take-over on PR). label Jul 17, 2026
peco-engineer-bot Bot mentioned this pull request Jul 17, 2026
Open
peco-review-bot Bot approved these changes Jul 17, 2026
View reviewed changes

peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Verdict: 1 Low

Looks good — the max → min change correctly makes delay_max a ceiling (matching get_backoff_time) and fixes the reported small-Retry-After inflation. Tests are updated appropriately and new coverage was added; one low-severity note about large Retry-After values now being clamped is filed inline.

Comment thread src/databricks/sql/auth/retry.py Outdated
ai: apply changes for #868 (1 review thread)
b29eb9a 
Addresses:
  - #3599845693 at src/databricks/sql/auth/retry.py:300

Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
peco-review-bot Bot approved these changes Jul 17, 2026
View reviewed changes

peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

✅ No issues identified by the review bot.

eric-wang-1990 enabled auto-merge July 17, 2026 02:03
eric-wang-1990 reviewed Jul 17, 2026
View reviewed changes
Comment thread src/databricks/sql/auth/retry.py Outdated
proposed_wait = self.get_backoff_time()

proposed_wait = max(proposed_wait, self.delay_max)
proposed_wait = min(proposed_wait, self.delay_max)

eric-wang-1990 Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The delay_max should not apply to server returned retry_after, that should always be the source of truth. In this function we should just use proposed_wait directly, since for get_backoff_time() it already have proposed_backoff = min(proposed_backoff, self.delay_max).

peco-engineer-bot Bot Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Made the change the reviewer asked for.

What changed:

  • src/databricks/sql/auth/retry.py: Removed proposed_wait = min(proposed_wait, self.delay_max) in sleep_for_retry(). The server's Retry-After is now honored as-is, and the get_backoff_time() path already caps itself at delay_max internally — so proposed_wait is used directly, exactly as the reviewer described.
  • tests/unit/test_retry.py: The existing test_sleep__large_retry_after_is_capped_at_delay_max encoded the now-incorrect behavior, so I updated it (renamed to test_sleep__large_retry_after_is_honored_as_is) to assert that a Retry-After: 120 sleeps for 120s rather than being clamped to delay_max.

All 14 tests in tests/unit/test_retry.py pass.

Pushed 834000b.

eric-wang-1990 mentioned this pull request Jul 17, 2026
Merged
eric-wang-1990 added engineer-bot Maintainer-applied gate: triggers engineer-bot (bug-fix on issue / take-over on PR). and removed engineer-bot Maintainer-applied gate: triggers engineer-bot (bug-fix on issue / take-over on PR). labels Jul 17, 2026
ai: apply changes for #868 (1 review thread)
834000b 
Addresses:
  - #3599995835 at src/databricks/sql/auth/retry.py:300

Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
peco-review-bot Bot approved these changes Jul 17, 2026
View reviewed changes

peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

✅ No issues identified by the review bot.

eric-wang-1990 mentioned this pull request Jul 17, 2026
Merged

vikrantpuppala commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

@jprakash-db you had mentioned there's a reason why we floor at 60s, can you elaborate?

jprakash-db commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

@eric-wang-1990 @vikrantpuppala This was done because in many cases the server returns retry after as 0 even when there is a need for retry and this had caused a major Sev1, because we would exhaust the number of retries very quickly

eric-wang-1990 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

@eric-wang-1990 @vikrantpuppala This was done because in many cases the server returns retry after as 0 even when there is a need for retry and this had caused a major Sev1, because we would exhaust the number of retries very quickly

Thanks for the info, should we change the logic to if retry_after is non-0 use it, otherwise use delay_max then? Is the bug that retry_after is 0, or is the bug retry_after is too small and for python we should not use it?

For ADBC/JDBC I believe we trust retry_after, not sure why python see a 0?

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

eric-wang-1990 eric-wang-1990 left review comments
peco-review-bot[bot] peco-review-bot[bot] approved these changes
vikrantpuppala Awaiting requested review from vikrantpuppala
sreekanth-db Awaiting requested review from sreekanth-db

Assignees

No one assigned

Labels

engineer-bot Maintainer-applied gate: triggers engineer-bot (bug-fix on issue / take-over on PR).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL