fix: release the ENI and external address of an abandoned partial launch (#961)

2026-10-15 09:01:31 +00:00 · 2026-09-01 09:31:21 +10:00
parent ebc95260ac
commit 3e02fc7613
3 changed files with 322 additions and 44 deletions
@@ -127,6 +127,13 @@ type VolumeDeleter interface {
 // ENIDeleter deletes ENIs. Implemented by handlers/ec2/vpc's VPCServiceImpl.
 type ENIDeleter interface {
 	DeleteNetworkInterface(ctx context.Context, input *ec2.DeleteNetworkInterfaceInput, accountID string) (*ec2.DeleteNetworkInterfaceOutput, error)
+
+	// DetachAndDeleteENI releases an ENI under a single KV read. A separate
+	// DetachENI + DeleteNetworkInterface pair lets a lagging replica serve the
+	// delete's read the pre-detach record, which then rejects as in-use and
+	// strands the interface. force skips the in-use guard for an owner
+	// tearing down its own ENI. deleted is false when the ENI was already gone.
+	DetachAndDeleteENI(ctx context.Context, accountID, eniID string, force bool) (deleted bool, err error)
 }

 // PublicIPReleaser releases a previously allocated public IP back to a pool.
@@ -614,6 +614,9 @@ func (s *InstanceServiceImpl) PrepareRunInstances(ctx context.Context, input *ec
 	var instances []*vm.VM
 	var allEC2Instances []*ec2.Instance
 	var lastRunErr error
+	// Whether this call created the instance's ENI, keyed by instance ID. An
+	// abandoned batch deletes the ones it made and only detaches the rest.
+	autoCreatedENI := make(map[string]bool)

 	for i := 0; i < launchCount; i++ {
 		instance, ec2Instance, err := s.RunInstance(input)
@@ -716,18 +719,14 @@ func (s *InstanceServiceImpl) PrepareRunInstances(ctx context.Context, input *ec
 			eni := eniOut.NetworkInterface
 			instance.ENIId = *eni.NetworkInterfaceId
 			instance.ENIMac = *eni.MacAddress
+			autoCreatedENI[instance.ID] = true

 			if _, attachErr := s.eniCreator.AttachENI(ctx, accountID, instance.ENIId, instance.ID, 0); attachErr != nil {
 				// Without the attachment RegisterTargets silently drops the target;
 				// aborting prevents a leak of the auto-assigned EIP.
 				slog.ErrorContext(ctx, "PrepareRunInstances: AttachENI failed — aborting launch",
 					"eniId", instance.ENIId, "instanceId", instance.ID, "err", attachErr)
-				if _, delErr := s.eniDeleter.DeleteNetworkInterface(ctx, &ec2.DeleteNetworkInterfaceInput{
-					NetworkInterfaceId: &instance.ENIId,
-				}, accountID); delErr != nil {
-					slog.WarnContext(ctx, "PrepareRunInstances: failed to delete auto-created ENI after attach failure",
-						"eniId", instance.ENIId, "err", delErr)
-				}
+				s.detachAndDeleteENI(ctx, accountID, instance.ID, instance.ENIId, true)
 				lastRunErr = attachErr
 				if reservationID == "" {
 					s.resourceMgr.Deallocate(instanceType)
@@ -771,20 +770,10 @@ func (s *InstanceServiceImpl) PrepareRunInstances(ctx context.Context, input *ec
 			if wantPublic {
 				publicIP, poolName, allocErr := s.allocatePublicIP(ctx, *eni.NetworkInterfaceId, instance.ID)
 				if allocErr != nil {
-					// Fail rather than boot an unreachable instance;
-					// detach before delete since in-use ENIs reject deletion.
+					// Fail rather than boot an unreachable instance.
 					slog.ErrorContext(ctx, "PrepareRunInstances: public IP allocation failed — aborting launch",
 						"instanceId", instance.ID, "eniId", *eni.NetworkInterfaceId, "err", allocErr)
-					if detErr := s.eniCreator.DetachENI(ctx, accountID, *eni.NetworkInterfaceId); detErr != nil {
-						slog.WarnContext(ctx, "PrepareRunInstances: failed to detach ENI after public-IP allocation failure",
-							"eniId", *eni.NetworkInterfaceId, "err", detErr)
-					}
-					if _, delErr := s.eniDeleter.DeleteNetworkInterface(ctx, &ec2.DeleteNetworkInterfaceInput{
-						NetworkInterfaceId: eni.NetworkInterfaceId,
-					}, accountID); delErr != nil {
-						slog.WarnContext(ctx, "PrepareRunInstances: failed to delete ENI after public-IP allocation failure",
-							"eniId", *eni.NetworkInterfaceId, "err", delErr)
-					}
+					s.detachAndDeleteENI(ctx, accountID, instance.ID, *eni.NetworkInterfaceId, true)
 					// Carry the allocator's own error rather than a flat
 					// capacity code: the gateway resolves the AWS code out
 					// of the wrap chain, so a genuinely exhausted pool still
@@ -835,7 +824,11 @@ func (s *InstanceServiceImpl) PrepareRunInstances(ctx context.Context, input *ec
 	}

 	if len(instances) < minCount {
-		for range instances {
+		// The instances that succeeded are abandoned here too, so their
+		// interfaces and external addresses have to go back with the capacity —
+		// an orphaned ENI pins its subnet, VPC and account undeletable.
+		for _, instance := range instances {
+			s.rollbackPreparedInstance(ctx, accountID, instance, autoCreatedENI[instance.ID])
 			if reservationID == "" {
 				s.resourceMgr.Deallocate(instanceType)
 			} else {
@@ -2572,32 +2565,105 @@ func (s *InstanceServiceImpl) allocatePublicIP(ctx context.Context, eniID, insta
 // rollbackAutoAssignedPublicIP unwinds a failed auto-assign: clears the ENI
 // public IP record, releases the IPAM lease, then detaches and deletes the ENI.
 func (s *InstanceServiceImpl) rollbackAutoAssignedPublicIP(ctx context.Context, accountID, instanceID, eniID, publicIP, poolName string) {
+	_ = s.releaseAutoAssignedPublicIP(ctx, accountID, eniID, publicIP, poolName)
+	_ = s.detachAndDeleteENI(ctx, accountID, instanceID, eniID, true)
+}
+
+// releaseAutoAssignedPublicIP clears the ENI's public IP record and returns the
+// lease to its pool, reporting whether both steps landed. Clearing the record
+// first is what keeps the ENI delete from releasing the same address again.
+func (s *InstanceServiceImpl) releaseAutoAssignedPublicIP(ctx context.Context, accountID, eniID, publicIP, poolName string) bool {
+	released := true
 	if s.eniCreator != nil {
 		if err := s.eniCreator.UpdateENIPublicIP(ctx, accountID, eniID, "", ""); err != nil {
-			slog.WarnContext(ctx, "PrepareRunInstances: failed to clear ENI public IP during NAT-failure rollback",
+			slog.WarnContext(ctx, "PrepareRunInstances: failed to clear ENI public IP during launch rollback",
 				"eniId", eniID, "publicIp", publicIP, "err", err)
+			released = false
 		}
 	}
 	if s.ipReleaser != nil {
 		if err := s.ipReleaser.ReleaseIP(ctx, poolName, publicIP, eniID); err != nil {
-			slog.WarnContext(ctx, "PrepareRunInstances: failed to release public IP during NAT-failure rollback",
+			slog.WarnContext(ctx, "PrepareRunInstances: failed to release public IP during launch rollback",
 				"publicIp", publicIP, "pool", poolName, "err", err)
+			released = false
 		}
 	}
-	if s.eniCreator != nil {
+	return released
+}
+
+// detachAndDeleteENI releases the interface a rolled-back launch is holding and
+// reports whether it is safely gone. An auto-created ENI goes through the
+// atomic detach-and-delete: the launch owns it, so force is right, and the
+// single read is what stops a lagging replica rejecting the delete as in-use
+// and stranding the very orphan this rollback exists to prevent. A
+// caller-supplied ENI outlives the launch it was offered to, so it is detached
+// only, matching DeleteOnTermination=false on the terminate sweep.
+func (s *InstanceServiceImpl) detachAndDeleteENI(ctx context.Context, accountID, instanceID, eniID string, autoCreated bool) bool {
+	if !autoCreated {
+		if s.eniCreator == nil {
+			slog.ErrorContext(ctx, "PrepareRunInstances: no ENI service wired — caller-supplied ENI left attached to an instance that never launched",
+				"eniId", eniID, "instanceId", instanceID)
+			return false
+		}
 		if err := s.eniCreator.DetachENI(ctx, accountID, eniID); err != nil {
-			slog.WarnContext(ctx, "PrepareRunInstances: failed to detach ENI during NAT-failure rollback",
+			slog.ErrorContext(ctx, "PrepareRunInstances: failed to detach caller-supplied ENI during launch rollback",
 				"eniId", eniID, "instanceId", instanceID, "err", err)
+			return false
 		}
+		return true
 	}
-	if s.eniDeleter != nil {
-		if _, err := s.eniDeleter.DeleteNetworkInterface(ctx, &ec2.DeleteNetworkInterfaceInput{
-			NetworkInterfaceId: &eniID,
-		}, accountID); err != nil {
-			slog.WarnContext(ctx, "PrepareRunInstances: failed to delete ENI during NAT-failure rollback",
-				"eniId", eniID, "instanceId", instanceID, "err", err)
+
+	if s.eniDeleter == nil {
+		slog.ErrorContext(ctx, "PrepareRunInstances: no ENI deleter wired — auto-created ENI orphaned by launch rollback",
+			"eniId", eniID, "instanceId", instanceID)
+		return false
+	}
+	deleted, err := s.eniDeleter.DetachAndDeleteENI(ctx, accountID, eniID, true)
+	if err != nil {
+		// Not a warning: the interface survives, and it will refuse its
+		// subnet's delete and then its VPC's until something reaps it.
+		slog.ErrorContext(ctx, "PrepareRunInstances: failed to release auto-created ENI during launch rollback — interface orphaned",
+			"eniId", eniID, "instanceId", instanceID, "err", err)
+		return false
+	}
+	slog.InfoContext(ctx, "PrepareRunInstances: released auto-created ENI during launch rollback",
+		"eniId", eniID, "instanceId", instanceID, "deleted", deleted)
+	return true
+}
+
+// rollbackPreparedInstance unwinds the network resources a prepared instance
+// holds when the batch is abandoned, in the reverse order they were taken: NAT
+// rule, external address, then the interface itself. The VM needs no teardown —
+// it is metadata the caller never received.
+func (s *InstanceServiceImpl) rollbackPreparedInstance(ctx context.Context, accountID string, instance *vm.VM, autoCreatedENI bool) {
+	if instance == nil || instance.ENIId == "" {
+		return
+	}
+	rolledBack := true
+	if instance.PublicIP != "" {
+		vpcID := ""
+		privateIP := ""
+		if instance.Instance != nil {
+			vpcID = aws.StringValue(instance.Instance.VpcId)
+			privateIP = aws.StringValue(instance.Instance.PrivateIpAddress)
 		}
+		utils.PublishNATEvent(s.natsConn, "vpc.delete-nat", vpcID, instance.PublicIP, privateIP,
+			topology.Port(instance.ENIId), instance.ENIMac)
+		rolledBack = s.releaseAutoAssignedPublicIP(ctx, accountID, instance.ENIId, instance.PublicIP, instance.PublicIPPool)
 	}
+	// Evaluated before the &&: the interface must be released whether or not
+	// the address came back.
+	rolledBack = s.detachAndDeleteENI(ctx, accountID, instance.ID, instance.ENIId, autoCreatedENI) && rolledBack
+
+	if !rolledBack {
+		slog.ErrorContext(ctx, "PrepareRunInstances: prepared-instance rollback incomplete — ENI or address may be orphaned",
+			"instanceId", instance.ID, "eniId", instance.ENIId,
+			"publicIp", instance.PublicIP, "eniAutoCreated", autoCreatedENI)
+		return
+	}
+	slog.InfoContext(ctx, "PrepareRunInstances: rolled back prepared instance",
+		"instanceId", instance.ID, "eniId", instance.ENIId,
+		"publicIp", instance.PublicIP, "eniAutoCreated", autoCreatedENI)
 }

 func (s *InstanceServiceImpl) releaseInstancePublicIP(ctx context.Context, instance *vm.VM, instanceID string) {
@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"testing"
 	"time"

@@ -1574,8 +1575,9 @@ func (f *fakeVolumeDeleter) DetachVolumeOnTerminate(_ context.Context, volumeID,
 }

 type fakeENIDeleter struct {
-	calls []string
-	err   error
+	calls  []string
+	forced []bool // force flag per DetachAndDeleteENI call
+	err    error
 }

 func (f *fakeENIDeleter) DeleteNetworkInterface(_ context.Context, input *ec2.DeleteNetworkInterfaceInput, _ string) (*ec2.DeleteNetworkInterfaceOutput, error) {
@@ -1586,10 +1588,24 @@ func (f *fakeENIDeleter) DeleteNetworkInterface(_ context.Context, input *ec2.De
 	return &ec2.DeleteNetworkInterfaceOutput{}, nil
 }

+// DetachAndDeleteENI records into the same calls slice as the plain delete:
+// callers assert which ENIs were released, not which entry point did it.
+// forced records the force flag per call so the owner-teardown contract is
+// assertable.
+func (f *fakeENIDeleter) DetachAndDeleteENI(_ context.Context, _, eniID string, force bool) (bool, error) {
+	f.calls = append(f.calls, eniID)
+	f.forced = append(f.forced, force)
+	if f.err != nil {
+		return false, f.err
+	}
+	return true, nil
+}
+
 type fakePublicIPReleaser struct {
 	pool     string
 	ip       string
 	ownerENI string
+	released []string // every address released, in call order
 	err      error
 }

@@ -1597,6 +1613,7 @@ func (f *fakePublicIPReleaser) ReleaseIP(_ context.Context, pool, ip, ownerENIID
 	f.pool = pool
 	f.ip = ip
 	f.ownerENI = ownerENIID
+	f.released = append(f.released, ip)
 	return f.err
 }

@@ -2984,14 +3001,17 @@ type fakeENICreator struct {
 	getENIByID      map[string]*ENIInfo
 	getENIErr       error
 	createOut       *ec2.CreateNetworkInterfaceOutput
+	createOuts      []*ec2.CreateNetworkInterfaceOutput // per-call outputs; overrides createOut when set
 	createCalls     int
 	createErr       error
 	createErrOnCall int // 1-based call index to fail; 0 fails every call when createErr is set
 	attachErr       error
+	attachErrOnCall int // 1-based call index to fail; 0 fails every call when attachErr is set
 	attachCalls     int
 	updateCalls     int
 	clearCalls      int // updateCalls where publicIP is ""
 	detachCalls     int
+	detached        []string
 	detachErr       error                // returned by every DetachENI call when set
 	instanceENIs    map[string][]ENIInfo // keyed by instanceID, for ListInstanceENIs
 	listENIsErr     error
@@ -3030,19 +3050,23 @@ func (f *fakeENICreator) CreateNetworkInterface(_ context.Context, _ *ec2.Create
 	if f.createErr != nil && (f.createErrOnCall == 0 || f.createErrOnCall == f.createCalls) {
 		return nil, f.createErr
 	}
+	if len(f.createOuts) > 0 {
+		return f.createOuts[min(f.createCalls, len(f.createOuts))-1], nil
+	}
 	return f.createOut, nil
 }

 func (f *fakeENICreator) AttachENI(_ context.Context, _, _, _ string, _ int64) (string, error) {
 	f.attachCalls++
-	if f.attachErr != nil {
+	if f.attachErr != nil && (f.attachErrOnCall == 0 || f.attachErrOnCall == f.attachCalls) {
 		return "", f.attachErr
 	}
 	return "attached", nil
 }

-func (f *fakeENICreator) DetachENI(_ context.Context, _, _ string) error {
+func (f *fakeENICreator) DetachENI(_ context.Context, _, eniID string) error {
 	f.detachCalls++
+	f.detached = append(f.detached, eniID)
 	return f.detachErr
 }

@@ -3062,15 +3086,21 @@ func (f *fakeENICreator) ListInstanceENIs(_ context.Context, _, instanceID strin
 }

 type fakeIPAllocator struct {
-	publicIP string
-	poolName string
-	err      error
+	publicIP  string
+	publicIPs []string // per-call addresses; overrides publicIP when set
+	calls     int
+	poolName  string
+	err       error
 }

 func (f *fakeIPAllocator) AllocateIP(_ context.Context, _, _, _, _, _, _ string) (string, string, error) {
+	f.calls++
 	if f.err != nil {
 		return "", "", f.err
 	}
+	if len(f.publicIPs) > 0 {
+		return f.publicIPs[min(f.calls, len(f.publicIPs))-1], f.poolName, nil
+	}
 	return f.publicIP, f.poolName, nil
 }

@@ -3229,9 +3259,10 @@ func TestPrepareRunInstances_PublicIPWithoutAllocator(t *testing.T) {
 			require.Error(t, err, "a public-IP launch with no allocator must fail, not boot an instance with no public address")
 			assert.Equal(t, awserrors.ErrorInsufficientAddressCapacity, err.Error())
 			assert.Empty(t, instances)
-			assert.Equal(t, 1, eni.detachCalls, "ENI must be detached before delete")
 			assert.Equal(t, []string{"eni-no-ipam"}, deleter.calls,
 				"the auto-created ENI must be deleted, otherwise it strands and blocks security group deletion")
+			assert.Equal(t, []bool{true}, deleter.forced, "the launch owns the ENI, so its teardown must force past the in-use guard")
+			assert.Zero(t, eni.detachCalls, "the detach belongs inside the atomic delete, not as a separate read")
 			assert.Len(t, prov.deallocated, 1, "capacity must be returned when the launch aborts")
 		})
 	}
@@ -3307,11 +3338,14 @@ func TestPrepareRunInstances_NATFailureRollsBackPublicIP(t *testing.T) {
 	assert.Equal(t, 2, eni.updateCalls, "ENI public IP should be set once, then cleared on rollback")
 	assert.Equal(t, 1, eni.clearCalls, "ENI rollback should call UpdateENIPublicIP with empty publicIP")

-	// ENI itself must be detached + deleted; otherwise the dropped instance
-	// leaks an eniKV record (vmMgr.Insert never runs, so terminateCleanup
-	// never fires) and the subnet exhausts private IPs under vpcd brown-out.
-	assert.Equal(t, 1, eni.detachCalls, "rollback must detach the ENI before delete")
+	// ENI itself must go; otherwise the dropped instance leaks an eniKV record
+	// (vmMgr.Insert never runs, so terminateCleanup never fires) and the subnet
+	// exhausts private IPs under vpcd brown-out. Detach and delete travel
+	// together under one read — a separate detach lets a lagging replica serve
+	// the delete a pre-detach record and reject it as in-use.
 	assert.Equal(t, []string{"eni-nat-fail"}, deleter.calls, "rollback must delete the auto-created ENI")
+	assert.Equal(t, []bool{true}, deleter.forced, "the launch owns the ENI, so its teardown must force past the in-use guard")
+	assert.Zero(t, eni.detachCalls, "the detach belongs inside the atomic delete, not as a separate read")

 	// Capacity deallocated so subsequent launches can use the slot.
 	require.Len(t, prov.deallocated, 1, "NAT failure must trigger Deallocate")
@@ -3331,6 +3365,174 @@ func TestPrepareRunInstances_NATFailureRollsBackPublicIP(t *testing.T) {
 	}
 }

+// TestPrepareRunInstances_PartialLaunchReleasesSucceededInstances covers the
+// batch that falls short of MinCount: the instances that *succeeded* are
+// abandoned too, so their ENIs, NAT rules and external addresses have to go
+// back. Leaving them is what wedged an account in TERMINATING, since an
+// orphaned ENI pins its subnet and VPC undeletable.
+func TestPrepareRunInstances_PartialLaunchReleasesSucceededInstances(t *testing.T) {
+	eni := &fakeENICreator{
+		subnet: &SubnetInfo{SubnetID: "subnet-1", VpcID: "vpc-1", MapPublicIpOnLaunch: true},
+		createOuts: []*ec2.CreateNetworkInterfaceOutput{
+			{NetworkInterface: &ec2.NetworkInterface{
+				NetworkInterfaceId: aws.String("eni-ok"),
+				MacAddress:         aws.String("aa:bb:cc:dd:ee:01"),
+				PrivateIpAddress:   aws.String("10.0.0.51"),
+				VpcId:              aws.String("vpc-1"),
+			}},
+			{NetworkInterface: &ec2.NetworkInterface{
+				NetworkInterfaceId: aws.String("eni-fail"),
+				MacAddress:         aws.String("aa:bb:cc:dd:ee:02"),
+				PrivateIpAddress:   aws.String("10.0.0.52"),
+				VpcId:              aws.String("vpc-1"),
+			}},
+		},
+	}
+	ipam := &fakeIPAllocator{publicIPs: []string{"203.0.113.20", "203.0.113.21"}, poolName: "wan"}
+	releaser := &fakePublicIPReleaser{}
+	deleter := &fakeENIDeleter{}
+	svc, prov := prepareSvcWithENI(t, eni, ipam)
+	svc.ipReleaser = releaser
+	svc.eniDeleter = deleter
+
+	// vpcd acks the first instance's rule and times out on the second, which
+	// is the production shape: one instance fully built, the batch short.
+	var addNATCalls atomic.Int32
+	sub, err := svc.natsConn.Subscribe("vpc.add-nat", func(msg *nats.Msg) {
+		if addNATCalls.Add(1) == 1 {
+			_ = msg.Respond([]byte(`{"success":true}`))
+			return
+		}
+		_ = msg.Respond([]byte(`{"success":false,"error":"northd unavailable"}`))
+	})
+	require.NoError(t, err)
+	defer func() { _ = sub.Unsubscribe() }()
+
+	deleteNATCh := make(chan natWirePayload, 2)
+	delSub, err := svc.natsConn.Subscribe("vpc.delete-nat", func(msg *nats.Msg) {
+		var p natWirePayload
+		_ = json.Unmarshal(msg.Data, &p)
+		select {
+		case deleteNATCh <- p:
+		default:
+		}
+	})
+	require.NoError(t, err)
+	defer func() { _ = delSub.Unsubscribe() }()
+
+	_, instances, _, err := svc.PrepareRunInstances(context.Background(), &ec2.RunInstancesInput{
+		InstanceType: aws.String("t3.micro"),
+		ImageId:      aws.String("ami-1"),
+		SubnetId:     aws.String("subnet-1"),
+		MinCount:     aws.Int64(2),
+		MaxCount:     aws.Int64(2),
+	}, "acc", "")
+
+	require.Error(t, err)
+	assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
+	assert.Empty(t, instances)
+
+	// Both interfaces gone: eni-fail by the in-loop NAT rollback, eni-ok by
+	// the MinCount rollback that used to release nothing but capacity.
+	assert.ElementsMatch(t, []string{"eni-ok", "eni-fail"}, deleter.calls,
+		"the succeeded instance's ENI must be deleted alongside the failed one")
+	assert.Equal(t, []bool{true, true}, deleter.forced,
+		"each teardown must force past the in-use guard — the launch owns both interfaces")
+	assert.Zero(t, eni.detachCalls,
+		"detach must travel with the delete under one read, not as a separate call a replica can race")
+	assert.ElementsMatch(t, []string{"203.0.113.20", "203.0.113.21"}, releaser.released,
+		"both external addresses must return to the pool")
+	assert.Len(t, prov.deallocated, 2, "both capacity slots must be returned")
+
+	// The succeeded instance's NAT rule was committed, so it must be
+	// withdrawn — otherwise it routes traffic for the next tenant that
+	// takes the released address.
+	var deletedNATIPs []string
+	for range 2 {
+		select {
+		case got := <-deleteNATCh:
+			deletedNATIPs = append(deletedNATIPs, got.ExternalIP)
+		case <-time.After(time.Second):
+			t.Fatal("rollback must publish vpc.delete-nat for every address it releases")
+		}
+	}
+	assert.ElementsMatch(t, []string{"203.0.113.20", "203.0.113.21"}, deletedNATIPs)
+}
+
+// TestPrepareRunInstances_PartialLaunchKeepsCallerSuppliedENI checks the EKS
+// launcher's path: an ENI created outside the launch is detached when the batch
+// is abandoned, never deleted, matching DeleteOnTermination=false.
+func TestPrepareRunInstances_PartialLaunchKeepsCallerSuppliedENI(t *testing.T) {
+	eni := &fakeENICreator{
+		subnet: &SubnetInfo{SubnetID: "subnet-1", VpcID: "vpc-1"},
+		getENIByID: map[string]*ENIInfo{
+			"eni-customer": {
+				NetworkInterfaceID: "eni-customer",
+				SubnetID:           "subnet-1",
+				VpcID:              "vpc-1",
+				PrivateIpAddress:   "10.0.0.60",
+				MacAddress:         "aa:bb:cc:dd:ee:03",
+				Status:             "available",
+			},
+		},
+		// The second attach fails, dropping the batch below MinCount.
+		attachErr:       errors.New("attach failed"),
+		attachErrOnCall: 2,
+	}
+	deleter := &fakeENIDeleter{}
+	svc, prov := prepareSvcWithENI(t, eni, nil)
+	svc.eniDeleter = deleter
+
+	_, instances, _, err := svc.PrepareRunInstances(context.Background(), &ec2.RunInstancesInput{
+		InstanceType: aws.String("t3.micro"),
+		ImageId:      aws.String("ami-1"),
+		MinCount:     aws.Int64(2),
+		MaxCount:     aws.Int64(2),
+		NetworkInterfaces: []*ec2.InstanceNetworkInterfaceSpecification{
+			{NetworkInterfaceId: aws.String("eni-customer")},
+		},
+	}, "acc", "")
+
+	require.Error(t, err)
+	assert.Empty(t, instances)
+	assert.Equal(t, []string{"eni-customer"}, eni.detached, "an abandoned launch must release its attachment")
+	assert.Empty(t, deleter.calls, "an ENI the launch did not create must survive the rollback")
+	assert.Len(t, prov.deallocated, 2, "both capacity slots must be returned")
+}
+
+// TestPrepareRunInstances_SuccessRollsBackNothing guards the other direction:
+// a batch that meets MinCount must not touch the interfaces it just built.
+func TestPrepareRunInstances_SuccessRollsBackNothing(t *testing.T) {
+	eni := &fakeENICreator{
+		subnet: &SubnetInfo{SubnetID: "subnet-1", VpcID: "vpc-1"},
+		createOut: &ec2.CreateNetworkInterfaceOutput{
+			NetworkInterface: &ec2.NetworkInterface{
+				NetworkInterfaceId: aws.String("eni-live"),
+				MacAddress:         aws.String("aa:bb:cc:dd:ee:04"),
+				PrivateIpAddress:   aws.String("10.0.0.70"),
+				VpcId:              aws.String("vpc-1"),
+			},
+		},
+	}
+	deleter := &fakeENIDeleter{}
+	svc, prov := prepareSvcWithENI(t, eni, nil)
+	svc.eniDeleter = deleter
+
+	_, instances, _, err := svc.PrepareRunInstances(context.Background(), &ec2.RunInstancesInput{
+		InstanceType: aws.String("t3.micro"),
+		ImageId:      aws.String("ami-1"),
+		SubnetId:     aws.String("subnet-1"),
+		MinCount:     aws.Int64(1),
+		MaxCount:     aws.Int64(2),
+	}, "acc", "")
+
+	require.NoError(t, err)
+	require.Len(t, instances, 2)
+	assert.Empty(t, eni.detached, "a launch that met MinCount must not detach anything")
+	assert.Empty(t, deleter.calls, "a launch that met MinCount must not delete anything")
+	assert.Empty(t, prov.deallocated, "a launch that met MinCount must keep its capacity")
+}
+
 // TestPrepareRunInstances_PublicIPAllocFailureAbortsLaunch verifies that a
 // failed public-IP allocation aborts the launch — detaches and deletes the ENI,
 // deallocates capacity — and reports the allocator's real cause. Reporting every
@@ -3386,10 +3588,13 @@ func TestPrepareRunInstances_PublicIPAllocFailureAbortsLaunch(t *testing.T) {
 				"the code must be resolved from the allocator's cause, not asserted by the caller")
 			assert.Empty(t, instances, "instance with no public IP must not be returned")

-			// The ENI is attached with a primary IP; rollback must detach before
-			// delete (in-use ENIs reject deletion) so no eniKV record leaks.
-			assert.Equal(t, 1, eni.detachCalls, "rollback must detach the ENI before delete")
+			// The ENI is attached with a primary IP, so the rollback must clear
+			// the attachment and the record together under one read — in-use
+			// ENIs reject deletion, and a separate detach lets a lagging
+			// replica serve the delete a pre-detach record.
 			assert.Equal(t, []string{"eni-pubip-fail"}, deleter.calls, "rollback must delete the auto-created ENI")
+			assert.Equal(t, []bool{true}, deleter.forced, "the launch owns the ENI, so its teardown must force past the in-use guard")
+			assert.Zero(t, eni.detachCalls, "the detach belongs inside the atomic delete, not as a separate read")
 			assert.Equal(t, 0, eni.updateCalls, "no public IP was allocated, so the ENI must never be updated with one")

 			require.Len(t, prov.deallocated, 1, "public-IP allocation failure must trigger Deallocate")