fix(oci): fail a publish at the first tag the registry rejects

internal/cli/collection_publish_tag_failure_test.go

@@ -0,0 +1,170 @@
+package cli
+
+import (
+	"crypto/sha256"
+	"encoding/hex"
+	"errors"
+	"fmt"
+	"io"
+	"net"
+	"net/http"
+	"net/http/httptest"
+	"path/filepath"
+	"slices"
+	"strings"
+	"sync"
+	"testing"
+
+	coreerrors "github.com/devcontainers/cli/internal/errors"
+	"github.com/devcontainers/cli/internal/log"
+	"github.com/devcontainers/cli/internal/oci"
+)
+
+type pushRegistry struct {
+	host    string
+	failTag string
+
+	mu        sync.Mutex
+	blobs     map[string][]byte
+	manifests map[string][]byte
+	attempts  map[string][]string
+}
+
+func newPushRegistry(t *testing.T, failTag string) *pushRegistry {
+	t.Helper()
+	r := &pushRegistry{failTag: failTag, blobs: map[string][]byte{}, manifests: map[string][]byte{}, attempts: map[string][]string{}}
+	srv := httptest.NewServer(http.HandlerFunc(r.serve))
+	t.Cleanup(srv.Close)
+	// A localhost registry is plain HTTP for both CLIs.
+	_, port, err := net.SplitHostPort(srv.Listener.Addr().String())
+	if err != nil {
+		t.Fatal(err)
+	}
+	r.host = "localhost:" + port
+	return r
+}
+
+func (r *pushRegistry) tagAttempts(repo string) []string {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+	return slices.Clone(r.attempts[repo])
+}
+
+func (r *pushRegistry) serve(w http.ResponseWriter, req *http.Request) {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+	path := strings.TrimPrefix(req.URL.Path, "/v2/")
+	notFound := func() {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusNotFound)
+		_, _ = w.Write([]byte(`{"errors":[{"code":"NAME_UNKNOWN","message":"not found"}]}`))
+	}
+	switch {
+	case path == "" || path == "/":
+		w.WriteHeader(http.StatusOK)
+	case strings.HasSuffix(path, "/tags/list"):
+		notFound()
+	case strings.Contains(path, "/blobs/uploads/") && req.Method == http.MethodPost:
+		w.Header().Set("Location", req.URL.Path+"session")
+		w.WriteHeader(http.StatusAccepted)
+	case strings.Contains(path, "/blobs/uploads/") && req.Method == http.MethodPut:
+		body, _ := io.ReadAll(req.Body)
+		digest := req.URL.Query().Get("digest")
+		r.blobs[digest] = body
+		w.Header().Set("Docker-Content-Digest", digest)
+		w.WriteHeader(http.StatusCreated)
+	case strings.Contains(path, "/blobs/"):
+		body, ok := r.blobs[path[strings.LastIndex(path, "/")+1:]]
+		if !ok {
+			notFound()
+			return
+		}
+		w.Header().Set("Content-Length", fmt.Sprint(len(body)))
+		if req.Method != http.MethodHead {
+			_, _ = w.Write(body)
+		}
+	case strings.Contains(path, "/manifests/"):
+		i := strings.Index(path, "/manifests/")
+		repo, ref := path[:i], path[i+len("/manifests/"):]
+		if req.Method != http.MethodPut {
+			body, ok := r.manifests[repo+":"+ref]
+			if !ok {
+				notFound()
+				return
+			}
+			w.Header().Set("Content-Type", "application/vnd.oci.image.manifest.v1+json")
+			w.Header().Set("Content-Length", fmt.Sprint(len(body)))
+			if req.Method != http.MethodHead {
+				_, _ = w.Write(body)
+			}
+			return
+		}
+		r.attempts[repo] = append(r.attempts[repo], ref)
+		if ref == r.failTag {
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusBadRequest)
+			_, _ = w.Write([]byte(`{"errors":[{"code":"TAG_INVALID","message":"tag rejected by test"}]}`))
+			return
+		}
+		body, _ := io.ReadAll(req.Body)
+		r.manifests[repo+":"+ref] = body
+		sum := sha256.Sum256(body)
+		w.Header().Set("Docker-Content-Digest", "sha256:"+hex.EncodeToString(sum[:]))
+		w.WriteHeader(http.StatusCreated)
+	default:
+		notFound()
+	}
+}
+
+func writeTagFailureCollection(t *testing.T, collectionType string) string {
+	t.Helper()
+	src := filepath.Join(t.TempDir(), "src")
+	if collectionType == "feature" {
+		writeSeamFeature(t, src, "good", "1.0.0")
+		return src
+	}
+	writeFile(t, filepath.Join(src, "good", "devcontainer-template.json"), `{"id":"good","version":"1.0.0","name":"Good"}`, 0o644)
+	writeFile(t, filepath.Join(src, "good", ".devcontainer.json"), `{"image":"alpine"}`, 0o644)
+	return src
+}
+
+func TestPublishCollection_FailedTagPushExits(t *testing.T) {
+	for _, collectionType := range []string{"feature", "template"} {
+		t.Run(collectionType, func(t *testing.T) {
+			reg := newPushRegistry(t, "1.0")
+			out := &captureOutput{}
+			logger := log.New(log.Options{Level: log.LevelInfo, Format: "text", Writer: out.Stderr()})
+			client := oci.NewClientWithAuthPolicy(logger, map[string]string{"DOCKER_CONFIG": t.TempDir()}, oci.DefaultAuthPolicy())
+
+			err := publishCollectionWith(t.Context(), out, client, writeTagFailureCollection(t, collectionType), reg.host, "me/coll", collectionType, "info")
+			if exit, ok := errors.AsType[*coreerrors.ExitCodeError](err); !ok || exit.Code != 1 {
+				t.Fatalf("err = %v, want exit code 1 after the rejected tag", err)
+			}
+			if out.out.Len() != 0 {
+				t.Errorf("stdout = %q, want no result after a failed tag push", out.out.String())
+			}
+
+			resource := reg.host + "/me/coll/good"
+			stderr := out.err.String()
+			for _, want := range []string{
+				"Failed to PUT manifest for tag 1.0\n",
+				fmt.Sprintf("(!) ERR: Failed to publish %s: '%s'", collectionType, resource),
+				fmt.Sprintf("(!) ERR: Failed to publish '%s'", resource),
+			} {
+				if !strings.Contains(stderr, want) {
+					t.Errorf("stderr is missing %q:\n%s", want, stderr)
+				}
+			}
+			if strings.Contains(stderr, "Published "+collectionType) {
+				t.Errorf("stderr reports a successful publish:\n%s", stderr)
+			}
+
+			if got, want := reg.tagAttempts("me/coll/good"), []string{"1", "1.0"}; !slices.Equal(got, want) {
+				t.Errorf("tag PUTs = %v, want %v: no tag is tried after the first failure", got, want)
+			}
+			if got := reg.tagAttempts("me/coll"); len(got) != 0 {
+				t.Errorf("collection metadata PUTs = %v, want none after a failed publish", got)
+			}
+		})
+	}
+}

