From fcbc85e699cc4723f8ffa859facdb44be7cce2cb Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Thu, 15 Oct 2026 22:40:54 +0100 Subject: [PATCH 1/2] fix(azure): match purchase dedupe on order display name, not only tags The Reservations API defines no tags on a purchase request or a reservation order (armreservations v1.1.0 PurchaseRequest and ReservationOrderResponse have no Tags field), so the cudly-idempotency-token tag the re-drive lookup keyed on may never be stored or listed back, and a retry could buy the same 1y/3y reservation again. ApplyPurchaseTags now also writes a deterministic marker derived from the idempotency token (cudly + 16 hex of SHA-256(token)) at the end of properties.displayName, which both models define, dropping trailing timestamp/random segments to stay within the 64-char limit. FindReservationOrderByIdempotencyToken matches an order whose tag or display-name marker carries the token. The tag is still written and matched for orders Azure did return tagged. Closes #290 Co-Authored-By: Claude Opus 5.5 --- .../reservations/idempotencymarker.go | 86 ++++++++++ .../reservations/idempotencymarker_test.go | 152 ++++++++++++++++++ .../internal/reservations/purchase.go | 36 +++-- 3 files changed, 265 insertions(+), 9 deletions(-) create mode 100644 providers/azure/services/internal/reservations/idempotencymarker.go create mode 100644 providers/azure/services/internal/reservations/idempotencymarker_test.go diff --git a/providers/azure/services/internal/reservations/idempotencymarker.go b/providers/azure/services/internal/reservations/idempotencymarker.go new file mode 100644 index 00000000..ff8708e2 --- /dev/null +++ b/providers/azure/services/internal/reservations/idempotencymarker.go @@ -0,0 +1,86 @@ +package reservations + +import ( + "crypto/sha256" + "encoding/hex" + "strings" +) + +// 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 = name[:room] + break + } + name = strings.TrimRight(name[:cut], "-") + } + if name == "" { + return marker + } + return name + "-" + marker +} + +// 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) +} diff --git a/providers/azure/services/internal/reservations/idempotencymarker_test.go b/providers/azure/services/internal/reservations/idempotencymarker_test.go new file mode 100644 index 00000000..6995060c --- /dev/null +++ b/providers/azure/services/internal/reservations/idempotencymarker_test.go @@ -0,0 +1,152 @@ +package reservations + +import ( + "context" + "encoding/json" + "net/http" + "strings" + "testing" + "time" + + "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, "")) +} diff --git a/providers/azure/services/internal/reservations/purchase.go b/providers/azure/services/internal/reservations/purchase.go index 39e071cf..3178e725 100644 --- a/providers/azure/services/internal/reservations/purchase.go +++ b/providers/azure/services/internal/reservations/purchase.go @@ -25,6 +25,10 @@ // - cudly-idempotency-token= -- 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) From 35c1c231d73a530c0d5b951906a681b6a835d54d Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 11:18:01 +0100 Subject: [PATCH 2/2] fix(azure): truncate idempotency display name at a UTF-8 rune boundary A long display name with no '-' was hard-cut with name[:room], which can split a multibyte rune and yield invalid UTF-8 (JSON-escaped past the 64-byte limit). Back the cut off to the rune start. Addresses CodeRabbit review on #422. Co-Authored-By: Claude Opus 5.5 --- .../reservations/idempotencymarker.go | 16 ++++++++++- .../reservations/idempotencymarker_test.go | 28 +++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/providers/azure/services/internal/reservations/idempotencymarker.go b/providers/azure/services/internal/reservations/idempotencymarker.go index ff8708e2..8884a4b8 100644 --- a/providers/azure/services/internal/reservations/idempotencymarker.go +++ b/providers/azure/services/internal/reservations/idempotencymarker.go @@ -4,6 +4,7 @@ import ( "crypto/sha256" "encoding/hex" "strings" + "unicode/utf8" ) // The Reservations API does not define tags on a reservation order: neither @@ -52,7 +53,7 @@ func withIdempotencyMarker(displayName, token string) string { for len(name) > room { cut := strings.LastIndexByte(name, '-') if cut <= 0 { - name = name[:room] + name = truncateToRuneBoundary(name, room) break } name = strings.TrimRight(name[:cut], "-") @@ -63,6 +64,19 @@ func withIdempotencyMarker(displayName, token string) string { 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) { diff --git a/providers/azure/services/internal/reservations/idempotencymarker_test.go b/providers/azure/services/internal/reservations/idempotencymarker_test.go index 6995060c..f4f93bd1 100644 --- a/providers/azure/services/internal/reservations/idempotencymarker_test.go +++ b/providers/azure/services/internal/reservations/idempotencymarker_test.go @@ -7,6 +7,7 @@ import ( "strings" "testing" "time" + "unicode/utf8" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -150,3 +151,30 @@ 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)) +}