PE-9203: Sync reads a single gateway, and stops letting one slow item block four - #2179
Conversation
…our PE-9203
Sync fetched every metadata item through the multi-gateway waterfall: the
configured gateway, up to two GAR gateways, then arweave.net, serially, five
seconds each. A flaky gateway turned each item into twenty seconds, and a sync
reads hundreds of them. Building that list also cost a Solana RPC call.
Sync now reads the configured gateway only - two attempts, five seconds each,
then the item is skipped. A 404 is not retried, because the same host will not
change its mind. Twenty seconds becomes ten, and the Solana RPC is gone from
the per-item path entirely.
The waterfall is untouched everywhere else. Downloads, previews, manifests,
shared links and drive attach keep it, and a recipient with one dead gateway
still gets their file.
The larger win is scheduling. The concurrency limiter was a barrier, not a
pool:
for (start = 0; start < items; start += 5)
await Future.wait(...five items...)
Each batch waited for its slowest member, so one ten-second failure left four
workers idle for it. A sliding window keeps five in flight at all times.
Measured over 300 items with 10% failing: 321s to 83s, 3.9x. Concurrency
itself is unchanged at five - the pool is the structural fix, and raising the
number is a separate knob best turned with evidence, not at the same moment we
removed the fallback.
Snapshot validation loses its GAR branch too; it already fast-failed on 404,
and the remaining path existed only for transient errors at the cost of that
same RPC.
What this PR found is worth more than what it changes: **sync already drops
files silently, today.** A failed metadata read returns empty bytes rather
than an error (`arweave_service.dart`), those bytes fail to parse, the parse
exception is logged as a warning, and the entity never reaches the drive -
while the watermark advances regardless. The block-height rewind is a two hour
recency window, not a retry mechanism, so anything older is gone until a
manual deep sync. Skipped transaction ids and their drive ids are now carried
out on the sync result instead of vanishing, and
`docs/SYNC_SKIPPED_ENTITY_PERSISTENCE.md` sets out the fix: a table, a
block-height range union, and a backoff.
No UI for skipped items in this PR. Per-file state cannot be coherent while
the list lives only in memory - rows would appear and vanish across restarts -
so it lands with the persistence that makes it stable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 4 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe sync flow now bounds metadata requests, retries through the configured gateway, records skipped transaction IDs, and reports them by drive through sync progress and completion states. Snapshot validation no longer uses ArioSDK or GAR fallback. A design note proposes persistent skipped-entity tracking. ChangesSkipped Entity Sync Reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SyncRepository
participant ArweaveService
participant DataGatewayFallback
participant SyncCubit
SyncRepository->>ArweaveService: Fetch drive entity metadata
ArweaveService->>DataGatewayFallback: fetchDataForSync(txId)
DataGatewayFallback-->>ArweaveService: Return metadata or failure
ArweaveService-->>SyncRepository: Return history with skippedTxIds
SyncRepository-->>SyncCubit: Return SyncProgress with skipped IDs by drive
SyncCubit-->>SyncCubit: Emit SyncCompleteWithErrors metadata
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🤖 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 `@lib/services/arweave/arweave_service.dart`:
- Around line 553-565: Convert the clamped concurrency values to integers by
appending `.toInt()` to both `maxConcurrentDataFetches.clamp(1, 100)`
expressions. Update the value passed to `runPooled` and the second value used
with `b +=`, `skip`, and `take`, without changing the existing bounds.
In `@test/services/arweave/data_gateway_fallback_test.dart`:
- Around line 23-35: Reset DataGatewayFallback.cachedGateways in setUp before
constructing fallback so every test starts with an empty gateway cache. Preserve
the existing mock setup and ensure each test independently exercises
arioSDK.getGateways() without relying on execution order.
In `@test/sync/data/snapshot_validation_service_test.dart`:
- Around line 40-50: Update the test around
SnapshotValidationService.validateSnapshotItems to pass a nonempty SnapshotItem
representing an unreachable configured gateway, then assert the returned
verified list is empty. Remove the vacuous _MockArioSDK interaction assertions,
since that mock is not injected into the service.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9988efb8-b606-4ce3-8c20-645ebd8be313
📒 Files selected for processing (12)
docs/SYNC_SKIPPED_ENTITY_PERSISTENCE.mdlib/main.dartlib/services/arweave/arweave_service.dartlib/services/arweave/data_gateway_fallback.dartlib/sync/data/snapshot_validation_service.dartlib/sync/domain/cubit/sync_cubit.dartlib/sync/domain/cubit/sync_state.dartlib/sync/domain/repositories/sync_repository.dartlib/sync/domain/sync_progress.darttest/services/arweave/data_gateway_fallback_test.darttest/sync/data/snapshot_validation_service_test.darttest/sync/domain/sync_progress_skipped_entities_test.dart
💤 Files with no reviewable changes (1)
- lib/main.dart
|
Visit the preview URL for this PR (updated for commit 84649a9): https://ardrive-web--pr2179-pe-9203-single-gatew-7z9espzg.web.app (expires Tue, 18 Aug 2026 03:48:32 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: a224ebaee2f0939e7665e7630e7d3d6cd7d0f8b0 |
…E-9203 From CodeRabbit. `validateSnapshotItems([])` returns before the loop, so the test asserted nothing about the path it was named for - no HEAD request, no rejection. It now passes a snapshot item, so the unreachable configured gateway is really contacted and really rejected, which is the point where `dev` would have reached for the GAR list. The `verifyZeroInteractions` on the SDK stays, but labelled for what it is: the service is never handed that object, so the compile-time signature above is what proves the branch is gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Went through all three. One was right and is fixed in Fixed — vacuous snapshot validation testCorrect, and the sharpest of the three. It now passes a snapshot item, so the unreachable configured gateway is genuinely contacted and genuinely rejects — visible in the test output as I kept Rejected —
|
`getUniqueUserDriveEntities` is sync's drive-discovery phase, and it called the waterfall once per drive transaction, in parallel. A user with a dozen drives opened a dozen fan-outs to GAR gateways on every sync - the exact cost this change exists to remove, sitting in the one sync path that was left alone. It was left alone on the belief that the call is shared with login. It is not: `getUniqueUserDriveEntities` has exactly one caller in the codebase, `_SyncRepository.updateUserDrives`. The login path is `getDriveSignatureForDrive` (`ardrive_auth.dart`), a different method, which keeps the waterfall and should. Audited the rest rather than trusting the earlier list: no other waterfall method - getLatestDriveEntityWithId, getAllFileEntitiesWithId, getLatestFileEntityWithId, dataFromTxId - is reachable from lib/sync at all. Drive discovery was the last one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Correction — you were right, and there was still a waterfall in the sync path.
driveTxs.map((e) => _gatewayFallback.fetchData(e.id, client)...)So a user with a dozen drives opened a dozen fan-outs to GAR gateways on every sync. That is precisely the cost this PR exists to remove, sitting in the one sync path that was left untouched. The reason it was left is wrong. It was believed to be shared with login. It is not — Fixed in So the claim in the PR description now actually holds: no fallback fan-out and no Solana RPC anywhere in a sync.
|
Auditing the rest of sync after the drive-discovery miss turned up two more, both reachable from every sync. `getTransactionConfirmations` had the identical chunked barrier the metadata reads had: chunks of GraphQL confirmation queries dispatched `maxConcurrent` at a time with an `await Future.wait` between batches, so each batch waited for its slowest query while the other workers sat idle. It runs on every sync through `_updateTransactionStatuses`. Same pool, same bound. Drive discovery was unbounded - a `Future.wait` over every drive transaction at once. That was survivable while it fanned out across several gateways; now that it reads one, an unbounded burst is aimed entirely at that single host, and a user with many drives would open every connection simultaneously. Bounded to the same limit the metadata reads use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 `@lib/services/arweave/arweave_service.dart`:
- Around line 915-942: Keep getUniqueUserDriveEntities sync-only by adding a
sync-specific private-drive signature lookup that uses
_gatewayFallback.fetchDataForSync instead of getDriveSignatureForDrive’s
waterfall fetchData path. Use this path when the private-drive key is not
cached, while preserving getDriveSignatureForDrive for non-sync callers; add
coverage for an uncached private-drive key verifying GAR is not contacted.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b547fee-3a49-461e-8332-4ec827d8bf44
📒 Files selected for processing (2)
lib/services/arweave/arweave_service.darttest/sync/data/snapshot_validation_service_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- test/sync/data/snapshot_validation_service_test.dart
From CodeRabbit, and correct. Drive discovery fetches a drive signature for every private drive whose key is not already in memory, and it did so through `getDriveSignatureForDrive` - the login path, which uses the waterfall. So the fan-out was back in the sync path by the side door, one private drive at a time. Both readers now share one implementation and differ only in how they fetch: `getDriveSignatureForDrive` keeps the waterfall for login, where one unreachable gateway must not cost someone their session, and `getDriveSignatureForDriveOnSync` reads the configured gateway only. Worth recording why this survived two audits: both greps that looked for waterfall callers excluded `arweave_service.dart` itself, and this call is inside it. An internal caller is invisible to a search that assumes the caller lives somewhere else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Valid, and fixed in Drive discovery fetches a drive signature for every private drive whose key isn't already in memory, and it did so via Both readers now share one implementation and differ only in how they fetch:
Added the coverage you asked for: a test asserting the sync-path signature read never contacts the GAR, alongside the existing pair that proves Worth recording why this survived two prior audits. Both greps that hunted for waterfall callers excluded
|
Sync metadata reads get two attempts, because one blip should not cost a file. Snapshot validation got one - and failing there is the more expensive outcome: a rejected snapshot discards its whole block range, which sync then re-queries over GraphQL. The cheap failure had the retry and the expensive one did not. Two attempts now, same budget and the same 300ms gap. Statuses the host will simply repeat - 400, 401, 402, 403 - still reject on the first answer rather than spending time to be told twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Brings in everything that landed after the branch was cut: #2161 parallelize upload preparation #2174 read the Turbo free-item cap from /v1/info #2175 self-describing share links, authenticated downloads, recipient page #2176 vendor pdf.js and add pdfx #2177 CLAUDE.md #2178 gateway fallback for every fetch, vendored CDN scripts, dead code #2179 sync reads a single gateway, pooled instead of chunked No conflicts. Verified on the merged tree: analyze clean, 1319 app tests, 43 crypto tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Sync fetched every metadata item through the multi-gateway waterfall — configured gateway, up to two GAR gateways, then arweave.net, serially, 5s each. A flaky gateway made each item cost 20s, and a sync reads hundreds. Building that gateway list also cost a Solana RPC.
What changed
Sync reads one gateway. Two attempts at 5s, then skip. A 404 isn't retried — the same host won't change its mind. 20s → 10.3s per failing item, and the Solana RPC is gone from the per-item path.
The bigger win was scheduling. The concurrency limiter was a barrier, not a pool:
Each batch waited for its slowest member, so one 10s failure idled four workers for it. A sliding window keeps five in flight continuously.
Both schedulers produce byte-identical positional output —
entityDatasis index-filled, and there's a test for out-of-order completion.Concurrency stays at 5. The pool is the structural fix; raising the number is an independent knob. Turning both at once — while also removing the fallback — would raise sustained pressure on a single gateway at exactly the moment we removed the thing that absorbs a 429. One knob, measure, then the other.
Snapshot validation loses its GAR branch. It already fast-failed on 404; the remaining path existed only for transient errors, at the cost of that same RPC.
The waterfall is untouched everywhere else — downloads, previews, manifests, shared links, drive attach. A recipient with one dead gateway still gets their file. Caller audit is in the commit message.
The thing this PR found
Sync already drops files silently, today, on
dev. A failed metadata read returnsUint8List(0)rather than an error; those empty bytes fail to parse; the parse exception is logged as a warning; the entity never reaches the drive — and the watermark advances regardless. The block-height rewind (kBlockHeightLookBack = 240) is a ~2 hour recency window, not a retry mechanism, and_lastSyncis in-memory, so anything older than that is gone until a manual deep sync.Skipped transaction ids and their drive ids now ride out on the sync result instead of vanishing.
docs/SYNC_SKIPPED_ENTITY_PERSISTENCE.mdsets out the fix — a table, a block-height range union, a backoff.No skipped-files UI here, deliberately
Per-file state can't be coherent while the list lives only in memory: rows would appear after one sync and vanish on restart, which is worse than none. It lands with the persistence that makes it stable. Worth knowing for the design — the GraphQL tags give us entity type, file id and parent folder id, but not the name, size or dataTxId, since those live in the body we failed to read. So the row can sit in the right folder, but it can't be named or downloaded.
Verification
flutter analyzeclean · 755 tests passing · 22 new, includingverifyNever(getGateways)on the sync path and proof the waterfall paths still consult it.🤖 Generated with Claude Code
https://claude.ai/code/session_01UpkQ6gRPTUuy3MCUFxFw2P
Summary by CodeRabbit
New Features
Bug Fixes
Documentation