fix(billing): make a namespace's subscription an optional object - #6947
Merged
Conversation
The billing record kept the subscription's Stripe status in a flat `status` field, which had to carry two unrelated facts: whether a subscription exists at all, and how the subscription is doing. Customer creation wrote "inactive" there, so a namespace that added a card but never completed checkout was indistinguishable from one whose subscription had failed. `Billing` now holds an optional `Subscription`. No subscription means no status, so the ambiguous state cannot be written. The stored `Active` boolean goes with it: it duplicated `Status.IsActive()` at five write sites and could drift from the status it mirrored. `IsActive()` derives it instead, and `Clone()` gives callers a deep copy so building the next state cannot mutate the one the namespace still holds. The SSH banner handler logged a firewall block and a billing block under one message, which made a production incident readable only by grouping on the error text. Each case gets its own message and the tenant. Ref: shellhub-io/team#214
Device acceptance answered both denials with ErrDeviceLimit, so a namespace that was well under its allowance was told it had reached a limit. A customer read that literally: they rejected and deleted all three of their devices, re-registered the agents, and were refused again, because the device count was never the cause. The evaluation now carries which rule denied the device, and the two get different errors and different console copy. Both stay HTTP 402: the quota case still asks the user to free a slot or upgrade, and the subscription case says the device count is not the problem. Ref: shellhub-io/team#214
Code Review CompleteThe automated review ran but did not post an updated summary — this usually means no new issues were found since the previous review. If you've pushed changes and want a fresh pass, comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pairs with shellhub-io/cloud#2510 (same branch name). Neither side builds without the other.
Why
On Cloud, a namespace that added a payment method but never completed checkout could accept no device and open no SSH session, and the API called it
device limit reached. Its free allowance was not reduced — it was zero. See shellhub-io/team#214 for the production analysis: 124 namespaces, 118 accepted devices unreachable.The cause is in this repo's model.
Billing.Statushad to carry two unrelated facts: whether a subscription exists at all, and how it is doing. Customer creation wroteinactivethere, and every reader had to guess which fact the value meant.What changes
The subscription becomes an optional object (
pkg/models/billing.go).BillingStatusInactiveis gone. No subscription means no status, so the ambiguous state cannot be written.Activeboolean is gone. It duplicatedStatus.IsActive()at five write sites and could drift from the status it mirrored;IsActive()derives it now.Clone()deep-copies. The webhook handlers usedbilling := *namespace.Billing, which with a pointer field would share the subscription with the namespace and mutate it.HasCutomeris spelledHasCustomer.Device acceptance stops lying (
server/api/services/).BillingEvaluationnow names the rule that denied the device, and the two denials get different errors:ErrDeviceLimitErrDeviceBillingBlockedBoth stay HTTP 402. The customer in the report deleted all three of their devices because the old message told them to.
Observability (
server/ssh/). The banner handler logged a firewall block and a billing block under one message, which made the incident readable only by grouping on the error text. Each case now has its own message, and both carry the tenant.EvaluateBillinglogs the block reason.API contract.
namespaceBillingfollows the model:subscriptionnested and optional,activeremoved, timestamps optional.billingStatuskeepsinactive, because that enum is shared with the payment-gatewaysubscriptionschema, where the console uses it as its own "no subscription" placeholder.Test plan
go test ./...—server/and root: pass. New:TestValidateBillingForDeviceAcceptance,TestErrDeviceBillingBlocked.golangci-lint run ./...— 0 issues in both modules.go mod tidy— no drift.npm run build,lint,test— pass, 3149 tests.Deploy note
Cloud carries migration 003, which reshapes the stored records. It runs at store construction, under a lock, before the server serves. During a rolling deploy an old replica that reads a migrated row sees an empty status and applies the free-tier math, so the window locks nobody out.