fix(azure): match purchase dedupe on order display name, not only tags

providers/azure/services/internal/reservations/idempotencymarker.go

@@ -0,0 +1,100 @@
+package reservations
+
+import (
+	"crypto/sha256"
+	"encoding/hex"
+	"strings"
+	"unicode/utf8"
+)
+
+// The Reservations API does not define tags on a reservation order: neither
+// armreservations.PurchaseRequest nor ReservationOrderResponse (SDK v1.1.0)
+// has a Tags field, so the cudly-idempotency-token tag written by
+// ApplyPurchaseTags may never be stored or listed back (issue #290). Both
+// models do define properties.displayName, so the idempotency token is also
+// carried there, as a short deterministic marker at the end of the name, and
+// FindReservationOrderByIdempotencyToken matches on either.
+
+// idempotencyMarkerPrefix starts every display-name marker. It is
+// alphanumeric so the marker stays inside Azure's [A-Za-z0-9_-] allowlist and
+// contains no '-' (the separator the marker is appended with).
+const idempotencyMarkerPrefix = "cudly"
+
+// idempotencyMarkerHexLen is how many hex digits of SHA-256(token) the marker
+// keeps: 64 bits, so a collision between two tokens within one tenant's order
+// listing is negligible, while leaving 42 of the 64 display-name characters
+// for the human-readable part.
+const idempotencyMarkerHexLen = 16
+
+// idempotencyDisplayNameMarker returns the display-name marker for token. It
+// is derived from a hash rather than a token prefix because callers may supply
+// tokens of any length or character set.
+func idempotencyDisplayNameMarker(token string) string {
+	sum := sha256.Sum256([]byte(token))
+	return idempotencyMarkerPrefix + hex.EncodeToString(sum[:])[:idempotencyMarkerHexLen]
+}
+
+// displayNameCarriesToken reports whether an order's display name ends with
+// the marker for token, as written by withIdempotencyMarker.
+func displayNameCarriesToken(displayName, token string) bool {
+	marker := idempotencyDisplayNameMarker(token)
+	return displayName == marker || strings.HasSuffix(displayName, "-"+marker)
+}
+
+// withIdempotencyMarker appends token's marker to displayName, keeping the
+// result within azureDisplayNameMaxLen. To make room it drops whole trailing
+// '-' segments first (for BuildDisplayName output that is the random suffix,
+// then the timestamp, which the marker makes redundant for uniqueness), and
+// hard-truncates only a name with no separator left to drop at.
+func withIdempotencyMarker(displayName, token string) string {
+	marker := idempotencyDisplayNameMarker(token)
+	room := azureDisplayNameMaxLen - len(marker) - 1
+	name := strings.TrimRight(displayName, "-")
+	for len(name) > room {
+		cut := strings.LastIndexByte(name, '-')
+		if cut <= 0 {
+			name = truncateToRuneBoundary(name, room)
+			break
+		}
+		name = strings.TrimRight(name[:cut], "-")
+	}
+	if name == "" {
+		return marker
+	}
+	return name + "-" + marker
+}
+
+// truncateToRuneBoundary returns the longest prefix of s that is at most
+// maxBytes long and does not split a multibyte UTF-8 rune.
+func truncateToRuneBoundary(s string, maxBytes int) string {
+	if len(s) <= maxBytes {
+		return s
+	}
+	cut := maxBytes
+	for cut > 0 && !utf8.RuneStart(s[cut]) {
+		cut--
+	}
+	return s[:cut]
+}
+
+// applyIdempotencyDisplayName rewrites body.properties.displayName to carry
+// token's marker, creating the properties map when it is absent.
+func applyIdempotencyDisplayName(body map[string]interface{}, token string) {
+	props, ok := body["properties"].(map[string]interface{})
+	if !ok {
+		if _, present := body["properties"]; present {
+			// Not a shape any service client builds; leave it untouched
+			// rather than overwrite properties of an unknown type.
+			return
+		}
+		props = map[string]interface{}{}
+		body["properties"] = props
+	}
+	current, isString := props["displayName"].(string)
+	if !isString {
+		// Absent (or not a string, which no service client builds): the
+		// marker alone becomes the display name.
+		current = ""
+	}
+	props["displayName"] = withIdempotencyMarker(current, token)
+}

providers/azure/services/internal/reservations/idempotencymarker_test.go

