fix: read only the named documents on explicit-ID describes (#1069)

spinifex/ebsmetadata/store.go

@@ -229,11 +229,11 @@ const (
 	amiPrefix      = "spinifex/ebsmetadata/v2/amis/"
 )
 
-// listFetchConcurrency bounds the object fetches a single listing may have in
-// flight. A listing costs one round trip per document, so fetching them one at
-// a time makes the call scale with the number of documents in the bucket rather
-// than with the answer's size.
-const listFetchConcurrency = 16
+// FetchConcurrency bounds the object fetches one metadata read may have in
+// flight, whether it is a listing or an explicit-ID fan-out. A fetch costs one
+// round trip per document, so fetching them one at a time makes the call scale
+// with the number of documents rather than with the answer's size.
+const FetchConcurrency = 16
 
 // listFetchTimeout bounds how long one document may hold up a listing. A
 // document whose shards no longer reassemble does not fail quickly — the read
@@ -316,7 +316,7 @@ func listDocuments[T any](
 	}
 
 	results := make([]fetched, len(keys))
-	slots := make(chan struct{}, listFetchConcurrency)
+	slots := make(chan struct{}, FetchConcurrency)
 	var wg sync.WaitGroup
 
 	for i, key := range keys {
@@ -359,6 +359,39 @@ func listDocuments[T any](
 	return documents, nil
 }
 
+// FetchResult is one document from an explicit-ID fan-out. Err is that ID's
+// own store error, so a failed read stays attributable to the ID that caused it.
+type FetchResult[T any] struct {
+	Document T
+	Err      error
+}
+
+// FetchByIDs reads one document per ID through get, bounded to the same
+// concurrency a listing uses and positionally aligned with ids so ordering and
+// per-ID errors do not depend on goroutine scheduling.
+//
+// No per-document timeout, unlike a listing: every named document is part of
+// the answer, so the caller's own deadline is the bound.
+func FetchByIDs[T any](ctx context.Context, ids []string, get func(context.Context, string) (T, error)) []FetchResult[T] {
+	results := make([]FetchResult[T], len(ids))
+	slots := make(chan struct{}, FetchConcurrency)
+	var wg sync.WaitGroup
+
+	for i, id := range ids {
+		wg.Add(1)
+		slots <- struct{}{}
+		go func(idx int, id string) {
+			defer wg.Done()
+			defer func() { <-slots }()
+
+			results[idx].Document, results[idx].Err = get(ctx, id)
+		}(i, id)
+	}
+	wg.Wait()
+
+	return results
+}
+
 func (s *Store) PutAMI(ctx context.Context, ami AMI) error {
 	key, err := AMIKey(ami.ImageID)
 	if err != nil {

spinifex/ebsmetadata/store_test.go

@@ -384,6 +384,45 @@ func TestListAMIs_SkipsCorruptButStrictDoesNot(t *testing.T) {
 	require.ErrorIs(t, err, ErrCorruptDocument)
 }
 
+// TestFetchByIDs_BoundedAndPositionallyAligned covers what an explicit-ID
+// describe needs from the fan-out: reads overlap, the pool bound holds however
+// many IDs a caller names, and each result stays with the ID that produced it.
+func TestFetchByIDs_BoundedAndPositionallyAligned(t *testing.T) {
+	objects := objectstore.NewMemoryObjectStore()
+	store := NewStore(objects, "control-plane")
+	ctx := context.Background()
+
+	const documents = FetchConcurrency * 4
+	ids := make([]string, 0, documents)
+	for i := range documents {
+		id := fmt.Sprintf("ami-%02d", i)
+		ids = append(ids, id)
+		require.NoError(t, store.PutAMI(ctx, AMI{ImageID: id, Name: id}))
+	}
+	// One ID names no document, to pin that its error lands on its own result
+	// rather than failing the whole fan-out.
+	ids = append(ids, "ami-absent")
+
+	slow := &slowGetStore{ObjectStore: objects, delay: 20 * time.Millisecond}
+	measured := NewStore(slow, "control-plane")
+
+	start := time.Now()
+	results := FetchByIDs(ctx, ids, measured.GetAMI)
+	elapsed := time.Since(start)
+
+	require.Len(t, results, len(ids))
+	assert.Less(t, elapsed, time.Duration(len(ids))*20*time.Millisecond/2,
+		"named documents must be read concurrently, not one after the other")
+	assert.LessOrEqual(t, slow.peak(), int64(FetchConcurrency),
+		"a caller naming hundreds of IDs must not open hundreds of reads")
+	for i, id := range ids[:documents] {
+		require.NoError(t, results[i].Err)
+		assert.Equal(t, id, results[i].Document.ImageID)
+	}
+	assert.True(t, objectstore.IsNoSuchKeyError(results[documents].Err),
+		"the absent ID's own result carries its error")
+}
+
 // TestListVolumes_FetchesDocumentsConcurrently is the reason listDocuments has
 // a worker pool at all. A listing costs one object fetch per document, and
 // fetching them one at a time makes DescribeVolumes scale with the number of
@@ -415,7 +454,7 @@ func TestListVolumes_FetchesDocumentsConcurrently(t *testing.T) {
 	assert.Less(t, elapsed, serial/2,
 		"fetching %d documents took %s; serially it would be %s", documents, elapsed, serial)
 	assert.Equal(t, int64(documents), slow.gets.Load(), "every document must still be fetched")
-	assert.LessOrEqual(t, slow.peak(), int64(listFetchConcurrency),
+	assert.LessOrEqual(t, slow.peak(), int64(FetchConcurrency),
 		"the pool bound must hold: an unbounded fan-out over a large bucket is its own problem")
 }
 

spinifex/handlers/ec2/image/describe_byid_test.go

@@ -0,0 +1,348 @@
+package handlers_ec2_image_test
+
+import (
+	"context"
+	"errors"
+	"strconv"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/aws/aws-sdk-go/service/s3"
+	"github.com/mulgadc/spinifex/spinifex/awserrors"
+	"github.com/mulgadc/spinifex/spinifex/ebsmetadata"
+	handlers_ec2_image "github.com/mulgadc/spinifex/spinifex/handlers/ec2/image"
+	"github.com/mulgadc/spinifex/spinifex/objectstore"
+	"github.com/mulgadc/spinifex/spinifex/testutil/recordingstore"
+	"github.com/mulgadc/spinifex/spinifex/utils"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	byIDBucket  = "test-bucket"
+	byIDAccount = "000000000001"
+	byIDOther   = "000000000002"
+	byIDImage   = "ami-target"
+)
+
+func putByIDAMI(t *testing.T, store objectstore.ObjectStore, ami ebsmetadata.AMI) {
+	t.Helper()
+	require.NoError(t, ebsmetadata.NewStore(store, byIDBucket).PutAMI(context.Background(), ami))
+}
+
+// byIDAMIDoc is a complete, readable AMI document owned by ownerAlias, which is
+// an account ID for a private image and a name like "amazon" for a system one.
+func byIDAMIDoc(imageID, ownerAlias string) ebsmetadata.AMI {
+	return ebsmetadata.AMI{
+		ImageID: imageID, Name: "debian-13", Description: "Debian 13",
+		Architecture: "x86_64", PlatformDetails: "Linux/UNIX", CreationDate: time.Now().UTC(),
+		RootDeviceType: "ebs", Virtualization: "hvm", ImageOwnerAlias: ownerAlias,
+		VolumeSizeGiB: 8, State: "available", Tags: map[string]string{"Name": "base"},
+	}
+}
+
+// seedByIDCatalog grows the AMI prefix with another account's images. The prefix
+// is unpartitioned, so before the fast path these were fetched and decoded on
+// every by-ID describe only to be discarded.
+func seedByIDCatalog(t *testing.T, store objectstore.ObjectStore, n int) {
+	t.Helper()
+	for i := range n {
+		putByIDAMI(t, store, byIDAMIDoc("ami-bulk"+strconv.Itoa(i), byIDOther))
+	}
+}
+
+func amiKey(t *testing.T, imageID string) string {
+	t.Helper()
+	key, err := ebsmetadata.AMIKey(imageID)
+	require.NoError(t, err)
+	return key
+}
+
+// A named image costs one document read and no listing, however many images the
+// cluster holds. The whole cluster's count was the old cost, so the operation
+// count is the assertion, not the answer.
+func TestDescribeImagesByID_ReadsOnlyTheNamedDocument(t *testing.T) {
+	for _, catalog := range []int{1, 500} {
+		t.Run("catalog"+strconv.Itoa(catalog), func(t *testing.T) {
+			store := recordingstore.New()
+			svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket)
+			putByIDAMI(t, store, byIDAMIDoc(byIDImage, byIDAccount))
+			seedByIDCatalog(t, store, catalog)
+
+			out, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{
+				ImageIds: aws.StringSlice([]string{byIDImage}),
+			}, byIDAccount)
+
+			require.NoError(t, err)
+			require.Len(t, out.Images, 1)
+			assert.Equal(t, byIDImage, aws.StringValue(out.Images[0].ImageId))
+			assert.Empty(t, store.ListPrefixes(), "a named image must not list the AMI prefix")
+			assert.Equal(t, []string{amiKey(t, byIDImage)}, store.Gets(),
+				"a named image must read its own document and nothing else")
+		})
+	}
+}
+
+// Naming the same image twice is one read and one result: the enumerating path
+// matched each document once however often the request named it.
+func TestDescribeImagesByID_MultipleAndDuplicateIDs(t *testing.T) {
+	store := recordingstore.New()
+	svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket)
+	putByIDAMI(t, store, byIDAMIDoc("ami-a", byIDAccount))
+	putByIDAMI(t, store, byIDAMIDoc("ami-b", byIDAccount))
+
+	out, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{
+		ImageIds: aws.StringSlice([]string{"ami-a", "ami-b", "ami-a"}),
+	}, byIDAccount)
+
+	require.NoError(t, err)
+	require.Len(t, out.Images, 2)
+	assert.Equal(t, "ami-a", aws.StringValue(out.Images[0].ImageId))
+	assert.Equal(t, "ami-b", aws.StringValue(out.Images[1].ImageId))
+	assert.Len(t, store.Gets(), 2, "a duplicated ID must not be read twice")
+	assert.Empty(t, store.ListPrefixes())
+}
+
+// The AMI key carries no tenant scoping, so the fast path reads a document it
+// must not return. Reporting anything other than not-found here discloses
+// another account's private image to a caller that named its ID.
+func TestDescribeImagesByID_HiddenAndMissingAreIndistinguishable(t *testing.T) {
+	tests := []struct {
+		name       string
+		seedOwner  string
+		seedImage  bool
+		requestID  string
+		wantHidden string
+	}{
+		{name: "another account's private image", seedOwner: byIDOther, seedImage: true, requestID: byIDImage},
+		{name: "an empty owner alias", seedOwner: "", seedImage: true, requestID: byIDImage},
+		{name: "an image that does not exist", seedImage: false, requestID: "ami-never-registered"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			store := recordingstore.New()
+			svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket)
+			if tt.seedImage {
+				putByIDAMI(t, store, byIDAMIDoc(byIDImage, tt.seedOwner))
+			}
+
+			out, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{
+				ImageIds: aws.StringSlice([]string{tt.requestID}),
+			}, byIDAccount)
+
+			require.Error(t, err)
+			assert.Nil(t, out)
+			assert.Equal(t, awserrors.ErrorInvalidAMIIDNotFound, err.Error(),
+				"an image the caller cannot read must answer exactly as a nonexistent one")
+		})
+	}
+}
+
+// A system AMI's alias is not an account ID, so it resolves to the global
+// account before any owner comparison and stays visible to every tenant.
+func TestDescribeImagesByID_SystemAMIIsVisibleToEveryAccount(t *testing.T) {
+	store := recordingstore.New()
+	svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket)
+	putByIDAMI(t, store, byIDAMIDoc(byIDImage, "amazon"))
+
+	out, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{
+		ImageIds: aws.StringSlice([]string{byIDImage}),
+		Owners:   aws.StringSlice([]string{"amazon"}),
+	}, byIDAccount)
+
+	require.NoError(t, err)
+	require.Len(t, out.Images, 1)
+	assert.Equal(t, utils.GlobalAccountID, aws.StringValue(out.Images[0].OwnerId))
+	assert.Equal(t, "amazon", aws.StringValue(out.Images[0].ImageOwnerAlias))
+}
+
+// The Owners parameter is matched against the resolved owner, and a named image
+// that no owner selects is not-found rather than an empty success.
+func TestDescribeImagesByID_OwnerFilters(t *testing.T) {
+	tests := []struct {
+		name       string
+		ownerAlias string
+		owners     []string
+		wantErr    string
+	}{
+		{name: "self against an own image", ownerAlias: byIDAccount, owners: []string{"self"}},
+		{name: "own numeric account", ownerAlias: byIDAccount, owners: []string{byIDAccount}},
+		{name: "system aliases against a system image", ownerAlias: "amazon",
+			owners: []string{"system", "aws-marketplace"}},
+		{name: "another account against an own image", ownerAlias: byIDAccount, owners: []string{byIDOther},
+			wantErr: awserrors.ErrorInvalidAMIIDNotFound},
+		{name: "self against a system image", ownerAlias: "amazon", owners: []string{"self"},
+			wantErr: awserrors.ErrorInvalidAMIIDNotFound},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			store := recordingstore.New()
+			svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket)
+			putByIDAMI(t, store, byIDAMIDoc(byIDImage, tt.ownerAlias))
+
+			out, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{
+				ImageIds: aws.StringSlice([]string{byIDImage}),
+				Owners:   aws.StringSlice(tt.owners),
+			}, byIDAccount)
+
+			if tt.wantErr != "" {
+				require.Error(t, err)
+				assert.Equal(t, tt.wantErr, err.Error())
+				return
+			}
+			require.NoError(t, err)
+			assert.Len(t, out.Images, 1)
+		})
+	}
+}
+
+// A filter applies to a directly-fetched document exactly as it did to a listed
+// one, tags included, and a named image a filter excludes is not-found.
+func TestDescribeImagesByID_AttributeFilters(t *testing.T) {
+	tests := []struct {
+		name        string
+		filterName  string
+		filterValue string
+		wantErr     string
+	}{
+		{name: "matching name", filterName: "name", filterValue: "debian-13"},
+		{name: "matching state", filterName: "state", filterValue: "available"},
+		{name: "matching tag", filterName: "tag:Name", filterValue: "base"},
+		{name: "non-matching name", filterName: "name", filterValue: "ubuntu-24",
+			wantErr: awserrors.ErrorInvalidAMIIDNotFound},
+		{name: "non-matching architecture", filterName: "architecture", filterValue: "arm64",
+			wantErr: awserrors.ErrorInvalidAMIIDNotFound},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			store := recordingstore.New()
+			svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket)
+			putByIDAMI(t, store, byIDAMIDoc(byIDImage, byIDAccount))
+
+			out, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{
+				ImageIds: aws.StringSlice([]string{byIDImage}),
+				Filters: []*ec2.Filter{{
+					Name:   aws.String(tt.filterName),
+					Values: aws.StringSlice([]string{tt.filterValue}),
+				}},
+			}, byIDAccount)
+
+			if tt.wantErr != "" {
+				require.Error(t, err)
+				assert.Equal(t, tt.wantErr, err.Error())
+				return
+			}
+			require.NoError(t, err)
+			assert.Len(t, out.Images, 1)
+		})
+	}
+}
+
+// The enumerating path skips a document it cannot decode so one bad AMI cannot
+// hide every other image. A named image is never one of those others, so its
+// unreadable document fails the call instead of reporting it as absent.
+func TestDescribeImagesByID_CorruptDocumentFailsTheCall(t *testing.T) {
+	store := recordingstore.New()
+	svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket)
+	_, err := store.PutObject(t.Context(), &s3.PutObjectInput{
+		Bucket: aws.String(byIDBucket),
+		Key:    aws.String(amiKey(t, byIDImage)),
+		Body:   strings.NewReader("not-json"),
+	})
+	require.NoError(t, err)
+
+	_, err = svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{
+		ImageIds: aws.StringSlice([]string{byIDImage}),
+	}, byIDAccount)
+
+	require.Error(t, err)
+	assert.Equal(t, awserrors.ErrorServerInternal, err.Error(),
+		"a named image that cannot be decoded must not report as not-found")
+}
+
+// getFailingStore fails the read of one key with an error that is not a missing
+// key, standing in for a store that is reachable but unwell.
+type getFailingStore struct {
+	*objectstore.MemoryObjectStore
+
+	failKey string
+}
+
+func (s *getFailingStore) GetObject(ctx context.Context, input *s3.GetObjectInput) (*s3.GetObjectOutput, error) {
+	if aws.StringValue(input.Key) == s.failKey {
+		return nil, errors.New("connection reset by peer")
+	}
+	return s.MemoryObjectStore.GetObject(ctx, input)
+}
+
+// A store failure is not absence: reporting not-found here would tell a caller
+// its image is deregistered on the strength of a broken connection.
+func TestDescribeImagesByID_StoreErrorIsInternal(t *testing.T) {
+	memory := objectstore.NewMemoryObjectStore()
+	store := &getFailingStore{MemoryObjectStore: memory, failKey: amiKey(t, byIDImage)}
+	svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket)
+	putByIDAMI(t, memory, byIDAMIDoc(byIDImage, byIDAccount))
+
+	_, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{
+		ImageIds: aws.StringSlice([]string{byIDImage}),
+	}, byIDAccount)
+
+	require.Error(t, err)
+	assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
+}
+
+// A store failure anywhere in the fan-out outranks a missing ID, whatever order
+// the request named them in. Answering not-found here would tell a caller its
+// image was deregistered on the strength of a broken connection.
+func TestDescribeImagesByID_StoreErrorOutranksAMissingID(t *testing.T) {
+	memory := objectstore.NewMemoryObjectStore()
+	store := &getFailingStore{MemoryObjectStore: memory, failKey: amiKey(t, byIDImage)}
+	svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket)
+	putByIDAMI(t, memory, byIDAMIDoc(byIDImage, byIDAccount))
+
+	_, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{
+		ImageIds: aws.StringSlice([]string{"ami-never-registered", byIDImage}),
+	}, byIDAccount)
+
+	require.Error(t, err)
+	assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
+}
+
+// A caller that has already given up gets an error, never an empty result that
+// reads as "this image does not exist".
+func TestDescribeImagesByID_CancelledContextFails(t *testing.T) {
+	store := recordingstore.New()
+	svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket)
+	putByIDAMI(t, store, byIDAMIDoc(byIDImage, byIDAccount))
+	ctx, cancel := context.WithCancel(t.Context())
+	cancel()
+
+	out, err := svc.DescribeImages(ctx, &ec2.DescribeImagesInput{
+		ImageIds: aws.StringSlice([]string{byIDImage}),
+	}, byIDAccount)
+
+	require.Error(t, err)
+	assert.Nil(t, out)
+	assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
+}
+
+// A request whose only IDs are nil names nothing, which is an empty result
+// rather than a missing image, and reads nothing to find that out.
+func TestDescribeImagesByID_OnlyNilIDs(t *testing.T) {
+	store := recordingstore.New()
+	svc := handlers_ec2_image.NewImageServiceImplWithStore(store, byIDBucket)
+	putByIDAMI(t, store, byIDAMIDoc(byIDImage, byIDAccount))
+
+	out, err := svc.DescribeImages(t.Context(), &ec2.DescribeImagesInput{ImageIds: []*string{nil}}, byIDAccount)
+
+	require.NoError(t, err)
+	assert.Empty(t, out.Images)
+	assert.Empty(t, store.Gets())
+	assert.Empty(t, store.ListPrefixes())
+}

