fix(cli): --max-instances is applied per service and region, not as the documented global total cap · Issue #1608 · LeanerCloud/cloud-commitments-cli · GitHub
You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(cli): --max-instances is applied per service and region, not as the documented global total cap #1608
Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before changing code, since a finding may have been fixed or moved.
--max-instances is documented, in two places, as a global total cap. On the primary purchase path it is applied once per (service, region) pair, so a single run can purchase roughly 150x the number the operator capped it at.
Nothing re-applies the cap to the merged allRecs slice before runPurchaseAndReport / executePurchasePipeline (cmd/multi_service.go:148, :230-249)
Documented contract: cmd/main.go:125 (flag help, "Maximum total number of instances to purchase"), docs/cli/README.md:39 ("Hard cap on the total number of instances purchased across all recommendations"), docs/cli/purchase-safety.md:129 (recommends it as "a final safety cap for a first run")
What
ApplyInstanceLimit runs inside the per-region fetch, so each (service, region) pair independently keeps up to MaxInstances recommendations. The merged slice is never re-capped.
The proof that this is a bug and not a design choice is the asymmetry between the two call sites. git grep -n "ApplyInstanceLimit(" origin/main -- cmd/ returns exactly two:
cmd/multi_service_helpers.go:559 - the per-region one, on the main purchase path.
cmd/multi_service.go:481 - the CSV path, which is applied globally to the whole loaded rec set.
The CSV path caps globally; the main purchase path does not. Both are fed by the same flag with the same documented meaning.
--all-services expands to 10 service slugs (cmd/main.go:227-240), and regions default to every opted-in region (roughly 17 to 30 via getAllAWSRegions). Each (service, region) pair keeps up to 10 instances, so the run can purchase on the order of
10 instances x 6 RI services x ~25 regions = ~1500 instances
instead of the 10 the operator capped it at.
The output actively reinforces the wrong belief: the per-region log line at cmd/multi_service_helpers.go:563 prints "limiting to 10 instances" once per region, which reads exactly like the cap is holding.
Fix direction
Apply ApplyInstanceLimit once to the merged slice in runToolMultiService after fetchAllRecs returns, before scoring and the confirmation prompt, and drop it from checkDuplicatesAndApplyLimit. Alternatively thread a shared remaining-budget counter through the per-region calls, but the single post-merge application matches the documented semantics and matches what the CSV path already does.
Note that fixing this changes which recommendations survive: today the survivors are the top N per region, afterwards they are the top N run-wide. Applying the cap after scoring rather than before makes that ordering deliberate.
A regression test must assert against the merged set across at least two regions; a single-region test stays green with the bug present.
Related
The same helper is the subject of a separate finding from this review: ApplyInstanceLimit mutates Count without scaling the cost fields, so the confirmation prompt understates the spend. Fixing the cap placement without fixing the cost scaling still leaves the prompt wrong.
Activity
fix(cli): drop recommendations --max-instances truncates below --min-…
fix(cli): apply --max-instances once run-wide, not per service and re…
fix(cli): drop recommendations --max-instances truncates below --min-…
refactor(cli): hoist the --max-instances refusal above the duplicate …
fix(cli): apply --max-instances once run-wide, not per service and re…
fix(cli): apply --max-instances once run-wide, not per service and re…