fix: cleanup no longer counts already-destroyed resources as removable

CHANGELOG.md

@@ -50,6 +50,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   callers for the same AZ now coalesce onto a single in-flight
   `CreatePlacementGroup` call instead of each issuing their own; callers for
   different AZs are not serialized against each other.
+- **`spawn cleanup --dry-run` (and `orphans`) reported already-destroyed
+  resources as "would be removed"** (#516). Two compounding defects: (1)
+  `enrichInstanceState`'s NotFound fallback only triggers on a batch
+  `DescribeInstances` *error*, but EC2 answers an aged-out (long-terminated)
+  instance id with an empty *result*, not an error — so `State` stayed `""`
+  for exactly the population `cleanup` exists to sweep, and (2) the
+  removable/running/address split in `cmd/cleanup.go` never consulted
+  `State` at all, so even a resource correctly resolved to `deleted` (e.g.
+  volumes, which *do* 400 on an already-deleted id) still landed in
+  `removable` and in the "N resource(s) would be removed" count — visibly
+  contradicting its own displayed `deleted` state in the same table. Fixed
+  both: an instance id absent from a successful `DescribeInstances` response
+  is now marked `deleted` directly (no longer relies on an error EC2 doesn't
+  raise for this case), and `cleanup`'s split now routes `State == "deleted"`
+  into its own bucket, excluded from the removable count and reported
+  separately as tag-mapping residue. Also added `ignoreNotFound` tolerance to
+  the instance branch of `RemoveResource`, matching its volume/key-pair/
+  security-group siblings, so a real (non-dry-run) sweep of stale residue
+  reports a satisfied request instead of a doomed `Terminate` failure.
 
 ## [0.100.2] - 2026-08-18
 

cmd/cleanup.go

@@ -78,21 +78,7 @@
 		return nil
 	}
 
-	// Split running instances out — they're never removed and gate cleanup.
-	// Elastic IPs are also split out: spawn never allocates them, so it never
-	// releases them (#262). They're reported for visibility but the user must
-	// release their own addresses.
-	var running, addresses, removable []aws.ManagedResource
-	for _, r := range found {
-		switch {
-		case r.IsRunningInstance():
-			running = append(running, r)
-		case r.ResourceType == "address":
-			addresses = append(addresses, r)
-		default:
-			removable = append(removable, r)
-		}
-	}
+	running, addresses, alreadyGone, removable := splitCleanupResources(found)
 
 	printResourceTable(cmd, found)
 
@@ -116,13 +102,32 @@
 		}
 	}
 
+	if len(alreadyGone) > 0 {
+		fmt.Fprintf(os.Stderr, "\nℹ️  %d resource(s) shown are tag-mapping residue for things that no longer exist (the Resource Groups Tagging API's index outlives the resource) — nothing to remove for these:\n", len(alreadyGone))
+		for _, r := range alreadyGone {
+			fmt.Fprintf(os.Stderr, "    %s %s (%s)\n", r.ResourceType, r.ID, r.Region)
+		}
+	}
+
 	if cleanupDryRun {
+		if len(removable) == 0 {
+			fmt.Fprintf(out, "\nDry run: 0 resource(s) would be removed")
+			if len(alreadyGone) > 0 {
+				fmt.Fprintf(out, " (%d tag mapping(s) are residue for resources that no longer exist)", len(alreadyGone))
+			}
+			fmt.Fprintln(out, ".")
+			return nil
+		}
 		fmt.Fprintf(out, "\nDry run: %d resource(s) would be removed. Re-run without --dry-run to delete.\n", len(removable))
 		return nil
 	}
 
 	if len(removable) == 0 {
-		fmt.Fprintln(out, "\nNo removable resources (only running instances present).")
+		if len(alreadyGone) > 0 {
+			fmt.Fprintln(out, "\nNo removable resources (only already-gone tag residue and/or running instances present).")
+		} else {
+			fmt.Fprintln(out, "\nNo removable resources (only running instances present).")
+		}
 		return nil
 	}
 
@@ -163,6 +168,33 @@
 	return nil
 }
 
