diff --git a/pkg/downloader/stall_test.go b/pkg/downloader/stall_test.go index 8e6a003c6..34ae2d348 100644 --- a/pkg/downloader/stall_test.go +++ b/pkg/downloader/stall_test.go @@ -2,6 +2,7 @@ package downloader_test import ( "context" + "fmt" "net/http" "net/http/httptest" "os" @@ -99,6 +100,54 @@ var _ = Describe("Download stall timeout", func() { Expect(info.Size()).To(BeNumerically(">", 0)) }) + It("does not count partial-file hashing time against the stall window when resuming", func() { + // A resumed download re-hashes the existing .partial before the copy + // loop starts. On slow storage a multi-GB partial takes longer to hash + // than the stall window, and since no network read happens while + // hashing, a watchdog armed before the hash aborts a healthy resume. + // Every retry then re-pays the same hash and fails identically, so the + // install wedges permanently (observed with a 7.9GB partial on a CIFS + // models share). The sparse file keeps disk usage at zero while still + // forcing the hash to churn through every byte. + const partialSize = int64(2) << 30 + tail := []byte("remaining-bytes") + total := partialSize + int64(len(tail)) + + partial, err := os.OpenFile(filePath+".partial", os.O_CREATE|os.O_WRONLY, 0600) + Expect(err).ToNot(HaveOccurred()) + Expect(partial.Truncate(partialSize)).To(Succeed()) + Expect(partial.Close()).To(Succeed()) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == "HEAD" { + w.Header().Set("Accept-Ranges", "bytes") + w.WriteHeader(http.StatusOK) + return + } + if r.Header.Get("Range") == "" { + // A resume must ask for a range; anything else would re-download + // (and here, concatenate) the whole body. + w.WriteHeader(http.StatusInternalServerError) + return + } + w.Header().Set("Content-Range", fmt.Sprintf("bytes %d-%d/%d", partialSize, total-1, total)) + w.WriteHeader(http.StatusPartialContent) + _, _ = w.Write(tail) + })) + defer server.Close() + + DownloadStallTimeout = 150 * time.Millisecond + + err = URI(server.URL).DownloadFileWithContext( + context.Background(), filePath, "", 1, 1, + func(s1, s2, s3 string, f float64) {}) + Expect(err).ToNot(HaveOccurred(), "a promptly-answered resume must not be aborted by hash time") + + info, statErr := os.Stat(filePath) + Expect(statErr).ToNot(HaveOccurred()) + Expect(info.Size()).To(Equal(total)) + }) + It("does not abort a slow-but-steady download", func() { // One byte every 100ms keeps the idle clock from ever expiring even // though the total transfer outlasts the stall timeout. diff --git a/pkg/downloader/uri.go b/pkg/downloader/uri.go index 1410b1577..1c985720c 100644 --- a/pkg/downloader/uri.go +++ b/pkg/downloader/uri.go @@ -674,6 +674,32 @@ func (uri URI) DownloadFileWithContext(ctx context.Context, filePath, sha string return fmt.Errorf("failed to check partial download file %q: %w", tmpFilePath, statErr) } + // Create parent directory + err = os.MkdirAll(filepath.Dir(filePath), 0750) + if err != nil { + return fmt.Errorf("failed to create parent directory for file %q: %v", filePath, err) + } + + // Open the partial and hash its existing bytes BEFORE issuing the request. + // The stall watchdog arms the moment the response body exists, and nothing + // reads that body while the partial is hashed; on slow storage a multi-GB + // partial takes longer to hash than the stall window, so hashing after the + // request aborts every resume, and each retry re-pays the same hash and + // fails identically, wedging the install permanently. Hashing first also + // keeps the origin from idling out the connection during the hash. + outFile, err := os.OpenFile(tmpFilePath, os.O_APPEND|os.O_RDWR|os.O_CREATE, 0600) + if err != nil { + return fmt.Errorf("failed to create / open file %q: %v", tmpFilePath, err) + } + defer func() { _ = outFile.Close() }() + if err := outFile.Chmod(0600); err != nil { + return fmt.Errorf("failed to restrict partial file %q permissions: %v", tmpFilePath, err) + } + hash, err := calculateHashForPartialFile(outFile) + if err != nil { + return fmt.Errorf("failed to calculate hash for partial file") + } + var source io.ReadCloser var contentLength int64 if _, e := os.Stat(uri.ResolveURL()); strings.HasPrefix(string(uri), LocalPrefix) || e == nil { @@ -743,25 +769,6 @@ func (uri URI) DownloadFileWithContext(ctx context.Context, filePath, sha string } defer source.Close() - // Create parent directory - err = os.MkdirAll(filepath.Dir(filePath), 0750) - if err != nil { - return fmt.Errorf("failed to create parent directory for file %q: %v", filePath, err) - } - - // Create and write file - outFile, err := os.OpenFile(tmpFilePath, os.O_APPEND|os.O_RDWR|os.O_CREATE, 0600) - if err != nil { - return fmt.Errorf("failed to create / open file %q: %v", tmpFilePath, err) - } - defer outFile.Close() - if err := outFile.Chmod(0600); err != nil { - return fmt.Errorf("failed to restrict partial file %q permissions: %v", tmpFilePath, err) - } - hash, err := calculateHashForPartialFile(outFile) - if err != nil { - return fmt.Errorf("failed to calculate hash for partial file") - } progress := &progressWriter{ fileName: tmpFilePath, total: contentLength,