Fix flake in `TestChangeWalletPasswordStateless` #10075

yyforyongyu · 2025-07-15T11:57:02Z

Have seen this a few times, e.g., from this build,

--- FAIL: TestChangeWalletPasswordStateless (0.31s)
    testing.go:1232: TempDir RemoveAll cleanup: unlinkat /tmp/TestChangeWalletPasswordStateless3763216656/001/mainnet: directory not empty
FAIL

Turns out there's a goroutine we should wait for in the unit test.

gemini-code-assist

Summary of Changes

Hello @yyforyongyu, 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 addresses a persistent flakiness issue in the TestChangeWalletPasswordStateless test by ensuring proper synchronization with a background goroutine. It also includes minor refactorings to a test helper function and various cosmetic improvements to test comments and names.

Highlights

Test Flakiness Fix: Resolved a known flakiness in the TestChangeWalletPasswordStateless test, which was caused by a race condition during temporary directory cleanup.
Goroutine Synchronization: Implemented explicit waiting for the doChangePassword goroutine to complete in the affected tests, preventing premature resource cleanup and ensuring test stability.
Test Helper Refactoring: Simplified the doChangePassword helper function by removing wallet store interaction logic, making it more focused on the password change RPC call and reducing its responsibilities.
Test Comment & Name Improvements: Applied several minor corrections to test function comments and names for improved clarity and grammatical accuracy.

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 addresses a flaky test, TestChangeWalletPasswordStateless, by ensuring the test waits for a helper goroutine to complete before exiting. This is achieved by waiting on a channel for the goroutine's result. The changes also include a refactoring of the doChangePassword test helper function to remove filesystem operations, simplifying its logic and removing potential sources of race conditions with test cleanup. The overall changes are solid and improve test stability and code quality.

gemini-code-assist · 2025-07-15T11:59:11Z

walletunlocker/service_test.go

