Fix label update consistency issue

[juan-fdz-hawa]

Persist label metadata and membership changes together in a single transaction so a failed update can't leave a partial change behind.

Checklist for submitter

If some of the following don't apply, delete the relevant line.

• Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
  See Changes files for more information.

• Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.

Testing

• Added/updated automated tests
• QA'd all new/changed functionality manually

Summary by CodeRabbit

• Bug Fixes
  • Updated label editing so manual membership changes and label metadata updates are persisted in a single atomic save.
  • If a label update fails (for example, due to rename/name conflicts), membership changes are not partially applied.
  • Audit/label activity is now emitted only when the label save succeeds, avoiding mismatches after failed edits.

[juan-fdz-hawa]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[coderabbitai]

Walkthrough

Label saving now accepts optional host IDs and replaces manual membership within the same transaction as label metadata updates. Membership replacement logic is shared between manual membership updates and label saves. The service passes membership changes through SaveLabel, and tests cover rollback on conflicting renames, successful updates, host ID propagation, and suppression of activity records when saving fails.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and clearly describes the main change: fixing label update consistency.
Description check	✅ Passed	The description covers the main fix, checklist items, testing, and manual QA, matching the template well overall.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 16789-audit-logging-bypass-in-modify-label-via-partial-state-commit

---

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.}

[coderabbitai]

Actionable comments posted: 2

🤖 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/datastore/mysql/labels.go`:
- Around line 669-690: Move the label readback currently performed by labelDB in
SaveLabel into the withRetryTxx transaction callback, after the metadata and
membership updates, and return the readback error from the callback so it rolls
back the transaction. Preserve the existing returned label, host IDs, and error
behavior while ensuring the read uses the transaction executor rather than
running after commit.
- Around line 441-472: Update the batch size used by batchHostIds in the label
membership insertion flow to 32,767 or fewer hosts, ensuring each batch’s two
parameters per host stays within MySQL’s 65,535 bind-parameter limit. Preserve
the existing team validation and INSERT IGNORE behavior.

🪄 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: 805fb8cd-434e-4d13-a44b-e72aaeb71b6c

📥 Commits

Reviewing files that changed from the base of the PR and between 128c455 and 4287eab4e9cb4c449fceae794c9de026da4d8184.

📒 Files selected for processing (7)

• changes/16789-modify-label-atomic-membership
• server/datastore/mysql/labels.go
• server/datastore/mysql/labels_test.go
• server/fleet/datastore.go
• server/mock/datastore_mock.go
• server/service/labels.go
• server/service/labels_test.go

[codecov]

Codecov Report

❌ Patch coverage is 75.51020% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.08%. Comparing base (363a56a) to head (7c5321e).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
server/datastore/mysql/labels.go	75.00%	6 Missing and 6 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #49493   +/-   ##
=======================================
  Coverage   68.07%   68.08%           
=======================================
  Files        3882     3882           
  Lines      246392   246404   +12     
  Branches    13169    13169           
=======================================
+ Hits       167742   167756   +14     
+ Misses      63508    63506    -2     
  Partials    15142    15142           

Flag	Coverage Δ
backend	69.54% <75.51%> (+<0.01%)	⬆️

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.

[coderabbitai]

🧹 Nitpick comments (1)

server/service/labels_test.go (1)

1198-1221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reset the mock invocation flag between subtests.

The mock datastore ds is shared across subtests. The first subtest ("using hostnames") mutates ds.SaveLabelFuncInvoked to true when the mock is called. Because this flag is not reset, the second subtest ("using IDs") will falsely pass its require.True(t, ds.SaveLabelFuncInvoked) assertion even if the ModifyLabel function fails to actually invoke the datastore save method.

Resetting ds.SaveLabelFuncInvoked = false at the start of each subtest ensures the assertion strictly checks the current run.

♻️ Proposed fix to reset mock state

 	t.Run("using hostnames", func(t *testing.T) {
+		ds.SaveLabelFuncInvoked = false
 		ds.SaveLabelFunc = func(ctx context.Context, lbl *fleet.Label, hostIDs []uint, filter fleet.TeamFilter) (*fleet.LabelWithTeamName, []uint, error) {
 			require.Equal(t, uint(1), lbl.ID)
 			require.Equal(t, []uint{99, 100}, hostIDs)
 			return &fleet.LabelWithTeamName{Label: *lbl}, hostIDs, nil
 		}
 		_, _, err := svc.ModifyLabel(ctx, 1, fleet.ModifyLabelPayload{
 			Hosts: []string{"host1", "host2"},
 		})
 		require.NoError(t, err)
 		require.True(t, ds.SaveLabelFuncInvoked)
 	})
 
 	t.Run("using IDs", func(t *testing.T) {
+		ds.SaveLabelFuncInvoked = false
 		ds.SaveLabelFunc = func(ctx context.Context, lbl *fleet.Label, hostIDs []uint, filter fleet.TeamFilter) (*fleet.LabelWithTeamName, []uint, error) {
 			require.Equal(t, uint(1), lbl.ID)

🤖 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/service/labels_test.go` around lines 1198 - 1221, Reset
ds.SaveLabelFuncInvoked to false at the start of both the “using hostnames” and
“using IDs” subtests before calling ModifyLabel, so each require.True assertion
reflects only that subtest’s SaveLabel invocation.

🤖 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/service/labels_test.go`:
- Around line 1198-1221: Reset ds.SaveLabelFuncInvoked to false at the start of
both the “using hostnames” and “using IDs” subtests before calling ModifyLabel,
so each require.True assertion reflects only that subtest’s SaveLabel
invocation.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3e2f1860-f76f-4ebd-af9d-e60c4cd38523

📥 Commits

Reviewing files that changed from the base of the PR and between 62d40bd7c73a202b9ce197b70f7f3faad44d8ddc and 7c5321e.

📒 Files selected for processing (8)

• changes/16789-modify-label-atomic-membership
• server/datastore/mysql/labels.go
• server/datastore/mysql/labels_test.go
• server/fleet/datastore.go
• server/mock/datastore_mock.go
• server/service/integration_core_test.go
• server/service/labels.go
• server/service/labels_test.go

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

• changes/16789-modify-label-atomic-membership
• server/fleet/datastore.go
• server/mock/datastore_mock.go
• server/datastore/mysql/labels.go
• server/service/labels.go