diff --git a/internal/billing/cloud_client.go b/internal/billing/cloud_client.go index 797a7cea4..caa9afc09 100644 --- a/internal/billing/cloud_client.go +++ b/internal/billing/cloud_client.go @@ -186,31 +186,37 @@ func (c *CloudClient) GetBillingMetrics() (*BillingMetricsResponse, error) { // success), so retries after a partial failure complete cleanly. // // Request: POST {baseURL}/billing/cancel-customer -// Body: {"customer_id": "cus_xxx", "cloud_secret": "..."} +// Body: {"customer_id": "cus_xxx", "user_id": "...", "cloud_secret": "..."} +// +// userID (BUG-3560) lets the sidecar also cancel every other customer the +// user owns (stamped pad_user_id), and makes an empty customerID valid: a +// user can own a stamped customer with nothing linked. A sidecar that +// predates it ignores user_id and refuses an empty customer_id with 400. // 200 OK: {"ok": true, "subscriptions_cancelled": N} // // Returns nil on 200. On any non-200, returns a *SidecarError so the caller // can branch on Status. On transport failure (DNS, connect, timeout) returns // a bare error — treated by callers as "retryable, abort the delete". -func (c *CloudClient) CancelCustomer(customerID string) error { +func (c *CloudClient) CancelCustomer(customerID, userID string) error { if c == nil { return errors.New("billing: CancelCustomer called on nil CloudClient") } - if customerID == "" { - // Defensive — handleDeleteAccount is expected to skip the call when - // StripeCustomerID is empty, but if somebody wires it differently - // we refuse to POST an empty cus_ that would burn a sidecar call - // and log-spam the 400 it would return. - return errors.New("billing: customerID is empty") + if customerID == "" && userID == "" { + // Nothing to name: the sidecar would refuse it with 400. + return errors.New("billing: customerID and userID are both empty") } if c.baseURL == "" || c.cloudSecret == "" { return errors.New("billing: CloudClient is not configured (missing baseURL or cloudSecret)") } - payload, err := json.Marshal(map[string]string{ + body := map[string]string{ "customer_id": customerID, "cloud_secret": c.cloudSecret, - }) + } + if userID != "" { + body["user_id"] = userID + } + payload, err := json.Marshal(body) if err != nil { return fmt.Errorf("billing: marshal cancel-customer request: %w", err) } diff --git a/internal/billing/cloud_client_test.go b/internal/billing/cloud_client_test.go index f2ddb6e0e..451415cba 100644 --- a/internal/billing/cloud_client_test.go +++ b/internal/billing/cloud_client_test.go @@ -186,7 +186,7 @@ func TestCancelCustomer_HappyPath_SendsCorrectRequest(t *testing.T) { _, _ = w.Write([]byte(`{"ok":true,"subscriptions_cancelled":2}`)) }) - if err := client.CancelCustomer("cus_abc"); err != nil { + if err := client.CancelCustomer("cus_abc", ""); err != nil { t.Fatalf("expected nil error on 200, got %v", err) } @@ -235,7 +235,7 @@ func TestCancelCustomer_NonOK_ReturnsSidecarError(t *testing.T) { _, _ = w.Write([]byte(tc.body)) }) - err := client.CancelCustomer("cus_abc") + err := client.CancelCustomer("cus_abc", "") if err == nil { t.Fatalf("expected error on %d, got nil", tc.status) } @@ -260,7 +260,7 @@ func TestCancelCustomer_TransportFailure_NotSidecarError(t *testing.T) { client := NewCloudClient("http://127.0.0.1:1", "test-secret") client.http.Timeout = 500 * time.Millisecond - err := client.CancelCustomer("cus_abc") + err := client.CancelCustomer("cus_abc", "") if err == nil { t.Fatal("expected transport error, got nil") } @@ -280,7 +280,7 @@ func TestCancelCustomer_EmptyCustomerID_ReturnsError(t *testing.T) { w.WriteHeader(http.StatusOK) }) - err := client.CancelCustomer("") + err := client.CancelCustomer("", "") if err == nil { t.Fatal("expected error for empty customer_id, got nil") } @@ -291,14 +291,14 @@ func TestCancelCustomer_EmptyCustomerID_ReturnsError(t *testing.T) { func TestCancelCustomer_NilClient_ReturnsError(t *testing.T) { var c *CloudClient - if err := c.CancelCustomer("cus_abc"); err == nil { + if err := c.CancelCustomer("cus_abc", ""); err == nil { t.Fatal("expected error from nil receiver, got nil") } } func TestCancelCustomer_UnconfiguredClient_ReturnsError(t *testing.T) { c := NewCloudClient("", "") - err := c.CancelCustomer("cus_abc") + err := c.CancelCustomer("cus_abc", "") if err == nil { t.Fatal("expected error for unconfigured client, got nil") } @@ -398,7 +398,7 @@ func TestCancelCustomer_LargeResponseBody_DoesNotReadPastCap(t *testing.T) { _, _ = w.Write([]byte(big)) }) - err := client.CancelCustomer("cus_abc") + err := client.CancelCustomer("cus_abc", "") var se *SidecarError if !errors.As(err, &se) { t.Fatalf("expected SidecarError, got %T", err) @@ -407,3 +407,21 @@ func TestCancelCustomer_LargeResponseBody_DoesNotReadPastCap(t *testing.T) { t.Errorf("body was %d bytes, expected <= %d (cap)", len(se.Body), maxResponseBody) } } + +// BUG-3560: the user id travels, and with it an empty customer id is a valid +// call (a stamped customer can exist with nothing linked). +func TestCancelCustomer_SendsUserIDAndAllowsEmptyCustomer(t *testing.T) { + var gotBody map[string]string + client, _ := newStub(t, func(w http.ResponseWriter, r *http.Request) { + b, _ := io.ReadAll(r.Body) + _ = json.Unmarshal(b, &gotBody) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"ok":true}`)) + }) + if err := client.CancelCustomer("", "user-1"); err != nil { + t.Fatalf("expected nil error, got %v", err) + } + if gotBody["user_id"] != "user-1" || gotBody["customer_id"] != "" { + t.Fatalf("body = %v, want user_id=user-1 and an empty customer_id", gotBody) + } +} diff --git a/internal/server/handlers_account.go b/internal/server/handlers_account.go index 45a1bf450..5ece8b2ea 100644 --- a/internal/server/handlers_account.go +++ b/internal/server/handlers_account.go @@ -9,6 +9,7 @@ import ( "strings" "time" + "github.com/PerpetualSoftware/pad/internal/billing" "github.com/PerpetualSoftware/pad/internal/models" "github.com/PerpetualSoftware/pad/internal/store" "github.com/PerpetualSoftware/pad/internal/watchevents" @@ -198,9 +199,8 @@ func (s *Server) handleDeleteAccount(w http.ResponseWriter, r *http.Request) { // would keep getting charged with no way for us to find and cancel // it. TASK-690 / PLAN-645. // - // Skipped when: - // - the user has no Stripe customer (free plan, OAuth-only, never paid) - // - the sidecar isn't configured (self-hosted deploys with no billing) + // Skipped when the sidecar isn't configured (self-hosted deploys with no + // billing). // // Failure strategy: any non-nil error aborts the delete with a 500. // pad-cloud normalizes Stripe 404/resource_missing to a 200 success on @@ -211,9 +211,27 @@ func (s *Server) handleDeleteAccount(w http.ResponseWriter, r *http.Request) { // continue": doing so would wipe the user's StripeCustomerID while // leaving the Stripe subscription billing, which is the exact // regression this task exists to prevent. + // + // BUG-3560: the sidecar is asked even when no customer is linked, with + // the user id, because a user can own a Stripe customer stamped with + // their id and not linked here (TASK-3549 keeps one link and unions the + // rest); it cancels every customer the user owns. A sidecar that predates + // user_id refuses an empty customer_id with 400: it could not have + // cancelled anything, which is what not asking did before, so that one + // answer lets the deletion proceed. stripeWasCancelled := false - if fullUser.StripeCustomerID != "" && s.cloudSidecar != nil { - if err := s.cloudSidecar.CancelCustomer(fullUser.StripeCustomerID); err != nil { + if s.cloudSidecar != nil { + err := s.cloudSidecar.CancelCustomer(fullUser.StripeCustomerID, fullUser.ID) + var sidecarErr *billing.SidecarError + switch { + case err == nil: + // Possibly only a stamped, unlinked customer: still billing that + // was cancelled, which the partial-delete alert below must know. + stripeWasCancelled = true + case fullUser.StripeCustomerID == "" && errors.As(err, &sidecarErr) && sidecarErr.Status == http.StatusBadRequest: + slog.Warn("delete account: sidecar predates user_id; no linked customer to cancel, continuing", + "user_id", user.ID) + default: slog.Error("delete account: sidecar cancel-customer failed, aborting delete", "user_id", user.ID, "customer_id", fullUser.StripeCustomerID, @@ -222,7 +240,6 @@ func (s *Server) handleDeleteAccount(w http.ResponseWriter, r *http.Request) { "We couldn't cancel your subscription. Your account was NOT deleted and you have NOT been charged anything new. Please try again in a few minutes or contact support.") return } - stripeWasCancelled = true } // TASK-3272: the deletion soft-deletes every live workspace this user diff --git a/internal/server/handlers_account_test.go b/internal/server/handlers_account_test.go index 67b994863..c08b2106b 100644 --- a/internal/server/handlers_account_test.go +++ b/internal/server/handlers_account_test.go @@ -25,14 +25,17 @@ import ( type fakeSidecar struct { calls int32 // atomic counter of CancelCustomer invocations lastCustomerID atomic.Pointer[string] + lastUserID atomic.Pointer[string] err error hook func(customerID string) // runs under CancelCustomer; optional } -func (f *fakeSidecar) CancelCustomer(customerID string) error { +func (f *fakeSidecar) CancelCustomer(customerID, userID string) error { atomic.AddInt32(&f.calls, 1) id := customerID // copy so the atomic pointer doesn't alias the caller's stack f.lastCustomerID.Store(&id) + uid := userID + f.lastUserID.Store(&uid) if f.hook != nil { f.hook(customerID) } @@ -151,11 +154,12 @@ func TestHandleDeleteAccount_CancelsStripeBeforeDelete(t *testing.T) { } } -// TestHandleDeleteAccount_SkipsWhenNoStripeCustomer verifies that users -// without a Stripe customer ID (free tier, OAuth-only, never paid) don't -// trigger a sidecar call. Otherwise we'd burn a sidecar RPC on every -// free-user delete and log-spam 400 "customer_id must start with 'cus_'". -func TestHandleDeleteAccount_SkipsWhenNoStripeCustomer(t *testing.T) { +// BUG-3560: a user with no LINKED Stripe customer can still own one stamped +// with their id (TASK-3549 keeps one link and unions the rest), so the +// sidecar is asked with the user id even then, and cancels whatever the user +// owns. (Before, such a user was never asked about, and that customer kept +// charging after the account was gone.) +func TestHandleDeleteAccount_AsksSidecarByUserWhenNoLinkedCustomer(t *testing.T) { srv := testServer(t) fake := &fakeSidecar{} srv.SetCloudSidecar(fake) @@ -166,26 +170,62 @@ func TestHandleDeleteAccount_SkipsWhenNoStripeCustomer(t *testing.T) { if rr.Code != http.StatusOK { t.Fatalf("delete-account: expected 200, got %d: %s", rr.Code, rr.Body.String()) } - - if fake.callCount() != 0 { - t.Errorf("CancelCustomer: expected 0 calls for non-paying user, got %d", fake.callCount()) + if fake.callCount() != 1 { + t.Fatalf("CancelCustomer: expected 1 call, got %d", fake.callCount()) + } + if got := *fake.lastCustomerID.Load(); got != "" { + t.Errorf("customer id = %q, want empty", got) + } + if got := *fake.lastUserID.Load(); got != userID { + t.Errorf("user id = %q, want %q", got, userID) } - u, err := srv.store.GetUser(userID) if err != nil { t.Fatalf("get user after delete: %v", err) } if u != nil { - t.Error("expected user to be deleted even without a Stripe customer") + t.Error("expected user to be deleted") + } +} + +// A sidecar that predates user_id refuses an empty customer_id with 400. It +// could not have cancelled anything, which is exactly what not asking did +// before, so this one answer lets the deletion of a user with no linked +// customer proceed: either deploy order is safe. +func TestHandleDeleteAccount_OlderSidecar400WithNoLinkedCustomerProceeds(t *testing.T) { + srv := testServer(t) + fake := &fakeSidecar{err: &billing.SidecarError{Status: http.StatusBadRequest, Body: `{"error":"customer_id must start with 'cus_'"}`}} + srv.SetCloudSidecar(fake) + + userID, token := bootstrapAccountDeleteUser(t, srv, "") + + rr := deleteAccountReq(srv, map[string]interface{}{"password": "correct-horse-battery-staple"}, token) + if rr.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + if u, _ := srv.store.GetUser(userID); u != nil { + t.Error("expected user to be deleted") + } +} + +// Any other failure with no linked customer still aborts: a stamped customer +// may exist, and deleting the account would leave it charging. +func TestHandleDeleteAccount_NoLinkedCustomerSidecar5xxAborts(t *testing.T) { + srv := testServer(t) + fake := &fakeSidecar{err: &billing.SidecarError{Status: http.StatusInternalServerError, Body: `{"error":"boom"}`}} + srv.SetCloudSidecar(fake) + + userID, token := bootstrapAccountDeleteUser(t, srv, "") + + rr := deleteAccountReq(srv, map[string]interface{}{"password": "correct-horse-battery-staple"}, token) + if rr.Code != http.StatusInternalServerError { + t.Fatalf("expected 500, got %d: %s", rr.Code, rr.Body.String()) + } + if u, _ := srv.store.GetUser(userID); u == nil { + t.Error("user must still exist after an aborted delete") } } -// TestHandleDeleteAccount_SkipsWhenNoSidecarConfigured verifies that a -// self-hosted deploy (no CloudSidecar wired) lets deletes complete for -// paying users without blowing up on nil dereference. In practice this -// arrangement shouldn't exist (if the user has a Stripe customer ID, -// cloud mode was configured at some point) but it's the graceful-fallback -// contract: missing sidecar ≠ broken deletes. func TestHandleDeleteAccount_SkipsWhenNoSidecarConfigured(t *testing.T) { srv := testServer(t) // No SetCloudSidecar — the reverse hook is nil. diff --git a/internal/server/handlers_admin_billing_test.go b/internal/server/handlers_admin_billing_test.go index a0fb3ed69..29a9c15aa 100644 --- a/internal/server/handlers_admin_billing_test.go +++ b/internal/server/handlers_admin_billing_test.go @@ -23,7 +23,7 @@ type fakeBillingSidecar struct { err error } -func (f *fakeBillingSidecar) CancelCustomer(string) error { +func (f *fakeBillingSidecar) CancelCustomer(string, string) error { return nil // unused by these tests } diff --git a/internal/server/server.go b/internal/server/server.go index 12c44ce0d..ce5d0d261 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -891,7 +891,7 @@ type CloudSidecar interface { // misconfig), or 5xx (upstream breakage). Continuing after an error // would wipe the user's StripeCustomerID while leaving the subscription // billing, which is exactly the regression TASK-690 exists to prevent. - CancelCustomer(customerID string) error + CancelCustomer(customerID, userID string) error // GetBillingMetrics fetches an aggregated Stripe-derived snapshot from // pad-cloud's /admin/metrics/billing endpoint (active subs, MRR, ARR,