Refactor makeAndroidAppAvailable to use staggered job queuing - #47880
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
🚥 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/worker/software_worker_test.go (1)
265-268: ⚡ Quick winAssert the full batch-job arg contract in this test.
This verifies only part of the queued args. Please also assert
AppTeamIDandAppConfigChangedso regressions in phase-2 job payload wiring are caught.Suggested patch
require.Equal(t, makeAndroidAppAvailableBatchTask, args.Task) require.Equal(t, "com.example.app", args.ApplicationID) + require.Equal(t, uint(1), args.AppTeamID) require.Equal(t, "enterprises/test", args.EnterpriseName) + require.False(t, args.AppConfigChanged)🤖 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/worker/software_worker_test.go` around lines 265 - 268, The test assertions in the block checking Task, ApplicationID, and EnterpriseName are incomplete and do not verify the full batch job argument contract. Add require.Equal assertions for the AppTeamID and AppConfigChanged fields on the args object immediately after the existing assertions to ensure the phase-2 job payload wiring is properly validated and prevent future regressions.
🤖 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/worker/software_worker_test.go`:
- Around line 265-268: The test assertions in the block checking Task,
ApplicationID, and EnterpriseName are incomplete and do not verify the full
batch job argument contract. Add require.Equal assertions for the AppTeamID and
AppConfigChanged fields on the args object immediately after the existing
assertions to ensure the phase-2 job payload wiring is properly validated and
prevent future regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5377a20c-6f07-484e-9209-46643eddcc33
📒 Files selected for processing (3)
changes/47543-android-staggered-job-queuingserver/worker/software_worker.goserver/worker/software_worker_test.go
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 refactors the Android “make app available” worker flow to avoid sleeping inside a single long-running job by splitting work into staggered, delayed sub-jobs. This aligns makeAndroidAppAvailable with the existing staggered queuing pattern used by other Android software worker tasks and helps prevent hitting the integrations worker max runtime on large host sets.
Changes:
- Added a new worker task (
make_android_app_available_batch) and corresponding handler to execute work on a chunk of hosts. - Updated
makeAndroidAppAvailableto resolve hosts at runtime and queue delayed batch jobs instead of doing in-job batching + sleeping. - Updated tests to validate that phase-1 queues the expected number of delayed batch jobs (and removed synctest/atomic usage).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/worker/software_worker.go | Adds a new batch task + refactors makeAndroidAppAvailable to queue staggered sub-jobs and implements the batch execution logic. |
| server/worker/software_worker_test.go | Updates batching test to assert job queuing + not_before staggering rather than observing in-job sleeps. |
| changes/47543-android-staggered-job-queuing | User-visible change entry (content excluded from review by policy). |
Files excluded by content exclusion policy (1)
- changes/47543-android-staggered-job-queuing
💡 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 #47880 +/- ##
==========================================
+ Coverage 67.23% 67.33% +0.09%
==========================================
Files 3639 3641 +2
Lines 230043 230669 +626
Branches 11836 11836
==========================================
+ Hits 154678 155313 +635
+ Misses 61474 61387 -87
- Partials 13891 13969 +78
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:
|
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 3 out of 4 changed files in this pull request and generated 4 comments.
Files excluded by content exclusion policy (1)
- changes/47543-android-staggered-job-queuing
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/worker/software_worker_test.go (1)
310-325: ⚡ Quick winStrengthen pending-config assertions to prevent false positives.
This test only checks
len(pendingConfigs) == 2; it can still pass if the wrong hosts (or duplicate host IDs) are updated. Capture UUID+policyVersion and assert exact host coverage (and expected version) for this batch.Suggested assertion upgrade
- var pendingConfigs []string + pendingConfigs := make(map[string]int64) ds.SetAndroidAppInstallPendingApplyConfigFunc = func(ctx context.Context, hostUUID, applicationID string, policyVersion int64) error { - pendingConfigs = append(pendingConfigs, hostUUID) + pendingConfigs[hostUUID] = policyVersion return nil } @@ - require.Len(t, pendingConfigs, 2, "appConfigChanged=true should update both hosts") + require.Len(t, pendingConfigs, 2, "appConfigChanged=true should update both hosts") + require.Equal(t, int64(42), pendingConfigs["host-1"]) + require.Equal(t, int64(42), pendingConfigs["host-2"])🤖 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/worker/software_worker_test.go` around lines 310 - 325, The test for makeAndroidAppAvailableBatch only asserts that pendingConfigs has length 2, which doesn't verify that the correct hosts or policy versions were actually updated. Modify the SetAndroidAppInstallPendingApplyConfigFunc mock to capture both the hostUUID and policyVersion parameters (not just hostUUID) into a data structure that can be verified. Then replace the simple length assertion with a more thorough assertion that verifies both the exact host IDs that were updated (matching the expected hosts map keys) and that they were all updated with the expected policyVersion value (1) to prevent false positives from wrong hosts or duplicates being processed.
🤖 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/worker/software_worker_test.go`:
- Around line 371-374: The test assertion checking for NotContains of the
placeholder variable is insufficient for verifying correct substitution. In the
loop iterating over capturedConfigs, replace the weak assertion with stronger
ones that verify the capturedConfigs count matches the expected number of hosts
and that each iteration of the loop contains the actual expected host UUID value
(not just the absence of the placeholder). This ensures that variable
substitution is actually working correctly with the proper values, not just that
placeholders are removed.
---
Nitpick comments:
In `@server/worker/software_worker_test.go`:
- Around line 310-325: The test for makeAndroidAppAvailableBatch only asserts
that pendingConfigs has length 2, which doesn't verify that the correct hosts or
policy versions were actually updated. Modify the
SetAndroidAppInstallPendingApplyConfigFunc mock to capture both the hostUUID and
policyVersion parameters (not just hostUUID) into a data structure that can be
verified. Then replace the simple length assertion with a more thorough
assertion that verifies both the exact host IDs that were updated (matching the
expected hosts map keys) and that they were all updated with the expected
policyVersion value (1) to prevent false positives from wrong hosts or
duplicates being processed.
🪄 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: 46237e14-6cda-4e85-a3a0-bc1338671926
📒 Files selected for processing (3)
server/service/integration_mdm_setup_experience_test.goserver/worker/software_worker.goserver/worker/software_worker_test.go
| const androidSoftwareInstallStaggerInterval = 60 * time.Second | ||
| const ( | ||
| androidSoftwareInstallStaggerInterval = 60 * time.Second | ||
| defaultAndroidBatchSize = 1000 |
There was a problem hiding this comment.
Given current rate limits should this batch size perhaps be smaller? Feels like this leaves little overhead
There was a problem hiding this comment.
Good point, 1000 would take up nearly 100% of our quota. That math only works if we don't share AMAPI quotas for our clients.
Related issue: Resolves #47543
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit
mdm.android_batch_size) to 100.