refactor(charges): make state machine invoice effects explicit - #4886
Conversation
📝 WalkthroughWalkthroughThe PR replaces legacy charge activation APIs with explicit stable-state and invoice-patch-boundary advancement. Flat-fee and usage-based services now return invoice-aware results. Charge orchestration applies invoice patches in rounds before continuing lifecycle advancement. ChangesCharge advancement and invoice patch flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ChargeService
participant StateMachine
participant InvoiceUpdater
ChargeService->>StateMachine: Advance until patches or stable
StateMachine-->>ChargeService: Charge and invoice patches
ChargeService->>InvoiceUpdater: Apply customer-level patch batch
InvoiceUpdater-->>ChargeService: Patch batch applied
ChargeService->>StateMachine: Resume charge advancement
StateMachine-->>ChargeService: Final charge or next invoice patches
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
openmeter/billing/charges/service/patch.go (1)
125-152: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNice round-based design. Consider a safety cap on the number of rounds.
The loop is well behaved for the current lifecycles: a charge only re-enters
nextPendingAdvancementwhen it both emits patches and reportsCanAdvance, so the loop drains naturally. There is no upper bound though, and the whole thing runs inside the enclosingtransaction.Run. If a future state machine ever emits an invoice patch on a transition that loops back, this spins while holding the transaction open.A cheap
maxRoundscounter that returns a descriptive error would turn that failure mode into a clear error instead of a stuck transaction.🛡️ Sketch: bound the rounds
+// maxInvoiceEffectRounds bounds lifecycle progression so a charge that keeps +// emitting invoice effects cannot hold the enclosing transaction open forever. +const maxInvoiceEffectRounds = 32 + func (s *service) advanceChargesAndApplyInvoicePatches( ctx context.Context, customerID customer.CustomerID, pendingAdvancement map[string]InvocableCharge, invoicePatches invoiceupdater.Patches, ) (map[string]TriggerPatchResult, error) { latestResults := make(map[string]TriggerPatchResult, len(pendingAdvancement)) + rounds := 0 for len(invoicePatches) > 0 { + rounds++ + if rounds > maxInvoiceEffectRounds { + return nil, fmt.Errorf("charge advancement exceeded %d invoice-effect rounds for customer %s", maxInvoiceEffectRounds, customerID.ID) + } + if err := s.invoiceUpdater.ApplyPatches(ctx, customerID, invoicePatches); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/service/patch.go` around lines 125 - 152, Bound the round-based loop in the surrounding charge patch flow with a maxRounds counter. Increment it for each iteration of the loop and return a descriptive error when the configured limit is exceeded, while preserving the existing patch application and advancement behavior below the limit.openmeter/billing/charges/statemachine/machine.go (1)
228-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: collapse the pass-through wrapper.
AdvanceUntilInvoicePatchesOrStableonly forwards toadvanceUntilInvoicePatchesOrStable. You can move the loop body into the exported method and let the internal callers use it directly. That removes one indirection without changing behavior. Totally fine to leave as-is if you prefer the symmetry withFireAndAdvanceUntilInvoicePatchesOrStable.As per coding guidelines: "Do not extract trivial or single-use helpers unless the name captures non-obvious domain intent; inline pass-through wrappers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/statemachine/machine.go` around lines 228 - 234, Optionally remove the trivial pass-through method AdvanceUntilInvoicePatchesOrStable by moving the implementation of advanceUntilInvoicePatchesOrStable into the exported method and updating internal callers to use it directly. Preserve existing behavior and retain FireAndAdvanceUntilInvoicePatchesOrStable symmetry only if preferred.Source: Coding guidelines
openmeter/billing/charges/statemachine/machine_test.go (2)
215-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the pending-patch fire guard.
fireAndActivatenow rejects a trigger whenm.invoicePatchesis not empty (machine.go lines 149-151). No test exercises that branch. A small subtest that callsAddInvoicePatchand thenFireAndAdvanceUntilInvoicePatchesOrStablewould lock in theErrUnhandledInvoicePatchescontract and the "no transition fired" guarantee.Want me to draft it?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/statemachine/machine_test.go` around lines 215 - 264, Add a subtest in TestMachine_FireAndAdvanceUntilStable that queues an invoice patch with AddInvoicePatch before calling FireAndAdvanceUntilInvoicePatchesOrStable. Assert the call returns ErrUnhandledInvoicePatches, preserves the initial charge status, and does not fire the requested transition, covering the pending-patch guard in fireAndActivate.
394-421: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit: these two subtests sit under the wrong parent.
Both subtests call
AdvanceUntilInvoicePatchesOrStable, and their ownwhen:comments say so, but they live insideTestMachine_AdvanceUntilStable. There is already aTestMachine_AdvanceUntilInvoicePatchesOrStableat line 346. Moving them there makes failure output point at the right API. Purely cosmetic, so feel free to defer.Also applies to: 454-481
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/statemachine/machine_test.go` around lines 394 - 421, Move the subtests at the shown location, including the case around lines 454–481, from TestMachine_AdvanceUntilStable into the existing TestMachine_AdvanceUntilInvoicePatchesOrStable test. Preserve their bodies and assertions unchanged so failure output is grouped under the API they exercise.openmeter/billing/charges/service/patch_test.go (1)
87-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the "can advance without a boundary" guard.
The new invariant in
openmeter/billing/charges/service/patch.go(lines 102-107 and 143-148) rejects a result that setsCanAdvancewith no invoice patches.scriptedInvocableChargealready makes that trivial to script: returnTriggerPatchResult{CanAdvance: true}with no patches and assert the error. That locks in a rule the rest of this PR depends on.Happy to write it if useful.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/service/patch_test.go` around lines 87 - 93, Add a test case in the patch service tests using scriptedInvocableCharge that returns TriggerPatchResult with CanAdvance true and no invoice patches, then assert the patch operation returns an error. Cover the guard in the patch flow and preserve the existing batch and call-count assertions for valid cases.Source: Path instructions
openmeter/billing/charges/service/advance.go (1)
69-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: the "can advance without a boundary" invariant is written three times.
The same guard appears here, at lines 114-117 for usage-based charges, and twice in
openmeter/billing/charges/service/patch.go(lines 102-107 and 143-148). A small helper that takes the charge ID and the result would keep the invariant and its message in one place. Nice-to-have only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/service/advance.go` around lines 69 - 77, The invoice-effect boundary guard is duplicated across charge advancement and patching flows. Extract a shared helper that accepts the charge ID and mapped result, performs the InvoicePatches validation, and returns the existing error; replace the guards near the flat-fee and usage-based advancement logic and both locations in patch.go while preserving current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/billing/charges/service/helpers.go`:
- Around line 153-160: Carry the already-resolved customer override and
feature-meter snapshots from the construction site in advanceCharges into
usageBasedInvocableCharge, and pass them as hints from AdvanceCharge when
calling usageBasedService.AdvanceCharge. Update the corresponding fields and
construction in advance.go and preserve the no-hints behavior for charges
created by applyInvocableChargePatches.
In `@openmeter/billing/worker/subscriptionsync/service/creditsonly_test.go`:
- Around line 974-975: Update the assertions for finalRun.MeteredQuantity and
finalRun.CreditsAllocated.Sum() to use require.Equal with each Decimal converted
via InexactFloat64(), following the repository convention for one-off Decimal
comparisons.
---
Nitpick comments:
In `@openmeter/billing/charges/service/advance.go`:
- Around line 69-77: The invoice-effect boundary guard is duplicated across
charge advancement and patching flows. Extract a shared helper that accepts the
charge ID and mapped result, performs the InvoicePatches validation, and returns
the existing error; replace the guards near the flat-fee and usage-based
advancement logic and both locations in patch.go while preserving current
behavior.
In `@openmeter/billing/charges/service/patch_test.go`:
- Around line 87-93: Add a test case in the patch service tests using
scriptedInvocableCharge that returns TriggerPatchResult with CanAdvance true and
no invoice patches, then assert the patch operation returns an error. Cover the
guard in the patch flow and preserve the existing batch and call-count
assertions for valid cases.
In `@openmeter/billing/charges/service/patch.go`:
- Around line 125-152: Bound the round-based loop in the surrounding charge
patch flow with a maxRounds counter. Increment it for each iteration of the loop
and return a descriptive error when the configured limit is exceeded, while
preserving the existing patch application and advancement behavior below the
limit.
In `@openmeter/billing/charges/statemachine/machine_test.go`:
- Around line 215-264: Add a subtest in TestMachine_FireAndAdvanceUntilStable
that queues an invoice patch with AddInvoicePatch before calling
FireAndAdvanceUntilInvoicePatchesOrStable. Assert the call returns
ErrUnhandledInvoicePatches, preserves the initial charge status, and does not
fire the requested transition, covering the pending-patch guard in
fireAndActivate.
- Around line 394-421: Move the subtests at the shown location, including the
case around lines 454–481, from TestMachine_AdvanceUntilStable into the existing
TestMachine_AdvanceUntilInvoicePatchesOrStable test. Preserve their bodies and
assertions unchanged so failure output is grouped under the API they exercise.
In `@openmeter/billing/charges/statemachine/machine.go`:
- Around line 228-234: Optionally remove the trivial pass-through method
AdvanceUntilInvoicePatchesOrStable by moving the implementation of
advanceUntilInvoicePatchesOrStable into the exported method and updating
internal callers to use it directly. Preserve existing behavior and retain
FireAndAdvanceUntilInvoicePatchesOrStable symmetry only if preferred.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 28ef681f-9689-495a-8e1d-795557cc8cc1
📒 Files selected for processing (34)
openmeter/billing/charges/README.mdopenmeter/billing/charges/creditpurchase/service/create.goopenmeter/billing/charges/creditpurchase/service/external.goopenmeter/billing/charges/creditpurchase/service/external_test.goopenmeter/billing/charges/creditpurchase/service/invoice.goopenmeter/billing/charges/creditpurchase/service/promotional_test.goopenmeter/billing/charges/creditpurchase/service/statemachine.goopenmeter/billing/charges/flatfee/service.goopenmeter/billing/charges/flatfee/service/creditheninvoice.goopenmeter/billing/charges/flatfee/service/lineengine.goopenmeter/billing/charges/flatfee/service/triggers.goopenmeter/billing/charges/meta/patch.goopenmeter/billing/charges/service/advance.goopenmeter/billing/charges/service/flatfee_costbasis_test.goopenmeter/billing/charges/service/helpers.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/patch.goopenmeter/billing/charges/service/patch_test.goopenmeter/billing/charges/statemachine/machine.goopenmeter/billing/charges/statemachine/machine_test.goopenmeter/billing/charges/usagebased/service.goopenmeter/billing/charges/usagebased/service/creditheninvoice_test.goopenmeter/billing/charges/usagebased/service/creditsonly_test.goopenmeter/billing/charges/usagebased/service/lineengine.goopenmeter/billing/charges/usagebased/service/payments.goopenmeter/billing/charges/usagebased/service/triggers.goopenmeter/billing/charges/usagebased/service/triggers_test.goopenmeter/billing/charges/usagebased/service_test.goopenmeter/billing/worker/subscriptionsync/service/creditsonly_test.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/billing/worker/subscriptionsync/service/sync_regression_test.goopenmeter/ledger/customerbalance/service_test.goopenmeter/ledger/customerbalance/testenv_test.gotest/credits/credit_then_invoice_test.go
💤 Files with no reviewable changes (1)
- openmeter/billing/charges/creditpurchase/service/statemachine.go
d8cd82c to
bf4c3d2
Compare
Co-authored-by: Robert Boros <robert.boros@konghq.com> Signed-off-by: Peter Turi <peter.turi@konghq.com>
Co-authored-by: Robert Boros <robert.boros@konghq.com> Signed-off-by: Peter Turi <peter.turi@konghq.com>
Co-authored-by: Robert Boros <robert.boros@konghq.com> Signed-off-by: Peter Turi <peter.turi@konghq.com>
Co-authored-by: Robert Boros <robert.boros@konghq.com> Signed-off-by: Peter Turi <peter.turi@konghq.com>
2 participants
Summary
AdvanceChargeno-op behavior when no continuation is availableWhy
Charge state-machine callers could previously fire transitions independently from handling their invoice effects. That allowed callers to forget pending invoice patches or to advance through a different execution path.
The new interface makes the choice explicit: invoice-aware calls return the first complete patch batch, while non-invoice calls fail if a transition produces invoice patches. Callers can no longer inspect or drain the internal patch buffer directly.
The coordinator applies each returned invoice patch batch before resuming the charge. This allows lifecycle states to chain correctly when completing an operation requires multiple invoice patch rounds.
Behavioral impact
Patch and line-engine workflows now continue synchronously until they either produce invoice patches or reach a stable lifecycle state.
AdvanceChargeretains its previousnilresult when the charge cannot firenext.Validation
go test ./openmeter/billing/charges/... -count=1nix develop --impure .#ci -c make test-nocache— 6,919 tests passed, 9 skippedSummary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
The PR makes charge advancement explicitly stop at invoice-effect boundaries so callers must apply each patch batch before resuming lifecycle processing.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant C as Charge coordinator participant S as Charge state machine participant I as Invoice updater C->>S: Advance until patch boundary or stable alt Invoice patches emitted S-->>C: Patch batch and CanAdvance C->>I: Apply patch batch I-->>C: Updated invoice C->>S: Resume charge advancement else Stable lifecycle state S-->>C: No patches and cannot advance endReviews (6): Last reviewed commit: "Update openmeter/billing/charges/usageba..." | Re-trigger Greptile
Context used: