Skip to content

fix(api/dashboard): exclude revoked commitments, stop fabricating current_savings, provider-filter KPIs (adversarial-review follow-ups) #1452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cristim wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(api/dashboard): exclude revoked commitments, stop fabricating current_savings, provider-filter KPIs (adversarial-review follow-ups) #1452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2f5b132
fix(api/dashboard): exclude revoked commitments, stop fabricating cur…
cristim Jul 17, 2026
27a7151
fix(api/types): correct "cancelled" misspelling to "canceled" in comment
cristim Jul 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 52 additions & 18 deletions internal/api/handler_dashboard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,13 @@ func (h *Handler) getDashboardSummary(ctx context.Context, req *events.LambdaFun

totalSavings, byService := summarizeRecommendationsWithCoverage(recommendations, coverageByKey)
targetCoverage := h.resolveTargetCoverage(ctx)
activeCommitments, committedMonthly, ytdSavings, currentSavingsByService := h.calculateCommitmentMetrics(ctx, accountUUIDs, accountExternalIDsByProvider)
activeCommitments, committedMonthly, ytdSavings, currentSavingsByService := h.calculateCommitmentMetrics(ctx, params["provider"], accountUUIDs, accountExternalIDsByProvider)

// Populate CurrentSavings on each per-service bucket so the Home page
// chart can render the green "Current Savings" bars with real data.
// Before this fix, CurrentSavings was always zero because the aggregation
// in summarizeRecommendationsWithCoverage only filled PotentialSavings.
// Populate CurrentSavings on each per-service bucket from actual
// purchase_history commitments. Services with recommendations but no
// active commitments correctly keep CurrentSavings=0 (absent from
// currentSavingsByService). summarizeRecommendationsWithCoverage only
// sets PotentialSavings; CurrentSavings is exclusively owned here.
for svc, monthlySavings := range currentSavingsByService {
entry := byService[svc]
entry.CurrentSavings = monthlySavings
Expand Down Expand Up @@ -251,6 +252,13 @@ func summarizeRecommendationsWithCoverage( //nolint:gocritic // unnamedResult: r
total += scaled
svc := byService[rep.rec.Service]
svc.PotentialSavings += scaled
// CurrentSavings is populated by getDashboardSummary from actual
// purchase_history commitments (calculateCommitmentMetrics). It is NOT
// derived from recommendations here: doing so would fabricate
// CurrentSavings == PotentialSavings for services with no active
// commitments (i.e. recommend-but-never-purchased). Services absent
// from currentSavingsByService correctly default to 0 per the
// dashboard.ts contract.
byService[rep.rec.Service] = svc
}
return total, byService
Expand Down Expand Up @@ -559,16 +567,23 @@ func commitmentExpiry(p config.PurchaseHistoryRecord) time.Time {
}

// isActiveCommitment reports whether the purchase is active: its term has not
// yet expired as of `now` AND its status is one of the successful terminal
// states ("" for DB-backed rows where the column is unpersisted, or
// "completed"). Rows synthesized from failed/canceled/expired executions
// carry a non-empty status other than "completed" and are excluded so they
// do not inflate the committed_monthly KPI. The boundary is strict (After):
// a commitment is active right up to the instant its term ends.
// yet expired as of `now`, its status is one of the successful terminal states
// ("" for DB-backed rows where the column is unpersisted, or "completed"), and
// it has not been revoked. Rows synthesized from failed/canceled/expired
// executions carry a non-empty status other than "completed" and are excluded
// so they do not inflate the committed_monthly KPI. The boundary is strict
// (After): a commitment is active right up to the instant its term ends.
//
// Same predicate shared by the dashboard aggregate and the per-commitment
// inventory endpoint. Status values: see PurchaseHistoryRecord.Status doc.
func isActiveCommitment(p config.PurchaseHistoryRecord, now time.Time) bool {
// Revoked commitments are never active regardless of status or term.
// GetActivePurchaseHistory already excludes them in SQL; this is a
// defense-in-depth guard for any caller that passes rows from a different
// query (e.g. GetPurchaseHistoryFiltered).
if p.RevokedAt != nil {
return false
}
// Status is unpersisted (dynamodbav:"-"); DB rows always read back as "".
// Synthesized rows set it to "failed", "expired", "canceled", "pending",
// "notified", "approved", "running", or "paused". Only "" and "completed"
Expand Down Expand Up @@ -598,10 +613,10 @@ func aggregateActiveCommitmentsPerService(purchases []config.PurchaseHistoryReco

// fetchCommitmentPurchases loads the active purchase_history rows that
// calculateCommitmentMetrics aggregates, applying the pre-resolved account
// scope. Extracted to keep the parent under the cyclomatic limit (mirrors the
// appendAccountPredicate / accountMatchesFilters pattern). Returns ok=false on
// a store error or an explicit zero-account scope so the caller emits zeroed
// KPIs without querying.
// scope and an optional provider filter. Extracted to keep the parent under the
// cyclomatic limit (mirrors the appendAccountPredicate / accountMatchesFilters
// pattern). Returns ok=false on a store error or an explicit zero-account scope
// so the caller emits zeroed KPIs without querying.
//
// - non-nil but empty accountUUIDs (no external groups): explicit "scoped to
// zero accounts" sentinel (a restricted session whose allowed_accounts match
Expand All @@ -613,7 +628,10 @@ func aggregateActiveCommitmentsPerService(purchases []config.PurchaseHistoryReco
// row cap, so older-but-still-active 1y/3y commitments cannot be silently
// dropped the way a newest-first LIMIT 1000 page dropped them (issue #1140);
// the result is bounded by the number of live commitments.
func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, bool) {
// - provider: when non-empty, only purchases for that provider are returned,
// mirroring fetchCommitmentRecords' in-memory provider filter so KPIs match
// the provider chip selection.
func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time, provider string, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) ([]config.PurchaseHistoryRecord, bool) {
if accountUUIDs != nil && len(accountUUIDs) == 0 && len(accountExternalIDsByProvider) == 0 {
return nil, false
}
Expand All @@ -623,6 +641,17 @@ func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time,
logging.Errorf("dashboard: failed to fetch commitment purchases; KPIs will be zeroed: %v", err)
return nil, false
}

if provider != "" {
filtered := purchases[:0]
for _rvc := range purchases {
if purchases[_rvc].Provider == provider {
filtered = append(filtered, purchases[_rvc])
}
}
purchases = filtered
}

return purchases, true
}

Expand All @@ -632,6 +661,11 @@ func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time,
// scope-to-query mapping (including the zero-account short-circuit for issue
// #956) lives in fetchCommitmentPurchases.
//
// provider, when non-empty, restricts the result set to purchases for that
// cloud provider so the KPIs match the provider chip selection. This mirrors
// the recommendations half which passes the same provider param to
// ListRecommendations.
//
// EstimatedSavings on purchase_history rows is always written in monthly units
// (populated from PurchaseExecution.EstimatedSavings which derives from
// recommendation monthly savings at purchase time, see SavePurchaseHistory
Expand All @@ -641,9 +675,9 @@ func (h *Handler) fetchCommitmentPurchases(ctx context.Context, asOf time.Time,
// EstimatedSavings, derived from aggregateActiveCommitmentsPerService so both
// this KPI path (committedMonthly) and the per-service chart use exactly the
// same gate.
func (h *Handler) calculateCommitmentMetrics(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) (activeCommitments int, committedMonthly, ytdSavings float64, savingsByService map[string]float64) {
func (h *Handler) calculateCommitmentMetrics(ctx context.Context, provider string, accountUUIDs []string, accountExternalIDsByProvider map[string][]string) (activeCommitments int, committedMonthly, ytdSavings float64, savingsByService map[string]float64) {
currentTime := time.Now()
purchases, ok := h.fetchCommitmentPurchases(ctx, currentTime, accountUUIDs, accountExternalIDsByProvider)
purchases, ok := h.fetchCommitmentPurchases(ctx, currentTime, provider, accountUUIDs, accountExternalIDsByProvider)
if !ok {
return 0, 0, 0, nil
}
Expand Down
116 changes: 107 additions & 9 deletions internal/api/handler_dashboard_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,7 +846,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, nil, map[string][]string{"": {"account-123"}})
activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})

assert.Equal(t, 0, activeCommitments)
assert.Equal(t, 0.0, committedMonthly)
Expand All @@ -860,7 +860,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, nil, map[string][]string{"": {"account-123"}})
activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})

