Fix label membership being cleared when a label query errors - #49403
Conversation
A label query that errors (e.g. extension socket unavailable) was being recorded the same as a query that ran fine and returned zero rows, causing existing label membership to be incorrectly removed. Query errors now leave existing membership untouched.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughLabel query errors are now represented as unknown results rather than non-matches. Asynchronous recording skips unknown results and avoids empty Redis updates. MySQL recording also skips unknown matches, preserving existing 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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 fixes dynamic label evaluation so that when a label query returns an error status (instead of a definitive “0 rows”), Fleet records the result as unknown and does not clear existing label membership—preventing unintended removal of label-scoped automation (resolves #46399).
Changes:
- Treat errored label query executions as
nil/unknown and skip membership deletes/updates for those labels (MySQL + async Redis path). - Update async Redis script/argument building to avoid writing “delete” entries for errored label results.
- Add/extend tests to validate that query errors preserve existing label membership.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/osquery_test.go | Adds coverage ensuring query error status is recorded as nil label result (unknown). |
| server/service/async/async_label.go | Skips nil label results when writing async label membership ops to Redis; guards ZADD when args are empty. |
| server/service/async/async_label_test.go | Updates expectations so nil results are skipped (not treated as deletes). |
| server/datastore/mysql/labels.go | Skips updates/deletes when label result is nil (unknown/error), preserving membership. |
| server/datastore/mysql/labels_test.go | Adds a regression test ensuring nil results keep existing membership (but currently has compile errors: invalid new("1") / new(true)). |
| changes/46399-query-errors-can-cause-label-membership-unassigned.md | Release note (content excluded by policy; not reviewed). |
Files excluded by content exclusion policy (1)
- changes/46399-query-errors-can-cause-label-membership-unassigned.md
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #49403 +/- ##
==========================================
- Coverage 68.15% 67.80% -0.35%
==========================================
Files 3849 3881 +32
Lines 243458 247434 +3976
Branches 13110 13110
==========================================
+ Hits 165934 167782 +1848
- Misses 62526 64487 +1961
- Partials 14998 15165 +167
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:
|
The test used a nil result as shorthand for "remove label membership," relying on the exact behavior fixed in the previous commit. Use an explicit false (non-match) instead, since nil now means "query errored, leave membership unchanged."
Related issue: Resolves #46399
When a label's query errors on a host (e.g. the extension socket is unavailable) instead of returning zero rows, Fleet was recording that error the same as a definitive "no match," clearing the host's existing label membership. This could unintentionally remove configuration profiles or other automations scoped to that label. The fix leaves existing label membership untouched when a label query errors.
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Setup: macOS VM enrolled as a Fleet host, with a dynamic label whose query targets a real, always-present table but with a deliberately invalid
WHEREclause, so the query fails deterministically (ano such columnSQL error).Before (bug reproduced on unpatched code)
After (fix verified)
Summary by CodeRabbit