internal/cli/publish_parity_test.go

@@ -1,11 +1,15 @@
 package cli
 
 import (
+	"bytes"
+	"errors"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"reflect"
+	"slices"
 	"sort"
+	"strings"
 	"testing"
 	"time"
 
@@ -25,6 +29,8 @@ func TestPublishParity(t *testing.T) {
 	if _, err := os.Stat(goCLI); err != nil {
 		t.Skip("Go CLI not built")
 	}
+	t.Run("tag failure", func(t *testing.T) { publishTagFailureParity(t, repoRoot, goCLI) })
+
 	if !isDockerAvailable() {
 		t.Skip("docker required")
 	}
@@ -115,3 +121,56 @@ func TestPublishParity(t *testing.T) {
 		})
 	}
 }
+
+func publishTagFailureParity(t *testing.T, repoRoot, goCLI string) {
+	for _, collectionType := range []string{"feature", "template"} {
+		t.Run(collectionType, func(t *testing.T) {
+			reg := newPushRegistry(t, "1.0")
+			src := writeTagFailureCollection(t, collectionType)
+			type side struct {
+				stdout   string
+				errLines []string
+				exit     int
+				attempts []string
+				metadata []string
+			}
+			publish := func(cli []string, namespace string) side {
+				args := append(slices.Clone(cli[1:]), collectionType+"s", "publish", src, "-r", reg.host, "-n", namespace)
+				cmd := exec.CommandContext(t.Context(), cli[0], args...)
+				cmd.Dir = repoRoot
+				cmd.Env = append(os.Environ(), "DOCKER_CONFIG="+t.TempDir())
+				var stdout, stderr bytes.Buffer
+				cmd.Stdout, cmd.Stderr = &stdout, &stderr
+				err := cmd.Run()
+				var exitErr *exec.ExitError
+				if err != nil && !errors.As(err, &exitErr) {
+					t.Fatalf("run %v: %v", cli, err)
+				}
+				var errLines []string
+				for line := range strings.Lines(strings.ReplaceAll(stderr.String(), namespace, "<ns>")) {
+					for _, marker := range []string{"(!) ERR:", "Failed to PUT manifest for tag"} {
+						if i := strings.Index(line, marker); i >= 0 {
+							errLines = append(errLines, strings.TrimSpace(line[i:]))
+						}
+					}
+				}
+				return side{
+					stdout:   stdout.String(),
+					errLines: errLines,
+					exit:     cmd.ProcessState.ExitCode(),
+					attempts: reg.tagAttempts(namespace + "/good"),
+					metadata: reg.tagAttempts(namespace),
+				}
+			}
+
+			ts := publish([]string{"node", filepath.Join(repoRoot, "reference", "devcontainer.js")}, "tsns/coll")
+			goSide := publish([]string{goCLI}, "gons/coll")
+			if !reflect.DeepEqual(ts, goSide) {
+				t.Errorf("publish with a rejected tag differs:\n--- TS\n%+v\n--- Go\n%+v", ts, goSide)
+			}
+			if ts.exit != 1 || !slices.Equal(ts.attempts, []string{"1", "1.0"}) {
+				t.Errorf("TS side = %+v: the fake registry did not exercise the failed tag", ts)
+			}
+		})
+	}
+}