@@ -0,0 +1,180 @@
+package reservations
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"strings"
+	"testing"
+	"time"
+	"unicode/utf8"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
+)
+
+// buildTaggedDisplayName runs a request body through ApplyPurchaseTags the
+// way every service client does and returns the properties.displayName that
+// would be sent to calculatePrice/purchase.
+func buildTaggedDisplayName(t *testing.T, displayName, token string) string {
+	t.Helper()
+	body := map[string]interface{}{
+		"properties": map[string]interface{}{"displayName": displayName},
+	}
+	ApplyPurchaseTags(body, common.PurchaseSourceWeb, token)
+	raw, err := json.Marshal(body)
+	require.NoError(t, err)
+	var decoded struct {
+		Properties struct {
+			DisplayName string `json:"displayName"`
+		} `json:"properties"`
+	}
+	require.NoError(t, json.Unmarshal(raw, &decoded))
+	return decoded.Properties.DisplayName
+}
+
+func vmDisplayName(now time.Time, randBytes []byte) string {
+	return BuildDisplayName(DisplayNameFields{
+		Service: "vm", Region: "eastus", ResourceType: "Standard_D2a_v4",
+		Count: 1, Term: "1yr", Payment: "all-upfront", Now: now,
+	}.WithRandSource(randBytes))
+}
+
+// TestDoIdempotentPurchaseTwoStep_UntaggedOrderMatchedByDisplayName is the
+// issue #290 regression: the reservation order model (armreservations
+// ReservationOrderResponse) defines no tags, so a list response can return the
+// order bought by a first attempt without the idempotency tag. The re-drive
+// must still find it through properties.displayName, which the purchase
+// request and the order both define, and must not buy again. The re-drive
+// rebuilds the display name with a new timestamp and random suffix, so the
+// match cannot depend on those segments.
+func TestDoIdempotentPurchaseTwoStep_UntaggedOrderMatchedByDisplayName(t *testing.T) {
+	const token = "4f1c2b0e9d8a7c6b5a4f3e2d1c0b9a8f7e6d5c4b3a2f1e0d9c8b7a6f5e4d3c2b"
+	firstAttempt := buildTaggedDisplayName(t,
+		vmDisplayName(time.Date(2026, 10, 1, 12, 0, 0, 0, time.UTC), []byte{0xaa, 0xbb, 0xcc, 0xdd}), token)
+
+	// Azure stored the display name but returned no tags at all.
+	listBody := `{"value":[{"name":"order-bought-earlier","properties":{"provisioningState":"Succeeded","displayName":"` + firstAttempt + `"}}]}`
+
+	m := &mockHTTPClient{}
+	listResp := fakeResp(http.StatusOK, listBody)
+	t.Cleanup(func() { require.NoError(t, listResp.Body.Close()) })
+	m.On("Do", mock.MatchedBy(func(r *http.Request) bool {
+		return r.Method == http.MethodGet && r.URL.String() == listURL
+	})).Return(listResp, nil).Once()
+
+	// Purchase calls stay mocked so a regression fails on the assertions
+	// below rather than on an unexpected mock call.
+	calcResp := fakeResp(http.StatusOK, `{"properties":{"reservationOrderId":"order-duplicate"}}`)
+	t.Cleanup(func() { require.NoError(t, calcResp.Body.Close()) })
+	m.On("Do", mock.MatchedBy(func(r *http.Request) bool {
+		return r.Method == http.MethodPost && r.URL.String() == calcURL
+	})).Return(calcResp, nil).Maybe()
+	purchaseResp := fakeResp(http.StatusOK, `{}`)
+	t.Cleanup(func() { require.NoError(t, purchaseResp.Body.Close()) })
+	m.On("Do", mock.MatchedBy(func(r *http.Request) bool {
+		return r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/purchase")
+	})).Return(purchaseResp, nil).Maybe()
+
+	redriveBody, err := json.Marshal(map[string]interface{}{
+		"properties": map[string]interface{}{
+			"displayName": buildTaggedDisplayName(t,
+				vmDisplayName(time.Date(2026, 10, 2, 8, 30, 0, 0, time.UTC), []byte{0x11, 0x22, 0x33, 0x44}), token),
+		},
+	})
+	require.NoError(t, err)
+
+	orderID, existing, err := DoIdempotentPurchaseTwoStep(context.Background(), m, calcURL, redriveBody, "bearer", token)
+	require.NoError(t, err)
+	assert.True(t, existing, "the order bought by the first attempt must be adopted")
+	assert.Equal(t, "order-bought-earlier", orderID)
+	m.AssertNotCalled(t, "Do", mock.MatchedBy(func(r *http.Request) bool {
+		return r.Method == http.MethodPost
+	}))
+}
+
+// TestFindReservationOrderByIdempotencyToken_DisplayNameOfOtherTokenDoesNotMatch
+// guards the opposite direction: an untagged order carrying a different
+// token's marker must not be adopted.
+func TestFindReservationOrderByIdempotencyToken_DisplayNameOfOtherTokenDoesNotMatch(t *testing.T) {
+	other := buildTaggedDisplayName(t, vmDisplayName(time.Now(), nil), "other-token")
+	listBody := `{"value":[{"name":"order-other","properties":{"provisioningState":"Succeeded","displayName":"` + other + `"}}]}`
+
+	m := &mockHTTPClient{}
+	listResp := fakeResp(http.StatusOK, listBody)
+	t.Cleanup(func() { require.NoError(t, listResp.Body.Close()) })
+	m.On("Do", mock.Anything).Return(listResp, nil).Once()
+
+	orderID, found, err := FindReservationOrderByIdempotencyToken(context.Background(), m, "bearer", "wanted-token")
+	require.NoError(t, err)
+	assert.False(t, found)
+	assert.Empty(t, orderID)
+}
+
+// TestApplyPurchaseTags_DisplayNameMarker pins the shape of the display name
+// written when a token is present: within Azure's 64-char allowlist, keeps
+// the leading identifying segments, and ends with the same marker for the
+// same token however the rest of the name was built.
+func TestApplyPurchaseTags_DisplayNameMarker(t *testing.T) {
+	const token = "tok-290"
+	marker := idempotencyDisplayNameMarker(token)
+
+	a := buildTaggedDisplayName(t, vmDisplayName(time.Now(), []byte{1, 2, 3, 4}), token)
+	b := buildTaggedDisplayName(t, vmDisplayName(time.Now().Add(time.Hour), []byte{5, 6, 7, 8}), token)
+	for _, got := range []string{a, b} {
+		assert.Regexp(t, `^[A-Za-z0-9_-]{1,64}$`, got)
+		assert.True(t, strings.HasPrefix(got, "vm-eastus-Standard_D2a_v4-1x-1yr"), got)
+		assert.True(t, strings.HasSuffix(got, "-"+marker), got)
+		assert.True(t, displayNameCarriesToken(got, token))
+	}
+	assert.NotEqual(t, marker, idempotencyDisplayNameMarker("tok-other"))
+
+	// A name that is already at the cap with no separators is truncated.
+	long := buildTaggedDisplayName(t, strings.Repeat("x", 64), token)
+	assert.Len(t, long, azureDisplayNameMaxLen)
+	assert.True(t, displayNameCarriesToken(long, token))
+
+	// A short name with spaces (synapse/managedredis style) is kept intact.
+	short := buildTaggedDisplayName(t, "Synapse SQL Pool Reservation - DW100c", token)
+	assert.Equal(t, "Synapse SQL Pool Reservation - DW100c-"+marker, short)
+
+	// Missing displayName: the marker alone is written.
+	assert.Equal(t, marker, buildTaggedDisplayName(t, "", token))
+}
+
+// TestApplyPurchaseTags_NoTokenLeavesDisplayNameAlone keeps the untokened
+// paths (CLI legacy, source-only) byte-identical to before issue #290.
+func TestApplyPurchaseTags_NoTokenLeavesDisplayNameAlone(t *testing.T) {
+	name := vmDisplayName(time.Now(), []byte{1, 2, 3, 4})
+	assert.Equal(t, name, buildTaggedDisplayName(t, name, ""))
+}
+
+// TestWithIdempotencyMarker_TruncatesAtRuneBoundary covers a long display
+// name with no '-' whose hard truncation point falls inside a multibyte
+// rune: the cut must back off to the rune start so the name stays valid
+// UTF-8 (JSON encoding would otherwise widen it past the byte limit).
+func TestWithIdempotencyMarker_TruncatesAtRuneBoundary(t *testing.T) {
+	const token = "tok-290"
+	marker := idempotencyDisplayNameMarker(token)
+	room := azureDisplayNameMaxLen - len(marker) - 1
+
+	// 'é' is two bytes; placing it at room-1 puts the cut between its bytes.
+	name := strings.Repeat("x", room-1) + "é" + strings.Repeat("y", 10)
+	got := withIdempotencyMarker(name, token)
+
+	assert.True(t, utf8.ValidString(got), "result must be valid UTF-8: %q", got)
+	assert.LessOrEqual(t, len(got), azureDisplayNameMaxLen)
+	assert.Equal(t, strings.Repeat("x", room-1)+"-"+marker, got)
+	assert.True(t, displayNameCarriesToken(got, token))
+}
+
+func TestTruncateToRuneBoundary(t *testing.T) {
+	assert.Equal(t, "abc", truncateToRuneBoundary("abc", 5))
+	assert.Equal(t, "ab", truncateToRuneBoundary("abc", 2))
+	assert.Equal(t, "a", truncateToRuneBoundary("aé", 2))
+	assert.Equal(t, "aé", truncateToRuneBoundary("aéb", 3))
+	assert.Equal(t, "", truncateToRuneBoundary("€", 2))
+}

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

