diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go index d7ebd387..db258827 100644 --- a/internal/api/handler_purchases_revoke.go +++ b/internal/api/handler_purchases_revoke.go @@ -28,6 +28,7 @@ import ( "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" armreservations "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" @@ -240,10 +241,11 @@ 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, 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. + // holds the claim and is posting the Return now (issue #1518), or an + // earlier Return failed without a definite answer and may have been + // accepted (issue #1529). 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 revocationInFlightResult(), nil } @@ -742,6 +744,13 @@ func (h *Handler) getAzureRevokeFactory() azureRevokeClientFactory { if h.azureRevokeFactory != nil { return *h.azureRevokeFactory } + return defaultAzureRevokeFactory(nil) +} + +// defaultAzureRevokeFactory builds the production clients. returnOpts is nil +// in production; tests point it at a local server to exercise the real Return +// client's pipeline, including returnAttemptPolicy. +func defaultAzureRevokeFactory(returnOpts *arm.ClientOptions) azureRevokeClientFactory { return azureRevokeClientFactory{ newCredential: func() (azcore.TokenCredential, error) { return azidentity.NewDefaultAzureCredential(nil) @@ -750,7 +759,7 @@ func (h *Handler) getAzureRevokeFactory() azureRevokeClientFactory { return armreservations.NewCalculateRefundClient(cred, nil) }, newReturnClient: func(cred azcore.TokenCredential) (azureReturnClient, error) { - return armreservations.NewReturnClient(cred, nil) + return newAzureReturnClient(cred, returnOpts) }, } } @@ -831,8 +840,8 @@ func (h *Handler) callAzureReturn( } // Step 2: Return (post the actual refund request). - if err := postAzureReturn(ctx, returnClient, orderID, reservationID, sessionID, quantity); err != nil { - return nil, h.handleAzureReturnError(ctx, record, err) + if attempts, err := postAzureReturn(ctx, returnClient, orderID, reservationID, sessionID, quantity); err != nil { + return nil, h.handleAzureReturnError(ctx, record, attempts, err) } return h.persistAzureRevocation(ctx, record, calcRefundAmount, calcRefundCurrency) @@ -845,9 +854,14 @@ func (h *Handler) callAzureReturn( // the RI exchange handler does once its claim is held (#790). A request // canceled mid-Return therefore still learns that Azure accepted it and // records it. -func postAzureReturn(reqCtx context.Context, returnClient azureReturnClient, orderID, reservationID, sessionID string, quantity int32) error { +// +// It also reports how many HTTP requests the Return client sent, counted by +// returnAttemptPolicy, so a failure can be judged definite or uncertain +// (issue #1529). +func postAzureReturn(reqCtx context.Context, returnClient azureReturnClient, orderID, reservationID, sessionID string, quantity int32) (int32, error) { ctx, cancel := ctxutil.DetachedWithTimeout(reqCtx, revokeReturnTimeout) defer cancel() + ctx, sends := withReturnAttemptCounter(ctx) _, err := returnClient.Post(ctx, orderID, armreservations.RefundRequest{ Properties: &armreservations.RefundRequestProperties{ ReservationToReturn: &armreservations.ReservationToReturn{ @@ -859,7 +873,7 @@ func postAzureReturn(reqCtx context.Context, returnClient azureReturnClient, ord Scope: toPtr("Reservation"), }, }, nil) - return err + return sends.Load(), err } // revocationInFlightResult is the answer for a row whose revocation another @@ -893,7 +907,14 @@ func revocationInFlightResult() *revokeReconcilePendingResult { // // Because only the claim holder reaches the Return, the ClearRevocationInFlight // in handleAzureReturnError only ever clears this request's own flag. +// +// On Lambda the claim is not taken at all when too little invocation time is +// left to post the Return and record its result (checkRevokeTimeLeft, issue +// #1529). func (h *Handler) claimRevocationInFlight(ctx context.Context, record *config.PurchaseHistoryRecord) (any, error) { + if err := h.checkRevokeTimeLeft(ctx, record.PurchaseID); err != nil { + return nil, err + } flipErr := h.config.FlipPurchaseRevocationInFlight(ctx, record.PurchaseID) switch { case flipErr == nil: @@ -974,18 +995,26 @@ func (h *Handler) azureCalculateRefund(ctx context.Context, calcClient azureCalc return sessionID, calcRefundAmount, calcRefundCurrency, nil } -// 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. The caller holds the claim from -// claimRevocationInFlight, so the flag it clears is its own (issue #1518). +// handleAzureReturnError handles a failed Return. Only a definite refusal +// (isAzureDefiniteRefusal) proves no refund was issued; any other failure +// keeps the in-flight flag and answers that the outcome is uncertain +// (azureReturnUncertain, issue #1529). +// +// After a definite refusal it clears the in-flight flag and maps the error to +// the right status: 422 on the 7-day window edge, 400 on other client errors, +// 500 otherwise. The caller holds the claim from claimRevocationInFlight, so +// the flag it clears is its own (issue #1518). // // The Return ran detached, so by the time it fails the request may be gone; // the clear runs on its own detached revokePersistTimeout budget so it can // still land (issue #1527). -func (h *Handler) handleAzureReturnError(reqCtx context.Context, record *config.PurchaseHistoryRecord, err error) error { +func (h *Handler) handleAzureReturnError(reqCtx context.Context, record *config.PurchaseHistoryRecord, attempts int32, err error) error { + if !isAzureDefiniteRefusal(err, attempts) { + return azureReturnUncertain(record, attempts, err) + } ctx, cancel := ctxutil.DetachedWithTimeout(reqCtx, revokePersistTimeout) defer cancel() - // Azure Return failed. Clear the in-flight flag so the row is not left in + // Azure refused the Return. Clear the in-flight flag so the row is not left in // a permanently sticky state that would mislead the finalize_revocations // sweep into thinking Azure already issued a refund (Finding D, second-wave // CR). Best-effort: log and continue even if the clear fails. diff --git a/internal/api/handler_purchases_revoke_claim_integration_test.go b/internal/api/handler_purchases_revoke_claim_integration_test.go index c9b0da5d..4b4fa69d 100644 --- a/internal/api/handler_purchases_revoke_claim_integration_test.go +++ b/internal/api/handler_purchases_revoke_claim_integration_test.go @@ -249,7 +249,8 @@ func TestRevokeAzure_RequestCanceledAfterReturn_RecordsAndRevokes(t *testing.T) // TestRevokeAzure_RequestCanceledDuringFailedReturn_ClearsFlag covers issue // #1527's error path on real Postgres: the request is canceled while the -// Return is in flight and the Return then fails (Azure answered 503). The +// Return is in flight and Azure then refuses the Return (a definite 400; since +// issue #1529 an ambiguous failure keeps the flag on purpose). The // in-flight clear must still land, or the row stays flagged and unrevocable // until the sweep releases it 2h later. Before the fix the clear ran on the // canceled request context and pgx refused it. @@ -271,7 +272,7 @@ func TestRevokeAzure_RequestCanceledDuringFailedReturn_ClearsFlag(t *testing.T) parties: 1, ready: make(chan struct{}), } - ret := &cancelAfterReturnClient{cancel: cancelReq, err: &azcore.ResponseError{StatusCode: 503}} + ret := &cancelAfterReturnClient{cancel: cancelReq, err: &azcore.ResponseError{StatusCode: 400, ErrorCode: "InvalidReservationID"}} h := &Handler{config: store, azureRevokeFactory: &azureRevokeClientFactory{ newCredential: func() (azcore.TokenCredential, error) { return nil, nil }, newCalculateRefundClient: func(azcore.TokenCredential) (azureCalculateRefundClient, error) { return calc, nil }, diff --git a/internal/api/handler_purchases_revoke_detached_test.go b/internal/api/handler_purchases_revoke_detached_test.go index 421163d2..6caac1c4 100644 --- a/internal/api/handler_purchases_revoke_detached_test.go +++ b/internal/api/handler_purchases_revoke_detached_test.go @@ -37,10 +37,11 @@ func TestRevokePersistScheduleFitsBudget(t *testing.T) { assert.LessOrEqual(t, schedule, revokePersistTimeout) } -// A Return that fails after the request was canceled under it must still +// A Return Azure refuses after the request was canceled under it must still // clear this request's in-flight flag, on a live context: before the fix the // clear ran on the dead request context, failed, and left the row flagged -// until the sweep released it 2h later. +// until the sweep released it 2h later. The refusal is a definite 4xx: since +// issue #1529 an ambiguous failure (a 5xx, say) keeps the flag on purpose. func TestCallAzureReturn_RequestCanceledDuringFailedReturn_ClearsInFlight(t *testing.T) { t.Parallel() reqCtx, cancel := context.WithCancel(context.Background()) @@ -53,13 +54,14 @@ func TestCallAzureReturn_RequestCanceledDuringFailedReturn_ClearsInFlight(t *tes mockStore.On("ClearRevocationInFlight", mock.Anything, r.PurchaseID).Return(nil).Once() calcClient := &stubCalcRefundClientWithAmount{amount: 10.0, currency: "USD", sessID: "s-cancel-fail"} - returnClient := &cancelAfterReturnClient{cancel: cancel, err: &azcore.ResponseError{StatusCode: 503}} + returnClient := &cancelAfterReturnClient{cancel: cancel, err: &azcore.ResponseError{StatusCode: 400, ErrorCode: "InvalidReservationID"}} h := &Handler{config: store} start := time.Now() _, err := h.callAzureReturn(reqCtx, calcClient, returnClient, r, "order-abc", "res-xyz", confirmedQuote(10, "USD")) require.Error(t, err) - _, isClientErr := IsClientError(err) - assert.False(t, isClientErr, "a 503 from Azure is a server-side failure") + ce, isClientErr := IsClientError(err) + require.True(t, isClientErr, "a 400 from Azure is a client error") + assert.Equal(t, 400, ce.code) returnClient.assertDetached(t, start) require.ErrorIs(t, reqCtx.Err(), context.Canceled) diff --git a/internal/api/handler_purchases_revoke_test.go b/internal/api/handler_purchases_revoke_test.go index ba45c9ce..d75c182f 100644 --- a/internal/api/handler_purchases_revoke_test.go +++ b/internal/api/handler_purchases_revoke_test.go @@ -320,9 +320,11 @@ func TestRevokePurchase_AzureReturnClientError(t *testing.T) { h := &Handler{config: mockStore} _, err := h.callAzureReturn(ctx, calcClient, returnClient, r, "order-abc", "res-xyz", confirmedQuote(10, "USD")) require.Error(t, err) - // 500 is not a client error -- expect wrapped error, not ClientError. - _, isClient := IsClientError(err) - assert.False(t, isClient, "server-side Azure errors should not be wrapped as 4xx ClientError") + // A server-side failure is not a 4xx: Azure may have taken the Return, so + // the answer is the uncertain 500 (issue #1529). + ce, isClient := IsClientError(err) + require.True(t, isClient, "want the uncertain-outcome ClientError, got %v", err) + assert.Equal(t, 500, ce.code, "server-side Azure errors must not be reported as 4xx") } // TestRevokePurchase_UsesStampedWindow asserts the window check reads @@ -1462,10 +1464,11 @@ func TestCallAzureReturn_RefundPolicyViolatedReturns422WindowEdge(t *testing.T) // --- Finding D: revocation_in_flight sticky on Azure error paths (second-wave CR) --- -// TestCallAzureReturn_TransientError_ClearsInFlight verifies that when the -// Azure Return call fails with a transient (non-window-edge, non-client) error, -// callAzureReturn calls ClearRevocationInFlight so the row is not left sticky. -func TestCallAzureReturn_TransientError_ClearsInFlight(t *testing.T) { +// TestCallAzureReturn_TransientError_KeepsInFlight verifies that when the +// Azure Return call fails with a transport error, which says nothing about +// whether Azure took the Return, the in-flight flag is kept (issue #1529) and +// the answer is the uncertain 500. Before #1529 the flag was cleared. +func TestCallAzureReturn_TransientError_KeepsInFlight(t *testing.T) { t.Parallel() ctx := context.Background() mockStore := new(MockConfigStore) @@ -1475,14 +1478,15 @@ func TestCallAzureReturn_TransientError_ClearsInFlight(t *testing.T) { returnClient := &stubReturnClient{err: errors.New("dial tcp: connection refused")} r := armReservationRecord() - mockStore.On("ClearRevocationInFlight", mock.Anything, r.PurchaseID).Return(nil).Once() h := &Handler{config: mockStore} _, err := h.callAzureReturn(ctx, calcClient, returnClient, r, "order-abc", "res-xyz", confirmedQuote(10, "USD")) require.Error(t, err) - // Must not be a ClientError -- transient errors surface as a 500 from the caller. - _, isClientErr := IsClientError(err) - assert.False(t, isClientErr, "transient Azure error must not be a ClientError") + ce, isClientErr := IsClientError(err) + require.True(t, isClientErr, "want the uncertain-outcome ClientError, got %v", err) + assert.Equal(t, 500, ce.code) + assert.Contains(t, ce.message, "verify the reservation state in the Azure portal") + mockStore.AssertNotCalled(t, "ClearRevocationInFlight") mockStore.AssertExpectations(t) } diff --git a/internal/api/handler_purchases_revoke_uncertain.go b/internal/api/handler_purchases_revoke_uncertain.go new file mode 100644 index 00000000..17f65ba7 --- /dev/null +++ b/internal/api/handler_purchases_revoke_uncertain.go @@ -0,0 +1,147 @@ +package api + +// handler_purchases_revoke_uncertain.go holds the two guards issue #1529 adds +// around the Azure Return in the revoke handler: +// +// - a Return that fails without a definite answer from Azure keeps the +// in-flight flag and tells the user the outcome is uncertain, as the RI +// exchange handler does for its ambiguous provider failures; +// - on Lambda, the claim is refused when too little invocation time is left +// to post the Return and record its result. + +import ( + "context" + "errors" + "net/http" + "sync/atomic" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + armreservations "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// revokeResponseMargin is the invocation time kept back, past the detached +// Return and the write that follows it, for building and returning the +// response. Nothing after that write talks to Azure or the database, so a few +// seconds is plenty; the 30s lambdatimeout.Margin also has to cover the work +// before the claim, which by the time of this check has already run. +const revokeResponseMargin = 5 * time.Second + +// revokeClaimMinRemaining is the least invocation time a Lambda revoke must +// have left to take the in-flight claim (issue #1529). Past the claim the +// handler runs the detached Return (revokeReturnTimeout), then exactly one of +// the writes recording an accepted Return or the in-flight clear after a +// refused one (each bounded by revokePersistTimeout; an uncertain failure +// writes nothing), then the response. Lambda ends the invocation at its +// timeout whatever a context says, so a claim taken with less time left could +// be cut off mid-Return or before the result is recorded. 3m35s with the +// current budgets. +const revokeClaimMinRemaining = revokeReturnTimeout + revokePersistTimeout + revokeResponseMargin + +// errRevokeTooLittleTime is the message for a claim refused for lack of time. +const errRevokeTooLittleTime = "not enough time was left in this request to return the reservation safely; " + + "nothing was sent to Azure and the purchase is unchanged, so retry the revoke" + +// errRevokeReturnUncertain mirrors the RI exchange handler's wording for an +// ambiguous provider failure (executeAzureExchange). +const errRevokeReturnUncertain = "Azure reservation return failed and may already have been accepted by Azure; " + + "verify the reservation state in the Azure portal before retrying" + +// checkRevokeTimeLeft refuses the claim on Lambda when the invocation has less +// than revokeClaimMinRemaining left. It runs before the claim, so a refusal +// leaves no flag behind and posts no Return; 503 says the request can simply +// be retried. +// +// Only Lambda needs it: elsewhere the process outlives the request, so the +// detached Return and writes finish after the request's 30s deadline +// (internal/server/http.go), and that deadline already guards the claim +// itself, which runs on the request context. +func (h *Handler) checkRevokeTimeLeft(ctx context.Context, purchaseID string) error { + if !h.isLambda { + return nil + } + deadline, ok := ctx.Deadline() + if !ok { + return nil + } + if left := time.Until(deadline); left < revokeClaimMinRemaining { + logging.Warnf("revoke azure: purchase_id=%s refused before the claim: %s of invocation time left, need %s (no Return posted)", + purchaseID, left.Round(time.Millisecond), revokeClaimMinRemaining) + return NewClientError(http.StatusServiceUnavailable, errRevokeTooLittleTime) + } + return nil +} + +// returnAttemptsKey carries a *atomic.Int32 that returnAttemptPolicy bumps once +// per HTTP request the Return client sends, retries included. +type returnAttemptsKey struct{} + +// withReturnAttemptCounter returns ctx carrying a fresh send counter. +func withReturnAttemptCounter(ctx context.Context) (context.Context, *atomic.Int32) { + n := new(atomic.Int32) + return context.WithValue(ctx, returnAttemptsKey{}, n), n +} + +// returnAttemptPolicy counts the sends of a Return. azcore retries a POST on +// transport errors and on 408, 429 and 5xx, so a 4xx that ends the call may +// answer a retry of a try Azure already accepted (a connection reset after +// the 202, say). Only a 4xx from the one and only send is a definite refusal. +// It sits among the per-retry policies, after the bearer-token policy, so it +// counts only requests that reach the transport. +type returnAttemptPolicy struct{} + +func (returnAttemptPolicy) Do(req *policy.Request) (*http.Response, error) { + if n, ok := req.Raw().Context().Value(returnAttemptsKey{}).(*atomic.Int32); ok { + n.Add(1) + } + return req.Next() +} + +// newAzureReturnClient builds the armreservations Return client with +// returnAttemptPolicy installed. opts may be nil; it is not modified. +func newAzureReturnClient(cred azcore.TokenCredential, opts *arm.ClientOptions) (*armreservations.ReturnClient, error) { + o := arm.ClientOptions{} + if opts != nil { + o = *opts + } + o.PerRetryPolicies = append(append([]policy.Policy(nil), o.PerRetryPolicies...), returnAttemptPolicy{}) + return armreservations.NewReturnClient(cred, &o) +} + +// isAzureDefiniteRefusal reports whether a failed Return is known not to have +// been accepted: Azure answered the only request sent with a 4xx other than +// 408 Request Timeout. Everything else (a transport error, a 5xx, the +// revokeReturnTimeout deadline, a response that could not be read, a 4xx +// after a retry, or any status other than the 202 the SDK expects) leaves +// open whether Azure took the Return. +// +// attempts is the send count from returnAttemptPolicy. A client built without +// the policy reports 0 and is judged on the status alone. +func isAzureDefiniteRefusal(err error, attempts int32) bool { + if attempts > 1 { + return false + } + var respErr *azcore.ResponseError + if !errors.As(err, &respErr) { + return false + } + code := respErr.StatusCode + return code >= 400 && code < 500 && code != http.StatusRequestTimeout +} + +// azureReturnUncertain handles a Return that failed without a definite answer +// (issue #1529). The in-flight flag stays set, so no request can post a +// second Return for this purchase, and the user is told to check the Azure +// portal. If no Return gets recorded, the finalize_revocations sweep clears +// the flag after its 2h stale window and logs the row for an operator. +func azureReturnUncertain(record *config.PurchaseHistoryRecord, attempts int32, err error) error { + logging.Errorf("revoke azure: purchase_id=%s account_id=%s Return failed without a definite answer after %d send(s), outcome uncertain: %v. "+ + "Kept the in-flight flag so no second Return is posted; finalize_revocations releases it after 2h if no Return is recorded. "+ + "If Azure did accept the Return, reconcile the row by hand", + record.PurchaseID, record.AccountID, attempts, err) + return NewClientError(http.StatusInternalServerError, errRevokeReturnUncertain) +} diff --git a/internal/api/handler_purchases_revoke_uncertain_integration_test.go b/internal/api/handler_purchases_revoke_uncertain_integration_test.go new file mode 100644 index 00000000..53ab5279 --- /dev/null +++ b/internal/api/handler_purchases_revoke_uncertain_integration_test.go @@ -0,0 +1,104 @@ +//go:build integration + +package api + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/LeanerCloud/cloud-commitments-platform/internal/purchase" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestRevokeAzure_UncertainReturn_FlagKeptUntilSweepReleases covers issue +// #1529 on real Postgres, from the revoke request to the sweep. Azure answers +// the Return with a 503, which does not say whether it took the Return: +// +// - the row stays flagged in flight with no recorded Return and not revoked, +// and the user is told the outcome is uncertain (before the fix the flag +// was cleared and the answer was a flat failure); +// - a retry is answered RECONCILE_PENDING and posts no second Return; +// - the finalize_revocations sweep leaves the fresh flag alone, and once it +// is older than the 2h stale window releases it without marking the +// purchase revoked. +// +// Stub Azure clients; nothing calls Azure. +func TestRevokeAzure_UncertainReturn_FlagKeptUntilSweepReleases(t *testing.T) { + pg, store := setupSubmitDB(t) + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + + record := withinWindowRecord("/providers/Microsoft.Capacity/reservationOrders/order-1529/reservations/res-1529") + record.AccountID = "acct-1529" + record.Region = "eastus" + record.ResourceType = "Standard_D2s_v3" + require.NoError(t, store.SavePurchaseHistory(ctx, record)) + + calc := &stubCalcRefundClientWithAmount{amount: 10.0, currency: "USD", sessID: "s-1529"} + ret := &stubReturnClient{err: &azcore.ResponseError{StatusCode: 503, ErrorCode: "ServiceUnavailable"}} + 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)} + + type rowState struct { + returnPostedAt, revokedAt *time.Time + inFlight bool + } + load := func() rowState { + t.Helper() + var s rowState + require.NoError(t, pg.DB.Pool().QueryRow(ctx, ` + SELECT return_posted_at, revoked_at, revocation_in_flight + FROM purchase_history WHERE purchase_id = $1`, record.PurchaseID). + Scan(&s.returnPostedAt, &s.revokedAt, &s.inFlight)) + return s + } + + _, err = h.loadAndRevokePurchaseHistory(ctx, req, revokeAdminSession(), record.PurchaseID) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "want the uncertain-outcome ClientError, got %v", err) + assert.Equal(t, 500, ce.code) + assert.Equal(t, errRevokeReturnUncertain, ce.message) + got := load() + assert.True(t, got.inFlight, "an uncertain Return must keep the in-flight flag") + assert.Nil(t, got.returnPostedAt, "an uncertain Return is not recorded as accepted") + assert.Nil(t, got.revokedAt, "an uncertain Return does not revoke the purchase") + + result, err := h.loadAndRevokePurchaseHistory(ctx, req, revokeAdminSession(), record.PurchaseID) + require.NoError(t, err) + pending, ok := result.(*revokeReconcilePendingResult) + require.True(t, ok, "want RECONCILE_PENDING on retry, got %T %+v", result, result) + assert.Equal(t, "RECONCILE_PENDING", pending.Code) + assert.Equal(t, 1, ret.calls, "the retry must not post a second Return") + + sweeper := purchase.NewManager(purchase.ManagerConfig{ConfigStore: store}) + _, err = sweeper.FinalizeInFlightRevocations(ctx) + require.NoError(t, err) + got = load() + assert.True(t, got.inFlight, "the sweep leaves a fresh uncertain flag alone") + assert.Nil(t, got.revokedAt, "the sweep must not revoke a purchase with no recorded Return") + + _, err = pg.DB.Pool().Exec(ctx, ` + UPDATE purchase_history + SET revocation_in_flight_at = now() - interval '2 hours 1 minute' + WHERE purchase_id = $1`, record.PurchaseID) + require.NoError(t, err) + res, err := sweeper.FinalizeInFlightRevocations(ctx) + require.NoError(t, err) + assert.GreaterOrEqual(t, res.Released, 1) + got = load() + assert.False(t, got.inFlight, "past the stale window the sweep releases the flag") + assert.Nil(t, got.revokedAt, "a released row is not revoked") + assert.Nil(t, got.returnPostedAt) +} diff --git a/internal/api/handler_purchases_revoke_uncertain_test.go b/internal/api/handler_purchases_revoke_uncertain_test.go new file mode 100644 index 00000000..ecdfebb5 --- /dev/null +++ b/internal/api/handler_purchases_revoke_uncertain_test.go @@ -0,0 +1,370 @@ +package api + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/url" + "sync/atomic" + "testing" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/LeanerCloud/cloud-commitments-platform/internal/testutil/lambdatimeout" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Issue #1529. Stub Azure clients, or the real armreservations Return client +// pointed at a local TLS server, and a mock store; nothing here calls Azure. + +// assertUncertainReturn checks the uncertain-outcome answer: a 500 telling the +// user to check the Azure portal, the in-flight flag kept, and nothing +// recorded or revoked. +func assertUncertainReturn(t *testing.T, mockStore *MockConfigStore, err error) { + t.Helper() + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "want the uncertain-outcome ClientError, got %v", err) + assert.Equal(t, 500, ce.code) + assert.Equal(t, errRevokeReturnUncertain, ce.message) + mockStore.AssertCalled(t, "FlipPurchaseRevocationInFlight") + mockStore.AssertNotCalled(t, "ClearRevocationInFlight") + mockStore.AssertNotCalled(t, "MarkRevocationReturnPosted") + mockStore.AssertNotCalled(t, "MarkPurchaseRevoked") +} + +// A Return that fails without a definite answer may have been accepted by +// Azure, so the flag that stops a second Return must stay set. Before #1529 +// every one of these cleared the flag and answered a flat failure, so a +// refunded purchase could read as active and failed. +func TestCallAzureReturn_AmbiguousFailure_KeepsFlagAndAnswersUncertain(t *testing.T) { + t.Parallel() + cases := map[string]error{ + "500": &azcore.ResponseError{StatusCode: 500, ErrorCode: "InternalServerError"}, + "502": &azcore.ResponseError{StatusCode: 502}, + "503": &azcore.ResponseError{StatusCode: 503}, + "504": &azcore.ResponseError{StatusCode: 504, ErrorCode: "GatewayTimeout"}, + "408 request timeout": &azcore.ResponseError{StatusCode: 408}, + "unexpected 200": &azcore.ResponseError{StatusCode: 200}, + "Return deadline": context.DeadlineExceeded, + "Return deadline wrapped": &url.Error{Op: "Post", URL: "https://management.azure.com/x", Err: context.DeadlineExceeded}, + "transport error": errors.New("dial tcp: connection reset by peer"), + "response cut off": fmt.Errorf("reading response body: %w", io.ErrUnexpectedEOF), + } + for name, returnErr := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + mockStore := new(MockConfigStore) + calcClient := &stubCalcRefundClientWithAmount{amount: 10.0, currency: "USD", sessID: "s-uncertain"} + returnClient := &stubReturnClient{err: returnErr} + h := &Handler{config: mockStore} + + _, err := h.callAzureReturn(context.Background(), calcClient, returnClient, armReservationRecord(), "order-abc", "res-xyz", confirmedQuote(10, "USD")) + assertUncertainReturn(t, mockStore, err) + assert.Equal(t, 1, returnClient.calls, "exactly one Return") + }) + } +} + +// A definite refusal (Azure answered the only request with a 4xx) proves no +// refund was issued, so the flag is cleared and the purchase can be revoked +// again, as before #1529. +func TestCallAzureReturn_DefiniteRefusal_ClearsFlag(t *testing.T) { + t.Parallel() + cases := map[string]struct { + err error + code int + }{ + "400": {&azcore.ResponseError{StatusCode: 400, ErrorCode: "InvalidReservationID"}, 400}, + "403": {&azcore.ResponseError{StatusCode: 403, ErrorCode: "AuthorizationFailed"}, 400}, + "409": {&azcore.ResponseError{StatusCode: 409, ErrorCode: "ReservationAlreadyReturned"}, 400}, + "window edge": {&azcore.ResponseError{StatusCode: 400, ErrorCode: "RefundPolicyViolated"}, 422}, + "401": {&azcore.ResponseError{StatusCode: 401, ErrorCode: "InvalidAuthenticationToken"}, 0}, + "429": {&azcore.ResponseError{StatusCode: 429, ErrorCode: "TooManyRequests"}, 0}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + r := armReservationRecord() + mockStore.On("ClearRevocationInFlight", mock.Anything, r.PurchaseID).Return(nil).Once() + calcClient := &stubCalcRefundClientWithAmount{amount: 10.0, currency: "USD", sessID: "s-refused"} + h := &Handler{config: mockStore} + + _, err := h.callAzureReturn(context.Background(), calcClient, &stubReturnClient{err: tc.err}, r, "order-abc", "res-xyz", confirmedQuote(10, "USD")) + require.Error(t, err) + ce, isClient := IsClientError(err) + if tc.code == 0 { + assert.False(t, isClient, "a refusal without a mapped status stays a plain 500, got %v", err) + } else { + require.True(t, isClient, "got %v", err) + assert.Equal(t, tc.code, ce.code) + } + assert.NotEqual(t, errRevokeReturnUncertain, err.Error(), "a definite refusal is not uncertain") + mockStore.AssertNotCalled(t, "MarkRevocationReturnPosted") + mockStore.AssertNotCalled(t, "MarkPurchaseRevoked") + }) + } +} + +func TestIsAzureDefiniteRefusal_AttemptsAndStatus(t *testing.T) { + t.Parallel() + refused := &azcore.ResponseError{StatusCode: 409} + assert.True(t, isAzureDefiniteRefusal(refused, 0), "a client without the counting policy is judged on the status") + assert.True(t, isAzureDefiniteRefusal(refused, 1)) + assert.True(t, isAzureDefiniteRefusal(fmt.Errorf("wrapped: %w", refused), 1)) + assert.False(t, isAzureDefiniteRefusal(refused, 2), "a 4xx after a retry may answer a retry of an accepted try") + assert.True(t, isAzureDefiniteRefusal(&azcore.ResponseError{StatusCode: 499}, 1)) + assert.False(t, isAzureDefiniteRefusal(&azcore.ResponseError{StatusCode: 500}, 1)) + assert.False(t, isAzureDefiniteRefusal(&azcore.ResponseError{StatusCode: 399}, 1)) + assert.False(t, isAzureDefiniteRefusal(context.Canceled, 1)) +} + +// fakeTokenCredential satisfies the bearer-token policy without Entra ID. +type fakeTokenCredential struct{} + +func (fakeTokenCredential) GetToken(context.Context, policy.TokenRequestOptions) (azcore.AccessToken, error) { + return azcore.AccessToken{Token: "fake-token", ExpiresOn: time.Now().Add(time.Hour)}, nil +} + +// returnServer stands in for ARM's Return endpoint. It answers the n-th +// request with answers[n] (the last one repeats) and counts requests. +type returnServer struct { + *httptest.Server + requests atomic.Int32 +} + +func newReturnServer(t *testing.T, answers ...func(http.ResponseWriter)) *returnServer { + t.Helper() + s := &returnServer{} + s.Server = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = io.Copy(io.Discard, r.Body) + n := int(s.requests.Add(1)) - 1 + answers[min(n, len(answers)-1)](w) + })) + t.Cleanup(s.Close) + return s +} + +// returnClientFor builds the production Return client (the default factory's, +// with returnAttemptPolicy) against srv, with millisecond retry delays. +func returnClientFor(t *testing.T, srv *returnServer) azureReturnClient { + t.Helper() + opts := &arm.ClientOptions{ + ClientOptions: policy.ClientOptions{ + Cloud: cloud.Configuration{Services: map[cloud.ServiceName]cloud.ServiceConfiguration{ + cloud.ResourceManager: {Endpoint: srv.URL, Audience: "https://management.core.windows.net/"}, + }}, + Retry: policy.RetryOptions{RetryDelay: time.Millisecond, MaxRetryDelay: 5 * time.Millisecond}, + Transport: srv.Client(), + }, + DisableRPRegistration: true, + } + client, err := defaultAzureRevokeFactory(opts).newReturnClient(fakeTokenCredential{}) + require.NoError(t, err) + return client +} + +func azureError(status int, code string) func(http.ResponseWriter) { + return func(w http.ResponseWriter) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("x-ms-error-code", code) + w.WriteHeader(status) + _, _ = fmt.Fprintf(w, `{"error":{"code":%q,"message":"stub"}}`, code) + } +} + +// truncatedAccepted answers 202 but closes the connection partway through the +// body, so the client cannot read Azure's answer to a Return Azure accepted. +func truncatedAccepted(w http.ResponseWriter) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("Content-Length", "500") + w.WriteHeader(http.StatusAccepted) + _, _ = io.WriteString(w, `{"id":"`) + if f, ok := w.(http.Flusher); ok { + f.Flush() + } + if hj, ok := w.(http.Hijacker); ok { + if conn, _, err := hj.Hijack(); err == nil { + _ = conn.Close() + } + } +} + +// The real Return client retries a 5xx, and the retry can meet Azure's +// refusal of a Return it already took. That final 4xx is not a definite +// refusal: the flag must stay. Nor does a 202 whose body was cut off prove +// anything. Before #1529 the flag was cleared in every case. +func TestCallAzureReturn_RealClient_AmbiguousKeepsFlag(t *testing.T) { + t.Parallel() + cases := map[string]struct { + answers []func(http.ResponseWriter) + requests int32 + }{ + "503 then 409": {[]func(http.ResponseWriter){azureError(503, "ServiceUnavailable"), azureError(409, "ReservationAlreadyReturned")}, 2}, + "504 then 400 RefundPol.": {[]func(http.ResponseWriter){azureError(504, "GatewayTimeout"), azureError(400, "RefundPolicyViolated")}, 2}, + "503 on every try": {[]func(http.ResponseWriter){azureError(503, "ServiceUnavailable")}, 4}, + "202 with its body cut off": {[]func(http.ResponseWriter){truncatedAccepted, azureError(400, "ReservationAlreadyReturned")}, 1}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + srv := newReturnServer(t, tc.answers...) + mockStore := new(MockConfigStore) + calcClient := &stubCalcRefundClientWithAmount{amount: 10.0, currency: "USD", sessID: "s-retry"} + h := &Handler{config: mockStore} + + _, err := h.callAzureReturn(context.Background(), calcClient, returnClientFor(t, srv), armReservationRecord(), "order-abc", "res-xyz", confirmedQuote(10, "USD")) + assertUncertainReturn(t, mockStore, err) + assert.Equal(t, tc.requests, srv.requests.Load(), "requests the client sent") + }) + } +} + +// A 4xx on the only request the real client sent is a definite refusal: the +// counting policy sees one send and the flag is cleared. +func TestCallAzureReturn_RealClient_Single4xxClearsFlag(t *testing.T) { + t.Parallel() + srv := newReturnServer(t, azureError(400, "InvalidReservationID")) + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + r := armReservationRecord() + mockStore.On("ClearRevocationInFlight", mock.Anything, r.PurchaseID).Return(nil).Once() + calcClient := &stubCalcRefundClientWithAmount{amount: 10.0, currency: "USD", sessID: "s-single"} + h := &Handler{config: mockStore} + + _, err := h.callAzureReturn(context.Background(), calcClient, returnClientFor(t, srv), r, "order-abc", "res-xyz", confirmedQuote(10, "USD")) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, 400, ce.code) + assert.Equal(t, int32(1), srv.requests.Load()) + mockStore.AssertNotCalled(t, "MarkPurchaseRevoked") +} + +// The real client's 202 still revokes; the counting policy changes nothing on +// the success path. +func TestCallAzureReturn_RealClient_AcceptedRevokes(t *testing.T) { + t.Parallel() + srv := newReturnServer(t, func(w http.ResponseWriter) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusAccepted) + _, _ = io.WriteString(w, `{}`) + }) + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + r := armReservationRecord() + mockStore.On("MarkRevocationReturnPosted", mock.Anything, r.PurchaseID).Return(nil).Once() + mockStore.On("MarkPurchaseRevoked", mock.Anything, r.PurchaseID, mock.AnythingOfType("time.Time"), + "direct-api", "", mock.Anything, mock.Anything).Return(nil).Once() + calcClient := &stubCalcRefundClientWithAmount{amount: 10.0, currency: "USD", sessID: "s-ok"} + h := &Handler{config: mockStore} + + result, err := h.callAzureReturn(context.Background(), calcClient, returnClientFor(t, srv), r, "order-abc", "res-xyz", confirmedQuote(10, "USD")) + require.NoError(t, err) + rr, ok := result.(*revokePurchaseResult) + require.True(t, ok, "got %T %+v", result, result) + assert.Equal(t, "revoked", rr.Status) + assert.Equal(t, int32(1), srv.requests.Load()) +} + +// The claim threshold is the detached Return plus one post-Return write budget +// plus the response margin, and it must leave every shipped Lambda timeout +// (at least those budgets plus lambdatimeout.Margin) time for the work before +// the claim, or every Lambda revoke would be refused. +func TestRevokeClaimMinRemaining_Budget(t *testing.T) { + t.Parallel() + assert.Equal(t, revokeReturnTimeout+revokePersistTimeout+revokeResponseMargin, revokeClaimMinRemaining) + assert.Equal(t, 3*time.Minute+35*time.Second, revokeClaimMinRemaining) + assert.Less(t, revokeClaimMinRemaining, revokeReturnTimeout+revokePersistTimeout+lambdatimeout.Margin) +} + +// On Lambda, a revoke with too little invocation time left must not take the +// claim or post the Return. Before #1529 it did both, and Lambda could end the +// invocation mid-Return. +func TestCallAzureReturn_LambdaTooLittleTime_RefusesBeforeClaim(t *testing.T) { + t.Parallel() + for name, left := range map[string]time.Duration{ + "one minute": time.Minute, + "just under the needed": revokeClaimMinRemaining - time.Second, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), left) + defer cancel() + mockStore := new(MockConfigStore) + // Registered so a regression fails the assertions below instead + // of panicking the test binary on an unexpected call. + mockStore.On("MarkPurchaseRevoked", mock.Anything, mock.Anything, mock.Anything, + mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + calcClient := &stubCalcRefundClientWithAmount{amount: 10.0, currency: "USD", sessID: "s-late"} + returnClient := &stubReturnClient{} + h := &Handler{config: mockStore, isLambda: true} + + result, err := h.callAzureReturn(ctx, calcClient, returnClient, armReservationRecord(), "order-abc", "res-xyz", confirmedQuote(10, "USD")) + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "got %v", err) + assert.Equal(t, http.StatusServiceUnavailable, ce.code) + assert.Equal(t, errRevokeTooLittleTime, ce.message) + assert.Zero(t, returnClient.calls, "no Return may be posted") + mockStore.AssertNotCalled(t, "FlipPurchaseRevocationInFlight") + mockStore.AssertNotCalled(t, "ClearRevocationInFlight") + mockStore.AssertNotCalled(t, "MarkRevocationReturnPosted") + mockStore.AssertNotCalled(t, "MarkPurchaseRevoked") + }) + } +} + +// The check must not refuse a revoke that has the time, nor apply off Lambda, +// where the process outlives the request's 30s deadline and the detached +// Return finishes anyway. +func TestCallAzureReturn_EnoughTimeOrNotLambda_Proceeds(t *testing.T) { + t.Parallel() + cases := map[string]struct { + left time.Duration // 0: no deadline + isLambda bool + }{ + "lambda with time to spare": {revokeClaimMinRemaining + 30*time.Second, true}, + "lambda without a deadline": {0, true}, + "http server's 30s deadline": {20 * time.Second, false}, + "non-lambda with little time": {time.Second, false}, + "non-lambda without a deadline": {0, false}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + if tc.left > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, tc.left) + defer cancel() + } + mockStore := new(MockConfigStore) + r := armReservationRecord() + mockStore.On("MarkPurchaseRevoked", mock.Anything, r.PurchaseID, mock.AnythingOfType("time.Time"), + "direct-api", "", mock.Anything, mock.Anything).Return(nil).Once() + calcClient := &stubCalcRefundClientWithAmount{amount: 10.0, currency: "USD", sessID: "s-time"} + returnClient := &stubReturnClient{} + h := &Handler{config: mockStore, isLambda: tc.isLambda} + + result, err := h.callAzureReturn(ctx, calcClient, returnClient, r, "order-abc", "res-xyz", confirmedQuote(10, "USD")) + require.NoError(t, err) + rr, ok := result.(*revokePurchaseResult) + require.True(t, ok, "got %T %+v", result, result) + assert.Equal(t, "revoked", rr.Status) + assert.Equal(t, 1, returnClient.calls) + mockStore.AssertCalled(t, "FlipPurchaseRevocationInFlight") + }) + } +} diff --git a/internal/purchase/finalize_revocations.go b/internal/purchase/finalize_revocations.go index 8fbe7e42..b49c7511 100644 --- a/internal/purchase/finalize_revocations.go +++ b/internal/purchase/finalize_revocations.go @@ -33,7 +33,10 @@ type FinalizeResult struct { // (handleHTTPRequest in internal/server/http.go), Lambda ends an invocation at // its function timeout, and past the flip the revoke handler runs the Return // and the writes recording it detached under their own budgets (3m and 30s, -// issue #1527). 2h is a wide margin over all of these. Correctness does not +// issue #1527). 2h is a wide margin over all of these. It is also how a row +// whose Return failed without a definite answer gets released: the revoke +// handler keeps that row flagged and records no Return (issue #1529), so the +// sweep releases it here and logs it for an operator. Correctness does not // depend on it (issue #1525): a Return recorded after the release is still // finalized, flag or not, and if the user revokes again while the first // Return is in flight, Azure refuses a second Return of the same reservation,