progress: nothing printed at phase start on non-TTY, bogus isatty, warnings race the spinner #13

New Issue

clawbot · 2026-08-09T03:45:37+02:00

clawbot commented

2026-08-09 03:45:37 +02:00

Three defects in progress.go, all small, all in the same file.

1. Non-TTY prints nothing until the first item completes. newProgress returns immediately without printing when stderr is not a TTY (progress.go:54-58); the first plain line only appears from increment() (progress.go:102-106). README §Progress requires that each phase's display be "rendered the moment the phase starts — a scan must never look hung." Under cron, tee or CI, stderr is a pipe. If the walk's first directory read blocks for minutes on a busy pool, the log is completely silent — precisely the failure the requirement exists to prevent. The hash phase can emit nothing at all if its first read is slow.

2. stderrIsTTY treats any character device as a terminal. progress.go:26-33 tests fi.Mode()&os.ModeCharDevice != 0. /dev/null and /dev/zero are character devices, so sfdupes scan ... 2>/dev/null takes the full ANSI redraw path and runs the library's 100ms spinner goroutine, writing escape sequences into the void.

3. Warnings race the spinner's background renderer. warnf (progress.go:109-119) calls p.bar.Clear() and then writes to os.Stderr from the main goroutine. For the indeterminate (-1) bars, progressbar.NewOptions64 starts an internal goroutine that renders to os.Stderr every 100ms with no synchronization against warnf. The walk phase is both a spinner bar and the phase that emits per-path warnings (scan.go:234) — and those warnings are what tell the operator which records are about to be deleted, so garbling them matters. The progress doc comment at progress.go:41-42 already claims single-goroutine ownership of stderr; today that is not true.

Definition of done

newProgress emits one zero-state plain line immediately on the non-TTY path, before any work begins.
stderrIsTTY uses a real terminal test. github.com/mattn/go-isatty and golang.org/x/term are both already in the module graph as indirect dependencies of progressbar; promote whichever is used to a direct dependency and update README §Constraints, which currently lists the permitted third-party dependencies.
The spinner's background render goroutine is disabled (OptionSetSpinnerChangeInterval(0), so the spinner advances only on Add) or warnings are routed through the bar's own writer — either way stderr has one writer, matching the doc comment.
Tests: a non-TTY writer receives a line before the first increment(); a pipe, a regular file and /dev/null all report not-a-TTY; interleaved warnings and increments each land on their own line.
make check green.

Three defects in `progress.go`, all small, all in the same file. **1. Non-TTY prints nothing until the first item completes.** `newProgress` returns immediately without printing when stderr is not a TTY (`progress.go:54-58`); the first plain line only appears from `increment()` (`progress.go:102-106`). README §Progress requires that each phase's display be "rendered the moment the phase starts — a scan must never look hung." Under cron, `tee` or CI, stderr is a pipe. If the walk's first directory read blocks for minutes on a busy pool, the log is completely silent — precisely the failure the requirement exists to prevent. The hash phase can emit nothing at all if its first read is slow. **2. `stderrIsTTY` treats any character device as a terminal.** `progress.go:26-33` tests `fi.Mode()&os.ModeCharDevice != 0`. `/dev/null` and `/dev/zero` are character devices, so `sfdupes scan ... 2>/dev/null` takes the full ANSI redraw path and runs the library's 100ms spinner goroutine, writing escape sequences into the void. **3. Warnings race the spinner's background renderer.** `warnf` (`progress.go:109-119`) calls `p.bar.Clear()` and then writes to `os.Stderr` from the main goroutine. For the indeterminate (`-1`) bars, `progressbar.NewOptions64` starts an internal goroutine that renders to `os.Stderr` every 100ms with no synchronization against `warnf`. The walk phase is both a spinner bar and the phase that emits per-path warnings (`scan.go:234`) — and those warnings are what tell the operator which records are about to be deleted, so garbling them matters. The `progress` doc comment at `progress.go:41-42` already claims single-goroutine ownership of stderr; today that is not true. ## Definition of done 1. `newProgress` emits one zero-state plain line immediately on the non-TTY path, before any work begins. 2. `stderrIsTTY` uses a real terminal test. `github.com/mattn/go-isatty` and `golang.org/x/term` are both already in the module graph as indirect dependencies of `progressbar`; promote whichever is used to a direct dependency and update README §Constraints, which currently lists the permitted third-party dependencies. 3. The spinner's background render goroutine is disabled (`OptionSetSpinnerChangeInterval(0)`, so the spinner advances only on `Add`) or warnings are routed through the bar's own writer — either way stderr has one writer, matching the doc comment. 4. Tests: a non-TTY writer receives a line before the first `increment()`; a pipe, a regular file and `/dev/null` all report not-a-TTY; interleaved warnings and increments each land on their own line. 5. `make check` green.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:45:37 +02:00

clawbot referenced this issue

2026-08-09 03:46:29 +02:00

Add a race-detector test target for the concurrent scan paths #18

clawbot referenced this issue

2026-08-09 03:47:54 +02:00

Land golangci-lint v2.12.2 and the canonical .golangci.yml on main #3

Sign in to join this conversation.