Defer Windows MDM profile removals via pending-delete retention - #47156
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.
# Conflicts: # server/mdm/reconcile/reconcile_test.go
Batch/single/team Windows config-profile deletes were O(profiles x hosts) and synchronous: they marked host_mdm_windows_profiles rows and generated <Delete> commands inside the request, so large removals (e.g. 40 profiles x 30K hosts) timed out and rolled back, never succeeding. Deletes now copy the profile definition into a new mdm_windows_configuration_profiles_pending_delete table before the live row is removed, keep only the cheap host-row cleanup (phases 0/1), and let the profile-manager cron generate the removals asynchronously in its bounded 2,000-host batches, the same path used for team transfers. The delete endpoints become O(profiles). GetMDMWindowsProfilesContents falls back to the retention table so the cron can build <Delete> commands after the definition is gone. Retention is GC'd by age (default 7-day grace) in the existing Windows MDM cleanup cron. The reconciler's <Delete> generation now protects LocURIs per host using the host's still-desired (applicable) profiles, so a removed profile's shared LocURI is not reverted where another applicable profile still enforces it. Protection is label-aware (a label-scoped profile only protects the hosts it applies to), matching the old synchronous two-pass behavior; hosts that share the same protected subset are grouped into one command.
|
@coderabbitai full review |
|
/agentic_review |
✅ Action performedFull review finished. |
Code Review by Qodo
1.
|
There was a problem hiding this comment.
Pull request overview
This PR addresses #46993 by making Windows MDM profile removals asynchronous (like installs already are) to prevent POST /api/latest/fleet/mdm/profiles/batch timeouts on large teams. It introduces a retention/tombstone table for deleted Windows profile SyncML so the profile-manager cron can generate <Delete> commands in bounded batches, and updates reconcile logic to keep LocURI protection correct when removals overlap with label-scoped desired profiles.
Changes:
- Retain deleted Windows profile definitions in
mdm_windows_configuration_profiles_pending_deleteand defer host fan-out<Delete>work to the Windows profile reconciler. - Update Windows profile reconcile execution to fetch “still-desired” profile contents and apply per-host (label-aware) LocURI protection when generating
<Delete>commands. - Add age-based GC for retained deleted-profile content via a new cron cleanup job and datastore method, with associated migrations/tests.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/service/microsoft_mdm.go | Passes per-host desired profile UUIDs into the reconcile execute step and updates <Delete> generation to be per-host/label-aware. |
| server/mdm/microsoft/reconcile.go | Adds helper to compute desired (applicable) profile UUIDs per host for LocURI protection. |
| server/datastore/mysql/microsoft_mdm.go | Copies deleted profile contents to pending-delete table, removes synchronous delete fan-out, adds content fallback + GC method. |
| server/datastore/mysql/teams.go | Updates team deletion path to retain Windows profile contents before the cascade removes definitions. |
| server/fleet/datastore.go | Extends the datastore interface with pending-delete GC. |
| server/mock/datastore_mock.go | Updates datastore mock to include the new GC method. |
| cmd/fleet/cron.go | Schedules periodic age-based cleanup of pending-delete profile retention rows. |
| server/datastore/mysql/migrations/tables/20260609143000_AddWindowsMDMConfigProfilesPendingDelete.go | Adds migration creating the pending-delete retention table. |
| server/datastore/mysql/migrations/tables/20260609143000_AddWindowsMDMConfigProfilesPendingDelete_test.go | Verifies the migration creates the table and supports the intended usage pattern. |
| server/datastore/mysql/schema.sql | Updates schema snapshot to include the new pending-delete table and migration entry. |
| server/datastore/mysql/microsoft_mdm_test.go | Adjusts/remodels tests to reflect async deletion and validates retention + GC behavior. |
| changes/46993-windows-batch-remove-async | User-visible change entry describing the fix and new async removal behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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 (2)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR implements asynchronous Windows MDM profile deletion to resolve batch-endpoint timeouts when removing large numbers of profiles from teams with many hosts. The fix introduces a Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/datastore/mysql/microsoft_mdm.go (1)
1684-1690:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
verifyingremove rows out of phase-0 cleanup.Lines 1232-1234 only treat remove rows as terminal when they reach
verifiedorfailed. Includingfleet.MDMDeliveryVerifyinghere deletes in-flight removals that the new async flow is supposed to leave behind for the cron, so a host can lose the last bit of state Fleet has for finishing or retrying that delete.Suggested fix
- terminalStatuses := []fleet.MDMDeliveryStatus{fleet.MDMDeliveryFailed, fleet.MDMDeliveryVerified, fleet.MDMDeliveryVerifying} + terminalStatuses := []fleet.MDMDeliveryStatus{fleet.MDMDeliveryFailed, fleet.MDMDeliveryVerified}🤖 Prompt for 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. In `@server/datastore/mysql/microsoft_mdm.go` around lines 1684 - 1690, The current phase-0 cleanup treats remove operations with status fleet.MDMDeliveryVerifying as terminal and deletes them; remove in-flight removals from this deletion set by excluding fleet.MDMDeliveryVerifying from the terminalStatuses slice used for remove operations (the slice named terminalStatuses used in the DELETE for host_mdm_windows_profiles with fleet.MDMOperationTypeRemove), so only fleet.MDMDeliveryFailed and fleet.MDMDeliveryVerified remain; update the code that builds delRemStmt/delRemArgs accordingly.
🧹 Nitpick comments (1)
server/datastore/mysql/microsoft_mdm_test.go (1)
5208-5214: ⚡ Quick winRestore
WindowsEnabledAndConfiguredafter mutating app config in tests.Both blocks set a global app-config flag but never restore it, which can leak state into later subtests sharing the same datastore and create order-dependent flakes.
Suggested fix
appCfg, acErr := ds.AppConfig(ctx) require.NoError(t, acErr) + prevWindowsEnabled := appCfg.MDM.WindowsEnabledAndConfigured + t.Cleanup(func() { + cfg, err := ds.AppConfig(ctx) + require.NoError(t, err) + cfg.MDM.WindowsEnabledAndConfigured = prevWindowsEnabled + require.NoError(t, ds.SaveAppConfig(ctx, cfg)) + }) appCfg.MDM.WindowsEnabledAndConfigured = true require.NoError(t, ds.SaveAppConfig(ctx, appCfg))Also applies to: 5564-5569
🤖 Prompt for 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. In `@server/datastore/mysql/microsoft_mdm_test.go` around lines 5208 - 5214, The test mutates the global app config flag MDM.WindowsEnabledAndConfigured via AppConfig() and SaveAppConfig() but never restores it, leaking state across subtests; capture the original value of appCfg.MDM.WindowsEnabledAndConfigured before setting it true, then after calling SaveAppConfig(ctx, appCfg) register a deferred restore that sets the flag back to the original value and calls SaveAppConfig(ctx, appCfg) to persist it. Apply the same pattern to the other block around lines noted (the second AppConfig/SaveAppConfig mutation) so both modifications are reverted after the test.
🤖 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.
Outside diff comments:
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 1684-1690: The current phase-0 cleanup treats remove operations
with status fleet.MDMDeliveryVerifying as terminal and deletes them; remove
in-flight removals from this deletion set by excluding
fleet.MDMDeliveryVerifying from the terminalStatuses slice used for remove
operations (the slice named terminalStatuses used in the DELETE for
host_mdm_windows_profiles with fleet.MDMOperationTypeRemove), so only
fleet.MDMDeliveryFailed and fleet.MDMDeliveryVerified remain; update the code
that builds delRemStmt/delRemArgs accordingly.
---
Nitpick comments:
In `@server/datastore/mysql/microsoft_mdm_test.go`:
- Around line 5208-5214: The test mutates the global app config flag
MDM.WindowsEnabledAndConfigured via AppConfig() and SaveAppConfig() but never
restores it, leaking state across subtests; capture the original value of
appCfg.MDM.WindowsEnabledAndConfigured before setting it true, then after
calling SaveAppConfig(ctx, appCfg) register a deferred restore that sets the
flag back to the original value and calls SaveAppConfig(ctx, appCfg) to persist
it. Apply the same pattern to the other block around lines noted (the second
AppConfig/SaveAppConfig mutation) so both modifications are reverted after the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ed974f17-8d77-43c3-862c-3eea3ed1b1ed
📒 Files selected for processing (12)
changes/46993-windows-batch-remove-asynccmd/fleet/cron.goserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/datastore/mysql/migrations/tables/20260609143000_AddWindowsMDMConfigProfilesPendingDelete.goserver/datastore/mysql/migrations/tables/20260609143000_AddWindowsMDMConfigProfilesPendingDelete_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/teams.goserver/fleet/datastore.goserver/mdm/microsoft/reconcile.goserver/mock/datastore_mock.goserver/service/microsoft_mdm.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #47156 +/- ##
==========================================
- Coverage 67.22% 67.20% -0.02%
==========================================
Files 3394 3395 +1
Lines 228363 228307 -56
Branches 11908 11908
==========================================
- Hits 153516 153441 -75
- Misses 61014 61048 +34
+ Partials 13833 13818 -15
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:
|
…lcheck lint
The Windows profile delete tests (TestDeleteMDMProfilesCancelsInstalls, TestDeleteTeamCancelsWindowsProfileInstalls,
integration TestDeleteMDMProfileCancelsInstalls) asserted the synchronous remove+pending flip that now happens via the
profile-manager cron. They each run/trigger the reconciler after the delete and enable Windows MDM in app config (restored
after, to avoid leaking into sibling subtests).
Lint: BuildDeleteCommandFromProfileBytes now takes a map[string]struct{} set instead of map[string]bool (setboolcheck), with
callers and tests updated.
| SyncML []byte `db:"syncml"` | ||
| Checksum []byte `db:"checksum"` | ||
| } | ||
| if err := sqlx.SelectContext(ctx, ds.reader(ctx), &pending, pdStmt, pdArgs...); err != nil { |
There was a problem hiding this comment.
Is this safe to get from the reader?
There was a problem hiding this comment.
yes, since it self-heals on the next tick. But I should not return an error if not found.
|
@ksykulev I merged this one with main and resolved conflicts. Ready to re-review/approve |
|
@ksykulev regenerated schema.sql again. Please approve. |
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.
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
Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit
Release Notes
Bug Fixes
New Features