fleetd Windows MDM wake (push vs poll)#46594
Merged
Merged
Conversation
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Actions performedFull review triggered. |
Code Review by Qodo
1. Deviceenroller lacks timeout
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a partial server-side step toward replacing Fleet's aggressive 1-minute Windows MDM polling (issue #43773). Instead of WNS push notifications, it introduces an in-band signal on the orbit/config polling path: when the server detects queued, unacknowledged Windows MDM commands for a host, it sets a new WindowsMDMSyncRequest notification, gated on a new CapabilityWindowsMDMSync capability. To avoid an extra DB round trip per check-in, the existing GetMDMWindowsAwaitingConfigurationByHostUUID lookup is folded into a new combined query GetMDMWindowsHostConfigState that also returns whether the host has pending commands.
Changes:
- New datastore method
GetMDMWindowsHostConfigStatereturning awaiting-configuration + has-pending-commands in one query; old method is reimplemented on top of it. - New
OrbitConfigNotifications.WindowsMDMSyncRequestfield andCapabilityWindowsMDMSync, withGetOrbitConfigsetting the field when commands are queued and the client advertises the capability. - Datastore mock and tests (mysql + orbit service) updated to cover the new state and capability gating.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/fleet/capabilities.go | Adds CapabilityWindowsMDMSync and advertises it from Windows fleetd. |
| server/fleet/orbit.go | Adds WindowsMDMSyncRequest notification field. |
| server/fleet/microsoft_mdm.go | Adds MDMWindowsHostConfigState type. |
| server/fleet/datastore.go | Declares new GetMDMWindowsHostConfigState datastore method. |
| server/datastore/mysql/microsoft_mdm.go | Implements combined awaiting-configuration + pending-commands query; old method delegates. |
| server/datastore/mysql/microsoft_mdm_test.go | New integration test covering enrollment, pending command, ack, and awaiting-configuration. |
| server/service/orbit.go | Switches orbit config path to combined query and sets new sync notification gated on capability. |
| server/service/orbit_test.go | Updates existing tests and adds cases for capability gating and ESP-vs-sync precedence. |
| server/mock/datastore_mock.go | Generated mock for the new datastore method. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThis PR adds a combined per-host Windows MDM config read (awaiting-configuration + denormalized has_pending_commands), persists a PollScheduleRelaxed flag on enrollments with a migration/backfill, recomputes has_pending_commands in command/response flows, reconciles DMClient poll schedule from management responses (enqueuing SyncML Replace when needed), introduces an Orbit receiver that triggers on‑demand Windows MDM syncs (with platform exec), and updates mocks and tests across datastore, service, orbit, and migrations. Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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.
🧹 Nitpick comments (1)
server/datastore/mysql/microsoft_mdm_test.go (1)
1975-2031: ⚡ Quick winAdd a re-enrollment case for
HasPendingCommands.This only exercises a single enrollment. The risky regression here is a host with multiple
mdm_windows_enrollmentsrows: if an older enrollment still has a queued command,GetMDMWindowsHostConfigStatecan incorrectly reportHasPendingCommands=truefor the current device and keep orbit sending spurious Windows MDM sync notifications. Please add a case that leaves a pending command on a superseded enrollment and asserts only the latest enrollment is considered.🤖 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 1975 - 2031, Add a re-enrollment scenario to ensure GetMDMWindowsHostConfigState only considers the latest enrollment: create a second enrollment for the same HostUUID (call MDMWindowsInsertEnrolledDevice again with a new MDMDeviceID/MDMHardwareID but same HostUUID), fetch both enrollment IDs via mdmWindowsEnrollmentIDByHardwareID, insert a pending command tied to the older enrollment_id directly (use ds.writer().ExecContext to INSERT into windows_mdm_commands or the appropriate commands table with enrollment_id = oldEnrollmentID), then call GetMDMWindowsHostConfigState(hostUUID) and assert HasPendingCommands is false (only the most recent enrollment should be considered); reference the test function testMDMWindowsGetHostConfigState and methods MDMWindowsInsertEnrolledDevice, mdmWindowsEnrollmentIDByHardwareID, GetMDMWindowsHostConfigState, and ds.writer() to locate where to add this case.
🤖 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.
Nitpick comments:
In `@server/datastore/mysql/microsoft_mdm_test.go`:
- Around line 1975-2031: Add a re-enrollment scenario to ensure
GetMDMWindowsHostConfigState only considers the latest enrollment: create a
second enrollment for the same HostUUID (call MDMWindowsInsertEnrolledDevice
again with a new MDMDeviceID/MDMHardwareID but same HostUUID), fetch both
enrollment IDs via mdmWindowsEnrollmentIDByHardwareID, insert a pending command
tied to the older enrollment_id directly (use ds.writer().ExecContext to INSERT
into windows_mdm_commands or the appropriate commands table with enrollment_id =
oldEnrollmentID), then call GetMDMWindowsHostConfigState(hostUUID) and assert
HasPendingCommands is false (only the most recent enrollment should be
considered); reference the test function testMDMWindowsGetHostConfigState and
methods MDMWindowsInsertEnrolledDevice, mdmWindowsEnrollmentIDByHardwareID,
GetMDMWindowsHostConfigState, and ds.writer() to locate where to add this case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4523e48-9a2d-43ab-b11c-3ec6a6ba9232
📒 Files selected for processing (9)
server/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/fleet/capabilities.goserver/fleet/datastore.goserver/fleet/microsoft_mdm.goserver/fleet/orbit.goserver/mock/datastore_mock.goserver/service/orbit.goserver/service/orbit_test.go
![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 #46594 +/- ##
========================================
Coverage 66.91% 66.91%
========================================
Files 2833 2836 +3
Lines 224964 225185 +221
Branches 11620 11620
========================================
+ Hits 150526 150676 +150
- Misses 60781 60821 +40
- Partials 13657 13688 +31
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:
|
Relax the Windows MDM DMClient poll schedule in-session, gated on the host's fleetd (orbit) version and reconciled against a persisted applied-state column. - getPollScheduleCommands: desired-relaxed (orbit >= 1.56.0, via GetHostOrbitInfo) vs applied (mdm_windows_enrollments.poll_schedule_relaxed). Emits one Replace on .../Poll/IntervalForFirstSetOfRetries (60 relax / 1 restore) only when they differ. - windowsMDMHostSupportsSync: missing/unlinked host or unknown version => not-capable (stay on the fast poll, the safe default). - Migration 20260601204606 adds poll_schedule_relaxed TINYINT(1) NOT NULL DEFAULT 0. - Datastore setter + enrolled-device SELECT + interface + regenerated mock. - Tests: TestGetPollScheduleCommands, testMDMWindowsPollScheduleRelaxed, migration test.
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Actions performedFull review triggered. |
|
Code review by qodo was updated up to the latest commit 1527c70 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
server/fleet/datastore.go (1)
2170-2173: ⚡ Quick winDeprecate or narrow the older orbit-path getter.
GetMDMWindowsAwaitingConfigurationByHostUUIDstill advertises itself as the orbit config polling API, so adding this consolidated getter leaves two interface contracts for the same path. That makes it easy for a future caller to pick the older one-field method and reintroduce the extra datastore round trip this PR is trying to eliminate. Consider marking the older method deprecated or updating its doc to steer orbit callers here instead.🤖 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/fleet/datastore.go` around lines 2170 - 2173, The older single-field getter GetMDMWindowsAwaitingConfigurationByHostUUID is now redundant and risks reintroducing extra datastore calls; update its declaration comment to mark it deprecated and clearly steer callers to use the new consolidated GetMDMWindowsHostConfigState(ctx, hostUUID string) method (mention the new method by name in the comment), or if you prefer, add a deprecation comment annotation above GetMDMWindowsAwaitingConfigurationByHostUUID and update any public docs/comments to instruct orbit config polling callers to use GetMDMWindowsHostConfigState instead so future developers choose the consolidated API.
🤖 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 `@orbit/pkg/update/execwinapi_windows.go`:
- Around line 234-261: The function fleetMDMEnrollmentGUID currently treats
EnrollmentState==1 as active; update the logic in fleetMDMEnrollmentGUID to
consider EnrollmentState==4 (the typical active/working MDM state) or accept
both 1 and 4 to be safe: change the enrollmentStateActive constant or the
comparison in the loop that checks state (the block that reads "EnrollmentState"
and compares it to enrollmentStateActive) so it matches 4 (or checks state == 1
|| state == 4) when deciding an active Fleet MDM enrollment; keep the rest of
the registry reading/validation (ProviderID check and providerID ==
fleetMDMProviderID) unchanged.
- Around line 209-229: The comment and behavior in TriggerWindowsMDMSync that
call deviceenroller.exe with "/o <GUID> /c" and assert that avoiding "/c /z"
avoids Event 4603 lack provenance; update the comment above deviceEnroller or at
the top of TriggerWindowsMDMSync to cite concrete sources or tests (e.g.,
Microsoft docs/KB, blog posts, or internal repro ticket) that document the
deviceenroller.exe CLI flags and the Event 4603 behavior for specific Windows
builds, or explicitly state that the flags and the Event 4603 mitigation are
based on observed behavior and list the tested Windows versions and reproduction
steps; include links/IDs and the exact OS versions tested so reviewers can
verify the claim.
In `@server/datastore/mysql/microsoft_mdm_test.go`:
- Around line 2010-2066: Add a re-enrollment scenario to
testMDMWindowsGetHostConfigState: insert a second MDMWindowsEnrolledDevice for
the same HostUUID (distinct MDMDeviceID/MDMHardwareID) via
MDMWindowsInsertEnrolledDevice so there are two enrollment rows; use
mdmWindowsEnrollmentIDByHardwareID to fetch the enrollment IDs for both rows,
then insert a pending command and/or responses against the older (stale)
enrollment via MDMWindowsInsertCommandForHosts and direct writer ExecContext but
leave the newer enrollment with no pending commands and set awaiting
configuration on the newest enrollment with SetMDMWindowsAwaitingConfiguration;
finally call GetMDMWindowsHostConfigState and assert the combined state is taken
from the latest enrollment only (AwaitingConfiguration matches the new
enrollment and HasPendingCommands is false despite pending data on the stale
enrollment) to ensure queries are scoped to the most recent enrollment.
In
`@server/datastore/mysql/migrations/tables/20260601204606_AddPollScheduleRelaxedToMDMWindowsEnrollments.go`:
- Around line 23-24: The Down_20260601204606 migration is a no-op; implement it
to reverse the Up change by removing the added column so downgrades work: in
Down_20260601204606(tx *sql.Tx) execute an ALTER TABLE mdm_windows_enrollments
DROP COLUMN poll_schedule_relaxed (using tx.Exec), handle and return any error
from Exec, and ensure the function returns nil only on success; this mirrors the
Up that added poll_schedule_relaxed and restores the schema to its prior state.
In `@server/service/microsoft_mdm.go`:
- Around line 2087-2089: The code is optimistically persisting
PollScheduleRelaxed via svc.ds.SetMDMWindowsEnrollmentPollScheduleRelaxed when
emitting the SyncML Replace, which can mark the state reconciled before the
device actually applies it; change the flow so that you only update the stored
PollScheduleRelaxed after the device acknowledges the command (i.e., on the
confirmation/ack handler), or instead enqueue the Replace through the existing
retryable command pipeline and derive/update the DB state when that retryable
command reports success. Locate usages of
SetMDMWindowsEnrollmentPollScheduleRelaxed, the Replace emission in the SyncML
response, and the device acknowledgment handler or retryable command codepath
and move the DB write there (or wire the Replace into the retryable command
queue) so failed/dropped responses will be retried and the DB only flips when
the device confirms the change.
---
Nitpick comments:
In `@server/fleet/datastore.go`:
- Around line 2170-2173: The older single-field getter
GetMDMWindowsAwaitingConfigurationByHostUUID is now redundant and risks
reintroducing extra datastore calls; update its declaration comment to mark it
deprecated and clearly steer callers to use the new consolidated
GetMDMWindowsHostConfigState(ctx, hostUUID string) method (mention the new
method by name in the comment), or if you prefer, add a deprecation comment
annotation above GetMDMWindowsAwaitingConfigurationByHostUUID and update any
public docs/comments to instruct orbit config polling callers to use
GetMDMWindowsHostConfigState instead so future developers choose the
consolidated API.
🪄 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: 5b5df59b-8b7b-4685-a25b-6f2f4568f13e
📒 Files selected for processing (19)
orbit/cmd/orbit/orbit.goorbit/pkg/update/execwinapi_stub.goorbit/pkg/update/execwinapi_windows.goorbit/pkg/update/notifications.goorbit/pkg/update/notifications_test.goserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/datastore/mysql/migrations/tables/20260601204606_AddPollScheduleRelaxedToMDMWindowsEnrollments.goserver/datastore/mysql/migrations/tables/20260601204606_AddPollScheduleRelaxedToMDMWindowsEnrollments_test.goserver/datastore/mysql/schema.sqlserver/fleet/capabilities.goserver/fleet/datastore.goserver/fleet/microsoft_mdm.goserver/fleet/orbit.goserver/mock/datastore_mock.goserver/service/microsoft_mdm.goserver/service/microsoft_mdm_test.goserver/service/orbit.goserver/service/orbit_test.go
…dm-wake # Conflicts: # server/datastore/mysql/schema.sql # server/service/microsoft_mdm_test.go
…edule setter; tidy tests
…dm-wake # Conflicts: # server/datastore/mysql/schema.sql
Member
Author
|
@claude review once |
![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.
getvictor
changed the title
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
ksykulev
reviewed
Jun 3, 2026
ksykulev
left a comment
ksykulev
left a comment
Contributor
There was a problem hiding this comment.
Now that these commands are through orbit they will get logged. The GET /api/v1/fleet/mdm/commands will return these? Do we want to filter them or do we not care?
| HasPendingCommands bool `db:"has_pending_commands"` | ||
| FleetdSyncCapable bool `db:"fleetd_sync_capable"` | ||
| } | ||
| if err := sqlx.GetContext(ctx, ds.reader(ctx), &row, stmt, hostUUID); err != nil { |
Contributor
There was a problem hiding this comment.
Is it ok that we read from the replica here? has_pending_commands is written on the primary right?
Member
Author
There was a problem hiding this comment.
Yes, we are talking about 30 second queries here from orbit, so we are not on a tight budget. Also, this read is on a hot path, so should use replica here.
Member
Author
@ksykulev The MDM commands are not through orbit. orbit tells Windows to sync with MDM, so Windows still initiates the MDM session and pulls down any commands (as before). We are adding the new command to change the Poll frequency, which I expect to only happen once in the lifetime of the host, and that will show up in mdm/commands. Didn't seem worth it to filter/hide it. |
2 tasks
ksykulev
approved these changes
Jun 3, 2026
ksykulev
left a comment
ksykulev
left a comment
Contributor
There was a problem hiding this comment.
I mis-understood the mechanism. This looks good then!
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 #46567 and Resolves #46737
Solution for the agressive polling:
mdm_windows_enrollments.fleetd_sync_capablepoll_schedule_relaxed)has_pending_commandsflips, the next orbit check-in returnsWindowsMDMSyncRequest, and fleetd runsdeviceenrollerto deliver it immediatelyDocs: #46780
Changes to osquery_perf and any additional changes after loadtesting will be done in a separate PR.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Timeouts are implemented and retries are limited to avoid infinite loops
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).fleetd/orbit/Fleet Desktop
runtime.GOOSis used as needed to isolate changesSummary by CodeRabbit
New Features
Enhancements
Tests