Reconcile incoming Windows software upgrade_codes with those in existing correlated software_titles - #36175
Conversation
GitHub Actions
Error
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds a lightweight SoftwareTitleSummary type and a reconciliation routine that updates existing software_titles' upgrade_code when incoming software (matched by checksum) supplies a non-empty upgrade_code; the reconciliation is invoked in Phase 1 of applyChangesForNewSoftwareDB before pre-insert and will halt Phase 1 on error. Tests exercise update/skip/conflict/NULL scenarios. Changes
Sequence DiagramsequenceDiagram
participant Apply as applyChangesForNewSoftwareDB
participant Recon as reconcileExistingTitleEmptyUpgradeCodes
participant DB as MySQL
Apply->>Recon: Call with incomingSoftwareByChecksum\nand incomingChecksumsToExistingTitleSummaries
Recon->>DB: Query existing titles for checksums
Recon->>Recon: Build list of title pairs\n(oldUpgradePtr, newUpgradePtr)
alt incoming upgrade_code non-empty & existing empty/NULL
Recon->>DB: Retry-wrapped UPDATE upgrade_code
DB-->>Recon: OK
else incoming upgrade_code empty
Recon-->>Apply: Skip update (no-op)
else existing non-empty different from incoming
Recon-->>Recon: Log warning, skip
end
Recon-->>Apply: Return success or error (errors halt Phase 1)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (2)📚 Learning: 2025-07-08T16:13:39.114ZApplied to files:
📚 Learning: 2025-09-14T21:42:42.400ZApplied to files:
🧬 Code graph analysis (1)server/datastore/mysql/software.go (1)
🔇 Additional comments (2)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 2
🧹 Nitpick comments (1)
server/datastore/mysql/software_upgrade_code_test.go (1)
26-27: Consider verifying expected log counts.The test defines
expectedInfoLogsandexpectedWarningLogsfields (lines 26-27) and sets them in test cases, but never verifies that the correct number of logs are actually emitted during reconciliation. While the core functionality is tested, validating logging behavior could help catch regressions.If you'd like to verify logs, you could capture log output during the test and assert the counts match expectations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/datastore/mysql/software.go(2 hunks)server/datastore/mysql/software_upgrade_code_test.go(1 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_upgrade_code_test.goserver/datastore/mysql/software.go
🧠 Learnings (2)
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 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_upgrade_code_test.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 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_upgrade_code_test.go
🔇 Additional comments (1)
server/datastore/mysql/software.go (1)
423-427: LGTM! Appropriate placement of upgrade_code reconciliation.The reconciliation is correctly placed before the pre-insertion step in Phase 1, ensuring upgrade_code consistency before software inventory entries are created. Error handling properly propagates failures to halt the Phase 1 process.
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #36175 +/- ##
=======================================
Coverage 65.97% 65.98%
=======================================
Files 2130 2130
Lines 181411 181465 +54
Branches 7564 7564
=======================================
+ Hits 119694 119732 +38
- Misses 50767 50775 +8
- Partials 10950 10958 +8
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:
|
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
I'm coming in on the tail end of this, but did the software table get modified as part of this, including checksum?
| switch { | ||
| case existingTitle.UpgradeCode == nil: | ||
| level.Warn(ds.logger).Log( | ||
| "msg", "Encountered Windows software title with a NULL upgrade_code, which shouldn't be possible. Writing the incoming non-empty upgrade code to the title.", |
There was a problem hiding this comment.
Is this not allowed to be null because we set it to '' for everything with the source programs in a DB migration?
Feels like we're going to have some customer migrate while the system is up and have cases of these spamming logs anyway. Which means that it at least feels to me like if we want to have blank UpgradeCodes on Windows software we coalesce nil to blank at the application layer and the DB is always either null or filled. Guessing there's a reason you have the blank value in the DB though?
There was a problem hiding this comment.
Not allowed to be null because we set it to '' for everything with the source programs in a DB migration?
Right
coalesce nil to blank at the application layer and the DB is always either null or filled. Guessing there's a reason you have the blank value in the DB though?
I'm not sure I'm fully understanding, but yes the idea was that the migration initially sets NULL,
"", or "<upgrade_code>", and any subsequent ingestion of software writes with the same
conditions, so this would be an unexpected state and useful to warn
There was a problem hiding this comment.
I guess my question is why have the empty state at all, as distinct from NULL.
There was a problem hiding this comment.
I may have to defer to another reviewer here as it feels like I'm missing context here.
There was a problem hiding this comment.
You're right that it's slightly redundant semantically: upgrade_code: "" could also be communicated as upgrade_code: NULL + source: "programs"
There was a problem hiding this comment.
...argh, I could've sworn I wrote this comment on the correct thread. Deleted the erroneous comment and readding in the right place.
There was a problem hiding this comment.
why have the empty state at all, as distinct from NULL.
The thinking also has to do with the fact the spec explicitly wants API responses to have upgrade_code a) omitted for non-Windows software, b) included and empty for Windows software without an upgrade_code, or c) included with the code when it's there
To help keep things clear, I basically implemented these 3 options throughout the stack. I can see that it might make sense to make the db column non-nullable and then make the necessary updates at the application layer based on source + ugprade_code
| existingTitlesToUpgradeCodesToWrite[existingTitle.ID] = *sw.UpgradeCode | ||
| case existingTitle.UpgradeCode != nil && *existingTitle.UpgradeCode != "" && *sw.UpgradeCode != *existingTitle.UpgradeCode: | ||
| // don't update this title | ||
| level.Warn(ds.logger).Log( |
There was a problem hiding this comment.
If there's a different upgrade code here, seems like this should be a brand new title, not an overwrite to the existing one (or a bail), as we're saying that upgrade codes are a dimension of uniqueness.
There was a problem hiding this comment.
Yea there is a bit of ambiguity/catch22 going on around this scenario. AFAIK it's only theoretical
at this point and this is the direction the team agreed on, so unless you urgently disagree I'd say
this is something we keep in mind and address later if/as needed
There was a problem hiding this comment.
Understood. Will defer remaining review here to @mostlikelee or the like as I wasn't part of those discussions so am missing context. Happy to review the FMA bits by contrast as I know what's going on there but I'm not being particularly useful here.
@iansltx yep fleet/server/fleet/software.go Lines 175 to 179 in b366ed2 |
GitHub Actions
Inactive
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
Thanks for the changes! Since I don't have software ingestion in my head at this point I don't feel like I'm in a position to vet previous decisions, so will defer to folks who were part of those earlier discussions.
83f6738 to
f65ae20
Compare
GitHub Actions
Error
GitHub Actions
Inactive
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| for _, title := range existingSoftwareTitlesForNewSoftwareWithoutBundleIdentifier { | ||
| checksums, ok := uniqueTitleStrToChecksums[UniqueSoftwareTitleStr(title.Name, title.Source, title.ExtensionFor)] | ||
| for _, titleSummary := range existingTitleSummariesForNewSoftwareWithoutBundleIdentifier { | ||
| // TODO - need to factor upgrade_code in here? |
There was a problem hiding this comment.
No, because you will update the upgrade code, right?
Do you have a test where software comes in with the same name/source/extensionFor but different upgrade codes? What is supposed to happen? We only save one of the upgrade codes?
There was a problem hiding this comment.
All of the tests in software_upgrade_code_test.go handle the situation where current title's
source and name and incoming software's source and name (and extension for, since not specified) are
the same, but upgrade_code differs
There was a problem hiding this comment.
I do recommend adding a test for 2 software with the same (name, source, extension_for) but different upgrade_code values
Is that what you're referring to, or a different case?
| } | ||
|
|
||
| var retrievedTitleSummaries []fleet.SoftwareTitleSummary | ||
| // TODO - include UpgradeCode in the below WHERE (these args) for additional specificity? |
There was a problem hiding this comment.
Same as previous TODO comment. We overwrite the upgrade code if 2 titles have the same everything else, right? Which means this WHERE should only return the 1 title?
There was a problem hiding this comment.
UPDATE: I understand we don't overwrite, but both software items should get associated to the same title in the DB.
| // For others, match by name | ||
| nameMatches := td.Name == title.Name | ||
| nameMatches := titleSummary.Name == title.Name | ||
| // TODO - similarly match if UpgradeCodes match? |
There was a problem hiding this comment.
Only if we allow same software names to have different upgrade codes. Which I don't think we intend to right now, right?
|
|
||
| // SoftwareTitleSummary contains a lightweight subset of the fields of a SoftwareTitle that are | ||
| // useful for processing incoming software | ||
| // TODO - embed this in `SoftwareTitle` to reduce redundancy |
There was a problem hiding this comment.
Nit. I recommend against leaving TODOs in code. If you really want to remember this, file an issue and put the link to the issue here without the TODO.
There was a problem hiding this comment.
Will keep this in mind going forward, thanks!
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Thanks for digging into this, Jacob.
I don't see any issues with this PR.
But I do recommend adding a test for 2 software with the same (name, source, extension_for) but different upgrade_code values, to make sure nothing is completely broken. This can be done in a separate PR.
…sting correlated `software_title`s (#36175) <!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #35724 This PR addresses Issue 3 in the above unreleased bug: when a new `software` comes in, its `upgrade_code` is compared with that of any corresponding `software_title`s, and reconciled appropriately - see code for the various cases ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually - [x] Load tests: @AndreyKizimenko since I don't think you've load tested the original story yet, this change will be covered by those tests when you do <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Ensured upgrade_code values on existing software titles are consistently updated when incoming data provides non-empty values; added logging and safe retry handling for anomalous states. * **Tests** * Added table-driven tests covering upgrade_code reconciliation scenarios (empty, non-empty, conflicting, NULL) and verification of resulting title values. * **Refactor** * Introduced a lightweight software title summary type and updated internal mappings to streamline pre-insert and reconciliation processing. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Related issue: Resolves #35724
This PR addresses Issue 3 in the above unreleased bug: when a new
softwarecomes in, itsupgrade_codeis compared with that of any correspondingsoftware_titles, and reconciled appropriately - see code for the various casesTesting
Summary by CodeRabbit
Bug Fixes
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.