feat(resume): --max-concurrent-auto derives concurrency from real quota - #496
Merged
Conversation
Extends #492's quota-derived concurrency ceiling to `spawn resume` (#494), which had the same class of gap: its --max-concurrent override was a plain user-typed int, same as launch's was before #492. Refactored the shared quota-derivation core out of resolveAutoMaxConcurrent into deriveMaxConcurrentFromCombos, so a second entry point -- resolveAutoMaxConcurrentFromConfigs -- can reuse it directly from resume's already-built []*aws.LaunchConfig (the pending parameter sets, already defaults-merged and per-entry overridden) instead of re-parsing raw params through a second merge pass. Scope: foreground (local-state) resume only. --detach is explicitly rejected with --max-concurrent-auto: the Lambda-orchestrated path's stored params live in S3 with no download helper yet (only upload exists via UploadParamsToS3), and there's no route today from resume into the orchestrator's stored MaxConcurrent before re-invoking. That gap is real but bigger than this fix -- not silently ignored, just not built yet. Also found and filed (not fixed here, out of scope): resume's region-from-defaults type assertion (`paramFormat.Defaults["region"]. (string)`) panics rather than falling through to its own "us-east-1" default when the params file has no region default and at least one instance was already launched (spawn#495). Fixes #494.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extends #492's quota-derived concurrency ceiling (
spawn launch --max-concurrent-auto, shipped in v0.99.0) tospawn resume— the follow-up filed as #494 when #492 shipped.spawn resume --max-concurrent-auto: re-derives the concurrency ceiling from the account's real AWS quota headroom for the PENDING parameter sets, instead of the sweep's original ceiling or a user-typed override.resolveAutoMaxConcurrentintoderiveMaxConcurrentFromCombos, so a newresolveAutoMaxConcurrentFromConfigscan reuse it directly fromresume's already-built[]*aws.LaunchConfig(the pending param sets, already defaults-merged and per-entry-overridden) — avoiding a second, subtly different merge pass over data that's already been merged once.--max-concurrent(same validation pattern aslaunch).Scope: foreground only
Per the discussion when #494 was filed,
--detach(Lambda-orchestrated resume) is explicitly out of scope here and rejected with a clear error rather than silently ignored. That path's stored parameters live in S3 with no download helper yet (onlyUploadParamsToS3exists), and there's no route today fromresumeinto the orchestrator's storedSweepRecord.MaxConcurrentbefore re-invoking Lambda — building that is a bigger, separate piece of work than this fix.Also found (filed separately, not fixed here)
While implementing this, found
resume.go's region-resolution has an unchecked type assertion (paramFormat.Defaults["region"].(string)) that panics instead of falling through to its own"us-east-1"default when the param file has noregiondefault and at least one instance was already launched. Filed as spawn#495 — out of scope for this PR, flagging since--max-concurrent-auto's correctness now depends onregionbeing reliably set.Fixes #494.
Test plan
TestCombosFromLaunchConfigs_*(3 tests) — pure extraction logic from already-built configs: dedup, missing-instance-type error, empty-input errorTestResolveAutoMaxConcurrentFromConfigs_*(2 tests, Substrate-backed) — derives the correct ceiling from a real seeded quota (same 32 vCPU Standard / 4 vCPU perc5.xlarge= 8 baseline as Sweep orchestrator's --max-concurrent is user-typed, not quota-derived — no automatic wave sizing to fit a real account ceiling #492's tests), and the region-must-be-resolved-first invariantTestRunResume_*(2 tests) — the--max-concurrent-auto/--max-concurrentmutual-exclusivity guard and the--detachrejection, both checked before any file/AWS I/OresolveAutoMaxConcurrent/sweepQuotaCombostests from Sweep orchestrator's --max-concurrent is user-typed, not quota-derived — no automatic wave sizing to fit a real account ceiling #492 pass unmodified — confirms the refactor didn't change that function's behaviorgo build ./...,go vet ./...,gofmt -lclean; fullgo test ./...(repo-wide) green, zero FAILsgolangci-lint run ./cmd/...— zero issues in touched filesmake check-docs— caught the new flag correctly,docs-gen/resume.mdregenerated and committed