From 5c408e0fad44c3f195274f3032f3a313e8271610 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 22 Oct 2026 01:42:41 +0100 Subject: [PATCH 1/4] feat(db): add generated az_config column and 12-column recommendations index (#408) Migration 000140 adds a generated az_config column, read verbatim from payload->'details'->>'az_config' (RDS single-az/multi-az, Azure SQL zone redundancy, '' otherwise), and two unique indexes that include it: the 11-column natural key plus az_config (the new upsert arbiter) and the 000120 source-account key plus az_config. The existing 11-column and 000120 source indexes are kept, so the current upsert keeps working and this can be applied before the store switch. Co-Authored-By: Claude Opus 5.5 --- .../000140_recommendations_az_config.down.sql | 4 + .../000140_recommendations_az_config.up.sql | 44 ++++++ .../000140_recommendations_az_config_test.go | 132 ++++++++++++++++++ 3 files changed, 180 insertions(+) create mode 100644 internal/database/postgres/migrations/000140_recommendations_az_config.down.sql create mode 100644 internal/database/postgres/migrations/000140_recommendations_az_config.up.sql create mode 100644 internal/database/postgres/migrations/000140_recommendations_az_config_test.go diff --git a/internal/database/postgres/migrations/000140_recommendations_az_config.down.sql b/internal/database/postgres/migrations/000140_recommendations_az_config.down.sql new file mode 100644 index 000000000..23050ab14 --- /dev/null +++ b/internal/database/postgres/migrations/000140_recommendations_az_config.down.sql @@ -0,0 +1,4 @@ +DROP INDEX IF EXISTS recommendations_natural_key_source_azc_idx; +DROP INDEX IF EXISTS recommendations_natural_key_az_tp_azc_idx; + +ALTER TABLE recommendations DROP COLUMN IF EXISTS az_config; diff --git a/internal/database/postgres/migrations/000140_recommendations_az_config.up.sql b/internal/database/postgres/migrations/000140_recommendations_az_config.up.sql new file mode 100644 index 000000000..ed47c2790 --- /dev/null +++ b/internal/database/postgres/migrations/000140_recommendations_az_config.up.sql @@ -0,0 +1,44 @@ +-- Migration 000140: az_config generated column and a 12-column unique index on +-- recommendations (#408, expand step of the RDS AZ-config identity work). +-- +-- Cost Explorer reports Single-AZ and Multi-AZ RDS reservations of the same +-- instance class, engine, term and payment as separate recommendations. They +-- are different offerings with different prices, but the 11-column natural key +-- cannot tell them apart, so a batch carrying both fails the whole collection +-- write with SQLSTATE 21000. This migration adds the AZ configuration to the +-- schema; nothing uses it yet. +-- +-- The column is derived verbatim from the stored payload (the JSON of the +-- RecommendationRecord) at payload->'details' (the DatabaseDetails blob), so +-- existing rows are backfilled by the table rewrite with no data migration. +-- No trimming or case-folding. Absent Details, JSON null, a missing key and a +-- null value all become ''. Every non-database row is ''. +-- +-- Two indexes are added: +-- - recommendations_natural_key_az_tp_azc_idx: the 11 columns plus az_config, +-- the new upsert arbiter. +-- - recommendations_natural_key_source_azc_idx: the source-account key of +-- migration 000120 (#799) plus az_config. Every unique index without +-- az_config rejects a Single-AZ/Multi-AZ pair, so the source-aware key has +-- to gain the column too; 000141 retires the 000120 index it replaces. +-- +-- The existing 11-column recommendations_natural_key_az_tp_idx and the +-- 000120 recommendations_natural_key_source_idx are KEPT, so the current +-- ON CONFLICT on the 11 columns keeps working for old and new code. Two rows +-- that differ only by az_config are still rejected by those indexes until +-- migration 000141 drops them (applied only once the store upserts on the +-- 12-column index). +-- +-- Lock impact: ADD COLUMN ... GENERATED ... STORED rewrites the table and the +-- index build blocks writes. The table is a regenerable cache re-populated +-- every collection cycle, so this is acceptable. CREATE INDEX CONCURRENTLY is +-- not possible: the migration runner wraps every migration in a transaction. +ALTER TABLE recommendations + ADD COLUMN IF NOT EXISTS az_config TEXT + GENERATED ALWAYS AS (COALESCE(payload->'details'->>'az_config', '')) STORED; + +CREATE UNIQUE INDEX IF NOT EXISTS recommendations_natural_key_az_tp_azc_idx + ON recommendations (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, az_config); + +CREATE UNIQUE INDEX IF NOT EXISTS recommendations_natural_key_source_azc_idx + ON recommendations (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, az_config, source_account_key); diff --git a/internal/database/postgres/migrations/000140_recommendations_az_config_test.go b/internal/database/postgres/migrations/000140_recommendations_az_config_test.go new file mode 100644 index 000000000..801dc4e9c --- /dev/null +++ b/internal/database/postgres/migrations/000140_recommendations_az_config_test.go @@ -0,0 +1,132 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/jackc/pgx/v5/pgconn" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const insertRecForMigration140 = ` + INSERT INTO recommendations + (collected_at, provider, service, region, resource_type, engine, payload, + upfront_cost, monthly_savings, term, payment_option) + VALUES (NOW(), 'aws', 'rds', 'us-east-1', $1, 'mysql', $2::jsonb, 0, 0, 1, 'all-upfront') +` + +// TestMigration_RecommendationsAZConfig locks down migration 000140 (issue +// #408, expand step): the generated az_config column is read verbatim from +// payload->'details', the 11-column index is kept so the previous upsert still +// works, the 12-column index is added, and rollback removes only the new +// column and index. The vectors are mirrored by TestDetailsAZConfig_Vectors +// (internal/config) so the Go reader cannot diverge from the column. +func TestMigration_RecommendationsAZConfig(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 140))) + + type vector struct{ payload, azConfig string } + vectors := map[string]vector{ + "details-absent": {`{"service":"rds"}`, ""}, + "details-null": {`{"details":null}`, ""}, + "details-empty": {`{"details":{}}`, ""}, + "az-config-null": {`{"details":{"az_config":null}}`, ""}, + "empty-string": {`{"details":{"az_config":""}}`, ""}, + "rds-single-az": {`{"details":{"az_config":"single-az"}}`, "single-az"}, + "rds-multi-az": {`{"details":{"az_config":"multi-az"}}`, "multi-az"}, + "azure-zone-redundant": {`{"details":{"az_config":"zoneRedundant"}}`, "zoneRedundant"}, + "verbatim-spaces": {`{"details":{"az_config":" Multi-AZ "}}`, " Multi-AZ "}, + "compute-details": {`{"details":{"platform":"Linux/UNIX","tenancy":"default"}}`, ""}, + // A top-level key is NOT consulted: only Details carries the field. + "top-level-ignored": {`{"az_config":"multi-az"}`, ""}, + } + for name, v := range vectors { + _, err = pool.Exec(ctx, insertRecForMigration140, name, v.payload) + require.NoError(t, err) + } + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 140)) + + for name, v := range vectors { + var got string + require.NoError(t, pool.QueryRow(ctx, `SELECT az_config FROM recommendations WHERE resource_type = $1`, name).Scan(&got)) + assert.Equal(t, v.azConfig, got, "az_config of %s", name) + } + + for _, idx := range []string{ + "recommendations_natural_key_az_tp_idx", "recommendations_natural_key_az_tp_azc_idx", + "recommendations_natural_key_source_idx", "recommendations_natural_key_source_azc_idx", + } { + var exists bool + require.NoError(t, pool.QueryRow(ctx, + `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE tablename = 'recommendations' AND indexname = $1)`, idx).Scan(&exists)) + assert.True(t, exists, "index %s must exist after 000140", idx) + } + var indexDef string + require.NoError(t, pool.QueryRow(ctx, + `SELECT indexdef FROM pg_indexes WHERE indexname = 'recommendations_natural_key_az_tp_azc_idx'`).Scan(&indexDef)) + assert.Contains(t, indexDef, "UNIQUE INDEX") + assert.Contains(t, indexDef, "(account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, az_config)") + require.NoError(t, pool.QueryRow(ctx, + `SELECT indexdef FROM pg_indexes WHERE indexname = 'recommendations_natural_key_source_azc_idx'`).Scan(&indexDef)) + assert.Contains(t, indexDef, "UNIQUE INDEX") + assert.Contains(t, indexDef, "(account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, az_config, source_account_key)") + + // The previous code's upsert (11-column ON CONFLICT) still works and + // updates the existing row in place, so 000140 can be applied before the + // store switch is released. + singleAZ := `{"details":{"az_config":"single-az"}}` + _, err = pool.Exec(ctx, insertRecForMigration140+` + ON CONFLICT (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy) + DO UPDATE SET upfront_cost = 5`, "pair", singleAZ) + require.NoError(t, err) + _, err = pool.Exec(ctx, insertRecForMigration140+` + ON CONFLICT (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy) + DO UPDATE SET upfront_cost = 5`, "pair", singleAZ) + require.NoError(t, err, "the existing 11-column upsert must keep working after 000140") + var n int + require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM recommendations WHERE resource_type = 'pair'`).Scan(&n)) + assert.Equal(t, 1, n) + + // Known limitation, resolved by 000141: a row differing only by az_config + // is still rejected by the kept indexes without az_config. + _, err = pool.Exec(ctx, insertRecForMigration140, "pair", `{"details":{"az_config":"multi-az"}}`) + var pgErr *pgconn.PgError + require.ErrorAs(t, err, &pgErr) + assert.Equal(t, "23505", pgErr.Code) + assert.Contains(t, []string{"recommendations_natural_key_az_tp_idx", "recommendations_natural_key_source_idx"}, pgErr.ConstraintName) + + // Rollback removes the new column and index only. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 140))) + var colExists bool + require.NoError(t, pool.QueryRow(ctx, + `SELECT EXISTS (SELECT 1 FROM information_schema.columns WHERE table_name = 'recommendations' AND column_name = 'az_config')`).Scan(&colExists)) + assert.False(t, colExists) + for idx, want := range map[string]bool{ + "recommendations_natural_key_az_tp_azc_idx": false, + "recommendations_natural_key_source_azc_idx": false, + "recommendations_natural_key_az_tp_idx": true, + "recommendations_natural_key_source_idx": true, + } { + var exists bool + require.NoError(t, pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE indexname = $1)`, idx).Scan(&exists)) + assert.Equal(t, want, exists, "index %s after rollback", idx) + } + require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM recommendations`).Scan(&n)) + assert.Equal(t, len(vectors)+1, n, "rollback must not delete rows") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 140)) +} From a15aff2abaac198097b0885023c862d1de403d7c Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 22 Oct 2026 01:42:41 +0100 Subject: [PATCH 2/4] fix(purchase): carry RDS az_config in rec ID, identity key and idempotency key (#408) Single-AZ and Multi-AZ RDS recommendations of the same class, engine, term and payment shared one rec ID and one identity key. Read az_config verbatim from Details (RecommendationRecord.DetailsAZConfig, pinned to the 000140 generated column by shared vectors) and add it to RecIdentityKey, the rec ID (appended only when set, so every other rec keeps its ID), the purchase idempotency key and describeRec. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_purchases.go | 10 ++- internal/api/purchase_azconfig_test.go | 88 +++++++++++++++++++ internal/api/purchase_pricing.go | 3 + internal/config/recommendation_identity.go | 33 ++++++- .../config/recommendation_identity_test.go | 56 ++++++++++++ .../scheduler/recommendation_identity_test.go | 34 +++++++ internal/scheduler/scheduler.go | 11 +++ 7 files changed, 230 insertions(+), 5 deletions(-) create mode 100644 internal/api/purchase_azconfig_test.go diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index af7c61b96..65e10cff4 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -3288,11 +3288,17 @@ func purchaseIdempotencyKey(creatorID string, recs []config.RecommendationRecord if err != nil { return "", err } - tuples = append(tuples, fmt.Sprintf("%s|%s|%s|%s|%s|%s|%d|%d|%s|%s|%s|%s", + // Single-AZ and Multi-AZ RDS buys of the same class are different + // purchases and must not dedupe into one (issue #408). + azConfig, err := r.DetailsAZConfig() + if err != nil { + return "", err + } + tuples = append(tuples, fmt.Sprintf("%s|%s|%s|%s|%s|%s|%d|%d|%s|%s|%s|%s|%s", strings.ToLower(strings.TrimSpace(r.Provider)), r.Service, r.Region, r.ResourceType, r.Engine, acct, r.Count, r.Term, strings.ToLower(strings.TrimSpace(r.Payment)), az, - ci.Platform, ci.Tenancy)) + ci.Platform, ci.Tenancy, azConfig)) } sort.Strings(tuples) h := sha256.New() diff --git a/internal/api/purchase_azconfig_test.go b/internal/api/purchase_azconfig_test.go new file mode 100644 index 000000000..b1572a18e --- /dev/null +++ b/internal/api/purchase_azconfig_test.go @@ -0,0 +1,88 @@ +package api + +import ( + "context" + "encoding/json" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Real RDS Details shape as persisted by the scheduler (issue #408). +func rdsDetails(azConfig string) json.RawMessage { + return json.RawMessage(`{"engine":"mysql","az_config":"` + azConfig + `","instance_class":"db.r5.large"}`) +} + +func rdsAZConfigRec(azConfig string, upfront float64) config.RecommendationRecord { + return config.RecommendationRecord{ + ID: "rec-rds-" + azConfig, Provider: "aws", Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large", + Engine: "mysql", Term: 1, Payment: "all-upfront", Count: 1, UpfrontCost: upfront, Savings: 10, + Details: rdsDetails(azConfig), + } +} + +// Stored Single-AZ and Multi-AZ rows of the same class must both load (no +// "more than one row for identity key" refusal) and the request, which carries +// az_config only in Details, is priced from its own variant's row. +func TestPriceRecommendationsFromStore_PricesFromRequestedAZConfig(t *testing.T) { + for _, want := range []struct { + azConfig string + upfront float64 + }{ + {"single-az", 100}, + {"multi-az", 300}, + } { + t.Run(want.azConfig, func(t *testing.T) { + store := new(MockConfigStore) + t.Cleanup(func() { store.AssertExpectations(t) }) + expectStoredRecs(store, rdsAZConfigRec("single-az", 100), rdsAZConfigRec("multi-az", 300)) + h := &Handler{config: store} + req := []config.RecommendationRecord{{ + Provider: "aws", Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large", Engine: "mysql", + Term: 1, Payment: "all-upfront", Count: 1, Details: rdsDetails(want.azConfig), + }} + require.NoError(t, h.priceRecommendationsFromStore(context.Background(), req)) + assert.Equal(t, want.upfront, req[0].UpfrontCost) + got, err := req[0].DetailsAZConfig() + require.NoError(t, err) + assert.Equal(t, want.azConfig, got) + }) + } +} + +// A request that does not name its az_config cannot be priced off an arbitrary +// RDS variant: it is refused with a 409. +func TestPriceRecommendationsFromStore_RequestWithoutAZConfigRefusedForRDS(t *testing.T) { + store := new(MockConfigStore) + t.Cleanup(func() { store.AssertExpectations(t) }) + expectStoredRecs(store, rdsAZConfigRec("multi-az", 300)) + h := &Handler{config: store} + req := []config.RecommendationRecord{{ + Provider: "aws", Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large", Engine: "mysql", + Term: 1, Payment: "all-upfront", Count: 1, + }} + err := h.priceRecommendationsFromStore(context.Background(), req) + require.Error(t, err) + ce, isClient := IsClientError(err) + require.True(t, isClient) + assert.Equal(t, 409, ce.code) +} + +func TestPurchaseIdempotencyKey_DiffersByAZConfig(t *testing.T) { + t.Parallel() + key := func(azConfig string) string { + return mustPurchaseIdempotencyKey("u", []config.RecommendationRecord{rdsAZConfigRec(azConfig, 1)}, 100) + } + assert.NotEqual(t, key("single-az"), key("multi-az"), + "a Multi-AZ submit right after a Single-AZ one must not dedupe into it") + assert.Equal(t, key("multi-az"), key("multi-az")) +} + +func TestDescribeRec_IncludesAZConfig(t *testing.T) { + r := rdsAZConfigRec("multi-az", 1) + assert.Contains(t, describeRec(&r), `az_config="multi-az"`) + bare := config.RecommendationRecord{Provider: "aws", Service: "rds"} + assert.NotContains(t, describeRec(&bare), "az_config=") +} diff --git a/internal/api/purchase_pricing.go b/internal/api/purchase_pricing.go index 0d3f76610..e270aa0b9 100644 --- a/internal/api/purchase_pricing.go +++ b/internal/api/purchase_pricing.go @@ -363,5 +363,8 @@ func describeRec(rec *config.RecommendationRecord) string { if ci, err := rec.ComputeIdentity(); err == nil && !ci.IsZero() { desc += fmt.Sprintf(" platform=%q tenancy=%q", ci.Platform, ci.Tenancy) } + if azConfig, err := rec.DetailsAZConfig(); err == nil && azConfig != "" { + desc += fmt.Sprintf(" az_config=%q", azConfig) + } return desc } diff --git a/internal/config/recommendation_identity.go b/internal/config/recommendation_identity.go index c278f85f4..86e8a8945 100644 --- a/internal/config/recommendation_identity.go +++ b/internal/config/recommendation_identity.go @@ -41,10 +41,33 @@ func (r *RecommendationRecord) ComputeIdentity() (ComputeIdentity, error) { return ComputeIdentity{Platform: d.Platform, Tenancy: d.Tenancy}, nil } -// RecIdentityKey is the tuple a price is a function of: the same eleven +// DetailsAZConfig reads the database AZ configuration (RDS "single-az" / +// "multi-az", Azure SQL "zoneRedundant" / "none") from the rec's Details +// blob, VERBATIM. Single-AZ and Multi-AZ RDS reservations are different +// offerings with different prices, yet share every other identity field +// (issue #408). It mirrors the generated az_config column of migration 000140 +// (COALESCE(payload->'details'->>'az_config', ”)) and is pinned to it by the +// shared vectors in TestDetailsAZConfig_Vectors: absent Details, JSON null, a +// missing key and a null value all yield "". Only DatabaseDetails carries the +// key. A non-string value is an error. +func (r *RecommendationRecord) DetailsAZConfig() (string, error) { + if len(r.Details) == 0 { + return "", nil + } + var d struct { + AZConfig string `json:"az_config"` + } + if err := json.Unmarshal(r.Details, &d); err != nil { + return "", fmt.Errorf("decode az_config of %s/%s details: %w", r.Region, r.ResourceType, err) + } + return d.AZConfig, nil +} + +// RecIdentityKey is the tuple a price is a function of: the same twelve // components (the ninth is the zonal EC2 availability zone, "" for every // other rec, issue #792; the tenth and eleventh are the EC2 platform and -// tenancy read from Details, "" when absent, issue #806) behind +// tenancy read from Details, "" when absent, issue #806; the twelfth is the +// database az_config read from Details, "" when absent, issue #408) behind // RecommendationRecord.ID, with a nil // and an empty CloudAccountID collapsed together (as purchaseConstraintSets // and the store's account_key both do). Provider and payment are lowercased @@ -54,6 +77,10 @@ func RecIdentityKey(rec *RecommendationRecord) (string, error) { if err != nil { return "", err } + azConfig, err := rec.DetailsAZConfig() + if err != nil { + return "", err + } account := "" if rec.CloudAccountID != nil { account = *rec.CloudAccountID @@ -61,6 +88,6 @@ func RecIdentityKey(rec *RecommendationRecord) (string, error) { return strings.Join([]string{ strings.ToLower(rec.Provider), account, rec.Service, rec.Region, rec.ResourceType, rec.Engine, strconv.Itoa(rec.Term), strings.ToLower(rec.Payment), - rec.AvailabilityZone, ci.Platform, ci.Tenancy, + rec.AvailabilityZone, ci.Platform, ci.Tenancy, azConfig, }, "\x1f"), nil } diff --git a/internal/config/recommendation_identity_test.go b/internal/config/recommendation_identity_test.go index 5f8fcf505..26c1372bf 100644 --- a/internal/config/recommendation_identity_test.go +++ b/internal/config/recommendation_identity_test.go @@ -45,3 +45,59 @@ func TestComputeIdentity_TopLevelKeyIgnoredAndMalformedFailsLoud(t *testing.T) { _, err = bad.ComputeIdentity() assert.Error(t, err) } + +// The vectors mirror TestMigration_RecommendationsAZConfig (migration 000140, +// internal/database/postgres/migrations) so the Go reader and the generated +// az_config column cannot diverge (issue #408). Details here is the details +// blob itself. +func TestDetailsAZConfig_Vectors(t *testing.T) { + vectors := []struct { + name, details, azConfig string + }{ + {"details-absent", ``, ""}, + {"details-null", `null`, ""}, + {"details-empty", `{}`, ""}, + {"az-config-null", `{"az_config":null}`, ""}, + {"empty-string", `{"az_config":""}`, ""}, + {"rds-single-az", `{"engine":"mysql","az_config":"single-az","instance_class":"db.r5.large"}`, "single-az"}, + {"rds-multi-az", `{"engine":"mysql","az_config":"multi-az","instance_class":"db.r5.large"}`, "multi-az"}, + {"azure-zone-redundant", `{"engine":"sqlserver","az_config":"zoneRedundant"}`, "zoneRedundant"}, + {"verbatim-spaces", `{"az_config":" Multi-AZ "}`, " Multi-AZ "}, + {"compute-details", `{"platform":"Linux/UNIX","tenancy":"default","scope":"Region"}`, ""}, + } + for _, v := range vectors { + t.Run(v.name, func(t *testing.T) { + rec := RecommendationRecord{Details: json.RawMessage(v.details)} + got, err := rec.DetailsAZConfig() + require.NoError(t, err) + assert.Equal(t, v.azConfig, got) + }) + } +} + +func TestDetailsAZConfig_MalformedFailsLoud(t *testing.T) { + bad := RecommendationRecord{Details: json.RawMessage(`{"az_config":5}`)} + _, err := bad.DetailsAZConfig() + require.Error(t, err) + _, err = RecIdentityKey(&bad) + require.Error(t, err, "the identity key must not silently drop an unreadable az_config") +} + +// Two otherwise identical RDS rows that differ only by az_config have +// different identity keys; the same az_config yields the same key. +func TestRecIdentityKey_DiffersByAZConfig(t *testing.T) { + rds := func(azConfig string) *RecommendationRecord { + return &RecommendationRecord{ + Provider: "aws", Service: "rds", Region: "us-east-1", ResourceType: "db.r5.large", Engine: "mysql", + Term: 1, Payment: "all-upfront", Details: json.RawMessage(`{"engine":"mysql","az_config":"` + azConfig + `"}`), + } + } + key := func(r *RecommendationRecord) string { + k, err := RecIdentityKey(r) + require.NoError(t, err) + return k + } + assert.NotEqual(t, key(rds("single-az")), key(rds("multi-az"))) + assert.NotEqual(t, key(rds("single-az")), key(rds(""))) + assert.Equal(t, key(rds("multi-az")), key(rds("multi-az"))) +} diff --git a/internal/scheduler/recommendation_identity_test.go b/internal/scheduler/recommendation_identity_test.go index c3d050d6f..8d98e686f 100644 --- a/internal/scheduler/recommendation_identity_test.go +++ b/internal/scheduler/recommendation_identity_test.go @@ -188,3 +188,37 @@ func TestRecommendationID_MalformedDetailsErrorsAndTagAccountDrops(t *testing.T) s := &Scheduler{} assert.Empty(t, s.tagAccount([]config.RecommendationRecord{bad}, "acct")) } + +func rdsAZConfigRec(azConfig string) common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderAWS, Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.r5.large", Count: 1, Term: "1yr", PaymentOption: "all-upfront", + Details: &common.DatabaseDetails{Engine: "mysql", AZConfig: azConfig, InstanceClass: "db.r5.large"}, + } +} + +// Two RDS recs for the same class, engine and term that differ only by +// Single-AZ vs Multi-AZ must get distinct IDs (issue #408); before, both got +// "aws||rds|us-east-1|db.r5.large|mysql|1|all-upfront". A database rec without +// az_config keeps its pre-#408 ID. +func TestConvertRecommendations_RDSAZConfigInID(t *testing.T) { + s := &Scheduler{} + records := s.convertRecommendations([]common.Recommendation{rdsAZConfigRec("single-az"), rdsAZConfigRec("multi-az"), rdsAZConfigRec("")}, "aws") + require.Len(t, records, 3) + assert.Equal(t, "aws||rds|us-east-1|db.r5.large|mysql|1|all-upfront|single-az", records[0].ID) + assert.Equal(t, "aws||rds|us-east-1|db.r5.large|mysql|1|all-upfront|multi-az", records[1].ID) + assert.Equal(t, "aws||rds|us-east-1|db.r5.large|mysql|1|all-upfront", records[2].ID) + + tagged := s.tagAccount(records[1:2], "acct") + require.Len(t, tagged, 1) + assert.Equal(t, "aws|acct|rds|us-east-1|db.r5.large|mysql|1|all-upfront|multi-az", tagged[0].ID) +} + +// An unreadable az_config is dropped loudly, never emitted with an ID that +// collides with its sibling. +func TestRecommendationID_MalformedAZConfigErrors(t *testing.T) { + bad := config.RecommendationRecord{Provider: "aws", Service: "rds", Details: []byte(`{"az_config":5}`)} + _, err := recommendationID(bad) + require.Error(t, err) + s := &Scheduler{} + assert.Empty(t, s.tagAccount([]config.RecommendationRecord{bad}, "acct")) +} diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 1d6953042..59e38cd39 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1150,6 +1150,10 @@ func recommendationID(rec config.RecommendationRecord) (string, error) { if err != nil { return "", err } + azConfig, err := rec.DetailsAZConfig() + if err != nil { + return "", err + } accountID := "" if rec.CloudAccountID != nil { accountID = *rec.CloudAccountID @@ -1169,6 +1173,13 @@ func recommendationID(rec config.RecommendationRecord) (string, error) { // spaces. id += "|" + ci.Platform + "|" + ci.Tenancy } + if azConfig != "" { + // Appended only for database recs whose Details carry it (RDS + // single-az/multi-az, Azure SQL zone redundancy), so every other rec + // keeps its ID (issue #408). Those recs never carry an AZ or a + // platform/tenancy pair, and the service field already differs. + id += "|" + azConfig + } return id, nil } From 913376f68000375146599f5caaa22cbe244673e8 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 22 Oct 2026 01:42:41 +0100 Subject: [PATCH 3/4] fix(config): upsert recommendations on the 12-column index with az_config (#408) Requires migration 000140. Co-Authored-By: Claude Opus 5.5 --- internal/config/store_postgres_recommendations.go | 10 ++++++---- internal/purchase/azure_pricing_integration_test.go | 6 +++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index d95877c47..cee71cf53 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -61,7 +61,8 @@ func (s *PostgresStore) ReplaceRecommendations(ctx context.Context, collectedAt // UpsertRecommendations is the incremental write path: it upserts each row // by natural key (cloud_account_id, provider, service, region, -// resource_type, engine, term, payment_option, availability_zone, platform, tenancy) and then evicts stale rows for the +// resource_type, engine, term, payment_option, availability_zone, platform, +// tenancy, az_config) and then evicts stale rows for the // set of (provider, account) pairs that successfully collected in this run. // Pairs whose collection failed are NOT in successfulCollects and their // stale rows stay — callers see older data with a banner rather than a @@ -71,8 +72,9 @@ func (s *PostgresStore) ReplaceRecommendations(ctx context.Context, collectedAt // so per-rec ON CONFLICT no longer collides on SQLSTATE 21000. Migration // 000109 added availability_zone, a GENERATED column derived from payload that // is never written here. Migration 000111 added the platform and tenancy -// generated columns the same way; the upsert arbiter is the 11-column -// recommendations_natural_key_az_tp_idx (issues #792, #806). +// generated columns the same way, and migration 000140 the az_config one; the +// upsert arbiter is the 12-column recommendations_natural_key_az_tp_azc_idx +// (issues #792, #806, #408). // // The eviction predicate uses (provider, account_key) IN (unnest($2, $3)) // where account_key matches the generated column on the table — nil @@ -226,7 +228,7 @@ func insertRecommendationsBatch(ctx context.Context, tx pgx.Tx, collectedAt time if onConflict { query += ` - ON CONFLICT (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy) + ON CONFLICT (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, az_config) DO UPDATE SET payload = EXCLUDED.payload, upfront_cost = EXCLUDED.upfront_cost, diff --git a/internal/purchase/azure_pricing_integration_test.go b/internal/purchase/azure_pricing_integration_test.go index 0fe2d4dcf..a734724cf 100644 --- a/internal/purchase/azure_pricing_integration_test.go +++ b/internal/purchase/azure_pricing_integration_test.go @@ -102,10 +102,10 @@ func TestApproveAzurePricingRequiresMigrationAndRecollection(t *testing.T) { require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 106)) t.Run("106 invalidated old prices", func(t *testing.T) { approve(t, ErrStaleAzurePricing, 0) }) rec.UpfrontCost = 700 - // The store's upsert needs the availability_zone, platform and tenancy columns and the - // 11-column index from migration 000111, and resets + // The store's upsert needs the availability_zone, platform, tenancy and az_config columns + // and the 12-column index from migration 000140 (issue #408), and resets // recommendations_state.last_collection_partial, added by migration 000129 (issue #87). - require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 129)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 140)) require.NoError(t, pricing.UpsertRecommendations(ctx, time.Now().Add(time.Minute), []config.RecommendationRecord{rec}, collects)) t.Run("recollected prices purchase once", func(t *testing.T) { approve(t, nil, 1) }) } From a035c5d52b70d2b59d77adc15af48f1b6ab23806 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 22 Oct 2026 01:42:41 +0100 Subject: [PATCH 4/4] fix(db): drop the recommendations indexes without az_config (#408) Migration 000141 drops the 11-column natural key and the 000120 source index, both superseded by the 000140 indexes that add az_config. While either exists, a collection batch carrying a Single-AZ and a Multi-AZ RDS recommendation of the same class fails as a whole. The rollback collapses az_config-only duplicates to the newest row and recreates both. Apply only after the 12-column upsert is live everywhere. Co-Authored-By: Claude Opus 5.5 --- ..._postgres_recommendations_azconfig_test.go | 159 ++++++++++++++++++ ...recommendations_11col_natural_key.down.sql | 28 +++ ...p_recommendations_11col_natural_key.up.sql | 26 +++ ..._recommendations_11col_natural_key_test.go | 106 ++++++++++++ 4 files changed, 319 insertions(+) create mode 100644 internal/config/store_postgres_recommendations_azconfig_test.go create mode 100644 internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.down.sql create mode 100644 internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.up.sql create mode 100644 internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key_test.go diff --git a/internal/config/store_postgres_recommendations_azconfig_test.go b/internal/config/store_postgres_recommendations_azconfig_test.go new file mode 100644 index 000000000..8eb61c822 --- /dev/null +++ b/internal/config/store_postgres_recommendations_azconfig_test.go @@ -0,0 +1,159 @@ +//go:build integration +// +build integration + +package config_test + +import ( + "context" + "encoding/json" + "sync" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func rdsAZConfigStoreRec(azConfig string, savings float64) config.RecommendationRecord { + rec := awsRec("rds-"+azConfig, "rds", "us-east-1", "db.r5.large", savings) + rec.Engine = "mysql" + rec.Details = json.RawMessage(`{"engine":"mysql","az_config":"` + azConfig + `","instance_class":"db.r5.large"}`) + return rec +} + +// Issue #408: Cost Explorer reports Single-AZ and Multi-AZ RDS reservations of +// the same class, engine, term and payment as separate recommendations. Once +// the store upserts on the 12-column index (with the generated az_config +// column) and migration 000141 drops the 11-column index, both rows upsert in +// ONE batch (the shape persistCollection writes) and both persist. Before, +// the batch failed with SQLSTATE 21000 ("ON CONFLICT DO UPDATE command cannot +// affect row a second time") and took the whole collection write down. +func TestPostgresStore_UpsertRecommendations_AZConfigBatch(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()) + pool := container.DB.Pool() + store := config.NewPostgresStore(container.DB) + t0 := time.Now().UTC().Truncate(time.Second) + collect := []config.SuccessfulCollect{{Provider: "aws"}} + + var oldIdx bool + require.NoError(t, pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE indexname = 'recommendations_natural_key_az_tp_idx')`).Scan(&oldIdx)) + require.False(t, oldIdx, "11-column index must be gone after the full migration chain (000141)") + + savingsByAZConfig := func(t *testing.T) map[string]float64 { + t.Helper() + rows, err := pool.Query(ctx, `SELECT az_config, monthly_savings FROM recommendations`) + require.NoError(t, err) + defer rows.Close() + out := map[string]float64{} + for rows.Next() { + var k string + var v float64 + require.NoError(t, rows.Scan(&k, &v)) + _, dup := out[k] + require.False(t, dup, "two stored rows share az_config %q", k) + out[k] = v + } + require.NoError(t, rows.Err()) + return out + } + reset := func(t *testing.T) { + t.Helper() + _, err := pool.Exec(ctx, `DELETE FROM recommendations`) + require.NoError(t, err) + } + + t.Run("single-az and multi-az in one batch both persist", func(t *testing.T) { + reset(t) + require.NoError(t, store.UpsertRecommendations(ctx, t0, + []config.RecommendationRecord{rdsAZConfigStoreRec("single-az", 10), rdsAZConfigStoreRec("multi-az", 20)}, collect)) + assert.Equal(t, map[string]float64{"single-az": 10, "multi-az": 20}, savingsByAZConfig(t)) + }) + + t.Run("same az_config re-upsert updates in place and leaves the sibling alone", func(t *testing.T) { + require.NoError(t, store.UpsertRecommendations(ctx, t0.Add(time.Hour), + []config.RecommendationRecord{rdsAZConfigStoreRec("single-az", 11), rdsAZConfigStoreRec("multi-az", 20)}, collect)) + assert.Equal(t, map[string]float64{"single-az": 11, "multi-az": 20}, savingsByAZConfig(t)) + }) + + t.Run("cross-run single-az then multi-az both persist", func(t *testing.T) { + reset(t) + require.NoError(t, store.UpsertRecommendations(ctx, t0, []config.RecommendationRecord{rdsAZConfigStoreRec("single-az", 10)}, collect)) + require.NoError(t, store.UpsertRecommendations(ctx, t0.Add(time.Hour), + []config.RecommendationRecord{rdsAZConfigStoreRec("single-az", 10), rdsAZConfigStoreRec("multi-az", 20)}, collect)) + assert.Equal(t, map[string]float64{"single-az": 10, "multi-az": 20}, savingsByAZConfig(t)) + }) + + t.Run("non-database rows alongside are unaffected", func(t *testing.T) { + reset(t) + ec2 := awsRec("e1", "ec2", "us-east-1", "m5.large", 30) + batch := []config.RecommendationRecord{rdsAZConfigStoreRec("single-az", 10), rdsAZConfigStoreRec("multi-az", 20), ec2} + require.NoError(t, store.UpsertRecommendations(ctx, t0, batch, collect)) + assert.Equal(t, map[string]float64{"single-az": 10, "multi-az": 20, "": 30}, savingsByAZConfig(t)) + }) + + t.Run("row written before the switch is found and updated", func(t *testing.T) { + reset(t) + // What the pre-#408 writer stored: az_config only inside details. The + // generated column picks it up, so the new arbiter matches it. + _, err := pool.Exec(ctx, ` + INSERT INTO recommendations + (collected_at, provider, service, region, resource_type, engine, payload, + upfront_cost, monthly_savings, term, payment_option) + VALUES ($1, 'aws', 'rds', 'us-east-1', 'db.r5.large', 'mysql', + '{"details":{"engine":"mysql","az_config":"multi-az"}}'::jsonb, 100, 5, 12, 'no-upfront')`, t0) + require.NoError(t, err) + require.NoError(t, store.UpsertRecommendations(ctx, t0.Add(time.Hour), []config.RecommendationRecord{rdsAZConfigStoreRec("multi-az", 40)}, collect)) + assert.Equal(t, map[string]float64{"multi-az": 40}, savingsByAZConfig(t), "the legacy row must be the conflict target, not duplicated") + }) +} + +// Concurrent collections (overlapping scheduled runs, or a manual refresh +// racing the cron) re-upsert an already stored Single-AZ/Multi-AZ pair at +// once. Every writer must succeed and exactly one row per az_config must +// remain: the 12-column arbiter routes each writer to its own variant's row. +// +// The rows are seeded first on purpose. Two writers inserting the same NEW +// row at the same instant can still fail with 23505 on the non-arbiter +// source-account unique index; that race predates #408 (migration 000120's +// recommendations_natural_key_source_idx on main has it too) and is out of +// this test's scope. +func TestPostgresStore_UpsertRecommendations_AZConfigConcurrent(t *testing.T) { + ctx := context.Background() + container := testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()) + pool := container.DB.Pool() + store := config.NewPostgresStore(container.DB) + t0 := time.Now().UTC().Truncate(time.Second) + collect := []config.SuccessfulCollect{{Provider: "aws"}} + require.NoError(t, store.UpsertRecommendations(ctx, t0, + []config.RecommendationRecord{rdsAZConfigStoreRec("single-az", 1), rdsAZConfigStoreRec("multi-az", 2)}, collect)) + + const writers = 8 + var wg sync.WaitGroup + errs := make([]error, writers) + start := make(chan struct{}) + for i := 0; i < writers; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + <-start + batch := []config.RecommendationRecord{ + rdsAZConfigStoreRec("single-az", float64(10+i)), + rdsAZConfigStoreRec("multi-az", float64(20+i)), + } + errs[i] = store.UpsertRecommendations(ctx, t0.Add(time.Duration(i+1)*time.Second), batch, collect) + }(i) + } + close(start) + wg.Wait() + for i, err := range errs { + require.NoError(t, err, "writer %d", i) + } + + var n, distinct int + require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*), COUNT(DISTINCT az_config) FROM recommendations`).Scan(&n, &distinct)) + assert.Equal(t, 2, n) + assert.Equal(t, 2, distinct) +} diff --git a/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.down.sql b/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.down.sql new file mode 100644 index 000000000..a5ab50e36 --- /dev/null +++ b/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.down.sql @@ -0,0 +1,28 @@ +-- Rollback of 000141: recreate the 11-column unique index exactly as defined +-- by 000111 and the source-account index exactly as defined by 000120. +-- +-- Only safe together with a code rollback to the 11-column upsert. The newer +-- code needs the 12-column arbiter from 000140, which this rollback keeps. The +-- recommendations table is a regenerable cache, so rows that differ only by +-- az_config (allowed while the 11-column index is absent) are collapsed to the +-- most recently collected one before the indexes are rebuilt; the next +-- collection cycle repopulates the rest. Only duplicates under the 11-column +-- key are deleted, which also clears every duplicate under the finer 000120 +-- source key. +DELETE FROM recommendations +WHERE id IN ( + SELECT id FROM ( + SELECT id, ROW_NUMBER() OVER ( + PARTITION BY account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy + ORDER BY collected_at DESC, id DESC + ) AS rn + FROM recommendations + ) ranked + WHERE rn > 1 +); + +CREATE UNIQUE INDEX IF NOT EXISTS recommendations_natural_key_az_tp_idx + ON recommendations (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy); + +CREATE UNIQUE INDEX IF NOT EXISTS recommendations_natural_key_source_idx + ON recommendations (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, source_account_key); diff --git a/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.up.sql b/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.up.sql new file mode 100644 index 000000000..71422a1fb --- /dev/null +++ b/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key.up.sql @@ -0,0 +1,26 @@ +-- Migration 000141: drop the 11-column recommendations_natural_key_az_tp_idx +-- and the 12-column recommendations_natural_key_source_idx (#408, contract step +-- of the RDS AZ-config identity work). +-- +-- Migration 000140 added the generated az_config column, the 12-column +-- recommendations_natural_key_az_tp_azc_idx and the 13-column +-- recommendations_natural_key_source_azc_idx, and kept the indexes without +-- az_config so the previous upsert (ON CONFLICT on the 11 columns) kept +-- working. The store now upserts on the 12-column index, so both indexes +-- without az_config only do harm: they reject a Single-AZ and a Multi-AZ RDS +-- row of the same class, engine, term and payment, which aborts the whole +-- collection write. Dropping them lets those rows coexist. The 000120 source +-- index is superseded by recommendations_natural_key_source_azc_idx (the same +-- key plus az_config), so the source-account work of #799 keeps a +-- source-aware unique key. +-- +-- DEPLOY ORDER: apply this only after the code that upserts on the 12-column +-- index is live everywhere. Applying it while older code still runs breaks +-- every collection, because ON CONFLICT on the 11 columns needs that index. +-- database-migration.yml applies ALL pending migrations when `steps` is 0 or +-- empty, so never dispatch it with the default before that code is released. +-- +-- Each replacement index is strictly finer than the one it supersedes, so no +-- existing row can violate it and nothing needs backfilling. +DROP INDEX IF EXISTS recommendations_natural_key_az_tp_idx; +DROP INDEX IF EXISTS recommendations_natural_key_source_idx; diff --git a/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key_test.go b/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key_test.go new file mode 100644 index 000000000..20aa2dce5 --- /dev/null +++ b/internal/database/postgres/migrations/000141_drop_recommendations_11col_natural_key_test.go @@ -0,0 +1,106 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const insertRecForMigration141 = ` + INSERT INTO recommendations + (collected_at, provider, service, region, resource_type, engine, payload, + upfront_cost, monthly_savings, term, payment_option) + VALUES ($1::timestamptz, 'aws', 'rds', $2, 'db.r5.large', 'mysql', $3::jsonb, 0, 0, 1, 'all-upfront') +` + +// TestMigration_DropRecommendations11ColNaturalKey locks down migration 000141 +// (issue #408, contract step): Single-AZ and Multi-AZ rows of the same class +// coexist once the 11-column and 000120 source indexes are gone, the 12-column index still dedupes, +// and the rollback recreates both indexes after collapsing those +// duplicates to the newest row. +func TestMigration_DropRecommendations11ColNaturalKey(t *testing.T) { + ctx := context.Background() + migrationsPath := getMigrationsPath() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + + indexExists := func(name string) bool { + var exists bool + require.NoError(t, pool.QueryRow(ctx, + `SELECT EXISTS (SELECT 1 FROM pg_indexes WHERE tablename = 'recommendations' AND indexname = $1)`, name).Scan(&exists)) + return exists + } + count := func(region string) int { + var n int + require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM recommendations WHERE region = $1`, region).Scan(&n)) + return n + } + const ( + single = `{"details":{"engine":"mysql","az_config":"single-az"}}` + multi = `{"details":{"engine":"mysql","az_config":"multi-az"}}` + none = `{"details":{"engine":"mysql"}}` + ) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 141))) + require.True(t, indexExists("recommendations_natural_key_az_tp_idx")) + require.True(t, indexExists("recommendations_natural_key_az_tp_azc_idx")) + require.True(t, indexExists("recommendations_natural_key_source_idx")) + require.True(t, indexExists("recommendations_natural_key_source_azc_idx")) + + _, err = pool.Exec(ctx, insertRecForMigration141, "2026-01-01T00:00:00Z", "us-east-1", single) + require.NoError(t, err) + _, err = pool.Exec(ctx, insertRecForMigration141, "2026-01-01T00:00:00Z", "us-west-2", single) + require.NoError(t, err) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 141)) + assert.False(t, indexExists("recommendations_natural_key_az_tp_idx"), "11-column index must be dropped") + assert.True(t, indexExists("recommendations_natural_key_az_tp_azc_idx"), "12-column index must remain") + assert.False(t, indexExists("recommendations_natural_key_source_idx"), "000120's source index (no az_config) must be dropped") + assert.True(t, indexExists("recommendations_natural_key_source_azc_idx"), "the source-aware key with az_config must remain") + + // Rows differing only by az_config now coexist; an exact duplicate is + // still rejected by the 12-column index. + _, err = pool.Exec(ctx, insertRecForMigration141, "2026-01-02T00:00:00Z", "us-east-1", multi) + require.NoError(t, err) + _, err = pool.Exec(ctx, insertRecForMigration141, "2026-01-03T00:00:00Z", "us-east-1", none) + require.NoError(t, err) + assert.Equal(t, 3, count("us-east-1")) + _, err = pool.Exec(ctx, insertRecForMigration141, "2026-01-04T00:00:00Z", "us-east-1", single) + require.Error(t, err, "the same az_config must still violate the 12-column index") + + // Rollback: az_config-only duplicates collapse to the newest, the other + // region's row is untouched, and the 11-column index is recreated with + // its original shape. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, 141))) + assert.Equal(t, 1, count("us-east-1"), "only duplicates under the 11-column key may be deleted") + assert.Equal(t, 1, count("us-west-2")) + var kept string + require.NoError(t, pool.QueryRow(ctx, `SELECT az_config FROM recommendations WHERE region = 'us-east-1'`).Scan(&kept)) + assert.Equal(t, "", kept, "the most recently collected row survives") + + var indexDef string + require.NoError(t, pool.QueryRow(ctx, + `SELECT indexdef FROM pg_indexes WHERE indexname = 'recommendations_natural_key_az_tp_idx'`).Scan(&indexDef)) + assert.Contains(t, indexDef, "UNIQUE INDEX") + assert.Contains(t, indexDef, "(account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy)") + assert.True(t, indexExists("recommendations_natural_key_az_tp_azc_idx")) + require.NoError(t, pool.QueryRow(ctx, + `SELECT indexdef FROM pg_indexes WHERE indexname = 'recommendations_natural_key_source_idx'`).Scan(&indexDef)) + assert.Contains(t, indexDef, "UNIQUE INDEX") + assert.Contains(t, indexDef, "(account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone, platform, tenancy, source_account_key)") + + // Re-applying the up migration after a rollback works. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 141)) + assert.False(t, indexExists("recommendations_natural_key_az_tp_idx")) + assert.False(t, indexExists("recommendations_natural_key_source_idx")) +}