build: Serialize rotating log writes through pipe by Roasbeef · Pull Request #11169 · lightningnetwork/lnd

Roasbeef · 2026-09-04T01:48:00Z

Overview

The rotating log writer starts a goroutine that consumes an io.Pipe, but
its Write method currently bypasses the pipe and writes to the underlying
rotator directly. Since subsystem loggers share this writer, concurrent log
calls can race over the rotator's file and size state. The direct write can
also race with the reader goroutine during startup.

This PR routes writes through the existing pipe. The reader goroutine becomes
the sole owner of the rotator, while io.Pipe serializes concurrent writers.
Shutdown now closes the pipe, waits for the reader, closes the rotator, and
returns any errors from those stages. Repeated Close calls are idempotent.

Testing

go test -race ./build
go test -race ./build -run TestRotatingLogWriterConcurrentWrites -count=20
make fmt-check
go vet ./build

The regression test writes concurrently across the rotation threshold, waits
for shutdown, and decompresses the resulting gzip backup through EOF.

The rotating log writer starts a goroutine that reads from an io.Pipe, but its Write method bypasses the pipe and accesses the rotator directly. Concurrent subsystem loggers can therefore race with each other and with the reader goroutine over the rotator's file and size state. In this commit, we send writes through the existing pipe so its reader is the sole owner of the rotator. Close now shuts down the pipe, waits for the reader, and returns any runtime rotation or close error.

github-actions · 2026-09-04T01:57:21Z

🟡 PR Severity: MEDIUM

file classification | 2 files | 137 lines changed

🟡 Medium (1 file)

build/logrotator.go - uncategorized Go source file (build/*), changes concurrency/shutdown behavior of the rotating log writer

🟢 Low (1 file)

build/logrotator_test.go - new regression test, no production code impact

Analysis

The only production code change is in build/logrotator.go, which doesn't fall under any of the CRITICAL or HIGH package categories (it's log-rotation infrastructure, not wallet/HTLC/contract/sweep/peer/keychain/etc. code), so it defaults to MEDIUM as an "other Go file." The change is concurrency-sensitive (serializing writes through an io.Pipe, changing Close semantics), so reviewers should double check the race-condition fix and shutdown ordering even though the package itself is low-risk. File/line counts are well below the bump thresholds (1 non-test file, 75 non-test lines changed), so no severity escalation applies.

_{To override, add a severity-override-{critical,high,medium,low} label.}

Lrifton92

Read this against jrick/logrotate v1.1.2 (the version pinned in go.mod) and the change is sound: Run is now the only goroutine touching the rotator, Rotator.Close waits on the compress WaitGroup, and closing the pipe first, then draining runErr, then closing the rotator is the right order. No deadlock if Run already exited with an error, since runErr is buffered and CloseWithError unblocks any writer.

Two things worth a look before merge:

Runtime rotation errors become invisible until shutdown. Before this PR a failed rotate() (disk full, rename refused) printed failed to run file rotator to stderr once. Now the error only lives in the pipe: every later Write returns it, but the btclog handlers drop write errors, so file logging stops silently for the rest of the process lifetime. The error resurfaces only in the deferred cfg.LogRotator.Close() in lnd.go (ltndLog.Errorf("Could not close log rotator")), and at that point the file handler is already closed, so it is only visible on the console handler at exit. Suggest keeping the one-shot stderr line in the goroutine when err != nil (after the io.EOF normalisation), in addition to CloseWithError.
Test constant coupling. testLogFileSize = 1000 * 1024 only triggers the rotation because logrotate defines its threshold as 1000 * thresholdKB bytes (rotator.go:79) while InitLogRotator passes MaxLogFileSize*1024. With MaxLogFileSize = 1 the threshold is exactly 1,024,000 bytes, so the second 512,000-byte line crosses it and produces exactly one .1.gz. It works, but a one-line comment on the constant (or deriving it as logConfig.MaxLogFileSize * 1024 * 1000) would save the next reader a detour into the dependency.

Minor: the errors.Is(err, io.EOF) normalisation is required, Run returns ReadLine's io.EOF verbatim when the pipe is closed. Moving the compressor check ahead of MkdirAll/rotator.New is a nice side effect too, no dangling open file on an unknown compressor.

litbot-9000 · 2026-09-11T09:58:10Z