From 4c6e603b6cd910f09f382fdaa622395eb9c48e93 Mon Sep 17 00:00:00 2001 From: scttfrdmn <3011922+scttfrdmn@users.noreply.github.com> Date: Tue, 18 Aug 2026 21:39:52 -0700 Subject: [PATCH] fix: --estimate-only on a sweep estimates instead of launching (#524) --estimate-only was checked only inside launchSweepDetached, so a sweep that took the FOREGROUND path launched every row while the user was asking for a preview. That inverts the one command a user runs specifically to avoid spending money. Two ordinary invocations reach the foreground path: --no-detach, which is the documented advice for a heterogeneous sweep because only that path detects an AMI per config (#372), and an explicit --detach with no --max-concurrent, which leaves maxConcurrent at 0 and so fails the `detach && maxConcurrent > 0` dispatch condition. In other words the sweep shape most in need of a cost preview was the one that could not get one. The check now lives once in launchParameterSweep, above the dispatch that chooses between the two paths, so "launches nothing" no longer depends on which path the sweep would have taken. It is deliberately placed before aws.NewClient: EstimateSweepCost reads only the param file, so a cost preview needs no credentials. The budget comparison is factored into reportSweepBudget and shared with the real detached launch so the two cannot drift, and the now-unreachable estimateOnly block inside launchSweepDetached is removed rather than left as dead code that would re-create the per-path shape behind the bug. The regression test asserts on the observable, not the flag: it queries EC2 for every instance in the sweep's region and requires the set to be empty, for all three invocations (default, --no-detach, bare --detach). It does not use `spawn list --state all`, which passes "all" through as a literal instance-state-name filter and therefore returns [] whether or not instances exist (#527) -- using it would have made the test unfailable, which is the defect class the test exists to catch. Fails before, passes after: 2 of the 3 subtests launched 2 instances each pre-fix. --- CHANGELOG.md | 18 ++++ cmd/launch_sweep.go | 74 +++++++++++--- test/e2e/tier0_sweep_estimate_test.go | 135 ++++++++++++++++++++++++++ 3 files changed, 212 insertions(+), 15 deletions(-) create mode 100644 test/e2e/tier0_sweep_estimate_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bb774b..f1b173f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **`--estimate-only` launched every row of a parameter sweep instead of + estimating it** (#524). The flag was checked only inside + `launchSweepDetached`, so any sweep that took the *foreground* path + provisioned the whole sweep while the user was asking for a preview — i.e. + the exact command run to avoid spending money spent it. Two ordinary + invocations reach that path: `--no-detach` (the documented advice for a + heterogeneous sweep, since only the foreground path detects an AMI per + config, #372), and an explicit `--detach` without `--max-concurrent`, which + leaves `maxConcurrent` at 0 and so fails the `detach && maxConcurrent > 0` + dispatch condition. `--estimate-only` is now handled once in + `launchParameterSweep`, above that dispatch and before the AWS client is + built (a cost preview reads only the param file, so it needs no + credentials), which makes "launches nothing" independent of which + orchestration path the sweep would have taken. Covered by a Tier 0 e2e + regression test that asserts zero instances exist afterwards by querying EC2 + directly, for all three invocations. + ## [0.100.3] - 2026-08-18 ### Fixed diff --git a/cmd/launch_sweep.go b/cmd/launch_sweep.go index 5c1d7c9..cfc6a84 100644 --- a/cmd/launch_sweep.go +++ b/cmd/launch_sweep.go @@ -140,6 +140,22 @@ func launchParameterSweep(ctx context.Context, baseConfig *aws.LaunchConfig, pla } fmt.Fprintf(os.Stderr, "\n") + // --estimate-only is handled HERE, above the detached/foreground dispatch + // below, so "launches nothing" does not depend on which orchestration path + // this sweep would have taken (#524). It used to be checked only inside + // launchSweepDetached, which meant any sweep reaching the FOREGROUND path + // launched every row while the user was asking for a preview — via + // --no-detach (the documented advice for a heterogeneous sweep, since only + // the foreground path detects an AMI per config, #372), or via an explicit + // --detach with no --max-concurrent, which leaves maxConcurrent at 0 and so + // fails the `detach && maxConcurrent > 0` condition below. + // + // Deliberately placed before the AWS client is constructed: EstimateSweepCost + // reads only the param file, so a cost preview needs no credentials. + if estimateOnly { + return estimateSweepOnly(paramFormat) + } + // Initialize AWS client awsClient, err := aws.NewClient(ctx) if err != nil { @@ -603,6 +619,43 @@ func launchWithRollingQueue(ctx context.Context, awsClient *aws.Client, launchCo return launchedInstances, failures, successCount, nil } +// estimateSweepOnly prints a per-row cost estimate for a sweep and returns +// without launching anything. This is the entirety of --estimate-only's +// behaviour on the sweep path; both orchestration paths reach it from the single +// check in launchParameterSweep (#524). +func estimateSweepOnly(paramFormat *ParamFileFormat) error { + fmt.Fprintf(os.Stderr, "💰 Estimating cost...\n") + costEstimate, err := pricing.EstimateSweepCost(&pricing.ParamFileFormat{ + Defaults: paramFormat.Defaults, + Params: paramFormat.Params, + }) + if err != nil { + return fmt.Errorf("failed to estimate cost: %w", err) + } + + fmt.Fprintf(os.Stderr, "\n%s\n\n", costEstimate.Display()) + reportSweepBudget(costEstimate.TotalCost) + fmt.Fprintf(os.Stderr, "✅ Estimate complete — no instances launched (--estimate-only)\n") + return nil +} + +// reportSweepBudget compares an estimated sweep cost against --budget. It only +// ever prints: --budget is a warning, not a cap, and nothing here blocks a +// launch. Shared by the estimate-only path and the real detached launch so the +// two cannot drift. +func reportSweepBudget(totalCost float64) { + if budget <= 0 { + return + } + if totalCost > budget { + fmt.Fprintf(os.Stderr, "⚠️ WARNING: Estimated cost ($%.2f) exceeds budget ($%.2f) by $%.2f\n\n", + totalCost, budget, totalCost-budget) + return + } + fmt.Fprintf(os.Stderr, "✓ Within budget: $%.2f remaining of $%.2f\n\n", + budget-totalCost, budget) +} + // launchSweepDetached launches a parameter sweep in detached mode (Lambda orchestration) func launchSweepDetached(ctx context.Context, paramFormat *ParamFileFormat, baseConfig *aws.LaunchConfig, sweepID, sweepName string, maxConcurrent int, launchDelay string) error { // Determine region (auto-detect if not specified) @@ -667,22 +720,13 @@ func launchSweepDetached(ctx context.Context, paramFormat *ParamFileFormat, base fmt.Fprintf(os.Stderr, "\n%s\n\n", costEstimate.Display()) - // Check budget - if budget > 0 { - if costEstimate.TotalCost > budget { - fmt.Fprintf(os.Stderr, "⚠️ WARNING: Estimated cost ($%.2f) exceeds budget ($%.2f) by $%.2f\n\n", - costEstimate.TotalCost, budget, costEstimate.TotalCost-budget) - } else { - fmt.Fprintf(os.Stderr, "✓ Within budget: $%.2f remaining of $%.2f\n\n", - budget-costEstimate.TotalCost, budget) - } - } + // Check budget (warning only — see reportSweepBudget) + reportSweepBudget(costEstimate.TotalCost) - // If estimate-only, exit here - if estimateOnly { - fmt.Fprintf(os.Stderr, "✅ Cost estimate complete (--estimate-only specified)\n") - return nil - } + // No --estimate-only check here: it is handled once in launchParameterSweep, + // before the dispatch that chooses this function, so that the guarantee holds + // on the foreground path too (#524). A second check here would be dead code + // and would re-create the per-path shape that caused the bug. // If not auto-approved, prompt for confirmation if !autoYes { diff --git a/test/e2e/tier0_sweep_estimate_test.go b/test/e2e/tier0_sweep_estimate_test.go new file mode 100644 index 0000000..ced7259 --- /dev/null +++ b/test/e2e/tier0_sweep_estimate_test.go @@ -0,0 +1,135 @@ +//go:build e2e_tier0 + +package e2e + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/ec2" +) + +// Tier 0 regression coverage for #524: --estimate-only must launch nothing on a +// parameter sweep, whichever orchestration path the sweep would have taken. +// +// The check used to live only inside launchSweepDetached, so a sweep that +// reached the FOREGROUND path — via --no-detach, or via an explicit --detach +// with no --max-concurrent — ran the full launch while the user was asking for a +// preview. The three subtests below are the three paths; (b) and (c) are the +// ones that failed before the fix. +// +// Why the assertion is "zero instances in EC2" and not "the estimateOnly flag +// was honoured": the bug WAS a correct flag check, sitting behind a dispatch. A +// test that trusts an internal flag would have passed for the entire time the +// bug was live. Only the observable distinguishes the two. + +// writeSweepParamFile writes a minimal two-row param file and returns its path. +// Two rows rather than one so a partial launch is still caught, and c5.large +// because Substrate models that family. +func writeSweepParamFile(t *testing.T) string { + t.Helper() + path := filepath.Join(t.TempDir(), "sweep.yaml") + body := `defaults: + on_complete: terminate +params: + - instance_type: c5.large + - instance_type: c5.xlarge +` + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatalf("write param file: %v", err) + } + return path +} + +// everyInstanceID returns every instance Substrate knows about, in any state, +// read straight from EC2. +// +// It deliberately does NOT go through `spawn list --state all`. That flag passes +// "all" through as a literal instance-state-name filter, which matches nothing, +// so it returns [] unconditionally (#527) — using it here would make this whole +// test unfailable, which is the same defect class the test exists to catch. +func (e *spawnEnv) everyInstanceID() []string { + e.t.Helper() + out, err := e.EC2Client().DescribeInstances(context.Background(), &ec2.DescribeInstancesInput{}) + if err != nil { + e.t.Fatalf("DescribeInstances: %v", err) + } + var ids []string + for _, r := range out.Reservations { + for _, inst := range r.Instances { + ids = append(ids, aws.ToString(inst.InstanceId)) + } + } + return ids +} + +// requireNothingLaunched asserts the emulator holds no instances at all, and +// that spawn's own inventory agrees. Both halves matter: EC2 is ground truth, +// and `spawn list` is what a user would check. +func (e *spawnEnv) requireNothingLaunched(what string) { + e.t.Helper() + if ids := e.everyInstanceID(); len(ids) != 0 { + e.t.Errorf("%s: expected ZERO instances, EC2 has %d: %v", what, len(ids), ids) + } + if listed := mustJSONArray(e.t, e.runOK("list", "-o", "json")); len(listed) != 0 { + e.t.Errorf("%s: spawn list reports %d instances, want 0: %v", what, len(listed), listed) + } +} + +// estimateOnlySweep runs a sweep with --estimate-only plus the caller's path +// flags, requires exit 0, and returns stderr (where the estimate is printed). +func (e *spawnEnv) estimateOnlySweep(paramFile string, pathFlags ...string) string { + e.t.Helper() + args := append([]string{ + "launch", "est-check", + "--param-file", paramFile, + "--region", "us-east-1", + "--estimate-only", + "--wait-for-running=false", + "--wait-for-ssh=false", + "-y", + }, pathFlags...) + stdout, stderr, code := e.run(args...) + if code != 0 { + e.t.Fatalf("spawn %v: expected exit 0, got %d\nstdout:\n%s\nstderr:\n%s", + args, code, stdout, stderr) + } + return stderr +} + +// TestTier0_SweepEstimateOnly_DetachedLaunchesNothing is the path that was +// already correct. It is here so the fix cannot regress into "only the path we +// just fixed is safe". +func TestTier0_SweepEstimateOnly_DetachedLaunchesNothing(t *testing.T) { + env := startSpawnSubstrate(t) + env.estimateOnlySweep(writeSweepParamFile(t)) + env.requireNothingLaunched("--estimate-only (detached, default)") +} + +// TestTier0_SweepEstimateOnly_NoDetachLaunchesNothing is the #524 case. +// +// --no-detach is the documented advice for a heterogeneous sweep, because the +// foreground path is the only one that detects an AMI per config (#372). Before +// the fix, taking that advice silently disabled --estimate-only and launched +// every row. --ttl is required by the --no-detach guard. +func TestTier0_SweepEstimateOnly_NoDetachLaunchesNothing(t *testing.T) { + env := startSpawnSubstrate(t) + env.estimateOnlySweep(writeSweepParamFile(t), "--no-detach", "--ttl", "1h") + env.requireNothingLaunched("--estimate-only --no-detach") +} + +// TestTier0_SweepEstimateOnly_ExplicitDetachLaunchesNothing is the second way +// into the foreground path, and the more surprising one: an explicit --detach +// with no --max-concurrent leaves maxConcurrent at 0, because the min(len,10) +// default is applied only inside the AUTO-enable branch. The dispatch condition +// `detach && maxConcurrent > 0` is then false, so the user who asked for +// detached orchestration got the foreground path — with no estimate and, before +// the fix, a live launch. +func TestTier0_SweepEstimateOnly_ExplicitDetachLaunchesNothing(t *testing.T) { + env := startSpawnSubstrate(t) + env.estimateOnlySweep(writeSweepParamFile(t), "--detach") + env.requireNothingLaunched("--estimate-only --detach (maxConcurrent=0)") +}