fix(oci,ci): bound registry ops, fail-closed on tag-list errors, unskip OCI tests

Addresses three findings:

1. OCI operations could block indefinitely. The oras HTTP client had no
   ResponseHeaderTimeout, and the non-Context wrappers used a deadline-less
   context.Background(), so a registry that accepts the connection but stops
   responding hung features/templates/outdated/publish until a signal. Set
   ResponseHeaderTimeout on the shared transport (bounds a connected-but-silent
   registry without capping large blob transfers) and give each non-Context
   wrapper a default per-operation deadline (60s metadata, 10m blob). Publish
   now threads the command context via GetPublishedTagsContext.

2. publish ignored failures when listing existing tags. GetPublishedTags's
   error was discarded, so an auth/network/5xx failure looked like a first
   publish and mobile aliases (latest, 1, 1.2) were recomputed off an empty
   list — risking clobbering. Only a 404 (repo not created yet) is now treated
   as empty; any other error aborts the item. Added oci.IsNotFound to classify.

3. Critical OCI tests ran in no normal CI lane. The unanchored
   -skip 'TestClient|...' also excluded TestClientAuthCacheReused and
   TestClientOperationsHonorCanceledContext, and test:integration only runs
   ./internal/httpx -run TestClient — so both fell out of unit/race/coverage
   AND integration. Anchored the pattern to
   '^TestClient$|^TestE2E|^TestParityMatrix$|^TestPublishParity$' so the httpx
   integration test stays skipped in the unit lane while the OCI tests run.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: aledbf

Taskfile.yml

@@ -42,7 +42,7 @@ tasks:
   test:unit:
     desc: Run hermetic unit tests
     cmds:
-      - go test ./cmd/... ./internal/... -skip 'TestClient|TestE2E|TestParityMatrix|TestPublishParity' -count=1 {{.CLI_ARGS}}
+      - go test ./cmd/... ./internal/... -skip '^TestClient$|^TestE2E|^TestParityMatrix$|^TestPublishParity$' -count=1 {{.CLI_ARGS}}
 
   test:integration:
     desc: Run local HTTP integration tests
@@ -54,7 +54,7 @@ tasks:
     env:
       CGO_ENABLED: "1"
     cmds:
-      - go test ./cmd/... ./internal/... -skip 'TestClient|TestE2E|TestParityMatrix|TestPublishParity' -race -count=1 {{.CLI_ARGS}}
+      - go test ./cmd/... ./internal/... -skip '^TestClient$|^TestE2E|^TestParityMatrix$|^TestPublishParity$' -race -count=1 {{.CLI_ARGS}}
 
   vuln:
     desc: Scan module and code paths for known vulnerabilities (govulncheck)
@@ -64,7 +64,7 @@ tasks:
   coverage:
     desc: Generate the hermetic unit coverage report and enforce the COVERAGE_MIN floor
     cmds:
-      - go test ./cmd/... ./internal/... -skip 'TestClient|TestE2E|TestParityMatrix|TestPublishParity' -coverprofile=coverage.out -count=1
+      - go test ./cmd/... ./internal/... -skip '^TestClient$|^TestE2E|^TestParityMatrix$|^TestPublishParity$' -coverprofile=coverage.out -count=1
       - go tool cover -func=coverage.out
       # Ratchet gate: fail if total statement coverage drops below COVERAGE_MIN.
       - cmd: |

internal/cli/collection_commands.go

@@ -475,8 +475,15 @@ func publishCollectionWith(ctx context.Context, out Output, reg oci.Registry, ta
 			failures++
 			return nil, false
 		}
-		// Empty on first publish (repository does not exist yet).
-		published, _ := ociClient.GetPublishedTags(ref)
+		// The repository is empty on first publish (404). Any OTHER error — auth,
+		// network, a 5xx — must abort: treating it as "no tags yet" would republish
+		// mobile aliases (latest, 1, 1.2) off an empty list and could clobber them.
+		published, tagsErr := ociClient.GetPublishedTagsContext(ctx, ref)
+		if tagsErr != nil && !oci.IsNotFound(tagsErr) {
+			logger.Write(fmt.Sprintf("(!) ERR: could not list existing tags for %q: %v", resource, tagsErr), log.LevelError)
+			failures++
+			return nil, false
+		}
 		tags, skip, tagErr := oci.SemanticTags(version, published)
 		if tagErr != nil {
 			logger.Write(fmt.Sprintf("(!) ERR: %v, skipping...", tagErr), log.LevelError)

internal/cli/collection_publish_partial_test.go

@@ -5,8 +5,63 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+
+	"oras.land/oras-go/v2/registry/remote/errcode"
 )
 