+// splitCleanupResources partitions a discovery sweep into the four buckets
+// cleanup's output depends on:
+//   - running: running/pending instances — never removed, gate cleanup unless --dry-run
+//   - addresses: Elastic IPs — spawn never allocates or releases them (#262)
+//   - alreadyGone: resources whose State is already "deleted" — Resource Groups
+//     Tagging API tag mappings outlive the resources they describe, so a
+//     discovery sweep routinely returns ids for things no longer there.
+//     Counting these as "removable" overstates both the dry-run preview and
+//     the real confirmation prompt (spawn#516) — they must be split out, not
+//     folded into removable just because they aren't running or an address.
+//   - removable: everything else, the actual candidates for RemoveResource.
+func splitCleanupResources(found []aws.ManagedResource) (running, addresses, alreadyGone, removable []aws.ManagedResource) {
+	for _, r := range found {
+		switch {
+		case r.IsRunningInstance():
+			running = append(running, r)
+		case r.ResourceType == "address":
+			addresses = append(addresses, r)
+		case r.State == "deleted":
+			alreadyGone = append(alreadyGone, r)
+		default:
+			removable = append(removable, r)
+		}
+	}
+	return running, addresses, alreadyGone, removable
+}
+
 // deletionOrderCmd orders resources dependents-first. It mirrors the package
 // helper but is reachable from the cmd layer.
 func deletionOrderCmd(resources []aws.ManagedResource) []aws.ManagedResource {

cmd/cleanup_test.go

@@ -0,0 +1,71 @@
+package cmd
+
+import (
+	"testing"
+
+	"github.com/spore-host/spawn/pkg/aws"
+)
+
+// TestSplitCleanupResources_AlreadyGoneIsNotRemovable is the spawn#516
+// regression test for cleanup's second compounding defect: a resource whose
+// State the discovery/enrichment pipeline has already resolved to "deleted"
+// (Resource Groups Tagging API tag-mapping residue for something that no
+// longer exists) must land in its own bucket, not in removable. Before the
+// fix, removable's switch had no case for State at all — only
+// IsRunningInstance() and ResourceType == "address" were consulted, so a
+// "deleted" instance or volume fell through to the default case and was
+// counted as removable, contradicting its own displayed State.
+func TestSplitCleanupResources_AlreadyGoneIsNotRemovable(t *testing.T) {
+	found := []aws.ManagedResource{
+		{ResourceType: "instance", ID: "i-live", State: "stopped"},
+		{ResourceType: "instance", ID: "i-gone", State: "deleted"},
+		{ResourceType: "volume", ID: "vol-gone", State: "deleted"},
+		{ResourceType: "instance", ID: "i-running", State: "running"},
+		{ResourceType: "address", ID: "eipalloc-x", State: "unassociated"},
+	}
+
+	running, addresses, alreadyGone, removable := splitCleanupResources(found)
+
+	if len(running) != 1 || running[0].ID != "i-running" {
+		t.Errorf("running = %v, want [i-running]", idsOf(running))
+	}
+	if len(addresses) != 1 || addresses[0].ID != "eipalloc-x" {
+		t.Errorf("addresses = %v, want [eipalloc-x]", idsOf(addresses))
+	}
+	if len(alreadyGone) != 2 {
+		t.Errorf("alreadyGone = %v, want [i-gone, vol-gone]", idsOf(alreadyGone))
+	}
+	for _, r := range alreadyGone {
+		if r.ID != "i-gone" && r.ID != "vol-gone" {
+			t.Errorf("unexpected resource in alreadyGone: %s", r.ID)
+		}
+	}
+	if len(removable) != 1 || removable[0].ID != "i-live" {
+		t.Errorf("removable = %v, want [i-live] (deleted resources must NOT be removable)", idsOf(removable))
+	}
+}
+
+// TestSplitCleanupResources_NoStateIsStillRemovable confirms the fix doesn't
+// over-correct: a resource with no resolved State yet (State == "", meaning
+// unknown/unresolved rather than confirmed-gone) still goes to removable —
+// only an explicit "deleted" is excluded.
+func TestSplitCleanupResources_NoStateIsStillRemovable(t *testing.T) {
+	found := []aws.ManagedResource{
+		{ResourceType: "security-group", ID: "sg-x", State: ""},
+	}
+	_, _, alreadyGone, removable := splitCleanupResources(found)
+	if len(alreadyGone) != 0 {
+		t.Errorf("alreadyGone = %v, want empty for a resource with no State opinion", idsOf(alreadyGone))
+	}
+	if len(removable) != 1 {
+		t.Errorf("removable = %v, want [sg-x]", idsOf(removable))
+	}
+}
+
+func idsOf(rs []aws.ManagedResource) []string {
+	ids := make([]string, len(rs))
+	for i, r := range rs {
+		ids[i] = r.ID
+	}
+	return ids
+}

pkg/aws/cleanup.go

@@ -245,6 +245,15 @@
 	}
 
 	ec2Client := ec2.NewFromConfig(cfg)
