Reconcile stuck Android MDM commands via AMAPI operations.get - #50177
Merged
Conversation
Android Lock/Wipe/Clear-passcode rows in mdm_android_commands only leave the pending status when the AMAPI Pub/Sub COMMAND notification arrives. If that notification is never delivered, the row stays pending forever, the host reads as perpetually pending, and the admin cannot re-issue the command. Add a daily mdm_android_command_reconciler cron that polls AMAPI enterprises.devices.operations.get for commands pending more than a day and writes the authoritative outcome. The pending-to-terminal transition (including the post-WIPE-ack unenroll side effects) is factored out of the Pub/Sub handler so both paths share it.
dantecatalfamo
had a problem deploying
to
Docker Hub
July 29, 2026 19:25 — with 
GitHub Actions
Failure
GitHub Actions
Failure
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
Adds an Android MDM “command reconciler” path that periodically polls AMAPI enterprises.devices.operations.get to transition long-stuck mdm_android_commands rows out of pending when Pub/Sub COMMAND notifications are dropped, unblocking re-issuing Lock/Wipe/Clear-passcode commands. This complements the existing Pub/Sub-driven lifecycle by sharing the same “pending → terminal” mapping and WIPE side effects.
Changes:
- Add daily cron
mdm_android_command_reconcilerthat lists old pending Android commands and reconciles them via AMAPIoperations.get, with rate limiting and NotFound grace handling. - Refactor Pub/Sub COMMAND handling to reuse shared helpers for terminal-state mapping and WIPE-ack side effects.
- Add fleetdm.com proxy route/controller for
operations.get, plus datastore query + tests for listing pending Android commands.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| website/config/routes.js | Adds proxy route for Android operations.get. |
| website/api/controllers/android-proxy/get-android-device-operation.js | New proxy controller to fetch an AMAPI operation for a device. |
| tools/hangar/frontend/src/lib/fleetctlCrons.ts | Exposes the new cron in Hangar’s cron list. |
| server/mock/datastore_mock.go | Adds datastore mock support for listing pending Android commands. |
| server/mdm/android/service/reconcile_commands.go | Implements reconciler cron logic and AMAPI polling. |
| server/mdm/android/service/reconcile_commands_test.go | Unit tests for reconciliation behavior and edge cases. |
| server/mdm/android/service/pubsub.go | Reuses shared terminal-state + wipe side-effect helpers from Pub/Sub handler. |
| server/mdm/android/service/androidmgmt/proxy_client.go | Adds EnterprisesDevicesOperationsGet to proxy client. |
| server/mdm/android/service/androidmgmt/google_client.go | Adds EnterprisesDevicesOperationsGet to direct Google client. |
| server/mdm/android/service/androidmgmt/client.go | Extends client interface and adds helpers to classify 404/429 errors. |
| server/mdm/android/mock/client.go | Adds mock client support for operations.get. |
| server/fleet/datastore.go | Extends Android datastore interface with list-pending method. |
| server/fleet/cron_schedules.go | Defines new cron schedule name constant. |
| server/datastore/mysql/android.go | Implements list-pending query for Android commands. |
| server/datastore/mysql/android_test.go | Adds MySQL tests for list-pending query semantics. |
| cmd/fleet/cron.go | Adds new cron schedule wiring for the Android command reconciler. |
| cmd/fleet/cron_registration.go | Registers the new cron under MDM crons. |
| changes/46145-android-command-reconcile.md | Changes file present but excluded from diff per policy. |
Files excluded by content exclusion policy (1)
- changes/46145-android-command-reconcile.md
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds daily reconciliation for aged pending Android MDM commands through AMAPI Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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
🧹 Nitpick comments (1)
server/datastore/mysql/android.go (1)
1180-1197: 🚀 Performance & Scalability | 🔵 TrivialVerify
mdm_android_commandshas a supporting index for(status, created_at).The query filters on
statusandcreated_at, then sorts bycreated_atand appliesLIMIT. This runs daily against a table that grows with every Lock/Wipe/Clear-passcode command. Please confirm an index covering(status, created_at)exists (or add one via migration) so this doesn't degrade into a full scan as the table grows.As per path instructions, SQL query filtering/injection has been reviewed:
"When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied... Review all SQL queries for possible SQL injection."The filtering and parameterization here are correct; this is purely an indexing/performance follow-up.#!/bin/bash # Look for an existing migration creating/altering mdm_android_commands with an index on (status, created_at). rg -n "mdm_android_commands" server/datastore/mysql/migrations --include='*.go' -l 2>/dev/null | xargs -r rg -n -i "INDEX|KEY"🤖 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/android.go` around lines 1180 - 1197, Verify that mdm_android_commands has an index with status as the leading column and created_at as the second column to support ListPendingMDMAndroidCommands. If no suitable index exists, add it through the project’s standard MySQL migration mechanism, preserving the existing query and parameterization.
🤖 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/mdm/android/service/pubsub.go`:
- Around line 230-248: Update setAndroidCommandTerminalState so the acknowledged
WIPE path invokes androidWipeAckUnenroll before UpdateMDMAndroidCommandStatus;
return immediately on side-effect failure so the command remains pending for
retry. Preserve the existing status update behavior for non-WIPE commands and
other terminal states.
In `@website/api/controllers/android-proxy/get-android-device-operation.js`:
- Around line 26-33: Add a dedicated 429 exit such as tooManyRequests with
responseType set to the framework’s 429 response mapping in the Android device
operation action’s exits, then update the 429 AMAPI intercept to return that
exit key instead of a generic Error. Preserve the existing typed exits for 403
and 404 responses.
---
Nitpick comments:
In `@server/datastore/mysql/android.go`:
- Around line 1180-1197: Verify that mdm_android_commands has an index with
status as the leading column and created_at as the second column to support
ListPendingMDMAndroidCommands. If no suitable index exists, add it through the
project’s standard MySQL migration mechanism, preserving the existing query and
parameterization.
🪄 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 Plus
Run ID: ea8d050c-cde2-4b22-ad84-251cea239806
⛔ Files ignored due to path filters (1)
changes/46145-android-command-reconcile.mdis excluded by!**/*.md
📒 Files selected for processing (17)
cmd/fleet/cron.gocmd/fleet/cron_registration.goserver/datastore/mysql/android.goserver/datastore/mysql/android_test.goserver/fleet/cron_schedules.goserver/fleet/datastore.goserver/mdm/android/mock/client.goserver/mdm/android/service/androidmgmt/client.goserver/mdm/android/service/androidmgmt/google_client.goserver/mdm/android/service/androidmgmt/proxy_client.goserver/mdm/android/service/pubsub.goserver/mdm/android/service/reconcile_commands.goserver/mdm/android/service/reconcile_commands_test.goserver/mock/datastore_mock.gotools/hangar/frontend/src/lib/fleetctlCrons.tswebsite/api/controllers/android-proxy/get-android-device-operation.jswebsite/config/routes.js
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #50177 +/- ##
==========================================
- Coverage 68.50% 68.42% -0.09%
==========================================
Files 3974 3966 -8
Lines 255606 255507 -99
Branches 13658 13556 -102
==========================================
- Hits 175108 174834 -274
- Misses 64901 65059 +158
- Partials 15597 15614 +17
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:
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
dantecatalfamo
temporarily deployed
to
Docker Hub
July 30, 2026 16:44 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
- Run the WIPE-ack side effects before the status write so a transient failure leaves the row pending and retriable instead of stranding an acknowledged row the reconciler can never select again. - Break ORDER BY ties on command_uuid so batching is stable across runs. - Log loudly when the Fleet server secret is missing, and stop the run when AMAPI rejects our credentials rather than working through the batch on 401s. - Keep 429 and 403 distinct from 404 on the new proxy route so the reconciler can tell rate limiting and lost access apart from an unknown operation.
dantecatalfamo
temporarily deployed
to
Docker Hub
July 30, 2026 16:52 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
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 17 out of 18 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/46145-android-command-reconcile.md
Comments suppressed due to low confidence (1)
server/mdm/android/service/reconcile_commands.go:104
- The "remaining" count in this warning log includes the current command (index i). At i=0 it reports all commands as remaining, even though one has already been processed/attempted. Use len(cmds)-(i+1) so the count reflects rows left after the current one.
return ctxerr.Wrap(ctx, ctx.Err(), "android command reconcile interrupted")
}
}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/mdm/android/service/pubsub.go`:
- Around line 245-249: Make WIPE acknowledgment handling retryable so the host
unenrollment transition and mdm_unenrolled activity cannot be partially
committed: update androidWipeAckUnenroll and its caller in
server/mdm/android/service/pubsub.go (lines 245-249) to preserve pending
activity emission when newActivityFn fails, and ensure retries emit the activity
exactly once before acknowledgment. Add the two-run failure-then-retry coverage
in server/mdm/android/service/reconcile_commands_test.go (lines 216-241),
verifying unenrollment succeeds, the first activity attempt fails, and the retry
creates one activity before acknowledging.
🪄 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 Plus
Run ID: c4efdbb5-7241-4864-9326-7b1e961857f0
📒 Files selected for processing (6)
server/datastore/mysql/android.goserver/mdm/android/service/androidmgmt/client.goserver/mdm/android/service/pubsub.goserver/mdm/android/service/reconcile_commands.goserver/mdm/android/service/reconcile_commands_test.gowebsite/api/controllers/android-proxy/get-android-device-operation.js
🚧 Files skipped from review as they are similar to previous changes (1)
- server/datastore/mysql/android.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
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
Files excluded by content exclusion policy (1)
- changes/46145-android-command-reconcile.md
Comments suppressed due to low confidence (2)
server/mdm/android/service/androidmgmt/client.go:149
errors.AsType[*googleapi.Error]is not a standard library API; useerrors.As(as done inIsBadRequestError) so 401/403 can be reliably detected.
// IsAuthenticationError reports whether the AMAPI error indicates that the
// request was rejected over credentials or access, rather than anything about
// the resource that was requested.
func IsAuthenticationError(err error) bool {
if ae, ok := errors.AsType[*googleapi.Error](err); ok {
return ae.Code == http.StatusUnauthorized || ae.Code == http.StatusForbidden
}
return false
server/mdm/android/service/androidmgmt/client.go:158
errors.AsType[*googleapi.Error]is not a standard library API; useerrors.Asso 429 quota errors are correctly classified by callers.
// IsTooManyRequestsError reports whether the AMAPI error indicates that we
// exceeded the project's request quota.
func IsTooManyRequestsError(err error) bool {
if ae, ok := errors.AsType[*googleapi.Error](err); ok {
return ae.Code == http.StatusTooManyRequests
}
return false
Comment on lines
+133
to
+140
| // IsNotFoundError reports whether the AMAPI error indicates that the requested | ||
| // resource does not exist. | ||
| func IsNotFoundError(err error) bool { | ||
| if ae, ok := errors.AsType[*googleapi.Error](err); ok { | ||
| return ae.Code == http.StatusNotFound | ||
| } | ||
| return false | ||
| } |
Comment on lines
+83
to
+86
| }).intercept({status: 429}, (err)=>{ | ||
| // If the Android management API returns a 429 response, log an additional warning that will trigger a help-p1 alert. | ||
| sails.log.warn(`p1: Android management API rate limit exceeded! When attempting to get a device operation for an Android enterprise (${androidEnterpriseId}), an error occurred. Error: ${require('util').inspect(err)}`); | ||
| // Pass the 429 through to the Fleet server rather than collapsing it into a 500, so its reconciler |
ksykulev
reviewed
Aug 3, 2026
Index mdm_android_commands on (status, created_at). The reconciler's batch query filters on status and created_at and the table only had the primary key, the operation_name unique key, and a host_uuid key -- nothing leading with status, so the daily run was a full scan of a table that grows with every Lock/Wipe/Clear-passcode ever issued. status (equality) leads and created_at (range) follows, which is also the order that lets the ORDER BY and LIMIT use the index instead of sorting. Stop the AMAPI proxy from returning 404 when fleetdm.com has no AndroidEnterprise row. The reconciler reads a 404 as "Google no longer has a record of this operation" and, past the grace period, marks the command permanently failed -- so a website-side data problem would silently fail commands for that Fleet server. A missing record now returns 403 like a loss of access does, which the reconciler classifies as an authorization failure and stops the whole run on. 404 is left to mean only what the Fleet server acts on. Keep the AMAPI error object out of the proxy's 429 warning. gaxios errors carry the request config, including the Authorization header used to call Google, and every other android-proxy controller keeps this p1 warning terse. Use errors.AsType in IsBadRequestError for consistency with the other classifiers in the file.
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 20 out of 21 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/46145-android-command-reconcile.md
10 tasks
eashaw
previously approved these changes
Aug 5, 2026
ksykulev
previously approved these changes
Aug 5, 2026
ksykulev
left a comment
ksykulev
left a comment
Contributor
There was a problem hiding this comment.
Let me know when you renumber the migration and I can re-approve. Looks good 👍
…-reconcile # Conflicts: # server/datastore/mysql/android.go # server/datastore/mysql/schema.sql # server/fleet/cron_schedules.go
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…-reconcile # Conflicts: # server/datastore/mysql/schema.sql # server/mdm/android/service/pubsub.go
…droid-command-reconcile # Conflicts: # server/datastore/mysql/schema.sql
dantecatalfamo
temporarily deployed
to
Docker Hub
August 7, 2026 15:17 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Member
Author
eashaw
previously approved these changes
Aug 7, 2026
…droid-command-reconcile # Conflicts: # server/datastore/mysql/schema.sql
dantecatalfamo
temporarily deployed
to
Docker Hub
August 7, 2026 17:55 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Member
Author
ksykulev
approved these changes
Aug 7, 2026
This was referenced Aug 12, 2026
3 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 #46145
Android Lock / Wipe / Clear passcode commands are tracked in
mdm_android_commandsand start aspending. They only reach a terminal status when the AMAPI Pub/SubCOMMANDnotification arrives. If that notification is never delivered — Fleet's push endpoint down longer than GCP's 7-day retention, a subscription misconfiguration, a Google Cloud incident — the row stayspendingforever,IsPendingLock/Wipe/ClearPasscodestay true, and the pending-state guard blocks the admin from re-issuing the command. Only a manual MySQL edit recovers it.This adds a daily
mdm_android_command_reconcilercron that asks AMAPI what actually happened.How it works
mdm_android_commandsrows that arependingand older than 24h, oldest first, 500 per run.enterprises.devices.operations.get(operation_name)for each, paced to 50 calls/minute.Operation.Done == true→ writesacknowledged, orerrorwitherror_code/error_messagefromOperation.Error.Operation.Done == false→ leaves the row alone; the command is still queued at AMAPI.404 NOT_FOUND→ leaves the row alone while it is younger than 7 days (GCP Pub/Sub's max retention, so a notification could still arrive), and only past that marks iterrorwithgoogle.rpccode 5. Without the grace period a WIPE that AMAPI is still holding for an offline device could be wrongly marked failed.429→ stops the run and returns an error. The daily cadence is the backoff; the next run resumes oldest-first. A transient failure on a single row is logged and skipped without aborting the batch.Two notes on the approach:
host_mdm_actions.lock_ref/wipe_ref; this writes the terminal status instead. Since Android commands backend #46031,GetHostLockWipeStatusreads the command row through that ref, and a terminal status is exactly what makesIsPending*false and the command re-issuable. Nulling the ref would break that read. An acknowledged WIPE still goes through the existing wipe-ack path, which does clearhost_mdm_actionsfor BYO.androidOperationTerminalState,setAndroidCommandTerminalState,androidWipeAckUnenroll) so the cron and Pub/Sub cannot drift on status mapping or the wipe-ack side effects. No behavior change — the existingTestPubSubCommandsuite passes unchanged.fleetdm.com proxy
operations.gethad no route on the AMAPI proxy, so the cron would 404 for every proxy-based deployment. This PR addsGET /api/android/v1/enterprises/:androidEnterpriseId/devices/:deviceId/operations/:operationIdplusget-android-device-operation.js, modeled onget-android-device.js(same secret auth, same 429/403/404 intercepts). The website change needs to be deployed before the Fleet server change is useful.Unlike the other android-proxy actions, this one reserves 404 for a single meaning: AMAPI has no record of the operation. That is the only 404 the Fleet server should act on, since past the grace period it marks the command permanently failed. A missing
AndroidEnterpriserow on fleetdm.com returns 403, not 404, so a website-side data problem stops the reconciler run instead of silently failing that server's commands.Apple and Windows have the same dropped-result failure shape, but neither has a single authoritative REST endpoint to poll, so they stay out of scope (per the issue).
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.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
New coverage:
TestReconcileAndroidCommands(13 subtests): done→acknowledged, done+error→error with code/message, still-running left pending, 404 inside grace left pending, 404 past grace→error code 5, acknowledged WIPE runs the unenroll side effects, failed WIPE does not unenroll, a per-row AMAPI or DB failure does not stop the batch, 429 stops the run, empty batch makes no AMAPI calls, DB failure surfaces, Android MDM off skips the run.TestAndroid/ListPendingMDMAndroidCommands: age cutoff, status filter, oldest-first ordering, limit, empty result. Rows are per-host, so the batch covers multiple hosts independently.TestUp_20260805182836: the new index exists on(status, created_at)in that column order, and an existing command row survives theALTERand is still readable through the reconciler's predicate.Ran locally:
go test ./server/mdm/android/... ./server/fleet/...,MYSQL_TEST=1 go test -run TestAndroid ./server/datastore/mysql/,MYSQL_TEST=1 go test -run TestUp_20260805182836 ./server/datastore/mysql/migrations/tables/,MYSQL_TEST=1 REDIS_TEST=1 go test ./server/service/,make lint-go-incremental, website eslint — all pass.schema.sqlwas regenerated from the migrations rather than hand-edited.Manual test plan (needs the proxy change deployed, or
FLEET_DEV_ANDROID_GOOGLE_CLIENT=1):mdm_android_commandsrow ispending.UPDATE mdm_android_commands SET created_at = NOW(6) - INTERVAL 2 DAY WHERE command_uuid = '<uuid>';fleetctl trigger --name mdm_android_command_reconcileracknowledged(orerrorwith a code), the host to leave the pending-lock state, and Lock to be issuable again.mdm_unenrolledactivity.Database migrations
One migration,
20260805182836_AddAndroidCommandsStatusCreatedAtIndex: addsidx_mdm_android_commands_status_created_at (status, created_at).mdm_android_commandshad only the primary key, theoperation_nameunique key, and ahost_uuidkey — nothing leading withstatus— so the reconciler's batch query was a full scan of a table that grows with every Lock/Wipe/Clear-passcode ever issued.status(equality) leads andcreated_at(range) follows, the order MySQL needs to use both predicates from one index; InnoDB appends the primary key (command_uuid) to secondary indexes, so theORDER BY created_at, command_uuidandLIMITare satisfied without a sort. Built withALGORITHM=INPLACE, LOCK=NONEso it does not block command inserts.Summary by CodeRabbit