From 8c9a85826df2ecdcda7010eba7aa2aca19d8aba9 Mon Sep 17 00:00:00 2001
From: Evan Jarrett <evan@evanjarrett.com>
Date: Tue, 25 Aug 2026 09:43:38 -0500
Subject: [PATCH] appview: stop leasing the billing tier refresh, and let it be
 cancelled
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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       |  7 ++-
 pkg/appview/server.go          | 13 +++--
 pkg/billing/billing.go         | 42 +++++++++++----
 pkg/billing/billing_stub.go    |  3 +-
 pkg/billing/hold_tiers_test.go | 93 ++++++++++++++++++++++++++++++++++
 5 files changed, 135 insertions(+), 23 deletions(-)
 create mode 100644 pkg/billing/hold_tiers_test.go

diff --git a/pkg/appview/db/leases.go b/pkg/appview/db/leases.go
index c75d092..e3bf7d1 100644
--- a/pkg/appview/db/leases.go
+++ b/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
diff --git a/pkg/appview/server.go b/pkg/appview/server.go
index b5bb4f7..eee70d2 100644
--- a/pkg/appview/server.go
+++ b/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
diff --git a/pkg/billing/billing.go b/pkg/billing/billing.go
index 7737a71..8bc449a 100644
--- a/pkg/billing/billing.go
+++ b/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
diff --git a/pkg/billing/billing_stub.go b/pkg/billing/billing_stub.go
index bffd320..a28b539 100644
--- a/pkg/billing/billing_stub.go
+++ b/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) {}
diff --git a/pkg/billing/hold_tiers_test.go b/pkg/billing/hold_tiers_test.go
new file mode 100644
index 0000000..cc62eeb
--- /dev/null
+++ b/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")
+	}
+}