feat(backend): implement invoice settlement mode for credits#3976
Open
mark-vass-konghq wants to merge 1 commit intomainfrom
Open
feat(backend): implement invoice settlement mode for credits#3976mark-vass-konghq wants to merge 1 commit intomainfrom
mark-vass-konghq wants to merge 1 commit intomainfrom
Conversation
mark-vass-konghq
added
the
release-note/feature
Contributor
📝 WalkthroughWalkthroughAdds invoiced-payment support for credit-purchase charges: adapter CRUD for invoiced payments, DB→domain mapping of InvoiceSettlement, service lifecycle for linking/authorizing/settling invoiced payments, and deferred invoicing integration into charge creation with post-transaction linking. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ChargeService
participant BillingService
participant CreditPurchaseService
participant Adapter
participant Handler
participant DB
Client->>ChargeService: Create CreditPurchase (Settlement=Invoice)
activate ChargeService
ChargeService->>CreditPurchaseService: onInvoiceSettlementPurchase
activate CreditPurchaseService
CreditPurchaseService->>Handler: OnCreditPurchaseInitiated
Handler-->>CreditPurchaseService: TimedGroupReference
CreditPurchaseService->>Adapter: UpdateCharge (persist realization)
Adapter->>DB: persist charge
deactivate CreditPurchaseService
ChargeService->>BillingService: Create pending invoice lines
BillingService->>DB: create pending invoice & lines
BillingService-->>ChargeService: pending line(s)
ChargeService->>BillingService: InvoicePendingLines (if due)
BillingService->>DB: finalize invoice
BillingService-->>ChargeService: invoiceID
ChargeService->>CreditPurchaseService: LinkInvoicedPayment(invoiceID, lineID)
activate CreditPurchaseService
CreditPurchaseService->>Adapter: CreateInvoicedPayment
Adapter->>DB: create invoiced payment
Adapter-->>CreditPurchaseService: invoiced payment
CreditPurchaseService->>Adapter: UpdateCharge (set InvoiceSettlement)
Adapter->>DB: persist charge
deactivate CreditPurchaseService
deactivate ChargeService
Client->>BillingService: Payment trigger (Authorized)
BillingService->>CreditPurchaseService: HandleInvoicePaymentAuthorized(invoiceID,line)
activate CreditPurchaseService
CreditPurchaseService->>Handler: OnCreditPurchasePaymentAuthorized
CreditPurchaseService->>Adapter: UpdateInvoicedPayment (record authorized TR)
Adapter->>DB: update payment
CreditPurchaseService->>Adapter: UpdateCharge
Adapter->>DB: persist charge
deactivate CreditPurchaseService
Client->>BillingService: Payment trigger (Paid)
BillingService->>CreditPurchaseService: HandleInvoicePaymentSettled(invoiceID,line)
activate CreditPurchaseService
CreditPurchaseService->>Handler: OnCreditPurchasePaymentSettled
CreditPurchaseService->>Adapter: UpdateInvoicedPayment (record settled TR & status)
Adapter->>DB: update payment
CreditPurchaseService->>Adapter: UpdateCharge (finalize)
Adapter->>DB: persist charge
deactivate CreditPurchaseService
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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)
📝 Coding Plan
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
mark-vass-konghq
force-pushed
the
feat/invoice-settlement-mode-for-credits
branch
from
8026b17 to
757056e
Compare
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
openmeter/billing/charges/service/creditpurchase_test.go (1)
363-565: Could we add a case for the deferred-invoicing branch too?This test covers the happy path where the gathering line is turned into a standard invoice during
Create(). The new flow inhandleInvoiceCreditPurchasePostCreate()also has acreatedLine.InvoiceAt.After(clock.Now())branch, and that’s the lifecycle edge most likely to regress becauseInvoiceSettlementstays unset until later.As per coding guidelines
**/*_test.go: 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/billing/charges/service/creditpurchase_test.go` around lines 363 - 565, Add a new sub-test to TestStandardInvoiceCreditPurchase that exercises the deferred-invoicing branch (createdLine.InvoiceAt.After(clock.Now())) in handleInvoiceCreditPurchasePostCreate: create a credit purchase intent whose invoice date is in the future (so InvoiceSettlement remains nil at Create()), assert in the "initiated" callback that charge.State.InvoiceSettlement is nil, then simulate the passage to invoice creation (invoke whatever path creates/archives the invoice or call the same CustomInvoicingService trigger used later) and finally assert that after the invoice is created and approved the charge's State.InvoiceSettlement is set and populated (check InvoiceID, Authorized/Settled group IDs and final charge status). Reference TestStandardInvoiceCreditPurchase, handleInvoiceCreditPurchasePostCreate, createdLine.InvoiceAt.After(clock.Now()), and InvoiceSettlement to locate where to add and what to assert.
🤖 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/creditpurchase/chargecreditpurchase.go`:
- Around line 99-123: The validateOptionalValidator function currently checks v
== nil but fails to handle typed-nil values stored in the validator interface
(causing panics when calling v.Validate()); update validateOptionalValidator to
detect an underlying nil pointer before calling Validate by either using a type
switch on validator to check each pointer concrete type for nil or by using
reflection (reflect.ValueOf(v).Kind()==Ptr && reflect.ValueOf(v).IsNil()) to
return nil early; ensure you still call v.Validate() only when the underlying
value is non-nil so functions like validateOptionalValidator,
validator.Validate, and callers (e.g., CreditGrantRealization,
ExternalPaymentSettlement, InvoiceSettlement) no longer panic.
In `@openmeter/billing/charges/creditpurchase/service/invoice.go`:
- Around line 49-61: Currently LinkInvoicedPayment seeds the
newPaymentSettlement (payment.InvoicedCreate) with Authorized and
StatusAuthorized using charge.State.CreditGrantRealization which makes the
settlement appear already authorized; change this to create the InvoicedCreate
without setting Authorized and leave Status as a pre-authorization state (e.g.,
payment.StatusPending or nil), and move the assignment of Authorized and the
transition to payment.StatusAuthorized into HandleInvoicePaymentAuthorized where
the actual authorization is handled, updating the same fields (Authorized and
Status) on the existing settlement record.
In `@openmeter/billing/charges/service/create.go`:
- Around line 261-297: The code only links invoiced payments when
createdLine.InvoiceAt is past/now; for future-dated lines you must record the
pending linkage so later invoice callbacks can complete it. When
createdLine.InvoiceAt is in the future, set the charge's invoice-settlement
information (e.g., populate charge.State.InvoiceSettlement or the equivalent
field with the pending line ID createdLine.ID and any needed invoice/customer
info) and persist the charge immediately using the same persistence path used
elsewhere in this flow so that later handlers can call
s.creditPurchaseService.LinkInvoicedPayment(charge, ...) and finish linking;
ensure the persistence uses the existing charge save/update method used in this
service so state is durable.
- Around line 160-169: The loop calling handleInvoiceCreditPurchasePostCreate()
can surface a hard error after the charge is already committed; change the flow
so Create() does not return a hard error post-commit: mark pending
invoice-credit purchases durable (e.g., set a processed/pending flag tied to the
charge ID) inside the main transaction, then perform
handleInvoiceCreditPurchasePostCreate() as a best-effort background/async step
(or catch errors, log them, and attach a non-fatal warning/state to the returned
charges) instead of returning fmt.Errorf; also make
handleInvoiceCreditPurchasePostCreate() idempotent by deduping on the
charge/transaction ID so retries/background processing cannot double-apply
credits (references: Create, result.pendingInvoiceCreditPurchases,
handleInvoiceCreditPurchasePostCreate, out, charges.NewCharge).
- Around line 23-26: The file fails the repository's Go formatting/import
ordering (gci) check for the createResult declaration; run gofmt and the
project's import sorter (gci or goimports as configured) on the file containing
the createResult struct so the struct definition (type createResult and its
fields charges.Charges and pendingInvoiceCreditPurchases of
charges.WithIndex[*creditPurchaseWithPendingGatheringLine]) is formatted and
imports reordered to match repo style, then stage the resulting changes.
In `@openmeter/billing/charges/service/invoice.go`:
- Around line 33-47: The credit-purchase authorization can be executed twice
because this file routes credit purchases to
creditPurchaseService.HandleInvoicePaymentAuthorized on
StandardInvoiceStatusPaymentProcessingPending while the credit-purchase service
already links the settlement in payment.StatusAuthorized; to fix, either stop
routing here (make the creditPurchase entry a no-op like in the Issued branch)
or make the called handler idempotent by short-circuiting when the settlement is
already authorized — implement the chosen fix by updating the processorByType
mapping in handleChargeEvent (the creditPurchase function assigned here) or by
adding an early return in creditpurchase.Service.HandleInvoicePaymentAuthorized
that checks the settlement/payment status and returns nil if already authorized.
---
Nitpick comments:
In `@openmeter/billing/charges/service/creditpurchase_test.go`:
- Around line 363-565: Add a new sub-test to TestStandardInvoiceCreditPurchase
that exercises the deferred-invoicing branch
(createdLine.InvoiceAt.After(clock.Now())) in
handleInvoiceCreditPurchasePostCreate: create a credit purchase intent whose
invoice date is in the future (so InvoiceSettlement remains nil at Create()),
assert in the "initiated" callback that charge.State.InvoiceSettlement is nil,
then simulate the passage to invoice creation (invoke whatever path
creates/archives the invoice or call the same CustomInvoicingService trigger
used later) and finally assert that after the invoice is created and approved
the charge's State.InvoiceSettlement is set and populated (check InvoiceID,
Authorized/Settled group IDs and final charge status). Reference
TestStandardInvoiceCreditPurchase, handleInvoiceCreditPurchasePostCreate,
createdLine.InvoiceAt.After(clock.Now()), and InvoiceSettlement to locate where
to add and what to assert.
🪄 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: cd5a7b49-62cf-4812-a0e6-f047b09bd57a
📒 Files selected for processing (12)
openmeter/billing/charges/creditpurchase/adapter.goopenmeter/billing/charges/creditpurchase/adapter/charge.goopenmeter/billing/charges/creditpurchase/adapter/mapper.goopenmeter/billing/charges/creditpurchase/adapter/payment.goopenmeter/billing/charges/creditpurchase/chargecreditpurchase.goopenmeter/billing/charges/creditpurchase/service.goopenmeter/billing/charges/creditpurchase/service/create.goopenmeter/billing/charges/creditpurchase/service/invoice.goopenmeter/billing/charges/service/create.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/billing/charges/service/invoice.goopenmeter/billing/charges/service/invoicependinglines.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
openmeter/billing/charges/creditpurchase/chargecreditpurchase.go (1)
118-131: Nice fix for the typed-nil interface issue!This correctly handles the tricky Go semantics where a nil pointer wrapped in an interface isn't
== nil. Small optimization: you're callingreflect.ValueOf(v)twice.♻️ Optional: cache the reflect.Value
func validateOptionalValidator(v validator, name string) error { // Handle typed-nil values: when a nil pointer is stored in an interface, // the interface itself is not nil, but the underlying value is. if v == nil { return nil } - if reflect.ValueOf(v).Kind() == reflect.Ptr && reflect.ValueOf(v).IsNil() { + rv := reflect.ValueOf(v) + if rv.Kind() == reflect.Ptr && rv.IsNil() { return nil } if err := v.Validate(); err != nil { return fmt.Errorf("%s: %w", name, err) } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/creditpurchase/chargecreditpurchase.go` around lines 118 - 131, In validateOptionalValidator, avoid calling reflect.ValueOf(v) twice by assigning rv := reflect.ValueOf(v) once and using rv.Kind() and rv.IsNil() for the nil-pointer checks; keep the same behavior and error wrapping using v.Validate() and fmt.Errorf with the provided name.openmeter/billing/charges/creditpurchase/service/invoice.go (1)
86-93: Idempotency check is clever but warrants a comment clarification.The logic is: if
InvoiceSettlement.Authorizedexists AND has a different transaction group ID thanCreditGrantRealization, then this handler already ran (sinceLinkInvoicedPaymentcopies the same ID, but this handler generates a new one). This is subtle - consider adding a brief inline explanation for future readers.📝 Clearer comment
// Idempotency check: if already authorized via invoice approval (not just initial credit grant), skip processing. - // LinkInvoicedPayment sets Authorized to CreditGrantRealization, so we check if it's been updated - // by this handler (which would have a different transaction group ID). + // LinkInvoicedPayment initially copies CreditGrantRealization's TransactionGroupID into Authorized. + // This handler generates a NEW TransactionGroupID when processing. So if Authorized.TransactionGroupID + // differs from CreditGrantRealization's, this handler already ran and we skip to avoid double-processing. if charge.State.InvoiceSettlement.Authorized != nil && charge.State.CreditGrantRealization != nil && charge.State.InvoiceSettlement.Authorized.TransactionGroupID != charge.State.CreditGrantRealization.GroupReference.TransactionGroupID { return nil }🤖 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 86 - 93, Clarify the idempotency check in invoice.go by adding a concise inline comment next to the conditional that explains the logic: state when InvoiceSettlement.Authorized exists and its TransactionGroupID differs from CreditGrantRealization.GroupReference.TransactionGroupID means this handler already ran (because LinkInvoicedPayment copies the original group ID while this handler generates a new one), so we should return; reference the symbols InvoiceSettlement.Authorized, CreditGrantRealization, LinkInvoicedPayment, and TransactionGroupID in the comment to make the intent obvious to future readers.openmeter/billing/charges/service/create.go (2)
160-174: Silent error swallowing is intentional but could use observability.The comment explains the rationale well - charges are already committed, and this is best-effort. However, silently dropping errors makes debugging tricky in production. Consider logging these failures so operators can spot patterns or manually intervene.
💡 Add structured logging for visibility
lo.ForEach(result.pendingInvoiceCreditPurchases, func(pending charges.WithIndex[*creditPurchaseWithPendingGatheringLine], _ int) { updatedCharge, err := s.handleInvoiceCreditPurchasePostCreate(ctx, pending.Value) if err != nil { // Skip this charge on error - it has already been committed and can be // processed later. The charge will not have InvoiceSettlement set. + s.logger.Warn("post-create handling failed for invoice credit purchase", + "charge_id", pending.Value.charge.ID, + "customer_id", pending.Value.customerID.ID, + "error", err, + ) return } out[pending.Index] = charges.NewCharge(updatedCharge) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/create.go` around lines 160 - 174, The post-create loop over result.pendingInvoiceCreditPurchases currently swallows errors; update it to log structured error details when s.handleInvoiceCreditPurchasePostCreate(ctx, pending.Value) returns an error while preserving the current behavior of skipping the charge. Specifically, add a processLogger (or existing logger) call that includes the charge id or pending.Value identifier, pending.Index, and the error returned from handleInvoiceCreditPurchasePostCreate so operators can monitor failures, but still return/continue without altering out[pending.Index] on error.
259-274: Acknowledged: duplicate gathering lines on retry are a known gap.The comment at lines 260-262 clearly documents this limitation. For future hardening, you could query by
ChargeIDbefore creating, or persist a partialInvoiceSettlementwith the gathering line ID immediately after creation. Not blocking for this PR since it's documented, but worth tracking.Want me to open an issue to track implementing deduplication for gathering line creation on retries?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/create.go` around lines 259 - 274, The current code calling s.billingService.CreatePendingInvoiceLines with pending.gatheringLine can create duplicate gathering lines on retries; before creating, call a billing-side lookup (e.g., implement and use a method like billingService.FindGatheringLinesByChargeID or ListPendingInvoiceLines filtered by pending.chargeID) and if a matching gathering line already exists skip CreatePendingInvoiceLines, otherwise call CreatePendingInvoiceLines and immediately persist a partial InvoiceSettlement containing the new gathering line ID (or record the created line ID on the pending object) so retries detect the existing line and no duplicate is created; update the logic around CreatePendingInvoiceLines and ensure checks use pending.chargeID and pending.gatheringLine identifiers.
🤖 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/creditpurchase/service/invoice.go`:
- Line 15: The function name has a typo: rename onInvoceSettlementPurchase to
onInvoiceSettlementPurchase and update all references/call sites and any tests
or interface implementations that use onInvoceSettlementPurchase to the
corrected name; ensure any exported/internal usages, documentation, and mocks
reflect the new identifier so builds and references remain consistent.
In `@openmeter/billing/charges/service/create.go`:
- Around line 290-292: The check that only inspects invoices[0] is brittle;
iterate all returned invoices from InvoicePendingLines (the invoices slice) and
search each invoice's Lines.OrEmpty() for the matching ChargeID rather than
assuming invoices[0] contains the standard line; if no matching line is found
across all invoices return the existing error ("no standard invoice created for
credit purchase" or "standard line not found") so downstream logic (the later
loop that uses ChargeID) will always have the correct line regardless of which
invoice it landed on.
---
Nitpick comments:
In `@openmeter/billing/charges/creditpurchase/chargecreditpurchase.go`:
- Around line 118-131: In validateOptionalValidator, avoid calling
reflect.ValueOf(v) twice by assigning rv := reflect.ValueOf(v) once and using
rv.Kind() and rv.IsNil() for the nil-pointer checks; keep the same behavior and
error wrapping using v.Validate() and fmt.Errorf with the provided name.
In `@openmeter/billing/charges/creditpurchase/service/invoice.go`:
- Around line 86-93: Clarify the idempotency check in invoice.go by adding a
concise inline comment next to the conditional that explains the logic: state
when InvoiceSettlement.Authorized exists and its TransactionGroupID differs from
CreditGrantRealization.GroupReference.TransactionGroupID means this handler
already ran (because LinkInvoicedPayment copies the original group ID while this
handler generates a new one), so we should return; reference the symbols
InvoiceSettlement.Authorized, CreditGrantRealization, LinkInvoicedPayment, and
TransactionGroupID in the comment to make the intent obvious to future readers.
In `@openmeter/billing/charges/service/create.go`:
- Around line 160-174: The post-create loop over
result.pendingInvoiceCreditPurchases currently swallows errors; update it to log
structured error details when s.handleInvoiceCreditPurchasePostCreate(ctx,
pending.Value) returns an error while preserving the current behavior of
skipping the charge. Specifically, add a processLogger (or existing logger) call
that includes the charge id or pending.Value identifier, pending.Index, and the
error returned from handleInvoiceCreditPurchasePostCreate so operators can
monitor failures, but still return/continue without altering out[pending.Index]
on error.
- Around line 259-274: The current code calling
s.billingService.CreatePendingInvoiceLines with pending.gatheringLine can create
duplicate gathering lines on retries; before creating, call a billing-side
lookup (e.g., implement and use a method like
billingService.FindGatheringLinesByChargeID or ListPendingInvoiceLines filtered
by pending.chargeID) and if a matching gathering line already exists skip
CreatePendingInvoiceLines, otherwise call CreatePendingInvoiceLines and
immediately persist a partial InvoiceSettlement containing the new gathering
line ID (or record the created line ID on the pending object) so retries detect
the existing line and no duplicate is created; update the logic around
CreatePendingInvoiceLines and ensure checks use pending.chargeID and
pending.gatheringLine identifiers.
🪄 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: 13a8615a-b91a-421a-8c80-8bd7d135049b
📒 Files selected for processing (3)
openmeter/billing/charges/creditpurchase/chargecreditpurchase.goopenmeter/billing/charges/creditpurchase/service/invoice.goopenmeter/billing/charges/service/create.go
mark-vass-konghq
force-pushed
the
feat/invoice-settlement-mode-for-credits
branch
from
4218be2 to
33bf9dc
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/charges/service/creditpurchase_test.go (1)
380-397:⚠️ Potential issue | 🟠 MajorPlease add coverage for the deferred invoice path too.
With
ServicePeriod.Fromfixed to January 1, 2026, this test only exercises the immediateInvoiceAt <= nowbranch. The new deferred path—pending line created first,InvoiceSettlementlinked later by invoice callbacks—is still uncovered, and that’s the branch most likely to regress here. A companion integration test with a future service period would make this change a lot safer.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. When appropriate, recommend e2e tests for critical changes."
Also applies to: 403-565
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/creditpurchase_test.go` around lines 380 - 397, The test only exercises the immediate invoice path because ServicePeriod.From is set to 2026-01-01 (making InvoiceAt <= now); add a companion test variant that sets the service period (createCreditPurchaseIntentInput.ServicePeriod.From) to a future date so the flow creates a pending line first and later links an InvoiceSettlement via the invoice callback path (exercise creditpurchase.NewSettlement/creditpurchase.InvoiceSettlement, InvoiceAt branch, and the pending-to-invoiced transition). In that new test, call CreateCreditPurchaseIntent with a future ServicePeriod.From, assert that the initial state is a pending line (no invoice created), then simulate the invoice callback that attaches the InvoiceSettlement and assert the invoice is created/linked and balances update accordingly; reuse existing helpers around CreateCreditPurchaseIntent and assertions to mirror the immediate-path test.
♻️ Duplicate comments (1)
openmeter/billing/charges/service/create.go (1)
245-267:⚠️ Potential issue | 🟠 MajorThe future-dated branch still isn’t safely retryable.
On the
InvoiceAt > nowpath,InvoiceSettlementstays nil, so this guard never flips on a replay. Any retry/background rerun of this helper can callCreatePendingInvoiceLines()again for the same charge and materialize duplicate pending invoice lines. This needs a durable “line already created” marker, or a lookup byChargeIDbefore creating another line.Also applies to: 316-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/create.go` around lines 245 - 267, handleInvoiceCreditPurchasePostCreate can create duplicate pending invoice lines for future-dated charges because InvoiceSettlement stays nil; ensure idempotency by checking for an existing gathering line tied to the charge before calling billingService.CreatePendingInvoiceLines or by persisting a partial InvoiceSettlement (including the created gathering line ID) immediately after creating the gathering line. Concretely: in handleInvoiceCreditPurchasePostCreate, before calling CreatePendingInvoiceLines, query the billing side for existing gathering lines filtered by ChargeID (or use an existing lookup method) and return early if one exists; if you must create a line, write back a durable marker (e.g., set Charge.State.InvoiceSettlement with gatheringLine ID or persist a minimal settlement record) so subsequent retries see InvoiceSettlement non-nil and skip creation. Reference: handleInvoiceCreditPurchasePostCreate, Charge.State.InvoiceSettlement, pending.gatheringLine, billingService.CreatePendingInvoiceLines, and the alternative LinkInvoicedPayment / HandleInvoicePaymentAuthorized hooks mentioned in comments.
🧹 Nitpick comments (2)
openmeter/billing/charges/service/creditpurchase_test.go (1)
516-522: These DEBUG probes should probably become assertions or go away.The extra fetches are fine if we want to guard the transition, but right now most of the value is just log noise. If the intermediate invoice state matters, I’d assert it directly; otherwise I’d drop the debug scaffolding before merge.
Also applies to: 539-552
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/creditpurchase_test.go` around lines 516 - 522, The two intermediate "DEBUG" fetches in the test use BillingService.GetStandardInvoiceById (variables betweenInvoice, betweenErr and the later equivalent) and currently only log results; either remove these extra fetches entirely or convert them into explicit assertions about the intermediate invoice state (e.g., assert no error and assert betweenInvoice.Status equals the expected status) so they provide test value instead of noise—apply the same change to the similar block referenced around the later case (lines 539-552) that uses the same GetStandardInvoiceById calls.openmeter/billing/charges/service/create.go (1)
165-170: Please emit some telemetry when this best-effort step fails.Since the error is intentionally swallowed here, a stuck invoice-settlement charge becomes invisible to operators. Even a structured log and metric with the charge/customer context would make recovery a lot easier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/create.go` around lines 165 - 170, In the loop over result.pendingInvoiceCreditPurchases where s.handleInvoiceCreditPurchasePostCreate(ctx, pending.Value) currently swallows errors, emit telemetry when err != nil: log a structured error including identifying fields from pending.Value (e.g. charge ID, customer ID, invoice ID) and the error, and increment a failure metric/counter (e.g. invoice_settlement_post_create_failures) tagged with those identifiers; use the service's existing logger/telemetry client (the same object owning handleInvoiceCreditPurchasePostCreate, e.g. s.logger or s.metrics) so operators can detect and filter these best-effort failures even though the code continues to return and skip processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openmeter/billing/charges/service/creditpurchase_test.go`:
- Around line 380-397: The test only exercises the immediate invoice path
because ServicePeriod.From is set to 2026-01-01 (making InvoiceAt <= now); add a
companion test variant that sets the service period
(createCreditPurchaseIntentInput.ServicePeriod.From) to a future date so the
flow creates a pending line first and later links an InvoiceSettlement via the
invoice callback path (exercise
creditpurchase.NewSettlement/creditpurchase.InvoiceSettlement, InvoiceAt branch,
and the pending-to-invoiced transition). In that new test, call
CreateCreditPurchaseIntent with a future ServicePeriod.From, assert that the
initial state is a pending line (no invoice created), then simulate the invoice
callback that attaches the InvoiceSettlement and assert the invoice is
created/linked and balances update accordingly; reuse existing helpers around
CreateCreditPurchaseIntent and assertions to mirror the immediate-path test.
---
Duplicate comments:
In `@openmeter/billing/charges/service/create.go`:
- Around line 245-267: handleInvoiceCreditPurchasePostCreate can create
duplicate pending invoice lines for future-dated charges because
InvoiceSettlement stays nil; ensure idempotency by checking for an existing
gathering line tied to the charge before calling
billingService.CreatePendingInvoiceLines or by persisting a partial
InvoiceSettlement (including the created gathering line ID) immediately after
creating the gathering line. Concretely: in
handleInvoiceCreditPurchasePostCreate, before calling CreatePendingInvoiceLines,
query the billing side for existing gathering lines filtered by ChargeID (or use
an existing lookup method) and return early if one exists; if you must create a
line, write back a durable marker (e.g., set Charge.State.InvoiceSettlement with
gatheringLine ID or persist a minimal settlement record) so subsequent retries
see InvoiceSettlement non-nil and skip creation. Reference:
handleInvoiceCreditPurchasePostCreate, Charge.State.InvoiceSettlement,
pending.gatheringLine, billingService.CreatePendingInvoiceLines, and the
alternative LinkInvoicedPayment / HandleInvoicePaymentAuthorized hooks mentioned
in comments.
---
Nitpick comments:
In `@openmeter/billing/charges/service/create.go`:
- Around line 165-170: In the loop over result.pendingInvoiceCreditPurchases
where s.handleInvoiceCreditPurchasePostCreate(ctx, pending.Value) currently
swallows errors, emit telemetry when err != nil: log a structured error
including identifying fields from pending.Value (e.g. charge ID, customer ID,
invoice ID) and the error, and increment a failure metric/counter (e.g.
invoice_settlement_post_create_failures) tagged with those identifiers; use the
service's existing logger/telemetry client (the same object owning
handleInvoiceCreditPurchasePostCreate, e.g. s.logger or s.metrics) so operators
can detect and filter these best-effort failures even though the code continues
to return and skip processing.
In `@openmeter/billing/charges/service/creditpurchase_test.go`:
- Around line 516-522: The two intermediate "DEBUG" fetches in the test use
BillingService.GetStandardInvoiceById (variables betweenInvoice, betweenErr and
the later equivalent) and currently only log results; either remove these extra
fetches entirely or convert them into explicit assertions about the intermediate
invoice state (e.g., assert no error and assert betweenInvoice.Status equals the
expected status) so they provide test value instead of noise—apply the same
change to the similar block referenced around the later case (lines 539-552)
that uses the same GetStandardInvoiceById calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b289c8c6-1441-453d-92e4-afc7b5d5a175
📒 Files selected for processing (14)
openmeter/billing/adapter/invoiceapp.goopenmeter/billing/charges/creditpurchase/adapter.goopenmeter/billing/charges/creditpurchase/adapter/charge.goopenmeter/billing/charges/creditpurchase/adapter/mapper.goopenmeter/billing/charges/creditpurchase/adapter/payment.goopenmeter/billing/charges/creditpurchase/chargecreditpurchase.goopenmeter/billing/charges/creditpurchase/service.goopenmeter/billing/charges/creditpurchase/service/create.goopenmeter/billing/charges/creditpurchase/service/invoice.goopenmeter/billing/charges/service/create.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/billing/charges/service/invoice.goopenmeter/billing/charges/service/invoicependinglines.goopenmeter/billing/stdinvoice.go
🚧 Files skipped from review as they are similar to previous changes (9)
- openmeter/billing/charges/creditpurchase/service/create.go
- openmeter/billing/charges/creditpurchase/adapter/payment.go
- openmeter/billing/charges/creditpurchase/adapter/charge.go
- openmeter/billing/charges/service/invoice.go
- openmeter/billing/charges/service/invoicependinglines.go
- openmeter/billing/charges/creditpurchase/chargecreditpurchase.go
- openmeter/billing/charges/creditpurchase/service.go
- openmeter/billing/charges/creditpurchase/adapter/mapper.go
- openmeter/billing/charges/creditpurchase/service/invoice.go
turip
reviewed
Mar 20, 2026
Comment on lines
+160
to
174
| // Handle invoice credit purchase post-creation steps outside the main transaction | ||
| // to avoid lock contention with transactionForInvoiceManipulation. | ||
| // This is a best-effort operation - errors are silently ignored since the charge | ||
| // has already been committed. The gathering line creation and invoicing can be | ||
| // retried later via a background process or manual intervention. | ||
| lo.ForEach(result.pendingInvoiceCreditPurchases, func(pending charges.WithIndex[*creditPurchaseWithPendingGatheringLine], _ int) { | ||
| updatedCharge, err := s.handleInvoiceCreditPurchasePostCreate(ctx, pending.Value) | ||
| if err != nil { | ||
| // Skip this charge on error - it has already been committed and can be | ||
| // processed later. The charge will not have InvoiceSettlement set. | ||
| return | ||
| } | ||
|
|
||
| out[pending.Index] = charges.NewCharge(updatedCharge) | ||
| }) |
Member
There was a problem hiding this comment.
These are just gathering lines the same as with other charges. The transactionForInvoiceManipulation is Reentrant lock.
| // handleInvoiceCreditPurchasePostCreate handles the post-creation steps for invoice credit purchases. | ||
| // This MUST be called outside the Create transaction to avoid lock contention. | ||
| // This function is idempotent - it can be safely called multiple times. | ||
| func (s *service) handleInvoiceCreditPurchasePostCreate(ctx context.Context, pending *creditPurchaseWithPendingGatheringLine) (creditpurchase.Charge, error) { |
Member
There was a problem hiding this comment.
move to creditpruchase and follow the same format as with flat fee
Comment on lines
+62
to
+75
| // Set invoice description from the first gathering line if available | ||
| if len(inScopeLines) > 0 && inScopeLines[0].Description != nil { | ||
| err = s.billingService.UpdateInvoiceFields(ctx, billing.UpdateInvoiceFieldsInput{ | ||
| Invoice: billing.InvoiceID{ | ||
| Namespace: createdInvoice.Namespace, | ||
| ID: createdInvoice.ID, | ||
| }, | ||
| Description: inScopeLines[0].Description, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("updating invoice description: %w", err) | ||
| } | ||
| createdInvoice.Description = inScopeLines[0].Description | ||
| } |
Member
There was a problem hiding this comment.
Let's not do this we are not sure if there are multiple lines present or not so description can be anything.
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.
First implementation for invoice settlement mode for credits
Summary by CodeRabbit
New Features
Bug Fixes
Tests