fix: return InvalidInstanceID.NotFound for nonexistent DescribeInstances IDs (#646)

DescribeInstances silently returned an empty Reservations list for an
instance ID that never existed, so callers could not distinguish "gone"
from "never existed". Mirror the two-pass found-set pattern from
DescribeSecurityGroups, but only assert absence when the NATS fan-out and
both stopped/terminated KV bucket queries are provably complete — a
node timing out during the fan-out must never produce a false NotFound
for an instance that actually exists on that node.

The assertion is split into a new DescribeInstancesChecked, used only by
the gateway's customer-facing action. The plain DescribeInstances stays
silent for internal callers (IMDS instance lookup, the RDS instance-state
resolver, the EKS control-plane reconciler) that treat "not found" as
"not currently present" rather than a fault.

spinifex/gateway/ec2.go

@@ -117,7 +117,7 @@ func ec2HandlerWithReq[In any](handler func(ctx context.Context, input *In, gw *
 
 var ec2Actions = map[string]EC2Handler{
 	"DescribeInstances": ec2Handler(func(ctx context.Context, input *ec2.DescribeInstancesInput, gw *GatewayConfig, accountID string) (any, error) {
-		out, err := gateway_ec2_instance.DescribeInstances(ctx, input, gw.NATSConn, gw.DiscoverActiveNodes(ctx), accountID)
+		out, err := gateway_ec2_instance.DescribeInstancesChecked(ctx, input, gw.NATSConn, gw.DiscoverActiveNodes(ctx), accountID)
 		if err != nil {
 			return out, err
 		}

spinifex/gateway/ec2/instance/DescribeInstances.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/mulgadc/spinifex/spinifex/awserrors"
 	handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam"
 	"github.com/mulgadc/spinifex/spinifex/utils"
 	"github.com/nats-io/nats.go"
@@ -18,8 +19,15 @@ import (
 
 // DescribeInstances fans out to all nodes via NATS and aggregates the results.
 // It is lenient: partial results from a slow or unreachable node are returned
-// without error, which is correct for user-facing describes. Callers that must
-// not act on a partial view (the quota reconcile) use DescribeInstancesForReconcile.
+// without error, and an explicitly named instance ID that is simply absent
+// from the aggregate is not an error either — the caller sees an empty list.
+// That silence is required by internal callers (the IMDS instance lookup, the
+// RDS instance-state resolver, the EKS control-plane reconciler) which treat
+// "not found" as "not currently running/present" rather than a fault.
+// The customer-facing gateway action uses DescribeInstancesChecked instead,
+// which adds the InvalidInstanceID.NotFound assertion AWS callers expect.
+// Callers that must not act on a partial view at all (the quota reconcile)
+// use DescribeInstancesForReconcile.
 func DescribeInstances(ctx context.Context, input *ec2.DescribeInstancesInput, natsConn *nats.Conn, expectedNodes int, accountID string) (*ec2.DescribeInstancesOutput, error) {
 	reservations, _, firstClient4xx, err := gatherInstances(ctx, input, natsConn, expectedNodes, accountID)
 	if err != nil {
@@ -32,6 +40,43 @@ func DescribeInstances(ctx context.Context, input *ec2.DescribeInstancesInput, n
 	return &ec2.DescribeInstancesOutput{Reservations: reservations}, nil
 }
 
+// DescribeInstancesChecked is the customer-facing variant served by the
+// gateway's DescribeInstances action. On top of DescribeInstances' aggregation,
+// it asserts InvalidInstanceID.NotFound for any explicitly named instance ID
+// absent from the result — but only when the sweep is provably complete
+// (every expected node answered, both KV buckets succeeded). A partial sweep
+// stays silent, the same as DescribeInstances: a node timing out during the
+// fan-out must never turn into a false NotFound for an instance that actually
+// exists on that node. A --filters-only query (no explicit IDs) is never
+// affected and keeps returning an empty list with no error.
+func DescribeInstancesChecked(ctx context.Context, input *ec2.DescribeInstancesInput, natsConn *nats.Conn, expectedNodes int, accountID string) (*ec2.DescribeInstancesOutput, error) {
+	reservations, complete, firstClient4xx, err := gatherInstances(ctx, input, natsConn, expectedNodes, accountID)
+	if err != nil {
+		return nil, err
+	}
+	if firstClient4xx != "" && len(reservations) == 0 {
+		return nil, errors.New(firstClient4xx)
+	}
+
+	if len(input.InstanceIds) > 0 && complete {
+		found := make(map[string]bool)
+		for _, res := range reservations {
+			for _, inst := range res.Instances {
+				if inst != nil && inst.InstanceId != nil {
+					found[*inst.InstanceId] = true
+				}
+			}
+		}
+		for _, id := range input.InstanceIds {
+			if id != nil && !found[*id] {
+				return nil, errors.New(awserrors.ErrorInvalidInstanceIDNotFound)
+			}
+		}
+	}
+
+	return &ec2.DescribeInstancesOutput{Reservations: reservations}, nil
+}
+
 // DescribeInstancesForReconcile is the strict variant the quota reconcile uses.
 // complete is true only when the sweep observed every expected node and both
 // instance buckets, so reconcile may lower a counter only from a provably

spinifex/gateway/ec2/instance/DescribeInstances_test.go

@@ -246,6 +246,156 @@ func TestDescribeInstances_EarlyExitWithExpectedNodes(t *testing.T) {
 	assert.Less(t, duration, 2*time.Second)
 }
 
+// subscribeEmptyInstanceBuckets makes the stopped/terminated KV bucket
+// queries in gatherInstances succeed with no instances, so a fan-out that
+// resolves fully still isn't marked incomplete just because the buckets have
+// nothing to say.
+func subscribeEmptyInstanceBuckets(t *testing.T, nc *nats.Conn) {
+	t.Helper()
+	for _, topic := range []string{"ec2.DescribeStoppedInstances", "ec2.DescribeTerminatedInstances"} {
+		_, err := nc.Subscribe(topic, func(msg *nats.Msg) {
+			data, _ := json.Marshal(&ec2.DescribeInstancesOutput{Reservations: []*ec2.Reservation{}})
+			msg.Respond(data)
+		})
+		require.NoError(t, err)
+	}
+	nc.Flush()
+}
+
+// TestDescribeInstancesChecked_ExplicitIDNotFound_CompleteSweep covers the
+// bead's primary criterion: naming a nonexistent instance ID with a
+// fully-answered fan-out returns InvalidInstanceID.NotFound instead of a
+// silent empty list.
+func TestDescribeInstancesChecked_ExplicitIDNotFound_CompleteSweep(t *testing.T) {
+	_, nc := startTestNATSServer(t)
+	subscribeEmptyInstanceBuckets(t, nc)
+
+	_, err := nc.Subscribe("ec2.DescribeInstances", func(msg *nats.Msg) {
+		data, _ := json.Marshal(&ec2.DescribeInstancesOutput{
+			Reservations: []*ec2.Reservation{
+				{
+					ReservationId: aws.String("r-1"),
+					Instances:     []*ec2.Instance{{InstanceId: aws.String("i-real")}},
+				},
+			},
+		})
+		msg.Respond(data)
+	})
+	require.NoError(t, err)
+	nc.Flush()
+
+	input := &ec2.DescribeInstancesInput{InstanceIds: []*string{aws.String("i-doesnotexist0000000")}}
+	_, err = DescribeInstancesChecked(context.Background(), input, nc, 1, "123456789012")
+
+	require.Error(t, err)
+	assert.Equal(t, awserrors.ErrorInvalidInstanceIDNotFound, err.Error())
+}
+
+// TestDescribeInstances_ExplicitIDNotFound_StaysSilent guards the internal
+// callers (IMDS lookup, RDS instance-state resolver, EKS control-plane
+// reconciler) that call the plain DescribeInstances and rely on "not found"
+// meaning an empty list, not an error. Only DescribeInstancesChecked — used
+// solely by the gateway's customer-facing action — may assert NotFound.
+func TestDescribeInstances_ExplicitIDNotFound_StaysSilent(t *testing.T) {
+	_, nc := startTestNATSServer(t)
+	subscribeEmptyInstanceBuckets(t, nc)
+
+	// Mirrors a real node: it filters by the requested InstanceIds itself, so
+	// a query for an ID it doesn't hold gets an empty reservations list back.
+	_, err := nc.Subscribe("ec2.DescribeInstances", func(msg *nats.Msg) {
+		data, _ := json.Marshal(&ec2.DescribeInstancesOutput{Reservations: []*ec2.Reservation{}})
+		msg.Respond(data)
+	})
+	require.NoError(t, err)
+	nc.Flush()
+
+	input := &ec2.DescribeInstancesInput{InstanceIds: []*string{aws.String("i-doesnotexist0000000")}}
+	output, err := DescribeInstances(context.Background(), input, nc, 1, "123456789012")
+
+	require.NoError(t, err)
+	require.NotNil(t, output)
+	assert.Empty(t, output.Reservations)
+}
+
+// TestDescribeInstancesChecked_ExplicitIDFound_CompleteSweep is the positive
+// counterpart: the same complete sweep, but the requested ID is present, so
+// no error is raised.
+func TestDescribeInstancesChecked_ExplicitIDFound_CompleteSweep(t *testing.T) {
+	_, nc := startTestNATSServer(t)
+	subscribeEmptyInstanceBuckets(t, nc)
+
+	_, err := nc.Subscribe("ec2.DescribeInstances", func(msg *nats.Msg) {
+		data, _ := json.Marshal(&ec2.DescribeInstancesOutput{
+			Reservations: []*ec2.Reservation{
+				{
+					ReservationId: aws.String("r-1"),
+					Instances:     []*ec2.Instance{{InstanceId: aws.String("i-real")}},
+				},
+			},
+		})
+		msg.Respond(data)
+	})
+	require.NoError(t, err)
+	nc.Flush()
+
+	input := &ec2.DescribeInstancesInput{InstanceIds: []*string{aws.String("i-real")}}
+	output, err := DescribeInstancesChecked(context.Background(), input, nc, 1, "123456789012")
+
+	require.NoError(t, err)
+	require.Len(t, output.Reservations, 1)
+}
+
+// TestDescribeInstancesChecked_FilterOnlyNoMatch_ReturnsEmptyNotNotFound
+// covers the bead's second criterion: a --filters query that matches nothing
+// must still return rc=0 with an empty list, never NotFound (no instance IDs
+// were named).
+func TestDescribeInstancesChecked_FilterOnlyNoMatch_ReturnsEmptyNotNotFound(t *testing.T) {
+	_, nc := startTestNATSServer(t)
+	subscribeEmptyInstanceBuckets(t, nc)
+
+	_, err := nc.Subscribe("ec2.DescribeInstances", func(msg *nats.Msg) {
+		data, _ := json.Marshal(&ec2.DescribeInstancesOutput{Reservations: []*ec2.Reservation{}})
+		msg.Respond(data)
+	})
+	require.NoError(t, err)
+	nc.Flush()
+
+	input := &ec2.DescribeInstancesInput{
+		Filters: []*ec2.Filter{{Name: aws.String("tag:nope"), Values: []*string{aws.String("nothing")}}},
+	}
+	output, err := DescribeInstancesChecked(context.Background(), input, nc, 1, "123456789012")
+
+	require.NoError(t, err)
+	require.NotNil(t, output)
+	assert.Empty(t, output.Reservations)
+}
+
+// TestDescribeInstancesChecked_ExplicitIDNotFound_NodeTimeout is the bead's
+// subtle third criterion: a node timing out during the fan-out must not
+// produce a false NotFound. Only one of two expected nodes answers, so the
+// sweep is provably incomplete and the naive found-set check must be
+// suppressed.
+func TestDescribeInstancesChecked_ExplicitIDNotFound_NodeTimeout(t *testing.T) {
+	_, nc := startTestNATSServer(t)
+	subscribeEmptyInstanceBuckets(t, nc)
+
+	// Only one of the two expected nodes has a subscriber; the second never
+	// answers, so the fan-out hits its 3s deadline with sum.TimedOut=true.
+	_, err := nc.Subscribe("ec2.DescribeInstances", func(msg *nats.Msg) {
+		data, _ := json.Marshal(&ec2.DescribeInstancesOutput{Reservations: []*ec2.Reservation{}})
+		msg.Respond(data)
+	})
+	require.NoError(t, err)
+	nc.Flush()
+
+	input := &ec2.DescribeInstancesInput{InstanceIds: []*string{aws.String("i-on-the-slow-node")}}
+	output, err := DescribeInstancesChecked(context.Background(), input, nc, 2, "123456789012")
+
+	require.NoError(t, err, "an incomplete sweep must never assert a false NotFound")
+	require.NotNil(t, output)
+	assert.Empty(t, output.Reservations)
+}
+
 func TestDescribeInstances_ClosedConnection(t *testing.T) {
 	_, nc := startTestNATSServer(t)