Repository navigation
fix(storer,pusher): retry a failed upload chunk once at the end of the queue - #5667
gacevicljubisa wants to merge 11 commits into
Conversation
| if !full { | ||
| return nil | ||
| } | ||
| // a full queue means an outage: let it pass before the |
There was a problem hiding this comment.
This wait runs inside the eg.Go goroutine, which still holds a directUploadLimiter slot (released by the defer at line 51). That limiter is node-wide: 128 slots shared by every direct upload and steward reupload. During an outage, one large upload that has filled its requeue can pin all 128 slots for up to 30s and stall every other direct upload on the node. A probe showed an unrelated session's Put blocking for exactly 30s.
The description says the retry happens "without holding a limiter slot", which is true for the Done path but not here. Could we release the slot before the wait and take it again before pushRequeued?
|
|
||
| // walk the map at most once per delay: chunksWorker waits on the same | ||
| // lock, and a burst of failures parks many chunks at once | ||
| if now.Sub(p.lastPrune) >= p.delay { |
There was a problem hiding this comment.
The entry is pruned once until + parkTTL (65 min) has passed, but a parked chunk is only seen again on the next full IteratePending pass, and that pass moves at push speed. With a backlog of a few million pending chunks a pass can take longer than 65 min. Then park() finds no entry, returns true again, and the chunk gets another fresh 6×32 round on every pass. So the "at most 2 rounds / 384 attempts" bound doesn't hold under backlog. A probe gave one chunk 5 rounds over 5 visits 70 min apart.
| if err := eg.Wait(); err != nil { | ||
| return err | ||
| } | ||
| items := q.drain() |
There was a problem hiding this comment.
The retry here fires right away, but errSkip keeps the refusing peers for 5 min (skiplistDur). For the out-of-depth case from the motivation, any upload that ends less than 5 min after the failure retries against the same skipped peers and gets ErrOutOfDepthStoring again at once. For shallow receipts it spends a second paid round right away. The 30s directUploadRequeueMinAge (requeue.go:23) is also shorter than skiplistDur. Was it intended that the retry only helps uploads that run at least 5 min past the failure? If so, could the description say so? If not, the wait could be tied to skiplistDur, at the cost of a slower response.
Checklist
Description
Stacked on #5390, base
fix/pushsync-out-of-depth-chunk-loss.With #5390 a chunk that no node in its neighborhood takes fails instead of a false success. This PR gives such a chunk one more round with a new budget of 6 x 32, and still fails at once on a systemic error. No protocol message changes, and a forwarder behaves as on #5390.
What changes
pkg/storer). A chunk that fails withErrCouldNotSyncgoes into a requeue of at most 16 per upload, and the upload goes on. AtDonethe netstore pushes every queued chunk once more. When the requeue is full, the netstore pushes the oldest chunk again, at the earliest 30 s after its failure. A second failure fails the upload at once.Putreturns the error at once. On fix(pushsync): prevent silent chunk loss on shallow receipts #5390Putaccepted the rest of the chunks and dropped them untilDonereturned the error.pkg/pusher,pkg/storer). A chunk that fails withErrOutOfDepthStoring, or withErrShallowReceiptafter 6 calls, parks for 5 minutes, as long aserrSkipkeeps its first hops. It stays in the upload queue, the pusher skips it, and its attempts reset to 0. The upload store offers the pending chunks to the pusher every minute, so a parked chunk is pushed again also on a node without new uploads. A second failure ends withChunkCouldNotSync, as on fix(pushsync): prevent silent chunk loss on shallow receipts #5390.pkg/pushsync). When the budget reaches 0 and pushes are still in flight, an origin waits for their results and sends nothing new. A valid receipt that arrives then ends the call with success. On fix(pushsync): prevent silent chunk loss on shallow receipts #5390 the call returned at once and a late receipt was lost. A forwarder does not wait.pkg/pusher). A direct push whose postage batch no longer exists fails withpostage.ErrNotFound, on fix(pushsync): prevent silent chunk loss on shallow receipts #5390 it returned nil.ErrOutOfDepthStoringon a direct chunk resets its attempts.direct_upload_requeuedanddirect_upload_requeue_failedin localstore,total_parkedin pusher.direct_upload_requeue_faileddoes not count a client that disconnected or a node that shuts down.total_could_not_syncnow counts the final give-up of a deferred chunk and each failed direct round.What does not change
errSkip, the 5 s wait onErrNotFound, and the resubmit at once onErrShallowReceiptfor a direct chunk.Motivation and Context (Optional)
On mainnet with #5390, 2 of 170 million chunks got
ErrOutOfDepthStoringwith every peer inerrSkip. Each one ended a multi-hour direct upload with 500. A second push at the end of the upload letserrSkipexpire without holding a limiter slot.Related Issue (Optional)
#5390, #5400
AI Disclosure