[IAmJSD]

Two items from the 2026-07-28 config/db review: item 5 (three copies of the dual-column account predicate) and item 6 (ri_utilization_cache has no account in its key).

Refs #120

Item 5: one shared account predicate

Root cause. Three separate builders produced the cloud_account_id = ANY(...) OR (provider ... AND account_id = ANY(...)) predicate: accountFilterClause + sortedProviderKeys in internal/analytics/postgres_analytics.go and in internal/api/analytics_postgres.go, and appendAccountPredicate + sortedProviderKeys in internal/config/store_postgres.go. They have already drifted. The config copy has the issue #24 provider gate, ((provider = '' OR LOWER(provider) = LOWER($p)) AND ...), but the two analytics copies still use (provider = $p AND ...). That is the partial-fix pattern the issue predicted.

Fix. New leaf package internal/accountpredicate (imports only fmt, sort, strings) with Build(args, uuids, externalIDsByProvider, match). The provider gate is a parameter (ProviderExact / ProviderCaseInsensitiveOrUnknown), so each caller's SQL and bind order stay byte-identical. All three callers now use thin wrappers that keep their "no filter" conventions: "TRUE" for the analytics queries, conds unchanged for config.

Pinning tests.

• internal/accountpredicate/accountpredicate_test.go pins the exact clause and args for both gates and checks determinism.
• TestAccountFilterClause_PinnedSQL (api, new) and the strengthened TestAccountFilterClause (analytics, exact args now) pin the per-caller output. TestAppendAccountPredicate_ProviderGate (config) gains a case for providers with no IDs.
• The api, analytics and config pinning tests were run against the unmodified main code first, and all passed. After the refactor they pass unchanged.
• Mutation probe: swapping the gate in either caller fails that caller's pin.

Item 6: account in the RI utilization cache key

Root cause. ri_utilization_cache was keyed (region, lookback_days) (migration 000031), and the singleflight key in internal/api/ri_utilization_cache.go was region#lookback. The payload is one AWS account's Cost Explorer result. Once the reshape path reads with per-account credentials, account B would be served account A's cached utilization for the same region within the TTL, and B's background refresh would join A's in-flight call. The issue notes this is latent: today both callers use the single ambient identity.

Fix.

• Migration 000150 ri_utilization_cache_account_key does four things:

  • adds cloud_account_id TEXT NOT NULL;
  • changes the PK to (cloud_account_id, region, lookback_days);
  • deletes existing rows, since they carry no account identity and the next read refetches them;
  • takes LOCK TABLE ... ACCESS EXCLUSIVE first, so a writer still on the previous release cannot insert a row between the DELETE and the NOT NULL add and fail the migration.

  The down migration empties the table and restores the old key. '' means "ambient identity not registered in cloud_accounts", because a PK column cannot be NULL.

• config.RIUtilizationCacheKey{CloudAccountID, Region, LookbackDays} replaces the (region, lookbackDays) arguments on GetRIUtilizationCache / UpsertRIUtilizationCache (store, interface, mock). RIUtilizationCacheEntry gains CloudAccountID.

• riUtilizationCache.getOrFetch takes the key. The singleflight key (riUtilizationFlightKey) includes the account, with fields quoted so a region string cannot alias another key.

• getRIUtilization and getReshapeRecommendations resolve the deployment's cloud account once through a new resolveReshapeAccountScope helper. The helper returns the account and the in-scope verdict, and that account keys the cache. Query parameters are now validated before the account is resolved, so a malformed request makes no STS or account lookup. getReshapeRecommendations used to resolve the account twice, once for the scope check (restricted sessions only) and once for the recs lookup. It now resolves once.

Behavior change to review. getRIUtilization now resolves the deployment account for unrestricted sessions too. Before, only restricted sessions resolved it. This costs one STS GetCallerIdentity plus a ListCloudAccounts per request. The read now fails closed when resolution fails, including when the running AWS account is not registered while other accounts are (resolveAWSCloudAccountID already refuses that case). getReshapeRecommendations already behaved this way. TestGetRIUtilization_AccountResolutionErrorFailsClosed pins it.