assert.Equal(t, 0, activeCommitments)
assert.Equal(t, 0.0, committedMonthly)
Expand All @@ -886,7 +886,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, nil, map[string][]string{"": {"account-123"}})
activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})

assert.Equal(t, 1, activeCommitments)
assert.Equal(t, 100.0, committedMonthly)
Expand All @@ -913,7 +913,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, nil, map[string][]string{"": {"account-123"}})
activeCommitments, committedMonthly, ytdSavings, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})

// Should skip expired commitments
assert.Equal(t, 0, activeCommitments)
Expand All @@ -940,7 +940,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(ctx, nil, map[string][]string{"": {"account-123"}})
activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})

assert.Equal(t, 1, activeCommitments)
assert.Equal(t, 50.0, committedMonthly)
Expand Down Expand Up @@ -973,7 +973,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, nil, map[string][]string{"": {"account-123"}})
activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, "", nil, map[string][]string{"": {"account-123"}})

// Only the status="" row counts; the failed row must be excluded.
assert.Equal(t, 1, activeCommitments,
Expand Down Expand Up @@ -1007,7 +1007,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, uuids, nil)
activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(ctx, "", uuids, nil)

assert.Equal(t, 2, activeCommitments)
assert.Equal(t, 250.0, committedMonthly,
Expand Down Expand Up @@ -1035,7 +1035,7 @@ func TestHandler_calculateCommitmentMetrics(t *testing.T) {
handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _, _ := handler.calculateCommitmentMetrics(
ctx, []string{"bbbbbbbb-1111-2222-3333-444444444444"}, map[string][]string{"aws": {"999988887777"}})
ctx, "", []string{"bbbbbbbb-1111-2222-3333-444444444444"}, map[string][]string{"aws": {"999988887777"}})

assert.Equal(t, 1, activeCommitments, "external-id-only commitment must be counted")
assert.Equal(t, 175.0, committedMonthly)
Expand Down Expand Up @@ -1097,7 +1097,7 @@ func TestHandler_calculateCommitmentMetrics_NoTruncationBeyond1000(t *testing.T)

handler := &Handler{config: mockStore}

activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(ctx, nil, nil)
activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(ctx, "", nil, nil)

assert.Equal(t, 1, activeCommitments,
"the still-active 3y commitment beyond the old 1000-row cap must be counted")
Expand Down Expand Up @@ -1585,3 +1585,101 @@ func TestFirstServiceConfig(t *testing.T) {
}
})
}

