fix(entitlement): invalidate snapshots after reset - #4845
Conversation
📝 WalkthroughWalkthrough
ChangesReset snapshot consistency
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/credit/balance.go (1)
319-321: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the backdated-reset ordering.
Add a short comment before Line 319. This call invalidates later snapshots so the replacement snapshot saved at Line 324 becomes the latest valid state. The ordering and stale-snapshot consequence are not obvious from
InvalidateAfteralone.Suggested comment
+ // Invalidate later snapshots before saving the reset snapshot so a backdated reset + // cannot leave stale balance data as the latest valid snapshot. if err := m.BalanceSnapshotService.InvalidateAfter(ctx, ownerID, at); err != nil {As per coding guidelines, comments should explain non-obvious intent, domain constraints, lifecycle state, and failure consequences.
🤖 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/credit/balance.go` around lines 319 - 321, Add a concise comment immediately before the BalanceSnapshotService.InvalidateAfter call explaining that later snapshots are invalidated before saving the replacement snapshot, ensuring the replacement becomes the latest valid state and preventing stale backdated snapshots from remaining active.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openmeter/credit/balance.go`:
- Around line 319-321: Add a concise comment immediately before the
BalanceSnapshotService.InvalidateAfter call explaining that later snapshots are
invalidated before saving the replacement snapshot, ensuring the replacement
becomes the latest valid state and preventing stale backdated snapshots from
remaining active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0517c35c-c995-4c7f-ae58-fd471732dcf8
📒 Files selected for processing (2)
openmeter/credit/balance.goopenmeter/entitlement/metered/reset_test.go
Overview
Backdated entitlement resets can land before an already-persisted balance snapshot. Because that snapshot represents derived state from before the reset, reusing it would skip the reset and return a stale balance.
This change invalidates snapshots after the reset's effective time before saving the replacement reset snapshot. The reset test now verifies that subsequent balance calculations resume from the reset snapshot instead of a stale later one.
Notes for reviewer
Validation
nix develop --impure .#ci -c env POSTGRES_HOST=127.0.0.1 go test -tags=dynamic ./openmeter/entitlement/metered -run 'TestResetEntitlementUsage/Should_invalidate_snapshots_after_the_reset_time$' -count=1nix develop --impure .#ci -c env POSTGRES_HOST=127.0.0.1 go test -tags=dynamic ./openmeter/entitlement/metered -run '^TestResetEntitlementUsage$' -count=1nix develop --impure .#ci -c env POSTGRES_HOST=127.0.0.1 go test -tags=dynamic ./openmeter/entitlement/metered -count=1Summary by CodeRabbit