Fixing issue where deleted profiles being sent to devices.

server/datastore/mysql/common_mysql/batch.go

@@ -0,0 +1,26 @@
+package common_mysql
+
+// BatchProcessSimple is a simple utility function to batch process a slice of payloads.
+// Provide a slice of payloads, a batch size, and a function to execute on each batch.
+func BatchProcessSimple[T any](
+	payloads []T,
+	batchSize int,
+	executeBatch func(payloadsInThisBatch []T) error,
+) error {
+	if len(payloads) == 0 || batchSize <= 0 || executeBatch == nil {
+		return nil
+	}
+
+	for i := 0; i < len(payloads); i += batchSize {
+		start := i
+		end := i + batchSize
+		if end > len(payloads) {
+			end = len(payloads)
+		}
+		if err := executeBatch(payloads[start:end]); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}

server/datastore/mysql/common_mysql/batch_test.go

@@ -0,0 +1,52 @@
+package common_mysql
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestBatchProcessSimple(t *testing.T) {
+	payloads := []int{1, 2, 3, 4, 5}
+	executeBatch := func(payloadsInThisBatch []int) error {
+		t.Fatal("executeBatch should not be called")
+		return nil
+	}
+
+	// No payloads
+	err := BatchProcessSimple(nil, 10, executeBatch)
+	assert.NoError(t, err)
+
+	// No batch size
+	err = BatchProcessSimple(payloads, 0, executeBatch)
+	assert.NoError(t, err)
+
+	// No executeBatch
+	err = BatchProcessSimple(payloads, 10, nil)
+	assert.NoError(t, err)
+
+	// Large batch size -- all payloads executed in one batch
+	executeBatch = func(payloadsInThisBatch []int) error {
+		assert.Equal(t, payloads, payloadsInThisBatch)
+		return nil
+	}
+	err = BatchProcessSimple(payloads, 10, executeBatch)
+	assert.NoError(t, err)
+
+	// Small batch size
+	numCalls := 0
+	executeBatch = func(payloadsInThisBatch []int) error {
+		numCalls++
+		switch numCalls {
+		case 1:
+			assert.Equal(t, []int{1, 2, 3}, payloadsInThisBatch)
+		case 2:
+			assert.Equal(t, []int{4, 5}, payloadsInThisBatch)
+		default:
+			t.Errorf("Unexpected number of calls to executeBatch: %d", numCalls)
+		}
+		return nil
+	}
+	err = BatchProcessSimple(payloads, 3, executeBatch)
+	assert.NoError(t, err)
+}

server/fleet/apple_mdm.go

@@ -330,10 +330,22 @@ type MDMAppleProfilePayload struct {
 
 // DidNotInstallOnHost indicates whether this profile was not installed on the host (and
 // therefore is not, as far as Fleet knows, currently on the host).
+// The profile in Pending status could be on the host, but Fleet has not received an Acknowledged status yet.
 func (p *MDMAppleProfilePayload) DidNotInstallOnHost() bool {
 	return p.Status != nil && (*p.Status == MDMDeliveryFailed || *p.Status == MDMDeliveryPending) && p.OperationType == MDMOperationTypeInstall
 }
 
+// FailedInstallOnHost indicates whether this profile failed to install on the host.
+func (p *MDMAppleProfilePayload) FailedInstallOnHost() bool {
+	return p.Status != nil && *p.Status == MDMDeliveryFailed && p.OperationType == MDMOperationTypeInstall
+}
+
+// PendingInstallOnHost indicates whether this profile is pending to install on the host.
+// The profile in Pending status could be on the host, but Fleet has not received an Acknowledged status yet.
+func (p *MDMAppleProfilePayload) PendingInstallOnHost() bool {
+	return p.Status != nil && *p.Status == MDMDeliveryPending && p.OperationType == MDMOperationTypeInstall
+}
+
 type MDMAppleBulkUpsertHostProfilePayload struct {
 	ProfileUUID       string
 	ProfileIdentifier string

server/mdm/apple/commander.go

@@ -425,6 +425,11 @@ func (svc *MDMAppleCommander) SendNotifications(ctx context.Context, hostUUIDs [
 	return nil
 }
 
+// BulkDeleteHostUserCommandsWithoutResults calls the storage method with the same name.
+func (svc *MDMAppleCommander) BulkDeleteHostUserCommandsWithoutResults(ctx context.Context, commandToIDs map[string][]string) error {
+	return svc.storage.BulkDeleteHostUserCommandsWithoutResults(ctx, commandToIDs)
+}
+
 // APNSDeliveryError records an error and the associated host UUIDs in which it
 // occurred.
 type APNSDeliveryError struct {

server/mdm/nanomdm/storage/allmulti/allmulti.go

@@ -104,3 +104,10 @@ func (ms *MultiAllStorage) ExpandEmbeddedSecrets(ctx context.Context, document s
 	})
 	return doc.(string), err
 }
+
+func (ms *MultiAllStorage) BulkDeleteHostUserCommandsWithoutResults(ctx context.Context, commandToIDs map[string][]string) error {
+	_, err := ms.execStores(ctx, func(s storage.AllStorage) (interface{}, error) {
+		return nil, s.BulkDeleteHostUserCommandsWithoutResults(ctx, commandToIDs)
+	})
+	return err
+}

server/mdm/nanomdm/storage/file/file.go

@@ -245,3 +245,8 @@ func (s *FileStorage) ExpandEmbeddedSecrets(_ context.Context, document string)
 	// NOT IMPLEMENTED
 	return document, nil
 }
+
+func (m *FileStorage) BulkDeleteHostUserCommandsWithoutResults(_ context.Context, _ map[string][]string) error {
+	// NOT IMPLEMENTED
+	return nil
+}

server/mdm/nanomdm/storage/mysql/queue.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/fleetdm/fleet/v4/server/contexts/ctxerr"
 	"github.com/fleetdm/fleet/v4/server/datastore/mysql/common_mysql"
 	"github.com/fleetdm/fleet/v4/server/mdm/nanomdm/mdm"
 	"github.com/google/uuid"
@@ -260,3 +261,54 @@ WHERE
 	)
 	return err
 }