spinifex/handlers/ec2/image/service_impl.go

@@ -138,129 +138,29 @@ func (s *ImageServiceImpl) DescribeImages(ctx context.Context, input *ec2.Descri
 	return s.describeImages(ctx, input, accountID, parsedFilters)
 }
 
-// describeImages enumerates AMIs via Store.ListAMIs, then filters and renders
-// each document into the AWS SDK image shape.
+// describeImages renders the AWS SDK image shape for every AMI the caller may
+// see. Named IDs read their own documents; an unnamed request enumerates the
+// whole catalog via Store.ListAMIs, which is proportional to its own answer.
 func (s *ImageServiceImpl) describeImages(ctx context.Context, input *ec2.DescribeImagesInput, accountID string, parsedFilters map[string][]string) (*ec2.DescribeImagesOutput, error) {
+	encryptedAtRest := s.clusterEncryptionEnabled()
+
+	if len(input.ImageIds) > 0 {
+		return s.describeImagesByIDs(ctx, input, accountID, parsedFilters, encryptedAtRest)
+	}
+
 	amis, err := s.metadata.ListAMIs(ctx)
 	if err != nil {
 		slog.ErrorContext(ctx, "DescribeImages: failed to list AMIs", "err", err)
 		return nil, errors.New(awserrors.ErrorServerInternal)
 	}
 
-	encryptedAtRest := s.clusterEncryptionEnabled()
 	var images []*ec2.Image
-
 	for _, amiMeta := range amis {
 		if amiMeta.ImageID == "" {
 			continue
 		}
-
-		if len(input.ImageIds) > 0 {
-			found := false
-			for _, filterID := range input.ImageIds {
-				if filterID != nil && *filterID == amiMeta.ImageID {
-					found = true
-					break
-				}
-			}
-			if !found {
-				continue
-			}
-		}
-
-		amiOwner := amiMeta.ImageOwnerAlias
-		isSystemAMI := amiOwner != "" && !utils.IsAccountID(amiOwner)
-
-		// Resolved up front so the owner filter below compares against the same
-		// value the OwnerId field reports, rather than the raw alias string a
-		// system AMI carries instead of a numeric account ID.
-		ownerID := amiOwner
-		if isSystemAMI {
-			ownerID = utils.GlobalAccountID
-		}
-
-		if !callerCanReadAMI(amiMeta, accountID) {
-			continue
-		}
-
-		if len(input.Owners) > 0 {
-			found := false
-			for _, owner := range input.Owners {
-				if owner == nil {
-					continue
-				}
-				switch *owner {
-				case "self":
-					if ownerID == accountID {
-						found = true
-					}
-				case "amazon", "system", "aws-marketplace":
-					if isSystemAMI {
-						found = true
-					}
-				default:
-					if ownerID == *owner {
-						found = true
-					}
-				}
-				if found {
-					break
-				}
-			}
-			if !found {
-				continue
-			}
-		}
-
-		image := &ec2.Image{
-			ImageId:            aws.String(amiMeta.ImageID),
-			Name:               aws.String(amiMeta.Name),
-			Description:        aws.String(amiMeta.Description),
-			Architecture:       aws.String(amiMeta.Architecture),
-			PlatformDetails:    aws.String(amiMeta.PlatformDetails),
-			Platform:           utils.PlatformFromDetails(amiMeta.PlatformDetails),
-			CreationDate:       aws.String(amiMeta.CreationDate.UTC().Format("2006-01-02T15:04:05.000Z")),
-			RootDeviceType:     aws.String(amiMeta.RootDeviceType),
-			VirtualizationType: aws.String(amiMeta.Virtualization),
-			ImageOwnerAlias:    aws.String(amiMeta.ImageOwnerAlias),
-			OwnerId:            aws.String(ownerID),
-			Public:             aws.Bool(false),
-			State:              aws.String(amiImageState(amiMeta.State)),
-			ImageType:          aws.String("machine"),
-			Hypervisor:         aws.String("xen"),
-		}
-
-		// RegisterImage takes BootMode as an optional input, so an AMI can
-		// record none. The enum has no empty member: omit it, don't emit "".
-		if amiMeta.BootMode != "" {
-			image.BootMode = aws.String(amiMeta.BootMode)
-		}
-
-		if bdms := synthesizeRootBlockDeviceMapping(amiMeta, encryptedAtRest); bdms != nil {
-			image.RootDeviceName = aws.String("/dev/sda1")
-			image.BlockDeviceMappings = bdms
-		}
-
-		image.Tags = utils.MapToEC2Tags(amiMeta.Tags)
-
-		if len(parsedFilters) > 0 && !imageMatchesFilters(image, parsedFilters, amiMeta.Tags) {
-			continue
-		}
-
-		images = append(images, image)
-	}
-
-	if len(input.ImageIds) > 0 {
-		foundIDs := make(map[string]bool, len(images))
-		for _, img := range images {
-			if img.ImageId != nil {
-				foundIDs[*img.ImageId] = true
-			}
-		}
-		for _, reqID := range input.ImageIds {
-			if reqID != nil && !foundIDs[*reqID] {
-				return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound)
-			}
+		if image := renderVisibleImage(amiMeta, input, accountID, parsedFilters, encryptedAtRest); image != nil {
+			images = append(images, image)
 		}
 	}
 
@@ -268,6 +168,164 @@ func (s *ImageServiceImpl) describeImages(ctx context.Context, input *ec2.Descri
 	return &ec2.DescribeImagesOutput{Images: images}, nil
 }
 
+// describeImagesByIDs reads each named AMI's document directly. Enumerating
+// instead would scale with the unpartitioned AMI prefix, so with every other
+// tenant's image count.
+//
+// The key carries no tenant scoping, so every visibility rule is applied after
+// the fetch, in the order the enumerating path applies them.
+func (s *ImageServiceImpl) describeImagesByIDs(ctx context.Context, input *ec2.DescribeImagesInput, accountID string,
+	parsedFilters map[string][]string, encryptedAtRest bool) (*ec2.DescribeImagesOutput, error) {
+	// Only nil entries names nothing, which the enumerating path answers with an
+	// empty result rather than a missing ID.
+	imageIDs := utils.DistinctIDs(input.ImageIds)
+	if len(imageIDs) == 0 {
+		return &ec2.DescribeImagesOutput{}, nil
+	}
+
+	for _, imageID := range imageIDs {
+		// An ID no key can be built for names no document, which is also what
+		// the enumerating path reports for it.
+		if _, err := ebsmetadata.AMIKey(imageID); err != nil {
+			return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound)
+		}
+	}
+
+	results := ebsmetadata.FetchByIDs(ctx, imageIDs, s.metadata.GetAMI)
+
+	// A read that failed once the deadline passed is not evidence the AMI is
+	// absent, which is the only other thing this path reports.
+	if err := ctx.Err(); err != nil {
+		slog.WarnContext(ctx, "DescribeImages gave up reading AMI documents", "requested", len(imageIDs), "err", err)
+		return nil, errors.New(awserrors.ErrorServerInternal)
+	}
+
+	// A failed read outranks a missing one however the request ordered them, so
+	// a broken store is never reported as a deregistered image, and every
+	// failure is logged rather than only the one that happens to come first.
+	var readErr error
+	for i, result := range results {
+		if result.Err != nil && !objectstore.IsNoSuchKeyError(result.Err) {
+			slog.ErrorContext(ctx, "DescribeImages: failed to read AMI document", "imageId", imageIDs[i], "err", result.Err)
+			readErr = result.Err
+		}
+	}
+	if readErr != nil {
+		return nil, errors.New(awserrors.ErrorServerInternal)
+	}
+
+	images := make([]*ec2.Image, 0, len(imageIDs))
+	for i, result := range results {
+		// Every other store error was classified above, so this is a missing key.
+		if result.Err != nil {
+			return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound)
+		}
+
+		// The document is keyed by its own ID, and the enumerating path matches
+		// on this field. The comparison stays as an assertion.
+		if result.Document.ImageID != imageIDs[i] {
+			return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound)
+		}
+
+		// A named ID no rule lets through is not-found, never an empty success.
+		image := renderVisibleImage(result.Document, input, accountID, parsedFilters, encryptedAtRest)
+		if image == nil {
+			return nil, errors.New(awserrors.ErrorInvalidAMIIDNotFound)
+		}
+		images = append(images, image)
+	}
+
+	slog.InfoContext(ctx, "DescribeImages completed", "count", len(images))
+	return &ec2.DescribeImagesOutput{Images: images}, nil
+}
+
+// renderVisibleImage applies the DescribeImages visibility rules to one AMI
+// document and renders the SDK shape, or reports nil for a document the caller
+// must not see, an owner that does not match, or a filter that excludes it.
+func renderVisibleImage(amiMeta ebsmetadata.AMI, input *ec2.DescribeImagesInput, accountID string,
+	parsedFilters map[string][]string, encryptedAtRest bool) *ec2.Image {
+	amiOwner := amiMeta.ImageOwnerAlias
+	isSystemAMI := amiOwner != "" && !utils.IsAccountID(amiOwner)
+
+	// Resolved up front so the owner filter below compares against the same
+	// value the OwnerId field reports, rather than the raw alias string a
+	// system AMI carries instead of a numeric account ID.
+	ownerID := amiOwner
+	if isSystemAMI {
+		ownerID = utils.GlobalAccountID
+	}
+
+	if !callerCanReadAMI(amiMeta, accountID) {
+		return nil
+	}
+
+	if len(input.Owners) > 0 {
+		found := false
+		for _, owner := range input.Owners {
+			if owner == nil {
+				continue
+			}
+			switch *owner {
+			case "self":
+				if ownerID == accountID {
+					found = true
+				}
+			case "amazon", "system", "aws-marketplace":
+				if isSystemAMI {
+					found = true
+				}
+			default:
+				if ownerID == *owner {
+					found = true
+				}
+			}
+			if found {
+				break
+			}
+		}
+		if !found {
+			return nil
+		}
+	}
+
+	image := &ec2.Image{
+		ImageId:            aws.String(amiMeta.ImageID),
+		Name:               aws.String(amiMeta.Name),
+		Description:        aws.String(amiMeta.Description),
+		Architecture:       aws.String(amiMeta.Architecture),
+		PlatformDetails:    aws.String(amiMeta.PlatformDetails),
+		Platform:           utils.PlatformFromDetails(amiMeta.PlatformDetails),
+		CreationDate:       aws.String(amiMeta.CreationDate.UTC().Format("2006-01-02T15:04:05.000Z")),
+		RootDeviceType:     aws.String(amiMeta.RootDeviceType),
+		VirtualizationType: aws.String(amiMeta.Virtualization),
+		ImageOwnerAlias:    aws.String(amiMeta.ImageOwnerAlias),
+		OwnerId:            aws.String(ownerID),
+		Public:             aws.Bool(false),
+		State:              aws.String(amiImageState(amiMeta.State)),
+		ImageType:          aws.String("machine"),
+		Hypervisor:         aws.String("xen"),
+	}
+
+	// RegisterImage takes BootMode as an optional input, so an AMI can
+	// record none. The enum has no empty member: omit it, don't emit "".
+	if amiMeta.BootMode != "" {
+		image.BootMode = aws.String(amiMeta.BootMode)
+	}
+
+	if bdms := synthesizeRootBlockDeviceMapping(amiMeta, encryptedAtRest); bdms != nil {
+		image.RootDeviceName = aws.String("/dev/sda1")
+		image.BlockDeviceMappings = bdms
+	}
+
+	image.Tags = utils.MapToEC2Tags(amiMeta.Tags)
+
+	if len(parsedFilters) > 0 && !imageMatchesFilters(image, parsedFilters, amiMeta.Tags) {
+		return nil
+	}
+
+	return image
+}
+
 // amiImageState maps AMIMetadata.State to the ec2.Image state string. An
 // empty State means the AMI was registered before the field existed, and
 // MUST report as "available" — those images are already complete and

spinifex/handlers/ec2/snapshot/describe_byid_test.go

@@ -0,0 +1,349 @@
+package handlers_ec2_snapshot_test
+
+import (
+	"context"
+	"errors"
+	"strconv"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/aws/aws-sdk-go/service/s3"
+	"github.com/mulgadc/spinifex/spinifex/awserrors"
+	"github.com/mulgadc/spinifex/spinifex/config"
+	"github.com/mulgadc/spinifex/spinifex/ebsmetadata"
+	handlers_ec2_snapshot "github.com/mulgadc/spinifex/spinifex/handlers/ec2/snapshot"
+	"github.com/mulgadc/spinifex/spinifex/objectstore"
+	"github.com/mulgadc/spinifex/spinifex/testutil/recordingstore"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	byIDBucket    = "test-bucket"
+	byIDAccount   = "111122223333"
+	byIDOther     = "444455556666"
+	byIDSnapshot  = "snap-target"
+	byIDSnapshots = "spinifex/ebsmetadata/v2/snapshots/"
+)
+
+// newByIDService builds a snapshot service over store, with no EBS provider:
+// every test here answers out of the metadata documents alone.
+func newByIDService(store objectstore.ObjectStore) *handlers_ec2_snapshot.SnapshotServiceImpl {
+	cfg := &config.Config{Predastore: config.PredastoreConfig{Bucket: byIDBucket}}
+	return handlers_ec2_snapshot.NewSnapshotServiceImplWithStore(cfg, store, nil)
+}
+
+func putByIDSnapshot(t *testing.T, store objectstore.ObjectStore, snapshot ebsmetadata.Snapshot) {
+	t.Helper()
+	require.NoError(t, ebsmetadata.NewStore(store, byIDBucket).PutSnapshot(context.Background(), snapshot))
+}
+
+// byIDSnapshotDoc is a complete, readable snapshot document owned by ownerID.
+func byIDSnapshotDoc(snapshotID, ownerID string) ebsmetadata.Snapshot {
+	return ebsmetadata.Snapshot{
+		SnapshotID: snapshotID, VolumeID: "vol-source", VolumeSize: 8, State: "completed",
+		Progress: "100%", OwnerID: ownerID, StartTime: time.Now().UTC(),
+		Tags: map[string]string{"Name": "nightly"},
+	}
+}
+
+// seedByIDCatalog grows one account's catalog with documents no request names,
+// so a by-ID describe's cost can be measured against a catalog it must ignore.
+func seedByIDCatalog(t *testing.T, store objectstore.ObjectStore, ownerID string, n int) {
+	t.Helper()
+	for i := range n {
+		putByIDSnapshot(t, store, byIDSnapshotDoc("snap-bulk"+strconv.Itoa(i), ownerID))
+	}
+}
+
+func snapshotKey(t *testing.T, accountID, snapshotID string) string {
+	t.Helper()
+	key, err := ebsmetadata.SnapshotKey(accountID, snapshotID)
+	require.NoError(t, err)
+	return key
+}
+
+// A named snapshot costs one document read and no listing, whatever the account
+// owns. The cost of the old path was the whole catalog, so the count is the
+// assertion, not the answer.
+func TestDescribeSnapshotsByID_ReadsOnlyTheNamedDocument(t *testing.T) {
+	for _, catalog := range []int{1, 500} {
+		t.Run("catalog"+strconv.Itoa(catalog), func(t *testing.T) {
+			store := recordingstore.New()
+			svc := newByIDService(store)
+			putByIDSnapshot(t, store, byIDSnapshotDoc(byIDSnapshot, byIDAccount))
+			seedByIDCatalog(t, store, byIDAccount, catalog)
+
+			out, err := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{
+				SnapshotIds: aws.StringSlice([]string{byIDSnapshot}),
+			}, byIDAccount)
+
+			require.NoError(t, err)
+			require.Len(t, out.Snapshots, 1)
+			assert.Equal(t, byIDSnapshot, aws.StringValue(out.Snapshots[0].SnapshotId))
+			assert.Empty(t, store.ListPrefixes(), "a named snapshot must not list a prefix")
+			assert.Equal(t, []string{snapshotKey(t, byIDAccount, byIDSnapshot)}, store.Gets(),
+				"a named snapshot must read its own document and nothing else")
+		})
+	}
+}
+
+// Naming the same snapshot twice is one read and one result: the listing path
+// matched each document once however often the request named it.
+func TestDescribeSnapshotsByID_MultipleAndDuplicateIDs(t *testing.T) {
+	store := recordingstore.New()
+	svc := newByIDService(store)
+	putByIDSnapshot(t, store, byIDSnapshotDoc("snap-a", byIDAccount))
+	putByIDSnapshot(t, store, byIDSnapshotDoc("snap-b", byIDAccount))
+
+	out, err := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{
+		SnapshotIds: aws.StringSlice([]string{"snap-a", "snap-b", "snap-a"}),
+	}, byIDAccount)
+
+	require.NoError(t, err)
+	require.Len(t, out.Snapshots, 2)
+	assert.Equal(t, "snap-a", aws.StringValue(out.Snapshots[0].SnapshotId))
+	assert.Equal(t, "snap-b", aws.StringValue(out.Snapshots[1].SnapshotId))
+	assert.Len(t, store.Gets(), 2, "a duplicated ID must not be read twice")
+	assert.Empty(t, store.ListPrefixes())
+}
+
+// Another account's snapshot and a snapshot that was never created are the same
+// answer, and neither reads outside the caller's prefix.
+func TestDescribeSnapshotsByID_CrossTenantAndMissingAreIndistinguishable(t *testing.T) {
+	store := recordingstore.New()
+	svc := newByIDService(store)
+	putByIDSnapshot(t, store, byIDSnapshotDoc("snap-theirs", byIDOther))
+
+	crossTenant, crossErr := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{
+		SnapshotIds: aws.StringSlice([]string{"snap-theirs"}),
+	}, byIDAccount)
+	missing, missingErr := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{
+		SnapshotIds: aws.StringSlice([]string{"snap-never-created"}),
+	}, byIDAccount)
+
+	require.Error(t, crossErr)
+	require.Error(t, missingErr)
+	assert.Nil(t, crossTenant)
+	assert.Nil(t, missing)
+	assert.Equal(t, awserrors.ErrorInvalidSnapshotNotFound, crossErr.Error())
+	assert.Equal(t, missingErr.Error(), crossErr.Error(),
+		"another account's snapshot must be indistinguishable from one that does not exist")
+	for _, key := range store.Gets() {
+		assert.True(t, strings.HasPrefix(key, byIDSnapshots+byIDAccount+"/"),
+			"a by-ID read must stay inside the caller's prefix, read %q", key)
+	}
+}
+
+// OwnerIds selecting only another account names snapshots the caller's prefix
+// cannot hold, so the named ID is absent without any read at all.
+func TestDescribeSnapshotsByID_OwnerIDsFilter(t *testing.T) {
+	tests := []struct {
+		name     string
+		ownerIDs []string
+		wantErr  string
+		wantGets int
+	}{
+		{name: "self", ownerIDs: []string{"self"}, wantGets: 1},
+		{name: "own numeric account", ownerIDs: []string{byIDAccount}, wantGets: 1},
+		{name: "another account", ownerIDs: []string{byIDOther},
+			wantErr: awserrors.ErrorInvalidSnapshotNotFound, wantGets: 0},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			store := recordingstore.New()
+			svc := newByIDService(store)
+			putByIDSnapshot(t, store, byIDSnapshotDoc(byIDSnapshot, byIDAccount))
+
+			out, err := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{
+				SnapshotIds: aws.StringSlice([]string{byIDSnapshot}),
+				OwnerIds:    aws.StringSlice(tt.ownerIDs),
+			}, byIDAccount)
+
+			if tt.wantErr != "" {
+				require.Error(t, err)
+				assert.Equal(t, tt.wantErr, err.Error())
+			} else {
+				require.NoError(t, err)
+				assert.Len(t, out.Snapshots, 1)
+			}
+			assert.Len(t, store.Gets(), tt.wantGets)
+		})
+	}
+}
+
+// A filter applies to a directly-fetched document exactly as it did to a listed
+// one, and a named snapshot a filter excludes is not-found rather than an empty
+// success.
+func TestDescribeSnapshotsByID_AttributeFilters(t *testing.T) {
+	tests := []struct {
+		name        string
+		filterName  string
+		filterValue string
+		wantErr     string
+	}{
+		{name: "matching status", filterName: "status", filterValue: "completed"},
+		{name: "matching tag", filterName: "tag:Name", filterValue: "nightly"},
+		{name: "non-matching status", filterName: "status", filterValue: "pending",
+			wantErr: awserrors.ErrorInvalidSnapshotNotFound},
+		{name: "non-matching volume", filterName: "volume-id", filterValue: "vol-other",
+			wantErr: awserrors.ErrorInvalidSnapshotNotFound},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			store := recordingstore.New()
+			svc := newByIDService(store)
+			putByIDSnapshot(t, store, byIDSnapshotDoc(byIDSnapshot, byIDAccount))
+
+			out, err := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{
+				SnapshotIds: aws.StringSlice([]string{byIDSnapshot}),
+				Filters: []*ec2.Filter{{
+					Name:   aws.String(tt.filterName),
+					Values: aws.StringSlice([]string{tt.filterValue}),
+				}},
+			}, byIDAccount)
+
+			if tt.wantErr != "" {
+				require.Error(t, err)
+				assert.Equal(t, tt.wantErr, err.Error())
+				return
+			}
+			require.NoError(t, err)
+			assert.Len(t, out.Snapshots, 1)
+		})
+	}
+}
+
+// The tolerant variant's skip exists to stop one bad document hiding unrelated
+// snapshots. A specifically-named snapshot is never unrelated, so an unreadable
+// document for it fails both variants rather than reporting it as absent.
+func TestDescribeSnapshotsByID_CorruptDocumentFailsBothVariants(t *testing.T) {
+	store := recordingstore.New()
+	svc := newByIDService(store)
+	_, err := store.PutObject(t.Context(), &s3.PutObjectInput{
+		Bucket: aws.String(byIDBucket),
+		Key:    aws.String(snapshotKey(t, byIDAccount, byIDSnapshot)),
+		Body:   strings.NewReader("not-json"),
+	})
+	require.NoError(t, err)
+	input := &ec2.DescribeSnapshotsInput{SnapshotIds: aws.StringSlice([]string{byIDSnapshot})}
+
+	_, tolerantErr := svc.DescribeSnapshots(t.Context(), input, byIDAccount)
+	_, strictErr := svc.DescribeSnapshotsStrict(t.Context(), input, byIDAccount)
+
+	require.Error(t, tolerantErr)
+	require.Error(t, strictErr)
+	assert.Equal(t, awserrors.ErrorServerInternal, tolerantErr.Error(),
+		"a named snapshot that cannot be decoded must not report as not-found")
+	assert.ErrorIs(t, strictErr, ebsmetadata.ErrCorruptDocument,
+		"the strict variant must surface the cause on the fast path as it does on the listing path")
+}
+
+// A store failure anywhere in the fan-out outranks a missing ID, whatever order
+// the request named them in. Answering not-found here would tell a caller its
+// snapshot was deleted on the strength of a broken connection.
+func TestDescribeSnapshotsByID_StoreErrorOutranksAMissingID(t *testing.T) {
+	memory := objectstore.NewMemoryObjectStore()
+	store := &getFailingStore{MemoryObjectStore: memory, failKey: snapshotKey(t, byIDAccount, byIDSnapshot)}
+	svc := newByIDService(store)
+	putByIDSnapshot(t, memory, byIDSnapshotDoc(byIDSnapshot, byIDAccount))
+
+	input := &ec2.DescribeSnapshotsInput{
+		SnapshotIds: aws.StringSlice([]string{"snap-never-created", byIDSnapshot}),
+	}
+	_, tolerantErr := svc.DescribeSnapshots(t.Context(), input, byIDAccount)
+	_, strictErr := svc.DescribeSnapshotsStrict(t.Context(), input, byIDAccount)
+
+	require.Error(t, tolerantErr)
+	require.Error(t, strictErr)
+	assert.Equal(t, awserrors.ErrorServerInternal, tolerantErr.Error())
+	assert.Contains(t, strictErr.Error(), "connection reset by peer")
+}
+
+// The strict variant reports an untenanted caller as the listing path does,
+// rather than flattening the reason the read could not be attempted.
+func TestDescribeSnapshotsByID_UntenantedCallerIsInternalInBothVariants(t *testing.T) {
+	store := recordingstore.New()
+	svc := newByIDService(store)
+	input := &ec2.DescribeSnapshotsInput{SnapshotIds: aws.StringSlice([]string{byIDSnapshot})}
+
+	_, tolerantErr := svc.DescribeSnapshots(t.Context(), input, "")
+	_, strictErr := svc.DescribeSnapshotsStrict(t.Context(), input, "")
+
+	require.Error(t, tolerantErr)
+	require.Error(t, strictErr)
+	assert.Equal(t, awserrors.ErrorServerInternal, tolerantErr.Error())
+	assert.Contains(t, strictErr.Error(), "invalid EBS metadata account ID")
+	assert.Empty(t, store.Gets(), "an untenanted caller has no prefix to read under")
+}
+
+// getFailingStore fails the read of one key with an error that is not a missing
+// key, standing in for a store that is reachable but unwell.
+type getFailingStore struct {
+	*objectstore.MemoryObjectStore
+
+	failKey string
+}
+
+func (s *getFailingStore) GetObject(ctx context.Context, input *s3.GetObjectInput) (*s3.GetObjectOutput, error) {
+	if aws.StringValue(input.Key) == s.failKey {
+		return nil, errors.New("connection reset by peer")
+	}
+	return s.MemoryObjectStore.GetObject(ctx, input)
+}
+
+// A store failure is not absence: reporting not-found here would tell a caller
+// its snapshot is gone on the strength of a broken connection.
+func TestDescribeSnapshotsByID_StoreErrorIsInternal(t *testing.T) {
+	memory := objectstore.NewMemoryObjectStore()
+	store := &getFailingStore{MemoryObjectStore: memory, failKey: snapshotKey(t, byIDAccount, byIDSnapshot)}
+	svc := newByIDService(store)
+	putByIDSnapshot(t, memory, byIDSnapshotDoc(byIDSnapshot, byIDAccount))
+
+	_, err := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{
+		SnapshotIds: aws.StringSlice([]string{byIDSnapshot}),
+	}, byIDAccount)
+
+	require.Error(t, err)
+	assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
+}
+
+// A caller that has already given up gets an error, never an empty result that
+// reads as "this snapshot does not exist".
+func TestDescribeSnapshotsByID_CancelledContextFails(t *testing.T) {
+	store := recordingstore.New()
+	svc := newByIDService(store)
+	putByIDSnapshot(t, store, byIDSnapshotDoc(byIDSnapshot, byIDAccount))
+	ctx, cancel := context.WithCancel(t.Context())
+	cancel()
+
+	out, err := svc.DescribeSnapshots(ctx, &ec2.DescribeSnapshotsInput{
+		SnapshotIds: aws.StringSlice([]string{byIDSnapshot}),
+	}, byIDAccount)
+
+	require.Error(t, err)
+	assert.Nil(t, out)
+	assert.Equal(t, awserrors.ErrorServerInternal, err.Error())
+}
+
+// A request whose only IDs are nil names nothing, which is an empty result
+// rather than a missing snapshot, and reads nothing to find that out.
+func TestDescribeSnapshotsByID_OnlyNilIDs(t *testing.T) {
+	store := recordingstore.New()
+	svc := newByIDService(store)
+	putByIDSnapshot(t, store, byIDSnapshotDoc(byIDSnapshot, byIDAccount))
+
+	out, err := svc.DescribeSnapshots(t.Context(), &ec2.DescribeSnapshotsInput{
+		SnapshotIds: []*string{nil},
+	}, byIDAccount)
+
+	require.NoError(t, err)
+	assert.Empty(t, out.Snapshots)
+	assert.Empty(t, store.Gets())
+	assert.Empty(t, store.ListPrefixes())
+}