+	return c.enrichInstanceStateWith(ctx, ec2Client, resources, ids, idx)
+}
+
+// enrichInstanceStateWith is enrichInstanceState's testable core: it takes the
+// narrow describeInstancesAPI interface instead of building a live *ec2.Client,
+// so the aged-out-instance regression (spawn#516) is unit-tested without real
+// AWS or the substrate emulator (which doesn't reproduce the batch-NotFound
+// failure the per-id fallback exists to handle).
+func (c *Client) enrichInstanceStateWith(ctx context.Context, ec2Client describeInstancesAPI, resources []ManagedResource, ids []string, idx map[string]int) error {
 	out, err := ec2Client.DescribeInstances(ctx, &ec2.DescribeInstancesInput{InstanceIds: ids})
 	if err != nil {
 		// EC2 fails the WHOLE batch with InvalidInstanceID.NotFound if ANY id is
@@ -256,14 +265,29 @@
 		}
 		return fmt.Errorf("describe instance state: %w", err)
 	}
+	seen := make(map[string]bool, len(ids))
 	for _, res := range out.Reservations {
 		for _, inst := range res.Instances {
 			id := aws.ToString(inst.InstanceId)
 			if i, ok := idx[id]; ok {
 				resources[i].State = string(inst.State.Name)
+				seen[id] = true
 			}
 		}
 	}
+	// An instance id EC2 aged out of DescribeInstances entirely (not an error —
+	// an empty result) is just as gone as one that 404s. Without this, State
+	// stays "" for every id in this population, which enrichInstanceStatePerID's
+	// fallback promises never happens but can't deliver on: the batch call
+	// SUCCEEDED here (aged-out ids don't trigger InvalidInstanceID.NotFound, only
+	// syntactically-invalid ones do), so the error branch above is never taken and
+	// the fallback never runs (spawn#516). Mark them "deleted" in the success
+	// path instead of relying on an error that EC2 doesn't raise for this case.
+	for id, i := range idx {
+		if !seen[id] {
+			resources[i].State = "deleted"
+		}
+	}
 	return nil
 }
 
@@ -449,8 +473,11 @@
 		if r.IsRunningInstance() {
 			return fmt.Errorf("refusing to remove running instance %s — stop or terminate it first", r.ID)
 		}
-		// stopped/stopping instances: terminate.
-		return c.Terminate(ctx, cfg.Region, r.ID)
+		// stopped/stopping instances: terminate. Tolerate NotFound like every
+		// other branch here — an instance already gone (e.g. tag-mapping
+		// residue that slipped past the removable/alreadyGone split) is a
+		// satisfied request, not a failure (spawn#516).
+		return ignoreNotFound(c.Terminate(ctx, cfg.Region, r.ID), "InvalidInstanceID.NotFound")
 
 	case r.ResourceType == "security-group":
 		ec2c := ec2.NewFromConfig(cfg)

pkg/aws/cleanup_test.go

@@ -240,3 +240,110 @@ func resourceTypes(rs []ManagedResource) []string {
 	}
 	return out
 }
