Reworked Windows profile delete batching - #44047
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/agentic_review |
Code Review by Qodo
1.
|
WalkthroughThe change raises 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 |
There was a problem hiding this comment.
Pull request overview
Reworks batching for Windows MDM profile deletion to better utilize batch updates and increase the configured batch size while staying under MySQL placeholder limits.
Changes:
- Increase
windowsMDMProfileDeleteBatchSizefrom 5,000 to 10,000 with updated rationale. - Replace per-profile batched UPDATE loop with a single batched UPDATE across profiles using
(host_uuid, profile_uuid) IN (...)andCASEto setcommand_uuid.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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.go (1)
1495-1547: Stream pending-remove batches instead of materializing all rows.
rowsToRemoveandtargetsalready hold the affected host/profile pairs;rowsduplicates the full set before batching. For large deletes, flush fixed-size batches while iteratingenqueuedTargetsto keep memory bounded.♻️ Proposed refactor
- var rows []pendingRemoveRow - for profUUID, target := range enqueuedTargets { - for _, hostUUID := range target.hostUUIDs { - rows = append(rows, pendingRemoveRow{ - hostUUID: hostUUID, - profileUUID: profUUID, - cmdUUID: target.cmdUUID, - }) - } - } - - if err := common_mysql.BatchProcessSimple(rows, windowsMDMProfileDeleteBatchSize, func(batch []pendingRemoveRow) error { + updatePendingRemoveBatch := func(batch []pendingRemoveRow) error { // Collect the profile_uuid -> cmd_uuid mapping needed by this batch. Most // batches span 1 to N profiles; we only need one CASE arm per distinct // profile in the batch. profileCmds := make(map[string]string) for _, r := range batch { @@ if _, err := tx.ExecContext(ctx, sb.String(), args...); err != nil { return ctxerr.Wrap(ctx, err, "updating host profiles to remove") } return nil - }); err != nil { - return err } + + batch := make([]pendingRemoveRow, 0, windowsMDMProfileDeleteBatchSize) + for profUUID, target := range enqueuedTargets { + for _, hostUUID := range target.hostUUIDs { + batch = append(batch, pendingRemoveRow{ + hostUUID: hostUUID, + profileUUID: profUUID, + cmdUUID: target.cmdUUID, + }) + if len(batch) == windowsMDMProfileDeleteBatchSize { + if err := updatePendingRemoveBatch(batch); err != nil { + return err + } + batch = batch[:0] + } + } + } + if len(batch) > 0 { + if err := updatePendingRemoveBatch(batch); err != nil { + return err + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/microsoft_mdm.go` around lines 1495 - 1547, Currently the code materializes all pendingRemoveRow entries into "rows" before calling common_mysql.BatchProcessSimple, which duplicates memory; instead stream and flush fixed-size batches as you iterate "enqueuedTargets": create a local []pendingRemoveRow "batch" and for each profUUID,hostUUID append to batch, and whenever len(batch) == windowsMDMProfileDeleteBatchSize call the existing batch-processing logic (the body that builds profileCmds map, strings.Builder SQL, args and calls tx.ExecContext) on that batch and then reset batch = batch[:0]; after the loops process any remaining entries in batch the same way. Keep the same variable names (enqueuedTargets, pendingRemoveRow, windowsMDMProfileDeleteBatchSize, profileCmds, tx.ExecContext) and preserve error wrapping (ctxerr.Wrap) and return behavior.
🤖 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 28-30: Update the explanatory comment that estimates placeholder
counts for the batched UPDATE (the "batched UPDATE with tuple IN + CASE per
profile" comment) to use the accurate formula "2 + 2*distinctProfiles + 2*rows"
and give the correct worst-case example: with 10,000 rows and 10,000 distinct
profiles this yields 40,002 placeholders (still < 65,535). Locate the comment
near the batched UPDATE logic and replace the current "~20,000" estimate and
text with the corrected formula and example so the documentation reflects the
true headroom.
---
Nitpick comments:
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 1495-1547: Currently the code materializes all pendingRemoveRow
entries into "rows" before calling common_mysql.BatchProcessSimple, which
duplicates memory; instead stream and flush fixed-size batches as you iterate
"enqueuedTargets": create a local []pendingRemoveRow "batch" and for each
profUUID,hostUUID append to batch, and whenever len(batch) ==
windowsMDMProfileDeleteBatchSize call the existing batch-processing logic (the
body that builds profileCmds map, strings.Builder SQL, args and calls
tx.ExecContext) on that batch and then reset batch = batch[:0]; after the loops
process any remaining entries in batch the same way. Keep the same variable
names (enqueuedTargets, pendingRemoveRow, windowsMDMProfileDeleteBatchSize,
profileCmds, tx.ExecContext) and preserve error wrapping (ctxerr.Wrap) and
return behavior.
🪄 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: 28b457f6-caad-4a0b-b312-f70df2973085
📒 Files selected for processing (1)
server/datastore/mysql/microsoft_mdm.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44047 +/- ##
==========================================
- Coverage 66.83% 66.75% -0.09%
==========================================
Files 2609 2622 +13
Lines 210432 210963 +531
Branches 9292 9292
==========================================
+ Hits 140645 140828 +183
- Misses 56969 57313 +344
- Partials 12818 12822 +4
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:
|
Three Copilot review points applied: 1. Fix the placeholder-count doc on windowsMDMProfileDeleteBatchSize. The comment's "~20,000" was the realistic case (tens of profiles); the mathematical worst case (10,000 distinct profiles sharing a 10,000-row batch) is 40,002 placeholders. Document the full formula 2 + 2*distinctProfilesInBatch + 2*rowsInBatch so future batch-size tuning has the correct bound. 2. Pre-allocate the flattened `rows` slice to the known total count. The previous unbounded `append` triggered repeated reallocations when many profiles or hosts were involved. 3. Generate SQL in deterministic order. Iterating enqueuedTargets and profileCmds via map range produced non-deterministic CASE-arm and IN-tuple order across calls, creating different query text for same-shape batches and reducing MySQL plan-cache / observability digest stability. Sort profile UUIDs before iterating both the outer flatten loop and the inner CASE generation.
![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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/datastore/mysql/microsoft_mdm_test.go (1)
1976-1984: Helper is not team-scoped — safe today, fragile later.
windowsProfileUUIDByNamefilters bynameonly. The unique constraint onmdm_windows_configuration_profiles.nameis per-team, so a future test that seeds same-named profiles across teams (or a test running against an already-populated DB) would silently hit whichever row MySQL returns first, or fail with "more than one row" fromsqlx.GetContext. All current callers create profiles at no-team scope so it's fine now, but consider accepting an optionalteamID *uintand addingAND team_id = ?(with0for no-team) to harden the helper before it gets reused.🤖 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 1976 - 1984, The helper windowsProfileUUIDByName is currently scoped only by name and should be made team-aware to avoid ambiguous results; change its signature to accept an optional teamID *uint, update the SQL inside ExecAdhocSQL/sqlx.GetContext to add "AND team_id = ?" (using 0 when teamID == nil to represent no-team), and update all callers to pass either nil or a pointer to the relevant team ID so the query deterministically selects the correct mdm_windows_configuration_profiles row.
🤖 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 2009-2024: The docstring for rawWindowsDeleteCommandForHostProfile
is incorrect about fallback behavior: because the implementation uses
sqlx.GetContext inside ExecAdhocSQL, a missing join row yields sql.ErrNoRows
which ExecAdhocSQL/require.NoError will surface and fail the test rather than
returning empty bytes; update the comment to state that the helper will fail the
test if no matching delete command is queued (it only returns nil when
raw_command is SQL NULL), or alternatively change the implementation (replace
sqlx.GetContext with a tolerant SelectContext into a *[]byte and return an empty
slice when len == 0) — reference rawWindowsDeleteCommandForHostProfile,
ExecAdhocSQL, and sqlx.GetContext to locate the code.
---
Nitpick comments:
In `@server/datastore/mysql/microsoft_mdm_test.go`:
- Around line 1976-1984: The helper windowsProfileUUIDByName is currently scoped
only by name and should be made team-aware to avoid ambiguous results; change
its signature to accept an optional teamID *uint, update the SQL inside
ExecAdhocSQL/sqlx.GetContext to add "AND team_id = ?" (using 0 when teamID ==
nil to represent no-team), and update all callers to pass either nil or a
pointer to the relevant team ID so the query deterministically selects the
correct mdm_windows_configuration_profiles row.
🪄 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: 15ae0507-dee3-46f4-b69c-0cd19429c2cc
📒 Files selected for processing (3)
changes/42545-windows-profile-delete-batchingserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.go
✅ Files skipped from review due to trivial changes (1)
- changes/42545-windows-profile-delete-batching
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/datastore/mysql/microsoft_mdm_test.go (2)
4727-4807: Good coverage of the new multi-profile UPDATE path.The test exercises the
CASE profile_uuidmapping by correlating each flipped row'scommand_uuidback towindows_mdm_commands.raw_commandand asserting the LocURI belongs to that specific profile. This would catch both CASE cross-wiring and an unintendedELSE command_uuidfire (since a stale/unchangedcommand_uuidwouldn't join to a queued remove command with the expected LocURI). Two hosts × three profiles is enough to verify host isolation and per-profile command distinctness without bloating runtime.One small gap worth considering: the test runs with the default
windowsMDMProfileDeleteBatchSize(10000), so the multi-batch boundary inBatchProcessSimpleisn't exercised here. If you want to cover the split-batch case as well, a variant that setsds.testDeleteMDMProfilesBatchSize(or the dedicated windows knob, if one is wired) to a small value like 2 would force multiple batches over the same 6 rows and validate that per-profileCASEarms are correctly emitted in each batch.🤖 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 4727 - 4807, Add a variant of testBatchDeleteMultipleWindowsProfiles that forces the multi-batch path by setting the datastore test batch size to a small value (e.g. ds.testDeleteMDMProfilesBatchSize = 2) before invoking batchSetMDMWindowsProfilesDB, run the same assertions, and then restore the original value; locate this change in the test function testBatchDeleteMultipleWindowsProfiles (or a new sibling test) and ensure the small batch size drives multiple BatchProcessSimple iterations so the CASE profile_uuid arms are exercised across batches.
1986-2007: Minor: identicalProfileNamefor all seeded rows reduces diagnostic signal.All seed payloads share
ProfileName: "test". That's harmless for the upsert (PK is(host_uuid, profile_uuid)), but if a future assertion fails on profile_name it'll be hard to tell rows apart. Consider deriving the name frompUUID(e.g."test-" + pUUID) so test failure messages point at the specific profile.🤖 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 1986 - 2007, The seeded payloads in installWindowsProfilesAsVerified use a constant ProfileName "test" which reduces diagnostic signal; update the loop that builds fleet.MDMWindowsBulkUpsertHostProfilePayload so ProfileName is derived from the profile UUID (e.g. concatenate a prefix like "test-" with pUUID) instead of the constant, leaving other fields and the call to ds.BulkUpsertMDMWindowsHostProfiles(t.Context(), payloads) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/datastore/mysql/microsoft_mdm_test.go`:
- Around line 4727-4807: Add a variant of testBatchDeleteMultipleWindowsProfiles
that forces the multi-batch path by setting the datastore test batch size to a
small value (e.g. ds.testDeleteMDMProfilesBatchSize = 2) before invoking
batchSetMDMWindowsProfilesDB, run the same assertions, and then restore the
original value; locate this change in the test function
testBatchDeleteMultipleWindowsProfiles (or a new sibling test) and ensure the
small batch size drives multiple BatchProcessSimple iterations so the CASE
profile_uuid arms are exercised across batches.
- Around line 1986-2007: The seeded payloads in installWindowsProfilesAsVerified
use a constant ProfileName "test" which reduces diagnostic signal; update the
loop that builds fleet.MDMWindowsBulkUpsertHostProfilePayload so ProfileName is
derived from the profile UUID (e.g. concatenate a prefix like "test-" with
pUUID) instead of the constant, leaving other fields and the call to
ds.BulkUpsertMDMWindowsHostProfiles(t.Context(), payloads) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b539fa6-1b2b-4a7a-a81d-c16173cb6a24
📒 Files selected for processing (1)
server/datastore/mysql/microsoft_mdm_test.go
Related issue: Resolves #42545
This rework does not significantly improve the worst case performance, but it does improve some cases (like lower number of hosts with a lot of profiles).
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.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