diff --git a/deploy/upcloud/provision.go b/deploy/upcloud/provision.go index 7f2cb12..c038322 100644 --- a/deploy/upcloud/provision.go +++ b/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 } diff --git a/pkg/hold/admin/handlers_crew.go b/pkg/hold/admin/handlers_crew.go index 0f872c7..a418068 100644 --- a/pkg/hold/admin/handlers_crew.go +++ b/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) diff --git a/pkg/hold/admin/handlers_crew_timeout_test.go b/pkg/hold/admin/handlers_crew_timeout_test.go new file mode 100644 index 0000000..e10b693 --- /dev/null +++ b/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) + } +}