feat(ledger): ubp, corrections and lineage tracking#4115
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces credit realization lineage tracking for billing charges, adding persistence, querying, and lifecycle management of credit allocation segments. It implements a new lineage adapter and service layer, integrates a ledger collector for accrual handling, and extends transaction templates with correction operations to support charge reversals and adjustments. Changes
Sequence DiagramssequenceDiagram
participant Client as Charges Service
participant Collector as Ledger Collector
participant Ledger as Ledger Repo
participant Lineage as Lineage Service
participant DB as Database
Client->>Collector: CollectToAccrued(charge, amount)
Collector->>Ledger: ResolveTransactions(FBO→Accrued template)
Ledger-->>Collector: Transaction inputs
Collector->>Ledger: CommitGroup(inputs)
Ledger->>DB: Insert transactions
Ledger-->>Collector: Committed group ID
Collector->>Client: Create allocation inputs
Client->>Lineage: CreateInitialLineages(allocations)
Lineage->>DB: Insert lineage roots + segments
Lineage-->>Client: ✓
Client->>Collector: CorrectCollectedAccrued(corrections)
Collector->>Lineage: LoadActiveSegments(realizations)
Lineage->>DB: Query segments by realization
Lineage-->>Collector: Active segments
Collector->>Collector: Plan segment consumption
Collector->>Ledger: CorrectTransaction(per-segment)
Ledger->>DB: Insert corrections
Ledger-->>Collector: Correction amounts
Collector->>Lineage: CloseSegment / CreateSegment
Lineage->>DB: Update segment state
Lineage-->>Collector: ✓
Collector-->>Client: Correction inputs
sequenceDiagram
participant FlatFee as Flat-Fee Handler
participant CreditAlloc as Credit Allocation Helper
participant Adapter as Charge Adapter
participant Lineage as Lineage Service
participant Ledger as Ledger
FlatFee->>CreditAlloc: createCreditAllocations(charge, allocations)
CreditAlloc->>Adapter: CreateCreditAllocations(chargeID, allocations)
Adapter-->>CreditAlloc: Realizations created
CreditAlloc->>Lineage: CreateInitialLineages(namespace, realizations)
Lineage-->>CreditAlloc: ✓
CreditAlloc->>Lineage: PersistCorrectionLineageSegments(namespace, realizations)
Lineage-->>CreditAlloc: ✓
CreditAlloc-->>FlatFee: Return realizations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR introduces a substantial new subsystem for credit realization lineage with diverse changes across charges services, ledger adapters, transaction templates, and database schemas. The logic density is moderate-to-high in collector/correction paths, and the heterogeneity spans domain models, service wiring, template corrections, and migration scripts—requiring separate reasoning for each area. Possibly related PRs
Suggested labels
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)
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
openmeter/ledger/routingrules/routingrules.go (1)
380-385: Keep the first validation error when both directions fail.Right now the helper always returns the second error if neither direction matches. That can hide the real failure mode — for example, a route-field mismatch in the intended direction gets rewritten into a
known_cost_basis_required/unknown_cost_basis_requiredfailure, which makes debugging pretty confusing.♻️ Suggested tweak
func requireKnownToUnknownCostBasisTranslationEitherDirection(leftEntries, rightEntries []EntryView, accountType ledger.AccountType, fields []RouteField) error { - if err := requireKnownToUnknownCostBasisTranslation(leftEntries, rightEntries, accountType, fields); err == nil { - return nil - } - - return requireKnownToUnknownCostBasisTranslation(rightEntries, leftEntries, accountType, fields) + if err := requireKnownToUnknownCostBasisTranslation(leftEntries, rightEntries, accountType, fields); err != nil { + if reverseErr := requireKnownToUnknownCostBasisTranslation(rightEntries, leftEntries, accountType, fields); reverseErr == nil { + return nil + } + return err + } + + return nil }As per coding guidelines, "In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/routingrules/routingrules.go` around lines 380 - 385, The helper requireKnownToUnknownCostBasisTranslationEitherDirection currently discards the first error; change it to call requireKnownToUnknownCostBasisTranslation for the left->right direction, store that error (err1), and if err1 is nil return nil; otherwise call requireKnownToUnknownCostBasisTranslation for right->left and if that returns nil return nil, else return the first stored err1 so the original validation failure is preserved; update the function body accordingly to keep the first error when both directions fail.openmeter/ledger/transactions/input.go (1)
74-100: Consider addingAsGroupInputoverride to prevent annotation loss via method promotion.The wrapper doesn't implement
AsGroupInput, so Go will promote the embedded method and call it on the wrapped input instead—dropping the annotations. While the codebase currently avoids this by usingGroupInputs(...)helper instead, it's worth adding an override as defensive programming to guard against future calls likeWithAnnotations(...).AsGroupInput(...).💡 Suggested fix
func (a *annotatedTransactionInput) Annotations() models.Annotations { return a.annotations } + +func (a *annotatedTransactionInput) AsGroupInput(namespace string, annotations models.Annotations) ledger.TransactionGroupInput { + return &TransactionGroupInput{ + namespace: namespace, + transactions: []ledger.TransactionInput{a}, + annotations: annotations, + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/transactions/input.go` around lines 74 - 100, The wrapper annotatedTransactionInput currently embeds ledger.TransactionInput but doesn't override AsGroupInput, so calls to WithAnnotations(...).AsGroupInput() will be forwarded to the embedded input and lose the annotations; implement an AsGroupInput method on annotatedTransactionInput that calls the embedded TransactionInput.AsGroupInput(), wraps each returned ledger.GroupInput (or its inputs) with the same annotations (e.g., via existing WithAnnotations or GroupInputs helper), and returns the annotated group input(s) to preserve annotations when AsGroupInput is used.tools/migrate/migrations/20260407102402_add_credit_realization_lineage.up.sql (1)
12-13: Redundant unique indexes on primary key columns.The unique indexes
creditrealizationlineage_id(line 13) andcreditrealizationlineagesegment_id(line 33) are redundant since thePRIMARY KEYconstraint already creates a unique index on theidcolumns. These extra indexes consume storage and add overhead to writes without providing any additional benefit.🧹 Consider removing the redundant indexes
-- create "credit_realization_lineages" table CREATE TABLE "credit_realization_lineages" ( ... PRIMARY KEY ("id") ); --- create index "creditrealizationlineage_id" to table: "credit_realization_lineages" -CREATE UNIQUE INDEX "creditrealizationlineage_id" ON "credit_realization_lineages" ("id"); -- create index "creditrealizationlineage_namespace" to table: "credit_realization_lineages"-- create "credit_realization_lineage_segments" table CREATE TABLE "credit_realization_lineage_segments" ( ... PRIMARY KEY ("id"), ... ); --- create index "creditrealizationlineagesegment_id" to table: "credit_realization_lineage_segments" -CREATE UNIQUE INDEX "creditrealizationlineagesegment_id" ON "credit_realization_lineage_segments" ("id"); -- create index "creditrealizationlineagesegment_lineage_id" to table: "credit_realization_lineage_segments"Also applies to: 32-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/migrate/migrations/20260407102402_add_credit_realization_lineage.up.sql` around lines 12 - 13, Remove the redundant unique indexes that duplicate the primary key uniqueness: delete the CREATE UNIQUE INDEX statements for "creditrealizationlineage_id" on "credit_realization_lineages" ("id") and for "creditrealizationlineagesegment_id" on "credit_realization_lineage_segments" ("id"); leave the primary key constraints intact and ensure no other code depends on these specific index names before dropping them from the migration.openmeter/ledger/collector/collect.go (1)
163-185: Consider logging when annotation derivation fails silently.The fallback to original annotations when template name extraction or merge fails (lines 165-167, 180-182) is a reasonable defensive choice, but these silent fallbacks might make debugging harder if something goes wrong.
You might want to add debug-level logging here to help with future troubleshooting, though this is optional given the fallback behavior is safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/collector/collect.go` around lines 163 - 185, In creditRealizationAnnotationsForCollectedInput, add debug-level logging when TransactionTemplateNameFromAnnotations(input.Annotations()) returns an error and when input.Annotations().Merge(...) returns an error so failures aren't silent; log the error value, the templateName failure context, and the input annotations (or a short identifier) before returning the original annotations. Reference the functions TransactionTemplateNameFromAnnotations, annotations.Merge, and creditrealization.LineageAnnotations so the logs clearly indicate which step failed.openmeter/ledger/transactions/correction.go (1)
80-107: Comprehensive template dispatch.The exhaustive switch covers all the transaction templates. The default case with a clear error for unknown templates is a nice fail-safe.
One thing to keep in mind: if new templates are added, they'll need to be registered here. Consider adding a comment near the template definitions to remind future contributors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/transactions/correction.go` around lines 80 - 107, The switch in transactionTemplateByName enumerates all transaction template types and requires manual updates when new templates are added; add a clear TODO comment near the template type definitions (and/or above transactionTemplateByName) stating that any new Template structs (e.g., IssueCustomerReceivableTemplate, FundCustomerReceivableTemplate, ConvertCurrencyTemplate, etc.) must be registered in transactionTemplateByName (or in a central registry if you later refactor) to prevent silent failures; keep the comment concise and include an example and pointer to the function name transactionTemplateByName for future contributors.openmeter/billing/charges/lineage/service/service.go (1)
149-215: Consider clarifying the intentional leftover handling with a code comment.The asymmetry with
WritebackCorrectionLineageSegmentsis real: corrections error if they can't be fully applied, while backfills silently exit if there's leftover amount. This likely reflects different domain semantics (corrections are strict, backfills are best-effort coverage), but it's worth adding a brief inline comment explaining why leftover backfill amounts are acceptable—especially for financial operations where silent loss of amounts can raise eyebrows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/lineage/service/service.go` around lines 149 - 215, Add a brief inline comment in BackfillAdvanceLineageSegments explaining why leftover remaining amounts are intentionally ignored (best-effort backfill semantics) and reference the asymmetry with WritebackCorrectionLineageSegments so reviewers understand that corrections are strict while backfills may leave unconsumed amounts; place the comment near the loop that iterates segments (for _, segment := range segments) or right before the remaining check (if !remaining.IsPositive()) to make the intent explicit.test/credits/sanity_lifecycle_test.go (1)
386-388: Minor:defer clock.UnFreeze()inside a setup helper.The
defer clock.UnFreeze()on line 388 will execute whensetupUsageBasedCreditOnlyLifecyclePartialBackfillCorrectionreturns, but the calling test continues to use the frozen clock. This works because the tests themselves also callclock.FreezeTime(), but it's a bit subtle.Since
TearDownTestalready handlesclock.UnFreeze(), you might consider removing this defer to avoid confusion. Not blocking though — the current behavior is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credits/sanity_lifecycle_test.go` around lines 386 - 388, Remove the deferred UnFreeze call inside setupUsageBasedCreditOnlyLifecyclePartialBackfillCorrection: the helper should call clock.FreezeTime(createAt) but not defer clock.UnFreeze(), because TearDownTest already handles clock.UnFreeze(); specifically remove the line that defers clock.UnFreeze() so the frozen clock's lifecycle is managed only by the global teardown and not by the setup helper.openmeter/ledger/collector/correct.go (1)
352-389: Consider extracting the FBO entry detection logic.The nested switch-case inside the loop (lines 377-384) is a bit dense. Since you're checking
AccountType == CustomerFBO && Amount.IsNegative(), you might consider extracting this into a small helper or adding a brief comment clarifying what constitutes a "collected source" entry.Not blocking, just a readability suggestion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/collector/correct.go` around lines 352 - 389, Extract the FBO entry detection into a helper to improve readability: create a function (e.g., isCollectedSourceEntry(entry ledger.Entry) bool) that encapsulates the condition entry.PostingAddress().AccountType() == ledger.AccountTypeCustomerFBO && entry.Amount().IsNegative(), and replace the inline check inside collectedSourcesForGroup with a call to that helper (or add a one-line explanatory comment if you prefer). Update collectedSourcesForGroup to call isCollectedSourceEntry for each entry before appending a collectedSource (preserve use of collectedSource struct and advanceReceivableIssueTransaction logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/ledger/routingrules/routingrules_test.go`:
- Around line 38-80: Add two focused unit tests that assert same-account
translations are allowed: create
TestDefaultValidator_AllowsReceivableToReceivable_SameAccount and
TestDefaultValidator_AllowsAccruedToAccrued_SameAccount that use
routingrules.DefaultValidator.ValidateEntries with two EntryInput items created
via addressForRoute for ledger.AccountTypeCustomerReceivable (include
TransactionAuthorizationStatus open if required) and
ledger.AccountTypeCustomerAccrued respectively, mirror the pattern in
TestDefaultValidator_AllowsFBOToReceivableReverse/TestDefaultValidator_AllowsAccruedToFBO
(use +/-50 alpacadecimal amounts) and assert require.NoError on the validation
to cover the requireKnownToUnknownCostBasisTranslationEitherDirection regression
path.
---
Nitpick comments:
In `@openmeter/billing/charges/lineage/service/service.go`:
- Around line 149-215: Add a brief inline comment in
BackfillAdvanceLineageSegments explaining why leftover remaining amounts are
intentionally ignored (best-effort backfill semantics) and reference the
asymmetry with WritebackCorrectionLineageSegments so reviewers understand that
corrections are strict while backfills may leave unconsumed amounts; place the
comment near the loop that iterates segments (for _, segment := range segments)
or right before the remaining check (if !remaining.IsPositive()) to make the
intent explicit.
In `@openmeter/ledger/collector/collect.go`:
- Around line 163-185: In creditRealizationAnnotationsForCollectedInput, add
debug-level logging when
TransactionTemplateNameFromAnnotations(input.Annotations()) returns an error and
when input.Annotations().Merge(...) returns an error so failures aren't silent;
log the error value, the templateName failure context, and the input annotations
(or a short identifier) before returning the original annotations. Reference the
functions TransactionTemplateNameFromAnnotations, annotations.Merge, and
creditrealization.LineageAnnotations so the logs clearly indicate which step
failed.
In `@openmeter/ledger/collector/correct.go`:
- Around line 352-389: Extract the FBO entry detection into a helper to improve
readability: create a function (e.g., isCollectedSourceEntry(entry ledger.Entry)
bool) that encapsulates the condition entry.PostingAddress().AccountType() ==
ledger.AccountTypeCustomerFBO && entry.Amount().IsNegative(), and replace the
inline check inside collectedSourcesForGroup with a call to that helper (or add
a one-line explanatory comment if you prefer). Update collectedSourcesForGroup
to call isCollectedSourceEntry for each entry before appending a collectedSource
(preserve use of collectedSource struct and advanceReceivableIssueTransaction
logic).
In `@openmeter/ledger/routingrules/routingrules.go`:
- Around line 380-385: The helper
requireKnownToUnknownCostBasisTranslationEitherDirection currently discards the
first error; change it to call requireKnownToUnknownCostBasisTranslation for the
left->right direction, store that error (err1), and if err1 is nil return nil;
otherwise call requireKnownToUnknownCostBasisTranslation for right->left and if
that returns nil return nil, else return the first stored err1 so the original
validation failure is preserved; update the function body accordingly to keep
the first error when both directions fail.
In `@openmeter/ledger/transactions/correction.go`:
- Around line 80-107: The switch in transactionTemplateByName enumerates all
transaction template types and requires manual updates when new templates are
added; add a clear TODO comment near the template type definitions (and/or above
transactionTemplateByName) stating that any new Template structs (e.g.,
IssueCustomerReceivableTemplate, FundCustomerReceivableTemplate,
ConvertCurrencyTemplate, etc.) must be registered in transactionTemplateByName
(or in a central registry if you later refactor) to prevent silent failures;
keep the comment concise and include an example and pointer to the function name
transactionTemplateByName for future contributors.
In `@openmeter/ledger/transactions/input.go`:
- Around line 74-100: The wrapper annotatedTransactionInput currently embeds
ledger.TransactionInput but doesn't override AsGroupInput, so calls to
WithAnnotations(...).AsGroupInput() will be forwarded to the embedded input and
lose the annotations; implement an AsGroupInput method on
annotatedTransactionInput that calls the embedded
TransactionInput.AsGroupInput(), wraps each returned ledger.GroupInput (or its
inputs) with the same annotations (e.g., via existing WithAnnotations or
GroupInputs helper), and returns the annotated group input(s) to preserve
annotations when AsGroupInput is used.
In `@test/credits/sanity_lifecycle_test.go`:
- Around line 386-388: Remove the deferred UnFreeze call inside
setupUsageBasedCreditOnlyLifecyclePartialBackfillCorrection: the helper should
call clock.FreezeTime(createAt) but not defer clock.UnFreeze(), because
TearDownTest already handles clock.UnFreeze(); specifically remove the line that
defers clock.UnFreeze() so the frozen clock's lifecycle is managed only by the
global teardown and not by the setup helper.
In
`@tools/migrate/migrations/20260407102402_add_credit_realization_lineage.up.sql`:
- Around line 12-13: Remove the redundant unique indexes that duplicate the
primary key uniqueness: delete the CREATE UNIQUE INDEX statements for
"creditrealizationlineage_id" on "credit_realization_lineages" ("id") and for
"creditrealizationlineagesegment_id" on "credit_realization_lineage_segments"
("id"); leave the primary key constraints intact and ensure no other code
depends on these specific index names before dropping them from the migration.
🪄 Autofix (Beta)
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
Run ID: 66112915-94a6-4687-8f28-ccf8e053b058
⛔ Files ignored due to path filters (28)
openmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage/creditrealizationlineage.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage/where.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_create.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_delete.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_query.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_update.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineagesegment.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineagesegment/creditrealizationlineagesegment.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineagesegment/where.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineagesegment_create.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineagesegment_delete.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineagesegment_query.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineagesegment_update.gois excluded by!**/ent/db/**openmeter/ent/db/cursor.gois excluded by!**/ent/db/**openmeter/ent/db/ent.gois excluded by!**/ent/db/**openmeter/ent/db/entmixinaccessor.gois excluded by!**/ent/db/**openmeter/ent/db/expose.gois excluded by!**/ent/db/**openmeter/ent/db/hook/hook.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/paginate.gois excluded by!**/ent/db/**openmeter/ent/db/predicate/predicate.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**openmeter/ent/db/tx.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (67)
AGENTS.mdapp/common/charges.goapp/common/customerbalance.goopenmeter/billing/charges/creditpurchase/service/external.goopenmeter/billing/charges/creditpurchase/service/invoice.goopenmeter/billing/charges/creditpurchase/service/service.goopenmeter/billing/charges/flatfee/adapter/charge.goopenmeter/billing/charges/flatfee/adapter/credits.goopenmeter/billing/charges/flatfee/service/creditsonly.goopenmeter/billing/charges/flatfee/service/invoice.goopenmeter/billing/charges/flatfee/service/lineage.goopenmeter/billing/charges/flatfee/service/service.goopenmeter/billing/charges/lineage/adapter/adapter.goopenmeter/billing/charges/lineage/adapter/lineage.goopenmeter/billing/charges/lineage/lineage.goopenmeter/billing/charges/lineage/service.goopenmeter/billing/charges/lineage/service/service.goopenmeter/billing/charges/models/creditrealization/lineage.goopenmeter/billing/charges/models/creditrealization/lineage_segment.goopenmeter/billing/charges/models/creditrealization/lineage_specs.goopenmeter/billing/charges/models/creditrealization/models.goopenmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/lineage_test.goopenmeter/billing/charges/testutils/service.goopenmeter/billing/charges/usagebased/adapter/charge.goopenmeter/billing/charges/usagebased/adapter/creditallocation.goopenmeter/billing/charges/usagebased/service/creditsonly.goopenmeter/billing/charges/usagebased/service/lineage.goopenmeter/billing/charges/usagebased/service/service.goopenmeter/ent/schema/creditrealizationlineage.goopenmeter/ledger/annotations.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/chargeadapter/helpers.goopenmeter/ledger/chargeadapter/usagebased.goopenmeter/ledger/chargeadapter/usagebased_test.goopenmeter/ledger/collector/collect.goopenmeter/ledger/collector/correct.goopenmeter/ledger/collector/service.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/ledger/historical/adapter/ledger.goopenmeter/ledger/historical/ledger.goopenmeter/ledger/historical/repo.goopenmeter/ledger/historical/transaction.goopenmeter/ledger/primitives.goopenmeter/ledger/routingrules/routingrules.goopenmeter/ledger/routingrules/routingrules_test.goopenmeter/ledger/transactions/accrual.goopenmeter/ledger/transactions/accrual_test.goopenmeter/ledger/transactions/correction.goopenmeter/ledger/transactions/correction_test.goopenmeter/ledger/transactions/customer.goopenmeter/ledger/transactions/earnings.goopenmeter/ledger/transactions/earnings_test.goopenmeter/ledger/transactions/fx.goopenmeter/ledger/transactions/input.goopenmeter/ledger/transactions/resolve.goopenmeter/ledger/transactions/resolve_test.goopenmeter/ledger/transactions/template.goopenmeter/ledger/transactions/testenv_test.goopenmeter/ledger/transactions/testutils/anytransaction.gotest/credits/sanity_lifecycle_test.gotest/credits/sanity_test.gotools/migrate/migrations/20260407102402_add_credit_realization_lineage.down.sqltools/migrate/migrations/20260407102402_add_credit_realization_lineage.up.sql
Comment on lines
+38
to
+80
| func TestDefaultValidator_AllowsAccruedToFBO(t *testing.T) { | ||
| validator := routingrules.DefaultValidator | ||
|
|
||
| err := validator.ValidateEntries([]ledger.EntryInput{ | ||
| &transactionstestutils.AnyEntryInput{ | ||
| Address: addressForRoute(t, ledger.AccountTypeCustomerAccrued, "sub-accrued", ledger.Route{ | ||
| Currency: currencyx.Code("USD"), | ||
| }), | ||
| AmountValue: alpacadecimal.NewFromInt(-50), | ||
| }, | ||
| &transactionstestutils.AnyEntryInput{ | ||
| Address: addressForRoute(t, ledger.AccountTypeCustomerFBO, "sub-fbo", ledger.Route{ | ||
| Currency: currencyx.Code("USD"), | ||
| }), | ||
| AmountValue: alpacadecimal.NewFromInt(50), | ||
| }, | ||
| }) | ||
|
|
||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| func TestDefaultValidator_AllowsFBOToReceivableReverse(t *testing.T) { | ||
| validator := routingrules.DefaultValidator | ||
| openStatus := ledger.TransactionAuthorizationStatusOpen | ||
|
|
||
| err := validator.ValidateEntries([]ledger.EntryInput{ | ||
| &transactionstestutils.AnyEntryInput{ | ||
| Address: addressForRoute(t, ledger.AccountTypeCustomerFBO, "sub-fbo", ledger.Route{ | ||
| Currency: currencyx.Code("USD"), | ||
| }), | ||
| AmountValue: alpacadecimal.NewFromInt(-50), | ||
| }, | ||
| &transactionstestutils.AnyEntryInput{ | ||
| Address: addressForRoute(t, ledger.AccountTypeCustomerReceivable, "sub-rec-open", ledger.Route{ | ||
| Currency: currencyx.Code("USD"), | ||
| TransactionAuthorizationStatus: &openStatus, | ||
| }), | ||
| AmountValue: alpacadecimal.NewFromInt(50), | ||
| }, | ||
| }) | ||
|
|
||
| require.NoError(t, err) | ||
| } |
Contributor
There was a problem hiding this comment.
Please add a direct same-account regression for the new cost-basis translation paths.
These cases are nice coverage for the reversed cross-account flows, but the code change also relaxed CustomerReceivable -> CustomerReceivable and CustomerAccrued -> CustomerAccrued validation through requireKnownToUnknownCostBasisTranslationEitherDirection. A focused test for those same-account transitions would make this much safer, because the new helper could regress while these tests still stay green.
As per coding guidelines, "Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/routingrules/routingrules_test.go` around lines 38 - 80, Add
two focused unit tests that assert same-account translations are allowed: create
TestDefaultValidator_AllowsReceivableToReceivable_SameAccount and
TestDefaultValidator_AllowsAccruedToAccrued_SameAccount that use
routingrules.DefaultValidator.ValidateEntries with two EntryInput items created
via addressForRoute for ledger.AccountTypeCustomerReceivable (include
TransactionAuthorizationStatus open if required) and
ledger.AccountTypeCustomerAccrued respectively, mirror the pattern in
TestDefaultValidator_AllowsFBOToReceivableReverse/TestDefaultValidator_AllowsAccruedToFBO
(use +/-50 alpacadecimal amounts) and assert require.NoError on the validation
to cover the requireKnownToUnknownCostBasisTranslationEitherDirection regression
path.
691df57 to
924447d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/common/customerbalance.go (1)
89-117:⚠️ Potential issue | 🔴 CriticalWire the new lineage service here too.
This path now builds
flatfeeserviceandusagebasedservicewithoutLineage, but both constructors validate that dependency. With credits enabled,NewCustomerBalanceServicewill now fail fast with the new “lineage service cannot be null” error instead of starting up.🛠️ Minimal fix sketch
+ lineageAdapter, err := lineageadapter.New(lineageadapter.Config{ + Client: db, + }) + if err != nil { + return nil, err + } + + lineageService, err := lineageservice.New(lineageservice.Config{ + Adapter: lineageAdapter, + }) + if err != nil { + return nil, err + } + flatFeeService, err := flatfeeservice.New(flatfeeservice.Config{ Adapter: flatFeeAdapter, Handler: ledgerchargeadapter.NewFlatFeeHandler(historicalLedger, transactions.ResolverDependencies{AccountService: accountResolver, SubAccountService: accountService}, collectorService), + Lineage: lineageService, MetaAdapter: metaAdapter, Locker: locker, }) @@ usageService, err := usagebasedservice.New(usagebasedservice.Config{ Adapter: usageAdapter, Handler: ledgerchargeadapter.NewUsageBasedHandler(collectorService), + Lineage: lineageService, Locker: locker, MetaAdapter: metaAdapter, CustomerOverrideService: billingRegistry.Billing,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/common/customerbalance.go` around lines 89 - 117, The flatfeeservice and usagebasedservice are being constructed without the required Lineage dependency, causing startup failures when the constructors validate it; obtain the existing lineage service instance used elsewhere (e.g., lineageService or billingRegistry.Lineage) and pass it into both flatfeeservice.New(...) and usagebasedservice.New(...) by adding Lineage: lineageService (or the correct identifier) to their Config structs so the constructors receive a non-nil Lineage.
🧹 Nitpick comments (1)
openmeter/billing/charges/creditpurchase/service/invoice.go (1)
18-45: Consider pulling this initiation flow into one shared helper.This block now matches
onExternalCreditPurchasepretty closely: begin tx, callOnCreditPurchaseInitiated, backfill lineage, mark active, persist. Keeping that in one place would make future changes to ordering or lineage rules much less likely to drift.As per coding guidelines:
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/creditpurchase/service/invoice.go` around lines 18 - 45, Extract the duplicated initiation flow into a single helper (e.g., processCreditPurchaseInitiation or handleCreditPurchaseInitiation) that encapsulates the transaction.RunWithNoValue call and the sequence: call s.handler.OnCreditPurchaseInitiated, set charge.State.CreditGrantRealization with ledgertransaction.TimedGroupReference, conditionally call s.lineage.BackfillAdvanceLineageSegments when ledgerTransactionGroupReference.TransactionGroupID != "", set charge.Status = meta.ChargeStatusActive, and persist via s.adapter.UpdateCharge; then replace the matching blocks in invoice.go and onExternalCreditPurchase to call this helper so ordering and lineage rules remain centralized and behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/billing/charges/lineage/adapter/lineage.go`:
- Around line 189-203: The CreateSegment write path (adapter.CreateSegment
handling lineage.CreateSegmentInput) must validate input invariants before
calling Save(): check that Amount is positive (>0) and that state-specific
requirements are met (e.g., if input.State == "advance_backfilled" then
input.BackingTransactionGroupID must be non-nil), returning a descriptive error
when validation fails; perform these checks at the start of CreateSegment
(before building create := tx.db.CreditRealizationLineageSegment.Create()) so
invalid combinations are rejected early and Save() is only called for valid
inputs.
- Around line 60-89: The CreateLineages method currently uses
TransactingRepoWithNoValue which only rebinds to an existing tx; ensure this
operation runs in a real DB transaction to avoid partial commits by either (a)
calling GetDriverFromContext(ctx) and opening a transaction if none exists (as
the lock methods do) or (b) explicitly starting a new transaction inside
CreateLineages, running the bulk CreateBulk/...Save calls within that tx and
rolling back on error; update CreateLineages to detect/obtain a driver/tx (via
GetDriverFromContext) and use it to create a proper transactional scope so both
CreditRealizationLineage and CreditRealizationLineageSegment creations succeed
or the whole operation is rolled back.
In `@openmeter/ent/schema/creditrealizationlineage.go`:
- Around line 36-40: The schema allows root_realization_id to be an empty
string; update the field declaration for root_realization_id in the
CreditRealizationLineage schema by adding a NotEmpty() constraint to the field
chain (alongside SchemaType(...).Immutable()) so the schema layer enforces
non-empty values like customer_id and lineage_id do; ensure the NotEmpty() is
applied on the field.String("root_realization_id") chain so DB contract matches
the domain model.
---
Outside diff comments:
In `@app/common/customerbalance.go`:
- Around line 89-117: The flatfeeservice and usagebasedservice are being
constructed without the required Lineage dependency, causing startup failures
when the constructors validate it; obtain the existing lineage service instance
used elsewhere (e.g., lineageService or billingRegistry.Lineage) and pass it
into both flatfeeservice.New(...) and usagebasedservice.New(...) by adding
Lineage: lineageService (or the correct identifier) to their Config structs so
the constructors receive a non-nil Lineage.
---
Nitpick comments:
In `@openmeter/billing/charges/creditpurchase/service/invoice.go`:
- Around line 18-45: Extract the duplicated initiation flow into a single helper
(e.g., processCreditPurchaseInitiation or handleCreditPurchaseInitiation) that
encapsulates the transaction.RunWithNoValue call and the sequence: call
s.handler.OnCreditPurchaseInitiated, set charge.State.CreditGrantRealization
with ledgertransaction.TimedGroupReference, conditionally call
s.lineage.BackfillAdvanceLineageSegments when
ledgerTransactionGroupReference.TransactionGroupID != "", set charge.Status =
meta.ChargeStatusActive, and persist via s.adapter.UpdateCharge; then replace
the matching blocks in invoice.go and onExternalCreditPurchase to call this
helper so ordering and lineage rules remain centralized and behavior is
unchanged.
🪄 Autofix (Beta)
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
Run ID: 2bf42029-bed9-4036-8e62-bfecb3a95f61
⛔ Files ignored due to path filters (27)
openmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage/creditrealizationlineage.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage/where.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_create.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_delete.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_query.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_update.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineagesegment.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineagesegment/creditrealizationlineagesegment.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineagesegment/where.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineagesegment_create.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineagesegment_delete.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineagesegment_query.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineagesegment_update.gois excluded by!**/ent/db/**openmeter/ent/db/cursor.gois excluded by!**/ent/db/**openmeter/ent/db/ent.gois excluded by!**/ent/db/**openmeter/ent/db/entmixinaccessor.gois excluded by!**/ent/db/**openmeter/ent/db/expose.gois excluded by!**/ent/db/**openmeter/ent/db/hook/hook.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/paginate.gois excluded by!**/ent/db/**openmeter/ent/db/predicate/predicate.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**openmeter/ent/db/tx.gois excluded by!**/ent/db/**
📒 Files selected for processing (47)
AGENTS.mdapp/common/charges.goapp/common/customerbalance.goopenmeter/billing/charges/creditpurchase/service/external.goopenmeter/billing/charges/creditpurchase/service/invoice.goopenmeter/billing/charges/creditpurchase/service/service.goopenmeter/billing/charges/flatfee/adapter/charge.goopenmeter/billing/charges/flatfee/adapter/credits.goopenmeter/billing/charges/flatfee/service/creditsonly.goopenmeter/billing/charges/flatfee/service/invoice.goopenmeter/billing/charges/flatfee/service/lineage.goopenmeter/billing/charges/flatfee/service/service.goopenmeter/billing/charges/lineage/adapter/adapter.goopenmeter/billing/charges/lineage/adapter/lineage.goopenmeter/billing/charges/lineage/lineage.goopenmeter/billing/charges/lineage/service.goopenmeter/billing/charges/lineage/service/service.goopenmeter/billing/charges/models/creditrealization/lineage.goopenmeter/billing/charges/models/creditrealization/lineage_segment.goopenmeter/billing/charges/models/creditrealization/lineage_specs.goopenmeter/billing/charges/models/creditrealization/models.goopenmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/lineage_test.goopenmeter/billing/charges/testutils/service.goopenmeter/billing/charges/usagebased/adapter/charge.goopenmeter/billing/charges/usagebased/adapter/creditallocation.goopenmeter/billing/charges/usagebased/service/creditsonly.goopenmeter/billing/charges/usagebased/service/lineage.goopenmeter/billing/charges/usagebased/service/service.goopenmeter/ent/schema/creditrealizationlineage.goopenmeter/ledger/annotations.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/chargeadapter/helpers.goopenmeter/ledger/chargeadapter/usagebased.goopenmeter/ledger/chargeadapter/usagebased_test.goopenmeter/ledger/collector/collect.goopenmeter/ledger/collector/correct.goopenmeter/ledger/collector/service.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/ledger/historical/adapter/ledger.goopenmeter/ledger/historical/ledger.goopenmeter/ledger/historical/repo.goopenmeter/ledger/historical/transaction.goopenmeter/ledger/noop/noop.go
✅ Files skipped from review due to trivial changes (8)
- openmeter/billing/charges/usagebased/adapter/creditallocation.go
- openmeter/billing/charges/flatfee/adapter/credits.go
- openmeter/billing/charges/flatfee/service/creditsonly.go
- openmeter/billing/charges/usagebased/service/service.go
- openmeter/ledger/chargeadapter/helpers.go
- openmeter/billing/charges/lineage/service/service.go
- openmeter/ledger/collector/correct.go
- openmeter/ledger/collector/service.go
🚧 Files skipped from review as they are similar to previous changes (17)
- openmeter/billing/charges/flatfee/service/invoice.go
- openmeter/ledger/chargeadapter/creditpurchase.go
- openmeter/billing/charges/creditpurchase/service/service.go
- openmeter/ledger/chargeadapter/creditpurchase_test.go
- openmeter/billing/charges/flatfee/service/service.go
- openmeter/billing/charges/flatfee/adapter/charge.go
- openmeter/billing/charges/usagebased/service/lineage.go
- openmeter/billing/charges/models/creditrealization/models.go
- openmeter/billing/charges/models/creditrealization/lineage_specs.go
- openmeter/billing/charges/lineage/service.go
- openmeter/billing/charges/usagebased/service/creditsonly.go
- openmeter/billing/charges/flatfee/service/lineage.go
- openmeter/billing/charges/lineage/lineage.go
- openmeter/billing/charges/service/lineage_test.go
- openmeter/ledger/annotations.go
- app/common/charges.go
- openmeter/billing/charges/models/creditrealization/lineage.go
Comment on lines
+60
to
+89
| func (a *adapter) CreateLineages(ctx context.Context, input lineage.CreateLineagesInput) error { | ||
| return entutils.TransactingRepoWithNoValue(ctx, a, func(ctx context.Context, tx *adapter) error { | ||
| rootCreates := make([]*entdb.CreditRealizationLineageCreate, 0, len(input.Specs)) | ||
| segmentCreates := make([]*entdb.CreditRealizationLineageSegmentCreate, 0, len(input.Specs)) | ||
|
|
||
| for _, spec := range input.Specs { | ||
| rootCreates = append(rootCreates, tx.db.CreditRealizationLineage.Create(). | ||
| SetID(spec.LineageID). | ||
| SetNamespace(input.Namespace). | ||
| SetRootRealizationID(spec.RootRealizationID). | ||
| SetCustomerID(input.CustomerID). | ||
| SetCurrency(input.Currency). | ||
| SetOriginKind(spec.OriginKind), | ||
| ) | ||
| segmentCreates = append(segmentCreates, tx.db.CreditRealizationLineageSegment.Create(). | ||
| SetLineageID(spec.LineageID). | ||
| SetAmount(spec.Amount). | ||
| SetState(spec.InitialState), | ||
| ) | ||
| } | ||
|
|
||
| if _, err := tx.db.CreditRealizationLineage.CreateBulk(rootCreates...).Save(ctx); err != nil { | ||
| return fmt.Errorf("create credit realization lineages: %w", err) | ||
| } | ||
| if _, err := tx.db.CreditRealizationLineageSegment.CreateBulk(segmentCreates...).Save(ctx); err != nil { | ||
| return fmt.Errorf("create initial credit realization lineage segments: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| }) |
Contributor
There was a problem hiding this comment.
Require a real transaction for the two-step lineage create.
TransactingRepoWithNoValue(...) only rebinds to a tx already present in ctx; it doesn’t open one. So if this method is called without a surrounding transaction, the root bulk insert can succeed and the segment bulk insert can still fail, leaving orphaned lineage rows behind. I’d either enforce GetDriverFromContext(ctx) here like the lock methods do, or make this method start its own transaction.
One simple guard
func (a *adapter) CreateLineages(ctx context.Context, input lineage.CreateLineagesInput) error {
return entutils.TransactingRepoWithNoValue(ctx, a, func(ctx context.Context, tx *adapter) error {
+ if _, err := entutils.GetDriverFromContext(ctx); err != nil {
+ return fmt.Errorf("create lineages must be called in a transaction: %w", err)
+ }
+
rootCreates := make([]*entdb.CreditRealizationLineageCreate, 0, len(input.Specs))
segmentCreates := make([]*entdb.CreditRealizationLineageSegmentCreate, 0, len(input.Specs))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/billing/charges/lineage/adapter/lineage.go` around lines 60 - 89,
The CreateLineages method currently uses TransactingRepoWithNoValue which only
rebinds to an existing tx; ensure this operation runs in a real DB transaction
to avoid partial commits by either (a) calling GetDriverFromContext(ctx) and
opening a transaction if none exists (as the lock methods do) or (b) explicitly
starting a new transaction inside CreateLineages, running the bulk
CreateBulk/...Save calls within that tx and rolling back on error; update
CreateLineages to detect/obtain a driver/tx (via GetDriverFromContext) and use
it to create a proper transactional scope so both CreditRealizationLineage and
CreditRealizationLineageSegment creations succeed or the whole operation is
rolled back.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
openmeter/ent/schema/creditrealizationlineage.go (1)
70-74: Add a currency-aware customer lookup index.
LockAdvanceLineagesForBackfillnow filters bynamespace,customer_id, andcurrencybefore it touches segments. With only(namespace, customer_id)indexed, that path still has to scan all of a customer's lineages across currencies. I'd add a(namespace, customer_id, currency)index here, and keep the shorter one only if you still need that lookup shape elsewhere.Possible tweak
func (CreditRealizationLineage) Indexes() []ent.Index { return []ent.Index{ index.Fields("namespace", "root_realization_id").Unique(), index.Fields("namespace", "customer_id"), + index.Fields("namespace", "customer_id", "currency"), } }As per coding guidelines, "Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ent/schema/creditrealizationlineage.go` around lines 70 - 74, The Indexes() method on CreditRealizationLineage currently only defines (namespace, customer_id) and (namespace, root_realization_id) index shapes; add a new index.Fields("namespace", "customer_id", "currency") to the returned slice so lookups in LockAdvanceLineagesForBackfill that filter by namespace, customer_id and currency are covered and avoid full-customer scans; keep or remove the existing (namespace, customer_id) index only if other code paths still rely on that two-column shape.openmeter/billing/charges/lineage/adapter/lineage.go (1)
60-89: Reuse the segment invariant check on the bulk-create path.This is the only insert path that writes
CreditRealizationLineageSegmentrows without going throughCreateSegmentInput.Validate(). Reusing the same validation here would keep initial lineage inserts aligned with later segment writes and stop a badInitialLineageSpecfrom seeding invalid state.One lightweight way to keep the rules aligned
for _, spec := range input.Specs { + if err := (lineage.CreateSegmentInput{ + LineageID: spec.LineageID, + Amount: spec.Amount, + State: spec.InitialState, + }).Validate(); err != nil { + return fmt.Errorf("validate initial lineage segment %s: %w", spec.LineageID, err) + } + rootCreates = append(rootCreates, tx.db.CreditRealizationLineage.Create(). SetID(spec.LineageID). SetNamespace(input.Namespace). SetRootRealizationID(spec.RootRealizationID). SetCustomerID(input.CustomerID).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/lineage/adapter/lineage.go` around lines 60 - 89, The CreateLineages function currently bulk-inserts CreditRealizationLineageSegment rows without running the same invariants as CreateSegmentInput.Validate; update CreateLineages to validate each initial segment (e.g., by calling the existing CreateSegmentInput.Validate or the shared segment invariant helper) for every spec/InitialLineageSpec before building segmentCreates and calling tx.db.CreditRealizationLineageSegment.CreateBulk(...). Ensure you reference the same validation routine used by segment creation (the Validate method or helper) and return a descriptive error if validation fails so initial lineage inserts cannot seed invalid segment state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/billing/charges/lineage/service.go`:
- Around line 64-81: The current
WritebackCorrectionLineageSegmentsInput.Validate() only checks
CorrectsRealizationID for correction-type realizations and misses full payload
validation (e.g., malformed amounts); update the method to invoke
i.Realizations.Validate() early (and return or append its error(s)) so the
writeback validation is as strict as CreateInitialLineagesInput; ensure you call
Realizations.Validate() inside
WritebackCorrectionLineageSegmentsInput.Validate() and incorporate its returned
error(s) into the final errors.Join result.
---
Nitpick comments:
In `@openmeter/billing/charges/lineage/adapter/lineage.go`:
- Around line 60-89: The CreateLineages function currently bulk-inserts
CreditRealizationLineageSegment rows without running the same invariants as
CreateSegmentInput.Validate; update CreateLineages to validate each initial
segment (e.g., by calling the existing CreateSegmentInput.Validate or the shared
segment invariant helper) for every spec/InitialLineageSpec before building
segmentCreates and calling
tx.db.CreditRealizationLineageSegment.CreateBulk(...). Ensure you reference the
same validation routine used by segment creation (the Validate method or helper)
and return a descriptive error if validation fails so initial lineage inserts
cannot seed invalid segment state.
In `@openmeter/ent/schema/creditrealizationlineage.go`:
- Around line 70-74: The Indexes() method on CreditRealizationLineage currently
only defines (namespace, customer_id) and (namespace, root_realization_id) index
shapes; add a new index.Fields("namespace", "customer_id", "currency") to the
returned slice so lookups in LockAdvanceLineagesForBackfill that filter by
namespace, customer_id and currency are covered and avoid full-customer scans;
keep or remove the existing (namespace, customer_id) index only if other code
paths still rely on that two-column shape.
🪄 Autofix (Beta)
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
Run ID: 6a16dcec-b65b-4c56-9e5b-50ae644945c8
⛔ Files ignored due to path filters (3)
openmeter/ent/db/creditrealizationlineage/creditrealizationlineage.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_create.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**
📒 Files selected for processing (4)
openmeter/billing/charges/lineage/adapter/lineage.goopenmeter/billing/charges/lineage/service.goopenmeter/billing/charges/service/lineage_test.goopenmeter/ent/schema/creditrealizationlineage.go
✅ Files skipped from review due to trivial changes (1)
- openmeter/billing/charges/service/lineage_test.go
Comment on lines
+64
to
+81
| func (i WritebackCorrectionLineageSegmentsInput) Validate() error { | ||
| var errs []error | ||
|
|
||
| if i.Namespace == "" { | ||
| errs = append(errs, errors.New("namespace is required")) | ||
| } | ||
|
|
||
| for idx, realization := range i.Realizations { | ||
| if realization.Type != creditrealization.TypeCorrection { | ||
| continue | ||
| } | ||
|
|
||
| if realization.CorrectsRealizationID == nil || *realization.CorrectsRealizationID == "" { | ||
| errs = append(errs, fmt.Errorf("realizations[%d]: corrects realization id is required for corrections", idx)) | ||
| } | ||
| } | ||
|
|
||
| return errors.Join(errs...) |
Contributor
There was a problem hiding this comment.
Validate the realization payload before writeback.
This method still derives writeback amounts from realization.Amount.Abs(), so checking only CorrectsRealizationID leaves a malformed correction slice able to consume coverage. Calling i.Realizations.Validate() here would make the writeback contract as strict as CreateInitialLineagesInput.
Small fix
func (i WritebackCorrectionLineageSegmentsInput) Validate() error {
var errs []error
if i.Namespace == "" {
errs = append(errs, errors.New("namespace is required"))
}
+ if err := i.Realizations.Validate(); err != nil {
+ errs = append(errs, fmt.Errorf("realizations: %w", err))
+ }
for idx, realization := range i.Realizations {
if realization.Type != creditrealization.TypeCorrection {
continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (i WritebackCorrectionLineageSegmentsInput) Validate() error { | |
| var errs []error | |
| if i.Namespace == "" { | |
| errs = append(errs, errors.New("namespace is required")) | |
| } | |
| for idx, realization := range i.Realizations { | |
| if realization.Type != creditrealization.TypeCorrection { | |
| continue | |
| } | |
| if realization.CorrectsRealizationID == nil || *realization.CorrectsRealizationID == "" { | |
| errs = append(errs, fmt.Errorf("realizations[%d]: corrects realization id is required for corrections", idx)) | |
| } | |
| } | |
| return errors.Join(errs...) | |
| func (i WritebackCorrectionLineageSegmentsInput) Validate() error { | |
| var errs []error | |
| if i.Namespace == "" { | |
| errs = append(errs, errors.New("namespace is required")) | |
| } | |
| if err := i.Realizations.Validate(); err != nil { | |
| errs = append(errs, fmt.Errorf("realizations: %w", err)) | |
| } | |
| for idx, realization := range i.Realizations { | |
| if realization.Type != creditrealization.TypeCorrection { | |
| continue | |
| } | |
| if realization.CorrectsRealizationID == nil || *realization.CorrectsRealizationID == "" { | |
| errs = append(errs, fmt.Errorf("realizations[%d]: corrects realization id is required for corrections", idx)) | |
| } | |
| } | |
| return errors.Join(errs...) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/billing/charges/lineage/service.go` around lines 64 - 81, The
current WritebackCorrectionLineageSegmentsInput.Validate() only checks
CorrectsRealizationID for correction-type realizations and misses full payload
validation (e.g., malformed amounts); update the method to invoke
i.Realizations.Validate() early (and return or append its error(s)) so the
writeback validation is as strict as CreateInitialLineagesInput; ensure you call
Realizations.Validate() inside
WritebackCorrectionLineageSegmentsInput.Validate() and incorporate its returned
error(s) into the final errors.Join result.
tothandras
reviewed
Apr 9, 2026
| } | ||
|
|
||
| flatFeeHandler := NewChargesFlatFeeHandler(ledgerService, accountResolver, accountService) | ||
| collectorService := NewChargesCollectorService(ledgerService, accountResolver, accountService) |
Contributor
There was a problem hiding this comment.
NewChargesFlatFeeService, NewChargesUsageBasedService, and NewChargesCreditPurchaseService all require a lineage.Service in their config now (Config.Validate() returns "lineage service cannot be null"), but none of them receive one here. The server will crash on startup.
Fix: create a lineage adapter + service here (same pattern as testutils/service.go:114-126) and pass it to all three service constructors. Same issue in customerbalance.go where the flat-fee and usage-based services are also constructed without lineage.
turip
reviewed
Apr 9, 2026
|
|
||
| func (CreditRealizationLineage) Fields() []ent.Field { | ||
| return []ent.Field{ | ||
| field.String("root_realization_id"). |
Member
There was a problem hiding this comment.
I would add at least a charge_id here and an FK to the charges table. It's not required for the code but will make our life easier when trying to find realizations.
Contributor
Author
There was a problem hiding this comment.
FK is fair, adding it, but if you dont mind i wouldn't necessarily add the extra fields you can always just write simple sql to solve that
Comment on lines
+18
to
+34
| if err := s.lineage.CreateInitialLineages(ctx, lineage.CreateInitialLineagesInput{ | ||
| Namespace: charge.Namespace, | ||
| CustomerID: charge.Intent.CustomerID, | ||
| Currency: charge.Intent.Currency, | ||
| Realizations: realizations, | ||
| }); err != nil { | ||
| return creditrealization.Realizations{}, fmt.Errorf("create initial credit realization lineages: %w", err) | ||
| } | ||
|
|
||
| if err := s.lineage.WritebackCorrectionLineageSegments(ctx, lineage.WritebackCorrectionLineageSegmentsInput{ | ||
| Namespace: charge.Namespace, | ||
| Realizations: realizations, | ||
| }); err != nil { | ||
| return creditrealization.Realizations{}, fmt.Errorf("write back correction lineage segments: %w", err) | ||
| } | ||
|
|
||
| lineage.AttachInitialActiveLineageSegments(realizations) |
Member
There was a problem hiding this comment.
Let's compact this into a single call (e.g. Create can receive everything required to be created).
|
|
||
| type Service interface { | ||
| CreateInitialLineages(ctx context.Context, input CreateInitialLineagesInput) error | ||
| WritebackCorrectionLineageSegments(ctx context.Context, input WritebackCorrectionLineageSegmentsInput) error |
Member
There was a problem hiding this comment.
Writeback is a strange word here. Update, Persist, etc might be better
Contributor
Author
There was a problem hiding this comment.
I see your point, it felt very normal to me but i'm biased :D Persist works for me
| type Service interface { | ||
| CreateInitialLineages(ctx context.Context, input CreateInitialLineagesInput) error | ||
| WritebackCorrectionLineageSegments(ctx context.Context, input WritebackCorrectionLineageSegmentsInput) error | ||
| BackfillAdvanceLineageSegments(ctx context.Context, input BackfillAdvanceLineageSegmentsInput) error |
Member
There was a problem hiding this comment.
This is not really a backfill, but rather the result of cost basis being known. May RecognizeAdvanceLineageSegements would be a better name.
Honestly for me Backfill doesn't really capture what this does.
Contributor
Author
There was a problem hiding this comment.
"Backfilling the Advance" is the phrase i've been using throughout. You're technically right in the sense that it does more than just backfilling the advanced receivable (which it does do) but also "attributes cost-basis" on the accrued accounts
If you feel strongly about this lets talk but then we should rename this things throughout the code
Member
There was a problem hiding this comment.
Then at least add a docstring on what it does, as right now I need to take a look at the implementation to see what's happening there.
The issue is that right now you are in context, but zooming out backfill feels like a db migration for me. It's just an overloaded word when not being laser focused on the ledger.
Comment on lines
+15
to
+16
| LineageOriginKindRealCredit LineageOriginKind = "real_credit" | ||
| LineageOriginKindAdvance LineageOriginKind = "advance" |
Member
There was a problem hiding this comment.
Please add description to these constants for the benefit of future readers.
Also maybe, as that's the thing we are capturing here:
Suggested change
| LineageOriginKindRealCredit LineageOriginKind = "real_credit" | |
| LineageOriginKindAdvance LineageOriginKind = "advance" | |
| LineageOriginKindRealized LineageOriginKind = "realized" | |
| LineageOriginKindAdvance LineageOriginKind = "advance" |
|
|
||
| const AnnotationLineageOriginKind = "billing.credit_realization.lineage_origin_kind" | ||
|
|
||
| type LineageOriginKind string |
Member
There was a problem hiding this comment.
Question: does it make sense to store this? I would think that this is not needed as the segments capture this information already.
Contributor
Author
There was a problem hiding this comment.
wdym? the segments don't rly capture this, once this is know they're written back after the fact
technically we could get this if we fetch the realization + some but its just easier to localize this (it basically captures the tx typename for the original usage acknowledgement)
Comment on lines
+57
to
+59
| LineageSegmentStateRealCredit LineageSegmentState = "real_credit" | ||
| LineageSegmentStateAdvanceUncovered LineageSegmentState = "advance_uncovered" | ||
| LineageSegmentStateAdvanceBackfilled LineageSegmentState = "advance_backfilled" |
Member
There was a problem hiding this comment.
Please add docstrings for the benefit of the next one reading this part :D
turip
approved these changes
Apr 9, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
openmeter/billing/charges/usagebased/service/creditsonly.go (1)
129-136: Batch the lineage fetch before the run loop.
LoadActiveSegmentsByRealizationIDalready takes a slice, but this version does one DB call per realization run. On charges with a lot of runs, delete/correction turns into an N+1 query pattern for no real gain. I'd load all realization IDs once, then reuse the returned map per run in memory. As per coding guidelines, performance should be a priority in critical code paths, including database operations.🤖 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 129 - 136, The code currently calls s.Service.lineage.LoadActiveSegmentsByRealizationID inside the loop over s.Charge.Realizations, causing an N+1 DB query pattern; instead, collect all realization IDs across s.Charge.Realizations (map/concat the IDs from each run.CreditsAllocated), call s.Service.lineage.LoadActiveSegmentsByRealizationID once to get lineageSegmentsByRealization, then inside the loop reuse that returned map for each run (lookup by realization ID) rather than making per-run DB calls; update error handling to reflect the single batched call and remove the per-run LoadActiveSegmentsByRealizationID invocation.openmeter/billing/charges/models/creditrealization/lineage.go (1)
12-17: Consider adding descriptions toLineageOriginKindconstants.The
LineageSegmentStateconstants have nice docstrings (lines 57-65) explaining what each state represents. It would be helpful to add similar descriptions toLineageOriginKindRealCreditandLineageOriginKindAdvancefor consistency and to help future readers understand what each origin represents.📝 Suggested docstrings
const ( + // LineageOriginKindRealCredit marks a realization that originated from + // actual credit balance (FBO subaccount consumption). LineageOriginKindRealCredit LineageOriginKind = "real_credit" + // LineageOriginKindAdvance marks a realization that originated from + // an advance (receivable-backed usage before credit purchase). LineageOriginKindAdvance LineageOriginKind = "advance" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/models/creditrealization/lineage.go` around lines 12 - 17, Add concise doc comments for the LineageOriginKind constants so readers know what each origin represents: add comments above LineageOriginKindRealCredit and LineageOriginKindAdvance (referencing the LineageOriginKind type and the constants LineageOriginKindRealCredit and LineageOriginKindAdvance) describing their meanings (e.g., that real_credit denotes credits from actual invoices/payments and advance denotes prepaid/advance payments) to match the existing style used for LineageSegmentState.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tools/migrate/migrations/20260409130630_add_credit_realization_lineage.up.sql`:
- Around line 14-15: Remove the redundant UNIQUE index creation for the
primary-key column: delete the CREATE UNIQUE INDEX "creditrealizationlineage_id"
ON "credit_realization_lineages" ("id") statement (and the similar duplicate at
lines 36-37) from the up migration, and also remove the matching DROP INDEX
statements from the down migration so you don't maintain duplicate indexes over
PRIMARY KEY ("id"); leave the table definitions and PKs intact.
---
Nitpick comments:
In `@openmeter/billing/charges/models/creditrealization/lineage.go`:
- Around line 12-17: Add concise doc comments for the LineageOriginKind
constants so readers know what each origin represents: add comments above
LineageOriginKindRealCredit and LineageOriginKindAdvance (referencing the
LineageOriginKind type and the constants LineageOriginKindRealCredit and
LineageOriginKindAdvance) describing their meanings (e.g., that real_credit
denotes credits from actual invoices/payments and advance denotes
prepaid/advance payments) to match the existing style used for
LineageSegmentState.
In `@openmeter/billing/charges/usagebased/service/creditsonly.go`:
- Around line 129-136: The code currently calls
s.Service.lineage.LoadActiveSegmentsByRealizationID inside the loop over
s.Charge.Realizations, causing an N+1 DB query pattern; instead, collect all
realization IDs across s.Charge.Realizations (map/concat the IDs from each
run.CreditsAllocated), call s.Service.lineage.LoadActiveSegmentsByRealizationID
once to get lineageSegmentsByRealization, then inside the loop reuse that
returned map for each run (lookup by realization ID) rather than making per-run
DB calls; update error handling to reflect the single batched call and remove
the per-run LoadActiveSegmentsByRealizationID invocation.
🪄 Autofix (Beta)
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
Run ID: b3c3b3eb-6c08-4a6f-aeb3-30ae6e713359
⛔ Files ignored due to path filters (17)
openmeter/ent/db/charge.gois excluded by!**/ent/db/**openmeter/ent/db/charge/charge.gois excluded by!**/ent/db/**openmeter/ent/db/charge/where.gois excluded by!**/ent/db/**openmeter/ent/db/charge_create.gois excluded by!**/ent/db/**openmeter/ent/db/charge_query.gois excluded by!**/ent/db/**openmeter/ent/db/charge_update.gois excluded by!**/ent/db/**openmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage/creditrealizationlineage.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage/where.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_create.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_query.gois excluded by!**/ent/db/**openmeter/ent/db/creditrealizationlineage_update.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (24)
app/common/charges.goapp/common/customerbalance.goopenmeter/billing/charges/flatfee/adapter/charge.goopenmeter/billing/charges/flatfee/handler.goopenmeter/billing/charges/flatfee/service/creditsonly.goopenmeter/billing/charges/flatfee/service/lineage.goopenmeter/billing/charges/lineage/adapter/lineage.goopenmeter/billing/charges/lineage/lineage.goopenmeter/billing/charges/lineage/service.goopenmeter/billing/charges/lineage/service/service.goopenmeter/billing/charges/models/creditrealization/lineage.goopenmeter/billing/charges/service/lineage_test.goopenmeter/billing/charges/usagebased/adapter/charge.goopenmeter/billing/charges/usagebased/handler.goopenmeter/billing/charges/usagebased/service/creditsonly.goopenmeter/billing/charges/usagebased/service/lineage.goopenmeter/ent/schema/charges.goopenmeter/ent/schema/creditrealizationlineage.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/usagebased.goopenmeter/ledger/collector/correct.goopenmeter/ledger/collector/service.gotools/migrate/migrations/20260409130630_add_credit_realization_lineage.down.sqltools/migrate/migrations/20260409130630_add_credit_realization_lineage.up.sql
✅ Files skipped from review due to trivial changes (6)
- openmeter/billing/charges/flatfee/adapter/charge.go
- openmeter/billing/charges/usagebased/adapter/charge.go
- openmeter/billing/charges/service/lineage_test.go
- openmeter/billing/charges/lineage/service.go
- openmeter/ledger/collector/correct.go
- openmeter/ledger/collector/service.go
🚧 Files skipped from review as they are similar to previous changes (4)
- openmeter/billing/charges/usagebased/service/lineage.go
- openmeter/billing/charges/flatfee/service/creditsonly.go
- openmeter/ent/schema/creditrealizationlineage.go
- openmeter/billing/charges/lineage/adapter/lineage.go
Comment on lines
+14
to
+15
| -- create index "creditrealizationlineage_id" to table: "credit_realization_lineages" | ||
| CREATE UNIQUE INDEX "creditrealizationlineage_id" ON "credit_realization_lineages" ("id"); |
Contributor
There was a problem hiding this comment.
Drop the duplicate PK indexes.
Both tables already get a unique index from PRIMARY KEY ("id"), so these extra UNIQUE INDEX ... ("id") statements just add extra index maintenance on every write. I'd remove them here and trim the matching DROP INDEX lines from the down migration too.
Also applies to: 36-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tools/migrate/migrations/20260409130630_add_credit_realization_lineage.up.sql`
around lines 14 - 15, Remove the redundant UNIQUE index creation for the
primary-key column: delete the CREATE UNIQUE INDEX "creditrealizationlineage_id"
ON "credit_realization_lineages" ("id") statement (and the similar duplicate at
lines 36-37) from the up migration, and also remove the matching DROP INDEX
statements from the down migration so you don't maintain duplicate indexes over
PRIMARY KEY ("id"); leave the table definitions and PKs intact.
This was referenced Apr 10, 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
3 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.
Summary
This PR refactors credit-only charge accrual/correction handling around a new shared ledger collector and adds lineage tracking for credit realizations.
What changed
credit_realization_lineage+ segments) to track what each realization currently represents over timecharges/lineageservice, and moved lineage triggering into the charge service/state-machine layer instead of hiding it inside adaptersWhy
Test coverage
Review Guidance
A good way to review this PR is in this order:
openmeter/ledger/collector/...openmeter/billing/charges/models/creditrealization/...andopenmeter/ent/schema/creditrealizationlineage.goopenmeter/billing/charges/lineage/...openmeter/billing/charges/flatfee/...,.../usagebased/...,.../creditpurchase/...test/credits/...andopenmeter/billing/charges/service/lineage_test.goBusiness Behavior Gotchas
FBOas available credit. It does not automatically re-cover other uncovered advance.cost_basis=nilreceivable represents uncovered advance exposure; purchasedcost_basisreceivable represents the purchase-side payment obligation.advance_backfilledadvance_uncoveredreal_creditSummary by CodeRabbit
New Features
Tests