Skip to content

feat: cleanup old binary versions after update#3255

Open
kristinemejia443-netizen wants to merge 1 commit into
tari-project:mainfrom
kristinemejia443-netizen:claude/cleanup-old-binaries-3028
Open

feat: cleanup old binary versions after update#3255
kristinemejia443-netizen wants to merge 1 commit into
tari-project:mainfrom
kristinemejia443-netizen:claude/cleanup-old-binaries-3028

Conversation

@kristinemejia443-netizen
Copy link
Copy Markdown

Summary

  • After a successful binary download, delete all previously downloaded version directories from the binary folder, keeping only the current version
  • Cleanup also runs automatically on startup for each binary that already has its current version installed
  • Errors during cleanup are logged but do not prevent normal operation

Changes

  • binaries_manager.rs: Added cleanup_old_versions() method that iterates the binary folder and removes all version subdirectories except the current one. Called after each successful download.
  • binaries_resolver.rs: Added cleanup_old_versions() that calls cleanup on all managed binaries. Wired into initialize_binary() so cleanup runs on startup even when no download is needed.

Acceptance Criteria (from #3028)

  • After a successful update, all previously downloaded binary versions are deleted from the download directory
  • The current (latest, just-installed) binary is always retained
  • Cleanup runs automatically on startup or after update completion (no user action required)

Closes #3028

🤖 Generated with Claude Code

After a successful binary download, delete all previously downloaded
version directories from the binary folder, keeping only the current
version. Cleanup also runs on startup for each binary that already has
its current version installed.

Closes tari-project#3028

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a cleanup mechanism for old binary versions within the BinaryManager and BinaryResolver. While the logic correctly identifies and removes unused directories, the feedback highlights several critical improvements: error handling should be 'best-effort' to prevent cleanup failures from interrupting the main download process or batch operations, and synchronous file system calls should be replaced with asynchronous ones to avoid blocking the Tokio runtime.

Comment thread src-tauri/src/binaries/binaries_manager.rs
Comment on lines +300 to +302
for (_binary, manager) in &self.managers {
manager.cleanup_old_versions().await?;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The cleanup process should be "best-effort" for all managed binaries. Using ? inside the loop causes the entire operation to stop if a single binary manager encounters an error during cleanup, preventing subsequent binaries from being processed. This contradicts the goal of not letting cleanup errors prevent normal operation.

        for (binary, manager) in &self.managers {
            let _unused = manager.cleanup_old_versions().await.inspect_err(|e| {
                log::warn!(target: LOG_TARGET_APP_LOGIC, "Failed to cleanup old versions for {}: {e}", binary.name());
            });
        }
References
  1. It is acceptable to implement workarounds to prevent errors in fragile downstream components, even if a more robust fix in the component itself is possible.

Comment on lines +463 to +476
let entries = std::fs::read_dir(&binary_folder)?;
for entry in entries.flatten() {
let path = entry.path();
if path.is_dir() {
if let Some(dir_name) = path.file_name().and_then(|n| n.to_str()) {
if dir_name != self.selected_version {
info!(target: LOG_TARGET_APP_LOGIC, "Cleaning up old binary version: {path:?}");
if let Err(e) = std::fs::remove_dir_all(&path) {
warn!(target: LOG_TARGET_APP_LOGIC, "Failed to remove old version directory {path:?}: {e}");
}
}
}
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This method uses synchronous I/O (std::fs::read_dir and std::fs::remove_dir_all) inside an async fn. Since cleanup involves iterating over directories and potentially performing recursive deletions of large binary folders, this can block the Tokio worker thread for a significant amount of time. This can lead to performance degradation or UI unresponsiveness in a Tauri application. Consider using tokio::fs for non-blocking file operations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cleanup old binary files

2 participants