Windows MDM improved host profile status performance - #44225
Conversation
![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.
There was a problem hiding this comment.
Pull request overview
This PR improves Windows MDM host profile ACK handling performance by avoiding a redundant UPSERT+DELETE cycle when processing verified remove acknowledgements, aligning the flow more closely with the Apple MDM approach.
Changes:
- Partition Windows profile ACK updates into “upsert” vs “delete” buckets to skip unnecessary writes for terminal remove acknowledgements.
- Add MySQL datastore tests covering verified remove deletion, failed remove persistence/detail, and mixed install+remove batches.
- Add a user-facing changelog entry describing the performance improvement.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/datastore/mysql/microsoft_mdm.go | Partitions profile status updates and introduces a batched DELETE path for terminal remove ACKs. |
| server/datastore/mysql/microsoft_mdm_test.go | Adds regression tests validating delete vs upsert behavior for Windows profile remove/install responses. |
| changes/44189-host-profile-perf | Adds changelog note for Windows MDM profile removal performance. |
💡 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)
WalkthroughA changelog entry was added. The Windows MDM host-profile ACK handler now partitions matched 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/datastore/mysql/microsoft_mdm_test.go (1)
3739-3749: Optional: collapse the two SELECTs into one round-trip.Minor readability/perf nit —
statusanddetailcan be fetched in a single query into a small struct, which also makes the assertions read as a single observation of the row rather than two independent ones.♻️ Suggested refactor
- // Row should persist with failed status and "Failed to remove: ..." detail. - var status, detail string - ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { - return sqlx.GetContext(t.Context(), q, &status, ` -SELECT status FROM host_mdm_windows_profiles -WHERE host_uuid = ? AND command_uuid = ?`, enrolledDevice1.HostUUID, deleteCommandUUID) - }) - ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { - return sqlx.GetContext(t.Context(), q, &detail, ` -SELECT detail FROM host_mdm_windows_profiles -WHERE host_uuid = ? AND command_uuid = ?`, enrolledDevice1.HostUUID, deleteCommandUUID) - }) - assert.Equal(t, "failed", status, "failed remove row should persist") - assert.Contains(t, detail, "Failed to remove:", "detail should have remove prefix") + // Row should persist with failed status and "Failed to remove: ..." detail. + var row struct { + Status string `db:"status"` + Detail string `db:"detail"` + } + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + return sqlx.GetContext(t.Context(), q, &row, ` +SELECT status, detail FROM host_mdm_windows_profiles +WHERE host_uuid = ? AND command_uuid = ?`, enrolledDevice1.HostUUID, deleteCommandUUID) + }) + assert.Equal(t, "failed", row.Status, "failed remove row should persist") + assert.Contains(t, row.Detail, "Failed to remove:", "detail should have remove prefix")🤖 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 3739 - 3749, The test currently issues two separate queries to fetch status and detail (using ExecAdhocSQL and sqlx.GetContext into the variables status and detail) which should be collapsed into a single SELECT that returns both columns from host_mdm_windows_profiles for the given enrolledDevice1.HostUUID and deleteCommandUUID; create a small local struct (e.g. with Status and Detail fields), run one sqlx.GetContext into that struct, and update the subsequent assertions to read from the struct instead of the separate status/detail variables.
🤖 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_test.go`:
- Around line 3754-3834: Add a new subtest that verifies
"resurrect-after-delete": insert a remove row into host_mdm_windows_profiles for
enrolledDevice1 using mdmWindowsInsertCommandForHostsDB + ExecAdhocSQL with a
delete command UUID and a specific profile_uuid, call MDMWindowsSaveResponse
with an enriched SyncML entry for Delete (StatusCode 200) via
createResponseAsEnrichedSyncML and assert the remove row is deleted, then insert
a fresh install row for the same profile_uuid but a different command UUID, call
MDMWindowsSaveResponse again with a Replace (StatusCode 200) entry and assert
the install row is upserted and marked "verified"; use the same helpers and
table (host_mdm_windows_profiles, mdmWindowsInsertCommandForHostsDB,
ExecAdhocSQL, createResponseAsEnrichedSyncML, MDMWindowsSaveResponse,
enrolledDevice1) to implement the flow.
---
Nitpick comments:
In `@server/datastore/mysql/microsoft_mdm_test.go`:
- Around line 3739-3749: The test currently issues two separate queries to fetch
status and detail (using ExecAdhocSQL and sqlx.GetContext into the variables
status and detail) which should be collapsed into a single SELECT that returns
both columns from host_mdm_windows_profiles for the given
enrolledDevice1.HostUUID and deleteCommandUUID; create a small local struct
(e.g. with Status and Detail fields), run one sqlx.GetContext into that struct,
and update the subsequent assertions to read from the struct instead of the
separate status/detail variables.
🪄 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: 39d17b67-9b5c-4d87-af36-3edb226fc931
📒 Files selected for processing (3)
changes/44189-host-profile-perfserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44225 +/- ##
==========================================
+ Coverage 66.72% 66.79% +0.06%
==========================================
Files 2626 2627 +1
Lines 211177 211165 -12
Branches 9392 9408 +16
==========================================
+ Hits 140914 141043 +129
+ Misses 57480 57303 -177
- Partials 12783 12819 +36
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:
|
There was a problem hiding this comment.
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_test.go`:
- Around line 3700-3752: The test "remove failed persists with detail prefix"
currently asserts that a failed remove persists with status "failed" and a
"Failed to remove:" detail; update it to reflect the new terminal-delete
behavior by asserting the host_mdm_windows_profiles row for the given host_uuid
and deleteCommandUUID is removed (e.g., query and expect no row /
sqlx.GetContext to return sql.ErrNoRows) after calling MDMWindowsSaveResponse,
rather than checking for status/detail; keep references to
MDMWindowsSaveResponse, enrolledDevice1.HostUUID and deleteCommandUUID so the
check targets the same record and remove the assert.Contains on detail and
assert.Equal on status.
🪄 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: ac0b45a1-e9a0-4ff0-b62c-ea06c54f9f2c
📒 Files selected for processing (2)
server/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.go
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Made a couple of comments. Might be able to merge as is.
Local load test resultsSetup: 1000 Windows hosts enrolled via Results:
InnoDB Row Writes at transfer time (zoomed): 
Peak at 18:31:25: 560 rows deleted, 4.11K rows inserted (new installs), 3.68K rows updated (new install status updates). Remove storm fully drained by 18:32:00.
|
Related issue: Resolves #44189
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.
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Testing
Summary by CodeRabbit
Performance
Bug Fixes
Tests