appview: stop leasing the billing tier refresh, and let it be cancelled

6a7ddb8 moved RefreshHoldTiers under a lease with the comment "one-shot; the
lease is released when it returns". It never returns: past the startup retries
it sits on a 30-minute ticker forever. Three consequences, all observed on a
two-instance run against one database:

The billing-tiers lease is never released on a clean stop, so it survives to
its TTL and the replacement instance waits a full minute for a worker the old
one is no longer running. Worse, the goroutine stays in the shutdown WaitGroup,
so "Timed out waiting for leased workers to stop" now fires on EVERY clean
shutdown. The other four leases released in 12ms and the warning fired anyway.
A warning that is always present cannot report the case it exists for, which is
the jetstream lease genuinely failing to release.

And the lease was the wrong tool regardless. The commit justified it as
"RefreshHoldTiers writes tier state derived from Stripe" that instances would
race on. It writes holdTierCache, a per-process map, from read-only ListTiers
calls; there is no shared state anywhere in the path. Electing one refresher
means every other instance keeps an empty cache forever, so
aggregateHoldFeatures reports "no hold data" on all but one — a regression that
only appears at the scale the lease was added to support. This is the hold
health worker's situation exactly, and that one was deliberately left unleased
in the same commit.

So it runs on every instance again, with a context. The retry backoff was
time.Sleep for up to 45s total against an unreachable hold; it and the ticker
now select on ctx.Done, and ListTiers gets the context instead of
context.Background. LeaseBillingTiers is gone rather than left as a dead name.

Verified: with the backoff restored to time.Sleep the new test fails on its own
2s deadline rather than hanging, which is how a shutdown regression here should
present.

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

pkg/appview/db/leases.go

@@ -10,10 +10,9 @@ import (
 // Lease names used by the AppView. Each names exactly one background worker
 // that must not run on more than one instance at a time.
 const (
-	LeaseJetstream    = "jetstream"
-	LeaseBackfill     = "backfill"
-	LeaseCleanup      = "cleanup"
-	LeaseBillingTiers = "billing-tiers"
+	LeaseJetstream = "jetstream"
+	LeaseBackfill  = "backfill"
+	LeaseCleanup   = "cleanup"
 )
 
 // LeaseLabeler returns the lease name for a labeler subscriber. Each labeler has

pkg/appview/server.go

@@ -310,13 +310,12 @@ func NewAppViewServer(cfg *Config, branding *BrandingOverrides) (*AppViewServer,
 			return nil, fmt.Errorf("billing is enabled but STRIPE_WEBHOOK_SECRET is not set; refusing to start with a forgeable webhook endpoint")
 		}
 		slog.Info("Billing enabled", "appview_did", appviewDID, "managed_holds", len(cfg.Server.ManagedHolds))
-		// Leased: RefreshHoldTiers writes tier state derived from Stripe, and
-		// several instances refreshing the same holds concurrently would race
-		// on those writes for no benefit.
-		s.Leases.Go(workerCtx, db.LeaseBillingTiers, func(context.Context) error {
-			s.BillingManager.RefreshHoldTiers()
-			return nil // one-shot; the lease is released when it returns
-		})
+		// Deliberately not leased. holdTierCache is per-process memory fed by
+		// read-only ListTiers calls, so there is nothing to serialise, and
+		// electing one refresher would leave every other instance reporting
+		// "no hold data" from aggregateHoldFeatures. Same reasoning as the
+		// hold health worker above.
+		go s.BillingManager.RefreshHoldTiers(workerCtx)
 	}
 
 	// Create webhook dispatcher

pkg/billing/billing.go

@@ -952,11 +952,20 @@ func (m *Manager) cacheCustomer(userDID string, cust *stripe.Customer) {
 
 const holdTierCacheTTL = 30 * time.Minute
 
-// RefreshHoldTiers queries all managed holds for their tier definitions and caches the results.
-// It runs once immediately (with retries for holds that aren't ready yet) and then
-// periodically in the background.
-// Safe to call from a goroutine.
-func (m *Manager) RefreshHoldTiers() {
+// RefreshHoldTiers queries all managed holds for their tier definitions and
+// caches the results. It runs once immediately (with retries for holds that are
+// not ready yet) and then periodically until ctx is cancelled.
+//
+// This runs on every instance rather than under a lease. holdTierCache is
+// per-process memory and refreshHoldTiersOnce issues read-only ListTiers calls,
+// so there is no shared state to serialise and nothing to race on. Electing one
+// refresher would leave every other instance with an empty cache, which
+// aggregateHoldFeatures reports as "no hold data" — the same reasoning that
+// keeps the hold health worker unleased.
+//
+// It only returns when ctx is cancelled, so start it with `go` and hand it a
+// context that shutdown closes.
+func (m *Manager) RefreshHoldTiers(ctx context.Context) {
 	if !m.Enabled() || len(m.managedHolds) == 0 {
 		return
 	}
@@ -967,7 +976,7 @@ func (m *Manager) RefreshHoldTiers() {
 	const initialDelay = 3 * time.Second
 
 	for attempt := range maxRetries {
-		m.refreshHoldTiersOnce()
+		m.refreshHoldTiersOnce(ctx)
 
 		// Check if all managed holds are cached
 		m.holdTierCacheMu.RLock()
@@ -982,20 +991,31 @@ func (m *Manager) RefreshHoldTiers() {
 			delay := initialDelay * time.Duration(1<<attempt) // 3s, 6s, 12s, 24s
 			slog.Info("Some managed holds not yet reachable, retrying",
 				"attempt", attempt+1, "maxRetries", maxRetries, "retryIn", delay)
-			time.Sleep(delay)
+			// Not time.Sleep: the backoff runs up to 45s in total with an
+			// unreachable hold, and shutdown must not have to wait it out.
+			select {
+			case <-time.After(delay):
+			case <-ctx.Done():
+				return
+			}
 		}
 	}
 
 	ticker := time.NewTicker(holdTierCacheTTL)
 	defer ticker.Stop()
-	for range ticker.C {
-		m.refreshHoldTiersOnce()
+	for {
+		select {
+		case <-ticker.C:
+			m.refreshHoldTiersOnce(ctx)
+		case <-ctx.Done():
+			return
+		}
 	}
 }
 
-func (m *Manager) refreshHoldTiersOnce() {
+func (m *Manager) refreshHoldTiersOnce(ctx context.Context) {
 	for _, holdDID := range m.managedHolds {
-		resp, err := holdclient.ListTiers(context.Background(), holdDID)
+		resp, err := holdclient.ListTiers(ctx, holdDID)
 		if err != nil {
 			slog.Warn("Failed to fetch tiers from hold", "holdDID", holdDID, "error", err)
 			continue

pkg/billing/billing_stub.go

@@ -3,6 +3,7 @@
 package billing
 
 import (
+	"context"
 	"database/sql"
 	"net/http"
 
@@ -93,4 +94,4 @@ func (m *Manager) GetFirstTierWithAllTriggers() string {
 func (m *Manager) RegisterRoutes(_ chi.Router) {}
 
 // RefreshHoldTiers is a no-op when billing is not compiled in.
-func (m *Manager) RefreshHoldTiers() {}
+func (m *Manager) RefreshHoldTiers(context.Context) {}

pkg/billing/hold_tiers_test.go

@@ -0,0 +1,93 @@
+//go:build billing
+
+package billing
+
+import (
+	"context"
+	"path/filepath"
+	"testing"
+	"time"
+
+	appdb "atcr.io/pkg/appview/db"
+)
+
+// newHoldTierManager builds a Manager whose only managed hold is unreachable,
+// so refreshHoldTiersOnce always fails and RefreshHoldTiers is forced onto its
+// retry backoff — the path that decides how long shutdown has to wait.
+func newHoldTierManager(t *testing.T, holds []string) *Manager {
+	t.Helper()
+
+	database, err := appdb.InitDB(filepath.Join(t.TempDir(), "ui.db"), appdb.LibsqlConfig{})
+	if err != nil {
+		t.Fatalf("init db: %v", err)
+	}
+	t.Cleanup(func() { _ = database.Close() })
+
+	t.Setenv("STRIPE_SECRET_KEY", "sk_test_fake")
+	t.Setenv("STRIPE_WEBHOOK_SECRET", "whsec_fake")
+
+	cfg := &Config{
+		StripeSecretKey: "sk_test_fake",
+		WebhookSecret:   "whsec_fake",
+		Tiers:           []BillingTierConfig{{Name: "free"}},
+	}
+	m := New(cfg, nil, "did:web:test-appview.local", holds, "http://test-appview.local", database)
+	if !m.Enabled() {
+		t.Fatal("manager should be enabled")
+	}
+	return m
+}
+
+// TestRefreshHoldTiersStopsOnContextCancel pins the shutdown behaviour.
+//
+// RefreshHoldTiers does not return on its own: past the startup retries it sits
+// on a 30-minute ticker forever. It used to ignore cancellation entirely, so
+// with an unreachable hold it held the process through up to 45s of backoff
+// (3+6+12+24) and then never finished at all. Under a WaitGroup that shutdown
+// waits on, that is a guaranteed drain timeout on every clean stop.
+//
+// The deadline goroutine matters: a regression here hangs rather than failing,
+// and a hung test is reported as a suite timeout minutes later rather than as
+// this test.
+func TestRefreshHoldTiersStopsOnContextCancel(t *testing.T) {
+	// Port 1 is not listening, so ListTiers fails fast and the retry backoff
+	// is entered on the first attempt.
+	m := newHoldTierManager(t, []string{"did:web:127.0.0.1%3A1"})
+
+	ctx, cancel := context.WithCancel(context.Background())
+	returned := make(chan struct{})
+	go func() {
+		defer close(returned)
+		m.RefreshHoldTiers(ctx)
+	}()
+
+	// Let it reach the first backoff (3s) before cancelling, so the test is
+	// about interrupting the wait rather than about racing to start it.
+	time.Sleep(300 * time.Millisecond)
+	cancel()
+
+	select {
+	case <-returned:
+	case <-time.After(2 * time.Second):
+		t.Fatal("RefreshHoldTiers did not return within 2s of cancellation; " +
+			"shutdown will hit the leased-worker drain timeout")
+	}
+}
+
+// TestRefreshHoldTiersReturnsImmediatelyWhenNotConfigured covers the two
+// guard clauses, which are the only other way out of the function.
+func TestRefreshHoldTiersReturnsImmediatelyWhenNotConfigured(t *testing.T) {
+	m := newHoldTierManager(t, nil)
+
+	returned := make(chan struct{})
+	go func() {
+		defer close(returned)
+		m.RefreshHoldTiers(context.Background())
+	}()
+
+	select {
+	case <-returned:
+	case <-time.After(2 * time.Second):
+		t.Fatal("RefreshHoldTiers blocked with no managed holds configured")
+	}
+}