diff --git a/Sources/ContainerCommands/Container/ContainerStart.swift b/Sources/ContainerCommands/Container/ContainerStart.swift index 349bda80..6d1f17e1 100644 --- a/Sources/ContainerCommands/Container/ContainerStart.swift +++ b/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, diff --git a/Sources/Services/ContainerAPIService/Containers/ContainersService.swift b/Sources/Services/ContainerAPIService/Containers/ContainersService.swift index 1a91e31b..10af3908 100644 --- a/Sources/Services/ContainerAPIService/Containers/ContainersService.swift +++ b/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, diff --git a/Tests/CLITests/Subcommands/Run/TestCLIRunLifecycle.swift b/Tests/CLITests/Subcommands/Run/TestCLIRunLifecycle.swift index 9a095029..bef9fb83 100644 --- a/Tests/CLITests/Subcommands/Run/TestCLIRunLifecycle.swift +++ b/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) + } + } }