diff --git a/backend/internal/garmin/client.go b/backend/internal/garmin/client.go index b4667cb..e39e9c9 100644 --- a/backend/internal/garmin/client.go +++ b/backend/internal/garmin/client.go @@ -116,6 +116,7 @@ type subprocessClient struct { scriptPath string // temp file holding the embedded wrapper.py, written once started bool nextID int + stderrDone chan struct{} // closed once the stderr-copy goroutine has finished reading } // ensureStarted spawns the wrapper subprocess if it isn't already running. @@ -172,7 +173,15 @@ func (c *subprocessClient) ensureStarted() error { if err := cmd.Start(); err != nil { return fmt.Errorf("spawn garmin wrapper subprocess: %w", err) } - go io.Copy(os.Stderr, stderr) // wrapper.py logs auth/rate-limit diagnostics to stderr + // wrapper.py logs auth/rate-limit diagnostics to stderr. Per os/exec's + // StderrPipe docs, it's incorrect to call Wait before all reads from the + // pipe have completed, so close() waits on stderrDone before Wait-ing. + c.stderrDone = make(chan struct{}) + stderrDone := c.stderrDone + go func() { + io.Copy(os.Stderr, stderr) + close(stderrDone) + }() c.cmd = cmd c.stdin = stdin @@ -246,11 +255,17 @@ func (c *subprocessClient) close() error { if c.cmd != nil && c.cmd.Process != nil { c.cmd.Process.Kill() } + if c.stderrDone != nil { + // Killing the process closes its end of the stderr pipe, which + // unblocks the copy goroutine's Read with EOF; wait for it to finish + // before Wait, per os/exec's StderrPipe doc. + <-c.stderrDone + } var err error if c.cmd != nil { err = c.cmd.Wait() } - c.cmd, c.stdin, c.enc, c.scanner = nil, nil, nil, nil + c.cmd, c.stdin, c.enc, c.scanner, c.stderrDone = nil, nil, nil, nil, nil return err } diff --git a/backend/internal/garmin/client_test.go b/backend/internal/garmin/client_test.go index 3d4286a..bdaec6c 100644 --- a/backend/internal/garmin/client_test.go +++ b/backend/internal/garmin/client_test.go @@ -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()") + } +}