From 3e02fc761305c705e3c94bd284ae84e81bfe651e Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Tue, 1 Sep 2026 09:31:21 +1000 Subject: [PATCH] fix: release the ENI and external address of an abandoned partial launch (#961) --- spinifex/handlers/ec2/instance/service.go | 7 + .../handlers/ec2/instance/service_impl.go | 124 ++++++--- .../ec2/instance/service_impl_test.go | 235 ++++++++++++++++-- 3 files changed, 322 insertions(+), 44 deletions(-) diff --git a/spinifex/handlers/ec2/instance/service.go b/spinifex/handlers/ec2/instance/service.go index b602fb870..52f8ab29e 100644 --- a/spinifex/handlers/ec2/instance/service.go +++ b/spinifex/handlers/ec2/instance/service.go @@ -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. diff --git a/spinifex/handlers/ec2/instance/service_impl.go b/spinifex/handlers/ec2/instance/service_impl.go index 138ff03d4..3b97c635c 100644 --- a/spinifex/handlers/ec2/instance/service_impl.go +++ b/spinifex/handlers/ec2/instance/service_impl.go @@ -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) { diff --git a/spinifex/handlers/ec2/instance/service_impl_test.go b/spinifex/handlers/ec2/instance/service_impl_test.go index 4c2284eca..c44e0a1b5 100644 --- a/spinifex/handlers/ec2/instance/service_impl_test.go +++ b/spinifex/handlers/ec2/instance/service_impl_test.go @@ -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")