Fixing issue where deleted profiles were being sent to devices. (#25095…

…) (#25177)

Cherry pick.

For #24804

# Checklist for submitter

- [x] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.
See [Changes

files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/Committing-Changes.md#changes-files)
for more information.
- [x] Added/updated tests
- [x] If database migrations are included, checked table schema to
confirm autoupdate
- [x] Manual QA for all new/changed functionality

(cherry picked from commit 7e1a808)

changes/24804-deleted-profiles

@@ -0,0 +1 @@
+Fixed issue where deleted Apple config profiles were installing on devices because devices were offline when the profile was added.

server/datastore/mysql/apple_mdm.go

@@ -2719,7 +2719,8 @@ func (ds *Datastore) BulkUpsertMDMAppleHostProfiles(ctx context.Context, payload
               detail,
               command_uuid,
               checksum,
-              secrets_updated_at
+              secrets_updated_at,
+			  ignore_error
             )
             VALUES %s
             ON DUPLICATE KEY UPDATE
@@ -2728,6 +2729,7 @@ func (ds *Datastore) BulkUpsertMDMAppleHostProfiles(ctx context.Context, payload
               detail = VALUES(detail),
               checksum = VALUES(checksum),
               secrets_updated_at = VALUES(secrets_updated_at),
+              ignore_error = VALUES(ignore_error),
               profile_identifier = VALUES(profile_identifier),
               profile_name = VALUES(profile_name),
               command_uuid = VALUES(command_uuid)`,
@@ -2747,9 +2749,9 @@ func (ds *Datastore) BulkUpsertMDMAppleHostProfiles(ctx context.Context, payload
 	}
 
 	generateValueArgs := func(p *fleet.MDMAppleBulkUpsertHostProfilePayload) (string, []any) {
-		valuePart := "(?, ?, ?, ?, ?, ?, ?, ?, ?, ?),"
+		valuePart := "(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?),"
 		args := []any{p.ProfileUUID, p.ProfileIdentifier, p.ProfileName, p.HostUUID, p.Status, p.OperationType, p.Detail, p.CommandUUID,
-			p.Checksum, p.SecretsUpdatedAt}
+			p.Checksum, p.SecretsUpdatedAt, p.IgnoreError}
 		return valuePart, args
 	}
 
@@ -2767,14 +2769,25 @@ func (ds *Datastore) BulkUpsertMDMAppleHostProfiles(ctx context.Context, payload
 }
 
 func (ds *Datastore) UpdateOrDeleteHostMDMAppleProfile(ctx context.Context, profile *fleet.HostMDMAppleProfile) error {
-	if profile.OperationType == fleet.MDMOperationTypeRemove &&
-		profile.Status != nil &&
-		(*profile.Status == fleet.MDMDeliveryVerifying || *profile.Status == fleet.MDMDeliveryVerified) {
-		_, err := ds.writer(ctx).ExecContext(ctx, `
+	if profile.OperationType == fleet.MDMOperationTypeRemove && profile.Status != nil {
+		var ignoreError bool
+		if *profile.Status == fleet.MDMDeliveryFailed {
+			// Check whether we should ignore the error.
+			err := sqlx.GetContext(ctx, ds.reader(ctx), &ignoreError, `
+				SELECT ignore_error FROM host_mdm_apple_profiles WHERE host_uuid = ? AND command_uuid = ?`,
+				profile.HostUUID, profile.CommandUUID)
+			if err != nil {
+				return ctxerr.Wrap(ctx, err, "get ignore error")
+			}
+		}
+		if ignoreError ||
+			(*profile.Status == fleet.MDMDeliveryVerifying || *profile.Status == fleet.MDMDeliveryVerified) {
+			_, err := ds.writer(ctx).ExecContext(ctx, `
           DELETE FROM host_mdm_apple_profiles
           WHERE host_uuid = ? AND command_uuid = ?
         `, profile.HostUUID, profile.CommandUUID)
-		return err
+			return err
+		}
 	}
 
 	detail := profile.Detail

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/datastore/mysql/migrations/tables/20250102121439_AddIgnoreErrorToHostProfiles.go

@@ -0,0 +1,23 @@
+package tables
+
+import (
+	"database/sql"
+	"fmt"
+)
+
+func init() {
+	MigrationClient.AddMigration(Up_20250102121439, Down_20250102121439)
+}
+
+func Up_20250102121439(tx *sql.Tx) error {
+	_, err := tx.Exec(`ALTER TABLE host_mdm_apple_profiles
+    	ADD COLUMN ignore_error TINYINT(1) NOT NULL DEFAULT 0`)
+	if err != nil {
+		return fmt.Errorf("failed to add ignore_error to host_mdm_apple_profiles table: %w", err)
+	}
+	return nil
+}
+
+func Down_20250102121439(_ *sql.Tx) error {
+	return nil
+}

server/fleet/apple_mdm.go

@@ -326,14 +326,27 @@ type MDMAppleProfilePayload struct {
 	OperationType     MDMOperationType   `db:"operation_type"`
 	Detail            string             `db:"detail"`
 	CommandUUID       string             `db:"command_uuid"`
+	IgnoreError       bool               `db:"ignore_error"`
 }
 
 // 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
@@ -345,6 +358,7 @@ type MDMAppleBulkUpsertHostProfilePayload struct {
 	Detail            string
 	Checksum          []byte
 	SecretsUpdatedAt  *time.Time
+	IgnoreError       bool
 }
 
 // MDMAppleFileVaultSummary reports the number of macOS hosts being managed with Apples disk

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 (s *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,25 @@ 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)
+			// IgnoreError is set since the removal command is likely to fail.
+			p.IgnoreError = true
+		}
 
 		target := removeTargets[p.ProfileUUID]
 		if target == nil {
@@ -3496,6 +3504,7 @@ func ReconcileAppleProfiles(
 			ProfileName:       p.ProfileName,
 			Checksum:          p.Checksum,
 			SecretsUpdatedAt:  p.SecretsUpdatedAt,
+			IgnoreError:       p.IgnoreError,
 		})
 	}
 
@@ -3504,6 +3513,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")
 	}