Prevent silent corruption of software title icons#44540
Open
Conversation
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
![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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the software title icon storage path to prevent the DB (software_title_icons.storage_id) and the icon bytes store from silently diverging, which can lead to missing/incorrect icons during GitOps runs.
Changes:
- Read in-use
storage_ids from the MySQL writer inCleanupUnusedSoftwareTitleIconsto avoid replica lag causing deletion of newly-referenced icon bytes. - Make filesystem icon
Putatomic via temp-file + fsync + rename, avoiding truncated final files on mid-write failure. - Make filesystem icon
Existsvalidate integrity (reject 0-byte and SHA-256 mismatches), and add unit + integration test coverage for corruption/recovery paths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/datastore/mysql/software_title_icons.go | Uses writer connection for storage-id enumeration to avoid replica-lag deletion. |
| server/datastore/filesystem/software_title_icons.go | Implements atomic writes and integrity-checking Exists for filesystem-backed icon store. |
| server/datastore/filesystem/software_title_icons_test.go | Adds unit tests for corruption detection and atomic write behavior. |
| cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go | Adds GitOps integration coverage for “bytes missing but DB hash unchanged” recovery workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+86
to
+92
| } | ||
| if err := tmp.Close(); err != nil { | ||
| return ctxerr.Wrap(ctx, err, "closing software title icon file in filesystem store") | ||
| } | ||
| if err := os.Rename(tmpPath, finalPath); err != nil { | ||
| return ctxerr.Wrap(ctx, err, "renaming software title icon file in filesystem store") | ||
| } |
Contributor
WalkthroughThis pull request modifies software title icon storage and retrieval to improve reliability. The filesystem-based icon store's 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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
🧹 Nitpick comments (1)
cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go (1)
3050-3052: ⚡ Quick winStrengthen the recovery assertion to compare bytes, not just size.
A wrong file with the same byte length would still pass currently. Comparing content (or hash) makes the regression guard precise.
Suggested enhancement
info, err = os.Stat(iconPath) require.NoError(t, err) require.Equal(t, originalSize, info.Size()) + + expectedIconBytes, err := os.ReadFile(filepath.Join(dirPath, "testdata", "gitops", "lib", "icon.png")) + require.NoError(t, err) + gotIconBytes, err := os.ReadFile(iconPath) + require.NoError(t, err) + require.Equal(t, expectedIconBytes, gotIconBytes)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go` around lines 3050 - 3052, The test currently only compares file size (variables info, iconPath, originalSize) after recovery; instead, read the recovered file bytes and compare them to the original bytes (or compute and compare a hash) to ensure content equality. Replace the require.Equal(t, originalSize, info.Size()) check with code that opens iconPath, reads its contents, and asserts equality against the previously saved original byte slice (or compares their SHA256 hashes) using require.Equal or require.EqualValues to guarantee the recovered file content matches exactly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/datastore/filesystem/software_title_icons.go`:
- Around line 112-115: In the Exists function, treat a post-Stat ENOENT from
os.Open as "not present" instead of an error: when os.Open(path) returns an
error, check os.IsNotExist(err) (or errors.Is(err, os.ErrNotExist)) and return
(false, nil) in that case; otherwise wrap and return the error as currently
done. Also ensure any opened file (f) is closed on the success path to avoid
leaks. Use the function name Exists and the variables path, f, err, os.Open, and
ctxerr.Wrap to locate and update the logic.
---
Nitpick comments:
In `@cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go`:
- Around line 3050-3052: The test currently only compares file size (variables
info, iconPath, originalSize) after recovery; instead, read the recovered file
bytes and compare them to the original bytes (or compute and compare a hash) to
ensure content equality. Replace the require.Equal(t, originalSize, info.Size())
check with code that opens iconPath, reads its contents, and asserts equality
against the previously saved original byte slice (or compares their SHA256
hashes) using require.Equal or require.EqualValues to guarantee the recovered
file content matches exactly.
🪄 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
Run ID: a6bc3a1c-5cf3-48c4-b03e-94e55099f6fb
📒 Files selected for processing (4)
cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.goserver/datastore/filesystem/software_title_icons.goserver/datastore/filesystem/software_title_icons_test.goserver/datastore/mysql/software_title_icons.go
Comment on lines
+112
to
+115
| f, err := os.Open(path) | ||
| if err != nil { | ||
| return false, ctxerr.Wrap(ctx, err, "opening software title icon for hash verification") | ||
| } |
Contributor
There was a problem hiding this comment.
Handle post-Stat ENOENT in Exists as not-present, not error.
If cleanup/remove happens after Line 102 but before Line 112, Exists returns an error instead of false, which can surface transient failures in callers that expect corruption/missing bytes to fall through to re-upload.
Suggested fix
f, err := os.Open(path)
if err != nil {
+ if os.IsNotExist(err) {
+ return false, nil
+ }
return false, ctxerr.Wrap(ctx, err, "opening software title icon for hash verification")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/datastore/filesystem/software_title_icons.go` around lines 112 - 115,
In the Exists function, treat a post-Stat ENOENT from os.Open as "not present"
instead of an error: when os.Open(path) returns an error, check
os.IsNotExist(err) (or errors.Is(err, os.ErrNotExist)) and return (false, nil)
in that case; otherwise wrap and return the error as currently done. Also ensure
any opened file (f) is closed on the success path to avoid leaks. Use the
function name Exists and the variables path, f, err, os.Open, and ctxerr.Wrap to
locate and update the logic.
Member
Author
There was a problem hiding this comment.
I'm not sure this "callers that expect corruption/missing bytes to fall through to re-upload" is a thing, but will check.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44540 +/- ##
==========================================
+ Coverage 66.76% 66.78% +0.01%
==========================================
Files 2636 2637 +1
Lines 211834 212130 +296
Branches 9388 9388
==========================================
+ Hits 141437 141672 +235
- Misses 57552 57590 +38
- Partials 12845 12868 +23
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:
|
Member
Author
|
@claude review once |
There was a problem hiding this comment.
LGTM — atomic temp+rename, hash-checked Exists, and writer-side cleanup read are the right fixes for the corruption/divergence paths and are well-covered by the new unit + integration tests.
Extended reasoning...
Overview
The PR makes three small, focused changes to prevent silent divergence between software_title_icons rows and on-disk bytes:
server/datastore/filesystem/software_title_icons.go:Putnow writes to a temp file in the same directory, fsyncs, closes, and atomically renames into place;Existsrejects zero-byte files and verifies the file's SHA-256 matchesiconID.server/datastore/mysql/software_title_icons.go:CleanupUnusedSoftwareTitleIconsnow selects from the writer rather than a replica, so a stale replica missing a freshly-insertedstorage_idwon't cause its in-use file to be deleted.server/datastore/filesystem/software_title_icons_test.go+cmd/fleetctl/integrationtest/gitops/...: new unit tests coverExistsrejection (zero-byte, hash mismatch, intact) andPutatomicity (mid-write failure leaves no final file); a new integration test pins down the upload → corrupt → recover flow.
Security risks
None of concern. The hash verification in Exists actually strengthens integrity guarantees by ensuring on-disk bytes match the content-addressed ID before the store reports them as present. The temp filename prefix (.tmp-icon-*) is created via os.CreateTemp in the icons directory, so there's no path-traversal or symlink-attack surface beyond what the existing store already had.
Level of scrutiny
Moderate — this touches a data-integrity boundary (file storage + DB cleanup), but the changes are individually small, follow well-established patterns (write-temp-then-rename is textbook), and the tests are appropriately targeted. The MySQL change is a one-line writer/reader swap with a clear correctness argument.
Other factors
The reviewer concerns from Copilot and CodeRabbit are minor edge cases:
- The Windows
os.Renameconcern is largely moot — modern Go usesMoveFileExWwithMOVEFILE_REPLACE_EXISTINGfor files, so overwriting works on Windows. - The post-
StatENOENT race inExistsis a narrow window (only Cleanup removes files) and the caller inee/server/service/software_title_icons.gowould just propagate an error that the user would retry; not load-bearing for this PR's correctness.
The CI failure (TestIntegrationsVulnerabilityDataStream) is unrelated — it's a network test that timed out fetching Canonical OVAL definitions, with no connection to the icon-store changes.
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
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.
Fixes #43161
Summary
Three preventive changes against divergence between
software_title_iconsrows and bytes in the icon store:CleanupUnusedSoftwareTitleIconsreads the writer, not a replica.Putis atomic (temp > fsync > rename).Existsrejects 0-byte and hash-mismatched files.Testing
Existsrejection andPutatomicitySummary by CodeRabbit