Skip to content

🛡️ Sentinel: Stop retrying on 4xx client errors #188

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

Closed
abhimehro wants to merge 1 commit into from
Closed

🛡️ Sentinel: Stop retrying on 4xx client errors #188

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,14 @@
response.raise_for_status()
return response
except (httpx.HTTPError, httpx.TimeoutException) as e:
# Security: Don't retry on client errors (4xx) unless it's rate limiting (429)
if isinstance(e, httpx.HTTPStatusError):
status = e.response.status_code
if 400 <= status < 500 and status != 429:
if hasattr(e, "response") and e.response is not None:
log.debug(f"Response content: {sanitize_for_log(e.response.text)}")

Check notice

Code scanning / Pylintpython3 (reported by Codacy)

Use lazy % formatting in logging functions Note

Use lazy % formatting in logging functions
Comment on lines +605 to +608
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the HTTPStatusError branch, e.response is accessed to read status_code before the hasattr(e, "response") / e.response is not None guard. That guard is therefore ineffective (and redundant for HTTPStatusError, which always carries a response from raise_for_status()). Consider removing the guard here, or moving the response existence check before reading status_code (e.g., via getattr(e, "response", None)).

Suggested change
status = e.response.status_code
if 400 <= status < 500 and status != 429:
if hasattr(e, "response") and e.response is not None:
log.debug(f"Response content: {sanitize_for_log(e.response.text)}")
# Use getattr to avoid AttributeError if a HTTPStatusError ever lacks a response
response_obj = getattr(e, "response", None)
status = response_obj.status_code if response_obj is not None else None
if status is not None and 400 <= status < 500 and status != 429:
if response_obj is not None:
log.debug(
f"Response content: {sanitize_for_log(response_obj.text)}"
)
# Don't retry non-rate-limited client errors

Copilot uses AI. Check for mistakes.
raise
Comment on lines +607 to +609
Copy link

@gemini-code-assist gemini-code-assist bot Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This check for e.response is redundant. The isinstance(e, httpx.HTTPStatusError) check on line 604 already guarantees that e.response exists and is not None. You can simplify this block by removing the conditional check around the logging statement.

Suggested change
if hasattr(e, "response") and e.response is not None:
log.debug(f"Response content: {sanitize_for_log(e.response.text)}")
raise
log.debug(f"Response content: {sanitize_for_log(e.response.text)}")
raise

Comment on lines +603 to +609
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change introduces new retry behavior (do not retry 4xx except 429), but there doesn't appear to be a test asserting the retry count/behavior for (a) a non-429 4xx (should call the request func once) and (b) 429 (should still retry). Adding focused unit tests around _retry_request for these status codes will prevent regressions.

Copilot generated this review using guidance from repository custom instructions.

if attempt == max_retries - 1:
if hasattr(e, "response") and e.response is not None:
log.debug(f"Response content: {sanitize_for_log(e.response.text)}")
Expand Down Expand Up @@ -1138,7 +1146,7 @@
batch_idx,
len(batch_data),
)
return batch_data

Check warning

Code scanning / Prospector (reported by Codacy)

Use lazy % formatting in logging functions (logging-fstring-interpolation) Warning

Use lazy % formatting in logging functions (logging-fstring-interpolation)
except httpx.HTTPError as e:
if USE_COLORS:
sys.stderr.write("\n")
Expand Down
Loading