diff --git a/candy/plugin-build/buildkit.go b/candy/plugin-build/buildkit.go index fe5a48c..b564c2a 100644 --- a/candy/plugin-build/buildkit.go +++ b/candy/plugin-build/buildkit.go @@ -155,7 +155,7 @@ func (c driveConfig) buildImage(ctx context.Context, box spec.BuildResolveBox, c if err := annotateExport(dest, box.FullTag, created); err != nil { return fmt.Errorf("naming the %s export: %w", box.Name, err) } - if _, err := storeAdd(dest, box.FullTag); err != nil { + if err := storeAdd(dest, box.FullTag); err != nil { return fmt.Errorf("storing %s in the charly image store: %w", box.Name, err) } if err := importExport(ctx, runner, c.RunEngine, dest, box.FullTag); err != nil { diff --git a/candy/plugin-build/imagestore.go b/candy/plugin-build/imagestore.go index fa1e232..c39db80 100644 --- a/candy/plugin-build/imagestore.go +++ b/candy/plugin-build/imagestore.go @@ -18,6 +18,7 @@ package build import ( "archive/tar" + "bytes" "encoding/json" "errors" "fmt" @@ -115,17 +116,23 @@ func ensureLayout(dir string) error { return writeFileAtomic(marker, []byte(imageStoreLayoutMarker)) } -// writeFileAtomic publishes data at dest through a UNIQUE temp file in dest's own directory, then -// renames. Concurrent writers must never share a temp name, and a reader must never see a partial -// index. +// writeFileAtomic publishes data at dest through a unique temp file in dest's own directory. func writeFileAtomic(dest string, data []byte) error { + return copyFileAtomic(dest, bytes.NewReader(data)) +} + +// copyFileAtomic publishes a STREAM at dest through a unique temp file in dest's own directory, +// then renames. Concurrent writers must never share a temp name, and a reader must never see a +// partial index. The stream shape matters: a blob is copied layer by layer, and buffering a whole +// layer in memory is not something an image build can afford. +func copyFileAtomic(dest string, src io.Reader) error { f, err := os.CreateTemp(filepath.Dir(dest), "."+filepath.Base(dest)+".*") if err != nil { return err } name := f.Name() defer func() { _ = os.Remove(name) }() // no-op once the rename has consumed it - if _, err := f.Write(data); err != nil { + if _, err := io.Copy(f, src); err != nil { _ = f.Close() return err } @@ -325,43 +332,43 @@ func exportIndex(tarPath string) (*storeIndex, error) { // The blobs are content-addressed, so an existing blob is the same bytes and is left alone; the // index is the only mutable file and it is rewritten atomically under a cross-process lock (two // levels of a build write the store concurrently — see drive.go's errgroup). -func storeAdd(tarPath, ref string) (string, error) { +func storeAdd(tarPath, ref string) error { dir, err := imageStoreDir() if err != nil { - return "", err + return err } if err := ensureLayout(dir); err != nil { - return "", err + return err } exported, err := exportIndex(tarPath) if err != nil { - return "", fmt.Errorf("read the OCI export index: %w", err) + return fmt.Errorf("read the OCI export index: %w", err) } if len(exported.Manifests) == 0 { - return "", fmt.Errorf("the OCI export %s carries no image manifest", tarPath) + return fmt.Errorf("the OCI export %s carries no image manifest", tarPath) } if exported.Manifests[0].Annotations[imageNameAnnotation] == "" { // A store entry without the naming annotation is not addressable, which is the whole // contract of this file — refuse rather than write a dangling entry. - return "", fmt.Errorf("the OCI export %s carries no %s annotation", tarPath, imageNameAnnotation) + return fmt.Errorf("the OCI export %s carries no %s annotation", tarPath, imageNameAnnotation) } if err := copyBlobsIntoStore(tarPath, dir); err != nil { - return "", err + return err } lockPath, err := storeLockPath() if err != nil { - return "", err + return err } release, err := lock.AcquireFileLock(lockPath, true) if err != nil { - return "", fmt.Errorf("lock the charly image store index: %w", err) + return fmt.Errorf("lock the charly image store index: %w", err) } defer func() { _ = release() }() // the release error carries nothing a caller can act on idx, err := readStoreIndex(dir) if err != nil { - return "", err + return err } kept := idx.Manifests[:0] for _, m := range idx.Manifests { @@ -373,12 +380,12 @@ func storeAdd(tarPath, ref string) (string, error) { idx.Manifests = append(kept, exported.Manifests...) data, err := json.Marshal(idx) if err != nil { - return "", fmt.Errorf("render the charly image store index: %w", err) + return fmt.Errorf("render the charly image store index: %w", err) } if err := writeFileAtomic(storeIndexPath(dir), data); err != nil { - return "", fmt.Errorf("write the charly image store index: %w", err) + return fmt.Errorf("write the charly image store index: %w", err) } - return exported.Manifests[0].Digest, nil + return nil } // copyBlobsIntoStore copies every `blobs/` entry of the archive into the layout, skipping blobs @@ -414,11 +421,8 @@ func copyBlobsIntoStore(tarPath, dir string) error { if merr := os.MkdirAll(filepath.Dir(dest), 0o755); merr != nil { return merr } - data, rerr := io.ReadAll(tr) - if rerr != nil { - return fmt.Errorf("read blob %s from %s: %w", name, tarPath, rerr) - } - if werr := writeFileAtomic(dest, data); werr != nil { + // STREAM the blob: a layer can be gigabytes, and the store must not hold one in memory. + if werr := copyFileAtomic(dest, tr); werr != nil { return fmt.Errorf("write blob %s: %w", name, werr) } } diff --git a/candy/plugin-build/imagestore_live_test.go b/candy/plugin-build/imagestore_live_test.go index 53282b4..264f777 100644 --- a/candy/plugin-build/imagestore_live_test.go +++ b/candy/plugin-build/imagestore_live_test.go @@ -17,6 +17,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" "github.com/opencharly/sdk/buildkitdrive" @@ -123,10 +124,17 @@ func TestImageStore_AnnotationNamesTheImport_AndItsAbsenceDoesNot(t *testing.T) }); err != nil { t.Fatalf("stripping the control's naming annotation: %v", err) } - if err := importExport(ctx, runner, "podman", controlTar, control); err == nil { + // Assert WHERE the control fails: the load must SUCCEED (the archive is valid) and the + // reference must fail to RESOLVE. An error from the load would prove nothing about naming. + cerr := importExport(ctx, runner, "podman", controlTar, control) + if cerr == nil { t.Fatalf("an archive with NO %s annotation imported and reported %s resolvable — the "+ "annotation would not be what names the image", imageNameAnnotation, control) } + if !strings.Contains(cerr.Error(), "not resolvable") { + t.Fatalf("the control failed BEFORE the reference check (%v) — the load, not the naming, "+ + "is what failed, so this proves nothing", cerr) + } if kit.LocalImageExists("podman", control) { t.Fatalf("the unnamed control archive landed tagged as %s", control) } diff --git a/candy/plugin-build/imagestore_test.go b/candy/plugin-build/imagestore_test.go index b2d65b4..ce52c08 100644 --- a/candy/plugin-build/imagestore_test.go +++ b/candy/plugin-build/imagestore_test.go @@ -183,7 +183,7 @@ func TestStoreAddCopiesBlobsAndKeepsOneEntryPerReference(t *testing.T) { if err := annotateExport(first, ref, time.Now()); err != nil { t.Fatal(err) } - if _, err := storeAdd(first, ref); err != nil { + if err := storeAdd(first, ref); err != nil { t.Fatalf("storeAdd first: %v", err) } if digest, ok, err := storeLookup(ref); err != nil || !ok || digest != firstDigest { @@ -194,7 +194,7 @@ func TestStoreAddCopiesBlobsAndKeepsOneEntryPerReference(t *testing.T) { if err := annotateExport(second, ref, time.Now()); err != nil { t.Fatal(err) } - if _, err := storeAdd(second, ref); err != nil { + if err := storeAdd(second, ref); err != nil { t.Fatalf("storeAdd second: %v", err) } idx, err := readStoreIndex(dir) @@ -267,7 +267,7 @@ func TestStoreLookupIgnoresTheTagOnlyAnnotation(t *testing.T) { func TestStoreAddRefusesAnUnnamedExport(t *testing.T) { storeTempDir(t) path, _ := writeExportFixture(t, "unnamed") - if _, err := storeAdd(path, "ghcr.io/opencharly/fedora:2026.296.1200"); err == nil { + if err := storeAdd(path, "ghcr.io/opencharly/fedora:2026.296.1200"); err == nil { t.Fatal("storeAdd accepted an export with no io.containerd.image.name annotation") } }