feat(billing): add collected invoice line limit#4632
Conversation
|
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 a configurable cap for collected invoice lines, validates and documents the new setting, propagates it through billing wiring, and applies it to gathering and subscription-sync invoice batching. ChangesInvoice line cap flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
turip
added
area/billing
kind/feature
Greptile SummaryThis PR adds a configurable limit for collected invoice lines. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (10): Last reviewed commit: "test(billing): remove repeated sync coll..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/service/gatheringinvoicependinglines.go (1)
208-213: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTruncation can silently drop explicitly-requested lines.
gatherInScopeLinesalready validates (and filters down to) the caller's explicitly requestedLinesToIncludebefore this point — those lines were confirmed billable. ButlimitGatheringLinesForInvoiceis then applied unconditionally, so if the number of explicitly requested lines exceedsMaxLinesPerInvoice, some pre-validated, user-requested lines get silently dropped with no error surfaced. That breaks the guarantee that requesting specific billable lines results in them being billed.Consider skipping the truncation when
input.IncludePendingLinesis present, or returning an explicit error if the requested set exceeds the limit.🩹 Possible fix
- inScopeLines = limitGatheringLinesForInvoice(inScopeLines, options.MaxLinesPerInvoice) + if !input.IncludePendingLines.IsPresent() { + inScopeLines = limitGatheringLinesForInvoice(inScopeLines, options.MaxLinesPerInvoice) + }🤖 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/service/gatheringinvoicependinglines.go` around lines 208 - 213, The truncation in gatherInScopeLines is dropping explicitly requested billable lines because limitGatheringLinesForInvoice is applied unconditionally after LinesToInclude has already been validated. Update the logic in gatheringinvoicependinglines.go around gatherInScopeLines so that explicit requests via input.IncludePendingLines/LinesToInclude are not silently truncated; either skip limitGatheringLinesForInvoice for requested lines or return an error when the requested set exceeds options.MaxLinesPerInvoice. Keep the fix localized to gatherInScopeLines and limitGatheringLinesForInvoice handling.
🧹 Nitpick comments (3)
openmeter/billing/invoice.go (1)
482-484: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a short doc comment for
MaxLinesPerInvoice.
PartialInvoiceLinesEnabledright below has a clear doc block explaining its semantics, butMaxLinesPerInvoice(0 = no limit, positive = cap) has none. A one-liner would keep the contract obvious to callers usingWithMaxLinesPerInvoice.As per coding guidelines: "Add docstrings to domain helpers when the name compresses important business semantics, and describe the observable business contract rather than implementation mechanics."
✏️ Suggested doc comment
type InvoicePendingLinesOptions struct { BypassCollectionAlignment bool - MaxLinesPerInvoice int + // MaxLinesPerInvoice caps the number of pending lines collected into a single invoice. + // 0 means no limit. + MaxLinesPerInvoice int🤖 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/invoice.go` around lines 482 - 484, Add a short doc comment for MaxLinesPerInvoice in InvoicePendingLinesOptions to make its business contract explicit for callers of WithMaxLinesPerInvoice; document that 0 means no limit and positive values cap the number of lines per invoice, matching the style used for PartialInvoiceLinesEnabled.Source: Coding guidelines
openmeter/billing/worker/subscriptionsync/service/sync_unit_test.go (1)
14-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the empty-success loop exit.
Both tests exercise looping and the cap<=0 single-call path, but the
len(invoices) == 0termination branch insync.go(a successful call returning zero invoices) isn't exercised anywhere. Worth adding a mock result like{invoices: []billing.StandardInvoice{}}to confirm the loop exits cleanly on that path too, since it's new behavior introduced by this PR.🤖 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/worker/subscriptionsync/service/sync_unit_test.go` around lines 14 - 44, Add a unit test in invoicePendingLines coverage that exercises the successful zero-invoices termination path in Service.invoicePendingLines. Use invoicePendingLinesBillingService with a mocked result returning an empty invoices slice, then assert the loop exits cleanly with no error and only the expected single call(s) to billingService. Reference the existing TestInvoicePendingLinesCollectsCappedBatchesUntilEmpty / invoicePendingLines setup to keep the new case aligned with the current batching behavior.Source: Path instructions
openmeter/billing/worker/subscriptionsync/service/sync.go (1)
36-56: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a safety cap on the collection loop.
The loop has no upper bound — it depends entirely on the invariant that each successful
InvoicePendingLinescall consumes lines from the gathering pool so it eventually hitsErrInvoiceCreateNoLinesor an empty result. That should hold today, but there's no backstop if that invariant is ever broken (bug, data inconsistency, future refactor) — the worker would spin forever on a single customer with no visibility.A small pass-count guard would make this robust without changing normal-case behavior.
🛡️ Proposed safety-net for the collection loop
return span.Wrap(func(ctx context.Context) error { - for { + const maxCollectionPasses = 10_000 // generous upper bound; each pass consumes at least one line + for pass := 0; pass < maxCollectionPasses; pass++ { invoices, err := s.billingService.InvoicePendingLines( ctx, billing.InvoicePendingLinesInput{ Customer: customer, }, billing.WithPartialInvoiceLinesDisabled(), billing.WithMaxLinesPerInvoice(s.featureFlags.MaxLinesPerCollectedInvoice), ) if err != nil { if errors.Is(err, billing.ErrInvoiceCreateNoLines) { return nil } return err } if s.featureFlags.MaxLinesPerCollectedInvoice <= 0 || len(invoices) == 0 { return nil } } + return fmt.Errorf("exceeded max collection passes for customer %s", customer.ID) })🤖 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/worker/subscriptionsync/service/sync.go` around lines 36 - 56, The collection loop in Sync should have a hard safety cap so it cannot spin forever if InvoicePendingLines stops making progress. Update the looping logic in Sync to track how many successful collection passes have run for a customer and exit once a small maximum is reached, while preserving the existing exit conditions for billing.ErrInvoiceCreateNoLines and empty invoices. Use the existing Sync method and InvoicePendingLines call as the main points to locate and adjust the loop.
🤖 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.
Inline comments:
In `@app/config/billing.go`:
- Around line 42-49: BillingFeatureSwitchesConfiguration.Validate currently
returns the raw errors.Join result, but it should follow the config package
validation contract. Update Validate in BillingFeatureSwitchesConfiguration to
wrap the joined errors with
models.NewNillableGenericValidationError(errors.Join(errs...)), keeping the
existing err collection logic intact.
In `@openmeter/billing/service/gatheringinvoicependinglines.go`:
- Around line 141-144: Wrap the new MaxLinesPerInvoice validation failure in
billing.ValidationError, matching the existing validation paths in
GatherInvoicePendingLines. In the options.MaxLinesPerInvoice check, return
billing.ValidationError{Err: ...} instead of a bare errors.New so downstream
handling treats it as a client input error, consistent with input.Validate() and
the namespace lockdown validation.
---
Outside diff comments:
In `@openmeter/billing/service/gatheringinvoicependinglines.go`:
- Around line 208-213: The truncation in gatherInScopeLines is dropping
explicitly requested billable lines because limitGatheringLinesForInvoice is
applied unconditionally after LinesToInclude has already been validated. Update
the logic in gatheringinvoicependinglines.go around gatherInScopeLines so that
explicit requests via input.IncludePendingLines/LinesToInclude are not silently
truncated; either skip limitGatheringLinesForInvoice for requested lines or
return an error when the requested set exceeds options.MaxLinesPerInvoice. Keep
the fix localized to gatherInScopeLines and limitGatheringLinesForInvoice
handling.
---
Nitpick comments:
In `@openmeter/billing/invoice.go`:
- Around line 482-484: Add a short doc comment for MaxLinesPerInvoice in
InvoicePendingLinesOptions to make its business contract explicit for callers of
WithMaxLinesPerInvoice; document that 0 means no limit and positive values cap
the number of lines per invoice, matching the style used for
PartialInvoiceLinesEnabled.
In `@openmeter/billing/worker/subscriptionsync/service/sync_unit_test.go`:
- Around line 14-44: Add a unit test in invoicePendingLines coverage that
exercises the successful zero-invoices termination path in
Service.invoicePendingLines. Use invoicePendingLinesBillingService with a mocked
result returning an empty invoices slice, then assert the loop exits cleanly
with no error and only the expected single call(s) to billingService. Reference
the existing TestInvoicePendingLinesCollectsCappedBatchesUntilEmpty /
invoicePendingLines setup to keep the new case aligned with the current batching
behavior.
In `@openmeter/billing/worker/subscriptionsync/service/sync.go`:
- Around line 36-56: The collection loop in Sync should have a hard safety cap
so it cannot spin forever if InvoicePendingLines stops making progress. Update
the looping logic in Sync to track how many successful collection passes have
run for a customer and exit once a small maximum is reached, while preserving
the existing exit conditions for billing.ErrInvoiceCreateNoLines and empty
invoices. Use the existing Sync method and InvoicePendingLines call as the main
points to locate and adjust the loop.
🪄 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: 561e8b1d-baa6-4167-96b0-fd88e52f45a2
📒 Files selected for processing (13)
app/common/billing.goapp/config/billing.goapp/config/billing_test.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.goconfig.example.yamlopenmeter/billing/invoice.goopenmeter/billing/service/gatheringinvoicependinglines.goopenmeter/billing/service/gatheringinvoicependinglines_test.goopenmeter/billing/worker/collect/collect.goopenmeter/billing/worker/subscriptionsync/service/service.goopenmeter/billing/worker/subscriptionsync/service/sync.goopenmeter/billing/worker/subscriptionsync/service/sync_unit_test.go
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/worker/subscriptionsync/service/sync.go (1)
30-63: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftHeads up: a low cap + big customer could actually hit the 10,000-pass ceiling.
Nice safety net with
maxInvoicePendingCollectionPasses, but worth thinking through the numbers: withMaxLinesPerCollectedInvoiceset low (say1) and a customer with a large volume of pending lines, this loop could genuinely need more than 10,000 round-trips — each one doing a fullInvoicePendingLinescall (invoice creation, calculation, DB writes). That's a real production failure mode (hard error surfaced to the caller), not just a theoretical one.A couple of light-touch improvements that could help without a big design change:
- Log a warning (with pass count + customer ID) once we're getting close to the ceiling, so ops has visibility before it turns into a hard failure.
- Consider whether the constant should scale with (or at least be validated against) the configured
MaxLinesPerCollectedInvoicevalue, since a smaller cap directly multiplies the number of passes needed.💡 Sketch of a warning before hitting the ceiling
for pass := 0; pass < maxInvoicePendingCollectionPasses; pass++ { + if pass == maxInvoicePendingCollectionPasses-1 { + s.logger.Warn("approaching max invoice pending collection passes", "customer_id", customer.ID, "pass", pass) + } invoices, err := s.billingService.InvoicePendingLines(🤖 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/worker/subscriptionsync/service/sync.go` around lines 30 - 63, The loop in invoicePendingLines can legitimately approach the maxInvoicePendingCollectionPasses ceiling when MaxLinesPerCollectedInvoice is very low, so add a warning before the hard stop that includes the pass count and customer ID for visibility. Use the existing invoicePendingLines method and its tracing/logging context to emit the warning as the pass count nears the ceiling, and consider validating or deriving maxInvoicePendingCollectionPasses from featureFlags.MaxLinesPerCollectedInvoice so the cap reflects the configured batch size.
🤖 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/worker/subscriptionsync/service/sync.go`:
- Around line 30-63: The loop in invoicePendingLines can legitimately approach
the maxInvoicePendingCollectionPasses ceiling when MaxLinesPerCollectedInvoice
is very low, so add a warning before the hard stop that includes the pass count
and customer ID for visibility. Use the existing invoicePendingLines method and
its tracing/logging context to emit the warning as the pass count nears the
ceiling, and consider validating or deriving maxInvoicePendingCollectionPasses
from featureFlags.MaxLinesPerCollectedInvoice so the cap reflects the configured
batch size.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 637a352c-8ffd-418c-8b39-22c4de4cb65b
📒 Files selected for processing (6)
app/config/billing.goopenmeter/billing/invoice.goopenmeter/billing/service/gatheringinvoicependinglines.goopenmeter/billing/service/gatheringinvoicependinglines_test.goopenmeter/billing/worker/subscriptionsync/service/sync.goopenmeter/billing/worker/subscriptionsync/service/sync_unit_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- openmeter/billing/invoice.go
- app/config/billing.go
- openmeter/billing/service/gatheringinvoicependinglines.go
- openmeter/billing/service/gatheringinvoicependinglines_test.go
turip
force-pushed
the
feat/collected-invoice-line-limit
branch
from
dd357c8 to
c05acc7
Compare
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/worker/subscriptionsync/service/sync.go (1)
36-60: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPlease bound the capped collection loop.
With
MaxLinesPerCollectedInvoice > 0, this keeps collecting while every pass returns at least one invoice. A large backlog—or pending lines being added concurrently—can keep one subscription sync busy for an unbounded number of billing/database passes. Consider reintroducing a per-sync pass limit and leaving the rest for a later collection pass.Possible shape
- for pass := 0; ; pass++ { + for pass := 0; pass < maxInvoicePendingCollectionPasses; pass++ { if pass > 0 && pass%10_000 == 0 && s.logger != nil { s.logger.WarnContext(ctx, "invoice pending collection is still processing capped batches", "customer_id", customer.ID, "pass", pass) } @@ if s.featureFlags.MaxLinesPerCollectedInvoice <= 0 || len(invoices) == 0 { return nil } } + + if s.logger != nil { + s.logger.WarnContext(ctx, "invoice pending collection reached capped pass limit", "customer_id", customer.ID, "passes", maxInvoicePendingCollectionPasses) + } + return nilAs per path instructions, performance should be prioritized for message processing and database operations.
🤖 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/worker/subscriptionsync/service/sync.go` around lines 36 - 60, The capped collection loop in the subscription sync flow is unbounded when `MaxLinesPerCollectedInvoice` stays positive and invoices keep being produced. Update the loop in `Sync` to enforce a per-sync pass limit before repeatedly calling `billingService.InvoicePendingLines`, and exit early so remaining work is handled by a later run. Use the existing `pass` counter and the `Sync`/`InvoicePendingLines` flow to locate the loop, and keep the existing nil/error handling intact while adding the bound.Source: Path instructions
🤖 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/worker/subscriptionsync/service/sync.go`:
- Around line 36-60: The capped collection loop in the subscription sync flow is
unbounded when `MaxLinesPerCollectedInvoice` stays positive and invoices keep
being produced. Update the loop in `Sync` to enforce a per-sync pass limit
before repeatedly calling `billingService.InvoicePendingLines`, and exit early
so remaining work is handled by a later run. Use the existing `pass` counter and
the `Sync`/`InvoicePendingLines` flow to locate the loop, and keep the existing
nil/error handling intact while adding the bound.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0e92a39a-450c-4c1e-b335-15c34f5d2160
📒 Files selected for processing (1)
openmeter/billing/worker/subscriptionsync/service/sync.go
turip
force-pushed
the
feat/collected-invoice-line-limit
branch
from
c05acc7 to
0c21b7e
Compare
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/worker/subscriptionsync/service/sync.go (1)
33-73: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEarly return inside the chunk loop can silently skip remaining, still-valid chunks.
lineIDChunksis now a fixed partition of the specific target line IDs computed upstream (viainvoiceLineIDsForPendingCollection), not an open-ended poll. When any chunk's call returnsErrInvoiceCreateNoLinesor an emptyinvoicesslice, the function returnsnilimmediately — abandoning any subsequent chunks even though they contain distinct, still-eligible line IDs. This looks like a leftover from the old "loop until exhausted" pattern, which doesn't fit the new bounded-chunk design. Impact is softened since untouched lines stay pending for the next sync pass, but within this pass some eligible lines get dropped without cause.🐛 Proposed fix: continue to the next chunk instead of aborting
for _, lineIDChunk := range lineIDChunks { invoices, err := s.billingService.InvoicePendingLines( ctx, billing.InvoicePendingLinesInput{ Customer: customer, IncludePendingLines: mo.Some(lineIDChunk), }, billing.WithPartialInvoiceLinesDisabled(), billing.WithMaxLinesPerInvoice(s.featureFlags.MaxLinesPerCollectedInvoice), ) if err != nil { if errors.Is(err, billing.ErrInvoiceCreateNoLines) { - return nil + continue } return err } if len(invoices) == 0 { - return nil + continue } }🤖 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/worker/subscriptionsync/service/sync.go` around lines 33 - 73, The chunk loop in invoicePendingLines is aborting the whole sync pass too early when Billing returns ErrInvoiceCreateNoLines or an empty invoices slice, which can skip later valid chunks. Update the control flow in invoicePendingLines so that these cases only skip the current lineIDChunk and continue processing the remaining chunks instead of returning nil from the whole function. Keep the existing error handling for real failures and preserve the current chunking behavior driven by s.featureFlags.MaxLinesPerCollectedInvoice.
🧹 Nitpick comments (2)
openmeter/billing/worker/subscriptionsync/service/sync_unit_test.go (1)
70-94: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest gap: no coverage for a mid-batch empty chunk followed by more valid chunks.
This test only uses a single line ID, so
lineIDChunkshas exactly one entry — it can't actually exercise "exit early vs. continue" behavior since there's no subsequent chunk to skip. Given the loop-termination concern raised insync.go(lines 33-73), a test with ≥2 chunks where an earlier chunk returns empty/ErrInvoiceCreateNoLineswhile a later chunk still has real invoices would catch that regression; current tests would pass either way.As per path instructions: "Suggest missing tests only when the PR introduces behavior without meaningful coverage, or when the current tests would pass despite a concrete regression in the changed code."
🤖 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/worker/subscriptionsync/service/sync_unit_test.go` around lines 70 - 94, Add a test in invoicePendingLinesExitsOnEmptySuccess that uses multiple line chunks so it actually exercises the loop in Service.invoicePendingLines: make invoicePendingLinesBillingService return an empty success or ErrInvoiceCreateNoLines for an earlier chunk and a real invoice result for a later chunk. Verify the method continues past the empty chunk instead of stopping early by asserting both billingService calls are made and the later chunk is processed correctly.Source: Path instructions
openmeter/billing/worker/subscriptionsync/service/sync.go (1)
279-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a short docstring for the new domain helpers.
invoiceLineIDsForPendingCollectionandisLineInvoiceableNowcompress non-obvious business rules (dedup across create/update patches, gathering-invoice-only updates, and truncating tostreaming.MinimumWindowSizeDurationto decide "invoiceable now"). A brief comment on the observable contract (e.g., why truncation to the minimum window is used to decide eligibility) would help future readers avoid re-deriving the intent from the implementation.As per coding guidelines: "Add docstrings to domain helpers when the name compresses important business semantics, and describe the observable business contract rather than implementation mechanics."
🤖 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/worker/subscriptionsync/service/sync.go` around lines 279 - 366, Add brief docstrings/comments for the new domain helpers `invoiceLineIDsForPendingCollection` and `isLineInvoiceableNow`. The issue is that these helpers encode important business rules that are not obvious from their names alone, so document the observable contract: which invoice patch cases are included, that line IDs are deduplicated, and that “invoiceable now” is determined by comparing `GetInvoiceAt()` against `clock.Now()` after truncating to `streaming.MinimumWindowSizeDuration`. Keep the comments focused on intent rather than implementation details.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/worker/subscriptionsync/service/sync.go`:
- Around line 33-73: The chunk loop in invoicePendingLines is aborting the whole
sync pass too early when Billing returns ErrInvoiceCreateNoLines or an empty
invoices slice, which can skip later valid chunks. Update the control flow in
invoicePendingLines so that these cases only skip the current lineIDChunk and
continue processing the remaining chunks instead of returning nil from the whole
function. Keep the existing error handling for real failures and preserve the
current chunking behavior driven by s.featureFlags.MaxLinesPerCollectedInvoice.
---
Nitpick comments:
In `@openmeter/billing/worker/subscriptionsync/service/sync_unit_test.go`:
- Around line 70-94: Add a test in invoicePendingLinesExitsOnEmptySuccess that
uses multiple line chunks so it actually exercises the loop in
Service.invoicePendingLines: make invoicePendingLinesBillingService return an
empty success or ErrInvoiceCreateNoLines for an earlier chunk and a real invoice
result for a later chunk. Verify the method continues past the empty chunk
instead of stopping early by asserting both billingService calls are made and
the later chunk is processed correctly.
In `@openmeter/billing/worker/subscriptionsync/service/sync.go`:
- Around line 279-366: Add brief docstrings/comments for the new domain helpers
`invoiceLineIDsForPendingCollection` and `isLineInvoiceableNow`. The issue is
that these helpers encode important business rules that are not obvious from
their names alone, so document the observable contract: which invoice patch
cases are included, that line IDs are deduplicated, and that “invoiceable now”
is determined by comparing `GetInvoiceAt()` against `clock.Now()` after
truncating to `streaming.MinimumWindowSizeDuration`. Keep the comments focused
on intent rather than implementation details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc452452-cdca-4c9f-9c9b-ee2b651b698f
📒 Files selected for processing (2)
openmeter/billing/worker/subscriptionsync/service/sync.goopenmeter/billing/worker/subscriptionsync/service/sync_unit_test.go
turip
force-pushed
the
feat/collected-invoice-line-limit
branch
from
0c21b7e to
a0597a6
Compare
turip
force-pushed
the
feat/collected-invoice-line-limit
branch
from
97242a9 to
4560541
Compare
3 participants
Summary
Adds a billing feature switch for limiting how many pending invoice lines are collected into each standard invoice.
billing.featureSwitches.maxLinesPerCollectedInvoice0as the default unlimited behaviorNotes
Filtering happens before line preparation, standard invoice creation, quantity snapshotting, and invoice calculation. Lines beyond the limit stay on the gathering invoice for a later collection pass.
Validation
go test -tags=dynamic ./app/config ./openmeter/billing ./openmeter/billing/service ./openmeter/billing/worker/collect ./openmeter/billing/worker/subscriptionsync/service ./app/common ./cmd/billing-worker ./cmd/jobs/internal go vet -tags=dynamic ./app/config ./app/common ./openmeter/billing ./openmeter/billing/service ./openmeter/billing/worker/collect ./openmeter/billing/worker/subscriptionsync/serviceSummary by CodeRabbit
billing.featureSwitches.maxLinesPerCollectedInvoiceto cap invoice line collection during subscription processing.maxLinesPerInvoicesupport to cap pending invoice line generation, including deterministic capped selection.MaxLinesPerInvoiceconfiguration into pending line generation.config.example.yamlwithbilling.featureSwitches.maxLinesPerCollectedInvoice.