appview/holdclient: cover the tier fan-out itself, and its failure path

The existing tests covered updateCrewTierWithRetry and UpdateCrewTierOnHold.
UpdateCrewTierOnAllHolds -- the function the Stripe webhook actually calls, and
whose error decides whether a paid upgrade is retried or dropped -- had none.

Three cases: the joined error names every failing hold and not the one that
succeeded; a hold that accepts and never answers does not starve the holds
after it (mutation-verified by making the fan-out serial, which leaves the
healthy hold contacted zero times); and a context deadline aborts the retry
loop rather than running to tierUpdateMaxAttempts.

That last one records a real mismatch rather than an intent. Three attempts at
a 5s client timeout need ~15s, and the webhook allows the whole fan-out 10s, so
under a hang the budget funds two attempts and never three -- confirmed against
a blackholed hold on the dev stack, which failed at exactly 10.0s with a bare
context error rather than the "after N attempts" wrapper. If either constant or
the deadline moves, that test is where the arithmetic gets re-checked.

Also covers the other half in pkg/billing: a fan-out failure has to reach
Stripe as a 5xx and leave stripe_processed_events empty. A hold that is briefly
down otherwise costs the customer their tier permanently -- the same shape of
loss as the customer-lookup hole, one layer further out.

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

pkg/appview/holdclient/tier_update_test.go

@@ -4,8 +4,10 @@ import (
 	"context"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"sync/atomic"
 	"testing"
+	"time"
 
 	"atcr.io/pkg/atproto"
 	"github.com/bluesky-social/indigo/atproto/atcrypto"
@@ -82,3 +84,149 @@ func TestUpdateCrewTierOnHold_PostsToEndpoint(t *testing.T) {
 		t.Errorf("posted to %q, want %q", gotPath, atproto.HoldUpdateCrewTier)
 	}
 }
+
+// The tests above cover the two helpers. UpdateCrewTierOnAllHolds — the
+// function the Stripe webhook actually calls, and the one whose error decides
+// whether a paid upgrade is retried or dropped — had no test at all.
+
+// didFor turns an httptest server URL into a did:web that ResolveHoldDIDToURL
+// maps straight back to it. The port makes DIDWebToURL choose http, and test
+// mode is what lets a did:web that no directory can resolve fall back to being
+// decoded from the DID itself.
+func didFor(t *testing.T, serverURL string) string {
+	t.Helper()
+	host := strings.TrimPrefix(serverURL, "http://")
+	return "did:web:" + strings.ReplaceAll(host, ":", "%3A")
+}
+
+func testKey(t *testing.T) *atcrypto.PrivateKeyP256 {
+	t.Helper()
+	priv, err := atcrypto.GeneratePrivateKeyP256()
+	if err != nil {
+		t.Fatalf("generate key: %v", err)
+	}
+	return priv
+}
+
+// TestUpdateCrewTierOnAllHolds_JoinedErrorNamesEveryFailingHold: the caller
+// 5xxs the Stripe webhook on any non-nil return, and the operator's only
+// account of which holds are behind is this error. One failing hold must not
+// mask another.
+func TestUpdateCrewTierOnAllHolds_JoinedErrorNamesEveryFailingHold(t *testing.T) {
+	atproto.SetTestMode(true)
+	t.Cleanup(func() { atproto.SetTestMode(false) })
+
+	ok := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer ok.Close()
+	bad1 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Error(w, "down", http.StatusServiceUnavailable)
+	}))
+	defer bad1.Close()
+	bad2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Error(w, "broken", http.StatusInternalServerError)
+	}))
+	defer bad2.Close()
+
+	okDID, bad1DID, bad2DID := didFor(t, ok.URL), didFor(t, bad1.URL), didFor(t, bad2.URL)
+
+	err := UpdateCrewTierOnAllHolds(context.Background(),
+		[]string{okDID, bad1DID, bad2DID}, "did:plc:user", 1, testKey(t), "did:web:appview")
+	if err == nil {
+		t.Fatal("expected an error when two of three holds fail")
+	}
+	for _, did := range []string{bad1DID, bad2DID} {
+		if !strings.Contains(err.Error(), did) {
+			t.Errorf("joined error does not name failing hold %s: %v", did, err)
+		}
+	}
+	if strings.Contains(err.Error(), okDID) {
+		t.Errorf("joined error names the hold that succeeded (%s): %v", okDID, err)
+	}
+}
+
+// TestUpdateCrewTierOnAllHolds_SlowHoldDoesNotStarveOthers pins the concurrency
+// the function's doc claims.
+//
+// Contacted serially, one hold that burns the whole deadline means the holds
+// after it are never contacted at all — and since the webhook retries in the
+// same order, a persistently slow first hold would mean later holds are never
+// updated on any delivery. The assertion is that the healthy hold is reached
+// even though the slow one is listed first and never answers.
+func TestUpdateCrewTierOnAllHolds_SlowHoldDoesNotStarveOthers(t *testing.T) {
+	atproto.SetTestMode(true)
+	t.Cleanup(func() { atproto.SetTestMode(false) })
+
+	release := make(chan struct{})
+	slow := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		<-release
+	}))
+	defer slow.Close()
+	defer close(release)
+
+	var healthyHits atomic.Int32
+	healthy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		healthyHits.Add(1)
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer healthy.Close()
+
+	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+	defer cancel()
+
+	// Slow hold first: serial contact would spend the entire budget on it.
+	err := UpdateCrewTierOnAllHolds(ctx,
+		[]string{didFor(t, slow.URL), didFor(t, healthy.URL)},
+		"did:plc:user", 1, testKey(t), "did:web:appview")
+
+	if err == nil {
+		t.Error("expected an error naming the slow hold")
+	}
+	if got := healthyHits.Load(); got != 1 {
+		t.Errorf("healthy hold contacted %d times, want 1 — it was starved by the slow hold", got)
+	}
+}
+
+// TestUpdateCrewTierOnAllHolds_DeadlineCutsRetriesShort documents a real
+// mismatch rather than asserting an intent.
+//
+// tierUpdateMaxAttempts is 3 and each attempt is bounded by a 5s client
+// timeout, so three attempts against a hold that accepts and never answers
+// need ~15s. The Stripe webhook allows the whole fan-out 10s. Under a hang the
+// budget therefore funds two attempts, never three, and the caller gets the
+// context error rather than the "after N attempts" wrapper. If the deadline or
+// either constant changes, this test is where the arithmetic gets re-checked.
+func TestUpdateCrewTierOnAllHolds_DeadlineCutsRetriesShort(t *testing.T) {
+	atproto.SetTestMode(true)
+	t.Cleanup(func() { atproto.SetTestMode(false) })
+
+	release := make(chan struct{})
+	var attempts atomic.Int32
+	hung := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts.Add(1)
+		<-release
+	}))
+	defer hung.Close()
+	defer close(release)
+
+	// Deadline deliberately shorter than tierUpdateMaxAttempts would need.
+	ctx, cancel := context.WithTimeout(context.Background(), 400*time.Millisecond)
+	defer cancel()
+
+	start := time.Now()
+	err := UpdateCrewTierOnAllHolds(ctx, []string{didFor(t, hung.URL)},
+		"did:plc:user", 1, testKey(t), "did:web:appview")
+	elapsed := time.Since(start)
+
+	if err == nil {
+		t.Fatal("expected an error from a hold that never answers")
+	}
+	if elapsed > 3*time.Second {
+		t.Errorf("fan-out took %v; the context deadline did not abort the retry loop", elapsed)
+	}
+	if got := attempts.Load(); got >= int32(tierUpdateMaxAttempts) {
+		t.Errorf("hung hold was attempted %d times under a deadline that cannot fund %d",
+			got, tierUpdateMaxAttempts)
+	}
+}

pkg/billing/webhook_retry_test.go

@@ -15,6 +15,8 @@ import (
 	"time"
 
 	appdb "atcr.io/pkg/appview/db"
+	"atcr.io/pkg/atproto"
+	"github.com/bluesky-social/indigo/atproto/atcrypto"
 	"github.com/stripe/stripe-go/v84"
 	"github.com/stripe/stripe-go/v84/webhook"
 )
@@ -285,3 +287,48 @@ func TestHandleSubscriptionChange_NilCustomerDoesNotPanic(t *testing.T) {
 		t.Errorf("HandleWebhook error = %v, want nil for an event with no customer", err)
 	}
 }
+
+// TestHandleSubscriptionChange_HoldFanoutFailureIsRetryable closes the loop the
+// other tests in this file only cover one half of.
+//
+// The tier is resolved, the customer is known, and the only thing that fails is
+// the push to the managed hold. That has to reach Stripe as a 5xx and leave
+// stripe_processed_events empty: a hold that is briefly down otherwise costs
+// the customer their tier permanently, which is the same shape of loss as the
+// customer-lookup hole above, one layer further out.
+func TestHandleSubscriptionChange_HoldFanoutFailureIsRetryable(t *testing.T) {
+	atproto.SetTestMode(true)
+	t.Cleanup(func() { atproto.SetTestMode(false) })
+
+	const secret = "whsec_fanout_test"
+	m, database := newTestManager(t, secret)
+
+	// The customer resolves cleanly — this test is about what happens after.
+	stripeAPIReturning(t, http.StatusOK,
+		`{"id":"cus_fanout","object":"customer","metadata":{"user_did":"did:plc:fanoutuser"}}`)
+
+	hold := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Error(w, "hold is down", http.StatusServiceUnavailable)
+	}))
+	defer hold.Close()
+	m.managedHolds = []string{"did:web:" + strings.ReplaceAll(
+		strings.TrimPrefix(hold.URL, "http://"), ":", "%3A")}
+
+	priv, err := atcrypto.GeneratePrivateKeyP256()
+	if err != nil {
+		t.Fatalf("generate key: %v", err)
+	}
+	m.privateKey = priv
+
+	err = postWebhook(t, m, secret,
+		signedSubscriptionEvent(t, secret, "evt_fanout_fail", "cus_fanout", time.Now().Unix()))
+	if err == nil {
+		t.Fatal("a hold that cannot be updated must fail the webhook so Stripe redelivers")
+	}
+	if !strings.Contains(err.Error(), "push tier to managed holds") {
+		t.Errorf("error does not identify the fan-out as the cause: %v", err)
+	}
+	if n := processedCount(t, database); n != 0 {
+		t.Errorf("stripe_processed_events holds %d rows; a failed event must stay redeliverable", n)
+	}
+}