From 9228579b07710be1efcdbe4c94dc8a7085ad5d93 Mon Sep 17 00:00:00 2001
From: Evan Jarrett <evan@evanjarrett.com>
Date: Wed, 9 Sep 2026 09:31:16 -0500
Subject: [PATCH] appview: share one HTTP transport across blob proxy requests

NewProxyBlobStore built a fresh http.Client and http.Transport on every
call, and it is called once per registry request because the routing
repository is created per request. So no connection to the hold or to a
presigned S3 URL was ever reused: every XRPC call and every part upload
paid a TCP and TLS handshake, HTTP/2 was never negotiated, the idle pool
settings were dead config, and each discarded transport kept its idle
sockets open for the full 90s timeout.

One package-level transport and client now back every store. Settings
are unchanged; ForceAttemptHTTP2 is explicit to document intent. The
load balancer negotiates h2 on the frontend (416ba4a) but reaches the
hold over HTTP/1.1, so multiplexing stops at the LB. The win is the
handshake and socket churn, not multiplexing.

The struct field stays per-instance so tests can substitute a client.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Yf1ZVA7sXYhQNb9tCo1m5
---
 pkg/appview/storage/proxy_blob_store.go | 43 +++++++++++++++++++------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/pkg/appview/storage/proxy_blob_store.go b/pkg/appview/storage/proxy_blob_store.go
index bb03ad6..1210c71 100644
--- a/pkg/appview/storage/proxy_blob_store.go
+++ b/pkg/appview/storage/proxy_blob_store.go
@@ -32,6 +32,36 @@ var (
 	globalUploadsMu sync.RWMutex
 )
 
+// The transport and client below are package-level on purpose. RoutingRepository
+// (and therefore ProxyBlobStore) is built fresh on every registry request, so a
+// per-instance transport was thrown away after a single request: nothing was ever
+// reused, every XRPC call to the hold and every presigned S3 request paid for a
+// fresh TCP + TLS handshake, the MaxIdleConns settings below were dead config, and
+// each discarded transport still sat on its idle sockets for the full
+// IdleConnTimeout. Sharing one transport process-wide is what makes the idle pool
+// and keep-alives mean anything.
+//
+// ForceAttemptHTTP2 is explicit to document intent; no custom DialContext or
+// TLSClientConfig is set, so Go's automatic HTTP/2 negotiation over ALPN stays on
+// (see 416ba4a, where the load balancer started speaking HTTP/2 on the frontend).
+// The LB talks HTTP/1.1 to the hold backend, so multiplexing stops at the LB. The
+// win here is not multiplexing: it is removing the handshake and socket churn.
+var (
+	sharedTransport = &http.Transport{
+		DisableKeepAlives:   false,
+		MaxIdleConns:        100,
+		MaxIdleConnsPerHost: 100,
+		MaxConnsPerHost:     0, // unlimited
+		IdleConnTimeout:     90 * time.Second,
+		ForceAttemptHTTP2:   true,
+	}
+
+	sharedHTTPClient = &http.Client{
+		Timeout:   5 * time.Minute, // Timeout for presigned URL requests and uploads
+		Transport: sharedTransport,
+	}
+)
+
 // ProxyBlobStore proxies blob requests to an external storage service
 type ProxyBlobStore struct {
 	ctx        *RegistryContext // All context and services
@@ -49,16 +79,9 @@ func NewProxyBlobStore(ctx *RegistryContext) *ProxyBlobStore {
 	return &ProxyBlobStore{
 		ctx:     ctx,
 		holdURL: holdURL,
-		httpClient: &http.Client{
-			Timeout: 5 * time.Minute, // Timeout for presigned URL requests and uploads
-			Transport: &http.Transport{
-				DisableKeepAlives:   false, // Re-enable keep-alive
-				MaxIdleConns:        100,
-				MaxIdleConnsPerHost: 100,
-				MaxConnsPerHost:     0, // unlimited
-				IdleConnTimeout:     90 * time.Second,
-			},
-		},
+		// Field stays per-instance so tests can substitute a client; the default
+		// points at the process-wide client so connections are actually reused.
+		httpClient: sharedHTTPClient,
 	}
 }