+// TestPublishCollection_AbortsOnTagListError covers the finding that publish must
+// NOT treat a failed tag lookup as an empty registry: a non-404 error (auth, 5xx,
+// network) has to abort the item — otherwise mobile aliases would be recomputed
+// off an empty list and could clobber existing ones.
+func TestPublishCollection_AbortsOnTagListError(t *testing.T) {
+	target := t.TempDir()
+	src := filepath.Join(target, "src")
+	writeSeamFeature(t, src, "good", "1.0.0")
+
+	reg := &fakeRegistry{
+		published: map[string][]string{},
+		failIDs:   map[string]bool{},
+		tagListErr: map[string]error{
+			"ghcr.io/me/features/good": &errcode.ErrorResponse{StatusCode: 500},
+		},
+	}
+	out := &captureOutput{}
+
+	err := publishCollectionWith(t.Context(), out, reg, target, "ghcr.io", "me/features", "feature", "info")
+	if err == nil || !strings.Contains(err.Error(), "publish operation(s) failed") {
+		t.Fatalf("err = %v, want a publish failure from the tag-list error", err)
+	}
+	// The artifact must NOT have been pushed — we aborted before computing tags.
+	if len(reg.pushed) != 0 {
+		t.Fatalf("artifact pushed despite a tag-list error: %v", reg.pushed)
+	}
+}
+
+// TestPublishCollection_TreatsNotFoundAsFirstPublish covers the complementary
+// case: a 404 from the tag listing means the repository does not exist yet, so
+// the item publishes normally as a first publish.
+func TestPublishCollection_TreatsNotFoundAsFirstPublish(t *testing.T) {
+	target := t.TempDir()
+	src := filepath.Join(target, "src")
+	writeSeamFeature(t, src, "good", "1.0.0")
+
+	reg := &fakeRegistry{
+		published: map[string][]string{},
+		failIDs:   map[string]bool{},
+		tagListErr: map[string]error{
+			"ghcr.io/me/features/good": &errcode.ErrorResponse{StatusCode: 404},
+		},
+	}
+	out := &captureOutput{}
+
+	if err := publishCollectionWith(t.Context(), out, reg, target, "ghcr.io", "me/features", "feature", "info"); err != nil {
+		t.Fatalf("404 tag listing should be a clean first publish, got: %v", err)
+	}
+	if len(reg.pushed) != 1 || !strings.HasSuffix(reg.pushed[0], "/good") {
+		t.Fatalf("pushed = %v, want [.../good]", reg.pushed)
+	}
+}
+
 // TestPublishCollection_CollectionMetadataFailureCounts complements the
 // item-level partial-failure test (seams_test.go): here every ITEM publishes
 // fine but the SEPARATE collection-metadata push fails. That failure must be

internal/cli/seams_test.go

@@ -29,6 +29,7 @@ func (c *captureOutput) Stderr() io.Writer { return &c.err }
 // publishCollectionWith without contacting a real registry.
 type fakeRegistry struct {
 	published      map[string][]string // resource -> already-published tags
+	tagListErr     map[string]error    // resource -> error to return from GetPublishedTags*
 	failIDs        map[string]bool
 	pushed         []string            // resources successfully pushed (for assertions)
 	pushedTags     map[string][]string // resource -> tags requested on push
@@ -45,6 +46,15 @@ func (f *fakeRegistry) FetchBlob(ref *oci.Ref, digest string) ([]byte, error) {
 }
 
 func (f *fakeRegistry) GetPublishedTags(ref *oci.Ref) ([]string, error) {
+	return f.GetPublishedTagsContext(context.Background(), ref)
+}
+
+func (f *fakeRegistry) GetPublishedTagsContext(_ context.Context, ref *oci.Ref) ([]string, error) {
+	if f.tagListErr != nil {
+		if err, ok := f.tagListErr[ref.Resource]; ok {
+			return nil, err
+		}
+	}
 	return f.published[ref.Resource], nil
 }
 

internal/oci/client.go

@@ -4,18 +4,39 @@ package oci
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"strings"
+	"time"
 
 	orascontent "oras.land/oras-go/v2/content"
+	"oras.land/oras-go/v2/errdef"
 	"oras.land/oras-go/v2/registry/remote/auth"
+	"oras.land/oras-go/v2/registry/remote/errcode"
 	"oras.land/oras-go/v2/registry/remote/retry"
 
 	"github.com/devcontainers/cli/internal/httpx"
 	"github.com/devcontainers/cli/internal/log"
 )
 
