Keep litd running after lnd stops#1329
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! 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 decouples the lifecycle of litd from the embedded lnd instance. Previously, litd would exit whenever lnd stopped. With these changes, litd now maintains its own shutdown source, allowing it to continue running after lnd terminates. This ensures that the status endpoint remains available to report lnd's failure state, improving observability. The change also includes updates to the itest harness to handle the new shutdown behavior and ensures that lnd-dependent sub-servers are cleaned up gracefully when lnd stops. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
Customization To customize the 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request decouples the lifecycle of litd from lnd in integrated mode by introducing a dedicated shutdownSource for litd. This allows litd to remain running and serve its status endpoint even after lnd stops. The feedback highlights two key issues: first, the new signal test in shutdown_test.go uses Unix-specific syscalls that will break compilation on Windows, and should be moved to a build-constrained file; second, the integration test testLitdSurvivesLndShutdown incorrectly initializes the test node in remote mode instead of integrated mode, which misses the target behavior.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // TestShutdownSourceSignal checks that an OS termination signal closes the | ||
| // shutdown channel. The signal is captured by signal.Notify, so it does not | ||
| // terminate the test process. | ||
| func TestShutdownSourceSignal(t *testing.T) { | ||
| // Not parallel: this sends a process-wide signal, which every | ||
| // registered shutdownSource would observe. | ||
| s := newShutdownSource() | ||
| defer s.Stop() | ||
|
|
||
| requireOpen(t, s.ShutdownChannel()) | ||
|
|
||
| require.NoError(t, syscall.Kill(syscall.Getpid(), syscall.SIGTERM)) | ||
|
|
||
| waitClosed(t, s.ShutdownChannel()) | ||
| } |
There was a problem hiding this comment.
This test uses syscall.Kill and syscall.SIGTERM, which are not supported on Windows and will cause compilation failures on Windows platforms. To keep the test suite cross-platform, please move TestShutdownSourceSignal to a separate file (e.g., shutdown_unix_test.go) with the //go:build !windows build constraint.
| node, err := net.NewNode( | ||
| t.t, "lnd-stop", nil, false, true, "--autopilot.disable", | ||
| ) |
There was a problem hiding this comment.
The 5th argument to net.NewNode is remoteMode. Passing true runs the test in remote mode, which does not test the integrated mode lifecycle changes introduced in this PR. This should be set to false to test the integrated mode behavior.
| node, err := net.NewNode( | |
| t.t, "lnd-stop", nil, false, true, "--autopilot.disable", | |
| ) | |
| node, err := net.NewNode( | |
| t.t, "lnd-stop", nil, false, false, "--autopilot.disable", | |
| ) |
|
Thanks for the review.
|
|
Sorry for taking some time with this one @Vandit1604, I'll review this tomorrow. |
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Thanks a lot for this contribution @Vandit1604! This actually works really well, and was a smaller change that I thought it would be. I've tested it locally, and it introduces the functionality that we'd like, so thanks for that 🔥.
Here are the things I've tested:
- A
log.Criticalerror inlndshutsdown justlndand + dependent sub-servers, and notlitd. Something to note is that it however just leaves "lnd shutdown" in thelitcli statuserror forlndthough, not the actual error. But I think that's good enough. - A runtime error in
lnddoes shutdown justlndand the dependent sub-servers, but notlitd. It does also print the real error inlitcli statuslnderror. - An error in
litddoes shut downlndas well.
There is however some architectural feedback that I think needs to be resolved. I'm leaving those in my first 2 comments below. The rest of the feedback is just minor.
Great work 🔥
| // tear down the lnd-dependent sub-servers, and keep litd running so its | ||
| // status endpoint stays available. Only an OS signal, litd StopDaemon, | ||
| // a critical litd error or a fatal sub-system error stops litd. | ||
| for { |
There was a problem hiding this comment.
With the change you've included here, I think this observing here that remains up until litd is placed incorrectly. It's currently placed in the start function, but the right place for this should be the Run function. I therefore suggest you just return nil once you've set LIT as running above, and make this the observing part the responsibility of the Run function.
There was a problem hiding this comment.
Done. start now returns nil once LIT is set to running, and the steady state wait lives in Run as waitForShutdown. Run blocks there, observes lndQuit to tear down the lnd-dependent sub-servers if lnd stops, and only returns on a real litd shutdown.
| // a critical litd error or a fatal sub-system error stops litd. | ||
| for { | ||
| select { | ||
| case err := <-g.errQueue.ChanOut(): |
There was a problem hiding this comment.
Hmmm I don't really like here that this introduces different behaviour for what an error sent to the g.errQueue.ChanOut channel means depending on if it is during the startup of litd, or after all sub-servers have been started. I.e. with this change, an error during startup means that litd is shutdown, but is later changed to just mean that it's logged.
I think you need to separate that with two different channels or something similar, where sending over one of them means "fatal" errors, and the one which is used after this Start function has returned is non-fatal and logged only errors.
There was a problem hiding this comment.
Done. The post-startup channel is now a dedicated non-fatal one (subServerErrs), read only in waitForShutdown after start has returned, and those errors are logged only. Fatal errors are no longer routed through it: startup failures are returned directly from start, and an lnd stop during startup comes in over lndQuit. The middleware is the only component that reports async errors and it only does so at runtime, so the queue now has a single, unambiguous meaning.
| // Note that Go delivers a signal to every channel registered via | ||
| // signal.Notify, so listening here does not prevent lnd's own | ||
| // signal.Interceptor from also receiving the same signal. An OS signal | ||
| // therefore stops both lnd and litd, the same as before this change. |
There was a problem hiding this comment.
Commenting specifically regarding the "the same as before this change" part of this comment:
We usually don't mention changes from previous behaviour in the docs. This is because the previous version is irrelevant to people reviewing the current state of the code, unless knowledge about that previous version of the code is explicitly needed to understand why a specific code behaviour is implemented the way it is, which is not the case here.
There was a problem hiding this comment.
Done, dropped the comparison to the previous behaviour.
| if litdShutdown != nil { | ||
| litdShutdown.RequestShutdown() | ||
| } | ||
| interceptor.RequestShutdown() |
There was a problem hiding this comment.
nit: new line before this line.
| // its interceptor. Both are needed now that litd no longer shuts down | ||
| // just because lnd's interceptor fired. |
There was a problem hiding this comment.
See comment https://github.com/lightninglabs/lightning-terminal/pull/1329/changes#r3435314091 as it applies here as well.
There was a problem hiding this comment.
Done, reworded without referencing the old behaviour.
| // If start() failed before creating a client, we will just wait for the | ||
| // child process to die. | ||
| if !hn.Cfg.RemoteMode && hn.LightningClient != nil { | ||
| // Calling lnd's StopDaemon no longer shuts down litd (litd keeps running |
There was a problem hiding this comment.
Same point regarding mentioning previous behaviour here: "no longer shuts down litd".
| // Don't watch for an error: the connection may already be | ||
| // closing (e.g. litd has begun shutting down), which is fine. |
There was a problem hiding this comment.
I think you can just require that the error is that specific type if the err != nil, instead of not checking the error at all.
There was a problem hiding this comment.
Done. It now returns the error unless it is an Unavailable status, which is the expected case when the connection is torn down as litd shuts down.
| t *harnessTest) { | ||
|
|
||
| // Use a dedicated, throwaway node so that stopping its lnd cannot | ||
| // affect any other test. Disable autopilot to keep the node quiet. |
There was a problem hiding this comment.
Not sure what the disable autopilot to "keep the node quiet." is referring to hear. But I think you can just remove that part of the comment and keep the passed "--autopilot.disable" arg.
There was a problem hiding this comment.
Done, removed that part of the comment and kept the --autopilot.disable arg.
|
|
||
| return nil | ||
| }, defaultTimeout) | ||
| require.NoError(t.t, err) |
There was a problem hiding this comment.
You should also test here that the lit sub-server remains up and running after this, i.e. that statusClient.SubServerStatus still responds and that the LIT sub-server the has "Running" set to true. Could be worth testing that this is the case for maybe 3 more seconds or something similar.
There was a problem hiding this comment.
Done. After lnd is reported stopped, the test now checks that the LIT sub-server stays reachable and Running for 3 seconds.
| @@ -0,0 +1,29 @@ | |||
| //go:build !windows | |||
There was a problem hiding this comment.
This last commit can be squashed with the first one, instead of being a separate commit :)
There was a problem hiding this comment.
Done, folded into the first commit.
1f1ca2f to
dc882bb
Compare
|
Thanks for the thorough review. I've pushed an update addressing all the points:
Replied inline on each thread too. |
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Thanks for the updates @Vandit1604. Unfortunately though, I think these changes were a bit more confusing than the previous version and still have the same behaviour. I've therefore left a comment below of an alternative solution building upon your previous version of the PR, which would solve the issue.
Let me know if you'd want some additional clarifications!
| // lndQuit is observed both here (during startup, where an lnd stop is | ||
| // fatal) and by Run (after startup, where it tears down the lnd-dependent | ||
| // sub-servers but keeps litd alive). It lives on g so Run can reach it. | ||
| g.lndQuit = make(chan struct{}) | ||
| lndQuit := g.lndQuit |
There was a problem hiding this comment.
Hmmm now we have the same behaviour where wanted to avoid but for your newly added g.lndQuit channel instead (i.e. that an error sent over the channel means different thing depending on wether we've started or starting)...
I think this new version is more confusing than your last version as well. I therefore suggest reverting to your previous version, and building upon that.
When looking through that version again, I noticed the following which would probably a clean way we could handle this instead:
As the lnd shutdown doesn't trigger the <-g.shutdown.ShutdownChannel() to be sent over, not even during the startup phase, we can just let it be the case that errors from the start function just triggers the lit sub-server to be stopped, but the status server keeps being up and running. This also applies during the startup phase of the start function.
That way we can keep g.errQueue as the main error source, and not have to add these additional channels i.e. keep it the way it was during your previous version of this PR.
However, due to the signaling change (i.e. that stopping lnd doesn't trigger <-g.shutdown.ShutdownChannel()), we need to also change one thing with the error checking of the start function in the Run function, i.e.:
// Attempt to start Lit and all of its sub-servers. If an error is
// returned, it means that either one of Lit's internal sub-servers
// could not start or LND could not start or be connected to.
startErr := g.start(ctx)
if startErr != nil {
g.statusMgr.SetErrored(
subservers.LIT, "could not start Lit: %v", startErr,
)
}
We can't let litd remain up and running if the lnd errors during the startup, when the following hasn't been run yet in start:
// Now start the RPC proxy that will handle all incoming gRPC, grpc-web
// and REST requests.
if err := g.rpcProxy.Start(g.lndConn, bakeSuperMac); err != nil {
return fmt.Errorf("error starting lnd gRPC proxy server: %v",
err)
}
The reason for that is that users won't be able to run litcli stop before that if we keep the status server up and running.
Therefore, in the startErr check in the Run function, you'd want to check if the rpcProxy has been started or not. If it hasn't litd should shutdown as well.
Finally, to keep the behaviour cooherent, you should also change so that the lit sub-server is always stopped when lnd is stopped, independently of wether lnd stops during the startup phase, or during runtime after its been started.
Let me know if you need any clarifications to the above!
|
Thanks for the detailed write up. I reworked it close to what you suggested:
There is one part I could not get working: having Here is exactly what fails and why, in case you see something I missed. When the wait blocks inside So the proxied I ruled out:
The litd shutdown log sequence is the same in both versions, so it looks like a teardown timing or scheduling difference between litd tearing down its proxy and lnd sending GOAWAY on that proxied stream. I could not pin the exact cause. Net effect: |
7741a9f to
684dd7e
Compare
684dd7e to
a46f463
Compare
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Thanks for the update @Vandit1604. This looks better, but I still think there are some spots where you didn't revert back to using the errQueue as the source for errors.
You have also added the "revert" in an additional commit as the second last commit of the PR. Please squash it with the "terminal: keep litd running after lnd stops" commit, as the current commit structure makes this quite complicated to review.
Also responding to part of your comment above:
There is one part I could not get working: having start() itself block and return the error on a runtime lnd stop.
I'm sorry if I was unclear there, that was not the approach I intended to convey, so your idea of blocking in Run is the correct approach.
There was a problem hiding this comment.
We still want to pass the runtime errors into the errQueue, as we reverted to it being the "single source" of errors.
There was a problem hiding this comment.
You've removed this check for the errQueue.ChanOut() here and in many other spots in this file. I don't think that is what you want after reverting back to it being the "single error source". We especially want it to return here if lnd fails during startup, so we don't proceed to start the other sub-servers (i.e. loop, tapd etc. if lnd startup failed)
It should be fine to return here now, as your other logic should have changed that the litd daemon doesn't shutdown when lnd has stopped, as the status server is kept up and running.
fdb8737 to
3f2475a
Compare
|
@ViktorT-11 I pushed the review changes (errQueue back as the single error source, and squashed the rework into the "keep litd running after lnd stops" commit). Before I push a fix for the failing CI I wanted your call on it, since it touches the itest harness. CI fails on The tests stop a node by calling lnd's The harness already switched to stopping nodes through litd's own Two harness-only ways to fix it, no production change:
I am leaning towards option 1. Does that sound right, or would you prefer a different approach? |
|
Thanks for the changes and the writeup @Vandit1604! Regarding the issue with stateless init mode:
I propose one alternative solution you could try first: This is how you'd create such a macaroon in a real stateless init setup:
The outputted litd macaroon will then be saved to Obviously for the Test if you can get that working. Else I agree with you that option 1 is best of the 2 options you suggested. |
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Just leaving a few minor comments after looking through the new changes. I haven't tested this again, so these are primarily quick feedback and a question for you.
| "Error running main lnd: %v", err, | ||
| ) | ||
| log.Errorf(lndErr) | ||
| g.statusMgr.SetErrored(subservers.LND, lndErr) |
There was a problem hiding this comment.
nit: don't remove the new line above this line.
| select { | ||
| case err := <-g.errQueue.ChanOut(): | ||
| return fmt.Errorf("error from subsystem: %v", err) | ||
| return fmt.Errorf("error while starting: %w", err) |
There was a problem hiding this comment.
I don't think this necessarily only have to be startup issues, so I would not change this. Potentially you could keep the change to use %w though.
| @@ -1614,7 +1734,7 @@ func (g *LightningTerminal) shutdownSubServers() error { | |||
| } | |||
There was a problem hiding this comment.
Hmmm in what scenario does this change lead to that the errQueue.ChanOut now contains an error that hasn't been handled prior to the shutting down litd and that we'd therefore only want to log and not assign to returnErr?
fd045b9 to
5f59b69
Compare
|
Pushed the updates, thanks for the detailed review. The three inline nits are done:
I also squashed the rework into the "terminal: keep litd running after lnd stops" commit, so the history is clean now. For the stateless init issue I went with your suggestion. In Locally this passes: both the stateless init test and the litd survives lnd shutdown test pass, with no regressions. One small note: the "Bob 200" line is just a non fatal log from CI looks like it is waiting on approval to run. Once it is kicked off and passes, this should be good to merge from my side. |
5f59b69 to
f67c4e8
Compare
|
@ViktorT-11: review reminder |
|
|
||
| ### Functional Changes/Additions | ||
|
|
||
| * [Keep litd running after lnd |
There was a problem hiding this comment.
Sorry for the delay in the review here. I'll get this reviewed before the end of the week!
In the meantime, please update this to instead be part of the release notes for v0.17.1:
https://github.com/lightninglabs/lightning-terminal/blob/master/docs/release-notes/release-notes-0.17.1.md
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Thanks again for this contribution @Vandit1604! Appologies for taking some time to review this again.
This looks very close from my point of view now. Just leaving a few minor comments below.
Additionally, I've also tested once more that the changes in this PR ensures the following:
- A
log.Criticalerror inlndshutsdown justlndand + dependent sub-servers, and notlitd. Something to note is that it however just leaves "lnd shutdown" in thelitcli statuserror forlndthough, not the actual error. But I think that's good enough. - A runtime error in
lnddoes shutdown justlndand the dependent sub-servers, but notlitd. It does also print the real error inlitcli statuslnderror. - An error in
litddoes shut downlndas well.
| // listens for OS signals independently of lnd's interceptor. | ||
| g.shutdown = newShutdownSource() | ||
| defer g.shutdown.Stop() | ||
| litdShutdown = g.shutdown |
There was a problem hiding this comment.
Instead of letting litdShutdown be a global variable in log.go that is assigned here, I think it'd make more sense to pass a reference to it in the log.go SetupLoggers & rpc_proxy.go newRpcProxy functions.
The reason behind that is in that in general it's better to avoid global variables if possible, and especially here it makes code readability worse as it's hard to understand that the terminal.go Run function needs to run prior to the executing the dependent code in log.go & rpc_rpoxy.go.
| // reports coherently, and tear down the lnd-dependent | ||
| // sub-servers while keeping litd running. | ||
| g.lndStopped = true | ||
| g.statusMgr.SetStopped(subservers.LIT) |
There was a problem hiding this comment.
When we shutdown the lit sub-server due to that lnd has stopped, we can set it as errored with an error explaining that it was stopped as lnd was shut down.
| // litConn is the underlying connection to Lit's grpc endpoint. | ||
| litConn *grpc.ClientConn | ||
|
|
||
| // litMacPath, when set, is the macaroon file used to authenticate |
There was a problem hiding this comment.
You can break out these changes which are related to being able to shutdown the itest litd node in stateless init though the litcli StopDaemon into a separate PR, as it is quite separate to the rest of the changes in this PR.
We can then review that separately, and base this PR on that new PR which contains only those changes.
| // longer running, since litd cannot function without lnd, but the | ||
| // endpoint continuing to answer at all is what proves litd did not | ||
| // cascade down with lnd. Check this holds for a few seconds. | ||
| for i := 0; i < 3; i++ { |
There was a problem hiding this comment.
Instead of doing this for loop with a time.Sleep, you can instead use the wait.Invariant() helper. That aligns better with how we typically test that a specific condition remains true over time (i.e. in this case that the litd status endpoint remains up).
| // litd itself must keep running after lnd has stopped. Check that the | ||
| // LIT sub-server stays reachable and reports as running for a few | ||
| // seconds, confirming it does not shut down shortly after lnd does. | ||
| // litd's process itself must keep running after lnd has stopped: its |
There was a problem hiding this comment.
This last commit can be squashed with the commit called:
"itest: add test for litd surviving lnd shutdown"
f67c4e8 to
c62c684
Compare
c62c684 to
9950a34
Compare
|
Thanks for the review @ViktorT-11. All addressed, rebased on latest master.
Lit is now set errored with "lit was stopped as lnd was shut down" instead of just stopped. The itest checks the error string too. Also done: The harness change is split out into #1356. The bottom two commits here are from it. Locally |
3 participants
Closes #1201 (initial implementation).
Makes litd keep running after lnd stops in integrated mode, so the status endpoint stays up to report the failure instead of litd exiting along with lnd.
Based on #1356, which splits out the itest harness change. The bottom two commits here belong to that PR.
Why not the wrapper from the issue
The pinned lnd's
signal.Interceptcan only be called once, its channels are unexported, andlnd.Maintakes the interceptor by value, so a wrapper type can't be passed in. Instead litd owns its own shutdown source and stops tying its lifecycle to lnd's interceptor.What it does
StopDaemon, and critical litd errors still stop everything. litdStopDaemonalso stops the embedded lnd.Behaviour change to flag
Calling lnd's
StopDaemon(for examplelncli stop) no longer stops litd in integrated mode. That is the goal of the issue but it is user facing. The itest harness now stops litd through its ownStopDaemon, and outside tooling that relied on the old cascade would need the same. I can add a config flag to keep the old behaviour if you prefer.Testing
litd survives lnd shutdown: stop lnd, then check the status endpoint still answers and reports lnd not running