Reuse sandbox deletion helpers in workload manager

pkg/workloadmanager/garbage_collection.go

@@ -18,11 +18,8 @@ package workloadmanager
 
 import (
 	"context"
-	"fmt"
 	"time"
 
-	"k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/klog/v2"
 
@@ -121,9 +118,9 @@ func (gc *garbageCollector) once() {
 	// delete sandboxes
 	for _, gcSandbox := range gcSandboxes {
 		if gcSandbox.Kind == types.SandboxClaimsKind {
-			err = gc.deleteSandboxClaim(ctx, gcSandbox.SandboxNamespace, gcSandbox.Name)
+			err = deleteSandboxClaim(ctx, gc.k8sClient.dynamicClient, gcSandbox.SandboxNamespace, gcSandbox.Name)
 		} else {
-			err = gc.deleteSandbox(ctx, gcSandbox.SandboxNamespace, gcSandbox.Name)
+			err = deleteSandbox(ctx, gc.k8sClient.dynamicClient, gcSandbox.SandboxNamespace, gcSandbox.Name)
 		}
 		if err != nil {
 			errs = append(errs, err)
@@ -141,28 +138,6 @@ func (gc *garbageCollector) once() {
 	}
 }
 
-func (gc *garbageCollector) deleteSandbox(ctx context.Context, namespace, name string) error {
-	err := gc.k8sClient.dynamicClient.Resource(SandboxGVR).Namespace(namespace).Delete(ctx, name, metav1.DeleteOptions{})
-	if err != nil {
-		if errors.IsNotFound(err) {
-			return nil
-		}
-		return fmt.Errorf("error deleting sandbox %s/%s: %w", namespace, name, err)
-	}
-	return nil
-}
-
-func (gc *garbageCollector) deleteSandboxClaim(ctx context.Context, namespace, name string) error {
-	err := gc.k8sClient.dynamicClient.Resource(SandboxClaimGVR).Namespace(namespace).Delete(ctx, name, metav1.DeleteOptions{})
-	if err != nil {
-		if errors.IsNotFound(err) {
-			return nil
-		}
-		return fmt.Errorf("error deleting sandboxClaim %s/%s: %w", namespace, name, err)
-	}
-	return nil
-}
-
 // deduplicateSandboxes merges multiple sandbox lists and removes duplicates
 // by SessionID, preserving the order of first occurrence.
 func deduplicateSandboxes(lists ...[]*types.SandboxInfo) []*types.SandboxInfo {

pkg/workloadmanager/garbage_collection_test.go

@@ -26,6 +26,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	dynamicfake "k8s.io/client-go/dynamic/fake"
+	k8stesting "k8s.io/client-go/testing"
 
 	"github.com/volcano-sh/agentcube/pkg/common/types"
 	"github.com/volcano-sh/agentcube/pkg/store"
@@ -272,3 +273,24 @@ func TestGarbageCollector_once_noSandboxes(t *testing.T) {
 	gc.once()
 	assert.Empty(t, fs.deleted)
 }
+
+func TestGarbageCollector_once_k8sDeleteFailurePreservesSession(t *testing.T) {
+	for _, kind := range []string{types.SandboxKind, types.SandboxClaimsKind} {
+		t.Run(kind, func(t *testing.T) {
+			fs := &gcFakeStore{expired: []*types.SandboxInfo{{
+				Kind: kind, SessionID: "session-1", Name: "sandbox-1", SandboxNamespace: "default",
+			}}}
+			gc := newTestGC(fs)
+			dc := dynamicfake.NewSimpleDynamicClient(runtime.NewScheme())
+			gc.k8sClient.dynamicClient = dc
+			dc.PrependReactor("delete", "*",
+				func(k8stesting.Action) (bool, runtime.Object, error) {
+					return true, nil, fmt.Errorf("Kubernetes unavailable")
+				})
+
+			gc.once()
+
+			assert.Empty(t, fs.deleted, "failed Kubernetes deletion must preserve the session for retry")
+		})
+	}
+}

pkg/workloadmanager/handlers.go

@@ -520,28 +520,13 @@ func (s *Server) handleDeleteSandbox(c *gin.Context) {
 
 	if sandbox.Kind == types.SandboxClaimsKind {
 		err = deleteSandboxClaim(c.Request.Context(), dynamicClient, sandbox.SandboxNamespace, sandbox.Name)
-		if err != nil {
-			if apierrors.IsNotFound(err) {
-				// Already deleted, consider as success
-				klog.Infof("sandbox claim %s/%s already deleted", sandbox.SandboxNamespace, sandbox.Name)
-			} else {
-				klog.Errorf("failed to delete sandbox claim %s/%s: %v", sandbox.SandboxNamespace, sandbox.Name, err)
-				respondError(c, http.StatusInternalServerError, "internal server error")
-				return
-			}
-		}
 	} else {
 		err = deleteSandbox(c.Request.Context(), dynamicClient, sandbox.SandboxNamespace, sandbox.Name)
-		if err != nil {
-			if apierrors.IsNotFound(err) {
-				// Already deleted, consider as success
-				klog.Infof("sandbox %s/%s already deleted", sandbox.SandboxNamespace, sandbox.Name)
-			} else {
-				klog.Errorf("failed to delete sandbox %s/%s: %v", sandbox.SandboxNamespace, sandbox.Name, err)
-				respondError(c, http.StatusInternalServerError, "internal server error")
-				return
-			}
-		}
+	}
+	if err != nil {
+		klog.Errorf("failed to delete %s %s/%s: %v", sandbox.Kind, sandbox.SandboxNamespace, sandbox.Name, err)
+		respondError(c, http.StatusInternalServerError, "internal server error")
+		return
 	}
 
 	// Use a detached context for the store delete so a client disconnect

pkg/workloadmanager/handlers_test.go

@@ -1015,3 +1015,43 @@ func TestHandleSandboxCreate_IdentityErrors(t *testing.T) {
 		})
 	}
 }
+
+func TestHandleDeleteSandbox_KubernetesResult(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	for _, kind := range []string{types.SandboxKind, types.SandboxClaimsKind} {
+		for _, failDelete := range []bool{false, true} {
+			t.Run(fmt.Sprintf("%s/fail=%t", kind, failDelete), func(t *testing.T) {
+				s := newFakeServer()
+				fs := &fakeStore{}
+				s.storeClient = fs
+				patches := gomonkey.NewPatches()
+				defer patches.Reset()
+				patches.ApplyMethod(reflect.TypeOf(fs), "GetSandboxBySessionID", func(_ *fakeStore, _ context.Context, _ string) (*types.SandboxInfo, error) {
+					return &types.SandboxInfo{Kind: kind, Name: "sandbox-1", SandboxNamespace: "default"}, nil
+				})
+				// An empty tracker returns NotFound; the shared helpers treat it as success.
+				dc := dynamicfake.NewSimpleDynamicClient(runtime.NewScheme())
+				if failDelete {
+					dc.PrependReactor("delete", "*", func(k8stesting.Action) (bool, runtime.Object, error) {
+						return true, nil, errors.New("Kubernetes unavailable")
+					})
+				}
+				s.k8sClient.dynamicClient = dc
+				w := httptest.NewRecorder()
+				c, _ := gin.CreateTestContext(w)
+				c.Request = httptest.NewRequest(http.MethodDelete, "/sandboxes/session-1", nil)
+				c.Params = gin.Params{{Key: "sessionId", Value: "session-1"}}
+
+				s.handleDeleteSandbox(c)
+
+				if failDelete {
+					require.Equal(t, http.StatusInternalServerError, w.Code)
+					require.Zero(t, fs.deleteSessionCalls)
+				} else {
+					require.Equal(t, http.StatusOK, w.Code)
+					require.Equal(t, 1, fs.deleteSessionCalls)
+				}
+			})
+		}
+	}
+}