Rolling deploy. During a rolling deploy, a container still on the old release fails its upserts with "no unique or exclusion constraint matching the ON CONFLICT specification". The cache logs and ignores upsert errors, so it only loses that write. Its reads by (region, lookback_days) keep working against the single ambient identity, as today.

Verification

All evidence below is from unit tests with fakes/mocks or narrow integration tests against a local Postgres container. There was no real AWS call and no deployment. Ran 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 ./...          # OK
go test -count=1 ./internal/accountpredicate/ ./internal/analytics/ ./internal/config/ \
        ./internal/mocks/ ./internal/database/postgres/migrations/ ./internal/api/   # one package at a time, all ok
go test -tags integration -count=1 -timeout 20m -run 'TestMigration_RIUtilizationCacheAccountKey' ./internal/database/postgres/migrations/   # PASS
go test -tags integration -count=1 -timeout 20m -run 'TestRIUtilizationCache_Integration_|TestReshapeRecommendations_Integration_' ./internal/api/   # 9 PASS
go test -tags integration -count=1 -timeout 20m -run 'TestAccountPredicate_ProviderCaseAndEmpty_Integration|TestGetPurchaseHistoryFiltered_DualColumnAccount_Integration|TestGetActivePurchaseHistory_MatchesLegacyPredicate' ./internal/config/   # PASS
go test -tags integration -count=1 -timeout 20m -run 'TestPostgresAnalyticsStore_Query' ./internal/analytics/   # PASS
golangci-lint v2.10.1 run --allow-parallel-runners [--build-tags integration] on the touched packages   # 0 issues (both)
gocyclo -over 10 -ignore "_test\.go" <changed non-test files>   # clean

New tests for item 6:

• TestMigration_RIUtilizationCacheAccountKey (real Postgres) checks:
  • the PK columns before, after, after down, and after re-up;
  • a legacy row is dropped;
  • two accounts and the '' key can all hold a row for the same region and lookback;
  • one account still gets one row per key;
  • an insert that does not name its account is refused.
• TestRIUtilizationCache_Integration_AccountsInSameRegionKeepOwnRows (real Postgres) runs 8 concurrent upserts per key across three keys that share a region and lookback. Each key reads back only its own payload, and an account with no row misses.
• TestGetRIUtilization_AccountsInSameRegionDoNotShareCache (handler, fake store) checks that account B is served B's rows and not A's cached rows, and that A then still hits its own cached row.
• TestRIUtilizationCache_AccountsInSameRegionDoNotShareRow and TestRIUtilizationCache_SingleflightKeyedByAccount (cache, fake store). In the second, B's background refresh runs its own fetch while A's is blocked in flight.
• TestRIUtilizationFlightKey_IncludesEveryKeyField and TestGetRIUtilization_AccountResolutionErrorFailsClosed.

Fail-before evidence comes from mutation probes, because the old API had no account parameter and the new tests cannot compile against main:

