From 15ec6662bd6d4e4a29793516bca2347cabb5fd9e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 07:44:59 +0200 Subject: [PATCH 1/3] fix(azure): surface the read error that decides purchase retry-vs-abort doPurchase discarded the io.ReadAll error, and its error string is the only input to IsSessionTimeout, which is the only thing DoPurchaseTwoStep consults to decide whether to retry. A 400 whose body was truncated before the "Session timed out" fragment therefore produced a bare `status 400: ` error, IsSessionTimeout returned false, and the purchase was abandoned on its first attempt -- the exact condition purchaseMaxAttempts exists to survive -- with nothing in the message to say the response had not been fully read. The failure direction is closed: it abandons a purchase rather than double-buying. But a designed-retryable failure became permanent silently, and the diagnostic that would explain it was the thing that went missing. Retryability is no longer classified from a body that was never fully received. On a non-2xx response with a failed read, the read error is wrapped and surfaced along with the partial body. Success is still decided by the status code alone, so an unreadable body cannot turn a completed purchase into a reported failure. Two regression tests, both driven by a body that yields 12 bytes and then fails. The first pins that an unreadable body cannot silently produce a non-retryable classification; it fails against the pre-fix code, where the error read `reservation purchase failed with status 400: {"error":{"c` and carried neither the read failure nor the session-timeout fragment. The second guards the opposite direction, that a truncated body on a 200 still succeeds. The two sibling io.ReadAll discards in this file were checked rather than assumed and are not affected: :287 in doCalculatePrice and :390 in fetchReservationOrdersPage both feed a json.Unmarshal that fails on truncation, and :390's caller DoIdempotentPurchaseTwoStep explicitly refuses to purchase on a lookup error rather than falling through. golangci-lint on providers/azure, compared as a set diff rather than by totals: zero new findings, one removed (the errcheck this fixes), 592 -> 591. The totals alone would have hidden it -- the first attempt at this change removed one finding and added two misspellings in new comments. Closes #1766 Co-Authored-By: Claude Opus 5 (1M context) --- .../internal/reservations/purchase.go | 15 +++- .../internal/reservations/purchase_test.go | 81 +++++++++++++++++++ 2 files changed, 95 insertions(+), 1 deletion(-) diff --git a/providers/azure/services/internal/reservations/purchase.go b/providers/azure/services/internal/reservations/purchase.go index df2f31d43..6076eb3ff 100644 --- a/providers/azure/services/internal/reservations/purchase.go +++ b/providers/azure/services/internal/reservations/purchase.go @@ -472,11 +472,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 } + if readErr != nil { + // IsSessionTimeout classifies this error by searching its text for the + // fragment Azure puts in the body, and DoPurchaseTwoStep retries only + // when that matches. Discarding the read error therefore made a + // retryable session timeout look permanent whenever the body failed to + // read, abandoning the purchase on its first attempt with an empty + // diagnostic. Surface the read failure rather than classifying + // retryability from a body that was never fully received. + return fmt.Errorf("reservation purchase failed with status %d and its response body could not be read, so retryability is unknown (partial body: %q): %w", + resp.StatusCode, string(body), 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..523617188 100644 --- a/providers/azure/services/internal/reservations/purchase_test.go +++ b/providers/azure/services/internal/reservations/purchase_test.go @@ -6,6 +6,7 @@ import ( "errors" "io" "net/http" + "strings" "testing" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" @@ -810,3 +811,83 @@ 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, "could not be read", + "a failed body read must be surfaced, not discarded: the retry decision is made from this error's text") + 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") + + // The essential property: the failure must not be silently classified as a + // permanent, non-retryable error on the strength of a body that never + // arrived. Either it is recognizably retryable, or the read failure is + // visible — never a bare status line that quietly means "give up". + assert.True(t, + IsSessionTimeout(err) || strings.Contains(err.Error(), "could not be read"), + "an unreadable body must not silently produce a non-retryable classification") + + m.AssertExpectations(t) +} + +// 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) +} From 6eef4ea3b8579783d9dd69000223ccdb5fb94aa7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 09:30:21 +0200 Subject: [PATCH 2/3] fix(azure): retry a purchase whose retryability could not be read The previous commit surfaced the discarded read error in the message but did not change the retry decision. IsSessionTimeout still did strings.Contains on a string built from a truncated body, so a session-timeout 400 whose body did not read completely was still abandoned on its first attempt -- the exact condition purchaseMaxAttempts exists to survive. The message improved; the behaviour did not. Measured on the real loop with a counting client, minting a fresh response per attempt because the truncating body is stateful: readable session-timeout 400 3 purchase attempts (control) truncated session-timeout 400 1 purchase attempt (defect) The control moves, so the probe measures the loop rather than a constant. Retryability is now carried out of band. doPurchase wraps errRetryabilityUnknown when a non-2xx body fails to read, and DoPurchaseTwoStep treats errors.Is(err, errRetryabilityUnknown) as retryable alongside IsSessionTimeout. Retrying is safe here: a non-2xx means the order did not commit, the loop is bounded by purchaseMaxAttempts, and re-drives across invocations are guarded by DoIdempotentPurchaseTwoStep, which refuses to purchase when its idempotency lookup fails. The regression test is an ATTEMPT-COUNT assertion, not a message assertion. The previous test asserted on the error text and passed while the purchase was still abandoned, which is how the gap survived review; only counting attempts tells a changed message from changed behaviour. It fails against the message-only fix with expected 3, actual 1, and carries the readable control so it measures the loop rather than a constant. The "essential property" assertion it replaces was tautological once the message changed, while being labelled as pinning retryability -- a property that did not hold. That is the same defect class as #1595 and #1740: an assertion reading as a guarantee it does not provide. Drive-by, disclosed: a global spelling fix in this test file also corrected two pre-existing `behaviour` misspellings that main already carried. Set diff against main: zero new findings, three removed. Closes #1766 Co-Authored-By: Claude Opus 5 (1M context) --- .../internal/reservations/purchase.go | 36 ++++++--- .../internal/reservations/purchase_test.go | 79 ++++++++++++++++--- 2 files changed, 93 insertions(+), 22 deletions(-) diff --git a/providers/azure/services/internal/reservations/purchase.go b/providers/azure/services/internal/reservations/purchase.go index 6076eb3ff..164890fd4 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,21 @@ type calculatePriceResponse struct { // phrasing changes in future API versions. const sessionTimeoutFragment = "Session timed out" +// errRetryabilityUnknown marks a 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. +// +// Retrying is safe here: a non-2xx means the order did not commit, the loop is +// bounded by purchaseMaxAttempts, and re-drives across invocations are guarded +// by DoIdempotentPurchaseTwoStep, which refuses to purchase when its idempotency +// lookup fails. +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 +271,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 { @@ -481,15 +500,12 @@ func doPurchase(ctx context.Context, httpClient HTTPClient, purchaseURL string, return nil } if readErr != nil { - // IsSessionTimeout classifies this error by searching its text for the - // fragment Azure puts in the body, and DoPurchaseTwoStep retries only - // when that matches. Discarding the read error therefore made a - // retryable session timeout look permanent whenever the body failed to - // read, abandoning the purchase on its first attempt with an empty - // diagnostic. Surface the read failure rather than classifying - // retryability from a body that was never fully received. - return fmt.Errorf("reservation purchase failed with status %d and its response body could not be read, so retryability is unknown (partial body: %q): %w", - resp.StatusCode, string(body), readErr) + // The body did not read completely, so its text cannot be used to + // classify retryability. 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 523617188..04342b139 100644 --- a/providers/azure/services/internal/reservations/purchase_test.go +++ b/providers/azure/services/internal/reservations/purchase_test.go @@ -6,7 +6,6 @@ import ( "errors" "io" "net/http" - "strings" "testing" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" @@ -498,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() @@ -644,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() @@ -860,22 +859,78 @@ func TestDoPurchase_UnreadableBodyDoesNotSilentlyLookPermanent(t *testing.T) { err := doPurchase(ctx, m, "https://example.invalid/purchase", []byte(testBody), "tok") require.Error(t, err) - assert.ErrorContains(t, err, "could not be read", - "a failed body read must be surfaced, not discarded: the retry decision is made from this error's text") + 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") - // The essential property: the failure must not be silently classified as a - // permanent, non-retryable error on the strength of a body that never - // arrived. Either it is recognizably retryable, or the read failure is - // visible — never a bare status line that quietly means "give up". - assert.True(t, - IsSessionTimeout(err) || strings.Contains(err.Error(), "could not be read"), - "an unreadable body must not silently produce a non-retryable classification") + // 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. From c3b629c1c3acd28c82c3e3cde927f127c6f4efd3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 17:44:53 +0200 Subject: [PATCH 3/3] fix(azure): scope the retryability sentinel to 400 errRetryabilityUnknown fired on ANY non-2xx whose body failed to read -- 409, 500, 502, 504 alike -- which is far broader than the IsSessionTimeout path it sits beside, and broader than the defect it was written for. That breadth is load-bearing because the idempotency guard does not cover in-loop retries. DoIdempotentPurchaseTwoStep calls FindReservationOrderByIdempotencyToken exactly once, before DoPurchaseTwoStep is entered; the loop then runs up to purchaseMaxAttempts with no recheck between attempts, and each retry mints a fresh reservationOrderId via doCalculatePrice. Verified here: zero lookups occur inside the loop. So the guard catches a re-drive ACROSS invocations and nothing within one, which makes "a non-2xx means the order did not commit" the only thing standing between a truncated read and a double purchase -- and that premise is unverified for this API on 409 and 5xx. A truncated response is also mechanistically different from a cleanly read one: truncation is the transport-failure class associated with genuine commit ambiguity, since the server may have finished processing and begun writing when the connection died. A cleanly read 500 is a deliberate synchronous statement. The previous comment treated the two as equivalent. Issue #1766's defect is a 400 "Session timed out" with a truncated body, the only scenario in the issue, the PR description and both regression tests, so scoping to 400 closes it with no loss of coverage while removing the unverified commit-ambiguous statuses from the retry surface. Everything else keeps its pre-existing behavior. The sentinel's doc comment no longer asserts that a non-2xx means no commit. It states the scope, why the wider surface is excluded, and that the bound is purchaseMaxAttempts rather than the idempotency guard. Two tests pin the narrowing so it cannot be silently widened again: 409, 500, 502 and 504 with truncated bodies must not carry the sentinel, and a truncated 500 must be attempted exactly once. Both fail when the condition is widened back to any non-2xx. The attempt-count regression test still fails against the message-only revision with expected 3, actual 1. The missing in-loop idempotency recheck is pre-existing on the IsSessionTimeout path and is tracked as #1774; it is deliberately not addressed here. Refs #1766 Co-Authored-By: Claude Opus 5 (1M context) --- .../internal/reservations/purchase.go | 32 +++++++--- .../internal/reservations/purchase_test.go | 59 +++++++++++++++++++ 2 files changed, 82 insertions(+), 9 deletions(-) diff --git a/providers/azure/services/internal/reservations/purchase.go b/providers/azure/services/internal/reservations/purchase.go index 164890fd4..88c8f3ebd 100644 --- a/providers/azure/services/internal/reservations/purchase.go +++ b/providers/azure/services/internal/reservations/purchase.go @@ -210,8 +210,8 @@ type calculatePriceResponse struct { // phrasing changes in future API versions. const sessionTimeoutFragment = "Session timed out" -// errRetryabilityUnknown marks a purchase failure whose retryability could not -// be determined because the response body did not read completely. +// 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 @@ -219,10 +219,21 @@ const sessionTimeoutFragment = "Session timed out" // lets DoPurchaseTwoStep retry instead of silently abandoning a purchase that // the retry loop exists to recover. // -// Retrying is safe here: a non-2xx means the order did not commit, the loop is -// bounded by purchaseMaxAttempts, and re-drives across invocations are guarded -// by DoIdempotentPurchaseTwoStep, which refuses to purchase when its idempotency -// lookup fails. +// 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" @@ -499,9 +510,12 @@ func doPurchase(ctx context.Context, httpClient HTTPClient, purchaseURL string, if resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusCreated || resp.StatusCode == http.StatusAccepted { return nil } - if readErr != nil { - // The body did not read completely, so its text cannot be used to - // classify retryability. Surface the read failure AND tag the error + // 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", diff --git a/providers/azure/services/internal/reservations/purchase_test.go b/providers/azure/services/internal/reservations/purchase_test.go index 04342b139..2d91086bd 100644 --- a/providers/azure/services/internal/reservations/purchase_test.go +++ b/providers/azure/services/internal/reservations/purchase_test.go @@ -946,3 +946,62 @@ func TestDoPurchase_UnreadableBodyOnSuccessStillSucceeds(t *testing.T) { 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)") +}