Skip to content

feat: cleanup old binary files after successful update#3263

Open
Ojas2095 wants to merge 2 commits into
tari-project:mainfrom
Ojas2095:fix-3028-cleanup-binaries
Open

feat: cleanup old binary files after successful update#3263
Ojas2095 wants to merge 2 commits into
tari-project:mainfrom
Ojas2095:fix-3028-cleanup-binaries

Conversation

@Ojas2095

Copy link
Copy Markdown

Summary

Closes #3028

This PR implements the automatic cleanup of old binary files after a successful update and on startup.

Implementation Details

  • Created a cleanup_old_versions() method in BinaryManager that iterates over the binary's root directory and removes all subdirectories that do not match the current selected_version.
  • Triggered cleanup_old_versions() from BinaryResolver::initialize_binary() whenever the binary files are verified as existing, or after a new download successfully completes.
  • It seamlessly integrates with the existing flow and does not disrupt the file locking mechanism for the Tari Suite.

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

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.

Code Review

This pull request introduces a cleanup mechanism (cleanup_old_versions) to BinaryManager to remove old binary version directories and retain only the currently selected version, which is triggered during binary initialization in BinaryResolver. Feedback on the implementation points out several issues in cleanup_old_versions, including synchronous blocking I/O calls on Tokio threads, silent error suppression during directory traversal, unnecessary path allocations, and potential failures during concurrent deletions. A non-blocking, more robust implementation is suggested to address these concerns.

Comment on lines +488 to +513
pub async fn cleanup_old_versions(&self) -> Result<(), Error> {
let binary_folder = self.adapter.get_binary_folder()?;
if !binary_folder.exists() {
return Ok(());
}

let current_version = &self.selected_version;

let mut entries = tokio::fs::read_dir(&binary_folder).await?;
while let Ok(Some(entry)) = entries.next_entry().await {
let path = entry.path();
if !path.is_dir() {
continue;
}
if let Some(folder_name) = path.file_name().and_then(|n| n.to_str()) {
if folder_name != current_version {
info!(target: LOG_TARGET_APP_LOGIC, "Cleaning up old binary version directory: {}", path.display());
if let Err(e) = tokio::fs::remove_dir_all(&path).await {
warn!(target: LOG_TARGET_APP_LOGIC, "Failed to remove old binary version directory {}: {}", path.display(), e);
}
}
}
}

Ok(())
}

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

Several issues exist in the cleanup_old_versions implementation:

  1. Synchronous I/O Blocking Tokio Threads: binary_folder.exists() and path.is_dir() are synchronous blocking calls. Calling them inside an async function blocks the Tokio executor thread, which can degrade application performance. Using tokio::fs::read_dir error handling and entry.file_type().await is non-blocking and much more efficient.
  2. Silent Error Suppression: The loop condition while let Ok(Some(entry)) = entries.next_entry().await will silently terminate if next_entry() returns an Err. It is safer to propagate the error using ?.
  3. Unnecessary Allocations: entry.path() allocates a new PathBuf on every iteration. We can use entry.file_name() to check the folder name first, and only allocate the full path via entry.path() when we actually need to delete the directory.
  4. Handling Concurrent Deletions: If multiple binary managers try to clean up the same folder concurrently, tokio::fs::remove_dir_all might fail with ErrorKind::NotFound. We should ignore NotFound errors to avoid logging false-positive warnings.
    pub async fn cleanup_old_versions(&self) -> Result<(), Error> {
        let binary_folder = self.adapter.get_binary_folder()?;
        let mut entries = match tokio::fs::read_dir(&binary_folder).await {
            Ok(entries) => entries,
            Err(ref e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()),
            Err(e) => return Err(e.into()),
        };

        let current_version = &self.selected_version;
        
        while let Some(entry) = entries.next_entry().await? {
            let file_type = match entry.file_type().await {
                Ok(ft) => ft,
                Err(_) => continue,
            };
            if !file_type.is_dir() {
                continue;
            }
            if let Some(folder_name) = entry.file_name().to_str() {
                if folder_name != current_version {
                    let path = entry.path();
                    info!(target: LOG_TARGET_APP_LOGIC, "Cleaning up old binary version directory: {}", path.display());
                    if let Err(e) = tokio::fs::remove_dir_all(&path).await {
                        if e.kind() != std::io::ErrorKind::NotFound {
                            warn!(target: LOG_TARGET_APP_LOGIC, "Failed to remove old binary version directory {}: {}", path.display(), e);
                        }
                    }
                }
            }
        }
        
        Ok(())
    }

@Ojas2095 Ojas2095 force-pushed the fix-3028-cleanup-binaries branch from bb48ba3 to 12068af Compare May 29, 2026 09:03
@Ojas2095 Ojas2095 force-pushed the fix-3028-cleanup-binaries branch from 12068af to ffeb01d Compare May 29, 2026 09:03
@Ojas2095

Ojas2095 commented Jun 9, 2026

Copy link
Copy Markdown
Author

Hi team! Just checking in to see if you've had a chance to review this PR, or if there's any feedback or further changes needed to merge this. Thanks!

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

1 participant