fix: release an ENI's EIP association and reap ENIs whose instance is gone (#955)

spinifex/daemon/eni_orphan_reaper.go

@@ -2,6 +2,7 @@ package daemon
 
 import (
 	"context"
+	"fmt"
 	"log/slog"
 	"time"
 
@@ -17,35 +18,86 @@ import (
 // this leaves a wide margin past the point where a launch could still attach.
 const eniOrphanMinAge = 15 * time.Minute
 
-// eniOrphanReaper deletes ENI records left behind by a launch that created the
-// interface and then died before attaching it. The record outlives the launch,
-// and because the security group dependency check counts an ENI that lists the
-// group, the SG can never be deleted afterwards.
+// instanceIndex answers whether the control plane still knows an instance. Both
+// listings together cover an instance's whole life: the shared record space
+// holds it while it runs and while it is stopped, and the terminated bucket
+// holds it for the hour after it goes away.
+type instanceIndex interface {
+	ListInstanceRecords() ([]*vm.InstanceRecord, error)
+	ListTerminatedInstanceRecords() ([]*vm.InstanceRecord, error)
+}
+
+// eipDisassociator releases the association an Elastic IP holds on an ENI,
+// leaving the allocation itself in place.
+type eipDisassociator interface {
+	DisassociateByENI(ctx context.Context, accountID, eniID string) (bool, error)
+}
+
+// eniOrphanReaper deletes ENI records no instance can still be using. Two shapes
+// qualify, and neither has anything else that would ever remove it:
 //
-// Cluster-wide rather than node-local: the record has no instance, so no node
-// owns it, and nothing local would ever look at it again.
+// A launch that created the interface and then died before attaching it leaves a
+// record with no instance at all. Because the security group dependency check
+// counts an ENI that lists the group, the SG can never be deleted afterwards.
+//
+// A teardown that never finished leaves the opposite: a record still naming an
+// instance that exists nowhere. The node-local teardown reaper would have
+// re-driven it, but only from the terminated bucket, which expires after an
+// hour — past that the ENI, its logical switch port and its EIP association are
+// permanent, and the network reconciler keeps driving toward a port that can
+// never bind.
+//
+// Cluster-wide rather than node-local: neither shape has a live instance, so no
+// node owns it, and nothing local would ever look at it again.
 type eniOrphanReaper struct {
-	vpc    *handlers_ec2_vpc.VPCServiceImpl
-	minAge time.Duration
+	vpc       *handlers_ec2_vpc.VPCServiceImpl
+	instances instanceIndex
+	eip       eipDisassociator
+	minAge    time.Duration
+	// missing holds the ENIs whose instance was absent last sweep. An instance
+	// record is written after its ENI is attached, so a single absent reading
+	// can be a launch mid-flight rather than a zombie; only an ENI missing from
+	// two consecutive sweeps is reaped.
+	missing map[string]bool
 }
 
 var _ vm.Reaper = (*eniOrphanReaper)(nil)
 
 // newENIOrphanReaper builds the sweep over the daemon's VPC service, or nil
-// when there is no control plane on this node to sweep with.
+// when there is no control plane on this node to sweep with. A nil jsManager or
+// EIP service degrades the sweep rather than disabling it: see Sweep.
 func (d *Daemon) newENIOrphanReaper() *eniOrphanReaper {
 	if d.vpcService == nil {
 		return nil
 	}
-	return &eniOrphanReaper{vpc: d.vpcService, minAge: eniOrphanMinAge}
+	r := &eniOrphanReaper{vpc: d.vpcService, minAge: eniOrphanMinAge}
+	if d.jsManager != nil {
+		r.instances = d.jsManager
+	}
+	if eip, ok := d.eipService.(eipDisassociator); ok {
+		r.eip = eip
+	}
+	return r
 }
 
 func (r *eniOrphanReaper) Class() string         { return "eni-orphan" }
 func (r *eniOrphanReaper) Scope() vm.ReaperScope { return vm.ScopeClusterWide }
 
-// Sweep deletes every abandoned instance ENI. A per-record failure is logged
-// and skipped so one bad record cannot stall the rest of the sweep.
+// Sweep deletes every abandoned launch ENI and every ENI whose instance is gone.
+// A per-record failure is logged and skipped so one bad record cannot stall the
+// rest of the sweep.
 func (r *eniOrphanReaper) Sweep(ctx context.Context) (int, error) {
+	reaped, err := r.sweepAbandonedLaunches(ctx)
+	if err != nil {
+		return reaped, err
+	}
+	stale, err := r.sweepStaleAttachments(ctx)
+	return reaped + stale, err
+}
+
+// sweepAbandonedLaunches deletes ENIs that never reached an attachment. A plain
+// delete suffices: with no attachment there is no in-use guard to clear.
+func (r *eniOrphanReaper) sweepAbandonedLaunches(ctx context.Context) (int, error) {
 	orphans, err := r.vpc.ListAbandonedInstanceENIs(ctx, r.minAge)
 	if err != nil {
 		return 0, err
@@ -78,3 +130,161 @@ func (r *eniOrphanReaper) Sweep(ctx context.Context) (int, error) {
 	}
 	return reaped, nil
 }
+
+// sweepStaleAttachments deletes ENIs still naming an instance the control plane
+// no longer holds anywhere. The delete uses the force path, which exists for
+// exactly this owner-driven teardown; the in-use guard is left alone, since it
+// is right to reject a delete aimed at another live instance's interface.
+func (r *eniOrphanReaper) sweepStaleAttachments(ctx context.Context) (int, error) {
+	if r.instances == nil {
+		return 0, nil
+	}
+
+	// Candidates before the keep set, so the set is never older than the
+	// listing it judges, and so a cluster with nothing to sweep pays for
+	// neither instance listing.
+	attached, err := r.vpc.ListAttachedInstanceENIs(ctx, r.minAge)
+	if err != nil {
+		return 0, err
+	}
+	if len(attached) == 0 {
+		r.missing = nil
+		return 0, nil
+	}
+
+	known, err := r.knownInstanceIDs()
+	if err != nil {
+		// Fail closed. A short listing reads as "the instance is gone" for every
+		// instance missing from it, which would reap live guests' interfaces.
+		slog.WarnContext(ctx, "eni-orphan: instance listing failed; skipping stale-attachment sweep", "err", err)
+		r.missing = nil
+		return 0, nil
+	}
+	if len(known) == 0 {
+		// The record space is recreated when missing and republished only on a
+		// node's next state change, so an empty listing beside live ENIs is far
+		// likelier to be an emptied bucket than a cluster with no instances.
+		slog.WarnContext(ctx, "eni-orphan: no instance records at all beside attached ENIs; declining the stale-attachment sweep",
+			"candidates", len(attached))
+		r.missing = nil
+		return 0, nil
+	}
+
+	reaped := 0
+	stillMissing := make(map[string]bool, len(r.missing))
+	for _, candidate := range attached {
+		if ctx.Err() != nil {
+			r.missing = stillMissing
+			return reaped, ctx.Err()
+		}
+		if known[candidate.Record.InstanceId] {
+			continue
+		}
+
+		eniID := candidate.Record.NetworkInterfaceId
+		if !r.missing[eniID] {
+			stillMissing[eniID] = true
+			slog.InfoContext(ctx, "eni-orphan: ENI names an instance the control plane does not hold; deferring to the next sweep",
+				"eniId", eniID, "accountId", candidate.AccountID, "instanceId", candidate.Record.InstanceId)
+			continue
+		}
+		if r.reapStale(ctx, candidate) {
+			reaped++
+			continue
+		}
+		stillMissing[eniID] = true
+	}
+	r.missing = stillMissing
+	return reaped, nil
+}
+
+// reapStale frees one zombie ENI, reporting whether the record is settled. An
+// interface the owner asked to keep is detached rather than deleted, matching
+// what terminate does with the same flag.
+func (r *eniOrphanReaper) reapStale(ctx context.Context, candidate handlers_ec2_vpc.AccountENI) bool {
+	if keep := candidate.Record.DeleteOnTermination; keep != nil && !*keep {
+		return r.detachStale(ctx, candidate)
+	}
+
+	eniID := candidate.Record.NetworkInterfaceId
+	slog.InfoContext(ctx, "eni-orphan: deleting ENI whose instance no longer exists",
+		"eniId", eniID,
+		"accountId", candidate.AccountID,
+		"instanceId", candidate.Record.InstanceId,
+		"privateIp", candidate.Record.PrivateIpAddress,
+		"createdAt", candidate.Record.CreatedAt)
+
+	if err := r.vpc.ForceDeleteInstanceENI(ctx, candidate.AccountID, eniID); err != nil {
+		slog.WarnContext(ctx, "eni-orphan: stale ENI delete failed",
+			"eniId", eniID, "accountId", candidate.AccountID, "err", err)
+		return false
+	}
+
+	// The EIP goes after the interface. The delete decides whether the ENI's
+	// public address may return to the pool by looking for an EIP that names
+	// the ENI, so clearing the association first would hand a still-allocated
+	// address back to IPAM.
+	r.releaseEIP(ctx, candidate.AccountID, eniID)
+	return true
+}
+
+// detachStale clears a dead instance off an ENI that was asked to outlive it —
+// an RDS endpoint, a customer's reserved address. Detaching takes the record
+// out of the sweep's reach and leaves the interface, and its EIP, in place.
+func (r *eniOrphanReaper) detachStale(ctx context.Context, candidate handlers_ec2_vpc.AccountENI) bool {
+	eniID := candidate.Record.NetworkInterfaceId
+	slog.InfoContext(ctx, "eni-orphan: detaching keep-on-terminate ENI whose instance no longer exists",
+		"eniId", eniID,
+		"accountId", candidate.AccountID,
+		"instanceId", candidate.Record.InstanceId,
+		"privateIp", candidate.Record.PrivateIpAddress)
+
+	if err := r.vpc.DetachENI(ctx, candidate.AccountID, eniID); err != nil {
+		slog.WarnContext(ctx, "eni-orphan: stale ENI detach failed",
+			"eniId", eniID, "accountId", candidate.AccountID, "err", err)
+		return false
+	}
+	return true
+}
+
+// releaseEIP drops any EIP association held on eniID, after the interface is
+// gone. Nothing retries a failure here: the ENI record the sweep finds these
+// through no longer exists, so the association is stranded until an operator
+// disassociates the allocation by hand.
+func (r *eniOrphanReaper) releaseEIP(ctx context.Context, accountID, eniID string) {
+	if r.eip == nil {
+		return
+	}
+	found, err := r.eip.DisassociateByENI(ctx, accountID, eniID)
+	if err != nil {
+		slog.ErrorContext(ctx, "eni-orphan: EIP association stranded on a deleted ENI; recover with DisassociateAddress",
+			"eniId", eniID, "accountId", accountID, "err", err)
+		return
+	}
+	if found {
+		slog.InfoContext(ctx, "eni-orphan: released EIP association from a stale ENI",
+			"eniId", eniID, "accountId", accountID)
+	}
+}
+
+// knownInstanceIDs is every instance the control plane still holds a record for,
+// running, stopped or recently terminated.
+func (r *eniOrphanReaper) knownInstanceIDs() (map[string]bool, error) {
+	live, err := r.instances.ListInstanceRecords()
+	if err != nil {
+		return nil, fmt.Errorf("list instance records: %w", err)
+	}
+	terminated, err := r.instances.ListTerminatedInstanceRecords()
+	if err != nil {
+		return nil, fmt.Errorf("list terminated instance records: %w", err)
+	}
+
+	known := make(map[string]bool, len(live)+len(terminated))
+	for _, record := range live {
+		known[record.Metadata.Name] = true
+	}
+	for _, record := range terminated {
+		known[record.Metadata.Name] = true
+	}
+	return known, nil
+}

spinifex/daemon/eni_orphan_reaper_test.go

@@ -2,12 +2,14 @@ package daemon
 
 import (
 	"context"
+	"errors"
 	"testing"
 	"time"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc"
+	"github.com/mulgadc/spinifex/spinifex/resource"
 	"github.com/mulgadc/spinifex/spinifex/testutil"
 	"github.com/mulgadc/spinifex/spinifex/vm"
 	"github.com/stretchr/testify/assert"
@@ -66,3 +68,241 @@ func TestENIOrphanReaperSweep(t *testing.T) {
 	require.NoError(t, err)
 	assert.Zero(t, reaped)
 }
+
+// fakeInstanceIndex answers the instance-existence question from fixed sets.
+type fakeInstanceIndex struct {
+	live       []string
+	terminated []string
+	liveErr    error
+	termErr    error
+}
+
+func instanceRecords(ids []string) []*vm.InstanceRecord {
+	out := make([]*vm.InstanceRecord, 0, len(ids))
+	for _, id := range ids {
+		out = append(out, &vm.InstanceRecord{Metadata: resource.Metadata{Name: id}})
+	}
+	return out
+}
+
+func (f *fakeInstanceIndex) ListInstanceRecords() ([]*vm.InstanceRecord, error) {
+	return instanceRecords(f.live), f.liveErr
+}
+
+func (f *fakeInstanceIndex) ListTerminatedInstanceRecords() ([]*vm.InstanceRecord, error) {
+	return instanceRecords(f.terminated), f.termErr
+}
+
+// fakeEIPDisassociator records which ENIs were asked to give up their EIP.
+type fakeEIPDisassociator struct {
+	associated map[string]bool
+	calls      []string
+	err        error
+	// onCall observes the world as the disassociation sees it, so a test can
+	// pin the ordering the release depends on.
+	onCall func(eniID string)
+}
+
+func (f *fakeEIPDisassociator) DisassociateByENI(_ context.Context, _, eniID string) (bool, error) {
+	if f.onCall != nil {
+		f.onCall(eniID)
+	}
+	f.calls = append(f.calls, eniID)
+	if f.err != nil {
+		return false, f.err
+	}
+	return f.associated[eniID], nil
+}
+
+// staleReaperFixture builds a VPC with one ENI attached to instanceID, aged past
+// the sweep's guard, plus the reaper under test.
+func staleReaperFixture(t *testing.T, instanceID string, index instanceIndex, eip eipDisassociator) (*eniOrphanReaper, *handlers_ec2_vpc.VPCServiceImpl, string) {
+	t.Helper()
+	_, nc, _ := testutil.StartTestJetStream(t)
+	vpcSvc, err := handlers_ec2_vpc.NewVPCServiceImplWithNATS(t.Context(), nil, nc)
+	require.NoError(t, err)
+	testutil.StubVpcdSGResponder(t, nc)
+
+	ctx := t.Context()
+	vpcOut, err := vpcSvc.CreateVpc(ctx, &ec2.CreateVpcInput{CidrBlock: aws.String("10.0.0.0/16")}, reaperTestAccountID)
+	require.NoError(t, err)
+	subnetOut, err := vpcSvc.CreateSubnet(ctx, &ec2.CreateSubnetInput{
+		VpcId:     vpcOut.Vpc.VpcId,
+		CidrBlock: aws.String("10.0.1.0/24"),
+	}, reaperTestAccountID)
+	require.NoError(t, err)
+
+	eniOut, err := vpcSvc.CreateNetworkInterface(ctx, &ec2.CreateNetworkInterfaceInput{
+		SubnetId:    subnetOut.Subnet.SubnetId,
+		Description: aws.String(handlers_ec2_vpc.AutoENIDescriptionPrefix + instanceID),
+	}, reaperTestAccountID)
+	require.NoError(t, err)
+	eniID := *eniOut.NetworkInterface.NetworkInterfaceId
+
+	_, err = vpcSvc.AttachENI(reaperTestAccountID, eniID, instanceID, 0)
+	require.NoError(t, err)
+	require.NoError(t, vpcSvc.UpdateENI(reaperTestAccountID, eniID, func(rec *handlers_ec2_vpc.ENIRecord) {
+		rec.CreatedAt = time.Now().Add(-time.Hour)
+	}))
+
+	return &eniOrphanReaper{vpc: vpcSvc, instances: index, eip: eip, minAge: 15 * time.Minute}, vpcSvc, eniID
+}
+
+// TestENIOrphanReaperReapsStaleAttachment is the zombie case: the ENI still
+// names an instance, but that instance is in neither the live nor the terminated
+// space, so nothing else will ever delete the record — and until it goes, its
+// logical switch port stays unbindable and its EIP stays associated.
+func TestENIOrphanReaperReapsStaleAttachment(t *testing.T) {
+	index := &fakeInstanceIndex{live: []string{"i-somebody-else"}, terminated: []string{"i-recent"}}
+	eip := &fakeEIPDisassociator{associated: map[string]bool{}}
+	r, vpcSvc, eniID := staleReaperFixture(t, "i-gone", index, eip)
+	eip.associated[eniID] = true
+	// The delete decides whether the ENI's public address may go back to the
+	// pool by looking for an EIP that names the ENI, so the association has to
+	// still be there while it runs.
+	eip.onCall = func(id string) {
+		_, err := vpcSvc.GetENIRecord(reaperTestAccountID, id)
+		assert.Error(t, err, "the EIP must be released after the interface, never before")
+	}
+
+	// First sighting only defers: an instance record is written after its ENI
+	// is attached, so one absent reading can be a launch mid-flight.
+	reaped, err := r.Sweep(t.Context())
+	require.NoError(t, err)
+	assert.Zero(t, reaped)
+	_, err = vpcSvc.GetENIRecord(reaperTestAccountID, eniID)
+	require.NoError(t, err, "a single absent reading is not evidence enough to delete an interface")
+	assert.Empty(t, eip.calls)
+
+	reaped, err = r.Sweep(t.Context())
+	require.NoError(t, err)
+	assert.Equal(t, 1, reaped)
+
+	_, err = vpcSvc.GetENIRecord(reaperTestAccountID, eniID)
+	require.Error(t, err, "the record must be gone, or the reconciler keeps driving toward a port that cannot bind")
+	assert.Equal(t, []string{eniID}, eip.calls, "the EIP association must be released with the interface")
+
+	reaped, err = r.Sweep(t.Context())
+	require.NoError(t, err)
+	assert.Zero(t, reaped)
+}
+
+// TestENIOrphanReaperDeclinesOnEmptyInstanceListing pins the other half of the
+// fail-closed rule. The record space is recreated when missing and republished
+// only on a node's next state change, so an empty listing beside live ENIs is
+// an emptied bucket far more often than a cluster with no instances — and
+// reaping against it would cut every guest in the cluster off the network.
+func TestENIOrphanReaperDeclinesOnEmptyInstanceListing(t *testing.T) {
+	eip := &fakeEIPDisassociator{associated: map[string]bool{}}
+	r, vpcSvc, eniID := staleReaperFixture(t, "i-gone", &fakeInstanceIndex{}, eip)
+
+	for range 2 {
+		reaped, err := r.Sweep(t.Context())
+		require.NoError(t, err)
+		assert.Zero(t, reaped)
+	}
+
+	_, err := vpcSvc.GetENIRecord(reaperTestAccountID, eniID)
+	assert.NoError(t, err)
+	assert.Empty(t, eip.calls)
+}
+
+// TestENIOrphanReaperDetachesKeepOnTerminateENI pins that the sweep honours the
+// flag terminate honours. DeleteOnTermination=false means the interface outlives
+// its instance — an RDS endpoint holds its address this way — so a teardown that
+// never finished must not become the thing that destroys it.
+func TestENIOrphanReaperDetachesKeepOnTerminateENI(t *testing.T) {
+	index := &fakeInstanceIndex{live: []string{"i-somebody-else"}}
+	eip := &fakeEIPDisassociator{associated: map[string]bool{}}
+	r, vpcSvc, eniID := staleReaperFixture(t, "i-gone", index, eip)
+	require.NoError(t, vpcSvc.UpdateENI(reaperTestAccountID, eniID, func(rec *handlers_ec2_vpc.ENIRecord) {
+		rec.DeleteOnTermination = aws.Bool(false)
+	}))
+
+	require.NoError(t, sweepTwice(t, r))
+
+	rec, err := vpcSvc.GetENIRecord(reaperTestAccountID, eniID)
+	require.NoError(t, err, "the interface must survive; only its dead instance goes")
+	assert.Empty(t, rec.InstanceId)
+	assert.Equal(t, "available", rec.Status)
+	assert.Empty(t, eip.calls, "the EIP belongs to an interface that is still there")
+}
+
+// sweepTwice drives the two sightings a stale ENI needs before it is acted on.
+func sweepTwice(t *testing.T, r *eniOrphanReaper) error {
+	t.Helper()
+	if _, err := r.Sweep(t.Context()); err != nil {
+		return err
+	}
+	_, err := r.Sweep(t.Context())
+	return err
+}
+
+// TestENIOrphanReaperSparesKnownInstances pins the guard that matters most: an
+// ENI whose instance is running, stopped, or recently terminated is in use, and
+// deleting it would cut a live guest off the network.
+func TestENIOrphanReaperSparesKnownInstances(t *testing.T) {
+	for name, index := range map[string]*fakeInstanceIndex{
+		"running or stopped": {live: []string{"i-known"}},
+		"terminated":         {terminated: []string{"i-known"}},
+	} {
+		t.Run(name, func(t *testing.T) {
+			eip := &fakeEIPDisassociator{associated: map[string]bool{}}
+			r, vpcSvc, eniID := staleReaperFixture(t, "i-known", index, eip)
+
+			require.NoError(t, sweepTwice(t, r))
+
+			_, err := vpcSvc.GetENIRecord(reaperTestAccountID, eniID)
+			assert.NoError(t, err)
+			assert.Empty(t, eip.calls)
+		})
+	}
+}
+
+// TestENIOrphanReaperSkipsStaleSweepOnListingError pins the fail-closed rule. A
+// listing that errors part-way reads as "the instance is gone" for everything
+// missing from it, so the sweep must decline rather than reap against it.
+func TestENIOrphanReaperSkipsStaleSweepOnListingError(t *testing.T) {
+	for name, index := range map[string]*fakeInstanceIndex{
+		"live listing failed":       {liveErr: errors.New("kv unavailable")},
+		"terminated listing failed": {live: []string{"i-other"}, termErr: errors.New("kv unavailable")},
+	} {
+		t.Run(name, func(t *testing.T) {
+			eip := &fakeEIPDisassociator{associated: map[string]bool{}}
+			r, vpcSvc, eniID := staleReaperFixture(t, "i-gone", index, eip)
+
+			require.NoError(t, sweepTwice(t, r), "a listing fault is not the sweep's failure; it just cannot decide")
+
+			_, err := vpcSvc.GetENIRecord(reaperTestAccountID, eniID)
+			assert.NoError(t, err)
+		})
+	}
+}
+
+// TestENIOrphanReaperWithoutInstanceIndexSkipsStaleSweep pins that a node with
+// no instance-record source (no JetStream manager) still runs the
+// abandoned-launch sweep and simply declines the staleness question.
+func TestENIOrphanReaperWithoutInstanceIndexSkipsStaleSweep(t *testing.T) {
+	r, vpcSvc, eniID := staleReaperFixture(t, "i-gone", nil, nil)
+
+	require.NoError(t, sweepTwice(t, r))
+
+	_, err := vpcSvc.GetENIRecord(reaperTestAccountID, eniID)
+	assert.NoError(t, err)
+}
+
+// TestENIOrphanReaperDeletesENIDespiteEIPFailure pins that a stuck EIP
+// disassociation cannot pin the ENI. Leaving the interface behind is the worse
+// outcome: the zombie port stays unbindable, while the stranded association is
+// at least logged and recoverable by hand.
+func TestENIOrphanReaperDeletesENIDespiteEIPFailure(t *testing.T) {
+	index := &fakeInstanceIndex{live: []string{"i-somebody-else"}}
+	eip := &fakeEIPDisassociator{err: errors.New("kv unavailable")}
+	r, vpcSvc, eniID := staleReaperFixture(t, "i-gone", index, eip)
+
+	require.NoError(t, sweepTwice(t, r))
+
+	_, err := vpcSvc.GetENIRecord(reaperTestAccountID, eniID)
+	assert.Error(t, err)
+	assert.Equal(t, []string{eniID}, eip.calls)
+}

spinifex/daemon/vm_adapters.go

@@ -832,6 +832,9 @@ func (a *instanceCleanerAdapter) DetachAndDeleteENI(instance *vm.VM) error {
 			slog.Info("ENI already absent on termination",
 				"eni", instance.ENIId, "instanceId", instance.ID)
 		}
+		if err == nil {
+			a.disassociateENIEIP(instance.AccountID, instance.ENIId)
+		}
 	}
 
 	a.releaseAttachedENIs(instance)
@@ -876,6 +879,7 @@ func (a *instanceCleanerAdapter) releaseAttachedENIs(instance *vm.VM) {
 				"eni", eniId, "instanceId", instance.ID, "err", err)
 			continue
 		}
+		a.disassociateENIEIP(instance.AccountID, eniId)
 		if deleted {
 			slog.Info("Deleted attached ENI on termination",
 				"eni", eniId, "instanceId", instance.ID)
@@ -886,6 +890,30 @@ func (a *instanceCleanerAdapter) releaseAttachedENIs(instance *vm.VM) {
 	}
 }
 
+// disassociateENIEIP releases any Elastic IP associated with an ENI that has
+// just been deleted. Nothing else did: the ENI delete deliberately leaves an
+// EIP-owned public address alone, since the allocation outlives the interface,
+// but the association is the interface's and has to go with it. Left behind, the
+// EIP record stays "associated" and the network reconciler keeps re-asserting a
+// NAT rule for a guest that no longer exists.
+//
+// Called after the delete, never before: the delete decides whether the ENI's
+// public address may return to the pool by looking for an EIP that names it.
+func (a *instanceCleanerAdapter) disassociateENIEIP(accountID, eniID string) {
+	eip, ok := a.d.eipService.(eipDisassociator)
+	if !ok {
+		return
+	}
+	found, err := eip.DisassociateByENI(context.Background(), accountID, eniID)
+	if err != nil {
+		slog.Warn("Failed to disassociate EIP on termination", "eni", eniID, "accountId", accountID, "err", err)
+		return
+	}
+	if found {
+		slog.Info("Disassociated EIP on termination", "eni", eniID, "accountId", accountID)
+	}
+}
+
 // RemoveFromPlacementGroup unbinds the instance from its placement group
 // if one is set. No-op for ungrouped instances and when the placement
 // group service is not configured.

spinifex/daemon/vm_adapters_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/mulgadc/spinifex/spinifex/awserrors"
 	"github.com/mulgadc/spinifex/spinifex/config"
 	"github.com/mulgadc/spinifex/spinifex/gpu"
+	handlers_ec2_eip "github.com/mulgadc/spinifex/spinifex/handlers/ec2/eip"
 	handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc"
 	"github.com/mulgadc/spinifex/spinifex/tags"
 	"github.com/mulgadc/spinifex/spinifex/testutil"
@@ -447,6 +448,8 @@ func TestInstanceCleanerAdapter_DetachAndDeleteENI_ReleasesPostLaunchAttach(t *t
 func TestInstanceCleanerAdapter_DetachAndDeleteENI_DeleteOnTerminationFalseDetachesOnly(t *testing.T) {
 	f := newENIHotPlugFixture(t)
 	f.vmInst.AccountID = testAccountID
+	eip := newStubEIPDisassociator(f.eniID)
+	f.daemon.eipService = eip
 
 	_, err := f.daemon.vpcService.AttachENI(testAccountID, f.eniID, f.vmInst.ID, 1)
 	require.NoError(t, err)
@@ -461,6 +464,89 @@ func TestInstanceCleanerAdapter_DetachAndDeleteENI_DeleteOnTerminationFalseDetac
 	require.NoError(t, err, "DeleteOnTermination=false must detach, not delete")
 	assert.Equal(t, "available", rec.Status)
 	assert.Empty(t, rec.InstanceId)
+	assert.Empty(t, eip.calls,
+		"an EIP belongs to the interface, and the interface survives, so the association must too")
+}
+
+// stubEIPDisassociator embeds the EIP service interface so only the one method
+// terminate reaches for has to be real; anything else panics rather than
+// silently answering.
+type stubEIPDisassociator struct {
+	handlers_ec2_eip.EIPService
+
+	associated map[string]bool
+	calls      []string
+	// onCall observes the world as the disassociation sees it, so a test can
+	// pin the ordering the release depends on.
+	onCall func(eniID string)
+}
+
+func newStubEIPDisassociator(associatedENIs ...string) *stubEIPDisassociator {
+	s := &stubEIPDisassociator{associated: make(map[string]bool, len(associatedENIs))}
+	for _, eniID := range associatedENIs {
+		s.associated[eniID] = true
+	}
+	return s
+}
+
+func (s *stubEIPDisassociator) DisassociateByENI(_ context.Context, _, eniID string) (bool, error) {
+	if s.onCall != nil {
+		s.onCall(eniID)
+	}
+	s.calls = append(s.calls, eniID)
+	return s.associated[eniID], nil
+}
+
+// TestInstanceCleanerAdapter_DetachAndDeleteENI_ReleasesEIPAssociation pins the
+// half of the zombie the ENI delete never covered. releaseENISideEffects
+// deliberately leaves an EIP-owned public address alone, because the allocation
+// outlives the interface — but nothing released the association, so the EIP
+// record stayed "associated" against a guest that no longer exists and the
+// network reconciler kept re-asserting NAT for a port that could never bind.
+func TestInstanceCleanerAdapter_DetachAndDeleteENI_ReleasesEIPAssociation(t *testing.T) {
+	f := newENIHotPlugFixture(t)
+	f.vmInst.AccountID = testAccountID
+	f.vmInst.ENIId = f.eniID
+	eip := newStubEIPDisassociator(f.eniID)
+	f.daemon.eipService = eip
+	// The ENI delete decides whether the interface's public address may return
+	// to the IPAM pool by looking for an EIP that names it, so the association
+	// has to outlast the delete.
+	eip.onCall = func(id string) {
+		_, err := f.daemon.vpcService.GetENIRecord(testAccountID, id)
+		assert.Error(t, err, "the EIP must be released after the interface, never before")
+	}
+
+	_, err := f.daemon.vpcService.AttachENI(testAccountID, f.eniID, f.vmInst.ID, 0)
+	require.NoError(t, err)
+
+	cleaner := newInstanceCleanerAdapter(f.daemon)
+	require.NoError(t, cleaner.DetachAndDeleteENI(f.vmInst))
+
+	assert.Equal(t, []string{f.eniID}, eip.calls,
+		"the association must go with the interface it belongs to")
+	_, err = f.daemon.vpcService.GetENIRecord(testAccountID, f.eniID)
+	assert.True(t, awserrors.IsErrorCode(err, awserrors.ErrorInvalidNetworkInterfaceIDNotFound))
+}
+
+// TestInstanceCleanerAdapter_DetachAndDeleteENI_ReleasesEIPOnPostLaunchAttach
+// extends the release to the KV enumeration sweep. A hot-plug attach never sets
+// instance.ENIId, so covering only the primary path would leave every EIP on a
+// post-launch interface associated to a dead guest.
+func TestInstanceCleanerAdapter_DetachAndDeleteENI_ReleasesEIPOnPostLaunchAttach(t *testing.T) {
+	f := newENIHotPlugFixture(t)
+	f.vmInst.AccountID = testAccountID
+	eip := newStubEIPDisassociator(f.eniID)
+	f.daemon.eipService = eip
+
+	_, err := f.daemon.vpcService.AttachENI(testAccountID, f.eniID, f.vmInst.ID, 1)
+	require.NoError(t, err)
+	require.Empty(t, f.vmInst.ENIId, "precondition: launch-time scalar must stay unset")
+
+	cleaner := newInstanceCleanerAdapter(f.daemon)
+	require.NoError(t, cleaner.DetachAndDeleteENI(f.vmInst))
+
+	assert.Equal(t, []string{f.eniID}, eip.calls)
 }
 
 // TestInstanceCleanerAdapter_DetachAndDeleteENI_PrimaryENIReleased covers the

spinifex/handlers/ec2/eip/service_impl.go

@@ -299,14 +299,7 @@ func (s *EIPServiceImpl) DisassociateAddress(ctx context.Context, input *ec2.Dis
 		s.publishNATEvent("vpc.delete-nat", record.VpcId, record.PublicIp, record.PrivateIp, record.ENIId, macAddr)
 	}
 
-	// Clear association fields, revert to "allocated" state.
-	record.AssociationId = ""
-	record.ENIId = ""
-	record.InstanceId = ""
-	record.PrivateIp = ""
-	record.VpcId = ""
-	record.MacAddress = ""
-	record.State = "allocated"
+	clearAssociation(record)
 
 	data, err := json.Marshal(record)
 	if err != nil {
@@ -321,6 +314,99 @@ func (s *EIPServiceImpl) DisassociateAddress(ctx context.Context, input *ec2.Dis
 	return &ec2.DisassociateAddressOutput{}, nil
 }
 
+// clearAssociation reverts a record to the unassociated "allocated" state. The
+// allocation itself survives: disassociating never hands the address back.
+func clearAssociation(record *EIPRecord) {
+	record.AssociationId = ""
+	record.ENIId = ""
+	record.InstanceId = ""
+	record.PrivateIp = ""
+	record.VpcId = ""
+	record.MacAddress = ""
+	record.State = "allocated"
+}
+
+// DisassociateByENI releases the association an EIP holds on eniID, reporting
+// whether one was found. Instance teardown deletes the ENI but nothing released
+// the EIP behind it, so the record stayed "associated" against a guest that no
+// longer exists and the reconciler kept re-asserting its dnat_and_snat and
+// probing a port that could never bind. No association is success, so a re-run
+// of terminate converges. The allocation is left intact, matching AWS: an EIP
+// survives the instance it was attached to.
+func (s *EIPServiceImpl) DisassociateByENI(ctx context.Context, accountID, eniID string) (bool, error) {
+	if eniID == "" {
+		return false, nil
+	}
+	record, key, revision, err := s.findByENI(ctx, accountID, eniID)
+	if err != nil {
+		return false, err
+	}
+	if record == nil {
+		return false, nil
+	}
+
+	// The MAC comes off the record rather than a fresh ENI lookup: this runs as
+	// part of tearing that ENI down, so the interface may already be gone.
+	s.publishNATEvent("vpc.delete-nat", record.VpcId, record.PublicIp, record.PrivateIp, eniID, record.MacAddress)
+
+	publicIP, allocationID := record.PublicIp, record.AllocationId
+	clearAssociation(record)
+
+	data, err := json.Marshal(record)
+	if err != nil {
+		return false, fmt.Errorf("marshal EIP record: %w", err)
+	}
+	if _, err := s.eipKV.Update(ctx, key, data, revision); err != nil {
+		return false, errors.New(awserrors.ErrorServerInternal)
+	}
+
+	slog.InfoContext(ctx, "DisassociateByENI completed",
+		"eniId", eniID, "publicIp", publicIP, "allocationId", allocationID, "accountID", accountID)
+	return true, nil
+}
+
+// findByENI returns the associated EIP record holding eniID, or a nil record
+// when the account has none. Unlike findByAssociationID an absent match is not
+// an error: teardown asks this of every ENI, and most carry no EIP.
+func (s *EIPServiceImpl) findByENI(ctx context.Context, accountID, eniID string) (*EIPRecord, string, uint64, error) {
+	prefix := accountID + "."
+	keys, err := s.eipKV.Keys(ctx)
+	if err != nil {
+		if errors.Is(err, jetstream.ErrNoKeysFound) {
+			return nil, "", 0, nil
+		}
+		return nil, "", 0, errors.New(awserrors.ErrorServerInternal)
+	}
+
+	for _, k := range keys {
+		if err := ctx.Err(); err != nil {
+			return nil, "", 0, err
+		}
+		if k == utils.VersionKey || !strings.HasPrefix(k, prefix) {
+			continue
+		}
+		entry, err := s.eipKV.Get(ctx, k)
+		if err != nil {
+			if errors.Is(err, jetstream.ErrKeyNotFound) {
+				continue
+			}
+			// An unreadable record must not read as "this ENI has no EIP":
+			// the caller deletes the interface on that answer, and the
+			// association would be stranded with nothing left to find it.
+			return nil, "", 0, fmt.Errorf("eipKV.Get(%s): %w", k, err)
+		}
+		var record EIPRecord
+		if err := json.Unmarshal(entry.Value(), &record); err != nil {
+			slog.WarnContext(ctx, "findByENI: malformed EIP record", "key", k, "err", err)
+			continue
+		}
+		if record.ENIId == eniID {
+			return &record, k, entry.Revision(), nil
+		}
+	}
+	return nil, "", 0, nil
+}
+
 // describeAddressesValidFilters defines the set of filter names accepted by DescribeAddresses.
 var describeAddressesValidFilters = map[string]bool{
 	"allocation-id":  true,

spinifex/handlers/ec2/eip/service_impl_test.go

@@ -200,6 +200,123 @@ func TestEIP_ReleaseByInstanceID_ReclaimsAssociatedEIP(t *testing.T) {
 	assert.Contains(t, err.Error(), "InvalidAllocationID.NotFound")
 }
 
+// associateToENI marks an allocated EIP as associated with eniID the way
+// AssociateAddress would, without needing a real VPC service behind it.
+func associateToENI(t *testing.T, svc *EIPServiceImpl, allocID, eniID string) {
+	t.Helper()
+	key := testAccountID + "." + allocID
+	entry, err := svc.eipKV.Get(t.Context(), key)
+	require.NoError(t, err)
+	var record EIPRecord
+	require.NoError(t, json.Unmarshal(entry.Value(), &record))
+	record.State = "associated"
+	record.AssociationId = "eipassoc-" + eniID
+	record.ENIId = eniID
+	record.InstanceId = "i-owner"
+	record.PrivateIp = "172.31.0.8"
+	record.VpcId = "vpc-test"
+	record.MacAddress = "02:52:f5:7f:9d:0e"
+	data, err := json.Marshal(record)
+	require.NoError(t, err)
+	_, err = svc.eipKV.Update(t.Context(), key, data, entry.Revision())
+	require.NoError(t, err)
+}
+
+// TestEIP_DisassociateByENI is the teardown path the zombie ENIs needed: the
+// association goes with the interface, and the allocation stays behind.
+func TestEIP_DisassociateByENI(t *testing.T) {
+	svc, _ := setupTestEIP(t)
+
+	out, err := svc.AllocateAddress(context.Background(), &ec2.AllocateAddressInput{}, testAccountID)
+	require.NoError(t, err)
+	allocID, publicIP := *out.AllocationId, *out.PublicIp
+	associateToENI(t, svc, allocID, "eni-zombie")
+
+	found, err := svc.DisassociateByENI(t.Context(), testAccountID, "eni-zombie")
+	require.NoError(t, err)
+	assert.True(t, found)
+
+	entry, err := svc.eipKV.Get(t.Context(), testAccountID+"."+allocID)
+	require.NoError(t, err, "the allocation survives: an EIP outlives the instance it was attached to")
+	var record EIPRecord
+	require.NoError(t, json.Unmarshal(entry.Value(), &record))
+	assert.Equal(t, "allocated", record.State)
+	assert.Equal(t, publicIP, record.PublicIp)
+	assert.Empty(t, record.ENIId)
+	assert.Empty(t, record.AssociationId)
+	assert.Empty(t, record.InstanceId)
+	assert.Empty(t, record.PrivateIp)
+	assert.Empty(t, record.VpcId)
+	assert.Empty(t, record.MacAddress)
+
+	// The record no longer names the ENI, so a re-run finds nothing and still
+	// succeeds — terminate and the orphan sweep both retry this.
+	found, err = svc.DisassociateByENI(t.Context(), testAccountID, "eni-zombie")
+	require.NoError(t, err)
+	assert.False(t, found)
+}
+
+// TestEIP_DisassociateByENI_NoMatchIsNoOp pins that an ENI carrying no EIP — the
+// overwhelmingly common case on terminate — leaves other accounts' associations
+// alone and reports no error.
+func TestEIP_DisassociateByENI_NoMatchIsNoOp(t *testing.T) {
+	svc, _ := setupTestEIP(t)
+
+	out, err := svc.AllocateAddress(context.Background(), &ec2.AllocateAddressInput{}, testAccountID)
+	require.NoError(t, err)
+	allocID := *out.AllocationId
+	associateToENI(t, svc, allocID, "eni-other")
+
+	found, err := svc.DisassociateByENI(t.Context(), testAccountID, "eni-unrelated")
+	require.NoError(t, err)
+	assert.False(t, found)
+
+	found, err = svc.DisassociateByENI(t.Context(), testAccountID, "")
+	require.NoError(t, err)
+	assert.False(t, found)
+
+	entry, err := svc.eipKV.Get(t.Context(), testAccountID+"."+allocID)
+	require.NoError(t, err)
+	var record EIPRecord
+	require.NoError(t, json.Unmarshal(entry.Value(), &record))
+	assert.Equal(t, "associated", record.State)
+	assert.Equal(t, "eni-other", record.ENIId)
+}
+
+// TestEIP_DisassociateByENI_ScopedToAccount pins that the scan honours the
+// account prefix: a cluster-wide sweep passes the account the key was found
+// under, and matching across accounts would disassociate a stranger's address.
+func TestEIP_DisassociateByENI_ScopedToAccount(t *testing.T) {
+	svc, _ := setupTestEIP(t)
+
+	out, err := svc.AllocateAddress(context.Background(), &ec2.AllocateAddressInput{}, testAccountID)
+	require.NoError(t, err)
+	associateToENI(t, svc, *out.AllocationId, "eni-zombie")
+
+	found, err := svc.DisassociateByENI(t.Context(), "210987654321", "eni-zombie")
+	require.NoError(t, err)
+	assert.False(t, found)
+}
+
+// TestEIP_DisassociateByENI_SkipsMalformedRecord pins that one undecodable
+// entry cannot hide a real association. Teardown deletes the interface on a
+// "no EIP" answer, and the association would then be stranded with nothing
+// left that could find it.
+func TestEIP_DisassociateByENI_SkipsMalformedRecord(t *testing.T) {
+	svc, _ := setupTestEIP(t)
+
+	_, err := svc.eipKV.Put(t.Context(), testAccountID+".eipalloc-corrupt", []byte("{"))
+	require.NoError(t, err)
+
+	out, err := svc.AllocateAddress(context.Background(), &ec2.AllocateAddressInput{}, testAccountID)
+	require.NoError(t, err)
+	associateToENI(t, svc, *out.AllocationId, "eni-zombie")
+
+	found, err := svc.DisassociateByENI(t.Context(), testAccountID, "eni-zombie")
+	require.NoError(t, err)
+	assert.True(t, found)
+}
+
 func TestEIP_ReleaseByInstanceID_NoMatchIsNoOp(t *testing.T) {
 	svc, _ := setupTestEIP(t)
 

spinifex/handlers/ec2/vpc/eni.go

@@ -795,6 +795,25 @@ type AccountENI struct {
 // the residue of a launch that died in between. Such a record keeps its
 // security group undeletable forever, since the SG dependency check counts it.
 func (s *VPCServiceImpl) ListAbandonedInstanceENIs(ctx context.Context, minAge time.Duration) ([]AccountENI, error) {
+	return s.scanENIs(ctx, func(record *ENIRecord) bool {
+		return eniIsAbandonedLaunch(record, minAge)
+	})
+}
+
+// ListAttachedInstanceENIs returns every ENI older than minAge that still names
+// an instance. The caller decides which of them are stale by asking whether the
+// instance still exists; this only narrows the bucket to the records where that
+// question is worth asking.
+func (s *VPCServiceImpl) ListAttachedInstanceENIs(ctx context.Context, minAge time.Duration) ([]AccountENI, error) {
+	return s.scanENIs(ctx, func(record *ENIRecord) bool {
+		return eniNamesInstance(record, minAge)
+	})
+}
+
+// scanENIs walks every account's ENI records and returns those match accepts.
+// An unreadable or undecodable record is skipped: one bad entry must not stop a
+// cluster-wide sweep.
+func (s *VPCServiceImpl) scanENIs(ctx context.Context, match func(*ENIRecord) bool) ([]AccountENI, error) {
 	keys, err := s.eniKV.Keys(ctx)
 	if err != nil {
 		if errors.Is(err, jetstream.ErrNoKeysFound) {
@@ -820,7 +839,7 @@ func (s *VPCServiceImpl) ListAbandonedInstanceENIs(ctx context.Context, minAge t
 		if err := json.Unmarshal(entry.Value(), &record); err != nil {
 			continue
 		}
-		if !eniIsAbandonedLaunch(&record, minAge) {
+		if !match(&record) {
 			continue
 		}
 		out = append(out, AccountENI{AccountID: accountID, Record: record})
@@ -844,6 +863,20 @@ func eniIsAbandonedLaunch(record *ENIRecord, minAge time.Duration) bool {
 	return time.Since(record.CreatedAt) >= minAge
 }
 
+// eniNamesInstance reports whether a record claims an instance and is old enough
+// that a launch could no longer still be wiring it up. A zero CreatedAt is
+// excluded for the same reason it is in eniIsAbandonedLaunch: age is the only
+// guard against acting on an ENI a launch is still using.
+func eniNamesInstance(record *ENIRecord, minAge time.Duration) bool {
+	if record.InstanceId == "" {
+		return false
+	}
+	if record.CreatedAt.IsZero() {
+		return false
+	}
+	return time.Since(record.CreatedAt) >= minAge
+}
+
 // FindENIByAttachment scans the ENI bucket for the record with the given
 // AttachmentId. Used by DetachNetworkInterface which identifies by attachment ID.
 func (s *VPCServiceImpl) FindENIByAttachment(accountID, attachmentId string) (ENIRecord, error) {

spinifex/handlers/ec2/vpc/eni_abandoned_test.go

@@ -65,6 +65,59 @@ func TestListAbandonedInstanceENIs(t *testing.T) {
 		"only the aged, unattached, launch-created ENI is residue; the others are live or deliberate")
 }
 
+// TestListAttachedInstanceENIs pins the complement of the abandoned listing: the
+// records that do name an instance, which is where a zombie hides. The two
+// listings must not overlap, or a sweep would act on the same record twice.
+func TestListAttachedInstanceENIs(t *testing.T) {
+	svc := setupTestVPCService(t)
+	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
+	subnetID := createTestSubnet(t, svc, vpcID, "10.0.1.0/24")
+
+	attached := createAutoENI(t, svc, subnetID, "i-gone", nil, time.Hour)
+	_, err := svc.AttachENI(testAccountID, attached, "i-gone", 0)
+	require.NoError(t, err)
+
+	// Attached but still inside the age guard: a launch this young may still be
+	// wiring the instance up, so its ENI is not a candidate for anything.
+	fresh := createAutoENI(t, svc, subnetID, "i-launching", nil, time.Minute)
+	_, err = svc.AttachENI(testAccountID, fresh, "i-launching", 0)
+	require.NoError(t, err)
+
+	// Aged but never attached — the abandoned-launch listing owns this one.
+	createAutoENI(t, svc, subnetID, "i-abandoned", nil, time.Hour)
+
+	got, err := svc.ListAttachedInstanceENIs(context.Background(), 15*time.Minute)
+	require.NoError(t, err)
+
+	ids := make([]string, 0, len(got))
+	for _, o := range got {
+		assert.Equal(t, testAccountID, o.AccountID)
+		ids = append(ids, o.Record.NetworkInterfaceId)
+	}
+	assert.Equal(t, []string{attached}, ids,
+		"only the aged ENI that names an instance is a candidate for the staleness check")
+}
+
+// TestListAttachedInstanceENIsIgnoresZeroCreatedAt pins the age guard's fallback.
+// A record written before CreatedAt existed carries no age, and age is the only
+// thing standing between this listing and an ENI a launch is still using.
+func TestListAttachedInstanceENIsIgnoresZeroCreatedAt(t *testing.T) {
+	svc := setupTestVPCService(t)
+	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
+	subnetID := createTestSubnet(t, svc, vpcID, "10.0.1.0/24")
+
+	eniID := createAutoENI(t, svc, subnetID, "i-undated", nil, time.Hour)
+	_, err := svc.AttachENI(testAccountID, eniID, "i-undated", 0)
+	require.NoError(t, err)
+	require.NoError(t, svc.UpdateENI(testAccountID, eniID, func(rec *ENIRecord) {
+		rec.CreatedAt = time.Time{}
+	}))
+
+	got, err := svc.ListAttachedInstanceENIs(context.Background(), 15*time.Minute)
+	require.NoError(t, err)
+	assert.Empty(t, got)
+}
+
 // TestAbandonedENIBlocksSecurityGroupDelete pins the consequence that makes this
 // worth sweeping: the SG dependency check counts an abandoned ENI, so the group
 // stays undeletable for as long as the record survives.