Skip to content

Commit caf495c

Browse files
committed
fix(internal/syncwriter): suppress benign "reader gone" write errors
Ignore io.ErrClosedPipe and syscall.EPIPE in Write, the same way Sync already ignores EINVAL, ENOTTY, and EBADF. When a sink's reader has gone away the entry is undeliverable and the error is not actionable, so reporting it only adds shutdown-time noise. os.ErrClosed is left reported on purpose, since it usually means a write to a writer we closed ourselves and is worth surfacing. Evidence of the benign case: in a real coder/coder CI run a passing TestRetryWithInterval was marked unknown because a concurrent "port-forward -v" test logged into a closed pipe during teardown. https://github.com/coder/coder/actions/runs/26586696037/job/78334548165
1 parent 7674226 commit caf495c

2 files changed

Lines changed: 59 additions & 0 deletions

File tree

internal/syncwriter/syncwriter.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,12 @@ func (w *Writer) Write(name string, p []byte) {
3636
defer w.mu.Unlock()
3737
_, err := w.w.Write(p)
3838
if err != nil {
39+
// A closed destination is benign: the reader is gone, so the
40+
// entry is undeliverable and not worth reporting.
41+
// See https://github.com/coder/slog/issues/225
42+
if errorsIsAny(err, io.ErrClosedPipe, syscall.EPIPE) {
43+
return
44+
}
3945
w.errorf("%v: failed to write entry: %+v", name, err)
4046
}
4147
}

internal/syncwriter/syncwriter_test.go

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
package syncwriter
22

33
import (
4+
"fmt"
45
"io"
56
"os"
67
"strings"
8+
"syscall"
79
"testing"
810

911
"cdr.dev/slog/v3/internal/assert"
@@ -75,6 +77,57 @@ func TestWriter_Sync(t *testing.T) {
7577
})
7678
}
7779

80+
// Benign "destination closed" errors must not be reported.
81+
func TestWriter_Write_suppressesClosedDestinationErrors(t *testing.T) {
82+
t.Parallel()
83+
84+
benign := []error{
85+
io.ErrClosedPipe,
86+
syscall.EPIPE,
87+
fmt.Errorf("wrapped: %w", io.ErrClosedPipe),
88+
}
89+
90+
for _, target := range benign {
91+
target := target
92+
t.Run(fmt.Sprintf("%v", target), func(t *testing.T) {
93+
t.Parallel()
94+
95+
errors := 0
96+
w := New(syncWriter{
97+
wf: func([]byte) (int, error) { return 0, target },
98+
sf: func() error { return nil },
99+
})
100+
w.errorf = func(string, ...interface{}) { errors++ }
101+
102+
w.Write("sloghuman", []byte("entry"))
103+
assert.Equal(t, "errors", 0, errors)
104+
})
105+
}
106+
}
107+
108+
func TestWriter_Write_reportsOtherErrors(t *testing.T) {
109+
t.Parallel()
110+
111+
// os.ErrClosed is not suppressed: it usually means a write to an
112+
// already-closed writer, which is worth surfacing.
113+
for _, target := range []error{io.EOF, os.ErrClosed} {
114+
target := target
115+
t.Run(fmt.Sprintf("%v", target), func(t *testing.T) {
116+
t.Parallel()
117+
118+
errors := 0
119+
w := New(syncWriter{
120+
wf: func([]byte) (int, error) { return 0, target },
121+
sf: func() error { return nil },
122+
})
123+
w.errorf = func(string, ...interface{}) { errors++ }
124+
125+
w.Write("sloghuman", []byte("entry"))
126+
assert.Equal(t, "errors", 1, errors)
127+
})
128+
}
129+
}
130+
78131
// The default handler must report through the os.Stderr variable, not the
79132
// builtin println that bypasses it. Reverting to println fails this test.
80133
//

0 commit comments

Comments
 (0)