sec(azure): idempotency is not re-checked between in-process purchase retries, so a committed-but-failed attempt can double-buy

[cristim]

Found during the safety review of PR #1767. Pre-existing on main, not introduced by that PR — live since the retry path was added (#677 / #1550). Filed separately so #1767 can land narrowly.

The gap

DoIdempotentPurchaseTwoStep calls FindReservationOrderByIdempotencyToken exactly once, before DoPurchaseTwoStep is entered. DoPurchaseTwoStep's internal loop then runs up to purchaseMaxAttempts (3) with no idempotency recheck between attempts.

Each retry calls doCalculatePrice fresh, minting a new distinct reservationOrderId, then doPurchase again. Nothing looks up the idempotency token in between.

Demonstrated, not inferred

A probe drove attempt 1 to return a retryable failure and attempt 2 to return 200 OK. Result:

2 distinct calculatePrice calls   (order IDs "order-1", "order-2")
2 distinct purchase calls
0 idempotency lookups between them

Both succeeded.

What this means

The idempotency guard catches a re-drive across separate top-level DoIdempotentPurchaseTwoStep invocations — a crashed process, a re-run execution, a scheduler retry. That is the case it was built for and it works.

It does not catch a double-commit within a single invocation's own retry loop. If attempt 1 silently committed on the provider side and returned a failure the client classified as retryable, attempt 2 would commit again, with a different order ID, unguarded.

Blast radius is bounded at purchaseMaxAttempts — 2-3 duplicate orders, not unbounded.

Why it matters more than it did

Today the only trigger reaching this path is IsSessionTimeout: a 400 and an Azure-authored "Session timed out" message. That is a narrow, explicit, vendor-stated signal that the session died — so "the order did not commit" is a reasonable reading of Azure's own words.

Any widening of the retry trigger set inherits this unguarded path. PR #1767 originally widened it to any non-2xx paired with a transport-level read failure — an inferred condition with no equivalent vendor guarantee — and is being narrowed back to 400 for exactly this reason. That narrowing is a mitigation, not a fix: the underlying gap remains.

Fix directions (not decided — needs a call)

1. Re-check idempotency between in-loop attempts. Most direct. Costs a store lookup per retry, and needs care that the lookup's own failure does not become a new fall-through — DoIdempotentPurchaseTwoStep already refuses to purchase on a lookup error, and that property must hold here too.
2. Reuse the reservationOrderId across attempts of the same logical purchase rather than minting a new one per attempt, so the provider can deduplicate. Depends on whether Azure treats a re-submitted order ID as idempotent — verify against the API contract before assuming, since the whole issue is an unverified vendor-behaviour assumption.
3. Accept and document. If the trigger set stays narrow enough that commit-on-failure is genuinely implausible, state the bound explicitly in DoPurchaseTwoStep's doc comment — currently nothing there warns that the idempotency guard does not span the loop.

Option 2 is the most attractive if the contract supports it, because it removes the ambiguity rather than detecting it after the fact. Establish the Azure behaviour first; that question is what made this reviewable-but-unresolvable in #1767.

Verification for whatever lands

Both directions, and the second is the one that gets skipped:

• a retry after a possibly-committed first attempt must not produce a second order
• a retry after a genuinely-failed first attempt must still succeed — a fix that refuses all retries would pass a duplicate-only test while breaking the recovery this path exists for

Drive the real loop with a counting client. Note the fixture bodies used in this area are stateful and cannot be reused across attempts — mint a fresh one per response, or the control will lie.

Related

• fix(azure): surface the read error that decides purchase retry-vs-abort #1767 — narrows the trigger set back to 400; mitigates, does not fix.
• fix(azure): discarded io.ReadAll error decides retry-vs-abort on the reservation purchase path #1766 — the truncated-read defect that surfaced this.