Our agentssh package handles running commands in a pseudoterminal, and we've had several problems with not returning command output.

The implementation prior to this PR seems to behave how we want it to, (at least on Linux), but it

1. uses needlessly complex concurrency and synchronization
2. incorrectly explainswhy it works
3. doesn't fix the problem on Windows

This leaves us in a precarious situation where if someone tries to fix or enhance this function, they'll misunderstand how and why the current code works.

This PR attempts to address these problems.

This was the subject of severalfixes:#6777#6833 and#6862

The initial bug was that the SSH handler runsio.Copy() in a goroutine to copy the command output to the SSH session, but we didn't wait for this copy to finish before closing down the session, causing a race that sometimes lost output.

However, simply waiting for theio.Copy() to finish would result in a deadlock because of another, much more subtle bug in the way pseudoterminals are handled by the OS.

Linux/macOS

The current code makes an unnecessary call to the low-level syscalldup to duplicate a file descriptor. I'll try to explain what was happening and why it was bad:

On POSIX systems, the way pseudo-TTYs work is that open a pair of files, which I'll refer to as thepty and thetty to match the terminology in our code (Linux itself calls themptm andpts). Thetty acts just like a "real" teletype device for any program interacting with it --- and when we launch commands, we pass thetty asall three of the stdio file descriptors (stdin, stdout, stderr) to the child process.

After forking/exec'ing to start the child process this is what things looked like:

go processPTY:  - pty  ------------------------------> pty <--- linux kernel ----> tty <---- child process  - tty  ------------------------------------------------------------^

Notice that our go process kept thetty file open. What happens is when you read from thepty, and there is no data available to read, it blocks until either

1. there is data to read
2. all file descriptors to thetty are closed

We kept an open file descriptor to thetty so even after the child process exited, we never hit condition 2, until we calledClose() on ourPTY object, which then closed bothpty andtty at the same time.

The current code gets around this by callingdup on thepty file descriptor, so we end up with two references to the PTY.

go processSSH Handler:  - stdout ------------------------------+PTY:                                     V  - pty  ------------------------------> pty <--- linux kernel ----> tty <---- child process  - tty  ------------------------------------------------------------^

The explanation given for this is that it allows us to closestdin andstdout separately, but that's a misunderstanding. The child process interacts only with thetty for bothstdin andstdout --- duplicating thepty doesn't change this.

However, thisdoes allow us to break the deadlock, because callingClose() on thePTY closestty, but since we have the duplicated file, we can still read the output as long as the child process is still alive or there is unread buffered data in the kernel. But, this is needlessly wasteful of file descriptors (which are a limited kernel resource), and essentially uses a kernel primitive to accomplish the task of closingtty without closingpty.

The essential features of this PR on Linux/macOS are to

1. Close thetty immediately after creating the child process - this allows us to safely read all the output from the child process without fear of deadlocking.
2. Fix the SSH handler to simply callio.Copy() to copy output, then grab the exit code and return --- no extra file descriptors, wait groups, or synchronization.

Windows

Instead of a single read-write file descriptor, Windows uses a pair of pipes (which are unidirectional). On Windows, though, the pseudoconsole is hosted by a separate process, called the Console Host (conhost.exe), rather than being handled entirely in-kernel like on Linux/macOS. The lifecycle of the pseudoterminal is explicit via a Handle returned on creation. So on Windows, we create 5 handles:

1. Input Read Pipe
2. Input Write Pipe
3. Output Read Pipe
4. Output Write Pipe
5. Console

In the current implementation, we keep all 5 handles open until the PTY is closed, at which point we close all 5. This then causes a race for a reader trying to read all the data before the pipe gets closed.

Docs for PseudoConsoles explain that once you pass the Input Read Pipe and Output Write Pipe to the pseudoconsole host, you should close them.

On Windows we also have to close the Pseudoconsole itself to unblock our output reads, since the Pseudoconsole is a separate process from the child application, and it has an explicit lifetime. So, in this PR, we also add code that closes the Pseudoconsole as soon as the child application exits (for PTYs attached to commands). Crucially, we leave the Output Read Pipe open, so that we can still read any data buffered in the OS.

Other features

• split thePTY interface in 2, since there are still use cases in the code where we create a PTY but don't start a child process. So, we have one interface when we are interacting with a child, and a different interface when we hold both ends of the PTY -- this is done for safety so that you can't attempt to read from thetty after we've closed it.
• add a unit test to check that we clean up orphaned processes when the SSH client disconnects
• add a unit test to check for truncated output from commands started in a PTY