From f2fb061088cbe4b38b36450bcc45c7be11e4eb04 Mon Sep 17 00:00:00 2001
From: Nir Soffer <nsoffer@redhat.com>
Date: Fri, 11 Oct 2024 03:30:25 +0300
Subject: [PATCH] Fix parallel downloads of the same image

When starting multiple vms in parallel using the same uncached yet
image, we downloaded the data to the same temporary file (data.tmp) and
wrote metadata to the same files (url, time, type, {digest}.digest). We
could fail in many ways:

- One process removes the cache directory when another tries to rename a
  temporary file to the destination file.
- One process fails to remove the cache directory since another process
  started to download and the directory is not empty.
- Corrupting data when writing to the same file from multiple processes.
  Mostly likely when writing downloaded data.

Fixes:

- Do not remove the cache directory before the download. This avoids the
  failing to remove non-empty directory, and failure to rename a
  temporary file.
- Write all data to temporary files, replacing the destination file when
  the write is complete. This avoid corrupted data.
- Do not remove destination file when creating a new temporary file or
  renaming since creating a file truncate existing file and renaming
  replace it.

Issues:

- On windows os.Rename is not atomic, but it should work when using
  NTFS.
- On failures we may have lefover temporary files ({name}.tmp.{pid}).

Fixes #2722

Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 pkg/downloader/downloader.go | 56 +++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 13 deletions(-)

diff --git a/pkg/downloader/downloader.go b/pkg/downloader/downloader.go
index a03fea0d249..18418e845b0 100644
--- a/pkg/downloader/downloader.go
+++ b/pkg/downloader/downloader.go
@@ -12,6 +12,7 @@ import (
 	"os/exec"
 	"path"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"time"
 
@@ -262,14 +263,11 @@ func Download(ctx context.Context, local, remote string, opts ...Opt) (*Result,
 			return res, nil
 		}
 	}
-	if err := os.RemoveAll(shad); err != nil {
-		return nil, err
-	}
 	if err := os.MkdirAll(shad, 0o700); err != nil {
 		return nil, err
 	}
 	shadURL := filepath.Join(shad, "url")
-	if err := os.WriteFile(shadURL, []byte(remote), 0o644); err != nil {
+	if err := atomicWrite(shadURL, []byte(remote), 0o644); err != nil {
 		return nil, err
 	}
 	if err := downloadHTTP(ctx, shadData, shadTime, shadType, remote, o.description, o.expectedDigest); err != nil {
@@ -280,7 +278,7 @@ func Download(ctx context.Context, local, remote string, opts ...Opt) (*Result,
 		return nil, err
 	}
 	if shadDigest != "" && o.expectedDigest != "" {
-		if err := os.WriteFile(shadDigest, []byte(o.expectedDigest.String()), 0o644); err != nil {
+		if err := atomicWrite(shadDigest, []byte(o.expectedDigest.String()), 0o644); err != nil {
 			return nil, err
 		}
 	}
@@ -600,10 +598,8 @@ func downloadHTTP(ctx context.Context, localPath, lastModified, contentType, url
 		return fmt.Errorf("downloadHTTP: got empty localPath")
 	}
 	logrus.Debugf("downloading %q into %q", url, localPath)
-	localPathTmp := localPath + ".tmp"
-	if err := os.RemoveAll(localPathTmp); err != nil {
-		return err
-	}
+
+	localPathTmp := perProcessTempfile(localPath)
 	fileWriter, err := os.Create(localPathTmp)
 	if err != nil {
 		return err
@@ -616,13 +612,13 @@ func downloadHTTP(ctx context.Context, localPath, lastModified, contentType, url
 	}
 	if lastModified != "" {
 		lm := resp.Header.Get("Last-Modified")
-		if err := os.WriteFile(lastModified, []byte(lm), 0o644); err != nil {
+		if err := atomicWrite(lastModified, []byte(lm), 0o644); err != nil {
 			return err
 		}
 	}
 	if contentType != "" {
 		ct := resp.Header.Get("Content-Type")
-		if err := os.WriteFile(contentType, []byte(ct), 0o644); err != nil {
+		if err := atomicWrite(contentType, []byte(ct), 0o644); err != nil {
 			return err
 		}
 	}
@@ -674,10 +670,44 @@ func downloadHTTP(ctx context.Context, localPath, lastModified, contentType, url
 	if err := fileWriter.Close(); err != nil {
 		return err
 	}
-	if err := os.RemoveAll(localPath); err != nil {
+
+	return os.Rename(localPathTmp, localPath)
+}
+
+// To allow parallel download we use a per-process unique suffix for tempoary
+// files. Renaming the temporary file to the final file is safe without
+// synchronization on posix.
+// https://github.com/lima-vm/lima/issues/2722
+func perProcessTempfile(path string) string {
+	return path + ".tmp." + strconv.FormatInt(int64(os.Getpid()), 10)
+}
+
+// atomicWrite writes data to path, creating a new file or replacing existing
+// one. Multiple processess can write to the same path safely. Safe on posix and
+// likely safe on windows when using NTFS.
+func atomicWrite(path string, data []byte, perm os.FileMode) error {
+	tmpPath := perProcessTempfile(path)
+	tmp, err := os.OpenFile(tmpPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm)
+	if err != nil {
+		return err
+	}
+	defer func() {
+		if err != nil {
+			tmp.Close()
+			os.RemoveAll(tmpPath)
+		}
+	}()
+	if _, err = tmp.Write(data); err != nil {
 		return err
 	}
-	return os.Rename(localPathTmp, localPath)
+	if err = tmp.Sync(); err != nil {
+		return err
+	}
+	if err = tmp.Close(); err != nil {
+		return err
+	}
+	err = os.Rename(tmpPath, path)
+	return err
 }
 
 // CacheEntries returns a map of cache entries.