refactor: prepare for charges integration#4023
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds invoice-ID filtering to listing and refactors subscription-sync reconciliation: introduces a persisted Item abstraction, moves persisted invoices into state, replaces legacy patch types with invoice-scoped patch collections and a router, and routes invoice-listing by IDs through ListInvoices. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as Subscription Service
participant Loader as PersistedState Loader
participant InvoiceSvc as Billing/ListInvoices
participant Reconciler as Reconciler (Plan/Apply)
participant Router as PatchCollection Router
participant Updater as Invoice Updater
Service->>Loader: LoadForSubscription(subscription)
Loader->>InvoiceSvc: ListInvoices(Namespaces, IDs, IncludeDeleted)
InvoiceSvc-->>Loader: invoices map
Loader-->>Service: State{ByUniqueID: Items, Invoices}
Service->>Reconciler: Plan(state, targets)
Reconciler->>Router: newPatchCollectionRouter(invoices)
Reconciler->>Router: route items -> PatchCollection
Router-->>Reconciler: InvoicePatches
Reconciler->>Updater: Apply(InvoicePatches)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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: 4
🧹 Nitpick comments (2)
openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoice.go (1)
41-43: Optional hardening: return defensive copies of patch slices.Returning internal slices directly can let callers mutate collection internals by accident.
Possible tweak
func (c invoicePatchCollectionBase) Patches() []InvoicePatch { - return c.patches + return append([]InvoicePatch(nil), c.patches...) } @@ func (p genericInvoicePatch) GetInvoicePatches() ([]invoiceupdater.Patch, error) { - return p.invoiceUpdates, nil + return append([]invoiceupdater.Patch(nil), p.invoiceUpdates...), nil }Also applies to: 67-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoice.go` around lines 41 - 43, The Patches() method on invoicePatchCollectionBase currently returns the internal slice directly, allowing external mutation; modify invoicePatchCollectionBase.Patches (and the equivalent method at the other occurrence) to return a defensive copy of c.patches (e.g., allocate a new slice, copy elements, and return that) so callers cannot mutate internal state while preserving the original element values and slice length/capacity semantics.openmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.go (1)
224-231: Consider collapsing duplicate last-line annotation lookups in this loop.Right now we call
HasLastLineAnnotation(...)twice on the same persisted item (ignore+forceContinuous). For hierarchy-backed items this can mean two child scans per iteration; a single-fetch shape (or API that returns both flags together) would keep this path leaner.As per coding guidelines "Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations ... should be vetted for potential performance bottlenecks."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.go` around lines 224 - 231, The loop is calling HasLastLineAnnotation twice on the same persisted item (existingPreviousLine) which can be expensive for hierarchy-backed items; change the code to fetch both annotation flags in a single call or helper so you only scan children once: use existingPreviousLine.IsSubscriptionManaged() together with a single retrieval like a new method (e.g., existingPreviousLine.GetLastLineAnnotationFlags or GetLastLineAnnotations) or a single HasLastLineAnnotation call that returns both billing.AnnotationSubscriptionSyncIgnore and billing.AnnotationSubscriptionSyncForceContinuousLines values, then use those two booleans instead of calling HasLastLineAnnotation twice (references: existingPreviousLine.IsSubscriptionManaged, existingPreviousLine.HasLastLineAnnotation, billing.AnnotationSubscriptionSyncIgnore, billing.AnnotationSubscriptionSyncForceContinuousLines).
🤖 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/worker/subscriptionsync/service/persistedstate/item.go`:
- Around line 82-86: Guard against a nil Item before performing the type
assertion in ItemAsLine so you don't call getErrorDetails(nil); explicitly check
if in == nil and return a clear error before asserting to LineGetter, and apply
the same nil-check hardening to the other conversion helper in this file (the
other function that asserts to LineGetter around the later block) so
getErrorDetails is only invoked for non-nil values.
In `@openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go`:
- Around line 46-66: The code is building a map byUniqueID from lines but still
calls loadInvoicesForSubscriptionLines(ctx, subs, lines) over the unfiltered
lines; change the call to only pass the rows that were actually added to
byUniqueID (e.g., collect filteredLines while looping or derive them from the
map) so loadInvoicesForSubscriptionLines receives only persisted rows. Update
the call site of loadInvoicesForSubscriptionLines to use the filtered slice (or
the map keys/values) and keep the existing error handling in loader.go around
NewItemFromLineOrHierarchy and byUniqueID checks.
In `@openmeter/billing/worker/subscriptionsync/service/reconciler/patch.go`:
- Around line 71-79: The GetCollectionFor method on patchCollectionRouter
currently returns nil for unknown persistedstate.Item types which leads to a
later panic in diffItem() when it immediately calls Add* on the returned
PatchCollection; change this to fail explicitly by either (A) returning an
erroring implementation of PatchCollection that returns a descriptive error from
its Add* methods (so callers like diffItem() get deterministic errors), or (B)
change the signature of patchCollectionRouter.GetCollectionFor to return
(PatchCollection, error) and return a clear error for unsupported item types,
updating callers (e.g., diffItem()) to handle the error instead of dereferencing
a nil collection.
In
`@openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.go`:
- Around line 112-127: When reviving managed child lines you must also revive
the parent split group: after you call existingHierarchy.Group.ToUpdate() (in
both AddShrink and AddExtend) and you perform updatedLine.SetDeletedAt(nil) for
a managed child, set updatedGroup.DeletedAt = nil before creating the
NewUpdateSplitLineGroupPatch; this ensures the group's DeletedAt is cleared
alongside managed children so the parent is not left deleted while children are
active.
---
Nitpick comments:
In
`@openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoice.go`:
- Around line 41-43: The Patches() method on invoicePatchCollectionBase
currently returns the internal slice directly, allowing external mutation;
modify invoicePatchCollectionBase.Patches (and the equivalent method at the
other occurrence) to return a defensive copy of c.patches (e.g., allocate a new
slice, copy elements, and return that) so callers cannot mutate internal state
while preserving the original element values and slice length/capacity
semantics.
In
`@openmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.go`:
- Around line 224-231: The loop is calling HasLastLineAnnotation twice on the
same persisted item (existingPreviousLine) which can be expensive for
hierarchy-backed items; change the code to fetch both annotation flags in a
single call or helper so you only scan children once: use
existingPreviousLine.IsSubscriptionManaged() together with a single retrieval
like a new method (e.g., existingPreviousLine.GetLastLineAnnotationFlags or
GetLastLineAnnotations) or a single HasLastLineAnnotation call that returns both
billing.AnnotationSubscriptionSyncIgnore and
billing.AnnotationSubscriptionSyncForceContinuousLines values, then use those
two booleans instead of calling HasLastLineAnnotation twice (references:
existingPreviousLine.IsSubscriptionManaged,
existingPreviousLine.HasLastLineAnnotation,
billing.AnnotationSubscriptionSyncIgnore,
billing.AnnotationSubscriptionSyncForceContinuousLines).
🪄 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: bd9d7b51-ef67-4d94-9af5-6c903203248c
📒 Files selected for processing (20)
openmeter/billing/invoice.goopenmeter/billing/service/invoice.goopenmeter/billing/worker/subscriptionsync/service/persistedstate/item.goopenmeter/billing/worker/subscriptionsync/service/persistedstate/loader.goopenmeter/billing/worker/subscriptionsync/service/persistedstate/state.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patch.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchcreate.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchdelete.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchextend.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchhelpers.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoice.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoiceline.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchprorate.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchshrink.goopenmeter/billing/worker/subscriptionsync/service/reconciler/prorate.goopenmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.goopenmeter/billing/worker/subscriptionsync/service/sync.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.go
💤 Files with no reviewable changes (6)
- openmeter/billing/worker/subscriptionsync/service/sync.go
- openmeter/billing/worker/subscriptionsync/service/reconciler/patchdelete.go
- openmeter/billing/worker/subscriptionsync/service/reconciler/patchcreate.go
- openmeter/billing/worker/subscriptionsync/service/reconciler/patchshrink.go
- openmeter/billing/worker/subscriptionsync/service/reconciler/patchprorate.go
- openmeter/billing/worker/subscriptionsync/service/reconciler/patchextend.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go (1)
80-97: Consider adding a default case for future-proofing.The switch handles the current two
LineOrHierarchyTypevalues, but if a new type is ever added, this would silently skip it without collecting its invoice IDs. That could lead to confusing "invoice not found" errors downstream rather than failing at the source.This is a minor defensive coding suggestion - totally fine to defer if the type enum is considered stable.
🛡️ Optional: Add explicit default case
case billing.LineOrHierarchyTypeHierarchy: hierarchy, err := line.AsHierarchy() if err != nil { return Invoices{}, fmt.Errorf("getting hierarchy invoice ids: %w", err) } for _, child := range hierarchy.Lines { invoiceIDs[child.Invoice.GetID()] = struct{}{} } + default: + return Invoices{}, fmt.Errorf("unsupported line type for invoice loading: %s", line.Type()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go` around lines 80 - 97, The switch over line.Type() currently handles billing.LineOrHierarchyTypeLine and billing.LineOrHierarchyTypeHierarchy but omits a default case; add a default branch to the switch that returns a clear error (using fmt.Errorf) including the unexpected line.Type() value so the caller (and the invoiceIDs map population) fails fast when a new LineOrHierarchyType is introduced; update the existing switch surrounding invoiceIDs and line (the block that calls line.AsGenericLine()/line.AsHierarchy() and populates invoiceIDs) to include this default error-returning case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go`:
- Around line 80-97: The switch over line.Type() currently handles
billing.LineOrHierarchyTypeLine and billing.LineOrHierarchyTypeHierarchy but
omits a default case; add a default branch to the switch that returns a clear
error (using fmt.Errorf) including the unexpected line.Type() value so the
caller (and the invoiceIDs map population) fails fast when a new
LineOrHierarchyType is introduced; update the existing switch surrounding
invoiceIDs and line (the block that calls
line.AsGenericLine()/line.AsHierarchy() and populates invoiceIDs) to include
this default error-returning case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d930735-e884-4ac5-9494-3ea4a0e179bd
📒 Files selected for processing (1)
openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go (1)
46-73: LGTM! Clean refactor of the persisted state construction.The explicit loop with proper error handling for
NewItemFromLineOrHierarchyis much clearer than the previousslicesx.UniqueGroupByapproach. The duplicate ID detection and early-return pattern keeps the logic easy to follow.One tiny nit: the duplicate error message on line 59 could include the actual ID for easier debugging:
✨ Optional: include duplicate ID in error
if _, ok := byUniqueID[*uniqueID]; ok { - return State{}, fmt.Errorf("duplicate unique ids in the existing lines") + return State{}, fmt.Errorf("duplicate unique ids in the existing lines: %s", *uniqueID) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go` around lines 46 - 73, The duplicate-ID error message in the loop that builds byUniqueID (inside the same loader.go loop using uniqueID and NewItemFromLineOrHierarchy) should include the actual duplicated ID for easier debugging; update the error returned when detecting an existing key (the branch that checks if _, ok := byUniqueID[*uniqueID]; ok) to format the message with *uniqueID (and optionally surrounding context like subscription/line info) instead of the generic "duplicate unique ids in the existing lines".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go`:
- Around line 46-73: The duplicate-ID error message in the loop that builds
byUniqueID (inside the same loader.go loop using uniqueID and
NewItemFromLineOrHierarchy) should include the actual duplicated ID for easier
debugging; update the error returned when detecting an existing key (the branch
that checks if _, ok := byUniqueID[*uniqueID]; ok) to format the message with
*uniqueID (and optionally surrounding context like subscription/line info)
instead of the generic "duplicate unique ids in the existing lines".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5562e580-ab58-4b03-b9ab-ceca0add029e
📒 Files selected for processing (1)
openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go
tothandras
requested changes
Mar 28, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/billing/worker/subscriptionsync/service/reconciler/patch.go (1)
88-96: Consider adding a brief comment on when nil patches can appear.The nil filtering is good defensive coding, but it might help future maintainers to understand when/why a patch entry could be nil. If it's an invariant that should never happen in practice, a comment like
// filter defensive: should not occur if collections are constructed correctlywould clarify intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/reconciler/patch.go` around lines 88 - 96, Add a short explanatory comment above the nil-filter in patchCollectionRouter.CollectInvoicePatches explaining when nil InvoicePatch entries can appear (e.g., if lineCollection.Patches() or hierarchyCollection.Patches() may return nil entries during construction/merge or when a collector fails) or state that nils are a defensive invariant (e.g., "// defensive: nil patches may appear if a sub-collector failed; should not occur with correct construction"). Keep the comment brief and colocate it immediately before the lo.Filter call referencing InvoicePatch, lineCollection.Patches, and hierarchyCollection.Patches.openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.go (1)
160-189: In-place sort modifies the original hierarchy's Lines slice.Line 161 assigns
lines := existingHierarchy.Lineswhich shares the backing array, so theslices.SortFuncon line 162 mutates the original hierarchy. This works fine since the hierarchy isn't reused, but could surprise future maintainers.A defensive copy would make this safer:
Optional: defensive copy
if len(existingHierarchy.Lines) > 0 { - lines := existingHierarchy.Lines + lines := slices.Clone(existingHierarchy.Lines) slices.SortFunc(lines, func(i, j billing.LineWithInvoiceHeader) int {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.go` around lines 160 - 189, The code sorts existingHierarchy.Lines in-place (lines := existingHierarchy.Lines followed by slices.SortFunc) which mutates the original hierarchy; make a defensive copy of the slice before sorting (copy into a new []billing.LineWithInvoiceHeader) so the original existingHierarchy.Lines is not modified, then run slices.SortFunc on that new slice and continue using that copied slice for deriving lastChild and creating the invoiceupdater.NewUpdateLinePatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/billing/worker/subscriptionsync/service/reconciler/patch.go`:
- Around line 88-96: Add a short explanatory comment above the nil-filter in
patchCollectionRouter.CollectInvoicePatches explaining when nil InvoicePatch
entries can appear (e.g., if lineCollection.Patches() or
hierarchyCollection.Patches() may return nil entries during construction/merge
or when a collector fails) or state that nils are a defensive invariant (e.g.,
"// defensive: nil patches may appear if a sub-collector failed; should not
occur with correct construction"). Keep the comment brief and colocate it
immediately before the lo.Filter call referencing InvoicePatch,
lineCollection.Patches, and hierarchyCollection.Patches.
In
`@openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.go`:
- Around line 160-189: The code sorts existingHierarchy.Lines in-place (lines :=
existingHierarchy.Lines followed by slices.SortFunc) which mutates the original
hierarchy; make a defensive copy of the slice before sorting (copy into a new
[]billing.LineWithInvoiceHeader) so the original existingHierarchy.Lines is not
modified, then run slices.SortFunc on that new slice and continue using that
copied slice for deriving lastChild and creating the
invoiceupdater.NewUpdateLinePatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2be4ac4a-0bf5-4d80-b589-7af700374995
📒 Files selected for processing (3)
openmeter/billing/worker/subscriptionsync/service/reconciler/patch.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.goopenmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go
tothandras
approved these changes
Mar 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Overview
This refactor introduces a package-owned persisted-state abstraction for subscription sync, replacing direct billing.LineOrHierarchy usage with typed persistedstate.Item wrappers and moving referenced invoice loading into
the persisted snapshot itself.
This allows us to add charges as one of these Items, thus charges updates will be driver via a different codepath without the danger of changing the existing flow.
It also restructures reconciliation around per-persisted-shape invoice patch collections, so line-backed and split-line-hierarchy-backed states are patched through separate direct-billing
paths while preserving current behavior.
Along the way, target filtering for direct billing was made explicit, unknown persisted invoices now surface errors instead of being silently assumed gathering, and the progressive
billing cancellation path was fixed so emptied hierarchy children are deleted rather than updated.
Detailed changes
The main differences introduced by this refactor in subscriptionsync are:
See openmeter/billing/worker/subscriptionsync/service/persistedstate/item.go, openmeter/billing/worker/subscriptionsync/service/persistedstate/state.go, openmeter/billing/worker/subscriptionsync/service/persistedstate/
loader.go.
This is a real behavior change versus main, which loaded broader customer invoice state.
Missing referenced invoices now fail explicitly instead of being tolerated.
That is a meaningful safety change.
It now emits realized invoice patches through per-shape collections.
See openmeter/billing/worker/subscriptionsync/service/reconciler/patch.go, openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoice.go, openmeter/billing/worker/subscriptionsync/service/reconciler/
patchinvoiceline.go, openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.go.
Existing line-backed and hierarchy-backed states go through separate patching logic, which is the main architectural change for future charge support.
It now uses target-state service period directly via openmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.go.
non-billable items and items whose GetExpectedLine() is nil are removed from invoice-sync scope up front.
See openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go.
The persisted-item abstraction now owns “subscription managed” and “last line annotation” semantics.
See openmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.go and openmeter/billing/worker/subscriptionsync/service/persistedstate/item.go.
That was a real behavior fix validated by the progressive billing cancellation scenario.
Fixes #(issue)
Notes for reviewer
Summary by CodeRabbit
New Features
Bug Fixes
Refactor