Deep Review: 20260327-223819-pr-4637

pkg/downloader/downloader.go, important, regression

				} else {
					origStat, _ := os.Stat(shadData)
					if origStat.ModTime().After(rawStat.ModTime()) {
						needConvert = true
					}
				}

				if needConvert {
					logrus.Infof("Converted raw image is missing or stale; (re)converting now.")
					converted, rawDigest, err := ensureRawInCache(ctx, shadData, imageFormat, o.expectedDigest)
					if err != nil {
						return nil, err
					}
					shadData = converted
					o.expectedDigest = rawDigest
					shadDigest = rawImgConvDigestPath
				} else {
					shadData = rawImgConvPath
					if currentDigestData, err := os.ReadFile(rawImgConvDigestPath); err == nil {
						currentDigest := strings.TrimSpace(string(currentDigestData))
						if d, err := digest.Parse(currentDigest); err == nil {
							o.expectedDigest = d
							shadDigest = rawImgConvDigestPath
						}
					}
				}
			}
		}

When an image requires conversion, lines 311–313 replace o.expectedDigest with the raw image's digest and redirect shadDigest to rawImgConvDigestPath. The original user-provided digest (from the template) is never validated against the cached source image. Later, validateCachedDigest at line 329 compares the raw digest against its own file — a self-confirming check.

	ext := path.Ext(remote)
	logrus.Debugf("file %q is cached as %q", localPath, shadData)
	if _, err := os.Stat(shadDigest); err == nil {
		logrus.Debugf("Comparing digest %q with the cached digest file %q, not computing the actual digest of %q",
			o.expectedDigest, shadDigest, shadData)
		if err := validateCachedDigest(shadDigest, o.expectedDigest); err != nil {
			return nil, err
		}
		if err := copyLocal(ctx, localPath, shadData, ext, o.decompress, "", ""); err != nil {
			return nil, err
		}
	} else {
		if match, lmCached, lmRemote, err := matchLastModified(ctx, shadTime, remote); err != nil {
			logrus.WithError(err).Info("Failed to retrieve last-modified for cached digest-less image; using cached image.")
		} else if match {
			if err := copyLocal(ctx, localPath, shadData, ext, o.decompress, o.description, o.expectedDigest); err != nil {
				return nil, err
			}
		} else {
			logrus.Infof("Re-downloading digest-less image: last-modified mismatch (cached: %q, remote: %q)", lmCached, lmRemote)
			return nil, nil
		}
	}
	res := &Result{

If an upstream image at the same URL is updated and the user changes the expected digest in their template, the stale cached image is served without detection.

Fix: Validate the original cached file against the caller's expectedDigest before running conversion logic. Move the image-conversion block after the existing digest validation (lines 329–340), or validate explicitly within the conversion block:

 if needConvert {
+    if o.expectedDigest != "" && shadDigest != "" {
+        if err := validateCachedDigest(shadDigest, o.expectedDigest); err != nil {
+            return nil, nil // force re-download
+        }
+    }
     converted, rawDigest, err := ensureRawInCache(ctx, shadData, imageFormat, o.expectedDigest)

pkg/downloader/downloader.go, important, regression

			} else {
				shadData = rawImgConvPath
				if currentDigestData, err := os.ReadFile(rawImgConvDigestPath); err == nil {
					currentDigest := strings.TrimSpace(string(currentDigestData))
					if d, err := digest.Parse(currentDigest); err == nil {
						o.expectedDigest = d
						shadDigest = rawImgConvDigestPath
					}
				}
			}
		}
	}

ensureRawInCache renames the raw file into place (line 453) before writing raw.digest (line 467). If the process crashes between these two operations, raw.digest is missing on restart. The else block at line 314 sets shadData to the raw path but silently falls through when ReadFile fails, leaving shadDigest and o.expectedDigest unchanged. The subsequent validation at line 329 then validates the original digest file against the original expected digest — which passes — but copies the unverified raw file (via shadData).

		return "", "", fmt.Errorf("failed to close raw tmp file %q: %w", rawPathTmp, err)
	}

	if err := os.Rename(rawPathTmp, rawImgConvPath); err != nil {
		return "", "", fmt.Errorf("failed to replace original with raw image: %w", err)
	}

	algo := digest.Canonical
	if originalDigest != "" {
		algo = originalDigest.Algorithm()
	}
	rawDigest, err := calculateFileDigest(rawImgConvPath, algo)
	if err != nil {
		return "", "", fmt.Errorf("failed to calculate digest of raw image: %w", err)
	}

	rawDigestPath := filepath.Join(imgConvPath, "raw.digest")
	if err := os.WriteFile(rawDigestPath, []byte(rawDigest.String()), 0o644); err != nil {
		return "", "", fmt.Errorf("failed to write raw digest file: %w", err)
	}

	return rawImgConvPath, rawDigest, nil
}

Fix: Treat a raw file without a valid raw.digest as incomplete. Force reconversion:

 } else {
 	shadData = rawImgConvPath
 	if currentDigestData, err := os.ReadFile(rawImgConvDigestPath); err == nil {
 		currentDigest := strings.TrimSpace(string(currentDigestData))
 		if d, err := digest.Parse(currentDigest); err == nil {
 			o.expectedDigest = d
 			shadDigest = rawImgConvDigestPath
+		} else {
+			needConvert = true
 		}
+	} else {
+		needConvert = true
 	}
 }

Then re-execute conversion if needConvert was set in the else block.

pkg/downloader/downloader.go, important, regression

			} else {
				shadData = rawImgConvPath
				if currentDigestData, err := os.ReadFile(rawImgConvDigestPath); err == nil {
					currentDigest := strings.TrimSpace(string(currentDigestData))
					if d, err := digest.Parse(currentDigest); err == nil {
						o.expectedDigest = d
						shadDigest = rawImgConvDigestPath
					}
				}
			}
		}
	}

When a cached raw image exists with a valid raw.digest, lines 318–320 populate o.expectedDigest and shadDigest. The subsequent check at line 329 (os.Stat(shadDigest) succeeds) routes into the digest-validation branch and skips the matchLastModified check at line 339. Images that previously revalidated via Last-Modified become permanently cached after conversion, never detecting upstream updates.

	ext := path.Ext(remote)
	logrus.Debugf("file %q is cached as %q", localPath, shadData)
	if _, err := os.Stat(shadDigest); err == nil {
		logrus.Debugf("Comparing digest %q with the cached digest file %q, not computing the actual digest of %q",
			o.expectedDigest, shadDigest, shadData)
		if err := validateCachedDigest(shadDigest, o.expectedDigest); err != nil {
			return nil, err
		}
		if err := copyLocal(ctx, localPath, shadData, ext, o.decompress, "", ""); err != nil {
			return nil, err
		}
	} else {
		if match, lmCached, lmRemote, err := matchLastModified(ctx, shadTime, remote); err != nil {
			logrus.WithError(err).Info("Failed to retrieve last-modified for cached digest-less image; using cached image.")
		} else if match {
			if err := copyLocal(ctx, localPath, shadData, ext, o.decompress, o.description, o.expectedDigest); err != nil {
				return nil, err
			}
		} else {
			logrus.Infof("Re-downloading digest-less image: last-modified mismatch (cached: %q, remote: %q)", lmCached, lmRemote)
			return nil, nil
		}
	}
	res := &Result{

Fix: Preserve remote freshness and local integrity as separate checks. For callers without a user-provided digest, run matchLastModified before serving imgconv/raw; use raw.digest only for local integrity verification:

+	originalExpectedDigest := o.expectedDigest
 	// ... conversion block ...
+	// After conversion, restore original digest state for freshness logic
+	if originalExpectedDigest == "" {
+		o.expectedDigest = ""
+		shadDigest = ""
+	}

pkg/downloader/downloader.go and pkg/fileutils/download.go, important, regression

func isNonISOImage(description, remote string, supportedImageFormats []string) bool {
	return strings.Contains(description, "image") && supportedImageFormats != nil && path.Ext(remote) != ".iso"
}

		downloader.WithCache(),
		downloader.WithDecompress(decompress),
		downloader.WithDescription(fmt.Sprintf("%s (%s)", description, path.Base(f.Location))),
		downloader.WithExpectedDigest(f.Digest),
	}
	if strings.Contains(description, "image") && supportedImageFormats != nil {
		opts = append(opts, downloader.WithImageFormats(supportedImageFormats))
	}

Both locations parse the human-readable description string to decide whether to trigger image conversion. A future caller that uses "image" in a description for a non-disk artifact (e.g., "container image manifest") would trigger unwanted conversion. This matches AkihiroSuda's unresolved review comment: "description is not expected to be parsed."

Fix: Remove description parsing. Guard by supportedImageFormats alone, as AkihiroSuda suggested:

 // fileutils/download.go
-if strings.Contains(description, "image") && supportedImageFormats != nil {
+if len(supportedImageFormats) > 0 {

 // downloader.go
 func isNonISOImage(description, remote string, supportedImageFormats []string) bool {
-	return strings.Contains(description, "image") && supportedImageFormats != nil && path.Ext(remote) != ".iso"
+	return supportedImageFormats != nil && path.Ext(remote) != ".iso"
 }

pkg/driverutil/disk.go, important, regression

		}
	} else {
		format, err := nativeimgutil.DetectFormat(imagePath)
		if err != nil {
			return err
		}

		if format == string(diskImageFormat) {
			if err = os.Rename(imagePath, diskPath); err != nil {
				return err
			}
		} else {
			if err := diskUtil.Convert(ctx, diskImageFormat, imagePath, diskPath, &diskSizeInBytes, false); err != nil {
				return fmt.Errorf("failed to convert %q to %q: %w", imagePath, diskPath, err)
			}
			os.Remove(imagePath)
		}
	}

The old code always called diskUtil.Convert, which both converted and resized to diskSizeInBytes. The new rename path at line 105 skips the resize. This works today because prepareDisk at pkg/instance/start.go:135 handles resizing after CreateDisk. But EnsureDisk was previously self-contained; it now silently depends on its caller to resize, an implicit contract change.

Fix: Either add the resize call or document the dependency:

+		// Resize handled by prepareDisk() after CreateDisk().
 		if format == string(diskImageFormat) {
 			if err = os.Rename(imagePath, diskPath); err != nil {
 				return err
 			}