From 163189800595d9001c6115fc196c7362f8a8d8b3 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Wed, 2 Sep 2026 21:02:40 -0500 Subject: [PATCH] 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) Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9 --- pkg/appview/middleware/registry.go | 76 +++++++++++- pkg/appview/middleware/registry_test.go | 154 ++++++++++++++++++++++++ 2 files changed, 224 insertions(+), 6 deletions(-) diff --git a/pkg/appview/middleware/registry.go b/pkg/appview/middleware/registry.go index 6d13f07..2833b06 100644 --- a/pkg/appview/middleware/registry.go +++ b/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 diff --git a/pkg/appview/middleware/registry_test.go b/pkg/appview/middleware/registry_test.go index 80d9bf9..83304ab 100644 --- a/pkg/appview/middleware/registry_test.go +++ b/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") +}