Skip to content

fix(revoke): detach the Azure Return once the in-flight claim is held #1528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
IAmJSD merged 1 commit into from
Oct 27, 2026
Merged

fix(revoke): detach the Azure Return once the in-flight claim is held #1528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 99 additions & 23 deletions internal/api/handler_purchases_revoke.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,15 +127,43 @@ var revokeMarkRetryBackoffs = []time.Duration{
}

// revokePersistTimeout bounds the writes that record an Azure Return Azure has
// already accepted (issue #1525). They run detached from the request, so a
// client disconnect or upstream timeout right after the Return does not lose
// the record. 30s covers the whole retry schedule (13s of backoff across
// revokeMarkRetryBackoffs) plus four attempts of two single-row UPDATEs, while
// still capping how long a hung database can hold the handler after its
// caller has gone. If it runs out, the result is the same 207 as exhausted
// retries.
// already accepted (issue #1525), and the in-flight clear after a failed
// Return. They run detached from the request, so a client disconnect or the
// app server's 30s per-request timeout (internal/server/http.go) right after
// the Return does not lose the record. 30s covers the whole retry schedule:
// four attempts of revokePersistAttemptTimeout plus 13s of backoff across
// revokeMarkRetryBackoffs (29s), while still capping how long a hung database
// can hold the handler after its caller has gone. The waits between attempts
// honor it too (issue #1527). If it runs out, the result is the same 207 as
// exhausted retries.
const revokePersistTimeout = 30 * time.Second

// revokePersistAttemptTimeout bounds one persist attempt (stamp the Return,
// then mark the purchase revoked: two single-row UPDATEs by primary key), so
// a first attempt stuck on a hung connection cannot use up the whole
// revokePersistTimeout and leave the retries nothing (issue #1527). Both
// writes are idempotent, so an attempt cut off after its UPDATE committed is
// safe to retry.
const revokePersistAttemptTimeout = 4 * time.Second

// revokeReturnTimeout bounds the Azure Return call, which runs detached from
// the request once this request holds the in-flight claim (issue #1527): Azure
// may accept a Return whose caller has gone, so a client disconnect or the app
// server's 30s per-request timeout must not abort it and lose the record.
//
// The budget is sized so the Azure client's own limits, not this deadline,
// end a failing call. The armreservations client runs on azcore's default
// pipeline: up to 4 tries (MaxRetries 3) with at most about 11.4s of backoff
// between them (800ms base, exponential, jitter up to 1.3x), each try bounded
// by the transport's 30s dial and 10s TLS handshake timeouts, and no per-try
// timeout. Four tries that each exhaust dial and handshake take about 171s, so
// 3m lets the client report its own error; only a response that never arrives
// (which nothing in the client bounds) or Azure throttling with long
// Retry-After waits (honored up to 60s each) runs into this deadline. On
// Lambda this budget plus revokePersistTimeout must fit the function timeout,
// enforced by TestLambdaTimeoutCoversRevokeBudget (#913).
const revokeReturnTimeout = 3 * time.Minute

// revokePurchase handles POST /api/purchases/{purchaseId}/revoke.
//
// Authorization: session required + revoke-own:purchases (or revoke-any for
Expand Down Expand Up @@ -796,12 +824,31 @@ func (h *Handler) callAzureReturn(
return nil, quoteErr
}

// The claim stays on the request context: a request canceled before it
// takes no claim and posts no Return.
if stop, claimErr := h.claimRevocationInFlight(ctx, record); stop != nil || claimErr != nil {
return stop, claimErr
}

