Fixing issue with OSV artifact cleanup on date boundaries#43408
Fixing issue with OSV artifact cleanup on date boundaries#43408
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
WalkthroughThe changes refine how the OSV downloader categorizes synchronization results. The 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Pull request overview
Fixes an OSV artifact cleanup edge case at date boundaries by ensuring Ubuntu versions that are missing from the current release aren’t treated as “up to date” (which could trigger deletion of the last-known-good local artifact).
Changes:
- Introduce
SyncResult.NotInReleaseto distinguish “missing in GitHub release” from checksum-based “Skipped”. - Update sync logic to classify missing assets as
NotInReleaseinstead ofSkipped. - Update/extend tests around sync fault tolerance and date-boundary cleanup behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/vulnerabilities/osv/sync_test.go | Adds a new date-boundary test and updates expected sync result fields/error behavior. |
| server/vulnerabilities/osv/downloader.go | Extends SyncResult and changes missing-release-asset handling to populate NotInRelease. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 2404 is in NotInRelease, not Skipped | ||
| // so removeOldOSVArtifacts should not touch it | ||
| err := removeOldOSVArtifacts(today, tmpDir, []string{}) | ||
| require.NoError(t, err) | ||
|
|
||
| // Yesterday's artifact must still exist since the version wasn't in the successful set | ||
| _, err = os.Stat(filepath.Join(tmpDir, "osv-ubuntu-2404-2026-04-09.json.gz")) | ||
| require.NoError(t, err, "old artifact must be preserved when version is not in release") |
There was a problem hiding this comment.
This test doesn’t actually exercise the date-boundary scenario in the sync flow: it calls removeOldOSVArtifacts with an empty successfulVersions slice directly, so it would pass even if SyncOSV still classified missing release assets as “Skipped” (the original bug). Consider constructing a ReleaseInfo that lacks today’s asset, calling syncOSVWithDownloader for version 2404, building upToDateVersions from result.Downloaded+result.Skipped (as Refresh does), and then calling removeOldOSVArtifacts to assert yesterday’s artifact is preserved.
| // 2404 is in NotInRelease, not Skipped | |
| // so removeOldOSVArtifacts should not touch it | |
| err := removeOldOSVArtifacts(today, tmpDir, []string{}) | |
| require.NoError(t, err) | |
| // Yesterday's artifact must still exist since the version wasn't in the successful set | |
| _, err = os.Stat(filepath.Join(tmpDir, "osv-ubuntu-2404-2026-04-09.json.gz")) | |
| require.NoError(t, err, "old artifact must be preserved when version is not in release") | |
| release := ReleaseInfo{ | |
| Assets: []ReleaseAsset{ | |
| { | |
| Name: "osv-ubuntu-2204-2026-04-10.json.gz", | |
| BrowserDownloadURL: "https://example.com/osv-ubuntu-2204-2026-04-10.json.gz", | |
| }, | |
| }, | |
| } | |
| downloader := func(_ context.Context, _, _ string) error { | |
| return errors.New("downloader should not be called for a version missing from the release") | |
| } | |
| result, err := syncOSVWithDownloader(context.Background(), today, tmpDir, release, []string{"2404"}, downloader) | |
| require.NoError(t, err) | |
| require.Empty(t, result.Downloaded, "2404 should not be downloaded when today's asset is missing from the release") | |
| require.Empty(t, result.Skipped, "2404 should not be marked skipped when today's asset is missing from the release") | |
| require.Equal(t, []string{"2404"}, result.NotInRelease, "2404 should be classified as not in release") | |
| upToDateVersions := append([]string{}, result.Downloaded...) | |
| upToDateVersions = append(upToDateVersions, result.Skipped...) | |
| err = removeOldOSVArtifacts(today, tmpDir, upToDateVersions) | |
| require.NoError(t, err) | |
| // Yesterday's artifact must still exist since the version was not treated as up-to-date. | |
| _, err = os.Stat(filepath.Join(tmpDir, "osv-ubuntu-2404-2026-04-09.json.gz")) | |
| require.NoError(t, err, "old artifact must be preserved when today's release asset is missing") |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #43408 +/- ##
=======================================
Coverage 66.88% 66.88%
=======================================
Files 2590 2590
Lines 207650 207651 +1
Branches 9202 9202
=======================================
+ Hits 138892 138893 +1
- Misses 56122 56123 +1
+ Partials 12636 12635 -1
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:
|
| if !ok { | ||
| // Artifact not available, skip | ||
| result.Skipped = append(result.Skipped, ubuntuVersion) | ||
| result.NotInRelease = append(result.NotInRelease, ubuntuVersion) |
There was a problem hiding this comment.
Making the distinction between File must exist locally AND checksum must match vs No file in the release that matches this criteria. The cleanup job needs to treat these differently.
mostlikelee
left a comment
mostlikelee
left a comment
There was a problem hiding this comment.
Nice, i think this also addresses an issue where local artifacts are deleted if there's an outage feeds can't be generated for a day.
Unreleased bug fix for #42063 **Related issue:** Resolves #39900 ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually For unreleased bug fixes in a release candidate, one of: - [x] Confirmed that the fix is not expected to adversely impact load test results - [x] Alerted the release DRI if additional load testing is needed We shouldn't need any additional load testing. This change will not have a large impact on load.
Unreleased bug fix for #42063
Related issue: Resolves #39900
https://fleet-cm.sentry.io/issues/7397434783/?alert_rule_id=14022333&alert_timestamp=1775694137225&alert_type=email¬ification_uuid=980228f0-4f9c-4ed1-8a88-9646b887b927&project=4504912025550848&referrer=alert_email
missing FleetError in chain: loading OSV artifact: finding OSV artifact for Ubuntu 2404: no OSV artifact found for Ubuntu 2404Testing
For unreleased bug fixes in a release candidate, one of:
We shouldn't need any additional load testing. This change will not have a large impact on load.