@@ -25,6 +25,10 @@
 //   - cudly-idempotency-token=<deterministic hex> -- the (execution, rec)
 //     identity from common.DeriveIdempotencyToken.
 //
+// The API does not define tags on reservation orders (issue #290), so the
+// token is also carried as a marker in properties.displayName, and the lookup
+// below matches on either.
+//
 // Before invoking the two-step flow, DoIdempotentPurchaseTwoStep lists existing
 // reservation orders and short-circuits when one already carries the same
 // idempotency token, so a re-driven purchase of a stranded execution (issue
@@ -190,14 +194,20 @@ func ReservationOrdersListURL() string {
 // value is empty. When both are empty, no tags map is added (preserving the
 // untagged shape some legacy CLI test paths expect).
 //
-// The tags are sent verbatim in the calculatePrice + purchase request bodies
-// and are persisted by Azure onto the resulting reservation order so the
-// idempotency tag can be read back by FindReservationOrderByIdempotencyToken
-// on a re-drive.
+// The tags are sent verbatim in the calculatePrice + purchase request bodies,
+// but the Reservations API does not define tags on a purchase request or a
+// reservation order (issue #290), so Azure may drop them. When a token is
+// present the idempotency identity is therefore also written into
+// properties.displayName, a field both models define, as a deterministic
+// marker that FindReservationOrderByIdempotencyToken matches on a re-drive
+// (see idempotencymarker.go).
 func ApplyPurchaseTags(body map[string]interface{}, source, idempotencyToken string) {
 	if source == "" && idempotencyToken == "" {
 		return
 	}
+	if idempotencyToken != "" {
+		applyIdempotencyDisplayName(body, idempotencyToken)
+	}
 	tags := map[string]string{}
 	if source != "" {
 		tags[common.PurchaseTagKey] = source
@@ -440,7 +450,9 @@ type reservationOrdersListResponse struct {
 		// reservationOrderId during calculatePrice and what DoPurchaseTwoStep
 		// returns on success.
 		Name string `json:"name"`
-		// Tags carries any tags stamped on the order at purchase time.
+		// Tags carries any tags stamped on the order at purchase time. The
+		// API model defines none (issue #290); kept so an order Azure did
+		// return tagged still matches.
 		Tags map[string]string `json:"tags"`
 		// Properties.ProvisioningState lets us skip terminal-failed orders so a
 		// canceled/failed reservation with the same idempotency tag does not
@@ -450,6 +462,10 @@ type reservationOrdersListResponse struct {
 		// not just succeeded ones).
 		Properties struct {
 			ProvisioningState string `json:"provisioningState"`
+			// DisplayName carries the idempotency marker written by
+			// ApplyPurchaseTags (armreservations.ReservationOrderProperties
+			// defines it, unlike tags).
+			DisplayName string `json:"displayName"`
 		} `json:"properties"`
 	} `json:"value"`
 	NextLink string `json:"nextLink"`
@@ -590,13 +606,15 @@ func fetchReservationOrdersPage(ctx context.Context, httpClient HTTPClient, page
 	return &page, nil
 }
 
-// matchReservationOrderInPage scans one decoded page for an order tagged with
-// the idempotency token and applies reservationOrderDispositions. Extracted
-// from FindReservationOrderByIdempotencyToken to keep the function under the
+// matchReservationOrderInPage scans one decoded page for an order carrying
+// the idempotency token, in its tag or its display-name marker (issue #290),
+// and applies reservationOrderDispositions. Extracted from
+// FindReservationOrderByIdempotencyToken to keep the function under the
 // gocyclo:10 threshold enforced by the pre-commit hook.
 func matchReservationOrderInPage(page *reservationOrdersListResponse, idempotencyToken string) (orderID string, found bool, err error) {
 	for _, order := range page.Value {
-		if order.Tags[common.IdempotencyTagKey] != idempotencyToken {
+		if order.Tags[common.IdempotencyTagKey] != idempotencyToken &&
+			!displayNameCarriesToken(order.Properties.DisplayName, idempotencyToken) {
 			continue
 		}
 		state := armreservations.ProvisioningState(order.Properties.ProvisioningState)