Fixed Windows profile modify batch - #48474
Merged
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Action performedFull review finished. |
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 (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughChangesThis PR replaces the Windows MDM "pending delete" retention mechanism with a versioned "prior content" retention scheme keyed by (profile_uuid, checksum). Profile deletion, team deletion, and content-changing edit/upsert paths now retain outgoing SyncML for later reconciliation. Reconciliation logic is extended to compute deferred, version-aware Sequence Diagram(s)sequenceDiagram
participant Cron
participant Reconciler
participant Datastore
participant PriorContentTable
participant CommandQueue
Cron->>Datastore: CleanupWindowsMDMProfilePriorContent
Datastore->>PriorContentTable: delete unreferenced (profile_uuid, checksum) rows
Reconciler->>Datastore: GetWindowsMDMProfilePriorContents(version keys)
Datastore->>PriorContentTable: read retained SyncML
Reconciler->>Reconciler: diff retained vs live LocURIs, apply protection
Reconciler->>Datastore: MDMWindowsInsertCommandForHostUUIDs(hostUUIDs, deleteCmd)
Datastore->>CommandQueue: enqueue supplemental Delete command
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
🤖 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/service/microsoft_mdm.go`:
- Around line 4251-4281: The reconcile path in the prior-content lookup can
silently skip deleted LocURIs if `GetWindowsMDMProfilePriorContents` misses a
retained row after `host_mdm_windows_profiles` has already advanced to the new
checksum, so fix `GetWindowsMDMProfilePriorContents`/its caller in
`microsoft_mdm.go` to use a strongly consistent read or to validate prior
content before updating host checksums. Ensure the logic that builds
`removedByKey` and consumes `modifyHostsByKey` can retry when prior content is
temporarily unavailable, rather than treating `removedURIs == 0` as a permanent
no-op once `PreviousInstalledChecksum` is gone.
- Around line 3928-3935: The supplemental edit-delete path currently uses
modifyHostsByKey for every requested modify-install, which can enqueue <Delete>
even when the reinstall was skipped or failed. Update the modify flow in
MicrosoftMDM handling around MDMWindowsEnqueueCommandAndUpsertHostProfiles and
the later delete pass so only hosts whose reinstall was actually enqueued and
successfully inserted are tracked and used for the <Delete> command. Use the
existing symbols modifyHostsByKey, seenModifyHost, and the per-host success
result from the enqueue/upsert step to gate the delete list.
🪄 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: 660ea9dd-66bd-4387-a397-a73558c3e3e2
📒 Files selected for processing (15)
changes/48349-windows-batch-modify-asynccmd/fleet/cron.goserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/datastore/mysql/migrations/tables/20260630120000_AddWindowsMDMConfigProfilesPriorContent.goserver/datastore/mysql/migrations/tables/20260630120000_AddWindowsMDMConfigProfilesPriorContent_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/teams.goserver/fleet/datastore.goserver/fleet/windows_mdm.goserver/mdm/microsoft/reconcile.goserver/mock/datastore_mock.goserver/service/integration_mdm_profiles_test.goserver/service/microsoft_mdm.goserver/service/microsoft_mdm_test.go
Contributor
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR addresses #48349 by deferring Windows profile “modify” cleanup work (LocURI reverts) to the profile-manager cron, avoiding synchronous per-host fan-out on the batch endpoint. It introduces version-keyed retention of prior Windows profile content so the cron can generate supplemental <Delete> commands for LocURIs removed by profile edits, similar to the existing async delete path.
Changes:
- Add reconcile-time logic to enqueue supplemental
<Delete>commands for LocURIs removed by edited Windows profiles (using retained prior content keyed by checksum). - Replace the delete-only pending-delete retention table with a unified
..._prior_contenttable keyed by(profile_uuid, checksum), plus new datastore APIs and cleanup job. - Update MySQL datastore logic and tests to retain, look up, and garbage-collect prior profile versions; adjust integration/unit tests accordingly.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/service/microsoft_mdm.go | Adds modify-install removed-LocURI delete enqueue logic during Windows reconcile batches. |
| server/service/microsoft_mdm_test.go | Initializes new mock datastore funcs to avoid nil panics in existing reconcile tests. |
| server/service/integration_mdm_profiles_test.go | Updates integration test expectations/comments to reflect reconciler-owned removed-LocURI deletes. |
| server/mock/datastore_mock.go | Updates mock datastore interface for prior-content retention + new enqueue method. |
| server/mdm/microsoft/reconcile.go | Adds PreviousInstalledChecksum propagation on modify-triggered installs. |
| server/fleet/windows_mdm.go | Extends payload + introduces version-key/prior-content structs used by cron reconciliation. |
| server/fleet/datastore.go | Adds datastore interface methods for prior-content GC and keyed prior-content lookup. |
| server/datastore/mysql/teams.go | Renames call site to unified prior-content retention helper during team deletion. |
| server/datastore/mysql/schema.sql | Updates schema to use mdm_windows_configuration_profiles_prior_content and bumps schema version metadata. |
| server/datastore/mysql/migrations/tables/20260630120000_AddWindowsMDMConfigProfilesPriorContent.go | Migration creating unified prior-content table, backfilling from pending-delete, and dropping old table. |
| server/datastore/mysql/migrations/tables/20260630120000_AddWindowsMDMConfigProfilesPriorContent_test.go | Tests migration behavior (backfill + PK semantics). |
| server/datastore/mysql/microsoft_mdm.go | Implements prior-content retention/GC, keyed lookup, and new UUID-based bulk enqueue method; updates batch-set behavior. |
| server/datastore/mysql/microsoft_mdm_test.go | Adds/updates tests for deferred edit deletes and unified prior-content retention + GC. |
| cmd/fleet/cron.go | Updates cleanup job name and calls new prior-content GC method. |
| changes/48349-windows-batch-modify-async | (Excluded from diff) user-visible change note for the fix. |
Files excluded by content exclusion policy (1)
- changes/48349-windows-batch-modify-async
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #48474 +/- ##
==========================================
+ Coverage 68.03% 68.06% +0.02%
==========================================
Files 3688 3684 -4
Lines 234199 234099 -100
Branches 12303 12398 +95
==========================================
- Hits 159347 159341 -6
+ Misses 60541 60435 -106
- Partials 14311 14323 +12
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:
|
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
# Conflicts: # server/datastore/mysql/schema.sql
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
Files excluded by content exclusion policy (1)
- changes/48349-windows-batch-modify-async
|
Code review by qodo was updated up to the latest commit 6f57f2a |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/datastore/mysql/teams.go (1)
236-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
enqueueWindowsDeleteCommandsForTeam— it no longer enqueues anything.Post-refactor, this function only retains prior content and cancels installs; actual
<Delete>enqueue is now deferred to the profile-manager cron. The current name misleadingly implies synchronous command enqueue, which could mislead future maintainers working in this async-delete-critical path.✏️ Suggested rename
-func (ds *Datastore) enqueueWindowsDeleteCommandsForTeam(ctx context.Context, tid uint) error { +func (ds *Datastore) retainWindowsProfilesPriorContentForTeam(ctx context.Context, tid uint) error {(update the call site in
DeleteTeamand the accompanying comment accordingly)🤖 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/teams.go` around lines 236 - 254, The function enqueueWindowsDeleteCommandsForTeam no longer actually enqueues delete commands, so rename it to reflect its new behavior and update the DeleteTeam call site and any nearby comment accordingly. Use the function name and the surrounding retainWindowsProfilePriorContentDB and cancelWindowsHostInstallsForDeletedMDMProfiles flow to locate the change, and pick a name that describes retaining prior content plus canceling installs rather than synchronous enqueueing.
🤖 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.go`:
- Around line 2932-2948: The CleanupWindowsMDMProfilePriorContent GC is
currently a single unbounded DELETE over
mdm_windows_configuration_profiles_prior_content, so update this datastore
method to delete in batches instead of one full-table pass. Use the same
batching pattern as the other cleanup jobs in Datastore by looping on a limited
DELETE statement and stopping when no rows are affected, while keeping the
existing NOT EXISTS criteria on host_mdm_windows_profiles and the same error
handling via ds.writer(ctx).ExecContext and ctxerr.Wrap.
---
Nitpick comments:
In `@server/datastore/mysql/teams.go`:
- Around line 236-254: The function enqueueWindowsDeleteCommandsForTeam no
longer actually enqueues delete commands, so rename it to reflect its new
behavior and update the DeleteTeam call site and any nearby comment accordingly.
Use the function name and the surrounding retainWindowsProfilePriorContentDB and
cancelWindowsHostInstallsForDeletedMDMProfiles flow to locate the change, and
pick a name that describes retaining prior content plus canceling installs
rather than synchronous enqueueing.
🪄 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: 00e7ef2c-84ae-457b-9e65-0fd235ba2f47
📒 Files selected for processing (17)
changes/48349-windows-batch-modify-asynccmd/fleet/cron.goserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/datastore/mysql/migrations/tables/20260703114904_AddWindowsMDMConfigProfilesPriorContent.goserver/datastore/mysql/migrations/tables/20260703114904_AddWindowsMDMConfigProfilesPriorContent_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/teams.goserver/fleet/datastore.goserver/fleet/microsoft_mdm.goserver/fleet/microsoft_mdm_test.goserver/fleet/windows_mdm.goserver/mdm/microsoft/reconcile.goserver/mock/datastore_mock.goserver/service/integration_mdm_profiles_test.goserver/service/microsoft_mdm.goserver/service/microsoft_mdm_test.go
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
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.
ksykulev
reviewed
Jul 7, 2026
# Conflicts: # server/datastore/mysql/schema.sql
…main The migration timestamp (20260703114904) was older than migrations already merged to main (latest 20260706174522), so it was bumped to 20260707071142 and schema.sql regenerated. Claude-Session: https://claude.ai/code/session_019zNzvuWwhFMiwBKojUnfdH
Member
Author
|
@ksykulev ready for re-review |
ksykulev
approved these changes
Jul 7, 2026
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 #48349, as well as a few other minor issues found during dev (such as canonical LocURI, ensuring we delete the CSP version actually on the device, etc.).
Load tested the fix.
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