// Step 2: Return (post the actual refund request).
_, err = returnClient.Post(ctx, orderID, armreservations.RefundRequest{
if err := postAzureReturn(ctx, returnClient, orderID, reservationID, sessionID, quantity); err != nil {
return nil, h.handleAzureReturnError(ctx, record, err)
}

return h.persistAzureRevocation(ctx, record, calcRefundAmount, calcRefundCurrency)
}

// postAzureReturn posts the Return for a reservation whose in-flight claim the
// caller holds. Past the claim Azure may refund whether or not the request is
// still alive, so the call runs on a context that keeps the request's values
// but not its cancellation, bounded by revokeReturnTimeout (issue #1527), as
// 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 {
ctx, cancel := ctxutil.DetachedWithTimeout(reqCtx, revokeReturnTimeout)
defer cancel()
_, err := returnClient.Post(ctx, orderID, armreservations.RefundRequest{
Properties: &armreservations.RefundRequestProperties{
ReservationToReturn: &armreservations.ReservationToReturn{
ReservationID: &reservationID,
Expand All @@ -812,11 +859,7 @@ func (h *Handler) callAzureReturn(
Scope: toPtr("Reservation"),
},
}, nil)
if err != nil {
return nil, h.handleAzureReturnError(ctx, record, err)
}

return h.persistAzureRevocation(ctx, record, calcRefundAmount, calcRefundCurrency)
return err
}

// revocationInFlightResult is the answer for a row whose revocation another
Expand Down Expand Up @@ -935,7 +978,13 @@ func (h *Handler) azureCalculateRefund(ctx context.Context, calcClient azureCalc
// 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).
func (h *Handler) handleAzureReturnError(ctx context.Context, record *config.PurchaseHistoryRecord, err error) error {
//
// 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 {
ctx, cancel := ctxutil.DetachedWithTimeout(reqCtx, revokePersistTimeout)
defer cancel()
// 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
// sweep into thinking Azure already issued a refund (Finding D, second-wave
Expand Down Expand Up @@ -968,21 +1017,27 @@ func (h *Handler) handleAzureReturnError(ctx context.Context, record *config.Pur
//
// Azure has already refunded by the time this runs, so the writes ignore the
// request's cancellation and run under their own revokePersistTimeout (issue
// #1525): a request canceled right after the Return still records it. Nothing
// before the Return is detached.
// #1525): a request canceled right after the Return still records it. Each
// attempt is capped at revokePersistAttemptTimeout, and a wait that would
// outlast the budget is not taken (issue #1527). Nothing before the claim is
// detached.
func (h *Handler) persistAzureRevocation(reqCtx context.Context, record *config.PurchaseHistoryRecord, calcRefundAmount *float64, calcRefundCurrency string) (any, error) {
ctx, cancel := ctxutil.DetachedWithTimeout(reqCtx, revokePersistTimeout)
defer cancel()
now := time.Now().UTC()
returnRecorded := false
attempts := 1
markErr := h.recordAzureReturnAndMark(ctx, record.PurchaseID, now, calcRefundAmount, calcRefundCurrency, &returnRecorded)
for attempt, backoff := range revokeMarkRetryBackoffs {
for _, backoff := range revokeMarkRetryBackoffs {
if markErr == nil {
break
}
logging.Warnf("revoke azure: MarkPurchaseRevoked attempt %d failed for %s: %v (retrying in %s)",
attempt+1, record.PurchaseID, markErr, backoff)
time.Sleep(backoff)
attempts, record.PurchaseID, markErr, backoff)
if !waitRevokeRetry(ctx, backoff) {
break
}
attempts++
markErr = h.recordAzureReturnAndMark(ctx, record.PurchaseID, now, calcRefundAmount, calcRefundCurrency, &returnRecorded)
}
if markErr != nil {
Expand All @@ -992,7 +1047,7 @@ func (h *Handler) persistAzureRevocation(reqCtx context.Context, record *config.
// will reconcile the DB write on its next tick, provided the Return
// was recorded.
logging.Errorf("revoke azure: MarkPurchaseRevoked failed for %s after %d attempts (Azure already returned, return recorded=%t): %v",
record.PurchaseID, len(revokeMarkRetryBackoffs)+1, returnRecorded, markErr)
record.PurchaseID, attempts, returnRecorded, markErr)
return &revokeReconcilePendingResult{
Code: "RECONCILE_PENDING",
AzureReturned: true,
Expand All @@ -1010,10 +1065,31 @@ func (h *Handler) persistAzureRevocation(reqCtx context.Context, record *config.
}, nil
}

// waitRevokeRetry waits d before the next persist attempt and reports whether
// to make it. It returns false at once when ctx's deadline is less than d
// away, and as soon as ctx is done, so the retries never run past
// revokePersistTimeout (issue #1527).
func waitRevokeRetry(ctx context.Context, d time.Duration) bool {
if deadline, ok := ctx.Deadline(); ok && time.Until(deadline) < d {
return false
}
timer := time.NewTimer(d)
defer timer.Stop()
select {
case <-timer.C:
return true
case <-ctx.Done():
return false
}
}

// recordAzureReturnAndMark is one persist attempt after Azure accepted the
// Return: stamp return_posted_at unless an earlier attempt already did (a
// failure is logged; the mark may still land), then MarkPurchaseRevoked.
func (h *Handler) recordAzureReturnAndMark(ctx context.Context, purchaseID string, now time.Time, calcRefundAmount *float64, calcRefundCurrency string, returnRecorded *bool) error {
// Return, capped at revokePersistAttemptTimeout: stamp return_posted_at unless
// an earlier attempt already did (a failure is logged; the mark may still
// land), then MarkPurchaseRevoked.
func (h *Handler) recordAzureReturnAndMark(budgetCtx context.Context, purchaseID string, now time.Time, calcRefundAmount *float64, calcRefundCurrency string, returnRecorded *bool) error {
ctx, cancel := context.WithTimeout(budgetCtx, revokePersistAttemptTimeout)
defer cancel()
if !*returnRecorded {
if err := h.config.MarkRevocationReturnPosted(ctx, purchaseID); err != nil {
logging.Warnf("revoke azure: MarkRevocationReturnPosted for %s failed (continuing to the mark): %v", purchaseID, err)
Expand Down
63 changes: 61 additions & 2 deletions internal/api/handler_purchases_revoke_claim_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,13 @@ func runConcurrentFirstRevokes(t *testing.T, store *config.PostgresStore, round
// finalize_revocations sweep would release it after 2h as if nothing had been
// refunded. Now those writes run detached, so the row ends up with the Return
// recorded and revoked.
//
// It also covers issue #1527: the stub cancels the request while the Return is
// in flight and, like the real SDK call, reports a canceled context as an
// error even though Azure accepted the Return. Before the fix the Return ran
// on the request context, so the handler saw context.Canceled, recorded
// nothing and left the flag set; now the Return runs detached and reports the
// acceptance.
func TestRevokeAzure_RequestCanceledAfterReturn_RecordsAndRevokes(t *testing.T) {
// NOT parallel: mutates the package-global revokeMarkRetryBackoffs, so a
// regression fails fast instead of sleeping through the retries.
Expand Down Expand Up @@ -220,10 +227,11 @@ func TestRevokeAzure_RequestCanceledAfterReturn_RecordsAndRevokes(t *testing.T)
require.NoError(t, err)
req := &events.LambdaFunctionURLRequest{Body: string(body)}

start := time.Now()
result, err := h.loadAndRevokePurchaseHistory(reqCtx, req, revokeAdminSession(), record.PurchaseID)
require.NoError(t, err)
require.Equal(t, 1, ret.calls, "exactly one Return")
require.ErrorIs(t, reqCtx.Err(), context.Canceled, "the request was canceled after the Return")
ret.assertDetached(t, start)
require.ErrorIs(t, reqCtx.Err(), context.Canceled, "the request was canceled during the Return")
res, ok := result.(*revokePurchaseResult)
require.True(t, ok, "want revoked, got %T %+v", result, result)
assert.Equal(t, "revoked", res.Status)
Expand All @@ -238,3 +246,54 @@ func TestRevokeAzure_RequestCanceledAfterReturn_RecordsAndRevokes(t *testing.T)
assert.NotNil(t, revokedAt, "the purchase must be marked revoked")
assert.False(t, inFlight, "MarkPurchaseRevoked clears the in-flight flag")
}

// 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
// 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.
func TestRevokeAzure_RequestCanceledDuringFailedReturn_ClearsFlag(t *testing.T) {
pg, store := setupSubmitDB(t)
ctx, cancelTest := context.WithTimeout(context.Background(), 60*time.Second)
defer cancelTest()

record := withinWindowRecord("/providers/Microsoft.Capacity/reservationOrders/order-1527/reservations/res-1527")
record.AccountID = "acct-1527"
record.Region = "eastus"
record.ResourceType = "Standard_D2s_v3"
require.NoError(t, store.SavePurchaseHistory(ctx, record))

reqCtx, cancelReq := context.WithCancel(ctx)
defer cancelReq()
calc := &barrierCalcRefundClient{
price: &armreservations.Price{Amount: toPtr(10.0), CurrencyCode: toPtr("USD")},
parties: 1,
ready: make(chan struct{}),
}
ret := &cancelAfterReturnClient{cancel: cancelReq, err: &azcore.ResponseError{StatusCode: 503}}
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)}

start := time.Now()
_, err = h.loadAndRevokePurchaseHistory(reqCtx, req, revokeAdminSession(), record.PurchaseID)
require.Error(t, err, "a failed Return is an error")
ret.assertDetached(t, start)
require.ErrorIs(t, reqCtx.Err(), context.Canceled, "the request was canceled during the Return")

var returnPostedAt, revokedAt *time.Time
var inFlight bool
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(&returnPostedAt, &revokedAt, &inFlight))
assert.False(t, inFlight, "the failed Return's in-flight flag must be cleared")
assert.Nil(t, returnPostedAt, "a failed Return must not be recorded")
assert.Nil(t, revokedAt, "a failed Return must not revoke the purchase")
}
Loading
Loading