Re-adding and optimizing macos software name renaming on ingestion - #33094
Re-adding and optimizing macos software name renaming on ingestion#33094ksykulev wants to merge 7 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #33094 +/- ##
========================================
Coverage 63.92% 63.92%
========================================
Files 2052 2050 -2
Lines 202440 202725 +285
Branches 6627 6555 -72
========================================
+ Hits 129416 129601 +185
- Misses 62832 62912 +80
- Partials 10192 10212 +20
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:
|
|
|
||
| // First, we remove incoming software that already exists in the software table. | ||
| if len(softwareChecksums) > 0 { | ||
| if len(softwareChecksums) > 0 || len(existingBundleIDsToUpdate) > 0 { |
There was a problem hiding this comment.
Is adding || len(existingBundleIDsToUpdate) > 0 ok?
existingSoftware is from getSoftwareIDsByChecksums which gets existing software ids for checksums. existingSoftware should only contain software that corresponds to the incoming checksums (softwareChecksums). However, the new renaming logic changed how macos software gets added to softwareChecksums and existingSoftware:
for bid := range existingBundleIDsToUpdate {
if cs, ok := bundleIDsToChecksum[bid]; ok {
// we don't want this to be treated as a new software title, because then a new software
// entry will be created. Instead, we want to update the existing entries with the new
// names.
delete(incomingChecksumToSoftware, cs)
}
}
This means that software gets removed from softwareChecksums but still exists in existingSoftware. Any macos software that is present in existingBundleIDsToUpdate will always trigger the !ok case. We don't have to worry about this software accidentally getting added to insertedSoftware, even if len(softwareChecksums) == 0.
GitHub Actions
Failure
| strings.Join(updateIDs, ","), | ||
| ) | ||
|
|
||
| if _, err := tx.ExecContext(ctx, updateStmt, updateArgs...); err != nil { |
There was a problem hiding this comment.
IIRC, one of the core issues last time was that this updating logic was executing inside an already very busy transaction. This lead to deadlocks, which lead to the DB issues.
I think we should do this update in a separate transaction.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds a batch-oriented update path to rename existing macOS software entries on ingest using bundle identifiers and checksums. Modifies insert logic to return inserted and updated software sets, then applies targeted name updates post-insert. Updates tests to validate both insertion and rename flows. Adds a changelog entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Host as Host
participant Svc as Service Layer
participant DS as Datastore (MySQL)
participant DB as MySQL
Host->>Svc: Ingest software inventory
Svc->>DS: applyChangesForNewSoftwareDB(hostID, inventory)
DS->>DB: insertNewInstalledHostSoftwareDB(..., checksums, ...)
DB-->>DS: inserted[], updated[] (renamed-by-checksum)
DS->>DB: updateTargetedBundleIDs(hostID, inserted[], updated[], existingBundleIDsToUpdate)
note over DS,DB: Batched CASE UPDATE on software names<br/>Batched INSERT to host_software for last_opened_at
DB-->>DS: OK
DS-->>Svc: Completion
Svc-->>Host: Ingest complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/datastore/mysql/software.go (1)
391-409: MoveupdateTargetedBundleIDsoutside the transaction
The call on line 407 still lives insideds.withRetryTxx, which contradicts the PR goal of running renaming logic outside the TX. Extract this (and any related rename work) to after the transaction completes.
🧹 Nitpick comments (6)
changes/29053-sw-names-on-ingest (1)
1-1: Capitalize macOS and tighten wording; omit issue/PR refsExisting change notes don't include issue/PR references—mirror their capitalization and tone.
File: changes/29053-sw-names-on-ingest (line 1)
- Re-add and optimize macOS software ingestion to prevent duplicate entries when an end-user renames an app on the host.
server/datastore/mysql/software.go (2)
423-497: Consider handling update errors more gracefully.The
updateTargetedBundleIDsfunction performs batch updates but if any batch fails, the entire operation fails. Consider implementing partial failure handling or retry logic for individual batches to improve resilience.Consider wrapping the batch operations with error collection:
const batchSize = 100 +var updateErrors []error return common_mysql.BatchProcessSimple(softwareIDs, batchSize, func(batch []uint) error { // ... existing batch processing code ... if _, err := tx.ExecContext(ctx, updateStmt, updateArgs...); err != nil { - return ctxerr.Wrap(ctx, err, "batch update software names") + updateErr := ctxerr.Wrap(ctx, err, fmt.Sprintf("batch update software names for batch starting at ID %d", batch[0])) + updateErrors = append(updateErrors, updateErr) + // Continue processing other batches + return nil } // ... rest of the code ... }) + +if len(updateErrors) > 0 { + return fmt.Errorf("encountered %d errors during batch updates: %v", len(updateErrors), updateErrors[0]) +}
477-478: Hard-coded version string may cause maintenance issues.The hard-coded string
'bundle_4.67'forname_sourcecould make it difficult to track which version of the software performed the update. Consider making this configurable or version-aware.Consider using a constant or configuration value:
+const bundleNameSource = "bundle_4.67" // Update this when the logic changes + updateStmt := fmt.Sprintf( - `UPDATE software SET name = CASE id %s END, name_source = 'bundle_4.67' WHERE id IN (%s)`, + `UPDATE software SET name = CASE id %s END, name_source = '%s' WHERE id IN (%s)`, strings.Join(updateCases, " "), + bundleNameSource, strings.Join(updateIDs, ","), )server/datastore/mysql/software_test.go (3)
266-273: Defer tx.Rollback() and avoid repeated tx boilerplateIn tests, a panic/require failure before Commit can leak an open transaction. Add a defer tx.Rollback() right after Beginx(), and consider a tiny helper to DRY the tx pattern.
Apply this pattern to each Beginx():
tx, err := ds.writer(ctx).Beginx() require.NoError(t, err) +defer tx.Rollback()Also applies to: 296-301, 329-336, 392-399, 435-441, 483-489, 537-544
709-724: Add a negative control to prove rename is checksum‑scoped, not bundle‑wideIssue #29053 requires targeting by checksum to avoid over‑broad renames. Add a second existing software row with the same bundle identifier but a different checksum/version, then verify only the intended row is renamed.
Example insertion and assertion snippet (adapt as needed):
@@ // there's a new row for the new software, but existing software was renamed require.Len(t, software, 2) for _, s := range software { require.NotEmpty(t, s.TitleID) // software.name is updated now for GoLand if s.BundleIdentifier == "com.jetbrains.goland" { require.Equal(t, "GoLand 3.app", s.Name) } } +// Insert another software with same bundle id but different checksum/version +_, err = ds.writer(ctx).ExecContext(ctx, ` + INSERT INTO software (name, version, source, bundle_identifier, checksum) + VALUES ('GoLand Legacy.app', '2023.1', 'apps', 'com.jetbrains.goland', UNHEX(MD5('gl-legacy'))) +`) +require.NoError(t, err) +// Re-run rename logic against a different incoming name; ensure legacy row is unaffected +// (expect only the targeted checksum/name to change, not all rows sharing the bundle id)
811-813: Clarify variable names to match getExistingSoftware return orderThe identifiers (incomingByChecksum, existingSoftware, existingTitlesForNewSoftware) don’t match the semantic order used elsewhere (currentSoftware, incomingChecksumToSoftware, incomingChecksumToTitle). Rename for clarity to prevent future mixups.
- incomingByChecksum, existingSoftware, existingTitlesForNewSoftware, _, err := ds.getExistingSoftware( + currentSoftware, incomingChecksumToSoftware, incomingChecksumToTitle, _, err := ds.getExistingSoftware( context.Background(), make(map[string]fleet.Software), incoming, ) @@ - _, _, err = ds.insertNewInstalledHostSoftwareDB( - context.Background(), tx, host1.ID, incomingByChecksum, existingSoftware, existingTitlesForNewSoftware, map[string]fleet.Software{}, + _, _, err = ds.insertNewInstalledHostSoftwareDB( + context.Background(), tx, host1.ID, currentSoftware, incomingChecksumToSoftware, incomingChecksumToTitle, map[string]fleet.Software{}, )Also applies to: 841-843
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
changes/29053-sw-names-on-ingest(1 hunks)server/datastore/mysql/software.go(12 hunks)server/datastore/mysql/software_test.go(12 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/datastore/mysql/software_test.goserver/datastore/mysql/software.go
🧠 Learnings (2)
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
PR: fleetdm/fleet#31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: Fleet repo targets Go 1.24.5 (root go.mod), which supports testing.T.Context(). Do not flag usage of t.Context() or suggest replacing it with context.Background() in tests (e.g., server/datastore/mysql/labels_test.go Line 2031 and similar).
Applied to files:
server/datastore/mysql/software.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
PR: fleetdm/fleet#31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: In fleetdm/fleet repository tests (server/datastore/mysql/labels_test.go and similar), using testing.T.Context() is valid because the project targets a recent Go version where testing.T.Context() exists. Do not suggest replacing t.Context() with context.Background() in this codebase.
Applied to files:
server/datastore/mysql/software.go
🔇 Additional comments (10)
server/datastore/mysql/software.go (5)
811-811: Good addition for handling existing bundle IDs.The condition
|| len(existingBundleIDsToUpdate) > 0correctly ensures that software marked for renaming is processed even whensoftwareChecksumsis empty.
799-1001: Updated function signature properly handles both inserted and updated software.The changes to return both
insertedSoftwareandupdatedSoftwarecorrectly separate new entries from existing ones that need renaming, which aligns with the PR objectives.
820-821: Good handling of software marked for update.The logic correctly identifies software that needs renaming and stores it in
updatedSoftwarefor later processing.
456-496: Efficient batch processing implementation.The use of
common_mysql.BatchProcessSimplewith a batch size of 100 and the combination of CASE statements for updates is an efficient approach that should handle the performance requirements mentioned in the PR objectives.
486-493: Verify INSERT IGNORE behavior for host_software.Using
INSERT IGNOREforhost_softwarecould silently skip updates tolast_opened_atif a record already exists. Ensure this is the intended behavior.Let me check if there's an ON DUPLICATE KEY UPDATE clause that might be more appropriate:
server/datastore/mysql/software_test.go (5)
41-41: Add test case to the suite: LGTMGood coverage hook for the new ingestion/rename path.
245-572: New ingestion tests: solid scenarios; consider one more assertionThe cases exercise “new”, “update by bundle id”, mixed insert/update, Windows/Linux/Browser sources. Looks good.
As a follow‑up, consider asserting title_id presence in “Bundle id updates with empty softwareChecksums” (post insert) to mirror other tests’ guarantees.
645-656: Nice guard against duplicate host_software during renameValidates no extra host_software rows are created after updateTargetedBundleIDs. Good.
741-743: Mechanical return-shape updates: LGTMCall sites adjusted to the new (inserted, updated, err) signature.
Also applies to: 772-775
419-446: Drop argument-order verification
The test invocations already match theSoftwareFromOsqueryRowsignature (name, version, source, vendor, installedPath, release, arch, bundleIdentifier, extensionId, browser, lastOpenedAt), so no parameter reordering is needed.
| // checking inserted software | ||
| for _, software := range insertedSoftware { | ||
| if software.BundleIdentifier != "" { | ||
| if updatedSoftware, needsUpdate := bundleIDsToSoftware[software.BundleIdentifier]; needsUpdate { |
There was a problem hiding this comment.
Is the re-use of the updatedSoftware var name intentional here (and in the "check existing software" loop below)? Got me a bit turned around. Maybe updatedSoftwareID?
There was a problem hiding this comment.
I think my intention here was to highlight that this was a software that needs updating. Since bundleIDsToSoftware is a map[string]fleet.Software (aka bundle id => software).
However, I see the confusion here since we are also passing updatedSoftware into the func. Happy to change the name to something that makes sense.
|
Changes were added to #33399 |
3 participants
Fixes #29053
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)Testing
Summary by CodeRabbit