From 5a2fe5279b10283849ba876b4221592bb1ed88c6 Mon Sep 17 00:00:00 2001 From: Gabriel Arrillaga Date: Tue, 12 May 2026 21:02:08 -0500 Subject: [PATCH 1/2] fix(http): expand retry budget + use exponential backoff on DNS failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- skills/last30days/scripts/lib/http.py | 49 +++++++++++++++-- tests/test_http_v3.py | 76 +++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 5 deletions(-) diff --git a/skills/last30days/scripts/lib/http.py b/skills/last30days/scripts/lib/http.py index 1b09d76..06a69a2 100644 --- a/skills/last30days/scripts/lib/http.py +++ b/skills/last30days/scripts/lib/http.py @@ -2,6 +2,7 @@ import json import re +import socket import sys import time import urllib.error @@ -22,9 +23,19 @@ def log(msg: str): MAX_RETRIES = 5 MAX_429_RETRIES = 2 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)" +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): """HTTP request error with status code.""" def __init__(self, message: str, status_code: Optional[int] = None, body: Optional[str] = None): @@ -85,7 +96,13 @@ def request( last_error = None 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: with urllib.request.urlopen(req, timeout=timeout) as response: body = response.read().decode('utf-8') @@ -115,7 +132,7 @@ def request( if rate_limit_count >= max_429_retries: raise last_error - if attempt < retries - 1: + if attempt < effective_retries - 1: if e.code == 429: # Respect Retry-After header, fall back to exponential backoff 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 else: 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: delay = RETRY_DELAY * (2 ** attempt) time.sleep(delay) except urllib.error.URLError as e: log(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)) except json.JSONDecodeError as e: log(f"JSON decode error: {e}") @@ -143,9 +180,11 @@ def request( # Handle socket-level errors (connection reset, timeout, etc.) log(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)) + attempt += 1 + if last_error: raise last_error raise HTTPError("Request failed with no error details") diff --git a/tests/test_http_v3.py b/tests/test_http_v3.py index a50a61a..240efcd 100644 --- a/tests/test_http_v3.py +++ b/tests/test_http_v3.py @@ -104,3 +104,79 @@ class TestParamsEncoding(unittest.TestCase): sent_url = self._sent_url(mock_urlopen) self.assertIn("count=25", 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) From 719cdef2fb29008bf3ac34740337e80f1d5aa552 Mon Sep 17 00:00:00 2001 From: Trevin Chow Date: Sat, 16 May 2026 21:13:17 -0700 Subject: [PATCH 2/2] fix(http): contain DNS retry-budget widening to DNS path only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #382 introduced an `effective_retries` widening on the first gaierror, but the widening leaked: every non-DNS error path (HTTPError, non-DNS URLError, OSError) was gated on `effective_retries - 1` and so inherited the expanded bound. A caller passing `retries=2` who hit DNS-then-non-DNS got 3 attempts instead of 2 — contrary to the PR description and the fail-fast intent of small retry budgets. Fix: - Gate every non-DNS sleep/retry decision on the caller's original `retries`, not the widened `effective_retries`. - Add an explicit `break` in each non-DNS branch when the original budget is exhausted, so the (possibly widened) outer loop bound can't pull us into an extra attempt. Adds two regression tests covering the DNS-then-non-DNS-URLError and DNS-then-OSError sequences flagged in Greptile review on PR #382. --- skills/last30days/scripts/lib/http.py | 26 +++++++++++++++--- tests/test_http_v3.py | 38 +++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/skills/last30days/scripts/lib/http.py b/skills/last30days/scripts/lib/http.py index 06a69a2..8afd464 100644 --- a/skills/last30days/scripts/lib/http.py +++ b/skills/last30days/scripts/lib/http.py @@ -132,7 +132,9 @@ def request( if rate_limit_count >= max_429_retries: raise last_error - if attempt < effective_retries - 1: + # HTTP errors respect the caller's original `retries`; only DNS + # failures get the widened `effective_retries` budget. + if attempt < retries - 1: if e.code == 429: # Respect Retry-After header, fall back to exponential backoff retry_after = e.headers.get("Retry-After") if hasattr(e, 'headers') else None @@ -143,10 +145,15 @@ def request( delay = RETRY_DELAY * (2 ** attempt) + 1 else: delay = RETRY_DELAY * (2 ** attempt) + 1 # 3s, 5s, 9s... - log(f"Rate limited (429). Waiting {delay:.1f}s before retry {attempt + 2}/{effective_retries}") + log(f"Rate limited (429). Waiting {delay:.1f}s before retry {attempt + 2}/{retries}") else: delay = RETRY_DELAY * (2 ** attempt) time.sleep(delay) + else: + # Caller's original retry budget exhausted; an earlier DNS + # failure may have widened `effective_retries`, but that + # widening is DNS-only — don't grant extra HTTP attempts. + break except urllib.error.URLError as e: log(f"URL Error: {e.reason}") last_error = HTTPError(f"URL Error: {e.reason}") @@ -170,8 +177,15 @@ def request( f"retrying in {delay:.1f}s" ) time.sleep(delay) - elif attempt < effective_retries - 1: + elif attempt < retries - 1: + # Non-DNS URLError (e.g. ConnectionRefused) respects the + # caller's original retry budget, not the DNS-widened bound. time.sleep(RETRY_DELAY * (attempt + 1)) + else: + # Caller's original retry budget exhausted; an earlier DNS + # failure widening `effective_retries` does not carry over + # to non-DNS error paths. + break except json.JSONDecodeError as e: log(f"JSON decode error: {e}") last_error = HTTPError(f"Invalid JSON response: {e}") @@ -180,8 +194,12 @@ def request( # Handle socket-level errors (connection reset, timeout, etc.) log(f"Connection error: {type(e).__name__}: {e}") last_error = HTTPError(f"Connection error: {type(e).__name__}: {e}") - if attempt < effective_retries - 1: + if attempt < retries - 1: + # Socket errors respect the caller's original retry budget. time.sleep(RETRY_DELAY * (attempt + 1)) + else: + # Original budget exhausted; DNS widening doesn't apply here. + break attempt += 1 diff --git a/tests/test_http_v3.py b/tests/test_http_v3.py index 240efcd..9fbb876 100644 --- a/tests/test_http_v3.py +++ b/tests/test_http_v3.py @@ -180,3 +180,41 @@ class TestDNSResolutionRetry(unittest.TestCase): # Caller passed retries=2, and non-DNS URLError doesn't expand it. self.assertEqual(mock_urlopen.call_count, 2) + + @patch("lib.http.urllib.request.urlopen") + @patch("lib.http.time.sleep") + def test_dns_widening_does_not_leak_into_subsequent_non_dns_urlerror( + self, mock_sleep, mock_urlopen + ): + """Mixed sequence: DNS-then-non-DNS must respect caller's original retries. + + Without the fix, the first gaierror widens effective_retries from 2 to + MIN_DNS_RETRIES=3, and a subsequent ConnectionRefused on attempt 1 + slips into a third overall attempt — exceeding what the caller asked + for. Each non-DNS error path must gate on the original `retries`. + """ + import socket + dns_err = urllib.error.URLError(socket.gaierror(-2, "Name or service not known")) + conn_err = urllib.error.URLError(ConnectionRefusedError(111, "Connection refused")) + mock_urlopen.side_effect = [dns_err, conn_err, conn_err] # 3rd would only fire if budget leaked + + with self.assertRaises(http.HTTPError): + http.request("GET", "http://flaky.example", retries=2) + + # Caller asked for at most 2 attempts. DNS widening must not give us a 3rd. + self.assertEqual(mock_urlopen.call_count, 2) + + @patch("lib.http.urllib.request.urlopen") + @patch("lib.http.time.sleep") + def test_dns_widening_does_not_leak_into_subsequent_oserror( + self, mock_sleep, mock_urlopen + ): + """Mixed sequence: DNS-then-OSError must respect caller's original retries.""" + import socket + dns_err = urllib.error.URLError(socket.gaierror(-2, "Name or service not known")) + mock_urlopen.side_effect = [dns_err, TimeoutError("timed out"), TimeoutError("timed out")] + + with self.assertRaises(http.HTTPError): + http.request("GET", "http://flaky.example", retries=2) + + self.assertEqual(mock_urlopen.call_count, 2)