From af1d36a0c40a7a9afb1e6fb79db8d761ef42de50 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 23:47:02 +0200 Subject: [PATCH 01/11] feat(settings): configurable EC2 RI OfferingClass (closes #694) Add a GlobalConfig.OfferingClass field ("convertible" | "standard") that controls which EC2 Reserved Instance offering class is purchased. Empty / absent values default to "convertible" to preserve pre-694 behaviour. Backend: - pkg/common/types.go: OfferingClass field on PurchaseOptions - internal/config/types.go: OfferingClass on GlobalConfig - internal/config/store_postgres.go: read/write offering_class column ($20) - internal/purchase/execution.go: load GlobalConfig and propagate OfferingClass into PurchaseOptions before the fan-out - providers/aws/services/ec2/client.go: resolveOfferingClassType() fails loudly on unknown values (feedback_empty_string_vs_error.md); empty string maps to convertible so the DB default is safe - internal/database/postgres/migrations/000064_ec2_ri_offering_class: ADD COLUMN offering_class TEXT NOT NULL DEFAULT 'convertible' Frontend: - frontend/src/index.html: fieldset with + + + + + + +
AWS Service Defaults diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index cc64614f0..ab6498048 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -3202,6 +3202,16 @@ export async function loadGlobalSettings(): Promise { populateGraceInput('setting-grace-azure', gpMap['azure']); populateGraceInput('setting-grace-gcp', gpMap['gcp']); + // EC2 RI offering class. Absent/null from the API means "convertible" + // (backend default). Only "convertible" and "standard" are valid; any + // other value from a future API version falls back to "convertible" so + // the dropdown always has a valid selection. + const offeringClassSelect = byId('setting-ec2-offering-class'); + if (offeringClassSelect) { + const oc = data.global.offering_class; + offeringClassSelect.value = (oc === 'standard') ? 'standard' : 'convertible'; + } + // Recommendations cycle params const staleHoursInput = byId('setting-recs-stale-hours'); if (staleHoursInput) { @@ -3476,6 +3486,9 @@ export async function saveGlobalSettings(e: Event): Promise { return; } + const rawOfferingClass = byId('setting-ec2-offering-class')?.value ?? 'convertible'; + const offeringClass: 'convertible' | 'standard' = (rawOfferingClass === 'standard') ? 'standard' : 'convertible'; + const settings: api.Config = { enabled_providers: enabledProviders, notification_email: byId('setting-notification-email')?.value || '', @@ -3488,6 +3501,7 @@ export async function saveGlobalSettings(e: Event): Promise { grace_period_days: gracePeriodDays, recommendations_cache_stale_hours: rawStaleHours, recommendations_lookback_days: parseInt(byId('setting-recs-lookback-days')?.value || '7', 10), + offering_class: offeringClass, }; // Include laddering_enabled in the payload when the Purchasing panel's @@ -3625,6 +3639,9 @@ export async function resetSettings(): Promise { populateGraceInput('setting-grace-azure', 7); populateGraceInput('setting-grace-gcp', 7); + const offeringClassSelect = byId('setting-ec2-offering-class'); + if (offeringClassSelect) offeringClassSelect.value = 'convertible'; + const staleHoursInput = byId('setting-recs-stale-hours'); if (staleHoursInput) staleHoursInput.value = '24'; const lookbackSelect = byId('setting-recs-lookback-days'); diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 4e29f7e4a..2b9e51dfb 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -373,6 +373,8 @@ export interface GlobalConfig { // suppression feature. Keys: 'aws' / 'azure' / 'gcp'. Missing keys // fall back to the backend default (7). Explicit 0 = disabled. grace_period_days?: Record; + // EC2 Reserved Instance offering class. "convertible" (default) or "standard". + offering_class?: 'convertible' | 'standard'; // Age (hours) after which the recommendations cache triggers a background // stale-while-revalidate refresh. 0 disables automatic background refresh. // Valid range: 0–8760. Default: 24. diff --git a/internal/config/store_postgres.go b/internal/config/store_postgres.go index cff7b69c2..3a6e0080e 100644 --- a/internal/config/store_postgres.go +++ b/internal/config/store_postgres.go @@ -81,7 +81,8 @@ func getGlobalConfigFrom(ctx context.Context, q globalConfigExecutor) (*GlobalCo recommendations_cache_stale_hours, recommendations_lookback_days, COALESCE(purchase_delay_hours, 0), COALESCE(laddering_enabled, false), - COALESCE(ladder_execution_enabled, false) + COALESCE(ladder_execution_enabled, false), + offering_class FROM global_config WHERE id = 1 ` @@ -113,6 +114,7 @@ func getGlobalConfigFrom(ctx context.Context, q globalConfigExecutor) (*GlobalCo &config.PurchaseDelayHours, &config.LadderingEnabled, &config.LadderExecutionEnabled, + &config.OfferingClass, ) if err != nil { @@ -135,6 +137,7 @@ func getGlobalConfigFrom(ctx context.Context, q globalConfigExecutor) (*GlobalCo RecommendationsCacheStaleHours: DefaultRecommendationsCacheStaleHours, RecommendationsLookbackDays: DefaultRecommendationsLookbackDays, PurchaseDelayHours: DefaultPurchaseDelayHours, + OfferingClass: "convertible", }, nil } return nil, fmt.Errorf("failed to get global config: %w", err) @@ -211,8 +214,8 @@ func saveGlobalConfigWith(ctx context.Context, q globalConfigExecutor, config *G auto_collect, collection_schedule, notification_days_before, grace_period_days, recommendations_cache_stale_hours, recommendations_lookback_days, - purchase_delay_hours, laddering_enabled, ladder_execution_enabled - ) VALUES (1, $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22) + purchase_delay_hours, laddering_enabled, ladder_execution_enabled, offering_class + ) VALUES (1, $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20, $21, $22, $23) ON CONFLICT (id) DO UPDATE SET enabled_providers = $1, notification_email = $2, @@ -236,6 +239,7 @@ func saveGlobalConfigWith(ctx context.Context, q globalConfigExecutor, config *G purchase_delay_hours = $20, laddering_enabled = $21, ladder_execution_enabled = $22, + offering_class = $23, updated_at = NOW() ` @@ -259,7 +263,7 @@ func saveGlobalConfigWith(ctx context.Context, q globalConfigExecutor, config *G riExchangeUtilizationThreshold = 95.0 } - // Marshal GracePeriodDays → JSON text column. Empty map encodes as + // Marshal GracePeriodDays -> JSON text column. Empty map encodes as // "{}" so the DB column is never NULL and GetGlobalConfig can // treat "{}" and "" uniformly as "no explicit entries". gracePeriodJSON := "{}" @@ -271,6 +275,14 @@ func saveGlobalConfigWith(ctx context.Context, q globalConfigExecutor, config *G gracePeriodJSON = string(gpBytes) } + // Default offering_class to "convertible" when unset so the DB column + // never stores an empty string (the NOT NULL DEFAULT 'convertible' + // column handles inserts, but upserts overwrite with whatever we pass). + offeringClass := config.OfferingClass + if offeringClass == "" { + offeringClass = "convertible" + } + _, err := q.Exec(ctx, query, config.EnabledProviders, config.NotificationEmail, @@ -294,6 +306,7 @@ func saveGlobalConfigWith(ctx context.Context, q globalConfigExecutor, config *G config.PurchaseDelayHours, config.LadderingEnabled, config.LadderExecutionEnabled, + offeringClass, ) if err != nil { diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index e29285cd4..ac6259e9e 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -61,6 +61,7 @@ func TestPGXMock_GetGlobalConfig_Success(t *testing.T) { "purchase_delay_hours", "laddering_enabled", "ladder_execution_enabled", + "offering_class", } rows := pgxmock.NewRows(cols).AddRow( []string{"aws"}, strPtr("ops@example.com"), true, @@ -73,6 +74,7 @@ func TestPGXMock_GetGlobalConfig_Success(t *testing.T) { 0, false, false, + "convertible", ) mock.ExpectQuery("SELECT").WillReturnRows(rows) @@ -83,6 +85,7 @@ func TestPGXMock_GetGlobalConfig_Success(t *testing.T) { assert.Equal(t, "ops@example.com", *cfg.NotificationEmail) assert.Equal(t, 24, cfg.RecommendationsCacheStaleHours) assert.Equal(t, 7, cfg.RecommendationsLookbackDays) + assert.Equal(t, "convertible", cfg.OfferingClass) assert.NoError(t, mock.ExpectationsWereMet()) } @@ -115,6 +118,7 @@ func TestPGXMock_GetGlobalConfig_GracePeriodDays(t *testing.T) { "purchase_delay_hours", "laddering_enabled", "ladder_execution_enabled", + "offering_class", } baseRow := func(graceJSON string) []any { return []any{ @@ -128,6 +132,7 @@ func TestPGXMock_GetGlobalConfig_GracePeriodDays(t *testing.T) { 0, false, false, + "convertible", } } diff --git a/internal/config/types.go b/internal/config/types.go index 3b202f15d..a3f3d1535 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -50,7 +50,7 @@ type GlobalConfig struct { // RecommendationsLookbackDays is the AWS Cost Explorer lookback window // (days) used when fetching fresh recommendations. Must be one of 7, - // 30, or 60 — the AWS Cost Explorer LookbackPeriodInDays enum. + // 30, or 60 -- the AWS Cost Explorer LookbackPeriodInDays enum. // GCP CUD Recommender has no equivalent lookback parameter (fixed // internally); this setting applies to AWS only. // Default: 7. @@ -78,6 +78,14 @@ type GlobalConfig struct { // operator explicitly opts in. Fail-loud: wireLadderWriteSide returns // a typed ErrLadderExecutionDisabled when this is false. LadderExecutionEnabled bool `json:"ladder_execution_enabled" db:"ladder_execution_enabled"` + + // OfferingClass controls the EC2 Reserved Instance offering class used + // during purchase. Accepted values: "convertible" (default) and + // "standard". Convertible RIs can be exchanged for a different + // instance family/size/region/OS; Standard RIs are locked to the exact + // instance type for the full term but are ~5% cheaper. + // Unknown values are rejected at purchase time with an explicit error. + OfferingClass string `json:"offering_class,omitempty" dynamodbav:"offering_class,omitempty"` } // DefaultGracePeriodDays is the fallback window used when a provider diff --git a/internal/database/postgres/migrations/000078_ec2_ri_offering_class.down.sql b/internal/database/postgres/migrations/000078_ec2_ri_offering_class.down.sql new file mode 100644 index 000000000..a31499202 --- /dev/null +++ b/internal/database/postgres/migrations/000078_ec2_ri_offering_class.down.sql @@ -0,0 +1 @@ +ALTER TABLE global_config DROP COLUMN IF EXISTS offering_class; diff --git a/internal/database/postgres/migrations/000078_ec2_ri_offering_class.up.sql b/internal/database/postgres/migrations/000078_ec2_ri_offering_class.up.sql new file mode 100644 index 000000000..3bb4de059 --- /dev/null +++ b/internal/database/postgres/migrations/000078_ec2_ri_offering_class.up.sql @@ -0,0 +1,6 @@ +-- offering_class controls whether EC2 Reserved Instances are purchased as +-- Convertible (exchangeable for different families/sizes/OS; default) or +-- Standard (~5% cheaper but locked to the exact instance type for the full +-- term). An empty value is treated as "convertible" in application code. +ALTER TABLE global_config + ADD COLUMN IF NOT EXISTS offering_class TEXT NOT NULL DEFAULT 'convertible'; diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 026f586e7..851f7d607 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -513,6 +513,18 @@ func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *conf ExecutionID: exec.ExecutionID, } + // Load GlobalConfig to pick up the OfferingClass setting for EC2 RI + // purchases. A config-load failure is non-fatal: we fall back to the + // empty string which the EC2 client maps to "convertible" (pre-694 + // behaviour). The error is logged so operators can diagnose a DB + // issue without the entire purchase batch failing. + if globalCfg, err := m.config.GetGlobalConfig(ctx); err != nil { + logging.Errorf("purchase[%s]: failed to load GlobalConfig for offering class (defaulting to convertible): %v", + exec.ExecutionID, err) + } else if globalCfg != nil { + opts.OfferingClass = globalCfg.OfferingClass + } + // 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) diff --git a/pkg/common/types.go b/pkg/common/types.go index 9a494611d..4a379c43b 100644 --- a/pkg/common/types.go +++ b/pkg/common/types.go @@ -334,6 +334,12 @@ type PurchaseOptions struct { // CloudWatch / DB execution row (issue #667). The CLI purchase path has // no owning execution and leaves it empty. ExecutionID string + // OfferingClass is the EC2 Reserved Instance offering class for this + // purchase: "convertible" (exchangeable) or "standard" (locked, ~5% + // cheaper). Empty means the caller has not set one; the EC2 client + // defaults to "convertible" to preserve pre-694 behaviour. + // Only meaningful for EC2 RI purchases; ignored by other providers. + OfferingClass string } // NormalizeSource lowercases s and returns it when it matches an allowed diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 0e9af15e0..ba1816ad7 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -145,7 +145,7 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati } // Find the offering ID - offeringID, err := c.findOfferingID(ctx, rec, opts.ExecutionID) + offeringID, err := c.findOfferingID(ctx, rec, opts.ExecutionID, opts.OfferingClass) if err != nil { result.Error = fmt.Errorf("failed to find offering: %w", err) return result, result.Error @@ -374,6 +374,23 @@ type ec2OfferingQuery struct { scope string duration int64 wantOfferingType types.OfferingTypeValues + offeringClass types.OfferingClassType +} + +// resolveOfferingClassType maps the GlobalConfig string value to the AWS SDK +// type. "convertible" and "" both produce OfferingClassTypeConvertible so the +// absence of a stored value preserves the pre-694 default. Any other string +// is an explicit error: the caller must not silently fall back, because a DB +// typo would otherwise buy the wrong class (feedback_empty_string_vs_error.md). +func resolveOfferingClassType(s string) (types.OfferingClassType, error) { + switch s { + case "", "convertible": + return types.OfferingClassTypeConvertible, nil + case "standard": + return types.OfferingClassTypeStandard, nil + default: + return "", fmt.Errorf("unknown EC2 RI offering class %q: must be \"convertible\" or \"standard\"", s) + } } // buildEC2OfferingQuery resolves the typed lookup parameters from a rec, @@ -410,13 +427,17 @@ func buildEC2OfferingQuery(rec common.Recommendation, details *common.ComputeDet // typed lookup. Typed fields land on AWS's primary indices; only scope has no // typed equivalent and stays in Filters[]. func describeInputFromQuery(q ec2OfferingQuery, nextToken *string) *ec2.DescribeReservedInstancesOfferingsInput { + oc := q.offeringClass + if oc == "" { + oc = types.OfferingClassTypeConvertible + } return &ec2.DescribeReservedInstancesOfferingsInput{ InstanceType: q.instanceType, ProductDescription: q.productDesc, InstanceTenancy: q.tenancy, MinDuration: aws.Int64(q.duration), MaxDuration: aws.Int64(q.duration), - OfferingClass: types.OfferingClassTypeConvertible, + OfferingClass: oc, OfferingType: q.wantOfferingType, IncludeMarketplace: aws.Bool(false), MaxResults: aws.Int32(100), @@ -461,11 +482,18 @@ func (c *Client) buildEC2QueryFromRec(rec common.Recommendation) (ec2OfferingQue // // execID is the purchase execution UUID for log correlation; pass "" when // calling outside of a purchase flow (ValidateOffering, GetOfferingDetails). -func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, execID string) (string, error) { +// offeringClassStr is the GlobalConfig.OfferingClass value; "" is treated as +// "convertible" to preserve pre-694 behaviour. Unknown values fail loudly. +func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, execID string, offeringClassStr string) (string, error) { q, err := c.buildEC2QueryFromRec(rec) if err != nil { return "", err } + oc, err := resolveOfferingClassType(offeringClassStr) + if err != nil { + return "", fmt.Errorf("offering class config error: %w", err) + } + q.offeringClass = oc tag := execID if tag == "" { @@ -537,15 +565,19 @@ func scanEC2OfferingPage(offerings []types.ReservedInstancesOffering, wantType t return "" } -// ValidateOffering checks if an offering exists without purchasing +// ValidateOffering checks if an offering exists without purchasing. +// Uses the convertible class (empty = convertible default) since the +// validation path has no GlobalConfig context. func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation) error { - _, err := c.findOfferingID(ctx, rec, "") + _, err := c.findOfferingID(ctx, rec, "", "") return err } -// GetOfferingDetails retrieves offering details +// GetOfferingDetails retrieves offering details. +// Uses the convertible class (empty = convertible default) since the +// details-fetch path has no GlobalConfig context. func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) { - offeringID, err := c.findOfferingID(ctx, rec, "") + offeringID, err := c.findOfferingID(ctx, rec, "", "") if err != nil { return nil, err } diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index ae621601b..39a63e95e 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -610,7 +610,7 @@ func TestFindOfferingID_PaginationCapFires(t *testing.T) { }, nil).Once() } - _, err := client.findOfferingID(context.Background(), rec, "") + _, err := client.findOfferingID(context.Background(), rec, "", "") if assert.Error(t, err) { assert.Contains(t, err.Error(), "pagination cap reached") @@ -653,7 +653,7 @@ func TestFindOfferingID_WrongVariantRejected(t *testing.T) { }, }, nil).Once() - _, err := client.findOfferingID(context.Background(), rec, "") + _, err := client.findOfferingID(context.Background(), rec, "", "") if assert.Error(t, err) { assert.Contains(t, err.Error(), "no offerings found") @@ -691,7 +691,7 @@ func TestFindOfferingID_HappyPath(t *testing.T) { }, }, nil).Once() - id, err := client.findOfferingID(context.Background(), rec, "") + id, err := client.findOfferingID(context.Background(), rec, "", "") assert.NoError(t, err) assert.Equal(t, "offering-ok", id) @@ -1072,3 +1072,68 @@ func TestClient_CancelMarketplaceListing_APIError(t *testing.T) { assert.Contains(t, err.Error(), "CancelReservedInstancesListing failed") mockEC2.AssertExpectations(t) } + +func TestResolveOfferingClassType(t *testing.T) { + t.Parallel() + tests := []struct { + input string + want types.OfferingClassType + wantErr bool + }{ + {"convertible", types.OfferingClassTypeConvertible, false}, + {"", types.OfferingClassTypeConvertible, false}, // empty = default = convertible + {"standard", types.OfferingClassTypeStandard, false}, + {"STANDARD", "", true}, // case-sensitive + {"unknown", "", true}, // unknown value must error + {"Convertible", "", true}, // wrong case must error + } + for _, tc := range tests { + tc := tc + t.Run(tc.input, func(t *testing.T) { + t.Parallel() + got, err := resolveOfferingClassType(tc.input) + if tc.wantErr { + assert.Error(t, err, "expected error for input %q", tc.input) + } else { + assert.NoError(t, err) + assert.Equal(t, tc.want, got) + } + }) + } +} + +func TestDescribeInputFromQuery_OfferingClass(t *testing.T) { + t.Parallel() + base := ec2OfferingQuery{ + instanceType: types.InstanceTypeT3Micro, + productDesc: types.RIProductDescriptionLinuxUnix, + tenancy: types.TenancyDefault, + scope: string(types.ScopeRegional), + duration: 94608000, + wantOfferingType: types.OfferingTypeValuesNoUpfront, + } + + t.Run("convertible by default (empty field)", func(t *testing.T) { + t.Parallel() + q := base + q.offeringClass = "" + inp := describeInputFromQuery(q, nil) + assert.Equal(t, types.OfferingClassTypeConvertible, inp.OfferingClass) + }) + + t.Run("convertible explicit", func(t *testing.T) { + t.Parallel() + q := base + q.offeringClass = types.OfferingClassTypeConvertible + inp := describeInputFromQuery(q, nil) + assert.Equal(t, types.OfferingClassTypeConvertible, inp.OfferingClass) + }) + + t.Run("standard explicit", func(t *testing.T) { + t.Parallel() + q := base + q.offeringClass = types.OfferingClassTypeStandard + inp := describeInputFromQuery(q, nil) + assert.Equal(t, types.OfferingClassTypeStandard, inp.OfferingClass) + }) +} From 1932b658b605f48784eae451adca00afba56c395 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 17:01:37 +0200 Subject: [PATCH 02/11] fix(settings): validate OfferingClass on PUT + assert it reaches the EC2 SDK call (refs #694) - Add validateOfferingClass() to GlobalConfig.Validate() so an invalid offering_class is rejected at PUT time with a clear error rather than silently persisting and only failing at purchase time. Accepts "" | "convertible" | "standard" (case-sensitive, matching resolveOfferingClassType). - Add ValidOfferingClasses exported var for reference/documentation. - Add six table-driven test cases in TestGlobalConfig_Validate covering empty (valid), both valid values, wrong-case "Convertible", "STANDARD", and an unknown value. - Add capturingMockEC2Client embedding MockEC2Client that records the last DescribeReservedInstancesOfferingsInput to enable SDK-level assertions. - Add TestFindOfferingID_OfferingClassReachesSDKCall: three subtests ("", "convertible", "standard") each calling findOfferingID and asserting that the captured OfferingClass on the outbound SDK call equals the expected types.OfferingClassType. This test fails if the wiring from offeringClassStr through resolveOfferingClassType to describeInputFromQuery regresses. --- internal/config/validation.go | 25 +++++++ internal/config/validation_test.go | 52 +++++++++++++ providers/aws/services/ec2/client_test.go | 89 +++++++++++++++++++++++ 3 files changed, 166 insertions(+) diff --git a/internal/config/validation.go b/internal/config/validation.go index 014014100..b29cde081 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -166,6 +166,11 @@ func crossProviderPaymentAlias(provider, raw string) (string, bool) { return "", false } +// ValidOfferingClasses lists the accepted EC2 RI offering class values for +// GlobalConfig. The empty string is also accepted (maps to "convertible" at +// purchase time to preserve pre-694 behaviour). +var ValidOfferingClasses = []string{"convertible", "standard"} + // ValidRampScheduleTypes lists all supported ramp schedule types. var ValidRampScheduleTypes = []string{"immediate", "weekly", "monthly", "custom"} @@ -198,6 +203,9 @@ func (c *GlobalConfig) Validate() error { if err := c.validateGracePeriodDays(); err != nil { return err } + if err := validateOfferingClass(c.OfferingClass); err != nil { + return err + } return c.validateRecommendationsFields() } @@ -658,3 +666,20 @@ func (c *LadderConfigDB) validateLadderRampSchedule() error { } return nil } + +// validateOfferingClass validates the EC2 RI offering class value. +// The empty string is accepted (maps to "convertible" at purchase time). +// Only lowercase "convertible" and "standard" are valid non-empty values; +// the check is case-sensitive to match how resolveOfferingClassType parses +// the field in providers/aws/services/ec2/client.go. +func validateOfferingClass(oc string) error { + if oc == "" { + return nil + } + for _, valid := range ValidOfferingClasses { + if oc == valid { + return nil + } + } + return fmt.Errorf("invalid offering_class: %q (valid: %s)", oc, strings.Join(ValidOfferingClasses, ", ")) +} diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 22ffacd80..94aa7ad02 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -263,6 +263,58 @@ func TestGlobalConfig_Validate(t *testing.T) { }, wantErr: false, }, + // Issue #694: OfferingClass validation on PUT + { + name: "offering_class empty is valid (defaults to convertible)", + config: GlobalConfig{ + DefaultTerm: 3, + OfferingClass: "", + }, + wantErr: false, + }, + { + name: "offering_class convertible is valid", + config: GlobalConfig{ + DefaultTerm: 3, + OfferingClass: "convertible", + }, + wantErr: false, + }, + { + name: "offering_class standard is valid", + config: GlobalConfig{ + DefaultTerm: 3, + OfferingClass: "standard", + }, + wantErr: false, + }, + { + name: "offering_class wrong case is rejected", + config: GlobalConfig{ + DefaultTerm: 3, + OfferingClass: "Convertible", + }, + wantErr: true, + errMsg: "invalid offering_class", + }, + { + name: "offering_class STANDARD is rejected (case-sensitive)", + config: GlobalConfig{ + DefaultTerm: 3, + OfferingClass: "STANDARD", + }, + wantErr: true, + errMsg: "invalid offering_class", + }, + { + name: "offering_class unknown value is rejected", + config: GlobalConfig{ + DefaultTerm: 3, + OfferingClass: "premium", + }, + wantErr: true, + errMsg: "invalid offering_class", + }, } for _, tt := range tests { diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index 39a63e95e..59d542c51 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -38,6 +38,19 @@ func (m *MockEC2Client) DescribeReservedInstancesOfferings(ctx context.Context, return args.Get(0).(*ec2.DescribeReservedInstancesOfferingsOutput), args.Error(1) } +// lastDescribeOfferingsInput captures the most recent +// DescribeReservedInstancesOfferings params for assertion in integration tests. +// Only used in tests that set this field explicitly; nil means uncaptured. +type capturingMockEC2Client struct { + MockEC2Client + LastDescribeOfferingsInput *ec2.DescribeReservedInstancesOfferingsInput +} + +func (m *capturingMockEC2Client) DescribeReservedInstancesOfferings(ctx context.Context, params *ec2.DescribeReservedInstancesOfferingsInput, optFns ...func(*ec2.Options)) (*ec2.DescribeReservedInstancesOfferingsOutput, error) { + m.LastDescribeOfferingsInput = params + return m.MockEC2Client.DescribeReservedInstancesOfferings(ctx, params, optFns...) +} + func (m *MockEC2Client) DescribeReservedInstances(ctx context.Context, params *ec2.DescribeReservedInstancesInput, optFns ...func(*ec2.Options)) (*ec2.DescribeReservedInstancesOutput, error) { args := m.Called(ctx, params) if args.Get(0) == nil { @@ -1137,3 +1150,79 @@ func TestDescribeInputFromQuery_OfferingClass(t *testing.T) { assert.Equal(t, types.OfferingClassTypeStandard, inp.OfferingClass) }) } + +// TestFindOfferingID_OfferingClassReachesSDKCall is the integration test for +// issue #694: it verifies that the offeringClassStr argument passed to +// findOfferingID is wired all the way through to the OfferingClass field on +// the outbound DescribeReservedInstancesOfferings SDK call. The test fails if +// the wiring regresses (e.g. the field is dropped or hardcoded). +func TestFindOfferingID_OfferingClassReachesSDKCall(t *testing.T) { + t.Parallel() + + rec := common.Recommendation{ + ResourceType: "m5.large", + PaymentOption: "all-upfront", + Term: "1yr", + Details: &common.ComputeDetails{ + Platform: "Linux/UNIX", + Tenancy: "default", + Scope: "Region", + }, + } + + offeringOutput := &ec2.DescribeReservedInstancesOfferingsOutput{ + ReservedInstancesOfferings: []types.ReservedInstancesOffering{ + { + ReservedInstancesOfferingId: aws.String("offering-std-123"), + InstanceType: types.InstanceTypeM5Large, + OfferingType: types.OfferingTypeValuesAllUpfront, + }, + }, + } + + tests := []struct { + name string + offeringClassStr string + wantOfferingClass types.OfferingClassType + }{ + { + name: "empty string defaults to convertible", + offeringClassStr: "", + wantOfferingClass: types.OfferingClassTypeConvertible, + }, + { + name: "convertible explicit reaches SDK as convertible", + offeringClassStr: "convertible", + wantOfferingClass: types.OfferingClassTypeConvertible, + }, + { + name: "standard reaches SDK as standard", + offeringClassStr: "standard", + wantOfferingClass: types.OfferingClassTypeStandard, + }, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + inner := &MockEC2Client{} + inner.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). + Return(offeringOutput, nil).Once() + + cap := &capturingMockEC2Client{MockEC2Client: *inner} + client := &Client{client: cap, region: "us-east-1"} + + id, err := client.findOfferingID(context.Background(), rec, "", tc.offeringClassStr) + assert.NoError(t, err) + assert.Equal(t, "offering-std-123", id) + + if assert.NotNil(t, cap.LastDescribeOfferingsInput, "DescribeReservedInstancesOfferings must have been called") { + assert.Equal(t, tc.wantOfferingClass, cap.LastDescribeOfferingsInput.OfferingClass, + "OfferingClass on the SDK call must match the configured value") + } + inner.AssertExpectations(t) + }) + } +} From dc3affbce22e83a92bf130ddafe2ac5572c6b848 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 14:41:00 -0700 Subject: [PATCH 03/11] refactor(config): extract validateScheduleAndNotifications to reduce cyclomatic complexity Validate had complexity 11 after adding the OfferingClass check in #694. Extract the CollectionSchedule + NotificationDaysBefore + GracePeriodDays checks into validateScheduleAndNotifications, keeping Validate under 10. --- internal/config/validation.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/internal/config/validation.go b/internal/config/validation.go index b29cde081..cd07b9b2b 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -194,13 +194,7 @@ func (c *GlobalConfig) Validate() error { if err := validateCoverage(c.DefaultCoverage); err != nil { return err } - if !isValidCollectionSchedule(c.CollectionSchedule) { - return fmt.Errorf("invalid collection_schedule: %q (valid: hourly, daily, weekly)", c.CollectionSchedule) - } - if c.NotificationDaysBefore < 0 || c.NotificationDaysBefore > MaxNotificationDaysBefore { - return fmt.Errorf("notification_days_before must be between 0 and %d, got: %d", MaxNotificationDaysBefore, c.NotificationDaysBefore) - } - if err := c.validateGracePeriodDays(); err != nil { + if err := c.validateScheduleAndNotifications(); err != nil { return err } if err := validateOfferingClass(c.OfferingClass); err != nil { @@ -209,6 +203,19 @@ func (c *GlobalConfig) Validate() error { return c.validateRecommendationsFields() } +// validateScheduleAndNotifications validates collection_schedule and +// notification_days_before. Extracted to keep Validate's cyclomatic +// complexity under the project limit. +func (c *GlobalConfig) validateScheduleAndNotifications() error { + if !isValidCollectionSchedule(c.CollectionSchedule) { + return fmt.Errorf("invalid collection_schedule: %q (valid: hourly, daily, weekly)", c.CollectionSchedule) + } + if c.NotificationDaysBefore < 0 || c.NotificationDaysBefore > MaxNotificationDaysBefore { + return fmt.Errorf("notification_days_before must be between 0 and %d, got: %d", MaxNotificationDaysBefore, c.NotificationDaysBefore) + } + return c.validateGracePeriodDays() +} + // validateRecommendationsFields validates the recommendation-cycle parameters // added in #301 (cache-staleness threshold and AWS Cost Explorer lookback). // Extracted to keep Validate's cyclomatic complexity under the project limit. From 26fc37797f1a48cc2e95492d764ceb9b59d73cef Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 21:18:11 -0700 Subject: [PATCH 04/11] fix(test): avoid mutex copy in TestFindOfferingID_OfferingClassReachesSDKCall Construct capturingMockEC2Client directly instead of copying *MockEC2Client to silence the go vet copylocks warning (MockEC2Client embeds mock.Mock which contains sync.Mutex). --- providers/aws/services/ec2/client_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index 59d542c51..05f623f61 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -1207,11 +1207,10 @@ func TestFindOfferingID_OfferingClassReachesSDKCall(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - inner := &MockEC2Client{} - inner.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). + cap := &capturingMockEC2Client{} + cap.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). Return(offeringOutput, nil).Once() - cap := &capturingMockEC2Client{MockEC2Client: *inner} client := &Client{client: cap, region: "us-east-1"} id, err := client.findOfferingID(context.Background(), rec, "", tc.offeringClassStr) @@ -1222,7 +1221,7 @@ func TestFindOfferingID_OfferingClassReachesSDKCall(t *testing.T) { assert.Equal(t, tc.wantOfferingClass, cap.LastDescribeOfferingsInput.OfferingClass, "OfferingClass on the SDK call must match the configured value") } - inner.AssertExpectations(t) + cap.AssertExpectations(t) }) } } From f287d0b4faefaff78ff114e063147cc00e0c3664 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 8 Jun 2026 18:29:49 -0700 Subject: [PATCH 05/11] fix(purchase): fail EC2 RI recs on GlobalConfig load error instead of defaulting OfferingClass When GetGlobalConfig returns an error, the old code logged it and continued with an empty OfferingClass, which resolveOfferingClassType maps to "convertible". This caused a transient DB failure to silently buy the wrong (and more expensive, irreversible) RI class instead of the operator-configured "standard". Violates the no-silent-fallbacks-on-money-paths rule. Fix: change processPurchaseRecommendations to return (float64, float64, []string, error) and propagate a hard error on GetGlobalConfig failure. Both call sites (executeSingleAccount and executeForAccount) check the new error return and abort without making any cloud purchase. Regression test (HOLE 1): TestProcessPurchaseRecommendations_GlobalConfigError_FailsInsteadOfDefaulting confirms the function returns an error and never calls the provider factory when GetGlobalConfig fails. Verified red on pre-fix code, green after. Also add TestSaveGlobalConfig_OfferingClassBindsAt21 (HOLE 2): a pgxmock test against the real PostgresStore.SaveGlobalConfig verifying offering_class binds as the 21st positional arg. The hand-maintained testablePostgresStore in store_postgres_mock_test.go omits this field entirely, so no fast test previously guarded the 21-placeholder write query. --- .../config/store_postgres_coverage_test.go | 69 +++++++++++++++++++ internal/purchase/execution.go | 37 ++++++---- internal/purchase/execution_test.go | 52 ++++++++++++++ .../purchase/money_path_regression_test.go | 3 +- 4 files changed, 148 insertions(+), 13 deletions(-) diff --git a/internal/config/store_postgres_coverage_test.go b/internal/config/store_postgres_coverage_test.go index f2c979c78..f291bc403 100644 --- a/internal/config/store_postgres_coverage_test.go +++ b/internal/config/store_postgres_coverage_test.go @@ -5,7 +5,9 @@ import ( "testing" "time" + "github.com/pashagolub/pgxmock/v4" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // These tests exercise the real PostgresStore methods to gain code coverage. @@ -535,3 +537,70 @@ func TestPostgresStore_GetAllPurchaseHistory_NilDB(t *testing.T) { assert.True(t, panicked, "expected panic with nil db connection") } + +// TestSaveGlobalConfig_OfferingClassBindsAt21 is the HOLE 2 regression guard +// (issue #694): the real PostgresStore.SaveGlobalConfig must bind offering_class +// as the 21st positional argument ($21). The testablePostgresStore in +// store_postgres_mock_test.go is a hand-maintained copy that omits the field +// entirely, so no fast test guarded the placeholder count until now. +// +// Using pgxmock directly against the real PostgresStore (not the hand-maintained +// testablePostgresStore wrapper) ensures the live query is tested, not a stale copy. +func TestSaveGlobalConfig_OfferingClassBindsAt21(t *testing.T) { + ctx := context.Background() + mock, err := pgxmock.NewPool() + require.NoError(t, err) + defer mock.Close() + + // Wire the pgxmock pool directly into the real PostgresStore via the + // unexported db field (test is in package config so this is allowed). + store := &PostgresStore{db: mock} + + email := "ops@example.com" + cfg := &GlobalConfig{ + EnabledProviders: []string{"aws"}, + NotificationEmail: &email, + ApprovalRequired: true, + DefaultTerm: 12, + DefaultPayment: "all-upfront", + DefaultCoverage: 80.0, + DefaultRampSchedule: "immediate", + OfferingClass: "standard", + } + + // Expect exactly 21 args; pgxmock validates arg count and types. + // The 21st arg must be the string "standard" (offering_class). + // If the real query regresses to a different arg count, pgxmock + // will return an unexpected-call error and the test will fail. + mock.ExpectExec(`INSERT INTO global_config`). + WithArgs( + pgxmock.AnyArg(), // $1 enabled_providers + pgxmock.AnyArg(), // $2 notification_email + pgxmock.AnyArg(), // $3 approval_required + pgxmock.AnyArg(), // $4 default_term + pgxmock.AnyArg(), // $5 default_payment + pgxmock.AnyArg(), // $6 default_coverage + pgxmock.AnyArg(), // $7 default_ramp_schedule + pgxmock.AnyArg(), // $8 ri_exchange_enabled + pgxmock.AnyArg(), // $9 ri_exchange_mode + pgxmock.AnyArg(), // $10 ri_exchange_utilization_threshold + pgxmock.AnyArg(), // $11 ri_exchange_max_per_exchange_usd + pgxmock.AnyArg(), // $12 ri_exchange_max_daily_usd + pgxmock.AnyArg(), // $13 ri_exchange_lookback_days + pgxmock.AnyArg(), // $14 auto_collect + pgxmock.AnyArg(), // $15 collection_schedule + pgxmock.AnyArg(), // $16 notification_days_before + pgxmock.AnyArg(), // $17 grace_period_days + pgxmock.AnyArg(), // $18 recommendations_cache_stale_hours + pgxmock.AnyArg(), // $19 recommendations_lookback_days + pgxmock.AnyArg(), // $20 purchase_delay_hours + "standard", // $21 offering_class -- the field this test guards + ). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + + err = store.SaveGlobalConfig(ctx, cfg) + require.NoError(t, err, "SaveGlobalConfig must succeed when the DB accepts all 21 args") + + require.NoError(t, mock.ExpectationsWereMet(), + "offering_class must be bound as the 21st argument to SaveGlobalConfig") +} diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 851f7d607..9fea9b245 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -101,7 +101,10 @@ func (m *Manager) executeSingleAccount(ctx context.Context, exec *config.Purchas } } - totalSavings, totalUpfront, purchaseErrors := m.processPurchaseRecommendations(ctx, exec, plan, accountID, provCfg) + totalSavings, totalUpfront, purchaseErrors, procErr := m.processPurchaseRecommendations(ctx, exec, plan, accountID, provCfg) + if procErr != nil { + return procErr + } if len(purchaseErrors) > 0 && !anyRecPurchased(exec.Recommendations) { // Nothing committed — a clean total failure. @@ -241,7 +244,13 @@ func (m *Manager) executeForAccount(ctx context.Context, baseExec *config.Purcha } accountID := account.ExternalID - totalSavings, totalUpfront, purchaseErrors := m.processPurchaseRecommendations(ctx, &acctExec, plan, accountID, provCfg) + totalSavings, totalUpfront, purchaseErrors, procErr := m.processPurchaseRecommendations(ctx, &acctExec, plan, accountID, provCfg) + if procErr != nil { + acctExec.Status = "failed" + acctExec.Error = procErr.Error() + _ = m.config.SavePurchaseExecution(ctx, &acctExec) + return false, procErr + } // #642: a per-account run can be a partial success — some recs committed // to purchase_history (Purchased=true) while others failed. Marking the @@ -502,7 +511,7 @@ type recPurchaseOutcome struct { index int } -func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *config.PurchaseExecution, plan *config.PurchasePlan, accountID string, provCfg *provider.ProviderConfig) (float64, float64, []string) { //nolint:gocritic // unnamedResult: return names would conflict with body locals +func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *config.PurchaseExecution, plan *config.PurchasePlan, accountID string, provCfg *provider.ProviderConfig) (float64, float64, []string, error) { //nolint:gocritic // unnamedResult: return names would conflict with body locals // ExecutionID is carried into PurchaseOptions so executeSinglePurchase // can tag every per-rec log line with the owning exec UUID. Without // this, CloudWatch filtering by exec ID returns zero hits and a stuck @@ -514,14 +523,17 @@ func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *conf } // Load GlobalConfig to pick up the OfferingClass setting for EC2 RI - // purchases. A config-load failure is non-fatal: we fall back to the - // empty string which the EC2 client maps to "convertible" (pre-694 - // behaviour). The error is logged so operators can diagnose a DB - // issue without the entire purchase batch failing. - if globalCfg, err := m.config.GetGlobalConfig(ctx); err != nil { - logging.Errorf("purchase[%s]: failed to load GlobalConfig for offering class (defaulting to convertible): %v", + // purchases. A load error is fatal: proceeding with an empty OfferingClass + // silently defaults to "convertible" even when the operator configured + // "standard", which is an irreversible, more-expensive purchase. Fail the + // run so the operator can diagnose the DB issue and retry with correct + // settings (no-silent-fallbacks-on-money-paths rule). + globalCfg, err := m.config.GetGlobalConfig(ctx) + if err != nil { + return 0, 0, nil, fmt.Errorf("purchase[%s]: failed to load GlobalConfig for offering class: %w", exec.ExecutionID, err) - } else if globalCfg != nil { + } + if globalCfg != nil { opts.OfferingClass = globalCfg.OfferingClass } @@ -531,7 +543,7 @@ func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *conf if len(selected) == 0 { logging.Infof("purchase[%s]: no selected recommendations, nothing to execute (account=%s plan=%q)", exec.ExecutionID, accountID, plan.Name) - return 0, 0, nil + return 0, 0, nil, nil } logging.Infof("purchase[%s]: dispatching %d recommendation(s) for account=%s plan=%q", exec.ExecutionID, len(selected), accountID, plan.Name) @@ -582,7 +594,8 @@ func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *conf // there are no concurrent writes to totals, exec.Recommendations, or // purchaseErrors (05-N2). Do NOT move the aggregation inside the FanOut // closure or run it concurrently with the fan-out. - return m.aggregatePurchaseOutcomes(ctx, exec, plan, accountID, results) + totalSavings, totalUpfront, purchaseErrors := m.aggregatePurchaseOutcomes(ctx, exec, plan, accountID, results) + return totalSavings, totalUpfront, purchaseErrors, nil } // aggregatePurchaseOutcomes walks the fan-out results serially and writes diff --git a/internal/purchase/execution_test.go b/internal/purchase/execution_test.go index 17702d5b7..502ca53f2 100644 --- a/internal/purchase/execution_test.go +++ b/internal/purchase/execution_test.go @@ -1772,3 +1772,55 @@ func TestManager_ExecutePurchase_SingleAccount_StampsTargetAccount(t *testing.T) }) } } + +// TestProcessPurchaseRecommendations_GlobalConfigError_FailsInsteadOfDefaulting is the +// issue #694 regression guard for HOLE 1: when GetGlobalConfig returns an error, +// processPurchaseRecommendations must return an error and must NOT proceed with +// an empty OfferingClass that silently defaults to "convertible". +// +// The pre-fix code logged the error and continued, so a transient DB failure +// would cause a standard-configured RI to be purchased as convertible (more +// expensive, and irreversible). This test asserts the function returns the DB +// error and PurchaseCommitment is never called. +func TestProcessPurchaseRecommendations_GlobalConfigError_FailsInsteadOfDefaulting(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockFactory := new(MockProviderFactory) + + dbErr := errors.New("connection refused: GlobalConfig read failed") + // Register the GetGlobalConfig expectation so the mock routes to testify + // (rather than the no-op default that returns an empty config). + mockStore.On("GetGlobalConfig", ctx).Return(nil, dbErr) + + exec := &config.PurchaseExecution{ + ExecutionID: "exec-globalcfg-err", + PlanID: "", + Recommendations: []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", ResourceType: "m5.large", Region: "us-east-1", Count: 1, UpfrontCost: 300, Savings: 50, Selected: true}, + }, + } + plan := &config.PurchasePlan{Name: "Direct purchase"} + + manager := &Manager{ + config: mockStore, + providerFactory: mockFactory, + dashboardURL: "https://dashboard.example.com", + } + + _, _, purchaseErrors, procErr := manager.processPurchaseRecommendations(ctx, exec, plan, "111111111111", nil) + + // The function must return an error derived from the DB failure. + require.Error(t, procErr, "GlobalConfig load failure must abort processing, not silently default to convertible") + assert.Contains(t, procErr.Error(), "GlobalConfig", "error must mention GlobalConfig so operators can diagnose the DB issue") + + // purchaseErrors must be nil: no rec-level errors should be emitted + // because the function must abort before reaching the fan-out. + assert.Nil(t, purchaseErrors, "no rec-level errors expected — function must abort before the fan-out") + + // PurchaseCommitment must NEVER be called: assert the factory was never + // invoked, which is the clearest proxy for "no cloud API call happened". + mockFactory.AssertNotCalled(t, "CreateAndValidateProvider") + + mockStore.AssertExpectations(t) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) +} diff --git a/internal/purchase/money_path_regression_test.go b/internal/purchase/money_path_regression_test.go index eaaaf374b..61d96837b 100644 --- a/internal/purchase/money_path_regression_test.go +++ b/internal/purchase/money_path_regression_test.go @@ -67,7 +67,8 @@ func captureIdempotencyTokens(t *testing.T, exec *config.PurchaseExecution) map[ // provCfg is nil: the mock factory matches mock.Anything for it and the // real factory is never reached. - _, _, errs := manager.processPurchaseRecommendations(ctx, exec, plan, "111111111111", nil) + _, _, errs, procErr := manager.processPurchaseRecommendations(ctx, exec, plan, "111111111111", nil) + require.NoError(t, procErr, "processPurchaseRecommendations must not fail in the happy path") require.Empty(t, errs, "all recs must commit so the captured token reflects a real purchase") return tokens } From 45ae4887a3c1ce48b526b9f7d3ab0ddbe51b44ff Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 9 Jun 2026 06:49:41 -0700 Subject: [PATCH 06/11] refactor(purchase): extract applyAccountOutcome to drop executeForAccount below gocyclo 10 The gocyclo pre-commit hook (-over 10) failed: executeForAccount was at cyclomatic complexity 11. Extract the per-account status-resolution switch (partially_completed / failed / completed stamping, #642) and the committed gate (#1014) into a cohesive applyAccountOutcome helper, dropping the function below the threshold without a //nolint or a threshold bump. Behavior is unchanged: same status transitions, same error-note appending, same committed semantics (anyRecPurchased is now evaluated once and reused for the partial flag). The OfferingClass typed-enum validation and no-silent-fallback behavior are untouched. --- internal/purchase/execution.go | 66 +++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 9fea9b245..4d78a5fba 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -252,34 +252,9 @@ func (m *Manager) executeForAccount(ctx context.Context, baseExec *config.Purcha return false, procErr } - // #642: a per-account run can be a partial success — some recs committed - // to purchase_history (Purchased=true) while others failed. Marking the - // row "failed" in that case is wrong (it hides the real commitments and - // invites a re-approve that double-buys). Record "partially_completed" - // instead so the row reflects reality, and still send the confirmation - // for the recs that did purchase. - partial := len(purchaseErrors) > 0 && anyRecPurchased(acctExec.Recommendations) - switch { - case partial: - now := time.Now() - acctExec.Status = "partially_completed" - // Append so any audit-gap note already stamped by - // aggregatePurchaseOutcomes (issue #621) survives alongside the - // per-rec failure list. - acctExec.Error = appendErrNote(acctExec.Error, strings.Join(purchaseErrors, "; ")) - acctExec.CompletedAt = &now - case len(purchaseErrors) > 0: - acctExec.Status = "failed" - acctExec.Error = appendErrNote(acctExec.Error, strings.Join(purchaseErrors, "; ")) - default: - now := time.Now() - acctExec.Status = "completed" - acctExec.CompletedAt = &now - } - - // committed is the gate executeMultiAccount uses to distinguish a partial - // run from a flat failure (issue #1014): true when any rec purchased. - committed = anyRecPurchased(acctExec.Recommendations) + // Resolve the final status/error/completion stamp from the per-rec outcome + // (#642), and learn whether any rec committed (#1014). + partial, committed := applyAccountOutcome(&acctExec, purchaseErrors) if saveErr := m.config.SavePurchaseExecution(ctx, &acctExec); saveErr != nil { return committed, fmt.Errorf("AUDIT LOSS: failed to save execution record for account %s: %w", account.ID, saveErr) @@ -320,6 +295,41 @@ func (m *Manager) persistFailedAccountExecution(ctx context.Context, acctExec *c return cause } +// applyAccountOutcome stamps the final Status / Error / CompletedAt on acctExec +// from the per-rec purchase outcome and reports whether the run was a partial +// success and whether any rec committed. +// +// A per-account run can be a partial success (#642): some recs committed to +// purchase_history (Purchased=true) while others failed. Marking the row +// "failed" in that case is wrong (it hides the real commitments and invites a +// re-approve that double-buys), so record "partially_completed" instead and let +// the caller still send the confirmation for the recs that did purchase. +// +// committed is the gate executeMultiAccount uses to distinguish a partial run +// from a flat failure (#1014): true when any rec purchased. +func applyAccountOutcome(acctExec *config.PurchaseExecution, purchaseErrors []string) (partial, committed bool) { + committed = anyRecPurchased(acctExec.Recommendations) + partial = len(purchaseErrors) > 0 && committed + switch { + case partial: + now := time.Now() + acctExec.Status = "partially_completed" + // Append so any audit-gap note already stamped by + // aggregatePurchaseOutcomes (issue #621) survives alongside the + // per-rec failure list. + acctExec.Error = appendErrNote(acctExec.Error, strings.Join(purchaseErrors, "; ")) + acctExec.CompletedAt = &now + case len(purchaseErrors) > 0: + acctExec.Status = "failed" + acctExec.Error = appendErrNote(acctExec.Error, strings.Join(purchaseErrors, "; ")) + default: + now := time.Now() + acctExec.Status = "completed" + acctExec.CompletedAt = &now + } + return partial, committed +} + // resolveSingleAccountProvider derives per-account credentials for the // single-account execution path. Returns (nil, "", nil) when no account can be // identified (ambient credentials are used in that case), or when no recs are From a758db46ad46e8382c531fecb86c13aaba371f42 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 9 Jul 2026 22:54:42 +0200 Subject: [PATCH 07/11] fix(test): update SQL parameter positions after laddering_enabled shift Rebasing onto main added laddering_enabled as $21 in global_config INSERT, shifting offering_class from $21 to $22. Update the pgxmock and coverage tests to reflect the new column count and binding positions. --- .../config/store_postgres_coverage_test.go | 22 ++++++++++--------- .../config/store_postgres_pgxmock_test.go | 9 +++++--- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/internal/config/store_postgres_coverage_test.go b/internal/config/store_postgres_coverage_test.go index f291bc403..c30a27ebc 100644 --- a/internal/config/store_postgres_coverage_test.go +++ b/internal/config/store_postgres_coverage_test.go @@ -538,15 +538,16 @@ func TestPostgresStore_GetAllPurchaseHistory_NilDB(t *testing.T) { assert.True(t, panicked, "expected panic with nil db connection") } -// TestSaveGlobalConfig_OfferingClassBindsAt21 is the HOLE 2 regression guard +// TestSaveGlobalConfig_OfferingClassBindsAt22 is the HOLE 2 regression guard // (issue #694): the real PostgresStore.SaveGlobalConfig must bind offering_class -// as the 21st positional argument ($21). The testablePostgresStore in -// store_postgres_mock_test.go is a hand-maintained copy that omits the field -// entirely, so no fast test guarded the placeholder count until now. +// as the 22nd positional argument ($22), with laddering_enabled at $21. +// The testablePostgresStore in store_postgres_mock_test.go is a hand-maintained +// copy that omits the field entirely, so no fast test guarded the placeholder +// count until now. // // Using pgxmock directly against the real PostgresStore (not the hand-maintained // testablePostgresStore wrapper) ensures the live query is tested, not a stale copy. -func TestSaveGlobalConfig_OfferingClassBindsAt21(t *testing.T) { +func TestSaveGlobalConfig_OfferingClassBindsAt22(t *testing.T) { ctx := context.Background() mock, err := pgxmock.NewPool() require.NoError(t, err) @@ -568,8 +569,8 @@ func TestSaveGlobalConfig_OfferingClassBindsAt21(t *testing.T) { OfferingClass: "standard", } - // Expect exactly 21 args; pgxmock validates arg count and types. - // The 21st arg must be the string "standard" (offering_class). + // Expect exactly 22 args; pgxmock validates arg count and types. + // The 21st arg is laddering_enabled; the 22nd arg must be "standard" (offering_class). // If the real query regresses to a different arg count, pgxmock // will return an unexpected-call error and the test will fail. mock.ExpectExec(`INSERT INTO global_config`). @@ -594,13 +595,14 @@ func TestSaveGlobalConfig_OfferingClassBindsAt21(t *testing.T) { pgxmock.AnyArg(), // $18 recommendations_cache_stale_hours pgxmock.AnyArg(), // $19 recommendations_lookback_days pgxmock.AnyArg(), // $20 purchase_delay_hours - "standard", // $21 offering_class -- the field this test guards + pgxmock.AnyArg(), // $21 laddering_enabled + "standard", // $22 offering_class -- the field this test guards ). WillReturnResult(pgxmock.NewResult("INSERT", 1)) err = store.SaveGlobalConfig(ctx, cfg) - require.NoError(t, err, "SaveGlobalConfig must succeed when the DB accepts all 21 args") + require.NoError(t, err, "SaveGlobalConfig must succeed when the DB accepts all 22 args") require.NoError(t, mock.ExpectationsWereMet(), - "offering_class must be bound as the 21st argument to SaveGlobalConfig") + "offering_class must be bound as the 22nd argument to SaveGlobalConfig") } diff --git a/internal/config/store_postgres_pgxmock_test.go b/internal/config/store_postgres_pgxmock_test.go index ac6259e9e..1135c926f 100644 --- a/internal/config/store_postgres_pgxmock_test.go +++ b/internal/config/store_postgres_pgxmock_test.go @@ -191,6 +191,7 @@ var globalConfigCols = []string{ "purchase_delay_hours", "laddering_enabled", "ladder_execution_enabled", + "offering_class", } // TestPGXMock_UpdateGlobalConfigAtomic_LockedReadModifyWrite proves the F2 @@ -218,8 +219,9 @@ func TestPGXMock_UpdateGlobalConfigAtomic_LockedReadModifyWrite(t *testing.T) { "{}", 24, 7, 48, - false, // laddering_enabled = false - false, // ladder_execution_enabled = false + false, // laddering_enabled = false + false, // ladder_execution_enabled = false + "convertible", // offering_class ) // Strict order: the SELECT and the UPSERT must sit between the same @@ -228,7 +230,7 @@ func TestPGXMock_UpdateGlobalConfigAtomic_LockedReadModifyWrite(t *testing.T) { mock.ExpectExec("pg_advisory_xact_lock").WithArgs(pgxmock.AnyArg()). WillReturnResult(pgxmock.NewResult("SELECT", 1)) mock.ExpectQuery("FROM global_config").WillReturnRows(seeded) - mock.ExpectExec("INSERT INTO global_config").WithArgs(anyArgsCfg(22)...). + mock.ExpectExec("INSERT INTO global_config").WithArgs(anyArgsCfg(23)...). WillReturnResult(pgxmock.NewResult("INSERT", 1)) mock.ExpectCommit() @@ -275,6 +277,7 @@ func TestPGXMock_UpdateGlobalConfigAtomic_ApplyErrorRollsBack(t *testing.T) { 0, false, false, + "convertible", ) mock.ExpectBegin() From 90aa6ca91fc491b0796765e929e1d4868deca7e2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 10 Jul 2026 17:15:19 +0200 Subject: [PATCH 08/11] style(purchase,config): clear new-from-rev lint on OfferingClass diff Resolve the three golangci-lint findings that --new-from-rev=origin/main attributes to the #694 OfferingClass changes: - errcheck: replace the silent `_ =` discard of SavePurchaseExecution on the processPurchaseRecommendations error path with the existing saveExecutionStatusBestEffort helper, which persists and logs the audit-save failure instead of dropping it. - gocritic unnamedResult: name the four results of processPurchaseRecommendations (its return set grew to include error); switch the trailing assignment from := to = accordingly. - misspell: pre-694 behaviour -> behavior in the ValidOfferingClasses doc. No behavior change to the purchase path; base-debt Lint/Security failures are pre-existing on main and out of scope. --- internal/config/validation.go | 2 +- internal/purchase/execution.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/config/validation.go b/internal/config/validation.go index cd07b9b2b..2f5d80677 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -168,7 +168,7 @@ func crossProviderPaymentAlias(provider, raw string) (string, bool) { // ValidOfferingClasses lists the accepted EC2 RI offering class values for // GlobalConfig. The empty string is also accepted (maps to "convertible" at -// purchase time to preserve pre-694 behaviour). +// purchase time to preserve pre-694 behavior). var ValidOfferingClasses = []string{"convertible", "standard"} // ValidRampScheduleTypes lists all supported ramp schedule types. diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index 4d78a5fba..e6b4ab6f2 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -248,7 +248,7 @@ func (m *Manager) executeForAccount(ctx context.Context, baseExec *config.Purcha if procErr != nil { acctExec.Status = "failed" acctExec.Error = procErr.Error() - _ = m.config.SavePurchaseExecution(ctx, &acctExec) + m.saveExecutionStatusBestEffort(ctx, &acctExec) return false, procErr } @@ -521,7 +521,7 @@ type recPurchaseOutcome struct { index int } -func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *config.PurchaseExecution, plan *config.PurchasePlan, accountID string, provCfg *provider.ProviderConfig) (float64, float64, []string, error) { //nolint:gocritic // unnamedResult: return names would conflict with body locals +func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *config.PurchaseExecution, plan *config.PurchasePlan, accountID string, provCfg *provider.ProviderConfig) (totalSavings, totalUpfront float64, purchaseErrors []string, procErr error) { // ExecutionID is carried into PurchaseOptions so executeSinglePurchase // can tag every per-rec log line with the owning exec UUID. Without // this, CloudWatch filtering by exec ID returns zero hits and a stuck @@ -604,7 +604,7 @@ func (m *Manager) processPurchaseRecommendations(ctx context.Context, exec *conf // there are no concurrent writes to totals, exec.Recommendations, or // purchaseErrors (05-N2). Do NOT move the aggregation inside the FanOut // closure or run it concurrently with the fan-out. - totalSavings, totalUpfront, purchaseErrors := m.aggregatePurchaseOutcomes(ctx, exec, plan, accountID, results) + totalSavings, totalUpfront, purchaseErrors = m.aggregatePurchaseOutcomes(ctx, exec, plan, accountID, results) return totalSavings, totalUpfront, purchaseErrors, nil } From b4b461ad8eed332219cc67292ff34c4bcba9fb4c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 11 Jul 2026 00:03:19 +0200 Subject: [PATCH 09/11] chore(migration): renumber ec2_ri_offering_class from 000078 to 000083 Migration 000078 collided with PR #828. Main's highest is 000081; #1277 reserves 000082; 000083 is the next free slot for this PR. Renamed via git mv (up + down); no Go code referenced the number directly. --- ...ering_class.down.sql => 000083_ec2_ri_offering_class.down.sql} | 0 ..._offering_class.up.sql => 000083_ec2_ri_offering_class.up.sql} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename internal/database/postgres/migrations/{000078_ec2_ri_offering_class.down.sql => 000083_ec2_ri_offering_class.down.sql} (100%) rename internal/database/postgres/migrations/{000078_ec2_ri_offering_class.up.sql => 000083_ec2_ri_offering_class.up.sql} (100%) diff --git a/internal/database/postgres/migrations/000078_ec2_ri_offering_class.down.sql b/internal/database/postgres/migrations/000083_ec2_ri_offering_class.down.sql similarity index 100% rename from internal/database/postgres/migrations/000078_ec2_ri_offering_class.down.sql rename to internal/database/postgres/migrations/000083_ec2_ri_offering_class.down.sql diff --git a/internal/database/postgres/migrations/000078_ec2_ri_offering_class.up.sql b/internal/database/postgres/migrations/000083_ec2_ri_offering_class.up.sql similarity index 100% rename from internal/database/postgres/migrations/000078_ec2_ri_offering_class.up.sql rename to internal/database/postgres/migrations/000083_ec2_ri_offering_class.up.sql From 72082e6361e6dceb173d922c2413361e5341e26d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 19:45:12 +0300 Subject: [PATCH 10/11] chore(migration): renumber ec2_ri_offering_class from 000083 to 000090 Migration 000083 is now taken by ladder_execution_enabled (merged to main while this PR was in review). Next free slot after 000089_rename_cancelled_to_canceled is 000090. --- ...ering_class.down.sql => 000090_ec2_ri_offering_class.down.sql} | 0 ..._offering_class.up.sql => 000090_ec2_ri_offering_class.up.sql} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename internal/database/postgres/migrations/{000083_ec2_ri_offering_class.down.sql => 000090_ec2_ri_offering_class.down.sql} (100%) rename internal/database/postgres/migrations/{000083_ec2_ri_offering_class.up.sql => 000090_ec2_ri_offering_class.up.sql} (100%) diff --git a/internal/database/postgres/migrations/000083_ec2_ri_offering_class.down.sql b/internal/database/postgres/migrations/000090_ec2_ri_offering_class.down.sql similarity index 100% rename from internal/database/postgres/migrations/000083_ec2_ri_offering_class.down.sql rename to internal/database/postgres/migrations/000090_ec2_ri_offering_class.down.sql diff --git a/internal/database/postgres/migrations/000083_ec2_ri_offering_class.up.sql b/internal/database/postgres/migrations/000090_ec2_ri_offering_class.up.sql similarity index 100% rename from internal/database/postgres/migrations/000083_ec2_ri_offering_class.up.sql rename to internal/database/postgres/migrations/000090_ec2_ri_offering_class.up.sql From d8ded22ed8e4db29c80d280c4963add727feba9b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 19:49:00 +0300 Subject: [PATCH 11/11] fix(rebase): restore saveExecutionStatusBestEffort; update param-23 guard test Two issues introduced when rebasing onto main (which added ladder_execution_enabled in parallel): 1. saveExecutionStatusBestEffort helper was dropped during conflict resolution of the applyAccountOutcome refactor commit; restored from the pre-rebase branch. 2. TestSaveGlobalConfig_OfferingClassBindsAt22 expected 22 args; with ladder_execution_enabled now at $22, offering_class moved to $23. Renamed the test to TestSaveGlobalConfig_OfferingClassBindsAt23 and added the $22 AnyArg placeholder. --- .../config/store_postgres_coverage_test.go | 19 +++++++++++-------- internal/purchase/execution.go | 9 +++++++++ 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/internal/config/store_postgres_coverage_test.go b/internal/config/store_postgres_coverage_test.go index c30a27ebc..cf152869f 100644 --- a/internal/config/store_postgres_coverage_test.go +++ b/internal/config/store_postgres_coverage_test.go @@ -538,16 +538,17 @@ func TestPostgresStore_GetAllPurchaseHistory_NilDB(t *testing.T) { assert.True(t, panicked, "expected panic with nil db connection") } -// TestSaveGlobalConfig_OfferingClassBindsAt22 is the HOLE 2 regression guard +// TestSaveGlobalConfig_OfferingClassBindsAt23 is the HOLE 2 regression guard // (issue #694): the real PostgresStore.SaveGlobalConfig must bind offering_class -// as the 22nd positional argument ($22), with laddering_enabled at $21. +// as the 23rd positional argument ($23), with laddering_enabled at $21 and +// ladder_execution_enabled at $22. // The testablePostgresStore in store_postgres_mock_test.go is a hand-maintained // copy that omits the field entirely, so no fast test guarded the placeholder // count until now. // // Using pgxmock directly against the real PostgresStore (not the hand-maintained // testablePostgresStore wrapper) ensures the live query is tested, not a stale copy. -func TestSaveGlobalConfig_OfferingClassBindsAt22(t *testing.T) { +func TestSaveGlobalConfig_OfferingClassBindsAt23(t *testing.T) { ctx := context.Background() mock, err := pgxmock.NewPool() require.NoError(t, err) @@ -569,8 +570,9 @@ func TestSaveGlobalConfig_OfferingClassBindsAt22(t *testing.T) { OfferingClass: "standard", } - // Expect exactly 22 args; pgxmock validates arg count and types. - // The 21st arg is laddering_enabled; the 22nd arg must be "standard" (offering_class). + // Expect exactly 23 args; pgxmock validates arg count and types. + // The 21st arg is laddering_enabled; the 22nd is ladder_execution_enabled; + // the 23rd arg must be "standard" (offering_class). // If the real query regresses to a different arg count, pgxmock // will return an unexpected-call error and the test will fail. mock.ExpectExec(`INSERT INTO global_config`). @@ -596,13 +598,14 @@ func TestSaveGlobalConfig_OfferingClassBindsAt22(t *testing.T) { pgxmock.AnyArg(), // $19 recommendations_lookback_days pgxmock.AnyArg(), // $20 purchase_delay_hours pgxmock.AnyArg(), // $21 laddering_enabled - "standard", // $22 offering_class -- the field this test guards + pgxmock.AnyArg(), // $22 ladder_execution_enabled + "standard", // $23 offering_class -- the field this test guards ). WillReturnResult(pgxmock.NewResult("INSERT", 1)) err = store.SaveGlobalConfig(ctx, cfg) - require.NoError(t, err, "SaveGlobalConfig must succeed when the DB accepts all 22 args") + require.NoError(t, err, "SaveGlobalConfig must succeed when the DB accepts all 23 args") require.NoError(t, mock.ExpectationsWereMet(), - "offering_class must be bound as the 22nd argument to SaveGlobalConfig") + "offering_class must be bound as the 23rd argument to SaveGlobalConfig") } diff --git a/internal/purchase/execution.go b/internal/purchase/execution.go index e6b4ab6f2..0eefefcee 100644 --- a/internal/purchase/execution.go +++ b/internal/purchase/execution.go @@ -201,6 +201,15 @@ func (m *Manager) executeMultiAccount(ctx context.Context, baseExec *config.Purc return fmt.Errorf("%w: %s", errAllAccountsFailed, strings.Join(errs, "; ")) } +// saveExecutionStatusBestEffort saves the execution record and logs any error. +// Used in error paths where we need to persist the failure status but cannot +// propagate the save error (the original error is already being returned). +func (m *Manager) saveExecutionStatusBestEffort(ctx context.Context, exec *config.PurchaseExecution) { + if err := m.config.SavePurchaseExecution(ctx, exec); err != nil { + logging.Warnf("execution: failed to persist error status for account %v: %v", exec.CloudAccountID, err) + } +} + // executeForAccount runs a single plan execution for one cloud account. // It creates a new PurchaseExecution record tagged with cloud_account_id, resolves // per-account credentials, executes purchases, and saves the result.