hold/admin: bound the crew tab's identity lookups, and drop backend HTTP/2

Two corrections to the HTTP/2 change, both found by running it.

UpCloud refuses http2_enabled on a backend that is not TLS:

  invalid_params_properties.http2_enabled='Tls must be enabled to enable HTTP2.'

This load balancer reaches both origins over the private network in cleartext,
so the option is not available here at all, and asking for it failed the whole
provision run with a 400 after the frontend had already been changed. Backend
HTTP/2 is removed rather than made conditional: it would need TLS terminated at
the origins, which is a much larger change than it earns. The frontend is the
leg that mattered, and it is enabled and confirmed live — all three domains now
negotiate h2 by ALPN.

The second correction is more interesting: enabling HTTP/2 did not fix the
symptom it was aimed at, and briefly made it look worse. With the browser no
longer rationing itself to ~6 connections, every row of the crew tab now reaches
the server at once, and the 504s went from a flat 10s to a climbing ladder:

  [HTTP/2 504 10026ms] ... [HTTP/2 504 15230ms] ... [HTTP/2 504 25976ms]

HTTP/1.1 had been hiding a server-side limit by throttling the client. The queue
moved; it did not disappear. What is actually slow is resolveHandle, which calls
live identity resolution per row with no bound of its own, so a DID whose
resolution hangs — a did:web on a host that stopped answering, a PDS that
accepts and then stalls — holds its request open until the proxy gives up. The
crew tab issues one request per member, so a hold with hundreds of crew gets
hundreds of chances to hit one.

resolveHandle now bounds each lookup at 2s. A handle is decoration on a row
whose DID is already rendered beside it, so waiting seconds for one and failing
the row when it does not arrive trades something load-bearing for something
cosmetic. Two seconds is far above a warm cached lookup and far below the
proxy's 10s cut, so a slow DID costs its own row a handle and costs the page
nothing.

The quota lookup on the same handler was the other suspect and was measured out:
records(collection, did) is indexed and the aggregation returns immediately on
the production database.

This does not remove the N+1 — the tab still issues one request per member, and
a batch endpoint is the real fix. It removes the failure.

Verified: the guard is covered by a test using a directory that hangs until its
context is cancelled, asserting both that the call returns near the bound and
that it did not return suspiciously early. make lint 0 issues, make test green
across 44 packages.

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

deploy/upcloud/provision.go

@@ -752,20 +752,27 @@ func httpFrontendProperties() *upcloud.LoadBalancerFrontendProperties {
 	}
 }
 
-// backendHTTP2 reports whether a backend may be spoken to over HTTP/2.
+// Backends are deliberately left at HTTP/1.1.
 //
-// False for the hold, and that is not an oversight. The hold serves
+// UpCloud refuses http2_enabled on a backend that is not TLS:
+//
+//	invalid_params_properties.http2_enabled='Tls must be enabled to enable HTTP2.'
+//
+// This LB reaches both origins over the private network in cleartext, so the
+// option is simply unavailable here; requesting it fails the whole provision
+// run with a 400. Backend HTTP/2 would first require terminating TLS at the
+// origins, which is a larger change than the frontend win justifies.
+//
+// The frontend is where it mattered anyway: that is the leg a browser speaks,
+// and the ~6-connections-per-origin limit it removes is what made one slow
+// endpoint block every other request to the same host.
+//
+// Both origins still wrap their handler in h2c, so if backend TLS is ever
+// added, enabling this becomes a config change rather than a code change. Note
+// the hold would still have to opt out even then: it serves
 // com.atproto.sync.subscribeRepos over WebSocket to external relays and to the
-// scanner, and WebSocket over HTTP/2 requires the RFC 8441 Extended CONNECT
-// that Go's http2 server does not implement for Upgrade:. Routing that backend
-// over h2 would break the firehose. The appview accepts no inbound WebSocket,
-// so it has no such constraint.
-//
-// Both origins wrap their handler in h2c regardless, so enabling this later is
-// a config change rather than a code change.
-func backendHTTP2(name string) *bool {
-	return new(name != "hold")
-}
+// scanner, and WebSocket over HTTP/2 needs the RFC 8441 Extended CONNECT that
+// Go's http2 server does not implement for Upgrade:.
 
 func createLoadBalancer(ctx context.Context, svc *service.Service, cfg *InfraConfig, naming Naming, networkUUID, appviewIP, holdIP, holdDomain, labelerDomain string, withLabeler bool) (*upcloud.LoadBalancer, error) {
 	frontendRules := []request.LoadBalancerFrontendRule{
@@ -817,7 +824,6 @@ func createLoadBalancer(ctx context.Context, svc *service.Service, cfg *InfraCon
 			Properties: &upcloud.LoadBalancerBackendProperties{
 				HealthCheckType: upcloud.LoadBalancerHealthCheckTypeHTTP,
 				HealthCheckURL:  "/health",
-				HTTP2Enabled:    backendHTTP2("appview"),
 			},
 		},
 		{
@@ -836,7 +842,6 @@ func createLoadBalancer(ctx context.Context, svc *service.Service, cfg *InfraCon
 			Properties: &upcloud.LoadBalancerBackendProperties{
 				HealthCheckType: upcloud.LoadBalancerHealthCheckTypeHTTP,
 				HealthCheckURL:  "/xrpc/_health",
-				HTTP2Enabled:    backendHTTP2("hold"),
 			},
 		},
 	}
@@ -1057,34 +1062,6 @@ func ensureLBHTTP2(ctx context.Context, svc *service.Service, lbUUID string) err
 		fmt.Printf("  Frontend HTTP/2: enabled (timeout_client=%ds)\n", want.TimeoutClient)
 	}
 
-	backends, err := svc.GetLoadBalancerBackends(ctx, &request.GetLoadBalancerBackendsRequest{ServiceUUID: lbUUID})
-	if err != nil {
-		return fmt.Errorf("get backends: %w", err)
-	}
-	for _, b := range backends {
-		wantH2 := backendHTTP2(b.Name)
-		have := b.Properties != nil && b.Properties.HTTP2Enabled != nil && *b.Properties.HTTP2Enabled
-		if have == *wantH2 {
-			continue
-		}
-		// Send the health check back with it: Properties replaces the object
-		// wholesale, so omitting these would drop the backend's health check.
-		props := &upcloud.LoadBalancerBackendProperties{HTTP2Enabled: wantH2}
-		if b.Properties != nil {
-			p := *b.Properties
-			p.HTTP2Enabled = wantH2
-			props = &p
-		}
-		if _, err := svc.ModifyLoadBalancerBackend(ctx, &request.ModifyLoadBalancerBackendRequest{
-			ServiceUUID: lbUUID,
-			Name:        b.Name,
-			Backend:     request.ModifyLoadBalancerBackend{Properties: props},
-		}); err != nil {
-			return fmt.Errorf("modify backend %s: %w", b.Name, err)
-		}
-		fmt.Printf("  Backend %s HTTP/2: %t\n", b.Name, *wantH2)
-	}
-
 	return nil
 }
 

