From 2753abdb7d4328e52d0a78281e99cc4df0e3070c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 06:44:16 +0200 Subject: [PATCH 1/4] fix(ri-exchange): exclude convertible RIs with non-positive duration monthlyCostFromConvertibleRI returned 0 when Duration <= 0. reshape's pricing gate treats MonthlyCost <= 0 as "no source pricing" and skips the dollar-units check, so such an RI behaved as free and every cross-family alternative passed unfiltered. The function now returns an error, and convertToExchangeTypes (api) and convertForAutoExchange (server) log a warning and drop that RI from the reshape / auto-exchange inputs. Valid RIs in the same list are unaffected. Two existing fixtures lacked a Duration and now set one. Closes #1181 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_ri_exchange.go | 27 ++++++++------- internal/api/handler_ri_exchange_test.go | 37 ++++++++++++++++++-- internal/server/handler_ri_exchange.go | 26 ++++++++------ internal/server/handler_ri_exchange_test.go | 38 +++++++++++++++++++++ internal/server/ri_exchange_zone_test.go | 4 +-- 5 files changed, 106 insertions(+), 26 deletions(-) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 89a961297..745db2e36 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -1598,42 +1598,45 @@ func parseThresholdParam(params map[string]string) (float64, error) { // // monthly = (FixedPrice / hours_per_term + UsagePrice + recurring_hourly) × 730 // -// 730 is AWS's canonical hours-per-month constant. Returns zero when -// Duration is zero (defensive — would otherwise divide by zero). +// 730 is AWS's canonical hours-per-month constant. Returns an error when +// Duration is non-positive: a zero cost would read as a free RI and skip +// reshape's dollar-units pricing gate, so callers must exclude the RI. // // Used to populate exchange.RIInfo.MonthlyCost so the cross-family // dollar-units pre-filter can compare against per-target offering // costs computed with the same formula. -func monthlyCostFromConvertibleRI(ri ec2svc.ConvertibleRI) float64 { +func monthlyCostFromConvertibleRI(ri ec2svc.ConvertibleRI) (float64, error) { if ri.Duration <= 0 { - return 0 + return 0, fmt.Errorf("convertible RI %s has non-positive duration %d: monthly cost is not computable", ri.ReservedInstanceID, ri.Duration) } hoursPerTerm := float64(ri.Duration) / 3600 - if hoursPerTerm <= 0 { - return 0 - } - return ((ri.FixedPrice / hoursPerTerm) + ri.UsagePrice + ri.RecurringHourlyAmount) * 730 + return ((ri.FixedPrice / hoursPerTerm) + ri.UsagePrice + ri.RecurringHourlyAmount) * 730, nil } // convertToExchangeTypes converts provider-specific types to the exchange package types. func convertToExchangeTypes(instances []ec2svc.ConvertibleRI, utilData []recommendations.RIUtilization) ([]exchange.RIInfo, []exchange.UtilizationInfo) { - riInfos := make([]exchange.RIInfo, len(instances)) + riInfos := make([]exchange.RIInfo, 0, len(instances)) for i := range instances { inst := instances[i] - riInfos[i] = exchange.RIInfo{ + monthlyCost, err := monthlyCostFromConvertibleRI(inst) + if err != nil { + logging.Warnf("convertToExchangeTypes: excluding RI from reshape: %v", err) + continue + } + riInfos = append(riInfos, exchange.RIInfo{ ID: inst.ReservedInstanceID, InstanceType: inst.InstanceType, InstanceCount: inst.InstanceCount, OfferingClass: "convertible", NormalizationFactor: inst.NormalizationFactor, - MonthlyCost: monthlyCostFromConvertibleRI(inst), + MonthlyCost: monthlyCost, CurrencyCode: inst.CurrencyCode, // Plumb the AWS-reported RI duration straight through — // reshape's term-match guard rejects alternatives whose // TermSeconds differs from the source so a 3y RI never // surfaces as an alternative to a 1y commitment. TermSeconds: inst.Duration, - } + }) } utilInfos := make([]exchange.UtilizationInfo, len(utilData)) diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 9e3818c71..ff74d8f00 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -974,7 +974,7 @@ func TestGetReshapeRecommendations_EmptyRegionUsesConfigRegion(t *testing.T) { reshapeEC2Factory: func(_ aws.Config) reshapeEC2Client { return &fakeReshapeEC2Stub{ instances: []ec2svc.ConvertibleRI{ - {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, CurrencyCode: "USD"}, + {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, CurrencyCode: "USD", Duration: 31536000}, }, } }, @@ -1060,7 +1060,7 @@ func TestGetReshapeRecommendations_AlternativeCurrencyFromRIs(t *testing.T) { auth: &mockAuthForExchange{}, reshapeEC2Factory: func(_ aws.Config) reshapeEC2Client { return &fakeReshapeEC2Stub{instances: []ec2svc.ConvertibleRI{ - {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, CurrencyCode: tc.riCurrency}, + {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, CurrencyCode: tc.riCurrency, Duration: 31536000}, }} }, reshapeRecsFactory: func(_ aws.Config) reshapeRecsClient { @@ -2718,3 +2718,36 @@ func TestExecuteApprovedExchange_ReservationInfrastructureError_HidesDetail(t *t assert.Equal(t, "daily spending cap check failed", resp.(map[string]any)["reason"]) assert.False(t, called) } + +// A convertible RI with a non-positive Duration has no computable monthly +// cost. Converting it to MonthlyCost=0 would make reshape's pricing gate +// treat it as free and skip the dollar-units check, so it must be excluded. +func TestConvertToExchangeTypes_ExcludesNonPositiveDuration(t *testing.T) { + valid := ec2svc.ConvertibleRI{ + ReservedInstanceID: "ri-valid", InstanceType: "m5.large", InstanceCount: 2, + NormalizationFactor: 4, FixedPrice: 8760, UsagePrice: 0.01, Duration: 31536000, CurrencyCode: "USD", + } + instances := []ec2svc.ConvertibleRI{ + {ReservedInstanceID: "ri-zero", InstanceType: "m5.large", InstanceCount: 1, FixedPrice: 8760, Duration: 0}, + valid, + {ReservedInstanceID: "ri-neg", InstanceType: "c5.large", InstanceCount: 1, FixedPrice: 8760, Duration: -1}, + } + + riInfos, _ := convertToExchangeTypes(instances, nil) + + require.Len(t, riInfos, 1) + assert.Equal(t, "ri-valid", riInfos[0].ID) + assert.InDelta(t, (8760.0/8760+0.01)*730, riInfos[0].MonthlyCost, 1e-6) + assert.Equal(t, int64(31536000), riInfos[0].TermSeconds) +} + +func TestMonthlyCostFromConvertibleRI_NonPositiveDurationErrors(t *testing.T) { + for _, d := range []int64{0, -1} { + _, err := monthlyCostFromConvertibleRI(ec2svc.ConvertibleRI{ReservedInstanceID: "ri-x", FixedPrice: 100, Duration: d}) + require.Error(t, err) + assert.Contains(t, err.Error(), "ri-x") + } + cost, err := monthlyCostFromConvertibleRI(ec2svc.ConvertibleRI{FixedPrice: 8760, Duration: 31536000}) + require.NoError(t, err) + assert.InDelta(t, 730.0, cost, 1e-6) +} diff --git a/internal/server/handler_ri_exchange.go b/internal/server/handler_ri_exchange.go index 73406b5bb..86d18605f 100644 --- a/internal/server/handler_ri_exchange.go +++ b/internal/server/handler_ri_exchange.go @@ -12,6 +12,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/exchange" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" awsprovider "github.com/LeanerCloud/cloud-commitments-go/providers/aws" "github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations" ec2svc "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/ec2" @@ -243,37 +244,42 @@ func buildExchangeNotificationData(result *exchange.AutoExchangeResult, dashboar // // monthly = (FixedPrice / hours_per_term + UsagePrice + recurring_hourly) × 730 // +// Returns an error when Duration is non-positive: a zero cost would read +// as a free RI and skip the pricing gate, so callers must exclude the RI. +// // Used to populate exchange.RIInfo.MonthlyCost so the cross-family // dollar-units pre-filter compares apples-to-apples against per-target // offering costs. -func monthlyCostFromConvertibleRI(ri ec2svc.ConvertibleRI) float64 { +func monthlyCostFromConvertibleRI(ri ec2svc.ConvertibleRI) (float64, error) { if ri.Duration <= 0 { - return 0 + return 0, fmt.Errorf("convertible RI %s has non-positive duration %d: monthly cost is not computable", ri.ReservedInstanceID, ri.Duration) } hoursPerTerm := float64(ri.Duration) / 3600 - if hoursPerTerm <= 0 { - return 0 - } - return ((ri.FixedPrice / hoursPerTerm) + ri.UsagePrice + ri.RecurringHourlyAmount) * 730 + return ((ri.FixedPrice / hoursPerTerm) + ri.UsagePrice + ri.RecurringHourlyAmount) * 730, nil } // convertForAutoExchange converts provider-specific types to exchange package types, // including RI metadata needed for offering lookup. func convertForAutoExchange(instances []ec2svc.ConvertibleRI, utilData []recommendations.RIUtilization) ([]exchange.RIInfo, []exchange.UtilizationInfo, map[string]exchange.RIMetadataInfo) { - riInfos := make([]exchange.RIInfo, len(instances)) + riInfos := make([]exchange.RIInfo, 0, len(instances)) riMetadata := make(map[string]exchange.RIMetadataInfo, len(instances)) for i := range instances { inst := instances[i] - riInfos[i] = exchange.RIInfo{ + monthlyCost, err := monthlyCostFromConvertibleRI(inst) + if err != nil { + logging.Warnf("convertForAutoExchange: excluding RI from auto-exchange: %v", err) + continue + } + riInfos = append(riInfos, exchange.RIInfo{ ID: inst.ReservedInstanceID, InstanceType: inst.InstanceType, InstanceCount: inst.InstanceCount, OfferingClass: "convertible", NormalizationFactor: inst.NormalizationFactor, - MonthlyCost: monthlyCostFromConvertibleRI(inst), + MonthlyCost: monthlyCost, CurrencyCode: inst.CurrencyCode, - } + }) riMetadata[inst.ReservedInstanceID] = exchange.RIMetadataInfo{ ProductDescription: inst.ProductDescription, InstanceTenancy: inst.InstanceTenancy, diff --git a/internal/server/handler_ri_exchange_test.go b/internal/server/handler_ri_exchange_test.go index 5ab105a12..6d4d998bc 100644 --- a/internal/server/handler_ri_exchange_test.go +++ b/internal/server/handler_ri_exchange_test.go @@ -1068,3 +1068,41 @@ func (m *mockConfigStoreForExchange) ClearRevocationInFlight(_ context.Context, func (m *mockConfigStoreForExchange) GetPurchaseHistoryInFlight(_ context.Context) ([]*config.PurchaseHistoryRecord, error) { return nil, nil } + +// A convertible RI with a non-positive Duration has no computable monthly +// cost. Converting it to MonthlyCost=0 would make reshape's pricing gate +// treat it as free and skip the dollar-units check, so it must be excluded. +func TestConvertForAutoExchange_ExcludesNonPositiveDuration(t *testing.T) { + instances := []ec2svc.ConvertibleRI{ + {ReservedInstanceID: "ri-zero", InstanceType: "m5.large", InstanceCount: 1, FixedPrice: 8760, Duration: 0}, + { + ReservedInstanceID: "ri-valid", InstanceType: "m5.large", InstanceCount: 2, + NormalizationFactor: 4, FixedPrice: 8760, UsagePrice: 0.01, Duration: 31536000, CurrencyCode: "USD", + }, + {ReservedInstanceID: "ri-neg", InstanceType: "c5.large", InstanceCount: 1, FixedPrice: 8760, Duration: -1}, + } + + riInfos, _, meta := convertForAutoExchange(instances, nil) + + if len(riInfos) != 1 || riInfos[0].ID != "ri-valid" { + t.Fatalf("want only ri-valid, got %+v", riInfos) + } + if want := (8760.0/8760 + 0.01) * 730; riInfos[0].MonthlyCost < want-1e-6 || riInfos[0].MonthlyCost > want+1e-6 { + t.Fatalf("MonthlyCost = %v, want %v", riInfos[0].MonthlyCost, want) + } + if _, ok := meta["ri-zero"]; ok || len(meta) != 1 { + t.Fatalf("metadata must cover only ri-valid, got %+v", meta) + } +} + +func TestMonthlyCostFromConvertibleRI_NonPositiveDurationErrors(t *testing.T) { + for _, d := range []int64{0, -1} { + if _, err := monthlyCostFromConvertibleRI(ec2svc.ConvertibleRI{ReservedInstanceID: "ri-x", FixedPrice: 100, Duration: d}); err == nil || !strings.Contains(err.Error(), "ri-x") { + t.Fatalf("duration %d: want error naming ri-x, got %v", d, err) + } + } + cost, err := monthlyCostFromConvertibleRI(ec2svc.ConvertibleRI{FixedPrice: 8760, Duration: 31536000}) + if err != nil || cost < 730-1e-6 || cost > 730+1e-6 { + t.Fatalf("valid RI: cost=%v err=%v", cost, err) + } +} diff --git a/internal/server/ri_exchange_zone_test.go b/internal/server/ri_exchange_zone_test.go index 72b2f0b54..1515fd590 100644 --- a/internal/server/ri_exchange_zone_test.go +++ b/internal/server/ri_exchange_zone_test.go @@ -179,8 +179,8 @@ func TestNewConvertibleOfferingLookup_MapsEveryField(t *testing.T) { func TestConvertForAutoExchange_CarriesAvailabilityZone(t *testing.T) { _, _, meta := convertForAutoExchange([]ec2svc.ConvertibleRI{ - {ReservedInstanceID: "ri-zonal", Scope: "Availability Zone", AvailabilityZone: "us-east-1b"}, - {ReservedInstanceID: "ri-regional", Scope: "Region"}, + {ReservedInstanceID: "ri-zonal", Scope: "Availability Zone", AvailabilityZone: "us-east-1b", Duration: 31536000}, + {ReservedInstanceID: "ri-regional", Scope: "Region", Duration: 31536000}, }, nil) assert.Equal(t, "us-east-1b", meta["ri-zonal"].AvailabilityZone) assert.Empty(t, meta["ri-regional"].AvailabilityZone) From f17adc475007ba05ad11bd7c262a9547db6eb37d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 07:11:00 +0200 Subject: [PATCH 2/4] fix(ri-exchange): surface RIs excluded from reshape analysis Logging alone left an RI with no term length silently without a recommendation while the instances list still showed it. The reshape response now carries excluded_ris ({ri_id, reason}) and the reshape view shows a notice above the table naming them. The server auto-exchange path keeps log-only exclusion since it has no user-facing response. Refs #1181 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/riexchange.test.ts | 50 +++++++++++++++++++++ frontend/src/api/index.ts | 2 +- frontend/src/api/riexchange.ts | 7 +++ frontend/src/riexchange.ts | 27 ++++++++++++ internal/api/handler_ri_exchange.go | 21 +++++++-- internal/api/handler_ri_exchange_test.go | 54 ++++++++++++++++++++++- internal/api/openapi.yaml | 14 ++++++ 7 files changed, 169 insertions(+), 6 deletions(-) diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index 07a6f5ab8..5a534315e 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -64,6 +64,7 @@ import { loadReshapeRecommendations, loadRIExchange, openExchangeModal, + renderExcludedRIsNotice, renderReshapeStalenessBanner, setupRIExchangeHandlers, } from '../riexchange'; @@ -1313,3 +1314,52 @@ describe('exchange history table counts (issue #286)', () => { expect(cells).not.toContain('4'); }); }); + +// Issue #1181: RIs left out of reshape analysis must be visible in the UI. +describe('excluded RIs notice', () => { + let listEl: HTMLDivElement; + + beforeEach(() => { + const wrapper = document.createElement('div'); + listEl = document.createElement('div'); + listEl.id = 'ri-exchange-recommendations-list'; + wrapper.appendChild(listEl); + document.body.appendChild(wrapper); + }); + + afterEach(() => { + document.body.innerHTML = ''; + jest.resetAllMocks(); + }); + + it('lists the excluded ids as text when excluded_ris is non-empty', async () => { + (api.getReshapeRecommendations as jest.Mock).mockResolvedValue({ + recommendations: [], + excluded_ris: [ + { ri_id: 'ri-0', reason: 'no term' }, + { ri_id: '', reason: 'no term' }, + ], + }); + + await loadReshapeRecommendations(); + + const notice = document.getElementById('ri-exchange-recommendations-excluded'); + expect(notice?.textContent).toContain('2 convertible RIs were left out because AWS returned no term length'); + expect(notice?.textContent).toContain('ri-0'); + expect(notice?.textContent).toContain(''); + expect(notice?.querySelector('img')).toBeNull(); + }); + + it('shows no notice when excluded_ris is empty or absent, and clears a stale one', () => { + renderExcludedRIsNotice([{ ri_id: 'ri-0', reason: 'x' }]); + expect(document.getElementById('ri-exchange-recommendations-excluded')?.textContent).toContain('1 convertible RI was left out'); + renderExcludedRIsNotice(undefined); + expect(document.getElementById('ri-exchange-recommendations-excluded')?.textContent).toBe(''); + }); + + it('does not render a notice for an empty list on load', async () => { + (api.getReshapeRecommendations as jest.Mock).mockResolvedValue({ recommendations: [], excluded_ris: [] }); + await loadReshapeRecommendations(); + expect(document.getElementById('ri-exchange-recommendations-excluded')?.textContent ?? '').toBe(''); + }); +}); diff --git a/frontend/src/api/index.ts b/frontend/src/api/index.ts index 20a6ebb4c..562b6f5ef 100644 --- a/frontend/src/api/index.ts +++ b/frontend/src/api/index.ts @@ -193,7 +193,7 @@ export { } from './inventory'; // Re-export RI exchange functions -export type { ReshapeRecommendationsResponse } from './riexchange'; +export type { ReshapeRecommendationsResponse, ExcludedRI } from './riexchange'; export { listConvertibleRIs, listExchangeableAzureRIs, diff --git a/frontend/src/api/riexchange.ts b/frontend/src/api/riexchange.ts index d23423d78..b7ee73d11 100644 --- a/frontend/src/api/riexchange.ts +++ b/frontend/src/api/riexchange.ts @@ -61,8 +61,15 @@ export async function getRIUtilization(lookbackDays?: number): Promise e.ri_id).join(', ')}`; +} + /** * Render (or clear) the staleness banner above the reshape-recommendations * table. The banner slot is a sibling element with id diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 745db2e36..ed6ee02d5 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -1614,13 +1614,17 @@ func monthlyCostFromConvertibleRI(ri ec2svc.ConvertibleRI) (float64, error) { } // convertToExchangeTypes converts provider-specific types to the exchange package types. -func convertToExchangeTypes(instances []ec2svc.ConvertibleRI, utilData []recommendations.RIUtilization) ([]exchange.RIInfo, []exchange.UtilizationInfo) { +// RIs whose monthly cost cannot be computed are returned as excluded rather +// than dropped silently, so the response can surface them. +func convertToExchangeTypes(instances []ec2svc.ConvertibleRI, utilData []recommendations.RIUtilization) ([]exchange.RIInfo, []exchange.UtilizationInfo, []ExcludedRI) { + var excluded []ExcludedRI riInfos := make([]exchange.RIInfo, 0, len(instances)) for i := range instances { inst := instances[i] monthlyCost, err := monthlyCostFromConvertibleRI(inst) if err != nil { logging.Warnf("convertToExchangeTypes: excluding RI from reshape: %v", err) + excluded = append(excluded, ExcludedRI{RIID: inst.ReservedInstanceID, Reason: "AWS returned no term length (duration), so its monthly cost cannot be computed"}) continue } riInfos = append(riInfos, exchange.RIInfo{ @@ -1647,7 +1651,7 @@ func convertToExchangeTypes(instances []ec2svc.ConvertibleRI, utilData []recomme } } - return riInfos, utilInfos + return riInfos, utilInfos, excluded } // reshapeRequestParams groups parsed query parameters for getReshapeRecommendations. @@ -1726,7 +1730,7 @@ func (h *Handler) getReshapeRecommendations(ctx context.Context, req *events.Lam return nil, fmt.Errorf("failed to get RI utilization: %w", err) } - riInfos, utilInfos := convertToExchangeTypes(instances, utilData) + riInfos, utilInfos, excluded := convertToExchangeTypes(instances, utilData) // Cross-family alternatives are sourced from the cached AWS Cost // Explorer purchase recommendations table in Postgres — no per-rec // DescribeReservedInstancesOfferings fan-out, no hand-curated @@ -1745,7 +1749,7 @@ func (h *Handler) getReshapeRecommendations(ctx context.Context, req *events.Lam lookup := purchaseRecLookupFromStore(h.config, cloudAccountID) recs := exchange.AnalyzeReshapingWithRecs(ctx, riInfos, utilInfos, p.threshold, p.region, currencyCode, lookup) - resp := &ReshapeRecommendationsResponse{Recommendations: recs} + resp := &ReshapeRecommendationsResponse{Recommendations: recs, ExcludedRIs: excluded} h.attachReshapeStaleness(ctx, resp) return resp, nil } @@ -2043,6 +2047,15 @@ type ReshapeRecommendationsResponse struct { RecsCollectedAt *time.Time `json:"recs_collected_at,omitempty"` RecsStaleness string `json:"recs_staleness,omitempty"` Recommendations []exchange.ReshapeRecommendation `json:"recommendations"` + // ExcludedRIs lists RIs left out of cost-based analysis (and so have no + // recommendation) because their monthly cost could not be computed. + ExcludedRIs []ExcludedRI `json:"excluded_ris,omitempty"` +} + +// ExcludedRI names a convertible RI omitted from reshape analysis and why. +type ExcludedRI struct { + RIID string `json:"ri_id"` + Reason string `json:"reason"` } // reshapeSoftStaleThreshold is the age at which the reshape recs banner diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index ff74d8f00..499923dd2 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -2733,12 +2733,64 @@ func TestConvertToExchangeTypes_ExcludesNonPositiveDuration(t *testing.T) { {ReservedInstanceID: "ri-neg", InstanceType: "c5.large", InstanceCount: 1, FixedPrice: 8760, Duration: -1}, } - riInfos, _ := convertToExchangeTypes(instances, nil) + riInfos, _, excluded := convertToExchangeTypes(instances, nil) require.Len(t, riInfos, 1) assert.Equal(t, "ri-valid", riInfos[0].ID) assert.InDelta(t, (8760.0/8760+0.01)*730, riInfos[0].MonthlyCost, 1e-6) assert.Equal(t, int64(31536000), riInfos[0].TermSeconds) + require.Len(t, excluded, 2) + assert.Equal(t, "ri-zero", excluded[0].RIID) + assert.Equal(t, "ri-neg", excluded[1].RIID) + assert.Contains(t, excluded[0].Reason, "term length") +} + +// The reshape response must name RIs left out of cost-based analysis so the +// UI can show them instead of silently giving them no recommendation. +func TestGetReshapeRecommendations_ReportsExcludedRIs(t *testing.T) { + mockStore := &MockConfigStore{} + mockStore.On("ListStoredRecommendations", mock.Anything, mock.Anything). + Return([]config.RecommendationRecord(nil), nil) + h := &Handler{ + config: mockStore, + auth: &mockAuthForExchange{}, + reshapeEC2Factory: func(_ aws.Config) reshapeEC2Client { + return &fakeReshapeEC2Stub{instances: []ec2svc.ConvertibleRI{ + {ReservedInstanceID: "ri-bad", InstanceType: "m5.xlarge", InstanceCount: 1, FixedPrice: 8760, Duration: 0, CurrencyCode: "USD"}, + {ReservedInstanceID: "ri-ok", InstanceType: "m5.xlarge", InstanceCount: 1, FixedPrice: 8760, Duration: 31536000, CurrencyCode: "USD"}, + }} + }, + reshapeRecsFactory: func(_ aws.Config) reshapeRecsClient { + return &fakeReshapeRecsStub{utilization: []recommendations.RIUtilization{ + {ReservedInstanceID: "ri-bad", UtilizationPercent: 50.0}, + {ReservedInstanceID: "ri-ok", UtilizationPercent: 50.0}, + }} + }, + reshapeAccountResolver: func(_ context.Context) (string, error) { return "", nil }, + } + h.awsCfgOnce.Do(func() { h.awsCfg = aws.Config{Region: "us-east-1"} }) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer test-token"}, + QueryStringParameters: map[string]string{"lookback_days": "30", "threshold": "95.0"}, + } + out, err := h.getReshapeRecommendations(context.Background(), req) + require.NoError(t, err) + resp, ok := out.(*ReshapeRecommendationsResponse) + require.True(t, ok) + + require.Len(t, resp.ExcludedRIs, 1) + assert.Equal(t, "ri-bad", resp.ExcludedRIs[0].RIID) + assert.NotEmpty(t, resp.ExcludedRIs[0].Reason) + require.NotEmpty(t, resp.Recommendations) + for _, r := range resp.Recommendations { + assert.NotEqual(t, "ri-bad", r.SourceRIID) + } + assert.Contains(t, resp.Recommendations[0].SourceRIID, "ri-ok") + + raw, err := json.Marshal(resp) + require.NoError(t, err) + assert.Contains(t, string(raw), `"excluded_ris":[{"ri_id":"ri-bad"`) } func TestMonthlyCostFromConvertibleRI_NonPositiveDurationErrors(t *testing.T) { diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 9667c4d6d..31f3dbd43 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -1329,6 +1329,20 @@ paths: application/json: schema: type: object + properties: + excluded_ris: + type: array + description: > + Convertible RIs left out of cost-based analysis (for + example AWS returned no term length), so they have no + recommendation. Omitted when empty. + items: + type: object + properties: + ri_id: + type: string + reason: + type: string '401': $ref: '#/components/responses/Unauthorized' '403': From edb51107e8f7723ad109da1d638d3f712efa1038 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 07:43:32 +0200 Subject: [PATCH 3/4] test(ri-exchange): give integration RI fixtures a term length The reshape integration fixtures had no Duration, so the new non-positive duration guard excluded them. They now use a 1-year term, and the end-to-end case also asserts an RI without a term is reported in excluded_ris. Refs #1181 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_ri_exchange_integration_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/internal/api/handler_ri_exchange_integration_test.go b/internal/api/handler_ri_exchange_integration_test.go index 67057dce8..13b71e06a 100644 --- a/internal/api/handler_ri_exchange_integration_test.go +++ b/internal/api/handler_ri_exchange_integration_test.go @@ -135,7 +135,8 @@ func TestReshapeRecommendations_Integration_EndToEnd(t *testing.T) { ec2Fake := &fakeReshapeEC2{ instances: []ec2svc.ConvertibleRI{ - {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1}, + {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000}, + {ReservedInstanceID: "ri-noterm", InstanceType: "m5.xlarge", InstanceCount: 1}, }, } recsFake := &fakeReshapeRecs{ @@ -153,6 +154,8 @@ func TestReshapeRecommendations_Integration_EndToEnd(t *testing.T) { rec := body.Recommendations[0] require.Equal(t, "ri-1", rec.SourceRIID) require.Equal(t, "m5.large", rec.TargetInstanceType) + require.Len(t, body.ExcludedRIs, 1, "an RI with no term length must be reported, not silently skipped") + require.Equal(t, "ri-noterm", body.ExcludedRIs[0].RIID) // Same-family m5 stripped; c5 + r5 surface ascending by cost. require.Len(t, rec.AlternativeTargets, 2, @@ -197,7 +200,7 @@ func TestReshapeRecommendations_Integration_SecondCallHitsCache(t *testing.T) { ec2Fake := &fakeReshapeEC2{ instances: []ec2svc.ConvertibleRI{ - {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1}, + {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000}, }, } recsFake := &fakeReshapeRecs{ @@ -233,7 +236,7 @@ func TestReshapeRecommendations_Integration_NoCachedRecsReturnsPrimaryOnly(t *te ec2Fake := &fakeReshapeEC2{ instances: []ec2svc.ConvertibleRI{ - {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1}, + {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000}, }, } recsFake := &fakeReshapeRecs{ @@ -334,7 +337,7 @@ func TestReshapeRecommendations_Integration_ScopedAccount_FiltersToAccount(t *te ec2Fake := &fakeReshapeEC2{ instances: []ec2svc.ConvertibleRI{ - {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1}, + {ReservedInstanceID: "ri-1", InstanceType: "m5.xlarge", InstanceCount: 1, Duration: 31536000}, }, } recsFake := &fakeReshapeRecs{ From 4956af611aa8c298b570b9f6d296c38e33bf0264 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 08:17:27 +0200 Subject: [PATCH 4/4] fix(ri-exchange): clear the excluded-RI notice when the AWS view is left The notice lived in a sibling slot that the account-resolution error, GCP, Azure and failed-reload paths never cleared, so it could outlive the AWS reshape view it described. Refs #1181 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/riexchange.test.ts | 38 +++++++++++++++++++++++ frontend/src/riexchange.ts | 4 +++ 2 files changed, 42 insertions(+) diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index 5a534315e..e9f42d41c 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -1272,6 +1272,44 @@ describe('RI Exchange global filter scoping (issue #871)', () => { // Modal is closed synchronously on the chip event. expect(modal.classList.contains('hidden')).toBe(true); }); + + describe('excluded-RI notice lifecycle (issue #1181)', () => { + const noticeText = (): string => document.getElementById('ri-exchange-recommendations-excluded')?.textContent ?? ''; + + beforeEach(async () => { + (api.getReshapeRecommendations as jest.Mock).mockResolvedValue({ + recommendations: [], + excluded_ris: [{ ri_id: 'ri-noterm', reason: 'no term' }], + }); + await loadRIExchange(); + expect(noticeText()).toContain('ri-noterm'); + }); + + it('clears after a failed reload', async () => { + (api.getReshapeRecommendations as jest.Mock).mockRejectedValue(new Error('boom')); + await loadReshapeRecommendations(); + expect(noticeText()).toBe(''); + }); + + it('clears when switching to GCP', async () => { + stateMod().getCurrentProvider.mockReturnValue('gcp'); + await loadRIExchange(); + expect(noticeText()).toBe(''); + }); + + it('clears when switching to Azure', async () => { + stateMod().getCurrentProvider.mockReturnValue('azure'); + stateMod().getCurrentAccountIDs.mockReturnValue(['uuid-az']); + await loadRIExchange(); + expect(noticeText()).toBe(''); + }); + + it('clears on an account-resolution error', async () => { + stateMod().getCurrentAccountIDs.mockReturnValue(['uuid-missing']); + await loadRIExchange(); + expect(noticeText()).toBe(''); + }); + }); }); describe('exchange history table counts (issue #286)', () => { diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index 66961bbab..4b0855f7b 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -116,6 +116,7 @@ export async function loadRIExchange(): Promise { if (el) renderEmptyState(el, message); } renderReshapeStalenessBanner('', null); + renderExcludedRIsNotice(undefined); const container = document.getElementById('ri-exchange-instances-list'); if (container) { container.innerHTML = `

Failed to resolve the selected account: ${escapeHtml((error as Error).message)}

`; @@ -168,6 +169,7 @@ function renderGCPEmptyStates(): void { if (history) renderEmptyState(history, "RI Exchange isn't available for GCP."); renderReshapeStalenessBanner('', null); + renderExcludedRIsNotice(undefined); } /** @@ -182,6 +184,7 @@ function renderReshapeNotApplicable(provider: string): void { renderEmptyState(container, `Reshape recommendations are not available for ${providerLabel(provider)}.`); } renderReshapeStalenessBanner('', null); + renderExcludedRIsNotice(undefined); } /** @@ -538,6 +541,7 @@ export async function loadReshapeRecommendations(generation: number = scopeGener } catch (error) { if (generation !== scopeGeneration) return; teardownSkeleton(container); + renderExcludedRIsNotice(undefined); const err = error as Error; container.innerHTML = `

Failed to load recommendations: ${escapeHtml(err.message)}

`; }