watch: timeout path leaks one goroutine per kill and races on the shared output buffer
- Langage dominant
- Go
- Étoiles
- 588
- Forks
- 73
- Métriques de merge des PR
- Aucune PR mergée en 30 j
Description
## 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.
Guide de contribution
Aucun guide de contribution indexé pour ce dépôt
Piste de recherche
Commencez dans command/watch.go, au niveau de doWatch et evalCmdOutput, puis exécutez la reproduction gotify watch fournie sous le race detector. L’issue est terminée lorsque des ticks de timeout répétés n’accumulent plus de goroutines et que la sortie n’est lue qu’une fois que le processus enfant et le travail de copie de sa sortie sont terminés.
Rédigé par le modèle d'indexation à partir du texte de l'issue.
Évaluation
- Stack technique
- go
- Domaine
- cli
- Type d'issue
- Bug
- Difficulté
- 3/5
- Temps estimé
- 1-2 jours
- Activité
- Active
- Clarté
- Clairement spécifiée
- Accessibilité débutants
- 74/100