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

[krishna3554]

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
• Function: doWatch → evalCmdOutput (lines ~86–108)

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:

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.