From b1d37b9217ace0119fff42440918679a1ef3ce41 Mon Sep 17 00:00:00 2001 From: Raj Date: Mon, 3 Nov 2025 14:43:36 -0800 Subject: [PATCH] clean up anonymous volumes when --rm containers exit --- Sources/ContainerClient/Core/Volume.swift | 8 ++ Sources/ContainerClient/Utility.swift | 9 +- .../Containers/ContainersService.swift | 64 +++++++++- .../Volumes/TestCLIAnonymousVolumes.swift | 112 ++++++++++-------- docs/command-reference.md | 2 +- 5 files changed, 135 insertions(+), 60 deletions(-) diff --git a/Sources/ContainerClient/Core/Volume.swift b/Sources/ContainerClient/Core/Volume.swift index 08da7e03..a6a7bdf2 100644 --- a/Sources/ContainerClient/Core/Volume.swift +++ b/Sources/ContainerClient/Core/Volume.swift @@ -62,10 +62,18 @@ extension Volume { /// Reserved label key for marking anonymous volumes public static let anonymousLabel = "com.apple.container.resource.anonymous" + /// Reserved label key for storing the container ID that created this volume + public static let createdByLabel = "com.apple.container.resource.created-by" + /// Whether this is an anonymous volume (detected via label) public var isAnonymous: Bool { labels[Self.anonymousLabel] != nil } + + /// The container ID that created this volume + public var createdByContainerID: String? { + labels[Self.createdByLabel] + } } /// Error types for volume operations. diff --git a/Sources/ContainerClient/Utility.swift b/Sources/ContainerClient/Utility.swift index ed4fc7e7..1a5e9055 100644 --- a/Sources/ContainerClient/Utility.swift +++ b/Sources/ContainerClient/Utility.swift @@ -168,7 +168,7 @@ public struct Utility { case .filesystem(let fs): resolvedMounts.append(fs) case .volume(let parsed): - let volume = try await getOrCreateVolume(parsed: parsed) + let volume = try await getOrCreateVolume(parsed: parsed, containerID: id) let volumeMount = Filesystem.volume( name: parsed.name, format: volume.format, @@ -315,8 +315,11 @@ public struct Utility { /// Gets an existing volume or creates it if it doesn't exist. /// Shows a warning for named volumes when auto-creating. - private static func getOrCreateVolume(parsed: ParsedVolume) async throws -> Volume { - let labels = parsed.isAnonymous ? [Volume.anonymousLabel: ""] : [:] + private static func getOrCreateVolume(parsed: ParsedVolume, containerID: String) async throws -> Volume { + var labels = parsed.isAnonymous ? [Volume.anonymousLabel: ""] : [:] + if parsed.isAnonymous { + labels[Volume.createdByLabel] = containerID + } let volume: Volume do { diff --git a/Sources/Services/ContainerAPIService/Containers/ContainersService.swift b/Sources/Services/ContainerAPIService/Containers/ContainersService.swift index 8f1b7694..2592fa3e 100644 --- a/Sources/Services/ContainerAPIService/Containers/ContainersService.swift +++ b/Sources/Services/ContainerAPIService/Containers/ContainersService.swift @@ -434,12 +434,17 @@ public actor ContainersService { } private func handleContainerExit(id: String, code: ExitStatus? = nil) async throws { - try await self.lock.withLock { [self] context in + let shouldCleanupVolumes = try await self.lock.withLock { [self] context in try await handleContainerExit(id: id, code: code, context: context) } + + // Lock released, safe for XPC calls + if shouldCleanupVolumes { + await self.cleanupAnonymousVolumes(forContainer: id) + } } - private func handleContainerExit(id: String, code: ExitStatus?, context: AsyncLock.Context) async throws { + private func handleContainerExit(id: String, code: ExitStatus?, context: AsyncLock.Context) async throws -> Bool { if let code { self.log.info("Handling container \(id) exit. Code \(code)") } @@ -448,11 +453,11 @@ public actor ContainersService { do { state = try self.getContainerState(id: id, context: context) if state.snapshot.status == .stopped { - return + return false } } catch { // Was auto removed by the background thread, nothing for us to do. - return + return false } await self.exitMonitor.stopTracking(id: id) @@ -475,7 +480,9 @@ public actor ContainersService { let options = try getContainerCreationOptions(id: id) if options.autoRemove { try await self.cleanup(id: id, context: context) + return true } + return false } private static func fullLaunchdServiceLabel(runtimeName: String, instanceId: String) -> String { @@ -511,6 +518,55 @@ public actor ContainersService { try await self._cleanup(id: id) } + /// Clean up anonymous volumes created by a container + private func cleanupAnonymousVolumes(forContainer id: String) async { + do { + let allVolumes = try await ClientVolume.list() + let anonymousVolumes = allVolumes.filter { $0.isAnonymous && $0.createdByContainerID == id } + + guard !anonymousVolumes.isEmpty else { return } + + await withTaskGroup(of: (String, Error?).self) { group in + for volume in anonymousVolumes { + group.addTask { + do { + try await ClientVolume.delete(name: volume.name) + return (volume.name, nil) + } catch { + return (volume.name, error) + } + } + } + + for await (volumeName, error) in group { + if let error = error { + self.log.warning( + "Failed to delete anonymous volume", + metadata: [ + "volume": "\(volumeName)", + "container": "\(id)", + "error": "\(error)", + ]) + } else { + self.log.info( + "Deleted anonymous volume", + metadata: [ + "volume": "\(volumeName)", + "container": "\(id)", + ]) + } + } + } + } catch { + self.log.error( + "Failed to query anonymous volumes for cleanup", + metadata: [ + "container": "\(id)", + "error": "\(error)", + ]) + } + } + private func getContainerCreationOptions(id: String) throws -> ContainerCreateOptions { let path = self.containerRoot.appendingPathComponent(id) let bundle = ContainerClient.Bundle(path: path) diff --git a/Tests/CLITests/Subcommands/Volumes/TestCLIAnonymousVolumes.swift b/Tests/CLITests/Subcommands/Volumes/TestCLIAnonymousVolumes.swift index fd21af6a..c868644e 100644 --- a/Tests/CLITests/Subcommands/Volumes/TestCLIAnonymousVolumes.swift +++ b/Tests/CLITests/Subcommands/Volumes/TestCLIAnonymousVolumes.swift @@ -99,13 +99,13 @@ class TestCLIAnonymousVolumes: CLITest { let (_, _, _) = (try? run(arguments: args)) ?? ("", "", 1) } - @Test func testAnonymousVolumeCreationAndPersistence() async throws { + @Test func testAnonymousVolumeCreationAndAutoCleanup() async throws { let testName = getTestName() let containerName = "\(testName)_c1" defer { doRemoveIfExists(name: containerName, force: true) - // Clean up anonymous volumes + // Clean up any remaining anonymous volumes if let volumes = try? getAnonymousVolumeNames() { volumes.forEach { doVolumeDeleteIfExists(name: $0) } } @@ -129,7 +129,7 @@ class TestCLIAnonymousVolumes: CLITest { #expect(status == 0, "container run should succeed") - // Give time for container removal to complete + // Give time for cleanup to complete try await Task.sleep(for: .seconds(1)) // Verify container was removed @@ -138,9 +138,9 @@ class TestCLIAnonymousVolumes: CLITest { .filter { $0.contains(containerName) } #expect(containers.isEmpty, "container should be removed with --rm") - // Verify anonymous volume persists (no auto-cleanup) + // Verify anonymous volume was auto-cleaned up let afterCount = try getAnonymousVolumeNames().count - #expect(afterCount == beforeCount + 1, "anonymous volume should persist even with --rm") + #expect(afterCount == beforeCount, "anonymous volume should be auto-cleaned up with --rm") } @Test func testAnonymousVolumePersistenceWithoutRm() throws { @@ -191,43 +191,6 @@ class TestCLIAnonymousVolumes: CLITest { doVolumeDeleteIfExists(name: volumeID) } - @Test func testMultipleAnonymousVolumes() async throws { - let testName = getTestName() - let containerName = "\(testName)_c1" - - defer { - doRemoveIfExists(name: containerName, force: true) - // Clean up anonymous volumes - if let volumes = try? getAnonymousVolumeNames() { - volumes.forEach { doVolumeDeleteIfExists(name: $0) } - } - } - - let beforeCount = try getAnonymousVolumeNames().count - - // Run with multiple anonymous volumes - let (_, _, status) = try run(arguments: [ - "run", - "--rm", - "--name", - containerName, - "-v", "/data1", - "-v", "/data2", - "-v", "/data3", - alpine, - "sh", "-c", "ls -d /data*", - ]) - - #expect(status == 0, "container run should succeed") - - // Give time for container removal - try await Task.sleep(for: .seconds(1)) - - // All 3 volumes should persist (no auto-cleanup) - let afterCount = try getAnonymousVolumeNames().count - #expect(afterCount == beforeCount + 3, "all 3 anonymous volumes should persist") - } - @Test func testAnonymousMountSyntax() async throws { let testName = getTestName() let containerName = "\(testName)_c1" @@ -255,12 +218,12 @@ class TestCLIAnonymousVolumes: CLITest { #expect(status == 0, "container run with --mount should succeed") - // Give time for container removal - try await Task.sleep(for: .seconds(1)) + // Give time for container removal and volume cleanup + try await Task.sleep(for: .seconds(2)) - // Anonymous volume should persist (no auto-cleanup) + // Anonymous volume should be auto-cleaned up with --rm let afterCount = try getAnonymousVolumeNames().count - #expect(afterCount == beforeCount + 1, "anonymous volume should persist") + #expect(afterCount == beforeCount, "anonymous volume should be auto-cleaned up with --rm") } @Test func testAnonymousVolumeUUIDFormat() throws { @@ -394,15 +357,15 @@ class TestCLIAnonymousVolumes: CLITest { #expect(status == 0, "container run should succeed") - // Give time for container removal - try await Task.sleep(for: .seconds(1)) + // Give time for container removal and volume cleanup + try await Task.sleep(for: .seconds(2)) // Named volume should still exist let namedExists = try volumeExists(name: namedVolumeName) #expect(namedExists, "named volume should persist") let afterAnonCount = try getAnonymousVolumeNames().count - #expect(afterAnonCount == beforeAnonCount + 1, "anonymous volume should persist") + #expect(afterAnonCount == beforeAnonCount, "anonymous volume should be auto-cleaned up with --rm") } @Test func testAnonymousVolumeManualDeletion() throws { @@ -463,8 +426,8 @@ class TestCLIAnonymousVolumes: CLITest { #expect(status == 0, "detached container run should succeed") - // Wait for container to exit - try await Task.sleep(for: .seconds(3)) + // Wait for container to exit and volume cleanup + try await Task.sleep(for: .seconds(4)) // Container should be removed let (lsOutput, _, _) = try run(arguments: ["ls", "-a"]) @@ -473,6 +436,51 @@ class TestCLIAnonymousVolumes: CLITest { #expect(containers.isEmpty, "container should be auto-removed") let afterCount = try getAnonymousVolumeNames().count - #expect(afterCount == beforeCount + 1, "anonymous volume should persist") + #expect(afterCount == beforeCount, "anonymous volume should be auto-cleaned up with --rm") + } + + @Test func testMultipleAnonymousVolumesAutoCleanupWithRm() async throws { + let testName = getTestName() + let containerName = "\(testName)_c1" + + defer { + doRemoveIfExists(name: containerName, force: true) + // Clean up any remaining anonymous volumes + if let volumes = try? getAnonymousVolumeNames() { + volumes.forEach { doVolumeDeleteIfExists(name: $0) } + } + } + + let beforeCount = try getAnonymousVolumeNames().count + + // Run container with --rm and 5 anonymous volumes + let (_, _, status) = try run(arguments: [ + "run", + "--rm", + "--name", + containerName, + "-v", "/data1", + "-v", "/data2", + "-v", "/data3", + "-v", "/data4", + "-v", "/data5", + alpine, + "sh", "-c", "echo 'test' > /data1/file.txt && cat /data1/file.txt", + ]) + + #expect(status == 0, "container run should succeed") + + // Wait for cleanup to complete + try await Task.sleep(for: .seconds(2)) + + // Verify container was removed + let (lsOutput, _, _) = try run(arguments: ["ls", "-a"]) + let containers = lsOutput.components(separatedBy: .newlines) + .filter { $0.contains(containerName) } + #expect(containers.isEmpty, "container should be removed with --rm") + + // Verify all 5 anon volumes were cleaned up + let afterCount = try getAnonymousVolumeNames().count + #expect(afterCount == beforeCount, "all 5 anonymous volumes should be auto-cleaned up with --rm") } } diff --git a/docs/command-reference.md b/docs/command-reference.md index 68fda381..7c42570b 100644 --- a/docs/command-reference.md +++ b/docs/command-reference.md @@ -711,7 +711,7 @@ container run -v $VOL:/data alpine container volume rm $VOL ``` -**Note**: Unlike Docker, anonymous volumes do NOT auto-cleanup with `--rm`. Manual deletion is required. +**Note**: Anonymous volumes auto-cleanup when containers exit with `--rm` flag. Without `--rm`, they persist and require manual deletion. ### `container volume delete (rm)`