Make workload deletion asynchronous (#738)

This copies the same behaviour added recently for stop. This change also fixes a bug where the deletion operation did not clean up the ingress/egress containers.

While looking at the runtime code, I discovered that the runtime does a force delete of containers, so there is no need to explicitly stop the container before force deleting.

Author: dmjb

cmd/thv/app/rm.go

@@ -29,7 +29,13 @@ func rmCmdFunc(cmd *cobra.Command, args []string) error {
 	}
 
 	// Delete container.
-	if err := manager.DeleteWorkload(ctx, containerName); err != nil {
+	group, err := manager.DeleteWorkload(ctx, containerName)
+	if err != nil {
+		return fmt.Errorf("failed to delete container: %v", err)
+	}
+
+	// Wait for the deletion to complete.
+	if err := group.Wait(); err != nil {
 		return fmt.Errorf("failed to delete container: %v", err)
 	}
 

pkg/api/v1/workloads.go

@@ -124,7 +124,7 @@ func (s *WorkloadRoutes) stopWorkload(w http.ResponseWriter, r *http.Request) {
 	ctx := r.Context()
 	name := chi.URLParam(r, "name")
 	// Note that this is an asynchronous operation.
-	// In the API, we do not wait for the operation to complete.
+	// The request is not blocked on completion.
 	_, err := s.manager.StopWorkload(ctx, name)
 	if err != nil {
 		if errors.Is(err, workloads.ErrContainerNotFound) {
@@ -174,7 +174,9 @@ func (s *WorkloadRoutes) stopAllWorkloads(w http.ResponseWriter, r *http.Request
 func (s *WorkloadRoutes) deleteWorkload(w http.ResponseWriter, r *http.Request) {
 	ctx := r.Context()
 	name := chi.URLParam(r, "name")
-	err := s.manager.DeleteWorkload(ctx, name)
+	// Note that this is an asynchronous operation.
+	// The request is not blocked on completion.
+	_, err := s.manager.DeleteWorkload(ctx, name)
 	if err != nil {
 		if errors.Is(err, workloads.ErrContainerNotFound) {
 			http.Error(w, "Workload not found", http.StatusNotFound)

pkg/workloads/manager.go

@@ -29,11 +29,11 @@ type Manager interface {
 	GetWorkload(ctx context.Context, name string) (Workload, error)
 	// ListWorkloads lists all ToolHive-managed containers.
 	ListWorkloads(ctx context.Context, listAll bool) ([]Workload, error)
-	// DeleteWorkload deletes a container and its associated proxy process.
-	// The container will be stopped if it is still running.
-	DeleteWorkload(ctx context.Context, name string) error
+	// DeleteWorkload deletes a container and all associated processes/containers.
+	// It is implemented as an asynchronous operation which returns an errgroup.Group
+	DeleteWorkload(ctx context.Context, name string) (*errgroup.Group, error)
 	// StopWorkload stops the named workload.
-	// It is implemented as an asynchronous operation which returns a errgroup.Group
+	// It is implemented as an asynchronous operation which returns an errgroup.Group
 	StopWorkload(ctx context.Context, name string) (*errgroup.Group, error)
 	// StopAllWorkloads stops all running workloads.
 	// It is implemented as an asynchronous operation which returns an errgroup.Group
@@ -101,66 +101,55 @@ func (d *defaultManager) ListWorkloads(ctx context.Context, listAll bool) ([]Wor
 	return workloads, nil
 }
 
-func (d *defaultManager) DeleteWorkload(ctx context.Context, name string) error {
+func (d *defaultManager) DeleteWorkload(ctx context.Context, name string) (*errgroup.Group, error) {
 	// We need several fields from the container struct for deletion.
 	container, err := d.findContainerByName(ctx, name)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	containerID := container.ID
-	isRunning := isContainerRunning(container)
 	containerLabels := container.Labels
-
-	// Check if the container is running
-	if isRunning {
-		// Get the base container name for proxy stopping
-		containerBaseName := labels.GetContainerBaseName(containerLabels)
-
-		// Stop the proxy process first (like StopWorkload does)
-		if containerBaseName != "" {
-			proxy.StopProcess(containerBaseName)
-		}
-
-		// Stop the container if it's running
-		if err := d.stopContainer(ctx, containerID, name); err != nil {
-			return fmt.Errorf("failed to stop container: %v", err)
-		}
-	}
-
-	// Remove the container
-	logger.Infof("Removing container %s...", name)
-	if err := d.runtime.RemoveWorkload(ctx, containerID); err != nil {
-		return fmt.Errorf("failed to remove container: %v", err)
-	}
-
-	// Get the base name from the container labels
 	baseName := labels.GetContainerBaseName(containerLabels)
-	if baseName != "" {
-		// Clean up temporary permission profile before deleting saved state
-		if err := d.cleanupTempPermissionProfile(ctx, baseName); err != nil {
-			logger.Warnf("Warning: Failed to cleanup temporary permission profile: %v", err)
-		}
 
-		// Delete the saved state if it exists
-		if err := runner.DeleteSavedConfig(ctx, baseName); err != nil {
-			logger.Warnf("Warning: Failed to delete saved state: %v", err)
-		} else {
-			logger.Infof("Saved state for %s removed", baseName)
+	// Create second errorgroup for deletion.
+	deleteGroup := &errgroup.Group{}
+	deleteGroup.Go(func() error {
+		// Remove the container
+		logger.Infof("Removing container %s...", name)
+		if err := d.runtime.RemoveWorkload(ctx, containerID); err != nil {
+			return fmt.Errorf("failed to remove container: %v", err)
 		}
 
-		logger.Infof("Container %s removed", name)
+		// Get the base name from the container labels
+		if baseName != "" {
+			// Clean up temporary permission profile before deleting saved state
+			if err := d.cleanupTempPermissionProfile(ctx, baseName); err != nil {
+				logger.Warnf("Warning: Failed to cleanup temporary permission profile: %v", err)
+			}
 
-		if shouldRemoveClientConfig() {
-			if err := removeClientConfigurations(name); err != nil {
-				logger.Warnf("Warning: Failed to remove client configurations: %v", err)
+			// Delete the saved state if it exists
+			if err := runner.DeleteSavedConfig(ctx, baseName); err != nil {
+				logger.Warnf("Warning: Failed to delete saved state: %v", err)
 			} else {
-				logger.Infof("Client configurations for %s removed", name)
+				logger.Infof("Saved state for %s removed", baseName)
+			}
+
+			logger.Infof("Container %s removed", name)
+
+			if shouldRemoveClientConfig() {
+				if err := removeClientConfigurations(name); err != nil {
+					logger.Warnf("Warning: Failed to remove client configurations: %v", err)
+				} else {
+					logger.Infof("Client configurations for %s removed", name)
+				}
 			}
 		}
-	}
 
-	return nil
+		return nil
+	})
+
+	return deleteGroup, nil
 }
 
 func (d *defaultManager) StopWorkload(ctx context.Context, name string) (*errgroup.Group, error) {
@@ -484,17 +473,6 @@ func (d *defaultManager) findContainerByName(ctx context.Context, name string) (
 	return nil, fmt.Errorf("%w: %s", ErrContainerNotFound, name)
 }
 
-// stopContainer stops the container
-func (d *defaultManager) stopContainer(ctx context.Context, containerID, containerName string) error {
-	logger.Infof("Stopping container %s...", containerName)
-	if err := d.runtime.StopWorkload(ctx, containerID); err != nil {
-		return fmt.Errorf("failed to stop container: %w", err)
-	}
-
-	logger.Infof("Container %s stopped", containerName)
-	return nil
-}
-
 func shouldRemoveClientConfig() bool {
 	c := config.GetConfig()
 	return len(c.Clients.RegisteredClients) > 0 || c.Clients.AutoDiscovery
@@ -589,14 +567,13 @@ func (d *defaultManager) stopWorkloads(ctx context.Context, workloads []stopWork
 	group := errgroup.Group{}
 	for _, workload := range workloads {
 		group.Go(func() error {
-			logger.Infof("Stopping workload %s...", workload.Name)
 			// Stop the proxy process
 			proxy.StopProcess(workload.Name)
 
+			logger.Infof("Stopping containers for %s...", workload.Name)
 			// Stop the container
-			err := d.stopContainer(ctx, workload.ID, workload.Name)
-			if err != nil {
-				return err
+			if err := d.runtime.StopWorkload(ctx, workload.ID); err != nil {
+				return fmt.Errorf("failed to stop container: %w", err)
 			}
 
 			if shouldRemoveClientConfig() {