[shimV2] Fix guest/VM exit on their own

cmd/containerd-shim-lcow-v2/service/service.go

@@ -14,7 +14,7 @@ import (
 	"github.com/Microsoft/hcsshim/internal/shimdiag"
 
 	sandboxsvc "github.com/containerd/containerd/api/runtime/sandbox/v1"
-	tasksvc "github.com/containerd/containerd/api/runtime/task/v2"
+	tasksvc "github.com/containerd/containerd/api/runtime/task/v3"
 	"github.com/containerd/containerd/v2/core/runtime"
 	"github.com/containerd/containerd/v2/pkg/namespaces"
 	"github.com/containerd/containerd/v2/pkg/shutdown"
@@ -91,7 +91,7 @@ func NewService(ctx context.Context, eventsPublisher shim.Publisher, sd shutdown
 // RegisterTTRPC registers the Task, Sandbox, and ShimDiag TTRPC services on
 // the provided server so that containerd can call into the shim over TTRPC.
 func (s *Service) RegisterTTRPC(server *ttrpc.Server) error {
-	tasksvc.RegisterTaskService(server, s)
+	tasksvc.RegisterTTRPCTaskService(server, s)
 	sandboxsvc.RegisterTTRPCSandboxService(server, s)
 	shimdiag.RegisterShimDiagService(server, s)
 	return nil

cmd/containerd-shim-lcow-v2/service/service_task.go

@@ -9,14 +9,14 @@ import (
 	"github.com/Microsoft/hcsshim/internal/logfields"
 	"github.com/Microsoft/hcsshim/internal/oc"
 
-	"github.com/containerd/containerd/api/runtime/task/v2"
+	"github.com/containerd/containerd/api/runtime/task/v3"
 	"github.com/containerd/errdefs/pkg/errgrpc"
 	"go.opencensus.io/trace"
 	"google.golang.org/protobuf/types/known/emptypb"
 )
 
 // Ensure Service implements the TTRPCTaskService interface at compile time.
-var _ task.TaskService = &Service{}
+var _ task.TTRPCTaskService = &Service{}
 
 // State returns the current state of a task or process.
 // This method is part of the instrumentation layer and business logic is included in stateInternal.

cmd/containerd-shim-lcow-v2/service/service_task_internal.go

@@ -12,6 +12,7 @@ import (
 	container "github.com/Microsoft/hcsshim/internal/controller/linuxcontainer"
 	"github.com/Microsoft/hcsshim/internal/controller/pod"
 	"github.com/Microsoft/hcsshim/internal/controller/process"
+	"github.com/Microsoft/hcsshim/internal/controller/vm"
 	"github.com/Microsoft/hcsshim/internal/hcs"
 	hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2"
 	"github.com/Microsoft/hcsshim/internal/log"
@@ -23,7 +24,7 @@ import (
 
 	"github.com/Microsoft/hcsshim/pkg/ctrdtaskapi"
 	eventstypes "github.com/containerd/containerd/api/events"
-	"github.com/containerd/containerd/api/runtime/task/v2"
+	"github.com/containerd/containerd/api/runtime/task/v3"
 	"github.com/containerd/errdefs"
 	"github.com/containerd/typeurl/v2"
 	"github.com/opencontainers/runtime-spec/specs-go"
@@ -69,10 +70,6 @@ func (s *Service) getPodController(podID string) (*pod.Controller, bool) {
 
 // stateInternal returns the current status of a process within a container.
 func (s *Service) stateInternal(_ context.Context, request *task.StateRequest) (*task.StateResponse, error) {
-	if err := s.ensureVMRunning(); err != nil {
-		return nil, err
-	}
-
 	// Look up the container controller for the requested container.
 	ctrCtrl, err := s.getContainerController(request.ID)
 	if err != nil {
@@ -277,10 +274,6 @@ func (s *Service) startInternal(ctx context.Context, request *task.StartRequest)
 
 // deleteInternal deletes a process, container, or pod sandbox depending on the request.
 func (s *Service) deleteInternal(ctx context.Context, request *task.DeleteRequest) (*task.DeleteResponse, error) {
-	if err := s.ensureVMRunning(); err != nil {
-		return nil, err
-	}
-
 	// Look up the container controller for the target ID.
 	ctrCtrl, err := s.getContainerController(request.ID)
 	if err != nil {
@@ -328,7 +321,7 @@ func (s *Service) deleteInternal(ctx context.Context, request *task.DeleteReques
 		// left should be the sandbox container itself (request.ID).
 		remaining := podCtrl.ListContainers()
 		delete(remaining, request.ID) // exclude the sandbox container itself
-		if len(remaining) > 0 {
+		if len(remaining) > 0 && s.vmController.State() == vm.StateRunning {
 			return nil, fmt.Errorf("cannot delete sandbox container %s: %d workload container(s) still exist in the pod: %w",
 				request.ID, len(remaining), errdefs.ErrFailedPrecondition)
 		}

internal/controller/linuxcontainer/container.go

@@ -4,6 +4,7 @@ package linuxcontainer
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"sync"
 	"time"
@@ -12,7 +13,6 @@ import (
 	"github.com/Microsoft/hcsshim/cmd/containerd-shim-runhcs-v1/stats"
 	"github.com/Microsoft/hcsshim/internal/controller/process"
 	"github.com/Microsoft/hcsshim/internal/gcs"
-	"github.com/Microsoft/hcsshim/internal/hcs"
 	"github.com/Microsoft/hcsshim/internal/hcs/schema1"
 	hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2"
 	"github.com/Microsoft/hcsshim/internal/log"
@@ -25,7 +25,7 @@ import (
 
 	"github.com/Microsoft/go-winio/pkg/guid"
 	eventstypes "github.com/containerd/containerd/api/events"
-	"github.com/containerd/containerd/api/runtime/task/v2"
+	"github.com/containerd/containerd/api/runtime/task/v3"
 	containerdtypes "github.com/containerd/containerd/api/types/task"
 	"github.com/containerd/errdefs"
 	"github.com/containerd/typeurl/v2"
@@ -220,6 +220,14 @@ func (c *Controller) closeContainer() {
 	}
 }
 
+// isResourceAlreadyReleased reports whether a teardown failure indicates the
+// resource is already gone — either the in-guest agent died (so guest-side
+// state went with it) or the VM itself has exited (so host-side state went
+// with it). Either way, teardown can move on instead of retrying.
+func isResourceAlreadyReleased(err error) bool {
+	return errors.Is(err, gcs.ErrBridgeClosed) || vmutils.IsVMNotAvailableError(err)
+}
+
 // releaseResources undoes each allocation in reverse order.
 // It is idempotent — subsequent calls after the first are no-ops.
 func (c *Controller) releaseResources(ctx context.Context) error {
@@ -236,7 +244,7 @@ func (c *Controller) releaseResources(ctx context.Context) error {
 			ContainerRootPath: c.layers.rootfsPath,
 			Layers:            hcsLayers,
 			ScratchPath:       c.layers.scratch.guestPath,
-		}); err != nil {
+		}); err != nil && !isResourceAlreadyReleased(err) {
 			return fmt.Errorf("remove combined layers from guest: %w", err)
 		}
 
@@ -248,7 +256,7 @@ func (c *Controller) releaseResources(ctx context.Context) error {
 	// unmapped on a prior call.
 	var zeroGUID guid.GUID
 	if c.layers != nil && c.layers.scratch.id != zeroGUID {
-		if err := c.scsi.UnmapFromGuest(ctx, c.layers.scratch.id); err != nil {
+		if err := c.scsi.UnmapFromGuest(ctx, c.layers.scratch.id); err != nil && !isResourceAlreadyReleased(err) {
 			return fmt.Errorf("unmap scratch layer: %w", err)
 		}
 		c.layers.scratch = scsiReservation{}
@@ -258,7 +266,7 @@ func (c *Controller) releaseResources(ctx context.Context) error {
 	// resumes from the first failure.
 	if c.layers != nil {
 		for i, layer := range c.layers.roLayers {
-			if err := c.scsi.UnmapFromGuest(ctx, layer.id); err != nil {
+			if err := c.scsi.UnmapFromGuest(ctx, layer.id); err != nil && !isResourceAlreadyReleased(err) {
 				c.layers.roLayers = c.layers.roLayers[i:]
 				return fmt.Errorf("unmap ro layer: %w", err)
 			}
@@ -267,23 +275,23 @@ func (c *Controller) releaseResources(ctx context.Context) error {
 
 	// Unmap additional SCSI mounts.
 	for i, id := range c.scsiResources {
-		if err := c.scsi.UnmapFromGuest(ctx, id); err != nil {
+		if err := c.scsi.UnmapFromGuest(ctx, id); err != nil && !isResourceAlreadyReleased(err) {
 			c.scsiResources = c.scsiResources[i:]
 			return fmt.Errorf("unmap scsi resource: %w", err)
 		}
 	}
 
 	// Unmap Plan9 shares.
 	for i, id := range c.plan9Resources {
-		if err := c.plan9.UnmapFromGuest(ctx, id); err != nil {
+		if err := c.plan9.UnmapFromGuest(ctx, id); err != nil && !isResourceAlreadyReleased(err) {
 			c.plan9Resources = c.plan9Resources[i:]
 			return fmt.Errorf("unmap plan9 share: %w", err)
 		}
 	}
 
 	// Remove VPCI devices.
 	for i, id := range c.devices {
-		if err := c.vpci.RemoveFromVM(ctx, id); err != nil {
+		if err := c.vpci.RemoveFromVM(ctx, id); err != nil && !isResourceAlreadyReleased(err) {
 			c.devices = c.devices[i:]
 			return fmt.Errorf("remove vpci device: %w", err)
 		}
@@ -295,7 +303,7 @@ func (c *Controller) releaseResources(ctx context.Context) error {
 	if !c.isContainerStateDeleted && c.guest.Capabilities().IsDeleteContainerStateSupported() {
 		// GCS bridge evicts the container from its host-state map even if the inner Delete fails,
 		// so retries will always return not-found.
-		if err := c.guest.DeleteContainerState(ctx, c.gcsContainerID); err != nil && !hcs.IsNotExist(err) {
+		if err := c.guest.DeleteContainerState(ctx, c.gcsContainerID); err != nil && !isResourceAlreadyReleased(err) {
 			return fmt.Errorf("delete container state: %w", err)
 		}
 

internal/controller/linuxcontainer/container_test.go

@@ -606,38 +606,71 @@ func TestReleaseResources_Idempotent(t *testing.T) {
 	}
 }
 
-// TestReleaseResources_StopsOnFirstError verifies that releaseResources
-// returns the first error encountered and does not proceed to subsequent
-// resource categories.
+// TestReleaseResources_StopsOnFirstError verifies the chain's response to an
+// error at the SCSI-extras unmap leg. A real failure short-circuits the chain
+// (plan9/vpci skipped, slice retained for retry). Any "already-gone" error —
+// the GCS bridge being closed or the VM no longer being available — is
+// tolerated and the chain proceeds through the remaining categories.
 func TestReleaseResources_StopsOnFirstError(t *testing.T) {
 	t.Parallel()
-	c, scsiCtrl, _, _, _ := newContainerTestController(t)
 
-	scsiGUID, _ := guid.NewV4()
-	plan9GUID, _ := guid.NewV4()
-	deviceGUID, _ := guid.NewV4()
+	cases := []struct {
+		name      string
+		scsiErr   error
+		wantStops bool
+	}{
+		{name: "RealError_StopsChain", scsiErr: errUnmapSCSI, wantStops: true},
+		{name: "BridgeClosed_Continues", scsiErr: fmt.Errorf("transport gone: %w", gcs.ErrBridgeClosed)},
+		{name: "ComputeSystemDoesNotExist_Continues", scsiErr: fmt.Errorf("hcs::System::Modify: %w", hcs.ErrComputeSystemDoesNotExist)},
+		{name: "VmcomputeAlreadyStopped_Continues", scsiErr: fmt.Errorf("hcs::System::Modify: %w", hcs.ErrVmcomputeAlreadyStopped)},
+		{name: "VmcomputeOperationInvalidState_Continues", scsiErr: fmt.Errorf("hcs::System::Modify: %w", hcs.ErrVmcomputeOperationInvalidState)},
+		{name: "AlreadyClosed_Continues", scsiErr: fmt.Errorf("hcs::System::Modify: %w", hcs.ErrAlreadyClosed)},
+	}
 
-	c.scsiResources = []guid.GUID{scsiGUID}
-	c.plan9Resources = []guid.GUID{plan9GUID}
-	c.devices = []guid.GUID{deviceGUID}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			c, scsiCtrl, plan9Ctrl, vpciCtrl, guestCtrl := newContainerTestController(t)
 
-	// SCSI unmap fails; plan9/vpci should not be invoked.
-	scsiCtrl.EXPECT().UnmapFromGuest(gomock.Any(), scsiGUID).Return(errUnmapSCSI)
+			scsiGUID, _ := guid.NewV4()
+			plan9GUID, _ := guid.NewV4()
+			deviceGUID, _ := guid.NewV4()
 
-	err := c.releaseResources(t.Context())
-	if !errors.Is(err, errUnmapSCSI) {
-		t.Fatalf("releaseResources error = %v, want %v", err, errUnmapSCSI)
-	}
+			c.scsiResources = []guid.GUID{scsiGUID}
+			c.plan9Resources = []guid.GUID{plan9GUID}
+			c.devices = []guid.GUID{deviceGUID}
 
-	// Failed scsi entry is retained for retry; plan9 and devices unchanged.
-	if len(c.scsiResources) != 1 {
-		t.Errorf("scsiResources len = %d, want 1 (retained for retry)", len(c.scsiResources))
-	}
-	if len(c.plan9Resources) != 1 {
-		t.Errorf("plan9Resources len = %d, want 1 (untouched)", len(c.plan9Resources))
-	}
-	if len(c.devices) != 1 {
-		t.Errorf("devices len = %d, want 1 (untouched)", len(c.devices))
+			scsiCtrl.EXPECT().UnmapFromGuest(gomock.Any(), scsiGUID).Return(tc.scsiErr)
+
+			if !tc.wantStops {
+				// Tolerated error: the chain must proceed through plan9, vpci,
+				// and the DeleteContainerState capability gate.
+				plan9Ctrl.EXPECT().UnmapFromGuest(gomock.Any(), plan9GUID).Return(nil)
+				vpciCtrl.EXPECT().RemoveFromVM(gomock.Any(), deviceGUID).Return(nil)
+				guestCtrl.EXPECT().Capabilities().Return(&gcs.LCOWGuestDefinedCapabilities{})
+			}
+
+			err := c.releaseResources(t.Context())
+			if tc.wantStops {
+				if !errors.Is(err, tc.scsiErr) {
+					t.Fatalf("releaseResources error = %v, want %v", err, tc.scsiErr)
+				}
+				// Failed scsi entry retained; plan9 and devices untouched.
+				if len(c.scsiResources) != 1 {
+					t.Errorf("scsiResources len = %d, want 1 (retained for retry)", len(c.scsiResources))
+				}
+				if len(c.plan9Resources) != 1 {
+					t.Errorf("plan9Resources len = %d, want 1 (untouched)", len(c.plan9Resources))
+				}
+				if len(c.devices) != 1 {
+					t.Errorf("devices len = %d, want 1 (untouched)", len(c.devices))
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("releaseResources error = %v, want nil for tolerated error", err)
+			}
+		})
 	}
 }
 
@@ -860,32 +893,47 @@ func TestReleaseResources_DeleteContainerStateFails(t *testing.T) {
 	}
 }
 
-// TestReleaseResources_DeleteContainerStateNotFoundIsSuccess verifies that a
-// "compute system does not exist" error from DeleteContainerState is treated
-// as success. This matters because the GCS bridge removes the container from
-// its host-state map even when its inner Delete() fails, so any retry of the
-// RPC will always come back with "not found" — looping forever would otherwise
-// be the consequence.
-func TestReleaseResources_DeleteContainerStateNotFoundIsSuccess(t *testing.T) {
+// TestReleaseResources_DeleteContainerState_ToleratesAlreadyGone verifies
+// that any error indicating the resource is already gone is treated as a
+// successful delete: the GCS bridge being closed (so the in-guest agent
+// cannot answer) and any of the HCS conditions that surface when the VM has
+// already exited or is in an invalid state for further modifications. In all
+// cases the chain must complete and isContainerStateDeleted must flip so a
+// retry does not re-issue the RPC.
+func TestReleaseResources_DeleteContainerState_ToleratesAlreadyGone(t *testing.T) {
 	t.Parallel()
-	c, _, _, _, guestCtrl := newContainerTestController(t)
 
-	caps := &gcs.LCOWGuestDefinedCapabilities{}
-	caps.DeleteContainerStateSupported = true
+	cases := []struct {
+		name string
+		err  error
+	}{
+		{name: "BridgeClosed", err: fmt.Errorf("transport gone: %w", gcs.ErrBridgeClosed)},
+		{name: "ComputeSystemDoesNotExist", err: fmt.Errorf("guest RPC failure: %w", hcs.ErrComputeSystemDoesNotExist)},
+		{name: "VmcomputeAlreadyStopped", err: fmt.Errorf("guest RPC failure: %w", hcs.ErrVmcomputeAlreadyStopped)},
+		{name: "VmcomputeOperationInvalidState", err: fmt.Errorf("guest RPC failure: %w", hcs.ErrVmcomputeOperationInvalidState)},
+		{name: "AlreadyClosed", err: fmt.Errorf("guest RPC failure: %w", hcs.ErrAlreadyClosed)},
+	}
 
-	guestCtrl.EXPECT().Capabilities().Return(caps)
-	// The bridge's response Result code is wrapped through windows.Errno
-	// (= syscall.Errno) so errors.Is against hcs.ErrComputeSystemDoesNotExist
-	// matches.
-	guestCtrl.EXPECT().
-		DeleteContainerState(gomock.Any(), c.gcsContainerID).
-		Return(fmt.Errorf("guest RPC failure: %w", hcs.ErrComputeSystemDoesNotExist))
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			c, _, _, _, guestCtrl := newContainerTestController(t)
 
-	if err := c.releaseResources(t.Context()); err != nil {
-		t.Fatalf("releaseResources returned error for not-found: %v", err)
-	}
-	if !c.isContainerStateDeleted {
-		t.Fatal("isContainerStateDeleted should be true even when the guest reports the container is gone")
+			caps := &gcs.LCOWGuestDefinedCapabilities{}
+			caps.DeleteContainerStateSupported = true
+
+			guestCtrl.EXPECT().Capabilities().Return(caps)
+			guestCtrl.EXPECT().
+				DeleteContainerState(gomock.Any(), c.gcsContainerID).
+				Return(tc.err)
+
+			if err := c.releaseResources(t.Context()); err != nil {
+				t.Fatalf("releaseResources returned error: %v", err)
+			}
+			if !c.isContainerStateDeleted {
+				t.Fatal("isContainerStateDeleted should be true when the guest reports the container is gone")
+			}
+		})
 	}
 }
 

internal/controller/network/network_lcow.go

@@ -4,12 +4,15 @@ package network
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"github.com/Microsoft/hcsshim/hcn"
+	"github.com/Microsoft/hcsshim/internal/gcs"
 	hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2"
 	"github.com/Microsoft/hcsshim/internal/log"
 	"github.com/Microsoft/hcsshim/internal/protocol/guestresource"
+	"github.com/Microsoft/hcsshim/internal/vm/vmutils"
 )
 
 // guestNetwork exposes linux guest network operations.
@@ -81,7 +84,7 @@ func (c *Controller) removeEndpointFromGuestNamespace(ctx context.Context, nicID
 		if err := c.guestNetwork.RemoveNetworkInterface(ctx, &guestresource.LCOWNetworkAdapter{
 			NamespaceID: c.namespaceID,
 			ID:          nicID,
-		}); err != nil {
+		}); err != nil && !errors.Is(err, gcs.ErrBridgeClosed) {
 			return fmt.Errorf("remove NIC %s from guest: %w", nicID, err)
 		}
 
@@ -92,7 +95,7 @@ func (c *Controller) removeEndpointFromGuestNamespace(ctx context.Context, nicID
 	if err := c.vmNetwork.RemoveNIC(ctx, nicID, &hcsschema.NetworkAdapter{
 		EndpointId: endpoint.Id,
 		MacAddress: endpoint.MacAddress,
-	}); err != nil {
+	}); err != nil && !vmutils.IsVMNotAvailableError(err) {
 		return fmt.Errorf("remove NIC %s from host (endpoint %s): %w", nicID, endpoint.Id, err)
 	}