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)") +}