Removed duplicate FlippingPoliciesForHost DB calls#42845
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
This PR reduces redundant MySQL queries during host policy result submission by deduplicating repeated FlippingPoliciesForHost calls within SubmitDistributedQueryResults, aiming to lower DB load during large-fleet check-ins.
Changes:
- Precompute flipping policy sets once per request and reuse them for scripts, software installers, VPP, and webhook handling.
- Extend
RecordPolicyQueryExecutionsto optionally accept precomputed “newly passing” policy IDs to avoid an additional flipping query. - Update mocks/tests and add a changelog entry to reflect the optimization.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/osquery.go | Precomputes flipping results once; threads newFailingSet through policy automation consumers; replaces webhook flipping query with in-process filtering. |
| server/service/osquery_test.go | Updates mocked RecordPolicyQueryExecutionsFunc signature to include the new parameter. |
| server/service/orbit_test.go | Updates RecordPolicyQueryExecutions calls to include the new parameter. |
| server/service/integration_enterprise_test.go | Updates RecordPolicyQueryExecutions calls to include the new parameter across integration tests. |
| server/service/integration_core_test.go | Updates RecordPolicyQueryExecutions calls to include the new parameter across integration tests. |
| server/service/async/async_test.go | Updates mocked RecordPolicyQueryExecutionsFunc signature to include the new parameter. |
| server/service/async/async_policy.go | Threads new parameter through async task wrapper to datastore implementation. |
| server/service/async/async_policy_test.go | Updates RecordPolicyQueryExecutions calls to include the new parameter. |
| server/mock/datastore_mock.go | Updates datastore mock interface + forwarding method to accept the new parameter. |
| server/fleet/datastore.go | Updates datastore interface contract and documents the new optional argument. |
| server/datastore/mysql/policies.go | Uses provided “newly passing” IDs to skip internal FlippingPoliciesForHost call. |
| server/datastore/mysql/policies_test.go | Updates MySQL datastore tests for the new RecordPolicyQueryExecutions signature. |
| server/datastore/mysql/labels_test.go | Updates calls to RecordPolicyQueryExecutions to include the new parameter. |
| server/datastore/mysql/hosts_test.go | Updates calls to RecordPolicyQueryExecutions to include the new parameter. |
| server/datastore/mysql/conditional_access_bypass_test.go | Updates calls to RecordPolicyQueryExecutions to include the new parameter. |
| changes/42836-deduplicate-flipping-policies-queries | Adds a changelog note for the DB query reduction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThis PR deduplicates redundant database queries during policy result submission. When a host submits policy results via Possibly related PRs
🚥 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 (2)
server/service/osquery_test.go (1)
3352-3354: Consider assertingnewlyPassingPolicyIDsin these mocks.The new parameter is wired in, but not validated. Adding assertions here would lock in the new precomputed-flips contract and catch propagation regressions early.
Also applies to: 3663-3665
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/osquery_test.go` around lines 3352 - 3354, The mock for ds.RecordPolicyQueryExecutionsFunc currently accepts the newlyPassingPolicyIDs parameter but doesn't validate it; update the mock bodies (the one in the osquery test and the other instance around the second occurrence) to assert the received newlyPassingPolicyIDs equals the test's expected value (e.g., compare to an expectedNewlyPassingPolicyIDs slice or check length/contents) using the existing test assertion helpers (require/require.Equal/assert.ElementsMatch, etc.), so the precomputed-flips contract is enforced and regressions are caught.server/datastore/mysql/policies_test.go (1)
463-467: Add one explicit test for non-nilnewlyPassingPolicyIDs.This file now validates the new signature shape, but only with
nil. A focused case with a populatednewlyPassingPolicyIDswould better lock in the new execution path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/policies_test.go` around lines 463 - 467, The test suite never exercises a non-nil newlyPassingPolicyIDs map; add one explicit test call to RecordPolicyQueryExecutions that passes a non-nil map (e.g., map[uint]*bool{p.ID: ptr.Bool(true)}) for newlyPassingPolicyIDs and assert require.NoError on the call so the new execution path is covered; place this call alongside the existing RecordPolicyQueryExecutions calls in policies_test.go and reference RecordPolicyQueryExecutions, newlyPassingPolicyIDs and p.ID to locate where to add it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/datastore/mysql/policies_test.go`:
- Around line 463-467: The test suite never exercises a non-nil
newlyPassingPolicyIDs map; add one explicit test call to
RecordPolicyQueryExecutions that passes a non-nil map (e.g.,
map[uint]*bool{p.ID: ptr.Bool(true)}) for newlyPassingPolicyIDs and assert
require.NoError on the call so the new execution path is covered; place this
call alongside the existing RecordPolicyQueryExecutions calls in
policies_test.go and reference RecordPolicyQueryExecutions,
newlyPassingPolicyIDs and p.ID to locate where to add it.
In `@server/service/osquery_test.go`:
- Around line 3352-3354: The mock for ds.RecordPolicyQueryExecutionsFunc
currently accepts the newlyPassingPolicyIDs parameter but doesn't validate it;
update the mock bodies (the one in the osquery test and the other instance
around the second occurrence) to assert the received newlyPassingPolicyIDs
equals the test's expected value (e.g., compare to an
expectedNewlyPassingPolicyIDs slice or check length/contents) using the existing
test assertion helpers (require/require.Equal/assert.ElementsMatch, etc.), so
the precomputed-flips contract is enforced and regressions are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7daaf6d2-6f47-4ef4-a942-add0937a0257
📒 Files selected for processing (16)
changes/42836-deduplicate-flipping-policies-queriesserver/datastore/mysql/conditional_access_bypass_test.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/labels_test.goserver/datastore/mysql/policies.goserver/datastore/mysql/policies_test.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/service/async/async_policy.goserver/service/async/async_policy_test.goserver/service/async/async_test.goserver/service/integration_core_test.goserver/service/integration_enterprise_test.goserver/service/orbit_test.goserver/service/osquery.goserver/service/osquery_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #42845 +/- ##
==========================================
+ Coverage 66.81% 66.84% +0.02%
==========================================
Files 2541 2578 +37
Lines 203971 206826 +2855
Branches 9278 9278
==========================================
+ Hits 136282 138245 +1963
- Misses 55343 56017 +674
- Partials 12346 12564 +218
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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.
lucasmrod
left a comment
lucasmrod
left a comment
There was a problem hiding this comment.
LGTM! Left some questions.
We should add a test with non-nil newlyPassing on RecordPolicyQueryExecutions in the mysql package.
| @@ -0,0 +1 @@ | |||
| - Reduced redundant database queries during policy result submission by computing flipping policies once per host check-in instead of up to five times. | |||
There was a problem hiding this comment.
For most common scenario it won't be 5 times, a policy usually has one purpose, so the optimization will actually be from 2 to 1 time (any of the automations + RecordPolicyQueryExecutions), right? (Which is actually a good thing anyways.)
server/service/osquery.go
Outdated
| } else if hostWithoutPolicies { | ||
| // RecordPolicyQueryExecutions called with results=nil will still update the host's policy_updated_at column. | ||
| if err := svc.task.RecordPolicyQueryExecutions(ctx, host, nil, svc.clock.Now(), ac.ServerSettings.DeferredSaveHost); err != nil { | ||
| if err := svc.task.RecordPolicyQueryExecutions(ctx, host, nil, svc.clock.Now(), ac.ServerSettings.DeferredSaveHost, nil); err != nil { |
There was a problem hiding this comment.
Why nil here instead of []byte{}?
Related issue: Resolves #42836
This is another hot path optimization.
Before
When a host submits policy results via
SubmitDistributedQueryResults, the system needed to determine which policies "flipped" (changed from passing to failing or vice versa). Each consumer computed this independently:Each
FlippingPoliciesForHostcall runsSELECT policy_id, passes FROM policy_membership WHERE host_id = ? AND policy_id IN (?). All 5 queries hit the same table for the same host beforepolicy_membershipis updated, so they all see identical state.Each consumer also built intermediate maps to narrow down to its subset before calling
FlippingPoliciesForHost, then converted the result into yet another set for filtering. This meant 3-4 temporary maps per consumer.After
The intermediate subset maps and per-consumer set conversions are removed. Each process function goes directly from "policies with associated automation" to "is this policy in newFailingSet?" in a single map lookup.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit