fix: multigateway - #89
Conversation
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughKuboCAS now supports normalized multi-gateway reads with sequential failover, circuit-breaker health tracking, per-gateway concurrency limits, credential-safe redirects, optional CID verification, improved range handling, and expanded tests. Verification exceptions are publicly re-exported. ChangesMulti-gateway reads
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant KuboCAS
participant GatewayA
participant GatewayB
Caller->>KuboCAS: load(CID)
KuboCAS->>GatewayA: Request content with scoped credentials
GatewayA-->>KuboCAS: Error, invalid range, or mismatched content
KuboCAS->>GatewayB: Sequential failover request
GatewayB-->>KuboCAS: Verified content
KuboCAS-->>Caller: Content or aggregated failures
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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 (3)
tests/test_k15_multi_gateway_failover.py (2)
487-493: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an identity-multihash case to cover the last branch of
_cid_is_verifiable.
store_httpx.pyline 156 returnsFalsefor identity multihashes; nothing here exercises it.💚 Suggested addition
assert not store_module._cid_is_verifiable(GOOD_CID, 0, None) assert not store_module._cid_is_verifiable(GOOD_CID, None, 4) + identity_cid = CID("base32", 1, "raw", multihash.digest(BODY, "identity")) + assert not store_module._cid_is_verifiable(identity_cid, None, None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_k15_multi_gateway_failover.py` around lines 487 - 493, Add an assertion in test_dag_pb_cids_are_not_verifiable covering a CID whose multihash uses the identity codec, and verify that store_module._cid_is_verifiable returns False for it. Keep the existing dag-pb and argument-validation assertions unchanged.
381-381: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
import timeto the module imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_k15_multi_gateway_failover.py` at line 381, Move the time import from its current local position to the module-level import section in tests/test_k15_multi_gateway_failover.py, keeping the existing time usage unchanged.py_hamt/store_httpx.py (1)
1009-1014: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring and implementation disagree on
response_bytes.The docstring says
stats"accumulates the byte count ... across every gateway attempted", but line 1034 overwrites rather than accumulates (retries do accumulate). If overwrite is intended — reporting only the bytes actually returned to the caller — reword the docstring; otherwise use+=. As written a failing leg's byte count also lingers when a later leg raises before reading a body.Also applies to: 1034-1034
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@py_hamt/store_httpx.py` around lines 1009 - 1014, Update the gateway-fetch method’s response_bytes handling and docstring consistently: accumulate bytes across all gateway attempts with +=, and ensure a failed leg cannot leave stale byte counts when a later attempt raises before reading its body. Keep retry accumulation and load’s whole-operation stats behavior aligned with the resulting implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@py_hamt/store_httpx.py`:
- Around line 616-620: The shared _gateway_health state uses event-loop-relative
timestamps, causing incorrect cooldown comparisons across loops. Update
record_failure and is_healthy to use process-wide time.monotonic() instead of
asyncio.get_running_loop().time(), and remove any unnecessary async-context
requirement while preserving the existing health and cooldown behavior.
- Around line 1059-1064: Update the load flow surrounding _verify_cid_content so
verification exceptions set stats.status to the appropriate failure status
before propagating through gateway failover. Ensure the final end_cas_load call
reports failure when all gateways return mismatched content, while preserving
the existing success status for verified reads.
In `@tests/test_k15_multi_gateway_failover.py`:
- Around line 300-302: Update the test around the load assertion so the
documented further failure is actually exercised: append another (500, b"")
response to responses before the GOOD_CID load, while preserving the existing
assertions for the returned body and gateway_b hit count.
---
Nitpick comments:
In `@py_hamt/store_httpx.py`:
- Around line 1009-1014: Update the gateway-fetch method’s response_bytes
handling and docstring consistently: accumulate bytes across all gateway
attempts with +=, and ensure a failed leg cannot leave stale byte counts when a
later attempt raises before reading its body. Keep retry accumulation and load’s
whole-operation stats behavior aligned with the resulting implementation.
In `@tests/test_k15_multi_gateway_failover.py`:
- Around line 487-493: Add an assertion in test_dag_pb_cids_are_not_verifiable
covering a CID whose multihash uses the identity codec, and verify that
store_module._cid_is_verifiable returns False for it. Keep the existing dag-pb
and argument-validation assertions unchanged.
- Line 381: Move the time import from its current local position to the
module-level import section in tests/test_k15_multi_gateway_failover.py, keeping
the existing time usage unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b2864381-668e-412e-b751-9bd2a75dc8e2
📒 Files selected for processing (3)
py_hamt/__init__.pypy_hamt/store_httpx.pytests/test_k15_multi_gateway_failover.py
![da-code-reviewer[bot]](https://avatars.githubusercontent.com/u/78453614?s=60&v=4){kind=small}
| while retry_count <= self.max_retries: | ||
| try: | ||
| async with semaphore: # Throttle each gateway attempt | ||
| response = await client.get(url, headers=headers or None) |
There was a problem hiding this comment.
HIGH
The same configured client—and therefore its default Authorization, auth, and custom headers—is used for every gateway origin. A private primary with a public fallback will leak credentials to that fallback. Bind credentials per gateway or strip sensitive headers when the request origin changes.
| if cid.codec.code == KuboCAS.DAG_PB_MARKER: | ||
| return False | ||
| # An identity multihash inlines its content in the CID; nothing was fetched. | ||
| return cid.hashfun.name != "identity" |
There was a problem hiding this comment.
MEDIUM
Identity CIDs are excluded from verification even though load() still fetches and returns gateway bytes. A malicious gateway can therefore substitute content despite verify_content=True. Compare the response with cid.raw_digest (or return the embedded bytes) instead of skipping verification.
| mismatch; we cannot prove the content wrong, so the read is allowed through. | ||
| """ | ||
| try: | ||
| computed = multihash.digest(data, cid.hashfun.name) |
There was a problem hiding this comment.
MEDIUM
This computes the hash at its default length, but valid multihashes may be truncated, and variable-output hashes require an explicit size. Correct truncated content is rejected, while the broad exception handler silently skips verification for variable-output hashes. Compute with size=len(cid.raw_digest).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #89 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 8 8
Lines 3021 3166 +145
==========================================
+ Hits 3021 3166 +145 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| if strip_credentials: | ||
| safe_headers = { | ||
| name: value | ||
| for name, value in client.headers.items() |
There was a problem hiding this comment.
HIGH
Do not forward arbitrary client headers cross-origin. This copies every header not in the three-name denylist to the fallback. Since KuboCAS supports authentication through arbitrary headers, credentials such as X-API-Key or X-Auth-Token will still be disclosed to a public fallback after the primary fails. Use per-origin header configuration or an explicit allowlist of headers safe to forward.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_k15_multi_gateway_failover.py`:
- Around line 703-706: Update the failover assertions in the test around
fallback host handling to compare fallback["host"] directly with gateway_a’s
parsed host value, rather than using substring membership against gateway_a.url.
Preserve the existing assertion that fallback includes ordinary content
negotiation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 88111141-8448-444c-96c1-31f0ebd9521a
📒 Files selected for processing (3)
py_hamt/__init__.pypy_hamt/store_httpx.pytests/test_k15_multi_gateway_failover.py
🚧 Files skipped from review as they are similar to previous changes (2)
- py_hamt/init.py
- py_hamt/store_httpx.py
| origin of its choosing and harvest them. Following each hop ourselves | ||
| re-applies the full origin check to the *redirect target*. | ||
| """ | ||
| if not _carries_credentials(client): |
There was a problem hiding this comment.
HIGH
_carries_credentials() ignores client cookies and default query parameters. A client using a domainless session cookie or query-token therefore takes this client.get() path, and HTTPX merges those credentials into requests to foreign fallback gateways. Conversely, the manual path constructs a raw Request and drops them even for the primary. Build requests with the client configuration, then explicitly remove cookie/query credentials for non-credentialed origins.
2 participants
Summary by CodeRabbit