spinifex/handlers/ec2/snapshot/service_impl.go

@@ -451,27 +451,19 @@ func (s *SnapshotServiceImpl) describeSnapshots(ctx context.Context, input *ec2.
 
 	slog.InfoContext(ctx, "DescribeSnapshots request", "snapshotIds", input.SnapshotIds, "accountID", accountID)
 
-	snapshotIDFilter := make(map[string]bool)
-	for _, id := range input.SnapshotIds {
-		if id != nil {
-			snapshotIDFilter[*id] = true
-		}
-	}
-
 	parsedFilters, err := filterutil.ParseFilters(input.Filters, describeSnapshotsValidFilters)
 	if err != nil {
 		slog.WarnContext(ctx, "DescribeSnapshots: invalid filter", "err", err)
 		return nil, err
 	}
 
+	if len(input.SnapshotIds) > 0 {
+		return s.describeSnapshotsByIDs(ctx, input, accountID, parsedFilters, strict)
+	}
+
 	// The caller's prefix is the isolation and the whole answer: no other
-	// account's document is read, so none has to be filtered out afterwards.
-	// Named ids are matched against this listing rather than fetched directly —
-	// the listing already costs what the account owns.
-	//
-	// OwnerIds names either the caller, which the prefix already is, or another
-	// account, whose snapshots cannot be under it — so that case is empty without
-	// a read, and the tail below still reports a named id as not found.
+	// account's document is read, and OwnerIds either names the caller, whose
+	// prefix this is, or an account whose snapshots cannot be under it.
 	var configs []ebsmetadata.Snapshot
 	if ownerIDsIncludeCaller(input.OwnerIds, accountID) {
 		if strict {
@@ -499,10 +491,6 @@ func (s *SnapshotServiceImpl) describeSnapshots(ctx context.Context, input *ec2.
 
 	var snapshots []*ec2.Snapshot
 	for _, cfg := range configs {
-		if len(snapshotIDFilter) > 0 && !snapshotIDFilter[cfg.SnapshotID] {
-			continue
-		}
-
 		if len(parsedFilters) > 0 && !snapshotMatchesFilters(cfg, parsedFilters) {
 			continue
 		}
@@ -510,22 +498,108 @@ func (s *SnapshotServiceImpl) describeSnapshots(ctx context.Context, input *ec2.
 		snapshots = append(snapshots, snapshotConfigToEC2(cfg))
 	}
 
-	// Naming a specific, nonexistent snapshot ID is an error, unlike an
-	// unfiltered list or a --filters query that simply matches nothing.
-	if len(snapshotIDFilter) > 0 {
-		found := make(map[string]bool, len(snapshots))
-		for _, snap := range snapshots {
-			if snap.SnapshotId != nil {
-				found[*snap.SnapshotId] = true
-			}
+	slog.InfoContext(ctx, "DescribeSnapshots completed", "count", len(snapshots))
+
+	return &ec2.DescribeSnapshotsOutput{
+		Snapshots: snapshots,
+	}, nil
+}
+
+// describeSnapshotsByIDs reads each named snapshot's document directly, rather
+// than pay a fetch per snapshot the account owns however few it names.
+//
+// The key is scoped to the caller's account, so a cross-tenant ID reads a key
+// that does not exist and reports not-found — the same answer the listing path
+// gives, and for the same reason.
+//
+// strict keeps the control-plane contract the listing path has: the underlying
+// metadata failure is returned rather than flattened to an opaque code.
+func (s *SnapshotServiceImpl) describeSnapshotsByIDs(ctx context.Context, input *ec2.DescribeSnapshotsInput,
+	accountID string, parsedFilters map[string][]string, strict bool) (*ec2.DescribeSnapshotsOutput, error) {
+	// Only nil entries names nothing, which the listing path answers with an
+	// empty result rather than a missing ID.
+	snapshotIDs := utils.DistinctIDs(input.SnapshotIds)
+	if len(snapshotIDs) == 0 {
+		return &ec2.DescribeSnapshotsOutput{}, nil
+	}
+
+	// OwnerIds naming only another account selects snapshots the caller's
+	// prefix cannot contain, so a named ID is absent without any read.
+	if !ownerIDsIncludeCaller(input.OwnerIds, accountID) {
+		return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound)
+	}
+
+	// An untenanted caller has no prefix to read under; refuse here rather than
+	// report a snapshot that may well exist as absent. The key builder owns the
+	// wording, so the strict variant reports what the listing path reports.
+	if !utils.IsAccountID(accountID) {
+		_, keyErr := ebsmetadata.SnapshotKey(accountID, snapshotIDs[0])
+		slog.ErrorContext(ctx, "DescribeSnapshots refused a caller with no account", "accountID", accountID)
+		if strict {
+			return nil, fmt.Errorf("describe snapshots for %s: %w", accountID, keyErr)
 		}
-		for id := range snapshotIDFilter {
-			if !found[id] {
-				return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound)
-			}
+		return nil, errors.New(awserrors.ErrorServerInternal)
+	}
+
+	for _, snapshotID := range snapshotIDs {
+		// An ID no key can be built for names no document, which is also what
+		// the listing path reports for it.
+		if _, err := ebsmetadata.SnapshotKey(accountID, snapshotID); err != nil {
+			return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound)
 		}
 	}
 
+	results := ebsmetadata.FetchByIDs(ctx, snapshotIDs, func(ctx context.Context, id string) (ebsmetadata.Snapshot, error) {
+		return s.metadata.GetSnapshot(ctx, accountID, id)
+	})
+
+	// A read that failed once the deadline passed is not evidence the snapshot
+	// is absent, which is the only other thing this path reports.
+	if err := ctx.Err(); err != nil {
+		slog.WarnContext(ctx, "DescribeSnapshots gave up reading snapshot documents",
+			"requested", len(snapshotIDs), "err", err)
+		return nil, errors.New(awserrors.ErrorServerInternal)
+	}
+
+	// A named ID is never the unrelated document the tolerant listing's skip
+	// exists for, so an unreadable one fails both variants. A failed read also
+	// outranks a missing one however the request ordered them.
+	var readErr error
+	for i, result := range results {
+		if result.Err != nil && !objectstore.IsNoSuchKeyError(result.Err) {
+			slog.ErrorContext(ctx, "DescribeSnapshots failed to read a named snapshot document",
+				"snapshotId", snapshotIDs[i], "accountID", accountID, "err", result.Err)
+			readErr = result.Err
+		}
+	}
+	if readErr != nil {
+		if strict {
+			return nil, fmt.Errorf("describe snapshots for %s: %w", accountID, readErr)
+		}
+		return nil, errors.New(awserrors.ErrorServerInternal)
+	}
+
+	snapshots := make([]*ec2.Snapshot, 0, len(snapshotIDs))
+	for i, result := range results {
+		// Every other store error was classified above, so this is a missing key.
+		if result.Err != nil {
+			return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound)
+		}
+
+		// The read was keyed by the ID inside the caller's prefix, so the
+		// document is theirs and is the one named. Both stay as assertions.
+		if result.Document.SnapshotID != snapshotIDs[i] || result.Document.OwnerID != accountID {
+			return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound)
+		}
+
+		// A named snapshot a filter excludes is not-found, as on the listing path.
+		if len(parsedFilters) > 0 && !snapshotMatchesFilters(result.Document, parsedFilters) {
+			return nil, errors.New(awserrors.ErrorInvalidSnapshotNotFound)
+		}
+
+		snapshots = append(snapshots, snapshotConfigToEC2(result.Document))
+	}
+
 	slog.InfoContext(ctx, "DescribeSnapshots completed", "count", len(snapshots))
 
 	return &ec2.DescribeSnapshotsOutput{

spinifex/utils/utils.go

@@ -40,6 +40,22 @@ func GenerateResourceID(prefix string) string {
 	return prefix + "-" + hex.EncodeToString(b)[:17]
 }
 
+// DistinctIDs returns the distinct non-nil values of a request's resource-ID
+// slice, in request order. A describe that reads one document per named ID must
+// not read or report the same resource twice because the request listed it twice.
+func DistinctIDs(ids []*string) []string {
+	distinct := make([]string, 0, len(ids))
+	seen := make(map[string]bool, len(ids))
+	for _, id := range ids {
+		if id == nil || seen[*id] {
+			continue
+		}
+		seen[*id] = true
+		distinct = append(distinct, *id)
+	}
+	return distinct
+}
+
 func MarshalToXML(payload any) ([]byte, error) {
 	var buf bytes.Buffer
 	enc := xml.NewEncoder(&buf)

spinifex/utils/utils_test.go

@@ -26,6 +26,30 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// A describe that reads one document per named ID pays for every entry it is
+// given, so a repeated or nil ID has to be dropped before the fan-out and the
+// surviving order has to be the request's.
+func TestDistinctIDs(t *testing.T) {
+	first, second := "vol-1", "vol-2"
+	tests := []struct {
+		name string
+		ids  []*string
+		want []string
+	}{
+		{name: "nil slice", ids: nil, want: []string{}},
+		{name: "only nil entries", ids: []*string{nil, nil}, want: []string{}},
+		{name: "request order", ids: []*string{&second, &first}, want: []string{"vol-2", "vol-1"}},
+		{name: "duplicates", ids: []*string{&first, &second, &first}, want: []string{"vol-1", "vol-2"}},
+		{name: "nil among values", ids: []*string{&first, nil, &second}, want: []string{"vol-1", "vol-2"}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, DistinctIDs(tt.ids))
+		})
+	}
+}
+
 func TestGenerateResourceID(t *testing.T) {
 	tests := []struct {
 		prefix string