Skip to content

Deny once the overage cap is spent - #31

Merged
cbrady merged 4 commits into
mainfrom
chris/schx-582-overage-cap
Sep 4, 2026
Merged

Deny once the overage cap is spent#31
cbrady merged 4 commits into
mainfrom
chris/schx-582-overage-cap

Conversation

@cbrady

@cbrady cbrady commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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 -cap and 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.CreditOverageCaps is keyed by billing credit ID, like CreditBalances and CreditOverageEnabled, 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-rust carries the identical change and mirrors these cases one for one, since the two must agree (SCHY-515).

@cbrady cbrady self-assigned this Sep 3, 2026
@cbrady
cbrady requested a review from bpapillon September 3, 2026 18:51
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.
@cbrady
cbrady force-pushed the chris/schx-582-overage-cap branch from 82f417b to 6c7f048 Compare September 3, 2026 18:52
golangci-lint's predeclared linter flags 'cap' shadowing the builtin.

@bpapillon bpapillon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread rulecheck.go Outdated
return true, nil
}

return creditBalance > -*overageCap, nil

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@cbrady
cbrady requested a review from bpapillon September 3, 2026 21:01
@cbrady
cbrady merged commit 3cc81ba into main Sep 4, 2026
2 checks passed
@cbrady
cbrady deleted the chris/schx-582-overage-cap branch September 4, 2026 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants