Skip to content

feat: retry on more httpx exceptions#467

Open
Rajat-Ahuja1997 wants to merge 1 commit intomainfrom
rajat/generalize-sdk-retry
Open

feat: retry on more httpx exceptions#467
Rajat-Ahuja1997 wants to merge 1 commit intomainfrom
rajat/generalize-sdk-retry

Conversation

@Rajat-Ahuja1997
Copy link
Copy Markdown
Collaborator

@Rajat-Ahuja1997 Rajat-Ahuja1997 commented Mar 30, 2026

Summary by CodeRabbit

Bug Fixes

  • Improved HTTP client error handling to properly distinguish between timeout errors and network/transport connection failures with more specific error messages.

Refactor

  • Consolidated exception mapping logic in HTTP clients to reduce code duplication and enhance consistency across error scenarios while preserving retry behavior.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 30, 2026

Walkthrough

The pull request consolidates HTTP exception handling in both sync and async HTTP clients. Multiple httpx failure handlers (TimeoutException, ConnectError, NetworkError, RemoteProtocolError) are unified into single catch blocks that differentiate between timeout and transport/protocol errors, mapping them to appropriate SDK exception types while preserving retry logic.

Changes

Cohort / File(s) Summary
HTTP Exception Handling Consolidation
sdks/python/src/honcho/http/async_client.py, sdks/python/src/honcho/http/client.py
Consolidated multiple httpx exception handlers into unified blocks. Timeouts map to SDK's TimeoutError with timeout details; other transport/protocol/network errors map to ConnectionError. Retry control flow and backoff strategy unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 Errors once scattered now stand as one,
Timeouts and networks in sync—refactoring done!
The rabbit hops through cleaner code,
Where exception paths are fewer and toad. 🍄

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: retry on more httpx exceptions' directly describes the main change: expanding the retry logic to handle additional httpx exception types across both async and sync HTTP clients.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch rajat/generalize-sdk-retry

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
sdks/python/src/honcho/http/client.py (1)

134-147: ⚠️ Potential issue | 🔴 Critical

Guard retries for non-idempotent methods on transport/protocol failures.

httpx.NetworkError subclasses (WriteError, ReadError, CloseError) and httpx.RemoteProtocolError can occur after the request bytes have been sent to the server. Retrying these blindly causes duplicate writes for POST, PATCH, and other non-idempotent methods. The current code at lines 135–147 retries all transport errors without checking method idempotency.

Proposed fix
             ) as e:
                 if isinstance(e, httpx.TimeoutException):
                     error = TimeoutError(f"Request timed out after {request_timeout}s")
                 else:
                     error = ConnectionError(f"Connection error: {e}")
+
+                method_is_idempotent = method.upper() in {
+                    "GET",
+                    "HEAD",
+                    "OPTIONS",
+                    "PUT",
+                    "DELETE",
+                }
+                has_idempotency_key = any(
+                    key.lower() == "idempotency-key" for key in request_headers
+                )
+
                 if attempt < self.max_retries:
+                    if not method_is_idempotent and not has_idempotency_key:
+                        raise error from e
                     last_error = error
                     time.sleep(self._get_retry_delay(attempt))
                     attempt += 1
                     continue
                 raise error from e
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@sdks/python/src/honcho/http/client.py` around lines 134 - 147, The retry
logic currently retries httpx.NetworkError and httpx.RemoteProtocolError for all
HTTP methods; change it so only httpx.TimeoutException is always retried, while
NetworkError/RemoteProtocolError are retried only for idempotent methods. In the
except block (where variables attempt, self.max_retries, request_timeout, and
self._get_retry_delay are used and the local variable request represents the
outgoing Request), detect the HTTP method via request.method.upper() and only
set last_error and sleep/retry when the method is idempotent (e.g., GET, HEAD,
OPTIONS, PUT, DELETE); for non-idempotent methods (POST, PATCH, etc.) re-raise
or return the constructed ConnectionError immediately instead of retrying. Keep
the existing error messages (TimeoutError vs ConnectionError) but guard retries
for protocol/transport errors by method idempotency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@sdks/python/src/honcho/http/client.py`:
- Around line 134-147: The retry logic currently retries httpx.NetworkError and
httpx.RemoteProtocolError for all HTTP methods; change it so only
httpx.TimeoutException is always retried, while NetworkError/RemoteProtocolError
are retried only for idempotent methods. In the except block (where variables
attempt, self.max_retries, request_timeout, and self._get_retry_delay are used
and the local variable request represents the outgoing Request), detect the HTTP
method via request.method.upper() and only set last_error and sleep/retry when
the method is idempotent (e.g., GET, HEAD, OPTIONS, PUT, DELETE); for
non-idempotent methods (POST, PATCH, etc.) re-raise or return the constructed
ConnectionError immediately instead of retrying. Keep the existing error
messages (TimeoutError vs ConnectionError) but guard retries for
protocol/transport errors by method idempotency.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fdf74c38-16ea-4eb1-941b-07012ec3f427

📥 Commits

Reviewing files that changed from the base of the PR and between a5423b5 and 6d28667.

📒 Files selected for processing (2)
  • sdks/python/src/honcho/http/async_client.py
  • sdks/python/src/honcho/http/client.py
Copy link
Copy Markdown
Collaborator

@VVoruganti VVoruganti left a comment

Choose a reason for hiding this comment

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

Increment the versions by a patch 2.1.1 and update changelogs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants