Compare commits

...
Author SHA1 Message Date
copilot-swe-agent[bot]andmonkeyWie 0bd306773d 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 <[email protected]>
2026-03-12 01:22:06 +00:00
copilot-swe-agent[bot]andmonkeyWie 8b02844ec3 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 <[email protected]>
2026-03-11 10:53:52 +00:00
copilot-swe-agent[bot]andmonkeyWie b8e517b35b 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 <[email protected]>
2026-03-11 02:59:41 +00:00
copilot-swe-agent[bot]andmonkeyWie 2daa9c45df 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 <[email protected]>
2026-03-11 02:50:48 +00:00
copilot-swe-agent[bot] 27c0f81a1e Initial plan 2026-03-11 02:30:03 +00:00
2 changed files with 141 additions and 11 deletions

No files matched your search

+89 -11
View File
@@ -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
}
@@ -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,22 @@ 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 {
f.connMu.Lock()
remaining := f.meta.Res.Size - conn.Downloaded
if remaining <= 0 {
conn.Completed = true
conn.State = connCompleted
f.connMu.Unlock()
return
}
if remaining < int64(n) {
n = int(remaining)
}
f.connMu.Unlock()
}
f.fileMu.Lock()
if f.file != nil {
_, writeErr := f.file.WriteAt(buf[:n], conn.Chunk.Downloaded)
@@ -1191,8 +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 {
conn.Completed = true
conn.State = connCompleted
f.connMu.Unlock()
return
}
f.connMu.Unlock()
}
if err != nil {
if err == io.EOF {
@@ -1232,6 +1277,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 +1329,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 +1539,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
}
}
+52
View File
@@ -307,6 +307,58 @@ func TestFetcher_DownloadChunked(t *testing.T) {
downloadNormal(listener, 2, 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()
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)
}
// 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 {
t.Errorf("Download() got = %v, want %v", got, want)
}
}
func TestFetcher_DownloadPost(t *testing.T) {
listener := test.StartTestPostServer()
defer listener.Close()