From edff79bb2eeb5d1aea32ab669cf470605b45461d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 22:50:54 +0100 Subject: [PATCH 1/3] fix(purchase): refuse a GCP CUD when one for the same project, region and family started in the last 24 hours Before PurchaseCommitment for a GCP Compute Engine recommendation, the executor lists the project's live commitments and runs the library's duplicate check (recfilter.NewDuplicateChecker with the computeengine recent-commitment filter). If a commitment for the same project, region and machine family started in the last 24 hours, the rec is refused before any purchase call, with a fixed note. If the listing fails, the rec is refused too (fail closed, owner decision on #452). This execution's own commitments, identified by their deterministic names ("cud-" + token[:32], mirrored from the go client and pinned by a parity test against it), are not counted, so a lost-response retry still adopts its commitment. Both refusals carry the beforeSubmission class, so with #1842 a row whose every rec was refused records no_submission. Both notes are in the stored-error allowlist. Part of #682 Co-Authored-By: Claude Opus 5.5 --- internal/purchase/errnotes.go | 2 + internal/purchase/errnotes_test.go | 4 +- internal/purchase/execution.go | 23 +- internal/purchase/gcp_recent_check.go | 131 ++++++++ internal/purchase/gcp_recent_check_test.go | 365 +++++++++++++++++++++ internal/purchase/late_accounts_test.go | 2 +- internal/purchase/manager_test.go | 3 + internal/purchase/plan_step_proofs_test.go | 5 + internal/purchase/requested_at_test.go | 4 +- internal/purchase/zonal_az_test.go | 4 +- 10 files changed, 531 insertions(+), 12 deletions(-) create mode 100644 internal/purchase/gcp_recent_check.go create mode 100644 internal/purchase/gcp_recent_check_test.go diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index fe1a09f1f..dadb069b6 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -39,6 +39,8 @@ var serverNotePatterns = []string{ regexp.QuoteMeta(redshiftHorizonRefusal), regexp.QuoteMeta(redshiftNoStampRefusal), regexp.QuoteMeta(redshiftRecentBuyRefusal), + regexp.QuoteMeta(gcpDuplicateCheckFailedNote), + regexp.QuoteMeta(gcpRecentCommitmentNote), `EC2 recommendation for ` + resourceTypePattern + ` has zonal scope but no AvailabilityZone: refusing to buy a zonal RI in an arbitrary AZ; re-fetch the recommendation`, `zonal RI offering needs an availability zone; re-fetch the recommendation`, `email notifier not configured for this deployment`, diff --git a/internal/purchase/errnotes_test.go b/internal/purchase/errnotes_test.go index 9e40c15fd..d7014ecd8 100644 --- a/internal/purchase/errnotes_test.go +++ b/internal/purchase/errnotes_test.go @@ -57,6 +57,8 @@ func TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim(t *testing.T) { redshiftHorizonRefusal, redshiftNoStampRefusal, redshiftRecentBuyRefusal, + gcpDuplicateCheckFailedNote, + gcpRecentCommitmentNote, "EC2 recommendation for m5.large has zonal scope but no AvailabilityZone: refusing to buy a zonal RI in an arbitrary AZ; re-fetch the recommendation", "zonal RI offering needs an availability zone; re-fetch the recommendation", "email notifier not configured for this deployment", @@ -209,7 +211,7 @@ func TestSanitizeStoredError_ResolveNoteExchangeIDIsClosed(t *testing.T) { // The Redshift gate texts are fixed: an appended execution id or raw text must // not pass verbatim. func TestSanitizeStoredError_RedshiftGateRefusalsAreClosed(t *testing.T) { - for _, text := range []string{redshiftConflictRefusal, redshiftHorizonRefusal, redshiftNoStampRefusal, redshiftRecentBuyRefusal} { + for _, text := range []string{redshiftConflictRefusal, redshiftHorizonRefusal, redshiftNoStampRefusal, redshiftRecentBuyRefusal, gcpDuplicateCheckFailedNote, gcpRecentCommitmentNote} { assert.Equal(t, text, SanitizeStoredError(text)) for _, variant := range []string{ text + " (execution 00000000-0000-0000-0000-000000000000, running)", diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 78c77e954..3dc4a3241 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -781,6 +781,7 @@ func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *conf // Build the list of selected indices once so the fan-out closure only // has to look up rec[i] (no second pass over the full slice). selected := selectedIndices(exec.Recommendations) + ownGCPNames := gcpOwnCommitmentNames(exec, selected) if len(selected) == 0 { logging.Infof("purchase[%s]: no selected recommendations, nothing to execute (account=%s plan=%q)", exec.ExecutionID, accountID, plan.Name) @@ -833,7 +834,7 @@ func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *conf recOpts := opts recOpts.IdempotencyToken = common.DeriveIdempotencyToken(idempotencyLineageKey(exec), i) recOpts.AttemptJournal = m.attemptJournal - purchaseResult, err := m.executeSinglePurchase(ctx, rec, provCfg, recOpts) + purchaseResult, err := m.executeSinglePurchase(ctx, rec, provCfg, recOpts, ownGCPNames) return recPurchaseOutcome{index: i, purchase: purchaseResult, err: err}, nil }, getMaxAccountParallelism()) @@ -1247,7 +1248,7 @@ func logRecCtxErr(executionID, recTuple string, elapsed time.Duration, recCtxErr // provCfg carries optional per-account credentials; pass nil to use ambient credentials. // opts carries execution-level metadata (the source surface) that providers stamp // onto the commitment they create. -func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.RecommendationRecord, provCfg *provider.Config, opts common.PurchaseOptions) (common.PurchaseResult, error) { +func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.RecommendationRecord, provCfg *provider.Config, opts common.PurchaseOptions, ownGCPNames map[string]bool) (common.PurchaseResult, error) { // Per-purchase Info logs tagged with the owning execution ID so a // CloudWatch filter on the execution UUID surfaces every step of the // purchase attempt -- provider construction, service-client lookup, @@ -1345,13 +1346,11 @@ func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.Recommen // CloudWatch shows whether a failure came from the SDK call itself // (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 bounds its DescribeReservedNodes match window by RequestedAt, which must - // be the lineage's first dispatch time, never this call's. - opts, err = withDispatchTime(rec, opts) + opts, err = preSubmitChecks(recCtx, serviceClient, rec, recommendation, opts, ownGCPNames) if err != nil { return common.PurchaseResult{}, err } + tCall := time.Now() result, err := serviceClient.PurchaseCommitment(recCtx, recommendation, opts) elapsed := time.Since(tCall) if err != nil { @@ -1381,6 +1380,18 @@ func (m *Manager) executeSinglePurchase(ctx context.Context, rec config.Recommen return result, nil } +// preSubmitChecks runs what must hold before PurchaseCommitment: the GCP +// recent-commitment refusal (platform#682, a beforeSubmission error) and go's +// Redshift dispatch time, which bounds its DescribeReservedNodes match window +// by RequestedAt and must be the lineage's first dispatch time, never this +// call's. +func preSubmitChecks(ctx context.Context, client provider.ServiceClient, rec config.RecommendationRecord, recommendation common.Recommendation, opts common.PurchaseOptions, ownGCPNames map[string]bool) (common.PurchaseOptions, error) { + if err := gcpRecentCommitmentRefusal(ctx, client, recommendation, ownGCPNames); err != nil { + return opts, err + } + return withDispatchTime(rec, opts) +} + // purchaseCallError wraps a failed PurchaseCommitment. When the run context // ended mid-call the request may have landed, so the error also carries // ErrOutcomeUnknown: the row records an unknown outcome, never a plain failure. diff --git a/internal/purchase/gcp_recent_check.go b/internal/purchase/gcp_recent_check.go new file mode 100644 index 000000000..fe6be1b4f --- /dev/null +++ b/internal/purchase/gcp_recent_check.go @@ -0,0 +1,131 @@ +package purchase + +import ( + "context" + "errors" + "fmt" + "slices" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + "github.com/LeanerCloud/cloud-commitments-go/pkg/recfilter" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// Fixed refusal notes (platform#682). Both are stored verbatim and are on the +// stored-error allowlist (errnotes.go), so neither may carry provider text. +const ( + gcpDuplicateCheckFailedNote = "GCP duplicate check could not list existing commitments; no purchase was submitted" + gcpRecentCommitmentNote = "a GCP commitment for this project, region and machine family started in the last 24 hours; no purchase was submitted, to avoid a duplicate" +) + +// gcpCommitmentNameTokenLen mirrors computeengine idempotentNameTokenLen +// (providers/gcp/services/computeengine/client.go:1411). +const gcpCommitmentNameTokenLen = 32 + +// gcpCommitmentName mirrors computeengine idempotentCommitmentName +// (client.go:1423-1434) for a lowercase hex token: "cud-" + token[:32]. The +// parity test drives the real client, so a rule change there fails it. +func gcpCommitmentName(token string) string { + if len(token) > gcpCommitmentNameTokenLen { + token = token[:gcpCommitmentNameTokenLen] + } + return "cud-" + token +} + +func isGCPCompute(providerName string, service common.ServiceType) bool { + return common.ProviderType(providerName) == common.ProviderGCP && service == common.ServiceCompute +} + +// gcpOwnCommitmentNames returns the commitment names this execution's selected +// GCP compute recs will create, built from the same per-rec tokens the purchase +// path passes to the provider. A listed commitment with one of these names was +// created by this lineage (a lost-response re-drive, or an earlier rec of the +// same run), so it is not a duplicate of the rec being bought. +func gcpOwnCommitmentNames(exec *config.PurchaseExecution, selected []int) map[string]bool { + names := make(map[string]bool) + for _, i := range selected { + rec := exec.Recommendations[i] + service, _ := mapServiceSlug(rec.Service) + if !isGCPCompute(rec.Provider, service) { + continue + } + names[gcpCommitmentName(common.DeriveIdempotencyToken(idempotencyLineageKey(exec), i))] = true + } + return names +} + +// recentCommitmentFilter is the optional GCP filter recfilter asserts on the +// client it lists through (pkg/recfilter/dedupe.go:16-18, :71). +type recentCommitmentFilter interface { + FilterRecommendationsForRecentCommitments(recs []common.Recommendation, existing []common.Commitment) (passed, filtered []common.Recommendation, err error) +} + +// gcpRecentDedupeClient lists without this execution's own commitments and +// forwards the GCP filter. Embedding the interface does not promote +// FilterRecommendationsForRecentCommitments, and without it recfilter would +// fall back to its generic map path, which does not understand CUDs. +type gcpRecentDedupeClient struct { + provider.ServiceClient + own map[string]bool +} + +func (c gcpRecentDedupeClient) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { + all, err := c.ServiceClient.GetExistingCommitments(ctx) + if err != nil { + return nil, err + } + return slices.DeleteFunc(slices.Clone(all), func(cm common.Commitment) bool { return c.own[cm.CommitmentID] }), nil +} + +// FilterRecommendationsForRecentCommitments stamps the project onto the recs +// from the listed commitments (each carries Account = the client's project) and +// delegates. The platform does not know the project in ADC mode, so the rec +// cannot carry it itself. +func (c gcpRecentDedupeClient) FilterRecommendationsForRecentCommitments(recs []common.Recommendation, existing []common.Commitment) (passed, filtered []common.Recommendation, err error) { + inner, ok := c.ServiceClient.(recentCommitmentFilter) + if !ok { + return nil, nil, errors.New("GCP service client has no recent-commitment filter") + } + if len(existing) == 0 { + return nil, nil, errors.New("no recent commitments to filter against") + } + project := existing[0].Account + for i := range existing { + if existing[i].Account != project { + return nil, nil, fmt.Errorf("recent commitments span projects %q and %q", project, existing[i].Account) + } + } + stamped := slices.Clone(recs) + for i := range stamped { + stamped[i].Account = project + } + return inner.FilterRecommendationsForRecentCommitments(stamped, existing) +} + +// gcpRecentCommitmentRefusal refuses a GCP compute rec when the project already +// has a commitment of the rec's region and machine family that started in the +// last 24 hours, and fails closed when the commitments cannot be listed. It +// returns nil for every other rec. A refusal is a beforeSubmission error: the +// purchase call is never made. +func gcpRecentCommitmentRefusal(ctx context.Context, client provider.ServiceClient, rec common.Recommendation, ownNames map[string]bool) error { + if !isGCPCompute(string(rec.Provider), rec.Service) { + return nil + } + rec.CommitmentType = common.CommitmentCUD + passed, filtered, err := recfilter.NewDuplicateChecker(0).AdjustRecommendationsForExisting( + ctx, []common.Recommendation{rec}, gcpRecentDedupeClient{ServiceClient: client, own: ownNames}) + switch { + case err != nil: + logging.Errorf("GCP duplicate check for %s in %s failed: %v", rec.ResourceType, rec.Region, err) + return beforeSubmission(errors.New(gcpDuplicateCheckFailedNote)) + case len(filtered) > 0: + return beforeSubmission(errors.New(gcpRecentCommitmentNote)) + case len(passed) != 1: + logging.Errorf("GCP duplicate check for %s in %s returned %d passed recs, want 1", rec.ResourceType, rec.Region, len(passed)) + return beforeSubmission(errors.New(gcpDuplicateCheckFailedNote)) + } + return nil +} diff --git a/internal/purchase/gcp_recent_check_test.go b/internal/purchase/gcp_recent_check_test.go new file mode 100644 index 000000000..06ad09449 --- /dev/null +++ b/internal/purchase/gcp_recent_check_test.go @@ -0,0 +1,365 @@ +package purchase + +// #682 P1: a GCP compute rec is refused before PurchaseCommitment when the +// project already holds a same-region, same-family commitment that started in +// the last 24 hours, and fails closed when listing errors. The real library GCP +// client runs against a fake commitments service, so the dedupe, the filter and +// the Insert name are the pinned library's own. + +import ( + "context" + "errors" + "net/http" + "sync" + "testing" + "time" + + "cloud.google.com/go/compute/apiv1/computepb" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/services/computeengine" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "google.golang.org/api/googleapi" + "google.golang.org/api/iterator" + "google.golang.org/protobuf/proto" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +const ( + cudProject = "proj" + cudRegion = "us-central1" +) + +var ( + familyN2 = computepb.Commitment_GENERAL_PURPOSE_N2.String() + familyE2 = computepb.Commitment_GENERAL_PURPOSE_E2.String() +) + +type cudIter struct { + items []*computepb.Commitment + err error +} + +func (it *cudIter) Next() (*computepb.Commitment, error) { + if it.err != nil { + return nil, it.err + } + if len(it.items) == 0 { + return nil, iterator.Done + } + next := it.items[0] + it.items = it.items[1:] + return next, nil +} + +// fakeCUDService stores commitments like the API: Insert adds a live commitment +// and answers 409 for a name that already exists. +type fakeCUDService struct { + mu sync.Mutex + items []*computepb.Commitment + listErr error + inserted []string +} + +func (f *fakeCUDService) List(context.Context, *computepb.ListRegionCommitmentsRequest) computeengine.CommitmentsIterator { + f.mu.Lock() + defer f.mu.Unlock() + return &cudIter{items: append([]*computepb.Commitment(nil), f.items...), err: f.listErr} +} + +func (f *fakeCUDService) find(name string) *computepb.Commitment { + for _, c := range f.items { + if c.GetName() == name { + return c + } + } + return nil +} + +func (f *fakeCUDService) Insert(_ context.Context, req *computepb.InsertRegionCommitmentRequest) (computeengine.CommitmentsOperation, error) { + f.mu.Lock() + defer f.mu.Unlock() + res := req.GetCommitmentResource() + f.inserted = append(f.inserted, res.GetName()) + if f.find(res.GetName()) != nil { + return nil, &googleapi.Error{Code: http.StatusConflict} + } + stored := proto.Clone(res).(*computepb.Commitment) + stored.Status = proto.String(computepb.Commitment_ACTIVE.String()) + stored.StartTimestamp = proto.String(time.Now().UTC().Format(time.RFC3339)) + f.items = append(f.items, stored) + return fakeCommitmentOp{}, nil +} + +func (f *fakeCUDService) Get(_ context.Context, req *computepb.GetRegionCommitmentRequest) (*computepb.Commitment, error) { + f.mu.Lock() + defer f.mu.Unlock() + if c := f.find(req.GetCommitment()); c != nil { + return c, nil + } + return nil, errors.New("not found") +} + +func (f *fakeCUDService) Close() error { return nil } + +func (f *fakeCUDService) insertCount() int { + f.mu.Lock() + defer f.mu.Unlock() + return len(f.inserted) +} + +// cudFixture is a 4 vCPU / 16 GB, 12-month commitment, matching what a +// cudRec (term 1) asks Insert for. +func cudFixture(name, family string, age time.Duration) *computepb.Commitment { + return &computepb.Commitment{ + Name: proto.String(name), + Plan: proto.String(computepb.Commitment_TWELVE_MONTH.String()), + Type: proto.String(family), + Status: proto.String(computepb.Commitment_ACTIVE.String()), + StartTimestamp: proto.String(time.Now().Add(-age).UTC().Format(time.RFC3339)), + Resources: []*computepb.ResourceCommitment{ + {Type: proto.String(computepb.ResourceCommitment_VCPU.String()), Amount: proto.Int64(4)}, + {Type: proto.String(computepb.ResourceCommitment_MEMORY.String()), Amount: proto.Int64(16384)}, + }, + } +} + +func cudRec(t *testing.T, family, region string) config.RecommendationRecord { + t.Helper() + blob, err := common.MarshalServiceDetails(&common.ComputeDetails{GCPCommitmentType: family, MemoryGB: 16}) + require.NoError(t, err) + return config.RecommendationRecord{ + Provider: "gcp", Service: "compute", Region: region, ResourceType: "n2-standard-4", + Count: 4, Term: 1, Payment: "monthly", Selected: true, Details: blob, + } +} + +const cudLineage = "lineage-682" + +func cudOwnName(index int) string { + return gcpCommitmentName(common.DeriveIdempotencyToken(cudLineage, index)) +} + +// gcpRun drives processPurchaseRecommendations for the recs against svc. +func gcpRun(t *testing.T, svc *fakeCUDService, lineage string, recs ...config.RecommendationRecord) ([]string, bool, []config.RecommendationRecord) { + t.Helper() + client, err := computeengine.NewClient(cudProject, cudRegion) + require.NoError(t, err) + client.SetCommitmentsService(svc) + + store := new(MockConfigStore) + factory, prov := new(MockProviderFactory), new(MockProvider) + store.On("SavePurchaseHistory", mock.Anything, mock.Anything).Return(nil).Maybe() + factory.On("CreateAndValidateProvider", mock.Anything, "gcp", mock.Anything).Return(prov, nil) + prov.On("GetServiceClient", mock.Anything, common.ServiceCompute, mock.Anything).Return(client, nil) + m := &Manager{config: store, providerFactory: factory} + + exec := &config.PurchaseExecution{ExecutionID: "e", PlanID: "p", IdempotencyKey: lineage, Recommendations: recs} + _, _, errs, allRefused, procErr := m.processPurchaseRecommendations(context.Background(), exec, &config.PurchasePlan{Name: "P"}, cudProject, nil, "") + require.NoError(t, procErr) + return errs, allRefused, exec.Recommendations +} + +func TestGCPRecent_RefusedBeforePurchase(t *testing.T) { + cases := map[string]struct { + svc *fakeCUDService + note string + }{ + "recent same-family commitment": { + &fakeCUDService{items: []*computepb.Commitment{cudFixture("someone-else", familyN2, time.Hour)}}, + gcpRecentCommitmentNote, + }, + "listing errors": { + &fakeCUDService{listErr: errors.New("malformed commitment")}, + gcpDuplicateCheckFailedNote, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + errs, allRefused, recs := gcpRun(t, tc.svc, cudLineage, cudRec(t, familyN2, cudRegion)) + require.Len(t, errs, 1) + assert.Equal(t, tc.note, recs[0].Error, "the stored rec error is exactly the fixed note") + assert.Equal(t, tc.note, SanitizeStoredError(recs[0].Error)) + assert.Zero(t, tc.svc.insertCount(), "no Insert reached the provider") + assert.True(t, allRefused, "every rec refused before submission feeds no_submission") + assert.False(t, recs[0].Purchased) + }) + } +} + +func TestGCPRecent_Purchased(t *testing.T) { + cases := map[string]struct { + items []*computepb.Commitment + region string + }{ + "recent commitment in another family": {[]*computepb.Commitment{cudFixture("other", familyE2, time.Hour)}, cudRegion}, + "recent commitment in another region": {[]*computepb.Commitment{cudFixture("other", familyN2, time.Hour)}, "europe-west1"}, + "commitment older than 24h": {[]*computepb.Commitment{cudFixture("old", familyN2, 25*time.Hour)}, cudRegion}, + "own name after a lost response": {[]*computepb.Commitment{cudFixture(cudOwnName(0), familyN2, time.Minute)}, cudRegion}, + "no commitments, project unknown": {nil, cudRegion}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + svc := &fakeCUDService{items: tc.items} + errs, allRefused, recs := gcpRun(t, svc, cudLineage, cudRec(t, familyN2, tc.region)) + require.Empty(t, errs) + assert.False(t, allRefused) + assert.True(t, recs[0].Purchased) + assert.Equal(t, 1, svc.insertCount(), "the purchase call ran") + }) + } +} + +func TestGCPRecent_OwnNameRetryAdoptsExistingCommitment(t *testing.T) { + svc := &fakeCUDService{items: []*computepb.Commitment{cudFixture(cudOwnName(0), familyN2, time.Minute)}} + errs, _, recs := gcpRun(t, svc, cudLineage, cudRec(t, familyN2, cudRegion)) + require.Empty(t, errs) + assert.Equal(t, []string{cudOwnName(0)}, svc.inserted, "Insert hit the 409 and the library adopted the commitment") + assert.Equal(t, cudOwnName(0), recs[0].PurchaseID) +} + +func TestGCPRecent_TwoSameFamilyRecsInOneExecutionBothBought(t *testing.T) { + svc := &fakeCUDService{} + errs, allRefused, recs := gcpRun(t, svc, cudLineage, cudRec(t, familyN2, cudRegion), cudRec(t, familyN2, cudRegion)) + require.Empty(t, errs, "an earlier rec of the same run is not a duplicate of a later one") + assert.False(t, allRefused) + assert.True(t, recs[0].Purchased && recs[1].Purchased) + assert.ElementsMatch(t, []string{cudOwnName(0), cudOwnName(1)}, svc.inserted) +} + +func TestGCPRecent_FanOutSiblingsOwnTheirOwnNames(t *testing.T) { + root := "root-lineage" + recs := []config.RecommendationRecord{cudRec(t, familyN2, cudRegion)} + a := gcpOwnCommitmentNames(&config.PurchaseExecution{IdempotencyKey: root + ":acct-a", Recommendations: recs}, []int{0}) + b := gcpOwnCommitmentNames(&config.PurchaseExecution{IdempotencyKey: root + ":acct-b", Recommendations: recs}, []int{0}) + require.Len(t, a, 1) + require.Len(t, b, 1) + for name := range a { + assert.False(t, b[name], "siblings derive different names") + } + for _, acct := range []string{"acct-a", "acct-b"} { + svc := &fakeCUDService{} // each cloud account is its own project + errs, _, got := gcpRun(t, svc, root+":"+acct, recs...) + require.Empty(t, errs) + assert.True(t, got[0].Purchased) + } +} + +func TestGCPRecent_RefusedPlusBoughtIsNotAllRefused(t *testing.T) { + svc := &fakeCUDService{items: []*computepb.Commitment{cudFixture("someone-else", familyN2, time.Hour)}} + errs, allRefused, recs := gcpRun(t, svc, cudLineage, cudRec(t, familyN2, cudRegion), cudRec(t, familyE2, cudRegion)) + require.Len(t, errs, 1) + assert.False(t, allRefused) + assert.False(t, recs[0].Purchased) + assert.True(t, recs[1].Purchased) + assert.Equal(t, []string{cudOwnName(1)}, svc.inserted) +} + +func TestGCPOwnCommitmentNames_OnlySelectedGCPComputeRecs(t *testing.T) { + gcp := cudRec(t, familyN2, cudRegion) + unselected := gcp + unselected.Selected = false + aws := config.RecommendationRecord{Provider: "aws", Service: "ec2", Selected: true} + otherGCP := config.RecommendationRecord{Provider: "gcp", Service: "relational-db", Selected: true} + exec := &config.PurchaseExecution{IdempotencyKey: cudLineage, Recommendations: []config.RecommendationRecord{gcp, unselected, aws, otherGCP}} + assert.Equal(t, map[string]bool{cudOwnName(0): true}, gcpOwnCommitmentNames(exec, selectedIndices(exec.Recommendations))) +} + +// The mirror must equal the name the pinned library gives Insert. +func TestGCPCommitmentName_MatchesLibraryInsertName(t *testing.T) { + svc := &fakeCUDService{} + client, err := computeengine.NewClient(cudProject, cudRegion) + require.NoError(t, err) + client.SetCommitmentsService(svc) + token := common.DeriveIdempotencyToken(cudLineage, 3) + rec := common.Recommendation{ResourceType: "n2-standard-4", Term: "1yr", Count: 4, Details: common.ComputeDetails{MemoryGB: 16}} + + _, err = client.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{IdempotencyToken: token}) + require.NoError(t, err) + require.Len(t, svc.inserted, 1) + assert.Equal(t, svc.inserted[0], gcpCommitmentName(token)) +} + +// filterSpy is a client that implements the GCP filter and records its input. +type filterSpy struct { + *MockServiceClient + calls int + gotRecs []common.Recommendation + filtered bool +} + +func (s *filterSpy) FilterRecommendationsForRecentCommitments(recs []common.Recommendation, _ []common.Commitment) (passed, filtered []common.Recommendation, err error) { + s.calls++ + s.gotRecs = recs + if s.filtered { + return nil, recs, nil + } + return recs, nil, nil +} + +func spyRecent(own bool) []common.Commitment { + id := "other" + if own { + id = cudOwnName(0) + } + return []common.Commitment{{ + Provider: common.ProviderGCP, Service: common.ServiceCompute, CommitmentType: common.CommitmentCUD, + Account: cudProject, Region: cudRegion, State: common.CommitmentStateActive, + StartDate: time.Now().Add(-time.Hour), ResourceType: familyN2, CommitmentID: id, + }} +} + +func gcpGuardRec() common.Recommendation { + return common.Recommendation{Provider: common.ProviderGCP, Service: common.ServiceCompute, Region: cudRegion, + ResourceType: "n2-standard-4", Details: &common.ComputeDetails{GCPCommitmentType: familyN2}} +} + +func TestGCPRecentRefusal_ForwardsTheGCPFilterWithTheListedProject(t *testing.T) { + inner := new(MockServiceClient) + inner.On("GetExistingCommitments", mock.Anything).Return(spyRecent(false), nil) + spy := &filterSpy{MockServiceClient: inner, filtered: true} + + err := gcpRecentCommitmentRefusal(context.Background(), spy, gcpGuardRec(), nil) + + require.Error(t, err) + assert.Equal(t, gcpRecentCommitmentNote, err.Error()) + var notSubmitted *beforeSubmissionError + assert.True(t, errors.As(err, ¬Submitted)) + require.Equal(t, 1, spy.calls, "the wrapper forwarded the provider filter") + assert.Equal(t, cudProject, spy.gotRecs[0].Account, "project stamped from the listed commitments") + assert.Equal(t, common.CommitmentCUD, spy.gotRecs[0].CommitmentType) +} + +func TestGCPRecentRefusal_DropsOwnCommitmentsBeforeFiltering(t *testing.T) { + inner := new(MockServiceClient) + inner.On("GetExistingCommitments", mock.Anything).Return(spyRecent(true), nil) + spy := &filterSpy{MockServiceClient: inner, filtered: true} + + require.NoError(t, gcpRecentCommitmentRefusal(context.Background(), spy, gcpGuardRec(), map[string]bool{cudOwnName(0): true})) + assert.Zero(t, spy.calls, "nothing recent and foreign is left, so the filter is not reached") +} + +// A client without the GCP filter must fail closed, never reach recfilter's +// generic map path. +func TestGCPRecentRefusal_ClientWithoutFilterFailsClosed(t *testing.T) { + inner := new(MockServiceClient) + inner.On("GetExistingCommitments", mock.Anything).Return(spyRecent(false), nil) + + err := gcpRecentCommitmentRefusal(context.Background(), inner, gcpGuardRec(), nil) + + require.Error(t, err) + assert.Equal(t, gcpDuplicateCheckFailedNote, err.Error()) +} + +func TestGCPRecentRefusal_OtherRecsNeverTouchTheClient(t *testing.T) { + client := new(MockServiceClient) // any call panics: no expectations registered + for _, rec := range []common.Recommendation{ + {Provider: common.ProviderAWS, Service: common.ServiceEC2}, + {Provider: common.ProviderAzure, Service: common.ServiceCompute}, + {Provider: common.ProviderGCP, Service: common.ServiceRelationalDB}, + } { + assert.NoError(t, gcpRecentCommitmentRefusal(context.Background(), client, rec, nil), "%s/%s", rec.Provider, rec.Service) + } +} diff --git a/internal/purchase/late_accounts_test.go b/internal/purchase/late_accounts_test.go index 3d333b939..9c134cdfc 100644 --- a/internal/purchase/late_accounts_test.go +++ b/internal/purchase/late_accounts_test.go @@ -266,7 +266,7 @@ func TestExecuteSinglePurchase_OnlyACutOffCallIsOutcomeUnknown(t *testing.T) { prov.On("GetServiceClient", mock.Anything, common.ServiceEC2, "us-east-1").Return(client, nil) m := &Manager{config: store, providerFactory: factory} _, err := m.executeSinglePurchase(ctx, config.RecommendationRecord{Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1}, - nil, common.PurchaseOptions{}) + nil, common.PurchaseOptions{}, nil) require.Error(t, err) assert.Equal(t, cut, errors.Is(err, common.ErrOutcomeUnknown), "cut=%v: %v", cut, err) cancel() diff --git a/internal/purchase/manager_test.go b/internal/purchase/manager_test.go index e468b12f5..f9ba224da 100644 --- a/internal/purchase/manager_test.go +++ b/internal/purchase/manager_test.go @@ -774,6 +774,9 @@ func TestManager_RecoverStrandedApprovals_GCPRedrives(t *testing.T) { t.Cleanup(func() { mockFactory.AssertExpectations(t) }) t.Cleanup(func() { mockProvider.AssertExpectations(t) }) t.Cleanup(func() { mockServiceClient.AssertExpectations(t) }) + // The GCP recent-commitment check (#682) lists live commitments first; + // none exist, so the re-drive proceeds to the purchase. + mockServiceClient.On("GetExistingCommitments", mock.Anything).Return([]common.Commitment(nil), nil) stranded := config.PurchaseExecution{ ExecutionID: "exec-gcp-stranded", diff --git a/internal/purchase/plan_step_proofs_test.go b/internal/purchase/plan_step_proofs_test.go index 8e7b9dff5..46e7e3d05 100644 --- a/internal/purchase/plan_step_proofs_test.go +++ b/internal/purchase/plan_step_proofs_test.go @@ -96,6 +96,11 @@ func tokenCapture(t *testing.T, providerName string, exec *config.PurchaseExecut store.On("SavePurchaseHistory", ctx, mock.Anything).Return(nil) factory.On("CreateAndValidateProvider", mock.Anything, providerName, mock.Anything).Return(prov, nil) prov.On("GetServiceClient", mock.Anything, mock.Anything, mock.Anything).Return(svc, nil) + if providerName == "gcp" { + // The GCP recent-commitment check (#682) lists live commitments before + // buying; none exist here, so every rec proceeds to PurchaseCommitment. + svc.On("GetExistingCommitments", mock.Anything).Return([]common.Commitment(nil), nil) + } var mu sync.Mutex tokens := map[string]string{} svc.On("PurchaseCommitment", mock.Anything, mock.AnythingOfType("common.Recommendation"), mock.AnythingOfType("common.PurchaseOptions")). diff --git a/internal/purchase/requested_at_test.go b/internal/purchase/requested_at_test.go index ebd923e8b..50e6bc349 100644 --- a/internal/purchase/requested_at_test.go +++ b/internal/purchase/requested_at_test.go @@ -128,7 +128,7 @@ func TestExecuteSinglePurchase_EmptyTokenOptsOtherwiseUntouched(t *testing.T) { in := common.PurchaseOptions{Source: common.PurchaseSourceWeb, ExecutionID: "exec-1"} before := time.Now() - _, err := m.executeSinglePurchase(context.Background(), scopedTestRec("acct-A"), nil, in) + _, err := m.executeSinglePurchase(context.Background(), scopedTestRec("acct-A"), nil, in, nil) after := time.Now() require.NoError(t, err) @@ -363,7 +363,7 @@ func TestExecuteSinglePurchase_RedshiftWithoutRequestedAtRefuses(t *testing.T) { mockProviderInst.On("GetServiceClient", mock.Anything, common.ServiceRedshift, mock.Anything).Return(mockServiceClient, nil).Maybe() m := &Manager{providerFactory: mockFactory} - _, err := m.executeSinglePurchase(context.Background(), redshiftScopedRec("acct-A"), nil, common.PurchaseOptions{ExecutionID: "e"}) + _, err := m.executeSinglePurchase(context.Background(), redshiftScopedRec("acct-A"), nil, common.PurchaseOptions{ExecutionID: "e"}, nil) require.Error(t, err) mockServiceClient.AssertNotCalled(t, "PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything) diff --git a/internal/purchase/zonal_az_test.go b/internal/purchase/zonal_az_test.go index c79a21148..264795684 100644 --- a/internal/purchase/zonal_az_test.go +++ b/internal/purchase/zonal_az_test.go @@ -49,7 +49,7 @@ func TestExecuteSinglePurchase_ZonalRecKeepsAvailabilityZone(t *testing.T) { Run(func(args mock.Arguments) { got = args.Get(1).(common.Recommendation) }). Return(common.PurchaseResult{Success: true, CommitmentID: "ri-1"}, nil) - _, err := managerServing(t, sc).executeSinglePurchase(context.Background(), zonalRecRecord(t, "us-east-1b"), nil, common.PurchaseOptions{ExecutionID: "exec-az"}) + _, err := managerServing(t, sc).executeSinglePurchase(context.Background(), zonalRecRecord(t, "us-east-1b"), nil, common.PurchaseOptions{ExecutionID: "exec-az"}, nil) require.NoError(t, err) d, ok := got.Details.(*common.ComputeDetails) @@ -81,7 +81,7 @@ func TestExecuteSinglePurchase_StaleZonalRowWithoutAZFailsClosed(t *testing.T) { c := ec2svc.NewClient(awsConfigForTest()) c.SetEC2API(api) - _, err := managerServing(t, c).executeSinglePurchase(context.Background(), zonalRecRecord(t, ""), nil, common.PurchaseOptions{ExecutionID: "exec-stale"}) + _, err := managerServing(t, c).executeSinglePurchase(context.Background(), zonalRecRecord(t, ""), nil, common.PurchaseOptions{ExecutionID: "exec-stale"}, nil) require.Error(t, err) assert.Contains(t, err.Error(), "re-fetch") assert.Zero(t, api.purchases, "no purchase call") From a96345dc4a932a8d230ab0a21e4bbd2e1ee0794d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 08:52:23 +0100 Subject: [PATCH 2/3] test(purchase): pin that recent commitments from two projects fail closed The wrapper takes the project from the listed recent commitments. If they name two projects it refuses instead of stamping the first one, which could let a duplicate in the other project through. Without this test the guard could be deleted with every test passing. Part of #682 Co-Authored-By: Claude Opus 5.5 --- internal/purchase/gcp_recent_check_test.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/internal/purchase/gcp_recent_check_test.go b/internal/purchase/gcp_recent_check_test.go index 06ad09449..3b4577a7a 100644 --- a/internal/purchase/gcp_recent_check_test.go +++ b/internal/purchase/gcp_recent_check_test.go @@ -341,6 +341,26 @@ func TestGCPRecentRefusal_DropsOwnCommitmentsBeforeFiltering(t *testing.T) { assert.Zero(t, spy.calls, "nothing recent and foreign is left, so the filter is not reached") } +// Recent commitments from two projects cannot all describe this rec's project, +// so the check fails closed instead of stamping the first project and letting +// a duplicate in the other one through. +func TestGCPRecentRefusal_CommitmentsFromTwoProjectsFailClosed(t *testing.T) { + recent := append(spyRecent(false), spyRecent(false)...) + recent[1].Account = "other-project" + recent[1].CommitmentID = "other-2" + inner := new(MockServiceClient) + inner.On("GetExistingCommitments", mock.Anything).Return(recent, nil) + spy := &filterSpy{MockServiceClient: inner, filtered: false} + + err := gcpRecentCommitmentRefusal(context.Background(), spy, gcpGuardRec(), nil) + + require.Error(t, err) + assert.Equal(t, gcpDuplicateCheckFailedNote, err.Error()) + var notSubmitted *beforeSubmissionError + assert.True(t, errors.As(err, ¬Submitted)) + assert.Zero(t, spy.calls, "the provider filter is never reached with a mixed-project list") +} + // A client without the GCP filter must fail closed, never reach recfilter's // generic map path. func TestGCPRecentRefusal_ClientWithoutFilterFailsClosed(t *testing.T) { From 903695f5c38054dd345c1cd612e92e7dcc61eb7f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 8 Nov 2026 10:35:25 +0100 Subject: [PATCH 3/3] test(purchase): pin that another lineage's recent CUD is refused, not treated as own Only this execution's deterministic names count as own commitments. A recent commitment with a valid CUDly-shaped name from another lineage (another plan step, a re-approval, or a sibling account in the same project) must refuse the rec before submission. Without this test, treating any cud- name as own went unnoticed. Part of #682 Co-Authored-By: Claude Opus 5.5 --- internal/purchase/gcp_recent_check_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/internal/purchase/gcp_recent_check_test.go b/internal/purchase/gcp_recent_check_test.go index 3b4577a7a..0fe9e0a08 100644 --- a/internal/purchase/gcp_recent_check_test.go +++ b/internal/purchase/gcp_recent_check_test.go @@ -341,6 +341,27 @@ func TestGCPRecentRefusal_DropsOwnCommitmentsBeforeFiltering(t *testing.T) { assert.Zero(t, spy.calls, "nothing recent and foreign is left, so the filter is not reached") } +// A recent CUD bought by another CUDly lineage (another plan step, a +// re-approval, or a sibling account sharing the project) carries a valid +// CUDly-shaped name that is not this execution's, so it must count as a recent +// commitment and refuse the rec, not be mistaken for an own commitment. +func TestGCPRecentRefusal_OtherLineageCUDIsRefused(t *testing.T) { + recent := spyRecent(false) + recent[0].CommitmentID = gcpCommitmentName(common.DeriveIdempotencyToken("other-lineage", 0)) + inner := new(MockServiceClient) + inner.On("GetExistingCommitments", mock.Anything).Return(recent, nil) + spy := &filterSpy{MockServiceClient: inner, filtered: true} + + err := gcpRecentCommitmentRefusal(context.Background(), spy, gcpGuardRec(), map[string]bool{cudOwnName(0): true}) + + require.Error(t, err) + assert.Equal(t, gcpRecentCommitmentNote, err.Error()) + var notSubmitted *beforeSubmissionError + assert.True(t, errors.As(err, ¬Submitted)) + require.Equal(t, 1, spy.calls, "the other lineage's CUD reached the provider filter as a recent commitment") + inner.AssertNotCalled(t, "PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything) +} + // Recent commitments from two projects cannot all describe this rec's project, // so the check fails closed instead of stamping the first project and letting // a duplicate in the other one through.