Skip to content

fix(cli): apply --max-instances once run-wide, not per service and region #1725

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

Merged
cristim merged 4 commits into from
Aug 8, 2026
Merged

fix(cli): apply --max-instances once run-wide, not per service and region #1725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0e01fc7
fix(cli): apply --max-instances once run-wide, not per service and re…
cristim Aug 7, 2026
f6c0821
test(cli): pin the --max-instances selection order, not just the total
cristim Aug 7, 2026
326112e
fix(cli): drop recommendations --max-instances truncates below --min-…
cristim Aug 7, 2026
93fe75e
refactor(cli): hoist the --max-instances refusal above the duplicate …
cristim Aug 8, 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
18 changes: 16 additions & 2 deletions cmd/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,16 @@ func ApplyCountOverride(recs []common.Recommendation, overrideCount int32) []com
return result
}

// ApplyInstanceLimit limits the total number of instances.
// ApplyInstanceLimit truncates recs so their total Count does not exceed
// maxInstances. It is a single-shot cap over whatever slice it is handed: the
// caller is responsible for handing it the complete run-wide set, because
// applying it to a subset (one service, one region) caps that subset only and
// multiplies the effective cap by the number of subsets. See
// applyGlobalInstanceLimit in multi_service.go for the run-wide call site.
//
// Recommendations are consumed in slice order, so the caller controls which
// ones survive by ordering the slice (the main path caps the scorer's
// savings-sorted output, keeping the highest-value commitments).
func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []common.Recommendation {
if maxInstances <= 0 {
return recs
Expand All @@ -520,7 +529,12 @@ func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []comm
adjusted.Count = remaining
}
result = append(result, adjusted)
remaining -= adjusted.Count
// Only a positive Count consumes budget. Subtracting a non-positive
// Count would credit budget back and let later recommendations push
// the run past the cap.
if adjusted.Count > 0 {
remaining -= adjusted.Count
}
}
return result
}
Expand Down
137 changes: 133 additions & 4 deletions cmd/multi_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,8 @@ func runToolMultiService(ctx context.Context, cfg Config) {
AppLogger.Printf("\n📥 Fetching recommendations from all services...\n")
allRecs, drops := fetchAllRecs(ctx, awsCfg, recClient, accountCache, servicesToProcess, engineData, cfg, coverageMap)

// Phase 2: score and display.
scoredResult := scoreAndDisplay(allRecs, cfg)
// Phase 2: score, enforce the run-wide instance cap, and display.
scoredResult := scoreLimitAndDisplay(allRecs, cfg, drops)
if len(scoredResult.Passed) == 0 {
printDropSummary(drops)
AppLogger.Printf("\nℹ️ No recommendations passed filters. Nothing to purchase.\n")
Expand Down Expand Up @@ -202,20 +202,149 @@ func loadAWSConfig(ctx context.Context, cfg Config) (aws.Config, error) {
return awsconfig.LoadDefaultConfig(ctx, opts...)
}

// scoreAndDisplay runs the scorer on recs and prints the scored table and summary.
func scoreAndDisplay(recs []common.Recommendation, cfg Config) scorer.ScoredResult {
// scoreLimitAndDisplay runs the scorer on recs, enforces the run-wide
// --max-instances cap on the survivors, and prints the scored table and
// summary.
//
// The cap runs between scoring and rendering so the table, the confirmation
// prompt and the purchase loop all describe the same post-cap set, and so the
// instances that survive are the highest-savings ones run-wide (scorer.Score
// sorts Passed by savings percentage descending).
func scoreLimitAndDisplay(recs []common.Recommendation, cfg Config, drops *common.DropSummary) scorer.ScoredResult {
scorerCfg := scorer.Config{
MinSavingsPct: cfg.MinSavingsPct,
MaxBreakEvenMonths: cfg.MaxBreakEvenMonths,
MinCount: cfg.MinCount,
}
result := scorer.Score(recs, scorerCfg)
result.Passed = applyGlobalInstanceLimit(result.Passed, cfg, drops)
fmt.Print(reporter.RenderTable(result))
fmt.Print(reporter.RenderExcluded(result))
fmt.Print(reporter.RenderSummary(result))
Comment on lines 219 to 223

@coderabbitai coderabbitai Bot Aug 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Scale cost and savings fields when the cap reduces Count.

ApplyInstanceLimit copies each recommendation and edits only Count. EstimatedSavings and CommitmentCost keep their pre-cap values. reporter.RenderSummary and sumPassedRecs both sum those fields.

A run capped from 8 instances to 2 therefore prints and confirms the savings and upfront commitment of 8 instances. The operator sees a number that does not describe the purchase.

Scale the per-instance economics with the reduced count, or state in the table and the prompt that the figures are pre-cap.

Sketch: scale the monetary fields inside the truncation step 
 		adjusted := rec
 		if rec.Count > remaining {
+			ratio := float64(remaining) / float64(rec.Count)
 			adjusted.Count = remaining
+			adjusted.EstimatedSavings = rec.EstimatedSavings * ratio
+			adjusted.CommitmentCost = rec.CommitmentCost * ratio
+			adjusted.OnDemandCost = rec.OnDemandCost * ratio
 		}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/multi_service.go` around lines 219 - 223, Update applyGlobalInstanceLimit
and its underlying ApplyInstanceLimit flow so recommendations truncated from
their original Count also proportionally scale EstimatedSavings and
CommitmentCost to the retained count. Ensure the adjusted values are used by
reporter.RenderSummary and sumPassedRecs, while leaving recommendations
unaffected when the cap does not reduce Count.

@cristim cristim Aug 8, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and deliberately out of scope here. Tracked as #1611.

Your reading of the consequence is exactly right: a run capped from 8 instances to 2 prints and confirms the savings and upfront commitment of 8, so the operator approves a number that does not describe the purchase. It is a money-display defect on the approval screen, which is the worst place for one.

Three reasons it is not being fixed in this PR.

It is not introduced here. ApplyInstanceLimit has always assigned .Count and left CommitmentCost, OnDemandCost, EstimatedSavings and RecurringMonthlyCost describing the pre-truncation quantity. Pre-fix, sumPassedRecs already summed a post-cap set carrying those same stale fields; the cap simply ran per (service, region) instead of once. This PR changes where the cap is applied, not what the helper does to the fields, and it reduces how often the desync fires (once per run rather than once per service/region) without making any instance of it worse.

The fix belongs one level down, at the point Count is mutated. ApplyCountOverride has the identical defect in the opposite direction: it raises Count and leaves the costs low, so --override-count 10 on a Count = 1 recommendation understates the upfront by 10x. Fixing only ApplyInstanceLimit here would leave the sibling broken and mean touching the same lines twice. Both should route through common.ScaleRecommendationCosts the way applyCoverage (cmd/helpers.go:173-178) and applyTargetCoverageRI already do, with a guard for oldCount == 0, and the regression test has to assert the cost fields rather than Count — a test that only checks Count passes today with the bug present.

Folding a cost-model change into a cap-placement change would make both unreviewable. This PR is already carrying the placement fix, a --min-count interaction, a fail-closed guard and four regression tests on a money path. Cost scaling touches every consumer of those fields — the confirmation prompt, the UpfrontPayment and EstimatedSavings CSV columns, the TOTAL row, and pkg/common/audit.go's EstimatedCost — and deserves its own diff and its own review.

#1611 has been updated with this analysis, including that the fix should land at the mutation point so it covers both helpers together.

Not resolving this thread; leaving it visible with the justification attached.

@coderabbitai coderabbitai Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cristim, acknowledged. The defect predates this PR and the current change does not worsen it.

Deferring the cost-model fix to #1611 is appropriate. The fix should scale costs at every Count mutation point, including ApplyInstanceLimit and ApplyCountOverride, with regression tests for the monetary fields and zero-count handling.

I will leave this thread unresolved, as requested.

✏️ Learnings added 
Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 1725
File: cmd/multi_service.go:219-223
Timestamp: 2026-08-08T00:15:39.037Z
Learning: In the Go CLI codebase, `ApplyInstanceLimit` and `ApplyCountOverride` can change `common.Recommendation.Count` without scaling `CommitmentCost`, `OnDemandCost`, `EstimatedSavings`, and `RecurringMonthlyCost`. This pre-existing cost desynchronization is tracked in GitHub issue `#1611`. The eventual fix should occur at each `Count` mutation point through `common.ScaleRecommendationCosts`, guard against `oldCount == 0`, and test the scaled monetary fields rather than only the count.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

return result
}

// applyGlobalInstanceLimit enforces --max-instances once across the entire run.
//
// The flag is documented as a hard cap on the total number of instances
// purchased across all recommendations, so it has to see every service and
// every region together. Applying it inside the per-region fetch instead caps
// each (service, region) pair independently and multiplies the operator's cap
// by the number of pairs.
//
// passed must already be ordered best-first: ApplyInstanceLimit consumes the
// slice in order and drops the tail, so the ordering decides which commitments
// survive. scorer.Score guarantees that ordering.
//
// Truncation can push a recommendation under --min-count, which is a hard
// floor rather than advice, so dropTruncatedBelowMinCount removes any such
// recommendation instead of purchasing it short.
//
// Nothing is truncated silently. Every reduced or dropped recommendation is
// named on stdout, and the drops are counted into the end-of-run summary.
func applyGlobalInstanceLimit(passed []common.Recommendation, cfg Config, drops *common.DropSummary) []common.Recommendation {
if cfg.MaxInstances <= 0 {
return passed
}
totalBefore := CalculateTotalInstances(passed)
if totalBefore <= int(cfg.MaxInstances) {
return passed
}

limited := ApplyInstanceLimit(passed, cfg.MaxInstances)
limited, belowMin := dropTruncatedBelowMinCount(limited, cfg.MinCount)
reportInstanceLimit(passed, limited, len(belowMin), totalBefore, cfg.MaxInstances, drops)
reportMinCountDrops(belowMin, cfg.MinCount, drops)
return limited
}

// dropTruncatedBelowMinCount removes recommendations that the cap truncated to
// fewer instances than --min-count allows, returning the survivors and the
// removed recommendations at their truncated counts.
//
// --min-count is a hard floor everywhere else in the codebase, never advice:
// the scorer rejects recommendations under it outright
// (scorer.filterReason, "count %d below minimum %d"), the scheduler's
// meetsMinCount drops them, and both `docs/cli/filtering.md` and
// `docs/cli/README.md` describe it as dropping recommendations below the
// number. filtering.md applies it to "the adjusted instance count (after
// coverage scaling)", so the floor is meant to gate the *sized* count, and
// truncation by --max-instances is another form of sizing.
//
// Buying a commitment smaller than the operator's stated minimum can be worse
// than buying nothing, which is the whole reason the floor exists, so a
// truncated recommendation is dropped rather than purchased short. The freed
// budget is deliberately not redistributed: the next recommendation would have
// to fit in an even smaller remainder and would fail the same floor.
//
// Removal is always from the tail. ApplyInstanceLimit reduces at most one
// recommendation (the one where the budget runs out, which is the last it
// keeps), and every earlier one still carries the full count that already
// cleared the scorer's floor. Taking only from the tail keeps the result a
// prefix of the input, which reportInstanceLimit relies on.
func dropTruncatedBelowMinCount(limited []common.Recommendation, minCount int) (kept, removed []common.Recommendation) {
if minCount <= 0 {
return limited, nil
}
kept = limited
for len(kept) > 0 && kept[len(kept)-1].Count < minCount {
removed = append(removed, kept[len(kept)-1])
kept = kept[:len(kept)-1]
}
return kept, removed
}

// reportMinCountDrops names each recommendation the --min-count floor rejected
// after --max-instances truncated it. It continues the reportInstanceLimit
// listing, where these already appear as dropped, and explains why they were
// not simply purchased at the reduced count.
func reportMinCountDrops(removed []common.Recommendation, minCount int, drops *common.DropSummary) {
for i := range removed {
rec := removed[i]
AppLogger.Printf(" ↳ %s %s %s: the cap left room for only %d instances, below --min-count %d, so it is dropped rather than purchased short\n",
rec.Service, rec.Region, rec.ResourceType, rec.Count, minCount)
}
drops.Add(common.DropMinCountAfterCap, len(removed))
}

// reportInstanceLimit prints what --max-instances removed from the run.
// after must be the prefix of before produced by ApplyInstanceLimit, so
// after[i] and before[i] describe the same recommendation.
//
// belowMinCount is how many of the missing entries were removed by the
// --min-count floor rather than by the budget. They are still listed here as
// dropped (they were), but they are attributed to --min-count-after-cap by
// reportMinCountDrops, so excluding them from this tally keeps each dropped
// recommendation counted exactly once in the end-of-run summary.
func reportInstanceLimit(before, after []common.Recommendation, belowMinCount, totalBefore int, maxInstances int32, drops *common.DropSummary) {
AppLogger.Printf("\n🔒 --max-instances=%d caps the whole run: the %d recommendations that passed scoring total %d instances.\n",
maxInstances, len(before), totalBefore)
AppLogger.Printf(" Keeping the highest savings-percentage recommendations first. The following are reduced or dropped:\n")

reduced, dropped := 0, 0
for i := range before {
rec := before[i]
kept := 0
if i < len(after) {
kept = after[i].Count
}
switch {
case kept == rec.Count:
continue
case kept > 0:
reduced++
AppLogger.Printf(" • reduced: %s %s %s %d → %d instances\n", rec.Service, rec.Region, rec.ResourceType, rec.Count, kept)
default:
dropped++
AppLogger.Printf(" • dropped: %s %s %s (%d instances)\n", rec.Service, rec.Region, rec.ResourceType, rec.Count)
}
}

drops.Add(common.DropMaxInstances, dropped-belowMinCount)
AppLogger.Printf(" Proceeding with %d instances across %d recommendations (%d reduced, %d dropped).\n",
CalculateTotalInstances(after), len(after), reduced, dropped)
}

// sumPassedRecs returns total instance count and total estimated savings for passed recs.
func sumPassedRecs(recs []common.Recommendation) (total int, totalSavings float64) {
for _rvc := range recs {
Expand Down
49 changes: 34 additions & 15 deletions cmd/multi_service_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,25 @@ func processRegionRecommendations(

result.recommendations = filteredRecs

// --max-instances is a run-wide cap, and this legacy per-region entry point
// has no view of the other services and regions in the run, so it cannot
// evaluate the cap. Refuse to spend rather than purchase uncapped: an
// over-purchase of reserved capacity is not reversible. Dry runs continue
// so the recommendations are still reported.
//
// This is deliberately the first thing after the recommendations are
// recorded, ahead of building the service client and of the duplicate
// check. The decision depends only on cfg.MaxInstances and isDryRun, both
// already known, so reaching it through a cloud API call would be work
// done to arrive at an answer that was already determined -- and it would
// make the refusal path fail differently depending on whether the describe
// call happened to succeed.
if cfg.MaxInstances > 0 && !isDryRun {
log.Printf("❌ Refusing to purchase %s/%s: --max-instances is a run-wide cap and cannot be enforced on the per-region path. Use the multi-service pipeline (the default entry point).",
getServiceDisplayName(service), region)
return result
}

// Get service client and process purchases
regionalCfg := awsCfg.Copy()
regionalCfg.Region = region
Expand All @@ -407,8 +426,8 @@ func processRegionRecommendations(
return result
}

// Check for duplicate RIs and apply instance limit. Drop tracking skipped (nil).
adjustedRecs := checkDuplicatesAndApplyLimit(ctx, filteredRecs, serviceClient, cfg, nil)
// Check for duplicate RIs. Drop tracking skipped (nil).
adjustedRecs := checkDuplicates(ctx, filteredRecs, serviceClient, nil)

// Process purchases
regionResults := processPurchaseLoop(ctx, adjustedRecs, region, isDryRun, serviceClient, cfg)
Expand Down Expand Up @@ -548,13 +567,20 @@ func applyCoverageAndOverrides(recs []common.Recommendation, cfg Config, coverag
return filteredRecs
}

// checkDuplicatesAndApplyLimit checks for duplicate RIs and applies instance limits.
// checkDuplicates adjusts recommendations against already-owned RIs so the run
// does not double-purchase existing capacity.
//
// It deliberately does NOT apply --max-instances. This function runs once per
// (service, region), and capping here caps each region independently, which
// multiplies the operator's cap by the number of service/region pairs. The cap
// is applied once run-wide instead, after every region has been fetched
// (applyGlobalInstanceLimit in multi_service.go).
//
// drops accumulates per-reason drop counts for the end-of-run summary; pass nil to skip.
func checkDuplicatesAndApplyLimit(
func checkDuplicates(
ctx context.Context,
filteredRecs []common.Recommendation,
serviceClient provider.ServiceClient,
cfg Config,
drops *common.DropSummary,
) []common.Recommendation {
// Check for duplicate RIs to avoid double purchasing
Expand All @@ -574,15 +600,6 @@ func checkDuplicatesAndApplyLimit(
filteredRecs = adjustedRecs
}

// Apply instance limit if specified
if cfg.MaxInstances > 0 {
beforeLimit := len(filteredRecs)
filteredRecs = ApplyInstanceLimit(filteredRecs, cfg.MaxInstances)
if len(filteredRecs) < beforeLimit {
AppLogger.Printf(" 🔒 Applied instance limit: %d recommendations after limiting to %d instances\n", len(filteredRecs), cfg.MaxInstances)
}
}

return filteredRecs
}

Expand Down Expand Up @@ -643,8 +660,10 @@ func fetchAndFilterRegionRecs(
recs = applyCoverageAndOverrides(recs, cfg, coverageMap, expiringCommitments, drops)

// Deduplication: skip recs matching recently-purchased commitments.
// --max-instances is NOT applied here; it is enforced once run-wide by the
// caller so the cap covers every service and region together.
if serviceClient != nil {
recs = checkDuplicatesAndApplyLimit(ctx, recs, serviceClient, cfg, drops)
recs = checkDuplicates(ctx, recs, serviceClient, drops)
}

return recs
Expand Down
Loading
Loading