Fixed GET /api/v1/fleet/commands timeout in large Fleet deployments - #44297
Merged
Conversation
Reject `per_page > 1000` with a 400 to bound worst-case work on GET /api/v1/fleet/commands and GET /api/v1/fleet/mdm/commands. Default `per_page = 10` when omitted, matching the docs and the existing host-identifier path. Mirrors the activity bounded-context's pattern.
The old shape ran (apple_select) UNION ALL (windows_select) and applied ORDER BY ... LIMIT N OFFSET M to the merged result, forcing both halves to fully materialize before pagination. At ~420k Windows commands (10k+ hosts × ~40 commands each) this materialized half a million rows per request and timed out at the ALB. Push the team filter, request_type filter, cursor predicate, ORDER BY, and an inner LIMIT (page*per_page + per_page + 1) into each branch before the UNION ALL. With the per_page cap of 1000 from the prior commit, each branch now returns at most ~1001 rows, the merged set is at most ~2002, and the outer ORDER BY + LIMIT slices the requested page in O(per_page) work. getCombinedMDMCommandsQuery now wraps the new getMDMCommandsSubqueries helper for the single-command lookup at getMDMCommand, which still needs the WHERE-prefixed combined form. Refs #44170.
The Windows sub-statement joined mdm_windows_enrollments with
ON wmcq.enrollment_id = mwe.id OR wmcr.enrollment_id = mwe.id
The OR forced a nested-loop plan because no single index covers both
sides of the disjunction; on a 10k+ Windows-host loadtest this
contributed materially to the 90s timeout.
Split into two UNION ALL branches: one driven by command_queue
(rows pending or in flight), the other driven by command_results
(rows that produced a result and don't already appear in the queue;
the NOT EXISTS clause prevents double-counting). Each branch joins
mdm_windows_enrollments via a single equality, so the optimizer can
use the table's PRIMARY KEY (id).
Refs #44170.
Cap per_page at 1000, default to 10, push pagination into UNION branches, and replace the OR-join on the Windows side.
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Member
Author
|
/agentic_review |
Code Review by Qodo
1.
|
Drop SQL implementation details (UNION ALL, OR-join, indexable branches) in favor of the user-facing symptom (slow load times, timeouts) and the only behavior change that matters to a Fleet operator (the 1000 per_page cap and the default of 10).
Contributor
WalkthroughReworks MDM command listing: builds Apple and Windows branches separately, pushes ORDER BY/LIMIT into each UNION branch, and splits Windows into queue-driven and results-driven branches with deduplication via NOT EXISTS. Validates and defaults pagination/order options (Default per_page=10, Max per_page=1000, Max page=100), rejects PerPage==0, and enforces an order_key allowlist. Hostname is projected in SQL for host-scoped queries. Adjusts orbit/consumer call to request a single AccountConfiguration result. Adds/updates integration and datastore tests covering pagination, ordering, and cursor behavior. 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/datastore/mysql/mdm.go (1)
50-68:⚠️ Potential issue | 🟠 MajorInline
nano_view_queuehere instead of wrapping it.The Apple branch still reads from
nano_view_queue, which#44170identifies as the remaining materialization hotspot because the view bakes in its ownORDER BY. Putting the new per-branchLIMIToutside that view won't recover the intended index-driven plan, so large Apple fleets can still pay the full scan/materialization cost.🤖 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 50 - 68, The appleStmt query currently selects from the materialized view "nano_view_queue" which contains its own ORDER BY and forces view materialization; inline the view's underlying SELECT (replace the FROM nano_view_queue nvq with the view's SELECT projection/joins/filters) so there is no baked-in ORDER BY inside the referenced table expression, ensure any ORDER BY and LIMIT are applied outside this in the per-branch query, and keep the existing column aliases (nvq.id as host_uuid, nvq.command_uuid, etc.) so the appleStmt, its callers, and the host join (h.uuid) still match the symbols used in the code.
🤖 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.go`:
- Around line 162-167: The pagination helpers use
appendListOptionsWithCursorToSQL with user-controlled listOpts, allowing a
malicious order_key to inject SQL; replace both calls (the one inside
paginateBranch and the corresponding outer merge call) with
appendListOptionsWithCursorToSQLSecure and pass an explicit allowlist of allowed
order columns (e.g., the canonical host columns used elsewhere in this file).
Keep addRequestTypeFilter and ds.whereFilterHostsByTeams calls as-is, but ensure
you pass the same params/innerOpts variables to the secure helper and return the
wrapped SQL and params unchanged.
In `@server/service/mdm.go`:
- Around line 1058-1062: The per_page cap (req.ListOptions.PerPage vs
maxMDMCommandsPerPage) doesn't prevent large page values from making the
datastore compute branch limits as page*per_page + per_page; add validation for
req.ListOptions.Page (e.g., enforce a maxPage or compute maxPage =
floor(maxRowsAllowed / req.ListOptions.PerPage)) and return a
fleet.BadRequestError with a clear message when page is too large, or
alternatively require cursor-only pagination for deep traversal; apply the same
check where similar pagination is handled (the block around lines 1078-1080) so
oversized page values are rejected consistently.
---
Outside diff comments:
In `@server/datastore/mysql/mdm.go`:
- Around line 50-68: The appleStmt query currently selects from the materialized
view "nano_view_queue" which contains its own ORDER BY and forces view
materialization; inline the view's underlying SELECT (replace the FROM
nano_view_queue nvq with the view's SELECT projection/joins/filters) so there is
no baked-in ORDER BY inside the referenced table expression, ensure any ORDER BY
and LIMIT are applied outside this in the per-branch query, and keep the
existing column aliases (nvq.id as host_uuid, nvq.command_uuid, etc.) so the
appleStmt, its callers, and the host join (h.uuid) still match the symbols used
in the code.
🪄 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: 41a70c4f-ed18-451c-a6ca-f3d8058414db
📒 Files selected for processing (4)
changes/44170-list-mdm-commands-perfserver/datastore/mysql/mdm.goserver/service/integration_mdm_test.goserver/service/mdm.go
Contributor
There was a problem hiding this comment.
Pull request overview
Improves performance and reliability of the MDM command listing endpoints (GET /api/v1/fleet/commands and deprecated /mdm/commands) in large deployments by bounding pagination and restructuring the underlying MySQL query to avoid expensive UNION materialization and non-indexable joins.
Changes:
- Add endpoint-level pagination defaults/caps (
per_pagedefaults to 10, max 1000) with validation and integration test coverage. - Refactor the datastore query to push pagination into each UNION branch and replace the Windows
ORenrollment join with two UNION-able, index-friendly branches. - Add a changelog entry describing the performance fix.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/service/mdm.go | Enforces per-page default and maximum for the commands list endpoint. |
| server/service/integration_mdm_test.go | Adds integration assertions for per_page cap behavior. |
| server/datastore/mysql/mdm.go | Reworks combined Apple/Windows commands listing query to paginate per-branch and avoid OR joins. |
| changes/44170-list-mdm-commands-perf | Documents the performance fix in the changelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes from bot reviews on PR #44297: - SQL injection (CodeRabbit, critical): a crafted order_key reached ORDER BY through appendListOptionsWithCursorToSQL, which has no allowlist. Switch both the inner-branch and outer-merge calls to the Secure variant with an explicit allowlist of supported columns (host_uuid, command_uuid, status, updated_at, request_type, hostname). The helper's InvalidOrderKeyError is a 422 validation error. - Page cap (CodeRabbit + Qodo, perf): the per_page cap from the prior commit didn't bound the page parameter, so ?page=500000&per_page=1000 still asked each branch for ~500M rows. Cap page at 100 (max in-bounds offset 100*1000 = 100,000) and recommend cursor pagination for deeper traversal. - Cursor + page interplay (Copilot): zero outerOpts.Page when After is set so the outer query does not OFFSET past per-branch cursor results. - Datastore-layer defaults (Copilot): default OrderKey=updated_at and PerPage=10 inside ListMDMCommands path B for callers that bypass the endpoint, matching path A's hygiene. - Drop dead host_filter parameter (Copilot + Qodo): the parameter was always "" in practice, and the existing implementation had a latent UNION-ALL-only-second-branch-filtered bug if it ever became non-empty. Path A handles host scoping; remove the dead parameter.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44297 +/- ##
==========================================
+ Coverage 66.75% 66.78% +0.02%
==========================================
Files 2633 2634 +1
Lines 211720 211858 +138
Branches 9505 9538 +33
==========================================
+ Hits 141338 141480 +142
+ Misses 57541 57534 -7
- Partials 12841 12844 +3
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:
|
The helper had two callers (getMDMCommandsSubqueries and getCombinedMDMCommandsQuery), both of which lost their hostFilter parameter in the prior commit. The full golangci-lint suite (unused linter) flagged it; the incremental lint missed it because the function was already on main and only its callers changed. Also drop the dangling reference to it in two doc comments inside mdm.go that contrasted the old approach.
Member
Author
|
@JordanMontgomery FYI. I'm making optimizations to the |
The unscoped path (path B) already validates order_key against an allowlist via appendListOptionsWithCursorToSQLSecure, but the host-scoped path (listMDMCommandsByHostIdentifier, path A) was still using the non-secure helper. That left the same SQL injection / information-disclosure surface CodeRabbit flagged on path B, just under a different query string. Lift the allowlist to a package var (mdmCommandsOrderAllowlist) and have both paths reference it. Drop hostname from the allowlist: path A doesn't project hostname in SQL (it's merged from byUUID after the SELECT), so allowing it would have errored on path A, and a single-host scope makes hostname sort meaningless anyway. Allowed order_keys: host_uuid, command_uuid, status, updated_at, request_type. Other values get a 422 from either path.
Path A's apple and windows branches now JOIN hosts and project h.hostname, mirroring what path B's subqueries already do. The mdmCommandsOrderAllowlist gets hostname back, so order_key=hostname works on both paths with honest semantics (the SQL actually orders by hostname, which matters for cursor pagination correctness on the unscoped path). The byUUID merge in path A no longer needs to set Hostname (SQL provides it); team_id still merges from byUUID since it's not in any projection. Also extends the integration test to assert order_key=hostname returns 200 on both the unscoped and host-scoped paths. The hosts JOIN is keyed on hosts.uuid (idx_hosts_uuid), which combined with the existing host_identifier filter is a primary-key seek per host — negligible cost.
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Member
Author
|
/agentic_review |
|
Persistent review updated to latest commit aac5461 |
Member
Author
|
@claude review once |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/datastore/mysql/mdm.go (2)
53-71:⚠️ Potential issue | 🟠 MajorInline the Apple branch instead of reading
nano_view_queue.The all-hosts path still pulls Apple commands from
nano_view_queue, so the optimizer can still inherit the view’s baked-in join/order work before your per-branchLIMITruns. That leaves the Apple side ofGET /api/v1/fleet/commandsexposed to the same O(total_commands) behavior this PR is trying to remove.
486-500:⚠️ Potential issue | 🟠 MajorKeep list params separate from count params.
appendListOptionsWithCursorToSQLSecureappends the cursor value toparams. On host-scoped requests that filter to pending commands and useafter,countStmtis executed with that extra bind even though the count SQL has no cursor placeholder, so this path will fail at runtime.🛠️ Minimal fix
- var listStmt, countStmt string - var params []any + var listStmt, countStmt string + var params []any // Wrap in `SELECT * FROM (...) u WHERE TRUE` so the cursor and ORDER BY // predicates resolve against the unambiguous `u` projection — the inner // branches join multiple tables that all expose `command_uuid` / `updated_at`. // `WHERE TRUE` is required because the cursor helper picks AND vs WHERE by // substring-matching "where", picks AND from the inner branches, and would @@ case len(winUUIDs) > 0: listStmt = `SELECT * FROM (` + winStmt + `) u WHERE TRUE` countStmt = `SELECT COUNT(1) FROM (` + winStmt + `) u` params = winParams } + countParams := append([]any{}, params...) @@ - listStmt, params, err = appendListOptionsWithCursorToSQLSecure(listStmt, params, &listOpts.ListOptions, mdmCommandsOrderAllowlist) + listParams, params, err := appendListOptionsWithCursorToSQLSecure(listStmt, params, &listOpts.ListOptions, mdmCommandsOrderAllowlist) if err != nil { return nil, nil, nil, ctxerr.Wrap(ctx, err, "list commands pagination") } + listStmt = listParams @@ - if err := sqlx.SelectContext(ctx, ds.reader(ctx), &results, listStmt, params...); err != nil { + if err := sqlx.SelectContext(ctx, ds.reader(ctx), &results, listStmt, params...); err != nil { return nil, nil, nil, ctxerr.Wrap(ctx, err, "list commands") } @@ - if err := sqlx.GetContext(ctx, ds.reader(ctx), &total, countStmt, params...); err != nil { + if err := sqlx.GetContext(ctx, ds.reader(ctx), &total, countStmt, countParams...); err != nil { return nil, nil, nil, ctxerr.Wrap(ctx, err, "count commands") } }🤖 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 486 - 500, The code reuses the same params slice for both listStmt and countStmt; appendListOptionsWithCursorToSQLSecure appends a cursor to params which causes countStmt to be executed with an extra bind and fail. Fix by preserving a separate params slice for the count before appending the cursor: copy the original params (e.g., paramsForCount := append([]interface{}(nil), params...)) then call appendListOptionsWithCursorToSQLSecure to get the params for list (leave paramsForCount unchanged) and use paramsForCount with countStmt; update references around appendListOptionsWithCursorToSQLSecure, listStmt, countStmt, params and listOpts.ListOptions accordingly.
🧹 Nitpick comments (1)
server/datastore/mysql/mdm_test.go (1)
770-789: Exercise the Windows results-backed branch in this pagination setup too.This fixture only creates queued Windows commands, so the all-hosts pagination assertions never hit the results-backed Windows UNION branch or its dedupe path. A regression there would still pass this test. Consider marking one Windows command as responded before the page/cursor assertions and verifying its UUID still appears exactly once across the walk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/mdm_test.go` around lines 770 - 789, The test currently only enqueues Windows commands (via ds.MDMWindowsInsertCommandForHosts) so the results-backed Windows branch isn't exercised; update the fixture to mark one of the inserted Windows commands as responded before the pagination/cursor assertions by taking one cmdUUID from allUUIDs (or capture the cmdUUID when creating Windows commands) and inserting a corresponding result for that UUID (e.g., call the datastore helper that records Windows command results such as ds.MDMWindowsInsertCommandResult or the existing test helper that writes a command response for winDeviceID and the chosen cmdUUID), then proceed with the pagination walk and assert the chosen UUID still appears exactly once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/datastore/mysql/mdm.go`:
- Around line 486-500: The code reuses the same params slice for both listStmt
and countStmt; appendListOptionsWithCursorToSQLSecure appends a cursor to params
which causes countStmt to be executed with an extra bind and fail. Fix by
preserving a separate params slice for the count before appending the cursor:
copy the original params (e.g., paramsForCount := append([]interface{}(nil),
params...)) then call appendListOptionsWithCursorToSQLSecure to get the params
for list (leave paramsForCount unchanged) and use paramsForCount with countStmt;
update references around appendListOptionsWithCursorToSQLSecure, listStmt,
countStmt, params and listOpts.ListOptions accordingly.
---
Nitpick comments:
In `@server/datastore/mysql/mdm_test.go`:
- Around line 770-789: The test currently only enqueues Windows commands (via
ds.MDMWindowsInsertCommandForHosts) so the results-backed Windows branch isn't
exercised; update the fixture to mark one of the inserted Windows commands as
responded before the pagination/cursor assertions by taking one cmdUUID from
allUUIDs (or capture the cmdUUID when creating Windows commands) and inserting a
corresponding result for that UUID (e.g., call the datastore helper that records
Windows command results such as ds.MDMWindowsInsertCommandResult or the existing
test helper that writes a command response for winDeviceID and the chosen
cmdUUID), then proceed with the pagination walk and assert the chosen UUID still
appears exactly once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac06ba82-362c-4fb0-98f3-aa3f6ac47652
📒 Files selected for processing (10)
changes/44170-list-mdm-commands-perfchanges/44422-list-mdm-commands-host-identifier-afteree/server/service/orbit.goserver/datastore/mysql/mdm.goserver/datastore/mysql/mdm_test.goserver/datastore/mysql/mysql.goserver/fleet/mdm.goserver/service/integration_mdm_test.goserver/service/mdm.goserver/service/orbit.go
💤 Files with no reviewable changes (1)
- server/datastore/mysql/mysql.go
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
# Conflicts: # server/datastore/mysql/mdm.go
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
Apr 30, 2026
| h.team_id, | ||
| nvq.name | ||
| FROM | ||
| nano_view_queue nvq |
Contributor
There was a problem hiding this comment.
#44170
Lists
nano_view_queue is a VIEW with ORDER BY baked in (migration 20260409153713_AddNameToNanoCommands). MySQL re-materializes the view (3-way join over nano_enrollment_queue, nano_commands, nano_command_results with ORDER BY q.priority DESC, q.created_at) on every list query. No index can help because the ordering is post-join.
Is that still a problem here?
Member
Author
There was a problem hiding this comment.
Yes, but I didn't want to expand the scope beyond Windows. It affects the now-deprecated code path. I filed a separate issue: #44509
Contributor
There was a problem hiding this comment.
ahh gotcha! Sounds great!
Closed
2 tasks
ksykulev
reviewed
Apr 30, 2026
ksykulev
left a comment
ksykulev
left a comment
Contributor
There was a problem hiding this comment.
I think this looks good. One last question.
| WHERE NOT EXISTS ( | ||
| SELECT 1 FROM windows_mdm_command_queue wmcq2 | ||
| WHERE wmcq2.command_uuid = wmc.command_uuid AND wmcq2.enrollment_id = mwe.id | ||
| ) |
Contributor
There was a problem hiding this comment.
This is covered by the index correct? Does the optimizer pivot to start from enrollment id?
PRIMARY KEY (enrollment_id,command_uuid), <--
Member
Author
There was a problem hiding this comment.
Yes, covered by the index. The optimizer pivots enrollment_id into the leading position automatically.
Member
Author
|
@ksykulev Ready for re-review |
ksykulev
approved these changes
Apr 30, 2026
5 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 #44170 and Resolves #44422
Pagination is now pushed into each branch of the merged query, so per-tick work scales with page size instead of total commands. The Windows side was rewritten to avoid a disjunctive join that forced a nested-loop plan.
per_pageis capped (default 10),pageis capped, andorder_keyis enforced against a closed allowlist on both code paths. Cursor pagination is fixed and is the recommended way to traverse beyond the page cap.This PR improves but does not fix the use case of fetching commands from all hosts. Deprecate usage without host_identifier: https://github.com/fleetdm/fleet/pull/44392/changes
API doc updates: #44292
Checklist for submitter
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
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.If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary 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
New Features
Bug Fixes / Performance
Validation
Tests