Skip to content

Deduplicate ipfs-retriever candidates by service provider - #689

Draft
juliangruber wants to merge 1 commit into
serve-ipfs-retrievalsfrom
dedupe-ipfs-candidates-by-provider
Draft

Deduplicate ipfs-retriever candidates by service provider#689
juliangruber wants to merge 1 commit into
serve-ipfs-retrievalsfrom
dedupe-ipfs-candidates-by-provider

Conversation

@juliangruber

@juliangruber juliangruber commented Jun 25, 2026

Copy link
Copy Markdown
Member

Stacked on #312.
Closes #691

Addresses #312 (comment).

Problem

ipfs-retriever looks up candidates with WHERE pieces.ipfs_root_cid = ?, returning one row per piece. Under Storacha's sharding model one IPFS root CID maps to N pieces (shards) on the same service provider, so the lookup yields N candidates that differ only by piece, all with the same serviceUrl.

Retrieval is by IPFS root CID through the trustless gateway. retrieveIpfsContent uses only serviceUrl and ipfsRootCid, never the piece, so every one of those candidates is the exact same HTTP request. selectRetrievalCandidate then retries across all N on failure, hitting a genuinely failing provider N times before returning the 502.

Fix

Deduplicate candidates by service provider in validateQueryResultsAndGetCandidates, after the authorization and quota cascade. A failing provider is now attempted once rather than once per shard, while retries across distinct providers (content replicated across providers) are preserved. This also makes the code match the existing JSDoc, which already documents "one per service provider".

The dedup lives in the ipfs-retriever lookup rather than the shared selectRetrievalCandidate, because piece-retriever retrieves by piece CID, where candidates sharing a serviceUrl are distinct requests and must not be collapsed.

One IPFS root CID can map to many pieces (shards) on the same service
provider. Retrieval is by root CID and never uses the piece, so those
candidates are identical retrieval requests. Keep one candidate per service
provider so a failing provider is attempted once rather than once per shard,
while still retrying across distinct providers.
@juliangruber

Copy link
Copy Markdown
Member Author

Can we deduplicate at the SQL level?

Technically yes, with a GROUP BY or a window function. I'd recommend against it here, because doing it correctly forces us to give up something the current design deliberately keeps. The reason is an ordering constraint.

Dedup must happen after authorization, not before

A group of "same service_provider_id" rows for one root CID is not homogeneous. The shared query only filters on ipfs_root_cid (and is_deleted). The payer match, with_cdn, sanctioned check, service_url, and quotas are all applied later by the JS cascade. So shards on the same provider can sit in different data sets with different payer_address, with_cdn, quota, and so on.

If we GROUP BY service_provider_id over the raw join (picking an arbitrary representative per group) before auth, SQLite can hand us a row that fails authorization while discarding a sibling row on the same provider that would have passed, so we'd reject a valid candidate. The JS version is correct precisely because it dedups the already-authorized set: store.js runs the cascade, then collapses.

To dedup safely in SQL we'd have to push the whole auth and quota filter into the query:

SELECT service_provider_id, service_url, data_set_id, piece_id, ipfs_root_cid, ...
FROM pieces
JOIN data_sets ON ...
JOIN service_providers ON ...
LEFT JOIN data_set_egress_quotas ON ...
LEFT JOIN wallet_details ON ...
WHERE pieces.ipfs_root_cid = ?
  AND pieces.is_deleted IS FALSE
  AND service_providers.id IS NOT NULL AND service_providers.is_deleted IS FALSE
  AND LOWER(data_sets.payer_address) = ?
  AND data_sets.with_cdn = 1
  AND COALESCE(wallet_details.is_sanctioned, 0) = 0
  AND (NOT ? OR (COALESCE(cdn_egress_quota, 0) > 0 AND COALESCE(cache_miss_egress_quota, 0) > 0))
GROUP BY service_provider_id   -- or ROW_NUMBER() OVER (PARTITION BY service_provider_id) = 1

Why that's a net loss

  • We lose the cascade's granular errors. Today the JS cascade returns specific codes and messages: 404 "not indexed", 402 "no payment rail", 402 "withCDN=false", 403 "sanctioned", 402 "CDN/cache-miss quota exhausted". Once everything is a single WHERE, a zero-row result can't tell the client why, collapsing all of those into one generic 404 or 402.
  • Bigger blast radius. buildRetrievalCandidateQuery is shared with piece-retriever, and the payer filter is currently JS-side and case-insensitive (toLowerCase()). Moving it into SQL changes the shared contract for both workers.
  • No retrieval benefit. The problem this PR addresses is redundant HTTP retries against a failing provider. The JS dedup already eliminates that completely. SQL dedup would only shrink an already-small, already-authorized result set (the shard count for one root CID and payer), it doesn't make a single retrieval faster or more correct. The lookup cost itself is handled by the pieces_ipfs_root_cid index.
  • SQLite-specific smell. Bare columns under GROUP BY pick an arbitrary row (a SQLite extension), so the portable form is a ROW_NUMBER() window, which is more SQL to maintain for no functional gain.

Recommendation

Keep the dedup in JS. It's trivially correct because it runs on the post-authorization set, it preserves the specific error responses, and it fully fixes the redundant-retry problem. SQL-level dedup would only pay off if shard counts got large enough that transferring the rows mattered, and even then the right lever is the index plus maybe a LIMIT, not folding the auth cascade into SQL.

@juliangruber juliangruber mentioned this pull request Jun 25, 2026
16 tasks
@BravoNatalie
BravoNatalie self-requested a review June 26, 2026 00:27
@BravoNatalie BravoNatalie self-assigned this Jun 26, 2026
@BravoNatalie
BravoNatalie removed their request for review June 26, 2026 00:27
@BravoNatalie BravoNatalie linked an issue Jun 26, 2026 that may be closed by this pull request
4 tasks
@BigLep BigLep added this to FOC Jul 7, 2026
@github-project-automation github-project-automation Bot moved this to 📌 Triage in FOC Jul 7, 2026
@BigLep BigLep moved this from 📌 Triage to ⌨️ In Progress in FOC Jul 8, 2026
@BigLep BigLep moved this from ⌨️ In Progress to 🐱 Todo in FOC Jul 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: deduplicate retrieval candidates by service provider in ipfs-retriever

3 participants