feat(deleter): clear or report org delete blockers up front - #1857
feat(deleter): clear or report org delete blockers up front#1857whoAbhishekSah wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughOrganization deletion now performs billing preflight checks, aggregates structured blockers, and accepts token-forfeit acknowledgement. Billing-account deletion audits forfeited tokens. The Connect API returns structured failed-precondition details for blocked deletions. ChangesOrganization deletion workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
29a0e18 to
783da24
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 51d78c26-5a73-43e9-8050-b9aec5ac46cb
⛔ Files ignored due to path filters (1)
proto/v1beta1/frontier.pb.gois excluded by!**/*.pb.go,!proto/**
📒 Files selected for processing (13)
Makefilebilling/invoice/invoice.gocore/audit/audit.gocore/deleter/deleter.gocore/deleter/mocks/credit_service.gocore/deleter/mocks/subscription_service.gocore/deleter/service.gocore/deleter/service_test.gogo.modinternal/api/v1beta1connect/deleter.gointernal/api/v1beta1connect/deleter_test.gointernal/api/v1beta1connect/interfaces.gointernal/api/v1beta1connect/mocks/cascade_deleter.go
783da24 to
3c17128
Compare
3c17128 to
969ef16
Compare
whoAbhishekSah
changed the title
Coverage Report for CI Build 32450484349Coverage increased (+0.1%) to 48.919%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
969ef16 to
7117b6a
Compare
7117b6a to
b479ddd
Compare
b479ddd to
2711e64
Compare
efd1fca to
0d92025
Compare
0d92025 to
f8575b8
Compare
whoAbhishekSah
left a comment
whoAbhishekSah
left a comment
There was a problem hiding this comment.
Reviewed the deleter changes; 10 inline notes below. The two biggest for me: provider-drifted subs making an org permanently undeletable, and draft invoices slipping through the blocker check and getting discarded by the customer delete. None of these are fixed by the later commits in the stack.
f8575b8 to
3b1e2c8
Compare
rohilsurana
left a comment
rohilsurana
left a comment
There was a problem hiding this comment.
Stack-aware review (base org-delete-preflight-mocks). A few things to flag. The invoice one is the most important; the rest are smaller. I dropped two earlier notes after reading the stack: the empty-PlanID-is-paid case is a documented choice, and the mid-teardown balance read is reworked in #1880.
3b1e2c8 to
4664db3
Compare
4664db3 to
1d55f24
Compare
|
Round 3 sandbox results — retesting the behavior changes from the review (stack: #1857 → #1880 → #1894, tested at the tip 1. Credit-note invoice no longer blocks. Draft invoice with total −500 (amount_remaining 0). 2. Credit-balance-covered invoice no longer blocks. Customer with a 1000 credit balance and a draft invoice of total 800 (amount_due 0 on Stripe). Check passed and the delete succeeded. 3. Dangling plan yields a blocker, not an error. Subscription row pointing at a plan id that no longer exists (a real Stripe sub behind it). Check and delete both returned the 4. Cancel pass is two-phase. One org, two billing accounts: A with an active free-plan subscription, B with an open unpaid invoice. The delete blocked on B's invoice and A's subscription stayed active on Stripe and in the local rows. After voiding the invoice, the retry deleted the org and canceled A's subscription itself. Both Stripe customers were removed. 5. Retry recovers the forfeit notice from audit. Org with zero live balance plus an All test orgs, billing accounts, and Stripe customers were cleaned up afterwards. 🤖 Generated with Claude Code |
rohilsurana
left a comment
rohilsurana
left a comment
There was a problem hiding this comment.
Re-review after the fix commits. The earlier points are handled well: AmountRemaining for the filter, the unresolvablePlanBlocker for dangling plans, the judge-before-cancel restructure, and the test comment. A few new things the fixes surfaced, plus two small doc nits.
| // a negative total is a credit owed to the customer, and an open | ||
| // invoice fully covered by their credit balance has a positive | ||
| // total with nothing due. Neither asks for money. | ||
| if stripeInvoice.AmountRemaining <= 0 { |
There was a problem hiding this comment.
The filter now skips any invoice with AmountRemaining <= 0 for all three statuses, including draft. That is right for open and uncollectible, but a draft can have amount_remaining = 0 while it is still being finalized, with the real charge sitting in its total or line items. The invoiceBlockers docstring says drafts must block precisely so a pending charge is not silently lost, so this may now drop the very drafts it means to keep. Worth confirming what Stripe returns for amount_remaining on an unfinalized draft. If it is not reliable there, keep drafts blocking on total > 0 and use AmountRemaining only for open and uncollectible.
| // the length of one delete. A subscription without a plan counts as paid: | ||
| // blocking it is recoverable, canceling it is not. | ||
| func (d Service) isPaidPlan(ctx context.Context, planID string, cache map[string]bool) (bool, error) { | ||
| if planID == "" { |
There was a problem hiding this comment.
An empty PlanID returns paid = true here, so subscriptionBlockers emits a paidSubscriptionBlocker whose message tells the user to downgrade to the standard plan. There is no plan to downgrade. Now that unresolvablePlanBlocker exists with a clearer message (cancel the subscription), an empty PlanID reads better sent down that path so the guidance is actually doable.
| return fmt.Errorf("%s: %w", err.Error(), ErrDeleteNotAllowed) | ||
| // an org that is already gone has nothing left to check or tear down; | ||
| // disabled orgs stay deletable | ||
| if _, err := d.orgService.Get(ctx, id); err != nil && !errors.Is(err, organization.ErrDisabled) { |
There was a problem hiding this comment.
This preflight returns the error for any Get failure other than ErrDisabled. For an org whose row was already fully deleted (a retry after a delete that succeeded but whose response was lost), Get returns ErrNotExist and the whole call now fails with NotFound. The docstring just above says every step treats already-deleted data as success and the delete can simply be run again, so this quietly breaks that idempotency for the already-gone case. Treating ErrNotExist here as success would keep the retry idempotent.
| // are skipped. Where a local row exists for the invoice it keeps its local | ||
| // id; an invoice the sync has not seen yet is returned with an empty id and | ||
| // only its provider reference. Unlike SyncWithProvider this touches no local | ||
| // rows and reads only three small filtered pages, so it is cheap enough for |
There was a problem hiding this comment.
Small doc point: this says it reads only three small filtered pages and is cheap enough for a request path, but the Next() loop below auto-paginates through every page for each of the three statuses. For a customer with many open invoices that is more than three requests. Worth softening the comment so a later caller does not assume a bounded cost.
| return fmt.Errorf("%s: %w", err.Error(), ErrDeleteNotAllowed) | ||
| // an org that is already gone has nothing left to check or tear down; | ||
| // disabled orgs stay deletable | ||
| if _, err := d.orgService.Get(ctx, id); err != nil && !errors.Is(err, organization.ErrDisabled) { |
There was a problem hiding this comment.
This preflight returns the error for any Get failure other than ErrDisabled. For an org whose row was already fully deleted (a retry after a delete that succeeded but whose response was lost), Get returns ErrNotExist and the whole call now fails with NotFound. The docstring just above says every step treats already-deleted data as success and the delete can simply be run again, so this quietly breaks that idempotency for the already-gone case. Treating ErrNotExist here as success would keep the retry idempotent.
| // are skipped. Where a local row exists for the invoice it keeps its local | ||
| // id; an invoice the sync has not seen yet is returned with an empty id and | ||
| // only its provider reference. Unlike SyncWithProvider this touches no local | ||
| // rows and reads only three small filtered pages, so it is cheap enough for |
There was a problem hiding this comment.
Small doc point: this says it reads only three small filtered pages and is cheap enough for a request path, but the Next() loop below auto-paginates through every page for each of the three statuses. For a customer with many open invoices that is more than three requests. Worth softening the comment so a later caller does not assume a bounded cost.
1d55f24 to
896fb62
Compare
896fb62 to
b34ac05
Compare
The delete first checks everything that blocks it and returns all the reasons together as one failed_precondition response: a running subscription on a paid plan (the caller downgrades it to the standard plan), invoices that still ask for money, and a negative token balance which support has to settle. Plans resolve lazily, only when a running subscription references one. When nothing blocks, subscriptions still running on a free plan are canceled immediately with unbilled usage invoiced on the spot — tolerating copies already gone on the provider — and the invoice check runs again so a final invoice still blocks. The plan is judged again in that pass, so a paid subscription created mid-delete blocks instead of being canceled. Unused tokens do not block: the delete forfeits them and writes the amount to an audit record. An already-deleted org returns not found before any checks run. Invoices are judged straight from the billing provider through a new cheap ListPayableOnProvider (three status-filtered pages, no local writes) instead of a full sync, and drafts with a non-zero amount now block too: the provider finalizes them shortly, and deleting inside that window would silently lose the charge.
b34ac05 to
5172886
Compare
3 participants
Closes #1837. Based on #1865; the forfeit email (#1880) and the check RPC (#1893/#1894) stack on top.
DeleteOrganizationnow checks everything that blocks the delete before touching any data, and returns every reason together as onefailed_preconditionresponse with aPreconditionFailuredetail (one violation per blocker). The proto ack field from the issue text was dropped (raystack/proton#497 closed) — unused tokens never block.What blocks the delete:
ACTIVE_SUBSCRIPTION): any running subscription whose plan has a priced product. The caller downgrades it to the standard plan through the normal plan-change flow, then retries. Plans resolve lazily — only when a running subscription references one — so a broken plan reference can only affect orgs that actually hold such a subscription.UNPAID_INVOICE): open and uncollectible invoices the caller can pay, and drafts — the provider finalizes those shortly, and deleting inside that window would silently lose the charge, so they block until payable. "Asks for money" is judged by the provider'samount_remaining, so a negative-total credit note or an invoice fully covered by the customer's credit balance never blocks.NEGATIVE_TOKEN_BALANCE): the account owes tokens (overdraft). Support settles it.What does not block:
app.billing.tokens.forfeitedaudit record. feat(deleter): email the org owners when a delete forfeits tokens #1880 adds the owner email.Other changes:
invoice.ListPayableOnProvider— three status-filtered pages, no expansion, no local writes — instead of a fullSyncWithProvider, keeping the delete's latency independent of the org's invoice history. An invoice the sync has not stored yet is reported by its provider reference.not_foundbefore any checks run. Disabled orgs stay deletable.UncollectibleState; first use oferrdetailsin the repo (google.golang.org/genproto/googleapis/rpcbecomes a direct dependency).🤖 Generated with Claude Code