improvement(billing): serve whole-period usage totals from a maintained rollup - #6806
improvement(billing): serve whole-period usage totals from a maintained rollup#6806waleedlatif1 wants to merge 8 commits into
Conversation
…ed rollup The period total is SUM(usage_log.cost) over a billing period, which is O(events in the period) and sits on the per-execution quota path, so it gets slower as a period fills up and as an account scales. A covering index does not fix it: the rows being summed are always the newest in the period, which are exactly the pages the visibility map has not marked all-visible, so an index-only scan degrades to a heap fetch per row regardless of what the index carries. Add usage_log_period_total, keyed by (entity type, entity id, period start, period end), maintained transactionally by both usage_log mutation paths. Reads become a primary-key lookup, behind the usage-period-total-reads flag so the rollup can be reconciled against the ledger before anything depends on it. - recordUsage now opens its own transaction when the caller supplies none, so the ledger write and the increment commit together - only rows that actually land count toward the total; eventKey conflicts drop replays and must not inflate it - readUsagePeriodTotal returns null rather than 0 for a missing row, so a period predating the rollup falls back to aggregating the ledger - source-filtered totals are not covered by the rollup and still aggregate - reconcileUsagePeriodTotals recomputes absolute totals from the ledger, which stays the source of truth; it also covers the one non-transactional drift source, the ON DELETE CASCADE from user
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Period usage reads ( Daily refresh can use Rollout pieces: Reviewed by Cursor Bugbot for commit f785bca. Configure here. |
Greptile SummaryThe PR introduces a transactionally maintained daily usage rollup and feature-gated read paths for billing-period totals and daily-refresh calculations, with ledger fallbacks during rollout.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains after accounting for the fixes and prior-thread acknowledgments. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/billing/core/usage-daily-total.ts | Implements rollup writes, reads, and transactional reconciliation; the prior orphan cleanup issue is fixed, and the remaining reconciliation window was explicitly acknowledged in the prior discussion. |
| apps/sim/lib/billing/core/usage-log.ts | Atomically maintains daily rollups for inserted and cumulative usage while adding feature-gated rollup-backed period reads. |
| apps/sim/lib/billing/credits/daily-refresh.ts | Uses daily rollup buckets for eligible open-period calculations and preserves ledger fallback behavior for unsupported scopes. |
| packages/db/schema.ts | Defines the daily usage rollup table and key structure corresponding to the additive migration. |
| packages/db/migrations/0292_dapper_pandemic.sql | Adds the daily usage rollup storage required by the new dual-write and read paths. |
Reviews (6): Last reviewed commit: "improvement(billing): require the whole ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…ckets Daily refresh buckets usage_log by day to apply a per-day allowance cap. Like the whole-period total it is O(events in the period) and runs per execution, and it was the more expensive of the two. Widen the rollup to (user, day) grain so one table answers both: the period total sums every bucket, daily refresh sums them per day. Reads stay behind the flag and fall back to the ledger whenever the rollup cannot answer exactly. The day bucket cannot reproduce the ledger's cap in every case, so the fast path is taken only when it is provably exact — a billing entity is set, no per-user bounds (whose arbitrary instants can split a bucket), and the period is still open (a closed one caps at periodEnd, and a row can be stamped to a period yet created after it ends). Also fixes review findings: - reconcile deletes orphaned buckets instead of only refreshing surviving ones; a group whose ledger rows are entirely gone has nothing to recompute from and kept a stale total forever - one definition of the day-bucket expression, in JS and SQL, replacing three hand-copied formulas that had already drifted: the ledger fallback truncated the period start to whole seconds while the rollup kept milliseconds - the read flag moves into the rollup module, so a disabled flag uses the same null-means-fall-back channel every other case does - per-day capping and its telemetry are shared by both paths - add the reconcile script the rollout depends on; it had no caller
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 6c7c5a6. Configure here.
…uses it too getBillingPeriodUsageCostWithSourceSubset splits a period total into the Chat-family portion and the rest. Source was not part of the bucket key, so that call had to keep scanning the ledger — the last hot billing aggregate still doing so. Add source to the grain. It costs about a fifth more rows, because most buckets only ever see one source, and leaves the read essentially unchanged: the median period is still a single bucket. Reads now go through one source-keyed breakdown that answers all three whole-period aggregates — unfiltered total, source-filtered total, and total-plus-subset — so source-filtered calls stop falling back to the ledger as well. Folding both numbers from one read also preserves the property the single-statement aggregate was written for: the subset cannot exceed the total. A single recordUsage call can span sources, so the rollup delta is grouped per source rather than applied as one lump, which would have credited the whole amount to whichever source came back first.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 940bbe3. Configure here.
The two bucket readers repeated the entity+period WHERE prefix, the cost aggregate, and the row-to-map fold with its null-sum guard. The prefix is the one that mattered: it is the bucket key, so a change to it had to be made in both places or one reader would silently scope wrong. Extract those three. The readers stay separate — their narrowing differs deliberately (the daily read omits billingPeriodEnd to match the ledger's scope) and folding that into one parameterized reader would bury the asymmetry reviewers already found easy to miss.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a096397. Configure here.
The reconciler recomputes absolute totals from a snapshot, and ran its upsert and its orphan-delete as two separate statements. That let a concurrent usage write be lost two ways: the upsert could replace a newer increment with an older snapshot's total, and the delete could remove a bucket that had just become valid under a view the upsert never saw. Rollup-backed quota and overage would then undercount until the next reconcile. Run both passes in one transaction, and skip any bucket a live writer has touched since that transaction began. Live maintenance is exact, so leaving such a bucket alone is always safe; overwriting it from an older snapshot is not. One window remains and is documented rather than papered over: a ledger write that began before this transaction and commits after its snapshot is invisible here while its bucket still looks untouched. That is one usage write long, and re-running repairs it.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1b6ddce. Configure here.
…aluation Three defects found auditing the diff, none of which type-checking or the mocked unit tests could see. extract(epoch from $1) on a bare bind parameter is ambiguous to Postgres and throws at runtime. Consolidating the day-index formula moved the period start from a pre-truncated epoch literal into extract(), so the daily-refresh ledger fallback — the path taken for closed periods, for per-user bounds, and for every period before the rollup is reconciled — would have failed on its first real query. Both operands are now bound and cast explicitly. bucketCostSum held a sql template in a module constant, so importing this module evaluated the tag at import time. That made any test partially mocking drizzle-orm without sql fail on merely reaching billing transitively, which is how it surfaced: an unrelated connectors suite went red in CI. Built per call instead. sumSourceTotals walked the caller's source list and looked each up, so a repeated source counted its bucket twice. It now walks the breakdown and tests membership. Also types dayIndexSql's operands as SQLWrapper rather than unknown, which is what let the sql-date-binding audit see the first defect at all.
The reconcile aggregate keyed off billing_entity_type alone. The all-or-none CHECK on usage_log does hold that the other three key columns travel with it, but it is NOT VALID: it binds new writes and says nothing about rows predating it. A row with a null key column yields a null day_index, and both are NOT NULL in the rollup, so one such row would abort the entire all-or-nothing reconcile instead of being passed over. Such a row is unreadable through the rollup regardless, since every read matches on all four columns. Verified against a real database rather than mocks: seeding a malformed row alongside good ones, the reconcile now skips it and the rollup and ledger paths still return identical totals. Also records a sizing caveat on the schema: a subscription's period rolls over monthly and bounds an entity's buckets, but an account with no subscription bills against an open-ended sentinel period that never rolls over, so its buckets accumulate for the life of the account. Not a correctness difference — both paths admit the same rows — but those entities' reads grow slowly with age instead of staying flat.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f785bca. Configure here.
The scoped reconcile interpolated periodEndsAfter, a Date, straight into two sql templates. Drizzle swaps postgres-js's temporal serializer for an identity function, so the Date reached the wire encoder and the run died with ERR_INVALID_ARG_TYPE. That is the documented step before flipping the read flag, so the bounded repair was unusable; only the unbounded full-table run worked. Same bug class as the day-index binding, at a site check:sql-date-binding cannot see: its detector does not follow member expressions, so options?.periodEndsAfter never enters its set of tracked dates. Noted in a comment so the next reader does not take the passing audit as cover. Verified against a real database: --since now reconciles, the unbounded run still does, and the bucket totals are unchanged.
1 participant
Summary
SUM(usage_log.cost)over the period —O(events in the period), and it sits on the per-execution quota path, so it degrades both as a period fills up and as an account scales.Index Only Scanwith a 100% heap-fetch rate, and slower than the plain index scan.usage_log_period_total, keyed by(entity type, entity id, period start, period end)and maintained transactionally by bothusage_logmutation paths (the only two in the codebase). The read becomes a primary-key lookup.usage-period-total-readsflag. Dual-write is unconditional; only the read is gated, so the rollup can be reconciled against the ledger before anything depends on it, and the flag can go back off without losing data.usage_log_billing_period_cost_idx, which claimed these aggregates resolve index-only. They don't.Correctness details worth a look in review:
recordUsagenow opens its own transaction when the caller supplies none — otherwise the ledger insert and the increment are two statements, and a failure between them leaves the rollup permanently short.eventKeyconflict drops replays, and counting attempted cost would inflate the total with usage the ledger never recorded.readUsagePeriodTotalreturnsnull, not0, for a missing row, so a period predating the rollup falls back to aggregating the ledger instead of reporting no usage.source-filtered totals aren't covered by the rollup and still aggregate.usage_log.user_idisON DELETE CASCADE, so a user deletion removes ledger rows without lowering the rollup. That's the one non-transactional drift source; it over-counts only, andreconcileUsagePeriodTotalscorrects it.Rollout: deploy → run
reconcileUsagePeriodTotals()→ confirm it agrees with the ledger → enable the flag. No backfill in the migration on purpose — the migration runs before the new image is live, so the old code would write ledger rows without maintaining the rollup and leave a stale base for every later increment.Type of Change
Testing
usage-logtests to cover the transaction boundary and to distinguish ledger inserts from rollup inserts.null-vs-0return, and the transaction wrapper, and confirmed each corresponding test goes red before restoring.bun run check:migrations origin/stagingpasses — the migration is purely additive (new table, no backfill).bun run lint,check:audits(29 audits),check:api-validation:strict, block-registry check, andtype-checkall pass.lib/billing,lib/core/config, andlib/logs.Checklist