Refactor container lifecycle functions to perform scoped rollback on failure (#1080)

- Closes #977.
- Closes #1058.
- Prevents unexpected removal of containers on
  bootstrapping and starting failures, by reorganizing
  error handling for container `run`, `start`, and
  `exec` so that error handling only unwinds that
  which was done in the current scope.
- Relies on apple/containerization#495.

Package.resolved

@@ -1,5 +1,5 @@
 {
-  "originHash" : "6b2a03b8a5a190690d707d7c5454783bfed5c0c8b44490ef50ef4651038b88f9",
+  "originHash" : "ea5432dec5056581c1236ddc433fa8813eed012017e53b9e3a40abb648f2676b",
   "pins" : [
     {
       "identity" : "async-http-client",
@@ -15,8 +15,8 @@
       "kind" : "remoteSourceControl",
       "location" : "https://github.com/apple/containerization.git",
       "state" : {
-        "revision" : "26f3dcc796c89baf729827770c011cd694debcbf",
-        "version" : "0.21.1"
+        "revision" : "fd62f311b480d1fb5ef243a65927da54b4eecfae",
+        "version" : "0.23.1"
       }
     },
     {

Package.swift

@@ -23,7 +23,7 @@ import PackageDescription
 let releaseVersion = ProcessInfo.processInfo.environment["RELEASE_VERSION"] ?? "0.0.0"
 let gitCommit = ProcessInfo.processInfo.environment["GIT_COMMIT"] ?? "unspecified"
 let builderShimVersion = "0.7.0"
-let scVersion = "0.21.1"
+let scVersion = "0.23.1"
 
 let package = Package(
     name: "container",

Sources/ContainerCommands/Container/ContainerRun.swift

@@ -161,6 +161,7 @@ extension Application {
 
                 exitCode = try await io.handleProcess(process: process, log: log)
             } catch {
+                try? await container.delete()
                 if error is ContainerizationError {
                     throw error
                 }

Sources/Services/ContainerAPIService/Server/Containers/ContainersService.swift

@@ -268,20 +268,21 @@ public actor ContainersService {
     /// Bootstrap the init process of the container.
     public func bootstrap(id: String, stdio: [FileHandle?]) async throws {
         self.log.debug("\(#function)")
-        do {
-            try await self.lock.withLock { context in
-                var state = try await self.getContainerState(id: id, context: context)
+        try await self.lock.withLock { context in
+            var state = try await self.getContainerState(id: id, context: context)
 
-                // We've already bootstrapped this container. Ideally we should be able to
-                // return some sort of error code from the sandbox svc to check here, but this
-                // is also a very simple check and faster than doing an rpc to get the same result.
-                if state.client != nil {
-                    return
-                }
+            // We've already bootstrapped this container. Ideally we should be able to
+            // return some sort of error code from the sandbox svc to check here, but this
+            // is also a very simple check and faster than doing an rpc to get the same result.
+            if state.client != nil {
+                return
+            }
 
-                let path = self.containerRoot.appendingPathComponent(id)
-                let bundle = ContainerResource.Bundle(path: path)
-                let config = try bundle.configuration
+            let path = self.containerRoot.appendingPathComponent(id)
+            let bundle = ContainerResource.Bundle(path: path)
+            let config = try bundle.configuration
+
+            do {
                 try Self.registerService(
                     plugin: self.runtimePlugins.first { $0.name == config.runtimeHandler }!,
                     loader: self.pluginLoader,
@@ -303,14 +304,17 @@ public actor ContainersService {
 
                 state.client = sandboxClient
                 await self.setContainerState(id, state, context: context)
-            }
-        } catch {
-            do {
-                try await _cleanup(id: id)
             } catch {
-                self.log.error("failed to cleanup container \(id) after bootstrap failure: \(error)")
+                let label = Self.fullLaunchdServiceLabel(
+                    runtimeName: config.runtimeHandler,
+                    instanceId: id
+                )
+
+                await self.exitMonitor.stopTracking(id: id)
+                try? ServiceManager.deregister(fullServiceLabel: label)
+
+                throw error
             }
-            throw error
         }
     }
 
@@ -324,21 +328,12 @@ public actor ContainersService {
         self.log.debug("\(#function)")
 
         let state = try self._getContainerState(id: id)
-        do {
-            let client = try state.getClient()
-            try await client.createProcess(
-                processID,
-                config: config,
-                stdio: stdio
-            )
-        } catch {
-            do {
-                try await _cleanup(id: id)
-            } catch {
-                self.log.error("failed to cleanup container \(id) after start failure: \(error)")
-            }
-            throw error
-        }
+        let client = try state.getClient()
+        try await client.createProcess(
+            processID,
+            config: config,
+            stdio: stdio
+        )
     }
 
     /// Start a process in a container. This can either be a process created via
@@ -347,43 +342,43 @@ public actor ContainersService {
     public func startProcess(id: String, processID: String) async throws {
         self.log.debug("\(#function)")
 
-        do {
-            try await self.lock.withLock { context in
-                var state = try await self.getContainerState(id: id, context: context)
+        try await self.lock.withLock { context in
+            var state = try await self.getContainerState(id: id, context: context)
 
-                let isInit = Self.isInitProcess(id: id, processID: processID)
-                if state.snapshot.status == .running && isInit {
-                    return
-                }
-
-                let client = try state.getClient()
-                try await client.startProcess(processID)
-
-                if isInit {
-                    let log = self.log
-                    let waitFunc: ExitMonitor.WaitHandler = {
-                        log.info("registering container \(id) with exit monitor")
-                        let code = try await client.wait(id)
-                        log.info("container \(id) finished in exit monitor, exit code \(code)")
-
-                        return code
-                    }
-                    try await self.exitMonitor.track(id: id, waitingOn: waitFunc)
-
-                    let sandboxSnapshot = try await client.state()
-                    state.snapshot.status = .running
-                    state.snapshot.networks = sandboxSnapshot.networks
-                    state.snapshot.startedDate = Date()
-                    await self.setContainerState(id, state, context: context)
-                }
+            let isInit = Self.isInitProcess(id: id, processID: processID)
+            if state.snapshot.status == .running && isInit {
+                return
             }
-        } catch {
+
+            let client = try state.getClient()
+            try await client.startProcess(processID)
+
+            guard isInit else {
+                return
+            }
+
             do {
-                try await _cleanup(id: id)
+                let log = self.log
+                let waitFunc: ExitMonitor.WaitHandler = {
+                    log.info("registering container \(id) with exit monitor")
+                    let code = try await client.wait(id)
+                    log.info("container \(id) finished in exit monitor, exit code \(code)")
+
+                    return code
+                }
+                try await self.exitMonitor.track(id: id, waitingOn: waitFunc)
+
+                let sandboxSnapshot = try await client.state()
+                state.snapshot.status = .running
+                state.snapshot.networks = sandboxSnapshot.networks
+                state.snapshot.startedDate = Date()
+                await self.setContainerState(id, state, context: context)
             } catch {
-                self.log.error("failed to cleanup container \(id) after start failure: \(error)")
+                await self.exitMonitor.stopTracking(id: id)
+                try? await client.stop(options: ContainerStopOptions.default)
+
+                throw error
             }
-            throw error
         }
     }
 

Sources/Services/ContainerNetworkService/Server/ReservedVmnetNetwork.swift

@@ -181,7 +181,7 @@ public final class ReservedVmnetNetwork: Network {
         let prefixIpv6Bytes = withUnsafeBytes(of: prefixAddr.__u6_addr.__u6_addr8) {
             Array($0)
         }
-        let prefixIpv6Addr = IPv6Address(prefixIpv6Bytes)
+        let prefixIpv6Addr = try IPv6Address(prefixIpv6Bytes)
         let runningV6Subnet = try CIDRv6(prefixIpv6Addr, prefix: prefix)
 
         log.info(

Tests/CLITests/Subcommands/Containers/TestCLIExec.swift

@@ -107,4 +107,24 @@ class TestCLIExecCommand: CLITest {
             return
         }
     }
+
+    @Test func testExecOnExitingContainer() throws {
+        do {
+            let name = getTestName()
+            try doLongRun(name: name, containerArgs: ["sh"], autoRemove: false)
+            defer {
+                try? doRemove(name: name)
+            }
+            // Give time for container process to exit due to no stdin
+            sleep(1)
+
+            try doStart(name: name)
+            do {
+                _ = try doExec(name: name, cmd: ["sleep", "infinity"])
+            } catch CLIError.executionFailed(let message) {
+                #expect(message.contains("is not running"))
+            }
+            #expect(try getContainerStatus(name) == "stopped")
+        }
+    }
 }

Tests/CLITests/Subcommands/Run/TestCLIRunLifecycle.swift

@@ -14,6 +14,7 @@
 // limitations under the License.
 //===----------------------------------------------------------------------===//
 
+import ContainerizationError
 import Testing
 
 class TestCLIRunLifecycle: CLITest {
@@ -83,4 +84,32 @@ class TestCLIRunLifecycle: CLITest {
             try self.doStop(name: name)
         }
     }
+
+    @Test func testStartPortBindFails() async throws {
+        let port = UInt16.random(in: 50000..<60000)
+
+        let name = getTestName()
+        try self.doCreate(name: name, ports: ["\(port)"])
+        defer {
+            try? self.doRemove(name: name)
+        }
+
+        let server = "\(name)-server"
+        try doLongRun(
+            name: server,
+            image: "docker.io/library/python:alpine",
+            args: ["--publish", "\(port):\(port)"],
+            containerArgs: ["python3", "-m", "http.server", "\(port)"]
+        )
+        defer {
+            try? doStop(name: server)
+        }
+
+        #expect(throws: CLIError.self) {
+            try doStart(name: name)
+        }
+
+        let status = try getContainerStatus(name)
+        #expect(status == "stopped")
+    }
 }

Tests/CLITests/Utilities/CLITest.swift

@@ -269,7 +269,8 @@ class CLITest {
         image: String? = nil,
         args: [String]? = nil,
         volumes: [String] = [],
-        networks: [String] = []
+        networks: [String] = [],
+        ports: [String] = []
     ) throws {
         let image = image ?? alpine
         let args: [String] = args ?? ["sleep", "infinity"]
@@ -288,6 +289,10 @@ class CLITest {
             arguments += ["--network", network]
         }
 
+        for port in ports {
+            arguments += ["--publish", "\(port):\(port)"]
+        }
+
         arguments += [image] + args
 
         let (_, _, error, status) = try run(arguments: arguments)