+// Operation timeouts. These are vars (not consts) so tests can shrink them.
+//
+//   - responseHeaderTimeout bounds a registry that accepts the TCP/TLS connection
+//     but then stops responding: the base transport (a clone of DefaultTransport)
+//     sets a dial and TLS-handshake timeout but NO ResponseHeaderTimeout, so
+//     without this a hung registry blocks the command forever.
+//   - metadataOpTimeout is the overall ceiling for small metadata operations
+//     (manifest/tag list) when the caller supplies no context of its own.
+//   - blobOpTimeout is a generous ceiling for blob downloads (which can be large);
+//     it also backstops a mid-stream stall, which responseHeaderTimeout — only
+//     covering time-to-first-header — does not catch.
+var (
+	responseHeaderTimeout = 30 * time.Second
+	metadataOpTimeout     = 60 * time.Second
+	blobOpTimeout         = 10 * time.Minute
+)
+
 // Client performs OCI registry operations (pull manifest, download blob, list
 // tags, push artifacts) on top of oras-go.
 type Client struct {
@@ -37,20 +58,44 @@ type Client struct {
 // NewClient creates an OCI client. Auth and retries are handled by oras-go (see
 // repository()); the HTTP transport is the shared proxy/CA-aware transport.
 func NewClient(logger log.Logger, env map[string]string) *Client {
+	base := httpx.NewTransport()
+	// Cut off a registry that connects but never sends response headers.
+	base.ResponseHeaderTimeout = responseHeaderTimeout
 	return &Client{
 		log:         logger,
 		env:         env,
 		authCache:   auth.NewCache(),
-		retryClient: &http.Client{Transport: retry.NewTransport(httpx.NewTransport())},
+		retryClient: &http.Client{Transport: retry.NewTransport(base)},
+	}
+}
+
+// IsNotFound reports whether err indicates the target repository or reference
+// does not exist (a 404 from the registry), as opposed to an auth, network, or
+// other registry failure. Publishing uses this so a not-yet-created repository
+// is treated as "no tags published", while a real error aborts the publish
+// instead of silently proceeding on an empty tag list.
+func IsNotFound(err error) bool {
+	if err == nil {
+		return false
+	}
+	if errors.Is(err, errdef.ErrNotFound) {
+		return true
+	}
+	var respErr *errcode.ErrorResponse
+	if errors.As(err, &respErr) {
+		return respErr.StatusCode == http.StatusNotFound
 	}
+	return false
 }
 
 // FetchManifest fetches and validates an OCI manifest using oras-go for
 // transport/auth (bearer-token scope negotiation, retries), keeping the
 // devcontainer-specific validation (config media type must be
 // application/vnd.devcontainers) and result shape.
 func (c *Client) FetchManifest(ref *Ref, expectedDigest string) (*ManifestContainer, error) {
-	return c.FetchManifestContext(context.Background(), ref, expectedDigest)
+	ctx, cancel := context.WithTimeout(context.Background(), metadataOpTimeout)
+	defer cancel()
+	return c.FetchManifestContext(ctx, ref, expectedDigest)
 }
 
 // FetchManifestContext is FetchManifest with caller-controlled cancellation.
@@ -108,7 +153,9 @@ func (c *Client) FetchManifestContext(ctx context.Context, ref *Ref, expectedDig
 
 // FetchBlob downloads a blob and verifies its digest (via oras-go).
 func (c *Client) FetchBlob(ref *Ref, digest string) ([]byte, error) {
-	return c.FetchBlobContext(context.Background(), ref, digest)
+	ctx, cancel := context.WithTimeout(context.Background(), blobOpTimeout)
+	defer cancel()
+	return c.FetchBlobContext(ctx, ref, digest)
 }
 
 // FetchBlobContext is FetchBlob with caller-controlled cancellation.
@@ -128,7 +175,9 @@ func (c *Client) FetchBlobContext(ctx context.Context, ref *Ref, digest string)
 
 // GetPublishedTags lists all tags for a resource (via oras-go).
 func (c *Client) GetPublishedTags(ref *Ref) ([]string, error) {
-	return c.GetPublishedTagsContext(context.Background(), ref)
+	ctx, cancel := context.WithTimeout(context.Background(), metadataOpTimeout)
+	defer cancel()
+	return c.GetPublishedTagsContext(ctx, ref)
 }
 
 // GetPublishedTagsContext is GetPublishedTags with caller-controlled cancellation.

internal/oci/client_timeout_test.go

@@ -0,0 +1,79 @@
+package oci
+
+import (
+	"errors"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+
+	"oras.land/oras-go/v2/errdef"
+	"oras.land/oras-go/v2/registry/remote/errcode"
+
+	"github.com/devcontainers/cli/internal/log"
+)
+
+func TestIsNotFound(t *testing.T) {
+	cases := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{"nil", nil, false},
+		{"errdef.ErrNotFound", errdef.ErrNotFound, true},
+		{"wrapped ErrNotFound", fmt.Errorf("listing: %w", errdef.ErrNotFound), true},
+		{"404 ErrorResponse", &errcode.ErrorResponse{StatusCode: http.StatusNotFound}, true},
+		{"wrapped 404", fmt.Errorf("tags: %w", &errcode.ErrorResponse{StatusCode: 404}), true},
+		{"401 ErrorResponse", &errcode.ErrorResponse{StatusCode: http.StatusUnauthorized}, false},
+		{"500 ErrorResponse", &errcode.ErrorResponse{StatusCode: http.StatusInternalServerError}, false},
+		{"plain error", errors.New("connection refused"), false},
+	}
+	for _, c := range cases {
+		if got := IsNotFound(c.err); got != c.want {
+			t.Errorf("%s: IsNotFound = %v, want %v", c.name, got, c.want)
+		}
+	}
+}
+
+// TestGetPublishedTags_HangingRegistryTimesOut verifies the fix for the
+// indefinite-block finding: a registry that accepts the connection but never
+// sends response headers must not hang the operation forever — the default
+// per-operation deadline (shrunk here) cuts it off.
+func TestGetPublishedTags_HangingRegistryTimesOut(t *testing.T) {
+	// A server that accepts the request and then never responds.
+	blocked := make(chan struct{})
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		<-blocked // hang until the test ends
+	}))
+	defer srv.Close()
+	defer close(blocked)
+
+	// Shrink the metadata deadline so the test is fast.
+	orig := metadataOpTimeout
+	metadataOpTimeout = 200 * time.Millisecond
+	defer func() { metadataOpTimeout = orig }()
+
+	c := NewClient(log.New(log.Options{Level: log.LevelError}), nil)
+	ref := &Ref{Registry: strings.TrimPrefix(srv.URL, "http://"), Path: "me/feat", Resource: "me/feat"}
+
+	done := make(chan error, 1)
+	start := time.Now()
+	go func() {
+		_, err := c.GetPublishedTags(ref)
+		done <- err
+	}()
+
+	select {
+	case err := <-done:
+		if err == nil {
+			t.Fatal("expected a timeout error, got nil")
+		}
+		if elapsed := time.Since(start); elapsed > 5*time.Second {
+			t.Errorf("operation took %s, expected it to time out promptly", elapsed)
+		}
+	case <-time.After(5 * time.Second):
+		t.Fatal("GetPublishedTags did not honor the deadline — it blocked indefinitely")
+	}
+}

