From 85d688403cf1a3b4f9081816804e790f84b8d497 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 20:47:21 +0000 Subject: [PATCH] fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback FlipPurchaseRevocationInFlight was an unconditional UPDATE, so a stale concurrent Azure revoke reaching it after another request's MarkPurchaseRevoked set revocation_in_flight on a revoked row, where nothing clears it. The flip now only matches revoked_at IS NULL; a revoked row returns ErrPurchaseAlreadyRevoked (a missing row keeps its not-found error), and the handler answers already_revoked without posting a second Azure Return. Rolling back through 000059 and re-applying it failed: 000064's down is a no-op, so Purchaser stays at ...0007 and 000059's guard refused it as a foreign "Purchaser". 000059 now yields to that row (and restores its system_managed flag), and its admin backfill only targets ...0005 when that row really is Purchaser, so a re-apply no longer adds admins to Standard Users. Adds a 000059 down/up round-trip test and a down-to-version-1 round trip. No new migration. Refs #316 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_purchases_revoke.go | 39 +++++-- .../handler_purchases_revoke_guard_test.go | 56 ++++++++++ internal/config/errors.go | 6 ++ internal/config/interfaces.go | 4 +- internal/config/store_postgres.go | 26 ++++- ...tore_postgres_issue316_integration_test.go | 86 +++++++++++++++ .../000059_seed_purchaser_group.down.sql | 7 +- .../000059_seed_purchaser_group.up.sql | 35 ++++-- .../000059_seed_purchaser_group_test.go | 101 ++++++++++++++++++ .../migrations/migrate_down_all_test.go | 21 ++++ 10 files changed, 359 insertions(+), 22 deletions(-) diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go index a5ae9b027..d48c850f8 100644 --- a/internal/api/handler_purchases_revoke.go +++ b/internal/api/handler_purchases_revoke.go @@ -787,13 +787,8 @@ func (h *Handler) callAzureReturn( return nil, quoteErr } - // Partial-success guard (issue #290 Finding #6): flip the in-flight flag - // BEFORE calling Azure Return so that if the subsequent MarkPurchaseRevoked - // write fails, the row is visible to the finalize_revocations sweep rather - // than silently stuck. Best-effort: if the flip itself fails, log and - // continue — the in-flight flag is a safety net, not a hard precondition. - if flipErr := h.config.FlipPurchaseRevocationInFlight(ctx, record.PurchaseID); flipErr != nil { - logging.Warnf("revoke azure: FlipPurchaseRevocationInFlight for %s failed (continuing): %v", record.PurchaseID, flipErr) + if revoked := h.flipRevocationInFlight(ctx, record); revoked != nil { + return revoked, nil } // Step 2: Return (post the actual refund request). @@ -815,6 +810,36 @@ func (h *Handler) callAzureReturn( return h.persistAzureRevocation(ctx, record, calcRefundAmount, calcRefundCurrency) } +// flipRevocationInFlight sets the in-flight flag BEFORE the Azure Return +// (partial-success guard, issue #290 Finding #6) so that if the subsequent +// MarkPurchaseRevoked write fails, the row is visible to the +// finalize_revocations sweep rather than silently stuck. Best-effort: if the +// flip itself fails, log and continue — the in-flight flag is a safety net, +// not a hard precondition. +// +// The one exception is a row the flip found already revoked (issue #316): a +// concurrent request finished the revocation after this one loaded the row, +// so no second Return is posted and the caller answers already_revoked, as +// the up-front idempotency check would have. The result is nil otherwise. +func (h *Handler) flipRevocationInFlight(ctx context.Context, record *config.PurchaseHistoryRecord) *revokePurchaseResult { + flipErr := h.config.FlipPurchaseRevocationInFlight(ctx, record.PurchaseID) + if flipErr == nil { + return nil + } + if !errors.Is(flipErr, config.ErrPurchaseAlreadyRevoked) { + logging.Warnf("revoke azure: FlipPurchaseRevocationInFlight for %s failed (continuing): %v", record.PurchaseID, flipErr) + return nil + } + logging.Infof("revoke azure: purchase_id=%s was revoked by a concurrent request; not posting a second Return", record.PurchaseID) + result := &revokePurchaseResult{Status: "already_revoked"} + // Best-effort: the stamp only fills in the response. + if current, err := h.config.GetPurchaseHistoryByPurchaseID(ctx, record.PurchaseID); err == nil && current != nil && current.RevokedAt != nil { + result.RevokedAt = current.RevokedAt.Format(time.RFC3339) + result.RevokedVia = current.RevokedVia + } + return result +} + // checkConfirmedRefund is the TOCTOU-divergence check: the refund the user // confirmed must match Azure's current CalculateRefund quote in currency and, // within revokeQuoteEpsilon, in amount. A mismatch means the quote changed diff --git a/internal/api/handler_purchases_revoke_guard_test.go b/internal/api/handler_purchases_revoke_guard_test.go index 003cf5cf2..84fa3fdcd 100644 --- a/internal/api/handler_purchases_revoke_guard_test.go +++ b/internal/api/handler_purchases_revoke_guard_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "testing" + "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" armreservations "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" @@ -177,3 +178,58 @@ func TestRevokePurchase_AzureRefundGuard(t *testing.T) { func confirmedQuote(amount float64, currency string) revokeConfirmBody { return revokeConfirmBody{ExpectedRefundAmount: &amount, ExpectedRefundCurrency: currency} } + +// TestCallAzureReturn_FlipFindsRowRevoked covers issue #316: the in-flight +// flip is now a compare-and-set on revoked_at IS NULL. When it reports the +// row already revoked, a concurrent request finished the revocation after +// this one loaded the row, so no second Return is posted and the answer is +// already_revoked. Any other flip failure stays best-effort. +func TestCallAzureReturn_FlipFindsRowRevoked(t *testing.T) { + t.Parallel() + ctx := context.Background() + quote := &armreservations.Price{Amount: toPtr(10.0), CurrencyCode: toPtr("USD")} + + t.Run("already revoked: no Return, already_revoked", func(t *testing.T) { + t.Parallel() + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + r := armReservationRecord() + revokedAt := time.Date(2026, 10, 26, 12, 0, 0, 0, time.UTC) + mockStore.On("FlipPurchaseRevocationInFlight", ctx, r.PurchaseID). + Return(fmt.Errorf("FlipPurchaseRevocationInFlight: purchase_id %q: %w", r.PurchaseID, config.ErrPurchaseAlreadyRevoked)) + current := *r + current.RevokedAt = &revokedAt + current.RevokedVia = "direct-api" + mockStore.On("GetPurchaseHistoryByPurchaseID", ctx, r.PurchaseID).Return(¤t, nil) + returnClient := &stubReturnClient{} + + h := &Handler{config: mockStore} + result, err := h.callAzureReturn(ctx, &countingCalcRefundClient{price: quote}, returnClient, r, "order-abc", "res-xyz", confirmedQuote(10, "USD")) + require.NoError(t, err) + assert.Zero(t, returnClient.calls, "a revoked row must not get a second Return") + res, ok := result.(*revokePurchaseResult) + require.True(t, ok, "got %T", result) + assert.Equal(t, "already_revoked", res.Status) + assert.Equal(t, revokedAt.Format(time.RFC3339), res.RevokedAt) + assert.Equal(t, "direct-api", res.RevokedVia) + mockStore.AssertNotCalled(t, "MarkPurchaseRevoked", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) + + t.Run("other flip error: still returns and marks", func(t *testing.T) { + t.Parallel() + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + r := armReservationRecord() + mockStore.On("FlipPurchaseRevocationInFlight", ctx, r.PurchaseID).Return(fmt.Errorf("FlipPurchaseRevocationInFlight: connection reset")) + mockStore.On("MarkPurchaseRevoked", ctx, r.PurchaseID, mock.AnythingOfType("time.Time"), "direct-api", "", mock.Anything, mock.Anything).Return(nil) + returnClient := &stubReturnClient{} + + h := &Handler{config: mockStore} + result, err := h.callAzureReturn(ctx, &countingCalcRefundClient{price: quote}, returnClient, r, "order-abc", "res-xyz", confirmedQuote(10, "USD")) + require.NoError(t, err) + assert.Equal(t, 1, returnClient.calls) + res, ok := result.(*revokePurchaseResult) + require.True(t, ok, "got %T", result) + assert.Equal(t, "revoked", res.Status) + }) +} diff --git a/internal/config/errors.go b/internal/config/errors.go index cd60fa293..691f86bbc 100644 --- a/internal/config/errors.go +++ b/internal/config/errors.go @@ -12,6 +12,12 @@ var ErrNotFound = errors.New("not found") // so an empty ID would be silently dropped after the first one (#774). var ErrEmptyPurchaseID = errors.New("purchase history record has no purchase id") +// ErrPurchaseAlreadyRevoked is returned (wrapped) by +// FlipPurchaseRevocationInFlight when the purchase_history row exists but is +// already revoked: a concurrent revoke finished first, so the caller must not +// start another provider return (issue #316). +var ErrPurchaseAlreadyRevoked = errors.New("purchase is already revoked") + // ErrPurchasePlanConflict means a plan changed after the caller read it. var ErrPurchasePlanConflict = errors.New("purchase plan changed") diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 4046e1969..6efba46fc 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -415,7 +415,9 @@ type StoreInterface interface { // call so that the row can be identified by the finalize sweep if the // subsequent MarkPurchaseRevoked DB write fails (partial-success reconciliation, // issue #290 Finding #6, migration 000072). No-op when the flag is already - // true (idempotent). Returns a not-found error when no row matches. + // true (idempotent). Only an unrevoked row is flipped: a revoked one + // returns an error wrapping ErrPurchaseAlreadyRevoked (issue #316). + // Returns a not-found error when no row matches. FlipPurchaseRevocationInFlight(ctx context.Context, purchaseID string) error // ClearRevocationInFlight resets revocation_in_flight=false on a diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index 2d6ade951..83357b4fd 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -2974,21 +2974,39 @@ func (s *PostgresStore) MarkPurchaseRevoked(ctx context.Context, purchaseID stri // FlipPurchaseRevocationInFlight sets revocation_in_flight=true on the // purchase_history row for purchaseID. Called immediately before the Azure // Return API call to enable partial-success reconciliation (issue #290 -// Finding #6). Idempotent: already-true rows are not modified. Returns a -// not-found error when no row matches. +// Finding #6). Idempotent: an unrevoked row whose flag is already true is +// left as it is. +// +// Compare-and-set on revoked_at IS NULL (issue #316): only an unrevoked row +// can be flipped. A stale concurrent revoke request that reaches the flip +// after another request's MarkPurchaseRevoked committed would otherwise set +// the flag again on a revoked row, which MarkPurchaseRevoked (guarded on the +// same predicate) then never clears. The row lock serializes the two +// UPDATEs, and the flip re-checks the predicate on the committed row, so the +// flag ends false whichever lands first. A revoked row returns an error +// wrapping ErrPurchaseAlreadyRevoked; a missing row keeps the not-found +// error. func (s *PostgresStore) FlipPurchaseRevocationInFlight(ctx context.Context, purchaseID string) error { tag, err := s.db.Exec(ctx, ` UPDATE purchase_history SET revocation_in_flight = true WHERE purchase_id = $1 + AND revoked_at IS NULL `, purchaseID) if err != nil { return fmt.Errorf("FlipPurchaseRevocationInFlight: %w", err) } - if tag.RowsAffected() == 0 { + if tag.RowsAffected() > 0 { + return nil + } + var exists bool + if err := s.db.QueryRow(ctx, `SELECT EXISTS(SELECT 1 FROM purchase_history WHERE purchase_id = $1)`, purchaseID).Scan(&exists); err != nil { + return fmt.Errorf("FlipPurchaseRevocationInFlight existence check: %w", err) + } + if !exists { return fmt.Errorf("FlipPurchaseRevocationInFlight: purchase_id %q not found", purchaseID) } - return nil + return fmt.Errorf("FlipPurchaseRevocationInFlight: purchase_id %q: %w", purchaseID, ErrPurchaseAlreadyRevoked) } // ClearRevocationInFlight sets revocation_in_flight=false on a purchase_history diff --git a/internal/config/store_postgres_issue316_integration_test.go b/internal/config/store_postgres_issue316_integration_test.go index 8eb6c732f..0a84b1abb 100644 --- a/internal/config/store_postgres_issue316_integration_test.go +++ b/internal/config/store_postgres_issue316_integration_test.go @@ -6,6 +6,7 @@ package config import ( "context" + "fmt" "sync" "testing" "time" @@ -121,6 +122,91 @@ func TestMarkPurchaseRevoked_ClearsRevocationInFlight(t *testing.T) { }) } +// TestFlipPurchaseRevocationInFlight_CompareAndSet covers the follow-up to +// A04-013 tracked on issue #316: the flip was an unconditional UPDATE, so a +// stale revoke request reaching it after another request's +// MarkPurchaseRevoked set the flag again on a revoked row, and +// MarkPurchaseRevoked (guarded on revoked_at IS NULL) never cleared it. +func TestFlipPurchaseRevocationInFlight_CompareAndSet(t *testing.T) { + ctx := context.Background() + store := setupRampStepStore(ctx, t) + + t.Run("a revoked row is refused and keeps the flag clear", func(t *testing.T) { + const id = "azure-flip-316-a" + seedRevocationRow(ctx, t, store, id) + require.NoError(t, store.FlipPurchaseRevocationInFlight(ctx, id)) + require.NoError(t, store.MarkPurchaseRevoked(ctx, id, time.Now().UTC(), "direct-api", "", nil, "")) + + err := store.FlipPurchaseRevocationInFlight(ctx, id) + require.ErrorIs(t, err, ErrPurchaseAlreadyRevoked) + assert.NotContains(t, err.Error(), "not found") + + got, err := store.GetPurchaseHistoryByPurchaseID(ctx, id) + require.NoError(t, err) + require.NotNil(t, got.RevokedAt) + assert.False(t, got.RevocationInFlight, "a revoked row must not be flagged in flight again") + assert.Zero(t, inFlightIndexed(ctx, t, store, id)) + }) + + t.Run("an unrevoked row still flips, idempotently", func(t *testing.T) { + const id = "azure-flip-316-b" + seedRevocationRow(ctx, t, store, id) + require.NoError(t, store.FlipPurchaseRevocationInFlight(ctx, id)) + require.NoError(t, store.FlipPurchaseRevocationInFlight(ctx, id), "a second flip of an in-flight row is a no-op") + assert.Equal(t, 1, inFlightIndexed(ctx, t, store, id)) + }) + + t.Run("a missing row is still not found", func(t *testing.T) { + err := store.FlipPurchaseRevocationInFlight(ctx, "azure-flip-316-missing") + require.ErrorContains(t, err, "not found") + assert.NotErrorIs(t, err, ErrPurchaseAlreadyRevoked) + }) + + // The race the guard closes: one request's MarkPurchaseRevoked against + // stale requests' flips on the same in-flight row. The row lock + // serializes them; a flip that waits on the mark re-checks + // revoked_at IS NULL on the committed row and matches nothing. Whatever + // the interleaving, the row ends revoked with the flag clear, and every + // flip either landed before the mark or reports the row revoked. + t.Run("stale flips racing a mark", func(t *testing.T) { + const rounds, flips = 20, 4 + for round := range rounds { + id := fmt.Sprintf("azure-flip-316-race-%d", round) + seedRevocationRow(ctx, t, store, id) + require.NoError(t, store.FlipPurchaseRevocationInFlight(ctx, id)) + + errs := make([]error, flips+1) + start := make(chan struct{}) + var wg sync.WaitGroup + for i := range flips + 1 { + wg.Add(1) + go func() { + defer wg.Done() + <-start + if i == 0 { + errs[i] = store.MarkPurchaseRevoked(ctx, id, time.Now().UTC(), "direct-api", "", nil, "") + return + } + errs[i] = store.FlipPurchaseRevocationInFlight(ctx, id) + }() + } + close(start) + wg.Wait() + + require.NoError(t, errs[0], "round %d: the mark", round) + for i, err := range errs[1:] { + if err != nil { + require.ErrorIs(t, err, ErrPurchaseAlreadyRevoked, "round %d: flip %d", round, i) + } + } + got, err := store.GetPurchaseHistoryByPurchaseID(ctx, id) + require.NoError(t, err) + require.NotNil(t, got.RevokedAt, "round %d", round) + require.False(t, got.RevocationInFlight, "round %d: a flip that lost to the mark must not re-flag the revoked row", round) + } + }) +} + // 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 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 896ddcd9b..f030ad471 100644 --- a/internal/database/postgres/migrations/000059_seed_purchaser_group.down.sql +++ b/internal/database/postgres/migrations/000059_seed_purchaser_group.down.sql @@ -9,9 +9,10 @@ -- 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. +-- Not reversed: before issue #316's fix, 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. The up +-- now backfills only when ...0005 really is Purchaser. DO $$ DECLARE diff --git a/internal/database/postgres/migrations/000059_seed_purchaser_group.up.sql b/internal/database/postgres/migrations/000059_seed_purchaser_group.up.sql index 0c66eec62..6db02e1ff 100644 --- a/internal/database/postgres/migrations/000059_seed_purchaser_group.up.sql +++ b/internal/database/postgres/migrations/000059_seed_purchaser_group.up.sql @@ -12,6 +12,12 @@ ALTER TABLE groups ADD COLUMN IF NOT EXISTS system_managed BOOLEAN NOT NULL DEFAULT FALSE; -- Mark the four existing seed groups as system-managed. +-- +-- Re-applying this migration after a rollback (issue #316): 000064 moved +-- Purchaser to ...0007 and its down is a no-op, so after a rollback through +-- this migration that row is still there, and this migration's down dropped +-- the system_managed column. Restore the flag on it too. On a forward run +-- ...0007 does not exist yet, so this matches nothing extra. UPDATE groups SET system_managed = TRUE WHERE id IN ( @@ -19,29 +25,40 @@ WHERE id IN ( '00000000-0000-5000-8000-000000000002', '00000000-0000-5000-8000-000000000003', '00000000-0000-5000-8000-000000000004' -); +) + OR (id = '00000000-0000-5000-8000-000000000007' AND name = 'Purchaser'); -- Fail closed if a pre-existing group already owns the name "Purchaser" -- under a different UUID. Without this guard the bare ON CONFLICT DO -- NOTHING below would silently skip the seed and leave -- DefaultPurchaserGroupID absent, breaking the admin-backfill UPDATE -- further down and every callsite that keys off the fixed UUID. +-- The one exception is 000064's own Purchaser at ...0007, left behind by a +-- rollback through this migration (issue #316): 000064 owns that row, so the +-- seed below yields to it instead of refusing. DO $$ BEGIN IF EXISTS ( SELECT 1 FROM groups WHERE name = 'Purchaser' - AND id <> '00000000-0000-5000-8000-000000000005' + AND id NOT IN ( + '00000000-0000-5000-8000-000000000005', + '00000000-0000-5000-8000-000000000007' + ) ) THEN RAISE EXCEPTION 'migration 000059: a group named ''Purchaser'' already exists with a different id; rename it before applying this migration so the seeded id (00000000-0000-5000-8000-000000000005) can be created'; END IF; END $$; --- Insert the Purchaser group. Idempotent on the seeded id only -- a --- name-collision on a different id is caught by the DO block above so --- the seed never silently goes missing. +-- Insert the Purchaser group. A name-collision on any id other than +-- ...0005 and ...0007 is caught by the DO block above so the seed never +-- silently goes missing. The bare ON CONFLICT covers the two remaining +-- cases: ...0005 already taken (by Standard Users from 000057 on every real +-- database, issue #942) and Purchaser already at ...0007 (a re-apply after a +-- rollback, issue #316), where groups.name's UNIQUE constraint would +-- otherwise abort the migration. INSERT INTO groups (id, name, description, permissions, allowed_accounts, system_managed) VALUES ( '00000000-0000-5000-8000-000000000005', @@ -59,13 +76,17 @@ VALUES ( ARRAY['*'], TRUE ) -ON CONFLICT (id) DO NOTHING; +ON CONFLICT DO NOTHING; -- Auto-assign every existing admin-group member to the Purchaser group -- so upgrade preserves current behavior. Admins can later remove -- themselves to enforce strict separation of duties. -- We drive off group membership (Administrators group UUID) rather than -- the legacy role column (which has been dropped in migration 000057). +-- Only when ...0005 really is Purchaser: where 000057's Standard Users holds +-- it, this backfill used to add every admin to Standard Users instead +-- (issue #942), and on a re-apply after a rollback it would do so again for +-- every admin created since (issue #316). 000064 backfills the real Purchaser. UPDATE users SET group_ids = ARRAY( SELECT DISTINCT unnest( @@ -73,4 +94,4 @@ SET group_ids = ARRAY( ) ) WHERE '00000000-0000-5000-8000-000000000001'::UUID = ANY(COALESCE(group_ids, '{}')) - AND EXISTS (SELECT 1 FROM groups WHERE id = '00000000-0000-5000-8000-000000000005'); + AND EXISTS (SELECT 1 FROM groups WHERE id = '00000000-0000-5000-8000-000000000005' AND name = 'Purchaser'); diff --git a/internal/database/postgres/migrations/000059_seed_purchaser_group_test.go b/internal/database/postgres/migrations/000059_seed_purchaser_group_test.go index 18fdffa48..6e295c725 100644 --- a/internal/database/postgres/migrations/000059_seed_purchaser_group_test.go +++ b/internal/database/postgres/migrations/000059_seed_purchaser_group_test.go @@ -135,3 +135,104 @@ func TestMigration_SeedPurchaserGroup_Down(t *testing.T) { assert.ElementsMatch(t, []string{seedGroupDownSlotTest}, queryGroupIDsByEmail(t, ctx, pool, only)) }) } + +// groupsSnapshot is every group row, in id order, as "id|name|system_managed". +func groupsSnapshot(ctx context.Context, t *testing.T, pool *pgxpool.Pool) []string { + t.Helper() + rows, err := pool.Query(ctx, `SELECT id::text || '|' || name || '|' || system_managed::text FROM groups ORDER BY id`) + require.NoError(t, err) + defer rows.Close() + var out []string + for rows.Next() { + var row string + require.NoError(t, rows.Scan(&row)) + out = append(out, row) + } + require.NoError(t, rows.Err()) + return out +} + +// TestMigration_SeedPurchaserGroup_DownUpRoundTrip covers the round trip +// tracked on issue #316: 000064's down is a no-op, so rolling back through +// 000059 leaves Purchaser at ...0007, and re-applying 000059 refused because +// a group named Purchaser already existed under an id other than ...0005. +// The round trip must succeed and leave the groups, including the relocated +// Purchaser's system_managed flag, and the users' memberships as they were: +// an admin who is not in Standard Users must not be added to it by 000059's +// admin backfill, which only ever meant Purchaser. +func TestMigration_SeedPurchaserGroup_DownUpRoundTrip(t *testing.T) { + ctx := context.Background() + + t.Run("Standard Users at ...0005", func(t *testing.T) { + const admin, standard = "admin-059-roundtrip@test.example", "standard-059-roundtrip@test.example" + const lateAdmin = "late-admin-059-roundtrip@test.example" + pool := pinBeforeDown(ctx, t, 64, map[string][]string{ + admin: {adminGroupIDForPurchaserTest, seedGroupDownSlotTest, purchaserGroupIDTest}, + standard: {seedGroupDownSlotTest}, + lateAdmin: {adminGroupIDForPurchaserTest, purchaserGroupIDTest}, + }) + assertSeedPurchaserRoundTrip(ctx, t, pool) + assert.ElementsMatch(t, []string{adminGroupIDForPurchaserTest, seedGroupDownSlotTest, purchaserGroupIDTest}, queryGroupIDsByEmail(t, ctx, pool, admin)) + assert.ElementsMatch(t, []string{seedGroupDownSlotTest}, queryGroupIDsByEmail(t, ctx, pool, standard)) + assert.ElementsMatch(t, []string{adminGroupIDForPurchaserTest, purchaserGroupIDTest}, queryGroupIDsByEmail(t, ctx, pool, lateAdmin), + "the re-applied backfill must not add an admin to Standard Users") + }) + + // With ...0005 free, the re-applied seed no longer hits its id conflict + // and would insert a second "Purchaser", which groups.name's UNIQUE + // constraint refuses. + t.Run("...0005 free", func(t *testing.T) { + const admin = "admin-059-roundtrip-free@test.example" + pool := pinBeforeDown(ctx, t, 64, map[string][]string{ + admin: {adminGroupIDForPurchaserTest, purchaserGroupIDTest}, + }) + _, err := pool.Exec(ctx, `DELETE FROM groups WHERE id = $1`, seedGroupDownSlotTest) + require.NoError(t, err) + assertSeedPurchaserRoundTrip(ctx, t, pool) + _, ok := groupName(ctx, t, pool, seedGroupDownSlotTest) + assert.False(t, ok, "the re-applied seed must not create a group at ...0005") + assert.ElementsMatch(t, []string{adminGroupIDForPurchaserTest, purchaserGroupIDTest}, queryGroupIDsByEmail(t, ctx, pool, admin)) + }) +} + +// The re-apply yields only to 000064's Purchaser at ...0007. A "Purchaser" +// under any other id is still someone else's group, and 000059 still refuses +// rather than silently skip its seed. +func TestMigration_SeedPurchaserGroup_ForeignPurchaserStillRefused(t *testing.T) { + ctx := context.Background() + pool := pinBeforeDown(ctx, t, 64, nil) + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), previousMigrationVersion(t, 59))) + _, err := pool.Exec(ctx, `DELETE FROM groups WHERE id = $1`, purchaserGroupIDTest) + require.NoError(t, err) + _, err = pool.Exec(ctx, ` + INSERT INTO groups (id, name, description, permissions, allowed_accounts) + VALUES ('00000000-0000-4000-8000-000000000316', 'Purchaser', 'operator-made', '[]'::jsonb, ARRAY['*'])`) + require.NoError(t, err) + + err = migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), 64) + require.Error(t, err) + assert.Contains(t, err.Error(), "migration 000059: a group named 'Purchaser' already exists with a different id") +} + +// assertSeedPurchaserRoundTrip rolls a database pinned at 000064 back through +// 000059 and forward again, and checks every group came back unchanged. +func assertSeedPurchaserRoundTrip(ctx context.Context, t *testing.T, pool *pgxpool.Pool) { + t.Helper() + name, ok := groupName(ctx, t, pool, purchaserGroupIDTest) + require.True(t, ok) + require.Equal(t, "Purchaser", name, "precondition: 000064 put Purchaser at ...0007") + groupsBefore := groupsSnapshot(ctx, t, pool) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), previousMigrationVersion(t, 59)), + "rolling back through 000059 must succeed") + name, ok = groupName(ctx, t, pool, purchaserGroupIDTest) + require.True(t, ok, "precondition: 000064's no-op down leaves Purchaser at ...0007") + require.Equal(t, "Purchaser", name) + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, getMigrationsPath(), 64), + "re-applying 000059 after its rollback must succeed") + _, dirty, err := migrations.GetMigrationVersion(ctx, pool, getMigrationsPath()) + require.NoError(t, err) + require.False(t, dirty) + assert.Equal(t, groupsBefore, groupsSnapshot(ctx, t, pool), "the round trip must leave every group as it was") +} diff --git a/internal/database/postgres/migrations/migrate_down_all_test.go b/internal/database/postgres/migrations/migrate_down_all_test.go index 96fece35b..bd17254e4 100644 --- a/internal/database/postgres/migrations/migrate_down_all_test.go +++ b/internal/database/postgres/migrations/migrate_down_all_test.go @@ -47,3 +47,24 @@ func TestMigrations_DownAllRoundTrip(t *testing.T) { require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, latest), "re-applying every migration after a full down must succeed") } + +// TestMigrations_DownToOneRoundTrip rolls back to version 1 rather than 0, so +// the tables 000001 created survive with the rows later migrations seeded, +// then re-applies everything. 000064's down is a no-op by design, so its +// Purchaser at ...0007 is still there when 000059 re-runs, and 000059 refused +// it as a foreign "Purchaser" (issue #316). +func TestMigrations_DownToOneRoundTrip(t *testing.T) { + t.Parallel() + ctx := context.Background() + migrationsPath := getMigrationsPath() + pool := testhelpers.RequireMigratedDatabase(ctx, t, migrationsPath).DB.Pool() + latest := migrationVersionsDesc(t)[0] + + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, 1), "roll back to version 1") + require.NoError(t, migrations.MigrateToVersion(ctx, pool, migrationsPath, latest), + "re-applying every migration after a rollback to version 1 must succeed") + version, dirty, err := migrations.GetMigrationVersion(ctx, pool, migrationsPath) + require.NoError(t, err) + require.False(t, dirty) + require.Equal(t, latest, version) +}