Fixed stale MDM profiles after MDM toggle - #43719
Merged
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Member
Author
|
/agentic_review |
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
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)
WalkthroughAdds Datastore.CleanupAllHostMDMProfilesForPlatform(ctx, platform string) to bulk-delete host MDM profile rows by platform. The MySQL implementation deletes platform-specific tables (Apple: host_mdm_apple_profiles and host_mdm_apple_declarations; Windows: host_mdm_windows_profiles). The datastore interface and mock were extended. Calls to the new method were added when Apple or Windows MDM is disabled globally and from the Windows unenroll path; tests were updated to assert profile rows are cleared. 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)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/service/appconfig.go (1)
1133-1143:⚠️ Potential issue | 🟠 MajorCleanup becomes non-retriable after partial success
Line 1133 gates cleanup on a state transition only. Because config is already saved earlier, a failure at Line 1141 returns an error but leaves Windows MDM disabled; a retry with the same payload won’t hit this branch again, so stale rows can remain indefinitely.
Suggested fix
- // if Windows MDM was enabled or disabled, create the corresponding activity - if oldAppConfig.MDM.WindowsEnabledAndConfigured != appConfig.MDM.WindowsEnabledAndConfigured { + windowsMDMEnabledChanged := oldAppConfig.MDM.WindowsEnabledAndConfigured != appConfig.MDM.WindowsEnabledAndConfigured + windowsMDMDisabled := !appConfig.MDM.WindowsEnabledAndConfigured + + // Run cleanup whenever Windows MDM is disabled so retries can recover from partial failures. + if windowsMDMDisabled { + if err := svc.ds.CleanupAllHostMDMProfilesForPlatform(ctx, "windows"); err != nil { + return nil, ctxerr.Wrap(ctx, err, "cleaning up Windows host MDM profiles") + } + } + + // if Windows MDM was enabled or disabled, create the corresponding activity + if windowsMDMEnabledChanged { var act fleet.ActivityDetails if appConfig.MDM.WindowsEnabledAndConfigured { act = fleet.ActivityTypeEnabledWindowsMDM{} } else { act = fleet.ActivityTypeDisabledWindowsMDM{} - - // Clean up all pending Windows MDM profile rows since hosts can no longer receive MDM commands. - if err := svc.ds.CleanupAllHostMDMProfilesForPlatform(ctx, "windows"); err != nil { - return nil, ctxerr.Wrap(ctx, err, "cleaning up Windows host MDM profiles") - } } if err := svc.NewActivity(ctx, authz.UserFromContext(ctx), act); err != nil { return nil, ctxerr.Wrapf(ctx, err, "create activity %s", act.ActivityName()) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/appconfig.go` around lines 1133 - 1143, The cleanup of Windows MDM profiles is only attempted during the state transition and a failure returns an error after the config has already been saved, making the operation non-retriable; change the logic around oldAppConfig.MDM.WindowsEnabledAndConfigured vs appConfig.MDM.WindowsEnabledAndConfigured so that when Windows is being disabled you either (a) perform CleanupAllHostMDMProfilesForPlatform(ctx, "windows") before persisting the new appConfig, or (b) if cleanup must run after save, do not return a hard error on svc.ds.CleanupAllHostMDMProfilesForPlatform failure—instead record/log the error and enqueue or schedule a retry/background job to run the same svc.ds.CleanupAllHostMDMProfilesForPlatform call (or persist a pending-cleanup flag) so the cleanup is retriable; adjust handling around Activity creation (fleet.ActivityTypeDisabledWindowsMDM) accordingly.
🧹 Nitpick comments (2)
server/datastore/mysql/mdm.go (1)
703-713: Make Apple cleanup atomic to prevent partial stale state.Line 704 and Line 707 execute separate deletes without a transaction. If the second delete fails, cleanup is only partially applied.
Proposed change
func (ds *Datastore) CleanupAllHostMDMProfilesForPlatform(ctx context.Context, platform string) error { - // Each platform case uses literal SQL statements to avoid fmt.Sprintf with table names (gosec G202). - // Apple platforms (darwin, ios, ipados) share the same profile/declaration tables. - switch platform { - case "darwin", "ios", "ipados": - if _, err := ds.writer(ctx).ExecContext(ctx, `DELETE FROM host_mdm_apple_profiles`); err != nil { - return ctxerr.Wrap(ctx, err, "deleting all rows from host_mdm_apple_profiles") - } - if _, err := ds.writer(ctx).ExecContext(ctx, `DELETE FROM host_mdm_apple_declarations`); err != nil { - return ctxerr.Wrap(ctx, err, "deleting all rows from host_mdm_apple_declarations") - } - case "windows": - if _, err := ds.writer(ctx).ExecContext(ctx, `DELETE FROM host_mdm_windows_profiles`); err != nil { - return ctxerr.Wrap(ctx, err, "deleting all rows from host_mdm_windows_profiles") - } - default: - return ctxerr.Errorf(ctx, "unsupported platform %s for MDM profile cleanup", platform) - } - - return nil + return ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { + switch platform { + case "darwin", "ios", "ipados": + if _, err := tx.ExecContext(ctx, `DELETE FROM host_mdm_apple_profiles`); err != nil { + return ctxerr.Wrap(ctx, err, "deleting all rows from host_mdm_apple_profiles") + } + if _, err := tx.ExecContext(ctx, `DELETE FROM host_mdm_apple_declarations`); err != nil { + return ctxerr.Wrap(ctx, err, "deleting all rows from host_mdm_apple_declarations") + } + case "windows": + if _, err := tx.ExecContext(ctx, `DELETE FROM host_mdm_windows_profiles`); err != nil { + return ctxerr.Wrap(ctx, err, "deleting all rows from host_mdm_windows_profiles") + } + default: + return ctxerr.Errorf(ctx, "unsupported platform %s for MDM profile cleanup", platform) + } + return nil + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/mdm.go` around lines 703 - 713, The Apple cleanup runs two separate deletes (ds.writer(ctx).ExecContext for host_mdm_apple_profiles and host_mdm_apple_declarations) and can leave partial state if the second fails; wrap both delete statements in a single DB transaction (use ds.writer(ctx).BeginTx(ctx, nil) to get tx, execute tx.ExecContext for both DELETEs, call tx.Commit() on success and tx.Rollback() on any error) and return ctxerr.Wrap-wrapped errors from the tx operations so the cleanup is atomic.server/datastore/mysql/microsoft_mdm_test.go (1)
4498-4519: Broaden this test to cover remove-operation stale rows too.Current assertions only validate cleanup of an install row. The cleanup objective includes stale install/remove operations, so this test should exercise both.
Suggested test enhancement
ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { _, err := q.ExecContext(ctx, `INSERT INTO host_mdm_windows_profiles (host_uuid, status, operation_type, command_uuid, profile_name, checksum, profile_uuid) VALUES (?, ?, ?, ?, ?, UNHEX(MD5('test')), ?)`, host.UUID, fleet.MDMDeliveryPending, fleet.MDMOperationTypeInstall, uuid.NewString(), "TestProfile", profUUID) + if err != nil { + return err + } + _, err = q.ExecContext(ctx, `INSERT INTO host_mdm_windows_profiles + (host_uuid, status, operation_type, command_uuid, profile_name, checksum, profile_uuid) + VALUES (?, ?, ?, ?, ?, UNHEX(MD5('test2')), ?)`, + host.UUID, fleet.MDMDeliveryPending, fleet.MDMOperationTypeRemove, uuid.NewString(), "TestProfileRemove", profUUID) return err }) @@ winProfs, err := ds.GetHostMDMWindowsProfiles(ctx, host.UUID) require.NoError(t, err) - require.Len(t, winProfs, 1) + require.Len(t, winProfs, 2) @@ winProfs, err = ds.GetHostMDMWindowsProfiles(ctx, host.UUID) require.NoError(t, err) require.Empty(t, winProfs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/microsoft_mdm_test.go` around lines 4498 - 4519, Add a second test row representing a stale "remove" operation in the existing block so the cleanup path handles both install and remove stale entries: when inserting into host_mdm_windows_profiles via ExecAdhocSQL also insert a row with operation_type = fleet.MDMOperationTypeRemove (and an appropriate command_uuid/profile_uuid), then after calling ds.MDMWindowsDeleteEnrolledDeviceWithDeviceID(ctx, deviceID) assert via ds.GetHostMDMWindowsProfiles(ctx, host.UUID) that both the install row and the remove-operation row are removed (i.e., return empty); keep the same setup/use of uuid.NewString(), profUUID and existing assertions for the install row but extend them to verify the remove-operation row existed before unenroll and is cleaned up after.
🤖 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/mdm_test.go`:
- Around line 9221-9225: After calling CleanupAllHostMDMProfilesForPlatform(ctx,
"windows") add the same assertion you did for host2 to ensure host1's Apple
profile rows survived: call ds.GetHostMDMAppleProfiles(ctx, host1.UUID), check
require.NoError(t, err) and require.NotEmpty(t, appleProfsHost1) (or equivalent
variable name) to detect any accidental partial cleanup of Apple rows for host1.
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 238-248: The SELECT that looks up host_uuid inside the
withRetryTxx block uses `SELECT host_uuid FROM mdm_windows_enrollments WHERE
mdm_device_id = ?` and can return an arbitrary row when `mdm_device_id` is not
unique; update that query (used in the function that calls ds.withRetryTxx and
references `mdmDeviceID`) to mirror the pattern in
`MDMWindowsGetEnrolledDeviceWithDeviceID` by adding `ORDER BY created_at DESC
LIMIT 1` so you deterministically pick the latest enrollment before performing
the subsequent `DELETE` and profile cleanup.
---
Outside diff comments:
In `@server/service/appconfig.go`:
- Around line 1133-1143: The cleanup of Windows MDM profiles is only attempted
during the state transition and a failure returns an error after the config has
already been saved, making the operation non-retriable; change the logic around
oldAppConfig.MDM.WindowsEnabledAndConfigured vs
appConfig.MDM.WindowsEnabledAndConfigured so that when Windows is being disabled
you either (a) perform CleanupAllHostMDMProfilesForPlatform(ctx, "windows")
before persisting the new appConfig, or (b) if cleanup must run after save, do
not return a hard error on svc.ds.CleanupAllHostMDMProfilesForPlatform
failure—instead record/log the error and enqueue or schedule a retry/background
job to run the same svc.ds.CleanupAllHostMDMProfilesForPlatform call (or persist
a pending-cleanup flag) so the cleanup is retriable; adjust handling around
Activity creation (fleet.ActivityTypeDisabledWindowsMDM) accordingly.
---
Nitpick comments:
In `@server/datastore/mysql/mdm.go`:
- Around line 703-713: The Apple cleanup runs two separate deletes
(ds.writer(ctx).ExecContext for host_mdm_apple_profiles and
host_mdm_apple_declarations) and can leave partial state if the second fails;
wrap both delete statements in a single DB transaction (use
ds.writer(ctx).BeginTx(ctx, nil) to get tx, execute tx.ExecContext for both
DELETEs, call tx.Commit() on success and tx.Rollback() on any error) and return
ctxerr.Wrap-wrapped errors from the tx operations so the cleanup is atomic.
In `@server/datastore/mysql/microsoft_mdm_test.go`:
- Around line 4498-4519: Add a second test row representing a stale "remove"
operation in the existing block so the cleanup path handles both install and
remove stale entries: when inserting into host_mdm_windows_profiles via
ExecAdhocSQL also insert a row with operation_type =
fleet.MDMOperationTypeRemove (and an appropriate command_uuid/profile_uuid),
then after calling ds.MDMWindowsDeleteEnrolledDeviceWithDeviceID(ctx, deviceID)
assert via ds.GetHostMDMWindowsProfiles(ctx, host.UUID) that both the install
row and the remove-operation row are removed (i.e., return empty); keep the same
setup/use of uuid.NewString(), profUUID and existing assertions for the install
row but extend them to verify the remove-operation row existed before unenroll
and is cleaned up after.
🪄 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: 64d09e99-a84c-44e7-a460-07ab73c04c8e
📒 Files selected for processing (9)
changes/42427-cleanup-stale-mdm-profilesserver/datastore/mysql/mdm.goserver/datastore/mysql/mdm_test.goserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/service/appconfig.goserver/service/mdm.go
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes stale host-level MDM profile state that could persist in the DB when MDM is turned off (or when a Windows device unenrolls), preventing “ghost” pending operations from reappearing when MDM is re-enabled.
Changes:
- Add a datastore method to delete all host MDM profile rows for a given platform and invoke it when disabling Windows MDM / deleting Apple APNS cert.
- Ensure Windows unenrollment deletes any
host_mdm_windows_profilesrows for the unenrolled host. - Add MySQL datastore tests covering Windows unenroll cleanup and global cleanup behavior; add a user-visible changes entry.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/service/mdm.go | Cleanup Apple host MDM profile state when APNS cert is deleted (Apple MDM disabled). |
| server/service/appconfig.go | Cleanup Windows host MDM profile state when Windows MDM is toggled off. |
| server/fleet/datastore.go | Extend datastore interface with platform-wide host-profile cleanup method. |
| server/datastore/mysql/mdm.go | Implement platform-wide cleanup (Apple profiles/declarations, Windows profiles). |
| server/datastore/mysql/microsoft_mdm.go | Cleanup Windows host profile rows during device unenrollment transaction. |
| server/mock/datastore_mock.go | Add mock hook for new datastore interface method. |
| server/datastore/mysql/mdm_test.go | Add test coverage for the new cleanup method. |
| server/datastore/mysql/microsoft_mdm_test.go | Add test coverage for Windows unenroll profile cleanup. |
| changes/42427-cleanup-stale-mdm-profiles | Add changelog entry for the bugfix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #43719 +/- ##
==========================================
- Coverage 66.92% 66.91% -0.01%
==========================================
Files 2600 2600
Lines 208710 208742 +32
Branches 9339 9339
==========================================
+ Hits 139678 139680 +2
- Misses 56320 56337 +17
- Partials 12712 12725 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/datastore/mysql/mdm.go (1)
701-715: Consider handling"android"for symmetry / future-proofing.
host_mdm_android_profilesexists and is referenced elsewhere in this file (e.g. lines 877, 1944, 2218). If Android MDM is ever disabled globally, this helper will return"unsupported platform"instead of cleaning stale rows. If that's intentional (Android MDM toggle doesn't reuse this path), ignore; otherwise add an"android"case deleting fromhost_mdm_android_profiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/mdm.go` around lines 701 - 715, The switch over platform in the MDM cleanup (the block using tx.ExecContext and ctxerr.Wrap) currently handles "darwin"/"ios"/"ipados" and "windows" but not "android"; add a case "android" that executes a DELETE on host_mdm_android_profiles and wraps errors like the other cases (use tx.ExecContext(ctx, `DELETE FROM host_mdm_android_profiles`) and return ctxerr.Wrap on error) so stale Android MDM rows are cleaned the same way as the other platforms.
🤖 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 235-266: The comment in MDMWindowsDeleteEnrolledDeviceWithDeviceID
misleadingly says "pending profile rows" while the SQL DELETE on
host_mdm_windows_profiles removes all rows for the host; update the comment to
state that all host MDM profile rows are deleted (stale rows are wiped) or
otherwise clarify that the DELETE is unqualified and intentionally removes all
statuses, referencing the tx.ExecContext call that runs `DELETE FROM
host_mdm_windows_profiles WHERE host_uuid = ?` so future readers understand the
behavior.
- Around line 240-248: The code reads host_uuid into a plain string which will
cause a scan error when host_uuid is NULL; change the scan target to
sql.NullString (e.g., hostUUIDNull) in the SELECT using sqlx.GetContext, then
convert it to the string you need (use hostUUIDNull.Valid ? hostUUIDNull.String
: "" or treat as absent) before continuing so NULL host_uuid does not abort
unenrollment; update any subsequent uses of hostUUID in the Microsoft MDM
unenrollment flow in microsoft_mdm.go accordingly.
---
Nitpick comments:
In `@server/datastore/mysql/mdm.go`:
- Around line 701-715: The switch over platform in the MDM cleanup (the block
using tx.ExecContext and ctxerr.Wrap) currently handles "darwin"/"ios"/"ipados"
and "windows" but not "android"; add a case "android" that executes a DELETE on
host_mdm_android_profiles and wraps errors like the other cases (use
tx.ExecContext(ctx, `DELETE FROM host_mdm_android_profiles`) and return
ctxerr.Wrap on error) so stale Android MDM rows are cleaned the same way as the
other platforms.
🪄 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: a6bb9017-1ba7-4a44-bfc4-b4c4a784f33e
📒 Files selected for processing (4)
server/datastore/mysql/mdm.goserver/datastore/mysql/mdm_test.goserver/datastore/mysql/microsoft_mdm.goserver/service/mdm_test.go
cdcme
approved these changes
Apr 20, 2026
JordanMontgomery
approved these changes
Apr 20, 2026
JordanMontgomery
left a comment
JordanMontgomery
left a comment
Member
There was a problem hiding this comment.
One small comment but I think this is probably OK
| } | ||
|
|
||
| // Clean up all pending Apple MDM profile rows since hosts can no longer receive MDM commands. | ||
| if err := svc.ds.CleanupAllHostMDMProfilesForPlatform(ctx, "darwin"); err != nil { |
Member
There was a problem hiding this comment.
I am OK with this change but there is one potential downside which is that in the event a customer accidentally turns off Apple MDM it goes from being something that is reversible with a few DB queries to something that requires a full DB restore(as happened recently in dogfood). This is probably OK but may be worth being aware of. IIRC that's why it wasn't originally done but I don't think it's necessarily a reason not to do it. It might be worth asking if that's a customer concern at all
This was referenced Apr 21, 2026
This was referenced Apr 27, 2026
7 tasks
8 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 #42427
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit