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

[peco-engineer-bot]

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.

[peco-review-bot]

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.

[peco-review-bot]

✅ No issues identified by the review bot.

[eric-wang-1990]

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]

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.

[peco-review-bot]

✅ No issues identified by the review bot.

[vikrantpuppala]

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

[jprakash-db]

@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]

@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?