Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
peco-engineer-bot wants to merge 3 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f9ec88
Retry: sleep_for_retry uses max(wait, delay_max) — inflates small Ret…
Jul 17, 2026
b29eb9a
ai: apply changes for #868 (1 review thread)
peco-engineer-bot[bot] Jul 17, 2026
834000b
ai: apply changes for #868 (1 review thread)
peco-engineer-bot[bot] Jul 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/databricks/sql/auth/retry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ def sleep_for_retry(self, response: BaseHTTPResponse) -> bool:
else:
proposed_wait = self.get_backoff_time()

proposed_wait = max(proposed_wait, self.delay_max)
proposed_wait = min(proposed_wait, self.delay_max)
Comment thread
peco-review-bot[bot] marked this conversation as resolved.
Outdated

@eric-wang-1990 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.

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 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.

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.

self.check_proposed_wait(proposed_wait)
logger.debug(f"Retrying after {proposed_wait} seconds")
time.sleep(proposed_wait)
Expand Down
24 changes: 14 additions & 10 deletions tests/unit/test_retry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,21 @@ def test_sleep__no_retry_after(self, t_mock, retry_policy, error_history):
retry_policy.history = [error_history, error_history]
retry_policy.sleep(HTTPResponse(status=503))

expected_backoff_time = max(
self.calculate_backoff_time(
0, retry_policy.delay_min, retry_policy.delay_max
),
retry_policy.delay_max,
expected_backoff_time = self.calculate_backoff_time(
0, retry_policy.delay_min, retry_policy.delay_max
)
t_mock.assert_called_with(expected_backoff_time)

@patch("time.sleep")
def test_sleep__short_retry_after_is_not_inflated(self, t_mock, retry_policy):
# A small server Retry-After must be honored as-is (delay_max is a
# ceiling, not a floor). delay_max defaults to 30 in these fixtures.
retry_policy._retry_start_time = time.time()
retry_policy.history = []
retry_policy.sleep(HTTPResponse(status=503, headers={"Retry-After": "2"}))

t_mock.assert_called_with(2)

@patch("time.sleep")
def test_sleep__no_retry_after_header__multiple_retries(self, t_mock, retry_policy):
num_attempts = retry_policy.stop_after_attempts_count
Expand All @@ -62,11 +69,8 @@ def test_sleep__no_retry_after_header__multiple_retries(self, t_mock, retry_poli
expected_backoff_times = []
for attempt in range(num_attempts):
expected_backoff_times.append(
max(
self.calculate_backoff_time(
attempt, retry_policy.delay_min, retry_policy.delay_max
),
retry_policy.delay_max,
self.calculate_backoff_time(
attempt, retry_policy.delay_min, retry_policy.delay_max
)
)

Expand Down
Loading