+
+// BulkDeleteHostUserCommandsWithoutResults deletes all commands without results for the given host/user IDs.
+// This is used to clean up the queue when a profile is deleted from Fleet.
+func (m *MySQLStorage) BulkDeleteHostUserCommandsWithoutResults(ctx context.Context, commandToIDs map[string][]string) error {
+	if len(commandToIDs) == 0 {
+		return nil
+	}
+	return common_mysql.WithRetryTxx(ctx, sqlx.NewDb(m.db, ""), func(tx sqlx.ExtContext) error {
+		return m.bulkDeleteHostUserCommandsWithoutResults(ctx, tx, commandToIDs)
+	}, loggerWrapper{m.logger})
+}
+
+func (m *MySQLStorage) bulkDeleteHostUserCommandsWithoutResults(ctx context.Context, tx sqlx.ExtContext,
+	commandToIDs map[string][]string) error {
+	stmt := `
+DELETE
+    eq
+FROM
+    nano_enrollment_queue AS eq
+	LEFT JOIN nano_command_results AS cr
+		ON cr.command_uuid = eq.command_uuid AND cr.id = eq.id
+WHERE
+	cr.command_uuid IS NULL AND eq.command_uuid = ? AND eq.id IN (?);`
+
+	// We process each commandUUID one at a time, in batches of hostUserIDs.
+	// This is because the number of hostUserIDs can be large, and number of unique commands is normally small.
+	// If we have a use case where each host has a unique command, we can create a separate method for that use case.
+	for commandUUID, hostUserIDs := range commandToIDs {
+		if len(hostUserIDs) == 0 {
+			continue
+		}
+
+		batchSize := 10000
+		err := common_mysql.BatchProcessSimple(hostUserIDs, batchSize, func(hostUserIDsToProcess []string) error {
+			expanded, args, err := sqlx.In(stmt, commandUUID, hostUserIDsToProcess)
+			if err != nil {
+				return ctxerr.Wrap(ctx, err, "expanding bulk delete nano commands")
+			}
+			_, err = tx.ExecContext(ctx, expanded, args...)
+			if err != nil {
+				return ctxerr.Wrap(ctx, err, "bulk delete nano commands")
+			}
+			return nil
+		})
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}

server/mdm/nanomdm/storage/storage.go

@@ -27,6 +27,8 @@ type CommandAndReportResultsStore interface {
 	StoreCommandReport(r *mdm.Request, report *mdm.CommandResults) error
 	RetrieveNextCommand(r *mdm.Request, skipNotNow bool) (*mdm.CommandWithSubtype, error)
 	ClearQueue(r *mdm.Request) error
+	// BulkDeleteHostUserCommandsWithoutResults deletes all commands without results for the given host/user IDs.
+	BulkDeleteHostUserCommandsWithoutResults(ctx context.Context, commandToId map[string][]string) error
 }
 
 type BootstrapTokenStore interface {

server/mock/mdm/datastore_mdm_mock.go

@@ -29,6 +29,8 @@ type RetrieveNextCommandFunc func(r *mdm.Request, skipNotNow bool) (*mdm.Command
 
 type ClearQueueFunc func(r *mdm.Request) error
 
+type BulkDeleteHostUserCommandsWithoutResultsFunc func(ctx context.Context, commandToId map[string][]string) error
+
 type StoreBootstrapTokenFunc func(r *mdm.Request, msg *mdm.SetBootstrapToken) error
 
 type RetrieveBootstrapTokenFunc func(r *mdm.Request, msg *mdm.GetBootstrapToken) (*mdm.BootstrapToken, error)
@@ -89,6 +91,9 @@ type MDMAppleStore struct {
 	ClearQueueFunc        ClearQueueFunc
 	ClearQueueFuncInvoked bool
 
+	BulkDeleteHostUserCommandsWithoutResultsFunc        BulkDeleteHostUserCommandsWithoutResultsFunc
+	BulkDeleteHostUserCommandsWithoutResultsFuncInvoked bool
+
 	StoreBootstrapTokenFunc        StoreBootstrapTokenFunc
 	StoreBootstrapTokenFuncInvoked bool
 
@@ -198,6 +203,13 @@ func (fs *MDMAppleStore) ClearQueue(r *mdm.Request) error {
 	return fs.ClearQueueFunc(r)
 }
 
+func (fs *MDMAppleStore) BulkDeleteHostUserCommandsWithoutResults(ctx context.Context, commandToId map[string][]string) error {
+	fs.mu.Lock()
+	fs.BulkDeleteHostUserCommandsWithoutResultsFuncInvoked = true
+	fs.mu.Unlock()
+	return fs.BulkDeleteHostUserCommandsWithoutResultsFunc(ctx, commandToId)
+}
+
 func (fs *MDMAppleStore) StoreBootstrapToken(r *mdm.Request, msg *mdm.SetBootstrapToken) error {
 	fs.mu.Lock()
 	fs.StoreBootstrapTokenFuncInvoked = true

server/service/apple_mdm.go

@@ -3464,17 +3464,23 @@ func ReconcileAppleProfiles(
 	}
 
 	for _, p := range toRemove {
+		// Exclude profiles that are also marked for installation.
 		if _, ok := profileIntersection.GetMatchingProfileInDesiredState(p); ok {
 			hostProfilesToCleanup = append(hostProfilesToCleanup, p)
 			continue
 		}
 
-		if p.DidNotInstallOnHost() {
-			// then we shouldn't send an additional remove command since it wasn't installed on the
-			// host.
+		if p.FailedInstallOnHost() {
+			// then we shouldn't send an additional remove command since it failed to install on the host.
 			hostProfilesToCleanup = append(hostProfilesToCleanup, p)
 			continue
 		}
+		if p.PendingInstallOnHost() {
+			// The profile most likely did not install on host. However, it is possible that the profile
+			// is currently being installed. So, we clean up the profile from the database, but also send
+			// a remove command to the host.
+			hostProfilesToCleanup = append(hostProfilesToCleanup, p)
+		}
 
 		target := removeTargets[p.ProfileUUID]
 		if target == nil {
@@ -3504,6 +3510,16 @@ func ReconcileAppleProfiles(
 	// `InstallProfile` for the same identifier, which can cause race
 	// conditions. It's better to "update" the profile by sending a single
 	// `InstallProfile` command.
+	//
+	// Create a map of command UUIDs to host IDs
+	commandUUIDToHostIDsCleanupMap := make(map[string][]string)
+	for _, hp := range hostProfilesToCleanup {
+		commandUUIDToHostIDsCleanupMap[hp.CommandUUID] = append(commandUUIDToHostIDsCleanupMap[hp.CommandUUID], hp.HostUUID)
+	}
+	// We need to delete commands from the nano queue so they don't get sent to device.
+	if err := commander.BulkDeleteHostUserCommandsWithoutResults(ctx, commandUUIDToHostIDsCleanupMap); err != nil {
+		return ctxerr.Wrap(ctx, err, "deleting nano commands without results")
+	}
 	if err := ds.BulkDeleteMDMAppleHostsConfigProfiles(ctx, hostProfilesToCleanup); err != nil {
 		return ctxerr.Wrap(ctx, err, "deleting profiles that didn't change")
 	}