// TestIsActiveCommitment_RevokedReturnsFalse is the defect-#2 regression guard.
// A revoked commitment (RevokedAt != nil) must never be treated as active,
// regardless of whether its term window is still open. Pre-fix, isActiveCommitment
// did not check RevokedAt, so revoked rows slipped through into KPI totals.
func TestIsActiveCommitment_RevokedReturnsFalse(t *testing.T) {
now := time.Now()
revokedAt := now.AddDate(0, -1, 0) // revoked 1 month ago
p := config.PurchaseHistoryRecord{
Timestamp: now.AddDate(0, -3, 0), // purchased 3 months ago
Term: 1, // 1-year term, still within window
RevokedAt: &revokedAt,
Status: "",
}
// Pre-fix: isActiveCommitment returned true (only checked term expiry + status).
// Post-fix: must return false because RevokedAt != nil.
assert.False(t, isActiveCommitment(p, now),
"revoked commitment must not be active even when its term has not expired")
}

// TestHandler_calculateCommitmentMetrics_RevokedExcluded is the defect-#2
// regression guard for the dashboard KPI path. A revoked purchase returned by
// GetActivePurchaseHistory (e.g., from a DB snapshot before the SQL fix was
// deployed) must not contribute to activeCommitments or committedMonthly.
func TestHandler_calculateCommitmentMetrics_RevokedExcluded(t *testing.T) {
ctx := context.Background()
now := time.Now()
revokedAt := now.AddDate(0, -1, 0)
purchases := []config.PurchaseHistoryRecord{
{
// Active, non-revoked commitment.
Service: "ec2",
Timestamp: now.AddDate(0, -3, 0),
Term: 1,
EstimatedSavings: 100.0,
},
{
// Revoked commitment: term still open but revoked_at is set.
// Pre-fix: counted as active, inflating KPIs.
Service: "ec2",
Timestamp: now.AddDate(0, -6, 0),
Term: 1,
EstimatedSavings: 999.0,
RevokedAt: &revokedAt,
},
}

mockStore := new(MockConfigStore)
mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"),
[]string(nil), map[string][]string{"": {"acct-1"}}).Return(purchases, nil)
t.Cleanup(func() { mockStore.AssertExpectations(t) })

