Fix Shutdown deadlock in some scenarios by ziggie1984 · Pull Request #10044 · lightningnetwork/lnd

ziggie1984 · 2025-07-07T08:10:48Z

In case LND fails to startup correctly and fail before we would start the SubSwapper we would deadlock when shutting down the server here:

lnd/chanbackup/pubsub.go

Lines 206 to 207 in ff32e90

    
           case s.manualUpdates <- update: 
        
           case <-s.quit:

Happend while fixing: #10041

gemini-code-assist

Summary of Changes

Hello @ziggie1984, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a critical shutdown deadlock within the chanbackup component, specifically when the backup process might not have fully initialized. My changes enhance the SubSwapper's lifecycle management by introducing atomic state tracking and robust startup/shutdown logic, ensuring operations are only performed on a fully initialized component. I've also updated the release notes to reflect this important fix.

Highlights

Deadlock Fix: I've addressed a shutdown deadlock in the chanbackup component that could occur if the backuper had not fully started. This was achieved by refining the SubSwapper's lifecycle management.
Concurrency Control Improvement: I've replaced sync.Once with atomic.Bool for managing the SubSwapper's started and stopped states. This provides more explicit, robust, and thread-safe control over the component's initialization and termination.
State Validation: I've added checks to prevent operations like ManualUpdate on a SubSwapper that has not been successfully started. Additionally, the started state is now reset if the initial updateBackupFile() call fails during startup, preventing a partially initialized state.
Documentation: I've updated the release notes to include an entry for this fix, detailing the resolution of the shutdown deadlock.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request fixes a potential deadlock in the chanbackup.SubSwapper by refactoring the start/stop logic from sync.Once to atomic.Bool. This allows for more robust state management, including resetting the started state on a failed start and checking if the service is running before performing a manual update. The changes look good and address the described issue. I've found one potential issue in the new Stop() implementation that could lead to a deadlock and suggested a fix.

chanbackup/pubsub.go

ziggie1984 · 2025-07-07T09:00:48Z

/gemini review

gemini-code-assist

Code Review

This pull request fixes a potential shutdown deadlock by replacing sync.Once with atomic.Bool for managing the SubSwapper's lifecycle. This change allows for more robust state management, especially in startup failure scenarios. I've suggested one improvement to prevent a stopped SubSwapper from being restarted, which will make the state transitions more predictable.

chanbackup/pubsub.go

Roasbeef · 2025-07-07T20:47:25Z

