appview: give every Repository() error an OCI code instead of 500 {}

A user with an unresolvable defaultHold (did:web:localhost%3A8080) got HTTP
500 with a body of literally {} on every request in their namespace, and
crane retried it three times because 500 is retryable.

The cause is in the distribution library. handlers/app.go:755 switches on the
error type returned by Repository() with cases for ErrRepositoryUnknown,
ErrRepositoryNameInvalid and errcode.Error, and no default. A bare fmt.Errorf
matches none of them, so context.Errors stays empty; ServeJSON then finds no
ErrorCoder, leaves sc == 0, falls through to 500, and Errors.MarshalJSON
renders the nil slice as {} via omitempty.

So every error leaving Repository() must be coded. Four were not:

  hold URL unresolvable  -> 404 NAME_UNKNOWN when errors.Is
                            atproto.ErrHoldDIDPermanent, else 503 UNAVAILABLE
  no hold DID configured -> 500 UNKNOWN, but with a body and a log line
  invalid image name     -> 400 NAME_INVALID
  name missing an owner  -> 400 NAME_INVALID

The permanent/transient split is the point: a DNS blip must stay retryable,
but a did:web that can never resolve must not be retried at all. Stored user
data that cannot resolve is a 4xx condition, not a server fault, and the
NAME_UNKNOWN message now names the hold so the owner can fix their profile.
The hold DID is already world-readable in their sailor profile record.

Tests assert through a helper that replays distribution's exact type switch,
so an uncoded error still surfaces as 500 {} and the assertions bind to the
real behaviour rather than to the constructors.

Does not address the logrus line at app.go:757, which fires unconditionally
before the type switch and cannot be avoided by any returned error type.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9

pkg/appview/middleware/registry.go

@@ -2,6 +2,7 @@ package middleware
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"log/slog"
 	"net/http"
@@ -294,6 +295,55 @@ func (nr *NamespaceResolver) authErrorMessage(message string) error {
 	return errcode.ErrorCodeUnauthorized.WithMessage(fullMessage)
 }
 
+// noHoldConfiguredError reports that the appview itself has no hold to route
+// to. Unlike the other failures in Repository(), this one really is our fault,
+// so it keeps a 5xx, but it still has to be coded: an uncoded error is dropped
+// by distribution's type switch and served as `500 {}`, which tells the
+// operator reading the client's output nothing at all.
+func noHoldConfiguredError() errcode.Error {
+	return errcode.ErrorCodeUnknown.WithMessage(
+		"registry is misconfigured: no hold service is available, set default_hold_did in the appview middleware config")
+}
+
+// holdResolutionError classifies a hold-URL resolution failure into an OCI
+// error code. Everything returned from Repository() must be an errcode.Error:
+// distribution's dispatcher (registry/handlers/app.go) type-switches on the
+// error and has no default branch, so an uncoded error leaves the error list
+// empty and errcode.ServeJSON emits a bodyless `500 {}` that clients retry.
+//
+// The split matters as much as the coding. A DNS blip or a PLC outage is
+// genuinely transient, so it stays a retryable 503 UNAVAILABLE. A hold DID
+// that can never resolve (a malformed identifier, or one naming a host the
+// identity directory rejects outright, such as did:web:localhost%3A8080 left
+// in a sailor profile) is stored user data that no retry can fix, so it
+// terminates the client with 404 NAME_UNKNOWN instead of sending it round the
+// retry loop three more times.
+//
+// The message names the offending hold DID. That is not a disclosure: the
+// value lives in the owner's world-readable sailor profile record, and without
+// it the user has no way to know which setting to fix.
+func holdResolutionError(holdDID string, err error) error {
+	if errors.Is(err, atproto.ErrHoldDIDPermanent) {
+		slog.Debug("Hold DID is permanently unresolvable",
+			"component", "registry/middleware", "holdDID", holdDID, "error", err)
+		return errcode.Error{
+			Code: v2.ErrorCodeNameUnknown,
+			Message: fmt.Sprintf(
+				"repository name not known to registry: the storage hold configured for this repository, %s, cannot be resolved. The owner should update defaultHold in their sailor profile",
+				holdDID),
+		}
+	}
+
+	slog.Warn("Hold DID resolution failed",
+		"component", "registry/middleware", "holdDID", holdDID, "error", err)
+	return errcode.Error{
+		Code: errcode.ErrorCodeUnavailable,
+		Message: fmt.Sprintf(
+			"could not resolve the storage hold %s for this repository, please retry",
+			holdDID),
+	}
+}
+
 // Repository resolves the repository name and delegates to underlying namespace
 // Handles names like:
 // - atcr.io/alice/myimage → resolve alice to DID