internal/oci/registry.go

@@ -8,12 +8,14 @@ import "context"
 // tests without standing up a real registry. *Client satisfies it.
 //
 // The fetch/list methods keep their non-Context convenience variants (they are
-// short reads); the push methods take a ctx so a long upload participates in the
-// CLI's signal-cancellation.
+// short reads, and each now applies a default per-operation deadline); a Context
+// variant is exposed where a caller already holds the command context (e.g.
+// publish) so the operation participates in the CLI's signal-cancellation.
 type Registry interface {
 	FetchManifest(ref *Ref, expectedDigest string) (*ManifestContainer, error)
 	FetchBlob(ref *Ref, digest string) ([]byte, error)
 	GetPublishedTags(ref *Ref) ([]string, error)
+	GetPublishedTagsContext(ctx context.Context, ref *Ref) ([]string, error)
 	PushArtifact(ctx context.Context, ref *Ref, tgzPath string, tags []string, collectionType string, annotations map[string]string) (*PushResult, error)
 	PushCollectionMetadata(ctx context.Context, ref *Ref, collectionJSONPath string) (*PushResult, error)
 }

internal/templates/apply_fetch_test.go

@@ -87,6 +87,10 @@ func (f *fakeTemplateRegistry) GetPublishedTags(ref *oci.Ref) ([]string, error)
 	return nil, fmt.Errorf("GetPublishedTags not used")
 }
 
+func (f *fakeTemplateRegistry) GetPublishedTagsContext(_ context.Context, ref *oci.Ref) ([]string, error) {
+	return nil, fmt.Errorf("GetPublishedTagsContext not used")
+}
+
 func (f *fakeTemplateRegistry) PushArtifact(ctx context.Context, ref *oci.Ref, tgzPath string, tags []string, collectionType string, annotations map[string]string) (*oci.PushResult, error) {
 	return nil, fmt.Errorf("PushArtifact not used")
 }