From 27c0f81a1e506839f3548fa94638f255c0cbd0b4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Mar 2026 02:30:03 +0000 Subject: [PATCH 1/5] Initial plan From 2daa9c45df0507e1b47871fa73289c547925c503 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Mar 2026 02:50:48 +0000 Subject: [PATCH 2/5] Fix non-range download bug: downloaded bytes exceeding total file size When HTTP server doesn't return Content-Length or doesn't support Range requests, the download can accumulate extra bytes on pause/resume because: 1. Each retry/resume re-downloads from the beginning but writes at the accumulated offset 2. No size limit enforcement for non-range downloads 3. onDownloadComplete incorrectly uses chunk.remain() for non-range downloads, which gives negative values for chunk(0,0) Fixes: - Reset chunk state in downloadChunkOnce and runConnectionFallback for non-range downloads since server will send from beginning - Add size limit checks in download loops when Size is known - Fix onDownloadComplete to use conn.Completed for non-range downloads instead of unreliable chunk.remain() - Add test cases for non-range download, pause/resume, and size limit Co-authored-by: monkeyWie <13160176+monkeyWie@users.noreply.github.com> --- internal/protocol/http/fetcher.go | 94 +++++++++++++++++++++++--- internal/protocol/http/fetcher_test.go | 93 +++++++++++++++++++++++++ internal/test/httptest.go | 19 ++++++ 3 files changed, 197 insertions(+), 9 deletions(-) diff --git a/internal/protocol/http/fetcher.go b/internal/protocol/http/fetcher.go index 709cd70f8..5bf305d76 100644 --- a/internal/protocol/http/fetcher.go +++ b/internal/protocol/http/fetcher.go @@ -987,6 +987,14 @@ func (f *Fetcher) downloadChunkOnce(conn *connection, client *http.Client, buf [ f.connMu.Unlock() return nil } + // For non-range downloads, reset chunk state since server will send from beginning. + // Without Range support, each new request restarts the entire transfer. + if !f.meta.Res.Range { + conn.Chunk.Begin = 0 + conn.Chunk.End = 0 + conn.Chunk.Downloaded = 0 + conn.Downloaded = 0 + } rangeStart := conn.Chunk.Begin + conn.Chunk.Downloaded rangeEnd := conn.Chunk.End f.connMu.Unlock() @@ -1080,6 +1088,17 @@ func (f *Fetcher) downloadChunkOnce(conn *connection, client *http.Client, buf [ n = int(remain) finished = true } + } else if f.meta.Res.Size > 0 { + // For non-range downloads with known size, enforce size limit + remaining := f.meta.Res.Size - conn.Downloaded + if remaining <= 0 { + f.connMu.Unlock() + return nil + } + if remaining < int64(n) { + n = int(remaining) + finished = true + } } writeOffset = conn.Chunk.Begin + conn.Chunk.Downloaded f.connMu.Unlock() @@ -1174,6 +1193,21 @@ func (f *Fetcher) runConnectionWithResolveResp(conn *connection) { n, err := reader.Read(buf) if n > 0 { + // For non-range downloads with known size, enforce size limit + if !f.meta.Res.Range && f.meta.Res.Size > 0 { + remaining := f.meta.Res.Size - conn.Downloaded + if remaining <= 0 { + f.connMu.Lock() + conn.Completed = true + conn.State = connCompleted + f.connMu.Unlock() + return + } + if remaining < int64(n) { + n = int(remaining) + } + } + f.fileMu.Lock() if f.file != nil { _, writeErr := f.file.WriteAt(buf[:n], conn.Chunk.Downloaded) @@ -1193,6 +1227,15 @@ func (f *Fetcher) runConnectionWithResolveResp(conn *connection) { conn.Chunk.Downloaded += int64(n) conn.Downloaded += int64(n) + + // Check if size limit reached after updating counters + if !f.meta.Res.Range && f.meta.Res.Size > 0 && conn.Downloaded >= f.meta.Res.Size { + f.connMu.Lock() + conn.Completed = true + conn.State = connCompleted + f.connMu.Unlock() + return + } } if err != nil { if err == io.EOF { @@ -1232,6 +1275,14 @@ func (f *Fetcher) runConnectionFallback(conn *connection) { f.connMu.Lock() conn.State = connConnecting + // For non-range downloads, reset chunk state since server will send from beginning. + // Without Range support, each new request restarts the entire transfer. + if !f.meta.Res.Range { + conn.Chunk.Begin = 0 + conn.Chunk.End = 0 + conn.Chunk.Downloaded = 0 + conn.Downloaded = 0 + } f.connMu.Unlock() err := func() error { @@ -1276,6 +1327,17 @@ func (f *Fetcher) runConnectionFallback(conn *connection) { n, err := reader.Read(buf) if n > 0 { + // For non-range downloads with known size, enforce size limit + if !f.meta.Res.Range && f.meta.Res.Size > 0 { + remaining := f.meta.Res.Size - conn.Downloaded + if remaining <= 0 { + return nil + } + if remaining < int64(n) { + n = int(remaining) + } + } + f.fileMu.Lock() if f.file != nil { _, writeErr := f.file.WriteAt(buf[:n], conn.Chunk.Downloaded) @@ -1475,17 +1537,31 @@ func (f *Fetcher) onDownloadComplete() { // Check if all chunks are complete (no remaining bytes) allChunksComplete := true for _, conn := range f.connections { - if conn.Chunk != nil && conn.Chunk.remain() > 0 && !conn.Completed && conn.State != connCompleted { - // This connection has remaining work and isn't done - // Check if it failed with 403 (server limit) - these can be ignored if other connections completed the work - if conn.State == connFailed && conn.failed { - if re := extractRequestError(conn.lastErr); re != nil && re.Code == 403 { - // 403 is server connection limit, check if other connections will complete this chunk - continue + if !conn.Completed && conn.State != connCompleted { + if f.meta.Res.Range { + // For range downloads, use chunk remain() to check completion + if conn.Chunk != nil && conn.Chunk.remain() > 0 { + // This connection has remaining work and isn't done + // Check if it failed with 403 (server limit) - these can be ignored if other connections completed the work + if conn.State == connFailed && conn.failed { + if re := extractRequestError(conn.lastErr); re != nil && re.Code == 403 { + continue + } + } + allChunksComplete = false + break } + } else { + // For non-range downloads, completion is determined solely by EOF (conn.Completed). + // chunk.remain() is unreliable for non-range chunks (Begin=0, End=0). + if conn.State == connFailed && conn.failed { + if re := extractRequestError(conn.lastErr); re != nil && re.Code == 403 { + continue + } + } + allChunksComplete = false + break } - allChunksComplete = false - break } } diff --git a/internal/protocol/http/fetcher_test.go b/internal/protocol/http/fetcher_test.go index ec28b6a37..1c86c43ed 100644 --- a/internal/protocol/http/fetcher_test.go +++ b/internal/protocol/http/fetcher_test.go @@ -307,6 +307,99 @@ func TestFetcher_DownloadChunked(t *testing.T) { downloadNormal(listener, 2, t) } +func TestFetcher_DownloadChunkedContinue(t *testing.T) { + listener := test.StartTestCustomServer() + defer listener.Close() + + downloadContinue(listener, 1, t) +} + +func TestFetcher_DownloadNoRangeWithSize(t *testing.T) { + listener := test.StartTestNoRangeServer() + defer listener.Close() + + downloadNormal(listener, 1, t) +} + +func TestFetcher_DownloadNoRangeContinue(t *testing.T) { + listener := test.StartTestNoRangeServer() + defer listener.Close() + + downloadContinue(listener, 1, t) +} + +// TestFetcher_DownloadNoRangeNotExceedSize verifies that when a server returns +// Content-Length but doesn't support Range requests, the downloaded bytes never +// exceed the total file size after pause and resume. +func TestFetcher_DownloadNoRangeNotExceedSize(t *testing.T) { + listener := test.StartTestNoRangeServer() + defer listener.Close() + + fetcher := downloadReady(listener, 1, t) + err := fetcher.Start() + if err != nil { + t.Fatal(err) + } + // Let it download some data + time.Sleep(time.Millisecond * 50) + if err := fetcher.Pause(); err != nil { + t.Fatal(err) + } + time.Sleep(time.Millisecond * 50) + if err := fetcher.Start(); err != nil { + t.Fatal(err) + } + err = fetcher.Wait() + if err != nil { + t.Fatal(err) + } + + // Verify total downloaded does not exceed file size + totalDownloaded := fetcher.Progress().TotalDownloaded() + if totalDownloaded > test.BuildSize { + t.Errorf("Downloaded %d bytes exceeds file size %d", totalDownloaded, test.BuildSize) + } + + want := test.FileMd5(test.BuildFile) + got := test.FileMd5(test.DownloadFile) + if want != got { + t.Errorf("Download() got = %v, want %v", got, want) + } +} + +// TestFetcher_DownloadChunkedNotExceedSize verifies that when a server doesn't +// return Content-Length (chunked transfer), the download properly handles +// pause/resume without accumulating extra bytes. +func TestFetcher_DownloadChunkedNotExceedSize(t *testing.T) { + listener := test.StartTestCustomServer() + defer listener.Close() + + fetcher := downloadReady(listener, 1, t) + err := fetcher.Start() + if err != nil { + t.Fatal(err) + } + // Let it download some data + time.Sleep(time.Millisecond * 50) + if err := fetcher.Pause(); err != nil { + t.Fatal(err) + } + time.Sleep(time.Millisecond * 50) + if err := fetcher.Start(); err != nil { + t.Fatal(err) + } + err = fetcher.Wait() + if err != nil { + t.Fatal(err) + } + + want := test.FileMd5(test.BuildFile) + got := test.FileMd5(test.DownloadFile) + if want != got { + t.Errorf("Download() got = %v, want %v", got, want) + } +} + func TestFetcher_DownloadPost(t *testing.T) { listener := test.StartTestPostServer() defer listener.Close() diff --git a/internal/test/httptest.go b/internal/test/httptest.go index 677600093..f3cd535bf 100644 --- a/internal/test/httptest.go +++ b/internal/test/httptest.go @@ -636,6 +636,25 @@ func StartTestRangeBugServer() net.Listener { }) } +// StartTestNoRangeServer starts a server that returns Content-Length but does NOT support Range requests. +// This simulates servers where file size is known but range downloads are not supported. +func StartTestNoRangeServer() net.Listener { + return startTestServer(func(sl *shutdownListener) http.Handler { + mux := http.NewServeMux() + mux.HandleFunc("/"+BuildName, func(writer http.ResponseWriter, request *http.Request) { + file, err := os.Open(BuildFile) + if err != nil { + panic(err) + } + defer file.Close() + writer.Header().Set("Content-Length", fmt.Sprintf("%d", BuildSize)) + writer.WriteHeader(200) + io.Copy(writer, file) + }) + return mux + }) +} + func rangeFileHandle(writer http.ResponseWriter, request *http.Request, modifyEnd func(end int64) int64, iocpN func(file *os.File, n int64)) { r := request.Header.Get("Range") From b8e517b35b6e7115a4cd761d25ce0e072ff9ab22 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Mar 2026 02:59:41 +0000 Subject: [PATCH 3/5] Fix data race in expandConnections: remove orphaned waitForCompletion goroutine The go f.waitForCompletion() goroutine in expandConnections was redundant since the downloadLoop's slow start loop already handles stateSteady by calling waitForCompletion directly. The orphaned goroutine could race with doStart() when writing f.ctx during pause/resume. Co-authored-by: monkeyWie <13160176+monkeyWie@users.noreply.github.com> --- internal/protocol/http/fetcher.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/protocol/http/fetcher.go b/internal/protocol/http/fetcher.go index 5bf305d76..1b45cf843 100644 --- a/internal/protocol/http/fetcher.go +++ b/internal/protocol/http/fetcher.go @@ -872,9 +872,9 @@ func (f *Fetcher) expandConnections() { f.connMu.Unlock() if len(newConns) == 0 { - // No new connections could be created, stop expansion + // No new connections could be created, stop expansion. + // The downloadLoop will check stateSteady and call waitForCompletion. f.setState(stateSteady) - go f.waitForCompletion() return } @@ -1195,9 +1195,9 @@ func (f *Fetcher) runConnectionWithResolveResp(conn *connection) { if n > 0 { // For non-range downloads with known size, enforce size limit if !f.meta.Res.Range && f.meta.Res.Size > 0 { + f.connMu.Lock() remaining := f.meta.Res.Size - conn.Downloaded if remaining <= 0 { - f.connMu.Lock() conn.Completed = true conn.State = connCompleted f.connMu.Unlock() @@ -1206,6 +1206,7 @@ func (f *Fetcher) runConnectionWithResolveResp(conn *connection) { if remaining < int64(n) { n = int(remaining) } + f.connMu.Unlock() } f.fileMu.Lock() @@ -1225,17 +1226,18 @@ func (f *Fetcher) runConnectionWithResolveResp(conn *connection) { } f.fileMu.Unlock() + f.connMu.Lock() conn.Chunk.Downloaded += int64(n) conn.Downloaded += int64(n) // Check if size limit reached after updating counters if !f.meta.Res.Range && f.meta.Res.Size > 0 && conn.Downloaded >= f.meta.Res.Size { - f.connMu.Lock() conn.Completed = true conn.State = connCompleted f.connMu.Unlock() return } + f.connMu.Unlock() } if err != nil { if err == io.EOF { From 8b02844ec35d4fa3cafffb94549363046fb97eab Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Mar 2026 10:53:52 +0000 Subject: [PATCH 4/5] Fix tests: use StartTestCustomServer (no Content-Length) to match real bug scenario Remove StartTestNoRangeServer and its tests since the real bug is about servers that don't return Content-Length at all (like GitHub's zip downloads), not servers with Content-Length but no Range support. The chunked tests (TestFetcher_DownloadChunkedContinue and TestFetcher_DownloadChunkedNotExceedSize) correctly simulate the real-world bug using StartTestCustomServer which serves files without Content-Length. Co-authored-by: monkeyWie <13160176+monkeyWie@users.noreply.github.com> --- internal/protocol/http/fetcher_test.go | 59 ++------------------------ internal/test/httptest.go | 19 --------- 2 files changed, 4 insertions(+), 74 deletions(-) diff --git a/internal/protocol/http/fetcher_test.go b/internal/protocol/http/fetcher_test.go index 1c86c43ed..377266d58 100644 --- a/internal/protocol/http/fetcher_test.go +++ b/internal/protocol/http/fetcher_test.go @@ -314,62 +314,11 @@ func TestFetcher_DownloadChunkedContinue(t *testing.T) { downloadContinue(listener, 1, t) } -func TestFetcher_DownloadNoRangeWithSize(t *testing.T) { - listener := test.StartTestNoRangeServer() - defer listener.Close() - - downloadNormal(listener, 1, t) -} - -func TestFetcher_DownloadNoRangeContinue(t *testing.T) { - listener := test.StartTestNoRangeServer() - defer listener.Close() - - downloadContinue(listener, 1, t) -} - -// TestFetcher_DownloadNoRangeNotExceedSize verifies that when a server returns -// Content-Length but doesn't support Range requests, the downloaded bytes never -// exceed the total file size after pause and resume. -func TestFetcher_DownloadNoRangeNotExceedSize(t *testing.T) { - listener := test.StartTestNoRangeServer() - defer listener.Close() - - fetcher := downloadReady(listener, 1, t) - err := fetcher.Start() - if err != nil { - t.Fatal(err) - } - // Let it download some data - time.Sleep(time.Millisecond * 50) - if err := fetcher.Pause(); err != nil { - t.Fatal(err) - } - time.Sleep(time.Millisecond * 50) - if err := fetcher.Start(); err != nil { - t.Fatal(err) - } - err = fetcher.Wait() - if err != nil { - t.Fatal(err) - } - - // Verify total downloaded does not exceed file size - totalDownloaded := fetcher.Progress().TotalDownloaded() - if totalDownloaded > test.BuildSize { - t.Errorf("Downloaded %d bytes exceeds file size %d", totalDownloaded, test.BuildSize) - } - - want := test.FileMd5(test.BuildFile) - got := test.FileMd5(test.DownloadFile) - if want != got { - t.Errorf("Download() got = %v, want %v", got, want) - } -} - // TestFetcher_DownloadChunkedNotExceedSize verifies that when a server doesn't -// return Content-Length (chunked transfer), the download properly handles -// pause/resume without accumulating extra bytes. +// return Content-Length (chunked transfer, no Range support), the download +// properly handles pause/resume without accumulating extra bytes. +// This simulates the real-world bug where servers like GitHub's zip downloads +// don't return Content-Length. func TestFetcher_DownloadChunkedNotExceedSize(t *testing.T) { listener := test.StartTestCustomServer() defer listener.Close() diff --git a/internal/test/httptest.go b/internal/test/httptest.go index f3cd535bf..677600093 100644 --- a/internal/test/httptest.go +++ b/internal/test/httptest.go @@ -636,25 +636,6 @@ func StartTestRangeBugServer() net.Listener { }) } -// StartTestNoRangeServer starts a server that returns Content-Length but does NOT support Range requests. -// This simulates servers where file size is known but range downloads are not supported. -func StartTestNoRangeServer() net.Listener { - return startTestServer(func(sl *shutdownListener) http.Handler { - mux := http.NewServeMux() - mux.HandleFunc("/"+BuildName, func(writer http.ResponseWriter, request *http.Request) { - file, err := os.Open(BuildFile) - if err != nil { - panic(err) - } - defer file.Close() - writer.Header().Set("Content-Length", fmt.Sprintf("%d", BuildSize)) - writer.WriteHeader(200) - io.Copy(writer, file) - }) - return mux - }) -} - func rangeFileHandle(writer http.ResponseWriter, request *http.Request, modifyEnd func(end int64) int64, iocpN func(file *os.File, n int64)) { r := request.Header.Get("Range") From 0bd306773d2a9459b3ab88304123c894297c7f9c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 12 Mar 2026 01:22:06 +0000 Subject: [PATCH 5/5] Replace redundant chunked tests with targeted non-range pause/resume test Remove TestFetcher_DownloadChunkedContinue (redundant with existing tests). Replace TestFetcher_DownloadChunkedNotExceedSize with TestFetcher_DownloadNoRangePauseResume which specifically targets the bug: - Documents the root cause (non-range resume writes at accumulated offset instead of byte 0) - Asserts downloaded file size does not exceed source file size (the exact bug symptom from the issue) - Verifies file integrity via MD5 Co-authored-by: monkeyWie <13160176+monkeyWie@users.noreply.github.com> --- internal/protocol/http/fetcher_test.go | 36 ++++++++++++++++---------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/internal/protocol/http/fetcher_test.go b/internal/protocol/http/fetcher_test.go index 377266d58..c49502108 100644 --- a/internal/protocol/http/fetcher_test.go +++ b/internal/protocol/http/fetcher_test.go @@ -307,19 +307,14 @@ func TestFetcher_DownloadChunked(t *testing.T) { downloadNormal(listener, 2, t) } -func TestFetcher_DownloadChunkedContinue(t *testing.T) { - listener := test.StartTestCustomServer() - defer listener.Close() - - downloadContinue(listener, 1, t) -} - -// TestFetcher_DownloadChunkedNotExceedSize verifies that when a server doesn't -// return Content-Length (chunked transfer, no Range support), the download -// properly handles pause/resume without accumulating extra bytes. -// This simulates the real-world bug where servers like GitHub's zip downloads -// don't return Content-Length. -func TestFetcher_DownloadChunkedNotExceedSize(t *testing.T) { +// TestFetcher_DownloadNoRangePauseResume targets a specific bug where pausing +// and resuming a download from a server that doesn't return Content-Length +// (and doesn't support Range) causes the downloaded file to grow beyond the +// actual file size. Root cause: on resume the server resends from byte 0, but +// the old code wrote at the previously accumulated offset, appending a second +// copy instead of overwriting. The fix resets the write offset on each non-range +// retry so data is always written from the beginning. +func TestFetcher_DownloadNoRangePauseResume(t *testing.T) { listener := test.StartTestCustomServer() defer listener.Close() @@ -342,6 +337,21 @@ func TestFetcher_DownloadChunkedNotExceedSize(t *testing.T) { t.Fatal(err) } + // The critical assertion: the downloaded file must not be larger than + // the source file. Before the fix, each resume appended extra data, + // causing the file to exceed the actual size (the bug shown in the issue). + downloadInfo, err := os.Stat(test.DownloadFile) + if err != nil { + t.Fatal(err) + } + sourceInfo, err := os.Stat(test.BuildFile) + if err != nil { + t.Fatal(err) + } + if downloadInfo.Size() > sourceInfo.Size() { + t.Errorf("Downloaded file size %d exceeds source file size %d", downloadInfo.Size(), sourceInfo.Size()) + } + want := test.FileMd5(test.BuildFile) got := test.FileMd5(test.DownloadFile) if want != got {