diff --git a/CHANGELOG.md b/CHANGELOG.md index 161c9f4..2ef5e87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Added +- Every real purchase passes `PurchaseOptions.RequestedAt`, the first-dispatch + time of its idempotency key, recorded before the provider call and reused on + retries (go#515: Redshift finds an earlier purchase by start time). With + `CUDLY_MCP_BUDGET_STATE` set it is persisted in `.attempts` (30-day + retention; a corrupt or unwritable file refuses the purchase). Without it the + time is per process, and Redshift purchases are refused because their dedupe + must survive a restart. - `cudly_aws_ec2_ri_purchase` takes an optional `offering_class` (`standard` or `convertible`, default `convertible`). It is echoed in the response and recorded in the audit log (`raw_recommendation`). @@ -50,6 +57,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Changed +- Bumped the `cloud-commitments-go` pins to 1ce87bd. `computeengine.NewClient` no + longer takes a context (go#575); the GCP test call sites follow. Behavior + pulled in: Savings Plans in aws-cn are refused before any AWS call (go#566); + Azure Savings Plans refuse a malformed idempotency token before the purchase + PUT (e735fdb, go#495); Redshift purchases are deduped by DescribeReservedNodes + and require `RequestedAt` (go#515, see Added); remaining AWS and MemoryDB + pagination loops are capped (go#573); RDS reservations count against other + sizes of the same family in dedupe (go#564). - Bumped the `cloud-commitments-go` pins to main (9dae64b). `ARCHERA_API_KEY` must now be printable ASCII (0x21-0x7E): the client rejects any other byte and its error never echoes the key. Library decode errors now show only @@ -95,6 +110,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- `cudly_aws_ec2_ri_purchase` with `offering_class=standard` is now capped by + `CUDLY_MCP_MAX_COMMITMENT_USD` and counted against `CUDLY_MCP_BUDGET_USD` at + the standard offering's price (`GetOfferingDetailsForClass`, go#574), not the + convertible one. A client that cannot price a class refuses a standard + purchase instead of falling back to convertible pricing. - The MCPB bundle no longer blocks sub-unit spend caps (for example 0.5 USD/h): the hourly-commitment and memory caps now declare `min` 0, and the server still rejects values `<= 0`. diff --git a/cmd/cudly-mcp/gcp_purchase_protocol_test.go b/cmd/cudly-mcp/gcp_purchase_protocol_test.go index 727d185..a5758a8 100644 --- a/cmd/cudly-mcp/gcp_purchase_protocol_test.go +++ b/cmd/cudly-mcp/gcp_purchase_protocol_test.go @@ -98,7 +98,7 @@ func (p *fakeGCPProvider) GetServiceClient(ctx context.Context, service common.S if service != common.ServiceCompute { return nil, errors.New("fakeGCPProvider: only compute is implemented") } - client, err := computeengine.NewClient(ctx, p.cfg.GCPProjectID, region) + client, err := computeengine.NewClient(p.cfg.GCPProjectID, region) if err != nil { return nil, err } diff --git a/go.mod b/go.mod index 1751285..8056ead 100644 --- a/go.mod +++ b/go.mod @@ -74,10 +74,10 @@ require ( ) require ( - github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261016175124-570d168c6bc7 - github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261012130811-9dae64b216d4 - github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261012130811-9dae64b216d4 - github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261012130811-9dae64b216d4 + github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261020104357-1ce87bd24c6e + github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261020104357-1ce87bd24c6e + github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261020104357-1ce87bd24c6e + github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261020104357-1ce87bd24c6e github.com/google/jsonschema-go v0.4.3 github.com/google/uuid v1.6.0 github.com/modelcontextprotocol/go-sdk v1.8.0 diff --git a/go.sum b/go.sum index 7596533..f955b1f 100644 --- a/go.sum +++ b/go.sum @@ -76,14 +76,14 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0 github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.55.0/go.mod h1:vB2GH9GAYYJTO3mEn8oYwzEdhlayZIdQz6zdzgUIRvA= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.55.0 h1:0s6TxfCu2KHkkZPnBfsQ2y5qia0jl3MMrmBhu3nCOYk= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.55.0/go.mod h1:Mf6O40IAyB9zR/1J8nGDDPirZQQPbYJni8Yisy7NTMc= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261016175124-570d168c6bc7 h1:mAvqQSMkTuw4P8m8o+/wdyoxGE9L6eiEzZFanGXqYcI= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261016175124-570d168c6bc7/go.mod h1:SCDEgHG6evjPh0RsoXof+J3aMmGXNFMrwbJIle7rMpQ= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261012130811-9dae64b216d4 h1:fnEv+2eCL0ObXn1nAFlBa2kZy1A0/nhc7LxYeDFCCkQ= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261012130811-9dae64b216d4/go.mod h1:C0ildZeRds0aRvDZp49MLY0kFWh/4FiM4lPWWWd3HFA= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261012130811-9dae64b216d4 h1:HljKKZOmfam4C3T5109DMeUQuyxVHRohroA4j/OU5oY= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261012130811-9dae64b216d4/go.mod h1:nM3Zqd5N2mMIsNoTtnKF6RFyM1E3HVjAGIsXVx9b3bY= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261012130811-9dae64b216d4 h1:o9hnEg+Ro/RynW8ZWDVMC0E2yWt08izWeFbrCsZZx/c= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261012130811-9dae64b216d4/go.mod h1:uJ7UpLaUqu1fpfCEECuqjLXV+As2tibtPCapPF3AZys= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261020104357-1ce87bd24c6e h1:8A1IrBXDZesumWuWAwoSfxxGyL9yRxsjggR9iQy5aG8= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261020104357-1ce87bd24c6e/go.mod h1:/yCOnBtEJRRydFrDm6g1UcHuJwuB35AqYTBdaHDG8wo= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261020104357-1ce87bd24c6e h1:Zpt/q0jPyX8ZQVz78caozIliEQ+e5NhyKcHEFrFh4og= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261020104357-1ce87bd24c6e/go.mod h1:DNq7Q17pdRT/8gvFzm0UGSN9cnDIj2p6LaebsHMu5vI= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261020104357-1ce87bd24c6e h1:5ufTdFO9ngBZ6KF3HpvqhBXmTZyYHu86zlG/DblSInw= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261020104357-1ce87bd24c6e/go.mod h1:N/wLbj7Yy5tb8nDbvni8YkEy7m52KX1V3Cnwjow7Mcg= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261020104357-1ce87bd24c6e h1:e2jnw3XhoBMfsey1iU+sZYAQ4n/0MVMfcBpEdn2+0cM= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261020104357-1ce87bd24c6e/go.mod h1:wIRAI3sBSWy5pL6mDSRYBVaqGy5nS5n0BMTxiNsP4Oo= github.com/aws/aws-sdk-go-v2 v1.47.3 h1:HRoT/MkNi3bjO5xF4dCzLzvVmkvRGHj3/cLINDnJejk= github.com/aws/aws-sdk-go-v2 v1.47.3/go.mod h1:xSl+OSI5ZUw6bSMupWc218zE5Pl+65MCCu3ySWFY99A= github.com/aws/aws-sdk-go-v2/config v1.29.12 h1:Y/2a+jLPrPbHpFkpAAYkVEtJmxORlXoo5k2g1fa2sUo= diff --git a/tools/attempt_store.go b/tools/attempt_store.go new file mode 100644 index 0000000..7b0eee5 --- /dev/null +++ b/tools/attempt_store.go @@ -0,0 +1,113 @@ +package tools + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io/fs" + "os" + "strings" + "sync" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +// attemptRetention is how long a first-attempt time is kept. The MCP has no +// idempotency horizon constant, so this is a chosen 30 days: far beyond any +// retry window, and a key older than that is a new purchase. +const attemptRetention = 30 * 24 * time.Hour + +const attemptStateVersion = 1 + +// attemptState maps an idempotency token to the UTC time of its first real +// dispatch (PurchaseOptions.RequestedAt). +type attemptState struct { + Attempts map[string]time.Time `json:"attempts"` + Version int `json:"version"` +} + +var ( + processAttemptsMu sync.Mutex + processAttempts = map[string]time.Time{} +) + +// firstAttemptTime returns the first-dispatch time for token, recording now +// durably when the key is new. cloud-commitments-go#515 finds an earlier +// Redshift purchase by start time relative to RequestedAt, so a retry must +// reuse the first attempt's time, not its own call time. The record is made +// before the provider is called. With EnvBudgetState set it lives in +// .attempts under the same lock and atomic-write rules as the budget +// file; a corrupt or unwritable file is an error, never a fresh time. Without +// it the time is kept in process memory only (like the in-memory budget), and +// Redshift, whose dedupe depends on it surviving a restart, is refused. +func firstAttemptTime(token string, service common.ServiceType) (time.Time, error) { + now := budgetNow().UTC() + path := strings.TrimSpace(os.Getenv(EnvBudgetState)) + if path == "" { + if service == common.ServiceRedshift { + return time.Time{}, fmt.Errorf("refusing real purchase: Redshift reserved nodes cannot be tagged, so a retry is deduped by the first attempt's start time, which must survive a restart; set %s", EnvBudgetState) + } + processAttemptsMu.Lock() + defer processAttemptsMu.Unlock() + if t, ok := processAttempts[token]; ok { + return t, nil + } + processAttempts[token] = now + return now, nil + } + t, err := fileFirstAttempt(path+".attempts", token, now) + if err != nil { + return time.Time{}, fmt.Errorf("refusing real purchase: first-attempt state unusable: %w", err) + } + return t, nil +} + +// fileFirstAttempt is firstAttemptTime for the persisted store at path. +func fileFirstAttempt(path, token string, now time.Time) (time.Time, error) { + if err := ensureAuditLogDirectory(path); err != nil { + return time.Time{}, fmt.Errorf("prepare directory for %s: %w", path, err) + } + unlock, err := lockBudgetFile(path+".lock", budgetLockTimeout) + if err != nil { + return time.Time{}, err + } + defer unlock() + st, err := loadAttempts(path) + if err != nil { + return time.Time{}, err + } + for k, t := range st.Attempts { + if now.Sub(t) > attemptRetention { + delete(st.Attempts, k) + } + } + if t, ok := st.Attempts[token]; ok { + return t, nil + } + st.Attempts[token] = now + return now, writeStateAtomically(path, st, "first-attempt state") +} + +func loadAttempts(path string) (attemptState, error) { + data, err := os.ReadFile(path) // #nosec G304 G703 -- operator-set path, not tool input + if errors.Is(err, fs.ErrNotExist) { + return attemptState{Version: attemptStateVersion, Attempts: map[string]time.Time{}}, nil + } + if err != nil { + return attemptState{}, fmt.Errorf("read %s: %w", path, err) + } + var st attemptState + dec := json.NewDecoder(bytes.NewReader(data)) + dec.DisallowUnknownFields() + if err := dec.Decode(&st); err != nil || dec.More() || st.Version != attemptStateVersion || st.Attempts == nil { + return attemptState{}, fmt.Errorf("%s is corrupt", path) + } + for k, t := range st.Attempts { + if k == "" || t.IsZero() { + return attemptState{}, fmt.Errorf("%s is corrupt: invalid entry %q", path, k) + } + } + return st, nil +} diff --git a/tools/attempt_store_test.go b/tools/attempt_store_test.go new file mode 100644 index 0000000..ae78bc9 --- /dev/null +++ b/tools/attempt_store_test.go @@ -0,0 +1,193 @@ +package tools + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + awsredshift "github.com/aws/aws-sdk-go-v2/service/redshift" + rstypes "github.com/aws/aws-sdk-go-v2/service/redshift/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/redshift" +) + +// attemptClock points the budget/attempt clock at a settable time. +func attemptClock(t *testing.T, start time.Time) *time.Time { + t.Helper() + cur := start + prev := budgetNow + budgetNow = func() time.Time { return cur } + t.Cleanup(func() { budgetNow = prev }) + return &cur +} + +func attemptStatePath(t *testing.T) string { + t.Helper() + path := filepath.Join(t.TempDir(), "budget.json") + t.Setenv(EnvBudgetState, path) + t.Setenv(EnvBudgetUSD, "1000") + t.Setenv(EnvMaxCommitmentUSD, "1000000") + return path +} + +func TestFirstAttemptRecordedThenReusedAfterLibraryWindow(t *testing.T) { + attemptStatePath(t) + clock := attemptClock(t, time.Date(2026, 3, 1, 12, 0, 0, 0, time.UTC)) + first := *clock + + got, err := firstAttemptTime("tok", common.ServiceRedshift) + require.NoError(t, err) + assert.Equal(t, first, got) + + *clock = first.Add(time.Hour) // far past the library's 5 minute window + got, err = firstAttemptTime("tok", common.ServiceRedshift) + require.NoError(t, err) + assert.Equal(t, first, got, "a retry must reuse the first attempt time") + + other, err := firstAttemptTime("other", common.ServiceRedshift) + require.NoError(t, err) + assert.Equal(t, first.Add(time.Hour), other) +} + +func TestFirstAttemptRetentionDropsOldEntries(t *testing.T) { + attemptStatePath(t) + clock := attemptClock(t, time.Date(2026, 3, 1, 12, 0, 0, 0, time.UTC)) + first := *clock + _, err := firstAttemptTime("tok", common.ServiceRedshift) + require.NoError(t, err) + + *clock = first.Add(attemptRetention + time.Hour) + got, err := firstAttemptTime("tok", common.ServiceRedshift) + require.NoError(t, err) + assert.Equal(t, *clock, got) +} + +func TestFirstAttemptCorruptStoreRefuses(t *testing.T) { + path := attemptStatePath(t) + attemptClock(t, time.Now()) + require.NoError(t, os.WriteFile(path+".attempts", []byte("{not json"), 0o600)) + + _, err := firstAttemptTime("tok", common.ServiceRedshift) + require.ErrorContains(t, err, "corrupt") + + fake := pricedFake(10, common.PurchaseResult{Success: true, CommitmentID: "id"}, nil) + err = runWithNonce(fake, "n") + require.ErrorContains(t, err, "first-attempt state unusable") + assert.Zero(t, fake.purchaseCalls) + assert.Zero(t, mustBudgetSpent(t, path), "reservation must be released") +} + +func TestFirstAttemptWithoutStateFileRefusesRedshiftOnly(t *testing.T) { + t.Setenv(EnvBudgetState, "") + attemptClock(t, time.Now()) + _, err := firstAttemptTime("tok-mem", common.ServiceRedshift) + require.ErrorContains(t, err, EnvBudgetState) + _, err = firstAttemptTime("tok-mem", common.ServiceEC2) + require.NoError(t, err) +} + +func mustBudgetSpent(t *testing.T, path string) float64 { + t.Helper() + st, err := newFileBudget(path).load() + require.NoError(t, err) + return st.totalUSD() +} + +type recordingProviderOpts struct { + *fakeServiceClient + got common.PurchaseOptions +} + +func (r *recordingProviderOpts) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { + r.got = opts + return r.fakeServiceClient.PurchaseCommitment(ctx, rec, opts) +} + +func TestPurchasePassesFirstAttemptAsRequestedAt(t *testing.T) { + attemptStatePath(t) + clock := attemptClock(t, time.Date(2026, 3, 1, 12, 0, 0, 0, time.UTC)) + first := *clock + fake := &recordingProviderOpts{fakeServiceClient: pricedFake(10, common.PurchaseResult{Success: true, CommitmentID: "id"}, nil)} + + require.NoError(t, runWithNonce(fake, "n")) + assert.Equal(t, first, fake.got.RequestedAt) + *clock = first.Add(time.Hour) + require.NoError(t, runWithNonce(fake, "n")) + assert.Equal(t, first, fake.got.RequestedAt) +} + +// fakeRedshiftAPI is a mocked Redshift API behind the real redshift.Client. +type fakeRedshiftAPI struct { + nodes []rstypes.ReservedNode + purchases int +} + +func (f *fakeRedshiftAPI) PurchaseReservedNodeOffering(_ context.Context, _ *awsredshift.PurchaseReservedNodeOfferingInput, _ ...func(*awsredshift.Options)) (*awsredshift.PurchaseReservedNodeOfferingOutput, error) { + f.purchases++ + return &awsredshift.PurchaseReservedNodeOfferingOutput{ReservedNode: &rstypes.ReservedNode{ReservedNodeId: aws.String("rn-1")}}, nil +} + +func (f *fakeRedshiftAPI) DescribeReservedNodeOfferings(_ context.Context, _ *awsredshift.DescribeReservedNodeOfferingsInput, _ ...func(*awsredshift.Options)) (*awsredshift.DescribeReservedNodeOfferingsOutput, error) { + return &awsredshift.DescribeReservedNodeOfferingsOutput{ReservedNodeOfferings: []rstypes.ReservedNodeOffering{{ + ReservedNodeOfferingId: aws.String("off-1"), NodeType: aws.String("dc2.large"), + Duration: aws.Int32(3 * 12 * 2592000), ReservedNodeOfferingType: rstypes.ReservedNodeOfferingTypeRegular, + UsagePrice: aws.Float64(0.1), + }}}, nil +} + +func (f *fakeRedshiftAPI) DescribeReservedNodes(_ context.Context, _ *awsredshift.DescribeReservedNodesInput, _ ...func(*awsredshift.Options)) (*awsredshift.DescribeReservedNodesOutput, error) { + return &awsredshift.DescribeReservedNodesOutput{ReservedNodes: f.nodes}, nil +} + +// realRedshiftClient prices through the shared fake and purchases through the +// real redshift.Client. +type realRedshiftClient struct { + *redshift.Client + pricing *fakeServiceClient +} + +func (c realRedshiftClient) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) { + return c.pricing.GetOfferingDetails(ctx, rec) +} + +func TestRedshiftPurchaseProceedsAndRetryAdoptsAfterWindow(t *testing.T) { + attemptStatePath(t) + clock := attemptClock(t, time.Date(2026, 3, 1, 12, 0, 0, 0, time.UTC)) + first := *clock + api := &fakeRedshiftAPI{} + rc := redshift.NewClient(aws.Config{Region: "us-east-1"}) + rc.SetRedshiftAPI(api) + var client provider.ServiceClient = realRedshiftClient{rc, pricedFake(10, common.PurchaseResult{}, nil)} + + run := func() (*PurchaseResponse, error) { + rec := testRecommendation() + rec.Service, rec.ResourceType, rec.Count, rec.PaymentOption = common.ServiceRedshift, "dc2.large", 1, "no-upfront" + return ExecutePurchase(context.Background(), PurchaseRequest{ + Region: rec.Region, Recommendation: rec, Confirm: true, CredentialScope: "scope", Nonce: "n", + ResolveClient: func(context.Context) (provider.ServiceClient, error) { return client, nil }, + }) + } + resp, err := run() + require.NoError(t, err) + assert.True(t, resp.Success) + assert.Equal(t, 1, api.purchases) + + // The node now exists, started right after the first attempt. A retry an + // hour later must still find it: RequestedAt stays at the first attempt. + api.nodes = []rstypes.ReservedNode{{ + ReservedNodeId: aws.String("rn-1"), ReservedNodeOfferingId: aws.String("off-1"), + NodeCount: aws.Int32(1), State: aws.String("active"), StartTime: aws.Time(first.Add(time.Second)), + }} + *clock = first.Add(time.Hour) + resp, err = run() + require.NoError(t, err) + assert.Equal(t, 1, api.purchases, "the retry must adopt the first purchase, not buy again") + assert.Equal(t, "rn-1", resp.CommitmentID) +} diff --git a/tools/aws_ec2_ri_test.go b/tools/aws_ec2_ri_test.go index cac946c..564c22f 100644 --- a/tools/aws_ec2_ri_test.go +++ b/tools/aws_ec2_ri_test.go @@ -327,7 +327,7 @@ func TestAWSEC2RIPurchaseOfferingClassPassThrough(t *testing.T) { } { t.Run("real_"+tc.want+"_from_"+tc.in, func(t *testing.T) { t.Parallel() - fake := &fakeServiceClient{purchaseResult: common.PurchaseResult{Success: true, CommitmentID: "ri-abc"}} + fake := newClassPricedClient() var gotService common.ServiceType var gotRegion string tool := &awsEC2RIPurchaseTool{ @@ -419,7 +419,7 @@ func TestAWSEC2RIPurchaseRejectsWrongCaseOfferingClass(t *testing.T) { func TestAWSEC2RIPurchaseAuditRecordCarriesOfferingClass(t *testing.T) { path := filepath.Join(t.TempDir(), "audit.jsonl") t.Setenv(EnvAuditLog, path) - fake := &fakeServiceClient{purchaseResult: common.PurchaseResult{Success: true, CommitmentID: "ri-abc"}} + fake := newClassPricedClient() var gotService common.ServiceType var gotRegion string tool := &awsEC2RIPurchaseTool{ diff --git a/tools/aws_simple_ri_test.go b/tools/aws_simple_ri_test.go index f38ba65..baff4df 100644 --- a/tools/aws_simple_ri_test.go +++ b/tools/aws_simple_ri_test.go @@ -202,6 +202,12 @@ func TestSimpleAWSRIPurchaseHandleRealPurchaseCallsCorrectService(t *testing.T) args.Confirm = boolPtr(true) _, resp, err := tool.handle(context.Background(), nil, args) + if product == "redshift" { + // Redshift needs the durable first-attempt store (EnvBudgetState). + require.ErrorContains(t, err, EnvBudgetState) + assert.Zero(t, fake.purchaseCalls) + return + } require.NoError(t, err) assert.True(t, resp.Success) assert.Equal(t, tool.spec.service, gotService) diff --git a/tools/budget_state.go b/tools/budget_state.go index 7026cf0..0b9c891 100644 --- a/tools/budget_state.go +++ b/tools/budget_state.go @@ -147,17 +147,22 @@ func (f *fileBudget) load() (budgetState, error) { return st, nil } -// save replaces the file atomically: temp file in the same directory, fsync, -// rename, then fsync of the directory so the rename survives a crash. func (f *fileBudget) save(st budgetState) error { - data, err := json.MarshalIndent(st, "", " ") + return writeStateAtomically(f.path, st, "budget state") +} + +// writeStateAtomically replaces path with the JSON of v: temp file in the same +// directory, fsync, rename, then fsync of the directory so the rename survives +// a crash. what names the file kind in errors. +func writeStateAtomically(path string, v any, what string) error { + data, err := json.MarshalIndent(v, "", " ") if err != nil { - return fmt.Errorf("encode budget state: %w", err) + return fmt.Errorf("encode %s: %w", what, err) } - dir := filepath.Dir(f.path) - tmp, err := os.CreateTemp(dir, filepath.Base(f.path)+".tmp-*") + dir := filepath.Dir(path) + tmp, err := os.CreateTemp(dir, filepath.Base(path)+".tmp-*") if err != nil { - return fmt.Errorf("create budget state temp file in %s: %w", dir, err) + return fmt.Errorf("create %s temp file in %s: %w", what, dir, err) } tmpName := tmp.Name() _, werr := tmp.Write(append(data, '\n')) @@ -165,15 +170,15 @@ func (f *fileBudget) save(st budgetState) error { cerr := tmp.Close() if jerr := errors.Join(werr, serr, cerr); jerr != nil { _ = os.Remove(tmpName) // #nosec G703 -- temp file we created in the operator-set directory - return fmt.Errorf("write budget state temp file %s: %w", tmpName, jerr) + return fmt.Errorf("write %s temp file %s: %w", what, tmpName, jerr) } - if rerr := os.Rename(tmpName, f.path); rerr != nil { // #nosec G703 -- operator-set path, not tool input + if rerr := os.Rename(tmpName, path); rerr != nil { // #nosec G703 -- operator-set path, not tool input _ = os.Remove(tmpName) // #nosec G703 -- temp file we created in the operator-set directory - return fmt.Errorf("replace budget state file %s: %w", f.path, rerr) + return fmt.Errorf("replace %s file %s: %w", what, path, rerr) } d, err := os.Open(dir) // #nosec G304 G703 -- parent of the operator-set path if err != nil { - return fmt.Errorf("open budget state directory %s: %w", dir, err) + return fmt.Errorf("open %s directory %s: %w", what, dir, err) } return errors.Join(d.Sync(), d.Close()) } diff --git a/tools/commitment_usd.go b/tools/commitment_usd.go index a34dbc1..0d454b2 100644 --- a/tools/commitment_usd.go +++ b/tools/commitment_usd.go @@ -9,24 +9,47 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/ec2" ) // committedUSD returns the total USD a purchase commits over the full term. // Anything it cannot price with confidence is an error, never zero. -func committedUSD(ctx context.Context, client provider.ServiceClient, rec common.Recommendation) (float64, error) { +// +// offeringClass is the EC2 RI offering class the purchase will use. The +// provider's GetOfferingDetails always prices the convertible offering, so a +// standard EC2 purchase is priced through classPricer instead, and refused if +// the client cannot do that (never priced as convertible). +func committedUSD(ctx context.Context, client provider.ServiceClient, rec common.Recommendation, offeringClass string) (float64, error) { switch { case rec.CommitmentType == common.CommitmentSavingsPlan: return savingsPlanUSD(rec) case rec.Provider == common.ProviderGCP || rec.CommitmentType == common.CommitmentCUD: return 0, errors.New("refusing real purchase: USD totals for GCP committed-use discounts are unavailable at the pinned cloud-commitments-go (go#321), so real GCP CUD purchases are refused") } - od, err := client.GetOfferingDetails(ctx, rec) + var od *common.OfferingDetails + var err error + if rec.Service == common.ServiceEC2 && offeringClass == string(OfferingClassStandard) { + pricer, ok := client.(classPricer) + if !ok { + return 0, errors.New("refusing real purchase: cannot determine USD total: the EC2 client cannot price a standard offering class (GetOfferingDetailsForClass missing, cloud-commitments-go#574), so a standard purchase is refused rather than priced as convertible") + } + od, err = pricer.GetOfferingDetailsForClass(ctx, rec, offeringClass) + } else { + od, err = client.GetOfferingDetails(ctx, rec) + } if err != nil { return 0, fmt.Errorf("refusing real purchase: cannot determine USD total: %w", err) } return unitOfferingUSD(od, rec.Count) } +// classPricer is implemented by (*ec2.Client) since cloud-commitments-go#574. +type classPricer interface { + GetOfferingDetailsForClass(ctx context.Context, rec common.Recommendation, offeringClass string) (*common.OfferingDetails, error) +} + +var _ classPricer = (*ec2.Client)(nil) + // savingsPlanUSD is hourly commitment x hours x years, from the typed details // that the purchase itself uses (never the caller-visible CommitmentCost). func savingsPlanUSD(rec common.Recommendation) (float64, error) { @@ -71,11 +94,11 @@ func finiteUSD(total float64) (float64, error) { // enforceUSDCap refuses a purchase whose total exceeds limit (equal is // allowed) and returns the total otherwise. -func enforceUSDCap(ctx context.Context, client provider.ServiceClient, rec common.Recommendation, limit float64) (float64, error) { +func enforceUSDCap(ctx context.Context, client provider.ServiceClient, rec common.Recommendation, offeringClass string, limit float64) (float64, error) { if math.IsNaN(limit) || math.IsInf(limit, 0) || limit <= 0 { return 0, fmt.Errorf("refusing real purchase: %s limit %v must be a finite number above 0", EnvMaxCommitmentUSD, limit) } - total, err := committedUSD(ctx, client, rec) + total, err := committedUSD(ctx, client, rec, offeringClass) if err != nil { return 0, err } diff --git a/tools/commitment_usd_class_test.go b/tools/commitment_usd_class_test.go new file mode 100644 index 0000000..d6b94ad --- /dev/null +++ b/tools/commitment_usd_class_test.go @@ -0,0 +1,109 @@ +package tools + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" +) + +const ( + convertibleUnitUSD = 900.0 + standardUnitUSD = 400.0 +) + +// classPricedClient mirrors (*ec2.Client): GetOfferingDetails prices the +// convertible offering, GetOfferingDetailsForClass the requested class. +type classPricedClient struct { + *fakeServiceClient + classCalls []string +} + +func newClassPricedClient() *classPricedClient { + return &classPricedClient{fakeServiceClient: &fakeServiceClient{ + purchaseResult: common.PurchaseResult{Success: true, CommitmentID: "ri-abc"}, + offering: &common.OfferingDetails{TotalCost: convertibleUnitUSD, Currency: "USD"}, + }} +} + +func (c *classPricedClient) GetOfferingDetailsForClass(_ context.Context, _ common.Recommendation, class string) (*common.OfferingDetails, error) { + c.classCalls = append(c.classCalls, class) + if class == string(OfferingClassStandard) { + return &common.OfferingDetails{TotalCost: standardUnitUSD, Currency: "USD"}, nil + } + return &common.OfferingDetails{TotalCost: convertibleUnitUSD, Currency: "USD"}, nil +} + +func purchaseWithClass(client provider.ServiceClient, class string) error { + rec := testRecommendation() + rec.Count = 1 + _, err := ExecutePurchase(context.Background(), PurchaseRequest{ + Region: rec.Region, Recommendation: rec, Confirm: true, CredentialScope: "scope", + OfferingClass: class, + ResolveClient: func(context.Context) (provider.ServiceClient, error) { return client, nil }, + }) + return err +} + +func TestStandardClassIsCappedAndBudgetedAtStandardPrice(t *testing.T) { + resetBudget(t, "1000") + t.Setenv(EnvMaxCommitmentUSD, "500") // between standard (400) and convertible (900) + c := newClassPricedClient() + + require.NoError(t, purchaseWithClass(c, "standard")) + assert.Equal(t, 1, c.purchaseCalls) + assert.Equal(t, []string{"standard"}, c.classCalls) + assert.Zero(t, c.offeringCalls, "convertible pricing must not be consulted") + assert.InDelta(t, standardUnitUSD, processBudget.committedUSD(), 1e-9) +} + +func TestStandardClassCapRefusalReportsStandardPrice(t *testing.T) { + resetBudget(t, "1000") + t.Setenv(EnvMaxCommitmentUSD, "399") + c := newClassPricedClient() + + err := purchaseWithClass(c, "standard") + require.ErrorContains(t, err, "400.00") + assert.Zero(t, c.purchaseCalls) +} + +func TestStandardClassWithoutClassPricingIsRefused(t *testing.T) { + resetBudget(t, "1000") + c := &fakeServiceClient{purchaseResult: common.PurchaseResult{Success: true, CommitmentID: "ri-abc"}} + + err := purchaseWithClass(c, "standard") + require.ErrorContains(t, err, "GetOfferingDetailsForClass") + assert.Zero(t, c.purchaseCalls, "must be refused before any purchase") + assert.Zero(t, c.offeringCalls, "must not fall back to convertible pricing") + assert.Zero(t, processBudget.committedUSD()) +} + +func TestConvertibleAndDefaultClassKeepConvertiblePricing(t *testing.T) { + for _, class := range []string{"", "convertible"} { + t.Run("class="+class, func(t *testing.T) { + resetBudget(t, "1000") + c := newClassPricedClient() + + require.NoError(t, purchaseWithClass(c, class)) + assert.Empty(t, c.classCalls) + assert.Equal(t, 1, c.offeringCalls) + assert.InDelta(t, convertibleUnitUSD, processBudget.committedUSD(), 1e-9) + }) + } +} + +func TestStandardClassOnNonEC2StaysOnGenericPricing(t *testing.T) { + c := newClassPricedClient() + rec := testRecommendation() + rec.Service = common.ServiceRDS + rec.Count = 1 + + got, err := committedUSD(context.Background(), c, rec, "standard") + require.NoError(t, err) + assert.InDelta(t, convertibleUnitUSD, got, 1e-9) + assert.Empty(t, c.classCalls) +} diff --git a/tools/commitment_usd_test.go b/tools/commitment_usd_test.go index dda6ee0..4ee7c42 100644 --- a/tools/commitment_usd_test.go +++ b/tools/commitment_usd_test.go @@ -44,7 +44,7 @@ func TestCommittedUSDPerProvider(t *testing.T) { t.Run("aws-"+string(svc), func(t *testing.T) { c := &offeringClient{od: usdOffering(1234.5)} rec := common.Recommendation{Provider: common.ProviderAWS, Service: svc, CommitmentType: common.CommitmentReservedInstance, Count: 3} - got, err := committedUSD(context.Background(), c, rec) + got, err := committedUSD(context.Background(), c, rec, "") require.NoError(t, err) assert.InDelta(t, 3703.5, got, 1e-9) assert.Equal(t, 1, c.calls) @@ -53,7 +53,7 @@ func TestCommittedUSDPerProvider(t *testing.T) { t.Run("azure-compute", func(t *testing.T) { c := &offeringClient{od: usdOffering(500)} rec := common.Recommendation{Provider: common.ProviderAzure, Service: common.ServiceCompute, CommitmentType: common.CommitmentReservedInstance, Count: 4} - got, err := committedUSD(context.Background(), c, rec) + got, err := committedUSD(context.Background(), c, rec, "") require.NoError(t, err) assert.InDelta(t, 2000, got, 1e-9) }) @@ -63,7 +63,7 @@ func TestCommittedUSDPerProvider(t *testing.T) { }{{"1yr", 1}, {"3yr", 3}} { t.Run("savings-plan-"+tc.term, func(t *testing.T) { c := &offeringClient{} - got, err := committedUSD(context.Background(), c, usdSPRec(tc.term, &common.SavingsPlanDetails{HourlyCommitment: 2.5})) + got, err := committedUSD(context.Background(), c, usdSPRec(tc.term, &common.SavingsPlanDetails{HourlyCommitment: 2.5}), "") require.NoError(t, err) assert.InDelta(t, 2.5*8760*tc.years, got, 1e-9) assert.Zero(t, c.calls) @@ -105,7 +105,7 @@ func TestCommittedUSDFailsClosed(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { c := &offeringClient{od: tc.od, err: tc.err} - got, err := committedUSD(context.Background(), c, tc.rec) + got, err := committedUSD(context.Background(), c, tc.rec, "") require.Error(t, err) assert.Zero(t, got) assert.Equal(t, tc.wantCalls, c.calls) @@ -114,19 +114,19 @@ func TestCommittedUSDFailsClosed(t *testing.T) { t.Run("padded lowercase usd accepted", func(t *testing.T) { c := &offeringClient{od: &common.OfferingDetails{TotalCost: 10, Currency: " usd "}} - got, err := committedUSD(context.Background(), c, aws) + got, err := committedUSD(context.Background(), c, aws, "") require.NoError(t, err) assert.InDelta(t, 20, got, 1e-9) }) t.Run("lowercase usd accepted", func(t *testing.T) { c := &offeringClient{od: &common.OfferingDetails{TotalCost: 10, Currency: "usd"}} - got, err := committedUSD(context.Background(), c, aws) + got, err := committedUSD(context.Background(), c, aws, "") require.NoError(t, err) assert.InDelta(t, 20, got, 1e-9) }) t.Run("offering error is wrapped", func(t *testing.T) { cause := errors.New("boom") - _, err := committedUSD(context.Background(), &offeringClient{err: cause}, aws) + _, err := committedUSD(context.Background(), &offeringClient{err: cause}, aws, "") require.ErrorIs(t, err, cause) assert.Contains(t, err.Error(), "cannot determine USD total") }) @@ -136,17 +136,17 @@ func TestUSDCapBoundary(t *testing.T) { rec := common.Recommendation{Provider: common.ProviderAWS, Service: common.ServiceEC2, Count: 2} c := &offeringClient{od: usdOffering(500)} - got, err := enforceUSDCap(context.Background(), c, rec, 1000) + got, err := enforceUSDCap(context.Background(), c, rec, "", 1000) require.NoError(t, err) assert.InDelta(t, 1000, got, 1e-9) - _, err = enforceUSDCap(context.Background(), c, rec, 999.99) + _, err = enforceUSDCap(context.Background(), c, rec, "", 999.99) require.Error(t, err) assert.Contains(t, err.Error(), "CUDLY_MCP_MAX_COMMITMENT_USD") assert.Contains(t, err.Error(), "1000.00") assert.Contains(t, err.Error(), "999.99") - _, err = enforceUSDCap(context.Background(), &offeringClient{err: errors.New("x")}, rec, 1e12) + _, err = enforceUSDCap(context.Background(), &offeringClient{err: errors.New("x")}, rec, "", 1e12) require.Error(t, err) } @@ -154,7 +154,7 @@ func TestUSDCapInvalidLimit(t *testing.T) { rec := common.Recommendation{Provider: common.ProviderAWS, Service: common.ServiceEC2, Count: 1} for _, limit := range []float64{math.NaN(), math.Inf(1), 0, -1} { c := &offeringClient{od: usdOffering(1)} - _, err := enforceUSDCap(context.Background(), c, rec, limit) + _, err := enforceUSDCap(context.Background(), c, rec, "", limit) require.Error(t, err) assert.Contains(t, err.Error(), "CUDLY_MCP_MAX_COMMITMENT_USD") assert.Zero(t, c.calls) diff --git a/tools/gcp_computeengine_cud_safeguards_test.go b/tools/gcp_computeengine_cud_safeguards_test.go index b34f1ad..12bbc01 100644 --- a/tools/gcp_computeengine_cud_safeguards_test.go +++ b/tools/gcp_computeengine_cud_safeguards_test.go @@ -73,7 +73,7 @@ func (o *fakeGCPCommitmentsOperation) Wait(_ context.Context, _ ...gax.CallOptio func TestGCPComputeEnginePurchaseReportsExplicitZeroCost(t *testing.T) { ctx := context.Background() - client, err := computeengine.NewClient(ctx, "test-project", "us-central1") + client, err := computeengine.NewClient("test-project", "us-central1") require.NoError(t, err) fake := &fakeGCPCommitmentsService{} client.SetCommitmentsService(fake) @@ -122,7 +122,7 @@ func TestGCPComputeEnginePurchaseReportsExplicitZeroCost(t *testing.T) { func TestGCPComputeEngineDedupeDefersFamilyWithRecentCUD(t *testing.T) { ctx := context.Background() - client, err := computeengine.NewClient(ctx, "test-project", "us-central1") + client, err := computeengine.NewClient("test-project", "us-central1") require.NoError(t, err) // A well-formed recent ACTIVE CUD: non-nil name, a known commitment type diff --git a/tools/purchase.go b/tools/purchase.go index 59cda0e..ec756a0 100644 --- a/tools/purchase.go +++ b/tools/purchase.go @@ -362,7 +362,7 @@ func preflightRealPurchase(ctx context.Context, req PurchaseRequest, rec common. if err != nil { return nil, nil, fmt.Errorf("resolve %s service client: %w", rec.Provider, err) } - usd, err := enforceUSDCap(ctx, client, rec, limits.usd) + usd, err := enforceUSDCap(ctx, client, rec, req.OfferingClass, limits.usd) if err != nil { return nil, nil, err } @@ -423,10 +423,17 @@ func ExecutePurchase(ctx context.Context, req PurchaseRequest) (*PurchaseRespons } token := common.DeriveIdempotencyToken(idempotencyKeyFor(req.Region, rec, req.CredentialScope, req.Nonce, req.OfferingClass), 0) + requestedAt, err := firstAttemptTime(token, rec.Service) + if err != nil { + reservation.settle(false) + recordPurchaseRefusal(req, rec, err) + return nil, err + } opts := common.PurchaseOptions{ Source: common.PurchaseSourceMCP, IdempotencyToken: token, OfferingClass: req.OfferingClass, + RequestedAt: requestedAt, } return purchaseAndSettle(ctx, req, client, opts, reservation) diff --git a/tools/purchase_test.go b/tools/purchase_test.go index 1597508..23e58ea 100644 --- a/tools/purchase_test.go +++ b/tools/purchase_test.go @@ -78,8 +78,8 @@ type fakeServiceClient struct { purchaseErr error offeringErr error offering *common.OfferingDetails - purchaseResult common.PurchaseResult lastOpts common.PurchaseOptions + purchaseResult common.PurchaseResult purchaseCalls int offeringCalls int } diff --git a/tools/search_dedupe_test.go b/tools/search_dedupe_test.go index ab83249..f2e08d3 100644 --- a/tools/search_dedupe_test.go +++ b/tools/search_dedupe_test.go @@ -340,8 +340,7 @@ func TestDedupeMemoryDBIsFlagOnly(t *testing.T) { // GCP is flag-only, and the provider's own family filter must still be // reached through the memoizing client. func TestDedupeGCPForwardsTheFamilyHookAndOnlyFlags(t *testing.T) { - ctx := context.Background() - client, err := computeengine.NewClient(ctx, "test-project", "us-central1") + client, err := computeengine.NewClient("test-project", "us-central1") require.NoError(t, err) client.SetCommitmentsService(&fakeGCPCommitmentsService{commitments: []*computepb.Commitment{{ Name: proto.String("existing-n2-cud"),