feat[charges]: common state machine operations#4145
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a generic, reusable charge state machine with tests, implements required Get/With accessors on flatfee and usagebased Charge types, migrates those services to the generic machine, and updates several service call sites to use the exported RefetchCharge with explicit error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Machine as GenericMachine
participant Persistence
participant Adapter as DB/Adapter
Caller->>Machine: FireAndActivate(trigger, args...)
Machine->>Machine: Validate target STATUS
alt valid transition
Machine->>Persistence: UpdateBase(ctx, newBase)
Persistence->>Adapter: Adapter.UpdateCharge(...)
Adapter-->>Persistence: persistedBase
Persistence-->>Machine: persistedBase
Machine-->>Caller: nil (success)
else unsupported
Machine-->>Caller: ErrUnsupportedOperation
end
Note over Machine,Persistence: AdvanceUntilStateStable loops: Fire trigger(s) -> UpdateBase -> Refetch as needed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
turip
force-pushed
the
feat/state-machine-common
branch
from
c1216f5 to
06bab4b
Compare
turip
force-pushed
the
feat/state-machine-common
branch
from
06bab4b to
3e1fc80
Compare
turip
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
openmeter/billing/charges/usagebased/service/creditsonly.go (1)
140-145: Keep theRefetchChargeerror prefix consistent.These two sites wrap the same helper with different messages (
"get charge"vs"refetch charge"). Standardizing that wording will make logs and test expectations easier to grep and keeps the shared-machine paths a bit more uniform.Also applies to: 225-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/creditsonly.go` around lines 140 - 145, Standardize the RefetchCharge error wrap: replace the inconsistent fmt.Errorf("get charge: %w", err) (and the other occurrence using "get charge") with fmt.Errorf("refetch charge: %w", err) so both call sites that wrap s.RefetchCharge(ctx) use the same "refetch charge" prefix; locate the calls to s.RefetchCharge and update the error message strings accordingly.openmeter/billing/charges/statemachine/machine.go (3)
131-164: Consider adding a max iterations guard (optional).The loop relies on the state machine being correctly configured to eventually stop (no cycles via
TriggerNext). That's reasonable, but a defensive max iterations check could prevent runaway loops from misconfigured state machines.Something like:
const maxIterations = 100 for i := 0; i < maxIterations; i++ { // ... existing loop body } return nil, fmt.Errorf("exceeded max state transitions")This is purely defensive - if your state machines are well-tested, the current implementation is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/statemachine/machine.go` around lines 131 - 164, The loop in Machine.AdvanceUntilStateStable can spin indefinitely if the state machine cycles; add a defensive max-iterations guard to break and return an error after a reasonable limit (e.g. 100) to avoid runaway loops. Modify the for loop that calls CanFire(ctx, meta.TriggerNext) / FireAndActivate to count iterations (i) and if i >= maxIterations return an error like "exceeded max state transitions"; keep existing behavior for CanFire false and normal success paths, and ensure the guard increments on each successful transition (after m.Charge = m.Charge.WithBase(updatedBase)). Use the same error wrapping style as other errors returned from this method.
166-176: Consider wrapping the refetch error with context.Adding the charge ID to the error would help with debugging when refetch fails:
func (m *Machine[CHARGE, BASE, STATUS]) RefetchCharge(ctx context.Context) error { chargeID := m.Charge.GetChargeID() charge, err := m.config.Persistence.Refetch(ctx, chargeID) if err != nil { - return err + return fmt.Errorf("refetch charge [id=%s]: %w", chargeID.ID, err) } m.Charge = charge return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/statemachine/machine.go` around lines 166 - 176, In RefetchCharge, wrap the error returned by m.config.Persistence.Refetch with contextual information including the charge ID to aid debugging; change the error return to include the chargeID (e.g. use fmt.Errorf or errors.Wrapf) such as returning fmt.Errorf("RefetchCharge: failed to refetch charge %s: %w", chargeID, err) so callers see which charge failed when m.config.Persistence.Refetch returns an error.
58-62: Consider makingChargeunexported or removingGetCharge().The
Chargefield is exported while there's also aGetCharge()method (line 102). This creates two ways to access the same data. Having an exported field allows external code to modify it directly, potentially bypassing state machine invariants.If direct field access is intentional for performance or simplicity in trusted callers, that's fine - just wanted to flag the redundancy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/statemachine/machine.go` around lines 58 - 62, The Machine struct exposes the Charge field while also providing a GetCharge() accessor, creating redundant and potentially unsafe access; choose one: either make the field unexported by renaming Charge to charge and update all internal and external references (constructors, methods, tests) to use GetCharge() where necessary, or remove the GetCharge() method and keep the exported Charge field; update any callers that currently use the removed/renamed symbol to the remaining API (Machine.Charge or Machine.GetCharge()) and run tests to ensure no visibility regressions.
🤖 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/statemachine/machine.go`:
- Around line 131-164: The loop in Machine.AdvanceUntilStateStable can spin
indefinitely if the state machine cycles; add a defensive max-iterations guard
to break and return an error after a reasonable limit (e.g. 100) to avoid
runaway loops. Modify the for loop that calls CanFire(ctx, meta.TriggerNext) /
FireAndActivate to count iterations (i) and if i >= maxIterations return an
error like "exceeded max state transitions"; keep existing behavior for CanFire
false and normal success paths, and ensure the guard increments on each
successful transition (after m.Charge = m.Charge.WithBase(updatedBase)). Use the
same error wrapping style as other errors returned from this method.
- Around line 166-176: In RefetchCharge, wrap the error returned by
m.config.Persistence.Refetch with contextual information including the charge ID
to aid debugging; change the error return to include the chargeID (e.g. use
fmt.Errorf or errors.Wrapf) such as returning fmt.Errorf("RefetchCharge: failed
to refetch charge %s: %w", chargeID, err) so callers see which charge failed
when m.config.Persistence.Refetch returns an error.
- Around line 58-62: The Machine struct exposes the Charge field while also
providing a GetCharge() accessor, creating redundant and potentially unsafe
access; choose one: either make the field unexported by renaming Charge to
charge and update all internal and external references (constructors, methods,
tests) to use GetCharge() where necessary, or remove the GetCharge() method and
keep the exported Charge field; update any callers that currently use the
removed/renamed symbol to the remaining API (Machine.Charge or
Machine.GetCharge()) and run tests to ensure no visibility regressions.
In `@openmeter/billing/charges/usagebased/service/creditsonly.go`:
- Around line 140-145: Standardize the RefetchCharge error wrap: replace the
inconsistent fmt.Errorf("get charge: %w", err) (and the other occurrence using
"get charge") with fmt.Errorf("refetch charge: %w", err) so both call sites that
wrap s.RefetchCharge(ctx) use the same "refetch charge" prefix; locate the calls
to s.RefetchCharge and update the error message strings accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68308a1d-aae5-45b0-9c6f-fdfb7e4fa6e6
📒 Files selected for processing (9)
openmeter/billing/charges/flatfee/charge.goopenmeter/billing/charges/flatfee/service/creditsonly.goopenmeter/billing/charges/flatfee/service/statemachine.goopenmeter/billing/charges/statemachine/machine.goopenmeter/billing/charges/statemachine/machine_test.goopenmeter/billing/charges/usagebased/charge.goopenmeter/billing/charges/usagebased/service/creditheninvoice.goopenmeter/billing/charges/usagebased/service/creditsonly.goopenmeter/billing/charges/usagebased/service/statemachine.go
turip
force-pushed
the
feat/state-machine-common
branch
from
84d9b18 to
0898d28
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/billing/charges/usagebased/service/creditsonly.go (1)
225-227: Minor: Inconsistent error message wording.
DeleteChargeat line 141 wraps the error as"get charge: %w"while here it's"refetch charge: %w". Not a bug, but standardizing on one phrase (looks like"get charge"is used elsewhere increditheninvoice.go) would make error logs more consistent.💡 Optional fix for consistency
if err := s.RefetchCharge(ctx); err != nil { - return fmt.Errorf("refetch charge: %w", err) + return fmt.Errorf("get charge: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/creditsonly.go` around lines 225 - 227, Standardize the error message wording by replacing the "refetch charge: %w" wrap in the s.RefetchCharge error return inside creditsonly.go with the same phrase used elsewhere ("get charge: %w"); update the return that currently does `return fmt.Errorf("refetch charge: %w", err)` to `return fmt.Errorf("get charge: %w", err)` so it matches DeleteCharge and creditheninvoice.go and keeps logs consistent when referencing s.RefetchCharge failures.
🤖 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/usagebased/service/creditsonly.go`:
- Around line 225-227: Standardize the error message wording by replacing the
"refetch charge: %w" wrap in the s.RefetchCharge error return inside
creditsonly.go with the same phrase used elsewhere ("get charge: %w"); update
the return that currently does `return fmt.Errorf("refetch charge: %w", err)` to
`return fmt.Errorf("get charge: %w", err)` so it matches DeleteCharge and
creditheninvoice.go and keeps logs consistent when referencing s.RefetchCharge
failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb23f6fc-d10a-4bee-9e0c-2f813668d6fc
📒 Files selected for processing (9)
openmeter/billing/charges/flatfee/charge.goopenmeter/billing/charges/flatfee/service/creditsonly.goopenmeter/billing/charges/flatfee/service/statemachine.goopenmeter/billing/charges/statemachine/machine.goopenmeter/billing/charges/statemachine/machine_test.goopenmeter/billing/charges/usagebased/charge.goopenmeter/billing/charges/usagebased/service/creditheninvoice.goopenmeter/billing/charges/usagebased/service/creditsonly.goopenmeter/billing/charges/usagebased/service/statemachine.go
🚧 Files skipped from review as they are similar to previous changes (3)
- openmeter/billing/charges/usagebased/service/creditheninvoice.go
- openmeter/billing/charges/flatfee/charge.go
- openmeter/billing/charges/statemachine/machine.go
tothandras
approved these changes
Apr 17, 2026
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
2 participants
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.
Overview
Unify base state machine. This is essential so that the Fire/CanFire etc. behaves consistently between charges components.
Given there are different flavors of charges, there were already drifts happening.
Notes for reviewer
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Tests