fix: a sweep honours --ttl/--idle-timeout/--cost-limit instead of logging them (#525)

CHANGELOG.md

@@ -8,6 +8,37 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 
 ### Fixed
+- **A parameter sweep silently discarded `--ttl`, `--idle-timeout` and
+  `--cost-limit`, then logged `Using safeguards: ttl=...`** (#525).
+  `buildLaunchConfigFromParams` starts from an empty config and the sweep
+  dispatch copied only `Region`/`InstanceType`/`Name` off the base config, so no
+  CLI spend control reached a single row — while the `--no-detach` branch printed
+  a safeguards line from the very variables it was about to throw away, which is
+  worse than dropping them quietly: it affirmatively tells the operator a cap is
+  active at the instant it is discarded. `cost_limit:` in a param file did not
+  work either — there was no parser case, so it fell through to the unknown-key
+  arm and became a `PARAM_cost_limit` env var that capped nothing. With both
+  routes broken a sweep had no per-instance dollar cap at all, and its only bound
+  was the zombie guard's unrelated 1h *idle* timeout, which never fires on a
+  compute-bound row yet still shows up in `spawn list` and reads as bounded.
+
+  The flags are now applied as sweep `defaults:` — the one place that reaches both
+  orchestration paths, since the foreground path merges defaults into every row
+  and the detached path uploads them to S3 for the Lambda orchestrator — and
+  `cost_limit:` is a real parser case that *errors* on a value it cannot read
+  rather than leaving the cap at 0. Precedence is most-specific-wins: a row's own
+  `params:` > the CLI flag > the file's `defaults:`.
+
+  Two knock-on fixes in the same path: `--estimate-only` now prices the TTL you
+  passed (previously `--ttl 4h` on a file with no `ttl:` was quoted at the
+  estimator's 1h default, understating the worst case fourfold), and the
+  `--no-detach` guard checks the merged **per-row** bound instead of the CLI
+  variable. A `ttl:` in the param file used to be refused by that guard even
+  though it was the value which actually reached the instances, so the only way
+  through was to pass `--ttl` to satisfy the check and have it discarded — one
+  flag to pass the guard, another to take effect. The per-row form also catches a
+  file that bounds only *some* of its rows, which a check on the CLI variable
+  could not see, and names the offending rows.
 - **`--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

cmd/launch_sweep.go

@@ -46,6 +46,26 @@
 		return fmt.Errorf("either --param-file or --params must be specified for parameter sweep")
 	}
 
+	// The CLI spend-control flags used to be dropped on the floor for a sweep
+	// (#525). buildLaunchConfigFromParams "start[s] with an empty config" and the
+	// dispatch below copies only Region/InstanceType/Name off baseConfig, so
+	// --ttl, --idle-timeout and --cost-limit never reached a single row — while
+	// the --no-detach branch below printed "Using safeguards: ttl=..." from the
+	// very variables it was about to discard, which is worse than dropping them
+	// quietly: it affirmatively tells the operator a safeguard is in place.
+	//
+	// They are applied here as sweep *defaults*, which is the one place that
+	// reaches BOTH orchestration paths: the foreground path merges Defaults into
+	// every row (buildLaunchConfigFromParams), and the detached path uploads
+	// Defaults to S3 for the Lambda orchestrator, which does the same.
+	//
+	// This runs before everything that consumes them: the --no-detach safeguard
+	// log, the --estimate-only preview (whose per-row duration comes from
+	// defaults.ttl, so before this fix `--ttl 4h --estimate-only` on a file with
+	// no ttl: quoted a 1h estimate), and applyIdleTimeoutDefault, which now sees
+	// an explicit --ttl and stops substituting an unrelated 1h *idle* timeout.
+	appliedControls := applyCLISpendControlsToSweep(paramFormat)
+
 	// AUTO-ENABLE DETACHED MODE for parameter sweeps to prevent zombie instances
 	// If the CLI disconnects (laptop sleep/shutdown), detached mode ensures:
 	// - Sweep state persists in DynamoDB
@@ -76,11 +96,26 @@
 		// User explicitly disabled detached mode - warn about zombie instances
 		fmt.Fprintf(os.Stderr, "\n⚠️  WARNING: --no-detach specified\n")
 		fmt.Fprintf(os.Stderr, "   If CLI disconnects (laptop sleep/shutdown), instances may become zombies.\n")
-		if ttl == "" && idleTimeout == "" {
-			fmt.Fprintf(os.Stderr, "\n❌ ERROR: --no-detach requires --ttl or --idle-timeout to prevent zombie instances\n")
-			return fmt.Errorf("--no-detach requires --ttl or --idle-timeout for safety")
+		// The bound is checked per ROW, against the merged (defaults + row) values
+		// the launch will actually use, rather than against the CLI variables
+		// alone. Two reasons, both from #525:
+		//
+		//   * a `ttl:` in the param file is a real bound — it is the one that used
+		//     to reach the instances — but this check refused it, so the only way
+		//     through was to pass --ttl purely to satisfy the check and then have
+		//     its value discarded. One flag to pass the guard, another to take
+		//     effect, with nothing saying so.
+		//   * a per-row check also catches a file that bounds only SOME of its
+		//     rows, which a check on the CLI variables cannot see at all.
+		if unbounded := sweepRowsWithoutBound(paramFormat); len(unbounded) > 0 {
+			fmt.Fprintf(os.Stderr, "\n❌ ERROR: --no-detach requires a TTL or idle timeout on EVERY row to prevent zombie instances\n")
+			fmt.Fprintf(os.Stderr, "   Unbounded: %s\n", strings.Join(unbounded, ", "))
+			fmt.Fprintf(os.Stderr, "   Fix with --ttl/--idle-timeout for the whole sweep, or ttl:/idle_timeout: under defaults: or on those rows.\n")
+			return fmt.Errorf("--no-detach requires a TTL or idle timeout on every row for safety (%d unbounded)", len(unbounded))
 		}
-		fmt.Fprintf(os.Stderr, "   Using safeguards: ttl=%s, idle-timeout=%s\n\n", ttl, idleTimeout)
+		defTTL, defIdle := sweepRowBound(paramFormat.Defaults, nil)
+		fmt.Fprintf(os.Stderr, "   Using safeguards: ttl=%s, idle-timeout=%s\n", defTTL, defIdle)
+		fmt.Fprintf(os.Stderr, "   (sweep defaults, CLI flags already folded in; a row's own value wins — #525)\n\n")
 	}
 
 	// Generate sweep ID
@@ -138,6 +173,10 @@
 	if detach {
 		fmt.Fprintf(os.Stderr, "   Orchestration: Lambda (detached)\n")
 	}
+	if len(appliedControls) > 0 {
+		fmt.Fprintf(os.Stderr, "   From the command line: %s (each row's own value wins)\n",
+			strings.Join(appliedControls, ", "))
+	}
 	fmt.Fprintf(os.Stderr, "\n")
 
 	// --estimate-only is handled HERE, above the detached/foreground dispatch
@@ -619,6 +658,83 @@
 	return launchedInstances, failures, successCount, nil
 }
 
+// sweepRowBound returns the TTL and idle timeout that will actually apply to one
+// row: the row's own value if it has one, otherwise the sweep default. Pass a nil
+// row to read the defaults themselves.
+//
+// A non-string value (`ttl: 3600` rather than `ttl: 1h`) reads as absent, which
+// matches what buildLaunchConfigFromParams does with it — the type assertion
+// there fails and the field stays empty. Agreeing with the parser is the point:
+// it means the --no-detach guard refuses that file loudly instead of letting it
+// launch with a bound the parser quietly dropped.
+func sweepRowBound(defaults, row map[string]interface{}) (ttl, idle string) {
+	pick := func(key string) string {
+		for _, m := range []map[string]interface{}{row, defaults} {
+			if v, ok := m[key]; ok {
+				if s, ok := v.(string); ok && s != "" {
+					return s
+				}
+			}
+		}
+		return ""
+	}
+	return pick("ttl"), pick("idle_timeout")
+}
+
+// sweepRowsWithoutBound labels every row that would launch with neither a TTL nor
+// an idle timeout, for the --no-detach guard's error message. Naming the rows
+// matters: on a 30-row file "some row is unbounded" is not actionable.
+func sweepRowsWithoutBound(paramFormat *ParamFileFormat) []string {
+	var out []string
+	for i, row := range paramFormat.Params {
+		if ttl, idle := sweepRowBound(paramFormat.Defaults, row); ttl == "" && idle == "" {
+			label := fmt.Sprintf("row %d", i)
+			if it, ok := row["instance_type"].(string); ok && it != "" {
+				label += " (" + it + ")"
+			}
+			out = append(out, label)
+		}
+	}
+	return out
+}
+
+// applyCLISpendControlsToSweep writes the CLI spend-control flags into a sweep's
+// `defaults:` so they actually reach each row (#525), and returns a human-readable
+// list of what it set for the launch header.
+//
+// Precedence, most specific wins:
+//
+//	a row's own params: > the CLI flag > the param file's defaults:
+//
+// So a per-row `ttl:` (a GPU row on a shorter leash, say) still wins, while a flag
+// passed at invocation time beats a value checked into the file — which is the
+// same direction cobra/viper use for flags versus config, and the reason
+// `--ttl 30m` on a file that says `ttl: 8h` does what it looks like it does.
+//
+// A non-empty ttl/idleTimeout here can only have come from the command line:
+// applyLaunchDefaults (which would otherwise fold in ~/.spawn/config.yaml) runs
+// *after* the sweep early-return in launch_single.go, so ~/.spawn defaults never
+// silently outrank the param file.
+func applyCLISpendControlsToSweep(paramFormat *ParamFileFormat) []string {
+	if paramFormat.Defaults == nil {
+		paramFormat.Defaults = make(map[string]interface{})
+	}
+	var applied []string
+	if ttl != "" {
+		paramFormat.Defaults["ttl"] = ttl
+		applied = append(applied, "ttl="+ttl)
+	}
+	if idleTimeout != "" {
+		paramFormat.Defaults["idle_timeout"] = idleTimeout
+		applied = append(applied, "idle-timeout="+idleTimeout)
+	}
+	if costLimit > 0 {
+		paramFormat.Defaults["cost_limit"] = costLimit
+		applied = append(applied, fmt.Sprintf("cost-limit=$%.2f", costLimit))
+	}
+	return applied
+}
+
 // 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

cmd/sweep.go

@@ -5,6 +5,8 @@
 	"fmt"
 	"os"
 	"path/filepath"
+	"strconv"
+	"strings"
 	"time"
 
 	"github.com/spore-host/spawn/pkg/aws"
@@ -70,6 +72,44 @@
 	}, nil
 }
 
+// parseCostLimit coerces a param-file `cost_limit:` value to USD. It accepts every
+// shape a YAML/JSON/CSV param file can produce for a number — `8`, `8.50`, and the
+// quoted `"8.50"` — because the three parsers disagree about which Go type an
+// unquoted number becomes, and a type mismatch here would zero the only
+// per-instance dollar cap a sweep has (#525). Anything else is an error, never a
+// silent 0: a disabled cap must be spelled `cost_limit: 0`, not typo'd into one.
+func parseCostLimit(val interface{}) (float64, error) {
+	var f float64
+	switch v := val.(type) {
+	case float64:
+		f = v
+	case float32:
+		f = float64(v)
+	case int:
+		f = float64(v)
+	case int64:
+		f = float64(v)
+	case json.Number:
+		parsed, err := v.Float64()
+		if err != nil {
+			return 0, fmt.Errorf("%q is not a number", v.String())
+		}
+		f = parsed
+	case string:
+		parsed, err := strconv.ParseFloat(strings.TrimSpace(v), 64)
+		if err != nil {
+			return 0, fmt.Errorf("%q is not a number", v)
+		}
+		f = parsed
+	default:
+		return 0, fmt.Errorf("expected a number in USD, got %T (%v)", val, val)
+	}
+	if f < 0 {
+		return 0, fmt.Errorf("must not be negative, got %v", f)
+	}
+	return f, nil
+}
+
 // buildLaunchConfigFromParams merges defaults with parameter overrides
 func buildLaunchConfigFromParams(defaults, params map[string]interface{}, sweepID, sweepName string, index, total int) (aws.LaunchConfig, error) {
 	// Start with an empty config
@@ -133,6 +173,22 @@
 			if s, ok := val.(string); ok {
 				config.IdleTimeout = s
 			}
+		case "cost_limit":
+			// Terminate/stop when compute spend reaches this many USD, i.e. the
+			// param-file form of --cost-limit. Before #525 there was no case for
+			// it, so `cost_limit: 8` fell through to the default: arm and became a
+			// PARAM_cost_limit env var that capped nothing — a spend control that
+			// parsed, launched, and did nothing.
+			//
+			// Unlike the string cases above this one *errors* on a value it cannot
+			// use instead of leaving the field zero. A mistyped ttl costs you the
+			// difference between two timeouts; a mistyped cost limit silently
+			// removes the only per-instance dollar cap on this path.
+			f, err := parseCostLimit(val)
+			if err != nil {
+				return config, fmt.Errorf("cost_limit: %w", err)
+			}
+			config.CostLimit = f
 		case "hibernate_on_idle":
 			if b, ok := val.(bool); ok {
 				config.HibernateOnIdle = b