Add retries for connect failures - #14
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughVersion bumped 0.5.7 → 0.5.8. Added a module-level Retry configuration and a shared HTTP session in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant ApiSupport as api_support._session
participant Remote as Remote API
Caller->>ApiSupport: make HTTP request (GET/POST/...)
ApiSupport->>Remote: send request
alt transient failure
Remote--x ApiSupport: connection error / timeout
ApiSupport->>ApiSupport: Retry per _retry policy
ApiSupport->>Remote: resend request
end
Remote-->>ApiSupport: response
ApiSupport-->>Caller: return response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Add retries for connect failures by routing all
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/freeplay/api_support.py (1)
14-17: Consider addingstatus_forcelistfor server error retries.The current configuration only retries on connection failures. Server errors (e.g., 502/503/504) that often indicate transient issues won't trigger retries. If the goal is broader resilience, consider:
-_retry = Retry(connect=3, backoff_factor=0.5) +_retry = Retry( + connect=3, + read=3, + status=3, + status_forcelist=[502, 503, 504], + backoff_factor=0.5, + allowed_methods=["HEAD", "GET", "PUT", "DELETE", "OPTIONS", "TRACE", "POST"], +)Note: Adding
POSTtoallowed_methodsis needed if you want POST requests to retry on status codes (connect retries happen regardless of method).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/freeplay/api_support.py` around lines 14 - 17, The Retry instance named _retry currently only retries on connection failures; update its constructor to include a status_forcelist like {502,503,504} and (if you want POSTs retried) extend allowed_methods to include "POST" so HTTPAdapter(max_retries=_retry) will also retry on transient server errors; modify the _retry initialization (the Retry(...) call) to add status_forcelist and allowed_methods and keep the _session mounting of HTTPAdapter as-is to enable server-error retries for _session requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/freeplay/api_support.py`:
- Around line 14-17: The Retry instance named _retry currently only retries on
connection failures; update its constructor to include a status_forcelist like
{502,503,504} and (if you want POSTs retried) extend allowed_methods to include
"POST" so HTTPAdapter(max_retries=_retry) will also retry on transient server
errors; modify the _retry initialization (the Retry(...) call) to add
status_forcelist and allowed_methods and keep the _session mounting of
HTTPAdapter as-is to enable server-error retries for _session requests.
This allows our library to reconnect on failures to fetch prompt templates and other calls to the Freeplay server.
Summary by CodeRabbit
Chores
Improvements