From ae3439b877d49d924c0891eae8a821fc359fc583 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 22:32:46 +0200 Subject: [PATCH] docs(azure): correct two retry comments that #1767 made false Comment-only. Both make exhaustiveness claims that stopped being true when #1767 added a second retry trigger, and both now describe the current rule. purchase_test.go said doPurchase's error text is "the ONLY input to IsSessionTimeout, and DoPurchaseTwoStep retries only when that predicate matches". Neither half holds: the loop also retries on errors.Is(err, errRetryabilityUnknown). CodeRabbit flagged this on #1767; the PR merged before the fix landed, so the claim is live on main. DoPurchaseTwoStep's own doc had the same defect and nobody flagged it. It said retry happens on a "Session timed out" 400 and that "Other 4xx/5xx errors are returned immediately without retry", which omits the second trigger entirely. Found by sweeping the whole #1767 diff for the same class rather than fixing only the line that was reported. It now enumerates both cases and states that everything else -- including a cleanly read 4xx and any 5xx -- returns without retry. Rather than past-tensing the old text, each comment leads with the current rule and keeps the pre-fix explanation marked as the reason the test exists. A reader who stops after the first paragraph should come away with something true. Exhaustiveness claims are what readers rely on to decide they need not check, so a stale one is worse than none: #1757 (a live CSRF hole) survived behind a parity claim that was not true, and #1764's getAccountScope godoc described the inverse of its behavior after a change. Refs #1767, #1766 Co-Authored-By: Claude Opus 5 (1M context) --- .../internal/reservations/purchase.go | 15 ++++++++--- .../internal/reservations/purchase_test.go | 26 ++++++++++++------- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/providers/azure/services/internal/reservations/purchase.go b/providers/azure/services/internal/reservations/purchase.go index 88c8f3ebd..fdb4da3c8 100644 --- a/providers/azure/services/internal/reservations/purchase.go +++ b/providers/azure/services/internal/reservations/purchase.go @@ -261,10 +261,17 @@ const ( // DoPurchaseTwoStep executes the calculatePrice->purchase two-step flow. // // It POSTs bodyBytes to calculateURL to mint an Azure-assigned reservationOrderId, -// then POSTs the same body to the derived purchaseURL. On a "Session timed out" -// 400 from the purchase endpoint (Azure has retired the session) it re-runs -// calculatePrice from scratch (up to purchaseMaxAttempts total attempts). -// Other 4xx/5xx errors are returned immediately without retry. +// then POSTs the same body to the derived purchaseURL. It re-runs calculatePrice +// from scratch (up to purchaseMaxAttempts total attempts) in exactly two cases: +// +// 1. a "Session timed out" 400 from the purchase endpoint, meaning Azure has +// retired the session; and +// 2. a 400 whose response body did not read completely, where case 1 cannot be +// ruled out because the fragment it matches on may be in the part that never +// arrived (see errRetryabilityUnknown). +// +// Every other response, including a cleanly read 4xx or any 5xx, is returned +// immediately without retry. // // Returns the Azure-minted reservationOrderId on success, which the caller // should store as the CommitmentID. diff --git a/providers/azure/services/internal/reservations/purchase_test.go b/providers/azure/services/internal/reservations/purchase_test.go index 2d91086bd..d35418995 100644 --- a/providers/azure/services/internal/reservations/purchase_test.go +++ b/providers/azure/services/internal/reservations/purchase_test.go @@ -833,17 +833,23 @@ 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. +// Current rule: DoPurchaseTwoStep retries when IsSessionTimeout matches the +// error text, OR when the error carries errRetryabilityUnknown — which +// doPurchase attaches to a 400 whose body did not read completely, and to +// nothing else. Retryability is no longer inferred solely from text. // -// 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. +// Why the test exists (pre-fix behavior): the error text was once the only +// input to IsSessionTimeout, and that predicate the only retry trigger. With +// the read error discarded, a 400 truncated before the "Session timed out" +// fragment produced a bare `status 400: `; IsSessionTimeout 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. +// +// This test pins the surfacing and the out-of-band tag. The retry decision +// itself is pinned by TestDoPurchaseTwoStep_TruncatedSessionTimeoutStillRetries, +// which counts attempts, because a message assertion cannot tell a changed +// message from changed behavior. func TestDoPurchase_UnreadableBodyDoesNotSilentlyLookPermanent(t *testing.T) { ctx := context.Background() full := `{"error":{"code":"BadRequest","message":"Session timed out - Call CalculatePrice again"}}`