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

Đọc src/databricks/sql/auth/retry.py, bắt đầu với DatabricksRetryPolicy.sleep_for_retry và clamp get_backoff_time liền kề. Tái hiện trường hợp Retry-After nhỏ và kiểm tra tests/test_error_recovery.py, đặc biệt là kịch bản Retry-After tăng dần. Hoàn thành nghĩa là các giá trị Retry-After được giới hạn thay vì bị nâng lên delay_max, và các đường dẫn retry bị ảnh hưởng không còn phải chờ mức sàn 60 giây.

Summary

DatabricksRetryPolicy.sleep_for_retry applies delay_max as a floor instead of a ceiling, so a small server Retry-After (e.g. 2) is inflated to the full delay_max (default 60s) on every retry. This makes retryable errors that advertise a short Retry-After sleep far longer than the server requested.

Location

src/databricks/sql/auth/retry.py, in sleep_for_retry:

retry_after = self.get_retry_after(response)
if retry_after:
    proposed_wait = retry_after
else:
    proposed_wait = self.get_backoff_time()

proposed_wait = max(proposed_wait, self.delay_max)   # <-- BUG: floor, not ceiling
...
time.sleep(proposed_wait)

max(proposed_wait, self.delay_max) guarantees the sleep is at least delay_max. With the default _retry_delay_max = 60, a server response of Retry-After: 2 results in max(2, 60) = 60s.

Why it's a bug

The sibling method get_backoff_time in the same file does the opposite (and correct) clamp, with a docstring that states the intent:

# get_backoff_time():
#   "Never returns a value larger than self.delay_max"
proposed_backoff = min(proposed_backoff, self.delay_max)

So delay_max is intended as a ceiling on the wait. sleep_for_retry inverts it. The fix is to cap (not floor) the proposed wait — min(proposed_wait, self.delay_max) — or to not clamp an explicit server Retry-After upward at all.

Impact / repro

A server that returns 503 with a small Retry-After (say 2s, then 4s, then success) is honored as 60s, then 60s — 120s total instead of the intended ~6s.

Observed in the driver-test conformance suite (ERRORRECOV-001, "HTTP 503 with progressive Retry-After"): the request-executing test blocked in retry.py sleep_for_retry -> time.sleep(60) twice and hit the 120s pytest-timeout. faulthandler stack (SEA backend):

tests/test_error_recovery.py:70 cur.execute(SIMPLE_QUERY)
  -> databricks/sql/backend/sea/backend.py execute_command
  -> .../sea/utils/http_client.py _make_request
  -> urllib3 connectionpool.urlopen -> retries.sleep(response)
  -> databricks/sql/auth/retry.py:~301 sleep_for_retry -> time.sleep(proposed_wait)

Backend-agnostic: it's in the shared DatabricksRetryPolicy, so both the SEA and Thrift HTTP paths are affected (the kernel path uses a different retry mechanism and is unaffected).

Suggested fix

proposed_wait = min(proposed_wait, self.delay_max)

(and confirm delay_max is the intended upper bound on an honored Retry-After, matching get_backoff_time).