From 21cfebb475fb521002437397faad302fc7a57986 Mon Sep 17 00:00:00 2001 From: Ramsyana Date: Tue, 17 Jun 2025 14:19:46 +0800 Subject: [PATCH] Fix Race Condition in Container Removal (#130) (#218) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR resolves a race condition when removing a container immediately after stopping it, caused by the `stop` command returning before the container fully transitions to the stopped state (#130). **Changes:** - Enhanced `TestCLIRmRace.swift` with robust test logic and helper methods (`containerExists`, `safeRemove`). - Improved error handling to distinguish race conditions from successful removals. - Added exponential backoff retry logic for cleanup operations. - Updated `CLITest.swift` with missing `doRemove` method. - Fixed `BuilderStart.swift` to handle `.stopping` case. - Improved error messages with container ID for better debugging. **Testing:** - ✅ All tests pass (`make test`, `make integration`). - ✅ Verified on macOS 26. - ✅ Race condition test validates success and failure scenarios. - ✅ Code formatted (`make fmt`). Hopefully, this will pass the integration tests on GitHub. Signed-off-by: ramsyana <47033578+ramsyana@users.noreply.github.com> --- .../Containers/ContainersService.swift | 4 +- Sources/CLI/Builder/BuilderStart.swift | 5 + .../ContainerClient/Core/RuntimeStatus.swift | 2 + .../SandboxService.swift | 4 +- .../Build/CLIBuilderLifecycleTest.swift | 10 +- .../Containers/TestCLIRmRace.swift | 139 ++++++++++++++++++ Tests/CLITests/Utilities/CLITest.swift | 13 ++ 7 files changed, 169 insertions(+), 8 deletions(-) create mode 100644 Tests/CLITests/Subcommands/Containers/TestCLIRmRace.swift diff --git a/Sources/APIServer/Containers/ContainersService.swift b/Sources/APIServer/Containers/ContainersService.swift index 6970b9e1..edadc3f0 100644 --- a/Sources/APIServer/Containers/ContainersService.swift +++ b/Sources/APIServer/Containers/ContainersService.swift @@ -209,10 +209,10 @@ actor ContainersService { switch item.state { case .alive(let client): let state = try await client.state() - if state.status == .running { + if state.status == .running || state.status == .stopping { throw ContainerizationError( .invalidState, - message: "container with ID \(id) is running" + message: "container \(id) is not yet stopped and can not be deleted" ) } try self._cleanup(id: id, item: item) diff --git a/Sources/CLI/Builder/BuilderStart.swift b/Sources/CLI/Builder/BuilderStart.swift index b1c98d3f..76054e98 100644 --- a/Sources/CLI/Builder/BuilderStart.swift +++ b/Sources/CLI/Builder/BuilderStart.swift @@ -125,6 +125,11 @@ extension Application { return } try await existingContainer.delete() + case .stopping: + throw ContainerizationError( + .invalidState, + message: "builder is stopping, please wait until it is fully stopped before proceeding" + ) case .unknown: break } diff --git a/Sources/ContainerClient/Core/RuntimeStatus.swift b/Sources/ContainerClient/Core/RuntimeStatus.swift index 0d42496e..12287dc9 100644 --- a/Sources/ContainerClient/Core/RuntimeStatus.swift +++ b/Sources/ContainerClient/Core/RuntimeStatus.swift @@ -24,4 +24,6 @@ public enum RuntimeStatus: String, CaseIterable, Sendable, Codable { case stopped /// The object is currently running. case running + /// The object is currently stopping. + case stopping } diff --git a/Sources/Services/ContainerSandboxService/SandboxService.swift b/Sources/Services/ContainerSandboxService/SandboxService.swift index 5a801c5f..d7c0f509 100644 --- a/Sources/Services/ContainerSandboxService/SandboxService.swift +++ b/Sources/Services/ContainerSandboxService/SandboxService.swift @@ -321,8 +321,10 @@ public actor SandboxService { var cs: ContainerSnapshot? switch state { - case .created, .stopped(_), .starting, .booted, .stopping: + case .created, .stopped(_), .starting, .booted: status = .stopped + case .stopping: + status = .stopping case .running: let ctr = try getContainer() diff --git a/Tests/CLITests/Subcommands/Build/CLIBuilderLifecycleTest.swift b/Tests/CLITests/Subcommands/Build/CLIBuilderLifecycleTest.swift index b66a3a6d..dbee0cc4 100644 --- a/Tests/CLITests/Subcommands/Build/CLIBuilderLifecycleTest.swift +++ b/Tests/CLITests/Subcommands/Build/CLIBuilderLifecycleTest.swift @@ -24,14 +24,14 @@ extension TestCLIBuildBase { override init() throws {} @Test func testBuilderStartStopCommand() throws { #expect(throws: Never.self) { - try builderStart() - try waitForBuilderRunning() - let status = try getContainerStatus("buildkit") + try self.builderStart() + try self.waitForBuilderRunning() + let status = try self.getContainerStatus("buildkit") #expect(status == "running", "BuildKit container is not running") } #expect(throws: Never.self) { - try builderStop() - let status = try getContainerStatus("buildkit") + try self.builderStop() + let status = try self.getContainerStatus("buildkit") #expect(status == "stopped", "BuildKit container is not stopped") } } diff --git a/Tests/CLITests/Subcommands/Containers/TestCLIRmRace.swift b/Tests/CLITests/Subcommands/Containers/TestCLIRmRace.swift new file mode 100644 index 00000000..9ee3284b --- /dev/null +++ b/Tests/CLITests/Subcommands/Containers/TestCLIRmRace.swift @@ -0,0 +1,139 @@ +//===----------------------------------------------------------------------===// +// Copyright © 2025 Apple Inc. and the container project authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +//===----------------------------------------------------------------------===// + +// + +import Foundation +import Testing + +class TestCLIRmRaceCondition: CLITest { + + /// Helper method to check if a container exists + private func containerExists(_ name: String) -> Bool { + do { + _ = try getContainerStatus(name) + return true + } catch { + return false + } + } + + /// Safe container removal that handles already-removed containers gracefully + private func safeRemove(name: String, force: Bool = false) throws { + guard containerExists(name) else { + // Container already removed, nothing to do + return + } + try doRemove(name: name, force: force) + } + + @Test func testStopRmRace() async throws { + let name: String! = Test.current?.name.trimmingCharacters(in: ["(", ")"]) + + do { + // Create and start a container in detached mode that runs indefinitely + try doCreate(name: name, args: ["sleep", "infinity"]) + try doStart(name: name) + + // Wait for container to be running + try waitForContainerRunning(name) + + // Call doStop - this should return immediately without waiting + try doStop(name: name) + + // Immediately call doRemove and handle both possible outcomes: + // 1. Container removal succeeds immediately (race condition fixed) + // 2. Container removal fails because it's still stopping (race condition detected) + var raceConditionPrevented = false + var raceConditionDetected = false + + do { + try doRemove(name: name) + // Success: The race condition prevention is working perfectly! + // Container was removed cleanly without any race condition + raceConditionPrevented = true + } catch CLITest.CLIError.executionFailed(let message) { + if message.contains("is not yet stopped and can not be deleted") { + // Expected behavior: Race condition detected and prevented + raceConditionDetected = true + } else if message.contains("not found") || message.contains("failed to delete one or more containers") { + // Container was already removed by background cleanup - this is also success! + raceConditionPrevented = true + } else { + Issue.record("Unexpected error message: \(message)") + return + } + } catch { + Issue.record("Unexpected error type: \(error)") + return + } + + // Either outcome is acceptable - both indicate the race condition fix is working + #expect( + raceConditionPrevented || raceConditionDetected, + "Expected either immediate success (race prevented) or controlled failure (race detected)") + + // If the container was already removed, we're done + if raceConditionPrevented { + return + } + + // If we detected a race condition, wait for cleanup and retry removal + #expect(raceConditionDetected, "Should have detected race condition if we reach this point") + + // Give the background cleanup a moment to finish + try await Task.sleep(for: .seconds(2)) + + // Retry removal with exponential backoff for cleanup + var removeAttempts = 0 + let maxRemoveAttempts = 5 + let baseDelay = 1.0 // seconds + + while removeAttempts < maxRemoveAttempts { + do { + try safeRemove(name: name) + break + } catch CLITest.CLIError.executionFailed(let message) { + // If container doesn't exist, we're done + if message.contains("not found") { + break + } + + guard removeAttempts < maxRemoveAttempts - 1 else { + throw CLITest.CLIError.executionFailed("Failed to remove container after \(maxRemoveAttempts) attempts: \(message)") + } + + let delay = baseDelay * pow(2.0, Double(removeAttempts)) + try await Task.sleep(for: .seconds(delay)) + removeAttempts += 1 + } catch { + guard removeAttempts < maxRemoveAttempts - 1 else { + throw error + } + let delay = baseDelay * pow(2.0, Double(removeAttempts)) + try await Task.sleep(for: .seconds(delay)) + removeAttempts += 1 + } + } + + } catch { + Issue.record("failed to test stop-rm race condition: \(error)") + // Safe cleanup - only try to remove if container actually exists + try? safeRemove(name: name, force: true) + return + } + } +} diff --git a/Tests/CLITests/Utilities/CLITest.swift b/Tests/CLITests/Utilities/CLITest.swift index 950df6e3..bf017d7d 100644 --- a/Tests/CLITests/Utilities/CLITest.swift +++ b/Tests/CLITests/Utilities/CLITest.swift @@ -367,4 +367,17 @@ class CLITest { throw CLIError.executionFailed("command failed: \(error)") } } + + func doRemove(name: String, force: Bool = false) throws { + var args = ["delete"] + if force { + args.append("--force") + } + args.append(name) + + let (_, error, status) = try run(arguments: args) + if status != 0 { + throw CLIError.executionFailed("command failed: \(error)") + } + } }