handler := &Handler{config: mockStore}
activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(
ctx, "", nil, map[string][]string{"": {"acct-1"}})

assert.Equal(t, 1, activeCommitments,
"revoked commitment must not increment activeCommitments")
assert.InDelta(t, 100.0, committedMonthly, 0.001,
"revoked commitment savings must not appear in committedMonthly")
assert.InDelta(t, 100.0, savingsByService["ec2"], 0.001,
"revoked commitment must be excluded from per-service savings map")
}

// TestHandler_calculateCommitmentMetrics_ProviderFilter is the defect-#4
// regression guard. When a provider chip is active, commitment KPIs must be
// restricted to that provider only. Pre-fix, calculateCommitmentMetrics ignored
// the provider param, so aws-only KPIs mixed in azure/gcp purchases.
func TestHandler_calculateCommitmentMetrics_ProviderFilter(t *testing.T) {
ctx := context.Background()
now := time.Now()
purchases := []config.PurchaseHistoryRecord{
{Provider: "aws", Service: "ec2", Timestamp: now.AddDate(0, -3, 0), Term: 1, EstimatedSavings: 100.0},
{Provider: "azure", Service: "vm", Timestamp: now.AddDate(0, -3, 0), Term: 1, EstimatedSavings: 200.0},
{Provider: "gcp", Service: "cud", Timestamp: now.AddDate(0, -3, 0), Term: 1, EstimatedSavings: 300.0},
}

mockStore := new(MockConfigStore)
mockStore.On("GetActivePurchaseHistory", ctx, mock.AnythingOfType("time.Time"),
[]string(nil), map[string][]string(nil)).Return(purchases, nil)
t.Cleanup(func() { mockStore.AssertExpectations(t) })

handler := &Handler{config: mockStore}

// Filter to aws only.
activeCommitments, committedMonthly, _, savingsByService := handler.calculateCommitmentMetrics(
ctx, "aws", nil, nil)

assert.Equal(t, 1, activeCommitments,
"provider filter must restrict activeCommitments to aws rows only")
assert.InDelta(t, 100.0, committedMonthly, 0.001,
"provider filter must restrict committedMonthly to aws rows only")
assert.InDelta(t, 100.0, savingsByService["ec2"], 0.001)
assert.NotContains(t, savingsByService, "vm",
"azure rows must not appear when filtering to aws")
assert.NotContains(t, savingsByService, "cud",
"gcp rows must not appear when filtering to aws")
}
11 changes: 11 additions & 0 deletions internal/api/handler_history.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1018,6 +1018,17 @@ func summarizePurchaseHistory(purchases []config.PurchaseHistoryRecord) HistoryS
summary := HistorySummary{TotalPurchases: len(purchases)}
for _rvc := range purchases {
p := purchases[_rvc]
// Revoked commitments are excluded from dollar KPIs regardless of their
// status: the provider has cancelled the commitment, so UpfrontCost and
// EstimatedSavings no longer represent active spend or realized savings.
// They count toward TotalPurchases (for auditability) and TotalRevoked
// (so the UI can surface the count), but not TotalCompleted.
// GetActivePurchaseHistory already excludes revoked rows via SQL; this
// guard handles the GetPurchaseHistoryFiltered path used by History.
if p.RevokedAt != nil {
summary.TotalRevoked++
continue
}
// Non-completed rows count toward TotalPurchases and their specific
// bucket (pending / in-progress / failed / expired / canceled) but
// are excluded from the dollar totals — the money hasn't been committed
Expand Down
Loading
Loading