fix: refuse DeleteSnapshot while a registered AMI uses the snapshot (#1227)

DeleteSnapshot checked only volume documents, so a snapshot backing a
registered AMI deleted anyway and left the AMI pointing at nothing.
Add a strict, whole-cluster AMI read next to the existing volume-clone
check and refuse with InvalidSnapshot.InUse when a registered AMI
names the snapshot; an unreadable or undecodable AMI document fails
the delete closed with ServerInternal.

The e2e teardown sweep now deregisters images before deleting
snapshots, since a snapshot sweep that runs first would now be
blocked by its AMI.

Known residual, out of scope here: RegisterImage reading the snapshot
and DeleteSnapshot's clone/AMI checks are not atomic, so a concurrent
RegisterImage can still race a DeleteSnapshot that passed its checks.

spinifex/domains/ec2/image/account_termination_current_behaviour_test.go

@@ -124,9 +124,10 @@ func TestCurrentBehaviour_AccountTermination_CreateImageSnapshotDoesNotPinSource
 	assert.Equal(t, "vol-imaged", source, "a launch from the image still resolves the deleted volume")
 }
 
-// DeleteSnapshot consults volumes cloned from the snapshot, not images
-// registered on it, so the snapshot under a live image deletes.
-func TestCurrentBehaviour_AccountTermination_DeleteSnapshotIgnoresRegisteredImage(t *testing.T) {
+// DeleteSnapshot consults AMI documents as well as volumes cloned from the
+// snapshot, so the snapshot under a live image refuses to delete until the
+// image is deregistered.
+func TestAccountTermination_DeleteSnapshotRefusesARegisteredImage(t *testing.T) {
 	s := newStorageServices(t)
 	ctx := context.Background()
 
@@ -146,16 +147,18 @@ func TestCurrentBehaviour_AccountTermination_DeleteSnapshotIgnoresRegisteredImag
 	require.NoError(t, err)
 
 	_, err = s.snapshots.DeleteSnapshot(ctx, &ec2.DeleteSnapshotInput{SnapshotId: aws.String(snapshotID)}, ctAccountID)
-	require.NoError(t, err, "the snapshot deletes while an image is registered on it")
+	assert.EqualError(t, err, awserrors.ErrorInvalidSnapshotInUse, "the snapshot refuses to delete while an image is registered on it")
 
-	images, err := s.images.DescribeImages(ctx, &ec2.DescribeImagesInput{ImageIds: []*string{registered.ImageId}}, ctAccountID)
+	_, err = s.metadata.GetSnapshot(ctx, ctAccountID, snapshotID)
+	assert.NoError(t, err, "the snapshot document is still present")
+	_, held := s.provider.Snapshot(snapshotID)
+	assert.True(t, held, "the provider still holds the snapshot")
+
+	_, err = s.images.DeregisterImage(ctx, &ec2.DeregisterImageInput{ImageId: registered.ImageId}, ctAccountID)
 	require.NoError(t, err)
-	require.Len(t, images.Images, 1)
-	assert.Equal(t, ec2.ImageStateAvailable, aws.StringValue(images.Images[0].State), "the image still reports available")
 
-	_, err = s.images.GetAMISourceVolumeID(ctx, aws.StringValue(registered.ImageId))
-	assert.EqualError(t, err, awserrors.ErrorInvalidSnapshotNotFound, "a launch from the image can no longer resolve its snapshot")
-	require.NoError(t, s.deleteVolume("vol-registered"), "with the snapshot gone, the source volume deletes too")
+	_, err = s.snapshots.DeleteSnapshot(ctx, &ec2.DeleteSnapshotInput{SnapshotId: aws.String(snapshotID)}, ctAccountID)
+	require.NoError(t, err, "the snapshot deletes once the image is deregistered")
 }
 
 // deleteDuringClone runs a hook once, after the provider has accepted a clone

spinifex/domains/ec2/snapshot/service_impl.go

@@ -685,6 +685,25 @@ func (s *SnapshotServiceImpl) snapshotInUseByVolumes(ctx context.Context, snapsh
 	return false, nil
 }
 
+// amiInUseBySnapshot checks if any registered AMI names the given snapshot as
+// its root device. AWS refuses DeleteSnapshot while a registered AMI
+// references the snapshot, the same way it refuses one backing a volume.
+func (s *SnapshotServiceImpl) amiInUseBySnapshot(ctx context.Context, snapshotID string) (bool, error) {
+	// Whole-cluster and strict for the same reason as the volume check above:
+	// a system AMI can be registered by an account other than the snapshot's
+	// owner, and an AMI this cannot read is not evidence that none exists.
+	amis, err := s.metadata.ListAMIsStrict(ctx)
+	if err != nil {
+		return false, fmt.Errorf("amiInUseBySnapshot: failed to list AMIs: %w", err)
+	}
+	for _, ami := range amis {
+		if ami.SnapshotID == snapshotID {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
 // DeleteSnapshot deletes a snapshot after verifying the caller owns it.
 func (s *SnapshotServiceImpl) DeleteSnapshot(ctx context.Context, input *ec2.DeleteSnapshotInput, accountID string) (*ec2.DeleteSnapshotOutput, error) {
 	if input == nil || input.SnapshotId == nil {
@@ -724,6 +743,17 @@ func (s *SnapshotServiceImpl) DeleteSnapshot(ctx context.Context, input *ec2.Del
 		slog.InfoContext(ctx, "DeleteSnapshot blocked: snapshot in use by volume", "snapshotId", snapshotID)
 		return nil, errors.New(awserrors.ErrorInvalidSnapshotInUse)
 	}
+
+	// Check if any registered AMI names this snapshot as its root device
+	amiInUse, err := s.amiInUseBySnapshot(ctx, snapshotID)
+	if err != nil {
+		slog.ErrorContext(ctx, "DeleteSnapshot failed to check AMI usage", "snapshotId", snapshotID, "err", err)
+		return nil, errors.New(awserrors.ErrorServerInternal)
+	}
+	if amiInUse {
+		slog.InfoContext(ctx, "DeleteSnapshot blocked: snapshot in use by registered AMI", "snapshotId", snapshotID)
+		return nil, errors.New(awserrors.ErrorInvalidSnapshotInUse)
+	}
 	if s.provider == nil {
 		slog.ErrorContext(ctx, "no EBS provider configured", "op", "DeleteSnapshot")
 		return nil, errors.New(awserrors.ErrorServerInternal)

spinifex/domains/ec2/snapshot/service_impl_test.go

@@ -490,6 +490,101 @@ func TestDeleteSnapshot_InUseByVolume(t *testing.T) {
 	assert.Len(t, result.Snapshots, 1)
 }
 
+// TestDeleteSnapshot_InUseByRegisteredAMI tests that deleting a snapshot
+// fails while an AMI's own CreateImage snapshot still names it, matching AWS.
+func TestDeleteSnapshot_InUseByRegisteredAMI(t *testing.T) {
+	svc, store := setupTestSnapshotService(t)
+
+	createTestVolume(t, svc, store, "vol-source", 50)
+	snap, err := svc.CreateSnapshot(context.Background(), &ec2.CreateSnapshotInput{
+		VolumeId: aws.String("vol-source"),
+	}, testAccountID)
+	require.NoError(t, err)
+
+	require.NoError(t, svc.metadata.PutAMI(context.Background(), ebsmetadata.AMI{
+		ImageID: "ami-registered", Name: "ct-ami", SnapshotID: *snap.SnapshotId,
+	}))
+
+	_, err = svc.DeleteSnapshot(context.Background(), &ec2.DeleteSnapshotInput{
+		SnapshotId: snap.SnapshotId,
+	}, testAccountID)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), awserrors.ErrorInvalidSnapshotInUse)
+
+	// The provider and control-plane document both survive the refusal.
+	result, err := svc.DescribeSnapshots(context.Background(), &ec2.DescribeSnapshotsInput{
+		SnapshotIds: []*string{snap.SnapshotId},
+	}, testAccountID)
+	require.NoError(t, err)
+	assert.Len(t, result.Snapshots, 1)
+
+	require.NoError(t, svc.metadata.DeleteAMI(context.Background(), "ami-registered"))
+	_, err = svc.DeleteSnapshot(context.Background(), &ec2.DeleteSnapshotInput{
+		SnapshotId: snap.SnapshotId,
+	}, testAccountID)
+	require.NoError(t, err, "once the AMI is gone, the delete succeeds")
+}
+
+// TestDeleteSnapshot_InUseByRegisteredAMI_AnotherAccount tests that the AMI
+// check is whole-cluster: a system AMI registered by a different account than
+// the snapshot's owner still blocks the delete.
+func TestDeleteSnapshot_InUseByRegisteredAMI_AnotherAccount(t *testing.T) {
+	svc, store := setupTestSnapshotService(t)
+
+	createTestVolume(t, svc, store, "vol-source", 50)
+	snap, err := svc.CreateSnapshot(context.Background(), &ec2.CreateSnapshotInput{
+		VolumeId: aws.String("vol-source"),
+	}, testAccountID)
+	require.NoError(t, err)
+
+	require.NoError(t, svc.metadata.PutAMI(context.Background(), ebsmetadata.AMI{
+		ImageID: "ami-other-account", Name: "ct-system-ami",
+		ImageOwnerAlias: otherAccountID, SnapshotID: *snap.SnapshotId,
+	}))
+
+	_, err = svc.DeleteSnapshot(context.Background(), &ec2.DeleteSnapshotInput{
+		SnapshotId: snap.SnapshotId,
+	}, testAccountID)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), awserrors.ErrorInvalidSnapshotInUse)
+}
+
+// TestDeleteSnapshot_AMIReadFailureFailsClosed tests that an AMI document
+// this cannot decode aborts the delete rather than being treated as absent:
+// a partial AMI listing is not evidence that no AMI names the snapshot.
+func TestDeleteSnapshot_AMIReadFailureFailsClosed(t *testing.T) {
+	svc, store := setupTestSnapshotService(t)
+
+	createTestVolume(t, svc, store, "vol-source", 50)
+	snap, err := svc.CreateSnapshot(context.Background(), &ec2.CreateSnapshotInput{
+		VolumeId: aws.String("vol-source"),
+	}, testAccountID)
+	require.NoError(t, err)
+
+	key, err := ebsmetadata.AMIKey("ami-corrupt")
+	require.NoError(t, err)
+	_, err = store.PutObject(context.Background(), &s3.PutObjectInput{
+		Bucket: aws.String("test-bucket"),
+		Key:    aws.String(key),
+		Body:   strings.NewReader("not-json"),
+	})
+	require.NoError(t, err)
+
+	_, err = svc.DeleteSnapshot(context.Background(), &ec2.DeleteSnapshotInput{
+		SnapshotId: snap.SnapshotId,
+	}, testAccountID)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), awserrors.ErrorServerInternal)
+
+	// Nothing was deleted: the snapshot survives in both the provider and the
+	// control-plane document.
+	result, err := svc.DescribeSnapshots(context.Background(), &ec2.DescribeSnapshotsInput{
+		SnapshotIds: []*string{snap.SnapshotId},
+	}, testAccountID)
+	require.NoError(t, err)
+	assert.Len(t, result.Snapshots, 1)
+}
+
 // TestDeleteSnapshot_NotFound tests deleting a non-existent snapshot.
 func TestDeleteSnapshot_NotFound(t *testing.T) {
 	svc, _ := setupTestSnapshotService(t)

tests/e2e/harness/sweep.go

@@ -78,8 +78,10 @@ func (r *SweepReport) errf(format string, args ...any) {
 }
 
 // SweepRunResources deletes every EC2/ELBv2 resource tagged e2e:run=<runID>,
-// in dependency order (load balancers → instances → NAT gateways → snapshots →
-// volumes → images → security groups → subnets → key pairs). It is idempotent
+// in dependency order (load balancers → instances → NAT gateways → images →
+// snapshots → volumes → security groups → subnets → key pairs). Images come
+// before snapshots: DeleteSnapshot now refuses while a registered AMI names
+// the snapshot, so its AMI has to be deregistered first. It is idempotent
 // and best-effort; callers inspect SweepReport but a non-empty Errors list is
 // not necessarily a run failure (resources may already be gone, or still
 // settling). A blank runID returns an empty report without touching anything.
@@ -92,9 +94,9 @@ func SweepRunResources(ec2c ec2iface.EC2API, elbc elbv2iface.ELBV2API, runID str
 	sweepLoadBalancers(elbc, runID, rep)
 	sweepInstances(ec2c, runID, rep)
 	sweepNatGateways(ec2c, runID, rep)
+	sweepImages(ec2c, runID, rep)
 	sweepSnapshots(ec2c, runID, rep)
 	sweepVolumes(ec2c, runID, rep)
-	sweepImages(ec2c, runID, rep)
 	sweepSecurityGroups(ec2c, runID, rep)
 	sweepSubnets(ec2c, runID, rep)
 	sweepKeyPairs(ec2c, runID, rep)