Deduplicate Android MDM Pub/Sub deliveries and protect against reordering - #49792
Merged
Conversation
…ring Google Pub/Sub push delivery is at-least-once and unordered, but the Android MDM notification handler had no message-level dedup or staleness check, so redeliveries could re-run the setup experience and a stale DELETED arriving after a re-enrollment could leave a live host stuck showing unenrolled. - Add last_pubsub_message_id / last_pubsub_event_time to android_devices and skip a notification whose messageId matches, or whose event timestamp is older than, the last one processed for that host. - Record dedup state on every state-changing path, including the WIPE-ack unenroll, so a late in-flight STATUS_REPORT cannot re-enroll a wiped host. - Restore host_mdm.enrolled on a STATUS_REPORT from a host currently marked unenrolled, preserving its is_personal_enrollment classification. - Force dedup reads onto the primary so fast redeliveries are not missed on a lagging replica. - Document that MarkAllPendingVPPInstallsAsFailedForAndroidHost is already scoped to still-pending rows. Fixes #43502
Make the last_pubsub_message_id column's collation explicit (utf8mb4_unicode_ci) rather than relying on the table default.
dantecatalfamo
temporarily deployed
to
Docker Hub
July 22, 2026 21:25 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
dantecatalfamo
marked this pull request as draft
July 22, 2026 21:25
Contributor
WalkthroughAndroid Pub/Sub messages now carry message IDs and publish times. Fleet stores per-host deduplication state in Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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: 1
🧹 Nitpick comments (2)
server/mdm/android/service/pubsub.go (2)
542-574: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
SetAndroidHostEnrolledruns on every non-stale STATUS_REPORT.STATUS_REPORTs are a hot path, and
SetAndroidHostEnrolledperforms anAppConfigread plus a transaction/SELECTon each call even when the host is already enrolled (the common case), where it just no-ops. Consider gating the recovery call on a cheaper signal (e.g. only when the host is currently unenrolled) to avoid the per-report DB round trip.🤖 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/mdm/android/service/pubsub.go` around lines 542 - 574, Gate the recovery call in the STATUS_REPORT handling flow on the host’s current unenrolled state before invoking SetAndroidHostEnrolled. Preserve the existing stale-message protection and logging/error behavior when recovery is needed, while avoiding the AppConfig read and transaction for already-enrolled hosts.
370-391: 🩺 Stability & Availability | 🔵 TrivialDedup gate is check-then-act; concurrent redeliveries can still both process.
isDuplicateOrStalePubSubreads state andrecordPubSubProcessedwrites it in separate steps with no lock. Forcing the primary closes the replica-lag window, but two at-least-once redeliveries handled concurrently (same instance goroutines or different instances) can both pass this gate before either records, re-running enrollment/unenroll work. This is the residual risk your PR objectives already flag for worker-level idempotency on setup-experience jobs. Worth confirming the setup-experience worker (and the unenroll activity path) is idempotent enough to absorb this, since dedup alone does not serialize concurrent deliveries.This affects both
handlePubSubEnrollment(Lines 707-744) andhandlePubSubStatusReport(Lines 542-574), which share this helper.🤖 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/mdm/android/service/pubsub.go` around lines 370 - 391, The deduplication check in isDuplicateOrStalePubSub is not atomic with recordPubSubProcessed, allowing concurrent deliveries to process the same message. Add an atomic claim/record mechanism shared by handlePubSubEnrollment and handlePubSubStatusReport, or otherwise serialize the check and write across instances, and ensure both handlers proceed only when the claim succeeds; verify the setup-experience and unenroll paths remain idempotent for concurrent delivery.
🤖 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/datastore/mysql/android.go`:
- Around line 580-612: Update SetAndroidPubSubDedupState to inspect the result
of its UPDATE via RowsAffected and, when zero rows match, return a wrapped
notFound("AndroidDevice").WithID(hostID) error consistent with
GetAndroidPubSubDedupState. Preserve existing wrapping for execution errors, and
clarify GetAndroidPubSubDedupState’s comment to distinguish an absent
AndroidDevice row from a present row with no recorded dedup state.
---
Nitpick comments:
In `@server/mdm/android/service/pubsub.go`:
- Around line 542-574: Gate the recovery call in the STATUS_REPORT handling flow
on the host’s current unenrolled state before invoking SetAndroidHostEnrolled.
Preserve the existing stale-message protection and logging/error behavior when
recovery is needed, while avoiding the AppConfig read and transaction for
already-enrolled hosts.
- Around line 370-391: The deduplication check in isDuplicateOrStalePubSub is
not atomic with recordPubSubProcessed, allowing concurrent deliveries to process
the same message. Add an atomic claim/record mechanism shared by
handlePubSubEnrollment and handlePubSubStatusReport, or otherwise serialize the
check and write across instances, and ensure both handlers proceed only when the
claim succeeds; verify the setup-experience and unenroll paths remain idempotent
for concurrent delivery.
🪄 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: f8947160-4b54-427e-9f2c-8b3086f1c713
📒 Files selected for processing (13)
changes/43502-android-pubsub-dedupserver/datastore/mysql/android.goserver/datastore/mysql/android_test.goserver/datastore/mysql/migrations/tables/20260722203845_AddPubSubDedupToAndroidDevices.goserver/datastore/mysql/migrations/tables/20260722203845_AddPubSubDedupToAndroidDevices_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/vpp.goserver/fleet/datastore.goserver/mdm/android/pubsub.goserver/mdm/android/service/enterprises_test.goserver/mdm/android/service/pubsub.goserver/mdm/android/service/pubsub_dedup_test.goserver/mock/datastore_mock.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
This PR hardens Fleet’s Android MDM Pub/Sub push handler against Pub/Sub’s at-least-once and unordered delivery by adding per-host deduplication and staleness checks, and by improving recovery when a valid STATUS_REPORT arrives after an out-of-order DELETED.
Changes:
- Adds per-host Pub/Sub dedup/staleness tracking using new
android_devices.last_pubsub_message_idandandroid_devices.last_pubsub_event_time. - Applies dedup + staleness checks across ENROLLMENT/STATUS_REPORT/COMMAND flows and restores
host_mdm.enrolledon live STATUS_REPORTs when needed. - Adds/updates datastore APIs, migrations, and unit tests covering duplicate/stale/out-of-order cases.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/mock/datastore_mock.go | Extends datastore mock with new Android dedup/enrollment recovery methods. |
| server/mdm/android/service/pubsub.go | Implements dedup + staleness logic, records processing state, and adds STATUS_REPORT enrollment recovery. |
| server/mdm/android/service/pubsub_dedup_test.go | New unit tests for duplicate/stale/out-of-order handling and WIPE ack dedup recording. |
| server/mdm/android/service/enterprises_test.go | Initializes new mock datastore funcs for tests. |
| server/mdm/android/pubsub.go | Adds Pub/Sub envelope fields messageId and publishTime to the parsed struct. |
| server/fleet/datastore.go | Adds AndroidDatastore interface methods for dedup state and enrollment recovery. |
| server/datastore/mysql/vpp.go | Documents idempotency of pending-VPP-fail path to avoid duplicate activities on duplicate DELETED. |
| server/datastore/mysql/schema.sql | Adds new columns to android_devices and updates migration status table seed. |
| server/datastore/mysql/migrations/tables/20260722203845_AddPubSubDedupToAndroidDevices.go | Migration adding the new dedup columns. |
| server/datastore/mysql/migrations/tables/20260722203845_AddPubSubDedupToAndroidDevices_test.go | Migration test validating NULL defaults and writability of new columns. |
| server/datastore/mysql/android.go | Implements Get/SetAndroidPubSubDedupState and SetAndroidHostEnrolled. |
| server/datastore/mysql/android_test.go | Adds tests for enrollment recovery behavior and dedup state round-trip. |
| changes/43502-android-pubsub-dedup | Release note entry (content excluded from review). |
Files excluded by content exclusion policy (1)
- changes/43502-android-pubsub-dedup
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #49792 +/- ##
==========================================
- Coverage 68.04% 67.99% -0.05%
==========================================
Files 3929 3898 -31
Lines 250276 249942 -334
Branches 13431 13020 -411
==========================================
- Hits 170294 169958 -336
+ Misses 64681 64654 -27
- Partials 15301 15330 +29
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:
|
…dedup # Conflicts: # server/datastore/mysql/schema.sql
- SetAndroidPubSubDedupState now checks RowsAffected and returns a NotFound error when no android_devices row matches, so a missing row surfaces via the caller's log instead of silently dropping dedup state (clientFoundRows is enabled, so a no-op update on an existing row still reports 1 matched row). - Clarify GetAndroidPubSubDedupState doc: distinguish an absent row (NotFound) from a present row with nothing recorded yet. - Drop the unused esID parameter from the makeEnrollmentEnvelope test helper. - Add a datastore test asserting set-on-missing-row returns NotFound.
dantecatalfamo
temporarily deployed
to
Docker Hub
July 28, 2026 19:08 — 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 12 out of 13 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/43502-android-pubsub-dedup
Comments suppressed due to low confidence (1)
server/datastore/mysql/android.go:612
- SetAndroidPubSubDedupState unconditionally overwrites last_pubsub_event_time with the provided eventTime. When eventTime is nil (e.g. envelope/device timestamp missing or unparseable), this UPDATE will set last_pubsub_event_time to NULL and effectively disable staleness protection for future deliveries until a later message happens to carry a parseable timestamp. Consider preserving the existing last_pubsub_event_time (and similarly last_pubsub_message_id when messageID is empty) instead of clobbering it with NULL/empty values.
func (ds *Datastore) SetAndroidPubSubDedupState(ctx context.Context, hostID uint, messageID string, eventTime *time.Time) error {
result, err := ds.writer(ctx).ExecContext(ctx,
`UPDATE android_devices SET last_pubsub_message_id = ?, last_pubsub_event_time = ? WHERE host_id = ?`,
messageID, eventTime, hostID)
if err != nil {
dantecatalfamo
marked this pull request as ready for review
July 28, 2026 20:04
- Reconcile janitor now records dedup state after unenrolling a host missing from AMAPI, so a STATUS_REPORT already queued before the deletion cannot revert the unenroll (recovery flip-flop / duplicate mdm_unenrolled activity). - SetAndroidHostEnrolled does a cheap enrolled-state read before opening a write transaction, avoiding a BEGIN/COMMIT on the primary for every STATUS_REPORT in the common already-enrolled case. - WIPE-ack unenroll records dedup state only when it actually flips state, so a redelivered terminal wipe cannot move last_pubsub_event_time backwards. - recordPubSubProcessed logs a failed dedup write at Warn, not Error: a NotFound there is a benign host-deletion race, not an alert-worthy fault. - Note the cross-clock caveat in pubSubEventTime.
dantecatalfamo
temporarily deployed
to
Docker Hub
July 28, 2026 20:21 — 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 13 out of 14 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/43502-android-pubsub-dedup
Comments suppressed due to low confidence (3)
server/mdm/android/service/pubsub.go:685
- In the ENROLLMENT->DELETED branch, SetAndroidHostUnenrolled's boolean return value is ignored, but the code always emits an mdm_unenrolled activity afterwards. If the host was already unenrolled (e.g. retry/out-of-order delivery that isn't caught by messageId dedup), this can generate duplicate mdm_unenrolled activities. Consider matching the STATUS_REPORT deleted path: capture didUnenroll and return early (after recording dedup state) when no state change occurred.
if err := clearAndroidBYOWipeRef(ctx, svc.fleetDS, host.Host.ID); err != nil {
return ctxerr.Wrap(ctx, err, "clear byo wipe-ref on DELETED state (ENROLLMENT)")
}
if _, err := svc.ds.SetAndroidHostUnenrolled(ctx, host.Host.ID); err != nil {
return ctxerr.Wrap(ctx, err, "set android host unenrolled on DELETED state (ENROLLMENT)")
}
server/mdm/android/service/pubsub.go:373
- pubSubEventTime parses timestamps using time.RFC3339, but Google Pub/Sub publishTime (and many Google APIs) commonly include fractional seconds (RFC3339 with nanos). time.Parse(time.RFC3339, ...) will fail on those values, causing eventTime to be nil (staleness check disabled) and potentially clearing last_pubsub_event_time when recording state.
func pubSubEventTime(deviceTime, publishTime string) *time.Time {
for _, ts := range []string{deviceTime, publishTime} {
if ts == "" {
continue
}
if t, err := time.Parse(time.RFC3339, ts); err == nil {
return &t
}
}
server/datastore/mysql/migrations/tables/20260728120000_AddPubSubDedupToAndroidDevices_test.go:10
- PR description's reviewer command suggests running TestUp_20260722203845, but this PR adds TestUp_20260728120000. Consider updating the PR description command so reviewers run the correct migration test.
func TestUp_20260728120000(t *testing.T) {
db := applyUpToPrev(t)
7 tasks
ksykulev
reviewed
Jul 31, 2026
SetAndroidPubSubDedupState overwrote last_pubsub_event_time unconditionally, so a notification with no parseable timestamp (nil eventTime) cleared the column to NULL and disabled staleness protection for that host until some later message happened to carry one. Same for last_pubsub_message_id when messageID was empty, which is how ReconcileAndroidDevices records an out-of-band unenroll. An empty messageID or nil eventTime now leaves that column at its previous value, so the recorded state only ever moves forward. The RowsAffected == 0 -> NotFound check still holds on a value-preserving write because the DSN sets clientFoundRows=true, so RowsAffected counts matched rows rather than changed rows; the test pins that.
The ENROLLMENT DELETED branch discarded SetAndroidHostUnenrolled's return value and emitted mdm_unenrolled unconditionally, while the STATUS_REPORT DELETED branch returns early when the flip was a no-op. A DELETED that arrives under both notification types — or a redelivery that messageId dedup does not catch — therefore added a duplicate activity row. Match the STATUS_REPORT branch: record dedup state, then skip the activity when the host was already unenrolled.
Recording Pub/Sub dedup state on the ENROLLMENT path depended on a getExistingHost call that ran *after* enrollment and the setup-experience job had already succeeded. When that read failed (replica lag, a DB hiccup, a deploy returning 5xx) the handler logged and returned nil: Pub/Sub got a 200 ACK with no dedup state written, so the redelivery re-queued the setup experience — the exact duplicate this dedup exists to prevent. enrollHost and addNewHost now return the Fleet host ID they resolved, so the dedup write uses it directly and the re-fetch is gone. The STATUS_REPORT re-enroll path still re-reads, but only because it needs the full *fleet.AndroidHost for updateHost/updateHostSoftware, and a failure there returns an error so Pub/Sub retries rather than silently acking.
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. |
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 13 out of 14 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/43502-android-pubsub-dedup
Suppressed comments (1)
server/mdm/android/service/pubsub.go:373
- pubSubEventTime parses timestamps using time.RFC3339, but Google Pub/Sub publishTime commonly includes fractional seconds (RFC3339Nano). If parsing fails, eventTime becomes nil and staleness protection is silently skipped, weakening the core dedup/reordering defense.
// pubSubEventTime derives the AMAPI event timestamp used for staleness comparison.
// It prefers the device's LastStatusReportTime (present on STATUS_REPORT and,
// usually, ENROLLMENT device payloads) and falls back to the Pub/Sub envelope
// publishTime. Returns nil when neither is a parseable RFC3339 timestamp, in which
// case the staleness check is skipped and only messageId dedup applies.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
server/mdm/android/service/reconcile_devices.go (1)
99-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that this timestamp comes from the Fleet server clock.
time.Now().UTC()writes a Fleet-server timestamp intolast_pubsub_event_time.handlePubSubStatusReportcompares that value against AMAPI device times and Pub/Sub publish times, which come from Google clocks.pubSubEventTimeinserver/mdm/android/service/pubsub.goalready documents a cross-clock caveat for its two Google sources. This call adds a third clock.The direction is safe here. Reconcile only runs for devices already absent from AMAPI, so suppressing later status reports is the wanted result. Server clock skew only widens the suppression window. Add a short note so a future reader does not assume all recorded event times share one clock.
📝 Proposed comment addition
// mdm_unenrolled activity on the next reconcile). Best-effort: a missed record only // weakens dedup for the redelivery window, so log and continue. + // Note: this is the Fleet server clock, not a Google clock. It is compared against + // AMAPI device times and Pub/Sub publish times, so server skew widens or narrows the + // window in which later status reports are dropped as stale. now := time.Now().UTC()🤖 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/mdm/android/service/reconcile_devices.go` around lines 99 - 102, Add a concise inline comment immediately before the time.Now().UTC() assignment in the reconcile flow, documenting that the timestamp uses the Fleet server clock and may be compared with Google/AMAPI clock values. Keep the existing dedup-state update and error handling unchanged.server/mdm/android/service/pubsub_dedup_test.go (2)
79-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case that sets
LastStatusReportTime.
makeStatusEnvelopenever setsdevice.LastStatusReportTime.pubSubEventTimeprefers that field and falls back topublishTimeonly when the field is empty or unparseable. Every STATUS_REPORT subtest therefore exercises the fallback branch alone.In production the STATUS_REPORT ordering comparison normally uses the device time. Add one staleness case with
LastStatusReportTimeset, and set it to a value that disagrees withpublishTime. That proves the device time wins.💚 Proposed helper change
-func makeStatusEnvelope(t *testing.T, esID, messageID, publishTime string, deleted bool) *android.PubSubMessage { +func makeStatusEnvelope(t *testing.T, esID, messageID, publishTime string, deleted bool) *android.PubSubMessage { + return makeStatusEnvelopeAt(t, esID, messageID, publishTime, "", deleted) +} + +// makeStatusEnvelopeAt additionally sets the device's LastStatusReportTime, which +// pubSubEventTime prefers over the envelope publishTime. +func makeStatusEnvelopeAt(t *testing.T, esID, messageID, publishTime, deviceTime string, deleted bool) *android.PubSubMessage { device := androidmanagement.Device{ Name: createAndroidDeviceId("dedup"), + LastStatusReportTime: deviceTime, HardwareInfo: &androidmanagement.HardwareInfo{🤖 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/mdm/android/service/pubsub_dedup_test.go` around lines 79 - 105, Update makeStatusEnvelope to set device.LastStatusReportTime from a helper argument, then add a STATUS_REPORT staleness test case where that value differs from publishTime and confirms device time is preferred. Preserve existing fallback cases by leaving the field empty where needed.
333-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a subtest for the WIPE ack path that must not record dedup state.
This subtest covers the
didUnenroll == truebranch.handleAndroidWipeAckUnenrollinserver/mdm/android/service/pubsub.goalso has a!didUnenrollbranch at Lines 260-272. That branch deliberately returns without callingrecordPubSubProcessed, so a redelivered older wipe cannot movelast_pubsub_event_timebackwards.That rule is the subtlest new behavior in this change, and no test asserts it. Add a subtest where
SetAndroidHostUnenrolledFuncreturnsfalseand assert thatSetAndroidPubSubDedupStateFuncInvokedstaysfalse.💚 Proposed subtest
t.Run("WIPE ack on an already-unenrolled host records no dedup state", func(t *testing.T) { // A redelivered wipe must not move last_pubsub_event_time backwards when the // state flip was a no-op. svc, mockDS := createAndroidService(t) mockDS.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { return &fleet.AppConfig{MDM: fleet.MDM{AndroidEnabledAndConfigured: true}}, nil } stored := &android.MDMAndroidCommand{ CommandUUID: "cmd-wipe", HostUUID: hostUUID, OperationName: "enterprises/E/devices/D/operations/wipe-ack", CommandType: string(android.MDMAndroidCommandTypeWipe), Status: string(android.MDMAndroidCommandStatusPending), } mockDS.GetMDMAndroidCommandByOperationNameFunc = func(ctx context.Context, opName string) (*android.MDMAndroidCommand, error) { return stored, nil } mockDS.UpdateMDMAndroidCommandStatusFunc = func(ctx context.Context, commandUUID, status string, errorCode, errorMessage *string) error { return nil } mockDS.AndroidHostLiteByHostUUIDFunc = func(ctx context.Context, hUUID string) (*fleet.AndroidHost, error) { return &fleet.AndroidHost{Host: &fleet.Host{ID: hostID, UUID: hUUID}}, nil } mockDS.GetHostMDMFunc = func(ctx context.Context, id uint) (*fleet.HostMDM, error) { return &fleet.HostMDM{IsPersonalEnrollment: false}, nil } mockDS.ClearHostMDMActionsFunc = func(ctx context.Context, id uint) error { return nil } mockDS.SetAndroidHostUnenrolledFunc = func(ctx context.Context, id uint) (bool, error) { return false, nil // already unenrolled } mockDS.SetAndroidPubSubDedupStateFunc = func(ctx context.Context, id uint, messageID string, eventTime *time.Time) error { return nil } body, err := json.Marshal(androidmanagement.Operation{Name: stored.OperationName, Done: true}) require.NoError(t, err) msg := &android.PubSubMessage{ Attributes: map[string]string{"notificationType": string(android.PubSubCommand)}, Data: base64.StdEncoding.EncodeToString(body), MessageID: "wipe-msg-redelivered", PublishTime: "2026-07-22T12:00:00Z", } require.NoError(t, svc.ProcessPubSubPush(t.Context(), dedupToken, msg)) require.True(t, mockDS.SetAndroidHostUnenrolledFuncInvoked, "the unenroll must still be attempted") require.False(t, mockDS.SetAndroidPubSubDedupStateFuncInvoked, "a no-op wipe ack must not move the recorded event time backwards") require.False(t, mockDS.ListHostsLiteByIDsFuncInvoked, "no duplicate mdm_unenrolled activity") })🤖 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/mdm/android/service/pubsub_dedup_test.go` around lines 333 - 384, Add a subtest alongside the existing WIPE acknowledgement test covering the already-unenrolled path in handleAndroidWipeAckUnenroll: configure SetAndroidHostUnenrolledFunc to return false, process a completed WIPE message, and assert the unenrollment is attempted while SetAndroidPubSubDedupStateFunc is not invoked (and no duplicate unenrollment activity is created).
🤖 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/datastore/mysql/migrations/tables/20260728120000_AddPubSubDedupToAndroidDevices_test.go`:
- Around line 35-47: Update the round-trip assertions in the Android devices
migration test to use a timestamp with nonzero microseconds, then assert that
last_pubsub_event_time equals the exact stored timestamp including its
fractional precision instead of only checking it is non-NULL. Keep the existing
last_pubsub_message_id verification unchanged.
In
`@server/datastore/mysql/migrations/tables/20260728120000_AddPubSubDedupToAndroidDevices.go`:
- Around line 25-26: Implement Down_20260728120000 to remove both columns added
by the corresponding up migration, executing the required DROP COLUMN statements
through the provided transaction and returning any errors. Ensure the downgrade
fully reverses the schema changes so reapplying the migration does not encounter
existing columns.
---
Nitpick comments:
In `@server/mdm/android/service/pubsub_dedup_test.go`:
- Around line 79-105: Update makeStatusEnvelope to set
device.LastStatusReportTime from a helper argument, then add a STATUS_REPORT
staleness test case where that value differs from publishTime and confirms
device time is preferred. Preserve existing fallback cases by leaving the field
empty where needed.
- Around line 333-384: Add a subtest alongside the existing WIPE acknowledgement
test covering the already-unenrolled path in handleAndroidWipeAckUnenroll:
configure SetAndroidHostUnenrolledFunc to return false, process a completed WIPE
message, and assert the unenrollment is attempted while
SetAndroidPubSubDedupStateFunc is not invoked (and no duplicate unenrollment
activity is created).
In `@server/mdm/android/service/reconcile_devices.go`:
- Around line 99-102: Add a concise inline comment immediately before the
time.Now().UTC() assignment in the reconcile flow, documenting that the
timestamp uses the Fleet server clock and may be compared with Google/AMAPI
clock values. Keep the existing dedup-state update and error handling unchanged.
🪄 Autofix
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: 5adb6ce8-f228-410a-ba66-2f9130825c71
📒 Files selected for processing (14)
changes/43502-android-pubsub-dedupserver/datastore/mysql/android.goserver/datastore/mysql/android_test.goserver/datastore/mysql/migrations/tables/20260728120000_AddPubSubDedupToAndroidDevices.goserver/datastore/mysql/migrations/tables/20260728120000_AddPubSubDedupToAndroidDevices_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/vpp.goserver/fleet/datastore.goserver/mdm/android/pubsub.goserver/mdm/android/service/enterprises_test.goserver/mdm/android/service/pubsub.goserver/mdm/android/service/pubsub_dedup_test.goserver/mdm/android/service/reconcile_devices.goserver/mock/datastore_mock.go
🚧 Files skipped from review as they are similar to previous changes (9)
- server/datastore/mysql/vpp.go
- server/mdm/android/service/enterprises_test.go
- server/datastore/mysql/schema.sql
- server/datastore/mysql/android_test.go
- server/mdm/android/pubsub.go
- server/fleet/datastore.go
- changes/43502-android-pubsub-dedup
- server/mock/datastore_mock.go
- server/datastore/mysql/android.go
Comment on lines
+35
to
+47
| // The columns are writable and round-trip. | ||
| _, err = db.Exec(`UPDATE android_devices | ||
| SET last_pubsub_message_id = 'msg-123', last_pubsub_event_time = '2026-07-22 10:00:00.000000' | ||
| WHERE device_id = 'd1'`) | ||
| require.NoError(t, err) | ||
|
|
||
| require.NoError(t, db.Get(&messageID, `SELECT last_pubsub_message_id FROM android_devices WHERE device_id = 'd1'`)) | ||
| require.NotNil(t, messageID) | ||
| require.Equal(t, "msg-123", *messageID) | ||
|
|
||
| require.NoError(t, db.Get(&eventTime, `SELECT last_pubsub_event_time FROM android_devices WHERE device_id = 'd1'`)) | ||
| require.NotNil(t, eventTime) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the event-time value and microsecond precision.
The test only checks that last_pubsub_event_time is non-NULL. A column or write path that truncates microseconds would still pass. Store a nonzero fractional value and assert the exact returned value.
Proposed fix
- SET last_pubsub_message_id = 'msg-123', last_pubsub_event_time = '2026-07-22 10:00:00.000000'
+ SET last_pubsub_message_id = 'msg-123', last_pubsub_event_time = '2026-07-22 10:00:00.123456'
@@
require.NoError(t, db.Get(&eventTime, `SELECT last_pubsub_event_time FROM android_devices WHERE device_id = 'd1'`))
require.NotNil(t, eventTime)
+ require.Equal(t, "2026-07-22 10:00:00.123456", *eventTime)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // The columns are writable and round-trip. | |
| _, err = db.Exec(`UPDATE android_devices | |
| SET last_pubsub_message_id = 'msg-123', last_pubsub_event_time = '2026-07-22 10:00:00.000000' | |
| WHERE device_id = 'd1'`) | |
| require.NoError(t, err) | |
| require.NoError(t, db.Get(&messageID, `SELECT last_pubsub_message_id FROM android_devices WHERE device_id = 'd1'`)) | |
| require.NotNil(t, messageID) | |
| require.Equal(t, "msg-123", *messageID) | |
| require.NoError(t, db.Get(&eventTime, `SELECT last_pubsub_event_time FROM android_devices WHERE device_id = 'd1'`)) | |
| require.NotNil(t, eventTime) | |
| } | |
| // The columns are writable and round-trip. | |
| _, err = db.Exec(`UPDATE android_devices | |
| SET last_pubsub_message_id = 'msg-123', last_pubsub_event_time = '2026-07-22 10:00:00.123456' | |
| WHERE device_id = 'd1'`) | |
| require.NoError(t, err) | |
| require.NoError(t, db.Get(&messageID, `SELECT last_pubsub_message_id FROM android_devices WHERE device_id = 'd1'`)) | |
| require.NotNil(t, messageID) | |
| require.Equal(t, "msg-123", *messageID) | |
| require.NoError(t, db.Get(&eventTime, `SELECT last_pubsub_event_time FROM android_devices WHERE device_id = 'd1'`)) | |
| require.NotNil(t, eventTime) | |
| require.Equal(t, "2026-07-22 10:00:00.123456", *eventTime) |
🤖 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/migrations/tables/20260728120000_AddPubSubDedupToAndroidDevices_test.go`
around lines 35 - 47, Update the round-trip assertions in the Android devices
migration test to use a timestamp with nonzero microseconds, then assert that
last_pubsub_event_time equals the exact stored timestamp including its
fractional precision instead of only checking it is non-NULL. Keep the existing
last_pubsub_message_id verification unchanged.
Contributor
There was a problem hiding this comment.
Pedantic. But a fairly easy fix.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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 13 out of 14 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/43502-android-pubsub-dedup
Suppressed comments (3)
server/datastore/mysql/migrations/tables/20260728120000_AddPubSubDedupToAndroidDevices_test.go:33
- This migration test scans last_pubsub_event_time (TIMESTAMP(6)) into *string, which is likely to fail with parseTime enabled. Use *time.Time so the scan type matches the column type.
var eventTime *string
require.NoError(t, db.Get(&eventTime, `SELECT last_pubsub_event_time FROM android_devices WHERE device_id = 'd1'`))
require.Nil(t, eventTime)
server/mdm/android/service/pubsub.go:748
- The ENROLLMENT handler forces the primary for the pre-dedup host lookup, but then calls enrollHost with the original context. If the host was created by a prior delivery seconds earlier, replica lag can make enrollHost's internal AndroidHostLite lookup miss the host and attempt a duplicate insert (likely hitting hosts.uuid uniqueness) and trigger retries/duplicate work. Use the same primary-required context for the enrollHost call (and ideally reuse it for the pre-check too).
// Force the primary so a redelivered ENROLLMENT sees a host that a prior delivery just
// created (and thus its recorded dedup state), instead of missing it on a lagging replica
// and re-queuing the setup experience.
existing, herr := svc.getExistingHost(ctxdb.RequirePrimary(ctx, true), &device)
if herr != nil {
return ctxerr.Wrap(ctx, herr, "getting existing Android host for enrollment dedup")
server/mdm/android/service/pubsub.go:373
- pubSubEventTime parses timestamps using time.RFC3339, but Google Pub/Sub publishTime and AMAPI timestamps commonly include fractional seconds. With RFC3339, those values won't parse and staleness protection will be silently skipped. Use RFC3339Nano (it also accepts non-fractional RFC3339).
func pubSubEventTime(deviceTime, publishTime string) *time.Time {
for _, ts := range []string{deviceTime, publishTime} {
if ts == "" {
continue
}
if t, err := time.Parse(time.RFC3339, ts); err == nil {
return &t
}
Comment on lines
+3
to
+7
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) |
ksykulev
previously approved these changes
Aug 6, 2026
…dedup # Conflicts: # server/datastore/mysql/schema.sql
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. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/mdm/android/service/pubsub_dedup_test.go (1)
126-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify that every skipped notification does not overwrite deduplication state.
These stale and duplicate cases verify selected processing side effects. They do not verify that
SetAndroidPubSubDedupStatewas not called. A regression could overwrite the stored message ID on a skipped delivery and still pass these tests.Proposed test assertions
require.False(t, mockDS.UpdateAndroidHostFuncInvoked, "stale enrollment must not re-run updateHost") require.False(t, mockDS.NewJobFuncInvoked, "stale enrollment must not re-queue setup experience") +require.False(t, mockDS.SetAndroidPubSubDedupStateFuncInvoked, "stale enrollment must not overwrite dedup state")Add the equivalent assertion to the duplicate
STATUS_REPORT, staleDELETED, and staleSTATUS_REPORTcases.Also applies to: 248-260, 262-276, 299-315
🤖 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/mdm/android/service/pubsub_dedup_test.go` around lines 126 - 140, Extend the skipped-notification tests to verify deduplication state is unchanged: in the stale ENROLLMENT case and the duplicate STATUS_REPORT, stale DELETED, and stale STATUS_REPORT cases, assert that SetAndroidPubSubDedupState was not invoked. Keep the existing processing-side-effect assertions intact.
🤖 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/mdm/android/service/pubsub_dedup_test.go`:
- Around line 126-140: Extend the skipped-notification tests to verify
deduplication state is unchanged: in the stale ENROLLMENT case and the duplicate
STATUS_REPORT, stale DELETED, and stale STATUS_REPORT cases, assert that
SetAndroidPubSubDedupState was not invoked. Keep the existing
processing-side-effect assertions intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cf18ece6-6adc-431f-92ae-d386289250f2
📒 Files selected for processing (14)
changes/43502-android-pubsub-dedupserver/datastore/mysql/android.goserver/datastore/mysql/android_test.goserver/datastore/mysql/migrations/tables/20260806210232_AddPubSubDedupToAndroidDevices.goserver/datastore/mysql/migrations/tables/20260806210232_AddPubSubDedupToAndroidDevices_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/vpp.goserver/fleet/datastore.goserver/mdm/android/pubsub.goserver/mdm/android/service/enterprises_test.goserver/mdm/android/service/pubsub.goserver/mdm/android/service/pubsub_dedup_test.goserver/mdm/android/service/reconcile_devices.goserver/mock/datastore_mock.go
🚧 Files skipped from review as they are similar to previous changes (11)
- server/mdm/android/service/enterprises_test.go
- server/datastore/mysql/vpp.go
- server/mdm/android/pubsub.go
- server/datastore/mysql/android.go
- changes/43502-android-pubsub-dedup
- server/datastore/mysql/android_test.go
- server/datastore/mysql/schema.sql
- server/fleet/datastore.go
- server/mdm/android/service/reconcile_devices.go
- server/mock/datastore_mock.go
- server/mdm/android/service/pubsub.go
ksykulev
approved these changes
Aug 6, 2026
This was referenced Aug 14, 2026
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 #43502
Checklist for submitter
Changes file added for user-visible changes in
changes/.Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements).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
Summary
Google Cloud Pub/Sub push delivery (used for Android MDM notifications at
POST /api/v1/fleet/android_enterprise/pubsub) is at-least-once and unordered. The handler had no message-level deduplication and no timestamp-based staleness check, so:DELETEDredelivered after a re-ENROLLMENTcould unenroll a live host, and routineSTATUS_REPORTs never restoredhost_mdm.enrolled, so the host stayed stuck showing unenrolled until a freshENROLLMENT.This adds a per-host dedup + staleness mechanism and repairs the recovery path:
last_pubsub_message_id/last_pubsub_event_timecolumns onandroid_devices. On ENROLLMENT/STATUS_REPORT, once the host is resolved, a notification whosemessageIdmatches — or whose event timestamp (deviceLastStatusReportTime, falling back to the envelopepublishTime) is older than — the last processed for that host is skipped.STATUS_REPORTcannot re-enroll a just-wiped host.STATUS_REPORTfrom a host currently marked unenrolled restoreshost_mdm.enrolled=1, preserving the existingis_personal_enrollmentclassification.MarkAllPendingVPPInstallsAsFailedForAndroidHostis already scoped to still-pending rows (so duplicate DELETED deliveries emit no duplicate activities); this is now documented with a comment.Known limitation (documented in code): dedup stores only the last-processed
messageId, so it catches the common sequential redelivery but not two truly concurrent duplicate deliveries; the timestamp staleness check backstops ordering. Fully idempotent handling of concurrent duplicate setup-experience runs would require worker-level idempotency, which is out of scope for this change.Testing
Automated coverage:
server/mdm/android/service/pubsub_dedup_test.go— duplicate/stale ENROLLMENT and STATUS_REPORT are skipped; out-of-order DELETED is dropped by staleness; stale STATUS_REPORT does not trigger recovery; equal-timestamp distinct message is processed; WIPE ack records dedup state; successful paths record state.server/datastore/mysql/android_test.go—SetAndroidHostEnrolled(no-op when enrolled, recovers when unenrolled, preserves BYO/COBO classification) and Pub/Sub dedup state get/set round-trip.Reviewer commands:
Database migrations
COLLATE utf8mb4_unicode_ci).The migration only
ADDs two nullable columns toandroid_devices; existing rows getNULLand noON UPDATEtimestamp fires onADD COLUMN.Summary by CodeRabbit