feat: subscription sync semantic patching#4014
Merged
Merged
Conversation
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:
📝 WalkthroughWalkthroughRefactors subscription reconciliation to an interface-driven Patch model, moves invoice application into a new invoiceupdater package, threads a currency calculator and customer-deletion timestamp through plan/target building, and adds dry-run options to SynchronizeSubscription. Patches now produce invoice-level patches via invoiceupdater before applying. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as SynchronizeSubscription
participant Service as Subscriptionsync Service
participant Target as TargetState Builder
participant Reconciler as Reconciler
participant Patch as Patch (Create/Delete/Extend/Shrink/Prorate)
participant InvUpd as invoiceupdater.Updater
participant Billing as Billing / PersistedState
Caller->>Service: request sync(ctx, subs, asOf, opts...)
Service->>Service: resolve currency, customerDeletedAt, options
Service->>Target: BuildInput{AsOf, CustomerDeletedAt, SubscriptionView, Persisted}
Target-->>Service: State (targets)
Service->>Reconciler: buildSyncPlan(ctx, subsView, asOf, customerDeletedAt, currency)
Reconciler->>Patch: diff -> produce []Patch
loop for each Patch
Reconciler->>Patch: GetInvoicePatches({Subscription, Currency, Invoices})
Patch-->>Reconciler: []invoiceupdater.Patch / error
end
alt DryRun
Reconciler->>InvUpd: LogPatches(patches)
else Apply
Reconciler->>InvUpd: ApplyPatches(ctx, patches)
InvUpd->>Billing: modify invoices
Billing-->>InvUpd: result
end
InvUpd-->>Reconciler: result / errors
Reconciler-->>Service: apply result
Service-->>Caller: complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
turip
force-pushed
the
feat/semantic-patching
branch
from
c967619 to
12b4ea4
Compare
turip
added
release-note/feature
turip
changed the title
![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: 5
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/reconciler/invoiceupdater/invoiceupdate.go (1)
315-333:⚠️ Potential issue | 🟠 MajorImmutable flat-fee updates still miss period-only changes.
With
ProratePatchnow emitting flat-fee line updates for service-period changes, this branch only checks per-unit amount and thencontinues. A period-only change on an immutable invoice gets silently dropped instead of surfacing a validation issue.🐛 Suggested fix
if IsFlatFee(targetState) { + if !targetState.GetServicePeriod().Equal(existingLine.GetServicePeriod()) { + validationIssues = append(validationIssues, + newValidationIssueOnLine( + existingLine, + "flat fee line's service period cannot be changed on immutable invoice (new period: %s..%s)", + targetState.GetServicePeriod().From, + targetState.GetServicePeriod().To, + ), + ) + } + existingPerUnitAmount, err := GetFlatFeePerUnitAmount(existingLine) if err != nil { return fmt.Errorf("getting flat fee per unit amount: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/invoiceupdate.go` around lines 315 - 333, The current IsFlatFee branch only compares per-unit amounts via GetFlatFeePerUnitAmount and then continues, so period-only updates (from ProratePatch) on immutable invoices are ignored; update the branch in invoiceupdate.go: after computing existingPerUnitAmount and targetPerUnitAmount (and before the continue), if they differ keep the existing validationAppend using newValidationIssueOnLine as-is, otherwise also compare the service period on existingLine and targetState (e.g., existingLine.ServicePeriod vs targetState.ServicePeriod or the equivalent fields) and if the period differs append a validation issue stating the flat fee line's service period cannot be changed on immutable invoice; only continue when both per-unit and period are equal.
🧹 Nitpick comments (1)
openmeter/billing/worker/subscriptionsync/service/reconciler/patchextend.go (1)
85-89: A single scan is cheaper than resorting the whole hierarchy.
lines := existingHierarchy.Linesshares the backing slice, so this reorders the source hierarchy just to find the latest child. A one-pass max search keeps this O(n) and avoids the in-place mutation.As per coding guidelines, "Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks."♻️ Suggested shape
- lines := existingHierarchy.Lines - slices.SortFunc(lines, func(i, j billing.LineWithInvoiceHeader) int { - return timeutil.Compare(i.Line.GetServicePeriod().To, j.Line.GetServicePeriod().To) - }) - - lastChild, err := lines[len(lines)-1].Line.CloneWithoutChildren() + lastChildIdx := 0 + for i := 1; i < len(existingHierarchy.Lines); i++ { + if timeutil.Compare( + existingHierarchy.Lines[i].Line.GetServicePeriod().To, + existingHierarchy.Lines[lastChildIdx].Line.GetServicePeriod().To, + ) > 0 { + lastChildIdx = i + } + } + + lastChild, err := existingHierarchy.Lines[lastChildIdx].Line.CloneWithoutChildren()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/reconciler/patchextend.go` around lines 85 - 89, The code currently assigns lines := existingHierarchy.Lines and calls slices.SortFunc (using timeutil.Compare on billing.LineWithInvoiceHeader) which mutates the shared slice; instead perform a single-pass max search over existingHierarchy.Lines to find the child with the latest Line.GetServicePeriod().To without reordering the slice. Replace the slices.SortFunc usage and the temporary lines variable by iterating existingHierarchy.Lines, track the index or reference of the max element (compare using timeutil.Compare on each element's Line.GetServicePeriod().To), and use that element as the "latest child" so the hierarchy backing slice remains unchanged and you achieve O(n) time with no in-place mutation.
🤖 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/reconciler/invoiceupdater/patch.go`:
- Around line 150-156: The code currently always returns a NewDeleteLinePatch
for a line unless billing.AnnotationSubscriptionSyncIgnore is set; change the
logic to skip emitting a delete patch when the line is already deleted by
checking the line's DeletedAt/DeletedAt timestamp (e.g., via line.GetDeletedAt()
or equivalent) before creating NewDeleteLinePatch(line.GetLineID(),
line.GetInvoiceID()); if DeletedAt is non-zero/null then return nil,nil,
otherwise proceed to return the delete patch as before.
- Around line 157-178: The hierarchy branch must respect
billing.AnnotationSubscriptionSyncIgnore: when handling
lineOrHierarchy.AsHierarchy() (group variable), detect if the group or any child
line carries the billing.AnnotationSubscriptionSyncIgnore annotation and skip
deletions accordingly — do not append NewDeleteSplitLineGroupPatch if the group
is ignored or any child is ignored, and when iterating group.Lines skip
NewDeleteLinePatch for any child whose line annotations include
billing.AnnotationSubscriptionSyncIgnore (still skip lines with GetDeletedAt()
!= nil). Update the logic around group.Group.DeletedAt, the group deletion
append, and the loop over group.Lines to check annotations before appending
delete patches.
In `@openmeter/billing/worker/subscriptionsync/service/reconciler/patchshrink.go`:
- Around line 70-73: The getInvoicePatchesForHierarchy function currently
doesn't honor the hierarchy skip rules and may shrink hierarchies annotated with
billing.AnnotationSubscriptionSyncIgnore; add a guard at the top of
ShrinkUsageBasedPatch.getInvoicePatchesForHierarchy that calls
shouldSkipHierarchyPatch(existingHierarchy) and if true returns no patches (nil,
nil) to skip any truncation/deletion; ensure you reference the
shouldSkipHierarchyPatch helper and keep the existing shrink logic unchanged
after this early-return check.
In `@openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go`:
- Around line 133-147: The switch in reconciler.go only compares targetPeriod.To
vs existingPeriod.To and misses cases where correctPeriodStartForUpcomingLines()
advances ServicePeriod.Start while To is unchanged; update the logic to compare
both bounds (ServicePeriod.Start and ServicePeriod.To) and return an appropriate
patch when only the start changes (e.g., add a new StartShiftUsageBasedPatch or
extend Shrink/ExtendUsageBasedPatch semantics to handle start-only shifts),
ensuring the returned patch includes UniqueID, Existing, and Target so
downstream apply logic can persist the updated period; refactor the switch to
explicitly check targetPeriod.Start vs existingPeriod.Start (before/after) in
addition to the To comparisons or add a dedicated branch to handle start-only
adjustments.
- Around line 101-110: The branch in reconciler.go that handles "case target !=
nil && existing == nil" currently always returns a CreatePatch even when
GetExpectedLine() returned expectedLine == nil; update this conditional to only
return a CreatePatch when expectedLine != nil, and otherwise return no patch
(e.g., nil, false, nil) so Plan.IsEmpty() and CreatePatch.GetInvoicePatches()
don't produce a phantom create; make the change near the case handling in the
reconciler so CreatePatch, DeletePatch, GetExpectedLine, and Plan.IsEmpty()
behave correctly together.
---
Outside diff comments:
In
`@openmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/invoiceupdate.go`:
- Around line 315-333: The current IsFlatFee branch only compares per-unit
amounts via GetFlatFeePerUnitAmount and then continues, so period-only updates
(from ProratePatch) on immutable invoices are ignored; update the branch in
invoiceupdate.go: after computing existingPerUnitAmount and targetPerUnitAmount
(and before the continue), if they differ keep the existing validationAppend
using newValidationIssueOnLine as-is, otherwise also compare the service period
on existingLine and targetState (e.g., existingLine.ServicePeriod vs
targetState.ServicePeriod or the equivalent fields) and if the period differs
append a validation issue stating the flat fee line's service period cannot be
changed on immutable invoice; only continue when both per-unit and period are
equal.
---
Nitpick comments:
In `@openmeter/billing/worker/subscriptionsync/service/reconciler/patchextend.go`:
- Around line 85-89: The code currently assigns lines := existingHierarchy.Lines
and calls slices.SortFunc (using timeutil.Compare on
billing.LineWithInvoiceHeader) which mutates the shared slice; instead perform a
single-pass max search over existingHierarchy.Lines to find the child with the
latest Line.GetServicePeriod().To without reordering the slice. Replace the
slices.SortFunc usage and the temporary lines variable by iterating
existingHierarchy.Lines, track the index or reference of the max element
(compare using timeutil.Compare on each element's Line.GetServicePeriod().To),
and use that element as the "latest child" so the hierarchy backing slice
remains unchanged and you achieve O(n) time with no in-place mutation.
🪄 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: 4526daba-7691-4466-920d-3ffc3c460475
📒 Files selected for processing (16)
openmeter/billing/worker/subscriptionsync/service/reconcile.goopenmeter/billing/worker/subscriptionsync/service/reconciler/apply.goopenmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/feehelper.goopenmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/invoiceupdate.goopenmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/patch.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/patchprorate.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchshrink.goopenmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.goopenmeter/billing/worker/subscriptionsync/service/sync.goopenmeter/billing/worker/subscriptionsync/service/targetstate/phaseiterator.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.go
💤 Files with no reviewable changes (1)
- openmeter/billing/worker/subscriptionsync/service/reconciler/apply.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
openmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/patch.go (1)
169-174:⚠️ Potential issue | 🟠 MajorDon't let one ignored child cancel the whole hierarchy delete.
Right now the first ignored child returns
nil, nil, so non-ignored siblings never get cleaned up either. For mixed hierarchies, that leaves stale subscription-managed lines behind. A safer split is to skip the group delete when an ignored child is present, but still delete the non-ignored, non-deleted children.💡 Minimal fix
- // Skip the group if any of the lines are ignored - for _, line := range group.Lines { - if line.Line.GetAnnotations().GetBool(billing.AnnotationSubscriptionSyncIgnore) { - return nil, nil - } - } + skipGroupDelete := false + for _, line := range group.Lines { + if line.Line.GetAnnotations().GetBool(billing.AnnotationSubscriptionSyncIgnore) { + skipGroupDelete = true + break + } + } - if group.Group.DeletedAt == nil { + if !skipGroupDelete && group.Group.DeletedAt == nil { out = append(out, NewDeleteSplitLineGroupPatch(models.NamespacedID{ Namespace: group.Group.Namespace, ID: group.Group.ID, })) } for _, line := range group.Lines { + if line.Line.GetAnnotations().GetBool(billing.AnnotationSubscriptionSyncIgnore) { + continue + } if line.Line.GetDeletedAt() != nil { continue }Also applies to: 183-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/patch.go` around lines 169 - 174, The current loop over group.Lines returns nil when any child has billing.AnnotationSubscriptionSyncIgnore, which cancels deletion for the entire group; instead modify the logic in the invoice updater (the loop using group.Lines and billing.AnnotationSubscriptionSyncIgnore in patch.go) to continue past ignored children and only skip deletion for those specific lines while still processing/deleting non-ignored, non-deleted siblings; implement this by filtering or collecting deletable lines (skip when line.Line.GetAnnotations().GetBool(billing.AnnotationSubscriptionSyncIgnore) is true) and only return nil for the whole group when all children are ignored or already deleted, and apply the same change to the analogous block around lines 183-189.
🤖 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/reconciler/patchshrink.go`:
- Around line 81-120: The loop iterating existingHierarchy.Lines must skip
already-deleted children so they are not cloned/updated; in the loop over child
(existingHierarchy.Lines) add a guard that checks the child's deletion state
(e.g., child.Line.GetDeletedAt() != nil or an equivalent IsDeleted check) and
continue when deleted before any cloning or UpdateServicePeriod/SetDeletedAt
logic (affecting CloneWithoutChildren, UpdateServicePeriod, SetInvoiceAt,
SetDeletedAt, and building update/delete patches); ensure deleted children
remain untouched and only non-deleted children proceed to cloning and patch
generation.
In `@openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go`:
- Around line 299-320: Guard the Apply method against a nil or empty plan: at
the start of Service.Apply(ctx, input ApplyInput) check if input.Plan == nil ||
input.Plan.IsEmpty() and return nil (no-op) to avoid dereferencing a nil Plan;
this uses the existing Plan.IsEmpty() semantics but adds the explicit nil check
before iterating patches and calling invoiceupdater.ApplyPatches.
- Around line 242-289: The Plan.Patches order is non-deterministic because
deletedLines and inScopeLineUniqueIDs iterate map keys; make the patch
generation deterministic by sorting the IDs before creating patches.
Specifically, after computing deletedLines and inScopeLineUniqueIDs from
persisted.ByUniqueID and inScopeLinesByUniqueID, sort those slices (or iterate
persisted.Lines and input.Target.Items in stable order) and then call s.diffItem
for each id; ensure you still use persisted.ByUniqueID, inScopeLinesByUniqueID,
GetExpectedLine, and diffItem but append patches in the sorted iteration so
Plan.Patches is deterministic.
---
Duplicate comments:
In
`@openmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/patch.go`:
- Around line 169-174: The current loop over group.Lines returns nil when any
child has billing.AnnotationSubscriptionSyncIgnore, which cancels deletion for
the entire group; instead modify the logic in the invoice updater (the loop
using group.Lines and billing.AnnotationSubscriptionSyncIgnore in patch.go) to
continue past ignored children and only skip deletion for those specific lines
while still processing/deleting non-ignored, non-deleted siblings; implement
this by filtering or collecting deletable lines (skip when
line.Line.GetAnnotations().GetBool(billing.AnnotationSubscriptionSyncIgnore) is
true) and only return nil for the whole group when all children are ignored or
already deleted, and apply the same change to the analogous block around lines
183-189.
🪄 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: 066bfe45-463c-4fe6-aaed-2767f54fdff6
📒 Files selected for processing (3)
openmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/patch.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchshrink.goopenmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go (2)
300-328:⚠️ Potential issue | 🟠 MajorApply() still needs a nil plan guard.
Line 301 dereferences
input.Plan.Patcheswithout checking ifinput.Planis nil. WhilePlan.IsEmpty()handles nil gracefully,Apply()will panic if called with a nil plan.🛡️ Suggested fix
func (s *Service) Apply(ctx context.Context, input ApplyInput) error { + if input.Plan == nil || input.Plan.IsEmpty() { + return nil + } + invoicePatches := make([]invoiceupdater.Patch, 0, len(input.Plan.Patches))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go` around lines 300 - 328, Apply dereferences input.Plan.Patches and can panic when input.Plan is nil; add a nil guard at the start of Service.Apply (checking input.Plan == nil or input.Plan.IsEmpty()) and return early (no-op) when there's no plan to apply, so you avoid accessing Plan.Patches; move the invoicePatches creation/iteration after this guard (or skip it entirely when plan is nil) and preserve existing DryRun/logging behavior and error returns in Apply and invoiceupdater.ApplyPatches.
243-290:⚠️ Potential issue | 🟠 MajorPlan.Patches ordering is still non-deterministic.
lo.Keys()on lines 243-244 returns map keys in random order (Go's map iteration), and the subsequent loops at lines 249 and 264 iterate over these slices. This makesPlan.Patchesordering unstable across runs.For downstream consumers that may depend on patch ordering (e.g., for rollback sequencing or debugging), consider sorting the IDs before iterating:
🔧 Suggested fix
existingLineUniqueIDs := lo.Keys(persisted.ByUniqueID) inScopeLineUniqueIDs := lo.Keys(inScopeLinesByUniqueID) + sort.Strings(existingLineUniqueIDs) + sort.Strings(inScopeLineUniqueIDs) deletedLines, _ := lo.Difference(existingLineUniqueIDs, inScopeLineUniqueIDs) + sort.Strings(deletedLines)You'll need to add
"sort"to the imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go` around lines 243 - 290, The current creation of existingLineUniqueIDs and inScopeLineUniqueIDs using lo.Keys yields non-deterministic order and thus non-deterministic Plan.Patches; sort the ID slices before iterating (e.g., sort.Strings(existingLineUniqueIDs) and sort.Strings(inScopeLineUniqueIDs)) so the loops that build patches (over deletedLines and inScopeLineUniqueIDs) produce a stable ordering, and add the "sort" import; keep all diff logic using s.diffItem, patches slice creation, and append behavior unchanged.
🧹 Nitpick comments (2)
openmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/invoiceupdate.go (1)
131-147: Defaulting to mutable when invoice not found might mask issues.When the invoice isn't found in
invoicesByID,isMutableInvoicereturnstrue. For dry-run logging this is probably fine, but it could mask issues where the invoice map wasn't properly populated.Consider logging a warning when an invoice isn't found, so operators can investigate if this happens unexpectedly:
💡 Optional improvement
func isMutableInvoice(invoiceID string, invoicesByID map[string]billing.Invoice) bool { invoice, ok := invoicesByID[invoiceID] if !ok { + // Invoice not in the pre-loaded map; assume mutable for dry-run logging. + // If this happens frequently, the loader may need investigation. return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/invoiceupdate.go` around lines 131 - 147, The function isMutableInvoice currently returns true when invoiceID is missing from invoicesByID, which can hide bugs; update isMutableInvoice to emit a warning log when invoice lookup fails (include the invoiceID and context such as len(invoicesByID) or the caller/service name) and then continue to return true for compatibility. Locate the isMutableInvoice function and add a single warning log in the if !ok branch (use the existing package/service logger object available in this package or the passed-in logger if present) so operators can detect unexpected missing entries without changing the existing return behavior.openmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/patch.go (1)
143-158: Consider adding namespace to log entries for better traceability.The
Logmethod provides good observability, but for production debugging across namespaces, including the namespace in log entries (especially for delete operations) would help with filtering.💡 Optional improvement
case PatchOpLineDelete: - logger.Info("delete line patch", "line_id", p.deleteLinePatch.Line, "invoice_id", p.deleteLinePatch.InvoiceID) + logger.Info("delete line patch", "line_id", p.deleteLinePatch.Line.ID, "namespace", p.deleteLinePatch.Line.Namespace, "invoice_id", p.deleteLinePatch.InvoiceID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/patch.go` around lines 143 - 158, The Log method should include the namespace in every log entry for traceability; update each logger.Info call inside Patch.Log (cases PatchOpLineCreate, PatchOpLineDelete, PatchOpLineUpdate, PatchOpSplitLineGroupDelete, PatchOpSplitLineGroupUpdate, and default if applicable) to add a "namespace" field using the namespace available on the affected objects (for example use createLinePatch.Line.GetNamespace(), deleteLinePatch.Namespace, updateLinePatch.TargetState.GetNamespace(), deleteSplitLineGroupPatch.Group.Namespace, updateSplitLineGroupPatch.TargetState.Namespace or the equivalent getters on those structs) so all patch logs are filterable by namespace.
🤖 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/worker/subscriptionsync/service/reconciler/reconciler.go`:
- Around line 300-328: Apply dereferences input.Plan.Patches and can panic when
input.Plan is nil; add a nil guard at the start of Service.Apply (checking
input.Plan == nil or input.Plan.IsEmpty()) and return early (no-op) when there's
no plan to apply, so you avoid accessing Plan.Patches; move the invoicePatches
creation/iteration after this guard (or skip it entirely when plan is nil) and
preserve existing DryRun/logging behavior and error returns in Apply and
invoiceupdater.ApplyPatches.
- Around line 243-290: The current creation of existingLineUniqueIDs and
inScopeLineUniqueIDs using lo.Keys yields non-deterministic order and thus
non-deterministic Plan.Patches; sort the ID slices before iterating (e.g.,
sort.Strings(existingLineUniqueIDs) and sort.Strings(inScopeLineUniqueIDs)) so
the loops that build patches (over deletedLines and inScopeLineUniqueIDs)
produce a stable ordering, and add the "sort" import; keep all diff logic using
s.diffItem, patches slice creation, and append behavior unchanged.
---
Nitpick comments:
In
`@openmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/invoiceupdate.go`:
- Around line 131-147: The function isMutableInvoice currently returns true when
invoiceID is missing from invoicesByID, which can hide bugs; update
isMutableInvoice to emit a warning log when invoice lookup fails (include the
invoiceID and context such as len(invoicesByID) or the caller/service name) and
then continue to return true for compatibility. Locate the isMutableInvoice
function and add a single warning log in the if !ok branch (use the existing
package/service logger object available in this package or the passed-in logger
if present) so operators can detect unexpected missing entries without changing
the existing return behavior.
In
`@openmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/patch.go`:
- Around line 143-158: The Log method should include the namespace in every log
entry for traceability; update each logger.Info call inside Patch.Log (cases
PatchOpLineCreate, PatchOpLineDelete, PatchOpLineUpdate,
PatchOpSplitLineGroupDelete, PatchOpSplitLineGroupUpdate, and default if
applicable) to add a "namespace" field using the namespace available on the
affected objects (for example use createLinePatch.Line.GetNamespace(),
deleteLinePatch.Namespace, updateLinePatch.TargetState.GetNamespace(),
deleteSplitLineGroupPatch.Group.Namespace,
updateSplitLineGroupPatch.TargetState.Namespace or the equivalent getters on
those structs) so all patch logs are filterable by namespace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5277c862-d1ef-4896-b07d-0a821eeb048b
📒 Files selected for processing (6)
openmeter/billing/invoicelinesplitgroup.goopenmeter/billing/worker/subscriptionsync/service/persistedstate/loader.goopenmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/invoiceupdate.goopenmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/patch.goopenmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go (1)
119-137: Minor: Inconsistent pointer semantics between line types.
standardLineis passed by address (&standardLine) whilegatheringLineis already a pointer. This works but the asymmetry is a bit surprising. Might be worth a brief comment explaining this is due to howAsStandardLine()returns a value vsAsGatheringLine()returns a pointer.🤖 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 119 - 137, The switch handling in loader.go treats standardLine and gatheringLine differently because invoiceLine.AsStandardLine() returns a value while invoiceLine.AsGatheringLine() returns a pointer; update the code by adding a brief inline comment near the switch (or directly above the two return lines) that explains this pointer-semantic asymmetry and why billing.NewLineOrHierarchy is called with &standardLine but with gatheringLine as-is (or alternatively normalize by taking an explicit pointer/dereference to make both branches visually consistent), referencing invoiceLine.AsStandardLine(), invoiceLine.AsGatheringLine(), and billing.NewLineOrHierarchy so future readers understand the reason.openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go (1)
317-319: Defensive nil filter on patches.This filter at return time is defensive since
diffItemonly appends whenChanged == true(andPatchshould be non-nil). Keeping it is harmless though - belt and suspenders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go` around lines 317 - 319, The lo.Filter on Patches is a redundant defensive check because diffItem only appends when Changed==true and Patch is non-nil; remove the Filter call so you return patches directly (referencing the Patches variable and the diffItem logic that appends Patch instances) to simplify the return path while keeping behavior identical.
🤖 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 119-137: The switch handling in loader.go treats standardLine and
gatheringLine differently because invoiceLine.AsStandardLine() returns a value
while invoiceLine.AsGatheringLine() returns a pointer; update the code by adding
a brief inline comment near the switch (or directly above the two return lines)
that explains this pointer-semantic asymmetry and why billing.NewLineOrHierarchy
is called with &standardLine but with gatheringLine as-is (or alternatively
normalize by taking an explicit pointer/dereference to make both branches
visually consistent), referencing invoiceLine.AsStandardLine(),
invoiceLine.AsGatheringLine(), and billing.NewLineOrHierarchy so future readers
understand the reason.
In `@openmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.go`:
- Around line 317-319: The lo.Filter on Patches is a redundant defensive check
because diffItem only appends when Changed==true and Patch is non-nil; remove
the Filter call so you return patches directly (referencing the Patches variable
and the diffItem logic that appends Patch instances) to simplify the return path
while keeping behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd8897af-4cfe-451e-9e20-c74502cf38bf
📒 Files selected for processing (10)
openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.goopenmeter/billing/worker/subscriptionsync/service/persistedstate/state.goopenmeter/billing/worker/subscriptionsync/service/reconcile.goopenmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/invoiceupdate.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patch.goopenmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.goopenmeter/billing/worker/subscriptionsync/service/sync.goopenmeter/billing/worker/subscriptionsync/service/targetstate/phaseiterator.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.gopkg/currencyx/currency.go
🚧 Files skipped from review as they are similar to previous changes (3)
- openmeter/billing/worker/subscriptionsync/service/reconcile.go
- openmeter/billing/worker/subscriptionsync/service/targetstate/phaseiterator.go
- openmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.go
turip
force-pushed
the
feat/semantic-patching
branch
from
3395fc9 to
8ce0fc7
Compare
GAlexIHU
approved these changes
Mar 27, 2026
Contributor
GAlexIHU
left a comment
GAlexIHU
left a comment
There was a problem hiding this comment.
pursuent to slack thread
This was referenced 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
Split the existing patching structure in reconciler to two layers:
This separation is needed as charges will need to know the exact patch semantics in order to selectively roll-back ledger transactions.
Details
The main change is that planning now produces explicit reconciliation operations instead of loose create/update/delete buckets that Apply had to reinterpret later. The reconciler now emits one semantic patch per managed item:
Apply was simplified to execute those semantic patches by expanding them into low-level invoice updater patches, instead of rediscovering reconciliation intent. This makes the split clearer:
As part of that, the low-level invoice execution layer was moved into reconciler/invoiceupdater, and patch expansion was reorganized so patch- specific behavior lives with each patch type while shared mechanics stay in helpers. Flat-fee period changes are now explicitly modeled through ProratePatch, while shrink/extend are usage-based only.
Notes for reviewer
Summary by CodeRabbit
Refactor
New Features
Other