Atomic vulnerability count calculations #35317
Open
+1,117
−47
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mostlikelee
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #35317 +/- ##
==========================================
+ Coverage 66.12% 66.14% +0.01%
==========================================
Files 2075 2084 +9
Lines 175818 176465 +647
Branches 7198 7198
==========================================
+ Hits 116268 116714 +446
- Misses 48871 49014 +143
- Partials 10679 10737 +58
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:
|
mostlikelee
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
WalkthroughThis PR fixes vulnerabilities disappearing from Fleet during the vulnerability update job by replacing the unsafe in-place count zeroing with an atomic table-swap pattern. It also introduces performance testing tools—a vulnerability data seeder and performance tester—for validating the fix. Changes
Sequence DiagramsequenceDiagram
participant Job as Vulnerability Job
participant DS as Datastore
participant SwapTbl as Swap Table
participant MainTbl as Main Counts Table
Note over Job,MainTbl: Atomic Table Swap Pattern (New)
Job->>DS: UpdateVulnerabilityHostCounts()
DS->>SwapTbl: Ensure swap table exists & clear
DS->>DS: Aggregate counts (global, team, no-team)
DS->>SwapTbl: Batch insert new counts into swap table
rect rgb(150, 200, 150)
Note over DS,MainTbl: Atomic operation
DS->>MainTbl: RENAME TABLE swap→main, main→old
DS->>MainTbl: DROP old table
end
DS->>SwapTbl: Recreate empty swap table
Job->>Job: Continue (counts always visible, even if job interrupted)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Areas requiring extra attention:
Suggested reviewers
Pre-merge checks and finishing touches❌ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
changes/35043-missing-vuln-counts(1 hunks)server/datastore/mysql/vulnerabilities.go(2 hunks)tools/software/vulnerabilities/performance_test/README.md(1 hunks)tools/software/vulnerabilities/performance_test/seeder/volume_vuln_seeder.go(1 hunks)tools/software/vulnerabilities/performance_test/tester/performance_tester.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:
tools/software/vulnerabilities/performance_test/tester/performance_tester.goserver/datastore/mysql/vulnerabilities.gotools/software/vulnerabilities/performance_test/seeder/volume_vuln_seeder.go
🧠 Learnings (2)
📚 Learning: 2025-08-13T18:20:42.136Z
Learnt from: titanous
Repo: fleetdm/fleet PR: 31075
File: tools/redis-tests/elasticache/iam_auth.go:4-10
Timestamp: 2025-08-13T18:20:42.136Z
Learning: For test harnesses and CLI tools in the Fleet codebase, resource cleanup on error paths (like closing connections before log.Fatalf) may not be necessary since the OS handles cleanup when the process exits. These tools prioritize simplicity over defensive programming patterns used in production code.
Applied to files:
tools/software/vulnerabilities/performance_test/README.mdtools/software/vulnerabilities/performance_test/tester/performance_tester.gotools/software/vulnerabilities/performance_test/seeder/volume_vuln_seeder.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:
tools/software/vulnerabilities/performance_test/tester/performance_tester.go
🔇 Additional comments (1)
tools/software/vulnerabilities/performance_test/seeder/volume_vuln_seeder.go (1)
374-384: Fix compile-time range error.
for i := range softwareCountdoes not compile—softwareCountis anint, not a slice/map/string/channel. This prevents the seeder from building or running.- for i := range softwareCount { + for i := 0; i < softwareCount; i++ {⛔ Skipped due to learnings
Learnt from: getvictor Repo: fleetdm/fleet PR: 33218 File: orbit/pkg/table/santa/ringbuffer_test.go:48-50 Timestamp: 2025-10-07T19:42:55.988Z Learning: Go 1.22 introduced the range-over-int feature, allowing `for i := range N` syntax to iterate from 0 to N-1. This is valid and idiomatic Go code in projects using Go 1.22 or later.
mostlikelee
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
mostlikelee
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #35043
The meat of this issue is an atomic table swap when aggregating host counts for vulnerabilities. This approach turned out to be much more performant to the existing approach as well as just moving the inserts into a single transaction.
To ensure no performance regressions, new data seeder and performance tooling was created to seed data based on customer environments.
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)Testing
Added/updated automated tests (unable to replicate issue in automated tests)
QA'd all new/changed functionality manually
Summary by CodeRabbit
Bug Fixes
Chores