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.

Author: chrisgeo

Package.swift

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

Sources/ContainerXPC/XPCClient.swift

@@ -69,23 +69,66 @@ 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) 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)
+                }
+            }
+        }
+    }
+
+    /// 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.
     ///
-    /// Late completions from the other paths are dropped silently so the connection
-    /// remains valid for subsequent sends — important for callers that hold a long-lived
-    /// `XPCClient` (`ContainerClient`, `NetworkClient`). 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 `responseTimeout` ineffective whenever the remote service was wedged.
-    /// See docs/internal/help-freeze-analysis.md.
+    /// **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, responseTimeout: Duration? = nil) async throws -> XPCMessage {
+    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
@@ -100,18 +143,16 @@ extension XPCClient {
                     }
                 }
 
-                if let responseTimeout {
-                    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)"
-                            )
+                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)"
                         )
-                    }
+                    )
                 }
 
                 // Close the race window: if cancellation arrived before `set(cont)`
@@ -126,6 +167,23 @@ extension XPCClient {
         }
     }
 
+    /// 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:

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,