This smells funny in that it shouldn't be the job of the cli command to close stdin.

Also, in the case where rawSSH goes down unexpectedly, but the workspace stays up, I think that we could still be blocked on reading from stdin, creating a deadlock with the waitgroup.

It should be the job of the clibase invoker to close stdin when the command is done -- but here that can create a deadlock with the waitgroup, since the main command won't return until the waitgroup is done.

Instead of considering the waitgroup to be all the command goroutines except the main one, why don't we make the waitgroup all the command goroutines including the main one? Then we can have a separate goroutine that just waits for the group, and closes the log file.

e.g.

var wg sync.WaitGroup
wg.Add(1)
defer wg.Done()
if logDirPath != "" {
    logfile, err := os.OpenFile(...)
    if err != nil {...}
    go func() {
        wg.Wait()
        logfile.Close()
    }
}

wg.Add(1)
go func() {
    defer wg.Done()
    ...
}

I agree my solution is janky. The challenge with closing the logfile in a separate goroutine is it creates a leak — which was the main thing I was trying to fix when opening this PR.

Closing in a separate goroutine is not a leak (unless the waitgroup fails to complete). It just waits for the other things to finish, closes the file, then exits.

And, it's not just a question of jankiness. As written, it's still bugged in the case where the rawSSH closes for some external reason not related to the workspace being stopped. The command will deadlock.

Closing in a separate goroutine is not a leak (unless the waitgroup fails to complete). It just waits for the other things to finish, closes the file, then exits.

Maybe leak isn't the best word. It can show up in our leak detector and cause test flakiness, even though in production there is no leak.

As written, it's still bugged in the case where the rawSSH closes for some external reason not related to the workspace being stopped. The command will deadlock.

I don't understand this. If rawSSH closes for some external reason, and io.Copy is blocked on write, the write call would immediately fail?

I don't understand this. If rawSSH closes for some external reason, and io.Copy is blocked on write, the write call would immediately fail?

I'm worried about io.Copy blocking on Read() - the same thing you're trying to prevent here, but with rawSSH just closing for a different reason.

Maybe leak isn't the best word. It can show up in our leak detector and cause test flakiness, even though in production there is no leak.

Have you tried it and seen goleak firing? If there are goroutines still running, goleak will sleep and retry up to some maximum timeout, exactly so that things like what I'm proposing have time to resolve themselves before goleak complains about them.

I can't imagine this would be the case for waiting for a waitgroup and closing a file, but if for some reason the goroutine takes longer to resolve than goleak's default timeout, we can adjust the timeout.

Didn't know about the retry. I'll implement your proposal.