refactor: split get*subscription calls#4748
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| standardLines, err := l.billingService.GetStandardLinesForSubscription(ctx, getLinesInput) | ||
| if err != nil { | ||
| return State{}, fmt.Errorf("getting existing standard lines: %w", err) | ||
| } | ||
|
|
||
| gatheringLines, err := l.billingService.GetGatheringLinesForSubscription(ctx, getLinesInput) | ||
| if err != nil { | ||
| return State{}, fmt.Errorf("getting existing lines: %w", err) | ||
| return State{}, fmt.Errorf("getting existing gathering lines: %w", err) | ||
| } | ||
|
|
||
| lines, err = slicesx.MapWithErr(lines, normalizePersistedLineOrHierarchy) | ||
| splitLineGroups, err := l.billingService.GetSplitLineGroupsForSubscription(ctx, getLinesInput) | ||
| if err != nil { | ||
| return State{}, fmt.Errorf("getting existing split line groups: %w", err) | ||
| } |
There was a problem hiding this comment.
LoadForSubscription now issues three independent transactions where the original GetLinesForSubscription ran a single transaction that fetched standard lines, gathering lines, and split-line groups atomically. If a line transitions between statuses (e.g., gathering → standard invoice advancement) or a new split-line group is written between the three reads, the loader could observe a torn snapshot: the same line might appear in both the standard and gathering result sets, or the duplicate-ChildUniqueReferenceID guard could fail spuriously. Because subscription sync is idempotent and typically the sole writer for these entities, this window is small—but the snapshot guarantee is gone. Wrapping the three calls in a single outer transaction (or a read-only snapshot) at the service or loader level would restore the previous atomicity without changing the cleaner per-method API.
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go
Line: 52-65
Comment:
**Three separate DB transactions replace one**
`LoadForSubscription` now issues three independent transactions where the original `GetLinesForSubscription` ran a single transaction that fetched standard lines, gathering lines, and split-line groups atomically. If a line transitions between statuses (e.g., gathering → standard invoice advancement) or a new split-line group is written between the three reads, the loader could observe a torn snapshot: the same line might appear in both the standard and gathering result sets, or the duplicate-`ChildUniqueReferenceID` guard could fail spuriously. Because subscription sync is idempotent and typically the sole writer for these entities, this window is small—but the snapshot guarantee is gone. Wrapping the three calls in a single outer transaction (or a read-only snapshot) at the service or loader level would restore the previous atomicity without changing the cleaner per-method API.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
LineOrHierarchyownership into subscription sync and assemble typed results thereMotivation
This separates the subscription read paths as a prerequisite for moving split-line-group handling into the legacy billing line engine.
Reconstruction note
This draft reconstructs the original commit from #4745. The existing #4745 branch and PR were intentionally left unchanged.
Validation
make test-nocache: 6,156 passed, 9 skipped on the original commitGreptile Summary
This PR splits the single
GetLinesForSubscriptioncall (which returned aLineOrHierarchyunion type) into three typed, per-concern methods:GetStandardLinesForSubscription,GetGatheringLinesForSubscription, andGetSplitLineGroupsForSubscription. TheLineOrHierarchywrapper type and its helpers (NewItemFromLineOrHierarchy,GetDeletePatchesForLine) are removed, with assembly of the combined result moved intopersistedstate.Loader.ChildUniqueReferenceIDsanity check that previously lived in the adapter.server_test.goare updated to match the new interface split.Confidence Score: 4/5
Safe to merge; the refactor is well-scoped, all removed symbols have no remaining callers, and the 6k+ test suite passes. The one thing worth a second look is the loader making three sequential DB calls with independent transactions where a single transaction previously gave a consistent snapshot.
The split is mechanically correct — filter predicates on the three new adapter methods are complementary (StatusNEQ Gathering / StatusEQ Gathering / split-line-group table), removed types have zero remaining references, and the duplicate-ID sanity check is preserved in the loader. The behavioral change is that LoadForSubscription now reads across three separate transactions; a concurrent status transition between reads could produce a torn view and trigger the duplicate-ID guard or silently drop a line for one sync cycle. Given that subscription sync is designed to be idempotent this is unlikely to cause lasting harm, but the transactional guarantee that existed before is no longer present.
openmeter/billing/worker/subscriptionsync/service/persistedstate/loader.go — three sequential independent transactions replacing one atomic read.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Loader as persistedstate.Loader participant Svc as billing.Service participant AdapterStd as adapter (stdinvoicelines) participant AdapterGath as adapter (gatheringlines) participant AdapterSplit as adapter (invoicelinesplitgroup) participant DB as Database Note over Loader,DB: Before (single transaction) Loader->>Svc: GetLinesForSubscription(input) Svc->>AdapterStd: GetLinesForSubscription(input) AdapterStd->>DB: BEGIN TX AdapterStd->>DB: BillingInvoiceLine (std+gathering) + SplitLineGroup DB-->>AdapterStd: LineOrHierarchy[] AdapterStd->>DB: COMMIT AdapterStd-->>Svc: []LineOrHierarchy Svc-->>Loader: []LineOrHierarchy Note over Loader,DB: After (three separate transactions) Loader->>Svc: GetStandardLinesForSubscription(input) Svc->>AdapterStd: GetStandardLinesForSubscription(input) AdapterStd->>DB: BEGIN TX (StatusNEQ Gathering) DB-->>AdapterStd: StandardLines AdapterStd->>DB: COMMIT AdapterStd-->>Svc: StandardLines Svc-->>Loader: StandardLines Loader->>Svc: GetGatheringLinesForSubscription(input) Svc->>AdapterGath: GetGatheringLinesForSubscription(input) AdapterGath->>DB: BEGIN TX (StatusEQ Gathering) DB-->>AdapterGath: GatheringLines AdapterGath->>DB: COMMIT AdapterGath-->>Svc: GatheringLines Svc-->>Loader: GatheringLines Loader->>Svc: GetSplitLineGroupsForSubscription(input) Svc->>AdapterSplit: GetSplitLineGroupsForSubscription(input) AdapterSplit->>DB: BEGIN TX (SplitLineGroup) DB-->>AdapterSplit: []SplitLineHierarchy AdapterSplit->>DB: COMMIT AdapterSplit-->>Svc: []SplitLineHierarchy Svc-->>Loader: []SplitLineHierarchy Loader->>Loader: concat + normalize + duplicate-ID check Loader->>Loader: "assemble State{byUniqueID, invoices, charges}"%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Loader as persistedstate.Loader participant Svc as billing.Service participant AdapterStd as adapter (stdinvoicelines) participant AdapterGath as adapter (gatheringlines) participant AdapterSplit as adapter (invoicelinesplitgroup) participant DB as Database Note over Loader,DB: Before (single transaction) Loader->>Svc: GetLinesForSubscription(input) Svc->>AdapterStd: GetLinesForSubscription(input) AdapterStd->>DB: BEGIN TX AdapterStd->>DB: BillingInvoiceLine (std+gathering) + SplitLineGroup DB-->>AdapterStd: LineOrHierarchy[] AdapterStd->>DB: COMMIT AdapterStd-->>Svc: []LineOrHierarchy Svc-->>Loader: []LineOrHierarchy Note over Loader,DB: After (three separate transactions) Loader->>Svc: GetStandardLinesForSubscription(input) Svc->>AdapterStd: GetStandardLinesForSubscription(input) AdapterStd->>DB: BEGIN TX (StatusNEQ Gathering) DB-->>AdapterStd: StandardLines AdapterStd->>DB: COMMIT AdapterStd-->>Svc: StandardLines Svc-->>Loader: StandardLines Loader->>Svc: GetGatheringLinesForSubscription(input) Svc->>AdapterGath: GetGatheringLinesForSubscription(input) AdapterGath->>DB: BEGIN TX (StatusEQ Gathering) DB-->>AdapterGath: GatheringLines AdapterGath->>DB: COMMIT AdapterGath-->>Svc: GatheringLines Svc-->>Loader: GatheringLines Loader->>Svc: GetSplitLineGroupsForSubscription(input) Svc->>AdapterSplit: GetSplitLineGroupsForSubscription(input) AdapterSplit->>DB: BEGIN TX (SplitLineGroup) DB-->>AdapterSplit: []SplitLineHierarchy AdapterSplit->>DB: COMMIT AdapterSplit-->>Svc: []SplitLineHierarchy Svc-->>Loader: []SplitLineHierarchy Loader->>Loader: concat + normalize + duplicate-ID check Loader->>Loader: "assemble State{byUniqueID, invoices, charges}"Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "refactor: split get*subscription calls" | Re-trigger Greptile