From 93088b22f1a11015054bfa1aee4c6058550b4f72 Mon Sep 17 00:00:00 2001 From: Levi Date: Thu, 29 Jan 2026 22:13:42 +0800 Subject: [PATCH] fix: improve retry handling for expired HTTP redirect url (#1263) --- internal/protocol/http/fetcher.go | 54 +++++++- internal/protocol/http/fetcher_manager.go | 9 ++ internal/protocol/http/fetcher_test.go | 162 ++++++++++++++++++++++ internal/protocol/http/helper.go | 81 ++++++++++- internal/test/httptest.go | 112 +++++++++++++++ 5 files changed, 410 insertions(+), 8 deletions(-) diff --git a/internal/protocol/http/fetcher.go b/internal/protocol/http/fetcher.go index 073a567a..4b319db7 100644 --- a/internal/protocol/http/fetcher.go +++ b/internal/protocol/http/fetcher.go @@ -588,6 +588,10 @@ func (f *Fetcher) Start() error { // Already downloading, this is a resume from pause return f.doStart() + case stateError: + // Retry after error: reset and restart + return f.doStart() + default: return fmt.Errorf("cannot start in current state: %v", state) } @@ -598,10 +602,31 @@ func (f *Fetcher) doStart() error { <-f.resolvedCh state := f.getState() - if state == stateDone || state == stateError { + if state == stateDone { return nil } + // If retrying after error, reset connection states for retry + if state == stateError { + // Drain any pending error from doneCh before retry + select { + case <-f.doneCh: + default: + } + + f.connMu.Lock() + for _, conn := range f.connections { + // Reset connections that can be retried + if !conn.Completed && conn.State != connCompleted { + conn.State = connNotStarted + conn.failed = false + conn.retryTimes = 0 + conn.lastErr = nil + } + } + f.connMu.Unlock() + } + // Open or create target file first (needed for prefetch copy) name := f.meta.SingleFilepath() var err error @@ -948,6 +973,8 @@ func (f *Fetcher) runConnection(conn *connection) { } // downloadChunkOnce performs a single HTTP request for the current chunk without retrying. +// If the redirect URL fails with an expiration-related error (401, 403, 410), +// it will automatically retry with the original URL and update the redirect URL on success. func (f *Fetcher) downloadChunkOnce(conn *connection, client *http.Client, buf []byte) error { if conn.ctx.Err() != nil { return conn.ctx.Err() @@ -981,11 +1008,32 @@ func (f *Fetcher) downloadChunkOnce(conn *connection, client *http.Client, buf [ if err != nil { return err } - defer resp.Body.Close() if resp.StatusCode != base.HttpCodeOK && resp.StatusCode != base.HttpCodePartialContent { - return NewRequestError(resp.StatusCode) + resp.Body.Close() + originalErr := NewRequestError(resp.StatusCode) + + // Check if this might be a redirect URL expiration error + // If so, try falling back to the original URL + if f.hasRedirectURL() && isRedirectExpiredError(originalErr) { + fallbackResp, fallbackErr := f.tryFallbackToOriginalURL(conn.ctx, client, rangeStart, rangeEnd) + if fallbackErr == nil && fallbackResp != nil { + // Fallback succeeded, use this response instead + resp = fallbackResp + // Update the redirect URL from the response + f.updateRedirectURL(resp) + } else { + // Fallback also failed, return the original error + if fallbackResp != nil { + fallbackResp.Body.Close() + } + return originalErr + } + } else { + return originalErr + } } + defer resp.Body.Close() // Record successful connection time for adaptive timeout f.updateMaxConnTime(time.Since(connStartTime)) diff --git a/internal/protocol/http/fetcher_manager.go b/internal/protocol/http/fetcher_manager.go index 11f01b37..22666553 100644 --- a/internal/protocol/http/fetcher_manager.go +++ b/internal/protocol/http/fetcher_manager.go @@ -15,6 +15,7 @@ import ( type fetcherData struct { Connections []*connection + RedirectURL string // Saved redirect URL for resume } // ============================================================================ @@ -71,8 +72,12 @@ func (fm *FetcherManager) DefaultConfig() any { func (fm *FetcherManager) Store(f fetcher.Fetcher) (data any, err error) { _f := f.(*Fetcher) + _f.redirectLock.Lock() + redirectURL := _f.redirectURL + _f.redirectLock.Unlock() return &fetcherData{ Connections: _f.connections, + RedirectURL: redirectURL, }, nil } @@ -87,6 +92,10 @@ func (fm *FetcherManager) Restore() (v any, f func(meta *fetcher.FetcherMeta, v if len(fd.Connections) > 0 { fetcher.connections = fd.Connections } + // Restore redirect URL for resume + if fd.RedirectURL != "" { + fetcher.redirectURL = fd.RedirectURL + } return fetcher } } diff --git a/internal/protocol/http/fetcher_test.go b/internal/protocol/http/fetcher_test.go index c6e41278..fb82980d 100644 --- a/internal/protocol/http/fetcher_test.go +++ b/internal/protocol/http/fetcher_test.go @@ -778,6 +778,168 @@ func TestFetcher_AsyncPrefetch(t *testing.T) { }) } +// TestFetcher_DownloadExpiringRedirectURL tests that the fetcher correctly handles +// expiring redirect URLs by falling back to the original URL and getting a new redirect. +func TestFetcher_DownloadExpiringRedirectURL(t *testing.T) { + // Test with redirect expiring after 2 requests + // This means: + // - Request 1: Resolve (original URL redirects to temp URL v1) + // - Request 2: First download request to temp URL v1 succeeds + // - Request 3: Second download request to temp URL v1 returns 403 + // - Fetcher should then retry with original URL, get temp URL v2 + // - Continue downloading from temp URL v2 + t.Run("RedirectExpiresAfter2Requests", func(t *testing.T) { + os.Remove(test.DownloadFile) + + // Create server with redirect expiring after 2 requests, with slow transfer to ensure + // multiple connection attempts are needed + listener := test.StartTestExpiringRedirectServer(2, 100*time.Nanosecond) + defer listener.Close() + + fetcher := buildFetcher() + err := fetcher.Resolve(&base.Request{ + URL: "http://" + listener.Addr().String() + "/" + test.BuildName, + }, &base.Options{ + Name: test.DownloadName, + Path: test.Dir, + Extra: &http.OptsExtra{ + Connections: 4, // Use multiple connections to trigger redirect expiration + }, + }) + if err != nil { + t.Fatal(err) + } + + err = fetcher.Start() + if err != nil { + t.Fatal(err) + } + + err = fetcher.Wait() + if err != nil { + t.Fatal(err) + } + + // Verify file content is correct despite redirect expiration + want := test.FileMd5(test.BuildFile) + got := test.FileMd5(test.DownloadFile) + if want != got { + t.Errorf("Download() got = %v, want %v", got, want) + } + + os.Remove(test.DownloadFile) + }) + + // Test with redirect expiring after 5 requests (more room for initial connections) + t.Run("RedirectExpiresAfter5Requests", func(t *testing.T) { + os.Remove(test.DownloadFile) + + listener := test.StartTestExpiringRedirectServer(5, 100*time.Nanosecond) + defer listener.Close() + + fetcher := buildFetcher() + err := fetcher.Resolve(&base.Request{ + URL: "http://" + listener.Addr().String() + "/" + test.BuildName, + }, &base.Options{ + Name: test.DownloadName, + Path: test.Dir, + Extra: &http.OptsExtra{ + Connections: 8, + }, + }) + if err != nil { + t.Fatal(err) + } + + err = fetcher.Start() + if err != nil { + t.Fatal(err) + } + + err = fetcher.Wait() + if err != nil { + t.Fatal(err) + } + + // Verify file content + want := test.FileMd5(test.BuildFile) + got := test.FileMd5(test.DownloadFile) + if want != got { + t.Errorf("Download() got = %v, want %v", got, want) + } + + os.Remove(test.DownloadFile) + }) +} + +// TestFetcher_RetryAfterError tests that the fetcher can retry downloading +// after a previous download attempt failed by calling Start() again. +func TestFetcher_RetryAfterError(t *testing.T) { + os.Remove(test.DownloadFile) + + // Server fails first 3 requests (after resolve), then recovers + // With 1 connection and 3 retries: + // - First Start(): requests 2, 3, 4 → all fail (3 retries exhausted) → returns error + // - Second Start(): request 5 → succeeds (server recovered after 3 failures) + listener := test.StartTestFailThenRecoverServer(3) + defer listener.Close() + + fetcher := buildFetcher() + err := fetcher.Resolve(&base.Request{ + URL: "http://" + listener.Addr().String() + "/" + test.BuildName, + }, &base.Options{ + Name: test.DownloadName, + Path: test.Dir, + Extra: &http.OptsExtra{ + Connections: 1, // Use single connection to simplify test + }, + }) + if err != nil { + t.Fatal(err) + } + + // First download attempt - should fail because server returns 416 after resolve + err = fetcher.Start() + if err != nil { + t.Fatal(err) + } + + err = fetcher.Wait() + // First attempt should fail with 416 error + if err == nil { + t.Fatal("Expected first download attempt to fail, but it succeeded") + } + t.Logf("First attempt failed as expected: %v", err) + + // Check that fetcher is in error state + state := fetcher.getState() + if state != stateError { + t.Errorf("Expected fetcher to be in stateError, got %v", state) + } + + // Verify that we can call Start() again after error + // This tests the stateError handling in Start() + err = fetcher.Start() + if err != nil { + t.Fatalf("Start() after error failed: %v", err) + } + + // Wait for second attempt - should succeed now that server has recovered + err = fetcher.Wait() + if err != nil { + t.Fatalf("Retry failed: %v", err) + } + + // Verify file content + want := test.FileMd5(test.BuildFile) + got := test.FileMd5(test.DownloadFile) + if want != got { + t.Errorf("Download() got = %v, want %v", got, want) + } + + os.Remove(test.DownloadFile) +} + func TestFetcherManager_ParseName(t *testing.T) { type args struct { u string diff --git a/internal/protocol/http/helper.go b/internal/protocol/http/helper.go index ac7d90fa..062f1eb0 100644 --- a/internal/protocol/http/helper.go +++ b/internal/protocol/http/helper.go @@ -65,14 +65,23 @@ func extractRequestError(err error) *RequestError { return nil } -// ============================================================================ -// HTTP Request/Client Building -// ============================================================================ - +// buildRequest creates an HTTP request using the redirect URL if available. func (f *Fetcher) buildRequest(ctx context.Context, req *base.Request) (httpReq *http.Request, err error) { + return f.buildRequestWithURL(ctx, req, true) +} + +// buildRequestWithOriginalURL creates an HTTP request using the original URL. +// This is used for retrying when the redirect URL has expired. +func (f *Fetcher) buildRequestWithOriginalURL(ctx context.Context, req *base.Request) (httpReq *http.Request, err error) { + return f.buildRequestWithURL(ctx, req, false) +} + +// buildRequestWithURL creates an HTTP request. +// If useRedirect is true and a redirect URL exists, it will be used; otherwise the original URL is used. +func (f *Fetcher) buildRequestWithURL(ctx context.Context, req *base.Request, useRedirect bool) (httpReq *http.Request, err error) { var reqUrl string f.redirectLock.Lock() - if f.redirectURL != "" { + if useRedirect && f.redirectURL != "" { reqUrl = f.redirectURL } else { reqUrl = req.URL @@ -121,6 +130,68 @@ func (f *Fetcher) buildRequest(ctx context.Context, req *base.Request) (httpReq return httpReq, nil } +// updateRedirectURL updates the redirect URL from the response. +// This is called when a request using the original URL succeeds after the redirect URL expired. +func (f *Fetcher) updateRedirectURL(resp *http.Response) { + if resp != nil && resp.Request != nil { + newRedirectURL := resp.Request.URL.String() + f.redirectLock.Lock() + f.redirectURL = newRedirectURL + f.redirectLock.Unlock() + } +} + +// hasRedirectURL checks if a redirect URL exists and is different from the original URL. +func (f *Fetcher) hasRedirectURL() bool { + f.redirectLock.Lock() + defer f.redirectLock.Unlock() + return f.redirectURL != "" && f.redirectURL != f.meta.Req.URL +} + +// isRedirectExpiredError checks if the error indicates that the redirect URL may have expired. +// This includes 403 (Forbidden), 401 (Unauthorized), 410 (Gone), and network errors. +func isRedirectExpiredError(err error) bool { + if err == nil { + return false + } + + // Check for specific HTTP error codes that might indicate URL expiration + if re := extractRequestError(err); re != nil { + switch re.Code { + case 401, 403, 404, 410: + return true + } + } + + return false +} + +// tryFallbackToOriginalURL attempts to make a request using the original URL +// when the redirect URL has expired. Returns the response if successful. +func (f *Fetcher) tryFallbackToOriginalURL(ctx context.Context, client *http.Client, rangeStart, rangeEnd int64) (*http.Response, error) { + httpReq, err := f.buildRequestWithOriginalURL(ctx, f.meta.Req) + if err != nil { + return nil, err + } + + if f.meta.Res.Range && rangeEnd > 0 { + httpReq.Header.Set(base.HttpHeaderRange, + fmt.Sprintf(base.HttpHeaderRangeFormat, rangeStart, rangeEnd)) + } + + resp, err := client.Do(httpReq) + if err != nil { + return nil, err + } + + if resp.StatusCode != base.HttpCodeOK && resp.StatusCode != base.HttpCodePartialContent { + resp.Body.Close() + return nil, NewRequestError(resp.StatusCode) + } + + return resp, nil +} + // buildClient creates an HTTP client with the default connection timeout. // Used for resolve phase where we don't have connection time data yet. func (f *Fetcher) buildClient() *http.Client { diff --git a/internal/test/httptest.go b/internal/test/httptest.go index 72c08c62..a2653a8e 100644 --- a/internal/test/httptest.go +++ b/internal/test/httptest.go @@ -335,6 +335,67 @@ func StartTestOneTimeServer() net.Listener { }) } +// StartTestExpiringRedirectServer creates a server that simulates expiring redirect URLs. +// The original URL redirects to a temporary URL that expires after a specified number of requests. +// When the temporary URL expires (returns 403), the client should retry with the original URL +// to get a new redirect URL. +// Parameters: +// - requestsBeforeExpire: number of requests the temporary URL accepts before expiring +// - delayPerByte: optional delay per byte for slow transfer (use 0 for no delay) +func StartTestExpiringRedirectServer(requestsBeforeExpire int32, delayPerByte time.Duration) net.Listener { + var redirectVersion atomic.Int32 + var requestCount atomic.Int32 + redirectVersion.Store(1) + + return startTestServer(func(sl *shutdownListener) http.Handler { + return http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { + path := request.URL.Path + + // Original URL - redirects to current version of temporary URL + if path == "/"+BuildName { + // Reset request count when original URL is accessed + requestCount.Store(0) + // Redirect to versioned temporary URL + version := redirectVersion.Load() + redirectURL := fmt.Sprintf("/redirect-v%d/%s", version, BuildName) + http.Redirect(writer, request, redirectURL, http.StatusFound) + return + } + + // Temporary URL handler - matches /redirect-v{N}/... pattern + if strings.HasPrefix(path, "/redirect-v") { + // Check if the redirect has expired + count := requestCount.Add(1) + if count > requestsBeforeExpire { + // Redirect expired - increment version for next redirect + redirectVersion.Add(1) + writer.WriteHeader(403) + writer.Write([]byte("Redirect URL expired")) + return + } + + // Serve the file with range support + rangeFileHandle( + writer, + request, + nil, + func(file *os.File, n int64) { + if delayPerByte > 0 { + slowCopyNWithDelay(sl, writer, file, n, delayPerByte) + } else { + io.CopyN(writer, file, n) + } + }, + ) + return + } + + // Not found + writer.WriteHeader(404) + }) + }) +} + // StartTestSlowStartServer creates a server with configurable delay per request // This allows testing slow-start connection expansion to reach max connections func StartTestSlowStartServer(delayPerByte time.Duration) net.Listener { @@ -496,6 +557,57 @@ func StartTestTemporary500Server(errorDuration time.Duration) net.Listener { }) } +// StartTestFailThenRecoverServer creates a server that fails all connections initially, +// then recovers after a specified number of failed requests. +// This tests the retry functionality when calling Start() again after a download fails. +// Parameters: +// - failedRequestsBeforeRecover: number of requests that will fail before server recovers +// +// Note: Uses 416 (Range Not Satisfiable) instead of 500 because 5xx errors are exempt +// from failure counting and will retry indefinitely. 416 is counted as a failure. +func StartTestFailThenRecoverServer(failedRequestsBeforeRecover int32) net.Listener { + var requestCount atomic.Int32 + + return startTestServer(func(sl *shutdownListener) http.Handler { + mux := http.NewServeMux() + mux.HandleFunc("/"+BuildName, func(writer http.ResponseWriter, request *http.Request) { + reqNum := requestCount.Add(1) + + // First request (Resolve) always succeeds + if reqNum == 1 { + rangeFileHandle( + writer, + request, + nil, + func(file *os.File, n int64) { + io.CopyN(writer, file, n) + }, + ) + return + } + + // Subsequent requests fail until we've had enough failures + // Use 416 Range Not Satisfiable - this error is counted towards failure limit + if reqNum <= failedRequestsBeforeRecover+1 { // +1 because first request is resolve + writer.WriteHeader(416) + writer.Write([]byte("Range Not Satisfiable")) + return + } + + // After enough failures, server recovers + rangeFileHandle( + writer, + request, + nil, + func(file *os.File, n int64) { + io.CopyN(writer, file, n) + }, + ) + }) + return mux + }) +} + // StartTestRangeBugServer simulate bug server: // Don't follow Range request rules, always return more data than range, e.g. Range: bytes=0-100, return 150 bytes func StartTestRangeBugServer() net.Listener {