fix(http): expand retry budget + use exponential backoff on DNS failure
Transient DNS resolution failures (socket.gaierror, surfaced as urllib.error.URLError with reason=gaierror) were retried with the generic URLError handler — linear backoff (2s, 4s, 6s) and bounded by the caller-passed `retries` parameter. For callers that pass small retry values (e.g. lib/reddit.py::_subreddit_search uses retries=2), a single first-attempt DNS hiccup followed by one quick retry on the still-flaky resolver would exhaust the retry budget and wipe a whole subreddit sweep — which the caller's broad `except Exception` then silent-empties as `[]`. Fix: - Distinguish URLError-with-gaierror-reason from generic URLError via a new `_is_dns_failure()` helper. - For DNS failures, use exponential backoff (1s, 2s, 4s, ...) instead of the linear default. - For DNS failures, expand the effective retry budget to at least MIN_DNS_RETRIES (=3) on first occurrence, so callers that passed `retries=2` still get a meaningful retry budget for the transient case. Non-DNS URLErrors and HTTPErrors keep the caller's value. - DNS attempts are counted separately (`dns_attempts`) so unrelated URLError or OSError failures within the same call don't accidentally expand the budget further. Reported during a community-signal pass where the Reddit subreddit sweep silently returned zero items after a first-round transient DNS hiccup. The fix lives at the http layer (where the retry loop is) rather than per-source so every caller benefits. Tests: - Verifies a caller-passed retries=2 still gets MIN_DNS_RETRIES=3 attempts on gaierror. - Verifies gaierror-then-success returns successfully on attempt 2. - Verifies the exponential-backoff sleep pattern (1s, 2s) on the retry attempts before exhaustion. - Verifies a non-DNS URLError (ConnectionRefusedError reason) does NOT expand the retry budget — only true DNS failures do. All 12 http tests pass (8 baseline + 4 new). No regressions in the broader test suite (1373 pass / 14 fail, vs 1369 pass / 14 fail on main — the 14 failures are pre-existing and unrelated to this PR).
This commit is contained in:
committed by
Trevin Chow
parent
602de1ebda
commit
5a2fe5279b
@@ -2,6 +2,7 @@
|
|||||||
|
|
||||||
import json
|
import json
|
||||||
import re
|
import re
|
||||||
|
import socket
|
||||||
import sys
|
import sys
|
||||||
import time
|
import time
|
||||||
import urllib.error
|
import urllib.error
|
||||||
@@ -22,9 +23,19 @@ def log(msg: str):
|
|||||||
MAX_RETRIES = 5
|
MAX_RETRIES = 5
|
||||||
MAX_429_RETRIES = 2
|
MAX_429_RETRIES = 2
|
||||||
RETRY_DELAY = 2.0
|
RETRY_DELAY = 2.0
|
||||||
|
# DNS resolution failures (gaierror) are transient — typically resolved by a
|
||||||
|
# brief backoff and retry. Use a dedicated minimum attempt count + exponential
|
||||||
|
# delays (1s, 2s, 4s) so callers that pass a small `retries` value still get a
|
||||||
|
# meaningful chance to recover from a transient resolution failure.
|
||||||
|
MIN_DNS_RETRIES = 3
|
||||||
USER_AGENT = "last30days-skill/3.0 (Assistant Skill)"
|
USER_AGENT = "last30days-skill/3.0 (Assistant Skill)"
|
||||||
|
|
||||||
|
|
||||||
|
def _is_dns_failure(err: urllib.error.URLError) -> bool:
|
||||||
|
"""Return True if a URLError was caused by DNS resolution (gaierror)."""
|
||||||
|
return isinstance(getattr(err, "reason", None), socket.gaierror)
|
||||||
|
|
||||||
|
|
||||||
class HTTPError(Exception):
|
class HTTPError(Exception):
|
||||||
"""HTTP request error with status code."""
|
"""HTTP request error with status code."""
|
||||||
def __init__(self, message: str, status_code: Optional[int] = None, body: Optional[str] = None):
|
def __init__(self, message: str, status_code: Optional[int] = None, body: Optional[str] = None):
|
||||||
@@ -85,7 +96,13 @@ def request(
|
|||||||
|
|
||||||
last_error = None
|
last_error = None
|
||||||
rate_limit_count = 0
|
rate_limit_count = 0
|
||||||
for attempt in range(retries):
|
# DNS failures get a dedicated minimum attempt count + exponential backoff.
|
||||||
|
# `effective_retries` is the actual loop bound; we expand it on the first
|
||||||
|
# gaierror if the caller passed a smaller `retries` value than MIN_DNS_RETRIES.
|
||||||
|
effective_retries = retries
|
||||||
|
dns_attempts = 0
|
||||||
|
attempt = 0
|
||||||
|
while attempt < effective_retries:
|
||||||
try:
|
try:
|
||||||
with urllib.request.urlopen(req, timeout=timeout) as response:
|
with urllib.request.urlopen(req, timeout=timeout) as response:
|
||||||
body = response.read().decode('utf-8')
|
body = response.read().decode('utf-8')
|
||||||
@@ -115,7 +132,7 @@ def request(
|
|||||||
if rate_limit_count >= max_429_retries:
|
if rate_limit_count >= max_429_retries:
|
||||||
raise last_error
|
raise last_error
|
||||||
|
|
||||||
if attempt < retries - 1:
|
if attempt < effective_retries - 1:
|
||||||
if e.code == 429:
|
if e.code == 429:
|
||||||
# Respect Retry-After header, fall back to exponential backoff
|
# Respect Retry-After header, fall back to exponential backoff
|
||||||
retry_after = e.headers.get("Retry-After") if hasattr(e, 'headers') else None
|
retry_after = e.headers.get("Retry-After") if hasattr(e, 'headers') else None
|
||||||
@@ -126,14 +143,34 @@ def request(
|
|||||||
delay = RETRY_DELAY * (2 ** attempt) + 1
|
delay = RETRY_DELAY * (2 ** attempt) + 1
|
||||||
else:
|
else:
|
||||||
delay = RETRY_DELAY * (2 ** attempt) + 1 # 3s, 5s, 9s...
|
delay = RETRY_DELAY * (2 ** attempt) + 1 # 3s, 5s, 9s...
|
||||||
log(f"Rate limited (429). Waiting {delay:.1f}s before retry {attempt + 2}/{retries}")
|
log(f"Rate limited (429). Waiting {delay:.1f}s before retry {attempt + 2}/{effective_retries}")
|
||||||
else:
|
else:
|
||||||
delay = RETRY_DELAY * (2 ** attempt)
|
delay = RETRY_DELAY * (2 ** attempt)
|
||||||
time.sleep(delay)
|
time.sleep(delay)
|
||||||
except urllib.error.URLError as e:
|
except urllib.error.URLError as e:
|
||||||
log(f"URL Error: {e.reason}")
|
log(f"URL Error: {e.reason}")
|
||||||
last_error = HTTPError(f"URL Error: {e.reason}")
|
last_error = HTTPError(f"URL Error: {e.reason}")
|
||||||
if attempt < retries - 1:
|
if _is_dns_failure(e):
|
||||||
|
# DNS resolution failures are transient; expand the retry budget
|
||||||
|
# to MIN_DNS_RETRIES if the caller passed fewer, and use
|
||||||
|
# exponential backoff (1s, 2s, 4s, ...) instead of the linear
|
||||||
|
# default. Counts DNS attempts separately so other URLError
|
||||||
|
# causes don't bypass the regular retry budget.
|
||||||
|
dns_attempts += 1
|
||||||
|
if effective_retries < MIN_DNS_RETRIES:
|
||||||
|
log(
|
||||||
|
f"DNS resolution failed; expanding retry budget from "
|
||||||
|
f"{effective_retries} to {MIN_DNS_RETRIES}"
|
||||||
|
)
|
||||||
|
effective_retries = MIN_DNS_RETRIES
|
||||||
|
if attempt < effective_retries - 1:
|
||||||
|
delay = 2 ** (dns_attempts - 1) # 1s, 2s, 4s, 8s, ...
|
||||||
|
log(
|
||||||
|
f"DNS resolution failure (attempt {dns_attempts}); "
|
||||||
|
f"retrying in {delay:.1f}s"
|
||||||
|
)
|
||||||
|
time.sleep(delay)
|
||||||
|
elif attempt < effective_retries - 1:
|
||||||
time.sleep(RETRY_DELAY * (attempt + 1))
|
time.sleep(RETRY_DELAY * (attempt + 1))
|
||||||
except json.JSONDecodeError as e:
|
except json.JSONDecodeError as e:
|
||||||
log(f"JSON decode error: {e}")
|
log(f"JSON decode error: {e}")
|
||||||
@@ -143,9 +180,11 @@ def request(
|
|||||||
# Handle socket-level errors (connection reset, timeout, etc.)
|
# Handle socket-level errors (connection reset, timeout, etc.)
|
||||||
log(f"Connection error: {type(e).__name__}: {e}")
|
log(f"Connection error: {type(e).__name__}: {e}")
|
||||||
last_error = HTTPError(f"Connection error: {type(e).__name__}: {e}")
|
last_error = HTTPError(f"Connection error: {type(e).__name__}: {e}")
|
||||||
if attempt < retries - 1:
|
if attempt < effective_retries - 1:
|
||||||
time.sleep(RETRY_DELAY * (attempt + 1))
|
time.sleep(RETRY_DELAY * (attempt + 1))
|
||||||
|
|
||||||
|
attempt += 1
|
||||||
|
|
||||||
if last_error:
|
if last_error:
|
||||||
raise last_error
|
raise last_error
|
||||||
raise HTTPError("Request failed with no error details")
|
raise HTTPError("Request failed with no error details")
|
||||||
|
|||||||
@@ -104,3 +104,79 @@ class TestParamsEncoding(unittest.TestCase):
|
|||||||
sent_url = self._sent_url(mock_urlopen)
|
sent_url = self._sent_url(mock_urlopen)
|
||||||
self.assertIn("count=25", sent_url)
|
self.assertIn("count=25", sent_url)
|
||||||
self.assertIn("raw=True", sent_url)
|
self.assertIn("raw=True", sent_url)
|
||||||
|
|
||||||
|
|
||||||
|
class TestDNSResolutionRetry(unittest.TestCase):
|
||||||
|
"""DNS resolution failures (gaierror) must retry with exponential backoff.
|
||||||
|
|
||||||
|
Caller-passed `retries` values smaller than MIN_DNS_RETRIES are expanded
|
||||||
|
on the first gaierror so a transient resolution failure doesn't wipe a
|
||||||
|
request just because the caller passed retries=2.
|
||||||
|
"""
|
||||||
|
|
||||||
|
@patch("lib.http.urllib.request.urlopen")
|
||||||
|
@patch("lib.http.time.sleep")
|
||||||
|
def test_gaierror_retries_up_to_min_dns_retries_even_when_caller_passes_fewer(
|
||||||
|
self, mock_sleep, mock_urlopen
|
||||||
|
):
|
||||||
|
"""Caller passed retries=2; gaierror should still get MIN_DNS_RETRIES attempts."""
|
||||||
|
import socket
|
||||||
|
err = urllib.error.URLError(socket.gaierror(-2, "Name or service not known"))
|
||||||
|
mock_urlopen.side_effect = err
|
||||||
|
|
||||||
|
with self.assertRaises(http.HTTPError):
|
||||||
|
http.request("GET", "http://nonexistent.example", retries=2)
|
||||||
|
|
||||||
|
# Caller passed retries=2, but the budget expanded to MIN_DNS_RETRIES=3.
|
||||||
|
self.assertEqual(mock_urlopen.call_count, http.MIN_DNS_RETRIES)
|
||||||
|
|
||||||
|
@patch("lib.http.urllib.request.urlopen")
|
||||||
|
@patch("lib.http.time.sleep")
|
||||||
|
def test_gaierror_succeeds_after_transient_failure(self, mock_sleep, mock_urlopen):
|
||||||
|
"""gaierror on attempt 1, then success — should NOT raise."""
|
||||||
|
import socket
|
||||||
|
success_response = MagicMock()
|
||||||
|
success_response.read.return_value = b'{"ok": true}'
|
||||||
|
success_response.status = 200
|
||||||
|
success_response.__enter__ = lambda self: self
|
||||||
|
success_response.__exit__ = lambda *args: None
|
||||||
|
|
||||||
|
err = urllib.error.URLError(socket.gaierror(-2, "Name or service not known"))
|
||||||
|
mock_urlopen.side_effect = [err, success_response]
|
||||||
|
|
||||||
|
result = http.request("GET", "http://flaky.example", retries=2)
|
||||||
|
|
||||||
|
self.assertEqual(result, {"ok": True})
|
||||||
|
self.assertEqual(mock_urlopen.call_count, 2)
|
||||||
|
|
||||||
|
@patch("lib.http.urllib.request.urlopen")
|
||||||
|
@patch("lib.http.time.sleep")
|
||||||
|
def test_gaierror_uses_exponential_backoff(self, mock_sleep, mock_urlopen):
|
||||||
|
"""Backoff delays for gaierror should be 1s, 2s, 4s — not the linear default."""
|
||||||
|
import socket
|
||||||
|
err = urllib.error.URLError(socket.gaierror(-2, "Name or service not known"))
|
||||||
|
mock_urlopen.side_effect = err
|
||||||
|
|
||||||
|
with self.assertRaises(http.HTTPError):
|
||||||
|
http.request("GET", "http://nonexistent.example", retries=3)
|
||||||
|
|
||||||
|
# Expected sleep calls: 1s (after attempt 1), 2s (after attempt 2).
|
||||||
|
# No sleep after the final attempt (the loop exits to raise).
|
||||||
|
sleep_delays = [call.args[0] for call in mock_sleep.call_args_list]
|
||||||
|
self.assertEqual(sleep_delays, [1, 2])
|
||||||
|
|
||||||
|
@patch("lib.http.urllib.request.urlopen")
|
||||||
|
@patch("lib.http.time.sleep")
|
||||||
|
def test_non_dns_urlerror_uses_linear_backoff_not_dns_branch(
|
||||||
|
self, mock_sleep, mock_urlopen
|
||||||
|
):
|
||||||
|
"""A URLError that's NOT a gaierror must NOT expand the retry budget."""
|
||||||
|
# ConnectionRefusedError-style URLError reason (not gaierror)
|
||||||
|
err = urllib.error.URLError(ConnectionRefusedError(111, "Connection refused"))
|
||||||
|
mock_urlopen.side_effect = err
|
||||||
|
|
||||||
|
with self.assertRaises(http.HTTPError):
|
||||||
|
http.request("GET", "http://refused.example", retries=2)
|
||||||
|
|
||||||
|
# Caller passed retries=2, and non-DNS URLError doesn't expand it.
|
||||||
|
self.assertEqual(mock_urlopen.call_count, 2)
|
||||||
|
|||||||
Reference in New Issue
Block a user