Make container start idempotent (#792)

Fixes #772 

Today it fails in bootstrap the second go around, and we also have an
error handler that automatically cleans up the container if bootstrap
failed which is even worse. This change short circuits us first in the
cli if the state is running when we get() the container, and also adds
in a clause to bootstrap to just early return if we already have a
client.

Sources/ContainerCommands/Container/ContainerStart.swift

@@ -52,9 +52,24 @@ extension Application {
             }
             progress.start()
 
+            let detach = !self.attach && !self.interactive
             let container = try await ClientContainer.get(id: containerId)
+
+            // Bootstrap and process start are both idempotent and don't fail the second time
+            // around, however not doing an rpc is always faster :). The other bit is we don't
+            // support attach currently, so we can't do `start -a` a second time and have it succeed.
+            if container.status == .running {
+                if !detach {
+                    throw ContainerizationError(
+                        .invalidArgument,
+                        message: "attach is currently unsupported on already running containers"
+                    )
+                }
+                print(containerId)
+                return
+            }
+
             do {
-                let detach = !self.attach && !self.interactive
                 let io = try ProcessIO.create(
                     tty: container.configuration.initProcess.terminal,
                     interactive: self.interactive,

Sources/Services/ContainerAPIService/Containers/ContainersService.swift

@@ -215,6 +215,14 @@ public actor ContainersService {
         do {
             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
+                }
+
                 let runtime = state.snapshot.configuration.runtimeHandler
                 let sandboxClient = try await SandboxClient.create(
                     id: id,

Tests/CLITests/Subcommands/Run/TestCLIRunLifecycle.swift

@@ -44,4 +44,43 @@ class TestCLIRunLifecycle: CLITest {
             try self.doStop(name: name)
         }
     }
+
+    @Test func testStartIdempotent() throws {
+        let name = getTestName()
+
+        #expect(throws: Never.self, "expected container run to succeed") {
+            try self.doLongRun(name: name, args: [])
+            defer {
+                try? self.doStop(name: name)
+            }
+            try self.waitForContainerRunning(name)
+
+            let (output, _, status) = try self.run(arguments: ["start", name])
+            #expect(status == 0, "expected start to succeed on already running container")
+            #expect(output.trimmingCharacters(in: .whitespacesAndNewlines) == name, "expected output to be container name")
+
+            // Don't care about the resp, just that the container is still there and not cleaned up.
+            let _ = try inspectContainer(name)
+
+            try self.doStop(name: name)
+        }
+    }
+
+    @Test func testStartIdempotentAttachFails() throws {
+        let name = getTestName()
+
+        #expect(throws: Never.self, "expected container run to succeed") {
+            try self.doLongRun(name: name, args: [])
+            defer {
+                try? self.doStop(name: name)
+            }
+            try self.waitForContainerRunning(name)
+
+            let (_, error, status) = try self.run(arguments: ["start", "-a", name])
+            #expect(status != 0, "expected start with attach to fail on already running container")
+            #expect(error.contains("attach is currently unsupported on already running containers"), "expected error message about attach not supported")
+
+            try self.doStop(name: name)
+        }
+    }
 }