From 671a63b023929f6c0639fe2b87dc96b077ce315f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 12 Oct 2026 19:07:20 +0200 Subject: [PATCH 1/2] fix(config): upsert recommendations on the AZ-aware 9-column index --- .../config/store_postgres_recommendations.go | 9 ++- .../store_postgres_recommendations_test.go | 70 +++++++++++++++++++ 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/internal/config/store_postgres_recommendations.go b/internal/config/store_postgres_recommendations.go index 1e3ce00d..1baaaf0e 100644 --- a/internal/config/store_postgres_recommendations.go +++ b/internal/config/store_postgres_recommendations.go @@ -63,14 +63,17 @@ 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, term, payment_option) and then evicts stale rows for the +// resource_type, engine, term, payment_option, availability_zone) 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 // blank section. // // Migration 000032 broadened the natural key to include term + payment_option, -// so per-rec ON CONFLICT no longer collides on SQLSTATE 21000. +// 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; the upsert arbiter is the 9-column +// recommendations_natural_key_az_idx (issue #792). // // The eviction predicate uses (provider, account_key) IN (unnest($2, $3)) // where account_key matches the generated column on the table — nil @@ -224,7 +227,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) + ON CONFLICT (account_key, provider, service, region, resource_type, engine, term, payment_option, availability_zone) DO UPDATE SET payload = EXCLUDED.payload, upfront_cost = EXCLUDED.upfront_cost, diff --git a/internal/config/store_postgres_recommendations_test.go b/internal/config/store_postgres_recommendations_test.go index baf1ef32..1bd58845 100644 --- a/internal/config/store_postgres_recommendations_test.go +++ b/internal/config/store_postgres_recommendations_test.go @@ -5,6 +5,7 @@ package config_test import ( "context" + "encoding/json" "fmt" "testing" "time" @@ -529,3 +530,72 @@ func TestPostgresStore_UpsertRecommendations_ReplacesAzureWrongUnits(t *testing. assert.Equal(t, 3600.0, got[0].UpfrontCost) assert.Nil(t, got[0].MonthlyCost) } + +func zonalAWSRec(az string, savings float64) config.RecommendationRecord { + rec := awsRec("z-"+az, "ec2", "us-east-1", "m5.large", savings) + rec.AvailabilityZone = az + rec.Details = json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"default","scope":"Availability Zone","availability_zone":"` + az + `"}`) + return rec +} + +// Issue #792, PR2b: the upsert arbiter is the 9-column index that includes the +// generated availability_zone column. Same-AZ re-upserts update in place, a +// regional row (AZ ”) behaves as before, and a row written by pre-PR2b code +// (no top-level availability_zone in the payload) is found and updated. +// Multi-AZ rows on one 8-tuple stay rejected until the 8-column index is +// dropped (PR2c), so they are not asserted here. +func TestPostgresStore_UpsertRecommendations_AZArbiter(t *testing.T) { + ctx := context.Background() + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + defer container.Cleanup(ctx) + pool := container.DB.Pool() + require.NoError(t, migrations.RunMigrations(ctx, pool, getMigrationsPath(), "", "")) + store := config.NewPostgresStore(container.DB) + t0 := time.Now().UTC().Truncate(time.Second) + collect := []config.SuccessfulCollect{{Provider: "aws"}} + + t.Run("same-AZ re-upsert updates in place", func(t *testing.T) { + require.NoError(t, store.UpsertRecommendations(ctx, t0, []config.RecommendationRecord{zonalAWSRec("us-east-1a", 10)}, collect)) + require.NoError(t, store.UpsertRecommendations(ctx, t0.Add(time.Hour), []config.RecommendationRecord{zonalAWSRec("us-east-1a", 25)}, collect)) + got, err := store.ListStoredRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err) + require.Len(t, got, 1) + assert.InDelta(t, 25.0, got[0].Savings, 0.001) + assert.Equal(t, "us-east-1a", got[0].AvailabilityZone) + var az string + require.NoError(t, pool.QueryRow(ctx, `SELECT availability_zone FROM recommendations`).Scan(&az)) + assert.Equal(t, "us-east-1a", az, "the generated column is derived from the payload") + }) + + t.Run("regional row unchanged", func(t *testing.T) { + require.NoError(t, store.ReplaceRecommendations(ctx, t0, nil)) + require.NoError(t, store.UpsertRecommendations(ctx, t0, []config.RecommendationRecord{awsRec("r1", "ec2", "us-east-1", "c5.large", 10)}, collect)) + require.NoError(t, store.UpsertRecommendations(ctx, t0.Add(time.Hour), []config.RecommendationRecord{awsRec("r1", "ec2", "us-east-1", "c5.large", 30)}, collect)) + got, err := store.ListStoredRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err) + require.Len(t, got, 1) + assert.InDelta(t, 30.0, got[0].Savings, 0.001) + assert.Empty(t, got[0].AvailabilityZone) + }) + + t.Run("row written by pre-PR2b code is found and updated", func(t *testing.T) { + require.NoError(t, store.ReplaceRecommendations(ctx, t0, nil)) + // What the old code stored: AZ only inside details, 8-column semantics. + _, 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', 'ec2', 'us-east-1', 'm5.large', '', + '{"details":{"availability_zone":"us-east-1a"}}'::jsonb, 100, 5, 12, 'no-upfront')`, t0) + require.NoError(t, err) + require.NoError(t, store.UpsertRecommendations(ctx, t0.Add(time.Hour), []config.RecommendationRecord{zonalAWSRec("us-east-1a", 40)}, collect)) + var n int + require.NoError(t, pool.QueryRow(ctx, `SELECT COUNT(*) FROM recommendations`).Scan(&n)) + assert.Equal(t, 1, n, "the legacy row must be the conflict target, not duplicated") + got, err := store.ListStoredRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err) + require.Len(t, got, 1) + assert.InDelta(t, 40.0, got[0].Savings, 0.001) + }) +} From 4ebb55f5782c33263ed52603fd4299b5ce3a4543 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 12 Oct 2026 19:21:25 +0200 Subject: [PATCH 2/2] test(purchase): seed the pre-106 Azure row directly and migrate to 109 before upserting --- .../purchase/azure_pricing_integration_test.go | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/internal/purchase/azure_pricing_integration_test.go b/internal/purchase/azure_pricing_integration_test.go index 75a7d87c..85d98d65 100644 --- a/internal/purchase/azure_pricing_integration_test.go +++ b/internal/purchase/azure_pricing_integration_test.go @@ -4,6 +4,7 @@ package purchase import ( "context" + "encoding/json" "testing" "time" @@ -39,7 +40,18 @@ func TestApproveAzurePricingRequiresMigrationAndRecollection(t *testing.T) { rec.ID, rec.UpfrontCost = "azure-old-units", 70 rec.Payment = "upfront" collects := []config.SuccessfulCollect{{Provider: "azure"}} - require.NoError(t, pricing.UpsertRecommendations(ctx, time.Now(), []config.RecommendationRecord{rec}, collects)) + // Insert the pre-106 row directly: the store's upsert targets the availability_zone + // column added by migration 000109, which does not exist at version 105. + payload, err := json.Marshal(rec) + require.NoError(t, err) + _, err = pool.Exec(ctx, ` + INSERT INTO recommendations + (collected_at, cloud_account_id, provider, service, region, resource_type, engine, payload, + upfront_cost, monthly_savings, term, payment_option) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12)`, + time.Now(), rec.CloudAccountID, rec.Provider, rec.Service, rec.Region, rec.ResourceType, rec.Engine, payload, + rec.UpfrontCost, rec.Savings, rec.Term, rec.Payment) + require.NoError(t, err) approve := func(t *testing.T, expected error, purchases int) { t.Helper() @@ -90,6 +102,8 @@ 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 column from migration 000109. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, rampMigrationsPath(), 109)) 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) }) }