fix: sync deleted subscriptions at least once#4382
Merged
Conversation
As if a subscription was changed, then the subscription change gets canceled the subscription already yields the upcoming line and entitlements that should be deleted after the cancel.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces exported SynchronizeSubscription* APIs with new SyncBy* entrypoints (view & ID, invoice-customer variants), adds subscription-reference abstraction, handles deleted subscriptions via nil view and IncludeDeleted listing (two-phase reconcile), reshapes plan/apply contracts, migrates callers/tests, and adds a migration to reset sync state. ChangesSubscription Sync Refactoring for Deleted Subscription Support
Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant SubscriptionSyncService
participant BillingService
participant PersistedState
Reconciler->>SubscriptionSyncService: SyncByID(ctx, subscriptionID, asOf)
SubscriptionSyncService->>BillingService: WithLock(ctx, customerID, fn)
BillingService->>SubscriptionSyncService: locked result
SubscriptionSyncService->>PersistedState: UpdateSyncState(subscriptionID, nextSyncAfter, hasBillables)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
openmeter/billing/worker/subscriptionsync/service/sync_test.go (1)
3714-3733:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the edited view for the second sync
Nice catch adding the post-edit sync path here, but this call still syncs
subsView(pre-edit) instead ofupdatedSubsView. That makes this assertion block vulnerable to passing on stale data.Suggested fix
- s.NoError(s.Service.SyncByView(ctx, subsView, s.mustParseTime("2024-01-05T12:00:00Z"))) + s.NoError(s.Service.SyncByView(ctx, updatedSubsView, s.mustParseTime("2024-01-05T12:00:00Z")))🤖 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_test.go` around lines 3714 - 3733, The test currently calls Service.SyncByView with the pre-edit subsView instead of the edited updatedSubsView returned by SubscriptionWorkflowService.EditRunning; update the SyncByView invocation to pass updatedSubsView (the result of SubscriptionWorkflowService.EditRunning) and its timestamp so the post-edit state is what gets synchronized and asserted against.test/subscription/scenario_firstofmonth_test.go (1)
408-417:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSync after cancel should use a fresh subscription view
After canceling, this still syncs the earlier
view. With the new view-based API, that can mask cancellation-sync bugs because the call may run on pre-cancel state.Suggested fix
_, err = tDeps.subscriptionService.Cancel(ctx, s.NamespacedID, subscription.Timing{Custom: &cancelAt}) require.NoError(t, err) + + view, err = tDeps.SubscriptionService.GetView(ctx, s.NamespacedID) + require.NoError(t, err) // Let's advance time until after clock.SetTime(cancelAt.Add(time.Hour * 1)) // Sync up to the next anchor (July 1st). Lines should remain on gathering invoice until then. require.NoError(t, tDeps.subscriptionSyncService.SyncByViewAndInvoiceCustomer(ctx, view, firstOfNextMonth))🤖 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 `@test/subscription/scenario_firstofmonth_test.go` around lines 408 - 417, After calling subscriptionService.Cancel on s.NamespacedID you must fetch a fresh subscription view and use that for the sync instead of reusing the old `view`; update the test so after clock.SetTime(...) you call the service that returns the current subscription view (e.g., use the project's subscription view getter — the one that returns a view for s.NamespacedID, referenced in the codebase as the subscription view retrieval method) and assign it to `view` before calling subscriptionSyncService.SyncByViewAndInvoiceCustomer(ctx, view, firstOfNextMonth) so the sync runs against post-cancel state.openmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.go (1)
35-52: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPlease keep the repo’s
Validate()error shape here.This method now adds another conditional validation branch, but it still returns raw
errors.Join(errs...). In this package,Validate() errorhelpers are expected to wrap the join withmodels.NewNillableGenericValidationError(...)so upstream validation handling stays consistent. As per coding guidelines, "ForValidate() errormethods, collect all validation issues intovar errs []errorand returnmodels.NewNillableGenericValidationError(errors.Join(errs...))instead of returning on the first invalid field".🤖 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/targetstate/targetstate.go` around lines 35 - 52, The BuildInput.Validate method currently collects validation errors into errs but returns errors.Join(errs...) directly; change the return to wrap the joined error with models.NewNillableGenericValidationError so it follows the repo's Validate() error shape. Keep the existing checks (AsOf, SubscriptionView.Validate, Persisted.Validate) and the errs collection logic, then return models.NewNillableGenericValidationError(errors.Join(errs...)) from BuildInput.Validate to preserve consistent upstream handling.openmeter/billing/worker/subscriptionsync/service/reconcile.go (1)
16-23:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winThis deleted-subscription path still dereferences
subsView.
buildSyncPlan()now explicitly allowssubsView == nil, butpersistedLoader.LoadForSubscription(ctx, subsView.Subscription)still touches it unconditionally. On the first deleted sync this will panic before reconciliation gets a chance to delete persisted lines. The loader needs the base subscription identity passed separately from the optional view.🤖 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/reconcile.go` around lines 16 - 23, The buildSyncPlan function allows subsView to be nil but still dereferences subsView when calling persistedLoader.LoadForSubscription, which can panic; change the call site and persistedLoader usage so LoadForSubscription receives the subscription identity separately (e.g. pass subsView.Subscription or a required subscription ID/struct extracted before the nil-check) and add a nil guard in buildSyncPlan that uses the extracted subscription identity when subsView is nil; update the function call sites and the persistedstate.NewLoader/LoadForSubscription invocation to accept the standalone subscription identifier and ensure no dereference of subsView occurs before the nil check.
🧹 Nitpick comments (2)
openmeter/subscription/service/service_test.go (1)
678-695: ⚡ Quick winAdd a direct
DeletedAtfilter assertion in this new scenario.Nice coverage for
IncludeDeleted; it’d be great to also assert the newDeletedAtfilter path here (e.g., include deleted + timestamp window includessub4, and an earlier cutoff excludes it) so the query contract is fully pinned.As per coding guidelines
**/*_test.go: “Make sure the tests are comprehensive and cover the changes.”🤖 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/subscription/service/service_test.go` around lines 678 - 695, Add assertions that explicitly exercise the DeletedAt filter on service.List: after confirming includeDeletedScheduledList.Items[0].DeletedAt is non-nil, capture that timestamp (from includeDeletedScheduledList.Items[0].DeletedAt or sub4.DeletedAt) and call service.List with subscription.ListSubscriptionsInput{IncludeDeleted:true, Status:[]subscription.SubscriptionStatus{subscription.SubscriptionStatusScheduled}, DeletedAfter: deletedAt.Add(-time.Second)} to assert sub4 is returned, and then call service.List with DeletedBefore: deletedAt.Add(-time.Second) (or DeletedAfter/DeletedBefore equivalents used by the List API) to assert sub4 is excluded; use the same symbols service.List, subscription.ListSubscriptionsInput, IncludeDeleted, DeletedAfter/DeletedBefore (or the actual DeletedAt range field names in the code) and sub4/DeletedAt for locating where to add these checks.openmeter/billing/worker/worker.go (1)
195-195: ⚡ Quick winPlease drop or rewrite this TODO.
It’s stale now that deleted-event handling was added just above, so it’ll only send the next reader in the wrong direction. As per coding guidelines, "When refactoring or reverting code, preserve existing explanatory comments by default. Remove or rewrite a comment only when the code change makes it false, stale, or misleading".
🤖 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/worker.go` at line 195, The inline TODO "// TODO: let's add deleted event handling!!!~!" is now stale because deleted-event handling was implemented above; remove this TODO or replace it with a short, accurate comment stating that deleted-event handling is implemented (and referencing the existing handler), e.g., "Deleted-event handling implemented above; keep reader ordering intact" so future readers aren't misled by the obsolete task. Locate the exact TODO string in worker.go and either delete it or rewrite it to reflect the current implementation.
🤖 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 `@openmeter/billing/worker/subscriptionsync/reconciler/reconciler.go`:
- Around line 140-153: The deleted-subscription reconciliation loop is reusing
the previous pageIndex from the active-subscription loop, causing the deleted
pass to start at the wrong page; before calling r.subscriptionService.List for
the deleted-subscription pass (the call that sets DeletedAt and IncludeDeleted
on ListSubscriptionsInput and uses pagination.Page.PageNumber: pageIndex), reset
pageIndex back to the first page (e.g., set pageIndex = 1 or the same initial
value used for active pages) so the deleted-subscription pagination begins at
page one and reconciles all deleted pages.
In `@openmeter/billing/worker/subscriptionsync/service/ref.go`:
- Around line 59-75: The Validate method on subscriptionReferenceOrView
currently only checks for nil id in SubscriptionReferenceTypeID and returns
single errors; update subscriptionReferenceOrView.Validate() to collect
validation issues into a slice (e.g., var errs []error), validate ID-mode more
strictly by validating the actual id fields (namespace and id format) rather
than just non-nil—append any field-specific errors to errs; for
SubscriptionReferenceTypeView call r.view.Validate(true) and append its error
(if any) to errs; for invalid types append an explicit error about the type;
finally return models.NewNillableGenericValidationError(errors.Join(errs...))
(or nil if errs is empty) so the method follows the repository’s
aggregated-validation contract.
In `@openmeter/billing/worker/subscriptionsync/service/sync.go`:
- Around line 107-121: The code currently prefers a caller-provided view
(refOrView.AsSubscriptionView()) even when the authoritative subscription state
(subs) reports deleted; change the logic so that if subs.IsDeleted() is true you
always set subsView = nil (do not use refOrView.AsSubscriptionView()), otherwise
proceed to set subsView either from refOrView.AsSubscriptionView() when
refOrView.Type() == SubscriptionReferenceTypeView or from
s.subscriptionService.GetView(ctx, subscriptionID) when not deleted; ensure
references to SubscriptionReferenceTypeView, refOrView.AsSubscriptionView(),
subs.IsDeleted(), s.subscriptionService.GetView, and the subsView variable are
updated accordingly.
---
Outside diff comments:
In `@openmeter/billing/worker/subscriptionsync/service/reconcile.go`:
- Around line 16-23: The buildSyncPlan function allows subsView to be nil but
still dereferences subsView when calling persistedLoader.LoadForSubscription,
which can panic; change the call site and persistedLoader usage so
LoadForSubscription receives the subscription identity separately (e.g. pass
subsView.Subscription or a required subscription ID/struct extracted before the
nil-check) and add a nil guard in buildSyncPlan that uses the extracted
subscription identity when subsView is nil; update the function call sites and
the persistedstate.NewLoader/LoadForSubscription invocation to accept the
standalone subscription identifier and ensure no dereference of subsView occurs
before the nil check.
In `@openmeter/billing/worker/subscriptionsync/service/sync_test.go`:
- Around line 3714-3733: The test currently calls Service.SyncByView with the
pre-edit subsView instead of the edited updatedSubsView returned by
SubscriptionWorkflowService.EditRunning; update the SyncByView invocation to
pass updatedSubsView (the result of SubscriptionWorkflowService.EditRunning) and
its timestamp so the post-edit state is what gets synchronized and asserted
against.
In
`@openmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.go`:
- Around line 35-52: The BuildInput.Validate method currently collects
validation errors into errs but returns errors.Join(errs...) directly; change
the return to wrap the joined error with
models.NewNillableGenericValidationError so it follows the repo's Validate()
error shape. Keep the existing checks (AsOf, SubscriptionView.Validate,
Persisted.Validate) and the errs collection logic, then return
models.NewNillableGenericValidationError(errors.Join(errs...)) from
BuildInput.Validate to preserve consistent upstream handling.
In `@test/subscription/scenario_firstofmonth_test.go`:
- Around line 408-417: After calling subscriptionService.Cancel on
s.NamespacedID you must fetch a fresh subscription view and use that for the
sync instead of reusing the old `view`; update the test so after
clock.SetTime(...) you call the service that returns the current subscription
view (e.g., use the project's subscription view getter — the one that returns a
view for s.NamespacedID, referenced in the codebase as the subscription view
retrieval method) and assign it to `view` before calling
subscriptionSyncService.SyncByViewAndInvoiceCustomer(ctx, view,
firstOfNextMonth) so the sync runs against post-cancel state.
---
Nitpick comments:
In `@openmeter/billing/worker/worker.go`:
- Line 195: The inline TODO "// TODO: let's add deleted event handling!!!~!" is
now stale because deleted-event handling was implemented above; remove this TODO
or replace it with a short, accurate comment stating that deleted-event handling
is implemented (and referencing the existing handler), e.g., "Deleted-event
handling implemented above; keep reader ordering intact" so future readers
aren't misled by the obsolete task. Locate the exact TODO string in worker.go
and either delete it or rewrite it to reflect the current implementation.
In `@openmeter/subscription/service/service_test.go`:
- Around line 678-695: Add assertions that explicitly exercise the DeletedAt
filter on service.List: after confirming
includeDeletedScheduledList.Items[0].DeletedAt is non-nil, capture that
timestamp (from includeDeletedScheduledList.Items[0].DeletedAt or
sub4.DeletedAt) and call service.List with
subscription.ListSubscriptionsInput{IncludeDeleted:true,
Status:[]subscription.SubscriptionStatus{subscription.SubscriptionStatusScheduled},
DeletedAfter: deletedAt.Add(-time.Second)} to assert sub4 is returned, and then
call service.List with DeletedBefore: deletedAt.Add(-time.Second) (or
DeletedAfter/DeletedBefore equivalents used by the List API) to assert sub4 is
excluded; use the same symbols service.List,
subscription.ListSubscriptionsInput, IncludeDeleted, DeletedAfter/DeletedBefore
(or the actual DeletedAt range field names in the code) and sub4/DeletedAt for
locating where to add these checks.
🪄 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: d4aedc81-6bf5-4123-99a5-9fb2c011de29
⛔ Files ignored due to path filters (2)
migrations/atlas.sumis excluded by!**/*.sum,!**/*.sumtools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (25)
.agents/skills/subscriptionsync/SKILL.mdopenmeter/billing/validators/customer/customer.goopenmeter/billing/worker/subscriptionsync/reconciler/reconciler.goopenmeter/billing/worker/subscriptionsync/service.goopenmeter/billing/worker/subscriptionsync/service/creditsonly_test.goopenmeter/billing/worker/subscriptionsync/service/handlers.goopenmeter/billing/worker/subscriptionsync/service/reconcile.goopenmeter/billing/worker/subscriptionsync/service/reconciler/reconciler.goopenmeter/billing/worker/subscriptionsync/service/ref.goopenmeter/billing/worker/subscriptionsync/service/service.goopenmeter/billing/worker/subscriptionsync/service/sync.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/billing/worker/subscriptionsync/service/sync_test.goopenmeter/billing/worker/subscriptionsync/service/syncbillinganchor_test.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstate.goopenmeter/billing/worker/worker.goopenmeter/subscription/events.goopenmeter/subscription/list.goopenmeter/subscription/repo/subscriptionrepo.goopenmeter/subscription/service/service_test.goopenmeter/subscription/service/sync_test.gotest/billing/subscription_test.gotest/subscription/scenario_firstofmonth_test.gotools/migrate/migrations/20260519132345_reset-sync-state.down.sqltools/migrate/migrations/20260519132345_reset-sync-state.up.sql
turip
added
release-note/bug-fix
tothandras
previously approved these changes
May 19, 2026
tothandras
approved these changes
May 19, 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
As if a subscription was changed, then the subscription change gets canceled the subscription already yields the upcoming line and entitlements that should be deleted after the cancel.
This patch makes sure that:
Notes for reviewer
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests
Chores