+
+// fakeInstanceAPI reproduces the TWO distinct absence shapes real EC2's
+// DescribeInstances can return, which is exactly the distinction spawn#516
+// is about:
+//   - a syntactically-valid-but-never-existed id: the WHOLE batch call fails
+//     with InvalidInstanceID.NotFound (errorOnIDs)
+//   - a real id that has since aged out of the API entirely: the call
+//     SUCCEEDS with an empty (or partial) result — no error at all
+//     (states simply omits the id)
+//
+// The substrate emulator does not reproduce the first shape (hence
+// fakeVolumeAPI's real-world counterpart above), and no existing fake
+// reproduced the second, which is the shape enrichInstanceState mishandled.
+type fakeInstanceAPI struct {
+	states    map[string]string // id -> state; absent id == aged out (empty result, no error)
+	errorOnID string            // if set and present in the request, the WHOLE call 404s instead
+}
+
+func (f *fakeInstanceAPI) DescribeInstances(_ context.Context, in *ec2.DescribeInstancesInput, _ ...func(*ec2.Options)) (*ec2.DescribeInstancesOutput, error) {
+	if f.errorOnID != "" {
+		for _, id := range in.InstanceIds {
+			if id == f.errorOnID {
+				return nil, &notFoundErr{code: "InvalidInstanceID.NotFound"}
+			}
+		}
+	}
+	var reservations []ec2types.Reservation
+	for _, id := range in.InstanceIds {
+		state, ok := f.states[id]
+		if !ok {
+			continue // aged out: EC2 just omits it, no error (spawn#516)
+		}
+		id := id
+		reservations = append(reservations, ec2types.Reservation{
+			Instances: []ec2types.Instance{{
+				InstanceId: &id,
+				State:      &ec2types.InstanceState{Name: ec2types.InstanceStateName(state)},
+			}},
+		})
+	}
+	return &ec2.DescribeInstancesOutput{Reservations: reservations}, nil
+}
+
+// TestEnrichInstanceState_AgedOutInstanceIsMarkedDeleted is the spawn#516
+// regression test: DescribeInstances returning an EMPTY result for an
+// aged-out instance id (no error at all — the batch call SUCCEEDS) must
+// still result in State == "deleted", not "" (which cleanup's removable
+// split then misreads as "not running, therefore fine to delete", when the
+// real problem is the id doesn't exist to describe in the first place).
+//
+// Before the fix, enrichInstanceState only ever set State from a
+// reservation it actually saw; an id EC2 silently dropped from the response
+// never got any State at all, and the isInstanceNotFound-triggered per-id
+// fallback never ran because DescribeInstances didn't return an error here.
+func TestEnrichInstanceState_AgedOutInstanceIsMarkedDeleted(t *testing.T) {
+	c := &Client{}
+	api := &fakeInstanceAPI{states: map[string]string{
+		"i-live": "running",
+		// i-aged-out is absent from states -> DescribeInstances omits it,
+		// with NO error (this is the real-world shape; contrast with
+		// i-neverexisted below, which DOES error).
+	}}
+	resources := []ManagedResource{
+		{ResourceType: "instance", ID: "i-live"},
+		{ResourceType: "instance", ID: "i-aged-out"},
+	}
+	ids := []string{"i-live", "i-aged-out"}
+	idx := map[string]int{"i-live": 0, "i-aged-out": 1}
+
+	if err := c.enrichInstanceStateWith(context.Background(), api, resources, ids, idx); err != nil {
+		t.Fatalf("enrichInstanceStateWith: %v", err)
+	}
+	if resources[0].State != "running" {
+		t.Errorf("i-live state = %q, want running", resources[0].State)
+	}
+	if resources[1].State != "deleted" {
+		t.Errorf("i-aged-out state = %q, want deleted (not empty)", resources[1].State)
+	}
+}
+
+// TestEnrichInstanceState_BatchNotFoundStillFallsBack confirms the OTHER
+// absence shape (a batch 404 from a syntactically-valid-but-never-existed
+// id) still routes into the per-id fallback exactly as before — this fix
+// must not regress the existing #500-era path.
+func TestEnrichInstanceState_BatchNotFoundStillFallsBack(t *testing.T) {
+	c := &Client{}
+	api := &fakeInstanceAPI{
+		states:    map[string]string{"i-live": "stopped"},
+		errorOnID: "i-neverexisted",
+	}
+	resources := []ManagedResource{
+		{ResourceType: "instance", ID: "i-live"},
+		{ResourceType: "instance", ID: "i-neverexisted"},
+	}
+	ids := []string{"i-live", "i-neverexisted"}
+	idx := map[string]int{"i-live": 0, "i-neverexisted": 1}
+
+	if err := c.enrichInstanceStateWith(context.Background(), api, resources, ids, idx); err != nil {
+		t.Fatalf("enrichInstanceStateWith: %v", err)
+	}
+	if resources[0].State != "stopped" {
+		t.Errorf("i-live state = %q, want stopped", resources[0].State)
+	}
+	if resources[1].State != "deleted" {
+		t.Errorf("i-neverexisted state = %q, want deleted", resources[1].State)
+	}
+}