fix(cli): --max-instances is applied per service and region, not as the documented global total cap · Issue #1608 · LeanerCloud/cloud-commitments-cli · GitHub
Skip to content

fix(cli): --max-instances is applied per service and region, not as the documented global total cap #1608

Closed

Description

@cristim

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.

Where

  • cmd/multi_service_helpers.go:559-565 - ApplyInstanceLimit(filteredRecs, cfg.MaxInstances) inside checkDuplicatesAndApplyLimit
  • Reached from cmd/multi_service_helpers.go:628 -> fetchAndFilterRegionRecs -> fetchAllRecs (cmd/multi_service_helpers.go:661-664) -> runToolMultiService (cmd/multi_service.go:137)
  • 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:

  1. cmd/multi_service_helpers.go:559 - the per-region one, on the main purchase path.
  2. 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.

Failure scenario

cudly --all-services --purchase --yes --max-instances 10

--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.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions