fix(sync): retry missing content downloads on every successful sync - #112
Open
vladimirlogachev wants to merge 1 commit into
Open
fix(sync): retry missing content downloads on every successful sync#112vladimirlogachev wants to merge 1 commit into
vladimirlogachev wants to merge 1 commit into
Conversation
Contributor
|
This might be related: #88 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A record can arrive from a peer that does not have the record's content: the sender is a relay that received the record but never fetched the bytes (its download policy forbids it, or it simply hasn't downloaded yet). The receiver then parks the content hash in
missing_hashes— correctly, since the only known peer provably lacks the bytes. The problem is what unparks it: only a gossipOp::ContentReadybroadcast from a neighbor that finished downloading that content. Gossip broadcasts are best-effort, so if that one message is lost — or nobody else ever downloads the content — the hash stays parked forever. Re-running reconciliation does not help: the record is already present, so no newInsertRemotefires and no download is ever triggered. The node keeps completing successful syncs with peers that do have the bytes, and still never asks them.We hit this downstream at roughly a 0.7% failure rate (1 in 303 runs) in a four-node last-writer-wins scenario, where a record relayed ahead of its content permanently starved on one device.
Changes:
missing_hashesentries are keyed(NamespaceId, Hash)instead ofHash, so retries are scoped to the namespace being synced andPendingContentReadyattribution stays exact when several namespaces want the same hash.on_sync_finished, both origins), parked hashes of that namespace are retried against the just-synced peer via the existingstart_download(…, only_if_missing = true)path — the peer is a fresh provider candidate; if it lacks the bytes the download fails and the hash parks again, no worse than before.retry_after_failuremark:ProviderNodes::find_providerssnapshots the provider set when a download starts, so a provider registered mid-flight is invisible to the running attempt. If that attempt fails, the marked hash is re-queued once with the enriched provider set. The mark is consumed on use and cleared on success, so a repeated failure without a new sync cannot loop.start_downloadis split into provider registration plus a node-lessqueue_download, so the failure-path retry can re-queue without inventing a provider argument.sync_fetches_parked_content_from_later_sync_peer: writer → relay withDownloadPolicy::NothingExcept([])(record without bytes), writer leaves the document, receiver imports from the relay alone and parks the hash, writer returns, receiver completes an empty sync with it — the content must arrive. Red before this fix (starves for the full timeout), green after (relay leg included: the mid-flight window above is exactly what the test caught in the first version of this fix). The writer's leave/return is what keeps the red arm deterministic — otherwise gossip may connect receiver and writer directly and mask the starvation.Breaking Changes
None. All touched state is private to the live actor; public APIs and events are unchanged (nodes may now emit
ContentReadyin situations where they previously starved).Notes & open questions
find_providers, which would makeretry_after_failureunnecessary — larger change, left out of scope.Origin::ConnectandOrigin::Accept: an accepted peer is just as good a provider candidate as one we dialed.Change checklist