gotify / gotify/cli

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

オープン
#85 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る
主要言語
Go
スター
588
フォーク
73
PR マージ指標
30日以内にマージされた PR はありません

説明

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

コントリビューションガイド

このリポジトリのコントリビューションガイドは索引されていません

調査の方向性

command/watch.go の doWatch と evalCmdOutput から始め、次に提供されている gotify watch の再現手順を race detector で実行します。繰り返される timeout の tick によって goroutine が蓄積せず、child とその出力をコピーする処理が完了した後にのみ出力が読み取られるようになれば、issue は完了です。

索引モデルが issue の本文から書いたものです。

評価

技術スタック
go
領域
cli
issue の種類
バグ
難易度
3/5
見積もり時間
1〜2日
活発さ
活発
明瞭さ
明確に書かれている
初心者へのやさしさ
74/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。