feat[charges]: skaffold progressive billing support#4203
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughSplit usage-based invoice lifecycle into partial vs final flows: new triggers and run types, added partial-invoice substates, consolidated payment settlement into an awaiting-settlement state, added validation to prevent concurrent realization runs, and implemented per-line payment recording with tests and docs updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LineEngine
participant StateMachine as CreditThenInvoice<br/>StateMachine
participant RunSvc as RealizationRun<br/>Service
participant PaymentSvc as Payment<br/>Recording
Client->>LineEngine: OnStandardInvoiceCreated(invoice, lines)
LineEngine->>LineEngine: resolveInvoiceCreatedTrigger()
alt Partial invoice (billed < service period)
LineEngine->>StateMachine: Fire TriggerPartialInvoiceCreated
StateMachine->>RunSvc: StartPartialInvoiceRun(...)
RunSvc-->>StateMachine: create partial realization
StateMachine->>StateMachine: enter partial_waiting_for_collection
else Final invoice (billed == service period)
LineEngine->>StateMachine: Fire TriggerFinalInvoiceCreated
StateMachine->>RunSvc: StartFinalInvoiceRun(...)
RunSvc-->>StateMachine: create final realization
StateMachine->>StateMachine: enter final_waiting_for_collection
end
Client->>LineEngine: OnPaymentAuthorized(invoice, lines)
LineEngine->>PaymentSvc: recordPaymentAuthorized(...)
PaymentSvc->>RunSvc: BookInvoicedPaymentAuthorized(...)
Client->>LineEngine: OnPaymentSettled(invoice, lines)
LineEngine->>PaymentSvc: recordPaymentSettled(...)
PaymentSvc->>RunSvc: SettleInvoicedPayment(...)
PaymentSvc->>PaymentSvc: areAllInvoicedRunsSettled()?
alt All runs settled
PaymentSvc->>StateMachine: Fire TriggerAllPaymentsSettled
StateMachine->>StateMachine: transition to final
else Some runs pending
StateMachine->>StateMachine: remain awaiting_payment_settlement
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
9c0da77 to
7bd337f
Compare
7bd337f to
db51c1d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (8)
openmeter/billing/charges/usagebased/service/run/create.go (1)
36-38: Use the typed validation issue here too.This guard mirrors
ErrActiveRealizationRunAlreadyExists, but currently returns a plain error. DirectCreateRatedRuncallers would lose the stable error code / HTTP 400 mapping.♻️ Proposed consistency tweak
if i.Charge.State.CurrentRealizationRunID != nil { - return fmt.Errorf("charge: current realization run already exists [charge_id=%s]", i.Charge.GetChargeID()) + return usagebased.ErrActiveRealizationRunAlreadyExists.WithAttrs(models.Attributes{ + "charge_id": i.Charge.ID, + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/run/create.go` around lines 36 - 38, Replace the plain fmt.Errorf return in the guard that checks i.Charge.State.CurrentRealizationRunID inside CreateRatedRun with the typed validation issue ErrActiveRealizationRunAlreadyExists (or the package's validation issue constructor) so callers get the stable error code/HTTP 400 mapping; include the charge id as context/meta on that validation issue (using the same message or metadata currently passed to fmt.Errorf) instead of returning a raw error string.openmeter/billing/charges/service/usagebased_test.go (3)
594-617: Tiny shadow onpartialInvoiceinside the approval sub-run.Line 608 uses
:=with(partialInvoice, err), and sincepartialInvoicefrom the outer function scope isn't in the same lexical scope as thiss.Run(...)closure,:=creates a new localpartialInvoicethat shadows the outer one. It doesn't break behavior today (no later sub-run reads the outerpartialInvoiceafter this), but it's inconsistent with the earlier sub-runs (lines 541 and 547) that use=to reassign the outer. Swapping to=would also catch the case if someone later tries to use the approved invoice in a subsequent sub-run:🪞 Nit fix
- partialInvoice, err := s.BillingService.ApproveInvoice(ctx, partialInvoice.GetInvoiceID()) + var err error + partialInvoice, err = s.BillingService.ApproveInvoice(ctx, partialInvoice.GetInvoiceID())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/usagebased_test.go` around lines 594 - 617, The test creates a new local variable by shadowing the outer partialInvoice when calling s.BillingService.ApproveInvoice(...) inside the s.Run(...) closure (using :=); change that declaration to use assignment (=) so the outer partialInvoice is reassigned instead of shadowed, keeping behavior consistent with the earlier sub-runs and preventing future bugs if later assertions reference the approved invoice (also ensure you update the err handling to use = for err if needed).
169-173: Consider explicitStoredAton ingested events.These
MockStreamingConnector.AddSimpleEvent(...)calls set only the event time, lettingStoredAtdefault to whatever the mock picks. That's fine for lifecycle-progression assertions, but the usage-based finalization path is specifically gated onstored_at < cutoff(where cutoff isclock.Now() - InternalCollectionPeriod), and the coding guideline asks for explicitStoredAtin billing tests so we exercise that cutoff on purpose. Since you're already freezingclockper sub-run, layering instreaming/testutils.WithStoredAt(...)(or the equivalent helper that takes explicit stored-at) on at least the mid-period event would make the intent crisper and protect against future cutoff regressions:s.MockStreamingConnector.AddSimpleEvent( meterSlug, 15, datetime.MustParseTimeInLocation(s.T(), "2026-01-15T00:00:00Z", time.UTC).AsTime(), // WithStoredAt(...) so the stored-at cutoff behavior is exercised explicitly. )As per coding guidelines: "Use
MockStreamingConnectorwith explicitStoredAtvalues in usage-based billing tests to exercise stored-at cutoff logic in finalization".Also applies to: 225-229, 305-309, 525-529, 564-568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/usagebased_test.go` around lines 169 - 173, The test uses MockStreamingConnector.AddSimpleEvent(...) without explicit StoredAt which leaves stored_at to the mock's default and fails to exercise the stored_at < cutoff path used by the usage-based finalization (cutoff = clock.Now() - InternalCollectionPeriod). Update the AddSimpleEvent calls (e.g., the ones at 169-173 and also at 225-229, 305-309, 525-529, 564-568) to pass an explicit StoredAt timestamp (use streaming/testutils.WithStoredAt or the equivalent helper) for at least the mid-period event(s) so the stored_at cutoff logic (stored_at < cutoff) is triggered deterministically against the frozen clock; keep event time the same but set StoredAt relative to clock.Now() - InternalCollectionPeriod to cover both sides of the cutoff as needed.
322-322: TODO left in the test — intentional?The
// TODO[rating]: totals are off due to rating not yet supporting progressive billing via chargescomment is right where one might expect aRequireTotalsassertion onfinalInvoice. Just confirming this is intended to land as-is (tracked elsewhere) rather than getting a follow-up ticket, since without the totals check this sub-run's behavioral coverage on the final invoice is thinner than the mid-period one. Happy to help open an issue to track the rating work if useful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/usagebased_test.go` at line 322, The test currently omits a totals assertion for finalInvoice (see finalInvoice and the missing RequireTotals call), so either restore the missing assertion or track it with a follow-up ticket: open an issue for the rating/progressive-billing work, then update the inline TODO to reference that issue (e.g., TODO[rating]: totals are off — tracked in ISSUE-123) and add a short comment explaining why RequireTotals was removed; alternatively, if you want the test to fail when behavior changes, re-add the RequireTotals(finalInvoice, ...) assertion in usagebased_test.go but guard it with a clear TODO and/or t.Skip with the issue ID so the intent and tracking are explicit.openmeter/billing/charges/usagebased/service/payments.go (1)
123-141:areAllInvoicedRunsSettledsemantics look right, but worth a doc line.The "must have at least one invoiced final-realization run" guard is subtle and not obvious from the function name alone — it returns
falseif there are only invoiced partial runs that are all settled. That matches the state-machine intent (StatusActiveAwaitingPaymentSettlement → StatusFinalmust wait for the final invoice too), but a one-line comment explaining "requires the final run to be present and all invoiced runs settled" would help the next reader:📝 Optional comment
+// areAllInvoicedRunsSettled returns true only when (a) at least one final-realization +// run has been invoiced, and (b) every invoiced run (partial or final) is payment-settled. func areAllInvoicedRunsSettled(charge usagebased.Charge) bool {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/payments.go` around lines 123 - 141, The function areAllInvoicedRunsSettled has subtle semantics (it returns false unless there is at least one invoiced final-realization run and all invoiced runs are settled); add a one-line doc comment immediately above the areAllInvoicedRunsSettled function explaining that it requires the presence of an invoiced final realization (usagebased.RealizationRunTypeFinalRealization) and that it only returns true when every run with InvoiceUsage has a Payment with StatusSettled, referencing the function name areAllInvoicedRunsSettled and the payment.StatusSettled and usagebased.RealizationRunTypeFinalRealization symbols so future readers understand the guard.openmeter/billing/charges/usagebased/service/lineengine_test.go (1)
53-87: Test name promises more than it asserts.The test is called "KeepsChargeGrouping...ButWithoutChildReferences", but we never actually assert that
ChargeIDis preserved on eitherPreSplitAtLineorPostSplitAtLine. Also, since the inputlinenever setsSplitLineGroupID, therequire.Nil(..., SplitLineGroupID)assertions on lines 79 and 84 are trivially satisfied — a regression where split forgets to clear it wouldn't fail this test.Consider either setting
SplitLineGroupIDon the input and asserting it's cleared, or adding aChargeIDequality check to actually lock down "charge grouping is kept":🧪 Proposed tightening
line := billing.GatheringLine{ GatheringLineBase: billing.GatheringLineBase{ ... ChargeID: lo.ToPtr("01K00000000000000000000002"), ChildUniqueReferenceID: lo.ToPtr("original"), + SplitLineGroupID: lo.ToPtr("01K00000000000000000000003"), }, } ... require.Nil(t, result.PreSplitAtLine.SplitLineGroupID) require.Nil(t, result.PreSplitAtLine.ChildUniqueReferenceID) + require.Equal(t, line.ChargeID, result.PreSplitAtLine.ChargeID) require.Equal(t, splitAt, result.PreSplitAtLine.ServicePeriod.To) require.NotNil(t, result.PostSplitAtLine) require.Nil(t, result.PostSplitAtLine.SplitLineGroupID) require.Nil(t, result.PostSplitAtLine.ChildUniqueReferenceID) + require.Equal(t, line.ChargeID, result.PostSplitAtLine.ChargeID) require.Equal(t, splitAt, result.PostSplitAtLine.ServicePeriod.From)Heads up: if you add the
SplitLineGroupIDinput bit, double-check thatSplitGatheringLineactually clears it — from the snippet oflineengine.goit only clearsChildUniqueReferenceID, so this could surface a real gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/lineengine_test.go` around lines 53 - 87, The test name promises ensuring charge grouping is preserved without child references but it never asserts ChargeID and uses a nil SplitLineGroupID input (making nil assertions vacuous); update the test by setting line.SplitLineGroupID to a non-nil value in the input GatheringLine and add assertions that result.PreSplitAtLine.ChargeID and result.PostSplitAtLine.ChargeID equal the input ChargeID, and assert that result.PreSplitAtLine.SplitLineGroupID and result.PostSplitAtLine.SplitLineGroupID are cleared (nil) after SplitGatheringLine; also keep or adjust assertions for ChildUniqueReferenceID to reflect that SplitGatheringLine clears ChildUniqueReferenceID per the SplitGatheringLine implementation.openmeter/billing/charges/usagebased/service/lineengine.go (1)
413-424: Small error-context nit onisUsageBasedSplitPeriodEmpty.When
price == nilwe bubble up"price is nil"with no line/charge context, which will be painful to diagnose if it ever fires fromSplitGatheringLine. The callers at lines 62 and 79 do wrap with "checking if post/pre split line is empty", so it's not catastrophic, but adding the line ID (or just returningnilfor "not empty" in this case) would be friendlier:🏷️ Optional tweak
-func isUsageBasedSplitPeriodEmpty(line billing.GatheringLine) (bool, error) { - price := line.GetPrice() - if price == nil { - return false, fmt.Errorf("price is nil") - } +func isUsageBasedSplitPeriodEmpty(line billing.GatheringLine) (bool, error) { + price := line.GetPrice() + if price == nil { + return false, fmt.Errorf("usage based gathering line[%s]: price is nil", line.ID) + }Also worth a quick sanity check: a usage-based gathering line with a nil price shouldn't exist in practice, right? If so, this branch is more of a defensive assertion — happy to leave it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/lineengine.go` around lines 413 - 424, The error returned from isUsageBasedSplitPeriodEmpty when price == nil lacks context; update isUsageBasedSplitPeriodEmpty to include the gathering line identifier in the error message (e.g., use line.GetId() or line.GetLineId()) so callers like SplitGatheringLine will get useful context, or alternatively return false (treat as "not empty") if you prefer a defensive no-op; modify the branch that currently returns fmt.Errorf("price is nil") to produce a contextual error string referencing the line ID or to return (false, nil) depending on the chosen behavior.openmeter/billing/charges/usagebased/service/creditheninvoice.go (1)
263-269:resolveInvoiceCreatedTrigger— worth asserting the happy-path assumption.This picks
TriggerFinalInvoiceCreatedonly on exact equality of the normalized end timestamps, otherwise falls back to partial. That means ifbilledPeriod.Toever drifts pastcharge.Intent.ServicePeriod.To(e.g. a boundary rounding bug upstream), we'd quietly route it as a partial invoice — which the downstreamStatusActivePartialInvoice*states would then try to own. Probably impossible in practice given the truncation rules, but a small defensive tweak would make the intent crisper:🧭 Optional defensive tweak
func resolveInvoiceCreatedTrigger(charge usagebased.Charge, billedPeriod timeutil.ClosedPeriod) meta.Trigger { - if meta.NormalizeTimestamp(billedPeriod.To).Equal(meta.NormalizeTimestamp(charge.Intent.ServicePeriod.To)) { + if !meta.NormalizeTimestamp(billedPeriod.To).Before(meta.NormalizeTimestamp(charge.Intent.ServicePeriod.To)) { return meta.TriggerFinalInvoiceCreated } return meta.TriggerPartialInvoiceCreated }Feel free to ignore if upstream invariants already forbid
billedPeriod.To > servicePeriod.To.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/creditheninvoice.go` around lines 263 - 269, Add a defensive assertion in resolveInvoiceCreatedTrigger: normalize both billedPeriod.To and charge.Intent.ServicePeriod.To (using meta.NormalizeTimestamp), then if billed > service, either log/error or panic to surface the invariant breach instead of silently treating it as partial; otherwise keep the exact-equality check and return meta.TriggerFinalInvoiceCreated or meta.TriggerPartialInvoiceCreated as before. This uses resolveInvoiceCreatedTrigger, billedPeriod.To and charge.Intent.ServicePeriod.To to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/billing/charges/service/usagebased_test.go`:
- Around line 594-617: The test creates a new local variable by shadowing the
outer partialInvoice when calling s.BillingService.ApproveInvoice(...) inside
the s.Run(...) closure (using :=); change that declaration to use assignment (=)
so the outer partialInvoice is reassigned instead of shadowed, keeping behavior
consistent with the earlier sub-runs and preventing future bugs if later
assertions reference the approved invoice (also ensure you update the err
handling to use = for err if needed).
- Around line 169-173: The test uses MockStreamingConnector.AddSimpleEvent(...)
without explicit StoredAt which leaves stored_at to the mock's default and fails
to exercise the stored_at < cutoff path used by the usage-based finalization
(cutoff = clock.Now() - InternalCollectionPeriod). Update the AddSimpleEvent
calls (e.g., the ones at 169-173 and also at 225-229, 305-309, 525-529, 564-568)
to pass an explicit StoredAt timestamp (use streaming/testutils.WithStoredAt or
the equivalent helper) for at least the mid-period event(s) so the stored_at
cutoff logic (stored_at < cutoff) is triggered deterministically against the
frozen clock; keep event time the same but set StoredAt relative to clock.Now()
- InternalCollectionPeriod to cover both sides of the cutoff as needed.
- Line 322: The test currently omits a totals assertion for finalInvoice (see
finalInvoice and the missing RequireTotals call), so either restore the missing
assertion or track it with a follow-up ticket: open an issue for the
rating/progressive-billing work, then update the inline TODO to reference that
issue (e.g., TODO[rating]: totals are off — tracked in ISSUE-123) and add a
short comment explaining why RequireTotals was removed; alternatively, if you
want the test to fail when behavior changes, re-add the
RequireTotals(finalInvoice, ...) assertion in usagebased_test.go but guard it
with a clear TODO and/or t.Skip with the issue ID so the intent and tracking are
explicit.
In `@openmeter/billing/charges/usagebased/service/creditheninvoice.go`:
- Around line 263-269: Add a defensive assertion in
resolveInvoiceCreatedTrigger: normalize both billedPeriod.To and
charge.Intent.ServicePeriod.To (using meta.NormalizeTimestamp), then if billed >
service, either log/error or panic to surface the invariant breach instead of
silently treating it as partial; otherwise keep the exact-equality check and
return meta.TriggerFinalInvoiceCreated or meta.TriggerPartialInvoiceCreated as
before. This uses resolveInvoiceCreatedTrigger, billedPeriod.To and
charge.Intent.ServicePeriod.To to locate the change.
In `@openmeter/billing/charges/usagebased/service/lineengine_test.go`:
- Around line 53-87: The test name promises ensuring charge grouping is
preserved without child references but it never asserts ChargeID and uses a nil
SplitLineGroupID input (making nil assertions vacuous); update the test by
setting line.SplitLineGroupID to a non-nil value in the input GatheringLine and
add assertions that result.PreSplitAtLine.ChargeID and
result.PostSplitAtLine.ChargeID equal the input ChargeID, and assert that
result.PreSplitAtLine.SplitLineGroupID and
result.PostSplitAtLine.SplitLineGroupID are cleared (nil) after
SplitGatheringLine; also keep or adjust assertions for ChildUniqueReferenceID to
reflect that SplitGatheringLine clears ChildUniqueReferenceID per the
SplitGatheringLine implementation.
In `@openmeter/billing/charges/usagebased/service/lineengine.go`:
- Around line 413-424: The error returned from isUsageBasedSplitPeriodEmpty when
price == nil lacks context; update isUsageBasedSplitPeriodEmpty to include the
gathering line identifier in the error message (e.g., use line.GetId() or
line.GetLineId()) so callers like SplitGatheringLine will get useful context, or
alternatively return false (treat as "not empty") if you prefer a defensive
no-op; modify the branch that currently returns fmt.Errorf("price is nil") to
produce a contextual error string referencing the line ID or to return (false,
nil) depending on the chosen behavior.
In `@openmeter/billing/charges/usagebased/service/payments.go`:
- Around line 123-141: The function areAllInvoicedRunsSettled has subtle
semantics (it returns false unless there is at least one invoiced
final-realization run and all invoiced runs are settled); add a one-line doc
comment immediately above the areAllInvoicedRunsSettled function explaining that
it requires the presence of an invoiced final realization
(usagebased.RealizationRunTypeFinalRealization) and that it only returns true
when every run with InvoiceUsage has a Payment with StatusSettled, referencing
the function name areAllInvoicedRunsSettled and the payment.StatusSettled and
usagebased.RealizationRunTypeFinalRealization symbols so future readers
understand the guard.
In `@openmeter/billing/charges/usagebased/service/run/create.go`:
- Around line 36-38: Replace the plain fmt.Errorf return in the guard that
checks i.Charge.State.CurrentRealizationRunID inside CreateRatedRun with the
typed validation issue ErrActiveRealizationRunAlreadyExists (or the package's
validation issue constructor) so callers get the stable error code/HTTP 400
mapping; include the charge id as context/meta on that validation issue (using
the same message or metadata currently passed to fmt.Errorf) instead of
returning a raw error string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f6ef651-1d0c-4e9a-8d52-2c94802b5b31
⛔ Files ignored due to path filters (3)
openmeter/ent/db/chargeusagebased/chargeusagebased.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns/chargeusagebasedruns.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**
📒 Files selected for processing (15)
.agents/skills/charges/SKILL.mdopenmeter/billing/charges/meta/triggers.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/usagebased_test.goopenmeter/billing/charges/usagebased/errors.goopenmeter/billing/charges/usagebased/realizationrun.goopenmeter/billing/charges/usagebased/service/creditheninvoice.goopenmeter/billing/charges/usagebased/service/creditheninvoice_test.goopenmeter/billing/charges/usagebased/service/lineengine.goopenmeter/billing/charges/usagebased/service/lineengine_test.goopenmeter/billing/charges/usagebased/service/payments.goopenmeter/billing/charges/usagebased/service/run/create.goopenmeter/billing/charges/usagebased/service/run/payment_test.goopenmeter/billing/charges/usagebased/statemachine.gotest/credits/sanity_test.go
Overview
This PR extends usage-based charge lifecycles to support partial invoicing in the billing line engine, while keeping rating’s progressive-billing limitations explicitly out of scope.
The main architectural change is that usage-based charges can now create and manage PartialInvoiceRuns in addition to final-realization runs, and the state machine now models partial invoicing as a first-class realization
branch. This lets billing progressively invoice usage-based charges without pretending every invoice-backed run is a final realization.
Warning: Rating is broken for progressive billing/partial invoices. That will be fixed going forward in the next prs.
Why
Before this change, usage-based charges had a structural mismatch with billing:
That made partial invoicing hard to represent cleanly and forced semantics that were not true in the domain.
This PR fixes the structural model first:
Architectural Changes
Usage-based line engine:
New realization run type:
Usage-based state machine refactor:
Usage-based now enforces a strict invariant:
Payment model redesign for usage-based:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests