diff --git a/README.md b/README.md index 095b6e33c..fe1c544e3 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, 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. +7. **Duplicate-purchase dedup, fails closed** - every path (`--services` and `--input-csv`) subtracts commitments purchased within `--idempotency-window` (default `24h`, whole hours only; an invalid value is rejected at startup) before sizing a recommendation. 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/main.go b/cmd/main.go index 1fcde5dfa..44b3f07b9 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -62,6 +62,7 @@ type Config struct { TermYears int CoverageLookbackDays int MinCount int + IdempotencyWindowHours int MinSavingsPct float64 OverrideCount int32 MaxInstances int32 diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index 5d060706e..5770ed59e 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -203,7 +203,7 @@ func populateAccountNames(ctx context.Context, recs []common.Recommendation, acc // adjustRecsForDuplicates checks for existing RIs and adjusts recommendations to avoid duplicates. func adjustRecsForDuplicates(ctx context.Context, recs []common.Recommendation, serviceClient provider.ServiceClient) ([]common.Recommendation, error) { - duplicateChecker := NewDuplicateChecker(0) + duplicateChecker := NewDuplicateChecker(toolCfg.IdempotencyWindowHours) adjustedRecs, _, err := duplicateChecker.AdjustRecommendationsForExisting(ctx, recs, serviceClient) if err != nil { return recs, err // Return original recommendations with error @@ -598,7 +598,7 @@ func checkDuplicates( drops *common.DropSummary, ) []common.Recommendation { // Check for duplicate RIs to avoid double purchasing - duplicateChecker := NewDuplicateChecker(0) + duplicateChecker := NewDuplicateChecker(toolCfg.IdempotencyWindowHours) adjustedRecs, dedupedOut, err := duplicateChecker.AdjustRecommendationsForExistingRIs(ctx, filteredRecs, serviceClient) if err != nil { if !isDryRun { diff --git a/cmd/validators.go b/cmd/validators.go index e16588ab5..21bbbc58e 100644 --- a/cmd/validators.go +++ b/cmd/validators.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "strings" + "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/spf13/cobra" @@ -37,6 +38,26 @@ func validateFlags(cmd *cobra.Command, args []string) error { return err } + if err := validateIdempotencyWindow(); err != nil { + return err + } + + return nil +} + +// validateIdempotencyWindow parses --idempotency-window into the whole hours +// the duplicate check's lookback is measured in. Anything it cannot represent +// exactly is rejected rather than rounded, since a shorter window than asked +// for lets a duplicate purchase through. +func validateIdempotencyWindow() error { + window, err := time.ParseDuration(toolCfg.IdempotencyWindow) + if err != nil { + return fmt.Errorf("invalid idempotency-window %q: %w", toolCfg.IdempotencyWindow, err) + } + if window <= 0 || window%time.Hour != 0 { + return fmt.Errorf("invalid idempotency-window %q: must be a positive whole number of hours (e.g. 24h, 72h)", toolCfg.IdempotencyWindow) + } + toolCfg.IdempotencyWindowHours = int(window / time.Hour) return nil } diff --git a/cmd/validators_test.go b/cmd/validators_test.go index 202f862eb..c3e74ffac 100644 --- a/cmd/validators_test.go +++ b/cmd/validators_test.go @@ -1,10 +1,12 @@ package main import ( + "context" "os" "path/filepath" "strings" "testing" + "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/spf13/cobra" @@ -686,3 +688,76 @@ func TestValidateCSVModeFilterFlags(t *testing.T) { }) } } + +// TestValidateIdempotencyWindow covers #1262: the flag used to be accepted +// unparsed, so "banana" or "90m" ran silently with a hardcoded 24h window. +func TestValidateIdempotencyWindow(t *testing.T) { + tests := []struct { + window string + wantHours int + wantErr bool + }{ + {window: "24h", wantHours: 24}, + {window: "72h", wantHours: 72}, + {window: "1h", wantHours: 1}, + {window: "", wantErr: true}, + {window: "banana", wantErr: true}, + {window: "0h", wantErr: true}, + {window: "-24h", wantErr: true}, + {window: "90m", wantErr: true}, + {window: "1h30m", wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.window, func(t *testing.T) { + origCfg := toolCfg + defer func() { toolCfg = origCfg }() + toolCfg.IdempotencyWindow = tt.window + err := validateIdempotencyWindow() + if tt.wantErr { + if err == nil || !strings.Contains(err.Error(), "invalid idempotency-window") { + t.Fatalf("validateIdempotencyWindow(%q) error = %v, want invalid idempotency-window", tt.window, err) + } + return + } + if err != nil { + t.Fatalf("validateIdempotencyWindow(%q) unexpected error = %v", tt.window, err) + } + if toolCfg.IdempotencyWindowHours != tt.wantHours { + t.Errorf("IdempotencyWindowHours = %d, want %d", toolCfg.IdempotencyWindowHours, tt.wantHours) + } + }) + } +} + +// TestCheckDuplicates_HonorsIdempotencyWindow reproduces the #1262 scenario: +// a re-run 30h after a purchase with --idempotency-window 72h must subtract +// that purchase. With the window ignored the lookback stayed at 24h and the +// same 5 RIs were bought again. +func TestCheckDuplicates_HonorsIdempotencyWindow(t *testing.T) { + origCfg := toolCfg + defer func() { toolCfg = origCfg }() + toolCfg.IdempotencyWindow = "72h" + if err := validateIdempotencyWindow(); err != nil { + t.Fatalf("validateIdempotencyWindow: %v", err) + } + + ctx := context.Background() + recs := []common.Recommendation{{ + ResourceType: "db.t3.small", Region: "us-east-1", Count: 5, + Details: &common.DatabaseDetails{Engine: "mysql"}, + }} + existing := []common.Commitment{{ + ResourceType: "db.t3.small", Region: "us-east-1", Engine: "mysql", + Count: 5, State: "active", StartDate: time.Now().Add(-30 * time.Hour), + }} + mockClient := &MockServiceClient{} + mockClient.On("GetExistingCommitments", ctx).Return(existing, nil) + t.Cleanup(func() { mockClient.AssertExpectations(t) }) + + if got := checkDuplicates(ctx, recs, mockClient, false /* isDryRun */, nil); CalculateTotalInstances(got) != 0 { + t.Errorf("checkDuplicates kept %d instance(s); the 30h-old purchase is inside the 72h window and must be subtracted", CalculateTotalInstances(got)) + } + if got, err := adjustRecsForDuplicates(ctx, recs, mockClient); err != nil || CalculateTotalInstances(got) != 0 { + t.Errorf("adjustRecsForDuplicates (CSV path) kept %d instance(s), err=%v; want 0", CalculateTotalInstances(got), err) + } +} diff --git a/docs/cli/README.md b/docs/cli/README.md index 23e63fc3a..a0dee3980 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -50,7 +50,7 @@ All flags belong to the root command unless noted otherwise. | `--purchase` | | `false` | Execute real purchases. This is the only purchase control: a bare run is always a dry run, and `--purchase` alone executes real purchases (identically in cloud-fetch and `--input-csv` modes). Still gated by the `--yes` / interactive confirmation prompt. See [purchase-safety.md](purchase-safety.md). | | `--yes` | | `false` | Skip the interactive confirmation prompt. Use with caution in automation. | | `--audit-log` | | `./cudly-audit.jsonl` | Path to the JSONL audit log file. Written for every recommendation (dry-run and real). See [purchase-safety.md](purchase-safety.md). | -| `--idempotency-window` | | `24h` | Lookback window for duplicate purchase detection. Accepted as a Go duration string (not validated by the CLI; currently has no effect on CLI runs). See [purchase-safety.md](purchase-safety.md). | +| `--idempotency-window` | | `24h` | Lookback window for duplicate purchase detection. A Go duration string that must be a positive whole number of hours (e.g. `24h`, `72h`); anything else is rejected at startup. See [purchase-safety.md](purchase-safety.md). | ### Recommendation quality filters diff --git a/docs/cli/purchase-safety.md b/docs/cli/purchase-safety.md index 4652c7d5b..474ed0d20 100644 --- a/docs/cli/purchase-safety.md +++ b/docs/cli/purchase-safety.md @@ -84,16 +84,16 @@ The default path (`./cudly-audit.jsonl`) writes to the current working directory --idempotency-window string default: 24h ``` -A duplicate check runs before every purchase, on both the `--services` and `--input-csv` paths: it fetches existing commitments and subtracts anything purchased in the last 24 hours from each recommendation's count, so a retried run doesn't buy the same capacity twice. +A duplicate check runs before every purchase, on both the `--services` and `--input-csv` paths: it fetches existing commitments and subtracts anything purchased within the window from each recommendation's count, so a retried run doesn't buy the same capacity twice. -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). +The window is a Go duration string that must be a positive whole number of hours (e.g. `24h`, `48h`, `72h`). A value that doesn't parse, is zero or negative, or isn't whole hours (e.g. `90m`, `1h30m`) is rejected at startup, before any API call, rather than rounded or replaced with the default. 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. ```bash -# The dedup check always runs with a fixed 24h lookback; this flag's value is not applied: +# Subtract anything purchased in the last 72 hours: cudly --services rds --idempotency-window 72h ``` @@ -139,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 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. +6. Set `--idempotency-window` to cover the time since the earlier run (e.g. `72h` for a re-run two days later), and note 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).