fix(daemon): recover acceptLoop panics#1305
Open
mvanhorn wants to merge 1 commit into
Open
Conversation
Closes floatpane#1119. `go d.acceptLoop()` is launched without a recover, so a panic inside the goroutine (e.g. nil deref in `Accept`-returned conn state or in `addClient`) takes down the entire daemon. The maintainer noted the same risk applies to floatpane#979 / floatpane#980 / floatpane#981. Add a top-level defer with the existing pattern from main.go:4089: log the panic with a stack trace before letting the goroutine exit. The daemon stops accepting new connections after recovery, but existing clients and the sync loop survive, which is the documented expectation.
floatpanebot
previously requested changes
May 18, 2026
Member
floatpanebot
left a comment
floatpanebot
left a comment
There was a problem hiding this comment.
Hi @mvanhorn! Please fix the following issues with your PR:
- Title: Is too long (45 characters). The PR title must be strictly under 40 characters.
mvanhorn
changed the title
floatpanebot
dismissed
their stale review
Formatting issues have been resolved. Thank you!
FromSi
requested changes
May 18, 2026
| if r := recover(); r != nil { | ||
| log.Printf("daemon: acceptLoop panic recovered: %v\n%s", r, debug.Stack()) | ||
| } | ||
| }() |
Member
There was a problem hiding this comment.
This does not match the expected behavior: acceptLoop exits, while it should restart/continue running. At minimum, we could wrap the current loop body in an inner func inside the for loop, move the existing logic there, and place the defer func() recovery inside that inner function.
From issue:
Wrap the goroutine body in a recover that logs and restarts the loop.
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
3 participants
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?
Adds a top-level
defer func() { recover(); log }()to(*Daemon).acceptLoopso a panic in the accept goroutine logs with a stack trace instead of taking down the whole daemon.Closes #1119
Why?
daemon/daemon.go:120launchesgo d.acceptLoop()with no panic recovery. A panic inside the loop (or inaddClient/daemonrpc.NewConn) propagates up and the runtime aborts the process. The sync loop, IDLE watcher, and connected clients all die with it. The issue notes the same risk applies to the prior recovery fixes (#979 / #980 / #981).The recovery pattern matches the existing one at
main.go:4089: log the panic with aruntime/debug.Stack()trace and let the goroutine exit. The daemon stops accepting new connections after a recovered panic, but already-connected clients and the background sync loop keep running, which is what the issue body asks for.Verification
go build ./...cleango vet ./daemon/...cleango test ./daemon/...passesdaemon/daemon.go(one import, onedeferblock)