fix(commands): Fix indefinite hang on container --help / help / no-args (#8)

* Document the container --help freeze investigation

Adds docs/internal/help-freeze-analysis.md describing the two defects that
combine to produce an indefinite hang on `container --help` when
`com.apple.container.apiserver` is dead, wedged, or stale-registered in
launchd:

  A. The help path requires the daemon to be reachable, because
     `Application.main` calls `createPluginLoader()` (which pings the
     API server) before printing help.

  B. `XPCClient.send`'s timeout cannot actually unblock the function:
     the structured TaskGroup must await the XPC child task, which is
     suspended in a `withCheckedThrowingContinuation` that only resumes
     when the C callback fires.

The document is intended to be reviewed alongside the two follow-up
commits that implement the fixes.

* Skip API server ping for help and no-args paths

Help rendering must not depend on `com.apple.container.apiserver` being
reachable. When the API server is dead, wedged, or stale-registered in
launchd, the previous behavior was an indefinite hang on:

  container --help
  container help
  container

All three paths called `Application.createPluginLoader()` (which pings
the API server to fetch `appRoot`/`installRoot`/`logRoot`) just to
enrich the help text with plugin commands. The ping is structurally
unnecessary for help: `PluginLoader.alterCLIHelpText` only reads
`pluginDirectories` and `pluginFactories`.

This commit removes the call from each help path and extends
`printModifiedHelpText` with an optional `unavailableMessage:` so that
contexts which deliberately skipped plugin loading do not print the
misleading 'PLUGINS: not available, run `container system start`'
notice. `DefaultCommand` is reordered so the API server is contacted
only when there is a plugin command to dispatch.

Plugin enrichment in help output is removed by this commit. A follow-up
can restore it by extracting filesystem-only plugin discovery from
`PluginLoader.findPlugins` (see docs/internal/help-freeze-analysis.md
for the proposed shape).

Verified by running each path with no apiserver running on macOS 26:
all three return immediately with exit 0 and the original
`OVERVIEW: A container platform for macOS` block.

* Make XPCClient.send timeout actually cancel pending replies

The previous implementation raced `Task.sleep` against the XPC reply
inside a `withThrowingTaskGroup`. When the timeout won, structured
concurrency required the group to await the XPC child task before the
group scope could return — but that child was suspended in a
`withCheckedThrowingContinuation` that only resumes when the C
`xpc_connection_send_message_with_reply` callback fires. Cancelling a
Swift Task does not cancel the underlying C call. If the remote service
was wedged (no reply, no connection invalidation), the child never
resumed and the group never returned, regardless of the supplied
`responseTimeout`. The `responseTimeout` parameter was therefore
silently ineffective in exactly the failure mode it was meant to
mitigate.

This commit replaces the TaskGroup with a single-resume gate
(`ResumptionState`) over a `CheckedContinuation` wrapped in a
`withTaskCancellationHandler`. The continuation is resumed by whichever
of the following completes first:

  1. The XPC reply callback fires.
  2. `responseTimeout` elapses.
  3. The current Task is cancelled.

Late completions from the other paths are dropped silently, so the
underlying XPC connection remains valid for subsequent sends. This is
required for callers that hold a long-lived `XPCClient`
(`ContainerClient`, `NetworkClient`); a simpler design that called
`xpc_connection_cancel` on timeout would brick those clients after a
single timed-out send.

Tradeoffs documented in docs/internal/help-freeze-analysis.md:
  - On timeout/cancel, the eventual late XPC reply is retained by XPC
    until the connection is released. For short-lived clients this is
    GC'd within milliseconds; for long-lived reusable clients the worst
    case is one orphaned `xpc_object_t` per timed-out send.
  - The unstructured `Task` that runs the timeout sleep is not
    cancelled when the parent task is cancelled; it wakes up later and
    becomes a no-op via `tryResume`.

Reviewers: a unit test that injects a connection with a non-firing reply
would meaningfully cover both the timeout path and the reusable-client
guarantee. Happy to add it in this PR or as a follow-up — preference?

* Encode mutating-safe contract in XPCClient.send

The Codex adversarial review of this branch flagged that
`XPCClient.send(_:responseTimeout:)` can drop a late XPC reply after
its timeout fires. For idempotent reads (`ClientHealthCheck.ping`,
`list` operations) that is a deliberate tradeoff: the connection
remains valid for subsequent sends and the next caller can re-issue
the request. For mutating operations the same behavior is unsafe:
the caller surfaces `.timeout`, the user retries, and the original
operation may still commit on the server — duplicate or out-of-order
container/network state under any slow-but-not-dead daemon.

An independent audit of the call sites contradicted the operator note
written when the freeze fix was first proposed. Four mutating call
sites were already reaching the unsafe path:

  - ContainerClient.create (containerCreate, 60s default via xpcSend)
  - NetworkClient.create   (networkCreate,  60s default via xpcSend)
  - NetworkClient.delete   (networkDelete,  60s default via xpcSend)
  - SandboxClient.create   (sandboxCreateEndpoint, 60s timeout: param)

This commit removes that footgun at the API surface so future call
sites cannot reach for it by accident:

  send(_:)                                 -- mutating-safe; no timeout
  send(_:timeoutForIdempotentRequest:)     -- explicit; late-reply drop
                                              acknowledged at call site

The old `responseTimeout:` spelling is retained as
`@available(*, unavailable, ...)` so any reintroduction in a future
patch fails to build with a teaching error pointing at the two
overloads.

Cancellation contract:

  - send(_:) checks Task.isCancelled before dispatch via
    Task.checkCancellation(); after dispatch, cancellation is ignored
    and the call completes only when the daemon replies or the
    underlying connection is invalidated. Honoring cancellation after
    dispatch would re-introduce the same late-commit ambiguity as a
    timeout.
  - send(_:timeoutForIdempotentRequest:) keeps the existing
    reply/timeout/cancellation race semantics, with late replies
    dropped silently so reusable clients keep working.

Call-site migrations:

  - ContainerClient gains an `xpcSendIdempotent(message:timeout:)`
    helper. `create` uses the no-timeout `xpcSend(message:)`; `list`
    uses the idempotent helper with its existing 10s bound.
  - NetworkClient (APIService) follows the same split: `create` and
    `delete` use the no-timeout helper; `list` keeps its 1s bound via
    the idempotent helper.
  - SandboxClient.create drops its `timeout:` parameter; the only
    caller (ContainersService) was already passing the default.
  - ClientHealthCheck.ping calls the idempotent overload with a
    non-optional Duration. All seven ping callers in ContainerCommands
    are unchanged at the call site.

Tests: a new ContainerXPCTests target uses an in-process
`xpc_endpoint_create`-based listener so the contract can be exercised
without a live mach service. Six tests cover both overloads:

  - idempotentTimeoutReturnsWithinBound — verifies the .timeout error
    code (not .interrupted) and that elapsed time is within the
    expected window
  - reusableClientSurvivesIdempotentTimeout — same XPCClient instance
    survives a timeout and can complete a follow-up send
  - lateReplyAfterIdempotentTimeoutIsIgnoredCleanly — server replies
    after the client has timed out; subsequent send still works
  - plainSendCompletesWhenServerReplies — happy path
  - plainSendIgnoresCancellationAfterDispatch — Task.cancel() after
    dispatch must NOT short-circuit; the task waits for the reply
  - plainSendHonorsCancellationBeforeDispatch — pre-dispatch
    cancellation surfaces CancellationError

What this commit does not address:

  - No idempotency token or recovery query (Codex's third suggestion).
    This commit prevents the unsafe combination at the API; it does not
    give callers a way to safely time out a mutating request and then
    ask the daemon "did it actually commit?".
  - Reusable ContainerClient/NetworkClient mutating calls now have no
    timeout (correctly so, per the new contract). Wedged-daemon
    scenarios will hang those callers indefinitely; the user-visible
    workaround (`launchctl bootout`) remains the only escape today.

Both are reasonable follow-ups but out of scope for closing the freeze
regression.

* Document codex review and mutating-safe send follow-up

Adds docs/internal/codex-reviews.md capturing the verbatim output of
the two `codex` plugin reviews run against this branch:

  - /codex:review                — pass
  - /codex:adversarial-review    — needs-attention

The adversarial pass surfaced a high-severity design concern about
late XPC replies for mutating requests under a slow-but-not-dead
daemon. The doc preserves the Codex output for the record and adds a
follow-up section that:

  - Corrects the operator note's claim that no mutating call site used
    `responseTimeout` today. Independent verification surfaced four
    live mutating-with-timeout call sites (ContainerClient.create,
    NetworkClient.create, NetworkClient.delete, SandboxClient.create).

  - Records the decision to implement Codex's first suggestion
    (restrict `responseTimeout` to idempotent operations) at the
    XPCClient.send API surface, with the design rationale.

  - Documents the new API shape, the cancellation contract for both
    overloads, and the call-site migrations.

  - Lists the test coverage added in the new ContainerXPCTests target.

  - Notes the two issues the follow-up still does not address
    (idempotency tokens; wedged-daemon hang protection for reusable
    clients) and explains why they are out of scope for closing the
    freeze regression.

The implementation referenced in this doc landed in the prior commit
on this branch.

Author: chrisgeo

Package.swift

@@ -425,6 +425,12 @@ let package = Package(
                 "CAuditToken",
             ]
         ),
+        .testTarget(
+            name: "ContainerXPCTests",
+            dependencies: [
+                "ContainerXPC"
+            ]
+        ),
         .target(
             name: "ContainerOS",
             dependencies: [

Sources/ContainerCommands/Application.swift

@@ -118,10 +118,13 @@ public struct Application: AsyncLoggableCommand {
         } catch {
             // --help/-h on the root command (e.g. `container --help`) is intercepted
             // by ArgumentParser and lands here.
+            //
+            // Help rendering must not depend on the API server being reachable, otherwise
+            // a wedged or unregistered `com.apple.container.apiserver` causes an
+            // indefinite hang. See docs/internal/help-freeze-analysis.md.
             let containsHelp = fullArgs.contains("-h") || fullArgs.contains("--help")
             if fullArgs.count <= 2 && containsHelp {
-                let pluginLoader = try? await createPluginLoader()
-                await Self.printModifiedHelpText(pluginLoader: pluginLoader)
+                await Self.printModifiedHelpText(pluginLoader: nil, unavailableMessage: nil)
                 return
             }
             let errorAsString: String = String(describing: error)
@@ -234,11 +237,21 @@ public struct Application: AsyncLoggableCommand {
 extension Application {
     // Because we support plugins, we need to modify the help text to display
     // any if we found some.
-    static func printModifiedHelpText(pluginLoader: PluginLoader?) async {
+    //
+    // Pass `unavailableMessage: nil` from contexts that already deliberately skipped
+    // plugin loading (e.g. the `--help` / `help` / no-args paths that must not depend
+    // on the API server). The default preserves prior behavior for paths that *did*
+    // attempt to load plugins but couldn't reach the server.
+    static func printModifiedHelpText(
+        pluginLoader: PluginLoader?,
+        unavailableMessage: String? = "PLUGINS: not available, run `container system start`"
+    ) async {
         let original = Application.helpMessage(for: Application.self)
         guard let pluginLoader else {
             print(original)
-            print("PLUGINS: not available, run `container system start`")
+            if let unavailableMessage {
+                print(unavailableMessage)
+            }
             return
         }
         let altered = pluginLoader.alterCLIHelpText(original: original)

Sources/ContainerCommands/DefaultCommand.swift

@@ -33,13 +33,17 @@ struct DefaultCommand: AsyncLoggableCommand {
     var remaining: [String] = []
 
     func run() async throws {
-        // See if we have a possible plugin command.
-        let pluginLoader = try? await Application.createPluginLoader()
+        // No-args invocation prints help and must not depend on the API server.
+        // See docs/internal/help-freeze-analysis.md.
         guard let command = remaining.first else {
-            await Application.printModifiedHelpText(pluginLoader: pluginLoader)
+            await Application.printModifiedHelpText(pluginLoader: nil, unavailableMessage: nil)
             return
         }
 
+        // We have a candidate plugin command; load plugins (which contacts the
+        // API server) only on this path.
+        let pluginLoader = try? await Application.createPluginLoader()
+
         // Check for edge cases and unknown options to match the behavior in the absence of plugins.
         if command.isEmpty {
             throw ValidationError("unknown argument '\(command)'")

Sources/ContainerCommands/HelpCommand.swift

@@ -27,7 +27,8 @@ struct HelpCommand: AsyncLoggableCommand {
     public var logOptions: Flags.Logging
 
     func run() async throws {
-        let pluginLoader = try? await Application.createPluginLoader()
-        await Application.printModifiedHelpText(pluginLoader: pluginLoader)
+        // The `help` subcommand must not depend on the API server being reachable.
+        // See docs/internal/help-freeze-analysis.md.
+        await Application.printModifiedHelpText(pluginLoader: nil, unavailableMessage: nil)
     }
 }

Sources/ContainerXPC/XPCClient.swift

@@ -69,48 +69,121 @@ extension XPCClient {
         xpc_connection_get_pid(self.connection)
     }
 
-    /// Send the provided message to the service.
+    /// Send the provided message to the service and wait for the reply.
+    ///
+    /// Use this overload for **mutating or unknown-safety** requests. It has no
+    /// response timeout: once the message has been dispatched, this function
+    /// blocks until the XPC service replies or the underlying connection is
+    /// invalidated.
+    ///
+    /// Cancellation is honored only **before dispatch**
+    /// (`Task.checkCancellation()`). After the message is sent, `Task`
+    /// cancellation does not resume the caller, because doing so would let the
+    /// server commit a mutation after the caller had already given up — the
+    /// classic late-reply race that creates duplicate or out-of-order state.
+    ///
+    /// For read-only or idempotent requests where dropping a late reply is
+    /// acceptable, use ``send(_:timeoutForIdempotentRequest:)`` instead.
+    ///
+    /// See `docs/internal/codex-reviews.md` and
+    /// `docs/internal/help-freeze-analysis.md` for the full design rationale.
     @discardableResult
-    public func send(_ message: XPCMessage, responseTimeout: Duration? = nil) async throws -> XPCMessage {
-        try await withThrowingTaskGroup(of: XPCMessage.self, returning: XPCMessage.self) { group in
-            if let responseTimeout {
-                group.addTask {
-                    try await Task.sleep(for: responseTimeout)
-                    let route = message.string(key: XPCMessage.routeKey) ?? "nil"
-                    throw ContainerizationError(
-                        .internalError,
-                        message: "XPC timeout for request to \(self.service)/\(route)"
-                    )
+    public func send(_ message: XPCMessage) async throws -> XPCMessage {
+        try Task.checkCancellation()
+        return try await withCheckedThrowingContinuation { (cont: CheckedContinuation<XPCMessage, Error>) in
+            xpc_connection_send_message_with_reply(self.connection, message.underlying, nil) { reply in
+                do {
+                    cont.resume(returning: try self.parseReply(reply))
+                } catch {
+                    cont.resume(throwing: error)
                 }
             }
+        }
+    }
 
-            group.addTask {
-                try await withCheckedThrowingContinuation { cont in
-                    xpc_connection_send_message_with_reply(self.connection, message.underlying, nil) { reply in
-                        do {
-                            let message = try self.parseReply(reply)
-                            cont.resume(returning: message)
-                        } catch {
-                            cont.resume(throwing: error)
-                        }
+    /// Send the provided message to the service with an idempotency-safe
+    /// timeout.
+    ///
+    /// The response is delivered by whichever of the following completes first:
+    ///   1. The XPC reply callback fires.
+    ///   2. `responseTimeout` elapses.
+    ///   3. The current `Task` is cancelled.
+    ///
+    /// **Use this overload only for read-only or idempotent requests.** When
+    /// timeout or cancellation wins the race, the underlying XPC reply callback
+    /// may still fire later and the server may still complete the operation.
+    /// Late completions are dropped silently so the connection remains valid
+    /// for subsequent sends — important for callers that hold a long-lived
+    /// `XPCClient` (`ContainerClient`, `NetworkClient`). For mutating requests
+    /// this would create duplicate or out-of-order server-side state; use
+    /// ``send(_:)`` instead.
+    ///
+    /// A previous implementation used a `withThrowingTaskGroup`, but
+    /// structured-concurrency cleanup awaited the XPC child task, which could
+    /// not actually be cancelled because
+    /// `xpc_connection_send_message_with_reply` only resumes when its callback
+    /// fires. That made the timeout ineffective whenever the remote service was
+    /// wedged. See docs/internal/help-freeze-analysis.md.
+    @discardableResult
+    public func send(
+        _ message: XPCMessage,
+        timeoutForIdempotentRequest responseTimeout: Duration
+    ) async throws -> XPCMessage {
+        let state = ResumptionState<XPCMessage>()
+        return try await withTaskCancellationHandler {
+            try await withCheckedThrowingContinuation { (cont: CheckedContinuation<XPCMessage, Error>) in
+                state.set(cont)
+
+                xpc_connection_send_message_with_reply(self.connection, message.underlying, nil) { reply in
+                    do {
+                        let parsed = try self.parseReply(reply)
+                        state.tryResume(returning: parsed)
+                    } catch {
+                        state.tryResume(throwing: error)
                     }
                 }
-            }
 
-            let response = try await group.next()
-            // once one task has finished, cancel the rest.
-            group.cancelAll()
-            // we don't really care about the second error here
-            // as it's most likely a `CancellationError`.
-            try? await group.waitForAll()
+                let service = self.service
+                let route = message.string(key: XPCMessage.routeKey) ?? "nil"
+                Task { [state] in
+                    try? await Task.sleep(for: responseTimeout)
+                    state.tryResume(
+                        throwing: ContainerizationError(
+                            .timeout,
+                            message: "XPC timeout for request to \(service)/\(route)"
+                        )
+                    )
+                }
 
-            guard let response else {
-                throw ContainerizationError(.invalidState, message: "failed to receive XPC response")
+                // Close the race window: if cancellation arrived before `set(cont)`
+                // ran, the cancellation handler resumed against an empty state. Resume
+                // here so the continuation cannot be lost.
+                if Task.isCancelled {
+                    state.tryResume(throwing: CancellationError())
+                }
             }
-            return response
+        } onCancel: {
+            state.tryResume(throwing: CancellationError())
         }
     }
 
+    /// Compile-time guard against the previous footgun spelling.
+    ///
+    /// The previous API allowed any caller to pass `responseTimeout:` regardless
+    /// of whether the request mutated server-side state. When the timeout fired
+    /// against a mutating request, the server could still commit the operation
+    /// while the caller had already given up — the late-reply race documented in
+    /// `docs/internal/codex-reviews.md`.
+    ///
+    /// This unavailable shim keeps the old call shape compiling-as-error so
+    /// callers are forced to choose either ``send(_:)`` for mutating requests
+    /// or ``send(_:timeoutForIdempotentRequest:)`` for idempotent ones.
+    @available(*, unavailable, message: "responseTimeout may drop late replies. Use send(_:) for mutating requests, or send(_:timeoutForIdempotentRequest:) only for idempotent/read-only requests.")
+    @discardableResult
+    public func send(_ message: XPCMessage, responseTimeout: Duration?) async throws -> XPCMessage {
+        fatalError("unavailable")
+    }
+
     private func parseReply(_ reply: xpc_object_t) throws -> XPCMessage {
         switch xpc_get_type(reply) {
         case XPC_TYPE_ERROR:
@@ -133,4 +206,36 @@ extension XPCClient {
     }
 }
 
+/// Single-resume gate around a `CheckedContinuation`.
+///
+/// `XPCClient.send` races multiple completion sources (XPC reply callback, timeout
+/// sleep, parent-task cancellation) against a single continuation. Whichever wins
+/// resumes via `tryResume`; subsequent resumes are dropped silently.
+private final class ResumptionState<T: Sendable>: @unchecked Sendable {
+    private let lock = NSLock()
+    private var continuation: CheckedContinuation<T, Error>?
+
+    func set(_ continuation: CheckedContinuation<T, Error>) {
+        lock.lock()
+        defer { lock.unlock() }
+        self.continuation = continuation
+    }
+
+    func tryResume(returning value: T) {
+        lock.lock()
+        let c = continuation
+        continuation = nil
+        lock.unlock()
+        c?.resume(returning: value)
+    }
+
+    func tryResume(throwing error: Error) {
+        lock.lock()
+        let c = continuation
+        continuation = nil
+        lock.unlock()
+        c?.resume(throwing: error)
+    }
+}
+
 #endif

Sources/Services/ContainerAPIService/Client/ClientHealthCheck.swift

@@ -28,10 +28,10 @@ extension ClientHealthCheck {
         XPCClient(service: serviceIdentifier)
     }
 
-    public static func ping(timeout: Duration? = XPCClient.xpcRegistrationTimeout) async throws -> SystemHealth {
+    public static func ping(timeout: Duration = XPCClient.xpcRegistrationTimeout) async throws -> SystemHealth {
         let client = Self.newClient()
         let request = XPCMessage(route: .ping)
-        let reply = try await client.send(request, responseTimeout: timeout)
+        let reply = try await client.send(request, timeoutForIdempotentRequest: timeout)
         guard let appRootValue = reply.string(key: .appRoot), let appRoot = URL(string: appRootValue) else {
             throw ContainerizationError(.internalError, message: "failed to decode appRoot in health check")
         }

Sources/Services/ContainerAPIService/Client/ContainerClient.swift

@@ -37,11 +37,16 @@ public struct ContainerClient: Sendable {
     }
 
     @discardableResult
-    private func xpcSend(
+    private func xpcSend(message: XPCMessage) async throws -> XPCMessage {
+        try await xpcClient.send(message)
+    }
+
+    @discardableResult
+    private func xpcSendIdempotent(
         message: XPCMessage,
-        timeout: Duration? = XPCClient.xpcRegistrationTimeout
+        timeout: Duration
     ) async throws -> XPCMessage {
-        try await xpcClient.send(message, responseTimeout: timeout)
+        try await xpcClient.send(message, timeoutForIdempotentRequest: timeout)
     }
 
     /// Create a new container with the given configuration.
@@ -82,7 +87,7 @@ public struct ContainerClient: Sendable {
             let filterData = try JSONEncoder().encode(filters)
             request.set(key: .listFilters, value: filterData)
 
-            let response = try await xpcSend(
+            let response = try await xpcSendIdempotent(
                 message: request,
                 timeout: .seconds(10)
             )

Sources/Services/ContainerAPIService/Client/NetworkClient.swift

@@ -57,11 +57,16 @@ public struct NetworkClient: Sendable {
     }
 
     @discardableResult
-    private func xpcSend(
+    private func xpcSend(message: XPCMessage) async throws -> XPCMessage {
+        try await xpcClient.send(message)
+    }
+
+    @discardableResult
+    private func xpcSendIdempotent(
         message: XPCMessage,
-        timeout: Duration? = XPCClient.xpcRegistrationTimeout
+        timeout: Duration
     ) async throws -> XPCMessage {
-        try await xpcClient.send(message, responseTimeout: timeout)
+        try await xpcClient.send(message, timeoutForIdempotentRequest: timeout)
     }
 
     /// Creates a new network with the given configuration.
@@ -97,7 +102,7 @@ public struct NetworkClient: Sendable {
     public func list() async throws -> [NetworkState] {
         let request = XPCMessage(route: .networkList)
 
-        let response = try await xpcSend(message: request, timeout: .seconds(1))
+        let response = try await xpcSendIdempotent(message: request, timeout: .seconds(1))
         let responseData = response.dataNoCopy(key: .networkStates)
         guard let responseData else {
             return []

Sources/Services/ContainerSandboxService/Client/SandboxClient.swift

@@ -47,14 +47,19 @@ public struct SandboxClient: Sendable {
 
     /// Create a SandboxClient by ID and runtime string. The returned client is ready to be used
     /// without additional steps.
-    public static func create(id: String, runtime: String, timeout: Duration = XPCClient.xpcRegistrationTimeout) async throws -> SandboxClient {
+    ///
+    /// `createEndpoint` is mutating server-side: a successful reply hands back an XPC endpoint
+    /// that the sandbox process owns. Racing the call with a response timeout would risk
+    /// orphaning that endpoint when a slow sandbox replies after the caller has given up.
+    /// The send therefore blocks until the sandbox replies or its connection is invalidated.
+    public static func create(id: String, runtime: String) async throws -> SandboxClient {
         let label = Self.machServiceLabel(runtime: runtime, id: id)
         let client = XPCClient(service: label)
         let request = XPCMessage(route: SandboxRoutes.createEndpoint.rawValue)
 
         let response: XPCMessage
         do {
-            response = try await client.send(request, responseTimeout: timeout)
+            response = try await client.send(request)
         } catch {
             throw ContainerizationError(
                 .internalError,