Improved the performance of Windows MDM profile reconciliation

[getvictor]

Related issue: Resolves #44052

Improve performance by reducing the time for the synchronous API call to update profiles or switch teams. And spreading out the application of profiles by processing 2000 hosts every 30 seconds.

1. Windows profile reconciliation is no longer synchronous to bulk-set.
   Apple, Android, and Apple-declaration paths still write their pending state inside the bulk-set transaction. The Windows path commits the transactional inputs and lets the existing mdm_windows_profile_manager cron pick the work up on its next tick. The visible effect is that host_mdm_windows_profiles is no longer guaranteed to be populated by the time bulk-set returns; it converges within one cron interval.

2. The Windows reconciler now processes hosts in bounded batches, with a persisted cursor.
   Previous behavior was "scan the universe of pending Windows hosts on every tick." New behavior is a host-window query bounded by batch size and a host_uuid cursor, advanced after the batch commits successfully and persisted across ticks. A failed tick leaves the cursor untouched so the same window is retried.

3. Two replication races are now explicitly handled.

   • Admin-delete vs reconcile: the existence check the reconciler uses to avoid touching a just-deleted profile reads from the primary, not a replica.
   • Insert lag in the reconciler's own listings: hosts that appear in the cursor query but are not yet visible in the scoped listings advance the cursor instead of jamming the loop.

4. updates.WindowsConfigProfile from BulkSetPendingMDMHostProfiles is now always false in production.
   The only consumer ORs it with the transactional signal from BatchSetMDMProfiles, which is the accurate source. The bulk-set call no longer attempts to compute or return that activity signal itself.

5. Tests opt in to the old synchronous behavior via a named hook.
   Default test behavior matches production (deferred). Legacy tests whose assertions require Windows rows immediately after bulk-set call an explicit enable-hook and rely on t.Cleanup to restore.

Checklist for submitter

If some of the following don't apply, delete the relevant line.

• Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
  See Changes files for more information.

Testing

• Added/updated automated tests
• QA'd all new/changed functionality manually

Summary by CodeRabbit

• New Features
  • Windows MDM profile reconciliation batching improvements enable large team transfers and bulk profile change operations to complete faster, with profile updates rolling out in the background without blocking host check-ins or other MDM activity.

[getvictor]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[getvictor]

/agentic_review

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. NewActivity nil DB panic 🐞 Bug ☼ Reliability

Description

activity/mysql.NewDatastore now allows constructing a datastore with no DB handles (nil
DBConnections), but NewActivity still dereferences ds.primary without calling ensureConfigured,
causing a nil-pointer panic if NewActivity is invoked on an unconfigured activity datastore.

Code

server/activity/internal/mysql/activity.go[R30-47]

+// errActivityDatastoreNotConfigured is returned by every public method when
+// the datastore was constructed without a DB connection. Callers (e.g. test
+// harnesses with mocked top-level datastores) may want the activity routes
+// registered on the HTTP mux so apiendpoints.Init passes, even if they have
+// no real DB to back them. If a request actually reaches one of these routes,
+// the handler gets a clean error instead of a nil-pointer panic taking down
+// the test server.
+var errActivityDatastoreNotConfigured = errors.New("activity datastore is not configured (no DB connection)")
+
+// NewDatastore creates a new MySQL datastore for activities. A nil conns is
+// allowed for test harnesses that register routes without a backing DB;
+// every method on the resulting datastore fails closed with
+// errActivityDatastoreNotConfigured.
func NewDatastore(conns *platform_mysql.DBConnections, logger *slog.Logger) *Datastore {
+	if conns == nil {
+		return &Datastore{logger: logger}
+	}
return &Datastore{primary: conns.Primary, replica: conns.Replica, logger: logger}

Evidence

The PR explicitly permits constructing an activity datastore without DB connections (returns
&Datastore{logger: logger}), and adds ensureConfigured with the stated intent that public methods
fail closed instead of panicking. However, NewActivity (a public method in the activity datastore
interface) still calls platform_mysql.WithRetryTxx with ds.primary directly and does not call
ensureConfigured first, so ds.primary can be nil and will panic at runtime.

server/activity/internal/mysql/activity.go[30-59]
server/activity/internal/mysql/new_activity.go[16-105]
server/activity/internal/types/activity.go[78-91]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`server/activity/internal/mysql.NewDatastore` now supports `conns == nil` and returns a datastore with `primary/replica == nil`. Most public methods now call `ensureConfigured()`, but `NewActivity` does not and passes `ds.primary` into `platform_mysql.WithRetryTxx`, which can nil-deref and panic.
### Issue Context
This is intended to support test harnesses that register activity routes without a backing DB. The PR’s comments state every public method should fail closed with a clear error, but `NewActivity` currently violates that contract.
### Fix Focus Areas
- server/activity/internal/mysql/new_activity.go[16-105]
- server/activity/internal/mysql/activity.go[50-59]
### Suggested change
At the start of `NewActivity`, add:
- `if err := ds.ensureConfigured(); err != nil { return err }`
This ensures requests hitting activity creation routes return a clean error instead of panicking when the activity datastore is unconfigured.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Activity routes skipped no-opts ☑ 🐞 Bug ≡ Correctness

Description

RunServerForTestsWithServiceWithDS only registers activity routes when len(opts)>0, but
apiendpoints.Init requires /api/v1/fleet/activities from the embedded catalog, so tests that call
RunServerForTestsWithDS(t, ds) without options can fail during handler initialization.

Code

server/service/testing_utils.go[R514-517]

+	if len(opts) > 0 {
legacyAuthorizer, err := authz.NewAuthorizer()
require.NoError(t, err)
activityAuthorizer := authz.NewAuthorizerAdapter(legacyAuthorizer)

Evidence

apiendpoints.Init fails the test server startup if any endpoint from api_endpoints.yml is missing
from the router. /api/v1/fleet/activities is in the catalog and is registered by the activity
bounded-context handler, but RunServerForTestsWithServiceWithDS only adds those routes when opts are
provided; at least one test helper calls RunServerForTestsWithDS with no opts, which passes
len(opts)==0 through to RunServerForTestsWithServiceWithDS.

server/service/testing_utils.go[503-532]
server/api_endpoints/api_endpoints.yml[1-4]
server/api_endpoints/api_endpoints.go[33-70]
server/activity/internal/service/handler.go[15-28]
server/service/http_auth_test.go[151-166]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`RunServerForTestsWithServiceWithDS` only wires activity routes when `len(opts) > 0`. However, `apiendpoints.Init` validates that every endpoint in the embedded API catalog is registered, and the catalog includes `/api/v1/fleet/activities`. Any test that starts a server via `RunServerForTestsWithDS(t, ds)` without passing `TestServerOpts` can fail during initialization because activity routes were never added.
### Issue Context
- `apiendpoints.Init` walks the mux routes and errors if any catalog endpoint fingerprint is missing.
- The `/fleet/activities` routes are defined in the activity bounded context, not in the main service handler.
### Fix Focus Areas
- server/service/testing_utils.go[503-532]
- server/api_endpoints/api_endpoints.go[33-70]
- server/activity/internal/service/handler.go[15-28]
- server/service/http_auth_test.go[151-166]
### Suggested fix
Ensure activity routes are registered even when no `TestServerOpts` are provided. Options include:
1) In `RunServerForTestsWithServiceWithDS`, if `len(opts)==0`, create a local default `TestServerOpts{}` (without changing external behavior) and register activity routes into the `featureRoutes` passed to `MakeHandler`.
2) Alternatively, build `featureRoutes` in a local slice (not only via `opts[0].FeatureRoutes`) and always append the activity routes function.
If DBConns is nil, prefer registering a safe stub activity service that returns an error (e.g., 503/501) instead of a nil-DB-backed service that can panic when the endpoint is hit.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Deleted profile row resurrection ☑ 🐞 Bug ≡ Correctness

