Repository navigation
Conversation
…ngs rollups savings_snapshots.total_usage and total_commitment are nullable on purpose: NULL means the provider gave no figure. monthly_savings_summary and provider_savings_summary rolled them up with SUM and AVG, which skip NULL, so "us-east-1 $4000 + eu-west-1 unknown" became a definite $4000. Migration 000147 recreates both views so a timestamp's total is NULL when any of its rows is unknown, and the period average is NULL when any of its timestamps is. A known 0 stays 0. Column names and types, the unique indexes and the CONCURRENTLY refresh path are unchanged; the down migration restores the previous definitions. Integration tests on real Postgres cover unknown, known zero, fully known, unknown-at-one-instant, down/up, and a refresh_savings_materialized_views() call queued behind the migration's locks. Refs LeanerCloud#151 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
priority/p2
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB1. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
This was referenced Oct 25, 2026
Member
|
Independent review 1 (cold, fresh context): VERDICT: APPROVE d5518d9 Findings:
Unknown-value trace (producer -> SQL -> API -> UI):
Attacked:
@IAmJSD the open migration PRs now span 000140-000149: #1239 (140/141), #1240 (142), #1241 (143), #1253 (144), #1302 (145), #1268 (146), #1287 (147), #1116 (149). Merging them in number order (or renumbering whichever lands later above main's highest) keeps golang-migrate from skipping any. I'm adding a guard on our merge side that refuses a PR whose migration number is not above main's highest. |
Member
|
Independent review 2 (claude-fleet, cold): APPROVE at d5518d9 Blocking (file:line)
Non-blocking (file:line)
Is the fix wired in? Yes. The fix lives entirely in SQL (new migration files), and that is why the diff deletes nothing. migrate.go:468/507/540 loads every file in the migrations dir via file://, so 000147 runs on deploy. It drops and recreates both matviews, which replaces the 000078/000074 definitions at the DB level. No old collapsing definition is left live after up. No Go code changes, and none is needed. Collapse-point table (site / before / after / test)
Consumer table
Semantics check
Probes (on the head export, each restored with cp, cmp OK, FINAL_RESTORED)
Commands (GOTOOLCHAIN=go1.26.10, AWS config/creds set to /dev/null, AWS_PROFILE empty)
Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged. |
This branch has not been deployed
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
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.
Summary
This is the backend SQL slice of #151. Two analytics materialized views stop turning an unknown cost into a definite number. Every backend instance in the issue was re-checked against current main (a14dc8f). The table below gives each one's status. Frontend items are listed for the UI owner at the end and are not changed here.
Root cause
savings_snapshots.total_usage(nullable since 000067, H2) andsavings_snapshots.total_commitment(nullable since 000101, #450) store NULL when the provider gave no figure. For example, an all-upfront purchase has no recurring cost to derive usage from. The collector writes NULL on purpose, because a placeholder 0 would read as a real $0.monthly_savings_summary(000078) andprovider_savings_summary(000074) rolled these columns up withSUMandAVG, and both skip NULL. At one collection timestamp, "us-east-1 $4000 + eu-west-1 unknown" therefore became a definite$4000, with no sign that a region was missing. The same happened over time. If a month had one known instant ($100) and one unknown instant, it averaged to$100.The issue said
total_commitmentwas unaffected because it wasNOT NULL. That is no longer true: 000101 made it nullable too, so it is fixed alongsidetotal_usage.Fix
Migration
000147_savings_rollups_propagate_unknowndrops and recreates both views:CASE WHEN COUNT(col) = COUNT(*) THEN SUM(col) END. One unknown row makes that instant's total unknown. This is the same pattern the history totals already use forupfront_cost(internal/api/analytics_postgres.go:180).CASE WHEN COUNT(ts_col) = COUNT(*) THEN AVG(ts_col) END. One unknown instant makes the period unknown. Averaging only the known instants would understate a period in which a purchase with no figure became active.0stays0. Fully known data gives the same H5 sum-then-average result as before.total_savings(NOT NULL),avg_coverage(a percentage averaged over the rows that have one, which is deliberate per H2),snapshot_count, anddaily_savings_trend, which aggregates no nullable cost column.CASEwith noELSEkeeps the numeric type). The unique indexes are recreated as 000076 left them (plain columns,NULLS NOT DISTINCT), and both views are created populated, sorefresh_savings_materialized_views()can keep refreshing themCONCURRENTLY.pg_get_viewdefand the index definitions before and after a round trip.No Go code changes. No API reads these columns:
QueryMonthlyTotalsselects onlytotal_savings,avg_coverageandsnapshot_countfrommonthly_savings_summary, and nothing readsprovider_savings_summary. Only direct SQL consumers of the views see the change.Concurrency. The plausible race is a collector run calling
refresh_savings_materialized_views()while the migration is dropping and recreating the views during a deploy.TestMigration_SavingsRollupsPropagateUnknown_ConcurrentRefreshsets this up deterministically. A reader holdingprovider_savings_summarypauses the migration after it has replacedmonthly_savings_summaryand while it still holds that view's lock. A refresh is then started and confirmed (viapg_stat_activity) to be waiting on the lock. The reader is then released. The migration commits, and the queued refresh succeeds against the new views instead of failing on the dropped OID.Migration number: 000147. main is at 000134. Open PRs and our unopened fork branches use 000135 to 000146.
Refs #151
Per-instance status
APIPermissionConstraint.MaxAmountomitempty(internal/auth/service_api.go:59)0both mean "no cap" in every reader. An explicitmax_amount: 0is rejected with 400 on group writes (#983, merged in acb9182) and on API-key create (our open #1159, which covers that instance). A stored 0 therefore can never be a cap, and the round trip is already lossless. Droppingomitemptywould make it worse: every uncapped constraint would GET asmax_amount: 0, and a client that echoes it back would get the #983 400.PermissionConstraints.MaxPurchaseAmountomitempty(internal/auth/types.go:76)monthly_savings_summaryinnerSUM(total_usage)/ outerAVG(ts_usage)(000078:48, :34)daily_savings_trend" (000067:101, 000074:193)AVG(total_usage)definitions ofmonthly_savings_summary, which 000078 replaced. The currentdaily_savings_trendaggregates no nullable cost column.total_commitmentinmonthly_savings_summaryandprovider_savings_summary. The issue called itNOT NULL, but it has been nullable since 000101.Enabledfallback /AWSIsOrgRoot *bool(internal/api/handler_accounts.goupdateAccount,cloudAccountFromRequest)updateAccountlines aroundaccount := cloudAccountFromRequest(req), and our open #1141 (provider-immutable check) and #1248 (annotateAccountNotices) both edit those same lines. Follow up after they merge.assembleLadderPlanfabricatesexistingTotal = 0(internal/server/handler_ladder.go:600-607)existingTotalblock (on-demand-equivalent conversion). Follow up after it merges.groupModals.ts,settings.ts, numeric filter extractors, ramp progress /recommendations.tsJSON / API output changes
None. No
omitemptytag is changed, for the reasons in rows 1 and 2. The only output that changes is SQL:monthly_savings_summary.total_usage,monthly_savings_summary.total_commitmentandprovider_savings_summary.total_commitmentcan now be NULL where they used to hold a partial figure. No API endpoint or UI reads them today.Verification
All evidence comes from local runs on Linux against a real PostgreSQL 16 testcontainer. These are integration tests on fixture rows, not production data. The migrations package's integration tests each start their own container (
testhelpers.RequirePostgresContainer, like the other migration tests there). That package has noTestMainfor the shared-container helper, which onlyinternal/configuses.New
internal/database/postgres/migrations/000147_savings_rollups_propagate_unknown_test.go(integration tag):TestMigration_SavingsRollupsPropagateUnknownmigrates to the previous version and inserts four fixture accounts: mixed known/unknown regions at one instant, known zeros, fully known data across two instants, and known-then-unknown across two instants. It asserts the old views report the partial figures (4000,100), then migrates up and callsrefresh_savings_materialized_views()(theCONCURRENTLYpath). It then asserts:0, not NULL;225.Finally it migrates down, asserts the view and index definitions equal the originals and the old partial figure is back, and migrates up again.
TestMigration_SavingsRollupsPropagateUnknown_ConcurrentRefreshruns the refresh-during-migration race described above.Fail-before / pass-after:
CASE ... AVGwas reverted to a plainAVG. The known-then-unknown case fails (3 assertions), so both levels are needed.TestAnalyticsNestedRollup_COR02(000078, which migrates down past 000147 and back) also passes.Commands, all passing (run with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true):gocyclo: no non-test Go files changed. The fixtures use
acct-*strings, not 12-digit account IDs.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope
avg_coveragekeeps averaging over the rows that have a coverage figure. That is an average of a percentage, not a sum, and H2 chose it deliberately.total_commitment, the alternative is a companion known-ness flag column.Frontend follow-up for the UI owner
Status on main a14dc8f. Nothing under
frontend/is changed here.frontend/src/groups/groupModals.ts:293(populate) and:497(collect): already fixed. They use?? ''and.trim() !== '', consistent with the fix(frontend): a max_amount spend cap of 0 is silently erased when a group is edited #133 decision that 0 is refused rather than stored.frontend/src/settings.ts:3208and:3216(default_coverage): already fixed by fix(frontend/settings): keep a stored default_coverage of 0 on load #510 (??).frontend/src/plans.ts:251-252: the numeric filter returnsp.upfront_cost ?? 0andp.estimated_savings ?? 0. The types are non-null, but for a step withhas_recommendations === falsethese are placeholders (platform#609), so a=0filter matches them. It should returnNumber.NaNfor placeholder or absent values.frontend/src/history.ts:1045and:1734:p.estimated_savings ?? 0. Itsupfront_costneighbors at:1044/:1733already returnNumber.NaN. ReturnNumber.NaNhere too for consistency.frontend/src/riexchange.ts:686-687:source_count ?? 0/target_count ?? 0.target_countis optional (api/types.ts:802,:834), so an absent count matches=0. ReturnNumber.NaN, as the neighbors at:688-690do.frontend/src/plans.ts:1096and:1148: the ramp default{ current_step: 0, total_steps: 1 }and${current_step || 0}/${total_steps || 1} stepsstill render missing step data as "0/1 steps". Render--whentotal_stepsis not a positive number.frontend/src/recommendations.ts:881: theplr.savingsMin === null || plr.savingsMax === nullbranch is unreachable, becausepageLevelRange(:1257onward) initializes both to0andPageLevelRangetypes them asnumber(:1216-1217). Either propagate nullability out ofpageLevelRangeor delete the dead branch.Labels
A maintainer, please mirror the issue's triage labels onto this PR:
priority/p2,severity/high,urgency/this-quarter,impact/many,effort/l,type/bug,triaged.🤖 Generated with Claude Code