@@ -491,7 +491,7 @@ func TestChangeWalletPasswordNewRootKey(t *testing.T) {
 	// password that meets the length requirement, the password change
 	// should succeed.
 	errChan := make(chan error, 1)
-	go doChangePassword(service, testDir, req, errChan)


Adding a select block with a timeout ensures the test doesn't hang indefinitely if the doChangePassword goroutine fails to complete, improving test reliability.

select { case err := <-errChan: t.Fatalf("ChangePassword call failed: %v", err) case <-time.After(defaultTestTimeout): t.Fatalf("password not received") }

@gemini-code-assist I think it's done below, where this select is added to make sure it doesn't hang.

You are absolutely right, @yyforyongyu! My apologies for missing that. The select blocks added in DIFF 6 and DIFF 9 effectively address the potential for the test to hang by waiting for the doChangePassword goroutine to complete. This resolves the concern perfectly. Thank you for pointing that out!

gemini-code-assist · 2025-07-15T11:59:11Z

walletunlocker/service_test.go

@@ -491,7 +491,7 @@ func TestChangeWalletPasswordNewRootKey(t *testing.T) {
 	// password that meets the length requirement, the password change
 	// should succeed.
 	errChan := make(chan error, 1)
-	go doChangePassword(service, testDir, req, errChan)


The testDir argument is no longer needed in doChangePassword function, since the function no longer performs file operations. Removing it simplifies the function call and reduces the chance of passing incorrect or irrelevant data.

Suggested change

go doChangePassword(service, testDir, req, errChan)

go doChangePassword(service, req, errChan)

@gemini-code-assist It's removed?

Yes, that's correct. Looking at DIFF 9 and DIFF 10 in the provided patches, the testDir parameter has been removed from the doChangePassword function's signature, and all its usages within the function body have also been removed. This is why it's no longer necessary to pass it in the function call, simplifying the code as mentioned in my previous comment.

yyforyongyu · 2025-07-15T12:18:54Z

/gemini review

gemini-code-assist

Code Review

This pull request effectively fixes a flaky test, TestChangeWalletPasswordStateless, by ensuring the test waits for a spawned goroutine to complete before exiting. This is achieved by waiting on a channel, which is the correct approach for this kind of synchronization issue.

The refactoring of the doChangePassword helper function is a significant improvement. It simplifies the code by removing the now-unnecessary testDir parameter and eliminating buggy verification logic, making the helper more focused and robust. The other changes, such as fixing typos and aligning comments with function names, enhance the overall readability and maintainability of the test suite. The changes are well-executed and directly address the root cause of the flakiness.

guggero

Thanks for the fix. Though I think we shouldn't remove the validation...

guggero · 2025-07-15T14:15:54Z

walletunlocker/service_test.go

-
-	// Close the macaroon DB and try to open it and read the root key with
-	// the new password.
-	store, err := openOrCreateTestMacStore(


Why do we remove this validation? I think it makes sense to check that we actually have changed the password...

I don't think it actually checks anything - ran it locally and realized this line was always blocked at this line, only found it when I started to catch the error in the end,

lnd/walletunlocker/service_test.go

Line 115 in df6c02e

db, err := kvdb.Create(

But this re-opens the macaroon store with the new password, to make sure it was actually changed...

yeah I don't think it actually checks anything - it was using the old password before (da27748), yet the test never failed. Because we didn't close the old db, so this openOrCreateTestMacStore was always blocked and never finished. Because we didn't wait for the goroutine to finish before, it was never caught.

This is now fixed in the final commit with an assertion that the new password is taking effect.

walletunlocker/service_test.go

We are missing a return here, and we are testing opening the macaroon db using the new password, not the old one.

Make sure we wait for `doChangePassword` goroutine to finish before exiting the test. We also remove the `openOrCreateTestMacStore` call inside `doChangePassword` as it was never finished.

yyforyongyu added this to the v0.20.0 milestone Jul 15, 2025

yyforyongyu self-assigned this Jul 15, 2025

yyforyongyu added flake fix no-itest no-changelog size/micro small bug fix or feature, less than 15 mins of review, less than 250 labels Jul 15, 2025

gemini-code-assist bot reviewed Jul 15, 2025

View reviewed changes

yyforyongyu force-pushed the fix-TestChangeWalletPasswordStateless branch from 0702723 to 2a72ee0 Compare July 15, 2025 12:16

gemini-code-assist bot reviewed Jul 15, 2025

View reviewed changes

guggero requested changes Jul 15, 2025

View reviewed changes

yyforyongyu added 3 commits July 16, 2025 01:28

walletunlocker: add missing return and wrong password

da27748

We are missing a return here, and we are testing opening the macaroon db using the new password, not the old one.

walletunlocker: wait for goroutine to finish before exit

3a6e656

Make sure we wait for `doChangePassword` goroutine to finish before exiting the test. We also remove the `openOrCreateTestMacStore` call inside `doChangePassword` as it was never finished.

walletunlocker: fix typos in the tests

c643c47

yyforyongyu force-pushed the fix-TestChangeWalletPasswordStateless branch from 2a72ee0 to c643c47 Compare July 15, 2025 17:29

yyforyongyu requested a review from guggero July 15, 2025 17:29

walletunlocker: assert new password can be used to open the store

9039cd1

	go doChangePassword(service, testDir, req, errChan)
	go doChangePassword(service, req, errChan)

Fix flake in TestChangeWalletPasswordStateless #10075

Are you sure you want to change the base?

Fix flake in TestChangeWalletPasswordStateless #10075

Uh oh!

Conversation

yyforyongyu commented Jul 15, 2025

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!

gemini-code-assist bot Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

yyforyongyu Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

yyforyongyu Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

yyforyongyu commented Jul 15, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

guggero Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

yyforyongyu Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

guggero Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

yyforyongyu Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Fix flake in `TestChangeWalletPasswordStateless` #10075

Fix flake in `TestChangeWalletPasswordStateless` #10075