Tests/CLITests: drain stdout/stderr concurrently in CLITest.run

[mvanhorn]

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Motivation and Context

CLITest.run(arguments:stdin:currentDirectory:env:) reads stdout to EOF before stderr begins draining (Tests/CLITests/Utilities/CLITest.swift:182-183). If a child process writes more than a pipe-buffer's worth of data to stderr (~64 KB on macOS), the child blocks in write() on stderr while we block in readDataToEndOfFile() on stdout, and neither side makes progress until process.waitUntilExit() returns. This is a latent deadlock that any future test or CLI verbosity bump can trigger.

This PR drains both streams concurrently using readabilityHandler closures backed by Mutex<Data> buffers. After process.waitUntilExit() returns the handlers are cleared and readDataToEndOfFile() flushes any bytes the kernel buffered between the last handler invocation and exit. The error path also clears handlers so a failed process.run() does not leak callbacks. Mutex matches the locking primitive the file already uses for commandSeq.

Fixes #1456

Testing

• Tested locally
• Added/updated tests
• Added/updated docs

swift build --target CLITests passes against the change. make swift-fmt-check is clean. Behavior is unchanged for any test command that previously fit within a single pipe buffer, so the existing CLITests still exercise the helper end-to-end.

I did not add a new test that emits >64 KB to stderr because every existing CLITest invocation goes through executablePath (the container binary), and reproducing the deadlock requires a child that emits a controllable amount on stderr. Happy to follow up with a small refactor that extracts the drain into a static helper plus a regression test that drives it via /bin/sh if that would be useful.

[katiewasnothere]

Hi @mvanhorn thank you for contributing! What are your thoughts on redirecting the out/err to FileHandles vs using readabilityhandlers?

[mvanhorn]

Both are valid - the issue listed each as acceptable. I went with readabilityHandler because the diff stays in one file (no temp-file lifecycle or error-path cleanup) and Mutex is already used in CLITest.swift, so guarding the buffer matches the existing style.

The FileHandle route is arguably simpler to reason about (kernel handles buffering, no two-stage drain) at the cost of FS setup and cleanup per test.

If you'd prefer the FileHandle version, happy to swap - it's a clean rewrite of the same block. Which way would you like?

[mvanhorn]

also @katiewasnothere I live on Mercer Island! PNW represent!

[katiewasnothere]

@mvanhorn I would prefer the file based option. Many CLITests and helper functions already follow a pattern of creating and cleaning up temporary directories and files.

[mvanhorn]

Done in 20dddd2. Followed the withTempDir pattern already in CLITest.swift - UUID-named dir under FileManager.default.temporaryDirectory, FileHandle for the writes, defer cleanup.

Net -13 lines (no Mutex, no handler-clearing, no two-stage drain).

[jglogan]

@mvanhorn How about using defer here?

             let stdoutHandle = try FileHandle(forWritingTo: stdoutURL)
             let stderrHandle = try FileHandle(forWritingTo: stderrURL)
+            defer { try? stdoutHandle.close() }
+            let stderrHandle = try FileHandle(forWritingTo: stderrURL)
+            defer { try? stderrHandle.close() }
             process.standardOutput = stdoutHandle
             process.standardError = stderrHandle
 
             try process.run()
             process.waitUntilExit()
-
-            try? stdoutHandle.close()
-            try? stderrHandle.close()
-
             outputData = try Data(contentsOf: stdoutURL)
             errorData = try Data(contentsOf: stderrURL)

[mvanhorn]

Done in 71c649e. The defer approach also covers the try process.run() throw path, which the explicit close calls after waitUntilExit() didn't.