Improved Windows MDM reconciler - #47071
Merged
Merged
Conversation
Pure refactor, no behavior change. First step for #45635 (Windows batched in-memory reconciler). - New server/mdm/reconcile package holds the include/exclude label handlers and the team+label applicability dispatcher. The Apple platform gate stays in the Apple wrapper since platform eligibility is platform-specific. - New platform-neutral fleet types (MDMProfileLabelRef, MDMProfileIncludeMode, MDMLabeledEntity); the Apple names are now type aliases so existing code and tests are unchanged. - BulkGetHostLabelMemberships moves from apple_mdm_batched.go to a neutral file; it was already platform-agnostic. - The existing Apple label-scenario tests keep covering the shared logic through the delegating wrappers; the shared package also gets its own handler/dispatcher tests.
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Action performedFull review finished. |
Code Review by Qodo
1. Host-vitals exclude timing drift
|
# Conflicts: # server/mdm/reconcile/reconcile_test.go
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reworks the Windows MDM configuration profile reconciler to eliminate the expensive “pending host” set-difference query by switching to an indexed snapshot read + in-memory diff, then draining multiple scan windows per cron tick subject to a delivery cap and a wall-clock scan budget (per #45635).
Changes:
- Refactors
ReconcileWindowsProfilesto page through enrolled Windows hosts viaGetWindowsProfileReconcileSnapshot, compute install/remove deltas in memory, and drain successive windows within a tick. - Adds a new MySQL snapshot loader for the batched Windows reconciler and a shared in-memory delta computation implementation for Windows profiles.
- Expands unit/property tests to cover cursor semantics, delivery-cap throttling, and scan-budget halting.
Reviewed changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/reconcile_windows_profiles_property_test.go | Updates PBT fakes to drive the new snapshot-based reconciler and budget/cap tunables. |
| server/service/microsoft_mdm.go | Refactors Windows profile reconcile tick into a drain loop with delivery cap + scan budget; extracts legacy batch execution into a helper. |
| server/service/microsoft_mdm_test.go | Updates reconciler test harness to use snapshots; adds tests for delivery cap and scan budget behavior. |
| server/mock/datastore_mock.go | Extends the mock datastore with GetWindowsProfileReconcileSnapshot. |
| server/mdm/reconcile/reconcile_test.go | Fixes a slice-aliasing pitfall in HasBrokenLabel test entity helper. |
| server/mdm/microsoft/reconcile.go | Introduces Windows in-memory delta computation mirroring the legacy SQL diff rules. |
| server/mdm/microsoft/reconcile_test.go | Adds unit tests covering install/remove diff rules, team scoping, and label gating for Windows deltas. |
| server/fleet/windows_mdm.go | Adds reconcile snapshot DTOs (WindowsHostReconcileInfo, WindowsProfileForReconcile) and labeled-entity impl. |
| server/fleet/datastore.go | Adds the datastore interface method GetWindowsProfileReconcileSnapshot. |
| server/datastore/mysql/microsoft_mdm_batched.go | Implements the MySQL snapshot loader: host window + profiles/labels + host label memberships + current windows profile rows. |
| changes/45635-windows-batched-reconciler | Adds changelog entry for the Windows MDM reconciler redesign. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR refactors Windows MDM profile reconciliation to eliminate the query bottleneck ( Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/datastore/mysql/microsoft_mdm_batched.go`:
- Around line 104-125: The SELECT is returning mcpl.label_id which remains
non-NULL when the LEFT JOIN misses, so deleted labels are treated as live;
update the projection in the labelStmt to return the joined label's id (use
lbl.id AS label_id) instead of mcpl.label_id so that missing labels produce NULL
and preserve the “broken label” semantics; make the same change for the other
occurrence of this query (the similar block around the second label query) and
ensure any downstream variable named ref or LabelID is still fed from this
projected column.
In `@server/service/microsoft_mdm_test.go`:
- Around line 685-693: The helper currently creates one profile per host by
incrementing teamID for each host, causing duplicate WindowsProfileForReconcile
entries for the same ProfileUUID; change the logic to deduplicate by
ProfileUUID: build a map from ProfileUUID to a single teamID and a single
fleet.WindowsProfileForReconcile, assign that same teamID to every
fleet.WindowsHostReconcileInfo that references that profile (ensure
WindowsHostReconcileInfo.TeamID points to the mapped teamID), and only append
one WindowsProfileForReconcile per unique ProfileUUID (use hostToProfile,
profiles slice, WindowsProfileForReconcile, and WindowsHostReconcileInfo to
locate and update the code).
In `@server/service/microsoft_mdm.go`:
- Around line 3541-3575: The current loop counts all workHosts toward
deliveredHosts even if executeWindowsProfileReconcileBatch skipped some hosts;
change executeWindowsProfileReconcileBatch to return the slice/set of host UUIDs
that were actually queued or terminally marked (e.g. []string or
map[string]struct{}) and update this call site to increment deliveredHosts by
the size of that returned set instead of len(workHosts); keep commitCursor =
advanceTo unchanged (cursor advancement stays independent of scheduling), and
update any other callers/tests of executeWindowsProfileReconcileBatch to accept
the new return value and propagate errors as before.
🪄 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: 7fa70808-36ee-4dc9-a557-cf8869913504
📒 Files selected for processing (11)
changes/45635-windows-batched-reconcilerserver/datastore/mysql/microsoft_mdm_batched.goserver/fleet/datastore.goserver/fleet/windows_mdm.goserver/mdm/microsoft/reconcile.goserver/mdm/microsoft/reconcile_test.goserver/mdm/reconcile/reconcile_test.goserver/mock/datastore_mock.goserver/service/microsoft_mdm.goserver/service/microsoft_mdm_test.goserver/service/reconcile_windows_profiles_property_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47071 +/- ##
==========================================
+ Coverage 67.06% 67.10% +0.04%
==========================================
Files 2893 2891 -2
Lines 225198 226215 +1017
Branches 11772 11768 -4
==========================================
+ Hits 151018 151799 +781
- Misses 60503 60651 +148
- Partials 13677 13765 +88
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
5 tasks
ksykulev
approved these changes
Jun 10, 2026
ksykulev
left a comment
ksykulev
left a comment
Contributor
There was a problem hiding this comment.
Both non-blocking comments.
| // Delivery cap reached exactly at a window boundary. | ||
| return nil | ||
| case time.Now().After(deadline): | ||
| // Scan budget exhausted; resume next tick from cursor = advanceTo. |
Contributor
There was a problem hiding this comment.
Is it worth logging something here?
| errMsg := "windows profile has mixed include label modes; ignoring include labels" | ||
| ds.logger.ErrorContext(ctx, errMsg, "profile_uuid", uuid, "team_id", | ||
| p.TeamID) | ||
| ctxerr.Handle(ctx, errors.New(errMsg)) |
Contributor
There was a problem hiding this comment.
error or warning? I think the apple equivalent uses a warning.
ds.logger.WarnContext(ctx, "apple profile has mixed include label modes; ...", ...)
But non-blocking comment.
Member
Author
There was a problem hiding this comment.
I believe the intent here is this mixed-mode case should be impossible to reach, and the Apple equivalent should log an error as well. cc: @MagnusHJensen
Member
There was a problem hiding this comment.
We can do that, the most important is just it does not get pushed to the device, and some kind of message is hit, but error sounds fine to me, I'll get a small PR up
getvictor
added a commit
that referenced
this pull request
Jun 11, 2026
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #46993 Requires #47071 to merge first Loadtest shows reduction of batch delete of 40 profiles for 30K hosts down to ~3.9 seconds. # Checklist for submitter If some of the following don't apply, delete the relevant line. - [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/guides/committing-changes.md#changes-files) for more information. ## Testing - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually ## Database migrations - [x] Ensured the correct collation is explicitly set for character columns (`COLLATE utf8mb4_unicode_ci`). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Resolved timeout issues when removing large numbers of Windows configuration profiles from teams with many hosts. * **New Features** * Windows profile deletions now process asynchronously in the background, enabling faster API responses and consistent behavior with profile delivery operations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 tasks
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #45635
Moved profile reconciler work from SQL to code, similar to what Apple MDM team did last sprint.
The Windows MDM loadtest for 40 profiles with 30K hosts looks much better.
The writer spikes briefly to 16 AAS, but has CPU headroom, so I'd say we can claim to support 40 profies on 30K hosts.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit
Refactor
Performance / Reliability
Chore / Configuration
Tests