PE-9204: One drive's gateway hiccup killed the whole sync - #2180
Conversation
Reported from staging: a drive signature 404ed, and sync stopped dead before any drive had synced. Retrying by hand found the transaction present. Two causes, both fixed. **The failure was not contained.** Drive discovery skips a drive whose metadata cannot be read - `if (driveResponses[i] == null) continue` - but the drive-signature read a few lines later was the one unguarded await in that loop. It threw past every handler to the method's outer catch, which rethrows, so `getUniqueUserDriveEntities` failed, `updateUserDrives` failed, and the sync died at discovery. One drive's bad luck took every drive with it. It now drops that drive for the pass, like every other failure there. **A 404 was treated as final, and that was wrong.** The reasoning was that the same host will not turn a 404 into a 200. True of a host that has answered definitively, false of one that has not finished indexing - which is exactly what was seen: 404 on the first read, present on the second. A 404 now spends the second attempt like any other failure, and is only reported as `TransactionNotFound` if every attempt agrees. A genuinely missing transaction still costs two attempts, no more. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 31 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 (3)
📝 WalkthroughWalkthroughPrivate-drive discovery now skips drives whose v1 signature retrieval fails. Sync gateway fetches retry 404 responses before returning ChangesArweave resilience
Estimated code review effort: 3 (Moderate) | ~20 minutes 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
🧹 Nitpick comments (1)
test/services/arweave/data_gateway_fallback_test.dart (1)
80-96: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a mixed-status regression test.
The changed tests cover
404then200and404then404. They do not cover500then404or an exception then404. The current production condition misclassifies those sequences asTransactionNotFound. Assert a non-TransactionNotFoundfailure and exactly two gateway calls. This enforces the all-404 terminal contract in the PR objective.Also applies to: 108-110
🤖 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 `@test/services/arweave/data_gateway_fallback_test.dart` around lines 80 - 96, Add regression coverage in the fallback.fetchDataForSync tests for both 500-then-404 and exception-then-404 responses from primaryApi.getSandboxedTx. Assert each result is a non-TransactionNotFound failure and verify exactly two gateway calls, preserving TransactionNotFound only when all attempts return 404.
🤖 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 1037-1046: Update the pagination flow using
getUniqueUserDriveEntityTxs and drivesById to deduplicate Drive-Id values across
all pages, not just within each page. Track processed or failed IDs across page
iterations so an older transaction cannot populate stale metadata when the
newest transaction’s signature retrieval fails. Add a regression test covering
duplicate Drive-Id values split across pages.
In `@lib/services/arweave/data_gateway_fallback.dart`:
- Around line 111-122: Update the surrounding method documentation near the 404
handling in the sync flow to state that the first 404 is retried and
TransactionNotFound is reported only after the final attempt. Keep the
implementation and existing retry behavior unchanged.
- Around line 111-122: Enforce the all-404 terminal contract in the fallback
logic: in lib/services/arweave/data_gateway_fallback.dart lines 111-122, track
whether every sync attempt returned 404 before producing TransactionNotFound,
rather than checking only the final status. In
test/services/arweave/data_gateway_fallback_test.dart lines 80-96, add a
500-then-404 scenario; in lines 108-110, assert it is not TransactionNotFound
and that exactly two gateway calls occur.
---
Nitpick comments:
In `@test/services/arweave/data_gateway_fallback_test.dart`:
- Around line 80-96: Add regression coverage in the fallback.fetchDataForSync
tests for both 500-then-404 and exception-then-404 responses from
primaryApi.getSandboxedTx. Assert each result is a non-TransactionNotFound
failure and verify exactly two gateway calls, preserving TransactionNotFound
only when all attempts return 404.
🪄 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: 321b9885-0cf0-4ca6-abc1-feb6b589764b
📒 Files selected for processing (3)
lib/services/arweave/arweave_service.dartlib/services/arweave/data_gateway_fallback.darttest/services/arweave/data_gateway_fallback_test.dart
|
Visit the preview URL for this PR (updated for commit e226878): https://ardrive-web--pr2180-pe-9204-sync-signatu-idqv133t.web.app (expires Tue, 18 Aug 2026 13:04:04 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: a224ebaee2f0939e7665e7630e7d3d6cd7d0f8b0 |
All three valid, and the first is a consequence of this PR. **Skipping a drive let an older revision take its place.** `getUniqueUserDriveEntityTxs` dedupes per page, so the same Drive-Id can appear on several pages, newest first. The new `continue` on a signature failure skipped the newest transaction without recording the id, so the older one on a later page was no longer deduped and wrote stale metadata. Worse than the drive being late. The id is claimed on failure now. **`TransactionNotFound` claimed more than it knew.** The check was "the last attempt was a 404", so a 500 followed by a 404 reported the transaction as absent. That is an unwell gateway, not missing data. Every attempt must agree now - a timeout or socket error disqualifies it too. **The doc still said a 404 is not retried**, which this PR had already stopped being true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All three valid — fixed in Cross-page dedup — and this one was caused by this PR.
The doc was stale — it still said a 404 isn't retried, which this PR had already made untrue. Added both scenarios you asked for: 500-then-404 and exception-then-404, each asserting a non- I did not add the cross-page regression test — |
1 participant
Reported from staging: a drive signature 404ed and sync stopped dead before any drive had synced. Retrying by hand found the transaction present — the gateway had it, it just hadn't indexed it yet.
Two causes.
The failure wasn't contained
Drive discovery already skips a drive whose metadata can't be read (
if (driveResponses[i] == null) continue). But the drive-signature read a few lines below was the one unguardedawaitin that loop. It threw past every handler to the method's outer catch — which rethrows — sogetUniqueUserDriveEntitiesfailed,updateUserDrivesfailed, and sync died at discovery.One drive's bad luck took every drive with it, including public ones that would have synced fine. It now drops that drive for the pass, like every other failure there, and picks it up next time.
A 404 was treated as final, and that was wrong
The 404 fast-path reasoned that the same host will not turn a 404 into a 200. That holds for a host that has answered definitively. It does not hold for one still indexing — which is precisely what was observed: 404 on the first read, present on the second.
A 404 now spends the second attempt like any other failure and is only reported as
TransactionNotFoundwhen every attempt agrees. A genuinely missing transaction still costs two attempts, no more.Why this got worse recently
Before #2179 this path used the multi-gateway waterfall, so a gateway that was a beat behind was covered by the next one. Single-gateway reads removed that cushion, and the 404 fast-path removed the retry too — so an indexing lag went straight to a hard failure with no second chance.
Verification
flutter analyzeclean · 1,320 tests passing.The test asserting "does not retry a 404" encoded the behaviour this reverses; it's replaced by two — one proving a 404-then-200 succeeds, one proving a persistent 404 still raises
TransactionNotFoundin exactly two attempts.🤖 Generated with Claude Code
https://claude.ai/code/session_01UpkQ6gRPTUuy3MCUFxFw2P
Summary by CodeRabbit