From c6d8e654deba5057fcbde27ec4b9c6c569a0b3a6 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 19 Oct 2026 23:23:59 +0100 Subject: [PATCH] fix(plans): migrate stored pre-#608 Azure/GCP service ids to canonical slugs Plans saved before #608 can still store the old per-provider service ids (azure vm/sql/redis, gcp cloudsql/memorystore). Plan step resolution filters recommendations on the stored service, so those plans match nothing and buy nothing. #1123 fixed only the edit path. Migration 000136 rewrites the entry's service value to the canonical slug using the same table as legacyServiceAlias, keeping keys and every other field. It skips an entry when another entry of the plan already targets the same provider/canonical service, since two plan services matching the same recommendations fail the whole plan. Like an API edit, it clears a ramp baseline whose enabled-service shape changed and leaves the ledger guard trigger enabled; updated_at is bumped so stale application snapshots hit the optimistic-lock conflict instead of writing the legacy id back. The down migration is a no-op. Closes #1124 Co-Authored-By: Claude Opus 5.5 --- ...canonical_legacy_plan_service_ids.down.sql | 5 + ...6_canonical_legacy_plan_service_ids.up.sql | 99 ++++++ ..._canonical_legacy_plan_service_ids_test.go | 282 ++++++++++++++++++ 3 files changed, 386 insertions(+) create mode 100644 internal/database/postgres/migrations/000136_canonical_legacy_plan_service_ids.down.sql create mode 100644 internal/database/postgres/migrations/000136_canonical_legacy_plan_service_ids.up.sql create mode 100644 internal/database/postgres/migrations/000136_canonical_legacy_plan_service_ids_test.go diff --git a/internal/database/postgres/migrations/000136_canonical_legacy_plan_service_ids.down.sql b/internal/database/postgres/migrations/000136_canonical_legacy_plan_service_ids.down.sql new file mode 100644 index 000000000..f8ca417a0 --- /dev/null +++ b/internal/database/postgres/migrations/000136_canonical_legacy_plan_service_ids.down.sql @@ -0,0 +1,5 @@ +-- Intentionally a no-op: the up migration keeps each entry's key, and an API +-- edit since #1123 also stores the canonical service under a legacy key, so a +-- rewritten entry cannot be told apart from an edited one. Restoring the old +-- ids would also make those plans match no recommendations again. +SELECT 1; diff --git a/internal/database/postgres/migrations/000136_canonical_legacy_plan_service_ids.up.sql b/internal/database/postgres/migrations/000136_canonical_legacy_plan_service_ids.up.sql new file mode 100644 index 000000000..9142027d4 --- /dev/null +++ b/internal/database/postgres/migrations/000136_canonical_legacy_plan_service_ids.up.sql @@ -0,0 +1,99 @@ +-- Rewrite the service ids that plans saved before #608 still store to the +-- canonical slugs (issue #1124). +-- +-- The pre-#608 plan modal offered per-provider option ids (Azure vm, sql, +-- redis and GCP cloudsql, memorystore) and the API accepted any non-empty +-- service, so such plans store e.g. {"provider":"azure","service":"vm"}. +-- mapServiceSlug knows only the canonical slugs and the legacy AWS ones, and +-- plan step resolution filters recommendations on the stored service, so a +-- stored "vm" matches no Azure recommendation (they carry "compute") and the +-- plan silently buys nothing. #1123 fixed only the edit path. +-- +-- The mapping is the same table as legacyServiceAlias in +-- internal/api/plan_services_merge.go and LEGACY_PLAN_SERVICE_IDS in +-- frontend/src/plans.ts: +-- azure/vm -> compute, azure/sql -> relational-db, azure/redis -> cache, +-- gcp/cloudsql -> relational-db, gcp/memorystore -> cache. +-- Only the entry's "service" value changes. Keys stay as they are, because +-- #1123's singleton-alias edit path keeps a plan's original key. +-- +-- An entry is left alone when another entry of the same plan already targets +-- the same provider and canonical service (a canonical sibling, or a second +-- copy of the same legacy id under another key). Rewriting it would make two +-- plan services match the same recommendations, which plan step resolution +-- rejects for the whole plan ("matches more than one plan service"), so such +-- plans keep today's behavior and need a manual decision. +-- +-- The rewrite is one UPDATE per row with the same effect as editing the plan +-- through the API (PostgresStore.UpdatePurchasePlan): +-- * updated_at is bumped by update_purchase_plans_updated_at, so a plan +-- snapshot an application instance read before this migration fails its +-- optimistic-lock check (ErrPurchasePlanConflict) instead of writing the +-- legacy id back. +-- * When the enabled-service shape changes, ramp_baseline is cleared, as +-- UpdatePurchasePlan does via rampBaselineStale; ramp_ledger_shape is that +-- check's SQL mirror. guard_ramp_ledger_state stays enabled and applies +-- its usual rules: a never-started 'new' ramp stays 'new', and a plan that +-- had a baseline becomes 'legacy' (fail closed, as an API edit would). +-- * Rows are only touched when the rewrite changes them. +-- +-- Detection query for live databases (read only), listing every legacy entry +-- this migration considers: +-- +-- SELECT p.id, p.name, p.enabled, e.key, +-- e.value ->> 'provider' AS provider, e.value ->> 'service' AS service, +-- e.value ->> 'enabled' AS service_enabled +-- FROM purchase_plans p +-- CROSS JOIN LATERAL jsonb_each(CASE WHEN jsonb_typeof(p.services) = 'object' +-- THEN p.services ELSE '{}'::jsonb END) e +-- WHERE (e.value ->> 'provider', e.value ->> 'service') IN ( +-- ('azure', 'vm'), ('azure', 'sql'), ('azure', 'redis'), +-- ('gcp', 'cloudsql'), ('gcp', 'memorystore')) +-- ORDER BY p.id, e.key; +-- +-- The helper function is dropped again before the migration commits. + +CREATE OR REPLACE FUNCTION canonical_plan_services_000136(services JSONB) RETURNS JSONB +LANGUAGE plpgsql IMMUTABLE AS $$ +BEGIN + IF services IS NULL OR jsonb_typeof(services) <> 'object' THEN + RETURN services; + END IF; + RETURN COALESCE(( + WITH alias(provider, legacy, canonical) AS (VALUES + ('azure', 'vm', 'compute'), + ('azure', 'sql', 'relational-db'), + ('azure', 'redis', 'cache'), + ('gcp', 'cloudsql', 'relational-db'), + ('gcp', 'memorystore', 'cache') + ), entry AS ( + SELECT e.key, e.value, e.value ->> 'provider' AS provider, + COALESCE(a.canonical, e.value ->> 'service') AS target, + a.canonical IS NOT NULL AS legacy + FROM jsonb_each(services) e + LEFT JOIN alias a + ON a.provider = e.value ->> 'provider' AND a.legacy = e.value ->> 'service' + ) + SELECT jsonb_object_agg(x.key, CASE + WHEN x.legacy AND NOT EXISTS ( + SELECT 1 FROM entry y + WHERE y.key <> x.key AND y.provider = x.provider AND y.target = x.target) + THEN jsonb_set(x.value, '{service}', to_jsonb(x.target)) + ELSE x.value + END) + FROM entry x + ), services); +END +$$; + +UPDATE purchase_plans +SET services = canonical_plan_services_000136(services), + ramp_baseline = CASE + WHEN ramp_ledger_shape(services, ramp_schedule) + IS DISTINCT FROM ramp_ledger_shape(canonical_plan_services_000136(services), ramp_schedule) + THEN NULL + ELSE ramp_baseline + END +WHERE services IS DISTINCT FROM canonical_plan_services_000136(services); + +DROP FUNCTION canonical_plan_services_000136(JSONB); diff --git a/internal/database/postgres/migrations/000136_canonical_legacy_plan_service_ids_test.go b/internal/database/postgres/migrations/000136_canonical_legacy_plan_service_ids_test.go new file mode 100644 index 000000000..3173d63a4 --- /dev/null +++ b/internal/database/postgres/migrations/000136_canonical_legacy_plan_service_ids_test.go @@ -0,0 +1,282 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "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/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + m136Version = 136 + m136ImmediateRamp = `{"type":"immediate","percent_per_step":100,"total_steps":1}` + m136ProtectedRamp = `{"type":"weekly","percent_per_step":25,"total_steps":4,"current_step":0,"step_interval_days":7}` + m136Baseline = `{"aws|ec2|us-east-1|m5.large": {"count": 2}}` + m136EnabledEC2Entry = `"aws/ec2": {"provider":"aws","service":"ec2","enabled":true,"term":12,"payment":"no-upfront","coverage":80}` +) + +type m136Row struct { + services string + state string + baseline *string + updatedAt time.Time +} + +func m136Insert(ctx context.Context, t *testing.T, pool *pgxpool.Pool, services, schedule string) string { + t.Helper() + var id string + require.NoError(t, pool.QueryRow(ctx, ` + INSERT INTO purchase_plans (name, services, ramp_schedule, ramp_ledger_state) + VALUES ('legacy service plan', $1::jsonb, $2::jsonb, 'new') + RETURNING id::text`, services, schedule).Scan(&id)) + return id +} + +// m136Trust freezes a baseline the way SetPlanRampBaselineTx does, so the row +// carries the ledger state a sized plan has. +func m136Trust(ctx context.Context, t *testing.T, pool *pgxpool.Pool, id string) { + t.Helper() + _, err := pool.Exec(ctx, ` + UPDATE purchase_plans SET ramp_baseline = $2::jsonb, + ramp_ledger_state = CASE WHEN ramp_ledger_protected(ramp_schedule) THEN 'trusted' ELSE ramp_ledger_state END + WHERE id = $1`, id, m136Baseline) + require.NoError(t, err) +} + +func m136Read(ctx context.Context, t *testing.T, pool *pgxpool.Pool, id string) m136Row { + t.Helper() + var r m136Row + require.NoError(t, pool.QueryRow(ctx, ` + SELECT services::text, ramp_ledger_state, ramp_baseline::text, updated_at + FROM purchase_plans WHERE id = $1`, id).Scan(&r.services, &r.state, &r.baseline, &r.updatedAt)) + return r +} + +func m136Entry(provider, service string, enabled bool) string { + b, err := json.Marshal(map[string]any{ + "provider": provider, "service": service, "enabled": enabled, "term": 36, "payment": "no-upfront", + "coverage": 70, "ramp_schedule": "", "include_regions": []string{"eastus"}, "min_count": 2, + }) + if err != nil { + panic(err) + } + return string(b) +} + +type m136Case struct { + name, before, after string +} + +func m136RewriteCases() []m136Case { + vm, compute := m136Entry("azure", "vm", true), m136Entry("azure", "compute", true) + rewritten := []m136Case{ + {"azure/vm", `{"azure/vm":` + vm + `}`, `{"azure/vm":` + compute + `}`}, + {"azure/sql", `{"azure/sql":` + m136Entry("azure", "sql", true) + `}`, `{"azure/sql":` + m136Entry("azure", "relational-db", true) + `}`}, + {"azure/redis", `{"azure/redis":` + m136Entry("azure", "redis", true) + `}`, `{"azure/redis":` + m136Entry("azure", "cache", true) + `}`}, + {"gcp/cloudsql", `{"gcp/cloudsql":` + m136Entry("gcp", "cloudsql", true) + `}`, `{"gcp/cloudsql":` + m136Entry("gcp", "relational-db", true) + `}`}, + {"gcp/memorystore", `{"gcp/memorystore":` + m136Entry("gcp", "memorystore", false) + `}`, `{"gcp/memorystore":` + m136Entry("gcp", "cache", false) + `}`}, + {"legacy entry next to an unrelated service", + `{"azure/vm":` + vm + `,` + m136EnabledEC2Entry + `}`, + `{"azure/vm":` + compute + `,` + m136EnabledEC2Entry + `}`}, + {"several legacy ids with distinct targets", + `{"azure/vm":` + vm + `,"azure/sql":` + m136Entry("azure", "sql", true) + `,"azure/redis":` + m136Entry("azure", "redis", true) + `}`, + `{"azure/vm":` + compute + `,"azure/sql":` + m136Entry("azure", "relational-db", true) + `,"azure/redis":` + m136Entry("azure", "cache", true) + `}`}, + {"only the legacy id without a canonical sibling", + `{"azure/vm":` + vm + `,"azure/compute":` + m136Entry("azure", "compute", false) + `,"azure/sql":` + m136Entry("azure", "sql", true) + `}`, + `{"azure/vm":` + vm + `,"azure/compute":` + m136Entry("azure", "compute", false) + `,"azure/sql":` + m136Entry("azure", "relational-db", true) + `}`}, + } + unchanged := []m136Case{ + {name: "canonical value", before: `{"azure/compute":` + compute + `}`}, + {name: "canonical value under a legacy key (#1123 edit)", before: `{"azure/vm":` + compute + `}`}, + {name: "legacy AWS slug", before: `{` + m136EnabledEC2Entry + `}`}, + {name: "Azure id under another provider", before: `{"aws/sql":` + m136Entry("aws", "sql", true) + `,"gcp/vm":` + m136Entry("gcp", "vm", true) + `}`}, + {name: "GCP id under another provider", before: `{"azure/cloudsql":` + m136Entry("azure", "cloudsql", true) + `}`}, + {name: "split Savings Plans keys", before: `{"aws:savings-plans-compute":` + m136Entry("aws", "savings-plans", true) + `}`}, + {name: "canonical sibling", before: `{"azure/vm":` + vm + `,"azure/compute":` + compute + `}`}, + {name: "same legacy id under two keys", before: `{"gcp/cloudsql":` + m136Entry("gcp", "cloudsql", true) + `,"gcp:cloudsql":` + m136Entry("gcp", "cloudsql", true) + `}`}, + {name: "empty services", before: `{}`}, + {name: "JSON null services", before: `null`}, + } + cases := make([]m136Case, 0, len(rewritten)+len(unchanged)) + cases = append(cases, rewritten...) + for _, c := range unchanged { + c.after = c.before + cases = append(cases, c) + } + return cases +} + +// TestMigration_CanonicalLegacyPlanServiceIDs locks down migration 000136 +// (issue #1124): pre-#608 Azure/GCP service ids stored in purchase_plans are +// rewritten to the canonical slugs plan step resolution filters on, keys and +// every other field stay, and the ledger follows the same rules as an API edit. +func TestMigration_CanonicalLegacyPlanServiceIDs(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() + + prev := previousMigrationVersion(t, m136Version) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, prev)) + + cases := m136RewriteCases() + ids := make([]string, len(cases)) + before := make([]m136Row, len(cases)) + for i, c := range cases { + ids[i] = m136Insert(ctx, t, pool, c.before, m136ImmediateRamp) + before[i] = m136Read(ctx, t, pool, ids[i]) + } + + ledgerNew := m136Insert(ctx, t, pool, `{"azure/vm":`+m136Entry("azure", "vm", true)+`}`, m136ProtectedRamp) + ledgerTrusted := m136Insert(ctx, t, pool, `{"azure/vm":`+m136Entry("azure", "vm", true)+`,`+m136EnabledEC2Entry+`}`, m136ProtectedRamp) + m136Trust(ctx, t, pool, ledgerTrusted) + ledgerDisabled := m136Insert(ctx, t, pool, `{"azure/vm":`+m136Entry("azure", "vm", false)+`,`+m136EnabledEC2Entry+`}`, m136ProtectedRamp) + m136Trust(ctx, t, pool, ledgerDisabled) + singleStep := m136Insert(ctx, t, pool, `{"azure/vm":`+m136Entry("azure", "vm", true)+`,`+m136EnabledEC2Entry+`}`, m136ImmediateRamp) + m136Trust(ctx, t, pool, singleStep) + require.Equal(t, "new", m136Read(ctx, t, pool, ledgerNew).state) + require.Equal(t, "trusted", m136Read(ctx, t, pool, ledgerTrusted).state) + require.Equal(t, "trusted", m136Read(ctx, t, pool, ledgerDisabled).state) + require.NotNil(t, m136Read(ctx, t, pool, singleStep).baseline) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, m136Version)) + + for i, c := range cases { + after := m136Read(ctx, t, pool, ids[i]) + assert.JSONEq(t, c.after, after.services, c.name) + if c.before == c.after { + assert.True(t, before[i].updatedAt.Equal(after.updatedAt), "%s: an untouched row keeps updated_at", c.name) + } else { + assert.True(t, after.updatedAt.After(before[i].updatedAt), "%s: a rewritten row bumps updated_at", c.name) + } + } + + t.Run("never-started ramp stays new", func(t *testing.T) { + r := m136Read(ctx, t, pool, ledgerNew) + assert.JSONEq(t, `{"azure/vm":`+m136Entry("azure", "compute", true)+`}`, r.services) + assert.Equal(t, "new", r.state) + assert.Nil(t, r.baseline) + }) + t.Run("sized ramp drops its stale baseline and fails closed", func(t *testing.T) { + r := m136Read(ctx, t, pool, ledgerTrusted) + assert.JSONEq(t, `{"azure/vm":`+m136Entry("azure", "compute", true)+`,`+m136EnabledEC2Entry+`}`, r.services) + assert.Nil(t, r.baseline, "the enabled-service shape changed, as rampBaselineStale would report") + assert.Equal(t, "legacy", r.state) + }) + t.Run("disabled legacy entry keeps the ledger", func(t *testing.T) { + r := m136Read(ctx, t, pool, ledgerDisabled) + assert.JSONEq(t, `{"azure/vm":`+m136Entry("azure", "compute", false)+`,`+m136EnabledEC2Entry+`}`, r.services) + require.NotNil(t, r.baseline) + assert.JSONEq(t, m136Baseline, *r.baseline) + assert.Equal(t, "trusted", r.state) + }) + t.Run("single-step plan drops its stale baseline", func(t *testing.T) { + r := m136Read(ctx, t, pool, singleStep) + assert.JSONEq(t, `{"azure/vm":`+m136Entry("azure", "compute", true)+`,`+m136EnabledEC2Entry+`}`, r.services) + assert.Nil(t, r.baseline) + }) + + // The down migration is a no-op and the up migration is idempotent: a + // second pass changes nothing, not even updated_at. + snapshot := map[string]m136Row{} + all := append(append([]string{}, ids...), ledgerNew, ledgerTrusted, ledgerDisabled, singleStep) + for _, id := range all { + snapshot[id] = m136Read(ctx, t, pool, id) + } + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, prev)) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, m136Version)) + for _, id := range all { + assert.Equal(t, snapshot[id], m136Read(ctx, t, pool, id), "down + up must leave plan %s unchanged", id) + } + var leftover bool + require.NoError(t, pool.QueryRow(ctx, + `SELECT EXISTS (SELECT 1 FROM pg_proc WHERE proname = 'canonical_plan_services_000136')`).Scan(&leftover)) + assert.False(t, leftover, "the helper function must not outlive the migration") +} + +// TestMigration_CanonicalLegacyPlanServiceIDs_ConcurrentWriters covers the two +// ways an application instance can race the migration on a cold start: +// +// - A writer that holds the row lock and commits first: the migration's +// UPDATE waits, then rewrites the committed version, so the writer's change +// survives and the service still ends up canonical. +// - A writer holding a snapshot read before the migration: its optimistic +// update fails with ErrPurchasePlanConflict instead of writing the legacy +// id back, while an untouched plan's snapshot still saves. +func TestMigration_CanonicalLegacyPlanServiceIDs_ConcurrentWriters(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() + store := config.NewPostgresStore(container.DB) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, previousMigrationVersion(t, m136Version))) + + locked := m136Insert(ctx, t, pool, `{"azure/vm":`+m136Entry("azure", "vm", true)+`}`, m136ImmediateRamp) + staleLegacy := m136Insert(ctx, t, pool, `{"gcp/memorystore":`+m136Entry("gcp", "memorystore", true)+`}`, m136ImmediateRamp) + staleCanonical := m136Insert(ctx, t, pool, `{"azure/compute":`+m136Entry("azure", "compute", true)+`}`, m136ImmediateRamp) + legacySnap, err := store.GetPurchasePlan(ctx, staleLegacy) + require.NoError(t, err) + canonicalSnap, err := store.GetPurchasePlan(ctx, staleCanonical) + require.NoError(t, err) + + tx, err := pool.Begin(ctx) + require.NoError(t, err) + defer func() { _ = tx.Rollback(ctx) }() + _, err = tx.Exec(ctx, `SELECT 1 FROM purchase_plans WHERE id = $1 FOR UPDATE`, locked) + require.NoError(t, err) + _, err = tx.Exec(ctx, `UPDATE purchase_plans SET services = jsonb_set(services, '{azure/vm,coverage}', '55') WHERE id = $1`, locked) + require.NoError(t, err) + + done := make(chan error, 1) + go func() { done <- migrations.MigrateToVersion(ctx, pool, migrationsPath, m136Version) }() + require.Eventually(t, func() bool { + var waiting bool + qErr := pool.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM pg_stat_activity + WHERE datname = current_database() AND wait_event_type = 'Lock')`).Scan(&waiting) + return qErr == nil && waiting + }, 30*time.Second, 50*time.Millisecond, "the migration must block on the writer's row lock") + select { + case migErr := <-done: + t.Fatalf("migration finished while the row was locked: %v", migErr) + default: + } + require.NoError(t, tx.Commit(ctx)) + select { + case migErr := <-done: + require.NoError(t, migErr) + case <-time.After(60 * time.Second): + t.Fatal("migration did not finish after the writer committed") + } + + want := m136Entry("azure", "compute", true) + var wantMap map[string]any + require.NoError(t, json.Unmarshal([]byte(want), &wantMap)) + wantMap["coverage"] = 55 + wantJSON, err := json.Marshal(map[string]any{"azure/vm": wantMap}) + require.NoError(t, err) + assert.JSONEq(t, string(wantJSON), m136Read(ctx, t, pool, locked).services, + "the migration rewrites the committed version: the writer's coverage and the canonical service both survive") + + legacySnap.Name = "stale writer" + require.ErrorIs(t, store.UpdatePurchasePlan(ctx, legacySnap), config.ErrPurchasePlanConflict, + "a snapshot from before the migration must not write the legacy id back") + assert.JSONEq(t, `{"gcp/memorystore":`+m136Entry("gcp", "cache", true)+`}`, m136Read(ctx, t, pool, staleLegacy).services) + + canonicalSnap.Name = "untouched writer" + require.NoError(t, store.UpdatePurchasePlan(ctx, canonicalSnap), "a plan the migration did not touch keeps a valid snapshot") +}