Description

bulkSetPendingMDMWindowsHostProfilesBatched precomputes profilesToInstall/profilesToRemove outside
any transaction and later upserts those pairs without re-checking that the profile UUID still
exists. If a Windows profile is deleted after the pre-scan but before later batches run, this code
can re-insert host_mdm_windows_profiles rows for the deleted profile UUID, which can later cause
Windows profile command generation to error on “missing profile content”.

Code

server/datastore/mysql/microsoft_mdm.go[R3128-3137]

+	// The pre-scan listings are read-only and are run outside any
+	// transaction; they use the writer (same node that handled the prior tx)
+	// so they observe their own committed writes without replica lag.
+	profilesToInstall, err := ds.listMDMWindowsProfilesToInstallDB(ctx, ds.writer(ctx), hostUUIDs, onlyProfileUUIDs)
if err != nil {
return false, ctxerr.Wrap(ctx, err, "list profiles to install")
}
-	profilesToRemove, err := ds.listMDMWindowsProfilesToRemoveDB(ctx, tx, hostUUIDs, onlyProfileUUIDs)
+	profilesToRemove, err := ds.listMDMWindowsProfilesToRemoveDB(ctx, ds.writer(ctx), hostUUIDs, onlyProfileUUIDs)
if err != nil {

Evidence

The pre-scan reads desired state outside a transaction and the later per-batch upsert does not
validate that the profile still exists in mdm_windows_configuration_profiles at execution time.
Profile deletion runs in its own transaction and removes mdm_windows_configuration_profiles rows;
later, Windows command generation expects every profile UUID referenced by host profile work to have
contents and returns an error if content is missing.

server/datastore/mysql/microsoft_mdm.go[3128-3221]
server/datastore/mysql/microsoft_mdm.go[1222-1248]
server/service/microsoft_mdm.go[2691-2726]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Windows reconciliation batches can upsert `host_mdm_windows_profiles` entries for profile UUIDs that were deleted after the initial pre-scan, creating rows that reference non-existent profiles. Later, Windows MDM command generation fails when it cannot load contents for those UUIDs.
### Issue Context
- `bulkSetPendingMDMWindowsHostProfilesBatched` computes `profilesToInstall` once (outside a transaction) and then processes batches later.
- Concurrent profile deletion removes the profile row from `mdm_windows_configuration_profiles`.
- Subsequent reconciliation batches can still insert/update `host_mdm_windows_profiles` for that deleted UUID.
### Fix focus areas
- Before executing an upsert batch, filter the batch to profile UUIDs that still exist in `mdm_windows_configuration_profiles` (e.g., a `SELECT profile_uuid ... WHERE profile_uuid IN (?)` and drop missing ones).
- Alternatively, restructure the upsert to be `INSERT ... SELECT` from `mdm_windows_configuration_profiles` so the insert cannot occur if the profile no longer exists.
- Ensure removals/upserts remain deterministic and keep the deadlock-reduction benefits.
### Fix Focus Areas
- server/datastore/mysql/microsoft_mdm.go[3128-3221]
- server/datastore/mysql/microsoft_mdm.go[3225-3319]
- server/service/microsoft_mdm.go[2691-2726]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Host cursor query too heavy 🐞 Bug ➹ Performance

Description

ListNextPendingMDMWindowsHostUUIDs derives a host UUID batch by UNIONing two *per-(host,profile)*
derived sets and only then projecting to host_uuid, so the DB may need to produce/deduplicate many
rows per host even though only distinct host_uuid values are needed. This can inflate per-tick DB
work during large reconciliations versus a host_uuid-only pending-host query.

Code

server/datastore/mysql/microsoft_mdm.go[R2357-2369]

+	toInstall := fmt.Sprintf(windowsProfilesToInstallQuery, "TRUE", "TRUE", "TRUE", "TRUE")
+	toRemove := fmt.Sprintf(windowsProfilesToRemoveQuery, "TRUE", "TRUE", "TRUE", "TRUE", "TRUE")
+
+	stmt := fmt.Sprintf(`
+		SELECT host_uuid FROM (
+			SELECT host_uuid FROM (%s) AS install_set
+			UNION
+			SELECT host_uuid FROM (%s) AS remove_set
+		) AS combined
+		WHERE host_uuid > ?
+		ORDER BY host_uuid
+		LIMIT %d
+	`, toInstall, toRemove, batchSize)

Evidence

The cursor listing is built from the full install/remove listing queries (which return one row per
host/profile) and then reduced to host_uuid via an outer SELECT+UNION, meaning intermediate results
can include multiple rows per host before being deduped down to host_uuid.

server/datastore/mysql/microsoft_mdm.go[2357-2369]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ListNextPendingMDMWindowsHostUUIDs` currently UNIONs two derived subqueries that are shaped like the full per-(host,profile) install/remove listings, and only then selects `host_uuid`. This can force the DB to generate and deduplicate many rows per host even though the cron only needs distinct host UUIDs.
### Issue Context
The cron’s batching/cursor is meant to bound and smooth work per tick. The pending-host listing should ideally operate on *host UUIDs only* to minimize intermediate row counts.
### Fix Focus Areas
- server/datastore/mysql/microsoft_mdm.go[2357-2369]
### Suggested direction
Refactor the cursor query to compute **distinct host UUIDs** directly (e.g., `SELECT DISTINCT ds.host_uuid ...` and `SELECT DISTINCT hmwp.host_uuid ...`) while reusing the same predicates, so the DB doesn’t have to materialize full per-profile payload rows just to extract host UUIDs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Post-commit error semantics ☑ 🐞 Bug ☼ Reliability

Description

BulkSetPendingMDMHostProfiles now commits its transactional updates first, then runs Windows read
queries (only for the activity-signal boolean); if those reads fail it returns an error even though
the transactional state change already succeeded, so callers can surface an API failure for a
successful update.

Code

server/datastore/mysql/mdm.go[R739-777]

+	if err != nil {
+		return updates, err
+	}
+
+	// Apple-parity activity signal for Windows.
+	// bulkSetPendingMDMAppleHostProfilesDB returns true when host pending
+	// state actually changed (idempotent second calls return false). We
+	// match those semantics two ways:
+	//
+	//   1. Test path (hook installed by *_test.go init): the hook performs
+	//      synchronous reconciliation and returns true only if rows
+	//      actually changed. This is exact Apple-parity and lets tests
+	//      that pair BulkSet with assertions on host_mdm_windows_profiles
+	//      see the right state.
+	//
+	//   2. Production path (no hook): we use the same listing functions
+	//      the cron uses, scoped to just the resolved hosts and profiles.
+	//      This is a coarser approximation; it returns true whenever the
+	//      cron has any pending Windows work for these hosts, which
+	//      includes idempotent re-applies of the same profile. The
+	//      consequence is slightly over-firing the "edited Windows
+	//      profile" activity. The cron itself does the actual writes
+	//      asynchronously.
+	switch {
+	case ds.testWindowsEagerHook != nil:
+		updates.WindowsConfigProfile, err = ds.testWindowsEagerHook(ctx, winHosts, profileUUIDs)
+		if err != nil {
+			return updates, ctxerr.Wrap(ctx, err, "test windows eager hook")
+		}
+
+	case len(winHosts) > 0:
+		toInstall, lerr := ds.listMDMWindowsProfilesToInstallDB(ctx, ds.writer(ctx), winHosts, profileUUIDs)
+		if lerr != nil {
+			return updates, ctxerr.Wrap(ctx, lerr, "list windows profiles to install for activity signal")
+		}
+		toRemove, lerr := ds.listMDMWindowsProfilesToRemoveDB(ctx, ds.writer(ctx), winHosts, profileUUIDs)
+		if lerr != nil {
+			return updates, ctxerr.Wrap(ctx, lerr, "list windows profiles to remove for activity signal")
+		}

Evidence

The main transaction completes before Windows activity-signal reads run, but any error from those
reads is still returned. Service-layer code propagates that error, which means a user-facing request
can fail even though the DB transaction already committed.

server/datastore/mysql/mdm.go[720-780]
server/service/mdm.go[2239-2267]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`BulkSetPendingMDMHostProfiles` commits the Apple/Android (and Apple declarations) pending-state transaction first, then performs Windows listing queries purely to compute `updates.WindowsConfigProfile` for activity logging. If those post-commit reads fail, the method currently returns an error even though the transactional updates already succeeded, which can cause upstream API handlers to report failure for an update that actually took effect.
## Issue Context
This affects callers like `BatchSetMDMProfiles` that treat an error from `BulkSetPendingMDMHostProfiles` as a request failure.
## Fix Focus Areas
- server/datastore/mysql/mdm.go[720-780]
- server/service/mdm.go[2239-2267]
## Expected change
Adjust `BulkSetPendingMDMHostProfiles` so that failures in the *post-commit* Windows activity-signal reads do **not** fail the whole operation. Options:
- Treat listing failures as non-fatal: log a warning and return `(updates, nil)` with `updates.WindowsConfigProfile` left `false`/unchanged.
- Or, if you want strict atomic semantics, compute the Windows activity-signal value inside the same transaction (accepting the longer transaction), so any failure can roll back and preserve “error implies no state change.”
Keep the test-hook behavior unchanged.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Windows pending state delayed ☑ 🐞 Bug ≡ Correctness

Description

BulkSetPendingMDMHostProfiles no longer updates host_mdm_windows_profiles for Windows in production,
so any immediate reads expecting Windows pending-state changes will be stale until the
mdm_windows_profile_manager cron runs. Call sites that conceptually treat
BulkSetPendingMDMHostProfiles as “set pending now” for Windows can observe inconsistent behavior vs
Apple/Android and may mis-handle post-call assertions/flows.

Code

server/datastore/mysql/mdm.go[R725-753]

+	// Apple profiles, Apple declarations, and Android profiles reconcile
+	// eagerly inside one transaction here. Windows profile reconciliation is
+	// intentionally NOT performed synchronously in production: the
+	// mdm_windows_profile_manager cron computes the full desired-vs-actual
+	// diff globally every 30s (see ReconcileWindowsProfiles,
+	// windowsProfilesToInstallQuery, windowsProfilesToRemoveQuery). Doing it
+	// synchronously on top of large team transfers ties up the writer for
+	// minutes and starves ambient MDM / osquery checkins of row locks on
+	// host_mdm_windows_profiles.
+	//
+	// Tests set ds.testEagerWindowsProfileReconciliation = true so that they
+	// can observe post-call host_mdm_windows_profiles state without running
+	// the cron.
+	var winHosts []string
err = ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error {
-		updates, err = ds.bulkSetPendingMDMHostProfilesDB(ctx, tx, hostIDs, teamIDs, profileUUIDs, hostUUIDs)
-		return err
+		var innerErr error
+		updates, winHosts, innerErr = ds.bulkSetPendingMDMHostProfilesDB(ctx, tx, hostIDs, teamIDs, profileUUIDs, hostUUIDs)
+		return innerErr
})
-	return updates, err
+	if err != nil {
+		return updates, err
+	}
+
+	if ds.testEagerWindowsProfileReconciliation {
+		updates.WindowsConfigProfile, err = ds.bulkSetPendingMDMWindowsHostProfilesBatched(ctx, winHosts, profileUUIDs)
+		if err != nil {
+			return updates, ctxerr.Wrap(ctx, err, "bulk set pending windows host profiles")
+		}
+	}

Evidence

BulkSetPendingMDMHostProfiles now explicitly avoids synchronous Windows reconciliation unless a
test-only flag is enabled, relying on the cron to reconcile every ~30s. At least one service-layer
call site conceptually expects this call to “set pending status for windows profiles,” so post-call
reads/assumptions can now be incorrect for Windows (while remaining synchronous for Apple/Android).

server/datastore/mysql/mdm.go[720-755]
server/datastore/mysql/microsoft_mdm.go[3016-3032]
server/service/mdm.go[2215-2249]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`BulkSetPendingMDMHostProfiles` no longer performs Windows reconciliation in production, so callers that expect Windows `host_mdm_windows_profiles` to reflect changes immediately after this call can observe stale state until the cron runs.
### Issue Context
The datastore method now treats Windows differently from Apple/Android (cron-driven vs eager), but at least one service call site still documents/uses it as if it synchronously “sets pending status for windows profiles”.
### Fix Focus Areas
- server/datastore/mysql/mdm.go[720-755]
- server/datastore/mysql/microsoft_mdm.go[3016-3032]
- server/service/mdm.go[2215-2249]
### What to change
- Update service-layer comments and any logic/tests that assume immediate Windows pending-state changes after calling `BulkSetPendingMDMHostProfiles`.
- If any API/handler truly requires synchronous Windows state for correctness, introduce an explicit, clearly-named path (e.g., enqueue/trigger a scoped reconcile job) rather than relying on `BulkSetPendingMDMHostProfiles` semantics.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (2)

7. Partial commit on error ☑ 🐞 Bug ☼ Reliability

Description

BulkSetPendingMDMHostProfiles now commits Apple/Android changes and then performs Windows
reconciliation in independent per-batch transactions; if a later Windows batch fails, earlier
batches remain committed even though the method returns an error. This can make callers treat the
operation as failed while the DB is partially updated, leaving some hosts unreconciled until a later
run and potentially triggering confusing retries.

Code

server/datastore/mysql/mdm.go[R725-746]

+	// Apple profiles, Apple declarations, and Android profiles run inside a
+	// single outer transaction. Windows profile reconciliation is deliberately
+	// split out (see bulkSetPendingMDMWindowsHostProfilesBatched): a large
+	// team transfer can hold row locks on host_mdm_windows_profiles for
+	// minutes inside one giant transaction, which stalls ambient MDM /
+	// osquery checkins that also touch host rows. Per-batch commits keep the
+	// Windows lock hold-time short.
+	var winHosts []string
err = ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error {
-		updates, err = ds.bulkSetPendingMDMHostProfilesDB(ctx, tx, hostIDs, teamIDs, profileUUIDs, hostUUIDs)
-		return err
+		var innerErr error
+		updates, winHosts, innerErr = ds.bulkSetPendingMDMHostProfilesDB(ctx, tx, hostIDs, teamIDs, profileUUIDs, hostUUIDs)
+		return innerErr
})
-	return updates, err
+	if err != nil {
+		return updates, err
+	}
+
+	updates.WindowsConfigProfile, err = ds.bulkSetPendingMDMWindowsHostProfilesBatched(ctx, winHosts, profileUUIDs)
+	if err != nil {
+		return updates, ctxerr.Wrap(ctx, err, "bulk set pending windows host profiles")
+	}
+	return updates, nil

Evidence

BulkSetPendingMDMHostProfiles runs bulkSetPendingMDMHostProfilesDB inside a single transaction but
calls bulkSetPendingMDMWindowsHostProfilesBatched only after that transaction commits.
bulkSetPendingMDMWindowsHostProfilesBatched executes each batch inside its own withRetryTxx
transaction, so failures can occur after earlier batches have already committed.

server/datastore/mysql/mdm.go[720-746]
server/datastore/mysql/microsoft_mdm.go[3112-3223]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`BulkSetPendingMDMHostProfiles` can now return an error after some Windows host/profile rows have already been committed (per-batch transactions). This changes the error semantics from “no changes applied” to “some changes applied”, which can lead to confusing retries and partially unreconciled host sets.
### Issue Context
Windows reconciliation is intentionally split into per-batch transactions to reduce lock hold time, but the API surface still returns a single `(updates, error)` without indicating partial completion.
### Fix focus areas
- Consider returning a typed/sentinel error that explicitly indicates partial completion (so callers can avoid naive retries or can schedule a follow-up run).
- Alternatively, if the intended behavior is “best-effort”, consider logging and returning success while surfacing the batch error via metrics/logs, or returning both `updates` and a non-fatal warning channel.
### Fix Focus Areas
- server/datastore/mysql/mdm.go[720-746]
- server/datastore/mysql/microsoft_mdm.go[3112-3223]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. Batch size near limits ☑ 🐞 Bug ☼ Reliability

Description

bulkSetPendingMDMWindowsHostProfilesDB now defaults to batchSize=10000, which builds per-batch
INSERTs with 5 placeholders per row (~50k bound parameters). If the batch size is increased further
(via ds.testUpsertMDMDesiredProfilesBatchSize or future tuning), a single statement can exceed
placeholder-count ceilings noted elsewhere in the codebase and fail at runtime.

Code

server/datastore/mysql/microsoft_mdm.go[R3152-3156]

+	const defaultBatchSize = 10000
+	batchSize := defaultBatchSize
+	if ds.testUpsertMDMDesiredProfilesBatchSize > 0 {
+		batchSize = ds.testUpsertMDMDesiredProfilesBatchSize
+	}

Evidence

The function sets defaultBatchSize=10000 and uses it for the upsert batching; the VALUES template
shows 5 placeholders per row, so a full batch binds ~5*batchSize parameters. Elsewhere, the
datastore code explicitly calls out that very large placeholder counts (on the order of ~65K/2) can
become problematic, indicating this is an acknowledged constraint.

server/datastore/mysql/microsoft_mdm.go[3152-3156]
server/datastore/mysql/microsoft_mdm.go[3293-3297]
server/datastore/mysql/mdm.go[870-872]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`bulkSetPendingMDMWindowsHostProfilesDB` increased the default batch size to 10k. The upsert INSERT uses 5 placeholders per row, so larger batch sizes (including test overrides or future tuning) can push a single statement toward/over placeholder-count ceilings and hard-fail at execution time.
### Issue Context
- Default is now 10,000.
- INSERT values template: `(?, ?, ?, ?, NULL, '', ?),` => 5 placeholders per row.
- There is already awareness in the codebase that very large placeholder counts can be problematic.
### Fix Focus Areas
- Add a safety cap (or compute a cap) for `batchSize` based on an explicit `maxPlaceholdersPerStatement` and the per-row placeholder count used by this function.
- Keep test override behavior, but clamp it to the computed safe max and document why.
### Fix Focus Areas (code pointers)
- server/datastore/mysql/microsoft_mdm.go[3152-3156]
- server/datastore/mysql/microsoft_mdm.go[3293-3297]
- server/datastore/mysql/mdm.go[870-872]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. Read-only listings hit writer ☑ 🐞 Bug ➹ Performance

Description

The new cron batching path runs read-only Windows MDM listings inside ds.withTx, which uses the
primary writer connection and a read-write transaction. This increases primary load for SELECT-heavy
steps on every tick even though the operations are read-only.

Code

server/datastore/mysql/microsoft_mdm.go[R2316-2321]

+	var result []*fleet.MDMWindowsProfilePayload
+	err := ds.withTx(ctx, func(tx sqlx.ExtContext) error {
+		var err error
+		result, err = ds.listMDMWindowsProfilesToInstallDB(ctx, tx, hostUUIDs, nil)
+		return err
+	})

Evidence

The scoped listing uses ds.withTx(...), and withTx is implemented on top of ds.writer(ctx)
(primary). This means the new read-only scoped listing path (and the new cursor listing as well)
executes on the writer by default.

server/datastore/mysql/microsoft_mdm.go[2312-2322]
server/datastore/mysql/microsoft_mdm.go[2371-2381]
server/datastore/mysql/mysql.go[215-218]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Read-only cron listing methods (`ListMDMWindowsProfilesToInstallForHosts`, `ListMDMWindowsProfilesToRemoveForHosts`, and `ListNextPendingMDMWindowsHostUUIDs`) execute inside `ds.withTx`, which uses the primary writer DB and a read-write transaction.
### Issue Context
This PR’s goal is to reduce writer pressure during bulk operations. Even if using primary reads is desirable for freshness, using a read-only transaction (or `ds.withReadTx`) can reduce overhead, and where replica reads are acceptable it can further offload the primary.
### Fix Focus Areas
- server/datastore/mysql/microsoft_mdm.go[2312-2322]
- server/datastore/mysql/microsoft_mdm.go[2333-2339]
- server/datastore/mysql/microsoft_mdm.go[2371-2381]
- server/datastore/mysql/mysql.go[215-231]
### Suggested direction
Consider switching these pure-SELECT paths to `ds.withReadTx(...)` (which uses `ds.reader(ctx)` and a read-only transaction), and require primary explicitly only where truly needed (e.g., using `ctxdb.RequirePrimary`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

10. Wrong WindowsConfigProfile comment ☑ 🐞 Bug ⚙ Maintainability

Description

The fleet.Datastore interface comment says MDMProfilesUpdates.WindowsConfigProfile is always false
in production, but the MySQL implementation sets it true when there is pending Windows work (and the
service uses it to log activities).

Code

server/fleet/datastore.go[R1510-1519]

+	//
+	// This reconciles Apple profiles, Apple declarations, and Android profiles
+	// synchronously. Windows profile reconciliation is deferred: the
+	// mdm_windows_profile_manager cron computes the full desired-vs-actual
+	// diff globally every 30s (see ReconcileWindowsProfiles). Callers must not
+	// assume host_mdm_windows_profiles rows are written by the time this
+	// function returns — if a caller needs immediate Windows state (e.g. a
+	// test, or a synchronous UX flow), it must trigger reconciliation
+	// explicitly. MDMProfilesUpdates.WindowsConfigProfile is always false in
+	// production.

Evidence

The interface documentation claims the flag is always false in production, but the datastore
implementation sets it based on Windows pending work and the service consumes it to log an “edited
Windows profile” activity.

server/fleet/datastore.go[1506-1522]
server/datastore/mysql/mdm.go[743-779]
server/service/mdm.go[2259-2267]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `fleet.Datastore` interface docs for `BulkSetPendingMDMHostProfiles` state `MDMProfilesUpdates.WindowsConfigProfile` is always false in production, but the MySQL implementation can set it to true (based on listing pending Windows work) and service code uses it for activity logging.
## Issue Context
This is a documentation/contract mismatch that can mislead future callers.
## Fix Focus Areas
- server/fleet/datastore.go[1506-1522]
## Expected change
Update/remove the sentence claiming the flag is always false in production. Replace it with wording that matches actual behavior, e.g.:
- In production, Windows reconciliation is deferred to cron; `WindowsConfigProfile` is an activity signal that may be true when there is pending Windows work (and may over-fire on idempotent cases).
- In tests (with the eager hook), the bool reflects whether rows actually changed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

11. Unneeded winHosts allocation ☑ 🐞 Bug ➹ Performance

Description

BulkSetPendingMDMHostProfiles always collects Windows host UUIDs (winHosts) even though that list is
only used when ds.testEagerWindowsProfileReconciliation is true. On large host/team transfers, this
creates avoidable allocations and per-host work in production where the eager Windows path is
disabled.

Code

server/datastore/mysql/mdm.go[R738-753]

+	var winHosts []string
err = ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error {
-		updates, err = ds.bulkSetPendingMDMHostProfilesDB(ctx, tx, hostIDs, teamIDs, profileUUIDs, hostUUIDs)
-		return err
+		var innerErr error
+		updates, winHosts, innerErr = ds.bulkSetPendingMDMHostProfilesDB(ctx, tx, hostIDs, teamIDs, profileUUIDs, hostUUIDs)
+		return innerErr
})
-	return updates, err
+	if err != nil {
+		return updates, err
+	}
+
+	if ds.testEagerWindowsProfileReconciliation {
+		updates.WindowsConfigProfile, err = ds.bulkSetPendingMDMWindowsHostProfilesBatched(ctx, winHosts, profileUUIDs)
+		if err != nil {
+			return updates, ctxerr.Wrap(ctx, err, "bulk set pending windows host profiles")
+		}
+	}

Evidence

The PR adds winHosts as an output of bulkSetPendingMDMHostProfilesDB, appends to it for every
Windows host, and then only consumes it behind a test-only flag. In production, that slice-building
work is discarded.

server/datastore/mysql/mdm.go[720-755]
server/datastore/mysql/mdm.go[929-977]
server/datastore/mysql/testing_utils.go[376-386]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`winHosts` is built on every call but only used when `ds.testEagerWindowsProfileReconciliation` is enabled.
### Issue Context
In production the flag remains false, so the Windows host UUID collection is wasted work.
### Fix Focus Areas
- server/datastore/mysql/mdm.go[720-755]
- server/datastore/mysql/mdm.go[929-944]
### What to change
- In `bulkSetPendingMDMHostProfilesDB`, only append to `winHosts` when `ds.testEagerWindowsProfileReconciliation` is true.
- Example: in the `case "windows":` branch, guard `winHosts = append(...)` behind the flag.
- Optionally, document that `winHosts` is only populated for tests to prevent future production misuse.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (1)

12. Imprecise sorting comment 🐞 Bug ⚙ Maintainability

Description

The new comment claims sorting makes the SQL text deterministic and stabilizes plan-cache/query
digests, but the statements built here are placeholder-only so their text is already deterministic
for a given batch size. The comment should focus on the real effect of sorting in this function:
consistent lock acquisition order to reduce deadlocks.

Code

server/datastore/mysql/microsoft_mdm.go[R3136-3142]

+	// Sort by (HostUUID, ProfileUUID) to match the host_mdm_windows_profiles
+	// PRIMARY KEY (host_uuid, profile_uuid). Benefits:
+	//   - SQL text is deterministic across calls, which keeps MySQL plan-cache
+	//     entries and observability query digests stable.
+	//   - Concurrent callers acquire InnoDB row locks in a consistent order,
+	//     reducing deadlock risk on this path (see the retry comment at
+	//     apple_mdm.go around BulkSetPendingMDMHostProfiles).

Evidence

The comment asserts SQL text determinism as a benefit of sorting, but the UPDATE/INSERT builders
append repeated placeholder fragments like (?,?) / (?, ?, ?, ?, NULL, '', ?) and do not inline
values. Sorting therefore does not affect statement text, only bound parameter order and
(potentially) lock acquisition order.

server/datastore/mysql/microsoft_mdm.go[3136-3142]
server/datastore/mysql/microsoft_mdm.go[3167-3177]
server/datastore/mysql/microsoft_mdm.go[3296-3297]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The comment above the new sort suggests sorting stabilizes SQL text/plan-cache/query digests, but the queries built here are placeholder-only so their text does not vary with tuple order. This can confuse maintainers about why sorting is needed.
### Issue Context
Sorting is still useful here because it can make concurrent transactions acquire row locks in a consistent order, reducing deadlocks.
### Fix Focus Areas
- Edit the comment to remove/clarify the "SQL text is deterministic" bullet.
- Emphasize lock ordering / deadlock-reduction as the primary motivation.
### Fix Focus Areas (code pointers)
- server/datastore/mysql/microsoft_mdm.go[3136-3142]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

This pull request introduces cursor-based batching for Windows MDM profile reconciliation to improve scalability during large team transfers and bulk profile changes. The changes refactor Windows profile reconciliation from synchronous transactional behavior to deferred asynchronous processing via a global cron job. New datastore methods enable cursor-driven pagination through pending host UUIDs in configurable batch sizes, scoped install/remove profile listing by host, and concurrent-deletion safety through profile UUID validation. The reconciliation service now reads a persisted cursor, processes a bounded batch of hosts, and advances the cursor only on successful completion. Tests validate cursor state machine behavior, profile deletion handling, and end-to-end reconciliation flow using both eager (synchronous test-only) and deferred (production) paths.

Possibly related PRs

• SyncML <Delete> Windows profiles #42206: Modifies Windows MDM reconciliation and MySQL datastore logic for SyncML delete command generation and two-phase removals, with overlapping changes to ReconcileWindowsProfiles and Windows reconciliation flow.
• Windows profile delete fixes #42495: Modifies Windows MDM reconciliation logic in the service layer affecting the ReconcileWindowsProfiles function and related reconciliation behavior.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 61.29% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Improved the performance of Windows MDM profile reconciliation' accurately describes the main change: performance improvements to Windows profile reconciliation through batching and deferment.
Description check	✅ Passed	The PR description covers all key aspects of the change with detailed explanations of the five major modifications and their impact, meeting the repository template requirements.
Linked Issues check	✅ Passed	The PR directly addresses issue #44052 by reducing DB load through deferred Windows reconciliation, bounded batches, and explicit handling of replication races, successfully implementing the expected fix direction.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to Windows MDM profile reconciliation performance improvements; additions like test hooks, cursor persistence, batch processing, and property-based tests all serve the stated performance objectives.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch victor/42545-windows-profile-batch

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

server/datastore/mysql/microsoft_mdm.go (1)

3194-3230: LGTM — currentBatch correctly preserves iteration order for the pre-read SELECT.

Tracking the batch in a slice (instead of relying on map iteration) gives deterministic SQL text for the (host_uuid, profile_uuid) IN (...) pre-read, consistent with the new PK-aligned ordering elsewhere in this function. The SELECT tuple order and selectArgs appending (p.HostUUID, p.ProfileUUID) are aligned.

Minor nit (optional): profilesToInsert's map key is built as ProfileUUID\nHostUUID while the SELECT/UPDATE tuples are (host_uuid, profile_uuid). Functionally fine (the key only needs uniqueness), but flipping the key to match would make the code easier to follow.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/datastore/mysql/microsoft_mdm.go` around lines 3194 - 3230, The map
key for profilesToInsert is currently built as "ProfileUUID\nHostUUID", which
mismatches the tuple ordering used elsewhere (host_uuid, profile_uuid); change
the key construction to "HostUUID\nProfileUUID" wherever profilesToInsert is
populated so the logical ordering matches currentBatch, the pre-read SELECT
tuple order, and the upsert/update logic in executeUpsertBatch; update any
comments or variable usage that assume the old ordering to keep the code easier
to follow.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 3194-3230: The map key for profilesToInsert is currently built as
"ProfileUUID\nHostUUID", which mismatches the tuple ordering used elsewhere
(host_uuid, profile_uuid); change the key construction to
"HostUUID\nProfileUUID" wherever profilesToInsert is populated so the logical
ordering matches currentBatch, the pre-read SELECT tuple order, and the
upsert/update logic in executeUpsertBatch; update any comments or variable usage
that assume the old ordering to keep the code easier to follow.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4404989b-e722-4913-a0e0-8d7010dd9e1d

📥 Commits

Reviewing files that changed from the base of the PR and between ba0f6b3 and d181f3c.

📒 Files selected for processing (2)

• changes/42545-windows-profile-reconciliation-batching
• server/datastore/mysql/microsoft_mdm.go

[Copilot]

Pull request overview

This PR improves scalability of the Windows MDM “desired state” reconciliation path by making batched updates more efficient and less deadlock-prone under high concurrency (e.g., large team transfers or large profile assignment changes).

Changes:

• Sorts (host_uuid, profile_uuid) pairs to make SQL generation deterministic and to acquire row locks in a consistent order.
• Reorders tuple predicates to match the host_mdm_windows_profiles primary key and increases the batch size used for the batched UPDATE/UPSERT work.
• Adds a release note entry describing the performance improvements.

Reviewed changes

Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.

File	Description
server/datastore/mysql/microsoft_mdm.go	Sorts host/profile pairs, matches PK tuple order in IN clauses, and increases batch sizes to reduce query overhead and deadlock retries.
changes/42545-windows-profile-reconciliation-batching	Adds a changelog entry for the Windows profile reconciliation performance improvements.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

❌ Patch coverage is 70.50360% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.80%. Comparing base (1c89b79) to head (da2bdfe).
⚠️ Report is 35 commits behind head on main.

Files with missing lines	Patch %	Lines
...erver/datastore/mysqlredis/windows_recon_cursor.go	0.00%	17 Missing ⚠️
server/service/microsoft_mdm.go	77.77%	6 Missing and 4 partials ⚠️
server/datastore/mysql/mdm.go	66.66%	7 Missing and 1 partial ⚠️
server/datastore/mysql/microsoft_mdm.go	88.67%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #44075      +/-   ##
==========================================
+ Coverage   66.73%   66.80%   +0.06%     
==========================================
  Files        2627     2631       +4     
  Lines      211165   211319     +154     
  Branches     9423     9420       -3     
==========================================
+ Hits       140924   141167     +243     
+ Misses      57457    57334     -123     
- Partials    12784    12818      +34     

Flag	Coverage Δ
backend	68.57% <70.50%> (+0.07%)	⬆️
backend-activity	86.37% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[getvictor]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[getvictor]

/agentic_review

[qodo-free-for-open-source-projects]

Persistent review updated to latest commit 8516b6a

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 3296-3315: The INSERT into host_mdm_windows_profiles in baseStmt
never updates profile_name on duplicate, leaving profile_name stale; modify the
ON DUPLICATE KEY UPDATE clause in the baseStmt built in microsoft_mdm.go so that
profile_name is set to VALUES(profile_name) (i.e., add profile_name =
VALUES(profile_name) alongside the other updated columns) to ensure profile_name
is refreshed for existing rows used by the Windows verification/retry helpers.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 25c95363-201d-4cc2-a44d-25ec5260e181

📥 Commits

Reviewing files that changed from the base of the PR and between ba0f6b3 and 8516b6a.

📒 Files selected for processing (4)

• changes/42545-windows-profile-reconciliation-batching
• server/datastore/mysql/mdm.go
• server/datastore/mysql/mdm_test.go
• server/datastore/mysql/microsoft_mdm.go

[Copilot]

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[getvictor]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[getvictor]

/agentic_review

[qodo-free-for-open-source-projects]

Persistent review updated to latest commit f4c6f6b

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

_{Tip: disable this comment in your organization's Code Review settings.}

[qodo-free-for-open-source-projects]

CI Feedback 🧐

A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

Action: aggregate-result

Failed stage: Check for failures [❌]

Failed test name: activity-mysql8.0.44, activity-mysql9.5.0

Failure summary: The action failed in the step that aggregates downloaded test job statuses. - Two status artifact files were found and both contained fail: - ./activity-mysql8.0.44-status/status → marked as failed (activity-mysql8.0.44) - ./activity-mysql9.5.0-status/status → marked as failed (activity-mysql9.5.0) - Because failed_tests was non-empty, the script exited with code 1 (exit 1), producing: ❌ One or more test jobs failed: activity-mysql8.0.44, activity-mysql9.5.0.

Relevant error logs: 1: ##[group]Runner Image Provisioner 2: Hosted Compute Agent ... 79: Preparing to download the following artifacts: 80: - activity-mysql8.0.44-status (ID: 6672053511, Size: 133) 81: - activity-mysql9.5.0-status (ID: 6672051662, Size: 133) 82: Redirecting to blob download url: https://productionresultssa1.blob.core.windows.net/actions-results/ebffa02e-65ed-4600-90e0-b0ee552ae93f/workflow-job-run-ccbd1221-d97b-5fb7-9051-981bc523caf6/artifacts/8a65d871662d92ae0a65749ea5b9919373c2f801168229b5b35a657034995755.zip 83: Starting download of artifact to: /home/runner/work/fleet/fleet/activity-mysql8.0.44-status 84: Redirecting to blob download url: https://productionresultssa1.blob.core.windows.net/actions-results/ebffa02e-65ed-4600-90e0-b0ee552ae93f/workflow-job-run-016e8357-346e-564a-b0fc-a8bbd4d0d79d/artifacts/b675a423d978711ace5847e09fde140ffebc7685864754066cd6da9d6a8bbfe0.zip 85: Starting download of artifact to: /home/runner/work/fleet/fleet/activity-mysql9.5.0-status 86: Extracting artifact entry: /home/runner/work/fleet/fleet/activity-mysql8.0.44-status/status 87: (node:2640) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead. 88: (Use `node --trace-deprecation ...` to show where the warning was created) 89: Artifact download completed successfully. 90: Extracting artifact entry: /home/runner/work/fleet/fleet/activity-mysql9.5.0-status/status 91: Artifact download completed successfully. 92: Total of 2 artifact(s) downloaded 93: Download artifact has finished successfully 94: ##[group]Run failed_tests="" 95: �[36;1mfailed_tests=""�[0m 96: �[36;1mstatus_count=0�[0m 97: �[36;1m# Find all status files (they are in directories like 'activity-mysql8.0.44-status/status')�[0m 98: �[36;1mfor status_file in $(find ./ -type f -name 'status'); do�[0m 99: �[36;1m status_count=$((status_count + 1))�[0m 100: �[36;1m # Extract test name from parent directory (e.g., 'activity-mysql8.0.44-status')�[0m 101: �[36;1m test_dir=$(basename $(dirname "$status_file"))�[0m 102: �[36;1m # Remove '-status' suffix to get the test name�[0m 103: �[36;1m test_name="${test_dir%-status}"�[0m 104: �[36;1m status_content=$(cat "$status_file")�[0m 105: �[36;1m echo "Processing: $status_file (Test: $test_name) with status content: $status_content"�[0m 106: �[36;1m if grep -q "fail" "$status_file"; then�[0m 107: �[36;1m echo " ❌ Test failed: $test_name"�[0m 108: �[36;1m failed_tests="${failed_tests}${test_name}, "�[0m 109: �[36;1m else�[0m 110: �[36;1m echo " ✅ Test passed: $test_name"�[0m 111: �[36;1m fi�[0m 112: �[36;1mdone�[0m 113: �[36;1mif [[ $status_count -eq 0 ]]; then�[0m 114: �[36;1m echo "❌ ERROR: No status files found! This indicates a workflow issue."�[0m 115: �[36;1m exit 1�[0m 116: �[36;1mfi�[0m 117: �[36;1mif [[ -n "$failed_tests" ]]; then�[0m 118: �[36;1m echo "❌ One or more test jobs failed: ${failed_tests%, }"�[0m 119: �[36;1m exit 1�[0m 120: �[36;1mfi�[0m 121: �[36;1mecho "✅ All test jobs succeeded."�[0m 122: shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0} 123: ##[endgroup] 124: Processing: ./activity-mysql8.0.44-status/status (Test: activity-mysql8.0.44) with status content: fail 125: ❌ Test failed: activity-mysql8.0.44 126: Processing: ./activity-mysql9.5.0-status/status (Test: activity-mysql9.5.0) with status content: fail 127: ❌ Test failed: activity-mysql9.5.0 128: ❌ One or more test jobs failed: activity-mysql8.0.44, activity-mysql9.5.0 129: ##[error]Process completed with exit code 1. 130: Post job cleanup.

[Copilot]

Pull request overview

Copilot reviewed 15 out of 17 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

server/datastore/mysql/mdm_test.go (1)

8699-8704: Make the darwin probe eligible except for platform.

sameTeamDarwinHost is never enrolled, so this negative case can still pass if the query excludes it for lack of MDM eligibility rather than because platform != "windows". Enrolling it via Apple MDM would make this assertion actually prove the platform scoping.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@server/datastore/mysql/mdm_test.go` around lines 8699 - 8704, The darwin host
created as sameTeamDarwinHost is never enrolled, so the negative test might pass
for the wrong reason; update the test.NewHost call that creates
sameTeamDarwinHost to mark it as enrolled in Apple MDM (so it is MDM-eligible)
while keeping test.WithPlatform("darwin") and test.WithTeamID(team.ID); add the
appropriate helper option your test helpers provide (e.g.,
test.WithEnrolledInMDM / test.WithMDMEnrollment or the equivalent) to
sameTeamDarwinHost so the only exclusion is the platform check.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 2353-2379: The outer WHERE host_uuid > ? must be moved into each
subquery so the cursor filters are applied before the UNION; update the usages
of windowsProfilesToInstallQuery and windowsProfilesToRemoveQuery (the
toInstall/toRemove fmt.Sprintf calls) to scope the cursor predicate in their
host filters (e.g., add h.uuid > ? / hmwp.host_uuid > ? into those subqueries)
and remove the outer WHERE from stmt while keeping the outer ORDER BY and LIMIT;
then adjust the sqlx.SelectContext argument list in the withTx block to pass the
new cursor placeholder(s) in the correct order for the install/remove subquery
placeholders (and drop the now-removed outer afterHostUUID placeholder if you
removed that WHERE).

---

Nitpick comments:
In `@server/datastore/mysql/mdm_test.go`:
- Around line 8699-8704: The darwin host created as sameTeamDarwinHost is never
enrolled, so the negative test might pass for the wrong reason; update the
test.NewHost call that creates sameTeamDarwinHost to mark it as enrolled in
Apple MDM (so it is MDM-eligible) while keeping test.WithPlatform("darwin") and
test.WithTeamID(team.ID); add the appropriate helper option your test helpers
provide (e.g., test.WithEnrolledInMDM / test.WithMDMEnrollment or the
equivalent) to sameTeamDarwinHost so the only exclusion is the platform check.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 73837715-5597-4af2-a175-f53c7f20748e

📥 Commits

Reviewing files that changed from the base of the PR and between 1c89b79 and f4c6f6b.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (16)

• changes/42545-windows-profile-reconciliation-batching
• go.mod
• server/datastore/mysql/mdm.go
• server/datastore/mysql/mdm_test.go
• server/datastore/mysql/microsoft_mdm.go
• server/datastore/mysql/microsoft_mdm_eager_test.go
• server/datastore/mysql/microsoft_mdm_property_test.go
• server/datastore/mysql/microsoft_mdm_test.go
• server/datastore/mysql/mysql.go
• server/datastore/mysqlredis/windows_recon_cursor.go
• server/fleet/datastore.go
• server/mock/datastore_mock.go
• server/service/microsoft_mdm.go
• server/service/microsoft_mdm_integration_test.go
• server/service/microsoft_mdm_test.go
• server/service/reconcile_windows_profiles_property_test.go

[getvictor]

@claude review once

[ksykulev]

The cursor is lexicographic: host_uuid > cursor, ordered alphabetically. UUIDs are "random". This means if the cursor is at m... and a new host enrolls with UUID a... we have to wait until the cursor gets set to "" and wraps around?

[getvictor]

yes

[ksykulev]

Gotcha. Not ideal, but we don't have a timestamp we can use for the cursor. Also putting a queue of ids into redis seems not ideal. I think this is the best we can do without other major modifications. 👍

[ksykulev]

One other comment I forgot to add. There isn't really any observability on batch progress. There's no metric or logs on how many hosts remain in the pending universe, how many passes have completed, or how long a full pass takes. Maybe adding a logger.InfoContext at batch start with cursor, batch_size, and hosts_in_batch would make it much easier to diagnose convergence issues in production? Just a thought.