fix(purchase): make zonal EC2 AZ part of the guard, identity key, idempotency key and rec ID

internal/api/handler_purchases.go

@@ -2770,7 +2770,15 @@ func matchDuplicateInList(pending []config.PurchaseExecution, creatorID, idempot
 		if ex.CreatedByUserID != nil {
 			exCreator = *ex.CreatedByUserID
 		}
-		if exCreator == creatorID && purchaseIdempotencyKey(exCreator, ex.Recommendations, ex.CapacityPercent) == idempotencyKey {
+		if exCreator != creatorID {
+			continue
+		}
+		exKey, err := purchaseIdempotencyKey(exCreator, ex.Recommendations, ex.CapacityPercent)
+		if err != nil {
+			logging.Errorf("purchase idempotency: skipping pending execution %s, cannot derive its key: %v", ex.ExecutionID, err)
+			continue
+		}
+		if exKey == idempotencyKey {
 			return ex
 		}
 	}
@@ -2836,23 +2844,46 @@ const purchaseIdempotencyWindow = 2 * time.Minute
 // ordering can't change the hash. Account scope is implicit: each rec carries
 // its CloudAccountID, so the same recs targeting a different account hash
 // differently. Closes issue #644.
-func purchaseIdempotencyKey(creatorID string, recs []config.RecommendationRecord, capacityPercent int) string {
+func purchaseIdempotencyKey(creatorID string, recs []config.RecommendationRecord, capacityPercent int) (string, error) {
 	tuples := make([]string, 0, len(recs))
 	for _rvc := range recs {
 		r := recs[_rvc]
 		acct := ""
 		if r.CloudAccountID != nil {
 			acct = *r.CloudAccountID
 		}
-		tuples = append(tuples, fmt.Sprintf("%s|%s|%s|%s|%s|%s|%d|%d|%s",
+		az, err := recDetailsAvailabilityZone(&r)
+		if err != nil {
+			return "", err
+		}
+		tuples = append(tuples, fmt.Sprintf("%s|%s|%s|%s|%s|%s|%d|%d|%s|%s",
 			strings.ToLower(strings.TrimSpace(r.Provider)),
 			r.Service, r.Region, r.ResourceType, r.Engine, acct,
-			r.Count, r.Term, strings.ToLower(strings.TrimSpace(r.Payment))))
+			r.Count, r.Term, strings.ToLower(strings.TrimSpace(r.Payment)), az))
 	}
 	sort.Strings(tuples)
 	h := sha256.New()
 	fmt.Fprintf(h, "%s\x1f%d\x1f%s", creatorID, capacityPercent, strings.Join(tuples, "\x1e"))
-	return hex.EncodeToString(h.Sum(nil))
+	return hex.EncodeToString(h.Sum(nil)), nil
+}
+
+// recDetailsAvailabilityZone returns the zonal EC2 AZ carried in the rec's
+// Details blob, "" for non-compute recs and for absent or legacy Details.
+// The AZ is read from Details, not RecommendationRecord.AvailabilityZone,
+// because pending executions written before issue #792 carry it only there
+// and the idempotency key is recomputed from them (issue #792).
+func recDetailsAvailabilityZone(r *config.RecommendationRecord) (string, error) {
+	if r.Service != string(common.ServiceEC2) && r.Service != string(common.ServiceCompute) {
+		return "", nil
+	}
+	details, err := common.DecodeServiceDetailsFor(r.Service, r.Details)
+	if err != nil {
+		return "", fmt.Errorf("decode %s details of %s/%s: %w", r.Service, r.Region, r.ResourceType, err)
+	}
+	if cd, ok := details.(*common.ComputeDetails); ok && cd != nil {
+		return strings.TrimSpace(cd.AvailabilityZone), nil
+	}
+	return "", nil
 }
 
 // findDuplicatePendingExecution returns an existing pending/notified execution
@@ -2884,7 +2915,12 @@ func (h *Handler) findDuplicatePendingExecution(ctx context.Context, creatorID,
 		if exCreator != creatorID {
 			continue
 		}
-		if purchaseIdempotencyKey(exCreator, ex.Recommendations, ex.CapacityPercent) == key {
+		exKey, err := purchaseIdempotencyKey(exCreator, ex.Recommendations, ex.CapacityPercent)
+		if err != nil {
+			logging.Errorf("purchase idempotency: skipping pending execution %s, cannot derive its key: %v", ex.ExecutionID, err)
+			continue
+		}
+		if exKey == key {
 			return ex, nil
 		}
 	}
@@ -2962,7 +2998,10 @@ func (h *Handler) executePurchase(ctx context.Context, req *events.LambdaFunctio
 	// which closes the TOCTOU race that a pre-tx read alone cannot prevent.
 	creator := resolveCreatorUserID(session)
 	creatorID := derefStringOrEmpty(creator)
-	idempotencyKey := purchaseIdempotencyKey(creatorID, execReq.Recommendations, execReq.CapacityPercent)
+	idempotencyKey, err := purchaseIdempotencyKey(creatorID, execReq.Recommendations, execReq.CapacityPercent)
+	if err != nil {
+		return nil, fmt.Errorf("derive purchase idempotency key: %w", err)
+	}
 
 	execution, rawApprovalToken, err := newPendingExecution(&execReq, totalUpfront, totalSavings)
 	if err != nil {

internal/api/handler_purchases_guards_test.go

@@ -464,35 +464,35 @@ func TestPurchaseIdempotencyKey_StableAndDiscriminating(t *testing.T) {
 	reordered := []config.RecommendationRecord{r2, validRec()}
 
 	assert.Equal(t,
-		purchaseIdempotencyKey("user-1", recsA, 100),
-		purchaseIdempotencyKey("user-1", recsA, 100),
+		mustPurchaseIdempotencyKey("user-1", recsA, 100),
+		mustPurchaseIdempotencyKey("user-1", recsA, 100),
 		"identical input must hash identically")
 	assert.Equal(t,
-		purchaseIdempotencyKey("user-1", ordered, 100),
-		purchaseIdempotencyKey("user-1", reordered, 100),
+		mustPurchaseIdempotencyKey("user-1", ordered, 100),
+		mustPurchaseIdempotencyKey("user-1", reordered, 100),
 		"slice order must not change the key")
 
 	// Discriminating dimensions.
-	assert.NotEqual(t, purchaseIdempotencyKey("user-1", recsA, 100), purchaseIdempotencyKey("user-2", recsA, 100), "creator")
-	assert.NotEqual(t, purchaseIdempotencyKey("user-1", recsA, 100), purchaseIdempotencyKey("user-1", recsA, 50), "capacity")
+	assert.NotEqual(t, mustPurchaseIdempotencyKey("user-1", recsA, 100), mustPurchaseIdempotencyKey("user-2", recsA, 100), "creator")
+	assert.NotEqual(t, mustPurchaseIdempotencyKey("user-1", recsA, 100), mustPurchaseIdempotencyKey("user-1", recsA, 50), "capacity")
 
 	scaled := []config.RecommendationRecord{validRec()}
 	scaled[0].Count = 1
-	assert.NotEqual(t, purchaseIdempotencyKey("user-1", recsA, 100), purchaseIdempotencyKey("user-1", scaled, 100), "count")
+	assert.NotEqual(t, mustPurchaseIdempotencyKey("user-1", recsA, 100), mustPurchaseIdempotencyKey("user-1", scaled, 100), "count")
 
 	acctA := []config.RecommendationRecord{validRec()}
 	acctA[0].CloudAccountID = strptr("acct-A")
 	acctB := []config.RecommendationRecord{validRec()}
 	acctB[0].CloudAccountID = strptr("acct-B")
-	assert.NotEqual(t, purchaseIdempotencyKey("user-1", acctA, 100), purchaseIdempotencyKey("user-1", acctB, 100), "account")
+	assert.NotEqual(t, mustPurchaseIdempotencyKey("user-1", acctA, 100), mustPurchaseIdempotencyKey("user-1", acctB, 100), "account")
 }
 
 func TestFindDuplicatePendingExecution(t *testing.T) {
 	ctx := context.Background()
 	now := time.Now()
 	creator := "11111111-1111-1111-1111-111111111111"
 	recs := []config.RecommendationRecord{validRec()}
-	key := purchaseIdempotencyKey(creator, recs, 100)
+	key := mustPurchaseIdempotencyKey(creator, recs, 100)
 
 	makeExec := func(id string, age time.Duration, src string, c *string, capacity int) config.PurchaseExecution {
 		// Copy recs so a subtest that mutates exec.Recommendations does not
@@ -646,3 +646,11 @@ func TestValidateCapacityConsistency(t *testing.T) {
 		})
 	}
 }
+
+func mustPurchaseIdempotencyKey(creatorID string, recs []config.RecommendationRecord, capacityPercent int) string {
+	key, err := purchaseIdempotencyKey(creatorID, recs, capacityPercent)
+	if err != nil {
+		panic(err)
+	}
+	return key
+}

internal/api/handler_purchases_submit_dedupe_integration_test.go

@@ -49,7 +49,7 @@ func submitFixture(t *testing.T) ([]config.RecommendationRecord, string) {
 		Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large",
 		Count: 2, Term: 1, Payment: "no-upfront",
 	}}
-	return recs, purchaseIdempotencyKey("", recs, 100)
+	return recs, mustPurchaseIdempotencyKey("", recs, 100)
 }
 
 func newSubmitExecution(t *testing.T, recs []config.RecommendationRecord) *config.PurchaseExecution {
@@ -171,7 +171,7 @@ func TestPersistExecution_DedupeIsPerCreator(t *testing.T) {
 	submit := func(creator string) *config.PurchaseExecution {
 		exec := newSubmitExecution(t, recs)
 		exec.CreatedByUserID = &creator
-		dup, err := h.persistExecutionAndSuppressions(ctx, exec, nil, creator, purchaseIdempotencyKey(creator, recs, 100))
+		dup, err := h.persistExecutionAndSuppressions(ctx, exec, nil, creator, mustPurchaseIdempotencyKey(creator, recs, 100))
 		require.NoError(t, err)
 		return dup
 	}

internal/api/purchase_az_test.go

@@ -0,0 +1,174 @@
+package api
+
+import (
+	"context"
+	"encoding/json"
+	"testing"
+	"time"
+
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Real zonal EC2 Details shape as persisted by the scheduler (issue #792).
+func zonalEC2Details(az string) json.RawMessage {
+	return json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"default","scope":"Availability Zone","availability_zone":"` + az + `"}`)
+}
+
+func zonalEC2Rec(az string, upfront float64) config.RecommendationRecord {
+	return config.RecommendationRecord{
+		ID: "rec-" + az, Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large",
+		Term: 1, Payment: "all-upfront", Count: 1, UpfrontCost: upfront, Savings: 10,
+		AvailabilityZone: az, Details: zonalEC2Details(az),
+	}
+}
+
+func TestPurchaseIdempotencyKey_DiffersByAvailabilityZone(t *testing.T) {
+	t.Parallel()
+	a := []config.RecommendationRecord{zonalEC2Rec("us-east-1a", 100)}
+	b := []config.RecommendationRecord{zonalEC2Rec("us-east-1b", 100)}
+	assert.NotEqual(t, mustPurchaseIdempotencyKey("u", a, 100), mustPurchaseIdempotencyKey("u", b, 100),
+		"two submits differing only by AZ must not dedupe into one")
+	assert.Equal(t, mustPurchaseIdempotencyKey("u", a, 100), mustPurchaseIdempotencyKey("u", a, 100))
+}
+
+func TestPurchaseIdempotencyKey_LegacyRecWithoutTopLevelAZMatchesNewShape(t *testing.T) {
+	t.Parallel()
+	newShape := zonalEC2Rec("us-east-1a", 100)
+	legacy := newShape
+	legacy.AvailabilityZone = "" // pending row written before the field existed: AZ only in Details
+	assert.Equal(t,
+		mustPurchaseIdempotencyKey("u", []config.RecommendationRecord{newShape}, 100),
+		mustPurchaseIdempotencyKey("u", []config.RecommendationRecord{legacy}, 100),
+		"a double-submit straddling the deploy must still dedupe")
+}
+
+func TestPurchaseIdempotencyKey_MalformedComputeDetailsErrorsAndCandidateIsSkipped(t *testing.T) {
+	t.Parallel()
+	bad := zonalEC2Rec("us-east-1a", 100)
+	bad.Details = json.RawMessage(`{"availability_zone":`)
+	_, err := purchaseIdempotencyKey("u", []config.RecommendationRecord{bad}, 100)
+	require.Error(t, err)
+
+	now := time.Now()
+	good := []config.RecommendationRecord{zonalEC2Rec("us-east-1a", 100)}
+	key := mustPurchaseIdempotencyKey("u", good, 100)
+	creator := "u"
+	pending := []config.PurchaseExecution{
+		{ExecutionID: "bad", Source: common.PurchaseSourceWeb, ScheduledDate: now, CreatedByUserID: &creator, Recommendations: []config.RecommendationRecord{bad}, CapacityPercent: 100},
+		{ExecutionID: "good", Source: common.PurchaseSourceWeb, ScheduledDate: now, CreatedByUserID: &creator, Recommendations: good, CapacityPercent: 100},
+	}
+	got := matchDuplicateInList(pending, creator, key, now)
+	require.NotNil(t, got)
+	assert.Equal(t, "good", got.ExecutionID)
+}
+
+func TestPurchaseIdempotencyKey_NoDetailsIsEmptyAZ(t *testing.T) {
+	t.Parallel()
+	r := config.RecommendationRecord{Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large", Count: 1, Term: 1, Payment: "all-upfront"}
+	_, err := purchaseIdempotencyKey("u", []config.RecommendationRecord{r}, 100)
+	require.NoError(t, err)
+}
+
+func TestCheckPurchaseDetailIdentity_AvailabilityZoneMismatchRefused(t *testing.T) {
+	for name, matchAZ := range map[string]string{"different az": "us-east-1b", "unknown az": ""} {
+		t.Run(name, func(t *testing.T) {
+			origin := zonalEC2Rec("us-east-1a", 100)
+			match := zonalEC2Rec("us-east-1b", 100)
+			match.ID = "other"
+			if matchAZ == "" {
+				match.Details = json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"default","scope":"Availability Zone"}`)
+			}
+			req := config.RecommendationRecord{ID: origin.ID, Service: "ec2", Term: 1, Count: 1}
+			err := checkPurchaseDetailIdentity(&origin, &match, &req, 0)
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "availability_zone")
+			ce, isClient := IsClientError(err)
+			require.True(t, isClient)
+			assert.Equal(t, 409, ce.code)
+		})
+	}
+}
+
+func TestCheckPurchaseDetailIdentity_LegacyOriginWithoutAZAllowed(t *testing.T) {
+	origin := zonalEC2Rec("us-east-1a", 100)
+	origin.Details = json.RawMessage(`{"instance_type":"m5.large","platform":"Linux/UNIX","tenancy":"default","scope":"Availability Zone"}`)
+	match := zonalEC2Rec("us-east-1b", 100)
+	match.ID = "other"
+	req := config.RecommendationRecord{ID: origin.ID, Service: "ec2", Term: 1, Count: 1}
+	require.NoError(t, checkPurchaseDetailIdentity(&origin, &match, &req, 0))
+}
+
+func TestRecIdentityKey_DiffersByAvailabilityZone(t *testing.T) {
+	a, b := zonalEC2Rec("us-east-1a", 1), zonalEC2Rec("us-east-1b", 1)
+	assert.NotEqual(t, recIdentityKey(&a), recIdentityKey(&b))
+}
+
+// Two zonal rows that differ only by AZ must both load (no "more than one row"
+// refusal) and each request must be priced from its own AZ's row.
+func TestPriceRecommendationsFromStore_PricesFromRequestedAZ(t *testing.T) {
+	ctx := context.Background()
+	store := new(MockConfigStore)
+	t.Cleanup(func() { store.AssertExpectations(t) })
+	expectStoredRecs(store, zonalEC2Rec("us-east-1a", 100), zonalEC2Rec("us-east-1b", 900))
+	h := &Handler{config: store}
+
+	req := []config.RecommendationRecord{{
+		Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large",
+		Term: 1, Payment: "all-upfront", Count: 1, AvailabilityZone: "us-east-1b",
+	}}
+	require.NoError(t, h.priceRecommendationsFromStore(ctx, req))
+	assert.Equal(t, 900.0, req[0].UpfrontCost)
+	assert.Equal(t, "us-east-1b", req[0].AvailabilityZone)
+}
+
+func TestPriceRecommendationsFromStore_ZonalRequestWithoutAZRefused(t *testing.T) {
+	ctx := context.Background()
+	store := new(MockConfigStore)
+	t.Cleanup(func() { store.AssertExpectations(t) })
+	expectStoredRecs(store, zonalEC2Rec("us-east-1a", 100))
+	h := &Handler{config: store}
+
+	req := []config.RecommendationRecord{{
+		Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large",
+		Term: 1, Payment: "all-upfront", Count: 1,
+	}}
+	err := h.priceRecommendationsFromStore(ctx, req)
+	require.Error(t, err)
+	ce, isClient := IsClientError(err)
+	require.True(t, isClient)
+	assert.Equal(t, 409, ce.code)
+}
+
+// No-ID path: the request's stated Details AZ must agree with the matched row,
+// whether the top-level AZ is empty (no match) or names another AZ.
+func TestPriceRecommendationsFromStore_NoIDDetailsAZMismatchRefused(t *testing.T) {
+	for name, topLevel := range map[string]string{"empty top-level": "", "other top-level": "us-east-1b"} {
+		t.Run(name, func(t *testing.T) {
+			ctx := context.Background()
+			store := new(MockConfigStore)
+			t.Cleanup(func() { store.AssertExpectations(t) })
+			expectStoredRecs(store, zonalEC2Rec("us-east-1a", 100), zonalEC2Rec("us-east-1b", 900))
+			h := &Handler{config: store}
+			req := []config.RecommendationRecord{{
+				Provider: "aws", Service: "ec2", Region: "us-east-1", ResourceType: "m5.large",
+				Term: 1, Payment: "all-upfront", Count: 1,
+				AvailabilityZone: topLevel, Details: zonalEC2Details("us-east-1a"),
+			}}
+			err := h.priceRecommendationsFromStore(ctx, req)
+			require.Error(t, err)
+			ce, isClient := IsClientError(err)
+			require.True(t, isClient)
+			assert.Equal(t, 409, ce.code)
+		})
+	}
+}
+
+func TestDescribeRec_IncludesAvailabilityZone(t *testing.T) {
+	r := zonalEC2Rec("us-east-1a", 1)
+	assert.Contains(t, describeRec(&r), "az=us-east-1a")
+	r.AvailabilityZone = ""
+	assert.NotContains(t, describeRec(&r), "az=")
+}

internal/api/purchase_pricing.go

@@ -116,8 +116,9 @@ func (h *Handler) loadStoredRecommendationIndex(ctx context.Context, recs []conf
 	return byKey, byID, nil
 }
 
-// recIdentityKey is the tuple a price is a function of: the same eight
-// components the scheduler encodes into RecommendationRecord.ID, with a nil
+// recIdentityKey is the tuple a price is a function of: the same nine
+// components (the ninth is the zonal EC2 availability zone, "" for every
+// other rec; issue #792) behind RecommendationRecord.ID, with a nil
 // and an empty CloudAccountID collapsed together (as purchaseConstraintSets
 // and the store's account_key both do). Provider and payment are lowercased
 // because validatePurchaseRecommendation canonicalises the request side.
@@ -129,6 +130,7 @@ func recIdentityKey(rec *config.RecommendationRecord) string {
 	return strings.Join([]string{
 		strings.ToLower(rec.Provider), account, rec.Service, rec.Region, rec.ResourceType,
 		rec.Engine, strconv.Itoa(rec.Term), strings.ToLower(rec.Payment),
+		rec.AvailabilityZone,
 	}, "\x1f")
 }
 
@@ -322,6 +324,9 @@ func computeDetailMismatch(o, m *common.ComputeDetails) string {
 		mismatchField("tenancy", o.Tenancy, m.Tenancy),
 		mismatchField("platform", o.Platform, m.Platform),
 		mismatchField("scope", o.Scope, m.Scope),
+		// Zonal EC2 reservations are bought in one AZ; a match in another
+		// AZ (or with an unknown one) is a different commitment (issue #792).
+		mismatchField("availability_zone", o.AvailabilityZone, m.AvailabilityZone),
 		// GCP custom machine types (Compute Engine CUDs) read MemoryGB at
 		// purchase time to build the machine spec, so a memory mismatch is
 		// as purchase-critical as tenancy for that provider.
@@ -361,6 +366,10 @@ func formatOptionalCost(v *float64) string {
 }
 
 func describeRec(rec *config.RecommendationRecord) string {
-	return fmt.Sprintf("%s/%s %s %s engine=%q term=%d payment=%s account=%s",
+	desc := fmt.Sprintf("%s/%s %s %s engine=%q term=%d payment=%s account=%s",
 		rec.Provider, rec.Service, rec.Region, rec.ResourceType, rec.Engine, rec.Term, rec.Payment, derefStringOrEmpty(rec.CloudAccountID))
+	if rec.AvailabilityZone != "" {
+		desc += " az=" + rec.AvailabilityZone
+	}
+	return desc
 }