Clean up old binary versions after download#3252
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a cleanup mechanism for old binary versions within the binaries_manager.rs file. It adds a new function, cleanup_old_binary_versions, which iterates through the binary directory and removes any subdirectories that do not match the current version, accompanied by relevant unit tests. The reviewer suggested improving the robustness of this cleanup logic by ensuring the process continues even if an individual directory removal fails, rather than exiting early on the first error.
| for entry in std::fs::read_dir(binary_folder)? { | ||
| let entry = entry?; | ||
| let path = entry.path(); | ||
|
|
||
| if !path.is_dir() { | ||
| continue; | ||
| } | ||
|
|
||
| if entry.file_name().to_string_lossy() == current_version { | ||
| continue; | ||
| } | ||
|
|
||
| std::fs::remove_dir_all(&path)?; | ||
| info!( | ||
| target: LOG_TARGET_APP_LOGIC, | ||
| "Removed old binary version folder: {}", | ||
| path.display() | ||
| ); | ||
| removed_paths.push(path); | ||
| } |
There was a problem hiding this comment.
The current implementation of cleanup_old_binary_versions stops immediately if it fails to read a directory entry or remove a specific folder (due to the ? operator). This means that if one old version directory is locked or inaccessible (common on Windows), other potentially removable old versions will be left behind. It is more robust to log the error for the failing entry and continue the loop to ensure as much cleanup as possible is performed.
for entry in std::fs::read_dir(binary_folder)? {
let Ok(entry) = entry else { continue };
let path = entry.path();
if !path.is_dir() || entry.file_name().to_string_lossy() == current_version {
continue;
}
match std::fs::remove_dir_all(&path) {
Ok(_) => {
info!(
target: LOG_TARGET_APP_LOGIC,
"Removed old binary version folder: {}",
path.display()
);
removed_paths.push(path);
}
Err(error) => {
warn!(
target: LOG_TARGET_APP_LOGIC,
"Failed to remove old binary version folder {}: {error:?}",
path.display()
);
}
}
}There was a problem hiding this comment.
Addressed in 1dbce200: remove_dir_all failures are now logged and skipped so cleanup continues with later old version folders. Entry and file-type read failures were already handled the same way.
git diff --check passes locally; Rust tests still cannot run in this Windows environment because cargo is not installed on PATH.
9d15d76 to
d5e1fe2
Compare
d5e1fe2 to
1dbce20
Compare
Verification on Windows: - git diff --check origin/main...HEAD Rust tests were not rerun because cargo is not installed on PATH in this environment.
Summary
Fixes #3028 by cleaning up stale downloaded binary version folders after a binary version is resolved.
This change:
Verification
git diff --checkI could not run
cargo fmt --checkorcargo test cleanup_old_binary_versions --libin this Windows environment becausecargo/rustcare not installed on PATH here.