gotify / gotify/cli

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

Abierto
#85 0 comentarios 0 reacciones 0 asignados Ver en GitHub
Lenguaje dominante
Go
Estrellas
588
Forks
73
Métricas de merge de PR
Sin PR fusionados en 30 d

Descripción

## 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.

Guía de contribución

No hay ninguna guía de contribución indexada para este repositorio

Línea de trabajo

Comienza en command/watch.go, en doWatch y evalCmdOutput, y luego ejecuta la reproducción proporcionada de gotify watch bajo el race detector. El issue estará resuelto cuando los ticks de timeout repetidos ya no acumulen goroutines y la salida se lea únicamente después de que el proceso hijo y el trabajo de copia de su salida hayan terminado.

Escrito por el modelo de indexación a partir del texto del issue.

Evaluación

Stack tecnológico
go
Área
cli
Tipo de issue
Error
Dificultad
3/5
Tiempo estimado
1-2 días
Estado de actividad
Activo
Claridad
Bien especificado
Aptitud para principiantes
74/100

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.