watch: timeout path leaks one goroutine per kill and races on the shared output buffer
- Vorherrschende Sprache
- Go
- Sterne
- 588
- Forks
- 73
- PR-Merge-Kennzahlen
- Keine gemergten PRs in 30 T.
Beschreibung
## Summary
In `watch` mode, when the watched command exceeds the interval and is killed via the timeout path, `evalCmdOutput` **leaks a goroutine per timeout occurrence** and reads the shared output buffer concurrently with the still-running child pipeline, which is a data race.
## Location
- File: [`command/watch.go`](https://github.com/gotify/cli/blob/1a3712544653db1ca4d12bb52779be80ea152c79/command/watch.go)
- Function: `doWatch` → `evalCmdOutput` (lines ~86–108)
```go
done := make(chan error) // unbuffered
go func() {
err := cmd.Wait()
if err != nil {
done <- fmt.Errorf("command failed to invoke: %v", err)
}
done <- nil
}()
select {
case err := <-done:
return outputBuf.String(), err
case <-timeOut:
cmd.Process.Kill()
return outputBuf.String(), errors.New("command timed out")
}
```
## Problem
Two defects on the timeout path:
1. **Goroutine leak.** `done` is unbuffered, and the `select` abandons it when `timeOut` fires. The spawned goroutine calls `cmd.Wait()`; once the killed process is reaped, `Wait` returns and the goroutine attempts `done <- ...` with no receiver ever coming, so it blocks forever. Every interval tick that times out leaks one goroutine (plus the reaped-process bookkeeping), for as long as `gotify watch` runs — which is by design unbounded (`for range time.NewTicker(...).C`).
2. **Data race on `outputBuf`.** When `Stdout`/`Stderr` are non-`*os.File` writers (here a `*bytes.Buffer`), `os/exec` spawns internal copy goroutines that write into `outputBuf`, and normally only `cmd.Wait()` provides the happens-before edge that makes reading the buffer safe. On the timeout path `Process.Kill()` is called without `Wait()`, and `outputBuf.String()` runs concurrently with those copy goroutines, which may still be draining buffered pipe data from the dying child. Concurrent `Buffer.Write` and `Buffer.String` are not safe.
## Trigger / Reproduction
Static analysis finding — not confirmed by execution. Run:
```sh
gotify watch -n 1 -- sh -c 'sleep 5; echo done'
```
Every tick kills `sleep 5` after 1 s, taking the `case <-timeOut:` branch each time: one blocked-forever goroutine per tick, plus an unsynchronized read of `outputBuf`.
## Expected Behavior
The timeout path should reap the child (e.g. call `cmd.Wait()` after `Kill()` in a separate goroutine, or use `exec.CommandContext` with a per-run context so cancellation/reaping is handled by the stdlib) and only then read `outputBuf`; the completion goroutine should never block on an unreceived channel send (e.g. `done := make(chan error, 1)`).
## Actual Behavior
Goroutines accumulate indefinitely across ticks, and `outputBuf` can be read while exec's copy goroutines are still writing to it.
## Impact
Long-running `gotify watch` deployments that monitor slow commands accumulate leaked goroutines over time (unbounded memory growth). The unsynchronized buffer access is undefined behavior under the Go memory model and can, under `-race`, flag crashes/misreads; practically it can observe torn/partial output.
## Suggested Direction
Minimal fix: make `done` buffered (`make(chan error, 1)`), and after `cmd.Process.Kill()` call `<-done` (or `cmd.Wait()`) before returning `outputBuf.String()` so the child is fully reaped before its output is read. A cleaner alternative is switching to `exec.CommandContext(ctx, ...)` per run.
Related but distinct: issue #44 asks for a way to *disable* the timeout entirely; this report concerns the correctness of the existing timeout implementation itself.
Beitragsleitfaden
Für dieses Repository ist kein Beitragsleitfaden indexiert
Rechercherichtung
Start in command/watch.go at doWatch and evalCmdOutput, then run the provided gotify watch reproduction under the race detector. The issue is done when repeated timeout ticks no longer accumulate goroutines and output is read only after the child and its output-copy work have completed.
Vom Indexierungsmodell aus dem Issue-Text verfasst.
Bewertung
- Tech-Stack
- go
- Bereich
- cli
- Issue-Typ
- Bug
- Schwierigkeit
- 3/5
- Geschätzter Aufwand
- 1-2 Tage
- Aktivitätsstatus
- Aktiv
- Klarheit
- Klar beschrieben
- Anfängerfreundlichkeit
- 74/100