fix(proxycache): retry interrupted upstream blob fetches, verify digest before caching#23601
Open
oscrx wants to merge 6 commits into
Open
fix(proxycache): retry interrupted upstream blob fetches, verify digest before caching#23601oscrx wants to merge 6 commits into
oscrx wants to merge 6 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds resumable proxy-cache blob fetching and digest verification before local caching.
Changes:
- Adds bounded retries using HTTP range requests.
- Verifies cached blob digests.
- Adds retry and corruption tests.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/controller/proxy/blobretry.go |
Implements retry and verification readers. |
src/controller/proxy/blobretry_test.go |
Tests reader behavior. |
src/controller/proxy/controller.go |
Integrates retries and verification. |
src/controller/proxy/controller_test.go |
Tests controller cache population. |
src/controller/proxy/manifestcache.go |
Protects manifest dependency caching. |
src/controller/proxy/manifestcache_test.go |
Tests dependency blob handling. |
src/controller/proxy/remote.go |
Adds ranged blob reads. |
src/testing/controller/proxy/remote_interface.go |
Extends the remote mock. |
Files not reviewed (1)
- src/testing/controller/proxy/remote_interface.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…st before caching The proxy cache's blob fetch made a single-shot HTTP GET to the upstream registry with no retry: a connection dropped mid-stream (e.g. by a network intermediary killing the TCP session) just failed the pull, with no equivalent of docker/containerd's client-side retry of an interrupted layer. This affected both controller.putBlobToLocal (background cache population after a client pull) and the identical ManifestCache.putBlobToLocal used when caching a manifest list's dependent blobs. Add a resumingBlobReader that, on a retryable mid-stream read error, closes the broken connection and resumes the fetch from the last successfully read byte offset via an HTTP range request (RemoteInterface.BlobReaderAt, backed by the existing PullBlobChunk), with bounded exponential backoff. Applied to the client-serving fetch and both cache-populate fetches. Also add a verifyingReadCloser that recomputes the digest of the bytes streamed into each cache-populate path and surfaces a mismatch as a Read error (instead of io.EOF), so a corrupted-but-well-formed body can't be committed to local storage as if it were a complete, valid blob. Signed-off-by: Oscar Wieman <oscar@oscarr.nl>
…igest at size Addresses three gaps found in review of the initial fix: - PullBlobChunk never checked that the server actually honored the Range request. A registry is allowed to ignore Range and return a full 200 instead of a 206; resumingBlobReader would then splice the full blob (from byte 0) onto bytes already delivered, producing a same-length but corrupted blob - the exact failure mode this fix exists to prevent. Require a 206 whose Content-Range starts at the requested offset, or a plain 200 only when the whole blob was requested from the start. - resumingBlobReader accepted a clean io.EOF as a complete blob even when short of the known size. A network intermediary that terminates a stream gracefully (rather than resetting the connection) produces exactly this, and it would previously bypass retry entirely. Treat a premature clean EOF the same as a dropped connection. - verifyingReadCloser only checked the digest when its own Read returned io.EOF, but the real consumer (an HTTP request body capped by Content-Length) wraps it in io.LimitReader, which returns EOF itself once the declared length is reached and may never call back into the wrapped reader again if the last real read already returned its final bytes with a nil error. Track the expected size and verify as soon as it's reached, not only on a subsequent EOF call. Signed-off-by: Oscar Wieman <oscar@oscarr.nl>
… ctx Addresses two more gaps found in the second round of review: - PullBlobChunk's 200 fallback only checked start == 0, not that the whole blob was requested. copyBlobByChunk (replication) requests a start=0 first chunk that is shorter than the full blob for any blob bigger than one chunk, so an origin ignoring Range could return a full 200 that gets accepted as if it were just that first chunk. Use the (previously discarded) blobSize parameter to require start == 0 && end == blobSize-1 before accepting a 200, falling back to the weaker check only when blobSize is unknown. - resumingBlobReader's backoff wait and resume attempts weren't cancelable. If the pulling client disconnects mid-retry, the reader would still sleep out the remaining backoff and open further upstream requests - each unbounded by any context, since RemoteInterface doesn't accept one - while holding a connection-limit slot. Thread a context.Context through resumingBlobReader so backoff waits and not-yet-started resume attempts respect cancellation. The client-serving fetch uses the request context; both cache-populate fetches use context.Background(), since that background work is meant to keep going after the client disconnects. Signed-off-by: Oscar Wieman <oscar@oscarr.nl>
oscrx
force-pushed
the
proxy-cache-retry-fix
branch
from
July 22, 2026 09:31
c177a8d to
2a476c7
Compare
Signed-off-by: Oscar Wieman <oscar@oscarr.nl>
…success An io.Reader may legally return its last bytes together with a non-nil, non-EOF error in the same call, instead of returning them cleanly and erroring only on a subsequent call. resumingBlobReader didn't account for this: if those bytes brought offset to the full declared size, canRetry returned false (since offset is no longer < size) and the error was propagated anyway, even though the complete blob had just been assembled. Check for having reached the declared size before looking at the accompanying error, and treat that as success regardless. A completed flag makes the reader self-terminate (return io.EOF) on any further call, instead of re-reading a reader that may now be stale or exhausted - which a naive "just don't retry once size is reached" fix would otherwise turn into a spurious (0, nil) reads. Signed-off-by: Oscar Wieman <oscar@oscarr.nl>
Member
|
What negative effect might a retry have on rate limits? |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23601 +/- ##
==========================================
+ Coverage 66.37% 66.40% +0.03%
==========================================
Files 1073 1074 +1
Lines 117750 117885 +135
Branches 2965 2965
==========================================
+ Hits 78161 78287 +126
- Misses 35286 35287 +1
- Partials 4303 4311 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Raised in PR review: an interrupted blob whose resume attempt hits a 429 would previously be retried like any other transient failure. A few seconds of exponential backoff won't clear a real rate limit and just adds more rejected requests to an already-throttled upstream, so give up as soon as errors.IsRateLimitError reports the resume's error, instead of burning through the rest of the retry budget on attempts likely to fail the same way. Signed-off-by: Oscar Wieman <oscar@oscarr.nl>
Author
|
Good question. Two things worth separating here: How much extra load can retry actually add? It only ever triggers on a genuinely interrupted connection (dropped/reset mid-transfer), never on a normal successful pull, and it's bounded to at most 3 extra attempts per interrupted blob with exponential backoff (1s/2s/4s) - not a multiplier on regular traffic. I'd actually argue this reduces aggregate upstream load in the failure case: today, an interrupted blob fails the whole pull outright, so the kubelet/docker client has to retry the entire image (manifest + every blob) from scratch. This resumes just the missing tail of the one interrupted blob via a Range request instead, which is strictly less upstream traffic than an outer full-image retry. Where you're right that there was a real gap: if a resume attempt itself got rate-limited (429), the code was treating that the same as any other transient error and retrying it with the same 1-4s backoff - which won't help against a real rate limit (those typically need a much longer cool-down) and just adds more rejected requests. Fixed that in bbc3c78: a rate-limited resume now gives up immediately instead of consuming the rest of the retry budget, using the existing errors.IsRateLimitError check (same one already used elsewhere in this file for manifest 429 handling). Given the above I'd rather not add a feature flag - the behavior is already bounded and opt-out-shaped (only fires on failure), and a flag adds real config/test surface for a fairly narrow edge case. Happy to reconsider if you still think it's warranted, though. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- src/testing/controller/proxy/remote_interface.go: Generated file
Comments suppressed due to low confidence (1)
src/pkg/registry/client.go:414
- Validate the complete
Content-Range, not only its start.PullBlobChunkis also used bycopyBlobByChunk(src/controller/replication/transfer/image/transfer.go:429-440), so a server can currently answer a request forbytes=0-4with206 Content-Range: bytes 0-10/11; this check accepts the full body even though it violates that caller's chunk contract. Parse and require the returned end to equalend(and, whenblobSizeis known, the total to equal it), with a regression test for a matching start but mismatched end.
cr := resp.Header.Get("Content-Range")
got, crErr := parseContentRangeStart(cr)
if crErr != nil || got != start {
defer resp.Body.Close()
return 0, nil, fmt.Errorf("registry returned Content-Range %q for requested range bytes=%d-%d, refusing to treat it as that chunk", cr, start, end)
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.
Thank you for contributing to Harbor!
Comprehensive Summary of your change
The proxy cache's blob fetch made a single-shot HTTP GET to the upstream
registry with no retry: a connection dropped mid-stream (e.g. by a network
intermediary killing the TCP session, as described in #23600) just failed
the pull, with no equivalent of docker/containerd's client-side retry of an
interrupted layer. This affected both
controller.putBlobToLocal(background cache population after a client pull) and the identical
ManifestCache.putBlobToLocalused when caching a manifest list'sdependent blobs.
This adds a
resumingBlobReaderthat, on a retryable mid-stream read error,closes the broken connection and resumes the fetch from the last
successfully read byte offset via an HTTP range request
(
RemoteInterface.BlobReaderAt, backed by the existingPullBlobChunkwhich wasn't previously wired into the proxy-cache fetch path), with bounded
exponential backoff (3 attempts, 1s/2s/4s). It's applied to the
client-serving fetch and both cache-populate fetches.
It also adds a
verifyingReadCloserthat recomputes the digest of thebytes streamed into each cache-populate path and surfaces a mismatch as a
Read error (instead of
io.EOF), so a corrupted-but-well-formed body can'tbe committed to local storage as if it were a complete, valid blob.
No config surface, API, or docs are affected — the retry count/backoff are
internal constants, not exposed as project/system configuration.
Issue being fixed
Fixes #23600
Please indicate you've done the following: