Skip to content

Conversation

@Faolain
Copy link
Contributor

@Faolain Faolain commented Jun 4, 2025

Summary

  • allow KuboCAS to receive multiple gateway URLs and race requests
  • test that multi-gateway loading works

Testing

  • bash run-checks.sh

https://chatgpt.com/codex/tasks/task_e_683fe3c2c39c8322b1fc24eb58ce9ab2


Perform an AI-assisted review on CodePeer.com

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jun 4, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@Faolain
Copy link
Contributor Author

Faolain commented Jun 4, 2025

Autogenerated PR from here https://chatgpt.com/s/cd_683fe5a164fc8191b8bbb6f04d336fe4 no idea if it was any good just wanted to give it a "real task" and see how it would fare. None of this is my code it's autogenerated but uses my account to make commits.

ChatGPT added the following comments:

Must-fix issues

| Problem | Why it matters | Minimal fix

-- | -- | -- | --
1 | _normalize() can double-insert “/ipfs/”"http://gw/ipfs" → "http://gw/ipfs/ipfs/" | Broken URLs for users who already pass the full path (quite common with cloud gateways). | ```python
def _normalize(u: str) -> str: |   |   |
return u.rstrip("/") if u.endswith("/ipfs") or u.endswith("/ipfs/") else u.rstrip("/") + "/ipfs"

| 2 | Outstanding tasks are cancelled but not awaited | asyncio logs “Task was destroyed but it is pending!” on Python 3.12+ under -X dev, and the connector can leak sockets. | After cancelling, run: await asyncio.gather(*tasks, return_exceptions=True) |

| 3 | Concurrency semaphore is acquired inside the race | If you call load() with many gateway URLs, you still serialise them one-by-one. That defeats the whole point of the race. | Move async with self._sem outside _fetch() or use a reader semaphore (one per save, one per load) rather than a global one. |


2 Nice-to-have improvements

# Topic Note / suggestion
4 Back-pressure & retries A single transient 5xx on the first gateway drops immediately to the next, but the retry budget is effectively zero. Consider an exponential back-off or at least a single retry per gateway.
5 Weighted or ordered gateways Right now you always hit all gateways first. Let callers optionally mark a primary gateway (simple ordered list) or give each URL a weight.
6 Session fan-out vs. per-loop cache The per-event-loop cache is good, but remember that new loops created after KuboCAS() will silently create new sessions you never close if the instance lives long. A weak-value dict or finaliser hook could help.
7 CID coercion in save() Re-encoding a dag-pb CID to raw or dag-cbor after upload yields a logical CID that the gateway cannot serve (the underlying block on IPFS is still dag-pb). It hasn’t bitten us because tests always upload a small blob and immediately fetch it back, but it will break if someone elsewhere tries to ipfs cat that CID. Either (a) call /add?raw-leaves=true&cid-version=1 when codec=="raw", or (b) document that only dag-pb output is guaranteed.
8 Gateway health-checking A quick HEAD /ipfs/<junk> at startup lets you hard-exclude clearly dead URLs from the race, saving an RTT on every load().
9 Timeouts ClientTimeout(total=60) is fine for small blocks but too low for 1 MB chunk-range requests over slow links. Expose it as a kwarg.

3 Style / documentation nits

  • gateway_base_url and gateway_base_urls – the redundancy is fine for back-compat, but mark gateway_base_url as deprecated in the docstring so users migrate.
  • Update the README snippet that shows “one gateway only”.
  • The big docstring block is great; trim repeated paragraphs (session management is explained twice).
  • Typo: “will be used to create a new aiohttp.ClientSession if one is not provided” (missing “not”).

4 Test-suite comments

  • The new failing-gateway tests are excellent. Consider adding a test where the second URL returns first (shorter delay) to be 100 % sure we cancel the slower, still-running task.
  • Hypothesis deadline = 1000 ms is occasionally flaky on CI under load; bump to 1500 ms or pin MAX_SLOW marker.

Verdict

Functionally correct and a step forward, but I recommend fixing the three blocking items (URL normalisation, task-cancellation cleanup, semaphore placement) and, if bandwidth permits, the “CID re-encode” quirk before merging into main.

Let me know if you’d like a patch or a deeper dive on any of the above!

@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (48800e1) to head (8af4db8).

Additional details and impacted files
@@            Coverage Diff             @@
##           refactor       #63   +/-   ##
==========================================
  Coverage    100.00%   100.00%           
==========================================
  Files             5         5           
  Lines           597       619   +22     
==========================================
+ Hits            597       619   +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Faolain Faolain mentioned this pull request Jun 5, 2025
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants