fix(revoke): keep the in-flight flag on an ambiguous Azure Return failure and refuse a late Lambda claim

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.

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 },

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)

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

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)
+}