[IAmJSD]

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) and savings_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) and provider_savings_summary (000074) rolled these columns up with SUM and AVG, 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_commitment was unaffected because it was NOT NULL. That is no longer true: 000101 made it nullable too, so it is fixed alongside total_usage.

Fix

Migration 000147_savings_rollups_propagate_unknown drops and recreates both views:

• The inner per-timestamp total is 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 for upfront_cost (internal/api/analytics_postgres.go:180).
• The outer per-period average is 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.
• A known 0 stays 0. Fully known data gives the same H5 sum-then-average result as before.
• Unchanged: total_savings (NOT NULL), avg_coverage (a percentage averaged over the rows that have one, which is deliberate per H2), snapshot_count, and daily_savings_trend, which aggregates no nullable cost column.
• Column names and types are unchanged (a CASE with no ELSE keeps the numeric type). The unique indexes are recreated as 000076 left them (plain columns, NULLS NOT DISTINCT), and both views are created populated, so refresh_savings_materialized_views() can keep refreshing them CONCURRENTLY.
• The down migration restores the 000078 / 000074 definitions exactly. The test compares pg_get_viewdef and the index definitions before and after a round trip.

No Go code changes. No API reads these columns: QueryMonthlyTotals selects only total_savings, avg_coverage and snapshot_count from monthly_savings_summary, and nothing reads provider_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_ConcurrentRefresh sets this up deterministically. A reader holding provider_savings_summary pauses the migration after it has replaced monthly_savings_summary and while it still holds that view's lock. A refresh is then started and confirmed (via pg_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

#	Instance (from the issue)	Status
1	APIPermissionConstraint.MaxAmount omitempty (internal/auth/service_api.go:59)	No change. Superseded by the owner decision on #133. Absent and 0 both mean "no cap" in every reader. An explicit max_amount: 0 is 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. Dropping omitempty would make it worse: every uncapped constraint would GET as max_amount: 0, and a client that echoes it back would get the #983 400.
2	PermissionConstraints.MaxPurchaseAmount omitempty (internal/auth/types.go:76)	No change, for the same reason as 1.
3	monthly_savings_summary inner SUM(total_usage) / outer AVG(ts_usage) (000078:48, :34)	Fixed (000147).
4	"Same shape for daily_savings_trend" (000067:101, 000074:193)	Covered by 3. Those lines are the older flat AVG(total_usage) definitions of monthly_savings_summary, which 000078 replaced. The current daily_savings_trend aggregates no nullable cost column.
5	(found while re-verifying) total_commitment in monthly_savings_summary and provider_savings_summary. The issue called it NOT NULL, but it has been nullable since 000101.	Fixed (000147).
6	A02-013: accounts API Enabled fallback / AWSIsOrgRoot *bool (internal/api/handler_accounts.go updateAccount, cloudAccountFromRequest)	Deferred. The fix edits the updateAccount lines around account := cloudAccountFromRequest(req), and our open #1141 (provider-immutable check) and #1248 (annotateAccountNotices) both edit those same lines. Follow up after they merge.
7	A06-027: assembleLadderPlan fabricates existingTotal = 0 (internal/server/handler_ladder.go:600-607)	Deferred. Our open #1238 rewrites the same existingTotal block (on-demand-equivalent conversion). Follow up after it merges.
8-11	groupModals.ts, settings.ts, numeric filter extractors, ramp progress / recommendations.ts	Frontend. See below.

JSON / API output changes

None. No omitempty tag 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_commitment and provider_savings_summary.total_commitment can 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 no TestMain for the shared-container helper, which only internal/config uses.

New internal/database/postgres/migrations/000147_savings_rollups_propagate_unknown_test.go (integration tag):

• TestMigration_SavingsRollupsPropagateUnknown migrates 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 calls refresh_savings_materialized_views() (the CONCURRENTLY path). It then asserts:

  • the mixed and known-then-unknown accounts are NULL in all three columns;
  • the known-zero account is 0, not NULL;
  • the fully known account is 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_ConcurrentRefresh runs the refresh-during-migration race described above.

Fail-before / pass-after:

• Fail before (mutation standing in for main): the 000147 up file was replaced with the old definitions from its down file. Both tests fail: every "must be unknown (NULL)" assertion fails for the mixed, known-then-unknown, up-again and racing-refresh cases (12 assertions).
• Mutation, outer level only: the outer CASE ... AVG was reverted to a plain AVG. The known-then-unknown case fails (3 assertions), so both levels are needed.
• Pass after: both tests pass. The existing 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):

