[IAmJSD]

Summary

#806 and #792 already added the zonal EC2 AZ, platform and tenancy to the recommendations natural key, rec ID, identity key and idempotency key. What #408 still lists as missing is EC2 scope and RDS az_config.

• RDS az_config (fixed here). Cost Explorer reports Single-AZ and Multi-AZ RDS reservations of the same class, engine, term and payment as two separate recommendations (parseRDSDetails maps DeploymentOption to single-az/multi-az). They are different offerings, but both got the same natural key, rec ID and identity key. That breaks three things on main:
  1. Storage. UpsertRecommendations writes the whole multi-provider batch in one statement. When the batch has both variants it fails with SQLSTATE 21000 ("ON CONFLICT DO UPDATE command cannot affect row a second time"), so the whole collection write is lost, not just one rec.
  2. Purchase pricing. Once both rows are stored, loadStoredRecommendationIndex refuses every purchase for that provider with "more than one row for identity key".
  3. Idempotency. A Multi-AZ submit right after a Single-AZ one dedupes into it.
• EC2 scope (no change needed). The AWS parser derives scope from the AZ (resolveEC2Scope: zonal exactly when CE gives an AZ). The AZ is already in every identity, so two recs cannot differ only by scope. Azure collects Shared-scope recs only (properties/scope eq 'Shared') and GCP does not set scope. The purchase guard computeDetailMismatch still compares scope as a backstop.
• Provider offering filters. I checked these against the extended identity. RDS findOfferingID already filters by AZConfig and refuses an empty one, and EC2 filters already use platform, tenancy, scope and AZ, so no provider change is needed.

