Fix software ingestion lock convoys and unbatched deletes#49894
Fix software ingestion lock convoys and unbatched deletes#49894sharon-fdm wants to merge 21 commits intomainfleetdm/fleet:mainfrom fix-software-ingestion-lock-convoyfleetdm/fleet:fix-software-ingestion-lock-convoyCopy head branch name to clipboard
Conversation
…9805) Two performance fixes for software ingestion at scale: 1. Batch host_software_installed_paths DELETEs at 500 (#49805): Previously a single DELETE with 30k+ IDs would lock the table and never finish. Now batched at 500, matching the existing INSERT batching pattern. 2. Eliminate software_titles INSERT IGNORE lock convoys (#48719): Moved title INSERT IGNORE outside the main transaction so each is auto-committed independently (microsecond lock duration). Added singleflight to deduplicate concurrent INSERTs for the same title, and an in-process cache (sync.Map) so subsequent ingestions skip the INSERT entirely.
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
Improves MySQL software ingestion behavior under high concurrency and large data volumes by reducing lock contention in software_titles inserts and batching large deletes from host_software_installed_paths. Adds integration-style tests intended to reproduce the reported lock convoy and delete explosion scenarios.
Changes:
- Batch
host_software_installed_pathsdeletes into 500-id chunks to avoid hugeIN (...)statements. - Move
software_titlesINSERT IGNOREoperations out of the main ingestion transaction and deduplicate them withsingleflight+ an in-process cache. - Add new MySQL datastore tests for the lock convoy and delete explosion reproductions.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| server/datastore/mysql/software.go | Batches deletes; refactors pre-insert logic to insert titles outside the main transaction and adds cache/singleflight deduplication. |
| server/datastore/mysql/software_lock_convoy_test.go | Adds new tests attempting to reproduce the title insert lock convoy and large-delete behavior. |
| server/datastore/mysql/mysql.go | Adds datastore fields for singleflight and an in-process cache of known title keys. |
Files excluded by content exclusion policy (1)
- changes/48719-49805-software-ingestion-perf
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMySQL software ingestion now caches normalized software-title keys, deduplicates concurrent title inserts with singleflight, performs missing-title inserts outside the main retryable transaction, and resolves title IDs afterward. Deletions from 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/datastore/mysql/software.go (1)
1146-1163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the
uniqueTitlesconstruction shared with Lines 1086-1102.This block rebuilds
uniqueTitlesfromnewTitlesNeededidentically to the insert phase above. Keeping two copies of the keying logic in sync is fragile: if thetitleKeyderivation diverges between the two, the set of titles inserted won't match the set resolved by theSELECT, silently dropping title-ID resolution. Extract a single helper (e.g.buildUniqueTitles(newTitlesNeeded)) and call it in both places.🤖 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/datastore/mysql/software.go` around lines 1146 - 1163, Extract the duplicated uniqueTitles keying logic from the insert and SELECT phases into a shared helper such as buildUniqueTitles, using the existing titleKey derivation and deduplication behavior. Replace both constructions, including the block near the SELECT query, with calls to this helper so both phases always resolve identical title sets.server/datastore/mysql/software_lock_convoy_test.go (1)
176-185: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBatch-boundary coverage gap:
softwareCountexactly equals the 500-item batch size.
deleteHostSoftwareInstalledPathsbatches at 500 (seeserver/datastore/mysql/software.go); with exactly 500 items the delete loop runs a single iteration, so this reproduction test never exercises the multi-batch path (i += batchSizelooping more than once) that is the actual subject of#49805. Bumping the count above 500 (e.g., 1200) would exercise batch boundaries without materially slowing the test.Also applies to: 233-240
🤖 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/datastore/mysql/software_lock_convoy_test.go` around lines 176 - 185, The softwareCount fixture in the convoy test only exercises one delete batch because it equals the 500-item batch size. Increase softwareCount above 500, such as 1200, in the affected setup blocks so deleteHostSoftwareInstalledPaths executes multiple iterations and covers batch boundaries.
🤖 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/software_lock_convoy_test.go`:
- Around line 255-289: Update the concurrent worker in the software replacement
test to return errors from test.NewHost and ExecAdhocSQL instead of invoking
assertions inside the goroutine. Collect each worker’s error using the existing
error-group pattern from TestSoftwareTitlesInsertIgnoreLockConvoy, then perform
require.NoError assertions on the main test goroutine after all concurrent work
completes.
In `@server/datastore/mysql/software.go`:
- Around line 1109-1133: The knownSoftwareTitleKeys cache must not retain
entries after their corresponding software_titles rows are deleted. Update the
title deletion flow to invalidate the matching cache key, or defer storing keys
until insertion and subsequent selection succeeds; ensure report insertion can
recreate deleted titles with a valid title_id.
---
Nitpick comments:
In `@server/datastore/mysql/software_lock_convoy_test.go`:
- Around line 176-185: The softwareCount fixture in the convoy test only
exercises one delete batch because it equals the 500-item batch size. Increase
softwareCount above 500, such as 1200, in the affected setup blocks so
deleteHostSoftwareInstalledPaths executes multiple iterations and covers batch
boundaries.
In `@server/datastore/mysql/software.go`:
- Around line 1146-1163: Extract the duplicated uniqueTitles keying logic from
the insert and SELECT phases into a shared helper such as buildUniqueTitles,
using the existing titleKey derivation and deduplication behavior. Replace both
constructions, including the block near the SELECT query, with calls to this
helper so both phases always resolve identical title sets.
🪄 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 Plus
Run ID: 594616da-2614-4e05-913f-2ca72ce2eea4
📒 Files selected for processing (4)
changes/48719-49805-software-ingestion-perfserver/datastore/mysql/mysql.goserver/datastore/mysql/software.goserver/datastore/mysql/software_lock_convoy_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #49894 +/- ##
==========================================
- Coverage 67.81% 65.48% -2.34%
==========================================
Files 3890 3892 +2
Lines 247635 249100 +1465
Branches 13019 13019
==========================================
- Hits 167943 163114 -4829
- Misses 64529 70517 +5988
- Partials 15163 15469 +306
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:
|
CleanupSoftwareTitles uses early return when no more orphans are found, which skipped the cache invalidation code. Changed to break so we always reach the cache clear logic when titles were deleted.
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
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/48719-49805-software-ingestion-perf
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
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
Files excluded by content exclusion policy (1)
- changes/48719-49805-software-ingestion-perf
Comments suppressed due to low confidence (1)
server/datastore/mysql/software_lock_convoy_test.go:133
- Same data race as above: sharedSoftware is reused across goroutines, but UpdateHostSoftware mutates the slice in-place. Copy the slice per goroutine before calling UpdateHostSoftware.
g2.Go(func() error {
<-ready2
start := time.Now()
_, err := ds.UpdateHostSoftware(ctx, hostID, sharedSoftware)
elapsed := time.Since(start)
ms := elapsed.Milliseconds()
…tines - Fix gofmt alignment in const block - Copy shared software slices before passing to goroutines to avoid data races from UpdateHostSoftware's in-place mutation - Create test hosts synchronously to avoid require.* panics from goroutines
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
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/48719-49805-software-ingestion-perf
Comments suppressed due to low confidence (1)
server/datastore/mysql/software_lock_convoy_test.go:27
- The header comment still describes the current expected behavior as lock-convoy serialization/high contention, but this PR’s goal is to reduce/avoid that convoy. As written, it reads like a passing test indicates contention is expected rather than improved; consider updating to clarify that correctness is asserted and timing is informational (and should be improved vs. historical behavior).
// Expected: With the current code, concurrent INSERT IGNORE statements on the same
// unique-index rows serialize and cause high contention. This test measures timing
// to confirm the convoy is observable even at modest concurrency.
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
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
Files excluded by content exclusion policy (1)
- changes/48719-49805-software-ingestion-perf
The hard error breaks because withRetryTxx retries the transaction but the title INSERT is outside the transaction and won't re-run. The retry loops forever hitting the same missing title. Revert to the previous behavior: log the error and continue with NULL title_id. The cache eviction (self-healing) still ensures the title is re-created on the next ingestion cycle.
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
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/48719-49805-software-ingestion-perf
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
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/48719-49805-software-ingestion-perf
Comments suppressed due to low confidence (2)
server/datastore/mysql/software.go:1428
- This comment says we "return an error to abort this attempt" to avoid inserting software rows with NULL title_id, but the code below explicitly logs and continues (and then proceeds to insert with titleID=nil). Update the comment to match the actual behavior so future readers don't assume the ingestion aborts here.
// When title IDs are missing, a concurrent CleanupSoftwareTitles likely
// deleted the titles we just inserted (they were orphaned briefly outside the
// transaction). Clear those cache entries so they are re-inserted on the next
// agent check-in and return an error to abort this attempt without inserting
// software rows with NULL title_id.
server/datastore/mysql/software_lock_convoy_test.go:27
- The header comment still describes the pre-fix expectation (that INSERT IGNOREs serialize and a convoy is observable). After this PR, the expected behavior is that the convoy is mitigated; this test mainly verifies correctness and logs timings for comparison. Updating the comment will prevent confusion about what a passing run indicates.
// Expected: With the current code, concurrent INSERT IGNORE statements on the same
// unique-index rows serialize and cause high contention. This test measures timing
// to confirm the convoy is observable even at modest concurrency.
| strings.ToLower(normalizeForCollation(name)), | ||
| source, | ||
| extensionFor, | ||
| bundleID, |
There was a problem hiding this comment.
Fixed in a03b390. Added strings.ToLower(bundleID) in the cache key to match MySQL's case-insensitive collation and the EqualFold comparison in the matching logic. Resolved.
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
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (1)
- changes/48719-49805-software-ingestion-perf
| // When title IDs are missing, a concurrent CleanupSoftwareTitles likely | ||
| // deleted the titles we just inserted (they were orphaned briefly outside the | ||
| // transaction). Clear those cache entries so they are re-inserted on the next | ||
| // agent check-in and return an error to abort this attempt without inserting | ||
| // software rows with NULL title_id. |
Related issue: Resolves #49805, Resolves #48719
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/oree/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.Timeouts are implemented and retries are limited to avoid infinite loops
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Context
A customer (~2,500 hosts, v4.89.1) had their DB writer slammed with
DELETE FROM host_software_installed_pathsstatements carrying 30,000+ IDs each. These never completed, required repeated manual intervention, and the table grew from 14.5M to 14.8M rows in 2 days. This is #49805.While investigating, Victor linked #48719, a related
software_titlesINSERT lock convoy issue seen in load tests. Both are in the same software ingestion code path (server/datastore/mysql/software.go), so this PR fixes both.Root cause
#49805: Unbatched DELETEs on
host_software_installed_pathsWhen a host's software changes, Fleet computes a delta and deletes stale rows from
host_software_installed_paths. The functiondeleteHostSoftwareInstalledPaths()issued a singleDELETE FROM host_software_installed_paths WHERE id IN (?)with all IDs expanded bysqlx.In(). With 30,000+ IDs and 14.8M rows in the table, these massive statements held row locks for minutes, timed out, and never completed. On the next agent check-in, the same (or larger) DELETE was retried, creating a feedback loop where the table grew unboundedly.Notably, the INSERT function for the same table (
insertHostSoftwareInstalledPaths) already batched at 500 rows. The DELETE simply lacked the same treatment.#48719: INSERT IGNORE lock convoys on
software_titles(related)When a host reports software that Fleet hasn't seen before,
preInsertSoftwareInventory()runsINSERT IGNORE INTO software_titles (...)inside awithRetryTxxtransaction. For homogeneous fleets (many hosts sharing the same software catalog, typical for imaged corporate Windows machines), hundreds of concurrent goroutines try to INSERT IGNORE the same title rows simultaneously.Even though
INSERT IGNOREis a no-op when the row already exists, InnoDB still acquires row/gap locks on the unique index for the duration of the enclosing transaction. With many goroutines holding or waiting on the same index locks, the DB enters a "lock convoy" where sessions serialize on locks they don't actually need. In load tests (40 Fleet instances, 100K hosts, 141 identical Windows software items), this produced 690 average active sessions on the writer and 85s fleet-wide p99.The existing read-first check (
getIncomingSoftwareChecksumsToExistingTitles) prevents the convoy at steady state. But on cold start (emptysoftware_titles, e.g. after cleanup purges orphaned titles), the check finds nothing and all goroutines race to INSERT the same titles.How I reproduced it
Started MySQL via
docker compose up -d mysql_test, created a git worktree.#49805
TestHostSoftwareInstalledPathsDeleteExplosion: Created a host with 500 software items and installed paths, then replaced all software with an entirely new set. This triggersdeleteHostSoftwareInstalledPaths()with all 500 old IDs in a single unbatched DELETE statement. At 500 IDs the local test completes quickly, but the structure confirms the problem: at 30K+ IDs on production Aurora with 14M rows, these never finish.#48719
TestSoftwareTitlesInsertIgnoreLockConvoy: Created 50 hosts, each reporting 100 identical software items (simulating a homogeneous fleet). Used a barrier to release all 50 goroutines simultaneously, then measured two phases:software_titles): All 50 hosts concurrently callds.UpdateHostSoftware().Before fix:
The 79x slowdown confirms the lock convoy.
How I fixed it
#49805: Batch the DELETE at 500
Changed
deleteHostSoftwareInstalledPaths()from a singleDELETE ... WHERE id IN (all IDs)to a loop that processes 500 IDs per batch, matching the existing INSERT batching pattern in the same file.#48719: Three-layer defense against lock convoys
Layer 1 - Move title INSERT IGNORE outside the transaction. Previously,
INSERT IGNORE INTO software_titlesran insidewithRetryTxx, so locks were held for the full transaction duration. Now each title INSERT is executed viads.writer(ctx).ExecContext()outside any transaction, auto-committing independently and holding locks for microseconds.Layer 2 - singleflight per title key. Added a
singleflight.Groupon theDatastorestruct. For each title, only one goroutine actually executes the INSERT; concurrent goroutines wait and share the result.Layer 3 - In-process cache (
sync.Map). After a title is inserted, its key is stored inknownSoftwareTitleKeys. Subsequent ingestions check the cache first and skip the INSERT entirely.CleanupSoftwareTitlesclears the cache when it deletes orphaned titles.The three layers work together: the cache handles the common case (title already known), singleflight handles the cold-start race (only one INSERT per title), and auto-commit ensures even the winning INSERT holds locks for microseconds.
How I tested that it works
New reproduction tests
TestSoftwareTitlesInsertIgnoreLockConvoy: 50 concurrent hosts, 100 identical software items. Measures cold-start convoy factor and verifies all 100 titles are created.TestHostSoftwareInstalledPathsDeleteExplosion: Full software replacement path with 500 items per host, including concurrent hosts.Existing test suite
Ran all existing software tests including:
UpdateHostSoftware,UpdateHostSoftwareDeadlock,PreInsertSoftwareInventorySoftwareTitleUpgradeCodeDriftMatch,UpdateHostSoftwareSameBundleIDDifferentNamesCleanupSoftwareTitles(validates cache invalidation works correctly)SaveHost,SyncHostsSoftware, and ~80 other subtestsAll pass.
After-fix measurements
The remaining cold-start time is from other pipeline operations (
INSERT IGNORE INTO software, host_software linking), not fromsoftware_titles.Summary by CodeRabbit
Performance Improvements
Bug Fixes
Tests