fix: --estimate-only on a sweep estimates instead of launching (#524)

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

cmd/launch_sweep.go

@@ -140,6 +140,22 @@
 	}
 	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 @@
 	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 @@
 
 	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 {

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