Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
2 changes: 2 additions & 0 deletions internal/purchase/execution.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
8 changes: 4 additions & 4 deletions internal/purchase/execution_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down
124 changes: 124 additions & 0 deletions internal/purchase/requested_at_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
53 changes: 53 additions & 0 deletions known_issues/39_redshift_dedupe_window.md
Original file line number Diff line number Diff line change
@@ -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.
Loading