[rajendrachavan]

Related issue: Resolves #52263.

Summary

A scheduled query with no query_labels rows targets every host. NewQuery and SaveQuery committed the queries row first and wrote the labels in a second transaction, so a label-scoped query was briefly visible to every host polling /api/osquery/config, and their results were stored in the query report (diagnosed by @xpkoala in the issue).

This change writes the query row and its labels inside one withRetryTxx in both NewQuery and SaveQuery. The updateQueryLabels wrapper is removed; callers now use the existing updateQueryLabelsInTx with the outer transaction.

deleteQueryStats and deleteQueryResults intentionally stay outside the transaction, per @lucasmrod's TODO from #14710.

Testing

New datastore test TestQueries/QueryLabelsAtomic (server/datastore/mysql/queries_test.go):

• NewQuery with a label that doesn't exist must not leave a query row behind (fails on main at this assertion: an unscoped query row survives and is served to an out-of-scope host).
• A valid scoped query is served only to hosts in the label.
• SaveQuery with a failed label write rolls back the column updates and keeps the previous labels.

Ran locally: MYSQL_TEST=1 go test ./server/datastore/mysql/ -run TestQueries -count=1 (all 17 subtests pass), go vet, golangci-lint (no findings in changed files). HTTP-level integration tests (server/service/integration_*) were not run locally because they hard-code Redis at 127.0.0.1:6379 and flush keys; relying on CI for those.

Manual QA on fleet serve --dev --dev_license: enrolled two hosts via /api/osquery/enroll, put each in its own manual label, created a query with labels_include_any = label A, then edited it to label B, then to A+B. After each step /api/osquery/config for each host returned the query only when that host was in scope:

Scope	in-label host	out-of-label host
create, A	served	not served
edit → B	not served	served
edit → A+B	served	served

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 (withRetryTxx bounds retries).

Testing

• Added/updated automated tests
• Where appropriate, automated tests simulate multiple hosts and test for host isolation (in-label vs. out-of-label host)
• QA'd all new/changed functionality manually

Summary by CodeRabbit

Bug Fixes

• Fixed a race condition affecting label-scoped queries.
• Ensured query creation and updates save query details and labels atomically.
• Prevented incomplete or unscoped queries from being delivered.
• Ensured label-scoped queries are delivered only to matching hosts.
• Prevented failed updates from removing existing labels.
• Automatically rolled back changes when label persistence fails, preserving consistent query configuration.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: d5db401e-0357-4a18-a134-0e1c2faed304

📥 Commits

Reviewing files that changed from the base of the PR and between f26eeb4 and c6e1399.

📒 Files selected for processing (3)

• changes/49502-atomic-query-labels
• server/datastore/mysql/queries.go
• server/datastore/mysql/queries_test.go

🚧 Files skipped from review as they are similar to previous changes (3)

• changes/49502-atomic-query-labels
• server/datastore/mysql/queries.go
• server/datastore/mysql/queries_test.go

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

Walkthrough

Query creation and editing now persist query fields and labels in the same retryable transaction. New queries initialize packs after the transaction succeeds. Tests verify rollback when label writes fail, delivery only to matching hosts, and preservation of existing query data and labels after failed edits.

Merge Risk: ⚪ Minimal · up to c6e13

This localized change makes query and label updates atomic, preventing temporarily unscoped queries; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: writing queries and labels in one transaction.
Description check	✅ Passed	The description identifies issue #52263, explains the race and failure modes, documents the implementation, and records automated and manual testing. The relevant checklist items are complete.
Linked Issues check	✅ Passed	The changes meet issue #52263 by making NewQuery and SaveQuery write query data and labels atomically, preventing transient unscoped queries and rolling back failed writes.
Out of Scope Changes check	✅ Passed	The changes are limited to the datastore transaction logic, related tests, and the documented changes file. The changes support the linked issue objectives and include no unrelated code changes.

Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 87.30159% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.79%. Comparing base (f26eeb4) to head (c6e1399).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
server/datastore/mysql/queries.go	87.30%	4 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #51845      +/-   ##
==========================================
- Coverage   69.81%   69.79%   -0.02%     
==========================================
  Files        4071     4071              
  Lines      265492   265493       +1     
  Branches    14067    14067              
==========================================
- Hits       185341   185294      -47     
- Misses      63962    63995      +33     
- Partials    16189    16204      +15     

Flag	Coverage Δ
backend	70.25% <87.30%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

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 pull request fixes a race where label-scoped scheduled queries could be briefly visible to all hosts because the queries row and its query_labels rows were previously written in separate transactions. It makes query creation and updates atomic so out-of-scope hosts don’t ever observe an unscoped “global” scheduled query during host config polling.

Changes:

• Wrap NewQuery insert + label writes in a single withRetryTxx transaction.
• Wrap SaveQuery update + label writes in a single withRetryTxx transaction.
• Add a datastore regression test ensuring failed label writes roll back both NewQuery and SaveQuery changes.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File	Description
server/datastore/mysql/queries.go	Makes NewQuery and SaveQuery write the query row and query_labels atomically via a single transaction.
server/datastore/mysql/queries_test.go	Adds QueryLabelsAtomic regression test covering rollback behavior and host scoping.
changes/49502-atomic-query-labels	Release note entry (content excluded from review by policy).

Files excluded by content exclusion policy (1)

• changes/49502-atomic-query-labels

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[lucasmrod]

@rajendrachavan Hi! Thanks for the contribution. It seems this PR doesn't actually solve the linked bug report #49502 (I closed the issue because it was caused by some invalid caching, which is now fixed in main).

This PR actually solve a different issue described in Reed's tests: a very small window of time where reports temporarily a being scheduled on hosts that are not member of the labels.

I've created #52263 and updated the PR description.

[lucasmrod]

LGTM! Pending a merge conflict to allow merging.

[coderabbitai]

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.

[lucasmrod]

@rajendrachavan To save you some time I pushed the conflict resolution. Once CI passes I'll merge.

[rajendrachavan]

Thanks @lucasmrod for digging into the original report, filing #52263, sorting the conflict, and merging. Appreciate you saving me the round trip. Happy to pick up more bugs in this area.