fix: recover send-on-closed-channel panic in byteChannel sink#8
Merged
Conversation
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR prevents a process-crashing send on closed channel panic in the ToChannel sink by making byteChannel.UpdateFrom recover from a closed-channel send and return it as a sink error, and adds regression tests to validate both the closed-channel and normal-delivery behaviors.
Changes:
- Add a
defer/recoverguard around the channel send inbyteChannel.UpdateFromto convert the panic into an error. - Add unit tests covering closed-channel behavior (no panic, returns error) and successful delivery.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sink.go | Adds panic recovery in the byte-channel sink to avoid crashing when the caller closes the channel. |
| sink_test.go | Adds regression + happy-path tests for byte-channel sink behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…t 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 <noreply@anthropic.com>
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
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.
Problem
byteChannel.UpdateFromdid an unguarded send to a caller-owned channel:A
Runner(Start) callssyncOnce→UpdateFromon a ticker. The channel is owned by the caller (ToChanneltakes a caller channel), and consumers close it on shutdown / config reload:getlantern/fronted—fronted.go:257(keeps the masquerade list current)getlantern/radiance—kindling/dnstt/parser.go:162When a tick fires the send while/after the consumer has closed the channel,
s.ch <- bpanics withsend on closed channel, which crashes the whole process.Observed crashing the macOS v9 client (9.1.9) — four
Runnergoroutines panicked simultaneously on shutdown. User-visible as "beta cannot be used, and can't even submit an in-app report" (a crashed app can't upload the report). Freshdesk #176970 / lantern-forum-cn #1543.Fix
The sink can't know whether the caller's channel is closed, so make the send close-safe: recover the panic and return it as a sink error.
Runner.syncOncealready treats sink failures as recoverable viaOnSinkError(keepcurrent.go:128-130) — so a closed channel now degrades to a logged sink error instead of a process crash. No consumer changes required; fixes every consumer at once.Tests
Added
sink_test.go:TestByteChannelClosedDoesNotPanic— send to a closed channel returns an error, no panic (regression).TestByteChannelDelivers— healthy send still delivers unchanged.go build,go vet, and the full test suite pass.Notes
mainbefore this PR, and it's cross-platform (keepcurrent + fronted run on every client), so any v9 build could hit the crash whenever fronted/dnstt reloads at the wrong instant — macOS just happened to capture the crash log.Stop()the Runner beforeclose(ch)to coordinate shutdown cleanly.🤖 Generated with Claude Code