chanbackup/pubsub.go

 // Start starts the chanbackup.SubSwapper.
 func (s *SubSwapper) Start() error {
-	var startErr error
-	s.started.Do(func() {


How did the sync.Once usage mask things here? Mainly that w/ the public API you can't tell if something has actually run once or not?

yeah we have the call here:

lnd/server.go

Line 2801 in b815109

err := s.chanSubSwapper.ManualUpdate(singles)

but if we never started the swapper (because we fail before) we will deadlock there and not shutdown cleanly.

starius

LGTM! 💾
Great fix! I added small proposal to improve code readability.

chanbackup/pubsub.go

starius · 2025-07-08T13:12:19Z

chanbackup/pubsub.go

+
+	// stopped tracks whether the SubSwapper has been stopped and ensures
+	// it can only be stopped once.
+	stopped atomic.Bool


Can we remove stopped and just use quit state as an indication of closeness? These two variables seem to have the same meaning.

chanbackup/pubsub.go

bhandras

LGTM 🎉

bhandras · 2025-07-08T14:17:32Z

chanbackup/pubsub.go

-			return
-		}
+	// Ensure we only start the SubSwapper once.
+	if !s.started.CompareAndSwap(false, true) {


nit: an even simpler way to do this is to just:

if s.started.Swap(true) { // already started }

starius

LGTM!

starius · 2025-07-08T16:11:02Z

chanbackup/pubsub.go

+	}
+
+	s.wg.Add(1)
+	go s.backupUpdater()


ready is used only by Start and backupUpdater.
We can make it local to Start and pass to backupUpdater as an argument.

ziggie1984 · 2025-07-08T17:19:15Z

/gemini review

gemini-code-assist

Code Review

This pull request aims to fix a deadlock that can occur during shutdown if the SubSwapper is not started correctly. The changes involve using an atomic.Bool to track the started state and adding checks to prevent operations on a non-started SubSwapper.

The core logic change in ManualUpdate correctly prevents the deadlock. The use of a ready channel in Start to wait for the backupUpdater goroutine is also a good improvement for robustness.

However, I've identified a critical race condition in the Start method's implementation. The check and set of the started flag are not atomic, which could lead to issues under concurrent calls. I've provided suggestions to address this.

The release note update is clear and appropriate.

chanbackup/pubsub.go

ziggie1984 · 2025-07-08T17:52:41Z

/gemini review

gemini-code-assist

Code Review

This pull request aims to fix a shutdown deadlock in SubSwapper by introducing an isActive flag to prevent updates when the swapper is not running. This is a good approach, but the current implementation has a race condition where ManualUpdate can still block if Stop is called concurrently. I've provided a suggestion to fix this race condition.

Additionally, I've pointed out a misleading comment that was updated and suggested a correction to improve code clarity.

chanbackup/pubsub.go

Roasbeef

LGTM 🪖

gemini-code-assist bot reviewed Jul 7, 2025

View reviewed changes

chanbackup/pubsub.go Outdated Show resolved Hide resolved

ziggie1984 force-pushed the fix-shutdown-issue branch 2 times, most recently from adc4c55 to 3c468e0 Compare July 7, 2025 08:20

ziggie1984 requested a review from starius July 7, 2025 08:21

ziggie1984 self-assigned this Jul 7, 2025

gemini-code-assist bot reviewed Jul 7, 2025

View reviewed changes

chanbackup/pubsub.go Show resolved Hide resolved

saubyk added this to the v0.19.2 milestone Jul 7, 2025

Roasbeef reviewed Jul 7, 2025

View reviewed changes

ziggie1984 requested a review from Roasbeef July 7, 2025 22:17

starius approved these changes Jul 8, 2025

View reviewed changes

ziggie1984 force-pushed the fix-shutdown-issue branch from 3c468e0 to d9f57f5 Compare July 8, 2025 13:46

ziggie1984 requested review from bhandras and removed request for Roasbeef July 8, 2025 13:48

bhandras approved these changes Jul 8, 2025

View reviewed changes

ziggie1984 force-pushed the fix-shutdown-issue branch from d9f57f5 to fa165e8 Compare July 8, 2025 15:06

ziggie1984 requested a review from starius July 8, 2025 15:07

starius approved these changes Jul 8, 2025

View reviewed changes

ziggie1984 force-pushed the fix-shutdown-issue branch from fa165e8 to a119e0a Compare July 8, 2025 17:09

ziggie1984 requested a review from Roasbeef July 8, 2025 17:10

gemini-code-assist bot reviewed Jul 8, 2025

View reviewed changes

chanbackup/pubsub.go Outdated Show resolved Hide resolved

chanbackup/pubsub.go Outdated Show resolved Hide resolved

ziggie1984 force-pushed the fix-shutdown-issue branch 2 times, most recently from 1b87eac to be2c0a0 Compare July 8, 2025 17:51

gemini-code-assist bot reviewed Jul 8, 2025

View reviewed changes

chanbackup/pubsub.go Show resolved Hide resolved

chanbackup/pubsub.go Outdated Show resolved Hide resolved

ziggie1984 added 2 commits July 8, 2025 20:10

chanbackup: fix shutdown issue when backuper has not started yet

7a363ee

docs: add release-notes

354585e

ziggie1984 force-pushed the fix-shutdown-issue branch from be2c0a0 to 354585e Compare July 8, 2025 18:10

ziggie1984 requested review from bhandras and starius July 8, 2025 18:10

starius approved these changes Jul 8, 2025

View reviewed changes

Roasbeef approved these changes Jul 8, 2025

View reviewed changes

Roasbeef merged commit d40ac45 into lightningnetwork:master Jul 8, 2025
35 of 40 checks passed

Roasbeef mentioned this pull request Jul 9, 2025

release: create v0.19.2-rc2 branch #10047

Merged

Conversation

ziggie1984 commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

ziggie1984 commented Jul 7, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Roasbeef Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

ziggie1984 Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

starius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

starius Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

bhandras Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

starius left a comment

Choose a reason for hiding this comment

Uh oh!

starius Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

ziggie1984 commented Jul 8, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

ziggie1984 commented Jul 8, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ziggie1984 commented Jul 7, 2025 •

edited

Loading