From 1c967b81ad3d3af763fd9b0c21934b3bb7b19816 Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Fri, 5 Jun 2026 17:36:54 -0600 Subject: [PATCH 1/2] fix: recover send-on-closed-channel panic in byteChannel sink MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit byteChannel.UpdateFrom did an unguarded `s.ch <- b` to a caller-owned channel. When the consumer closes that channel (shutdown / config reload) while a keepcurrent Runner is mid-sync on its ticker, the send panics with "send on closed channel" and crashes the whole process — observed taking down the macOS v9 client (Freshdesk #176970). Consumers that close the channel: fronted fronted.go:257 and radiance kindling/dnstt/parser.go:162. The sink doesn't own the channel (ToChannel takes a caller channel) so it can't know whether it's been closed. Recover the panic and surface it as a sink error, which Runner already treats as recoverable via OnSinkError instead of dying. Adds regression tests. Co-Authored-By: Claude Opus 4.8 --- sink.go | 13 ++++++++++++- sink_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 sink_test.go diff --git a/sink.go b/sink.go index bd57c51..b56fd3e 100644 --- a/sink.go +++ b/sink.go @@ -1,6 +1,7 @@ package keepcurrent import ( + "fmt" "io" "io/ioutil" "os" @@ -72,7 +73,17 @@ func ToChannel(ch chan []byte) Sink { return &byteChannel{ch} } -func (s *byteChannel) UpdateFrom(r io.Reader) error { +func (s *byteChannel) UpdateFrom(r io.Reader) (err error) { + // The channel is owned by the caller (see ToChannel) and may be closed + // concurrently — e.g. on shutdown or config reload — while a Runner is + // mid-sync. A send on a closed channel panics, which would crash the whole + // process; recover and surface it as a sink error instead (Runner reports + // sink errors via OnSinkError rather than dying). + defer func() { + if rec := recover(); rec != nil { + err = fmt.Errorf("send on byte channel: %v", rec) + } + }() b, err := ioutil.ReadAll(r) if err != nil { return err diff --git a/sink_test.go b/sink_test.go new file mode 100644 index 0000000..d828553 --- /dev/null +++ b/sink_test.go @@ -0,0 +1,32 @@ +package keepcurrent + +import ( + "bytes" + "testing" +) + +// Regression test for the "send on closed channel" crash (Freshdesk #176970): +// the channel is owned by the caller and may be closed (shutdown / config +// reload) while a Runner is mid-sync. UpdateFrom must surface that as an error +// rather than panicking and crashing the whole process. +func TestByteChannelClosedDoesNotPanic(t *testing.T) { + ch := make(chan []byte) + close(ch) + s := ToChannel(ch) + err := s.UpdateFrom(bytes.NewReader([]byte("hello"))) + if err == nil { + t.Fatal("expected an error sending to a closed channel, got nil") + } +} + +// A healthy send still delivers the data unchanged. +func TestByteChannelDelivers(t *testing.T) { + ch := make(chan []byte, 1) + s := ToChannel(ch) + if err := s.UpdateFrom(bytes.NewReader([]byte("hello"))); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got := <-ch; string(got) != "hello" { + t.Fatalf("got %q, want %q", got, "hello") + } +} From 883bbc48d90c555133e4fe5a81ad77419c8b8c17 Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Fri, 5 Jun 2026 17:52:54 -0600 Subject: [PATCH 2/2] review: narrow recover to send-on-closed-channel; drop internal ticket ref Addresses Copilot review on #8: - byteChannel.UpdateFrom now recovers *only* the "send on closed channel" panic and re-panics anything else, so unrelated/programmer bugs still surface instead of being silently turned into a sink error. - sink_test.go: make the regression comment self-contained (drop the internal Freshdesk ticket reference) since this is a public repo. Co-Authored-By: Claude Opus 4.8 --- sink.go | 11 ++++++++--- sink_test.go | 8 ++++---- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/sink.go b/sink.go index b56fd3e..7dbdb59 100644 --- a/sink.go +++ b/sink.go @@ -77,11 +77,16 @@ func (s *byteChannel) UpdateFrom(r io.Reader) (err error) { // The channel is owned by the caller (see ToChannel) and may be closed // concurrently — e.g. on shutdown or config reload — while a Runner is // mid-sync. A send on a closed channel panics, which would crash the whole - // process; recover and surface it as a sink error instead (Runner reports - // sink errors via OnSinkError rather than dying). + // process; recover *only that specific panic* and surface it as a sink + // error instead (Runner reports sink errors via OnSinkError rather than + // dying). Any other panic is re-raised so genuine bugs still surface. defer func() { if rec := recover(); rec != nil { - err = fmt.Errorf("send on byte channel: %v", rec) + if e, ok := rec.(error); ok && e.Error() == "send on closed channel" { + err = fmt.Errorf("byte channel sink: %w", e) + return + } + panic(rec) } }() b, err := ioutil.ReadAll(r) diff --git a/sink_test.go b/sink_test.go index d828553..52b24a1 100644 --- a/sink_test.go +++ b/sink_test.go @@ -5,10 +5,10 @@ import ( "testing" ) -// Regression test for the "send on closed channel" crash (Freshdesk #176970): -// the channel is owned by the caller and may be closed (shutdown / config -// reload) while a Runner is mid-sync. UpdateFrom must surface that as an error -// rather than panicking and crashing the whole process. +// Regression test for the "send on closed channel" crash: the channel is owned +// by the caller and may be closed (shutdown / config reload) while a Runner is +// mid-sync. UpdateFrom must surface that as an error rather than panicking and +// crashing the whole process. func TestByteChannelClosedDoesNotPanic(t *testing.T) { ch := make(chan []byte) close(ch)