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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds full invoiced-payment support for credit-purchase charges: adapter methods for invoiced payments, DB mapping and validation for InvoiceSettlement, service-side lifecycle (creation, link, authorize, settle), changed charge creation to emit optional gathering lines for deferred invoicing, and invoice integration updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as ChargeService
participant CP as CreditPurchaseService
participant Adapter
participant Handler
participant DB as Database
Client->>Service: Create(intent with Invoice settlement)
Service->>CP: Create(ctx, intent)
CP->>Adapter: CreateCharge(...)
Adapter->>DB: Insert charge
Adapter-->>CP: Return charge
CP->>Handler: OnCreditPurchaseInitiated()
Handler-->>CP: ledger timed-group ref
CP->>Adapter: UpdateCharge(charge with CreditGrantRealization)
Adapter->>DB: Update charge
CP-->>Service: Return ChargeWithGatheringLine (pending)
Note over Service,CP: Post-transaction
Service->>CP: LinkInvoicedPayment(ctx, charge, invoiceLine)
CP->>Adapter: CreateInvoicedPayment(chargeID, paymentCreate)
Adapter->>DB: Insert invoiced payment
Adapter-->>CP: Return invoiced payment
CP->>Adapter: UpdateCharge(charge with InvoiceSettlement)
Adapter->>DB: Update charge
Adapter-->>CP: Return updated charge
sequenceDiagram
participant Billing
participant Service as ChargeService
participant CP as CreditPurchaseService
participant Adapter
participant Handler
participant DB as Database
Billing->>Service: Invoice status change (Authorized)
Service->>CP: HandleInvoicePaymentAuthorized(ctx, charge, lineWithHeader)
CP->>Adapter: Ensure/Create or Update invoiced payment (Authorized)
Adapter->>DB: Upsert payment authorized info
Adapter-->>CP: Return payment
CP->>Handler: OnCreditPurchasePaymentAuthorized(...)
Handler-->>CP: ack
CP->>Adapter: UpdateCharge(charge with updated InvoiceSettlement)
Adapter->>DB: Update charge
Billing->>Service: Invoice status change (Paid)
Service->>CP: HandleInvoicePaymentSettled(ctx, charge, lineWithHeader)
CP->>Handler: OnCreditPurchasePaymentSettled(...)
Handler-->>CP: ack
CP->>Adapter: UpdateInvoicedPayment(payment with Settled)
Adapter->>DB: Update payment
CP->>Adapter: UpdateCharge(charge status Final)
Adapter->>DB: Update charge
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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
openmeter/billing/charges/creditpurchase/chargecreditpurchase.go
Outdated
Show resolved
Hide resolved
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
openmeter/billing/charges/creditpurchase/chargecreditpurchase.go
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
openmeter/billing/charges/service/create.go (2)
161-171:⚠️ Potential issue | 🟠 MajorDon't let request cancellation kill the post-commit settlement work.
After the transaction commits, this still reuses the caller
ctx. If the request times out or the client disconnects,handleInvoiceCreditPurchasePostCreate()can fail right away, and that failure is dropped here, so the charge looks created even though the invoice-side settlement never happened. This follow-up needs an owned context with its own timeout, or a durable async handoff.🤖 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 161 - 171, The post-commit loop uses the caller ctx which can be canceled by the request; instead create an independent context with its own timeout or use a durable async handoff before calling handleInvoiceCreditPurchasePostCreate so request cancellation won't abort settlement. In the lo.ForEach over result.pendingInvoiceCreditPurchases, replace passing the incoming ctx to handleInvoiceCreditPurchasePostCreate with a created background-derived context (e.g., context.WithTimeout(context.Background(), <shortDuration>)) and ensure you call cancel() after the call; alternatively enqueue the pending item to a background worker/queue owned by the service so handleInvoiceCreditPurchasePostCreate runs independently of the request lifetime.
205-223:⚠️ Potential issue | 🔴 CriticalThis still isn't idempotent after the first billing write.
InvoiceSettlement != nilonly protects the fully linked case. IfCreatePendingInvoiceLines()succeeds and the function exits beforeLinkInvoicedPayment()persists state — orcreatedLine.InvoiceAtis still in the future — the next retry will create another pending line for the same charge. On a billing flow, retrying a non-idempotent write like this can duplicate invoice artifacts for one credit purchase. Please persist a durable "pending settlement / created line id" marker before returning, or dedupe the pending-line creation byChargeID.🤖 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 205 - 223, The current idempotency check only looks at charge.State.InvoiceSettlement and does not prevent duplicate billing writes if CreatePendingInvoiceLines succeeds but state isn't persisted; modify the flow in the processing function so that after receiving result from s.billingService.CreatePendingInvoiceLines (the created gathering line), you persist a durable marker (e.g., set charge.State.InvoiceSettlement with the created line ID / a "pendingSettlement" token) before any other return points and before calling LinkInvoicedPayment, or alternatively query/dedupe existing gathering lines by ChargeID via the billing service before creating; update the code paths around s.billingService.CreatePendingInvoiceLines, pending.gatheringLine, and LinkInvoicedPayment to ensure the created line ID is stored atomically on the charge to prevent duplicate pending-line creation on retries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openmeter/billing/charges/service/create.go`:
- Around line 161-171: The post-commit loop uses the caller ctx which can be
canceled by the request; instead create an independent context with its own
timeout or use a durable async handoff before calling
handleInvoiceCreditPurchasePostCreate so request cancellation won't abort
settlement. In the lo.ForEach over result.pendingInvoiceCreditPurchases, replace
passing the incoming ctx to handleInvoiceCreditPurchasePostCreate with a created
background-derived context (e.g., context.WithTimeout(context.Background(),
<shortDuration>)) and ensure you call cancel() after the call; alternatively
enqueue the pending item to a background worker/queue owned by the service so
handleInvoiceCreditPurchasePostCreate runs independently of the request
lifetime.
- Around line 205-223: The current idempotency check only looks at
charge.State.InvoiceSettlement and does not prevent duplicate billing writes if
CreatePendingInvoiceLines succeeds but state isn't persisted; modify the flow in
the processing function so that after receiving result from
s.billingService.CreatePendingInvoiceLines (the created gathering line), you
persist a durable marker (e.g., set charge.State.InvoiceSettlement with the
created line ID / a "pendingSettlement" token) before any other return points
and before calling LinkInvoicedPayment, or alternatively query/dedupe existing
gathering lines by ChargeID via the billing service before creating; update the
code paths around s.billingService.CreatePendingInvoiceLines,
pending.gatheringLine, and LinkInvoicedPayment to ensure the created line ID is
stored atomically on the charge to prevent duplicate pending-line creation on
retries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d09af33-ca60-495c-972a-11b00ded1251
📒 Files selected for processing (7)
openmeter/billing/charges/creditpurchase/chargecreditpurchase.goopenmeter/billing/charges/creditpurchase/service.goopenmeter/billing/charges/creditpurchase/service/create.goopenmeter/billing/charges/service/create.goopenmeter/billing/charges/service/invoicependinglines.goopenmeter/billing/service/gatheringinvoicependinglines.goopenmeter/billing/stdinvoice.go
✅ Files skipped from review due to trivial changes (1)
- openmeter/billing/service/gatheringinvoicependinglines.go
🚧 Files skipped from review as they are similar to previous changes (5)
- openmeter/billing/charges/creditpurchase/chargecreditpurchase.go
- openmeter/billing/stdinvoice.go
- openmeter/billing/charges/creditpurchase/service/create.go
- openmeter/billing/charges/creditpurchase/service.go
- openmeter/billing/charges/service/invoicependinglines.go
mark-vass-konghq
force-pushed
the
feat/invoice-settlement-mode-for-credits
branch
from
e4d672f to
3d77246
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
openmeter/billing/charges/service/create.go (1)
166-172:⚠️ Potential issue | 🟠 Major
Create()can still fail after the charge commit.By the time this loop runs,
transaction.Run()has already succeeded. IfhandleInvoiceCreditPurchasePostCreate()errors after creating pending lines or even invoicing them, the API returns a hard failure for an already-active charge, and a retry is not replay-safe because the helper only short-circuits onceInvoiceSettlementexists. This post-step needs a durable/idempotent retry path, or it should be treated as best-effort instead of failingCreate()here.🤖 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 166 - 172, Create() currently fails after transaction.Run() if handleInvoiceCreditPurchasePostCreate() errors, which can leave an active charge and make retries unsafe; change this to a best-effort post-commit handling: after transaction.Run() succeeds, call handleInvoiceCreditPurchasePostCreate() but do not return its error to the caller—log the error and enqueue a durable background retry (or mark a retryable work item) so the operation can be replayed safely; also make handleInvoiceCreditPurchasePostCreate() and its helpers idempotent by short-circuiting when InvoiceSettlement already exists (and by checking/creating pending lines atomically) so background retries are safe.openmeter/billing/charges/creditpurchase/service/invoice.go (1)
48-60:⚠️ Potential issue | 🟠 MajorLinking shouldn’t mark the invoice payment as authorized yet.
LinkInvoicedPayment()runs right after the standard line is created, while the invoice can still be draft/manual-approval, but this record is seeded withAuthorizedandStatusAuthorized. That makes the charge look invoice-authorized before the authorize hook on Line 80 actually runs. Please start this settlement in a pre-authorization state and letHandleInvoicePaymentAuthorized()own the real transition.🤖 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 48 - 60, LinkInvoicedPayment is seeding a payment.InvoicedCreate with Authorized and payment.StatusAuthorized too early; remove the Authorized assignment (set Base.Authorized to nil) and set Base.Status to the pre-authorization state constant (e.g. payment.StatusPreAuthorized or payment.StatusPending if that’s the repo's pre-auth constant) so the record is created as pre-authorized; let HandleInvoicePaymentAuthorized() perform the actual authorization and set Authorized + payment.StatusAuthorized later.
🤖 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/service/create.go`:
- Around line 144-147: The code calls s.createGatheringLines(ctx,
gatheringLinesToCreate) a second time, but gatheringLinesToCreate was already
flushed earlier (and invoice-settlement credit purchases are not appended to
it), causing duplicate creation of the same pending lines; remove the redundant
second call and instead ensure any invoice-settlement credit purchase lines are
appended to the slice before the first flush or handled by a separate slice and
call; update the logic around the gatheringLinesToCreate usage (and the
createGatheringLines invocation) so each intended line is created exactly once
(refer to gatheringLinesToCreate and s.createGatheringLines).
---
Duplicate comments:
In `@openmeter/billing/charges/creditpurchase/service/invoice.go`:
- Around line 48-60: LinkInvoicedPayment is seeding a payment.InvoicedCreate
with Authorized and payment.StatusAuthorized too early; remove the Authorized
assignment (set Base.Authorized to nil) and set Base.Status to the
pre-authorization state constant (e.g. payment.StatusPreAuthorized or
payment.StatusPending if that’s the repo's pre-auth constant) so the record is
created as pre-authorized; let HandleInvoicePaymentAuthorized() perform the
actual authorization and set Authorized + payment.StatusAuthorized later.
In `@openmeter/billing/charges/service/create.go`:
- Around line 166-172: Create() currently fails after transaction.Run() if
handleInvoiceCreditPurchasePostCreate() errors, which can leave an active charge
and make retries unsafe; change this to a best-effort post-commit handling:
after transaction.Run() succeeds, call handleInvoiceCreditPurchasePostCreate()
but do not return its error to the caller—log the error and enqueue a durable
background retry (or mark a retryable work item) so the operation can be
replayed safely; also make handleInvoiceCreditPurchasePostCreate() and its
helpers idempotent by short-circuiting when InvoiceSettlement already exists
(and by checking/creating pending lines atomically) so background retries are
safe.
🪄 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: 365c1e5a-7848-4e37-ad46-3b03028fab0b
📒 Files selected for processing (14)
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.goopenmeter/billing/service/gatheringinvoicependinglines.goopenmeter/billing/stdinvoice.go
✅ Files skipped from review due to trivial changes (1)
- openmeter/billing/charges/creditpurchase/adapter/payment.go
🚧 Files skipped from review as they are similar to previous changes (7)
- openmeter/billing/charges/creditpurchase/chargecreditpurchase.go
- openmeter/billing/charges/creditpurchase/adapter/charge.go
- openmeter/billing/service/gatheringinvoicependinglines.go
- openmeter/billing/charges/service/invoicependinglines.go
- openmeter/billing/charges/creditpurchase/adapter/mapper.go
- openmeter/billing/charges/creditpurchase/adapter.go
- openmeter/billing/charges/service/creditpurchase_test.go
mark-vass-konghq
force-pushed
the
feat/invoice-settlement-mode-for-credits
branch
from
3d77246 to
25e77ef
Compare
turip
reviewed
Mar 23, 2026
Member
|
Please consider this coderabbit test coverage issue: openmeter/billing/charges/service/creditpurchase_test.go (1) Please add coverage for the deferred invoice path too. With ServicePeriod.From fixed to January 1, 2026, this test only exercises the immediate InvoiceAt <= now branch. The new deferred path—pending line created first, InvoiceSettlement linked 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 |
mark-vass-konghq
force-pushed
the
feat/invoice-settlement-mode-for-credits
branch
from
25e77ef to
4c13c48
Compare
mark-vass-konghq
force-pushed
the
feat/invoice-settlement-mode-for-credits
branch
from
4c13c48 to
c2b7b7d
Compare
turip
reviewed
Mar 23, 2026
| // The charge should be active and ready for when the invoice date arrives | ||
| assert.Equal(s.T(), meta.ChargeStatusActive, creditPurchaseCharge.Status) | ||
| }) | ||
|
|
Member
There was a problem hiding this comment.
This is not entirely true. Please check the clock pakage, we use that for backdating.
But the justficiation is fine for not continuing the testcase.
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