From 8a65ecf20bf63d5576edf6ce5358a22d82e6eaab Mon Sep 17 00:00:00 2001 From: George Karr Date: Thu, 30 Jul 2026 12:08:17 -0500 Subject: [PATCH] Bound Android device reconciliation pagination loop (#49615) --- .../49364-android-reconcile-pagination-bound | 1 + .../mdm/android/service/reconcile_devices.go | 65 +++++++++++---- .../android/service/reconcile_devices_test.go | 82 +++++++++++++++++++ 3 files changed, 132 insertions(+), 16 deletions(-) create mode 100644 changes/49364-android-reconcile-pagination-bound create mode 100644 server/mdm/android/service/reconcile_devices_test.go diff --git a/changes/49364-android-reconcile-pagination-bound b/changes/49364-android-reconcile-pagination-bound new file mode 100644 index 0000000000..23ce36705b --- /dev/null +++ b/changes/49364-android-reconcile-pagination-bound @@ -0,0 +1 @@ +- Bounded the Android device reconciliation cron's Google API pagination so a malformed or cycling response can no longer cause an unbounded loop, and added periodic progress logging during pagination. diff --git a/server/mdm/android/service/reconcile_devices.go b/server/mdm/android/service/reconcile_devices.go index 18266f7c33..5781e05a93 100644 --- a/server/mdm/android/service/reconcile_devices.go +++ b/server/mdm/android/service/reconcile_devices.go @@ -7,6 +7,18 @@ import ( "github.com/fleetdm/fleet/v4/server/contexts/ctxerr" "github.com/fleetdm/fleet/v4/server/fleet" + "github.com/fleetdm/fleet/v4/server/mdm/android/service/androidmgmt" +) + +const ( + // androidReconcileMaxPages bounds the AMAPI device pagination loop so a malformed + // or cycling NextPageToken can't spin it forever. AMAPI returns 100 devices/page, + // so this permits up to ~1,000,000 devices — a safety net well above any realistic + // enterprise size, not a functional limit. + androidReconcileMaxPages = 10000 + // androidReconcilePageLogInterval controls how often pagination progress is logged + // so a slow or stuck reconcile can be diagnosed. + androidReconcilePageLogInterval = 100 ) // ReconcileAndroidDevices polls AMAPI for devices that Fleet still considers enrolled @@ -45,22 +57,9 @@ func ReconcileAndroidDevices(ctx context.Context, ds fleet.Datastore, logger *sl } // Make a list of all devices in Google - deviceNameMap := make(map[string]struct{}) - pageToken := "" - for { - // We use the partial call here, to avoid getting all data for a device when we only need a subset (name). - // should help with request speeds, and also cost for website in terms of network egress. - resp, err := client.EnterprisesDevicesListPartial(ctx, enterprise.Name(), pageToken) - if err != nil { - return ctxerr.Wrap(ctx, err, "listing android devices from AMAPI") - } - for _, dev := range resp.Devices { - deviceNameMap[dev.Name] = struct{}{} - } - if resp.NextPageToken == "" { - break - } - pageToken = resp.NextPageToken + deviceNameMap, err := listAllAndroidDeviceNames(ctx, client, logger, enterprise.Name()) + if err != nil { + return err } checked := 0 @@ -112,3 +111,37 @@ func ReconcileAndroidDevices(ctx context.Context, ds fleet.Datastore, logger *sl logger.DebugContext(ctx, "android reconcile complete", "checked", checked, "unenrolled", unenrolled) return nil } + +// listAllAndroidDeviceNames pages through AMAPI and returns the set of device resource names +// Google reports for the enterprise. The pagination loop is bounded by androidReconcileMaxPages +// so a malformed or cycling NextPageToken can't spin it forever; hitting the bound returns an +// error rather than a partial set, because a partial set would make present devices look missing +// and wrongly flip them to unenrolled. +func listAllAndroidDeviceNames(ctx context.Context, client androidmgmt.Client, logger *slog.Logger, enterpriseName string) (map[string]struct{}, error) { + deviceNameMap := make(map[string]struct{}) + pageToken := "" + for page := 1; ; page++ { + // We use the partial call here, to avoid getting all data for a device when we only need a subset (name). + // should help with request speeds, and also cost for website in terms of network egress. + resp, err := client.EnterprisesDevicesListPartial(ctx, enterpriseName, pageToken) + if err != nil { + return nil, ctxerr.Wrap(ctx, err, "listing android devices from AMAPI") + } + for _, dev := range resp.Devices { + deviceNameMap[dev.Name] = struct{}{} + } + if resp.NextPageToken == "" { + return deviceNameMap, nil + } + if page >= androidReconcileMaxPages { + logger.ErrorContext(ctx, "android reconcile pagination exceeded max pages; aborting to avoid unbounded loop", + "enterprise", enterpriseName, "max_pages", androidReconcileMaxPages, "page", page, + "page_token", pageToken, "next_page_token", resp.NextPageToken, "devices_seen", len(deviceNameMap)) + return nil, ctxerr.Errorf(ctx, "android reconcile pagination exceeded max pages (%d)", androidReconcileMaxPages) + } + if page%androidReconcilePageLogInterval == 0 { + logger.InfoContext(ctx, "android reconcile pagination progress", "pages", page, "devices_seen", len(deviceNameMap)) + } + pageToken = resp.NextPageToken + } +} diff --git a/server/mdm/android/service/reconcile_devices_test.go b/server/mdm/android/service/reconcile_devices_test.go new file mode 100644 index 0000000000..7c78f0d454 --- /dev/null +++ b/server/mdm/android/service/reconcile_devices_test.go @@ -0,0 +1,82 @@ +package service + +import ( + "context" + "fmt" + "io" + "log/slog" + "testing" + + "github.com/fleetdm/fleet/v4/server/mdm/android/mock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/api/androidmanagement/v1" +) + +func testLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} + +func TestListAllAndroidDeviceNames(t *testing.T) { + const enterpriseName = "enterprises/LC123" + + t.Run("aggregates device names across pages", func(t *testing.T) { + client := &mock.Client{} + var calls int + client.EnterprisesDevicesListPartialFunc = func(_ context.Context, gotEnterprise, pageToken string) (*androidmanagement.ListDevicesResponse, error) { + assert.Equal(t, enterpriseName, gotEnterprise) + calls++ + switch pageToken { + case "": + return &androidmanagement.ListDevicesResponse{ + Devices: []*androidmanagement.Device{{Name: "a"}, {Name: "b"}}, + NextPageToken: "page2", + }, nil + case "page2": + return &androidmanagement.ListDevicesResponse{ + Devices: []*androidmanagement.Device{{Name: "c"}}, + NextPageToken: "", + }, nil + default: + t.Fatalf("unexpected page token %q", pageToken) + return nil, nil + } + } + + names, err := listAllAndroidDeviceNames(t.Context(), client, testLogger(), enterpriseName) + require.NoError(t, err) + assert.Equal(t, 2, calls) + assert.Equal(t, map[string]struct{}{"a": {}, "b": {}, "c": {}}, names) + }) + + t.Run("propagates AMAPI errors", func(t *testing.T) { + client := &mock.Client{} + client.EnterprisesDevicesListPartialFunc = func(context.Context, string, string) (*androidmanagement.ListDevicesResponse, error) { + return nil, assert.AnError + } + + names, err := listAllAndroidDeviceNames(t.Context(), client, testLogger(), enterpriseName) + require.Error(t, err) + assert.Nil(t, names) + }) + + t.Run("aborts on cycling page token instead of looping forever", func(t *testing.T) { + client := &mock.Client{} + var calls int + // Always return a non-empty NextPageToken to simulate a malformed/cycling response. + client.EnterprisesDevicesListPartialFunc = func(_ context.Context, _, _ string) (*androidmanagement.ListDevicesResponse, error) { + calls++ + return &androidmanagement.ListDevicesResponse{ + Devices: []*androidmanagement.Device{{Name: fmt.Sprintf("dev-%d", calls)}}, + NextPageToken: "never-ends", + }, nil + } + + names, err := listAllAndroidDeviceNames(t.Context(), client, testLogger(), enterpriseName) + require.Error(t, err) + assert.Nil(t, names) + assert.Contains(t, err.Error(), "exceeded max pages") + // The loop is bounded: it stops after exactly androidReconcileMaxPages calls. + assert.Equal(t, androidReconcileMaxPages, calls) + }) +}