Fix (four commits, expand/contract like the #792/#806 chains)

1. feat(db) migration 000140 (expand). Adds:

   • a generated az_config column, COALESCE(payload->'details'->>'az_config', ''), verbatim. The table rewrite backfills it, so no data migration is needed.
   • recommendations_natural_key_az_tp_azc_idx: the 11 columns plus az_config.
   • recommendations_natural_key_source_azc_idx: 000120's source-account key plus az_config. This is needed because any unique index without az_config still rejects the pair. See the feat(insurance): per-recommendation Archera quotes (blocked on an Archera API mapping) #799 note below.

   The old indexes stay, so the current upsert keeps working.

2. fix(purchase): new RecommendationRecord.DetailsAZConfig(), pinned to the generated column by shared vectors. az_config now goes into:

   • RecIdentityKey. This covers the pricing index, planstep and the ramp-baseline netting.
   • the rec ID, appended as |<az_config> only when set, so EC2, cache and other recs keep their IDs.
   • purchaseIdempotencyKey.
   • describeRec.

3. fix(config): the upsert ON CONFLICT targets the 12-column index. This needs 000140.

4. fix(db) migration 000141 (contract). Drops the 11-column index and 000120's recommendations_natural_key_source_idx. Each has a strictly finer replacement from 000140, so no existing row can violate the new indexes. The down migration collapses az_config-only duplicates to the newest row (the table is a regenerable cache) and recreates both indexes exactly.

Prod deploy order (auto_migrate=false)

1. Dispatch database-migration.yml for 000140 only (steps counted from the live prod version).
2. Release this code.
3. Dispatch 000141 only after step 2 is live everywhere. Old code's 11-column ON CONFLICT needs the index that 000141 drops, so never run steps=0 before the release.

Dev auto-migrates at startup, so the window there is one deploy. If a maintainer prefers separate PRs as in #808/#810/#811, the four commits split cleanly.

Behavior changes to know about

• The IDs of RDS and Azure SQL recs whose Details carry az_config change once, on the next collection. A stale browser tab gets the usual 409 ("not in the current recommendation set").
• An RDS purchase request without Details can no longer match a stored row that has az_config, so it gets a 409. This is the same rule fix(purchase): carry EC2 platform and tenancy in rec ID, identity key and idempotency key (#806) #817 introduced for EC2 platform/tenancy, and the web modal echoes the listed rec's Details. I did not re-audit external API clients beyond what fix(purchase): carry EC2 platform and tenancy in rec ID, identity key and idempotency key (#806) #817 recorded.
• Old purchase_executions rows: the identity and idempotency keys are recomputed from each row's persisted Details on both sides, so in-flight rows still hash and match consistently. Rows from before fix(db): redact bound query arguments at every log level #453, which have no Details, read az_config as "", the same as fix(purchase): carry EC2 platform and tenancy in rec ID, identity key and idempotency key (#806) #817.

Interaction with the #799 source-account chain

Migration 000120 (#881) added recommendations_natural_key_source_idx as a prerequisite and did not switch the upsert to it. That index lacks az_config, so it would keep rejecting the Single-AZ/Multi-AZ pair. This PR replaces it with recommendations_natural_key_source_azc_idx, the same key plus az_config. When #799 later switches to a source-aware upsert, it should target that index. 000120's down-migration guard still works, because rolling back past 000141 recreates both old indexes first.

Verification

All runs were on macOS with Go 1.26 and real Postgres 17 through testcontainers. This is local unit, mock and integration evidence on synthetic fixtures. I did no real-cloud or real-account verification: a live CE response with both RDS deployment options was not exercised. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

Fail before / pass after

• Store: go test -tags integration ./internal/config/ -run 'AZConfig(Batch|Concurrent)' on the main schema and store code, with the index precondition disabled, fails in every batch case with failed to insert recommendations batch: ERROR: ON CONFLICT DO UPDATE command cannot affect row a second time (SQLSTATE 21000). It passes after the fix, six repeated runs.
• Pricing, identity and idempotency: with az_config removed from RecIdentityKey and purchaseIdempotencyKey (pre-fix behavior):
  • TestPriceRecommendationsFromStore_PricesFromRequestedAZConfig fails with stored recommendations for aws contain more than one row for identity key ....
  • TestPriceRecommendationsFromStore_RequestWithoutAZConfigRefusedForRDS, TestPurchaseIdempotencyKey_DiffersByAZConfig and TestRecIdentityKey_DiffersByAZConfig fail.
  • All of them pass after the fix.
• Rec ID: without the scheduler change, TestConvertRecommendations_RDSAZConfigInID shows both variants getting aws||rds|us-east-1|db.r5.large|mysql|1|all-upfront, and TestRecommendationID_MalformedAZConfigErrors fails. Both pass after the fix.

Migrations (go test -tags integration ./internal/database/postgres/migrations/ -run 'TestMigration_(RecommendationsAZConfig|DropRecommendations11ColNaturalKey|RecommendationsSourceAccount|DropRecommendations9ColNaturalKey|RecommendationsPlatformTenancy)$'): all pass. They cover:

• the generated-column vectors, which mirror the Go vectors;
• the old 11-column upsert still working after 000140;
• the known 23505 between 000140 and 000141;
• 000141 letting the pair coexist while exact duplicates are still rejected;
• the down migration's dedupe and exact index recreation;
• re-applying after rollback.

Concurrency. TestPostgresStore_UpsertRecommendations_AZConfigConcurrent runs 8 concurrent collections re-upserting the stored pair. All succeed and exactly one row per az_config remains.

Suites and checks

• go build ./...
• go vet on the touched packages, with and without -tags integration
• go test ./internal/mocks/ ./internal/config/ ./internal/scheduler/ ./internal/api/ ./internal/purchase/ ./internal/planstep/ ./internal/database/... -count=1
• go test -tags integration for ./internal/config/, ./internal/scheduler/, ./internal/api/, ./internal/purchase/ and ./internal/database/postgres/migrations/. One test needed a fixture bump: TestApproveAzurePricingRequiresMigrationAndRecollection now migrates to 140, because the upsert needs the new column.
• golangci-lint v2.10.1 with --allow-parallel-runners, with and without --build-tags integration: 0 issues
• gocyclo -over 10 on the changed non-test files: silent
• scripts/gosec-hook.sh: clean
• git secrets --scan in an isolated temp repo with the changed files tracked: clean. A planted fake key was flagged.
• scripts/test-git-secrets-allowlist.sh: 137 PASS
• git diff --check: clean

Out of scope / noted

• Pre-existing race on main, not caused by this PR. The non-arbiter unique source index from 000120 makes two collections that insert the same new row at the same instant fail with 23505 on that index. Postgres only routes conflicts on the arbiter index to DO UPDATE. I reproduced this on unmodified origin/main with 8 concurrent single-row upserts: it failed 1 run in 4 on recommendations_natural_key_source_idx. This PR keeps an equivalent index (..._source_azc_idx), so the race is unchanged. The concurrency test seeds the rows first so that it covers the steady-state re-collection path. The collection single-flight marker makes this rare. It could be fixed by making the source index non-unique until feat(insurance): per-recommendation Archera quotes (blocked on an Archera API mapping) #799 switches the arbiter, which is the owner's decision.
• TestMigration_ExecutionsDetachKeepsUpdatedAt (000128, purchase_executions) failed once during a full migrations-suite run and passed 3/3 in isolation. It is time-based and unrelated to this change.
• The approval email and other displays do not show az_config, which is display-only.
• Purchase suppression keys are untouched (fix(purchase): scope purchase suppressions to the AZ, platform and tenancy bought #1110 is reworking them for AZ, platform and tenancy).

Frontend follow-up for the UI owner

• cellKey (frontend/src/recommendations.ts) groups recs by physical cell. It includes the AZ and the platform/tenancy pair, but not az_config. Single-AZ and Multi-AZ RDS rows of the same class therefore render as variants of one cell. Purchases are still correct, because samePurchaseVariantIdentity already compares az_config for RDS, but they should be separate cells, the same way fix(frontend): include platform and tenancy in EC2 recommendation cell key #818 did it for EC2 platform/tenancy.

Closes #408

Maintainer: please mirror the issue's triage labels on this PR: priority/p2, type/chore, triaged, severity/low, urgency/this-quarter, impact/few, effort/m.

🤖 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.}

[cristim]

Independent review 2 (claude-fleet, cold): REQUEST_CHANGES a035c5d

Findings:

• [blocking] Purchase suppression is still keyed on the old identity, without az_config.
  • The sites are internal/config/recommendation_identity.go:100 (SuppressionVariant), internal/config/types.go:754 and internal/scheduler/scheduler.go:1452 on the merged tree. The key is the 6-tuple plus AZ, platform and tenancy.
  • fix(purchase): scope purchase suppressions to the AZ, platform and tenancy bought #1110 (migration 000134) is now on main, so the body's "suppression keys untouched (fix(purchase): scope purchase suppressions to the AZ, platform and tenancy bought #1110 is reworking them)" is out of date.
  • Before this PR, Single-AZ and Multi-AZ rows could never both be stored, so this path was unreachable. With the PR they coexist.
  • So buying N Single-AZ RIs subtracts N from (or hides) the Multi-AZ sibling for the whole grace period. That is an under-buy, or a refused buy.
  • Reproduced on the merged tree: a scratch TestR1239_SuppressionCrossAZConfig FAILS with "multi-az rec was suppressed by a single-az purchase".
  • This is the one identity path left on the old key.
  • Fix: add az_config to SuppressionVariant, and to the purchase_suppressions variant columns and unique key (a new migration). Alternatively, gate 000141 until that lands.
• [non-blocking, process] Migration numbering: this PR adds 000140/000141, while main is at 000134.
  • The gap 135-139 is legal, but golang-migrate never applies a lower number once a database is at 141.
  • The migration chain fix(auth): bind group account scope entries to account IDs #1116 (135) -> fix(plans): migrate stored pre-#608 Azure/GCP service ids to canonical slugs #1140 (136) -> fix(db): let ri_exchange_history.account_id hold Azure and GCP identifiers #1170 (137) -> fix(auth): reject sessions issued before the user's last password change #1197 (138) -> chore(db): backfill purchase_history.cloud_account_id from (provider, external_id) #1201 (139) is still open.
  • So this must merge after all of them, or be renumbered at merge. Otherwise those five would be silently skipped in every environment that already has 141.
• [non-blocking, disclosed] Azure SQL rec IDs can change between collections. Azure SQL recs also carry az_config, derived from a cached "dominant" server-inventory value that can flip between "", "none" and "zoneRedundant".
  • So Azure SQL rec IDs and identity keys can change between collections, giving a 409 on a stale tab.
  • Pricing was already refused on a mismatch (computeDetailMismatch, purchase_pricing.go:330), so this adds no new refusal path.
• [non-blocking] Key matching: DetailsAZConfig uses json.Unmarshal, which matches keys case-insensitively, while the generated column matches exactly. They diverge only on hand-crafted Details.
• [non-blocking, disclosed follow-up] Dashboard grouping: handler_dashboard.go:285 recCellKey and the frontend cellKey still group Single-AZ and Multi-AZ into one cell. This affects display and rollup only.

Attacked:

• Identity sites, on the merged tree:
  • RecIdentityKey (the pricing index, planstep resolve.go:237/324/339 and the ramp baseline store_postgres_ramp_baseline.go:159): az_config is included, and both sides are recomputed from persisted Details.
  • The rec ID (scheduler.go:1148): included.
  • The upsert ON CONFLICT: 12 columns, matching the 000140 index.
  • purchaseIdempotencyKey (handler_purchases.go:3378): included, and recomputed on both sides of matchDuplicateInList.
  • Suppression: missing (the blocking finding above).
  • Rec IDs are not persisted anywhere else.
• Mixed-key window:
  • The generated column's table rewrite backfills stored rows. A legacy multi-az row inserted at 134 reads az_config=multi-az after the up migration, and an EC2 row reads ''.
  • Between the release and 000141, a stored pair still fails with 23505 on the old index. Before this PR the same batch failed with 21000, so this is no regression.
  • Deploying the code before 000140 breaks, because the ON CONFLICT needs the column. The documented deploy order covers this.
• Non-RDS: rec IDs for these cases are byte-identical between base and merged:
  • EC2 zonal, and EC2 with platform/tenancy;
  • ElastiCache and Savings Plans;
  • RDS without az_config;
  • Azure VM and GCP.
• Fail-before:
  • The head scheduler identity tests FAIL on base (RDSAZConfigInID, MalformedAZConfigErrors).
  • The AZConfigBatch store test FAILS on base plus 000140 only.
• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  • M1, az_config dropped from RecIdentityKey: caught by RecIdentityKey_DiffersByAZConfig and both PriceRecommendationsFromStore tests.
  • M2, dropped from the idempotency key: caught.
  • M3, the rec ID suffix dropped: caught.
  • M4, ON CONFLICT reverted to 11 columns: caught by Batch and Concurrent.
  • M5, 000141 keeps the 000120 source index: caught by the store and migration tests.
  • M6, COALESCE removed from the generated column: caught by both migration tests and the store test.
  • No test covers the suppression gap.

Verified locally on the merged tree (origin/main 6af022f plus head, with merge-tree clean), with GOTOOLCHAIN=go1.26.10 and no cloud credentials, under the heavy lock, with testcontainers Postgres:

• Build, vet and unit tests:
  • go build ./...: OK.
  • go vet on config, scheduler, api, purchase, planstep and migrations, with and without -tags integration: OK. gofmt: clean.
  • go test -count=1 on config, scheduler, api, purchase, planstep and mocks: ok.
• Migration integration tests (-tags integration ./internal/database/postgres/migrations/): PASS.
  • RecommendationsAZConfig and DropRecommendations11ColNaturalKey.
  • The neighbouring tests: RecommendationsPlatformTenancy, DropRecommendations9ColNaturalKey, RecommendationsSourceAccount, PurchaseSuppressionsVariant and DownAllRoundTrip.
• Up/down/up cycle: two full cycles of 134 -> 141 -> 134 -> 141 in a scratch test.
  • After each down, the index set equals the pre-140 set and the az_config column is gone.
  • The pair collapses to the newest row.
  • An exact EC2 duplicate is still rejected at 141.
• Other integration tests:
  • ./internal/config -run 'AZConfig|UpsertRecommendations|Suppression': ok.
  • ./internal/purchase -run TestApproveAzurePricingRequiresMigrationAndRecollection: ok.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[IAmJSD]

Merged origin/main (1c5787a) into this branch. New head: 1fa7665.

Conflict resolution:

• internal/config/store_postgres_recommendations.go: main (fix(db): remove ReplaceRecommendations and its unscoped DELETE #1209) removed ReplaceRecommendations and the onConflict flag, so the natural-key ON CONFLICT clause is now unconditional. This branch had switched that clause to the 12-column key with az_config. I kept main's unconditional upsert, pointed it at the 12-column az_config arbiter, and kept this branch's doc comment.

I found no semantic conflicts. Nothing on the branch calls ReplaceRecommendations. The new tests build alongside #1400's TestMain files. Migrations 000140 and 000141 do not clash with main, which is at 000139.

Checks on the merged head:

• build, vet (also with -tags integration) and e2e vet pass.
• golangci-lint v2.10.1 found 0 issues with and without the integration tag, and gocyclo is silent.
• go test -race -short passes on the changed packages, and so does go test -race -tags integration (api, config, migrations, purchase, scheduler).
• The secrets scan is clean and the planted key was flagged. The allowlist test passes.

[IAmJSD]

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

Blocking:

• internal/config/recommendation_identity.go:100 (SuppressionVariant), internal/config/types.go:754 and internal/scheduler/scheduler.go:1452 (matchingSuppressions): purchase suppression is still keyed on the 6-tuple plus AZ/platform/tenancy, without az_config. I missed this in my own pass. It was first raised in cristim's earlier review on a035c5d5, it is unaddressed on this head (the only change since is a main merge), and I reproduced it independently.
  • Before this PR, a Single-AZ and a Multi-AZ RDS row of the same class could never both be stored. This PR makes them coexist, and that is the point of the PR.
  • So buying N Single-AZ RIs now builds a suppression whose variant ({"" "" ""}) also AppliesTo the Multi-AZ sibling. The sibling's count drops by N, or it is hidden, for the whole grace period. They are different offerings covering different DB instances, so that is a silent under-buy. fix(purchase): suppression key lacks availability zone, platform and tenancy #816/fix(purchase): scope purchase suppressions to the AZ, platform and tenancy bought #1110 fixed exactly this for EC2 platform/tenancy.
  • Repro on this head merged with main: a scratch test builds config.BuildSuppressions from a 2x single-az purchase and runs applySuppressionIndex over a 2x multi-az rec with the same 6-tuple. The multi-az rec is dropped (out=[]).
  • Fix: add az_config to SuppressionVariant, suppressionBucketOf, and the purchase_suppressions variant columns and unique key. That is a new migration, 000149 or above. Alternatively, hold 000141 until it lands.

Non-blocking:

• Deploy ordering is load-bearing in both directions, and nothing in code enforces it. I reproduced each failure mode:

  • New code before 000140: ON CONFLICT (..., az_config) has no arbiter, so every collection write fails with SQLSTATE 42P10.
  • Old code after 000141: the 11-column arbiter is gone, also 42P10. I hit this by running the merge-base store_postgres_recommendations.go against the migrated schema: TestPostgresStore_UpsertRecommendations_AZConfigBatch fails with 42P10.
  • New code between release and 000141: if a stored RDS row is single-az and Cost Explorer now returns only the multi-az variant of the same class, the 12-column arbiter sees no conflict. The INSERT then violates the still-present 11-column index (23505), and the whole multi-provider batch write is lost. On main that case silently overwrote the row.

  Dev auto-migrates both at once, so there is no window there. For prod, the 140, then release, then 141 runbook in the PR body has to be followed exactly, with 141 dispatched promptly after the release. A schema-version guard in UpsertRecommendations (like azurePricingMigrationVersion) would turn the first mode into a clear error, but that is optional.

• API contract change: an RDS (or Azure SQL) purchase request without details.az_config no longer matches a stored row that has one, and gets a 409. The web modal echoes Details. Any external or MCP/CLI client that submits RDS purchases without Details will start failing. This is disclosed, and it is consistent with fix(purchase): carry EC2 platform and tenancy in rec ID, identity key and idempotency key (#806) #817's EC2 rule.

• purchaseIdempotencyKey now appends |<az_config> for every rec, so every submit fingerprint changes on deploy. That is harmless: the fingerprint is only compared against keys recomputed from pending rows (matchDuplicateInList) and is never persisted (the idempotency_key column is the UUID lineage key, see purchase/execution.go). So in-flight dedupe stays consistent across the deploy.

• From the earlier review: DetailsAZConfig uses json.Unmarshal, which matches keys case-insensitively, while the generated column matches exactly. They diverge only on hand-crafted Details. Also, Azure SQL az_config is derived from a cached dominant server-inventory value, so Azure SQL rec IDs can change between collections, which gives a 409 on a stale tab.

• The frontend cellKey still groups Single-AZ and Multi-AZ RDS into one cell. The body flags this as a UI follow-up.

Attacked:

• Migrations. 140/141 are the numbers reserved for fix(recommendations): key stored recs, rec ID and purchase identity on RDS az_config (#408) #1239, and main is at 000139. The 000140 generated column uses the same COALESCE(payload->'details'->>'az_config','') that DetailsAZConfig mirrors. In 000141 down, deduping on the 11-column key also clears duplicates under the finer 000120 source key, and the down migration recreates both indexes byte-for-byte. 000120's down guard still holds because 141-down runs first. The table rewrite is on a regenerable cache table.

• Identity consumers: RecIdentityKey (pricing index, planstep, ramp-baseline netting), recommendationID, purchaseIdempotencyKey. Each is recomputed from persisted Details on both sides. No rec ID is persisted anywhere (no recommendation_id references in config). ComputeIdentity and DetailsAZConfig share the decode and error shape.

• Ran: go build ./...; go vet (plain and -tags integration) on api/config/scheduler/purchase/database; go test -race -short -count=1 on api/config/scheduler/planstep/database (ok); go test -race -tags integration on the targeted migration tests (000109/111/120/140/141 and the 9-col drop: all pass), config Recommendation|AZConfig|RampBaseline|Upsert, purchase Azure|AZConfig|Pricing, api Purchase|Recommendation|Pricing, and the whole scheduler package (all ok). The full internal/database/postgres/migrations integration suite passes on a local merge with current main (81 commits ahead of the merge base, clean merge; build, vet and unit also pass there). golangci-lint v2.10.1 with and without the integration tag: 0 issues. gocyclo -over 10 on the changed files: silent.

• Fail-before: with the merge-base scheduler.go and handler_purchases.go, and az_config blanked out of RecIdentityKey, these fail:

  • TestPriceRecommendationsFromStore_PricesFromRequestedAZConfig, ..._RequestWithoutAZConfigRefusedForRDS
  • TestPurchaseIdempotencyKey_DiffersByAZConfig
  • TestRecIdentityKey_DiffersByAZConfig
  • TestConvertRecommendations_RDSAZConfigInID, TestRecommendationID_MalformedAZConfigErrors

  Restored afterwards.

Body-stale:

• "Purchase suppression keys are untouched (fix(purchase): scope purchase suppressions to the AZ, platform and tenancy bought #1110 is reworking them for AZ, platform and tenancy)": fix(purchase): scope purchase suppressions to the AZ, platform and tenancy bought #1110 merged on 2026-10-23 (migration 000134). The suppression variant now exists on main, and it is the one identity path this PR leaves without az_config (see Blocking).

_{Reviewer: rB22. 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]

Migration renumbering needed before this merges. The reviewed, ready PRs have taken 000140-000145 so they could merge without waiting on owner decisions: #1240 (000140), #1302 (000141), #1268 (000142), #1287 (000143), #1289 (000144), #1116 (000145). This PR's migration(s) (000140, 000141) must be renumbered to start at 000146, or the next free number at that time, when it's ready to merge (for example 000140 -> 000146 and 000141 -> 000147, keeping the expand/contract pair adjacent and in order). golang-migrate skips a version below the database's current one, so a lower number merged after those would never apply.

Dependency check: none of the six ready migrations depends on this PR's schema. #1240's recommendations_scan_key_idx is a non-unique index on (COALESCE(payload->>'id','') COLLATE "C", id). It doesn't involve az_config or the natural-key indexes, and this PR's generated-column table rewrite just rebuilds it. From reading the SQL, nothing in the ready set gets in the way of this PR applying after them. That ordering hasn't been run, though.

[cristim]

VERDICT: REQUEST_CHANGES 1fa7665

Re-review of #1239. The previous verdict was REQUEST_CHANGES at a035c5d. The only change since then is the merge 1fa7665 of origin/main 1c5787a (git log --no-merges a035c5d5..1fa76651 ^origin/main is empty). The PR's own diff against origin/main is unchanged apart from the conflict resolution in store_postgres_recommendations.go, which now uses main's unconditional upsert with the 12-column az_config arbiter. That resolution is correct.

Findings:

• [blocking] internal/database/postgres/migrations/000140_recommendations_az_config..sql, 000141_drop_recommendations_11col_natural_key..sql: the migration versions collide with main.
  • origin/main (fetched 2026-10-26) now contains 000140_recommendations_scan_key_idx and 000141_savings_snapshots_commitment_type_vocabulary. It goes up to 000147_config_audit_events.
  • So this PR's numbers are not above the highest on main. They duplicate two existing versions.
  • git merge-tree reports no textual conflict because the file names differ, so the merge would go through silently.
  • Reproduced: I loaded the merged tree (429dacb5) through golang-migrate v4.20.1 iofs.New. It fails with duplicate migration file: 000140_recommendations_scan_key_idx.down.sql (source/iofs/iofs.go:68-72). Every migrate run, in every environment and in CI integration tests, would fail at startup.
  • The non-integration migration package tests still pass, so unit CI would not catch it.
  • Fix: renumber 000140 -> 000148 and 000141 -> 000149, or the next free pair at merge time, keeping the expand/contract pair adjacent. Update these with it:
    • the version references in the tests and comments (store_postgres_recommendations.go comment, DetailsAZConfig doc, the migration test names and files);
    • the 140-release-141 runbook in the PR body.
  • This confirms the author's 2026-10-26 comment. Its target of "000146" is itself already taken on main (000146_history_ramp_step_execution_error_not_null, 000147_config_audit_events).
• [blocking, carried over from a035c5d, NOT fixed] internal/config/recommendation_identity.go:100-105 (SuppressionVariant), internal/config/types.go:754, internal/scheduler/scheduler.go:1451-1452: purchase suppression still has no az_config.
  • The variant is still {AvailabilityZone, Platform, Tenancy}.
  • So with Single-AZ and Multi-AZ RDS rows now coexisting, buying N Single-AZ RIs suppresses N from the Multi-AZ sibling, or hides it, for the grace period. That is an under-buy.
  • The author's own independent review (2026-10-25T23:52) reproduced this as well.
  • Fix: as before, add az_config to SuppressionVariant, suppressionBucketOf, and the purchase_suppressions variant columns and unique key. That needs a new migration, numbered after the renumbered pair. Alternatively, hold the contract migration until that lands.
• [non-blocking, carried over, unchanged] These are all as described at a035c5d and in the author's review:
  • deploy ordering (expand, then release, then contract) is not enforced in code;
  • RDS purchases without details.az_config now get a 409;
  • Azure SQL az_config can flip between collections;
  • DetailsAZConfig uses case-insensitive json.Unmarshal, while the generated column matches keys exactly;
  • the dashboard cellKey/recCellKey still group Single-AZ and Multi-AZ.

Attacked:

• Delta a035c5d..1fa7665: no author commits. The merge's resolution of store_postgres_recommendations.go matches main's fix(db): remove ReplaceRecommendations and its unscoped DELETE #1209 shape, with the ON CONFLICT on the 12 columns including az_config. The diff against origin/main is 16 files, +738/-12.
• Earlier findings re-checked at the head:
  • suppression: still missing (finding 2);
  • migration ordering: now an outright version collision (finding 1);
  • the other non-blocking items: unchanged.
• Migration numbering:
  • the highest version on origin/main is 000147;
  • this PR adds 000140 and 000141, both duplicates;
  • golang-migrate's duplicate rejection was confirmed by running iofs.New on the merged tree.
• Merged-tree build: go test -count=1 ./internal/database/postgres/migrations/ (unit) is ok. That shows the collision is invisible without the integration tag or a real migrate run.
• Not repeated this round: mutation probes M1-M6 and the integration suite from the a035c5d review. The PR code is unchanged since then, and the gating finding above makes the merged tree unmigratable anyway. Re-run them after the renumbering.