fix(garmin): wait for stderr-copy goroutine before cmd.Wait()

ensureStarted's stderr-copy goroutine could still be mid-Read when close()
called cmd.Wait(), which os/exec's StderrPipe docs call out as incorrect
and can truncate/garble trailing stderr diagnostics or surface a spurious
"file already closed" error. close() now waits on a stderrDone channel,
closed by the copy goroutine once it hits EOF (unblocked by killing the
process), before calling Wait.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
2026-07-25 20:50:39 +02:00
parent 6c161f2206
commit df197f45fd
2 changed files with 61 additions and 2 deletions

View File

@@ -4,8 +4,10 @@ import (
"bufio"
"encoding/json"
"io"
"os/exec"
"strings"
"testing"
"time"
)
// wireResponsePayload is what a fake wrapper handler returns for one
@@ -147,3 +149,45 @@ func TestSubprocessClient_Close_NoOpWhenNotStarted(t *testing.T) {
t.Errorf("Close on an unstarted client = %v, want nil", err)
}
}
// TestSubprocessClient_Close_WaitsForStderrCopyGoroutine exercises close()
// against a real subprocess that writes to stderr, mirroring how
// ensureStarted wires up the stderr-copy goroutine. Per os/exec's
// StderrPipe docs it is incorrect to call Wait before all reads from the
// pipe have completed; this guards against close() calling cmd.Wait()
// before the copy goroutine has drained the pipe (which previously risked a
// race / truncated stderr / spurious "file already closed" errors).
func TestSubprocessClient_Close_WaitsForStderrCopyGoroutine(t *testing.T) {
cmd := exec.Command("sh", "-c", "for i in 1 2 3 4 5; do echo line$i 1>&2; done; sleep 5")
stderr, err := cmd.StderrPipe()
if err != nil {
t.Fatalf("StderrPipe: %v", err)
}
if err := cmd.Start(); err != nil {
t.Fatalf("Start: %v", err)
}
stderrDone := make(chan struct{})
go func() {
io.Copy(io.Discard, stderr)
close(stderrDone)
}()
c := &subprocessClient{started: true, cmd: cmd, stderrDone: stderrDone}
done := make(chan error, 1)
go func() { done <- c.close() }()
select {
case <-done:
// close() returned -- since it killed the process and waited on
// stderrDone before Wait-ing, this proves the ordering held without
// deadlocking.
case <-time.After(5 * time.Second):
t.Fatal("close() did not return in time -- likely blocked waiting on stderrDone")
}
if c.stderrDone != nil {
t.Error("stderrDone should be reset to nil after close()")
}
}