From 08fe7baf9f927ea87dae2870774a6332a4fde0ad Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 23:56:45 +0200 Subject: [PATCH 1/4] chore(deps): bump cloud-commitments-go pkg and aws for PurchaseOptions.RequestedAt Redshift purchases with an idempotency token now require RequestedAt to bound the DescribeReservedNodes dedupe window. Only pkg and providers/aws move; azure and gcp keep their pin. Must ship with the execution.go change that sets RequestedAt, or tokenized Redshift buys refuse. Part of #702 Co-Authored-By: Claude Opus 5.5 --- go.mod | 4 ++-- go.sum | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 56d536992..d5c9e57a7 100644 --- a/go.mod +++ b/go.mod @@ -84,8 +84,8 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations v1.1.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys v1.4.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets v1.4.0 - github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261017134002-3def3f898617 - github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261017134002-3def3f898617 + github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261018003623-541e05e5fd29 + github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261019192600-aeb8a6f7ff51 github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261017134002-3def3f898617 github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261017134002-3def3f898617 github.com/aws/aws-lambda-go v1.47.0 diff --git a/go.sum b/go.sum index 11c1b111b..3a32f2465 100644 --- a/go.sum +++ b/go.sum @@ -92,10 +92,10 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0 github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.54.0/go.mod h1:vB2GH9GAYYJTO3mEn8oYwzEdhlayZIdQz6zdzgUIRvA= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0 h1:s0WlVbf9qpvkh1c/uDAPElam0WrL7fHRIidgZJ7UqZI= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0/go.mod h1:Mf6O40IAyB9zR/1J8nGDDPirZQQPbYJni8Yisy7NTMc= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261017134002-3def3f898617 h1:uZXx7Zziae2LrhOHvHRQlMOr35hgNnUl8jNGaPG7I4k= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261017134002-3def3f898617/go.mod h1:SCDEgHG6evjPh0RsoXof+J3aMmGXNFMrwbJIle7rMpQ= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261017134002-3def3f898617 h1:nfrGAD+48vStyTHliIZyBp1l2LtVv47MedgNgtkcAnc= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261017134002-3def3f898617/go.mod h1:dc2TUUYOwkEaX90zQX1nvVQU85y9GVZrAt4YkuLxZ9A= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261018003623-541e05e5fd29 h1:QJkoZBHkwjvMGplDb1TcuBy3HdYPxlZ+AyR/7QEU8Hc= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261018003623-541e05e5fd29/go.mod h1:SCDEgHG6evjPh0RsoXof+J3aMmGXNFMrwbJIle7rMpQ= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261019192600-aeb8a6f7ff51 h1:fr8mS6aF7/y69cJ5UlF9MEvP15mSlH/4yKg25Cvk/YM= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261019192600-aeb8a6f7ff51/go.mod h1:p6TrYRa4+lvcdsPCAJ+sa0tI0sRfGQ/0YT1Af3fZk3w= github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261017134002-3def3f898617 h1:K9wBX9ac1QwhkjW7BvDt0gl2Kt9fGdDytJyndoPRcBU= github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261017134002-3def3f898617/go.mod h1:iOwl8e9EKGb3VQ4u/YjiqVzglhD6/GX1ABC3tBkqu0A= github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261017134002-3def3f898617 h1:VYVbQyoR+tIcPoXhkviclexKuU5H+A21hSXudB+kZag= From 6db77b0412ac6264ebec512a4dc234fbabf6bbe5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 00:04:29 +0200 Subject: [PATCH 2/4] fix(purchase): stamp RequestedAt at dispatch for Redshift dedupe go's Redshift purchase dedupe bounds its DescribeReservedNodes match window with PurchaseOptions.RequestedAt and refuses a tokenized buy without it. Set it to the UTC dispatch time immediately before the single PurchaseCommitment call, for all providers (others ignore it). Tests cover a two-account fan-out for EC2 and Redshift and the empty-token path. Part of #702 Co-Authored-By: Claude Opus 5.5 --- internal/purchase/execution.go | 2 + internal/purchase/execution_test.go | 8 +- internal/purchase/requested_at_test.go | 124 +++++++++++++++++++++++++ 3 files changed, 130 insertions(+), 4 deletions(-) create mode 100644 internal/purchase/requested_at_test.go diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index ebf6a6d60..037a88a8b 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -1180,6 +1180,8 @@ func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.Recommen // (which the AWS SDK retries up to 3× × 30s by default) versus // something earlier in the flow — issue #667. tCall := time.Now() + // go's Redshift dedupe needs the dispatch time to bound its DescribeReservedNodes match window. + opts.RequestedAt = time.Now().UTC() result, err := serviceClient.PurchaseCommitment(recCtx, recommendation, opts) elapsed := time.Since(tCall) if err != nil { diff --git a/internal/purchase/execution_test.go b/internal/purchase/execution_test.go index 9037222db..182322639 100644 --- a/internal/purchase/execution_test.go +++ b/internal/purchase/execution_test.go @@ -174,11 +174,11 @@ func TestManager_ExecutePurchase_WebSourcePropagates(t *testing.T) { // the execution ID and the rec's index, so the web path now also carries it. mockServiceClient.On("PurchaseCommitment", mock.MatchedBy(hasPerRecDeadline(30*time.Second)), mock.AnythingOfType("common.Recommendation"), - common.PurchaseOptions{ + optsIgnoringRequestedAt(common.PurchaseOptions{ Source: common.PurchaseSourceWeb, IdempotencyToken: common.DeriveIdempotencyToken("exec-web", 0), ExecutionID: "exec-web", - }, + }), ).Return(common.PurchaseResult{Success: true, CommitmentID: "ri-web"}, nil) manager := &Manager{ @@ -230,11 +230,11 @@ func TestManager_ExecutePurchase_InvalidSourceFallsBackUntagged(t *testing.T) { // The idempotency token (issue #636) is still derived regardless of source. mockServiceClient.On("PurchaseCommitment", mock.MatchedBy(hasPerRecDeadline(30*time.Second)), mock.AnythingOfType("common.Recommendation"), - common.PurchaseOptions{ + optsIgnoringRequestedAt(common.PurchaseOptions{ Source: "", IdempotencyToken: common.DeriveIdempotencyToken("exec-bad", 0), ExecutionID: "exec-bad", - }, + }), ).Return(common.PurchaseResult{Success: true, CommitmentID: "ri-bad"}, nil) manager := &Manager{ diff --git a/internal/purchase/requested_at_test.go b/internal/purchase/requested_at_test.go new file mode 100644 index 000000000..d7b3f2c02 --- /dev/null +++ b/internal/purchase/requested_at_test.go @@ -0,0 +1,124 @@ +package purchase + +import ( + "context" + "sync" + "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/mock" + "github.com/stretchr/testify/require" +) + +// runFanOutCapturingOpts runs a two-account fan-out of recs built by mk and +// returns the PurchaseOptions every PurchaseCommitment call received. +func runFanOutCapturingOpts(t *testing.T, service common.ServiceType, mk func(string) config.RecommendationRecord) []common.PurchaseOptions { + t.Helper() + ctx := context.Background() + accounts := []config.CloudAccount{ + {ID: "acct-A", Name: "A", Provider: "aws", ExternalID: "111111111111", AWSAuthMode: "access_keys"}, + {ID: "acct-B", Name: "B", Provider: "aws", ExternalID: "222222222222", AWSAuthMode: "access_keys"}, + } + mockStore := new(MockConfigStore) + mockEmail := new(MockEmailSender) + mockFactory := new(MockProviderFactory) + mockProviderInst := new(MockProvider) + mockServiceClient := new(MockServiceClient) + + exec := &config.PurchaseExecution{ + ExecutionID: "root-exec", IdempotencyKey: "root-lineage", PlanID: "plan-x", + Source: common.PurchaseSourceWeb, + Recommendations: []config.RecommendationRecord{mk("acct-A"), mk("acct-B")}, + } + mockStore.SavePurchaseExecutionFn = func(context.Context, *config.PurchaseExecution) error { return nil } + mockStore.On("SavePurchaseHistory", ctx, mock.AnythingOfType("*config.PurchaseHistoryRecord")).Return(nil) + mockEmail.On("SendPurchaseConfirmation", ctx, mock.AnythingOfType("email.NotificationData")).Return(nil) + mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil) + mockProviderInst.On("GetServiceClient", mock.Anything, service, mock.Anything).Return(mockServiceClient, nil) + + var mu sync.Mutex + var got []common.PurchaseOptions + mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + mu.Lock() + got = append(got, args.Get(2).(common.PurchaseOptions)) + mu.Unlock() + }).Return(common.PurchaseResult{Success: true, CommitmentID: "ri-ok"}, nil) + + manager := &Manager{ + config: mockStore, email: mockEmail, providerFactory: mockFactory, + credStore: awsAccessKeyCredStore(), dashboardURL: "https://dashboard.example.com", + } + require.NoError(t, manager.executeMultiAccount(ctx, exec, &config.PurchasePlan{ID: "plan-x", Name: "Plan X"}, accounts)) + return got +} + +// optsIgnoringRequestedAt matches PurchaseOptions equal to want apart from the +// dispatch-time RequestedAt, which is a clock reading. +func optsIgnoringRequestedAt(want common.PurchaseOptions) interface{} { + return mock.MatchedBy(func(got common.PurchaseOptions) bool { + got.RequestedAt = want.RequestedAt + return got == want + }) +} + +func assertRequestedAtStamped(t *testing.T, got []common.PurchaseOptions, before, after time.Time) { + t.Helper() + require.Len(t, got, 2) + for _, o := range got { + require.False(t, o.RequestedAt.IsZero(), "RequestedAt must be set at dispatch") + assert.Equal(t, time.UTC, o.RequestedAt.Location()) + assert.False(t, o.RequestedAt.Before(before), "RequestedAt %s before dispatch start %s", o.RequestedAt, before) + assert.False(t, o.RequestedAt.After(after), "RequestedAt %s after dispatch end %s", o.RequestedAt, after) + } +} + +func TestExecuteMultiAccount_StampsRequestedAtOnEveryDispatch(t *testing.T) { + before := time.Now() + got := runFanOutCapturingOpts(t, common.ServiceEC2, scopedTestRec) + assertRequestedAtStamped(t, got, before, time.Now()) + for _, o := range got { + assert.NotEmpty(t, o.IdempotencyToken) + assert.Equal(t, common.PurchaseSourceWeb, o.Source) + assert.NotEmpty(t, o.ExecutionID) + } +} + +func TestExecuteMultiAccount_StampsRequestedAtOnRedshift(t *testing.T) { + mk := func(acct string) config.RecommendationRecord { + r := scopedTestRec(acct) + r.Service, r.ResourceType = "redshift", "ra3.xlplus" + return r + } + before := time.Now() + got := runFanOutCapturingOpts(t, common.ServiceRedshift, mk) + assertRequestedAtStamped(t, got, before, time.Now()) +} + +// With no idempotency token the options pass through unchanged apart from +// RequestedAt. +func TestExecuteSinglePurchase_EmptyTokenOptsOtherwiseUntouched(t *testing.T) { + mockFactory := new(MockProviderFactory) + mockProviderInst := new(MockProvider) + mockServiceClient := new(MockServiceClient) + mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProviderInst, nil) + mockProviderInst.On("GetServiceClient", mock.Anything, common.ServiceEC2, mock.Anything).Return(mockServiceClient, nil) + var got common.PurchaseOptions + mockServiceClient.On("PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { got = args.Get(2).(common.PurchaseOptions) }). + Return(common.PurchaseResult{Success: true, CommitmentID: "ri-ok"}, nil) + m := &Manager{providerFactory: mockFactory} + in := common.PurchaseOptions{Source: common.PurchaseSourceWeb, ExecutionID: "exec-1"} + + before := time.Now() + _, err := m.executeSinglePurchase(context.Background(), scopedTestRec("acct-A"), nil, in) + after := time.Now() + + require.NoError(t, err) + assertRequestedAtStamped(t, []common.PurchaseOptions{got, got}, before, after) + got.RequestedAt = time.Time{} + assert.Equal(t, in, got) +} From cba7125010a0ee262fd1bd19503d612ddb2a3cde Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 00:05:17 +0200 Subject: [PATCH 3/4] docs(known-issues): record Redshift dedupe window limits go's Redshift match window (start >= RequestedAt-5min) has no upper bound, so a same-tuple purchase shortly after a completed one adopts the earlier node without buying, and an unknown outcome followed by a new same-tuple purchase outside the window can double-buy. Redshift re-drive stays refused. Part of #702 Co-Authored-By: Claude Opus 5.5 --- known_issues/39_redshift_dedupe_window.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 known_issues/39_redshift_dedupe_window.md diff --git a/known_issues/39_redshift_dedupe_window.md b/known_issues/39_redshift_dedupe_window.md new file mode 100644 index 000000000..2ea293ae8 --- /dev/null +++ b/known_issues/39_redshift_dedupe_window.md @@ -0,0 +1,23 @@ +# Known Issues: Redshift reserved-node dedupe window + +> **Audit status (2026-10-20):** `2 needs triage · 0 resolved` + +## MEDIUM: a repeat same-tuple Redshift purchase within ~5 minutes adopts the earlier node without buying + +**Files:** `internal/purchase/execution.go` (`executeSinglePurchase`), and in `LeanerCloud/cloud-commitments-go` the Redshift purchase dedupe in `providers/aws` + +**Description:** The platform stamps `PurchaseOptions.RequestedAt` at dispatch. go's Redshift dedupe matches an existing reserved node through `DescribeReservedNodes` when its start time is at or after `RequestedAt - 5min`, and the window has no upper bound. A later purchase with the same account, region, offering and node count made within about 5 minutes of a completed one finds the earlier node, adopts it and reports success without buying anything. + +**Why deferred:** The fix belongs in go (an upper bound or an identity stronger than the tuple), tracked in LeanerCloud/cloud-commitments-go#515. The platform only supplies `RequestedAt` (#702). + +**Status:** Open. Tracked in #702 and LeanerCloud/cloud-commitments-go#515. + +## LOW: unknown outcome followed by a new same-tuple purchase outside the window can double-buy + +**Files:** `internal/purchase/execution.go`, `internal/purchase/manager.go` + +**Description:** If a Redshift purchase ends with an unknown outcome and a NEW purchase (not a re-drive) of the same tuple is dispatched after the match window has passed, the dedupe cannot see the first node and both are bought. Redshift re-drive stays refused (`awsPurchaseHasNoDuplicateGuard`, `recRedriveRefusalReason`, `armedRedriveRefusal` are unchanged), so only a fresh purchase can hit this. + +**Why deferred:** Mitigated by purchase suppressions and the operator message shown for an unknown outcome. A complete fix needs a durable per-purchase identity on the Redshift side. + +**Status:** Open. Tracked in #702. From 86c4dcd4bf498d11ad4a8e7dfdb1c9121b91773b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 00:24:12 +0200 Subject: [PATCH 4/4] docs(known-issues): record concurrent Redshift dedupe and invisible adoption Review of the RequestedAt change found three gaps worth tracking: same-tuple purchases in parallel or sequence are matched by tuple only, adoption of an existing node is recorded as an ordinary purchase, and RequestedAt is stamped per dispatch rather than per lineage (safe only while re-drive is refused). Part of #702 Co-Authored-By: Claude Opus 5.5 --- known_issues/39_redshift_dedupe_window.md | 32 ++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/known_issues/39_redshift_dedupe_window.md b/known_issues/39_redshift_dedupe_window.md index 2ea293ae8..8f8b75b6a 100644 --- a/known_issues/39_redshift_dedupe_window.md +++ b/known_issues/39_redshift_dedupe_window.md @@ -1,6 +1,6 @@ # Known Issues: Redshift reserved-node dedupe window -> **Audit status (2026-10-20):** `2 needs triage · 0 resolved` +> **Audit status (2026-10-20):** `5 needs triage · 0 resolved` ## MEDIUM: a repeat same-tuple Redshift purchase within ~5 minutes adopts the earlier node without buying @@ -21,3 +21,33 @@ **Why deferred:** Mitigated by purchase suppressions and the operator message shown for an unknown outcome. A complete fix needs a durable per-purchase identity on the Redshift side. **Status:** Open. Tracked in #702. + +## MEDIUM: concurrent or duplicate same-tuple Redshift purchases are not distinguished + +**Files:** `internal/purchase/execution.go` (fan-out, ~650-690), `internal/purchase/manager.go` + +**Description:** Two recs in one execution, or two executions, for the same account, region, offering ID and node count get different idempotency tokens, but the library matches by tuple only. Run sequentially, the second adopts the first node and reports success without buying. Run in parallel (recs in one execution run concurrently, and the per-row claim does not span executions), both guard lookups can see no node and both buy (check-then-act). + +**Why deferred:** Needs a tuple-level lock or a stronger identity in go (cloud-commitments-go#515). + +**Status:** Open. Tracked in #702. + +## MEDIUM: adoption of an existing Redshift node is invisible + +**Files:** `internal/purchase/execution.go`, `internal/purchase/manager.go` + +**Description:** The platform does not read `PurchaseResult.ExistingCommitment`, so an adopted node (guard match or unknown-outcome recovery) is recorded as an ordinary successful purchase (history row, totals, Cost nil) with no flag that nothing was bought. + +**Why deferred:** Surfacing adoption needs a history/UI field and is separate from stamping `RequestedAt`. + +**Status:** Open. Tracked in #702. + +## LOW: RequestedAt is stamped per dispatch, not per purchase lineage + +**Files:** `internal/purchase/execution.go` + +**Description:** The library documents `RequestedAt` as stable across re-drives of the same token; the platform stamps a fresh time per dispatch. This is only safe because Redshift re-drive is refused. Lifting that refusal requires a persisted first-dispatch time (#702 option B). + +**Why deferred:** Re-drive stays refused, so there is no current impact. + +**Status:** Open. Tracked in #702.