From e16840433d3149c02c9e5f4302c017e0e62b75fc Mon Sep 17 00:00:00 2001 From: OliverTrautvetter <66372584+OliverTrautvetter@users.noreply.github.com> Date: Mon, 18 May 2026 09:22:37 +0200 Subject: [PATCH 1/2] fix: Improve download verification and retry logic --- cli/cmd/download_package.go | 77 ++++++++++++++++++++++--------------- 1 file changed, 47 insertions(+), 30 deletions(-) diff --git a/cli/cmd/download_package.go b/cli/cmd/download_package.go index 4d0ebf38..65305888 100644 --- a/cli/cmd/download_package.go +++ b/cli/cmd/download_package.go @@ -101,37 +101,54 @@ func (c *DownloadPackageCmd) DownloadBuild(p portal.Portal, build portal.Build, } fullFilename := build.BuildPackageFilename(filename) - out, err := c.FileWriter.OpenAppend(fullFilename) - if err != nil { - out, err = c.FileWriter.Create(fullFilename) - if err != nil { - return fmt.Errorf("failed to create file %s: %w", fullFilename, err) - } - } - defer util.CloseFileIgnoreError(out) - - // get already downloaded file size of fullFilename - fileSize := 0 - fileInfo, err := out.Stat() - if err == nil { - fileSize = int(fileInfo.Size()) - } - - err = p.DownloadBuildArtifact("codesphere", download, out, fileSize, c.Opts.Quiet) - if err != nil { - return fmt.Errorf("failed to download build: %w", err) - } + for retried := false; ; retried = true { + shouldRetry, err := func() (bool, error) { + out, err := c.FileWriter.OpenAppend(fullFilename) + if err != nil { + out, err = c.FileWriter.Create(fullFilename) + if err != nil { + return false, fmt.Errorf("failed to create file %s: %w", fullFilename, err) + } + } + defer util.CloseFileIgnoreError(out) + + // get already downloaded file size of fullFilename + fileSize := 0 + if fileInfo, statErr := out.Stat(); statErr == nil { + fileSize = int(fileInfo.Size()) + } + + if err = p.DownloadBuildArtifact("codesphere", download, out, fileSize, c.Opts.Quiet); err != nil { + return false, fmt.Errorf("failed to download build: %w", err) + } + + verifyFile, err := c.FileWriter.Open(fullFilename) + if err != nil { + return false, err + } + defer util.CloseFileIgnoreError(verifyFile) + + verifyErr := p.VerifyBuildArtifactDownload(verifyFile, download) + if verifyErr == nil { + return false, nil + } + + // A resumed download can carry stale or corrupt bytes from a previous + // interrupted attempt, causing verification to fail even though the + // re-download itself succeeded. Delete the partial file so the next + // iteration downloads the full artifact from scratch. + if !retried && fileSize > 0 { + log.Println("Verification failed on resumed download; retrying from scratch...") + if removeErr := c.FileWriter.Remove(fullFilename); removeErr == nil { + return true, nil + } + } - verifyFile, err := c.FileWriter.Open(fullFilename) - if err != nil { - return err - } - defer util.CloseFileIgnoreError(verifyFile) + return false, fmt.Errorf("failed to verify artifact: %w", verifyErr) + }() - err = p.VerifyBuildArtifactDownload(verifyFile, download) - if err != nil { - return fmt.Errorf("failed to verify artifact: %w", err) + if err != nil || !shouldRetry { + return err + } } - - return nil } From 8a7847a9fe375853337ec48163b45f1d746717b0 Mon Sep 17 00:00:00 2001 From: OliverTrautvetter <66372584+OliverTrautvetter@users.noreply.github.com> Date: Mon, 18 May 2026 09:27:17 +0200 Subject: [PATCH 2/2] test: Add verification for stale partial downloads in DownloadBuild --- cli/cmd/download_package_test.go | 83 ++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/cli/cmd/download_package_test.go b/cli/cmd/download_package_test.go index 6316b43c..1a2c0179 100644 --- a/cli/cmd/download_package_test.go +++ b/cli/cmd/download_package_test.go @@ -4,6 +4,7 @@ package cmd_test import ( + "fmt" "os" . "github.com/onsi/ginkgo/v2" @@ -244,4 +245,86 @@ var _ = Describe("DownloadPackages", func() { Expect(err).To(MatchError("failed to find artifact in package: artifact not found: installer-lite.tar.gz")) }) }) + + Context("Stale partial file causes verify failure", func() { + var ( + expectedBuildToDownload portal.Build + fullFilename string + staleFile *os.File + freshFile *os.File + ) + + BeforeEach(func() { + expectedBuildToDownload = portal.Build{ + Version: version, + Hash: hash, + Artifacts: []portal.Artifact{ + {Filename: filename}, + }, + } + fullFilename = version + "-" + hash + "-" + filename + + // Create a temp file with non-zero content to simulate a stale partial download. + var err error + staleFile, err = os.CreateTemp("", "stale-download-*") + Expect(err).NotTo(HaveOccurred()) + _, err = staleFile.Write([]byte("stale partial content")) + Expect(err).NotTo(HaveOccurred()) + _, err = staleFile.Seek(0, 0) + Expect(err).NotTo(HaveOccurred()) + + freshFile = os.NewFile(uintptr(0), filename) + }) + + AfterEach(func() { + _ = staleFile.Close() // may already be closed by the code under test + Expect(os.Remove(staleFile.Name())).To(Succeed()) + }) + + It("deletes the partial file and retries from scratch when resumed download fails verification", func() { + mockFileWriter.EXPECT().OpenAppend(fullFilename).Return(staleFile, nil).Once() + mockPortal.EXPECT().DownloadBuildArtifact(portal.CodesphereProduct, expectedBuildToDownload, mock.Anything, mock.AnythingOfType("int"), false).Return(nil).Once() + mockFileWriter.EXPECT().Open(fullFilename).Return(staleFile, nil).Once() + mockPortal.EXPECT().VerifyBuildArtifactDownload(mock.Anything, expectedBuildToDownload).Return(fmt.Errorf("invalid md5Sum: expected abc, but got xyz")).Once() + mockFileWriter.EXPECT().Remove(fullFilename).Return(nil).Once() + + // Retry: fresh download succeeds + mockFileWriter.EXPECT().OpenAppend(fullFilename).Return(nil, fmt.Errorf("file not found")).Once() + mockFileWriter.EXPECT().Create(fullFilename).Return(freshFile, nil).Once() + mockPortal.EXPECT().DownloadBuildArtifact(portal.CodesphereProduct, expectedBuildToDownload, mock.Anything, 0, false).Return(nil).Once() + mockFileWriter.EXPECT().Open(fullFilename).Return(freshFile, nil).Once() + mockPortal.EXPECT().VerifyBuildArtifactDownload(mock.Anything, expectedBuildToDownload).Return(nil).Once() + + err := c.DownloadBuild(mockPortal, build, filename) + Expect(err).NotTo(HaveOccurred()) + }) + + It("returns verify error without retry when file size is zero", func() { + emptyFile, createErr := os.CreateTemp("", "empty-download-*") + Expect(createErr).NotTo(HaveOccurred()) + DeferCleanup(func() { + _ = emptyFile.Close() // may already be closed by the code under test + Expect(os.Remove(emptyFile.Name())).To(Succeed()) + }) + + mockFileWriter.EXPECT().OpenAppend(fullFilename).Return(emptyFile, nil) + mockPortal.EXPECT().DownloadBuildArtifact(portal.CodesphereProduct, expectedBuildToDownload, mock.Anything, 0, false).Return(nil) + mockFileWriter.EXPECT().Open(fullFilename).Return(emptyFile, nil) + mockPortal.EXPECT().VerifyBuildArtifactDownload(mock.Anything, expectedBuildToDownload).Return(fmt.Errorf("invalid md5Sum: expected abc, but got xyz")) + + err := c.DownloadBuild(mockPortal, build, filename) + Expect(err).To(MatchError(ContainSubstring("failed to verify artifact"))) + }) + + It("returns verify error when remove of stale file fails", func() { + mockFileWriter.EXPECT().OpenAppend(fullFilename).Return(staleFile, nil) + mockPortal.EXPECT().DownloadBuildArtifact(portal.CodesphereProduct, expectedBuildToDownload, mock.Anything, mock.AnythingOfType("int"), false).Return(nil) + mockFileWriter.EXPECT().Open(fullFilename).Return(staleFile, nil) + mockPortal.EXPECT().VerifyBuildArtifactDownload(mock.Anything, expectedBuildToDownload).Return(fmt.Errorf("invalid md5Sum: expected abc, but got xyz")) + mockFileWriter.EXPECT().Remove(fullFilename).Return(fmt.Errorf("permission denied")) + + err := c.DownloadBuild(mockPortal, build, filename) + Expect(err).To(MatchError(ContainSubstring("failed to verify artifact"))) + }) + }) })