Deny once the overage cap is spent - #31
Conversation
Overage previously failed open with no floor: enabled meant the balance stopped gating the check entirely. The cap moves the floor to -cap rather than removing it, so a company consumes past zero until it has accrued the configured limit and is then denied, the way an exhausted balance denies with overage off. An absent cap stays uncapped, which is the behaviour that shipped first.
Overage has three states per credit — off, on-and-uncapped, on-and-capped — and the enabled-set plus cap-map spelling could express combinations that mean nothing: a cap on a credit that is not enabled, or enabled with no entry either side. Nothing prevented the two disagreeing. One map keyed by credit makes those unrepresentable: absent is off, a nil value is uncapped, and a set value is the cap. The check is one lookup rather than two. This replaces credit_overage_enabled from #30 rather than adding alongside it. Nothing consumes that field yet, so there is no reason to ship both and deprecate one later.
82f417b to
6c7f048
Compare
golangci-lint's predeclared linter flags 'cap' shadowing the builtin.
bpapillon
left a comment
There was a problem hiding this comment.
How confident are we in our serialization's ability to distinguish "no overage configuration" (meaning credit checks gate at 0 balance) from a present-but-null configuration (meaning don't gate credit checks)? This would probably live in the other repo, but I wonder if we have tests to ensure this is bulletproof, since it seems like a possible footgun with major repercussions
| return true, nil | ||
| } | ||
|
|
||
| return creditBalance > -*overageCap, nil |
There was a problem hiding this comment.
I think we need to take into account our cost/quantity options (blocks below). We could refactor everything here into a function that just returns a cost:
// Precedence on credit-balance conditions, most specific first. No
// options supplied falls through to the legacy single-unit check.
// 1. creditCost[credit_id]: caller-supplied per-call cost in credits;
// gate on balance >= cost.
// 2. eventUsage, when its event_subtype matches the condition's:
// simulated quantity for this specific event; gate on
// balance >= quantity × consumption_rate.
// 3. usage: generic quantity (no event disambiguation); gate on
// balance >= quantity × consumption_rate.
// 4. Legacy: balance >= consumption_rate (single unit).
if cost, ok := scope.creditCost[*condition.CreditID]; ok {
return creditBalance >= cost, nil
}
if eu := scope.eventUsage; eu != nil && condition.EventSubtype != nil &&
eu.eventSubtype == *condition.EventSubtype && eu.quantity > 0 {
return creditBalance >= float64(eu.quantity)*consumptionRate, nil
}
if scope.usage != nil && *scope.usage > 0 {
return creditBalance >= float64(*scope.usage)*consumptionRate, nil
}
Then, we check for overage configuration and, if present and unlimited, simply return open (since we would never gate someone in this situation, they are free to run up as large of a negative balance as they like). Set a "cap" from that as well, and then do a comparison like creditBalance + cap >= cost (where cap would just be 0 in the case that no overage configuration exists).
The overage branch returned before the cost/quantity precedence below it, so the cap was compared against the balance as it stood rather than as it would stand after the call. That enforced the cap only to within one call's cost: a company 5 credits short of a 100-credit cap passed a call costing 50 and landed 45 past it. Cost is now resolved first and overage shifts the floor it is measured against: balance + allowance >= cost. Uncapped still returns before the comparison, and with no overage the allowance is zero, which is exactly the balance >= cost check that has always applied. Adds the capped-plus-cost cases that were missing — every existing overage test used the legacy single-unit path, which is why this got through — and the serialization round-trip proving absent, present-and-null, and present-and-set stay distinct in both directions.
Follow-up to #30, which added the overage opt-in but had no ceiling: enabled meant the balance stopped gating the credit check entirely.
The cap moves the floor rather than removing it. With overage on, the balance may run down to
-capand the check denies beyond that — the same hard stop an exhausted balance gives with overage off. An absent cap stays uncapped, which is the behaviour #30 shipped.Company.CreditOverageCapsis keyed by billing credit ID, likeCreditBalancesandCreditOverageEnabled, and only means anything where the latter is true.This is what the acceptance review asked for: the API previously precomputed "cap reached" and flipped the overage flag off, which put the decision in two places and meant the engine inherited whatever the payload was built with. Now the engine answers it at check time.
Five subtests cover inside the cap, exactly at it, past it, a cap on a different credit not leaking, and no cap meaning uncapped.
rulesengine-rustcarries the identical change and mirrors these cases one for one, since the two must agree (SCHY-515).