Mutation	Tests that fail
handler keys the cache with CloudAccountID: ""	TestGetRIUtilization_AccountsInSameRegionDoNotShareCache
singleflight key drops the account (%q#%d of region, lookback)	TestRIUtilizationCache_SingleflightKeyedByAccount, TestRIUtilizationFlightKey_IncludesEveryKeyField
cache reads/writes the store with a blank account	TestGetRIUtilization_AccountsInSameRegionDoNotShareCache (plus a pre-existing SWR test)
store read ignores the account (WHERE $1::text IS NOT NULL AND region = $2 AND lookback_days = $3), real Postgres	TestRIUtilizationCache_Integration_AccountsInSameRegionKeepOwnRows

Each probe was reverted, and the suite passes on the final tree.

Found while testing: TestGetRIUtilization_LookbackValidation depends on test order. It passes in a full-package run only because a federation test helper os.Setenvs CUDLY_SOURCE_CLOUD=gcp without restoring it. Before the parameter-validation reorder above, it hit the no-network guard when run alone. With the reorder it passes in isolation (go test -run 'RIUtilization|Reshape|RIExchange|ExecuteExchange|ConvertibleRI|PerAccountPerms' ./internal/api/ with the variable unset: ok). The leaking os.Setenv in newFederationHandler is not changed here.

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 400d256b = origin/main 7b86e386 plus these heads, merged in this order:

Branch	Tested head
fix/issue-243-rest	007c163
fix/issue-1206	312072f
fix/issue-111-email	6d422e2
fix/issue-111-fail-closed	3817e29
fix/issue-712-age-cap	a91120b
fix/issue-712-fargate	aeaf0d4
fix/issue-613-legacy-ids	350e837
fix/issue-120-guards	6f38ebc
fix/issue-120-shared-predicate	cb25735
fix/issue-151-account-update-keep	d20cf01
fix/issue-713-upcoming-filter	e73fdfb
fix/issue-263-bastion-opts	7861f32
fix/issue-43-fixture-ids	8cee203
fix/issue-177-makefile-walk	365a9d6
fix/issue-27-seller-not-registered	d845f66
fix/issue-313-user-columns	6d759dd
fix/issue-353-mock-panics	aac2edf
fix/issue-8-config-stamps	b548ae5
fix/issue-30-monthly-totals	f637921

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. The full suite ran on f7c48020, which differs from this tree only by test-only commits on fix/issue-30, fix/issue-713 and fix/issue-263 that replace placeholder account-number literals with named constants:

• Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package that has them, one package at a time (-tags integration -count=1): internal/analytics, internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase, internal/scheduler and internal/server all pass. Migrations 000149, 000150 and 000151 apply in order on this tree.
• Frontend: the changed jest files (approval-details, settings-accounts, api) pass (221 tests).
• Secrets: the first run flagged 9 new fixture lines (placeholder account numbers next to an account key); fixed with named constants. Re-run on this tree: 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: 183 PASS, 0 FAIL.
• Terraform .tftest.hcl files were not run locally (CI runs them). Nothing was applied to any cloud.

Out of scope

• The other items of chore(config/db): medium/low findings from the 2026-07-28 full review #120 stay open. Items 2b, 7 and 9 are on another branch. Item 3 (external_id change orphaning history) and the 0 -> 95 utilization-threshold coercion are owner decisions.
• This PR does not decide whether the analytics reads (internal/analytics, internal/api) should adopt the case-insensitive / empty-provider gate the config path got in issue fix(config): account-filter provider gate drops rows with empty/wrong-case provider + NULL cloud_account_id #24. The shared builder makes that a one-word change per caller. It changes which rows the Trends and History analytics count, so it needs an owner call.
• cloud_account_id in ri_utilization_cache has no FK to cloud_accounts, because '' is a valid key. A deleted account's rows are never read again but are not removed.
• Possible textual conflicts with open PRs touching the same files: feat(ri-exchange): pass offered instance types to reshape analysis #1246 (handler_ri_exchange.go, handler_ri_utilization_scope_test.go), fix(config): validate ri_exchange_mode on PUT /api/config #1243, fix(api): four handler bugs found while documenting the API (#1102) #1141 (config/interfaces.go, mocks/stores.go), fix(db): contract the canceled rename: normalize, narrow the status CHECKs, stop using cancelled_by #1253 and feat(plans): settle a failed-with-submission row so a managed plan can be deleted #1241 (config/store_postgres.go, config/types.go).

Maintainer: please mirror the issue's triage labels onto this PR: impact/internal, effort/l, type/chore, triaged, priority/p3, severity/low, urgency/eventually.

🤖 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

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE cb25735796907e91eadf40037826df4c1b894cbe

Blocking:

• none

Non-blocking:

• Owner decision. Migration merge order: this PR uses 000150. 000149 belongs to fix(config/db): settled-only expiry cleanup, RI-exchange limit CHECKs, capped list reads (#120 slice) #1552 and 000151 to fix(settings): re-key service configs and overrides stored under the old Azure/GCP service ids #1551, and main is at 000148. golang-migrate only moves forward from the DB's current version. So if this PR merges and deploys before fix(config/db): settled-only expiry cleanup, RI-exchange limit CHECKs, capped list reads (#120 slice) #1552, any DB that reaches 150 will never apply fix(config/db): settled-only expiry cleanup, RI-exchange limit CHECKs, capped list reads (#120 slice) #1552's 000149. Merge fix(config/db): settled-only expiry cleanup, RI-exchange limit CHECKs, capped list reads (#120 slice) #1552 first, or renumber whichever PR lands second.
• Owner decision. Behavior change, internal/api/handler_ri_exchange.go getRIUtilization: unrestricted (admin) sessions now resolve the deployment account on every call, which costs an STS call plus a ListCloudAccounts. The read now fails when the running AWS account isn't registered but other AWS accounts are (resolveAWSCloudAccountID returns an error for that case). On such a deployment the RI Exchange page's utilization column, loadUtilization in frontend/src/riexchange.ts, goes empty for admins, with only a console.error. Before this PR it worked. The PR body discloses the change. Reshape already failed this way, and the scheduler only collects ambient recs when no accounts are registered, so this deployment shape is already half-broken. Still, it's a regression in exchange for a latent fix: the issue itself says this isn't exploitable today. chore(config/db): medium/low findings from the 2026-07-28 full review #120 also suggested an alternative that avoids it: key the cache by the ambient identity's STS account number (resolveAWSAccountID), which doesn't fail for an unregistered host. I'd accept either. The owner should pick one knowingly.
• internal/accountpredicate: the refactor deliberately keeps the existing drift. Analytics still uses ProviderExact, while config uses the case-insensitive gate. That fits the issue's fix direction (one builder, so the next fix is one edit), but the two pages can still disagree on rows stored as "AWS" or "". Worth a follow-up to decide whether analytics should switch to the fix(config): account-filter provider gate drops rows with empty/wrong-case provider + NULL cloud_account_id #24 gate.

Attacked:

• Refactor equivalence: compared accountpredicate.Build line by line with all three removed copies. SQL text, bind order, both-empty and all-providers-empty handling are identical, and each caller keeps its own "no filter" result ("TRUE", or conds unchanged).
• Fail-before: in the scratch tree, keyed the handler's cache with a blank account and dropped the account from riUtilizationFlightKey. TestGetRIUtilization_AccountsInSameRegionDoNotShareCache, TestRIUtilizationCache_SingleflightKeyedByAccount and TestRIUtilizationFlightKey_IncludesEveryKeyField all failed. Restored afterwards.
• Migration 000150: read up and down. The LOCK runs inside golang-migrate's implicit multi-statement transaction. The table is a small cache, so there's no long lock on a big table. The down empties the table before restoring the narrower PK. TestMigration_RIUtilizationCacheAccountKey exercises up, down and re-up against real Postgres.
• Rolling deploy: checked the old release's behavior. Its upsert fails and is only logged; its read is still single-identity.
• Commands: go build ./...; go vet with and without -tags integration on accountpredicate, analytics, api, config, mocks and migrations; go test -race -short -count=1 on accountpredicate, analytics, api, config and mocks (all ok). -race -tags integration: the migrations TestMigration_RIUtilizationCacheAccountKey, the api RIUtilizationCache_Integration_|ReshapeRecommendations_Integration_, the config predicate and RI utilization tests, and the whole analytics package (all ok).

Body-stale: none

_{Reviewer: rG4. 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.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 68604c1a2c073b14fc1db920e3d3c4e008995f2a

Blocking:

• none

Non-blocking:

• Unchanged from my earlier review of cb257357 on this PR. See that comment for the full findings.

Attacked: The head moved from cb257357 to 68604c1a after my earlier review. The only new commit is fix(deps): bump golang.org/x/net to v0.62.0. Its diff (go.mod/go.sum, 3+/3-) is byte-identical to #1564's and to the same commit on #1548, #1560 and #1556, and the PR's own changes are untouched. Re-checked on 68604c1a: go mod verify (all modules verified) and go build ./.... Also ran go vet and go test -short -count=1 on accountpredicate, analytics and config: ok.

Body-stale: the body doesn't mention the x/net bump (non-blocking).

_{Reviewer: rG4. 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.}