diff --git a/README.md b/README.md index 581303f8c..095b6e33c 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,7 @@ The CLI depends on the published shared Go modules in [cloud-commitments-go](htt 4. **RDS extended-support filtering** - by default, recommendations for instances running an engine version in AWS Extended Support are excluded, since the surcharge can erase RI savings; pass `--include-extended-support` to include them. 5. **Audit log written per recommendation** - the audit log path is checked for writability before any cloud API call. Each recommendation then gets its own audit record: for a dry run, written as soon as its (local, no-API-call) result is generated; for a real purchase, written after that purchase call returns. 6. **Permanent CSV exports** of every dry run and every purchase. -7. **Duplicate-purchase dedup, with a caveat** - every path (`--services` and `--input-csv`) subtracts commitments purchased in the last 24 hours before sizing a recommendation. `--idempotency-window` doesn't change that fixed 24h lookback yet ([#1262](https://github.com/LeanerCloud/cloud-commitments-cli/issues/1262)), and if the existing-commitments API call itself fails, the run continues un-deduplicated with a warning ([#1941](https://github.com/LeanerCloud/cloud-commitments-cli/issues/1941)). +7. **Duplicate-purchase dedup, fails closed** - every path (`--services` and `--input-csv`) subtracts commitments purchased in the last 24 hours before sizing a recommendation. `--idempotency-window` doesn't change that fixed 24h lookback yet ([#1262](https://github.com/LeanerCloud/cloud-commitments-cli/issues/1262)). If the existing-commitments API call itself fails, a dry run continues with a warning (nothing is bought); a `--purchase` run refuses that (service, region) with a "Refusing to purchase" line and buys nothing there. Full internals: [Purchase Safety](docs/cli/purchase-safety.md). diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 6ec97eaaf..c3ed439eb 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -80,7 +80,7 @@ func fetchExistingCoverage(ctx context.Context, awsCfg aws.Config, recClient pro // instead of sizing every recommendation as if nothing is owned. func coverageFetchFailure(cfg Config, err error) error { if effectiveDryRun(cfg) { - AppLogger.Printf(" ⚠️ %v; sizing will assume zero existing coverage (dry run only — a real --purchase run aborts instead, since --target-coverage would overbuy on top of what is already owned)\n", err) + AppLogger.Printf(" ⚠️ %v; sizing will assume zero existing coverage (dry run only; a real --purchase run aborts instead, since --target-coverage would overbuy on top of what is already owned)\n", err) return nil } return err @@ -564,11 +564,10 @@ func runToolFromCSV(ctx context.Context, cfg Config) error { continue } - // Check for duplicate RIs to avoid double purchasing - adjustedRecs, err := adjustRecsForDuplicates(ctx, recs, serviceClient) - if err != nil { - AppLogger.Printf(" ⚠️ Warning: Could not check for existing RIs: %v\n", err) - adjustedRecs = recs // Continue with original recommendations if check fails + // Check for duplicate RIs to avoid double purchasing. + adjustedRecs, ok := checkDuplicatesForCSVRegion(ctx, recs, serviceClient, service, region, isDryRun) + if !ok { + continue } // Deducting existing commitments shrinks Count, which can push a // row that cleared the floor in filterAndAdjustRecommendations back @@ -611,6 +610,29 @@ func runToolFromCSV(ctx context.Context, cfg Config) error { return nil } +// checkDuplicatesForCSVRegion runs the duplicate check for a single +// (service, region) pair in CSV mode and reports whether the caller should +// still process that region (ok). The duplicate check is the only guard +// between a re-run and a double purchase, so a failed check must not fall +// back to the un-deduplicated counts on a purchase run: that would buy +// reserved capacity the account already owns. A dry run logs a loud warning +// and continues with the un-deduplicated counts (nothing is bought, so +// reporting fidelity wins); a purchase run refuses to spend and returns +// ok=false, mirroring the fail-closed behavior of checkDuplicates on the +// non-CSV pipeline. +func checkDuplicatesForCSVRegion(ctx context.Context, recs []common.Recommendation, serviceClient provider.ServiceClient, service common.ServiceType, region string, isDryRun bool) (adjustedRecs []common.Recommendation, ok bool) { + adjustedRecs, err := adjustRecsForDuplicates(ctx, recs, serviceClient) + if err == nil { + return adjustedRecs, true + } + if !isDryRun { + AppLogger.Printf(" ❌ Refusing to purchase %s/%s: could not check for existing RIs (%v). Skipping this region rather than risking a duplicate purchase.\n", getServiceDisplayName(service), region, err) + return nil, false + } + AppLogger.Printf(" ⚠️ Warning: Could not check for existing RIs: %v (dry run; continuing with un-deduplicated counts)\n", err) + return recs, true +} + // filterAndAdjustRecommendations applies filters, coverage, count override, // the --min-count floor and the run-wide --max-instances cap to // recommendations loaded from --input-csv. diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index fc605f5a9..fe45c6509 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -21,6 +21,13 @@ type EC2ClientInterface interface { DescribeRegions(ctx context.Context, params *awsec2.DescribeRegionsInput, optFns ...func(*awsec2.Options)) (*awsec2.DescribeRegionsOutput, error) } +// dropDuplicateCheckFailed is a cmd-local drop reason for recommendations +// dropped because the duplicate-purchase check itself failed (not because it +// found and subtracted an actual duplicate, which is common.DropDuplicateDedup). +// It is not a common.Drop* constant because adding one is a +// cloud-commitments-go change; this string is exclusive to this package. +const dropDuplicateCheckFailed = "duplicate-check-failed" + // formatServices formats a list of services for display. func formatServices(services []common.ServiceType) string { names := make([]string, len(services)) @@ -427,7 +434,7 @@ func processRegionRecommendations( } // Check for duplicate RIs. Drop tracking skipped (nil). - adjustedRecs := checkDuplicates(ctx, filteredRecs, serviceClient, nil) + adjustedRecs := checkDuplicates(ctx, filteredRecs, serviceClient, isDryRun, nil) // Process purchases regionResults := processPurchaseLoop(ctx, adjustedRecs, region, isDryRun, serviceClient, cfg) @@ -576,18 +583,39 @@ func applyCoverageAndOverrides(recs []common.Recommendation, cfg Config, coverag // is applied once run-wide instead, after every region has been fetched // (applyGlobalInstanceLimit in multi_service.go). // +// The duplicate check is the only guard between a re-run and a double +// purchase, so a failed check (throttling, a describe-call AccessDenied, a +// transient 5xx) must not fall back to the un-deduplicated counts on a +// purchase run: that would buy reserved capacity the account already owns. +// isDryRun selects the behavior on error: a dry run logs a loud warning and +// continues (nothing is bought, so reporting fidelity wins), while a purchase +// run refuses to spend and drops these recommendations entirely, mirroring +// the "refuse to spend rather than purchase uncapped" stance already taken +// for --max-instances in processRegionRecommendations. +// // drops accumulates per-reason drop counts for the end-of-run summary; pass nil to skip. +// +// A recommendation dropped because the check itself failed is counted under +// dropDuplicateCheckFailed, not common.DropDuplicateDedup: the latter means +// "an actual duplicate was found and subtracted", and reusing it for a failed +// check would report the failure as a successful dedup in the summary. func checkDuplicates( ctx context.Context, filteredRecs []common.Recommendation, serviceClient provider.ServiceClient, + isDryRun bool, drops *common.DropSummary, ) []common.Recommendation { // Check for duplicate RIs to avoid double purchasing duplicateChecker := NewDuplicateChecker(0) adjustedRecs, dedupedOut, err := duplicateChecker.AdjustRecommendationsForExistingRIs(ctx, filteredRecs, serviceClient) if err != nil { - AppLogger.Printf(" ⚠️ Warning: Could not check for existing RIs: %v\n", err) + if !isDryRun { + AppLogger.Printf(" ❌ Refusing to purchase %d instance(s): could not check for existing RIs (%v). Dropping these recommendations rather than risking a duplicate purchase.\n", CalculateTotalInstances(filteredRecs), err) + drops.Add(dropDuplicateCheckFailed, len(filteredRecs)) + return nil + } + AppLogger.Printf(" ⚠️ Warning: Could not check for existing RIs: %v (dry run; continuing with un-deduplicated counts)\n", err) // Continue with original filteredRecs on error; adjustedRecs is not used in this branch. } else { // Always use the adjusted recommendations (they might have different counts even if same length) @@ -663,7 +691,7 @@ func fetchAndFilterRegionRecs( // --max-instances is NOT applied here; it is enforced once run-wide by the // caller so the cap covers every service and region together. if serviceClient != nil { - recs = checkDuplicates(ctx, recs, serviceClient, drops) + recs = checkDuplicates(ctx, recs, serviceClient, effectiveDryRun(cfg), drops) } return recs diff --git a/cmd/multi_service_helpers_test.go b/cmd/multi_service_helpers_test.go index d6d447691..16728df75 100644 --- a/cmd/multi_service_helpers_test.go +++ b/cmd/multi_service_helpers_test.go @@ -461,6 +461,48 @@ func TestAdjustRecsForDuplicatesError(t *testing.T) { mockClient.AssertExpectations(t) } +// TestCheckDuplicates_ErrorOnPurchaseRun_DropsRecsRatherThanFallingBack +// reproduces #1941 at the checkDuplicates layer used by the main (non-CSV) +// pipeline: a failed duplicate check on a purchase run (isDryRun=false) must +// not fall back to the un-deduplicated input. Before the fix, the error +// branch left filteredRecs unchanged and only logged a warning, so the run +// proceeded to buy the full pre-dedup counts. +func TestCheckDuplicates_ErrorOnPurchaseRun_DropsRecsRatherThanFallingBack(t *testing.T) { + ctx := context.Background() + recs := []common.Recommendation{ + {ResourceType: "db.t3.small", Count: 5}, + } + + mockClient := &MockServiceClient{} + mockClient.On("GetExistingCommitments", ctx).Return([]common.Commitment(nil), errors.New("throttling")) + + drops := common.NewDropSummary() + result := checkDuplicates(ctx, recs, mockClient, false /* isDryRun */, drops) + + assert.Nil(t, result, "a failed duplicate check on a purchase run must drop the recommendations, not fall back to the un-deduplicated input") + assert.Equal(t, 1, drops.Total()) + mockClient.AssertExpectations(t) +} + +// TestCheckDuplicates_ErrorOnDryRun_ContinuesUnadjusted asserts the dry-run +// side of the same branch is unchanged: nothing is purchased, so a failed +// duplicate check logs a warning and keeps reporting the un-deduplicated +// counts rather than silently dropping recommendations from the dry-run report. +func TestCheckDuplicates_ErrorOnDryRun_ContinuesUnadjusted(t *testing.T) { + ctx := context.Background() + recs := []common.Recommendation{ + {ResourceType: "db.t3.small", Count: 5}, + } + + mockClient := &MockServiceClient{} + mockClient.On("GetExistingCommitments", ctx).Return([]common.Commitment(nil), errors.New("throttling")) + + result := checkDuplicates(ctx, recs, mockClient, true /* isDryRun */, nil) + + assert.Equal(t, recs, result) + mockClient.AssertExpectations(t) +} + func TestGroupRecommendationsByServiceRegion(t *testing.T) { tests := []struct { expectedGroups map[common.ServiceType]map[string]int diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index 5b8197984..e12a60208 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -1782,6 +1782,48 @@ elasticache,us-west-2,cache.t3.micro,redis,1,1yr,All Upfront,123456789012 } } +// TestRunToolFromCSV_DuplicateCheckFailureRefusesToPurchase reproduces #1941: +// on a real purchase run (--input-csv ... --purchase), a failed duplicate +// check (throttling, a describe-call AccessDenied, a transient 5xx) must not +// fall back to purchasing the un-deduplicated counts. isolateAWSEnv points +// the SDK at invalid credentials so GetExistingCommitments fails the same way +// a real AWS error would, and the region must be skipped entirely rather than +// purchased with the original (potentially duplicate) counts. Before the fix, +// adjustRecsForDuplicates' error was logged as a warning and adjustedRecs was +// reset to the original recs, so the region proceeded into the purchase loop +// unchanged. +func TestRunToolFromCSV_DuplicateCheckFailureRefusesToPurchase(t *testing.T) { + origCfg := toolCfg + defer func() { toolCfg = origCfg }() + isolateAWSEnv(t) + + csvPath := writeTestRecommendationsCSV(t, `Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Account +rds,us-east-1,db.t3.small,postgres,2,1yr,All Upfront,123456789012 +`) + reportPath := filepath.Join(t.TempDir(), "report.csv") + auditPath := filepath.Join(t.TempDir(), "audit.jsonl") + + toolCfg.CSVInput = csvPath + toolCfg.CSVOutput = reportPath + toolCfg.AuditLog = auditPath + toolCfg.ActualPurchase = true // the money path: a duplicate-check failure here must fail closed + toolCfg.Coverage = 100.0 + toolCfg.TargetCoverage = 0 + toolCfg.MaxInstances = 0 + toolCfg.OverrideCount = 0 + + err := runToolFromCSV(context.Background(), toolCfg) + require.NoError(t, err) + + // The region is skipped before executePurchase or processPurchaseLoop + // ever run, so no PurchaseResult is produced and no report is written + // (writeMultiServiceCSVReport no-ops on an empty results slice). Before + // the fix, the un-deduplicated recommendation reached the purchase loop + // and a report row (attempting a real AWS purchase call) was produced. + _, statErr := os.Stat(reportPath) + assert.True(t, os.IsNotExist(statErr), "a failed duplicate check on a purchase run must skip the region entirely, not attempt to purchase the un-deduplicated counts") +} + // TestRunToolFromCSV_NonExistentFile asserts the unreadable-input error path // surfaces as an error (previously log.Fatalf, which made it untestable). func TestRunToolFromCSV_NonExistentFile(t *testing.T) { diff --git a/docs/cli/purchase-safety.md b/docs/cli/purchase-safety.md index 21e4af7b5..9b8ac37af 100644 --- a/docs/cli/purchase-safety.md +++ b/docs/cli/purchase-safety.md @@ -88,7 +88,7 @@ A duplicate check runs before every purchase, on both the `--services` and `--in That 24-hour lookback is fixed. This flag is accepted as a Go duration string (e.g. `24h`, `48h`, `1h30m`) and stored, but its value is never read by the check - passing `--idempotency-window 72h` (or any other value) has no effect on which recommendations are purchased ([#1262](https://github.com/LeanerCloud/cloud-commitments-cli/issues/1262) tracks wiring it in). -If the existing-commitments lookup itself fails (a transient API error), the check is skipped for that batch and the run continues un-deduplicated, with a warning printed to the log rather than the run stopping ([#1941](https://github.com/LeanerCloud/cloud-commitments-cli/issues/1941)). Treat that warning as a signal to check the audit log for the run before trusting its purchase counts. +If the existing-commitments lookup itself fails (a transient API error), the two modes diverge: a dry run continues with a warning printed to the log (nothing is bought, so reporting fidelity wins), while a `--purchase` run refuses that (service, region) - it prints a "Refusing to purchase" line and buys nothing there, rather than falling back to the un-deduplicated counts ([#1941](https://github.com/LeanerCloud/cloud-commitments-cli/issues/1941)). The audit status value `skipped_covered` (idempotency hit) is defined in the audit record schema for use by the server-side scheduler path and is not emitted by this CLI's dedup check. @@ -105,6 +105,8 @@ cudly --services rds --idempotency-window 72h When using `--target-coverage`, cudly subtracts existing RI coverage from the sizing calculation so it only recommends incremental purchases. By default this subtraction treats all existing RIs as fully covering demand regardless of when they expire. +If the Cost Explorer coverage fetch fails, a dry run warns and sizes as if nothing is owned; a `--purchase` run aborts before sizing, since sizing against unknown coverage risks buying on top of what the account already owns ([#1942](https://github.com/LeanerCloud/cloud-commitments-cli/issues/1942)). + Setting `--rebuy-window-days` changes that behavior: any existing RI whose remaining term is at most this many days is treated as if it has already expired, so `--target-coverage` sizes a replacement recommendation before it actually lapses. This is useful to avoid the coverage gap that would otherwise appear between an RI expiring and a new one taking effect. ```bash @@ -137,5 +139,5 @@ Before any real purchase run: 3. If using `--target-coverage`, verify `--rebuy-window-days` is set appropriately for your RI renewal cadence. 4. Narrow the scope with `--include-regions`, `--include-accounts`, or `--min-savings-pct` before buying across all services. 5. Consider `--max-instances` as a final safety cap for a first run. -6. Note that `--idempotency-window`'s value is not applied - dedup always uses a fixed 24h lookback ([#1262](https://github.com/LeanerCloud/cloud-commitments-cli/issues/1262)) - and that a failed existing-commitments lookup lets the run proceed un-deduplicated with a warning ([#1941](https://github.com/LeanerCloud/cloud-commitments-cli/issues/1941)); watch the log for that warning and check the audit log afterward. +6. Note that `--idempotency-window`'s value is not applied - dedup always uses a fixed 24h lookback ([#1262](https://github.com/LeanerCloud/cloud-commitments-cli/issues/1262)) - and that a failed existing-commitments lookup makes a dry run proceed un-deduplicated with a warning, while a `--purchase` run refuses to purchase for that (service, region) instead ([#1941](https://github.com/LeanerCloud/cloud-commitments-cli/issues/1941)); watch the log for either signal and check the audit log afterward. 7. If an AI agent or other automation drives `cudly`, never pass `--yes` to it directly - have the agent hand off the dry-run recommendation to a human, who runs `--purchase` themselves. See [Automation and AI agents](#automation-and-ai-agents).