Lock cache directory during download

To solve the races during concurrent downloads and avoid unneeded work
and bandwidth, we allow one concurrent download of the same image.

When a limactl process try to access the cache, it takes a lock on the
file cache directory. If multiple processes try to get the lock in the
same time, only one will take the lock, and the other will block.

The process that took the lock tries to get the file from the cache.
This is the fast path and the common case. This can fail if the file is
not in the cache, the digest does not match, or the cached last modified
time does not match the last modified returned from the server.

If the process cannot get the file from the cache, it downloads the file
from the remote server, and update the cached data and metadata files.

Finally the process release the lock on the cache directory. Other
limactl processes waiting on the lock wake up and take the lock. In the
common case they will find the image in the cache and will release the
lock quickly.

Since we have exactly one concurrent download, updating the metadata
files is trivial and we don't need the writeFirst() helper.

Fixes: #2902
Fixes: #2732
Signed-off-by: Nir Soffer <[email protected]>

Author: nirs

pkg/downloader/downloader.go

@@ -229,14 +229,25 @@ func Download(ctx context.Context, local, remote string, opts ...Opt) (*Result,
 		return res, nil
 	}
 
-	res, err := getCached(ctx, localPath, remote, o)
-	if err != nil {
+	shad := cacheDirectoryPath(o.cacheDir, remote)
+	if err := os.MkdirAll(shad, 0o700); err != nil {
 		return nil, err
 	}
-	if res != nil {
-		return res, nil
-	}
-	return fetch(ctx, localPath, remote, o)
+
+	var res *Result
+	err := lockutil.WithDirLock(shad, func() error {
+		var err error
+		res, err = getCached(ctx, localPath, remote, o)
+		if err != nil {
+			return err
+		}
+		if res != nil {
+			return nil
+		}
+		res, err = fetch(ctx, localPath, remote, o)
+		return err
+	})
+	return res, err
 }
 
 // getCached tries to copy the file from the cache to local path. Return result,
@@ -298,18 +309,15 @@ func fetch(ctx context.Context, localPath, remote string, o options) (*Result, e
 		return nil, err
 	}
 	ext := path.Ext(remote)
-	if err := os.MkdirAll(shad, 0o700); err != nil {
-		return nil, err
-	}
 	shadURL := filepath.Join(shad, "url")
-	if err := writeFirst(shadURL, []byte(remote), 0o644); err != nil {
+	if err := os.WriteFile(shadURL, []byte(remote), 0o644); err != nil {
 		return nil, err
 	}
 	if err := downloadHTTP(ctx, shadData, shadTime, shadType, remote, o.description, o.expectedDigest); err != nil {
 		return nil, err
 	}
 	if shadDigest != "" && o.expectedDigest != "" {
-		if err := writeFirst(shadDigest, []byte(o.expectedDigest.String()), 0o644); err != nil {
+		if err := os.WriteFile(shadDigest, []byte(o.expectedDigest.String()), 0o644); err != nil {
 			return nil, err
 		}
 	}
@@ -638,13 +646,13 @@ func downloadHTTP(ctx context.Context, localPath, lastModified, contentType, url
 	}
 	if lastModified != "" {
 		lm := resp.Header.Get("Last-Modified")
-		if err := writeFirst(lastModified, []byte(lm), 0o644); err != nil {
+		if err := os.WriteFile(lastModified, []byte(lm), 0o644); err != nil {
 			return err
 		}
 	}
 	if contentType != "" {
 		ct := resp.Header.Get("Content-Type")
-		if err := writeFirst(contentType, []byte(ct), 0o644); err != nil {
+		if err := os.WriteFile(contentType, []byte(ct), 0o644); err != nil {
 			return err
 		}
 	}
@@ -705,19 +713,7 @@ func downloadHTTP(ctx context.Context, localPath, lastModified, contentType, url
 		return err
 	}
 
-	// If localPath was created by a parallel download keep it. Replacing it
-	// while another process is copying it to the destination may fail the
-	// clonefile syscall. We use a lock to ensure that only one process updates
-	// data, and when we return data file exists.
-
-	return lockutil.WithDirLock(filepath.Dir(localPath), func() error {
-		if _, err := os.Stat(localPath); err == nil {
-			return nil
-		} else if !errors.Is(err, os.ErrNotExist) {
-			return err
-		}
-		return os.Rename(localPathTmp, localPath)
-	})
+	return os.Rename(localPathTmp, localPath)
 }
 
 var tempfileCount atomic.Uint64
@@ -732,18 +728,6 @@ func perProcessTempfile(path string) string {
 	return fmt.Sprintf("%s.tmp.%d.%d", path, os.Getpid(), tempfileCount.Add(1))
 }
 
-// writeFirst writes data to path unless path already exists.
-func writeFirst(path string, data []byte, perm os.FileMode) error {
-	return lockutil.WithDirLock(filepath.Dir(path), func() error {
-		if _, err := os.Stat(path); err == nil {
-			return nil
-		} else if !errors.Is(err, os.ErrNotExist) {
-			return err
-		}
-		return os.WriteFile(path, data, perm)
-	})
-}
-
 // CacheEntries returns a map of cache entries.
 // The key is the SHA256 of the URL.
 // The value is the path to the cache entry.