diff --git a/providers/azure/services/internal/reservations/purchase.go b/providers/azure/services/internal/reservations/purchase.go index df2f31d43..88c8f3ebd 100644 --- a/providers/azure/services/internal/reservations/purchase.go +++ b/providers/azure/services/internal/reservations/purchase.go @@ -42,6 +42,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "log" @@ -209,6 +210,32 @@ type calculatePriceResponse struct { // phrasing changes in future API versions. const sessionTimeoutFragment = "Session timed out" +// errRetryabilityUnknown marks a 400 purchase failure whose retryability could +// not be determined because the response body did not read completely. +// +// IsSessionTimeout classifies by searching the error text for a fragment Azure +// puts in the body, so a truncated read cannot be distinguished from a genuinely +// permanent failure by inspecting the message. Carrying the state out of band +// lets DoPurchaseTwoStep retry instead of silently abandoning a purchase that +// the retry loop exists to recover. +// +// Deliberately scoped to 400, matching IsSessionTimeout's own status scope and +// the only scenario issue #1766 reports. The wider non-2xx surface is excluded +// on purpose: a truncated response is mechanistically different from a cleanly +// read one, because the server may have finished processing and begun writing +// when the connection died. Whether a 409 or 5xx can follow a purchase that +// actually committed is unverified for this API, and a retry on an unverified +// commit-ambiguous status is a double-buy risk that buys nothing -- every such +// case keeps its pre-existing behavior, which nobody has reported as broken. +// +// Note what does NOT bound this: DoIdempotentPurchaseTwoStep performs its +// idempotency lookup exactly once, before DoPurchaseTwoStep is entered, and +// there is no recheck between attempts. Each retry mints a fresh +// reservationOrderId via doCalculatePrice, so the guard covers a re-drive +// ACROSS invocations, not the loop within one. The bound here is +// purchaseMaxAttempts. +var errRetryabilityUnknown = errors.New("purchase retryability unknown: response body did not read completely") + // IsSessionTimeout reports whether err looks like the "Session timed out" // 400 error from the Azure Reservations purchase endpoint. It matches the // error message produced by DoPurchaseTwoStep so callers can distinguish @@ -255,7 +282,10 @@ func DoPurchaseTwoStep(ctx context.Context, httpClient HTTPClient, calcURL strin return orderID, nil } - if IsSessionTimeout(purchaseErr) && attempt < purchaseMaxAttempts { + // An unreadable body is retried alongside a recognized session timeout: + // without this the truncated case was abandoned on its first attempt, + // because IsSessionTimeout can only classify what it can read. + if (IsSessionTimeout(purchaseErr) || errors.Is(purchaseErr, errRetryabilityUnknown)) && attempt < purchaseMaxAttempts { log.Printf("reservation purchase session timed out (attempt %d/%d), re-running calculatePrice in %s", attempt, purchaseMaxAttempts, purchaseRetryDelay) select { @@ -472,11 +502,24 @@ func doPurchase(ctx context.Context, httpClient HTTPClient, purchaseURL string, if err != nil { return fmt.Errorf("failed to purchase reservation: %w", err) } - body, _ := io.ReadAll(resp.Body) + body, readErr := io.ReadAll(resp.Body) resp.Body.Close() // #nosec G104 -- body fully drained by io.ReadAll before Close; transport close error does not affect correctness + // Success is decided by the status code alone, so an unreadable body cannot + // turn a completed purchase into a failure. if resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusCreated || resp.StatusCode == http.StatusAccepted { return nil } + // Scoped to 400: see errRetryabilityUnknown. Other statuses keep their + // pre-existing behavior rather than entering the retry loop on a body that + // could not be read. + if readErr != nil && resp.StatusCode == http.StatusBadRequest { + // A 400 whose body did not read completely: its text cannot be used to + // classify retryability, so surface the read failure AND tag the error + // with errRetryabilityUnknown so the retry loop treats it as retryable + // rather than abandoning the purchase on its first attempt. + return fmt.Errorf("reservation purchase failed with status %d (partial body: %q): %w: %w", + resp.StatusCode, string(body), errRetryabilityUnknown, readErr) + } return fmt.Errorf("reservation purchase failed with status %d: %s", resp.StatusCode, string(body)) } diff --git a/providers/azure/services/internal/reservations/purchase_test.go b/providers/azure/services/internal/reservations/purchase_test.go index f731b5af6..2d91086bd 100644 --- a/providers/azure/services/internal/reservations/purchase_test.go +++ b/providers/azure/services/internal/reservations/purchase_test.go @@ -497,7 +497,7 @@ func TestFindReservationOrderByIdempotencyToken_PaginatedFollowsNextLink(t *test // TestDoIdempotentPurchaseTwoStep_EmptyToken_NoLookup pins the CLI legacy path: // when no idempotency token is supplied the wrapper falls straight through to // the raw DoPurchaseTwoStep (no list call), preserving the pre-issue-721 -// behaviour for callers without an owning execution. +// behavior for callers without an owning execution. func TestDoIdempotentPurchaseTwoStep_EmptyToken_NoLookup(t *testing.T) { m := &mockHTTPClient{} ctx := context.Background() @@ -643,7 +643,7 @@ func TestDoIdempotentPurchaseTwoStep_DifferentTokens_DistinctReservations(t *tes // TestDoIdempotentPurchaseTwoStep_PreservesTwoStepFlow verifies the two-step // flow's session-timeout retry semantics from PR #680 still work under the // new wrapper -- the wrapper is purely additive for the lookup, and once it -// falls through to DoPurchaseTwoStep the original retry behaviour applies. +// falls through to DoPurchaseTwoStep the original retry behavior applies. func TestDoIdempotentPurchaseTwoStep_PreservesTwoStepFlow(t *testing.T) { m := &mockHTTPClient{} ctx := context.Background() @@ -810,3 +810,198 @@ func TestBillingPlanForPaymentOption_UnrecognizedIsNotBlamedOnPartialUpfront(t * require.Error(t, paddedErr) assert.Contains(t, paddedErr.Error(), "partial-upfront has no azure equivalent") } + +// truncatingBody yields the first n bytes of data and then fails, modeling a +// response whose body is cut short mid-transfer. +type truncatingBody struct { + data []byte + n int + off int +} + +func (b *truncatingBody) Read(p []byte) (int, error) { + if b.off >= b.n { + return 0, errors.New("unexpected EOF reading response body") + } + k := copy(p, b.data[b.off:b.n]) + b.off += k + return k, nil +} + +func (b *truncatingBody) Close() error { return nil } + +// TestDoPurchase_UnreadableBodyDoesNotSilentlyLookPermanent is the regression +// test for the retry-classification defect. +// +// doPurchase's error text is the ONLY input to IsSessionTimeout, and +// DoPurchaseTwoStep retries only when that predicate matches. When the read +// error was discarded, a 400 whose body was truncated before the +// "Session timed out" fragment produced a bare `status 400: ` error. +// IsSessionTimeout then returned false and the purchase was abandoned on its +// first attempt — the precise condition purchaseMaxAttempts exists to survive — +// with no indication that anything had gone wrong reading the response. +// +// The fix must not classify retryability from a body that was never fully +// received. Pre-fix this test fails: the error carries neither the read failure +// nor the session-timeout fragment, so both assertions below are false. +func TestDoPurchase_UnreadableBodyDoesNotSilentlyLookPermanent(t *testing.T) { + ctx := context.Background() + full := `{"error":{"code":"BadRequest","message":"Session timed out - Call CalculatePrice again"}}` + + m := &mockHTTPClient{} + m.On("Do", mock.Anything).Return(&http.Response{ + StatusCode: http.StatusBadRequest, + // Cut short well before the "Session timed out" fragment. + Body: &truncatingBody{data: []byte(full), n: 12}, + Header: make(http.Header), + }, nil).Once() + + err := doPurchase(ctx, m, "https://example.invalid/purchase", []byte(testBody), "tok") + require.Error(t, err) + + assert.ErrorContains(t, err, "did not read completely", + "a failed body read must be surfaced, not discarded") + assert.ErrorContains(t, err, "unexpected EOF reading response body", + "the underlying read error must be wrapped so the operator can see why retryability is unknown") + + // Retryability is carried out of band, because it cannot be read off a body + // that never fully arrived. Asserting the sentinel rather than the message + // is the point: the message is not what the retry loop consults. + assert.ErrorIs(t, err, errRetryabilityUnknown, + "an unreadable body must mark retryability unknown rather than leave it inferred from text") + + m.AssertExpectations(t) +} + +// countingPurchaseClient answers calculatePrice with a fixed order ID and counts +// purchase attempts, minting a NEW response per call because the truncating body +// is stateful and cannot be replayed across attempts. +type countingPurchaseClient struct { + purchases int + purchaseResp func() *http.Response +} + +func (c *countingPurchaseClient) Do(req *http.Request) (*http.Response, error) { + if req.URL.String() == PurchaseURL("order-1") { + c.purchases++ + return c.purchaseResp(), nil + } + return fakeResp(http.StatusOK, `{"properties":{"reservationOrderId":"order-1"}}`), nil +} + +// TestDoPurchaseTwoStep_TruncatedSessionTimeoutStillRetries is the regression +// test for the retry decision, and it is deliberately an ATTEMPT-COUNT +// assertion rather than a message assertion. +// +// An earlier revision of this fix surfaced the read error in the message and +// asserted on that text. That assertion passed while the purchase was still +// abandoned on its first attempt, because retryability was still being inferred +// from a truncated body: the message changed, the behavior did not. Only +// counting attempts tells the two apart. +// +// The readable control is included so this measures the loop rather than a +// constant. If the control ever stops reaching the cap, the second half is no +// longer evidence about anything. +func TestDoPurchaseTwoStep_TruncatedSessionTimeoutStillRetries(t *testing.T) { + ctx := context.Background() + full := `{"error":{"code":"BadRequest","message":"Session timed out - Call CalculatePrice again"}}` + + control := &countingPurchaseClient{ + purchaseResp: func() *http.Response { return fakeResp(http.StatusBadRequest, full) }, + } + _, err := DoPurchaseTwoStep(ctx, control, calcURL, []byte(testBody), "tok") + require.Error(t, err) + require.Equal(t, purchaseMaxAttempts, control.purchases, + "control: a readable session-timeout 400 must retry to the cap, or this test measures nothing") + + truncated := &countingPurchaseClient{ + purchaseResp: func() *http.Response { + return &http.Response{ + StatusCode: http.StatusBadRequest, + // Cut short well before the "Session timed out" fragment. + Body: &truncatingBody{data: []byte(full), n: 12}, + Header: make(http.Header), + } + }, + } + _, err = DoPurchaseTwoStep(ctx, truncated, calcURL, []byte(testBody), "tok") + require.Error(t, err) + assert.Equal(t, purchaseMaxAttempts, truncated.purchases, + "a session timeout whose body did not read completely must still be retried; "+ + "pre-fix this was 1 attempt because retryability was inferred from a truncated body") +} + +// TestDoPurchase_UnreadableBodyOnSuccessStillSucceeds guards the opposite +// direction: success is decided by the status code alone, so a body that fails +// to read must not turn a completed purchase into a reported failure. +func TestDoPurchase_UnreadableBodyOnSuccessStillSucceeds(t *testing.T) { + ctx := context.Background() + m := &mockHTTPClient{} + m.On("Do", mock.Anything).Return(&http.Response{ + StatusCode: http.StatusOK, + Body: &truncatingBody{data: []byte(`{"ok":true}`), n: 3}, + Header: make(http.Header), + }, nil).Once() + + require.NoError(t, doPurchase(ctx, m, "https://example.invalid/purchase", []byte(testBody), "tok")) + m.AssertExpectations(t) +} + +// TestDoPurchase_TruncatedNon400IsNotRetryable pins the deliberate narrowing of +// errRetryabilityUnknown to 400. +// +// Without this the sentinel fires on ANY non-2xx whose body fails to read -- +// 409, 500, 502, 504 alike -- which is far broader than IsSessionTimeout, whose +// existing scope is a 400 plus an Azure-authored message. Breadth matters here +// because DoIdempotentPurchaseTwoStep performs its idempotency lookup exactly +// once, BEFORE DoPurchaseTwoStep is entered: there is no recheck between +// attempts, and each retry mints a fresh reservationOrderId. So an in-loop +// retry after a status that might follow a committed purchase is a double-buy +// risk that the guard does not cover (tracked separately as issue #1774). +// +// Issue #1766's defect is a 400 "Session timed out" with a truncated body, so +// narrowing costs no coverage and removes the unverified commit-ambiguous +// statuses from the retry surface entirely. +func TestDoPurchase_TruncatedNon400IsNotRetryable(t *testing.T) { + ctx := context.Background() + for _, status := range []int{ + http.StatusConflict, + http.StatusInternalServerError, + http.StatusBadGateway, + http.StatusGatewayTimeout, + } { + m := &mockHTTPClient{} + m.On("Do", mock.Anything).Return(&http.Response{ + StatusCode: status, + Body: &truncatingBody{data: []byte(`{"error":"truncated"}`), n: 6}, + Header: make(http.Header), + }, nil).Once() + + err := doPurchase(ctx, m, "https://example.invalid/purchase", []byte(testBody), "tok") + require.Error(t, err) + assert.NotErrorIs(t, err, errRetryabilityUnknown, + "status %d with a truncated body must NOT be marked retryable: a retry there could "+ + "re-purchase after a commit the idempotency guard does not re-check for (issue #1774)", status) + m.AssertExpectations(t) + } +} + +// TestDoPurchaseTwoStep_TruncatedNon400DoesNotRetry is the behavioral half of +// the narrowing: the sentinel scoping must actually keep the retry loop out of +// the commit-ambiguous statuses, not merely leave the error untagged. +func TestDoPurchaseTwoStep_TruncatedNon400DoesNotRetry(t *testing.T) { + c := &countingPurchaseClient{ + purchaseResp: func() *http.Response { + return &http.Response{ + StatusCode: http.StatusInternalServerError, + Body: &truncatingBody{data: []byte(`{"error":"truncated"}`), n: 6}, + Header: make(http.Header), + } + }, + } + _, err := DoPurchaseTwoStep(context.Background(), c, calcURL, []byte(testBody), "tok") + require.Error(t, err) + assert.Equal(t, 1, c.purchases, + "a truncated 500 must be attempted exactly once; retrying it risks re-purchasing after "+ + "a commit that the once-only idempotency lookup cannot see (issue #1774)") +}