From ce28436e2b8a392c163f4c8f63e3eb00060c3f12 Mon Sep 17 00:00:00 2001 From: scttfrdmn <3011922+scttfrdmn@users.noreply.github.com> Date: Tue, 18 Aug 2026 18:53:13 -0700 Subject: [PATCH] fix: cleanup no longer counts already-destroyed resources as removable Two compounding defects: 1. enrichInstanceState's NotFound fallback only fires on a batch DescribeInstances error, but EC2 answers an aged-out instance id with an empty result, not an error -- so State stayed "" for exactly the population cleanup exists to sweep. An id absent from a successful response is now marked "deleted" directly, in the success path, instead of relying on an error EC2 doesn't raise for this case. 2. cmd/cleanup.go's removable/running/address split never consulted State at all, so even a resource correctly resolved to "deleted" still landed in removable and in the "N resource(s) would be removed" count -- contradicting its own displayed state one line away. Extracted the split into splitCleanupResources with a new alreadyGone bucket, reported separately as tag-mapping residue and excluded from the removable count. Also added ignoreNotFound tolerance to RemoveResource's instance branch, matching its volume/key-pair/security-group siblings, so a real sweep of stale residue is a satisfied request rather than a doomed Terminate failure. Fixes #516 --- CHANGELOG.md | 19 +++++++ cmd/cleanup.go | 64 ++++++++++++++++++------ cmd/cleanup_test.go | 71 ++++++++++++++++++++++++++ pkg/aws/cleanup.go | 31 +++++++++++- pkg/aws/cleanup_test.go | 107 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 274 insertions(+), 18 deletions(-) create mode 100644 cmd/cleanup_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 117c3bf..cedae84 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/cmd/cleanup.go b/cmd/cleanup.go index 07e0c35..bf44d99 100644 --- a/cmd/cleanup.go +++ b/cmd/cleanup.go @@ -78,21 +78,7 @@ func runCleanup(cmd *cobra.Command, args []string) error { 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 @@ func runCleanup(cmd *cobra.Command, args []string) error { } } + 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 @@ func runCleanup(cmd *cobra.Command, args []string) error { 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 { diff --git a/cmd/cleanup_test.go b/cmd/cleanup_test.go new file mode 100644 index 0000000..ac58150 --- /dev/null +++ b/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 +} diff --git a/pkg/aws/cleanup.go b/pkg/aws/cleanup.go index 68b404b..d57ffaa 100644 --- a/pkg/aws/cleanup.go +++ b/pkg/aws/cleanup.go @@ -245,6 +245,15 @@ func (c *Client) enrichInstanceState(ctx context.Context, cfg aws.Config, resour } 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 @@ func (c *Client) enrichInstanceState(ctx context.Context, cfg aws.Config, resour } 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 @@ func (c *Client) RemoveResource(ctx context.Context, r ManagedResource) error { 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) diff --git a/pkg/aws/cleanup_test.go b/pkg/aws/cleanup_test.go index b295ad0..8f6ed72 100644 --- a/pkg/aws/cleanup_test.go +++ b/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, ¬FoundErr{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) + } +}