diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index 9b8c05ec0..e10ed2f94 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -936,6 +936,17 @@ describe('reshape recommendations empty state', () => { normalization_factor: 4, }; + const utilFor = (id: string) => ({ + reserved_instance_id: id, + utilization_percent: 99, + purchased_hours: 720, + total_actual_hours: 713, + unused_hours: 7, + }); + + const utilizationCell = (): string => + instancesEl.querySelector('tbody tr td:nth-child(7)')?.textContent?.trim() ?? ''; + beforeEach(() => { instancesEl = document.createElement('div'); instancesEl.id = 'ri-exchange-instances-list'; @@ -983,8 +994,10 @@ describe('reshape recommendations empty state', () => { it('claims all RIs meet the threshold only when RIs actually exist', async () => { (api.listConvertibleRIs as jest.Mock).mockResolvedValue([sampleRI]); + (api.getRIUtilization as jest.Mock).mockResolvedValue([utilFor('ri-1')]); await loadRIExchange(); + await flushPromises(); expect(recsEl.textContent).toContain('meet your utilization threshold'); expect(recsEl.textContent).toContain('1 convertible RI '); @@ -994,8 +1007,10 @@ describe('reshape recommendations empty state', () => { // Issue #1135: the three loads race. An empty reshape response that lands // before the RI list must not keep the previous scope's count. it('re-renders the empty-state count when recommendations resolve before the RI list', async () => { + (api.getRIUtilization as jest.Mock).mockResolvedValue(['ri-1', 'ri-2', 'ri-3'].map(utilFor)); (api.listConvertibleRIs as jest.Mock).mockResolvedValueOnce([sampleRI]); await loadRIExchange(); + await flushPromises(); expect(recsEl.textContent).toContain('All 1 convertible RI '); let releaseRIs: (v: unknown) => void = () => undefined; @@ -1009,6 +1024,7 @@ describe('reshape recommendations empty state', () => { releaseRIs([sampleRI, { ...sampleRI, reserved_instance_id: 'ri-2' }, { ...sampleRI, reserved_instance_id: 'ri-3' }]); await reload; + await flushPromises(); expect(recsEl.textContent).toContain('All 3 convertible RIs meet your utilization threshold'); }); @@ -1032,6 +1048,198 @@ describe('reshape recommendations empty state', () => { expect(recsEl.querySelector('.error')?.textContent).toContain('Failed to load recommendations: ce down'); expect(recsEl.textContent).not.toContain('utilization threshold'); }); + + // Issue #1687: an RI with no utilization data was shown as "..." forever + // while the reshape panel claimed it met the utilization threshold. + describe('unknown utilization (issue #1687)', () => { + it('shows "No data" and makes no threshold claim when utilization has no entry for the RI', async () => { + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([sampleRI]); + (api.getRIUtilization as jest.Mock).mockResolvedValue([]); + + await loadRIExchange(); + await flushPromises(); + + expect(utilizationCell()).toBe('No data'); + expect(instancesEl.querySelector('.loading-inline')).toBeNull(); + expect(recsEl.textContent).not.toContain('meet your utilization threshold'); + expect(recsEl.textContent).toContain('Utilization data is unavailable for 1 of 1 convertible RI'); + }); + + it('shows "No data" and makes no threshold claim when the utilization load fails', async () => { + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([sampleRI]); + (api.getRIUtilization as jest.Mock).mockRejectedValue(new Error('ce down')); + jest.spyOn(console, 'error').mockImplementation(() => undefined); + + await loadRIExchange(); + await flushPromises(); + + expect(utilizationCell()).toBe('No data'); + expect(recsEl.textContent).not.toContain('meet your utilization threshold'); + expect(recsEl.textContent).toContain('Utilization data is unavailable for 1 of 1 convertible RI'); + }); + + it('counts only the RIs without utilization when some have it', async () => { + const ri2 = { ...sampleRI, reserved_instance_id: 'ri-2' }; + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([sampleRI, ri2]); + (api.getRIUtilization as jest.Mock).mockResolvedValue([utilFor('ri-1')]); + + await loadRIExchange(); + await flushPromises(); + + expect(recsEl.textContent).toContain('Utilization data is unavailable for 1 of 2 convertible RIs'); + expect(recsEl.textContent).not.toContain('meet your utilization threshold'); + }); + + it('keeps the loading placeholder and makes no claim while utilization is still loading', async () => { + // A fresh id: module state keeps the last known utilization per RI. + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([{ ...sampleRI, reserved_instance_id: 'ri-pending' }]); + (api.getRIUtilization as jest.Mock).mockReturnValue(new Promise(() => undefined)); + + await loadRIExchange(); + await flushPromises(); + + expect(instancesEl.querySelector('.loading-inline')).not.toBeNull(); + expect(recsEl.textContent).not.toContain('meet your utilization threshold'); + expect(recsEl.textContent).not.toContain('unavailable'); + expect(recsEl.textContent).toContain('No reshape recommendations right now.'); + }); + + it('keeps a reshape load error when utilization arrives afterwards', async () => { + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([sampleRI]); + (api.getReshapeRecommendations as jest.Mock).mockRejectedValueOnce(new Error('ce down')); + (api.getRIUtilization as jest.Mock).mockResolvedValue([utilFor('ri-1')]); + + await loadRIExchange(); + await flushPromises(); + + expect(recsEl.querySelector('.error')?.textContent).toContain('Failed to load recommendations: ce down'); + }); + + // Slow Cost Explorer: utilization lands after loadRIExchange has already + // rendered the neutral reshape copy. + const deferredUtilization = () => { + let resolve: (v: unknown) => void = () => undefined; + let reject: (e: unknown) => void = () => undefined; + (api.getRIUtilization as jest.Mock).mockReturnValueOnce(new Promise((res, rej) => { resolve = res; reject = rej; })); + return { resolve: (v: unknown) => resolve(v), reject: (e: unknown) => reject(e) }; + }; + + it('re-renders the reshape copy when utilization lands after the page loaded', async () => { + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([{ ...sampleRI, reserved_instance_id: 'ri-late' }]); + const util = deferredUtilization(); + + await loadRIExchange(); + expect(recsEl.textContent).toContain('No reshape recommendations right now.'); + + util.resolve([utilFor('ri-late')]); + await flushPromises(); + expect(recsEl.textContent).toContain('All 1 convertible RI meet your utilization threshold'); + }); + + it('re-renders the reshape copy when a late utilization load fails', async () => { + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([{ ...sampleRI, reserved_instance_id: 'ri-late-fail' }]); + jest.spyOn(console, 'error').mockImplementation(() => undefined); + const util = deferredUtilization(); + + await loadRIExchange(); + util.reject(new Error('ce timeout')); + await flushPromises(); + + expect(recsEl.textContent).toContain('Utilization data is unavailable for 1 of 1 convertible RI'); + expect(utilizationCell()).toBe('No data'); + expect(instancesEl.querySelector('td span[title]')?.getAttribute('title')).toBe('Utilization could not be loaded'); + }); + + it('ignores a superseded utilization failure', async () => { + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([{ ...sampleRI, reserved_instance_id: 'ri-superseded' }]); + jest.spyOn(console, 'error').mockImplementation(() => undefined); + const first = deferredUtilization(); + await loadRIExchange(); + deferredUtilization(); // the second load's utilization stays pending + await loadRIExchange(); + + first.reject(new Error('stale ce timeout')); + await flushPromises(); + + expect(instancesEl.querySelector('.loading-inline')).not.toBeNull(); + expect(recsEl.textContent).not.toContain('unavailable'); + }); + + it.each([ + ['fails', (u: ReturnType) => u.reject(new Error('ce timeout'))], + ['succeeds', (u: ReturnType) => u.resolve([utilFor('ri-switch')])], + ])('a late AWS utilization load that %s leaves the Azure view alone', async (_label, settle) => { + const stateMod = jest.requireMock('../state') as { getCurrentProvider: jest.Mock }; + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([{ ...sampleRI, reserved_instance_id: 'ri-switch' }]); + jest.spyOn(console, 'error').mockImplementation(() => undefined); + const util = deferredUtilization(); + await loadRIExchange(); + + stateMod.getCurrentProvider.mockReturnValue('azure'); + (api.listExchangeableAzureRIs as jest.Mock).mockResolvedValue([ + { reservation_order_id: 'o1', reservation_id: 'r1', sku: 'Standard_D2s_v3', quantity: 2, region: 'eastus', term: 'P1Y', instance_flexibility: 'On', display_name: 'web-rsv' }, + ]); + await loadRIExchange(); + settle(util); + await flushPromises(); + + expect(instancesEl.textContent).toContain('web-rsv'); + expect(recsEl.textContent).toContain('not available for Azure'); + }); + + it('a late failed AWS utilization load leaves the GCP view alone', async () => { + const stateMod = jest.requireMock('../state') as { getCurrentProvider: jest.Mock }; + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([{ ...sampleRI, reserved_instance_id: 'ri-gcp' }]); + jest.spyOn(console, 'error').mockImplementation(() => undefined); + const util = deferredUtilization(); + await loadRIExchange(); + + stateMod.getCurrentProvider.mockReturnValue('gcp'); + await loadRIExchange(); + util.reject(new Error('ce timeout')); + await flushPromises(); + + expect(instancesEl.textContent).toContain("isn't available for GCP"); + expect(recsEl.textContent).toContain("isn't available for GCP"); + }); + + it('drops the previous values when a same-scope reload fails to load utilization', async () => { + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([{ ...sampleRI, reserved_instance_id: 'ri-reload' }]); + (api.getRIUtilization as jest.Mock).mockResolvedValueOnce([utilFor('ri-reload')]); + await loadRIExchange(); + await flushPromises(); + expect(utilizationCell()).toBe('99.0%'); + + jest.spyOn(console, 'error').mockImplementation(() => undefined); + (api.getRIUtilization as jest.Mock).mockRejectedValueOnce(new Error('ce timeout')); + await loadRIExchange(); + await flushPromises(); + + expect(utilizationCell()).toBe('No data'); + expect(recsEl.textContent).toContain('Utilization data is unavailable for 1 of 1 convertible RI'); + expect(recsEl.textContent).not.toContain('meet your utilization threshold'); + }); + + it('ignores a utilization result that lands while the next scope resolves its account', async () => { + const stateMod = jest.requireMock('../state') as { getCurrentAccountIDs: jest.Mock }; + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([{ ...sampleRI, reserved_instance_id: 'ri-resolving' }]); + const util = deferredUtilization(); + await loadRIExchange(); + + let releaseAccounts: (v: unknown) => void = () => undefined; + (api.listAccountsMinimal as jest.Mock).mockReturnValueOnce(new Promise(r => { releaseAccounts = r; })); + stateMod.getCurrentAccountIDs.mockReturnValue(['uuid-1']); + const next = loadRIExchange(); + util.resolve([utilFor('ri-resolving')]); + await flushPromises(); + + // The previous scope's late result must not render while the new scope is pending. + expect(instancesEl.querySelector('.loading-inline')).not.toBeNull(); + + releaseAccounts([{ id: 'uuid-1', name: 'Payer', external_id: '123456789012', provider: 'aws' }]); + await next; + }); + }); }); // Staleness banner tests for issue #150. diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index a45b244d2..af4b8a847 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -48,7 +48,13 @@ let currentRIs: ConvertibleRI[] = []; // that failed to load (issue #1135). let currentRIsLoaded = false; let currentUtilization: Map = new Map(); +// An RI missing from currentUtilization is still loading only while this is +// 'loading'; after that its utilization is unknown (issue #1687). +let utilizationStatus: 'loading' | 'loaded' | 'failed' = 'loading'; let currentRecommendations: ReshapeRecommendation[] = []; +// True once currentRecommendations holds this scope's successfully loaded +// reshape list, so a late utilization result never overwrites a load error. +let currentRecsLoaded = false; // Registered account names by AWS account ID, for the account label (#1692). let riAccountNames: Map = new Map(); const exchangePaymentPattern = /^\d+(?:\.\d+)?$/; @@ -108,16 +114,20 @@ const MODE_LABELS: Record = { manual: "Manual Approval", auto: " */ export async function loadRIExchange(): Promise { const generation = ++scopeGeneration; + // Supersede any in-flight utilization load from the previous scope, before + // the account lookup, so a late result can't re-render over the new view. + utilizationGeneration++; let scope: Awaited>; try { scope = await resolveScope(); } catch (error) { if (generation !== scopeGeneration) return; - utilizationGeneration++; currentRIs = []; currentRIsLoaded = false; currentUtilization = new Map(); + utilizationStatus = 'loading'; currentRecommendations = []; + currentRecsLoaded = false; currentRIAccountID = undefined; const message = 'Select a valid account to load this section.'; for (const id of ['ri-exchange-recommendations-list', 'ri-exchange-history-list']) { @@ -172,7 +182,9 @@ function renderGCPEmptyStates(): void { currentRIs = []; currentRIsLoaded = false; currentUtilization = new Map(); + utilizationStatus = 'loading'; currentRecommendations = []; + currentRecsLoaded = false; currentRIAccountID = undefined; const instances = document.getElementById('ri-exchange-instances-list'); @@ -195,6 +207,7 @@ function renderGCPEmptyStates(): void { */ function renderReshapeNotApplicable(provider: string): void { currentRecommendations = []; + currentRecsLoaded = false; const container = document.getElementById('ri-exchange-recommendations-list'); if (container) { renderEmptyState(container, `Reshape recommendations are not available for ${providerLabel(provider)}.`); @@ -326,6 +339,7 @@ async function loadConvertibleRIs(accountID?: string, generation: number = scope if (generation !== scopeGeneration) return; currentRIs = ris; currentRIsLoaded = true; + utilizationStatus = 'loading'; renderRIsTable(container, accountID); // Load utilization asynchronously (Cost Explorer is slow) utilizationGeneration++; @@ -359,6 +373,7 @@ async function loadExchangeableAzureRIs(accountID: string | undefined, generatio currentRIs = []; currentRIsLoaded = false; currentUtilization = new Map(); + utilizationStatus = 'loading'; currentRIAccountID = undefined; try { @@ -398,13 +413,25 @@ async function loadUtilization(generation: number): Promise { // Discard if a newer load was started while we were waiting if (generation !== utilizationGeneration) return; currentUtilization = new Map(utilization.map(u => [u.reserved_instance_id, u])); - // Re-render table with utilization data, preserving the active account - // scope so a scoped empty-state message isn't lost (issue #871). - const container = document.getElementById('ri-exchange-instances-list'); - if (container) renderRIsTable(container, currentRIAccountID); + utilizationStatus = 'loaded'; } catch (error) { + if (generation !== utilizationGeneration) return; console.error('Failed to load RI utilization:', error); + // Values from an earlier load are no longer current (same-scope reload). + currentUtilization = new Map(); + utilizationStatus = 'failed'; } + // Re-render table with utilization data, preserving the active account + // scope so a scoped empty-state message isn't lost (issue #871). + const container = document.getElementById('ri-exchange-instances-list'); + if (container) renderRIsTable(container, currentRIAccountID); + // The reshape empty state depends on which RIs have known utilization. + if (currentRecsLoaded && currentRecommendations.length === 0) rerenderReshape(); +} + +function hasKnownUtilization(riID: string): boolean { + const pct = currentUtilization.get(riID)?.utilization_percent; + return typeof pct === 'number' && Number.isFinite(pct); } function renderRIsTable(container: HTMLElement, accountID?: string): void { @@ -455,10 +482,15 @@ function renderRIsTable(container: HTMLElement, accountID?: string): void { ${visibleRIs.map(ri => { - const util = currentUtilization.get(ri.reserved_instance_id); - const utilPct = util ? util.utilization_percent : null; + const utilPct = hasKnownUtilization(ri.reserved_instance_id) + ? currentUtilization.get(ri.reserved_instance_id)!.utilization_percent + : null; const utilClass = utilPct === null ? '' : utilPct >= 95 ? 'util-green' : utilPct >= 70 ? 'util-yellow' : 'util-red'; - const utilText = utilPct === null ? '...' : `${utilPct.toFixed(1)}%`; + const utilText = utilPct !== null + ? `${utilPct.toFixed(1)}%` + : utilizationStatus === 'loading' + ? '...' + : `No data`; return ` ${escapeHtml(ri.reserved_instance_id)} @@ -585,11 +617,13 @@ export async function loadReshapeRecommendations(generation: number = scopeGener // renderRecommendations: Source RI / Current / Suggested / // Alternatives / Utilization / Normalized Units / Reason / Actions. showSkeletonRows(container, 3, 8); + currentRecsLoaded = false; try { const resp: ReshapeRecommendationsResponse = await api.getReshapeRecommendations(); if (generation !== scopeGeneration) return false; currentRecommendations = resp.recommendations ?? []; + currentRecsLoaded = true; renderRecommendations(container); renderReshapeStalenessBanner(resp.recs_staleness, resp.recs_collected_at); renderExcludedRIsNotice(resp.excluded_ris); @@ -974,6 +1008,19 @@ function rerenderReshape(): void { if (container) renderRecommendations(container); } +// Empty-state copy for a loaded, non-empty RI list. Only RIs with known +// utilization can be said to meet the threshold (issue #1687). +function reshapeFleetCopy(): string { + const total = currentRIs.length; + const plural = total === 1 ? '' : 's'; + if (utilizationStatus === 'loading') return 'No reshape recommendations right now.'; + const unknown = currentRIs.filter(ri => !hasKnownUtilization(ri.reserved_instance_id)).length; + if (unknown === 0) { + return `All ${total} convertible RI${plural} meet your utilization threshold. No reshape needed.`; + } + return `No reshape recommendations right now. Utilization data is unavailable for ${unknown} of ${total} convertible RI${plural}, so ${unknown === 1 ? 'it was' : 'they were'} not checked against your utilization threshold.`; +} + function renderRecommendations(container: HTMLElement): void { if (!currentRecommendations || currentRecommendations.length === 0) { // The "well-utilized" copy is only truthful when the RI fleet actually @@ -986,7 +1033,7 @@ function renderRecommendations(container: HTMLElement): void { ? 'No reshape recommendations right now.' : currentRIs.length === 0 ? 'Reshape recommendations appear once your accounts have active convertible Reserved Instances — none are registered yet.' - : `All ${currentRIs.length} convertible RI${currentRIs.length === 1 ? '' : 's'} meet your utilization threshold. No reshape needed.`; + : reshapeFleetCopy(); container.innerHTML = `

${escapeHtml(copy)}

`; return; }