Cap Retry-After delay and stop pooling broken upload URLs after hard failures - #32
Open
b2-jared wants to merge 3 commits into
Open
Cap Retry-After delay and stop pooling broken upload URLs after hard failures#32b2-jared wants to merge 3 commits into
b2-jared wants to merge 3 commits into
Conversation
Writer.simpleWriteFile unconditionally returned its upload URL/token to
the bucket's urlPool on exit via a bare defer, even when retry.Do had
exhausted retries and returned a hard error. Since urlPool.get()/put()
do no liveness check on stored entries, an unrelated subsequent Writer
on the same bucket could pull that broken upload URL back out of the
pool and immediately fail the same way (e.g. a small lock-file write
right after a large upload's non-retryable failure on the same
bucket).
Guard the pooling with a boolean success flag set only right before
simpleWriteFile's final nil return, so a failed upload's last-used URL
(reassigned to a fresh one on each retry.OnRetry) is discarded instead
of pooled.
Adds TestFailedUploadDoesNotPoolURL, which forces a hard failure via
the existing testError{reupload:true, maxReuploads:0} fake and asserts
urlPool.get() returns nil afterward, and
TestSuccessfulUploadPoolsURL, which confirms the happy path still
pools the URL as before.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PR Backblaze#24 (internal/retry package) bounded the retry loop's iteration count, but base.Backoff() still converts the B2 server's Retry-After header straight into a time.Duration with no upper limit. A large or malformed Retry-After value (from B2 or a misbehaving proxy) could still cause a single very long sleep, undermining that bounded-retry-count fix. Clamp the result to 30 seconds, matching the ceiling internal/retry.Backoff already uses for its own exponential-backoff fallback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Fixes #31.
Two small, related follow-ups to #24's retry-count fix, found while root-causing a customer-reported ~80 hour hang in a restic B2 backup (small
locks/object upload).1. Cap the server-supplied
Retry-Afterdelaybase.Backoff()converted theRetry-Afterheader straight into atime.Durationwith no upper bound. #24 bounded the retry count, but a single retry-after-derived sleep could still be arbitrarily long. This clamps it to the same 30s ceilinginternal/retry.Backoffalready uses for its own exponential-backoff fallback.2. Don't return a broken upload URL to the shared pool after a hard failure
Writer.simpleWriteFile()'sdeferunconditionally returned the upload URL/token to the bucket's sharedurlPool, even when the function was about to return a hard, non-retryable error. Since the pool has no liveness check, a subsequent unrelatedWriteron the same bucket could pull that broken URL back out viagetUploadURL()and immediately hit the same failure. Now it's only pooled on success.Testing
base/base_test.go: table-drivenTestBackoffcovering pass-through, at-cap, and over-capRetry-Aftervalues.b2/b2_test.go:TestFailedUploadDoesNotPoolURL(hard failure → pool stays empty) andTestSuccessfulUploadPoolsURL(happy path still pools normally).go build ./...,go vet ./base/... ./b2/..., andgo test ./base/... ./b2/... -shortall pass (integration tests skip as expected without B2 credentials).