From eddaddaf8b6773b6afcd2ab86ad483bdaca810fe Mon Sep 17 00:00:00 2001
From: Pieter Noordhuis <pieter.noordhuis@databricks.com>
Date: Thu, 24 Oct 2024 14:03:12 +0200
Subject: [PATCH] Attempt to reduce test flakiness on Windows (#1845)

## Changes

Test failures indicate that both stdout and stderr are consumed, yet the
content of stdout doesn't end up in the intended output. This can happen
if the goroutines responsible for writing to the combined output buffer
attempt to write to the same underlying buffer concurrently.

Example failure:
```
=== RUN   TestBackgroundCombinedOutput
    background_test.go:65:
        	Error Trace:	D:/a/cli/cli/libs/process/background_test.go:65
        	Error:      	elements differ

        	            	extra elements in list A:
        	            	([]interface {}) (len=1) {
        	            	 (string) (len=1) "2"
        	            	}


        	            	listA:
        	            	([]string) (len=2) {
        	            	 (string) (len=1) "1",
        	            	 (string) (len=1) "2"
        	            	}


        	            	listB:
        	            	([]string) (len=1) {
        	            	 (string) (len=1) "1"
        	            	}
        	Test:       	TestBackgroundCombinedOutput
```

With the test body:

https://github.com/databricks/cli/blob/ca45e53f42c5c4b26f2833554ab7118802c017cb/libs/process/background_test.go#L48-L66

With the implementation of `WithCombinedOutput`:

https://github.com/databricks/cli/blob/ca45e53f42c5c4b26f2833554ab7118802c017cb/libs/process/opts.go#L72-L78

Notice that `c.Stdout` does get the "2", or the test failure would have
included the relevant assertion error. This leads me to believe that
there is a race on writing to `buf` from the two goroutines writing to
`c.Stdout` and `c.Stderr`.

## Tests

The test passes. If this PR has the intended effect remains to be
seen...
---
 libs/process/opts.go | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/libs/process/opts.go b/libs/process/opts.go
index 9516e49b..dd066751 100644
--- a/libs/process/opts.go
+++ b/libs/process/opts.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"os/exec"
+	"sync"
 )
 
 type execOption func(context.Context, *exec.Cmd) error
@@ -69,10 +70,24 @@ func WithStdoutWriter(dst io.Writer) execOption {
 	}
 }
 
+// safeWriter is a writer that is safe to use concurrently.
+// It serializes writes to the underlying writer.
+type safeWriter struct {
+	w io.Writer
+	m sync.Mutex
+}
+
+func (s *safeWriter) Write(p []byte) (n int, err error) {
+	s.m.Lock()
+	defer s.m.Unlock()
+	return s.w.Write(p)
+}
+
 func WithCombinedOutput(buf *bytes.Buffer) execOption {
+	sw := &safeWriter{w: buf}
 	return func(_ context.Context, c *exec.Cmd) error {
-		c.Stdout = io.MultiWriter(buf, c.Stdout)
-		c.Stderr = io.MultiWriter(buf, c.Stderr)
+		c.Stdout = io.MultiWriter(sw, c.Stdout)
+		c.Stderr = io.MultiWriter(sw, c.Stderr)
 		return nil
 	}
 }