feat: cleanup old binary versions after download/extraction#3278
feat: cleanup old binary versions after download/extraction#3278Able-ui666 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to clean up old binary versions, keeping only the retained versions to save disk space. It adds cleanup logic in BinaryManager and integrates it into BinaryResolver to run after verifying existing files or completing a download, while ensuring shared directories preserve versions used by other managers. The review feedback highlights a security vulnerability where entry.metadata() follows symlinks (which could lead to symlink traversal attacks), a logic bug where Vec::dedup is incorrectly used on non-consecutive duplicates, and an opportunity to simplify error handling by refactoring the cleanup helper method.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Use metadata to check if it's a directory (not following symlinks) | ||
| let metadata = match entry.metadata() { | ||
| Ok(m) => m, | ||
| Err(e) => { | ||
| warn!( | ||
| target: LOG_TARGET_APP_LOGIC, | ||
| "Could not get metadata for {} during cleanup of {}: {}", | ||
| path.display(), | ||
| binary_name, | ||
| e | ||
| ); | ||
| continue; | ||
| } | ||
| }; | ||
|
|
||
| // Skip if not a directory or if it's a symbolic link | ||
| if !metadata.is_dir() || metadata.file_type().is_symlink() { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The safety documentation states that this method avoids following symbolic links to prevent symlink traversal attacks. However, entry.metadata() actually traverses symbolic links to query information about the target. If a symlink points to a directory, metadata.is_dir() will return true and metadata.file_type().is_symlink() will return false, which bypasses the symlink check and could lead to deleting the contents of the target directory.
To fix this securely and efficiently, use entry.file_type() instead. It does not traverse symlinks and is faster because the file type is typically returned directly by the directory traversal API without an extra system call.
| // Use metadata to check if it's a directory (not following symlinks) | |
| let metadata = match entry.metadata() { | |
| Ok(m) => m, | |
| Err(e) => { | |
| warn!( | |
| target: LOG_TARGET_APP_LOGIC, | |
| "Could not get metadata for {} during cleanup of {}: {}", | |
| path.display(), | |
| binary_name, | |
| e | |
| ); | |
| continue; | |
| } | |
| }; | |
| // Skip if not a directory or if it's a symbolic link | |
| if !metadata.is_dir() || metadata.file_type().is_symlink() { | |
| continue; | |
| } | |
| // Use file_type to check if it's a directory without following symlinks | |
| let file_type = match entry.file_type() { | |
| Ok(ft) => ft, | |
| Err(e) => { | |
| warn!( | |
| target: LOG_TARGET_APP_LOGIC, | |
| "Could not get file type for {} during cleanup of {}: {}", | |
| path.display(), | |
| binary_name, | |
| e | |
| ); | |
| continue; | |
| } | |
| }; | |
| // Skip if not a directory (this also correctly filters out symlinks) | |
| if !file_type.is_dir() { | |
| continue; | |
| } |
| // Remove duplicates while preserving order | ||
| retained.dedup(); | ||
| retained |
There was a problem hiding this comment.
Vec::dedup only removes consecutive duplicate elements. If the list of retained versions contains duplicates that are not adjacent (e.g., ["v1.0.0", "v2.0.0", "v1.0.0"]), dedup will fail to remove them.
To correctly remove all duplicates while preserving the original order, use a HashSet to track seen elements during filtering.
// Remove duplicates while preserving order
let mut seen = std::collections::HashSet::new();
retained.retain(|x| seen.insert(x.clone()));
retained| async fn cleanup_old_binary_versions_for_manager( | ||
| &self, | ||
| binary: Binaries, | ||
| ) -> Result<(), Error> { | ||
| let manager = self | ||
| .managers | ||
| .get(&binary) | ||
| .ok_or_else(|| anyhow!("Couldn't find manager for binary: {}", binary.name()))?; | ||
|
|
||
| let binary_folder = manager.get_binary_folder()?; | ||
| let retained_versions = self.retained_versions_for_binary_folder(&binary_folder); | ||
|
|
||
| manager | ||
| .cleanup_old_binary_versions(retained_versions) | ||
| .await?; | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
All three call sites of cleanup_old_binary_versions_for_manager handle errors in the exact same way by logging a warning and continuing. We can simplify the call sites and remove duplicated boilerplate by handling the errors and logging the warnings directly inside this helper method, changing its return type to ().
async fn cleanup_old_binary_versions_for_manager(&self, binary: Binaries) {
let manager = match self.managers.get(&binary) {
Some(m) => m,
None => {
warn!(
target: LOG_TARGET_APP_LOGIC,
"Couldn't find manager for binary: {}",
binary.name()
);
return;
}
};
let binary_folder = match manager.get_binary_folder() {
Ok(folder) => folder,
Err(e) => {
warn!(
target: LOG_TARGET_APP_LOGIC,
"Failed to get binary folder for {}: {}",
binary.name(),
e
);
return;
}
};
let retained_versions = self.retained_versions_for_binary_folder(&binary_folder);
if let Err(e) = manager.cleanup_old_binary_versions(retained_versions).await {
warn!(
target: LOG_TARGET_APP_LOGIC,
"Failed to clean up old binary versions for {}: {}",
binary.name(),
e
);
}
}|
/opire try/opire try |
9c4b420 to
4aebd6e
Compare
Implements automatic cleanup of old binary version directories to prevent accumulation of unused binaries over time. Changes: - Add cleanup_old_binary_versions() to BinaryManager - Add cleanup trigger points in BinaryResolver::initialize_binary() - Add version folder name validation - Add symlink protection - Handle shared directories (tari-suite) - Add comprehensive unit tests (5 tests) Safety features: - Only deletes directories matching version naming pattern - Skips symbolic links - Non-fatal: cleanup failures don't block initialization Fixes tari-project#3028
4aebd6e to
50a9507
Compare
Description
Implements automatic cleanup of old binary version directories to prevent accumulation of unused binaries over time, as requested in #3028.
Changes
cleanup_old_binary_versions()toBinaryManagerBinaryResolver::initialize_binary()(after version check, post-lock check, and post-download)Safety Features
v1.0.0,1.2.3-alpha, etc.)Testing
Related Issue
Fixes #3028Implements automatic cleanup of old binary version directories to prevent accumulation of unused binaries over time.
Changes:
Safety features:
Fixes #3028
Description
Motivation and Context
How Has This Been Tested?
What process can a PR reviewer use to test or verify this change?
Breaking Changes