fix: timeouts with delays #80
Conversation
|
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
py_hamt/store_httpx.py (1)
309-347: Consider extracting retry logic to reduce code duplicationThe retry logic is duplicated between
save()andload()methods. This violates the DRY principle and makes maintenance harder.Consider extracting the retry logic into a generic helper method:
async def _retry_with_backoff(self, operation_name: str, operation_callable): """Execute an operation with retry and exponential backoff.""" retry_count = 0 while retry_count <= self.max_retries: try: return await operation_callable() except (httpx.TimeoutException, httpx.RequestError) as e: retry_count += 1 if retry_count > self.max_retries: raise httpx.TimeoutException( f"Failed to {operation_name} after {self.max_retries} retries: {str(e)}", request=e.request if isinstance(e, httpx.RequestError) else None, ) # Calculate backoff delay delay = self.initial_delay * (self.backoff_factor ** (retry_count - 1)) # Add jitter to prevent thundering herd jitter = delay * 0.1 * (random.random() - 0.5) await asyncio.sleep(delay + jitter) except httpx.HTTPStatusError: # Re-raise non-timeout HTTP errors immediately raise raise RuntimeError("Exited the retry loop unexpectedly.")Then simplify the methods to use this helper.
Also applies to: 350-383
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
py_hamt/store_httpx.py(4 hunks)tests/test_kubo_cas.py(2 hunks)tests/test_public_gateway.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Create project environment, run all checks (3.12)
🔇 Additional comments (6)
py_hamt/store_httpx.py (1)
2-2: LGTM!The addition of the
randommodule is appropriate for implementing jitter in the retry backoff mechanism.tests/test_public_gateway.py (1)
107-107: LGTM!Setting
max_retries=0appropriately disables the retry mechanism for these public gateway tests, ensuring they test direct request handling without retry interference.Also applies to: 138-138, 201-203
tests/test_kubo_cas.py (4)
171-260: LGTM!Comprehensive test coverage for retry behavior, including success scenarios and max retry failures for both save and load operations.
262-305: LGTM!Well-designed test that properly verifies exponential backoff timing with appropriate tolerances for jitter.
307-336: LGTM!Correctly verifies that HTTP status errors bypass the retry mechanism and are immediately re-raised.
338-349: Tests may need update based on parameter validationThese tests use
max_retries=-1to trigger the RuntimeError. If parameter validation is added as suggested, these tests would fail during initialization with a ValueError instead.Consider updating these tests to use a different approach to trigger the RuntimeError, or adjust them to test the parameter validation instead.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #80 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 673 707 +34
=========================================
+ Hits 673 707 +34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
py_hamt/store_httpx.py (2)
231-240: Parameter validation implemented — resolves prior reviewThe validation for max_retries, initial_delay, and backoff_factor is in place and matches the earlier recommendation.
386-389: Jitter now centered around zero — thundering herd mitigation fixedThe jitter formula uses (random.random() - 0.5), correctly centering around zero. This resolves the earlier issue where all jitter was positive.
Also applies to: 422-425
🧹 Nitpick comments (5)
py_hamt/store_httpx.py (3)
115-131: Document the new retry/backoff parameters in the constructor docsThe “Parameters” section lists hasher/client/headers/auth/rpc_base_url/gateway_base_url/chunker but omits the new retry controls. Please add brief bullets for max_retries (int, retries after the first attempt), initial_delay (seconds), and backoff_factor (multiplier). This avoids ambiguity about whether “N retries” includes the initial attempt (it does not: attempts = 1 + max_retries).
### Parameters - **hasher** (str): multihash name (defaults to *blake3*). @@ - **chunker** (str): chunking algorithm specification for Kubo's `add` RPC. Accepted formats are `"size-<positive int>"`, `"rabin"`, or `"rabin-<min>-<avg>-<max>"`. + - **max_retries** (int): number of retry attempts on transient network errors + (timeouts/request errors). Total attempts = 1 + `max_retries`. Default: 3. + - **initial_delay** (float): base backoff delay in seconds before the first + retry. Default: 1.0. + - **backoff_factor** (float): multiplicative backoff factor applied per retry + (k-th delay = initial_delay * backoff_factor**(k-1)). Must be ≥ 1.0.
354-371: Deduplicate retry/backoff logic between save() and load()The retry loops in save and load are nearly identical. Consider extracting a small helper (e.g., _request_with_retries) to centralize backoff and error wrapping, which reduces maintenance overhead and the chance of behavioral drift between the two paths.
# Example helper (add inside KuboCAS) async def _request_with_retries(self, op_name: str, fn: callable): retry = 0 while True: try: return await fn() except (httpx.TimeoutException, httpx.RequestError) as e: if retry >= self.max_retries: raise httpx.TimeoutException( f"Failed to {op_name} data after {self.max_retries} retries: {e}", request=e.request if isinstance(e, httpx.RequestError) else None, ) delay = self.initial_delay * (self.backoff_factor ** retry) jitter = delay * 0.1 * (random.random() - 0.5) await asyncio.sleep(max(0.0, delay + jitter)) retry += 1 # Usage sketch: async with self._sem: client = self._loop_client() response = await self._request_with_retries( "save", lambda: client.post(self.rpc_url, files=files, timeout=60.0) )Also applies to: 372-393
388-389: Harden sleep against rare negative values (future-proofing)While current validation guarantees delay > 0 and ensures total (delay + jitter) ≥ 0.95*delay, adding a max(0.0, ...) guard makes this robust against future changes.
- await asyncio.sleep(delay + jitter) + await asyncio.sleep(max(0.0, delay + jitter))Also applies to: 424-425
tests/test_public_gateway.py (1)
178-182: Also disable retries for public-gateway loop to reduce flakinessIn the for-loop instantiations, consider passing max_retries=0 to avoid long backoffs if ipfs.io/dweb.link is slow. You’re already skipping when the gateway is down, so retries mainly add latency.
- cas = KuboCAS( + cas = KuboCAS( rpc_base_url="http://127.0.0.1:5001", # Keep local RPC for saves gateway_base_url=gateway_url, # Use specified gateway for loads - ) + max_retries=0, + )tests/test_kubo_cas.py (1)
231-248: Minor: align failing request method with the patched client methodYou reuse a single failing_method for both post and get, but it always constructs a “POST” request. This is harmless, but using method-appropriate dummy requests improves error messages and test readability.
- async def failing_method(url, **kwargs): - dummy_request = httpx.Request( - "POST", url - ) # Create the dummy request + async def failing_post(url, **kwargs): + dummy_request = httpx.Request("POST", url) raise httpx.TimeoutException( "Simulated timeout", request=dummy_request ) @@ - with patch.object( - httpx.AsyncClient, - "post", - new=AsyncMock(side_effect=failing_method), - ): - with patch.object( - httpx.AsyncClient, - "get", - new=AsyncMock(side_effect=failing_method), - ): + with patch.object( + httpx.AsyncClient, + "post", + new=AsyncMock(side_effect=failing_post), + ): + async def failing_get(url, **kwargs): + dummy_request = httpx.Request("GET", url) + raise httpx.TimeoutException( + "Simulated timeout", request=dummy_request + ) + with patch.object( + httpx.AsyncClient, "get", new=AsyncMock(side_effect=failing_get) + ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
py_hamt/store_httpx.py(4 hunks)tests/test_kubo_cas.py(2 hunks)tests/test_public_gateway.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_public_gateway.py (2)
py_hamt/store_httpx.py (2)
KuboCAS(76-429)aclose(283-300)tests/test_kubocas_auth.py (2)
test_user_supplied_client_auth(36-56)test_internal_client_headers(27-32)
tests/test_kubo_cas.py (1)
py_hamt/store_httpx.py (7)
KuboCAS(76-429)save(28-32)save(49-52)save(354-393)load(35-36)load(54-73)load(395-429)
py_hamt/store_httpx.py (1)
py_hamt/hamt.py (4)
load(137-138)load(158-167)load(275-284)get(592-599)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Create project environment, run all checks (3.12)
🔇 Additional comments (6)
py_hamt/store_httpx.py (1)
150-153: Retry/backoff knobs added as keyword-only — API change looks goodAdding max_retries, initial_delay, and backoff_factor at the end of the keyword-only section preserves backward compatibility and provides useful control for flaky networks. No concerns on the signature itself.
tests/test_public_gateway.py (1)
152-156: Setting max_retries=0 in gateway tests improves determinism and speedGood call disabling retries where we’re asserting URL construction and local gateway behavior. This keeps CI fast and avoids backoff noise.
Also applies to: 218-223, 282-287, 295-298
tests/test_kubo_cas.py (4)
262-305: Backoff timing assertions are well-bounded given jitterNice bounds: they tolerate ±10% around the base delays, which safely encloses the ±5% jitter window.
320-336: No-retry on HTTP 5xx is verified correctlyThe test ensures httpx.HTTPStatusError is surfaced immediately and that asyncio.sleep is not called. This matches the production behavior.
339-374: Constructor validation coverage is comprehensiveEdge cases and negative paths for max_retries, initial_delay, and backoff_factor are well covered; also validates the equality edges (0 retries, factor 1.0).
181-189: Fix construction of dummy httpx objects in mocks (invalid kwargs used)Two issues will break these tests:
- httpx.Request does not accept files=...
- httpx.Response does not accept json=... (provide JSON via content and content-type)
Patch the mock to build valid httpx objects.
@@ - async def mock_post(url, **kwargs): + async def mock_post(url, **kwargs): nonlocal timeout_count - # Manually create a dummy request object - dummy_request = httpx.Request("POST", url, files=kwargs.get("files")) + # Manually create a dummy request object + dummy_request = httpx.Request("POST", url) if timeout_count < successful_after: timeout_count += 1 raise httpx.TimeoutException("Simulated timeout", request=dummy_request) - return httpx.Response(200, json={"Hash": test_cid}, request=dummy_request) + import json + payload = json.dumps({"Hash": test_cid}).encode("utf-8") + return httpx.Response( + 200, + content=payload, + headers={"content-type": "application/json"}, + request=dummy_request, + )Additionally, ensure json is imported once at the top of the test module for clarity.
@@ -import asyncio +import asyncio +import jsonLikely an incorrect or invalid review comment.
|
lgtm as well |
Summary by CodeRabbit
New Features
Tests