Conversation
Contributor
I couldn't replicate it locally. Do you have an input that triggers the deadlock case? |
Collaborator
Author
|
I believe this was one of the inputs. |
Contributor
|
I couldn't replicate it locally. I've run it 200 times and didn't deadlock. |
It turns out that removing the SIGKILL from lightningd/subd.c can cause CLN to deadlock on shutdown, which holds up coverage reporting. We can resolve most of the deadlock cases by closing the subdaemon socket and allowing the subdaemon to gracefully shut down.
Fields are dropped in declaration order, so the first field is dropped before the second. This change ensures our connection to the peer is always closed before we attempt to shut down the target. This only matters for local mode -- when fuzzing in Nyx the snapshot is always restored before the scenario object is dropped anway. In local mode, it avoids a shutdown deadlock for CLN where our open connection prevents CLN from shutting down cleanly until connectd's keepalive ping fails and causes CLN to drop the connection itself.
morehouse
force-pushed
the
cln_coverage_deadlock
branch
from
September 18, 2026 17:43
ed09485 to
6e62dce
Compare
Collaborator
Author
|
Thanks for pushing on this -- it made look harder at what was actually happening, and I think I've come up with a better approach now that avoids
Yeah, sorry. That input reproduces the issue against CLN's master branch, but not the v26.06.6 we have pinned. I have better repro inputs below that work on v26.06.6. There's actually two different deadlocks I ran into: Subdaemon socket deadlockA CLN subdaemon blocks waiting for input from it's connection to lightningd. lightningd blocks waiting for the subdaemon to shut down. Fixed in the first commit by closing the subdaemon socket to allow a graceful shutdown. Repro input: Peer connection socket deadlockA CLN subdaemon blocks waiting for input from its peer connection. lightningd blocks waiting for the subdaemon to shut down. Fixed in the second commit by closing our peer connections with the target before tearing the target down. Repro input: |
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
2 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.
It turns out that removing the
SIGKILLfromlightningd/subd.ccan cause CLN to deadlock on shutdown, which holds up coverage reporting. Add theSIGKILLback in, but first attempt to gracefully shutdown by closing the subdaemon socket and waiting 5s for it. The socket close handles most of the deadlock cases while preserving coverage data. The fallbackSIGKILLcauses some coverage data to be lost but prevents coverage runs from deadlocking forever.