Skip to content

feat(worker): introduce cleanup cmd to remove folders + containers #470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

JannikSt
Copy link
Member

@JannikSt JannikSt commented Jun 16, 2025

fixes #464

@JannikSt JannikSt requested a review from Copilot June 18, 2025 03:21
Copilot

This comment was marked as outdated.

@JannikSt JannikSt requested a review from Copilot June 18, 2025 04:02
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new cleanup feature to the worker, allowing removal of prime-worker Docker containers and task directories via a prompt or forced command.

  • Introduces CleanupManager in utils/cleanup.rs with prompt_and_cleanup and force_cleanup methods
  • Extends DockerManager to list prime containers and clean task directories
  • Adds a new Cleanup CLI command and integrates prompt-based cleanup in main.rs

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/worker/src/utils/mod.rs Exposed new cleanup submodule
crates/worker/src/utils/cleanup.rs Implemented CleanupManager for interactive and forced cleanup
crates/worker/src/main.rs Invoked prompt-based cleanup after shutdown
crates/worker/src/docker/docker_manager.rs Added list_prime_containers and cleanup_task_directories
crates/worker/src/cli/command.rs Defined Cleanup CLI variant and its execution logic
Comments suppressed due to low confidence (2)

crates/worker/src/cli/command.rs:1045

  • The CLI ignores the state_dir_overwrite argument by always setting storage_path to None. You should initialize storage_path from state_dir_overwrite so overrides actually take effect.
        Commands::Cleanup {

crates/worker/src/utils/cleanup.rs:1

  • New cleanup logic isn’t covered by any existing tests. Consider adding unit tests or integration tests for prompt_and_cleanup, force_cleanup, and directory cleanup scenarios.
use crate::console::Console;


if !has_containers {
debug!("No prime worker containers found to clean up");
return true;
Copy link
Preview

Copilot AI Jun 18, 2025

Choose a reason for hiding this comment

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

When there are no containers, the method returns early and skips cleaning task directories. Consider separating container and directory checks so directories still get cleaned even if no containers exist.

Suggested change
return true;

Copilot uses AI. Check for mistakes.

None => {
// Default to user home directory or current directory
const APP_DIR_NAME: &str = "prime-worker";
if let Ok(home) = std::env::var("HOME") {
Copy link
Preview

Copilot AI Jun 18, 2025

Choose a reason for hiding this comment

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

[nitpick] Default storage path logic is duplicated here and in CleanupManager::get_default_storage_path. Consider extracting this into a shared helper to reduce duplication.

Copilot uses AI. Check for mistakes.

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 when shutting down worker
1 participant