fix(config/db): compare-and-set the revocation in-flight flip; make 000059 re-applicable after a rollback

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

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(&current, 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)
+	})
+}

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")
 

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

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

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

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

internal/database/postgres/migrations/000059_seed_purchaser_group.up.sql

@@ -12,36 +12,53 @@
 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 (
     '00000000-0000-5000-8000-000000000001',
     '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,18 +76,22 @@ 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(
         COALESCE(group_ids, '{}') || ARRAY['00000000-0000-5000-8000-000000000005']::UUID[]
     )
 )
 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');