Android Load testing with osquery perf#48535
Open
ksykulev wants to merge 1 commit into
Open
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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends Fleet’s existing osquery-perf load-testing harness to simulate Android MDM behavior (enrollment, periodic status reporting, and command acknowledgements) and adds a lightweight Android Management API (AMAPI) mock/proxy to support realistic policy/command flows at scale.
Changes:
- Add Android-specific stats counters and reporting in
osquery-perf. - Introduce an
androidAgentsimulator that sends Android PubSub push payloads to Fleet and polls a mock AMAPI proxy for policy/command state. - Add
android-amapi-mock, a standalone mock/proxy server for AMAPI endpoints plus a coordination API used byosquery-perf.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/osquery-perf/osquery_perf/stats.go | Adds Android counters and logs Android activity alongside existing osquery/orbit/MDM metrics. |
| cmd/osquery-perf/android.tmpl | Placeholder template enabling --os_templates=android to work with existing template parsing. |
| cmd/osquery-perf/android_agent.go | Implements a fake Android device loop (enroll → periodic status → command ack) using Fleet’s PubSub endpoint + mock proxy coordination. |
| cmd/osquery-perf/agent.go | Wires Android template selection and adds Android-specific CLI flags for osquery-perf. |
| cmd/android-amapi-mock/middleware.go | Adds middleware for forwarding non-fake-device requests to Google AMAPI when configured. |
| cmd/android-amapi-mock/main.go | Implements the android-amapi-mock command wiring: routes, forwarding, and coordination API endpoints. |
| cmd/android-amapi-mock/handlers.go | Adds mock handlers for device/policy/command endpoints and coordination endpoints. |
| cmd/android-amapi-mock/google_forwarder.go | Implements Google AMAPI forwarding via the official SDK when credentials are provided. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
304
to
308
| log.Printf( | ||
| "uptime: %s, error rate: %.2f, osquery enrolls: %d, orbit enrolls: %d, mdm enrolls: %d, distributed/reads: %d, distributed/writes: %d, config requests: %d, result log requests: %d, mdm sessions initiated: %d, mdm on-demand syncs: %d, mdm commands received: %d, config errors: %d, distributed/read errors: %d, distributed/write errors: %d, log result errors: %d, orbit errors: %d, desktop errors: %d, mdm errors: %d, mdm scep requests: %d, mdm scep success: %d, mdm scep errors: %d, ddm tokens success: %d, ddm tokens errors: %d, ddm declaration items success: %d, ddm declaration items errors: %d, ddm activation success: %d, ddm activation errors: %d, ddm configuration success: %d, ddm configuration errors: %d, ddm status success: %d, ddm status errors: %d, buffered logs: %d, script execs (errs): %d (%d), software installs (errs): %d (%d)", | ||
| "uptime: %s, error rate: %.2f, osquery enrolls: %d, orbit enrolls: %d, mdm enrolls: %d, distributed/reads: %d, distributed/writes: %d, config requests: %d, result log requests: %d, mdm sessions initiated: %d, mdm on-demand syncs: %d, mdm commands received: %d, config errors: %d, distributed/read errors: %d, distributed/write errors: %d, log result errors: %d, orbit errors: %d, desktop errors: %d, mdm errors: %d, mdm scep requests: %d, mdm scep success: %d, mdm scep errors: %d, ddm tokens success: %d, ddm tokens errors: %d, ddm declaration items success: %d, ddm declaration items errors: %d, ddm activation success: %d, ddm activation errors: %d, ddm configuration success: %d, ddm configuration errors: %d, ddm status success: %d, ddm status errors: %d, buffered logs: %d, script execs (errs): %d (%d), software installs (errs): %d (%d), android enrolls: %d, android status reports: %d, android command acks: %d, android errors: %d", | ||
| time.Since(s.StartTime).Round(time.Second), | ||
| float64(s.errors)/float64(s.osqueryEnrollments), | ||
| s.osqueryEnrollments, |
Comment on lines
+229
to
+233
| if google != nil && hasSeenRealDevice.Load() { | ||
| go func() { | ||
| google.ForwardPoliciesPatch(&discardResponseWriter{}, r) | ||
| }() | ||
| } |
Comment on lines
+24
to
+33
| switch r.Method { | ||
| case "GET": | ||
| google.ForwardDevicesGet(w, r) | ||
| case "PATCH": | ||
| google.ForwardDevicesPatch(w, r) | ||
| case "DELETE": | ||
| google.ForwardDevicesDelete(w, r) | ||
| case "POST": | ||
| google.ForwardIssueCommand(w, r) | ||
| } |
Contributor
WalkthroughThis change adds a new 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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: 7
🧹 Nitpick comments (2)
cmd/osquery-perf/android_agent.go (1)
198-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegister/enrollment failures aren't counted in Stats.
Unlike the per-iteration status-report/poll/command-ack failures in the loop below (which call
a.stats.IncrementAndroidErrors()), the initialregisterWithProxyandsendEnrollmentfailures just log and return, leaving these failures invisible in the aggregated stats output.🔧 Suggested fix
if err := a.registerWithProxy(); err != nil { log.Printf("Android agent %d: failed to register with proxy: %v", a.agentIndex, err) + a.stats.IncrementAndroidErrors() return } // Step 2: Send ENROLLMENT PubSub to Fleet if err := a.sendEnrollment(); err != nil { log.Printf("Android agent %d: enrollment failed: %v", a.agentIndex, err) + a.stats.IncrementAndroidErrors() return }🤖 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 `@cmd/osquery-perf/android_agent.go` around lines 198 - 209, The initial register/enrollment failures in androidAgent.runLoop are only logged, so they never show up in aggregated error stats. Update the registerWithProxy and sendEnrollment error paths in runLoop to also call a.stats.IncrementAndroidErrors() before returning, matching the existing failure handling used for the per-iteration status-report/poll/command-ack paths.cmd/osquery-perf/agent.go (1)
4059-4062: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate Android flags before starting any agents, not mid-loop.
The required-flag check only fires when the loop reaches the first
android.tmplhost. If--os_templatesmixes android with other templates, earlier non-android hosts will already be enrolling against Fleet before this fatal error triggers, wasting the partial run.🔧 Suggested fix: validate right after flag parsing
flag.Parse() rand.Seed(*randSeed) + + if strings.Contains(*osTemplates, "android") && + (*androidPubSubToken == "" || *androidProxyAddress == "" || *androidEnterpriseID == "") { + log.Fatalf("Android template requires --android_pubsub_token, --android_proxy_address, and --android_enterprise_id flags") + }🤖 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 `@cmd/osquery-perf/agent.go` around lines 4059 - 4062, Move the Android required-flag validation out of the host-processing loop in agent.go and run it immediately after flag parsing, before any enrollment or agent-start logic begins. Use the existing android.tmpl check and the --android_pubsub_token, --android_proxy_address, and --android_enterprise_id flags to fail fast up front so mixed-template runs cannot partially enroll non-Android hosts before the fatal error.
🤖 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 `@cmd/android-amapi-mock/google_forwarder.go`:
- Around line 104-108: ForwardDevicesList is swallowing Google API list errors
by logging and breaking out of the loop, which can make callers think the
request succeeded with a partial or empty result. Update the error path in
googleForwarder’s device-list loop to return the failure from ForwardDevicesList
instead of continuing, and make the HTTP handler that calls it propagate that
error as a failed response rather than a 200. Use the existing
ForwardDevicesList and call.Do symbols to locate the affected flow.
In `@cmd/android-amapi-mock/handlers.go`:
- Around line 229-232: The asynchronous Google forward in the handler reuses the
live request, so `ForwardPoliciesPatch` may see a canceled context or a closed
body after the handler returns. In the `google != nil &&
hasSeenRealDevice.Load()` branch, clone the incoming request before starting the
goroutine by creating a detached copy with a new context and a copied body, then
pass that cloned request into `google.ForwardPoliciesPatch` instead of `r`.
- Around line 104-107: The request body parsing in the handler that reads r.Body
and unmarshals into reqBody currently ignores malformed JSON, causing bad device
patch requests to succeed as empty patches. Update this logic to check the
json.Unmarshal error in the relevant handler in handlers.go and return a 400
response for invalid JSON instead of proceeding; keep the fix localized around
the request parsing path that handles the patch request.
- Around line 315-323: The catch-all in handleCatchAll currently returns a
successful empty JSON response for unsupported AMAPI paths, which masks missing
coverage. Update handleCatchAll to send an error status for unhandled requests
and return an error payload instead of "{}"; keep the existing logging and use
the googleForwarder context to preserve the current log message behavior.
- Around line 189-202: Validate the parsed pageToken before using it to slice
allDevices in the handler that builds resp. The current offset can become
negative from fmt.Sscanf, so add bounds checks to clamp offset to the valid
range before computing end and before evaluating allDevices[offset:end],
ensuring both offset and end are safe for slicing.
- Around line 170-184: The device list handler is mixing fake devices from all
enterprises because it always uses store.allDeviceNames() in the GET
/v1/enterprises/{eid}/devices flow. Update the handler in handlers.go so the
fake-device lookup is scoped by the requested enterprise ID (the same
r.PathValue("eid") used for enterpriseName), and only append fake devices
belonging to that enterprise before returning allDevices.
In `@cmd/osquery-perf/android_agent.go`:
- Line 268: The outbound HTTP calls in registerWithProxy, pollProxyState, and
sendPubSubMessage currently use http.Post/http.Get with http.DefaultClient and
no timeout, so they can hang indefinitely. Update these paths to use a shared
http.Client with an explicit Timeout (while preserving the existing TLS
transport customization from main in agent.go), and route the proxy
registration, polling, and Fleet PubSub requests through that client instead of
the default helpers.
---
Nitpick comments:
In `@cmd/osquery-perf/agent.go`:
- Around line 4059-4062: Move the Android required-flag validation out of the
host-processing loop in agent.go and run it immediately after flag parsing,
before any enrollment or agent-start logic begins. Use the existing android.tmpl
check and the --android_pubsub_token, --android_proxy_address, and
--android_enterprise_id flags to fail fast up front so mixed-template runs
cannot partially enroll non-Android hosts before the fatal error.
In `@cmd/osquery-perf/android_agent.go`:
- Around line 198-209: The initial register/enrollment failures in
androidAgent.runLoop are only logged, so they never show up in aggregated error
stats. Update the registerWithProxy and sendEnrollment error paths in runLoop to
also call a.stats.IncrementAndroidErrors() before returning, matching the
existing failure handling used for the per-iteration
status-report/poll/command-ack paths.
🪄 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: 420d2d0b-a623-43da-ae9c-78a99b65d9e5
📒 Files selected for processing (8)
cmd/android-amapi-mock/google_forwarder.gocmd/android-amapi-mock/handlers.gocmd/android-amapi-mock/main.gocmd/android-amapi-mock/middleware.gocmd/osquery-perf/agent.gocmd/osquery-perf/android.tmplcmd/osquery-perf/android_agent.gocmd/osquery-perf/osquery_perf/stats.go
Comment on lines
+104
to
+108
| resp, err := call.Do() | ||
| if err != nil { | ||
| log.Printf("googleForwarder: list devices failed: %v", err) | ||
| break | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate Google list failures instead of returning a successful partial list.
Line 104 logs and breaks on Google API errors, so callers can return 200 with an empty/truncated device list. Return an error from ForwardDevicesList and let the HTTP handler emit a failure instead.
🐛 Proposed fix
-func (g *googleForwarder) ForwardDevicesList(enterpriseName string, ctx context.Context) []map[string]string {
+func (g *googleForwarder) ForwardDevicesList(enterpriseName string, ctx context.Context) ([]map[string]string, error) {
var allDevices []map[string]string
@@
resp, err := call.Do()
if err != nil {
- log.Printf("googleForwarder: list devices failed: %v", err)
- break
+ return nil, fmt.Errorf("list google devices: %w", err)
}
@@
- return allDevices
+ return allDevices, nil
}🤖 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 `@cmd/android-amapi-mock/google_forwarder.go` around lines 104 - 108,
ForwardDevicesList is swallowing Google API list errors by logging and breaking
out of the loop, which can make callers think the request succeeded with a
partial or empty result. Update the error path in googleForwarder’s device-list
loop to return the failure from ForwardDevicesList instead of continuing, and
make the HTTP handler that calls it propagate that error as a failed response
rather than a 200. Use the existing ForwardDevicesList and call.Do symbols to
locate the affected flow.
Comment on lines
+104
to
+107
| if r.Body != nil { | ||
| body, _ := io.ReadAll(r.Body) | ||
| json.Unmarshal(body, &reqBody) //nolint:errcheck | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return 400 for malformed device patch JSON.
Invalid JSON is currently ignored, so a bad Fleet request is treated as an empty successful patch.
🐛 Proposed fix
if r.Body != nil {
- body, _ := io.ReadAll(r.Body)
- json.Unmarshal(body, &reqBody) //nolint:errcheck
+ if err := json.NewDecoder(r.Body).Decode(&reqBody); err != nil && err != io.EOF {
+ http.Error(w, "invalid json: "+err.Error(), http.StatusBadRequest)
+ return
+ }
}📝 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
| if r.Body != nil { | |
| body, _ := io.ReadAll(r.Body) | |
| json.Unmarshal(body, &reqBody) //nolint:errcheck | |
| } | |
| if r.Body != nil { | |
| if err := json.NewDecoder(r.Body).Decode(&reqBody); err != nil && err != io.EOF { | |
| http.Error(w, "invalid json: "+err.Error(), http.StatusBadRequest) | |
| return | |
| } | |
| } |
🤖 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 `@cmd/android-amapi-mock/handlers.go` around lines 104 - 107, The request body
parsing in the handler that reads r.Body and unmarshals into reqBody currently
ignores malformed JSON, causing bad device patch requests to succeed as empty
patches. Update this logic to check the json.Unmarshal error in the relevant
handler in handlers.go and return a 400 response for invalid JSON instead of
proceeding; keep the fix localized around the request parsing path that handles
the patch request.
Comment on lines
+170
to
+184
| fakeNames := store.allDeviceNames() | ||
|
|
||
| var realDevices []map[string]string | ||
| if google != nil { | ||
| enterpriseName := "enterprises/" + r.PathValue("eid") | ||
| realDevices = google.ForwardDevicesList(enterpriseName, r.Context()) | ||
| if len(realDevices) > 0 { | ||
| hasSeenRealDevice.Store(true) | ||
| } | ||
| } | ||
|
|
||
| allDevices := make([]map[string]string, 0, len(realDevices)+len(fakeNames)) | ||
| allDevices = append(allDevices, realDevices...) | ||
| for _, name := range fakeNames { | ||
| allDevices = append(allDevices, map[string]string{"name": name}) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Scope fake device list results to the requested enterprise.
GET /v1/enterprises/{eid}/devices appends every fake device from store.allDeviceNames(), so multi-enterprise tests can see devices from the wrong enterprise.
🐛 Proposed fix
- fakeNames := store.allDeviceNames()
+ enterpriseID := r.PathValue("eid")
+ fakeNames := store.deviceNamesForEnterprise(enterpriseID)
@@
- enterpriseName := "enterprises/" + r.PathValue("eid")
+ enterpriseName := "enterprises/" + enterpriseID-func (ds *deviceStore) allDeviceNames() []string {
+func (ds *deviceStore) deviceNamesForEnterprise(enterpriseID string) []string {
ds.mu.RLock()
defer ds.mu.RUnlock()
names := make([]string, 0, len(ds.byName))
+ prefix := "enterprises/" + enterpriseID + "/devices/"
for name := range ds.byName {
- names = append(names, name)
+ if strings.HasPrefix(name, prefix) {
+ names = append(names, name)
+ }
}
return names
}📝 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
| fakeNames := store.allDeviceNames() | |
| var realDevices []map[string]string | |
| if google != nil { | |
| enterpriseName := "enterprises/" + r.PathValue("eid") | |
| realDevices = google.ForwardDevicesList(enterpriseName, r.Context()) | |
| if len(realDevices) > 0 { | |
| hasSeenRealDevice.Store(true) | |
| } | |
| } | |
| allDevices := make([]map[string]string, 0, len(realDevices)+len(fakeNames)) | |
| allDevices = append(allDevices, realDevices...) | |
| for _, name := range fakeNames { | |
| allDevices = append(allDevices, map[string]string{"name": name}) | |
| enterpriseID := r.PathValue("eid") | |
| fakeNames := store.deviceNamesForEnterprise(enterpriseID) | |
| var realDevices []map[string]string | |
| if google != nil { | |
| enterpriseName := "enterprises/" + enterpriseID | |
| realDevices = google.ForwardDevicesList(enterpriseName, r.Context()) | |
| if len(realDevices) > 0 { | |
| hasSeenRealDevice.Store(true) | |
| } | |
| } | |
| allDevices := make([]map[string]string, 0, len(realDevices)+len(fakeNames)) | |
| allDevices = append(allDevices, realDevices...) | |
| for _, name := range fakeNames { | |
| allDevices = append(allDevices, map[string]string{"name": name}) | |
| } |
Suggested change
| fakeNames := store.allDeviceNames() | |
| var realDevices []map[string]string | |
| if google != nil { | |
| enterpriseName := "enterprises/" + r.PathValue("eid") | |
| realDevices = google.ForwardDevicesList(enterpriseName, r.Context()) | |
| if len(realDevices) > 0 { | |
| hasSeenRealDevice.Store(true) | |
| } | |
| } | |
| allDevices := make([]map[string]string, 0, len(realDevices)+len(fakeNames)) | |
| allDevices = append(allDevices, realDevices...) | |
| for _, name := range fakeNames { | |
| allDevices = append(allDevices, map[string]string{"name": name}) | |
| func (ds *deviceStore) deviceNamesForEnterprise(enterpriseID string) []string { | |
| ds.mu.RLock() | |
| defer ds.mu.RUnlock() | |
| names := make([]string, 0, len(ds.byName)) | |
| prefix := "enterprises/" + enterpriseID + "/devices/" | |
| for name := range ds.byName { | |
| if strings.HasPrefix(name, prefix) { | |
| names = append(names, name) | |
| } | |
| } | |
| return names | |
| } |
🤖 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 `@cmd/android-amapi-mock/handlers.go` around lines 170 - 184, The device list
handler is mixing fake devices from all enterprises because it always uses
store.allDeviceNames() in the GET /v1/enterprises/{eid}/devices flow. Update the
handler in handlers.go so the fake-device lookup is scoped by the requested
enterprise ID (the same r.PathValue("eid") used for enterpriseName), and only
append fake devices belonging to that enterprise before returning allDevices.
Comment on lines
+189
to
+202
| if pt := r.URL.Query().Get("pageToken"); pt != "" { | ||
| fmt.Sscanf(pt, "%d", &offset) | ||
| } | ||
|
|
||
| end := offset + pageSize | ||
| if end > len(allDevices) { | ||
| end = len(allDevices) | ||
| } | ||
| if offset > len(allDevices) { | ||
| offset = len(allDevices) | ||
| } | ||
|
|
||
| resp := map[string]any{ | ||
| "devices": allDevices[offset:end], |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Validate pageToken before slicing.
A negative pageToken parses into a negative offset and panics at allDevices[offset:end].
🐛 Proposed fix
+ pageToken := r.URL.Query().Get("pageToken")
pageSize := 100
offset := 0
- if pt := r.URL.Query().Get("pageToken"); pt != "" {
- fmt.Sscanf(pt, "%d", &offset)
+ if pageToken != "" {
+ parsed, err := strconv.Atoi(pageToken)
+ if err != nil || parsed < 0 {
+ http.Error(w, "invalid pageToken", http.StatusBadRequest)
+ return
+ }
+ offset = parsed
}📝 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
| if pt := r.URL.Query().Get("pageToken"); pt != "" { | |
| fmt.Sscanf(pt, "%d", &offset) | |
| } | |
| end := offset + pageSize | |
| if end > len(allDevices) { | |
| end = len(allDevices) | |
| } | |
| if offset > len(allDevices) { | |
| offset = len(allDevices) | |
| } | |
| resp := map[string]any{ | |
| "devices": allDevices[offset:end], | |
| pageToken := r.URL.Query().Get("pageToken") | |
| pageSize := 100 | |
| offset := 0 | |
| if pageToken != "" { | |
| parsed, err := strconv.Atoi(pageToken) | |
| if err != nil || parsed < 0 { | |
| http.Error(w, "invalid pageToken", http.StatusBadRequest) | |
| return | |
| } | |
| offset = parsed | |
| } | |
| end := offset + pageSize | |
| if end > len(allDevices) { | |
| end = len(allDevices) | |
| } | |
| if offset > len(allDevices) { | |
| offset = len(allDevices) | |
| } | |
| resp := map[string]any{ | |
| "devices": allDevices[offset:end], |
🤖 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 `@cmd/android-amapi-mock/handlers.go` around lines 189 - 202, Validate the
parsed pageToken before using it to slice allDevices in the handler that builds
resp. The current offset can become negative from fmt.Sscanf, so add bounds
checks to clamp offset to the valid range before computing end and before
evaluating allDevices[offset:end], ensuring both offset and end are safe for
slicing.
Comment on lines
+229
to
+232
| if google != nil && hasSeenRealDevice.Load() { | ||
| go func() { | ||
| google.ForwardPoliciesPatch(&discardResponseWriter{}, r) | ||
| }() |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don’t reuse the live request in the async Google forward.
The goroutine uses r.Context() and r.Body after the handler can return, so the context may be canceled and the body closed before ForwardPoliciesPatch reads it. Clone the request with a detached context and copied body before starting the goroutine.
🤖 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 `@cmd/android-amapi-mock/handlers.go` around lines 229 - 232, The asynchronous
Google forward in the handler reuses the live request, so `ForwardPoliciesPatch`
may see a canceled context or a closed body after the handler returns. In the
`google != nil && hasSeenRealDevice.Load()` branch, clone the incoming request
before starting the goroutine by creating a detached copy with a new context and
a copied body, then pass that cloned request into `google.ForwardPoliciesPatch`
instead of `r`.
Comment on lines
+315
to
+323
| func handleCatchAll(google *googleForwarder) http.HandlerFunc { | ||
| return func(w http.ResponseWriter, r *http.Request) { | ||
| if google != nil { | ||
| log.Printf("Unhandled request (no Google SDK mapping): %s %s", r.Method, r.URL.Path) | ||
| } else { | ||
| log.Printf("Mock AMAPI: unhandled %s %s", r.Method, r.URL.Path) | ||
| } | ||
| w.Header().Set("Content-Type", "application/json") | ||
| fmt.Fprint(w, "{}") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return an error for unhandled AMAPI endpoints.
A 200 {} catch-all hides missing mock/forwarding coverage, which can make load tests pass while Fleet is calling an unsupported AMAPI endpoint.
🐛 Proposed fix
w.Header().Set("Content-Type", "application/json")
- fmt.Fprint(w, "{}")
+ w.WriteHeader(http.StatusNotImplemented)
+ fmt.Fprint(w, `{"error":{"code":501,"message":"AMAPI endpoint not implemented","status":"NOT_IMPLEMENTED"}}`)
}
}📝 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
| func handleCatchAll(google *googleForwarder) http.HandlerFunc { | |
| return func(w http.ResponseWriter, r *http.Request) { | |
| if google != nil { | |
| log.Printf("Unhandled request (no Google SDK mapping): %s %s", r.Method, r.URL.Path) | |
| } else { | |
| log.Printf("Mock AMAPI: unhandled %s %s", r.Method, r.URL.Path) | |
| } | |
| w.Header().Set("Content-Type", "application/json") | |
| fmt.Fprint(w, "{}") | |
| func handleCatchAll(google *googleForwarder) http.HandlerFunc { | |
| return func(w http.ResponseWriter, r *http.Request) { | |
| if google != nil { | |
| log.Printf("Unhandled request (no Google SDK mapping): %s %s", r.Method, r.URL.Path) | |
| } else { | |
| log.Printf("Mock AMAPI: unhandled %s %s", r.Method, r.URL.Path) | |
| } | |
| w.Header().Set("Content-Type", "application/json") | |
| w.WriteHeader(http.StatusNotImplemented) | |
| fmt.Fprint(w, `{"error":{"code":501,"message":"AMAPI endpoint not implemented","status":"NOT_IMPLEMENTED"}}`) |
🤖 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 `@cmd/android-amapi-mock/handlers.go` around lines 315 - 323, The catch-all in
handleCatchAll currently returns a successful empty JSON response for
unsupported AMAPI paths, which masks missing coverage. Update handleCatchAll to
send an error status for unhandled requests and return an error payload instead
of "{}"; keep the existing logging and use the googleForwarder context to
preserve the current log message behavior.
| return fmt.Errorf("marshal register body: %w", err) | ||
| } | ||
|
|
||
| resp, err := http.Post(a.proxyAddress+"/mock/devices/register", "application/json", bytes.NewReader(data)) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No timeout on outbound HTTP calls (proxy register/poll, Fleet PubSub POST).
registerWithProxy, pollProxyState, and sendPubSubMessage all use http.Post/http.Get, which use http.DefaultClient. That client's Transport is customized for TLS in agent.go's main(), but no Timeout is ever set, so any of these calls can hang indefinitely if the mock proxy or Fleet server stalls — blocking the agent goroutine and skewing load-test results/timing.
🔧 Suggested fix: use a client with a timeout
+var androidHTTPClient = &http.Client{Timeout: 30 * time.Second}
+
func (a *androidAgent) registerWithProxy() error {
...
- resp, err := http.Post(a.proxyAddress+"/mock/devices/register", "application/json", bytes.NewReader(data))
+ resp, err := androidHTTPClient.Post(a.proxyAddress+"/mock/devices/register", "application/json", bytes.NewReader(data))Apply similarly to pollProxyState and sendPubSubMessage.
Also applies to: 283-283, 409-409
🤖 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 `@cmd/osquery-perf/android_agent.go` at line 268, The outbound HTTP calls in
registerWithProxy, pollProxyState, and sendPubSubMessage currently use
http.Post/http.Get with http.DefaultClient and no timeout, so they can hang
indefinitely. Update these paths to use a shared http.Client with an explicit
Timeout (while preserving the existing TLS transport customization from main in
agent.go), and route the proxy registration, polling, and Fleet PubSub requests
through that client instead of the default helpers.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #48535 +/- ##
==========================================
+ Coverage 67.48% 67.79% +0.30%
==========================================
Files 3676 3682 +6
Lines 233629 234398 +769
Branches 12261 12261
==========================================
+ Hits 157672 158901 +1229
+ Misses 61806 61197 -609
- Partials 14151 14300 +149
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:
|
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 #26225
go run ./cmd/android-amapi-mock --listen :9999 --google-credentials <path/to/credentials>go run ./cmd/osquery-perf --server_url https://localhost:8080 --enroll_secret <secret> --os_templates android:2 --host_count 2 --android_pubsub_token '<token>' --android_proxy_address http://localhost:9999 --android_enterprise_id <enterprise id> --android_status_interval 30sTesting
Summary by CodeRabbit
New Features
Bug Fixes