diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go index d48c850f..cf307d8d 100644 --- a/internal/api/handler_purchases_revoke.go +++ b/internal/api/handler_purchases_revoke.go @@ -200,15 +200,13 @@ func (h *Handler) loadAndRevokePurchaseHistory(ctx context.Context, req *events. // Partial-success reconciliation (issue #290 Finding #6): if the // revocation_in_flight flag is set but revoked_at is still NULL, Azure - // already issued the refund but our DB write failed. Return 207 so the - // frontend does not retry the Azure call (which would fail with "already - // returned"). The finalize_revocations sweep will reconcile the row. + // already issued the refund but our DB write failed, or another request + // holds the claim and is posting the Return now (issue #1518). Return 207 + // so the frontend does not retry the Azure call (which would fail with + // "already returned"). The finalize_revocations sweep will reconcile the + // row. if record.RevocationInFlight { - return &revokeReconcilePendingResult{ - Code: "RECONCILE_PENDING", - AzureReturned: true, - Message: "Refund already issued. We will reconcile your audit record shortly. Do not retry.", - }, nil + return revocationInFlightResult(), nil } // Parse the confirmed quote from the request body. Azure revocations @@ -787,8 +785,8 @@ func (h *Handler) callAzureReturn( return nil, quoteErr } - if revoked := h.flipRevocationInFlight(ctx, record); revoked != nil { - return revoked, nil + if stop, claimErr := h.claimRevocationInFlight(ctx, record); stop != nil || claimErr != nil { + return stop, claimErr } // Step 2: Return (post the actual refund request). @@ -810,30 +808,58 @@ func (h *Handler) callAzureReturn( return h.persistAzureRevocation(ctx, record, calcRefundAmount, calcRefundCurrency) } -// flipRevocationInFlight sets the in-flight flag BEFORE the Azure Return +// revocationInFlightResult is the answer for a row whose revocation another +// request has in flight: the RECONCILE_PENDING response, which tells the +// frontend not to retry. +func revocationInFlightResult() *revokeReconcilePendingResult { + return &revokeReconcilePendingResult{ + Code: "RECONCILE_PENDING", + AzureReturned: true, + Message: "Refund already issued. We will reconcile your audit record shortly. Do not retry.", + } +} + +// claimRevocationInFlight 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. +// finalize_revocations sweep rather than silently stuck. // -// 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 { +// The flip is also the claim that lets exactly one request post the Return +// (issue #1518). Both results are nil only when this request won it; every +// other outcome stops before the Return: +// - the row was revoked by a concurrent request after this one loaded it +// (issue #316): already_revoked, as the up-front idempotency check would +// have answered; +// - another request holds the claim: RECONCILE_PENDING, as the up-front +// in-flight check would have answered; +// - the flip failed: an error (500). No Return has been posted, so a retry +// is safe, whereas posting one without the claim could race the holder's +// and, if this request's MarkPurchaseRevoked then failed too, leave a +// refund that no in-flight flag points the sweep at. +// +// Because only the claim holder reaches the Return, the ClearRevocationInFlight +// in handleAzureReturnError only ever clears this request's own flag. +func (h *Handler) claimRevocationInFlight(ctx context.Context, record *config.PurchaseHistoryRecord) (any, error) { 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 + switch { + case flipErr == nil: + return nil, nil + case errors.Is(flipErr, config.ErrRevocationInFlight): + logging.Infof("revoke azure: purchase_id=%s is already being revoked by a concurrent request; not posting a second Return", record.PurchaseID) + return revocationInFlightResult(), nil + case errors.Is(flipErr, config.ErrPurchaseAlreadyRevoked): + logging.Infof("revoke azure: purchase_id=%s was revoked by a concurrent request; not posting a second Return", record.PurchaseID) + return h.alreadyRevokedResult(ctx, record.PurchaseID), nil + default: + return nil, fmt.Errorf("revoke azure: claim revocation of %s (no Return posted): %w", record.PurchaseID, flipErr) } - logging.Infof("revoke azure: purchase_id=%s was revoked by a concurrent request; not posting a second Return", record.PurchaseID) +} + +// alreadyRevokedResult answers already_revoked for a row a concurrent request +// revoked. Best-effort: the re-read stamp only fills in the response. +func (h *Handler) alreadyRevokedResult(ctx context.Context, purchaseID string) *revokePurchaseResult { 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 { + if current, err := h.config.GetPurchaseHistoryByPurchaseID(ctx, purchaseID); err == nil && current != nil && current.RevokedAt != nil { result.RevokedAt = current.RevokedAt.Format(time.RFC3339) result.RevokedVia = current.RevokedVia } @@ -896,7 +922,8 @@ func (h *Handler) azureCalculateRefund(ctx context.Context, calcClient azureCalc // handleAzureReturnError clears the in-flight flag (no refund was issued) and // maps the Return error to the right status: 422 on the 7-day window edge, 400 -// on other client errors, 500 otherwise. +// on other client errors, 500 otherwise. The caller holds the claim from +// claimRevocationInFlight, so the flag it clears is its own (issue #1518). func (h *Handler) handleAzureReturnError(ctx context.Context, record *config.PurchaseHistoryRecord, err error) error { // Azure Return failed. Clear the in-flight flag so the row is not left in // a permanently sticky state that would mislead the finalize_revocations diff --git a/internal/api/handler_purchases_revoke_claim_integration_test.go b/internal/api/handler_purchases_revoke_claim_integration_test.go new file mode 100644 index 00000000..d5aad168 --- /dev/null +++ b/internal/api/handler_purchases_revoke_claim_integration_test.go @@ -0,0 +1,179 @@ +//go:build integration + +package api + +import ( + "context" + "encoding/json" + "fmt" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + armreservations "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// revokeRequestKey tags each concurrent revoke's context with its index, so +// the stub Return client knows which request is calling it. +type revokeRequestKey struct{} + +// barrierCalcRefundClient answers CalculateRefund with a fixed quote, but +// only once every party has called it. Both revokes have loaded the row (not +// revoked, not in flight) by then, so they reach the in-flight flip together, +// as two first-time revokes would. +type barrierCalcRefundClient struct { + price *armreservations.Price + parties int32 + arrived atomic.Int32 + ready chan struct{} +} + +func (c *barrierCalcRefundClient) Post(ctx context.Context, _ string, _ armreservations.CalculateRefundRequest, _ *armreservations.CalculateRefundClientPostOptions) (armreservations.CalculateRefundClientPostResponse, error) { + if c.arrived.Add(1) == c.parties { + close(c.ready) + } + select { + case <-c.ready: + case <-ctx.Done(): + return armreservations.CalculateRefundClientPostResponse{}, ctx.Err() + } + return armreservations.CalculateRefundClientPostResponse{ + CalculateRefundResponse: armreservations.CalculateRefundResponse{ + Properties: &armreservations.RefundResponseProperties{ + SessionID: toPtr("s-claim-race"), + BillingRefundAmount: c.price, + }, + }, + }, nil +} + +// raceReturnClient stands in for Azure's Return API. The first Return +// succeeds, but only after the other request has finished, so the other +// request runs its whole path while the first one's refund is in flight. +// Azure refuses a second Return of the same reservation, so any later call +// gets a 400. While the first Return is in flight, it records whether the row +// still carries the in-flight flag. +type raceReturnClient struct { + store *config.PostgresStore + purchaseID string + done [2]chan struct{} + calls atomic.Int32 + // flagHeld is whether the row was still flagged in flight once the other + // request had finished, while the first Return was still in flight. + flagHeld atomic.Bool +} + +func (c *raceReturnClient) Post(ctx context.Context, _ string, _ armreservations.RefundRequest, _ *armreservations.ReturnClientPostOptions) (armreservations.ReturnClientPostResponse, error) { + if c.calls.Add(1) > 1 { + return armreservations.ReturnClientPostResponse{}, &azcore.ResponseError{StatusCode: 400, ErrorCode: "ReservationAlreadyReturned"} + } + me, _ := ctx.Value(revokeRequestKey{}).(int) + select { + case <-c.done[1-me]: + case <-time.After(10 * time.Second): + // The other request never finished; the assertions report it. + case <-ctx.Done(): + return armreservations.ReturnClientPostResponse{}, ctx.Err() + } + row, err := c.store.GetPurchaseHistoryByPurchaseID(context.Background(), c.purchaseID) + if err == nil && row != nil { + c.flagHeld.Store(row.RevocationInFlight) + } + return armreservations.ReturnClientPostResponse{}, nil +} + +// TestRevokeAzure_ConcurrentFirstRevokes_OneReturn covers issue #1518 on real +// Postgres. Two first-time revokes of the same Azure purchase load the row +// (unrevoked, not in flight) and reach the in-flight flip together. Before +// the fix the flip let an already-flagged row be flipped again, so both +// posted a Return; Azure refused the second, and that request's error +// handling cleared the flag while the first request's refund was still in +// flight. Had the first request's MarkPurchaseRevoked then failed, the row +// would have been left neither revoked nor flagged, out of the +// finalize_revocations sweep's reach. +// +// Now exactly one request posts a Return and records the revocation, the +// other answers RECONCILE_PENDING without posting one, and the flag stays set +// until the holder's MarkPurchaseRevoked clears it. +func TestRevokeAzure_ConcurrentFirstRevokes_OneReturn(t *testing.T) { + _, store := setupSubmitDB(t) + const rounds = 5 + for round := range rounds { + t.Run(fmt.Sprintf("round %d", round), func(t *testing.T) { + runConcurrentFirstRevokes(t, store, round) + }) + } +} + +func runConcurrentFirstRevokes(t *testing.T, store *config.PostgresStore, round int) { + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + + record := withinWindowRecord(fmt.Sprintf( + "/providers/Microsoft.Capacity/reservationOrders/order-1518-%d/reservations/res-1518-%d", round, round)) + record.AccountID = fmt.Sprintf("acct-1518-%d", round) + record.Region = "eastus" + record.ResourceType = "Standard_D2s_v3" + require.NoError(t, store.SavePurchaseHistory(ctx, record)) + + calc := &barrierCalcRefundClient{ + price: &armreservations.Price{Amount: toPtr(10.0), CurrencyCode: toPtr("USD")}, + parties: 2, + ready: make(chan struct{}), + } + ret := &raceReturnClient{store: store, purchaseID: record.PurchaseID, done: [2]chan struct{}{make(chan struct{}), make(chan struct{})}} + h := &Handler{config: store, azureRevokeFactory: &azureRevokeClientFactory{ + newCredential: func() (azcore.TokenCredential, error) { return nil, nil }, + newCalculateRefundClient: func(azcore.TokenCredential) (azureCalculateRefundClient, error) { return calc, nil }, + newReturnClient: func(azcore.TokenCredential) (azureReturnClient, error) { return ret, nil }, + }} + body, err := json.Marshal(confirmedQuote(10, "USD")) + require.NoError(t, err) + req := &events.LambdaFunctionURLRequest{Body: string(body)} + + results := make([]any, 2) + errs := make([]error, 2) + var wg sync.WaitGroup + for i := range 2 { + wg.Add(1) + go func() { + defer wg.Done() + defer close(ret.done[i]) + reqCtx := context.WithValue(ctx, revokeRequestKey{}, i) + results[i], errs[i] = h.loadAndRevokePurchaseHistory(reqCtx, req, revokeAdminSession(), record.PurchaseID) + }() + } + wg.Wait() + + assert.Equal(t, int32(1), ret.calls.Load(), "exactly one of two concurrent first revokes may post an Azure Return") + assert.True(t, ret.flagHeld.Load(), "the other request must leave the in-flight flag set while the first one's Return is in flight") + + revoked, inFlight := 0, 0 + for i := range 2 { + require.NoError(t, errs[i], "request %d", i) + switch res := results[i].(type) { + case *revokePurchaseResult: + assert.Equal(t, "revoked", res.Status, "request %d", i) + revoked++ + case *revokeReconcilePendingResult: + assert.Equal(t, revocationInFlightResult(), res, "request %d", i) + inFlight++ + default: + t.Errorf("request %d: unexpected result %T %+v", i, results[i], results[i]) + } + } + assert.Equal(t, 1, revoked, "one request revokes") + assert.Equal(t, 1, inFlight, "the other answers in flight (RECONCILE_PENDING)") + + got, err := store.GetPurchaseHistoryByPurchaseID(ctx, record.PurchaseID) + require.NoError(t, err) + require.NotNil(t, got) + assert.NotNil(t, got.RevokedAt, "the winner's MarkPurchaseRevoked must land") + assert.False(t, got.RevocationInFlight, "MarkPurchaseRevoked clears the holder's flag") +} diff --git a/internal/api/handler_purchases_revoke_guard_test.go b/internal/api/handler_purchases_revoke_guard_test.go index 84fa3fdc..33ab05bb 100644 --- a/internal/api/handler_purchases_revoke_guard_test.go +++ b/internal/api/handler_purchases_revoke_guard_test.go @@ -183,7 +183,9 @@ func confirmedQuote(amount float64, currency string) revokeConfirmBody { // 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. +// already_revoked. Since issue #1518 the flip is also the claim that lets one +// request post the Return: a row another request holds answers +// RECONCILE_PENDING, and any other flip failure stops before the Return. func TestCallAzureReturn_FlipFindsRowRevoked(t *testing.T) { t.Parallel() ctx := context.Background() @@ -215,21 +217,52 @@ func TestCallAzureReturn_FlipFindsRowRevoked(t *testing.T) { 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) { + // Issue #1518: another request holds the claim. This one must neither + // post a Return nor touch the flag (clearing it would orphan the holder's + // refund from the finalize sweep), and answers RECONCILE_PENDING like the + // up-front in-flight check. + t.Run("in flight: no Return, no clear, RECONCILE_PENDING", 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) + mockStore.On("FlipPurchaseRevocationInFlight", ctx, r.PurchaseID). + Return(fmt.Errorf("FlipPurchaseRevocationInFlight: purchase_id %q: %w", r.PurchaseID, config.ErrRevocationInFlight)) + // Registered only so a regression fails on the assertions below + // rather than on an unexpected-call panic. + mockStore.On("MarkPurchaseRevoked", ctx, r.PurchaseID, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() 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) + assert.Zero(t, returnClient.calls, "the request that loses the claim must not post a Return") + assert.Equal(t, revocationInFlightResult(), result) + mockStore.AssertNotCalled(t, "ClearRevocationInFlight", mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "MarkPurchaseRevoked", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) + + // Issue #1518: a request that could not establish the claim does not know + // whether another request holds it, so it posts no Return (a retry is + // safe) and leaves the flag alone. + t.Run("other flip error: no Return, no clear, 500", 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.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + returnClient := &stubReturnClient{} + + h := &Handler{config: mockStore} + result, err := h.callAzureReturn(ctx, &countingCalcRefundClient{price: quote}, returnClient, r, "order-abc", "res-xyz", confirmedQuote(10, "USD")) + require.Error(t, err) + assert.Nil(t, result) + assert.ErrorContains(t, err, "connection reset") + _, isClientErr := IsClientError(err) + assert.False(t, isClientErr, "a store failure is a 500, not a client error") + assert.Zero(t, returnClient.calls, "no Return without the claim") + mockStore.AssertNotCalled(t, "ClearRevocationInFlight", mock.Anything, mock.Anything) + mockStore.AssertNotCalled(t, "MarkPurchaseRevoked", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) }) } diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 306fb9fa..14c9a2cb 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1480,7 +1480,9 @@ paths: one-hour safety margin, and the body must echo the quote from GET /api/purchases/{id}/revoke/calculate. An already revoked row answers "already_revoked"; a row whose Azure refund was issued but not yet - recorded answers code RECONCILE_PENDING and must not be retried. + recorded, or is being posted by a concurrent revoke of the same + purchase, answers code RECONCILE_PENDING and must not be retried. + Only one revoke at a time posts the Azure return. parameters: - $ref: '#/components/parameters/CSRFToken' requestBody: @@ -8341,7 +8343,8 @@ components: One of three shapes. A canceled scheduled execution: status "canceled" and message. A revoked purchase history row: status "revoked" or "already_revoked" with revoked_at and revoked_via. An - Azure refund that was issued but could not be recorded yet: code + Azure refund that was issued but could not be recorded yet, or that a + concurrent revoke of the same purchase is posting: code RECONCILE_PENDING, azure_returned and message (do not retry). properties: status: diff --git a/internal/config/errors.go b/internal/config/errors.go index 691f86bb..32dcb090 100644 --- a/internal/config/errors.go +++ b/internal/config/errors.go @@ -18,6 +18,13 @@ var ErrEmptyPurchaseID = errors.New("purchase history record has no purchase id" // start another provider return (issue #316). var ErrPurchaseAlreadyRevoked = errors.New("purchase is already revoked") +// ErrRevocationInFlight is returned (wrapped) by +// FlipPurchaseRevocationInFlight when the purchase_history row is unrevoked +// but another request already holds the in-flight claim: that request is +// posting (or has posted) the provider return, so the caller must not post +// another one (issue #1518). +var ErrRevocationInFlight = errors.New("purchase revocation is already in flight") + // 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 0cc6a9a4..65099876 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -416,14 +416,16 @@ type StoreInterface interface { // it is already non-null). Used by the revoke endpoint (issue #290). MarkPurchaseRevoked(ctx context.Context, purchaseID string, revokedAt time.Time, revokedVia string, supportCaseID string, calcRefundAmount *float64, calcRefundCurrency string) error - // FlipPurchaseRevocationInFlight atomically sets revocation_in_flight=true - // on a purchase_history row. Called immediately before the Azure Return API - // 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). 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 atomically claims a purchase_history row + // by setting revocation_in_flight=true. Called immediately before the Azure + // Return API 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). Only an + // unrevoked, unflagged row is flipped, so exactly one concurrent caller + // wins: a revoked row returns an error wrapping ErrPurchaseAlreadyRevoked + // (issue #316), and a row another request already flagged returns an error + // wrapping ErrRevocationInFlight (issue #1518). Returns a not-found error + // when no row matches. FlipPurchaseRevocationInFlight(ctx context.Context, purchaseID string) error // ClearRevocationInFlight resets revocation_in_flight=false on a @@ -432,7 +434,9 @@ type StoreInterface interface { // left in a permanently-sticky in-flight state that would prevent future // retries or mislead the finalize_revocations sweep (issue #290, second-wave // CR Finding D). No-op when the row is already false. Best-effort: callers - // should log on error but not surface it to the user. + // should log on error but not surface it to the user. Only the caller whose + // FlipPurchaseRevocationInFlight succeeded may call it, so it never clears + // a flag another request set (issue #1518). ClearRevocationInFlight(ctx context.Context, purchaseID string) error // GetPurchaseHistoryInFlight returns all purchase_history rows with diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index f0907564..fb28430c 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -3040,27 +3040,32 @@ func (s *PostgresStore) MarkPurchaseRevoked(ctx context.Context, purchaseID stri return nil } -// 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: an unrevoked row whose flag is already true is -// left as it is. +// FlipPurchaseRevocationInFlight claims the revocation of the +// purchase_history row for purchaseID by setting revocation_in_flight=true. +// Called immediately before the Azure Return API call to enable +// partial-success reconciliation (issue #290 Finding #6). // -// 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. +// It is a compare-and-set on two predicates: +// - revoked_at IS NULL (issue #316): a stale request that reaches the flip +// after another request's MarkPurchaseRevoked committed must not flag the +// revoked row again, since MarkPurchaseRevoked (guarded on the same +// predicate) would then never clear it. That returns an error wrapping +// ErrPurchaseAlreadyRevoked. +// - revocation_in_flight = false (issue #1518): only one request may hold +// the claim, so two first-time revokes cannot both post a Return. The +// request that loses gets an error wrapping ErrRevocationInFlight. +// +// The row lock serializes concurrent flips and marks, and under READ +// COMMITTED a waiting UPDATE re-checks both predicates against the committed +// row, so exactly one flip of an unrevoked, unflagged row succeeds. 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 + AND revocation_in_flight = false `, purchaseID) if err != nil { return fmt.Errorf("FlipPurchaseRevocationInFlight: %w", err) @@ -3068,21 +3073,35 @@ func (s *PostgresStore) FlipPurchaseRevocationInFlight(ctx context.Context, purc 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 { + // Nothing matched: tell a missing row from a revoked one and from one + // another request has claimed. A row that is unrevoked when re-read lost + // the claim to a concurrent request; it is reported in flight even if + // that request has since cleared the flag, so this caller never posts a + // Return it did not claim. + var total, unrevoked int + if err := s.db.QueryRow(ctx, ` + SELECT count(*), count(*) FILTER (WHERE revoked_at IS NULL) + FROM purchase_history + WHERE purchase_id = $1 + `, purchaseID).Scan(&total, &unrevoked); err != nil { return fmt.Errorf("FlipPurchaseRevocationInFlight existence check: %w", err) } - if !exists { + if total == 0 { return fmt.Errorf("FlipPurchaseRevocationInFlight: purchase_id %q not found", purchaseID) } - return fmt.Errorf("FlipPurchaseRevocationInFlight: purchase_id %q: %w", purchaseID, ErrPurchaseAlreadyRevoked) + if unrevoked == 0 { + return fmt.Errorf("FlipPurchaseRevocationInFlight: purchase_id %q: %w", purchaseID, ErrPurchaseAlreadyRevoked) + } + return fmt.Errorf("FlipPurchaseRevocationInFlight: purchase_id %q: %w", purchaseID, ErrRevocationInFlight) } // ClearRevocationInFlight sets revocation_in_flight=false on a purchase_history // row. Called when the Azure Return call fails transiently (before Azure actually // issued a refund) so the row is not left stuck in the in-flight state, which // would mislead the finalize_revocations sweep into thinking Azure succeeded -// (issue #290, second-wave CR Finding D). No-op when already false. +// (issue #290, second-wave CR Finding D). No-op when already false. The +// UPDATE is unconditional: only the request whose +// FlipPurchaseRevocationInFlight claimed the row may call it (issue #1518). func (s *PostgresStore) ClearRevocationInFlight(ctx context.Context, purchaseID string) error { tag, err := s.db.Exec(ctx, ` UPDATE purchase_history diff --git a/internal/config/store_postgres_issue316_integration_test.go b/internal/config/store_postgres_issue316_integration_test.go index 0a84b1ab..2e072535 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" + "errors" "fmt" "sync" "testing" @@ -148,11 +149,14 @@ func TestFlipPurchaseRevocationInFlight_CompareAndSet(t *testing.T) { assert.Zero(t, inFlightIndexed(ctx, t, store, id)) }) - t.Run("an unrevoked row still flips, idempotently", func(t *testing.T) { + t.Run("an unrevoked row flips once; a second flip finds it in flight", 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") + err := store.FlipPurchaseRevocationInFlight(ctx, id) + require.ErrorIs(t, err, ErrRevocationInFlight, "a second flip of an in-flight row must not claim it again (issue #1518)") + assert.NotErrorIs(t, err, ErrPurchaseAlreadyRevoked) + assert.NotContains(t, err.Error(), "not found") assert.Equal(t, 1, inFlightIndexed(ctx, t, store, id)) }) @@ -160,6 +164,7 @@ func TestFlipPurchaseRevocationInFlight_CompareAndSet(t *testing.T) { err := store.FlipPurchaseRevocationInFlight(ctx, "azure-flip-316-missing") require.ErrorContains(t, err, "not found") assert.NotErrorIs(t, err, ErrPurchaseAlreadyRevoked) + assert.NotErrorIs(t, err, ErrRevocationInFlight) }) // The race the guard closes: one request's MarkPurchaseRevoked against @@ -167,7 +172,8 @@ func TestFlipPurchaseRevocationInFlight_CompareAndSet(t *testing.T) { // 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. + // flip reports the row either still in flight (it ran before the mark; + // the row was already claimed, issue #1518) or revoked. t.Run("stale flips racing a mark", func(t *testing.T) { const rounds, flips = 20, 4 for round := range rounds { @@ -195,7 +201,8 @@ func TestFlipPurchaseRevocationInFlight_CompareAndSet(t *testing.T) { require.NoError(t, errs[0], "round %d: the mark", round) for i, err := range errs[1:] { - if err != nil { + require.Error(t, err, "round %d: flip %d must not claim a row that is already in flight", round, i) + if !errors.Is(err, ErrRevocationInFlight) { require.ErrorIs(t, err, ErrPurchaseAlreadyRevoked, "round %d: flip %d", round, i) } } @@ -207,6 +214,61 @@ func TestFlipPurchaseRevocationInFlight_CompareAndSet(t *testing.T) { }) } +// TestFlipPurchaseRevocationInFlight_ExclusiveClaim covers issue #1518: the +// flip let a row that was already in flight be flipped again, so two +// first-time revokes could both pass it and both post an Azure Return. Now +// it is a claim: of any number of concurrent flips of one unrevoked, +// unflagged row exactly one succeeds and the rest get ErrRevocationInFlight. +// After the holder clears the flag (its Return failed) the row can be +// claimed again. +func TestFlipPurchaseRevocationInFlight_ExclusiveClaim(t *testing.T) { + ctx := context.Background() + store := setupRampStepStore(ctx, t) + + t.Run("concurrent first flips: exactly one claims the row", func(t *testing.T) { + const rounds, flips = 20, 6 + for round := range rounds { + id := fmt.Sprintf("azure-flip-1518-race-%d", round) + seedRevocationRow(ctx, t, store, id) + + errs := make([]error, flips) + start := make(chan struct{}) + var wg sync.WaitGroup + for i := range flips { + wg.Add(1) + go func() { + defer wg.Done() + <-start + errs[i] = store.FlipPurchaseRevocationInFlight(ctx, id) + }() + } + close(start) + wg.Wait() + + winners := 0 + for i, err := range errs { + if err == nil { + winners++ + continue + } + require.ErrorIs(t, err, ErrRevocationInFlight, "round %d: flip %d", round, i) + } + require.Equal(t, 1, winners, "round %d: exactly one concurrent flip may claim the row", round) + require.Equal(t, 1, inFlightIndexed(ctx, t, store, id), "round %d", round) + } + }) + + t.Run("the holder's clear releases the claim", func(t *testing.T) { + const id = "azure-flip-1518-release" + seedRevocationRow(ctx, t, store, id) + require.NoError(t, store.FlipPurchaseRevocationInFlight(ctx, id)) + require.ErrorIs(t, store.FlipPurchaseRevocationInFlight(ctx, id), ErrRevocationInFlight) + require.NoError(t, store.ClearRevocationInFlight(ctx, id)) + require.NoError(t, store.FlipPurchaseRevocationInFlight(ctx, id), "a cleared row can be claimed by a later revoke") + assert.Equal(t, 1, 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