htlcswitch: fix hodlQueue deadlock by stopping htlcManager first
What the developer wrote
Authored by ziggie
htlcswitch: fix hodlQueue deadlock by stopping htlcManager first
The channelLink.Stop() teardown had an inverted ordering that could
cause a permanent deadlock of the invoice registry under concurrent
peer disconnect.
The previous order was:
1. HodlUnsubscribeAll -- removes subscriptions
2. hodlQueue.Stop() -- kills the queue's internal goroutine
3. cg.Quit() -- signals htlcManager to stop
4. cg.WgWait() -- waits for htlcManager to exit
The race window between steps 2 and 4 left htlcManager alive. A
RevokeAndAck arriving during that window could drive processRemoteAdds
→ processExitHop → NotifyExitHopHtlc, registering a new hodl
subscription backed by a dead hodlQueue (ChanIn() has no reader).
Any subsequent call to notifyHodlSubscribers (e.g. MPP auto-release
timer, expiry watcher, or explicit settle/cancel) would then block
indefinitely on the unbuffered ChanIn(), holding hodlSubscriptionsMux.
Concurrent NotifyExitHopHtlc calls waiting for that lock, plus callers
holding the invoice-level lock waiting for those, produce a full
deadlock of the invoice registry with no recovery path short of a
daemon restart.
The fix is to stop htlcManager before touching the hodl subscription
state. htlcManager is the sole caller of NotifyExitHopHtlc, so once
cg.WgWait() returns no new subscriptions can be registered, making
HodlUnsubscribeAll and hodlQueue.Stop() race-free.
What changed, and why it matters
This commit fixes a bug in LND's payment channel shutdown code that could freeze the entire invoice handling system. The old shutdown order stopped a message queue before stopping the goroutine that uses it, creating a brief window where a new subscription could be attached to a dead queue. Once that happened, later invoice operations would block forever waiting for a queue that would never respond, deadlocking the invoice registry and requiring a full node restart to recover. The fix simply reverses the shutdown order so the worker goroutine exits before the queue is torn down.
Apply the patch. It is a minimal, correct shutdown-order fix with no API or protocol changes. Nodes running affected versions should upgrade to avoid invoice-registry deadlock under peer disconnect. No immediate mitigation is available beyond restart if the deadlock is observed.
Security signals we found
Deadlock in invoice registry leading to denial of service
Race condition during channel teardown on concurrent peer disconnect
Unbuffered channel send to stopped queue causing indefinite block
Lock-order inversion involving hodlSubscriptionsMux and invoice-level locks
Fix reorders shutdown sequence to close producer before consumer
Evidence from the diff
The patch changes channelLink.Stop() in htlcswitch/link.go to call cg.Quit()/cg.WgWait() before HodlUnsubscribeAll() and hodlQueue.Stop(). Previously the order was HodlUnsubscribeAll, hodlQueue.Stop, cg.Quit, cg.WgWait. The inverted order allowed htlcManager to process a RevokeAndAck after hodlQueue.Stop() killed the queue’s reader, causing processRemoteAdds → processExitHop → NotifyExitHopHtlc to register a hodl subscription whose ChanIn() had no consumer. Subsequent notifyHodlSubscribers calls would then block on the unbuffered ChanIn() while holding hodlSubscriptionsMux, deadlocking the invoice registry. The commit message explicitly describes this as a permanent deadlock with no recovery path except daemon restart.
Changed components
htlcswitch/link.gochannelLink.Stop()htlcManager goroutinehodlQueueinvoice registry (HodlUnsubscribeAll / NotifyExitHopHtlc / notifyHodlSubscribers)Inspect captured patch +14 / −5
diff --git a/htlcswitch/link.go b/htlcswitch/link.go
index 1db005b..641f046 100644
--- a/htlcswitch/link.go
+++ b/htlcswitch/link.go
@@ -629,8 +629,20 @@ func (l *channelLink) Stop() {
l.log.Info("stopping")
- // As the link is stopping, we are no longer interested in htlc
- // resolutions coming from the invoice registry.
+ // Stop the htlcManager goroutine first. This is critical: htlcManager
+ // is the sole caller of NotifyExitHopHtlc, which registers new hodl
+ // subscriptions. We must guarantee it has fully exited before we
+ // remove subscriptions and stop the hodlQueue. Without this ordering,
+ // a RevokeAndAck processed in the race window between hodlQueue.Stop()
+ // and cg.Quit() can register an orphaned subscription against a dead
+ // queue, causing notifyHodlSubscribers to block permanently and
+ // deadlock the entire invoice registry.
+ l.cg.Quit()
+ l.cg.WgWait()
+
+ // htlcManager has fully exited — no new hodl subscriptions can be
+ // registered from this point on. It is now safe to remove all
+ // subscriptions and tear down the queue.
l.cfg.Registry.HodlUnsubscribeAll(l.hodlQueue.ChanIn())
if l.cfg.ChainEvents.Cancel != nil {
@@ -651,9 +663,6 @@ func (l *channelLink) Stop() {
l.hodlQueue.Stop()
}
- l.cg.Quit()
- l.cg.WgWait()
-
// Now that the htlcManager has completely exited, reset the packet
// courier. This allows the mailbox to revaluate any lingering Adds that
// were delivered but didn't make it on a commitment to be failed back
Why this scored 70/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.