fix: build the EC2 DNS desired set from the instance records so stale records prune (#960)

* fix: build the EC2 DNS desired set from the instance records so stale records prune

* fix: keep DNS records for stopped instances in the reconcile desired set

The desired set filtered on Running, but the lifecycle publish deliberately
does not withdraw a stopped instance's records: stop and start retain the
address, so they retain the name. With EC2 pruning enabled, absence from the
desired set became a deletion, so a stopped instance would have lost both its
names until the next publish.

Both sides now read one predicate, InstanceRetainsRecords, which replaces
needsDNSWithdrawal and lives next to the names it governs. Records carrying a
DeletionTimestamp are skipped as well, so a re-upsert cannot race the
withdrawal the terminate path has already published.

spinifex/daemon/dns_reconcile.go

@@ -2,6 +2,7 @@ package daemon
 
 import (
 	"context"
+	"log/slog"
 
 	"github.com/aws/aws-sdk-go/aws"
 	handlers_dns "github.com/mulgadc/spinifex/spinifex/handlers/dns"
@@ -10,7 +11,6 @@ import (
 	handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds"
 	"github.com/mulgadc/spinifex/spinifex/kvstore"
 	"github.com/mulgadc/spinifex/spinifex/reconciler"
-	"github.com/mulgadc/spinifex/spinifex/vm"
 )
 
 // dnsWatchSources names the buckets whose changes should wake the DNS
@@ -81,7 +81,10 @@ func (d *Daemon) rdsWatchBuckets(ctx context.Context) ([]*kvstore.Bucket, error)
 // records — the reconcile only ever repairs, never over-prunes, on a partial view.
 func (d *Daemon) dnsDesiredSet() handlers_dns.DesiredSet {
 	ds := handlers_dns.DesiredSet{}
-	ds.Changes = append(ds.Changes, d.desiredEC2DNSChanges()...)
+	if ch, ok := d.desiredEC2DNSChanges(); ok {
+		ds.Changes = append(ds.Changes, ch...)
+		ds.Prunable.EC2 = true
+	}
 
 	if d.elbv2Service != nil {
 		if ch, ok := d.elbv2Service.DesiredDNSChanges(); ok {
@@ -104,29 +107,46 @@ func (d *Daemon) dnsDesiredSet() handlers_dns.DesiredSet {
 	return ds
 }
 
-// desiredEC2DNSChanges returns UPSERTs for this node's running instances. EC2
-// records are node-local — vmMgr holds only this node's VMs — so they are never
-// pruned by the reconcile; the terminate hook owns EC2 record removal. The
-// domains mirror the lifecycle publish so re-asserting is a no-op when in sync.
-func (d *Daemon) desiredEC2DNSChanges() []handlers_dns.Change {
+// desiredEC2DNSChanges returns UPSERTs for every running instance in the
+// cluster, and whether that view was complete. The instance record key space
+// spans all nodes, unlike the vmMgr map it replaces here, which is what lets
+// the reconcile prune a record as well as repair one. The domains mirror the
+// lifecycle publish so re-asserting is a no-op when in sync.
+func (d *Daemon) desiredEC2DNSChanges() ([]handlers_dns.Change, bool) {
+	if d.jsManager == nil {
+		return nil, false
+	}
+	records, err := d.jsManager.ListInstanceRecords()
+	if err != nil {
+		// Reported without changes: a partial instance view is exactly the case
+		// pruning must not run on, and repairing from it would be repairing from
+		// the same partial view.
+		slog.Warn("dns reconcile: instance records unavailable, EC2 records left alone this cycle",
+			"error", err)
+		return nil, false
+	}
+
 	var changes []handlers_dns.Change
-	d.vmMgr.View(func(vms map[string]*vm.VM) {
-		for _, v := range vms {
-			if v == nil || v.Status != vm.StateRunning {
-				continue
-			}
-			privateIP := ""
-			if v.Instance != nil {
-				privateIP = aws.StringValue(v.Instance.PrivateIpAddress)
-			}
-			if v.PublicIP == "" && privateIP == "" {
-				continue
-			}
-			changes = append(changes, handlers_dns.EC2Changes(
-				handlers_dns.ActionUpsert, d.config.Region,
-				d.dnsBaseDomain, d.dnsInternalDomain, v.PublicIP, privateIP,
-			)...)
+	for _, record := range records {
+		// Stopped instances keep their addresses and their names, which is why
+		// the state test is the same one the lifecycle publish withdraws on.
+		if record == nil || !handlers_dns.InstanceRetainsRecords(record.Status.Status) {
+			continue
 		}
-	})
-	return changes
+		if record.Metadata.DeletionTimestamp != nil {
+			continue
+		}
+		privateIP := ""
+		if record.Status.Instance != nil {
+			privateIP = aws.StringValue(record.Status.Instance.PrivateIpAddress)
+		}
+		if record.Status.PublicIP == "" && privateIP == "" {
+			continue
+		}
+		changes = append(changes, handlers_dns.EC2Changes(
+			handlers_dns.ActionUpsert, d.config.Region,
+			d.dnsBaseDomain, d.dnsInternalDomain, record.Status.PublicIP, privateIP,
+		)...)
+	}
+	return changes, true
 }

spinifex/daemon/dns_reconcile_ec2_test.go

@@ -0,0 +1,170 @@
+//test:in-package — these build *Daemon and *JetStreamManager literals to drive
+//the desired-set builder against a seeded record store, which is only possible
+//from inside the package.
+
+package daemon
+
+import (
+	"testing"
+	"time"
+
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/mulgadc/spinifex/spinifex/config"
+	handlers_dns "github.com/mulgadc/spinifex/spinifex/handlers/dns"
+	"github.com/mulgadc/spinifex/spinifex/kvstore"
+	"github.com/mulgadc/spinifex/spinifex/testutil"
+	"github.com/mulgadc/spinifex/spinifex/vm"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// dnsTestDaemon returns a daemon whose instance records live in a real KV
+// bucket, plus a writer for seeding it. vmMgr is empty on purpose: the desired
+// set must not read it.
+func dnsTestDaemon(t *testing.T) (*Daemon, func(id, publicIP, privateIP string, state vm.InstanceState)) {
+	t.Helper()
+	_, _, js := testutil.StartTestJetStream(t)
+	store := kvstore.New[vm.InstanceRecord](js, kvstore.Config{
+		Name: InstanceStateBucket, History: 1, Replicas: 1,
+	})
+	d := &Daemon{
+		jsManager:         &JetStreamManager{js: js, records: store, replicas: 1},
+		vmMgr:             vm.NewManager(),
+		config:            &config.Config{Region: "ap-southeast-2"},
+		dnsBaseDomain:     "spx3.net",
+		dnsInternalDomain: "compute.internal",
+	}
+	seed := func(id, publicIP, privateIP string, state vm.InstanceState) {
+		t.Helper()
+		record := &vm.InstanceRecord{}
+		record.Metadata.Name = id
+		record.Status.Status = state
+		record.Status.PublicIP = publicIP
+		if privateIP != "" {
+			record.Status.Instance = &ec2.Instance{PrivateIpAddress: aws.String(privateIP)}
+		}
+		require.NoError(t, d.jsManager.WriteInstanceRecord(id, record))
+	}
+	return d, seed
+}
+
+func changeNames(changes []handlers_dns.Change) []string {
+	out := make([]string, 0, len(changes))
+	for _, c := range changes {
+		out = append(out, c.Name)
+	}
+	return out
+}
+
+// The desired set has to span every node, because that is the whole basis on
+// which the reconcile is allowed to delete an instance record. A record written
+// by another node is indistinguishable here from one written locally, which is
+// the point.
+func TestDesiredEC2DNSChanges_SpansEveryNode(t *testing.T) {
+	d, seed := dnsTestDaemon(t)
+	seed("i-local", "203.0.113.10", "172.31.0.10", vm.StateRunning)
+	seed("i-remote", "203.0.113.20", "172.31.0.20", vm.StateRunning)
+
+	changes, ok := d.desiredEC2DNSChanges()
+	require.True(t, ok, "a successful list is a cluster-wide view and carries prune authority")
+	assert.ElementsMatch(t, []string{
+		"ec2-203-0-113-10.ap-southeast-2.compute.spx3.net",
+		"ip-172-31-0-10.ap-southeast-2.compute.internal",
+		"ec2-203-0-113-20.ap-southeast-2.compute.spx3.net",
+		"ip-172-31-0-20.ap-southeast-2.compute.internal",
+	}, changeNames(changes))
+}
+
+// The mutation check for the change itself: a VM present only in this node's
+// in-memory manager must contribute nothing. Reading vmMgr is what made the
+// view node-local and pruning unsafe, so a fallback to it would quietly restore
+// the old behaviour while the prune authority claimed otherwise.
+func TestDesiredEC2DNSChanges_IgnoresTheLocalVMManager(t *testing.T) {
+	d, _ := dnsTestDaemon(t)
+	d.vmMgr.Replace(map[string]*vm.VM{
+		"i-inmemory": {
+			ID:       "i-inmemory",
+			Status:   vm.StateRunning,
+			PublicIP: "203.0.113.99",
+			Instance: &ec2.Instance{PrivateIpAddress: aws.String("172.31.0.99")},
+		},
+	})
+
+	changes, ok := d.desiredEC2DNSChanges()
+	require.True(t, ok)
+	assert.Empty(t, changes, "an instance with no record is not part of the cluster-wide view")
+}
+
+// A stopped instance keeps its addresses, so it keeps its names: the lifecycle
+// publish withdraws records on terminate and deliberately not on stop. Now that
+// absence from this set is a deletion, the two have to agree — a stopped
+// instance missing here would have its name pruned out from under it.
+func TestDesiredEC2DNSChanges_KeepsStoppedInstancesAndDropsTerminated(t *testing.T) {
+	d, seed := dnsTestDaemon(t)
+	seed("i-up", "203.0.113.10", "172.31.0.10", vm.StateRunning)
+	seed("i-stopping", "203.0.113.11", "172.31.0.11", vm.StateStopping)
+	seed("i-stopped", "203.0.113.12", "172.31.0.12", vm.StateStopped)
+	seed("i-gone", "203.0.113.13", "172.31.0.13", vm.StateTerminated)
+
+	changes, ok := d.desiredEC2DNSChanges()
+	require.True(t, ok)
+	names := changeNames(changes)
+	assert.ElementsMatch(t, []string{
+		"ec2-203-0-113-10.ap-southeast-2.compute.spx3.net",
+		"ip-172-31-0-10.ap-southeast-2.compute.internal",
+		"ec2-203-0-113-11.ap-southeast-2.compute.spx3.net",
+		"ip-172-31-0-11.ap-southeast-2.compute.internal",
+		"ec2-203-0-113-12.ap-southeast-2.compute.spx3.net",
+		"ip-172-31-0-12.ap-southeast-2.compute.internal",
+	}, names)
+	assert.NotContains(t, names, "ec2-203-0-113-13.ap-southeast-2.compute.spx3.net")
+}
+
+// An instance on its way out is not desired state, even while its record is
+// still readable: re-upserting it would race the withdrawal the terminate path
+// already published.
+func TestDesiredEC2DNSChanges_SkipsARecordMarkedForDeletion(t *testing.T) {
+	d, seed := dnsTestDaemon(t)
+	seed("i-doomed", "203.0.113.10", "172.31.0.10", vm.StateRunning)
+	_, err := d.jsManager.UpdateInstanceRecord("i-doomed", func(r *vm.InstanceRecord) {
+		now := time.Now()
+		r.Metadata.DeletionTimestamp = &now
+	})
+	require.NoError(t, err)
+
+	changes, ok := d.desiredEC2DNSChanges()
+	require.True(t, ok)
+	assert.Empty(t, changes)
+}
+
+// A view that could not be read is the case pruning exists to be protected
+// from. It must report no authority, and contribute no records either: repairing
+// from a partial view is repairing from the same partial view.
+func TestDesiredEC2DNSChanges_UnreadableStoreYieldsNoAuthority(t *testing.T) {
+	for name, d := range map[string]*Daemon{
+		"no jsManager":    {},
+		"no record store": {jsManager: &JetStreamManager{}},
+	} {
+		t.Run(name, func(t *testing.T) {
+			changes, ok := d.desiredEC2DNSChanges()
+			assert.False(t, ok, "an unread instance view must not grant prune authority")
+			assert.Empty(t, changes)
+		})
+	}
+}
+
+// The flag and the records travel together: a desired set carrying EC2 changes
+// without EC2 authority would repair but never prune, and one carrying the flag
+// without the changes would prune everything.
+func TestDNSDesiredSet_EC2AuthorityFollowsTheRecords(t *testing.T) {
+	d, seed := dnsTestDaemon(t)
+	seed("i-up", "203.0.113.10", "172.31.0.10", vm.StateRunning)
+
+	ds := d.dnsDesiredSet()
+	assert.True(t, ds.Prunable.EC2)
+	assert.NotEmpty(t, ds.Changes)
+
+	assert.False(t, (&Daemon{}).dnsDesiredSet().Prunable.EC2,
+		"a daemon that could not read any instance record claims no authority")
+}

spinifex/handlers/dns/naming.go

@@ -3,24 +3,42 @@ package dns
 import (
 	"fmt"
 	"strings"
+
+	"github.com/mulgadc/spinifex/spinifex/vm"
 )
 
+// InstanceRetainsRecords reports whether an instance in this state still owns
+// its A records. Stop and start retain the addresses, so they retain the names;
+// anything past stopped no longer resolves and its records are withdrawn.
+func InstanceRetainsRecords(status vm.InstanceState) bool {
+	return status == vm.StateRunning || status == vm.StateStopping || status == vm.StateStopped
+}
+
 // dashIP renders an IP for AWS-style hostnames (1.2.3.4 → 1-2-3-4).
 func dashIP(ip string) string {
 	return strings.ReplaceAll(ip, ".", "-")
 }
 
+const (
+	// ec2PublicLabelPrefix and ec2PrivateLabelPrefix open every instance record
+	// name. The reconcile matches on them to recognise a record as an instance
+	// record, so they are defined alongside the names they build.
+	ec2PublicLabelPrefix  = "ec2-"
+	ec2PrivateLabelPrefix = "ip-"
+)
+
 // EC2PublicName is the public AWS-shaped name for an instance:
 // ec2-{dashed-public-ip}.{region}.compute.{baseDomain}.
 func EC2PublicName(publicIP, region, baseDomain string) string {
-	return fmt.Sprintf("ec2-%s.%s.compute.%s", dashIP(publicIP), region, baseDomain)
+	return fmt.Sprintf("%s%s.%s.compute.%s", ec2PublicLabelPrefix, dashIP(publicIP), region, baseDomain)
 }
 
 // EC2PrivateName is the private AWS-parity name for an instance:
 // ip-{dashed-private-ip}.{region}.{internalDomain} (IMDS synthHostname). The
 // internal domain defaults to PrivateZone when empty.
 func EC2PrivateName(privateIP, region, internalDomain string) string {
-	return fmt.Sprintf("ip-%s.%s.%s", dashIP(privateIP), region, privateZoneOrDefault(internalDomain))
+	return fmt.Sprintf("%s%s.%s.%s", ec2PrivateLabelPrefix, dashIP(privateIP), region,
+		privateZoneOrDefault(internalDomain))
 }
 
 // EC2DNSNames returns the public and private AWS-shaped DNS names for an

spinifex/handlers/dns/naming_test.go

@@ -3,10 +3,34 @@ package dns
 import (
 	"testing"
 
+	"github.com/mulgadc/spinifex/spinifex/vm"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+// Two callers read this predicate in opposite directions: the terminate hook
+// withdraws when it is false, and the reconcile's desired set omits — and so
+// prunes — when it is false. They have to agree on every state.
+func TestInstanceRetainsRecords(t *testing.T) {
+	tests := []struct {
+		name   string
+		status vm.InstanceState
+		want   bool
+	}{
+		{name: "running", status: vm.StateRunning, want: true},
+		{name: "stopping", status: vm.StateStopping, want: true},
+		{name: "stopped", status: vm.StateStopped, want: true},
+		{name: "shutting down", status: vm.StateShuttingDown, want: false},
+		{name: "terminated", status: vm.StateTerminated, want: false},
+		{name: "error", status: vm.StateError, want: false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, InstanceRetainsRecords(tt.status))
+		})
+	}
+}
+
 func TestEC2Names(t *testing.T) {
 	assert.Equal(t, "ec2-1-2-3-4.ap-southeast-2.compute.spx3.net",
 		EC2PublicName("1.2.3.4", "ap-southeast-2", "spx3.net"))

spinifex/handlers/dns/reconcile.go

@@ -57,6 +57,11 @@ type PruneScope struct {
 	ELB bool
 	EKS bool
 	RDS bool
+
+	// EC2 is set only when the instance view spanned every node. A node's own
+	// VM manager does not qualify: pruning against it would delete the records
+	// of every instance running elsewhere.
+	EC2 bool
 }
 
 // Reconciler converges the zone toward the live inventory. On each pass it
@@ -70,21 +75,24 @@ type PruneScope struct {
 // the queue group load-balances rather than serialises, an idle cluster raced
 // its own zone object once per cycle.
 //
-// Only cluster-wide-enumerable records (load balancers, EKS clusters) are
-// pruned: any node sees the full ELB/EKS set from KV. EC2 records are never
-// pruned here because a node's vmMgr holds only its own instances — an
-// incomplete view would delete another node's records. EC2 removal stays with
-// the terminate hook; the reconcile only repairs missing/incorrect EC2 records.
+// Only cluster-wide-enumerable records are pruned, and only for the classes a
+// cycle enumerated in full. Every class the desired set carries is readable
+// from KV across the whole cluster, so a pass that reads them all can prune
+// them all; one that could not read a class repairs it without pruning it.
 type Reconciler struct {
 	enabled    bool
 	s3cfg      *nsconfig.S3Config
 	baseDomain string
-	nc         *nats.Conn
-	desired    DesiredFunc
-	interval   time.Duration
-	accountID  string
-	holder     string
-	sources    []reconciler.Source
+	// internalDomain is the private zone instance records land in. It is read
+	// as well as the base domain because pruning one zone and not the other
+	// leaves every stale private record behind.
+	internalDomain string
+	nc             *nats.Conn
+	desired        DesiredFunc
+	interval       time.Duration
+	accountID      string
+	holder         string
+	sources        []reconciler.Source
 }
 
 // NewReconciler builds the drift backstop. It is disabled (a no-op) when
@@ -110,6 +118,7 @@ func NewReconciler(cfg *config.Config, nc *nats.Conn, desired DesiredFunc, sourc
 	r.enabled = true
 	r.s3cfg = zoneCfg.s3
 	r.baseDomain = strings.TrimSpace(zoneCfg.server.DefaultDomain)
+	r.internalDomain = ResolveInternalDomain(cfg)
 	return r
 }
 
@@ -280,31 +289,50 @@ func splitReconcileChanges(changes []Change, payloadLimit int) ([][]Change, erro
 	return batches, nil
 }
 
-// computeBatch reads the base zone (the only zone holding prunable ELB/EKS
-// records) and converges the desired set against it.
+// computeBatch reads every zone holding prunable records and converges the
+// desired set against them. That is the base domain, plus the private zone once
+// instance records are prunable — a stale private record is as wrong as a stale
+// public one, and only this zone holds it.
 func (r *Reconciler) computeBatch() ([]Change, error) {
 	ds := r.desired()
 	existing := map[string][]zoneRecord{}
-	recs, ok, err := r.readZone(r.baseDomain)
-	if err != nil {
-		return nil, err
+	zones := []string{r.baseDomain}
+	if ds.Prunable.EC2 {
+		zones = append(zones, privateZoneOrDefault(r.internalDomain))
 	}
-	if ok {
-		existing[r.baseDomain] = recs
+	for _, zone := range zones {
+		if _, seen := existing[zone]; seen || zone == "" {
+			continue
+		}
+		recs, ok, err := r.readZone(zone)
+		if err != nil {
+			return nil, err
+		}
+		if ok {
+			existing[zone] = recs
+		}
 	}
 	return computeConverge(ds.Changes, existing, r.prunable(ds.Prunable))
 }
 
 // prunable returns the predicate deciding whether a (zone, label) record may be
-// deleted when absent from the desired set: load-balancer, EKS and RDS records
-// in the base domain, but only for the classes this cycle enumerated authoritatively
-// across all tenants. EC2 (`.compute.`) records are never pruned (a node sees
-// only its own instances); structural (apex/NS/glue) records never match.
+// deleted when absent from the desired set, for the classes this cycle
+// enumerated authoritatively across all tenants. Structural (apex/NS/glue)
+// records carry none of these prefixes, so they never match.
 func (r *Reconciler) prunable(scope PruneScope) func(zone, label string) bool {
+	private := privateZoneOrDefault(r.internalDomain)
 	return func(zone, label string) bool {
+		// The private zone holds instance records and nothing else: every other
+		// producer writes the base domain.
+		if zone == private {
+			return scope.EC2 && strings.HasPrefix(label, ec2PrivateLabelPrefix)
+		}
 		if zone != r.baseDomain {
 			return false
 		}
+		if scope.EC2 && strings.HasPrefix(label, ec2PublicLabelPrefix) {
+			return true
+		}
 		if scope.ELB && strings.Contains(label, ".elb.") {
 			return true
 		}

spinifex/handlers/dns/reconcile_ec2_prune_test.go

@@ -0,0 +1,151 @@
+//test:in-package — prunable, computeBatch, zoneRecord and the label prefixes are
+//all unexported, and what these tests pin is which records a pass is willing to
+//delete rather than anything the package exports.
+
+package dns
+
+import (
+	"fmt"
+	"testing"
+
+	nsconfig "github.com/mulgadc/northstar/pkg/config"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+const testPrivate = "compute.internal"
+
+// prunableForZones mirrors prunableFor with a private zone configured, which is
+// what makes instance records reachable at all.
+func prunableForZones(scope PruneScope) func(zone, label string) bool {
+	r := &Reconciler{baseDomain: testBase, internalDomain: testPrivate}
+	return r.prunable(scope)
+}
+
+// zoneTOML renders a minimal parseable zone carrying an apex NS plus the given
+// A records, so a test can seed one into the fake S3 and read it back.
+func zoneTOML(domain string, records map[string]string) string {
+	out := fmt.Sprintf("version = 1.0\n[domain]\ndomain = %q\nactive = true\nsoa = \"ns1.%s.\"\n"+
+		"[defaults]\nttl = 300\ntype = 1\nclass = 1\n"+
+		"[[records]]\ndomain = \"\"\ntype = 2\naddress = \"ns1.%s.\"\n", domain, domain, domain)
+	for label, addr := range records {
+		out += fmt.Sprintf("[[records]]\ndomain = %q\ntype = 1\naddress = %q\n", label, addr)
+	}
+	return out
+}
+
+// An instance record is prunable in both of the zones it lands in, and only
+// with the authority that says the instance view spanned every node. Without
+// that authority pruning would delete the records of every instance running
+// somewhere else, which is what the vmMgr-backed desired set could not avoid.
+func TestPrunable_InstanceRecordsNeedClusterWideAuthority(t *testing.T) {
+	public := "ec2-3-3-3-3.ap-southeast-2.compute"
+	private := "ip-172-31-0-6.ap-southeast-2"
+
+	withAuthority := prunableForZones(PruneScope{EC2: true})
+	assert.True(t, withAuthority(testBase, public), "a public instance record in the base zone")
+	assert.True(t, withAuthority(testPrivate, private), "a private instance record in the private zone")
+
+	without := prunableForZones(PruneScope{ELB: true, EKS: true, RDS: true})
+	assert.False(t, without(testBase, public),
+		"without EC2 authority the instance view may be one node's, so nothing may be pruned")
+	assert.False(t, without(testPrivate, private),
+		"authority for the other classes does not extend to instances")
+}
+
+// The private zone holds instance records and structural records, nothing else.
+// Pruning it on "absent from the desired set" alone would take the NS set with
+// it, which is a zone outage rather than a stale address.
+func TestPrunable_PrivateZoneStructuralRecordsSurvive(t *testing.T) {
+	prune := prunableForZones(PruneScope{EC2: true, ELB: true, EKS: true, RDS: true})
+	for _, label := range []string{"", "ns1", "soa"} {
+		assert.False(t, prune(testPrivate, label),
+			"structural record %q in the private zone must never be pruned", label)
+	}
+}
+
+// An address that no running instance holds is stale in both zones at once: the
+// record name is derived from the IP, so a reallocated address otherwise keeps
+// resolving to whatever held it before.
+func TestComputeConverge_PrunesAStaleInstanceRecordInBothZones(t *testing.T) {
+	desired := []Change{
+		upsert("ec2-3-3-3-3.ap-southeast-2.compute."+testBase, "3.3.3.3"),
+		{Action: ActionUpsert, Zone: testPrivate,
+			Name: "ip-172-31-0-6.ap-southeast-2." + testPrivate, Type: "A", Value: "172.31.0.6"},
+	}
+	// Stored labels are zone-relative and keep their trailing dot, which is what
+	// relativeLabel produces and therefore what a desired record is matched on.
+	existing := map[string][]zoneRecord{
+		testBase: {
+			existingA("ec2-3-3-3-3.ap-southeast-2.compute.", "3.3.3.3"),
+			existingA("ec2-9-9-9-9.ap-southeast-2.compute.", "9.9.9.9"),
+			{label: "", rtype: nsconfig.TypeNS, value: "ns1." + testBase + "."},
+		},
+		testPrivate: {
+			existingA("ip-172-31-0-6.ap-southeast-2.", "172.31.0.6"),
+			existingA("ip-172-31-0-99.ap-southeast-2.", "172.31.0.99"),
+			{label: "", rtype: nsconfig.TypeNS, value: "ns1." + testPrivate + "."},
+		},
+	}
+
+	batch, err := computeConverge(desired, existing, prunableForZones(PruneScope{EC2: true}))
+	require.NoError(t, err)
+
+	deletes := deletesOf(batch)
+	names := make([]string, 0, len(deletes))
+	for _, d := range deletes {
+		names = append(names, d.Name)
+	}
+	assert.ElementsMatch(t, []string{
+		"ec2-9-9-9-9.ap-southeast-2.compute." + testBase,
+		"ip-172-31-0-99.ap-southeast-2." + testPrivate,
+	}, names, "both zones prune the address no running instance holds, and nothing else")
+}
+
+// End to end through the real zone read: the private zone has to be one of the
+// zones a pass reads, or its stale records are never even considered. Whether
+// it is read when no pass could act on it is an S3 GET, not a behaviour, so it
+// is not asserted here.
+func TestComputeBatch_PrunesBothZonesUnderEC2Authority(t *testing.T) {
+	endpoint, objects := fakeS3(t, "northstar")
+	objects[testBase+".toml"] = zoneTOML(testBase, map[string]string{
+		"ec2-3-3-3-3.ap-southeast-2.compute.": "3.3.3.3",
+		"ec2-9-9-9-9.ap-southeast-2.compute.": "9.9.9.9",
+	})
+	objects[testPrivate+".toml"] = zoneTOML(testPrivate, map[string]string{
+		"ip-172-31-0-6.ap-southeast-2.":  "172.31.0.6",
+		"ip-172-31-0-99.ap-southeast-2.": "172.31.0.99",
+	})
+
+	// One instance is still running, so its two records must survive both passes.
+	live := []Change{
+		upsert("ec2-3-3-3-3.ap-southeast-2.compute."+testBase, "3.3.3.3"),
+		{Action: ActionUpsert, Zone: testPrivate,
+			Name: "ip-172-31-0-6.ap-southeast-2." + testPrivate, Type: "A", Value: "172.31.0.6"},
+	}
+	scope := PruneScope{}
+	r := &Reconciler{
+		enabled:        true,
+		baseDomain:     testBase,
+		internalDomain: testPrivate,
+		s3cfg: &nsconfig.S3Config{
+			Endpoint: endpoint, Bucket: "northstar", Region: "us-east-1",
+			AccessKey: "SYSTEM", SecretKey: "SYSTEMSECRET",
+		},
+		desired: func() DesiredSet { return DesiredSet{Changes: live, Prunable: scope} },
+	}
+
+	batch, err := r.computeBatch()
+	require.NoError(t, err)
+	assert.Empty(t, deletesOf(batch), "no EC2 authority prunes nothing in either zone")
+
+	scope = PruneScope{EC2: true}
+	batch, err = r.computeBatch()
+	require.NoError(t, err)
+	deletes := deletesOf(batch)
+	require.Len(t, deletes, 2, "with authority both zones are read and both stale records prune")
+	assert.ElementsMatch(t, []string{
+		"ec2-9-9-9-9.ap-southeast-2.compute." + testBase,
+		"ip-172-31-0-99.ap-southeast-2." + testPrivate,
+	}, []string{deletes[0].Name, deletes[1].Name})
+}

spinifex/handlers/dns/reconcile_test.go

@@ -46,7 +46,7 @@ func TestComputeConvergeUpsertsPassThroughAndPruneStale(t *testing.T) {
 		existingA("app-old-xyz.ap-southeast-2.elb.", "9.9.9.9"),
 		// A live EKS record still desired → kept (dedup by RRset, not re-deleted).
 		existingA("prod.ap-southeast-2.eks.", "2.2.2.2"),
-		// An EC2 record absent from this node's desired view → must NOT be pruned.
+		// An EC2 record, with no EC2 authority this cycle → must NOT be pruned.
 		existingA("ec2-4-4-4-4.ap-southeast-2.compute.", "4.4.4.4"),
 		// Structural apex NS → never pruned.
 		{label: "", rtype: nsconfig.TypeNS, value: "ns1.spx3.net."},
@@ -65,7 +65,7 @@ func TestComputeConvergeUpsertsPassThroughAndPruneStale(t *testing.T) {
 
 	// EC2 and structural records survive.
 	for _, d := range deletes {
-		assert.NotContains(t, d.Name, ".compute.", "EC2 records are never pruned")
+		assert.NotContains(t, d.Name, ".compute.", "EC2 pruning is suppressed when not enumerated")
 		assert.NotEmpty(t, d.Name, "structural apex records are never pruned")
 	}
 }
@@ -126,7 +126,7 @@ func TestComputeConvergePrunesRDSOnlyWhenEnumerated(t *testing.T) {
 	assert.Equal(t, "dropped-db.111122223333.ap-southeast-2.rds.spx3.net", deletes[0].Name)
 	for _, d := range deletes {
 		assert.NotContains(t, d.Name, ".elb.", "RDS authority does not grant ELB pruning")
-		assert.NotContains(t, d.Name, ".compute.", "EC2 records are never pruned")
+		assert.NotContains(t, d.Name, ".compute.", "EC2 pruning is suppressed when not enumerated")
 	}
 }
 

spinifex/handlers/ec2/instance/dns_hook_test.go

@@ -37,26 +37,6 @@ func TestPublishDNS(t *testing.T) {
 	enabled.publishDNS("123456789012", handlers_dns.ActionUpsert, nil)
 }
 
-func TestNeedsDNSWithdrawal(t *testing.T) {
-	tests := []struct {
-		name   string
-		status vm.InstanceState
-		want   bool
-	}{
-		{name: "running", status: vm.StateRunning, want: false},
-		{name: "stopping", status: vm.StateStopping, want: false},
-		{name: "stopped", status: vm.StateStopped, want: false},
-		{name: "shutting down", status: vm.StateShuttingDown, want: true},
-		{name: "terminated", status: vm.StateTerminated, want: true},
-		{name: "error", status: vm.StateError, want: true},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			require.Equal(t, tt.want, needsDNSWithdrawal(tt.status))
-		})
-	}
-}
-
 // The race the check exists for: terminate lands while the deferred launch is
 // still publishing the UPSERT, so Status has not caught up and only the
 // deletion mark says the record is going away.

spinifex/handlers/ec2/instance/service_impl.go

@@ -1024,17 +1024,13 @@ func (s *InstanceServiceImpl) LaunchRunInstances(ctx context.Context, instances
 	slog.InfoContext(ctx, "LaunchRunInstances: completed", "requested", len(instances), "launched", successCount)
 }
 
-func needsDNSWithdrawal(status vm.InstanceState) bool {
-	return status != vm.StateRunning && status != vm.StateStopping && status != vm.StateStopped
-}
-
 // terminateRacedLaunch reports whether a terminate raced the deferred launch
 // UPSERT: it reads the terminate flag stamped under the lock (plus status) since
 // the async state transition may not have landed, which a status-only check misses.
 func (s *InstanceServiceImpl) terminateRacedLaunch(instance *vm.VM) bool {
 	withdraw := false
 	s.vmMgr.Inspect(instance, func(v *vm.VM) {
-		withdraw = v.DeletionTimestamp != nil || needsDNSWithdrawal(v.Status)
+		withdraw = v.DeletionTimestamp != nil || !handlers_dns.InstanceRetainsRecords(v.Status)
 	})
 	return withdraw
 }