pkg/hold/admin/handlers_crew.go

@@ -30,6 +30,22 @@ type CrewMemberView struct {
 	AddedAt      time.Time
 }
 
+// resolveHandleTimeout bounds a single identity lookup.
+//
+// Without it a lookup inherits only the request context, so one DID whose
+// resolution hangs — a did:web on a host that no longer answers, a PDS that
+// accepts the connection and then stalls — holds the request open until the
+// proxy in front gives up and returns 504. The crew tab lazy-loads one request
+// per member, so on a hold with hundreds of crew that is hundreds of chances to
+// hit one, and the panel fills with "Server error" toasts.
+//
+// A handle is decoration on that row: the DID is already rendered beside it.
+// Waiting seconds for one, and failing the row when it does not arrive, trades
+// something load-bearing for something cosmetic. Two seconds is far above a
+// warm cached lookup and far below the proxy's cut, so a slow DID costs its own
+// row a handle and costs the page nothing.
+const resolveHandleTimeout = 2 * time.Second
+
 // resolveHandle attempts to resolve a DID to a handle
 // Returns empty string if resolution fails
 // resolveHandle is a package var so tests can count how many identity lookups a
@@ -37,6 +53,9 @@ type CrewMemberView struct {
 // resolving before the limit is applied scales with hold size, not with the
 // number of rows rendered.
 var resolveHandle = func(ctx context.Context, did string) string {
+	ctx, cancel := context.WithTimeout(ctx, resolveHandleTimeout)
+	defer cancel()
+
 	_, handle, _, err := atproto.ResolveIdentity(ctx, did)
 	if err != nil {
 		slog.Debug("Failed to resolve handle for DID", "did", did, "error", err)

pkg/hold/admin/handlers_crew_timeout_test.go

@@ -0,0 +1,51 @@
+package admin
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"atcr.io/pkg/atproto"
+	"github.com/bluesky-social/indigo/atproto/identity"
+	"github.com/bluesky-social/indigo/atproto/syntax"
+)
+
+// hangingDirectory stalls until its context is cancelled, standing in for the
+// case this guard exists for: a did:web host that accepts the connection and
+// then never answers.
+type hangingDirectory struct{ identity.Directory }
+
+func (hangingDirectory) LookupDID(ctx context.Context, _ syntax.DID) (*identity.Identity, error) {
+	<-ctx.Done()
+	return nil, ctx.Err()
+}
+
+func (h hangingDirectory) Lookup(ctx context.Context, _ syntax.AtIdentifier) (*identity.Identity, error) {
+	<-ctx.Done()
+	return nil, ctx.Err()
+}
+
+// A hanging lookup must give the row back without a handle rather than hold the
+// request until the proxy 504s it. Before this bound, one such DID took the
+// whole request down with it — and the crew tab issues one request per member.
+func TestResolveHandleBoundsAHangingLookup(t *testing.T) {
+	orig := atproto.GetDirectory()
+	atproto.SetDirectory(hangingDirectory{})
+	t.Cleanup(func() { atproto.SetDirectory(orig) })
+
+	// A context with no deadline of its own: the bound must come from
+	// resolveHandle, not from the caller.
+	start := time.Now()
+	got := resolveHandle(context.Background(), "did:plc:hdjmrwzdaehsvehbeocsleif")
+	elapsed := time.Since(start)
+
+	if got != "" {
+		t.Errorf("a failed lookup should yield no handle, got %q", got)
+	}
+	if elapsed >= 2*resolveHandleTimeout {
+		t.Errorf("took %v, expected to be bounded near %v", elapsed, resolveHandleTimeout)
+	}
+	if elapsed < resolveHandleTimeout/2 {
+		t.Errorf("returned in %v, suspiciously fast — did the lookup actually run?", elapsed)
+	}
+}