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