--estimate-only is ignored on the foreground sweep path — --no-detach launches every row instead of estimating

[scttfrdmn]

Severity: high — this spends money on the exact command a user runs to avoid spending money

--estimate-only is honoured on one of the two parameter-sweep orchestration paths. On
the other, it is silently ignored: no estimate is printed, and every row in the param file is
launched for real.

how the sweep is invoked	path taken	--estimate-only
--param-file f.yaml (nothing else)	detached / Lambda	honoured ✓
--param-file f.yaml --no-detach --ttl 1h	foreground	ignored — launches every row ✗
--param-file f.yaml --detach (no --max-concurrent)	foreground	ignored — launches every row ✗

The bare default is safe. The unsafe cases are the ones a careful user is most likely to
reach for:

• --no-detach is the documented advice for a heterogeneous sweep. The foreground path is
  the one that does per-config AMI detection (launch_sweep.go:220-241, #372), so a sweep
  mixing arm64 / x86 / GPU rows wants --no-detach. That is exactly the flag that disables
  --estimate-only. The two safeguards are mutually exclusive today, and nothing says so.
• --detach written explicitly, without --max-concurrent, also lands in the foreground
  path — the maxConcurrent default of min(len(params), 10) is only applied inside the
  auto-enable branch (launch_sweep.go:64-71), so an explicit --detach leaves it at 0 and
  the dispatch condition detach && maxConcurrent > 0 is false. The user asked for detached
  and got foreground, with no estimate and a live launch.

Reproduction (spawn v0.100.2, cmd/ at ad8a4e1)

cat > /tmp/two-rows.yaml <<'YAML'
defaults:
  region: us-west-2
  ttl: 1h
  on_complete: terminate
params:
  - instance_type: c8g.large
  - instance_type: c7g.large
YAML

spawn launch est-check --param-file /tmp/two-rows.yaml --no-detach --ttl 1h --estimate-only
spawn list --state all -o json --region us-west-2     # <-- two instances, not []

I have not executed this. Running it is the bug, and the account it was found in is not
one to demonstrate it in. The evidence below is a source trace at ad8a4e1, verifiable
without spending anything; the fix should be confirmed under the Substrate emulator, where a
launch costs nothing.

Evidence — cmd/launch_sweep.go

estimateOnly is referenced in exactly two places in the whole tree
(grep -n estimateOnly cmd/*.go pkg/**/*.go):

• launch_single.go:294 — the single-launch path. Correct.
• launch_sweep.go:682 — inside launchSweepDetached (declared at :607), before the
  S3 param upload and the DynamoDB record. Correct, but only reachable via that function.

The foreground sweep path has zero references. Its control flow:

line	what happens
launch_single.go:70-77	--param-file short-circuits to launchParameterSweep; launch_single.go:294 is never reached
launch_sweep.go:54-55	if !detach && !noDetach { detach = true } — detach auto-enables
launch_sweep.go:64-71	maxConcurrent defaults to min(len(params), 10) — only inside that auto-enable branch
launch_sweep.go:150-151	if detach && maxConcurrent > 0 { return launchSweepDetached(...) } — the only route to the estimateOnly check
launch_sweep.go:154-176	otherwise: build per-row configs and fall through
launch_sweep.go:220-241	detect AMI per config
launch_sweep.go:288	SetupSporedIAMRole → launch. No estimateOnly check anywhere in between.

So the check isn't misplaced by a few lines — it lives on one branch of a fork and is simply
absent from the other. A sweep's spend-preview guarantee currently depends on which
orchestration path it happens to take, which is not a property a user can be expected to
reason about.

Expected behaviour

spawn launch <name> --param-file f.yaml --estimate-only, with any combination of
--detach / --no-detach / --max-concurrent:

1. prints a per-row estimate and a total,
2. launches nothing,
3. exits 0,
4. leaves spawn list --state all unchanged.

Fix sketch

Hoist the estimateOnly handling above the dispatch at launch_sweep.go:150, so it is
checked once for the sweep rather than once per orchestration path. pricing.EstimateSweepCost
already takes only the param file, so the estimate needs no AWS calls and the early return can
sit before the AWS client is even constructed. Then delete the now-unreachable block at :682
(the detached path still needs costEstimate.TotalCost for the SweepRecord, so keep the
computation there, just not the estimateOnly return).

Structuring it that way is the point of the fix: a per-path check is what failed here, and
adding a second per-path check would leave the same shape in place for the next path someone
adds.

Related: #124 established that --estimate-only should also run #110's constraint validation
first. A sweep-wide estimate is the natural place to do that per row, but it needs AWS and can
be a follow-up — launching nothing is the part that matters.

Regression test to add

In cmd/ alongside sweep_test.go, against the Substrate emulator:

given  a 2-row param file
when   launch runs with --estimate-only, for EACH of:
         (a) no detach flags          [detached path]
         (b) --no-detach --ttl 1h     [foreground path]  <-- the failing case
         (c) --detach                 [foreground path, via maxConcurrent=0]
then   exit 0, an estimate is printed, and ZERO instances exist

The assertion must be spawn list --state all returns an empty set — not "the
estimateOnly flag was observed", not "launchSweepDetached was not called". This bug is a
correct-looking flag check sitting behind a dispatch: a test that trusts an internal flag would
have passed for the whole time the bug was live. Assert the observable.

Case (b) is the one that must fail before the fix and pass after. (a) and (c) are there so the
fix cannot regress into "only the path we happened to test is safe."

Cross-references

• Sweep dispatch silently drops --cost-limit and --ttl, then logs "Using safeguards: ttl=..." #525 — the same dispatch drops --cost-limit and --ttl from a sweep. Shared root cause:
  per-row configs are built from scratch (sweep.go:74-82, "Start with an empty config") and
  only Region/InstanceType/Name are copied off the CLI base config.
• Unknown param-file keys silently become PARAM_* env vars — a mistyped or invented spend control caps nothing #526 — unknown param-file keys become PARAM_* env vars silently, which is what makes the
  cost_limit: workaround for Sweep dispatch silently drops --cost-limit and --ttl, then logs "Using safeguards: ttl=..." #525 fail quietly rather than loudly.
• --estimate-only skips #110 constraint validation; should dry-run it before the cost estimate #124 (closed) — --estimate-only bypassing feat(spawn): validate instance type constraints before launch — EFA, placement groups, hibernation #110 constraint validation. Adjacent and
  narrower: that one printed an estimate for a config that couldn't launch. This one launches
  instead of estimating.

Found while building a deliberately heterogeneous 6-row GROMACS benchmark sweep (arm64 + x86 +
GPU) for the cost-to-result project, where --estimate-only was the first command in the
documented sequence — and where --no-detach had already been made mandatory for the AMI
reason above. That combination is the failing case, so the mitigation for one problem disabled
the safeguard for another. Recorded there as finding 15.

[scttfrdmn]

Correction to the original body (edited above). The first version of this issue said the
detached path returns before estimateOnly is checked and therefore launches. That was
backwards, and the record should say so rather than quietly read as if it were always right.

launch_sweep.go:682 is inside launchSweepDetached (declared at :607), ahead of the S3
param upload and the DynamoDB record — so the detached path, which is the default, honours
--estimate-only correctly. The path with no check at all is the foreground one, reached
via --no-detach or via an explicit --detach with no --max-concurrent.

The bug, the severity, and the fix sketch are unchanged — hoist the check above the dispatch at
:150 so it does not depend on which path a sweep takes. What changed is which invocation is
dangerous, and that matters for anyone reading this before the fix lands:

• safe today: spawn launch x --param-file f.yaml --estimate-only
• launches everything today: the same command plus --no-detach (or plus a bare --detach)

That inversion makes it worse than filed, not better. --no-detach is the documented advice for
a heterogeneous sweep, because the foreground path is the one that does per-config AMI detection
(#372) — so the two safeguards are currently mutually exclusive, and the user who takes the AMI
advice loses the spend preview without being told.