go build ./...
go vet ./...
go vet -tags integration ./...
go test -count=1 ./internal/database/postgres/migrations/
go test -count=1 ./internal/mocks/
go test -tags integration -count=1 -timeout 60m -run 'TestMigration_SavingsRollupsPropagateUnknown' ./internal/database/postgres/migrations/
go test -tags integration -count=1 -timeout 60m -run 'TestAnalyticsNestedRollup_COR02' ./internal/database/postgres/migrations/
golangci-lint run --allow-parallel-runners --build-tags integration ./internal/database/postgres/migrations/...   # v2.10.1: 0 issues

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/main a14dc8f2 plus these heads, merged in this order:

Branch	Tested head
fix/issue-337	ff3420c
fix/issue-169	7d75eb5
fix/issue-342	66b221f
fix/issue-310	5e11c21
fix/issue-326	5f0ea59
fix/issue-243	365e959
fix/issue-343	6c94036
fix/issue-312	d2848a8
fix/issue-316	acd60dd
fix/issue-339	75ea9b9
fix/issue-339-commitment-type	73bdb19
fix/issue-353	5d25fcd
fix/issue-313	3156bc4
fix/issue-357	53d9e58
fix/issue-368	0ba727d
fix/issue-419	61648c9
fix/issue-111	b04c372
fix/issue-686	0ad90b7
fix/issue-674	793095d
fix/issue-1055	4ec96f1
fix/issue-559	dfa9525
fix/issue-558	034eed4
fix/issue-874	1720539
fix/issue-666	1e120da
fix/issue-668	fb26345
fix/issue-568	ce04173
fix/issue-311	e54f250
fix/issue-177	0cbb88a
fix/issue-151	c21377c
fix/issue-166	41fb77b
fix/issue-27	2d67622
fix/issue-323	69288ae
fix/issue-59	36463e8
fix/issue-43	35e4069
fix/issue-158	6f76f8a
fix/issue-104	bb00491
fix/issue-55	971ef1b
fix/issue-361	89ac220
fix/issue-120	b458719
fix/issue-36	42bac6e
fix/issue-525	57c97dd
fix/issue-712	e5bcda0
fix/issue-72	6cc63ed

fix/issue-72 and fix/issue-312 conflict on one line in internal/api/handler_dashboard.go (both change the calculateCommitmentMetrics call). The tree used #72's side with #312's validated provider variable; 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:

• Passed at 936996ad: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks at 936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. tests/e2e is a separate module, so it was vetted under its own e2e tag. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package, one package at a time (-tags integration -count=1, 60-minute local timeout because the VM was shared):
  • At 936996ad, these pass: internal/server, internal/credentials, internal/scheduler, internal/database/postgres/migrations, internal/purchase and internal/api.
  • internal/config passes except TestPostgresStoreDB_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).
  • These were run on the first build of this tree (26795376, same branches at earlier heads): internal/analytics, internal/auth, internal/commitmentopts, internal/database and cmd/lambda/clear-rate-limit all pass. The later fixes don't touch those packages.
• What the combined run caught, each fixed on its own branch before this PR opened:
  • a compile break between fix/issue-104's test and fix/issue-326's httpToLambdaRequest signature change;
  • fix/issue-316 rejecting legacy-keyed plans;
  • fix/issue-120 versus main's global-filter test.
• Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns 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.
• Nothing was applied to any cloud.

Out of scope

• A02-013 and A06-027 are deferred for the overlap reasons in the table.
• avg_coverage keeps averaging over the rows that have a coverage figure. That is an average of a percentage, not a sum, and H2 chose it deliberately.
• fix(purchase): preserve unknown provider upfront costs #450 described the view commitment columns as "known-sample statistics without a completeness guarantee". This PR changes that: the views now report NULL rather than a known-sample figure. If the maintainer prefers the known-sample meaning for 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:3208 and :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 returns p.upfront_cost ?? 0 and p.estimated_savings ?? 0. The types are non-null, but for a step with has_recommendations === false these are placeholders (platform#609), so a =0 filter matches them. It should return Number.NaN for placeholder or absent values.
• frontend/src/history.ts:1045 and :1734: p.estimated_savings ?? 0. Its upfront_cost neighbors at :1044 / :1733 already return Number.NaN. Return Number.NaN here too for consistency.
• frontend/src/riexchange.ts:686-687: source_count ?? 0 / target_count ?? 0. target_count is optional (api/types.ts:802, :834), so an absent count matches =0. Return Number.NaN, as the neighbors at :688-690 do.
• frontend/src/plans.ts:1096 and :1148: the ramp default { current_step: 0, total_steps: 1 } and ${current_step || 0}/${total_steps || 1} steps still render missing step data as "0/1 steps". Render -- when total_steps is not a positive number.
• frontend/src/recommendations.ts:881: the plr.savingsMin === null || plr.savingsMax === null branch is unreachable, because pageLevelRange (:1257 onward) initializes both to 0 and PageLevelRange types them as number (:1216-1217). Either propagate nullability out of pageLevelRange or 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

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}