From 69c05a5d50a583b4c1a0fdbb205ada809f5622ea Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 27 Oct 2026 02:00:17 +0000 Subject: [PATCH] fix(revoke): detach the Azure Return once the in-flight claim is held The Azure Return ran on the request context. The app's HTTP server gives every request 30s (internal/server/http.go), so that deadline or a client disconnect could land while the Return was in flight: Azure could still accept it, but the handler saw context.Canceled, recorded nothing, and cleared the in-flight flag on the same dead context, which also failed. The sweep then released the row after 2h and a refunded purchase read as active. Once the claim is held, the Return now runs under ctxutil.DetachedWithTimeout(ctx, 3m), as the RI exchange handler (#790) and purchase/manager.go do. 3m lets azcore's default pipeline (4 tries, up to about 11.4s of backoff, 30s dial + 10s TLS timeouts per try, about 171s worst case) end a failing call itself. The claim and everything before it stay on the request context. If the Return fails, the in-flight clear runs on its own detached 30s budget so it can land after the request is gone. The persist retries now respect their 30s budget: each attempt is capped at 4s, so a hung first attempt leaves time for the retries, and a backoff that would outlast the budget is not taken (before, plain sleeps could run the worst case to about 43s). TestLambdaTimeoutCoversRevokeBudget keeps every shipped Lambda timeout above 3m30s plus margin. Also correct the comments that said Fargate and Container Apps do not cap request time: the app's own server caps every request at 30s. Tests (stub Azure clients; nothing calls Azure): the request is canceled while the Return is in flight and Azure accepts it, so the Return is recorded and the purchase revoked (unit, and integration on real Postgres); the same with a failed Return clears the flag on a live context (unit and integration); a 1h backoff is skipped against the 30s budget; a hung first attempt is cut at its cap and the retry revokes. The cancel-before-claim test still asserts no claim and no Return. Closes #1527 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_purchases_revoke.go | 122 ++++++++++++--- ...purchases_revoke_claim_integration_test.go | 63 +++++++- .../handler_purchases_revoke_detached_test.go | 142 ++++++++++++++++++ ...ler_purchases_revoke_return_posted_test.go | 53 +++++-- internal/api/handler_purchases_revoke_test.go | 6 +- internal/purchase/finalize_revocations.go | 11 +- .../purchase/finalize_revocations_test.go | 7 +- 7 files changed, 358 insertions(+), 46 deletions(-) create mode 100644 internal/api/handler_purchases_revoke_detached_test.go diff --git a/internal/api/handler_purchases_revoke.go b/internal/api/handler_purchases_revoke.go index 27667cb5..d7ebd387 100644 --- a/internal/api/handler_purchases_revoke.go +++ b/internal/api/handler_purchases_revoke.go @@ -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 @@ -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, @@ -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 @@ -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 @@ -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 { @@ -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, @@ -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) diff --git a/internal/api/handler_purchases_revoke_claim_integration_test.go b/internal/api/handler_purchases_revoke_claim_integration_test.go index c503ec99..c9b0da5d 100644 --- a/internal/api/handler_purchases_revoke_claim_integration_test.go +++ b/internal/api/handler_purchases_revoke_claim_integration_test.go @@ -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. @@ -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) @@ -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") +} diff --git a/internal/api/handler_purchases_revoke_detached_test.go b/internal/api/handler_purchases_revoke_detached_test.go new file mode 100644 index 00000000..421163d2 --- /dev/null +++ b/internal/api/handler_purchases_revoke_detached_test.go @@ -0,0 +1,142 @@ +package api + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "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 #1527: once the in-flight claim is held, the Azure Return runs +// detached from the request, the in-flight clear after a failed Return can +// still land, and the persist retries stay inside their budget. Stub Azure +// clients and a mock store; nothing here calls Azure. + +// TestLambdaTimeoutCoversRevokeBudget keeps every shipped AWS Lambda timeout +// above the detached revoke budget: the Return and then either the writes +// recording it or the in-flight clear run back to back after the claim, and +// Lambda ends the invocation at its timeout whatever the context says (#913). +func TestLambdaTimeoutCoversRevokeBudget(t *testing.T) { + lambdatimeout.AssertCovers(t, "../..", revokeReturnTimeout+revokePersistTimeout) +} + +// The whole persist schedule (every attempt at its cap plus every backoff) +// must fit the persist budget, or the last retries are dead on arrival. +func TestRevokePersistScheduleFitsBudget(t *testing.T) { + t.Parallel() + schedule := time.Duration(len(revokeMarkRetryBackoffs)+1) * revokePersistAttemptTimeout + for _, b := range revokeMarkRetryBackoffs { + schedule += b + } + assert.LessOrEqual(t, schedule, revokePersistTimeout) +} + +// A Return that fails 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. +func TestCallAzureReturn_RequestCanceledDuringFailedReturn_ClearsInFlight(t *testing.T) { + t.Parallel() + reqCtx, cancel := context.WithCancel(context.Background()) + defer cancel() + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + store := &ctxHonoringStore{MockConfigStore: mockStore} + + r := armReservationRecord() + 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}} + 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") + + returnClient.assertDetached(t, start) + require.ErrorIs(t, reqCtx.Err(), context.Canceled) + writes := store.seenWrites() + require.Len(t, writes, 1, "one in-flight clear") + require.NoError(t, writes[0].err, "the in-flight clear ran under a canceled context") + require.True(t, writes[0].bounded, "the in-flight clear must have its own bounded timeout") + assert.LessOrEqual(t, writes[0].remaining, revokePersistTimeout) + mockStore.AssertNotCalled(t, "MarkRevocationReturnPosted") + mockStore.AssertNotCalled(t, "MarkPurchaseRevoked") +} + +// A wait that would outlast the persist budget is not taken: before the fix +// the retries slept through their backoff whatever the budget said (about 43s +// worst case against a 30s budget), here an hour. +func TestPersistAzureRevocation_RetryWaitRespectsBudget(t *testing.T) { + // NOT parallel: mutates the package-global revokeMarkRetryBackoffs. + orig := revokeMarkRetryBackoffs + revokeMarkRetryBackoffs = []time.Duration{time.Hour} + t.Cleanup(func() { revokeMarkRetryBackoffs = orig }) + + 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(errors.New("db down")).Once() + h := &Handler{config: mockStore} + + type outcome struct { + result any + err error + } + done := make(chan outcome, 1) + go func() { + result, err := h.persistAzureRevocation(context.Background(), r, toPtr(10.0), "USD") + done <- outcome{result, err} + }() + select { + case got := <-done: + require.NoError(t, got.err) + pending, ok := got.result.(*revokeReconcilePendingResult) + require.True(t, ok, "want RECONCILE_PENDING, got %T %+v", got.result, got.result) + assert.Equal(t, "RECONCILE_PENDING", pending.Code) + case <-time.After(5 * time.Second): + t.Fatal("persistAzureRevocation waited past its 30s budget instead of giving up") + } +} + +// A first persist attempt stuck on a hung connection is cut off at its own +// cap, leaving budget for the retry, which records the Return and revokes. +func TestPersistAzureRevocation_HungAttemptLeavesBudgetForRetry(t *testing.T) { + // NOT parallel: mutates the package-global revokeMarkRetryBackoffs. + orig := revokeMarkRetryBackoffs + revokeMarkRetryBackoffs = []time.Duration{0, 0, 0} + t.Cleanup(func() { revokeMarkRetryBackoffs = orig }) + + mockStore := new(MockConfigStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + r := armReservationRecord() + var hungFor time.Duration + mockStore.On("MarkRevocationReturnPosted", mock.Anything, r.PurchaseID).Run(func(args mock.Arguments) { + start := time.Now() + <-args.Get(0).(context.Context).Done() + hungFor = time.Since(start) + }).Return(context.DeadlineExceeded).Once() + mockStore.On("MarkPurchaseRevoked", mock.Anything, r.PurchaseID, mock.AnythingOfType("time.Time"), + "direct-api", "", mock.Anything, mock.Anything).Return(context.DeadlineExceeded).Once() + 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() + h := &Handler{config: mockStore} + + result, err := h.persistAzureRevocation(context.Background(), r, toPtr(10.0), "USD") + require.NoError(t, err) + rr, ok := result.(*revokePurchaseResult) + require.True(t, ok, "want revoked, got %T %+v", result, result) + assert.Equal(t, "revoked", rr.Status) + assert.Less(t, hungFor, revokePersistTimeout/2, "the hung attempt must be cut off at its own cap, not the whole budget") +} diff --git a/internal/api/handler_purchases_revoke_return_posted_test.go b/internal/api/handler_purchases_revoke_return_posted_test.go index 8b842c0a..8a00357e 100644 --- a/internal/api/handler_purchases_revoke_return_posted_test.go +++ b/internal/api/handler_purchases_revoke_return_posted_test.go @@ -164,23 +164,54 @@ func (s *ctxHonoringStore) MarkPurchaseRevoked(ctx context.Context, purchaseID s return s.MockConfigStore.MarkPurchaseRevoked(ctx, purchaseID, revokedAt, revokedVia, supportCaseID, calcRefundAmount, calcRefundCurrency) } -// cancelAfterReturnClient accepts the Return, then cancels the request, as a -// client disconnecting right after Azure answered would. It records whether -// the Return itself ran under the request's (cancelable) context. +func (s *ctxHonoringStore) ClearRevocationInFlight(ctx context.Context, purchaseID string) error { + if err := s.seen(ctx); err != nil { + return err + } + return s.MockConfigStore.ClearRevocationInFlight(ctx, purchaseID) +} + +// cancelAfterReturnClient stands in for a Return that Azure accepts while the +// request is canceled underneath it (a client disconnect, or the app server's +// 30s per-request timeout, issue #1527). Like the real SDK call, whose HTTP +// request is aborted when its context is canceled, it then reports the +// context's error instead of the accepted response, or err when set (a Return +// that failed anyway). It records the state of the context it ran under. type cancelAfterReturnClient struct { - cancel context.CancelFunc - calls int - sawRequestCtx bool + cancel context.CancelFunc + err error + calls int + ctxErr error + deadline time.Time + hasDeadline bool } func (c *cancelAfterReturnClient) Post(ctx context.Context, _ string, _ armreservations.RefundRequest, _ *armreservations.ReturnClientPostOptions) (armreservations.ReturnClientPostResponse, error) { c.calls++ - before := ctx.Err() c.cancel() - c.sawRequestCtx = before == nil && ctx.Err() != nil - return armreservations.ReturnClientPostResponse{}, nil + c.ctxErr = ctx.Err() + c.deadline, c.hasDeadline = ctx.Deadline() + if c.ctxErr != nil { + return armreservations.ReturnClientPostResponse{}, c.ctxErr + } + return armreservations.ReturnClientPostResponse{}, c.err +} + +// assertDetached checks that canceling the request did not reach the Return, +// and that the Return carried its own 3m budget (a literal, so the test pins +// the budget TestLambdaTimeoutCoversRevokeBudget also covers). +func (c *cancelAfterReturnClient) assertDetached(t *testing.T, start time.Time) { + t.Helper() + require.Equal(t, 1, c.calls, "exactly one Return") + require.NoError(t, c.ctxErr, "canceling the request must not cancel a Return already posted to Azure") + require.True(t, c.hasDeadline, "the detached Return must still be bounded by its own timeout") + assert.WithinDuration(t, start.Add(3*time.Minute), c.deadline, 5*time.Second) } +// TestCallAzureReturn_RequestCanceledAfterReturn_StillRecordsAndMarks also +// covers issue #1527: the request is canceled while the Return is in flight +// and Azure accepts it. The Return runs detached, so it still reports the +// acceptance, and the handler records it and marks the purchase revoked. func TestCallAzureReturn_RequestCanceledAfterReturn_StillRecordsAndMarks(t *testing.T) { // NOT parallel: mutates the package-global revokeMarkRetryBackoffs, so a // regression fails in milliseconds instead of sleeping through the retries. @@ -202,11 +233,11 @@ func TestCallAzureReturn_RequestCanceledAfterReturn_StillRecordsAndMarks(t *test calcClient := &stubCalcRefundClientWithAmount{amount: 10.0, currency: "USD", sessID: "s-cancel"} returnClient := &cancelAfterReturnClient{cancel: cancel} h := &Handler{config: store} + start := time.Now() result, err := h.callAzureReturn(reqCtx, calcClient, returnClient, r, "order-abc", "res-xyz", confirmedQuote(10, "USD")) require.NoError(t, err) - require.Equal(t, 1, returnClient.calls) - assert.True(t, returnClient.sawRequestCtx, "the Return itself must run under the request's context") + returnClient.assertDetached(t, start) require.ErrorIs(t, reqCtx.Err(), context.Canceled) rr, ok := result.(*revokePurchaseResult) require.True(t, ok, "a canceled request must still record the accepted Return and revoke, got %T %+v", result, result) diff --git a/internal/api/handler_purchases_revoke_test.go b/internal/api/handler_purchases_revoke_test.go index d35ea290..ba45c9ce 100644 --- a/internal/api/handler_purchases_revoke_test.go +++ b/internal/api/handler_purchases_revoke_test.go @@ -1475,7 +1475,7 @@ func TestCallAzureReturn_TransientError_ClearsInFlight(t *testing.T) { returnClient := &stubReturnClient{err: errors.New("dial tcp: connection refused")} r := armReservationRecord() - mockStore.On("ClearRevocationInFlight", ctx, r.PurchaseID).Return(nil).Once() + 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")) @@ -1501,7 +1501,7 @@ func TestCallAzureReturn_ClientError_ClearsInFlight(t *testing.T) { } r := armReservationRecord() - mockStore.On("ClearRevocationInFlight", ctx, r.PurchaseID).Return(nil).Once() + 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")) @@ -1526,7 +1526,7 @@ func TestCallAzureReturn_WindowEdge_ClearsInFlight(t *testing.T) { } r := armReservationRecord() - mockStore.On("ClearRevocationInFlight", ctx, r.PurchaseID).Return(nil).Once() + 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")) diff --git a/internal/purchase/finalize_revocations.go b/internal/purchase/finalize_revocations.go index 2b22e135..8fbe7e42 100644 --- a/internal/purchase/finalize_revocations.go +++ b/internal/purchase/finalize_revocations.go @@ -27,10 +27,13 @@ type FinalizeResult struct { // staleRevocationInFlightAfter is how long a row may stay flagged with no // recorded Return before the sweep clears the flag so the purchase can be -// revoked again (issue #1523). It is not a bound on request time: Lambda caps -// a request at 900s and Cloud Run at 3600s, but Fargate and Container Apps do -// not cap it at all. 2h is a margin that keeps a normal revoke still between -// its flip and its Return from being released under it. Correctness does not +// revoked again (issue #1523). It only has to outlast a revoke still between +// its flip and its recorded Return, which every deployment bounds: the app's +// own HTTP server caps each request at 30s on every container platform +// (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 // 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, diff --git a/internal/purchase/finalize_revocations_test.go b/internal/purchase/finalize_revocations_test.go index ce263758..63e379a2 100644 --- a/internal/purchase/finalize_revocations_test.go +++ b/internal/purchase/finalize_revocations_test.go @@ -154,9 +154,10 @@ func TestFinalizeInFlightRevocations_ReleaseAndListErrorsJoined(t *testing.T) { assert.Nil(t, res) } -// The timeout should outlast a normal request still between its flip and its -// Return. Cloud Run's request timeout caps at an hour; Fargate and Container -// Apps have no cap, and correctness does not depend on this (issue #1525). +// The timeout should outlast a revoke still between its flip and its recorded +// Return: the app server caps a request at 30s (internal/server/http.go) and +// the detached Return plus its writes at 3m30s (issue #1527). Correctness does +// not depend on this (issue #1525). func TestStaleRevocationInFlightAfter_OutlastsLongestRequest(t *testing.T) { assert.Greater(t, staleRevocationInFlightAfter, time.Hour) }