fix(billing): [OM-434] delete empty custom-currency overage lines - #4843
Conversation
📝 WalkthroughWalkthroughThe change adds zero-fiat custom-currency overage completion states, finalization, settlement handling, stage-aware line population, deleted-line behavior, cleanup exceptions, broader detailed-line retrieval, and regression coverage. ChangesZero-fiat overage lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
turip
added
release-note/feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/usagebased_test.go (1)
185-355: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a usage-based rounding-to-zero lifecycle case.
The matrix covers zero overage from credits or absent usage. It does not cover a positive TOKENS overage that rounds to zero in USD.
Add a fractional-usage case with a positive run total and zero fiat invoice total. This helper runs both final and progressive variants, so it will protect both paths.
As per path instructions, “Make sure the tests are comprehensive and cover the changes.” The PR objective includes positive custom-currency overage that rounds to zero in fiat.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/service/usagebased_test.go` around lines 185 - 355, Add a usage-based lifecycle case to the tests table with fractional usage producing a positive TOKENS run total while the converted USD invoice total rounds to zero. Set expectations for both run creation and collection phases, including no payment settlement and preserving the positive overage line, so the shared helper exercises final and progressive paths.Source: Path instructions
🧹 Nitpick comments (3)
openmeter/billing/charges/flatfee/service/linemapper.go (1)
215-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why only these two stages mark the overage line deleted.
The condition restricts zero-fiat-overage deletion to
standardLinePopulationStageGatheringPreviewandstandardLinePopulationStageCollectionCompleted, excludinginvoiceCreated,manualAttachment, andintentReconciliation. This is a meaningful lifecycle rule, but nothing here explains why these two stages are the right places to delete the line and why the others are excluded.Add a short comment above the condition stating the domain reason, for example: the actual overage amount is not known until collection completes credit allocation, so deletion cannot be decided earlier than
collectionCompleted(andgatheringPreviewis a display-only stage with no accrual).As per coding guidelines: "Comments and docstrings should explain non-obvious intent, domain constraints, lifecycle state, and failure consequences; do not narrate visible conditions" and "Document domain helpers whose names compress important business semantics, including observable behavior and why cases are excluded."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/flatfee/service/linemapper.go` around lines 215 - 220, Add a concise domain comment immediately above the stage condition in the line-mapping logic, explaining that overage deletion requires the actual amount after collection completes credit allocation, while gatheringPreview is display-only and has no accrual; clarify why the other lifecycle stages are excluded without merely restating the condition.Source: Coding guidelines
openmeter/billing/charges/usagebased/service/linemapper.go (2)
158-166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider validating
ChargeandRunhere too.
populateStandardLineFromRunInput.Validate()only checksStage. The flat-fee sibling,populateFlatFeeStandardLineFromRunInput.Validate(), also validatesCharge, validatesRun, and confirms the run's service period matches the charge's effective service period.Since this is the first validation added to this struct, extending it now to cover
Charge/Runcatches malformed input earlier and keeps the two packages' population contracts consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/usagebased/service/linemapper.go` around lines 158 - 166, Extend populateStandardLineFromRunInput.Validate to validate both Charge and Run in addition to Stage, appending each validation error to errs before constructing the combined validation error. Match populateFlatFeeStandardLineFromRunInput.Validate by also checking that the run service period matches the charge effective service period, preserving the existing error aggregation behavior.
325-330: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why only these two stages mark the overage line deleted.
Same gap as
openmeter/billing/charges/flatfee/service/linemapper.go: the condition scopes deletion tostandardLinePopulationStageGatheringPreviewandstandardLinePopulationStageCollectionCompletedwithout explaining why, or whyinvoiceCreated/manualAttachmentare excluded.As per coding guidelines: "Comments and docstrings should explain non-obvious intent, domain constraints, lifecycle state, and failure consequences; do not narrate visible conditions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/usagebased/service/linemapper.go` around lines 325 - 330, In the overage-line deletion block, add a concise comment explaining the lifecycle/domain reason deletion applies only during standardLinePopulationStageGatheringPreview and standardLinePopulationStageCollectionCompleted, including why invoiceCreated and manualAttachment are excluded. Anchor the comment to the condition using isZeroFiatAmountOverageRun and do not merely restate the stage names.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@openmeter/billing/charges/service/usagebased_test.go`:
- Around line 185-355: Add a usage-based lifecycle case to the tests table with
fractional usage producing a positive TOKENS run total while the converted USD
invoice total rounds to zero. Set expectations for both run creation and
collection phases, including no payment settlement and preserving the positive
overage line, so the shared helper exercises final and progressive paths.
---
Nitpick comments:
In `@openmeter/billing/charges/flatfee/service/linemapper.go`:
- Around line 215-220: Add a concise domain comment immediately above the stage
condition in the line-mapping logic, explaining that overage deletion requires
the actual amount after collection completes credit allocation, while
gatheringPreview is display-only and has no accrual; clarify why the other
lifecycle stages are excluded without merely restating the condition.
In `@openmeter/billing/charges/usagebased/service/linemapper.go`:
- Around line 158-166: Extend populateStandardLineFromRunInput.Validate to
validate both Charge and Run in addition to Stage, appending each validation
error to errs before constructing the combined validation error. Match
populateFlatFeeStandardLineFromRunInput.Validate by also checking that the run
service period matches the charge effective service period, preserving the
existing error aggregation behavior.
- Around line 325-330: In the overage-line deletion block, add a concise comment
explaining the lifecycle/domain reason deletion applies only during
standardLinePopulationStageGatheringPreview and
standardLinePopulationStageCollectionCompleted, including why invoiceCreated and
manualAttachment are excluded. Anchor the comment to the condition using
isZeroFiatAmountOverageRun and do not merely restate the stage names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e8f1641-a8a0-4938-84e9-dfa3828ae071
📒 Files selected for processing (15)
openmeter/billing/charges/flatfee/service/creditheninvoice.goopenmeter/billing/charges/flatfee/service/lineengine.goopenmeter/billing/charges/flatfee/service/linemapper.goopenmeter/billing/charges/flatfee/statemachine.goopenmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/gathering_preview_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/usagebased_test.goopenmeter/billing/charges/usagebased/service/creditheninvoice.goopenmeter/billing/charges/usagebased/service/creditheninvoice_test.goopenmeter/billing/charges/usagebased/service/lineengine.goopenmeter/billing/charges/usagebased/service/linemapper.goopenmeter/billing/charges/usagebased/service/linemapper_test.goopenmeter/billing/charges/usagebased/service/payments.goopenmeter/billing/charges/usagebased/statemachine.go
7085b8e to
9e1c81e
Compare
|
Bot feedback disposition:
CodeRabbit's refreshed review passes. GitHub exposes no unresolved inline review threads for these findings because they were posted in the review summary. |
4824648 to
2359e94
Compare
tothandras
left a comment
tothandras
left a comment
There was a problem hiding this comment.
Pre-landing review
Scope looks clean — the diff does what the description says, for both engines. go build, go vet, and all runnable unit tests pass. Two notes before the findings:
- No migration needed, confirmed:
status_detailedischaracter varying, not a Postgres enum type (20260325084446_charges-refactor-tables.up.sql:23), so the new enum value only regenerates validators. The usual new-enum-value trap doesn't apply. - The Postgres lifecycle suites could not run in my environment either, so everything below is source-level analysis.
9 findings — 4 I'd want resolved before landing.
1. Soft-deleted zero-fiat line is still handed to OnInvoiceIssued
onCollectionCompleted mutates the in-memory line set via m.Invoice.Lines.ReplaceLinesByID(lines...) (openmeter/billing/service/stdinvoicestate.go:1001). onInvoiceIssued then reads m.Invoice.Lines.OrEmpty() (stdinvoicestate.go:1014), and groupStandardLinesByEngine (openmeter/billing/service/lineengine.go:167-170) applies no DeletedAt filter. Neither engine skips deleted lines — flat-fee OnInvoiceIssued (openmeter/billing/charges/flatfee/service/lineengine.go:931-942) calls FireAndActivate(TriggerInvoiceIssued) with no CanFire guard, and the charge is by then in StatusFinal, which permits no such trigger (flatfee/service/creditheninvoice.go:169-174).
This is new behavior: before this PR every expectLineDeleted: true lifecycle case was s.T().Skip(...)ed, so nothing produced a soft-deleted line during collection. The tests pass because they stop at DraftManualApprovalNeeded and re-issue in a separate call that refetches without deleted lines. An invoice that walks collecting → issuing in a single pass (auto-approval, zero draft period) looks like it would fail issuance.
Worth a test that advances collecting → issuing without an intermediate reload.
2. The flat-fee zero-fiat skip in cleanupDeletedStandardLines is unreachable
openmeter/billing/charges/flatfee/service/lineengine.go:775 sits after the guard at line 768:
if charge.Realizations.CurrentRun != nil && charge.Realizations.CurrentRun.ID.ID == run.ID.ID {
return fmt.Errorf("flat fee standard line[%s] cannot be deleted because realization run[%s] is still current for charge[%s]", ...)
}FinalizeZeroFiatAmountOverageRun only sets Immutable: mo.Some(true) and never detaches the run. The PR's own test asserts exactly that:
s.Equal(flatfee.StatusFinal, charge.Status)
s.Require().NotNil(charge.Realizations.CurrentRun)
s.Empty(charge.Realizations.PriorRuns)So the run is always still current when cleanup runs, the guard hard-errors first, and the skip below never executes. The usage-based side clears CurrentRealizationRunID (usagebased/service/creditheninvoice.go:604) — the asymmetry also leaves a flat-fee charge in StatusFinal with a current, immutable run, and StatusFinal still permits Extend/Shrink.
Either detach the run the way usage-based does, or move the zero-fiat check above the current-run guard — but as written the branch is dead.
3. The skip in OnMutableStandardLinesDeletedBySystem is unconditional and can leak credit allocations
openmeter/billing/charges/usagebased/service/lineengine.go:770 bypasses deleteMutableStandardLineRealization, which is the only caller of CorrectAllCredits and markMutableStandardLineRunDeleted (lineengine.go:826-838).
A usage-based custom-currency run is flagged NoFiatTransactionRequired at run creation (usagebased/service/run/create.go:219-229), well before collection. So a system delete of that still-mutable line for an unrelated reason — subscription change, invoice delete — now skips credit correction entirely. In the "fully covered by credits" case that leaves 10 TOKENS booked against the customer with the line gone, and CurrentRealizationRunID still pointing at a run whose line no longer exists.
The comment says the skip is for the collection-time deletion, but the condition doesn't distinguish that from any other system delete. Same shape at flatfee/service/lineengine.go:775.
4. No backfill story for charges already in active.realization.processing
StatusActiveRealizationProcessing previously only exited via TriggerInvoiceIssued. It now also permits TriggerNext → ZeroFiatAmountOverageCompleted gated purely on the persisted NoFiatTransactionRequired flag (flatfee/service/creditheninvoice.go:127-131, usagebased/service/creditheninvoice.go:132-136).
Charges sitting in processing at deploy time already have that flag set, but their invoice line was populated pre-deploy and is not marked deleted. On the next advance they take the new transition with a live line still on the invoice — and then run into finding #1 when that invoice is issued.
Smaller items
5. Dead branch in areAllRealizationRunsSettled — openmeter/billing/charges/usagebased/service/payments.go:128. isZeroFiatAmountOverageRun requires run.NoFiatTransactionRequired, and line 136 already does if run.NoFiatTransactionRequired { continue }. The new continue at 128-130 changes nothing; only the (run.InvoiceUsage != nil || isZeroFiatAmountOverage) clause at line 124 is load-bearing. Suggest dropping the branch and moving the comment onto line 124.
6. openmeter/billing/charges/flatfee/service/ has zero test files. Every new flat-fee path — FinalizeZeroFiatAmountOverageRun, IsCurrentRunZeroFiatAmountOverage, isZeroFiatAmountOverageRun, standardLinePopulationStage.Validate, the mapper deletion, the cleanup skip — is verifiable only through the Postgres suites. The usage-based twin has unit tests for all of these. And flat-fee isn't a mirror: it performs a DB write and goes straight to StatusFinal, bypassing AwaitingPaymentSettlement. Given #2 above, unit coverage here would have caught the dead branch.
7. The gathering_preview half of the flat-fee delete condition is unreachable — flatfee/service/linemapper.go:215. BuildStandardLinesForGatheringPreview short-circuits every custom-currency line at lineengine.go:87-95 (DeletedAt, Validate, continue) before any populate call, and populateCustomCurrencyOverageFromRun only runs when IsCustom(). So that stage can't reach line 215 — the condition can be reduced to collection_completed.
8. Preview semantics now diverge between the engines. Flat-fee omits all custom-currency preview lines; usage-based keeps its line and deletes only when the pre-credit total is zero (usagebased/service/run/preview.go:140 sets the flag from runTotals.Total.IsZero(), and preview deliberately doesn't allocate credits). Net effect: usage-based customers see a preview overage that will usually vanish at collection, flat-fee customers see nothing. Worth a deliberate call on which is right.
9. Unbounded per-hook detailed-line load — flatfee/adapter/detailedline.go:22. FetchDetailedLines now queries RunIDIn(currentRun + all priorRuns), and newStateMachineForStandardLine always requests ExpandDetailedLines (flatfee/service/lineengine.go:866). That state machine is constructed per standard line on every invoice hook, so a charge that accumulated prior runs through prorations pays an ever-growing load — even though only the GetByLineID run is used.
One hardening suggestion: at usagebased/service/linemapper.go:325 the deletion keys off the persisted run.NoFiatTransactionRequired, while the function has just computed fiatOverage.Amount fresh three lines above. Asserting fiatOverage.Amount.IsZero() alongside the flag — and erroring if they disagree — would turn any future drift between the two into a loud failure rather than a silently dropped invoice line.
The empty-invoice leftover is already documented as a known limitation, so not flagging it.
3 participants
Summary
Custom-currency credit-then-invoice charges no longer keep an overage line when the resolved fiat amount is zero.
This applies when:
Why
The fiat invoice represents only the overage remaining after the charge has been realized in its custom currency. Given that this is just an overage being billed, we do not want to add it to a fiat invoice unless there is an actual fiat amount due.
The deletion decision follows
Totals.Amountrather thanTotals.Totalsemantics. If fiat credit allocation for overage lines is added later, a line with a positive fiat amount but a zero final total must remain on the invoice so the customer can see the fiat credit usage.Behavioral changes
Known limitation
An invoice whose only line is the removed overage line remains present as an empty invoice. Deleting empty invoices will be handled separately.
Validation
go test ./openmeter/billing/charges/...make lint-go-fastThe database-backed lifecycle suite compiles but was skipped locally because Docker/Postgres was unavailable.
Summary by CodeRabbit
Greptile Summary
The PR removes custom-currency overage lines whose resolved fiat amount is zero while preserving realization and credit history.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Custom-currency charge realization] --> B[Allocate credits and resolve fiat amount] B --> C{Fiat Amount is zero?} C -->|No| D[Issue invoice and settle payment] C -->|Yes| E[Mark overage line deleted] E --> F[Finalize realization without fiat transaction] F --> G[Preserve run, credit allocations, and detailed history] D --> H[Complete charge lifecycle] G --> HReviews (4): Last reviewed commit: "test(billing): cover usage overage fiat ..." | Re-trigger Greptile
Context used: