gotify / gotify/cli

watch: timeout path leaks one goroutine per kill and races on the shared output buffer

Offen
#85 0 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
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

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.