[cristim]

Comment-only follow-up to #1767, which merged (d6e60f637) before these two fixes landed. No behavior change; the lint set diff is byte-identical to main.

What was wrong

Both comments make exhaustiveness claims that stopped being true when #1767 added a second retry trigger.

1. purchase_test.go — flagged by CodeRabbit on #1767, unfixed at merge time:

// 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).

2. DoPurchaseTwoStep's own doc — nobody flagged this one:

// 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.

That omits the second trigger entirely, in the production doc a caller reads to decide whether a failure will be retried. Found by sweeping the whole #1767 diff for the same class rather than fixing only the reported line.

What they say now

Each leads with the current rule and keeps the pre-fix explanation marked as the reason the test exists, rather than past-tensing the old text into history. A reader who stops after the first paragraph should come away with something true.

DoPurchaseTwoStep now enumerates both retry cases and states plainly that everything else — including a cleanly read 4xx and any 5xx — returns without retry.

Why this is worth its own PR

Exhaustiveness claims are what readers rely on to decide they do not need to check, so a stale one is worse than no comment. In this repo that class has already cost real defects: #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.

#1767 spent two review rounds removing an assertion that read as a guarantee it did not provide. Leaving comments that read as guarantees they no longer provide is the same defect in prose.

Gates

providers/azure: gofmt clean, build, vet, go test -race -count=1 ./... green. Root: build, vet, gocyclo -over 10 -ignore "_test\.go" . exit 0 empty, golangci-lint v2.10.1 exit 0 with a genuine 0 issues. line (both asserted).

providers/azure lint set diff vs main: 589 vs 589, zero only-in-HEAD, zero only-in-BASE — byte-identical, as a comment-only change should be.

Summary by CodeRabbit

• Bug Fixes
  • Improved handling of purchase requests when a 400 response body cannot be read, allowing eligible requests to be retried.
  • Preserved non-retryable behavior for clearly readable 4xx responses and all 5xx responses.
  • Improved retry decisions by relying on response context rather than error message text alone.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 769929e7-f145-4fc1-8bd3-5a859b96cc42

📥 Commits

Reviewing files that changed from the base of the PR and between d6e60f6 and ae3439b.

📒 Files selected for processing (2)

• providers/azure/services/internal/reservations/purchase.go
• providers/azure/services/internal/reservations/purchase_test.go

---

📝 Walkthrough

Walkthrough

The PR updates DoPurchaseTwoStep documentation and regression-test comments to describe retries for unreadable 400 responses, recognized session-timeout errors, and immediate returns for cleanly read 4xx and 5xx responses.

Changes

Azure purchase retry documentation

Layer / File(s)	Summary
Document retry behavior providers/azure/services/internal/reservations/purchase.go, providers/azure/services/internal/reservations/purchase_test.go	The documentation now covers the three-attempt limit for unreadable 400 responses, non-retriable cleanly read 4xx and 5xx responses, the errRetryabilityUnknown marker, and separate validation of error reporting and retry attempts.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Possibly related PRs

• LeanerCloud/CUDly#729: Modifies idempotency handling around the same reservation purchase flow.
• LeanerCloud/CUDly#1045: Modifies purchase.go and its tests.
• LeanerCloud/CUDly#1767: Covers retry behavior and unreadable response handling in DoPurchaseTwoStep.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the Azure documentation-only changes to correct two retry comments.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/1767-followup-stale-retry-comments

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Merging. Comment-only correction to two retry claims that #1767 falsified.

CodeRabbit caught one; the sweep found the one that mattered more, and CR did not flag it.

The reported line was a test function preamble. I initially escalated it, then downgraded on the reasoning that stale prose sitting next to a correct authoritative statement is not misleading. That reasoning was right and I applied it to the wrong file. DoPurchaseTwoStep's own doc comment said:

// Other 4xx/5xx errors are returned immediately without retry.

That is production documentation a caller reads from outside the package to decide whether a failure will be retried — precisely the thing someone relies on to skip a check. After #1767 it omitted the second retry trigger entirely.

The distinction that matters, and the rule taken from it: a stale comment blocks when something relies on it to skip a check. Security-control claims, godoc on a seam, a named test guarantee. Explanatory prose adjacent to a correct current statement does not. Three real defects here came from the first category — #1757's live CSRF vulnerability survived because a comment asserted parity that did not exist; #1764's godoc asserted the inverse of what its function returned; #1758's docstring named a backstop that can never fire.

The sweep is what found it. The finding was described as a class rather than a line, so the whole #1767 diff was swept rather than the reported site patched. The rest of its exhaustiveness language was then verified against the code — "the only scenario #1766 reports", "there is no recheck between attempts", "Success is decided by the status code alone", "and to nothing else" — all true, with the sentinel confirmed attached at exactly one site. That verification is what separates a sweep from a spot check.

Both comments lead with the current rule rather than past-tensing. CR's proposed diff was accurate but would have left the reader with history and no rule; someone who stops after the first paragraph should still come away with something true. The pre-fix explanation stays, marked as the reason the test exists.

The lint set diff is 589 vs 589, zero on either side — byte-identical, which is what a comment-only change must produce and the check that proves it is comment-only.

Gates: providers/azure gofmt/build/vet clean, go test -race green; root build, vet, gocyclo exit 0 empty, golangci-lint v2.10.1 exit 0 with a genuine 0 issues. line.