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= 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) +} diff --git a/known_issues/39_redshift_dedupe_window.md b/known_issues/39_redshift_dedupe_window.md new file mode 100644 index 000000000..8f8b75b6a --- /dev/null +++ b/known_issues/39_redshift_dedupe_window.md @@ -0,0 +1,53 @@ +# Known Issues: Redshift reserved-node dedupe window + +> **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 + +**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. + +## 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.