@@ -304,8 +354,14 @@ func (nr *NamespaceResolver) Repository(ctx context.Context, name reference.Name
 	parts := strings.SplitN(repoPath, "/", 2)
 
 	if len(parts) < 2 {
-		// No user specified, use default or return error
-		return nil, fmt.Errorf("repository name must include user: %s", repoPath)
+		// Must be a coded error: distribution's type switch in handlers/app.go
+		// drops anything that is not an errcode.Error and serves a bodyless
+		// 500, which clients treat as retryable. A name with no owner
+		// component is malformed input, so NAME_INVALID is the right answer.
+		return nil, errcode.Error{
+			Code:    v2.ErrorCodeNameInvalid,
+			Message: fmt.Sprintf("repository name must include an owner: %s", repoPath),
+		}
 	}
 
 	identityStr := parts[0]
@@ -352,8 +408,13 @@ func (nr *NamespaceResolver) Repository(ctx context.Context, name reference.Name
 	// Also returns the sailor profile so we can read preferences (e.g. AutoRemoveUntagged)
 	holdDID, sailorProfile := nr.findHoldDIDAndProfile(ctx, did, pdsEndpoint)
 	if holdDID == "" {
-		// This is a fatal configuration error - registry cannot function without a hold service
-		return nil, fmt.Errorf("no hold DID configured: ensure default_hold_did is set in middleware config")
+		// A fatal configuration error: the registry cannot function without a
+		// hold service, so a 5xx is honest here. It still has to be a coded
+		// error, or distribution serves it as an empty {} body with no clue
+		// for the operator reading the client's output.
+		slog.Error("No hold DID configured",
+			"component", "registry/middleware", "ownerDID", did)
+		return nil, noHoldConfiguredError()
 	}
 
 	// Single-hop hold migration: check if this hold has declared a successor
@@ -362,7 +423,7 @@ func (nr *NamespaceResolver) Repository(ctx context.Context, name reference.Name
 	// Resolve hold DID to HTTP URL via identity directory (cached 24h)
 	holdURL, err := atproto.ResolveHoldURL(ctx, holdDID)
 	if err != nil {
-		return nil, fmt.Errorf("failed to resolve hold URL for %s: %w", holdDID, err)
+		return nil, holdResolutionError(holdDID, err)
 	}
 
 	// Crew reconciliation moved to the auth-phase push gate
@@ -477,7 +538,10 @@ func (nr *NamespaceResolver) Repository(ctx context.Context, name reference.Name
 	canonicalName := fmt.Sprintf("%s/%s", handle, imageName)
 	ref, err := reference.ParseNamed(canonicalName)
 	if err != nil {
-		return nil, fmt.Errorf("invalid image name %s: %w", imageName, err)
+		return nil, errcode.Error{
+			Code:    v2.ErrorCodeNameInvalid,
+			Message: fmt.Sprintf("invalid image name: %s", imageName),
+		}
 	}
 
 	// Delegate to underlying namespace with modified name

pkg/appview/middleware/registry_test.go

@@ -3,12 +3,15 @@ package middleware
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 
 	"github.com/distribution/distribution/v3"
+	"github.com/distribution/distribution/v3/registry/api/errcode"
 	"github.com/distribution/reference"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -332,3 +335,154 @@ func TestNamespaceResolver_BlobStatter(t *testing.T) {
 	statter := resolver.BlobStatter()
 	assert.Nil(t, statter, "mockNamespace returns nil")
 }
+
+// serveAsDistributionWould replays distribution's dispatcher behaviour
+// (registry/handlers/app.go, the type switch after app.registry.Repository())
+// and reports the status code and JSON body a client would actually see.
+//
+// The switch has no default branch, so an error that is not an errcode.Error
+// leaves the error list empty and errcode.ServeJSON falls through to a
+// bodyless `500 {}`. Asserting through this helper is what proves each path
+// produces a real response rather than that empty envelope.
+func serveAsDistributionWould(t *testing.T, err error) (int, string) {
+	t.Helper()
+
+	var errs errcode.Errors
+	switch e := err.(type) {
+	case distribution.ErrRepositoryUnknown:
+		errs = append(errs, errcode.ErrorCodeNameUnknown.WithDetail(e))
+	case distribution.ErrRepositoryNameInvalid:
+		errs = append(errs, errcode.ErrorCodeNameInvalid.WithDetail(e))
+	case errcode.Error:
+		errs = append(errs, e)
+	}
+
+	rec := httptest.NewRecorder()
+	require.NoError(t, errcode.ServeJSON(rec, errs))
+	return rec.Code, strings.TrimSpace(rec.Body.String())
+}
+
+// TestHoldResolutionError_PermanentVsTransient is the core of the fix: a hold
+// DID that can never resolve must terminate the client, while a transient
+// failure must stay retryable.
+func TestHoldResolutionError_PermanentVsTransient(t *testing.T) {
+	const holdDID = "did:web:localhost%3A8080"
+
+	t.Run("permanent", func(t *testing.T) {
+		wrapped := fmt.Errorf("failed to resolve hold DID %s: %w: %w",
+			holdDID, atproto.ErrHoldDIDPermanent, errors.New("directory rejected host"))
+
+		err := holdResolutionError(holdDID, wrapped)
+
+		var coded errcode.Error
+		require.True(t, errors.As(err, &coded), "must be an errcode.Error")
+		assert.Equal(t, errcode.ErrorCodeNameUnknown, coded.Code)
+
+		status, body := serveAsDistributionWould(t, err)
+		assert.Equal(t, http.StatusNotFound, status, "permanent hold failure must not be retryable")
+		assert.Contains(t, body, "NAME_UNKNOWN")
+		assert.Contains(t, body, holdDID, "message must name the unresolvable hold DID")
+		assert.NotEqual(t, "{}", body, "must not be the bodyless envelope")
+	})
+
+	t.Run("transient", func(t *testing.T) {
+		wrapped := fmt.Errorf("failed to resolve hold DID %s: %w", holdDID, errors.New("dial tcp: i/o timeout"))
+
+		err := holdResolutionError(holdDID, wrapped)
+
+		var coded errcode.Error
+		require.True(t, errors.As(err, &coded), "must be an errcode.Error")
+		assert.Equal(t, errcode.ErrorCodeUnavailable, coded.Code)
+
+		status, body := serveAsDistributionWould(t, err)
+		assert.Equal(t, http.StatusServiceUnavailable, status, "a DNS or PLC blip must stay retryable")
+		assert.Contains(t, body, "UNAVAILABLE")
+		assert.Contains(t, body, holdDID)
+	})
+}
+
+// TestHoldResolutionError_FromRealResolver verifies the classification against
+// errors that atproto.ResolveHoldURL actually produces, not hand-built ones,
+// so the permanent/transient split can't silently drift from the resolver.
+func TestHoldResolutionError_FromRealResolver(t *testing.T) {
+	// testMode short-circuits did:web resolution to a derived URL, which would
+	// hide the permanent case entirely.
+	prevTestMode := atproto.IsTestMode()
+	atproto.SetTestMode(false)
+	t.Cleanup(func() { atproto.SetTestMode(prevTestMode) })
+
+	t.Run("stale defaultHold is permanent", func(t *testing.T) {
+		const holdDID = "did:web:localhost%3A8080"
+
+		_, err := atproto.ResolveHoldURL(context.Background(), holdDID)
+		require.Error(t, err)
+		require.ErrorIs(t, err, atproto.ErrHoldDIDPermanent,
+			"a did:web naming a port-qualified localhost can never resolve")
+
+		status, _ := serveAsDistributionWould(t, holdResolutionError(holdDID, err))
+		assert.Equal(t, http.StatusNotFound, status)
+	})
+
+	t.Run("interrupted lookup is transient", func(t *testing.T) {
+		const holdDID = "did:plc:aaaaaaaaaaaaaaaaaaaaaaaa"
+
+		ctx, cancel := context.WithCancel(context.Background())
+		cancel() // the lookup fails the way a network fault would, without a network
+
+		_, err := atproto.ResolveHoldURL(ctx, holdDID)
+		require.Error(t, err)
+		require.NotErrorIs(t, err, atproto.ErrHoldDIDPermanent,
+			"an interrupted lookup says nothing about the DID itself")
+
+		status, _ := serveAsDistributionWould(t, holdResolutionError(holdDID, err))
+		assert.Equal(t, http.StatusServiceUnavailable, status)
+	})
+}
+
+// TestRepository_MissingOwnerComponent covers the name-without-owner path.
+func TestRepository_MissingOwnerComponent(t *testing.T) {
+	resolver := &NamespaceResolver{Namespace: &mockNamespace{}}
+
+	ref, err := reference.WithName("myimage")
+	require.NoError(t, err)
+
+	_, err = resolver.Repository(context.Background(), ref)
+	require.Error(t, err)
+
+	var coded errcode.Error
+	require.True(t, errors.As(err, &coded))
+	assert.Equal(t, errcode.ErrorCodeNameInvalid, coded.Code)
+
+	status, body := serveAsDistributionWould(t, err)
+	assert.Equal(t, http.StatusBadRequest, status)
+	assert.Contains(t, body, "NAME_INVALID")
+	assert.Contains(t, body, "myimage")
+}
+
+// TestRepository_UnresolvableIdentity pins the existing identity precedent that
+// the hold-DID handling is modelled on.
+func TestRepository_UnresolvableIdentity(t *testing.T) {
+	resolver := &NamespaceResolver{Namespace: &mockNamespace{}}
+
+	ref, err := reference.WithName("definitely-not-a-real-handle.example/foo")
+	require.NoError(t, err)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel() // resolution fails without depending on the network
+
+	_, err = resolver.Repository(ctx, ref)
+	require.Error(t, err)
+
+	status, body := serveAsDistributionWould(t, err)
+	assert.Equal(t, http.StatusNotFound, status)
+	assert.Contains(t, body, "NAME_UNKNOWN")
+}
+
+// TestRepository_NoHoldConfigured checks the misconfiguration path still 5xxes
+// but carries a body an operator can act on.
+func TestRepository_NoHoldConfigured(t *testing.T) {
+	status, body := serveAsDistributionWould(t, noHoldConfiguredError())
+	assert.Equal(t, http.StatusInternalServerError, status)
+	assert.Contains(t, body, "default_hold_did")
+	assert.NotEqual(t, "{}", body, "the whole point is that a 500 stops being bodyless")
+}