Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR refactors usage-based realization runs to replace offset-based timestamps with an explicit stored-at-lower-bound model. Core fields Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Rating as Rating Service
participant DB as RealizationRun Persistence
participant SM as State Machine
participant Credit as Credit Allocation
Note over Client,Rating: OLD: offset-based snapshot
Client->>Rating: GetDetailedLines(StoredAtOffset)
Rating->>DB: snapshotQuantity using StoredAtOffset
Client->>DB: Create(AsOf, CollectionEnd, MeterValue)
DB-->>Client: persisted run
Client->>SM: AdvanceAfterCollectionPeriodEnd()
SM->>SM: compute snapshot-after from CollectionEnd + interval
SM->>Credit: allocate using CollectionEnd-derived time
Note over Client,Rating: NEW: stored-at-lower-bound
Client->>Rating: GetDetailedLines(StoredAtLT, ServicePeriodTo)
Rating->>DB: snapshotQuantity using StoredAtLT
Client->>DB: Create(StoredAtLT, ServicePeriodTo, MeteredQuantity)
DB-->>Client: persisted run
Client->>SM: AdvanceAfterCollectionPeriodEnd()
SM->>SM: compute snapshot-after from StoredAtLT + interval
SM->>Credit: allocate using StoredAtLT-derived time
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/charges/service/invoicable_test.go (1)
467-636:⚠️ Potential issue | 🔴 CriticalThese assertions contradict each other — one of them can't pass.
Inside the
#3.1 start final realizationblock,currentRun.StoredAtLTis asserted against two different instants within three lines:
- Line 633: must equal
firstStoredAtLT=2026-02-01T11:59:00Z- Line 635: must equal
expectedCollectionEnd=2026-02-03T00:00:00ZThose are two days apart, so whichever one the run actually has, the other assertion will fail. Looking at the runtime,
getFinalRunStoredAtLT()(statemachine.go) computesServicePeriod.To + collectionInterval(=expectedCollectionEnd), so line 633 seems to be a stale “offset-from-now” assertion that survived the rename.The downstream expectation also looks wrong with the current code:
- Line 636 expects
MeteredQuantity == 3, i.e. only events 1 and 2 are rated. That only happens if the cutoff is< 2026-02-01T12:00:00Z(thestoredAtof event 3). WithStoredAtLT = 2026-02-03T00:00:00ZfromgetFinalRunStoredAtLT(), all three events qualify and the sum would be6.- Line 647 asserts the
startedCallbacks[0].Input.AllocateAt == firstStoredAtLT, butCreateRatedRunsetsAllocateAt = in.ServicePeriodTo=NormalizeTimestamp(ServicePeriod.To)=2026-02-01T00:00:00Z, which is neither of the declared expectations.Either the test is reflecting the intended new semantic (compute
storedAtLTasclock.Now() - InternalCollectionPeriod) and the code hasn't caught up ingetFinalRunStoredAtLT/StartFinalRealizationRun, or the code is the source of truth and the test fixtures need to drop the oldfirstStoredAtOffset-style expectations. Either way, worth reconciling before this goes in — the same pattern repeats inTestUsageBasedCreditOnlyLifecycleVolumeTieredCorrectionat lines 784-785, 874, 913-914.Also applies to: 643-648
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/invoicable_test.go` around lines 467 - 636, The assertions in the test use two contradictory expectations for StoredAtLT and resulting metered totals; reconcile the test with the actual implementation: update the assertions in invoicable_test.go (and the other locations called out) to expect getFinalRunStoredAtLT()/StartFinalRealizationRun behavior (StoredAtLT == expectedCollectionEnd, which will include all three events so MeteredQuantity == 6) and assert AllocateAt equals the service period end normalized (NormalizeTimestamp(ServicePeriod.To)), not firstStoredAtLT; also remove the stale firstStoredAtLT equality checks and update the similar assertions in TestUsageBasedCreditOnlyLifecycleVolumeTieredCorrection to match the new semantics.
🧹 Nitpick comments (5)
openmeter/billing/charges/service/truncation_test.go (1)
167-171: Nit: duplicated assertion onfinalRun.StoredAtLT.
expectedCollectionEndandexpectedStoredAtLTare parsed from the exact same timestamp and both are asserted againstfinalRun.StoredAtLT, so the second line is a redundant check. Either drop one, or give them distinct semantics (e.g., keep theexpectedCollectionEndvariable only if you intend to later also assert against a separateServicePeriodTo-style field on the run).♻️ Suggested tidy-up
finalRun := finalCharge.Realizations[0] - expectedCollectionEnd := datetime.MustParseTimeInLocation(s.T(), "2026-02-01T00:01:00Z", time.UTC).AsTime() - expectedStoredAtLT := datetime.MustParseTimeInLocation(s.T(), "2026-02-01T00:01:00Z", time.UTC).AsTime() + expectedStoredAtLT := datetime.MustParseTimeInLocation(s.T(), "2026-02-01T00:01:00Z", time.UTC).AsTime() - s.True(expectedCollectionEnd.Equal(finalRun.StoredAtLT)) s.True(expectedStoredAtLT.Equal(finalRun.StoredAtLT))Also worth asking: should this test additionally assert
finalRun.ServicePeriodTonow that it's a first-class field on the run? That'd make the "truncation of persisted calculation timestamps" contract tighter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/truncation_test.go` around lines 167 - 171, The test contains a duplicated assertion: both expectedCollectionEnd and expectedStoredAtLT are parsed identically and both asserted against finalRun.StoredAtLT; remove the redundant assertion (drop one of the s.True(...) lines) or give them distinct semantics by asserting expectedCollectionEnd against finalRun.ServicePeriodTo and expectedStoredAtLT against finalRun.StoredAtLT so each variable verifies a different field (use finalRun.StoredAtLT and finalRun.ServicePeriodTo as the unique symbols to update).openmeter/billing/charges/usagebased/service/rating/totals.go (1)
41-42: Tiny doc-comment leftover from the old naming.The method comment still says "requested stored-at offset," but the input is now a concrete timestamp (
StoredAtLT), not an offset. Worth a quick tweak so the doc matches the field:📝 Proposed wording fix
-// GetTotalsForUsage returns the rated totals for the charge at the requested stored-at offset. +// GetTotalsForUsage returns the rated totals for the charge at the requested stored-at-lt cutoff. // It avoids generating detailed lines, so prefer it over GetDetailedLinesForUsage when only totals are needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/rating/totals.go` around lines 41 - 42, Update the doc comment for GetTotalsForUsage to reflect that it accepts a concrete timestamp rather than an offset: replace the phrase "requested stored-at offset" with wording that references the StoredAtLT timestamp (e.g., "requested stored-at timestamp (StoredAtLT)") so the comment matches the method signature and the StoredAtLT field.openmeter/billing/charges/usagebased/service/creditsonly.go (1)
185-185: Re-normalizing an already-normalized timestamp.
currentRun.StoredAtLTwas written viameta.NormalizeTimestamp(...)(either here inStartFinalRealizationRunline 159 or inrun/create.go), andNormalizeTimestampis idempotent, so this extra call is a no-op. Dropping it makes the intent a bit clearer:✨ Optional cleanup
- storedAtLT := meta.NormalizeTimestamp(currentRun.StoredAtLT) + storedAtLT := currentRun.StoredAtLTNot blocking — just tidying.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/creditsonly.go` at line 185, The line re-calls meta.NormalizeTimestamp on currentRun.StoredAtLT which is already normalized (done in StartFinalRealizationRun or run/create.go), so remove the redundant call: replace the assignment to storedAtLT in creditsonly.go to directly use currentRun.StoredAtLT (or otherwise stop calling meta.NormalizeTimestamp there) to clarify intent and avoid a no-op; update any nearby comments to reflect that currentRun.StoredAtLT is already normalized.openmeter/billing/charges/usagebased/realizationrun.go (2)
76-90: Tiny nit: group the temporal validations together.Totally optional polish — since
StoredAtLTandServicePeriodToare conceptually a pair (both timestamps, both required, same zero-check), reading the validator would flow a bit better if their checks sat next to each other instead of being separated by theMeteredQuantityandTotalschecks. Same applies toRealizationRunBase.Validate()at lines 194–208. Happy to leave as-is if you prefer the current ordering.✨ Suggested tidy-up
if r.StoredAtLT.IsZero() { errs = append(errs, fmt.Errorf("stored at lt must be set")) } + if r.ServicePeriodTo.IsZero() { + errs = append(errs, fmt.Errorf("service period to must be set")) + } + if r.MeteredQuantity.IsNegative() { errs = append(errs, fmt.Errorf("metered quantity must be zero or positive")) } if err := r.Totals.Validate(); err != nil { errs = append(errs, fmt.Errorf("totals: %w", err)) } - - if r.ServicePeriodTo.IsZero() { - errs = append(errs, fmt.Errorf("service period to must be set")) - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/realizationrun.go` around lines 76 - 90, Move the timestamp zero-checks so they are adjacent: in RealizationRun.Validate(), place the ServicePeriodTo.IsZero() check immediately after the StoredAtLT.IsZero() check (so both temporal validations are grouped together) and keep MeteredQuantity.IsNegative() and Totals.Validate() following them; do the same tidy-up in RealizationRunBase.Validate() by grouping its StoredAtLT and ServicePeriodTo zero-checks next to each other to improve logical ordering and readability while leaving the same error messages and behavior intact.
99-115: Consider adding a doc comment toUpdateRealizationRunInputclarifying thatServicePeriodTois immutable.The design looks solid —
ServicePeriodTois correctly set-once at creation viaCreateRealizationRunInputand intentionally absent from updates. No caller tries to mutate it. That said, a one-liner doc comment on the struct explaining "service period is fixed at creation and cannot be updated" would help future maintainers understand the constraint at a glance, since it's not immediately obvious why the field was omitted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/realizationrun.go` around lines 99 - 115, Add a one-line doc comment to the UpdateRealizationRunInput struct explaining that the service period end (ServicePeriodTo) is intentionally immutable and set during creation; specifically update the comment above type UpdateRealizationRunInput to state that ServicePeriodTo is fixed at creation and cannot be modified via UpdateRealizationRunInput so future maintainers understand why that field is omitted.
🤖 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/charges/service/invoicable_test.go`:
- Line 634: Change the assertions that use s.NotNil on the value-typed timestamp
RealizationRunBase.StoredAtLT to check for non-zero time instead: replace
s.NotNil(currentRun.StoredAtLT) and s.NotNil(finalRun.StoredAtLT) (and the same
occurrences at the other reported locations) with assertions that call .IsZero()
and assert it is false (e.g., s.False(currentRun.StoredAtLT.IsZero())) so the
test actually verifies the timestamp is present.
In
`@tools/migrate/migrations/20260424091352_usagebased_run_data_structure.down.sql`:
- Around line 1-7: Add a one-line comment at the top of the down migration
explaining the intentional semantic shift performed in the up migration: note
that the up migration preserved the query cutoff by copying collection_end into
stored_at_lt (and thereby overwrote asof), so when rolling back the down
migration the restored asof values come from stored_at_lt and therefore
represent collection_end timestamps rather than original creation times;
reference the table "charge_usage_based_runs" and the columns "asof",
"stored_at_lt", and "collection_end" in the comment.
---
Outside diff comments:
In `@openmeter/billing/charges/service/invoicable_test.go`:
- Around line 467-636: The assertions in the test use two contradictory
expectations for StoredAtLT and resulting metered totals; reconcile the test
with the actual implementation: update the assertions in invoicable_test.go (and
the other locations called out) to expect
getFinalRunStoredAtLT()/StartFinalRealizationRun behavior (StoredAtLT ==
expectedCollectionEnd, which will include all three events so MeteredQuantity ==
6) and assert AllocateAt equals the service period end normalized
(NormalizeTimestamp(ServicePeriod.To)), not firstStoredAtLT; also remove the
stale firstStoredAtLT equality checks and update the similar assertions in
TestUsageBasedCreditOnlyLifecycleVolumeTieredCorrection to match the new
semantics.
---
Nitpick comments:
In `@openmeter/billing/charges/service/truncation_test.go`:
- Around line 167-171: The test contains a duplicated assertion: both
expectedCollectionEnd and expectedStoredAtLT are parsed identically and both
asserted against finalRun.StoredAtLT; remove the redundant assertion (drop one
of the s.True(...) lines) or give them distinct semantics by asserting
expectedCollectionEnd against finalRun.ServicePeriodTo and expectedStoredAtLT
against finalRun.StoredAtLT so each variable verifies a different field (use
finalRun.StoredAtLT and finalRun.ServicePeriodTo as the unique symbols to
update).
In `@openmeter/billing/charges/usagebased/realizationrun.go`:
- Around line 76-90: Move the timestamp zero-checks so they are adjacent: in
RealizationRun.Validate(), place the ServicePeriodTo.IsZero() check immediately
after the StoredAtLT.IsZero() check (so both temporal validations are grouped
together) and keep MeteredQuantity.IsNegative() and Totals.Validate() following
them; do the same tidy-up in RealizationRunBase.Validate() by grouping its
StoredAtLT and ServicePeriodTo zero-checks next to each other to improve logical
ordering and readability while leaving the same error messages and behavior
intact.
- Around line 99-115: Add a one-line doc comment to the
UpdateRealizationRunInput struct explaining that the service period end
(ServicePeriodTo) is intentionally immutable and set during creation;
specifically update the comment above type UpdateRealizationRunInput to state
that ServicePeriodTo is fixed at creation and cannot be modified via
UpdateRealizationRunInput so future maintainers understand why that field is
omitted.
In `@openmeter/billing/charges/usagebased/service/creditsonly.go`:
- Line 185: The line re-calls meta.NormalizeTimestamp on currentRun.StoredAtLT
which is already normalized (done in StartFinalRealizationRun or run/create.go),
so remove the redundant call: replace the assignment to storedAtLT in
creditsonly.go to directly use currentRun.StoredAtLT (or otherwise stop calling
meta.NormalizeTimestamp there) to clarify intent and avoid a no-op; update any
nearby comments to reflect that currentRun.StoredAtLT is already normalized.
In `@openmeter/billing/charges/usagebased/service/rating/totals.go`:
- Around line 41-42: Update the doc comment for GetTotalsForUsage to reflect
that it accepts a concrete timestamp rather than an offset: replace the phrase
"requested stored-at offset" with wording that references the StoredAtLT
timestamp (e.g., "requested stored-at timestamp (StoredAtLT)") so the comment
matches the method signature and the StoredAtLT field.
🪄 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: d551c001-5d4a-494b-a124-f669ef96e08f
⛔ Files ignored due to path filters (8)
openmeter/ent/db/chargeusagebasedruns.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns/chargeusagebasedruns.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruns_update.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (25)
.agents/skills/ent/SKILL.mdopenmeter/billing/charges/service/featureid_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/truncation_test.goopenmeter/billing/charges/service/usagebased_test.goopenmeter/billing/charges/usagebased/adapter/detailedline_test.goopenmeter/billing/charges/usagebased/adapter/mapper.goopenmeter/billing/charges/usagebased/adapter/realizationrun.goopenmeter/billing/charges/usagebased/realizationrun.goopenmeter/billing/charges/usagebased/service/creditheninvoice.goopenmeter/billing/charges/usagebased/service/creditheninvoice_test.goopenmeter/billing/charges/usagebased/service/creditsonly.goopenmeter/billing/charges/usagebased/service/currenttotals.goopenmeter/billing/charges/usagebased/service/get.goopenmeter/billing/charges/usagebased/service/lineengine.goopenmeter/billing/charges/usagebased/service/rating/details.goopenmeter/billing/charges/usagebased/service/rating/quantitysnapshot.goopenmeter/billing/charges/usagebased/service/rating/service_test.goopenmeter/billing/charges/usagebased/service/rating/totals.goopenmeter/billing/charges/usagebased/service/run/create.goopenmeter/billing/charges/usagebased/service/run/payment_test.goopenmeter/billing/charges/usagebased/service/statemachine.goopenmeter/ent/schema/chargesusagebased.gotools/migrate/migrations/20260424091352_usagebased_run_data_structure.down.sqltools/migrate/migrations/20260424091352_usagebased_run_data_structure.up.sql
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/billing/charges/usagebased/service/run/create.go (1)
196-201: Nit:StoredAtLT/MeteredQuantityin the update are redundant.Inside this branch the only values that actually change post-allocation are
runTotals.CreditsTotalandrunTotals.Total.StoredAtLTandratingResult.Quantitywere already persisted a few lines above viacreateNewRealizationRun, so passing them again just causes a no-op write. If you wanted to keep the call lean, you could drop them and only shipTotals.♻️ Optional trim
currentRunBase, err := s.adapter.UpdateRealizationRun(ctx, usagebased.UpdateRealizationRunInput{ - ID: currentRun.ID, - StoredAtLT: mo.Some(in.StoredAtLT), - MeteredQuantity: mo.Some(ratingResult.Quantity), - Totals: mo.Some(runTotals), + ID: currentRun.ID, + Totals: mo.Some(runTotals), })Totally fine to defer — just flagging since the new shape makes the redundancy more visible than the old
AsOf/MeterValueversion did.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/run/create.go` around lines 196 - 201, The UpdateRealizationRun call is sending redundant fields StoredAtLT and MeteredQuantity (ratingResult.Quantity) which were already persisted by createNewRealizationRun; modify the call to s.adapter.UpdateRealizationRun in this branch to only include the fields that actually changed (e.g., Totals/runTotals with CreditsTotal and Total) and remove StoredAtLT and MeteredQuantity from the UpdateRealizationRunInput to avoid a no-op write.
🤖 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/charges/usagebased/service/run/create.go`:
- Around line 196-201: The UpdateRealizationRun call is sending redundant fields
StoredAtLT and MeteredQuantity (ratingResult.Quantity) which were already
persisted by createNewRealizationRun; modify the call to
s.adapter.UpdateRealizationRun in this branch to only include the fields that
actually changed (e.g., Totals/runTotals with CreditsTotal and Total) and remove
StoredAtLT and MeteredQuantity from the UpdateRealizationRunInput to avoid a
no-op write.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11f010e2-1606-4dfa-b8db-2cb28e4e2a84
📒 Files selected for processing (5)
openmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/usagebased_test.goopenmeter/billing/charges/usagebased/service/run/create.goopenmeter/billing/worker/subscriptionsync/service/creditsonly_test.goopenmeter/ledger/chargeadapter/usagebased_test.go
✅ Files skipped from review due to trivial changes (1)
- openmeter/billing/charges/service/usagebased_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/billing/charges/service/invoicable_test.go (1)
467-470: Tiny nit:expectedCollectionEndis a literal duplicate offinalStoredAtLT.Both variables are initialized to the exact same
2026-02-03T00:00:00Zvalue and used together in triplets likes.True(finalStoredAtLT.Equal(x))+s.True(expectedCollectionEnd.Equal(x.UTC())). Sincestored_at_ltis the collection cutoff in the new model, keeping both names around mostly just adds noise. Same thing happens at lines 783–784.Feel free to collapse them into one (and tweak the paired assertions accordingly) — purely a readability/maintenance nicety, nothing broken.
🧹 Suggested simplification
- finalStoredAtLT := datetime.MustParseTimeInLocation(s.T(), "2026-02-03T00:00:00Z", time.UTC).AsTime() - expectedCollectionEnd := datetime.MustParseTimeInLocation(s.T(), "2026-02-03T00:00:00Z", time.UTC).AsTime() + finalStoredAtLT := datetime.MustParseTimeInLocation(s.T(), "2026-02-03T00:00:00Z", time.UTC).AsTime()And at the assertion sites (e.g., 632–634, 728–730, 911–912, 999–1000, 1813–1815), a single equality check on
StoredAtLTis enough — once it equals a known non-zero timestamp, theIsZero()guard and the.UTC()re-check are redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/invoicable_test.go` around lines 467 - 470, Remove the duplicated timestamp variable and use a single shared variable (either finalStoredAtLT or expectedCollectionEnd) for the collection cutoff; update the tests that compare stored_at_lt (examples: assertions around StoredAtLT checks referenced in the diff such as the checks at 632–634, 728–730, 911–912, 999–1000, 1813–1815 and the similar pair at 783–784) to assert equality against the single timestamp variable and drop the redundant IsZero() and .UTC() re-checks so each stored_at_lt comparison is a single equality assertion to the known non-zero timestamp.
🤖 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/charges/service/invoicable_test.go`:
- Around line 467-470: Remove the duplicated timestamp variable and use a single
shared variable (either finalStoredAtLT or expectedCollectionEnd) for the
collection cutoff; update the tests that compare stored_at_lt (examples:
assertions around StoredAtLT checks referenced in the diff such as the checks at
632–634, 728–730, 911–912, 999–1000, 1813–1815 and the similar pair at 783–784)
to assert equality against the single timestamp variable and drop the redundant
IsZero() and .UTC() re-checks so each stored_at_lt comparison is a single
equality assertion to the known non-zero timestamp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: abfed217-c37c-465a-b9ad-f49572bd3caa
📒 Files selected for processing (1)
openmeter/billing/charges/service/invoicable_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/charges/SKILL.md:
- Around line 173-176: The doc list of timestamp surfaces is missing
usagebased.UpdateRealizationRunInput.ServicePeriodTo; update
.agents/skills/charges/SKILL.md to include
`usagebased.UpdateRealizationRunInput.ServicePeriodTo` alongside the existing
`StoredAtLT` entry so the checklist matches the guidance that references
SetServicePeriodTo(...), ensuring the UpdateRealizationRunInput timestamp
surfaces section lists both StoredAtLT and ServicePeriodTo.
🪄 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: f3af12cb-ad7a-45c5-91e7-3692099b8faf
📒 Files selected for processing (1)
.agents/skills/charges/SKILL.md
tothandras
approved these changes
Apr 24, 2026
This was referenced Apr 27, 2026
Merged
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 PR refactors usage-based realization run snapshot fields to make the persisted data model match the new run
semantics.
The run no longer stores the old
asof,collection_end, andmeter_valueconcepts. Instead it stores theexplicit query boundaries and the resulting quantity:
stored_at_ltservice_period_tometered_quantityIt also aligns usage-based invoice collection timing with the charge state machine by setting
StandardLine.OverrideCollectionPeriodEndfrom the run snapshot cutoff.Field Renames And Semantics
asofstored_at_ltstored_at < stored_at_lt.collection_endstored_at_ltplus invoice-lineoverride_collection_period_end.meter_valuemetered_quantityservice_period_toCalculation Rules
Meter queries for detailed run calculation use:
For partial invoice runs:
For final realization runs:
The charge state machine waits for the internal ingestion buffer before processing the run:
Usage-based standard invoice lines are aligned to the same readiness point:
This keeps the billing invoice state machine and usage-based charge state machine using the same collection
readiness timestamp.
Migration Backfill
The migration renames:
Historical rows used collection_end as the effective meter-query cutoff, so the migration backfills:
For service_period_to:
The non-final fallback is defensive. Persisted partial runs are not expected for current production data, but the
fallback keeps the migration safe for any existing non-final rows.
After backfill, collection_end is dropped.
Summary by CodeRabbit
Refactor
Documentation
Tests