fix(cli): discard log writes to closed pipes during shutdown#26082
Open
EhabY wants to merge 1 commit into
Open
fix(cli): discard log writes to closed pipes during shutdown#26082EhabY wants to merge 1 commit into
EhabY wants to merge 1 commit into
Conversation
Background goroutines keep logging after a CLI command starts tearing down, when the reader on the log destination has already gone away. The most visible case is "coder port-forward -v", whose tailnet goroutines log into inv.Stdout after the consumer is gone, but the same race exists for any verbose CLI sink (a closed in-memory pipe in tests, a broken OS pipe in real use). slog reports those failed writes to stderr, which is noise and, via its syncwriter, can interleave with and corrupt go test/test2json output (a passing test gets misreported as failed). See coder/slog#225. Add clilog.DiscardOnPipeError, a writer wrapper that drops writes failing with io.ErrClosedPipe or syscall.EPIPE while still returning every other error (including os.ErrClosed, which usually means a write to a writer we closed ourselves). Apply it to the clilog stdout/stderr sinks and the port-forward verbose sink. Other ad-hoc sinks can opt in the same way. This keeps the opt-out in coder rather than hiding sink errors inside slog.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Add
clilog.DiscardOnPipeError, a smallio.Writerwrapper that drops writes which fail because the destination pipe is gone (io.ErrClosedPipe,syscall.EPIPE), and apply it to the log sinks coder attaches to CLI stdio: theclilog/dev/stdoutand/dev/stderrsinks, and theport-forward -vverbose sink.Why
Background goroutines keep logging after a CLI command starts tearing down, once the reader on the log destination has gone away. The most visible case is
coder port-forward -v, whose tailnet goroutines log intoinv.Stdoutafter the consumer is gone (a closed in-memory pipe in tests, a broken OS pipe in real use such ascoder port-forward -v | head).slog reports those failed writes to stderr, which is noise and, through its
syncwriter, can interleave with and corruptgo test/test2jsonoutput, so a passing test gets misreported as failed. We saw this in a real CI run where a passingTestRetryWithIntervalwas markedunknown:The slog-side fix for the corruption is coder/slog#225 / coder/slog#226. Per maintainer guidance there, the suppression of these benign "reader gone" errors belongs in coder (the consumer that owns the closing writer), not inside slog, so a user-assigned sink failing is never hidden globally.
Approach
The discard logic lives in one place (
clilog.DiscardOnPipeError) and is applied at the two strategic sinks.os.ErrClosedand every other write error are still returned, so a write to a writer we closed ourselves is not hidden. This is intentionally not wrapped globally oninv.Stdout/inv.Stderr, which would change normal CLI pipe semantics (for example,coder list | headshould stop on a broken pipe rather than silently keep writing). Other ad-hocsloghuman.Sink(inv.Std*)sites can opt in with the same one-call wrap if they show the same noise.Testing
TestDiscardOnPipeError: discardsio.ErrClosedPipe/syscall.EPIPE(including wrapped), still reportsos.ErrClosedand other errors, and passes through on success.go build ./cli/...,go vet,gofmt, andgolangci-lint run ./cli/ ./cli/clilog/are clean.Why discard rather than report
These writes only fail during shutdown, after the reader is already gone, so the entries had nowhere to land regardless. The change is scoped to log sinks coder attaches to stdio, not slog's global behavior, and it does not affect normal command output.
os.ErrClosedstays reported to avoid masking a genuine write to an already-closed writer.Generated by Coder Agents on behalf of @EhabY.