diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index c1d5baad..b51227ee 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -1262,9 +1262,12 @@ func (h *Handler) deleteAccountServiceOverride(ctx context.Context, req *events. // - 400 ClientError listing every provider mismatch in one message // (so clients fix everything in one round-trip rather than // resubmitting to discover the next) -// - nil when all accounts match (or when the plan has no parseable -// services — defensive skip; production plans always carry at least -// one service, frontend enforces this) +// - 500 ClientError when a stored service key names no provider (fail +// closed: skipping the key could leave the provider set empty, which +// would accept an account of any provider, issue #316) +// - nil when all accounts match (or when the plan has no services at +// all — production plans always carry at least one service, frontend +// enforces this) // // Pulled out of setPlanAccounts to keep that function under the gocyclo // budget (limit 10). No business logic lives here that isn't otherwise @@ -1275,7 +1278,13 @@ func (h *Handler) validatePlanAccountProviders(ctx context.Context, planID strin return err } - expected := config.DerivePlanProviders(plan) + expected, err := config.DerivePlanProviders(plan) + if err != nil { + // The key is stored data, not the caller's input; log it and keep + // the plan UUID out of the response, as the 500s above do. + logging.Errorf("validatePlanAccountProviders: %v", err) + return NewClientError(500, "failed to validate plan accounts") + } if len(expected) == 0 { return nil } diff --git a/internal/api/handler_accounts_test.go b/internal/api/handler_accounts_test.go index bf6dd62d..2bd1550b 100644 --- a/internal/api/handler_accounts_test.go +++ b/internal/api/handler_accounts_test.go @@ -1430,6 +1430,128 @@ func TestSetPlanAccounts_EmptyServicesSkipsValidation(t *testing.T) { assert.Equal(t, []string{azureAcct1}, capturedIDs, "empty services map → validation skipped → write proceeds") } +// TestSetPlanAccounts_ColonKeyedPlanStillChecksProvider: migration 000040 +// wrote "aws:savings-plans-" keys. DerivePlanProviders used to skip +// them, so a plan keyed only that way had an empty provider set and accepted +// an Azure account (issue #316, A04-012). +func TestSetPlanAccounts_ColonKeyedPlanStillChecksProvider(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + + setCalled := false + store := setupAdminMock(ctx) + store.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) { + return &config.PurchasePlan{ID: planID209, Name: "sp-plan", Services: map[string]config.ServiceConfig{ + "aws:savings-plans-compute": {Provider: "aws", Service: "savings-plans"}, + "aws:savings-plans-ec2instance": {Provider: "aws", Service: "savings-plans"}, + }}, nil + } + store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, Name: "prod-azure", Provider: "azure"}, nil + } + store.SetPlanAccountsFn = func(_ context.Context, _ string, _ []string) error { + setCalled = true + return nil + } + handler := &Handler{auth: mockAuth, config: store} + + body := `{"account_ids":["` + azureAcct1 + `"]}` + _, err := handler.setPlanAccounts(ctx, adminRequest(body), planID209) + require.Error(t, err, "an Azure account must not be assignable to an AWS-only plan") + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got %T", err) + assert.Equal(t, 400, ce.code) + assert.Contains(t, ce.Error(), "prod-azure") + assert.False(t, setCalled, "SetPlanAccounts must NOT be called when validation fails") +} + +// TestSetPlanAccounts_LegacyKeyedPlanUsesProviderField: a legacy alias kept +// under its original key (#1123) has no provider prefix, but its Provider +// field names one. A matching account is accepted and a mismatched one is +// refused, instead of the plan accepting every provider (main) or none (the +// first #316 commit). +func TestSetPlanAccounts_LegacyKeyedPlanUsesProviderField(t *testing.T) { + ctx := context.Background() + for _, tc := range []struct { + name, accountProvider string + wantCode int + }{ + {"matching provider accepted", "azure", 0}, + {"mismatched provider refused", "aws", 400}, + } { + t.Run(tc.name, func(t *testing.T) { + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + setCalled := false + store := setupAdminMock(ctx) + store.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) { + return &config.PurchasePlan{ID: planID209, Name: "alias plan", Services: map[string]config.ServiceConfig{ + "original-key": {Provider: "azure", Service: "vm", Enabled: true}, + }}, nil + } + store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, Name: "acct-" + tc.accountProvider, Provider: tc.accountProvider}, nil + } + store.SetPlanAccountsFn = func(_ context.Context, _ string, _ []string) error { + setCalled = true + return nil + } + handler := &Handler{auth: mockAuth, config: store} + + body := `{"account_ids":["` + azureAcct1 + `"]}` + _, err := handler.setPlanAccounts(ctx, adminRequest(body), planID209) + if tc.wantCode == 0 { + require.NoError(t, err) + assert.True(t, setCalled) + return + } + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got %T", err) + assert.Equal(t, tc.wantCode, ce.code) + assert.False(t, setCalled) + }) + } +} + +// TestSetPlanAccounts_UnparseableServiceKeyFailsClosed: stored entries that +// identify no provider (no prefix in the key, empty Provider field) used to be +// skipped silently; when none identified one, the provider set came out empty +// and any account was accepted. It now fails the request instead (issue #316, +// A04-012). +func TestSetPlanAccounts_UnparseableServiceKeyFailsClosed(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + + setCalled := false + store := setupAdminMock(ctx) + store.GetPurchasePlanFn = func(_ context.Context, _ string) (*config.PurchasePlan, error) { + return &config.PurchasePlan{ID: planID209, Name: "bad-key", Services: map[string]config.ServiceConfig{ + "ec2": {Service: "ec2"}, + "future": {}, + }}, nil + } + store.GetCloudAccountFn = func(_ context.Context, id string) (*config.CloudAccount, error) { + return &config.CloudAccount{ID: id, Name: "prod-azure", Provider: "azure"}, nil + } + store.SetPlanAccountsFn = func(_ context.Context, _ string, _ []string) error { + setCalled = true + return nil + } + handler := &Handler{auth: mockAuth, config: store} + + body := `{"account_ids":["` + azureAcct1 + `"]}` + _, err := handler.setPlanAccounts(ctx, adminRequest(body), planID209) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError, got %T", err) + assert.Equal(t, 500, ce.code) + assert.NotContains(t, ce.Error(), planID209, "the plan UUID must not leak into the response") + assert.False(t, setCalled, "SetPlanAccounts must NOT be called when the plan's providers cannot be derived") +} + // TestSetPlanAccounts_RejectsEmpty verifies the universal-plans fix: // PUT /api/plans/:id/accounts with an empty account_ids returns HTTP 400 // and never reaches the store. Eliminates the back-door for re-creating a diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 5875421a..7924ab5f 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -380,7 +380,9 @@ type StoreInterface interface { // support_case_id on a purchase_history row identified by purchase_id. // calcRefundAmount and calcRefundCurrency capture the Azure CalculateRefund // quote for audit (migration 000071, Finding #4); both nil/empty for - // non-Azure paths or legacy rows written before the migration. + // non-Azure paths or legacy rows written before the migration. Also + // clears revocation_in_flight (migration 000072): a revoked row is no + // longer in flight. // Returns a not-found error when no row matches. Idempotent: a second // call for the same row is a no-op (revoked_at is not overwritten when // it is already non-null). Used by the revoke endpoint (issue #290). diff --git a/internal/config/purchase_plan_providers.go b/internal/config/purchase_plan_providers.go index 455867bc..57ebd1a8 100644 --- a/internal/config/purchase_plan_providers.go +++ b/internal/config/purchase_plan_providers.go @@ -1,29 +1,52 @@ package config import ( + "fmt" "sort" "strings" ) -// DerivePlanProviders extracts the distinct set of providers a plan targets -// by parsing the keys of plan.Services. Keys are expected to use the -// "provider/service" format produced by buildServiceConfig. -func DerivePlanProviders(plan *PurchasePlan) []string { +// DerivePlanProviders extracts the distinct set of providers a plan targets. +// Each service entry names its provider through its key, in the +// "provider/service" form buildServiceConfig writes or the legacy +// "provider:service" form migration 000040 wrote (internal/api findServiceKey +// accepts both). An entry whose key has neither form, such as a legacy alias +// kept under its original key, is identified by its own Provider field, the +// same field the legacy edit path (mergeRequestedServices) and plan step +// resolution go by. +// +// An entry identified neither way is skipped: that can only narrow the set, +// so the check stays at least as strict. What it must never do is leave the +// set empty, because callers read an empty set as "no restriction" and would +// accept an account of any provider (issue #316). So when the plan has +// services but none of them identifies a provider, this returns an error. A +// nil plan, or a plan with no services, yields an empty set. +func DerivePlanProviders(plan *PurchasePlan) ([]string, error) { if plan == nil { - return nil + return nil, nil } seen := make(map[string]struct{}, len(plan.Services)) for k := range plan.Services { - provider, _, ok := strings.Cut(k, "/") - if !ok || provider == "" { - continue + if provider := serviceEntryProvider(k, plan.Services[k].Provider); provider != "" { + seen[provider] = struct{}{} } - seen[provider] = struct{}{} + } + if len(seen) == 0 && len(plan.Services) > 0 { + return nil, fmt.Errorf("none of the plan's %d services identifies a provider by key or provider field", len(plan.Services)) } providers := make([]string, 0, len(seen)) for provider := range seen { providers = append(providers, provider) } sort.Strings(providers) - return providers + return providers, nil +} + +// serviceEntryProvider returns the provider a services entry names: the key's +// prefix when the key has one, else the entry's Provider field, else "". +func serviceEntryProvider(key, fieldProvider string) string { + if i := strings.IndexAny(key, "/:"); i > 0 { + return key[:i] + } + return strings.TrimSpace(fieldProvider) } diff --git a/internal/config/purchase_plan_providers_test.go b/internal/config/purchase_plan_providers_test.go index ed5f0fd3..6bdb8c6c 100644 --- a/internal/config/purchase_plan_providers_test.go +++ b/internal/config/purchase_plan_providers_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestDerivePlanProviders(t *testing.T) { @@ -12,11 +13,90 @@ func TestDerivePlanProviders(t *testing.T) { "aws/ec2": {}, "aws/rds": {}, "azure/compute": {}, - "gcp:compute": {}, - "malformed": {}, }, } - assert.Equal(t, []string{"aws", "azure"}, DerivePlanProviders(plan)) - assert.Nil(t, DerivePlanProviders(nil)) + got, err := DerivePlanProviders(plan) + require.NoError(t, err) + assert.Equal(t, []string{"aws", "azure"}, got) + + got, err = DerivePlanProviders(nil) + require.NoError(t, err) + assert.Nil(t, got) + + got, err = DerivePlanProviders(&PurchasePlan{}) + require.NoError(t, err) + assert.Empty(t, got, "a plan with no services restricts nothing") +} + +// TestDerivePlanProviders_ColonKeys covers the "provider:service" keys that +// migration 000040 wrote (issue #316, A04-012). They name a provider just as +// clearly as the "provider/service" keys buildServiceConfig writes, and +// findServiceKey in internal/api already treats them as valid. Skipping them +// left a colon-keyed plan with an empty provider set, which both +// account-assignment guards read as "no restriction". +func TestDerivePlanProviders_ColonKeys(t *testing.T) { + plan := &PurchasePlan{ + Services: map[string]ServiceConfig{ + "aws:savings-plans-compute": {}, + "aws:savings-plans-ec2instance": {}, + "gcp/compute": {}, + }, + } + + got, err := DerivePlanProviders(plan) + require.NoError(t, err) + assert.Equal(t, []string{"aws", "gcp"}, got) +} + +// TestDerivePlanProviders_UnidentifiableEntries: an entry whose key names no +// provider and whose Provider field is empty is skipped when other entries do +// identify one (skipping can only narrow the set, and plans may carry entries +// this version does not know). When no entry identifies a provider the result +// must be an error, not an empty set: both callers read an empty set as +// "accept an account of any provider" (issue #316, A04-012). +func TestDerivePlanProviders_UnidentifiableEntries(t *testing.T) { + for _, key := range []string{"malformed", "/ec2", ":ec2", ""} { + t.Run("only "+key, func(t *testing.T) { + got, err := DerivePlanProviders(&PurchasePlan{Services: map[string]ServiceConfig{key: {Service: "ec2", Provider: " "}}}) + require.Error(t, err) + assert.Nil(t, got) + assert.Contains(t, err.Error(), "identifies a provider") + }) + t.Run("alongside aws/ec2 "+key, func(t *testing.T) { + got, err := DerivePlanProviders(&PurchasePlan{Services: map[string]ServiceConfig{ + "aws/ec2": {}, + key: {}, + }}) + require.NoError(t, err) + assert.Equal(t, []string{"aws"}, got) + }) + } +} + +// TestDerivePlanProviders_LegacyKeyUsesProviderField: a legacy alias kept +// under its original key (the singleton-alias edit path, #1123) has no +// provider in its key, but its ServiceConfig names one. That is the field the +// legacy edit path and plan step resolution go by, so it counts. Main skipped +// such keys (provider set empty, no restriction); the first #316 commit +// refused them outright. +func TestDerivePlanProviders_LegacyKeyUsesProviderField(t *testing.T) { + got, err := DerivePlanProviders(&PurchasePlan{Services: map[string]ServiceConfig{ + "original-key": {Provider: "azure", Service: "vm"}, + }}) + require.NoError(t, err) + assert.Equal(t, []string{"azure"}, got) + + got, err = DerivePlanProviders(&PurchasePlan{Services: map[string]ServiceConfig{ + "aws/ec2": {Provider: "aws", Service: "ec2"}, + "original-key": {Provider: "azure", Service: "vm"}, + }}) + require.NoError(t, err) + assert.Equal(t, []string{"aws", "azure"}, got, "every entry's provider counts, since plan step resolution buys for each") + + got, err = DerivePlanProviders(&PurchasePlan{Services: map[string]ServiceConfig{ + "gcp/compute": {Provider: "azure", Service: "compute"}, + }}) + require.NoError(t, err) + assert.Equal(t, []string{"gcp"}, got, "a key that names a provider still wins, as before") } diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 911f015e..d5f8d081 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -2926,9 +2926,11 @@ func (s *PostgresStore) GetPurchaseHistoryByPurchaseID(ctx context.Context, purc // MarkPurchaseRevoked stamps revoked_at / revoked_via / support_case_id and // the refund-quote audit columns (calc_refund_amount, calc_refund_currency) on -// the purchase_history row identified by purchaseID. The UPDATE is a no-op -// when revoked_at is already non-null (idempotency guard). Returns a not-found -// error when zero rows are affected and revoked_at was previously NULL. +// the purchase_history row identified by purchaseID, and clears +// revocation_in_flight as migration 000072 documents: a completed revocation +// is no longer in flight (issue #316). The UPDATE is a no-op when revoked_at +// is already non-null (idempotency guard). Returns a not-found error when zero +// rows are affected and revoked_at was previously NULL. func (s *PostgresStore) MarkPurchaseRevoked(ctx context.Context, purchaseID string, revokedAt time.Time, revokedVia, supportCaseID string, calcRefundAmount *float64, calcRefundCurrency string) error { var supportCaseIDPtr *string if supportCaseID != "" { @@ -2944,7 +2946,8 @@ func (s *PostgresStore) MarkPurchaseRevoked(ctx context.Context, purchaseID stri revoked_via = $3, support_case_id = $4, calc_refund_amount = $5, - calc_refund_currency = $6 + calc_refund_currency = $6, + revocation_in_flight = false WHERE purchase_id = $1 AND revoked_at IS NULL `, purchaseID, revokedAt, revokedVia, supportCaseIDPtr, calcRefundAmount, calcCurrencyPtr) @@ -4408,7 +4411,10 @@ func (s *PostgresStore) validatePlanAccountProvidersTx(ctx context.Context, tx p if len(accountIDs) == 0 { return nil } - expected := DerivePlanProviders(&PurchasePlan{Services: services}) + expected, err := DerivePlanProviders(&PurchasePlan{Services: services}) + if err != nil { + return fmt.Errorf("validate plan account providers: %w", err) + } if len(expected) == 0 { return nil } diff --git a/internal/config/store_postgres_issue316_integration_test.go b/internal/config/store_postgres_issue316_integration_test.go new file mode 100644 index 00000000..8eb6c732 --- /dev/null +++ b/internal/config/store_postgres_issue316_integration_test.go @@ -0,0 +1,187 @@ +//go:build integration + +package config + +// Real-DB tests for the store-side findings of issue #316 (2026-09-02 audit). + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func seedRevocationRow(ctx context.Context, t *testing.T, store *PostgresStore, purchaseID string) { + t.Helper() + require.NoError(t, store.SavePurchaseHistory(ctx, &PurchaseHistoryRecord{ + AccountID: "acct", PurchaseID: purchaseID, Timestamp: time.Now(), Provider: "azure", Service: "compute", + Region: "eastus", ResourceType: "Standard_D2s_v3", Count: 1, Term: 1, Payment: "All Upfront", + })) +} + +// inFlightIndexed counts the row under the predicate of the partial index +// idx_purchase_history_revocation_in_flight (migration 000072). +func inFlightIndexed(ctx context.Context, t *testing.T, store *PostgresStore, purchaseID string) int { + t.Helper() + var n int + require.NoError(t, store.db.QueryRow(ctx, + `SELECT count(*) FROM purchase_history WHERE purchase_id = $1 AND revocation_in_flight`, purchaseID).Scan(&n)) + return n +} + +// TestMarkPurchaseRevoked_ClearsRevocationInFlight covers A04-013: migration +// 000072 and PurchaseHistoryRecord.RevocationInFlight document that a +// successful MarkPurchaseRevoked resets the flag, but the UPDATE left it set, +// so every revoked Azure row read back as in flight and stayed in the +// partial index for good. +func TestMarkPurchaseRevoked_ClearsRevocationInFlight(t *testing.T) { + ctx := context.Background() + store := setupRampStepStore(ctx, t) + + t.Run("flip then mark clears the flag", func(t *testing.T) { + const id = "azure-revoke-316-a" + seedRevocationRow(ctx, t, store, id) + require.NoError(t, store.FlipPurchaseRevocationInFlight(ctx, id)) + require.Equal(t, 1, inFlightIndexed(ctx, t, store, id), "precondition: the flip sets the flag") + + revokedAt := time.Now().UTC().Truncate(time.Microsecond) + amount := 12.5 + require.NoError(t, store.MarkPurchaseRevoked(ctx, id, revokedAt, "direct-api", "", &amount, "USD")) + + got, err := store.GetPurchaseHistoryByPurchaseID(ctx, id) + require.NoError(t, err) + require.NotNil(t, got.RevokedAt) + assert.True(t, revokedAt.Equal(*got.RevokedAt)) + assert.False(t, got.RevocationInFlight, "a completed revocation must not read back as in flight") + assert.Zero(t, inFlightIndexed(ctx, t, store, id), "the row must leave the in-flight partial index") + + pending, err := store.GetPurchaseHistoryInFlight(ctx) + require.NoError(t, err) + for _, r := range pending { + assert.NotEqual(t, id, r.PurchaseID) + } + }) + + t.Run("a repeat mark is still a no-op", func(t *testing.T) { + const id = "azure-revoke-316-b" + seedRevocationRow(ctx, t, store, id) + first := time.Now().UTC().Truncate(time.Microsecond) + require.NoError(t, store.MarkPurchaseRevoked(ctx, id, first, "direct-api", "", nil, "")) + require.NoError(t, store.MarkPurchaseRevoked(ctx, id, first.Add(time.Hour), "support-case", "case-1", nil, "")) + + got, err := store.GetPurchaseHistoryByPurchaseID(ctx, id) + require.NoError(t, err) + require.NotNil(t, got.RevokedAt) + assert.True(t, first.Equal(*got.RevokedAt), "the first revocation's stamp must stand") + assert.Equal(t, "direct-api", got.RevokedVia) + assert.False(t, got.RevocationInFlight) + }) + + // The handler's retry loop and the finalize_revocations sweep can both be + // marking the same in-flight row. The UPDATE's row lock serializes them: + // one stamps the row, the rest re-check revoked_at IS NULL after the lock, + // match nothing and take the idempotent path. Whatever the interleaving, + // the flag ends cleared and exactly one stamp lands. + t.Run("concurrent marks of one in-flight row", func(t *testing.T) { + const id = "azure-revoke-316-c" + seedRevocationRow(ctx, t, store, id) + require.NoError(t, store.FlipPurchaseRevocationInFlight(ctx, id)) + + const workers = 8 + base := time.Now().UTC().Truncate(time.Microsecond) + stamps := make([]time.Time, workers) + errs := make([]error, workers) + start := make(chan struct{}) + var wg sync.WaitGroup + for i := range workers { + stamps[i] = base.Add(time.Duration(i) * time.Second) + wg.Add(1) + go func() { + defer wg.Done() + <-start + errs[i] = store.MarkPurchaseRevoked(ctx, id, stamps[i], "direct-api", "", nil, "") + }() + } + close(start) + wg.Wait() + for i, err := range errs { + require.NoError(t, err, "worker %d", i) + } + + got, err := store.GetPurchaseHistoryByPurchaseID(ctx, id) + require.NoError(t, err) + require.NotNil(t, got.RevokedAt) + assert.Contains(t, stamps, got.RevokedAt.UTC(), "exactly one worker's stamp must land") + assert.False(t, got.RevocationInFlight) + assert.Zero(t, inFlightIndexed(ctx, t, store, id)) + }) +} + +// TestSetPlanAccounts_ProviderCheckFailsClosed covers the store-side guard of +// A04-012. DerivePlanProviders skipped "provider:service" keys (the form +// migration 000040 wrote) and legacy keys with no provider prefix, and an +// empty provider set meant "accept an account of any provider". +func TestSetPlanAccounts_ProviderCheckFailsClosed(t *testing.T) { + ctx := context.Background() + store := setupRampStepStore(ctx, t) + + azure := &CloudAccount{Name: "prod-azure-316", Provider: "azure", ExternalID: uuid.NewString()} + require.NoError(t, store.CreateCloudAccount(ctx, azure)) + + for _, tc := range []struct { + name string + services map[string]ServiceConfig + wantErr string + }{ + { + name: "colon-keyed AWS plan rejects an Azure account", + services: map[string]ServiceConfig{ + "aws:savings-plans-compute": {Provider: "aws", Service: "savings-plans", Enabled: true}, + "aws:savings-plans-ec2instance": {Provider: "aws", Service: "savings-plans", Enabled: true}, + }, + wantErr: "plan provider mismatch:", + }, + { + name: "legacy-keyed AWS plan rejects an Azure account by its provider field", + services: map[string]ServiceConfig{"original-key": {Provider: "aws", Service: "ec2", Enabled: true}}, + wantErr: "plan provider mismatch:", + }, + { + name: "plan with no identifiable provider rejects every account", + services: map[string]ServiceConfig{"ec2": {Service: "ec2", Enabled: true}}, + wantErr: "identifies a provider", + }, + } { + t.Run(tc.name, func(t *testing.T) { + plan := &PurchasePlan{Name: "plan " + tc.name, Services: tc.services} + require.NoError(t, store.CreatePurchasePlan(ctx, plan)) + + err := store.SetPlanAccounts(ctx, plan.ID, []string{azure.ID}) + require.ErrorContains(t, err, tc.wantErr) + + accounts, err := store.GetPlanAccounts(ctx, plan.ID) + require.NoError(t, err) + assert.Empty(t, accounts, "the Azure account must not be attached") + }) + } + + // A legacy alias kept under its original key (#1123's singleton-alias + // edit path) is a supported plan shape: its provider field identifies it, + // so a matching account is still accepted. + t.Run("legacy-keyed Azure plan accepts an Azure account", func(t *testing.T) { + plan := &PurchasePlan{Name: "legacy alias plan", Services: map[string]ServiceConfig{ + "original-key": {Provider: "azure", Service: "vm", Enabled: true}, + }} + require.NoError(t, store.CreatePurchasePlan(ctx, plan)) + require.NoError(t, store.SetPlanAccounts(ctx, plan.ID, []string{azure.ID})) + + accounts, err := store.GetPlanAccounts(ctx, plan.ID) + require.NoError(t, err) + require.Len(t, accounts, 1) + assert.Equal(t, azure.ID, accounts[0].ID) + }) +} diff --git a/internal/database/postgres/migrations/000027_savings_snapshots_pk.up.sql b/internal/database/postgres/migrations/000027_savings_snapshots_pk.up.sql index 409fcd88..79d7c96c 100644 --- a/internal/database/postgres/migrations/000027_savings_snapshots_pk.up.sql +++ b/internal/database/postgres/migrations/000027_savings_snapshots_pk.up.sql @@ -47,19 +47,28 @@ -- re-run 000027 will incur the same PK-rebuild cost as the original -- apply (no new concern, but worth knowing). +-- +-- PARTITIONS: `ctid` is a position within one partition's heap, so rows in +-- different `savings_snapshots_YYYY_MM` partitions can share a ctid such as +-- (0,1). The DELETE therefore matches on (tableoid, ctid); matching on ctid +-- alone also deleted the unrelated rows of every other partition that sat +-- at a duplicate's ctid (issue #316, A04-011). Duplicates of one +-- (id, timestamp) always share a partition, so the tableoid tie-break in +-- the ORDER BY never changes which row is kept. + ALTER TABLE savings_snapshots DROP CONSTRAINT IF EXISTS savings_snapshots_pkey; WITH duplicates AS ( - SELECT id, timestamp, ctid, + SELECT tableoid, ctid, ROW_NUMBER() OVER ( PARTITION BY id, timestamp - ORDER BY created_at DESC, ctid DESC + ORDER BY created_at DESC, tableoid DESC, ctid DESC ) AS row_num FROM savings_snapshots ) DELETE FROM savings_snapshots s USING duplicates d -WHERE s.ctid = d.ctid AND d.row_num > 1; +WHERE s.tableoid = d.tableoid AND s.ctid = d.ctid AND d.row_num > 1; ALTER TABLE savings_snapshots ADD CONSTRAINT savings_snapshots_pkey PRIMARY KEY (id, timestamp); diff --git a/internal/database/postgres/migrations/000059_seed_purchaser_group.down.sql b/internal/database/postgres/migrations/000059_seed_purchaser_group.down.sql index db99b728..896ddcd9 100644 --- a/internal/database/postgres/migrations/000059_seed_purchaser_group.down.sql +++ b/internal/database/postgres/migrations/000059_seed_purchaser_group.down.sql @@ -1,14 +1,45 @@ --- Reverse migration: remove Purchaser group memberships from users, --- then delete the Purchaser group, then drop the system_managed column. +-- Reverse migration: remove the Purchaser group that 000059 seeded (if it +-- seeded one), then drop the system_managed column. +-- +-- 000059's INSERT targeted 00000000-0000-5000-8000-000000000005, which +-- 000057 had already given to "Standard Users", so on every database that ran +-- both the INSERT was a silent no-op (issue #942). 000064 then created +-- Purchaser at ...0007 and owns its lifecycle (its own down is a no-op). So +-- the row at ...0005 is only ours to remove when it really is Purchaser. +-- Deleting it unconditionally destroyed Standard Users and stripped it from +-- every member (issue #316, A04-010). +-- +-- Not reversed: 000059's admin backfill added ...0005 to every Administrators +-- member. Where ...0005 is Standard Users those memberships cannot be told +-- apart from deliberate ones, so they stay. --- Remove the Purchaser group from all user group_ids arrays. -UPDATE users -SET group_ids = array_remove(group_ids, '00000000-0000-5000-8000-000000000005'::UUID) -WHERE '00000000-0000-5000-8000-000000000005'::UUID = ANY(group_ids); +DO $$ +DECLARE + v_uuid UUID := '00000000-0000-5000-8000-000000000005'; + v_stranded INT; +BEGIN + IF NOT EXISTS (SELECT 1 FROM groups WHERE id = v_uuid AND name = 'Purchaser') THEN + RETURN; + END IF; --- Delete the Purchaser group. -DELETE FROM groups -WHERE id = '00000000-0000-5000-8000-000000000005'; + -- Fail loud rather than leave a user with no group (users_min_one_group + -- would abort anyway, with a less useful message). + SELECT count(*) INTO v_stranded + FROM users + WHERE v_uuid = ANY(group_ids) + AND cardinality(array_remove(group_ids, v_uuid)) = 0; + IF v_stranded > 0 THEN + RAISE EXCEPTION + 'migration 000059 down: % user(s) belong only to Purchaser (%); give them another group before rolling back', + v_stranded, v_uuid; + END IF; --- Drop the system_managed column (rolls back the ALTER TABLE above). + UPDATE users + SET group_ids = array_remove(group_ids, v_uuid) + WHERE v_uuid = ANY(group_ids); + + DELETE FROM groups WHERE id = v_uuid AND name = 'Purchaser'; +END $$; + +-- Drop the system_managed column (rolls back the ALTER TABLE in the up). ALTER TABLE groups DROP COLUMN IF EXISTS system_managed; diff --git a/internal/database/postgres/migrations/000059_seed_purchaser_group_test.go b/internal/database/postgres/migrations/000059_seed_purchaser_group_test.go new file mode 100644 index 00000000..18fdffa4 --- /dev/null +++ b/internal/database/postgres/migrations/000059_seed_purchaser_group_test.go @@ -0,0 +1,137 @@ +//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/pgxpool" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// seedGroupDownSlotTest is the UUID 000059 tried to seed Purchaser at. 000057 +// had already given it to Standard Users, which is what it holds on every real +// database (issue #942). +const seedGroupDownSlotTest = "00000000-0000-5000-8000-000000000005" + +// pinBeforeDown hands back a migrated database pinned at version, with the +// given users inserted, ready for a rollback of that one migration. +func pinBeforeDown(ctx context.Context, t *testing.T, version uint, users map[string][]string) *pgxpool.Pool { + t.Helper() + pool := testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()).DB.Pool() + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), version)) + for email, groups := range users { + _, err := pool.Exec(ctx, ` + INSERT INTO users (id, email, password_hash, salt, active, group_ids, created_at, updated_at) + VALUES (gen_random_uuid(), $1, '', '', true, $2::uuid[], NOW(), NOW())`, email, groups) + require.NoError(t, err) + } + return pool +} + +func groupName(ctx context.Context, t *testing.T, pool *pgxpool.Pool, id string) (string, bool) { + t.Helper() + var name string + err := pool.QueryRow(ctx, `SELECT name FROM groups WHERE id = $1`, id).Scan(&name) + if err != nil { + return "", false + } + return name, true +} + +// makeSlotPurchaser models a database where 000059's seed did land at ...0005. +// Rolling back from head leaves 000064's Purchaser at ...0007 (its down is a +// no-op), so that row goes first to free the name. +func makeSlotPurchaser(ctx context.Context, t *testing.T, pool *pgxpool.Pool) { + t.Helper() + _, err := pool.Exec(ctx, `DELETE FROM groups WHERE name = 'Purchaser'`) + require.NoError(t, err) + _, err = pool.Exec(ctx, `UPDATE groups SET name = 'Purchaser' WHERE id = $1`, seedGroupDownSlotTest) + require.NoError(t, err) +} + +// TestMigration_SeedPurchaserGroup_Down covers issue #316 (A04-010). Rolling +// back 000059 deleted whatever sat at ...0005 and stripped it from every user, +// and at that point ...0005 is Standard Users, not Purchaser. +func TestMigration_SeedPurchaserGroup_Down(t *testing.T) { + ctx := context.Background() + below := previousMigrationVersion(t, 59) + + t.Run("Standard Users and its members survive the rollback", func(t *testing.T) { + const standard, admin = "standard-059-down@test.example", "admin-059-down@test.example" + pool := pinBeforeDown(ctx, t, 59, map[string][]string{ + standard: {seedGroupDownSlotTest}, + admin: {adminGroupIDForPurchaserTest, seedGroupDownSlotTest}, + }) + name, ok := groupName(ctx, t, pool, seedGroupDownSlotTest) + require.True(t, ok) + require.Equal(t, "Standard Users", name, "precondition: 000057 owns ...0005") + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), below), + "rolling back 000059 must not trip users_min_one_group for a Standard-Users-only user") + + name, ok = groupName(ctx, t, pool, seedGroupDownSlotTest) + assert.True(t, ok, "Standard Users must not be deleted by 000059's rollback") + assert.Equal(t, "Standard Users", name) + assert.ElementsMatch(t, []string{seedGroupDownSlotTest}, queryGroupIDsByEmail(t, ctx, pool, standard)) + assert.ElementsMatch(t, []string{adminGroupIDForPurchaserTest, seedGroupDownSlotTest}, queryGroupIDsByEmail(t, ctx, pool, admin)) + + var hasColumn bool + require.NoError(t, pool.QueryRow(ctx, ` + SELECT EXISTS (SELECT 1 FROM information_schema.columns + WHERE table_name = 'groups' AND column_name = 'system_managed')`).Scan(&hasColumn)) + assert.False(t, hasColumn, "the rollback still drops system_managed") + }) + + // With no Standard-Users-only user nothing tripped the CHECK, so the old + // rollback "succeeded" and silently destroyed the group. + t.Run("Standard Users is not silently destroyed", func(t *testing.T) { + const admin = "admin-059-silent@test.example" + pool := pinBeforeDown(ctx, t, 59, map[string][]string{ + admin: {adminGroupIDForPurchaserTest, seedGroupDownSlotTest}, + }) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), below)) + + name, ok := groupName(ctx, t, pool, seedGroupDownSlotTest) + assert.True(t, ok, "Standard Users must not be deleted by 000059's rollback") + assert.Equal(t, "Standard Users", name) + assert.ElementsMatch(t, []string{adminGroupIDForPurchaserTest, seedGroupDownSlotTest}, queryGroupIDsByEmail(t, ctx, pool, admin)) + }) + + // Where the seed did land (...0005 really is Purchaser), the rollback still + // removes it, and refuses rather than leave a Purchaser-only user with no + // group. + t.Run("a real Purchaser at ...0005 is still removed", func(t *testing.T) { + const admin = "admin-059-purchaser@test.example" + pool := pinBeforeDown(ctx, t, 59, map[string][]string{ + admin: {adminGroupIDForPurchaserTest, seedGroupDownSlotTest}, + }) + makeSlotPurchaser(ctx, t, pool) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), below)) + + _, ok := groupName(ctx, t, pool, seedGroupDownSlotTest) + assert.False(t, ok, "the Purchaser row 000059 seeded must be removed") + assert.ElementsMatch(t, []string{adminGroupIDForPurchaserTest}, queryGroupIDsByEmail(t, ctx, pool, admin)) + }) + + t.Run("a Purchaser-only user stops the rollback with a clear error", func(t *testing.T) { + const only = "only-purchaser-059@test.example" + pool := pinBeforeDown(ctx, t, 59, map[string][]string{only: {seedGroupDownSlotTest}}) + makeSlotPurchaser(ctx, t, pool) + + err := migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), below) + require.Error(t, err) + assert.Contains(t, err.Error(), "migration 000059 down") + + _, ok := groupName(ctx, t, pool, seedGroupDownSlotTest) + assert.True(t, ok, "nothing is removed when the rollback refuses") + assert.ElementsMatch(t, []string{seedGroupDownSlotTest}, queryGroupIDsByEmail(t, ctx, pool, only)) + }) +} diff --git a/internal/database/postgres/migrations/000096_seed_ri_exchanger_group.down.sql b/internal/database/postgres/migrations/000096_seed_ri_exchanger_group.down.sql index 353586e0..85b6b3c6 100644 --- a/internal/database/postgres/migrations/000096_seed_ri_exchanger_group.down.sql +++ b/internal/database/postgres/migrations/000096_seed_ri_exchanger_group.down.sql @@ -7,7 +7,21 @@ DO $$ DECLARE v_uuid UUID := '00000000-0000-5000-8000-000000000008'; + v_stranded INT; BEGIN + -- A user whose only group is RI Exchanger would be left with none, which + -- users_min_one_group (000057) forbids. Fail loud with an actionable + -- message instead of the bare CHECK violation (issue #316, A04-010). + SELECT count(*) INTO v_stranded + FROM users + WHERE v_uuid = ANY(COALESCE(group_ids, '{}')) + AND cardinality(array_remove(group_ids, v_uuid)) = 0; + IF v_stranded > 0 THEN + RAISE EXCEPTION + 'migration 000096 down: % user(s) belong only to RI Exchanger (%); give them another group before rolling back', + v_stranded, v_uuid; + END IF; + UPDATE users SET group_ids = array_remove(COALESCE(group_ids, '{}'), v_uuid) WHERE v_uuid = ANY(COALESCE(group_ids, '{}')); diff --git a/internal/database/postgres/migrations/000096_seed_ri_exchanger_group_test.go b/internal/database/postgres/migrations/000096_seed_ri_exchanger_group_test.go index f2c31f0e..e9b78b5d 100644 --- a/internal/database/postgres/migrations/000096_seed_ri_exchanger_group_test.go +++ b/internal/database/postgres/migrations/000096_seed_ri_exchanger_group_test.go @@ -134,3 +134,39 @@ func TestMigration_SeedRIExchangerGroup(t *testing.T) { assert.Equal(t, 1, count, "RI Exchanger must appear exactly once in group_ids") }) } + +// TestMigration_SeedRIExchangerGroup_Down covers the 000096 half of issue #316 +// (A04-010): the rollback strips RI Exchanger from every user, and a user +// whose only group it is would be left with none. users_min_one_group made +// that abort with a bare CHECK violation; the rollback now refuses up front +// and says which migration and group are in the way. +func TestMigration_SeedRIExchangerGroup_Down(t *testing.T) { + ctx := context.Background() + below := previousMigrationVersion(t, 96) + + t.Run("members with another group are detached and the group dropped", func(t *testing.T) { + const admin = "admin-096-down@test.example" + pool := pinBeforeDown(ctx, t, 96, map[string][]string{ + admin: {adminGroupIDForPurchaserTest, riExchangerGroupIDTest}, + }) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), below)) + + _, ok := groupName(ctx, t, pool, riExchangerGroupIDTest) + assert.False(t, ok, "RI Exchanger must be dropped") + assert.ElementsMatch(t, []string{adminGroupIDForPurchaserTest}, queryGroupIDsByEmail(t, ctx, pool, admin)) + }) + + t.Run("an RI-Exchanger-only user stops the rollback with a clear error", func(t *testing.T) { + const only = "only-riexchanger-096@test.example" + pool := pinBeforeDown(ctx, t, 96, map[string][]string{only: {riExchangerGroupIDTest}}) + + err := migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), below) + require.Error(t, err) + assert.Contains(t, err.Error(), "migration 000096 down") + + _, ok := groupName(ctx, t, pool, riExchangerGroupIDTest) + assert.True(t, ok, "nothing is removed when the rollback refuses") + assert.ElementsMatch(t, []string{riExchangerGroupIDTest}, queryGroupIDsByEmail(t, ctx, pool, only)) + }) +} diff --git a/internal/database/postgres/migrations/000146_purchase_history_clear_revoked_in_flight.down.sql b/internal/database/postgres/migrations/000146_purchase_history_clear_revoked_in_flight.down.sql new file mode 100644 index 00000000..4ee4b195 --- /dev/null +++ b/internal/database/postgres/migrations/000146_purchase_history_clear_revoked_in_flight.down.sql @@ -0,0 +1,8 @@ +-- Intentionally a no-op. +-- +-- The up migration cleared a flag that carried no information on revoked +-- rows (revoked_at decides whether a purchase is revoked; the flag only +-- matters while revoked_at is NULL). Which rows held it is not recorded, and +-- setting it back on every revoked row would put rows into the in-flight +-- index that the application never flagged. +SELECT 1; diff --git a/internal/database/postgres/migrations/000146_purchase_history_clear_revoked_in_flight.up.sql b/internal/database/postgres/migrations/000146_purchase_history_clear_revoked_in_flight.up.sql new file mode 100644 index 00000000..a7ae8bf8 --- /dev/null +++ b/internal/database/postgres/migrations/000146_purchase_history_clear_revoked_in_flight.up.sql @@ -0,0 +1,22 @@ +-- Clear revocation_in_flight on purchase_history rows that are already +-- revoked (issue #316, A04-013). +-- +-- Migration 000072 documents that a successful MarkPurchaseRevoked resets the +-- flag, but until #316 its UPDATE never did, so every Azure revocation that +-- completed left revocation_in_flight = true behind. Those rows read back as +-- in flight and never leave idx_purchase_history_revocation_in_flight. +-- MarkPurchaseRevoked now clears the flag; this brings existing rows in line. +-- +-- Rows with revoked_at IS NULL are not touched: a flag on an unrevoked row is +-- the finalize_revocations sweep's signal that Azure refunded but the DB write +-- failed, and clearing it would lose that reconciliation. +-- +-- Concurrency: nothing sets revoked_at back to NULL, so a row this statement +-- matches is finished and has nothing left in flight. A revoke request racing +-- the migration can at worst re-flip a revoked row; the handler reads +-- revoked_at first and that request's own failed Return clears it again. + +UPDATE purchase_history + SET revocation_in_flight = false + WHERE revocation_in_flight + AND revoked_at IS NOT NULL; diff --git a/internal/database/postgres/migrations/000146_purchase_history_clear_revoked_in_flight_test.go b/internal/database/postgres/migrations/000146_purchase_history_clear_revoked_in_flight_test.go new file mode 100644 index 00000000..778f50c7 --- /dev/null +++ b/internal/database/postgres/migrations/000146_purchase_history_clear_revoked_in_flight_test.go @@ -0,0 +1,65 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "testing" + "time" + + "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" +) + +// TestMigration_000146_ClearRevokedInFlight covers the data half of issue #316 +// (A04-013): before the fix MarkPurchaseRevoked left revocation_in_flight set, +// so every completed Azure revocation still reads back as in flight. 000146 +// clears the flag on revoked rows only; an unrevoked flagged row is the +// finalize_revocations sweep's work queue and must keep it. +func TestMigration_000146_ClearRevokedInFlight(t *testing.T) { + ctx := context.Background() + path := getMigrationsPath() + pool := testhelpers.RequireMigratedDatabase(ctx, t, path).DB.Pool() + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, previousMigrationVersion(t, 146))) + + revokedAt := time.Now().UTC() + rows := []struct { + id string + inFlight bool + revokedAt *time.Time + want bool + }{ + {"ri-146-revoked-flagged", true, &revokedAt, false}, // the stale flag + {"ri-146-pending-flagged", true, nil, true}, // sweep must still see it + {"ri-146-revoked-clear", false, &revokedAt, false}, // untouched + {"ri-146-active", false, nil, false}, // untouched + } + for _, r := range rows { + _, err := pool.Exec(ctx, ` + INSERT INTO purchase_history ( + account_id, purchase_id, timestamp, provider, service, region, + resource_type, term, payment, revocation_in_flight, revoked_at + ) VALUES ('acct', $1, NOW(), 'azure', 'compute', 'eastus', 'Standard_D2s_v3', 12, 'All Upfront', $2, $3)`, + r.id, r.inFlight, r.revokedAt) + require.NoError(t, err) + } + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, 146)) + + for _, r := range rows { + var got bool + require.NoError(t, pool.QueryRow(ctx, + `SELECT revocation_in_flight FROM purchase_history WHERE purchase_id = $1`, r.id).Scan(&got)) + assert.Equal(t, r.want, got, r.id) + } + + // The down is a deliberate no-op: it must apply and leave the rows alone. + require.NoError(t, migrations.MigrateToVersion(ctx, pool, path, previousMigrationVersion(t, 146))) + var flagged int + require.NoError(t, pool.QueryRow(ctx, + `SELECT count(*) FROM purchase_history WHERE revocation_in_flight AND purchase_id LIKE 'ri-146-%'`).Scan(&flagged)) + assert.Equal(t, 1, flagged, "only the unrevoked in-flight row stays flagged after the rollback") +} diff --git a/internal/database/postgres/migrations/savings_snapshots_pk_test.go b/internal/database/postgres/migrations/savings_snapshots_pk_test.go index fd5a12dd..25663bdc 100644 --- a/internal/database/postgres/migrations/savings_snapshots_pk_test.go +++ b/internal/database/postgres/migrations/savings_snapshots_pk_test.go @@ -5,7 +5,10 @@ package migrations_test import ( "context" + "os" + "path/filepath" "testing" + "time" "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" @@ -94,3 +97,65 @@ func TestMigration_SavingsSnapshotsPK(t *testing.T) { assert.True(t, constraintExists, "savings_snapshots_pkey should be restored after re-apply") }) } + +// TestMigration_SavingsSnapshotsPK_DedupeStaysInPartition covers issue #316 +// (A04-011). savings_snapshots is partitioned by month and ctid is a position +// within one partition's heap, so the first row of every partition sits at +// (0,1). The dedupe joined on ctid alone: deleting a duplicate at (0,1) in one +// partition also deleted the unrelated row at (0,1) of every other partition. +// The migration's own header anticipates a forced re-run, which is when that +// would hit a database that has accumulated a duplicate. +func TestMigration_SavingsSnapshotsPK_DedupeStaysInPartition(t *testing.T) { + ctx := context.Background() + pool := testhelpers.RequireMigratedDatabase(ctx, t, getMigrationsPath()).DB.Pool() + + const ( + dupID = "00000000-0000-0000-0000-000000316001" + unrelatedID = "00000000-0000-0000-0000-000000316002" + ) + jan := time.Date(2041, 1, 15, 0, 0, 0, 0, time.UTC) + feb := time.Date(2041, 2, 15, 0, 0, 0, 0, time.UTC) + _, err := pool.Exec(ctx, `SELECT create_savings_snapshot_partition('2041-01-15'), create_savings_snapshot_partition('2041-02-15')`) + require.NoError(t, err) + + // The pre-000027 state: no primary key, so duplicates can exist. + _, err = pool.Exec(ctx, `ALTER TABLE savings_snapshots DROP CONSTRAINT savings_snapshots_pkey`) + require.NoError(t, err) + + insert := func(id string, ts, createdAt time.Time) { + t.Helper() + _, insertErr := pool.Exec(ctx, ` + INSERT INTO savings_snapshots (id, account_id, timestamp, provider, service, region, commitment_type, created_at) + VALUES ($1, 'acct-316', $2, 'aws', 'ec2', 'us-east-1', 'RI', $3)`, id, ts, createdAt) + require.NoError(t, insertErr) + } + now := time.Now().UTC() + insert(dupID, jan, now.Add(-time.Hour)) // the older duplicate: deleted + insert(dupID, jan, now) // the newer duplicate: kept + insert(unrelatedID, feb, now.Add(-time.Hour)) + + ctidOf := func(where string, args ...any) string { + t.Helper() + var ctid string + require.NoError(t, pool.QueryRow(ctx, `SELECT ctid::text FROM savings_snapshots WHERE `+where, args...).Scan(&ctid)) + return ctid + } + require.Equal(t, ctidOf(`id = $1 AND created_at < $2`, dupID, now), ctidOf(`id = $1`, unrelatedID), + "precondition: the deleted duplicate and the unrelated row share a ctid in different partitions") + + upSQL, err := os.ReadFile(filepath.Join(getMigrationsPath(), "000027_savings_snapshots_pk.up.sql")) + require.NoError(t, err) + _, err = pool.Exec(ctx, string(upSQL)) + require.NoError(t, err, "re-running 000027 must succeed") + + var unrelated int + require.NoError(t, pool.QueryRow(ctx, `SELECT count(*) FROM savings_snapshots WHERE id = $1`, unrelatedID).Scan(&unrelated)) + assert.Equal(t, 1, unrelated, "a row in another partition must not be deleted by the dedupe") + + var kept int + var keptCreated time.Time + require.NoError(t, pool.QueryRow(ctx, + `SELECT count(*), max(created_at) FROM savings_snapshots WHERE id = $1`, dupID).Scan(&kept, &keptCreated)) + assert.Equal(t, 1, kept, "exactly one copy of the duplicate must remain") + assert.WithinDuration(t, now, keptCreated, time.Millisecond, "the newest copy is the one kept") +}