hold: report blob size on read presigns so the appview can skip its S3 HEAD

distribution calls Stat before every blob GET and HEAD. The appview's Stat asked the hold for a presigned HEAD URL and then HEADed S3 with it purely to read Content-Length for the descriptor: two round trips to learn one number. The hold's getBlob response for OCI digests on GET and HEAD now carries "size". It comes from the records index when a layer record exists (a SQLite lookup on a new digest index, no network) and from a HeadObject otherwise, which is where config blobs land. If storage says the object does not exist the hold answers 404 instead of signing a URL that can only fail. The PUT and ATProto CID paths are untouched. The appview builds the descriptor from the reported size and makes no S3 request. When the field is absent it HEADs the presigned URL as before, so a new appview works against a hold that has not been upgraded, and an old appview ignores the extra field. A hold 404 maps to ErrBlobUnknown. Tests prove the index answered by leaving the mock bucket empty and counting zero HeadObject calls, prove the fallback with exactly one, and count requests reaching the fake S3 origin on the appview side rather than trusting the returned size. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Yf1ZVA7sXYhQNb9tCo1m5
2026-09-29 21:45:33 +00:00 · 2026-09-09 09:31:16 -05:00
parent 61a934debb
commit 034ea5988b
10 changed files with 729 additions and 38 deletions
@@ -143,7 +143,7 @@ func (p *ProxyBlobStore) Stat(ctx context.Context, dgst digest.Digest) (distribu

 	method := "HEAD"

-	url, err := p.getPresignedURL(ctx, method, dgst)
+	blob, err := p.getPresignedURL(ctx, method, dgst)
 	if err != nil {
 		// Preserve an authorization verdict. distribution calls Stat before
 		// ServeBlob on both GET and HEAD, so flattening everything to
@@ -157,8 +157,22 @@ func (p *ProxyBlobStore) Stat(ctx context.Context, dgst digest.Digest) (distribu
 		return distribution.Descriptor{}, distribution.ErrBlobUnknown
 	}

-	// Make HEAD request to presigned URL
-	req, err := http.NewRequestWithContext(ctx, method, url, nil)
+	if blob.Size != nil {
+		// The hold reported the size, so the descriptor is complete and the
+		// blob's bytes never have to be touched. This is the whole point of the
+		// field: Stat is called before every GET and HEAD of a blob, and the
+		// round trip below was buying nothing but Content-Length.
+		return distribution.Descriptor{
+			Digest:    dgst,
+			Size:      *blob.Size,
+			MediaType: "application/octet-stream",
+		}, nil
+	}
+
+	// No size in the response: the hold predates the field. Fall back to the
+	// original behaviour and read Content-Length off the presigned URL, so a
+	// new AppView keeps working against a hold that has not been upgraded.
+	req, err := http.NewRequestWithContext(ctx, method, blob.URL, nil)
 	if err != nil {
 		return distribution.Descriptor{}, distribution.ErrBlobUnknown
 	}
@@ -198,13 +212,13 @@ func (p *ProxyBlobStore) Get(ctx context.Context, dgst digest.Digest) ([]byte, e

 	method := "GET"

-	url, err := p.getPresignedURL(ctx, method, dgst)
+	blob, err := p.getPresignedURL(ctx, method, dgst)
 	if err != nil {
 		return nil, err
 	}

 	// Download the blob from presigned URL
-	req, err := http.NewRequestWithContext(ctx, method, url, nil)
+	req, err := http.NewRequestWithContext(ctx, method, blob.URL, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -233,13 +247,13 @@ func (p *ProxyBlobStore) Open(ctx context.Context, dgst digest.Digest) (io.ReadS

 	method := "GET"

-	url, err := p.getPresignedURL(ctx, method, dgst)
+	blob, err := p.getPresignedURL(ctx, method, dgst)
 	if err != nil {
 		return nil, err
 	}

 	// Download the blob from presigned URL
-	req, err := http.NewRequestWithContext(ctx, method, url, nil)
+	req, err := http.NewRequestWithContext(ctx, method, blob.URL, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -319,13 +333,13 @@ func (p *ProxyBlobStore) ServeBlob(ctx context.Context, w http.ResponseWriter, r
 		return err
 	}

-	url, err := p.getPresignedURL(ctx, r.Method, dgst)
+	blob, err := p.getPresignedURL(ctx, r.Method, dgst)
 	if err != nil {
 		return err
 	}

 	// Redirect to presigned URL
-	http.Redirect(w, r, url, http.StatusTemporaryRedirect)
+	http.Redirect(w, r, blob.URL, http.StatusTemporaryRedirect)
 	return nil
 }

@@ -388,8 +402,21 @@ func (p *ProxyBlobStore) Resume(ctx context.Context, id string) (distribution.Bl
 	return writer, nil
 }

-// getPresignedURL returns the XRPC endpoint URL for blob operations
-func (p *ProxyBlobStore) getPresignedURL(ctx context.Context, operation string, dgst digest.Digest) (string, error) {
+// presignedBlob is the hold's answer to a getBlob presign request.
+type presignedBlob struct {
+	// URL is the presigned S3 URL for the requested operation.
+	URL string
+	// Size is the blob's byte size as reported by the hold, or nil when the
+	// hold did not report one. A pointer rather than a plain int64 so that
+	// "the hold said nothing" stays distinguishable from "the hold said zero":
+	// a hold older than the size field reports nothing, and callers must fall
+	// back rather than believe in a zero-length blob.
+	Size *int64
+}
+
+// getPresignedURL asks the hold for a presigned URL for a blob operation, and
+// for reads gets the blob's size back with it.
+func (p *ProxyBlobStore) getPresignedURL(ctx context.Context, operation string, dgst digest.Digest) (presignedBlob, error) {
 	// Use XRPC endpoint: /xrpc/com.atproto.sync.getBlob?did={userDID}&cid={digest}
 	// The 'did' parameter is the USER's DID (whose blob we're fetching), not the hold service DID
 	// Per migration doc: hold accepts OCI digest directly as cid parameter (checks for sha256: prefix)
@@ -398,44 +425,55 @@ func (p *ProxyBlobStore) getPresignedURL(ctx context.Context, operation string,

 	req, err := http.NewRequestWithContext(ctx, "GET", xrpcURL, nil)
 	if err != nil {
-		return "", fmt.Errorf("failed to create request: %w", err)
+		return presignedBlob{}, fmt.Errorf("failed to create request: %w", err)
 	}

 	resp, err := p.doAuthenticatedRequest(ctx, req)
 	if err != nil {
 		// Don't wrap errcode errors - return them directly
 		if _, ok := err.(errcode.Error); ok {
-			return "", err
+			return presignedBlob{}, err
 		}
-		return "", fmt.Errorf("failed to get presigned URL: %w", err)
+		return presignedBlob{}, fmt.Errorf("failed to get presigned URL: %w", err)
 	}
 	defer resp.Body.Close()

 	if resp.StatusCode == http.StatusForbidden && p.ctx.Anonymous {
 		// Stale local captain cache let an anonymous request through, but the
 		// hold says private. Surface a 401 so the client re-authenticates.
-		return "", errcode.ErrorCodeUnauthorized.WithMessage("authentication required")
+		return presignedBlob{}, errcode.ErrorCodeUnauthorized.WithMessage("authentication required")
+	}
+
+	if resp.StatusCode == http.StatusNotFound {
+		// The hold checked its storage and the blob is not there. Return the
+		// sentinel rather than a generic failure: Stat passes it through as
+		// blob-unknown, and Get and Open hand their callers the error the
+		// distribution interface documents for a missing blob.
+		return presignedBlob{}, distribution.ErrBlobUnknown
 	}

 	if resp.StatusCode != http.StatusOK {
 		bodyBytes, _ := io.ReadAll(resp.Body)
-		return "", fmt.Errorf("hold service returned error: status %d, body: %s", resp.StatusCode, string(bodyBytes))
+		return presignedBlob{}, fmt.Errorf("hold service returned error: status %d, body: %s", resp.StatusCode, string(bodyBytes))
 	}

-	// Parse JSON response to get presigned HEAD URL
+	// Parse JSON response to get the presigned URL, and the size when the hold
+	// reports one. Size is a pointer so an older hold, which sends no size
+	// field at all, is not read as a zero-length blob.
 	var result struct {
-		URL string `json:"url"`
+		URL  string `json:"url"`
+		Size *int64 `json:"size"`
 	}
 	if err := json.NewDecoder(resp.Body).Decode(&result); err != nil {
-		return "", fmt.Errorf("failed to parse hold service response: %w", err)
+		return presignedBlob{}, fmt.Errorf("failed to parse hold service response: %w", err)
 	}

 	if result.URL == "" {
-		return "", fmt.Errorf("hold service returned empty URL")
+		return presignedBlob{}, fmt.Errorf("hold service returned empty URL")
 	}

-	slog.Debug("Got presigned HEAD URL from hold service", "component", "proxy_blob_store", "url", result.URL)
-	return result.URL, nil
+	slog.Debug("Got presigned URL from hold service", "component", "proxy_blob_store", "url", result.URL, "size_reported", result.Size != nil)
+	return presignedBlob{URL: result.URL, Size: result.Size}, nil
 }

 // startMultipartUpload initiates a multipart upload via XRPC initiateUpload endpoint
@@ -5,6 +5,7 @@ import (
 	"context"
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -2099,3 +2100,156 @@ func TestMultipartEndpoints_CorrectURLs(t *testing.T) {
 		})
 	}
 }
+
+// TestStat_UsesHoldReportedSize pins that a hold which reports the size ends
+// the Stat there: the descriptor is built from it and S3 is never touched.
+func TestStat_UsesHoldReportedSize(t *testing.T) {
+	cases := []struct {
+		name string
+		size int64
+	}{
+		{"normal blob", 4096},
+		// Zero is a real size, and it must not be read as "the hold said
+		// nothing". That is why the field is decoded into a pointer.
+		{"empty blob", 0},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			var s3Requests int64
+			var mu sync.Mutex
+
+			s3Server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+				mu.Lock()
+				s3Requests++
+				mu.Unlock()
+				w.WriteHeader(http.StatusOK)
+			}))
+			defer s3Server.Close()
+
+			holdServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+				w.Header().Set("Content-Type", "application/json")
+				json.NewEncoder(w).Encode(map[string]any{
+					"url":  s3Server.URL + "/blob?X-Amz-Signature=fake",
+					"size": tc.size,
+				})
+			}))
+			defer holdServer.Close()
+
+			store := NewProxyBlobStore(&RegistryContext{
+				DID:          "did:plc:test",
+				HoldDID:      "did:web:hold.example.com",
+				Repository:   "test-repo",
+				ServiceToken: "test-service-token",
+			})
+			store.holdURL = holdServer.URL
+
+			dgst := digest.FromString("sized-blob")
+			desc, err := store.Stat(context.Background(), dgst)
+			if err != nil {
+				t.Fatalf("Stat() failed: %v", err)
+			}
+
+			if desc.Size != tc.size {
+				t.Errorf("Expected size %d from the hold, got %d", tc.size, desc.Size)
+			}
+			if desc.Digest != dgst {
+				t.Errorf("Expected digest %s, got %s", dgst, desc.Digest)
+			}
+
+			mu.Lock()
+			defer mu.Unlock()
+			if s3Requests != 0 {
+				t.Errorf("Expected no request to S3 when the hold reports the size, got %d", s3Requests)
+			}
+		})
+	}
+}
+
+// TestStat_FallsBackToHeadWhenSizeAbsent pins the rollout direction that
+// matters: a new AppView against a hold that predates the size field must keep
+// working, by HEADing the presigned URL exactly as it always did.
+func TestStat_FallsBackToHeadWhenSizeAbsent(t *testing.T) {
+	const blobSize = 8192
+
+	var mu sync.Mutex
+	var s3Methods []string
+
+	s3Server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		mu.Lock()
+		s3Methods = append(s3Methods, r.Method)
+		mu.Unlock()
+
+		w.Header().Set("Content-Length", strconv.Itoa(blobSize))
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer s3Server.Close()
+
+	holdServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		// An old hold answers with the url and nothing else.
+		json.NewEncoder(w).Encode(map[string]string{
+			"url": s3Server.URL + "/blob?X-Amz-Signature=fake",
+		})
+	}))
+	defer holdServer.Close()
+
+	store := NewProxyBlobStore(&RegistryContext{
+		DID:          "did:plc:test",
+		HoldDID:      "did:web:hold.example.com",
+		Repository:   "test-repo",
+		ServiceToken: "test-service-token",
+	})
+	store.holdURL = holdServer.URL
+
+	desc, err := store.Stat(context.Background(), digest.FromString("unsized-blob"))
+	if err != nil {
+		t.Fatalf("Stat() failed: %v", err)
+	}
+
+	if desc.Size != blobSize {
+		t.Errorf("Expected size %d from Content-Length, got %d", blobSize, desc.Size)
+	}
+
+	mu.Lock()
+	defer mu.Unlock()
+	if len(s3Methods) != 1 {
+		t.Fatalf("Expected exactly 1 request to S3, got %d: %v", len(s3Methods), s3Methods)
+	}
+	if s3Methods[0] != http.MethodHead {
+		t.Errorf("Expected a HEAD to the presigned URL, got %s", s3Methods[0])
+	}
+}
+
+// TestStat_HoldNotFoundIsBlobUnknown pins the error mapping for a hold that
+// checked storage and found nothing. It must read as a missing blob, not as a
+// server error and not as an authentication problem.
+func TestStat_HoldNotFoundIsBlobUnknown(t *testing.T) {
+	holdServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusNotFound)
+		json.NewEncoder(w).Encode(map[string]string{
+			"error":   "BlobUnknown",
+			"message": "blob not found",
+		})
+	}))
+	defer holdServer.Close()
+
+	store := NewProxyBlobStore(&RegistryContext{
+		DID:          "did:plc:test",
+		HoldDID:      "did:web:hold.example.com",
+		Repository:   "test-repo",
+		ServiceToken: "test-service-token",
+	})
+	store.holdURL = holdServer.URL
+
+	_, err := store.Stat(context.Background(), digest.FromString("missing-blob"))
+	if !errors.Is(err, distribution.ErrBlobUnknown) {
+		t.Errorf("Expected ErrBlobUnknown for a hold 404, got %v", err)
+	}
+
+	// Get and Open get the same sentinel rather than a generic failure.
+	if _, err := store.Get(context.Background(), digest.FromString("missing-blob")); !errors.Is(err, distribution.ErrBlobUnknown) {
+		t.Errorf("Expected ErrBlobUnknown from Get for a hold 404, got %v", err)
+	}
+}