internal/oci/push.go

@@ -79,23 +79,17 @@ func (c *Client) PushArtifact(ctx context.Context, ref *Ref, tgzPath string, tag
 		return nil, err
 	}
 
-	var publishedTags []string
 	for _, tag := range tags {
 		if err := repo.PushReference(ctx, manifestDesc, bytes.NewReader(manifestBytes), tag); err != nil {
-			c.log.Write(fmt.Sprintf("Failed to push tag %s: %v", tag, err), log.LevelWarning)
-			continue
+			c.log.Write(fmt.Sprintf("Failed to PUT manifest for tag %s\n%v", tag, err), log.LevelError)
+			return nil, fmt.Errorf("push tag %s of %s: %w", tag, ref.Resource, err)
 		}
-		publishedTags = append(publishedTags, tag)
 		c.log.Write(fmt.Sprintf("Published tag: %s", tag), log.LevelInfo)
 	}
 
-	if len(publishedTags) == 0 {
-		return nil, fmt.Errorf("failed to publish any tags")
-	}
-
 	return &PushResult{
 		Digest:        manifestDesc.Digest.String(),
-		PublishedTags: publishedTags,
+		PublishedTags: tags,
 	}, nil
 }
 

internal/oci/push_partial_test.go

@@ -1,8 +1,12 @@
 package oci
 
 import (
+	"bytes"
+	"maps"
 	"os"
 	"path/filepath"
+	"slices"
+	"strings"
 	"testing"
 
 	"github.com/devcontainers/cli/internal/log"
@@ -17,38 +21,45 @@ func writeTarball(t *testing.T, content string) string {
 	return p
 }
 
-func TestPushArtifact_BearerAuthAndPartialTagFailure(t *testing.T) {
+func TestPushArtifact_BearerAuthAndStopsAtFirstFailedTag(t *testing.T) {
 	const user, pass = "u", "p"
 	reg := newFakeOCIRegistry()
 	reg.requireBearer = true
 	reg.wantUser, reg.wantPass = user, pass
-	reg.failTags = map[string]bool{"latest": true}
+	reg.failTags = map[string]bool{"1.0": true}
 	registry := reg.start(t)
 
 	ref, err := ParseRef(registry + "/ns/hello:1.0.0")
 	if err != nil {
 		t.Fatal(err)
 	}
-	client := NewClient(log.Null, dockerConfigEnv(t, registry, user, pass))
+	var logs bytes.Buffer
+	client := NewClient(log.New(log.Options{Writer: &logs, Level: log.LevelInfo}), dockerConfigEnv(t, registry, user, pass))
 
-	res, err := client.PushArtifact(t.Context(), ref, writeTarball(t, "layer"), []string{"1.0.0", "latest"}, "feature", nil)
-	if err != nil {
-		t.Fatalf("push returned error, want partial success: %v", err)
+	res, err := client.PushArtifact(t.Context(), ref, writeTarball(t, "layer"), []string{"1", "1.0", "1.0.0", "latest"}, "feature", nil)
+	if err == nil {
+		t.Fatalf("push returned %+v, want an error for the rejected tag 1.0 (reference putManifestWithTags returns undefined)", res)
 	}
-
-	if got := res.PublishedTags; len(got) != 1 || got[0] != "1.0.0" {
-		t.Fatalf("PublishedTags = %v, want [1.0.0] (partial, no rollback)", got)
+	if res != nil {
+		t.Errorf("result = %+v, want nil alongside the error", res)
+	}
+	if !strings.Contains(err.Error(), "1.0") || !strings.Contains(err.Error(), ref.Resource) {
+		t.Errorf("error %q does not name the tag and resource", err)
 	}
+	if !strings.Contains(logs.String(), "Failed to PUT manifest for tag 1.0\n") {
+		t.Errorf("log does not report the rejected tag:\n%s", logs.String())
+	}
+
 	reg.mu.Lock()
-	_, has100 := reg.manifests["1.0.0"]
-	_, hasLatest := reg.manifests["latest"]
+	attempted := slices.Clone(reg.putManifestAttempt)
+	tagged := slices.Sorted(maps.Keys(reg.manifests))
 	tokenReqs := reg.tokenRequests
 	reg.mu.Unlock()
-	if !has100 {
-		t.Error("1.0.0 tag was not published")
+	if want := []string{"1", "1.0"}; !slices.Equal(attempted, want) {
+		t.Errorf("tag PUTs attempted = %v, want %v: tags after the first failure must not be tried", attempted, want)
 	}
-	if hasLatest {
-		t.Error("latest tag was published despite the injected failure")
+	if want := []string{"1"}; !slices.Equal(tagged, want) {
+		t.Errorf("tags in registry = %v, want %v (tags before the failure stay pushed)", tagged, want)
 	}
 	if tokenReqs == 0 {
 		t.Error("token endpoint never called; the Bearer auth loop did not run")

internal/oci/push_registry_test.go

@@ -27,10 +27,11 @@ type fakeOCIRegistry struct {
 	failTags         map[string]bool
 	failAllManifests bool
 
-	tokenRequests int
-	putManifestOK []string
-	blobUploads   int
-	blobHeadHits  int
+	tokenRequests      int
+	putManifestAttempt []string
+	putManifestOK      []string
+	blobUploads        int
+	blobHeadHits       int
 }
 
 func newFakeOCIRegistry() *fakeOCIRegistry {
@@ -159,6 +160,7 @@ func (r *fakeOCIRegistry) handleManifest(w http.ResponseWriter, req *http.Reques
 
 	switch req.Method {
 	case http.MethodPut:
+		r.putManifestAttempt = append(r.putManifestAttempt, ref)
 		if r.failAllManifests || r.failTags[ref] {
 			// 400 is a non-retryable client error, so oras returns immediately
 			// (a 5xx would trigger retry backoff and slow the test), while still