fix: prevent LN client access during shutdown#2096
fix: prevent LN client access during shutdown#2096im-adithya wants to merge 3 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughRemoved the lnClient parameter from Alby OAuth callback, introduced a service shutdown flag that makes GetLNClient() return nil during termination, and refactored many API handlers and mocks to fetch and nil-check a single LNClient per request with defensive nil-safe mock wrappers. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant HTTP as "HTTP Alby handler"
participant Alby as "AlbyOAuthService"
participant Svc as "Service"
participant LN as "LNClient"
Browser->>HTTP: GET /alby/callback?code=...
HTTP->>Alby: CallbackHandler(ctx, code)
Alby->>Svc: GetLNClient()
alt svc returns nil (shutting down)
Svc-->>Alby: nil
Alby-->>HTTP: error (no LN client)
HTTP-->>Browser: redirect/error response
else svc returns lnClient
Svc-->>Alby: lnClient
Alby->>LN: exchange code / link account
LN-->>Alby: success
Alby-->>HTTP: success/redirect
HTTP-->>Browser: redirect OK
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/lsp.go (1)
25-27:⚠️ Potential issue | 🟠 MajorAvoid breaking older clients that omit
lspType.Line 25 now rejects empty
request.LSPType; clients that previously relied on LSPS1 default behavior will fail. Default empty toLSPS1before validation.🔧 Proposed fix
- if request.LSPType != lsp.LSP_TYPE_LSPS1 { - return nil, fmt.Errorf("unsupported LSP type: %v", request.LSPType) + lspType := request.LSPType + if lspType == "" { + lspType = lsp.LSP_TYPE_LSPS1 + } + if lspType != lsp.LSP_TYPE_LSPS1 { + return nil, fmt.Errorf("unsupported LSP type: %v", lspType) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/lsp.go` around lines 25 - 27, The check on request.LSPType currently rejects empty values and breaks older clients; before validating against lsp.LSP_TYPE_LSPS1, treat an empty request.LSPType as lsp.LSP_TYPE_LSPS1 (e.g., set request.LSPType = lsp.LSP_TYPE_LSPS1 when it's empty or zero-valued) and then perform the existing comparison to avoid rejecting clients that rely on the default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/service.go`:
- Around line 35-37: Add a sync.RWMutex to the service struct and use it to
protect all accesses to lnClient and lnClientShuttingDown: introduce a field
(e.g., lnClientMu sync.RWMutex) in the same struct where lnClient and
lnClientShuttingDown are declared, wrap reads of lnClient/lnClientShuttingDown
in RLock/RUnlock (e.g., inside GetLNClient()) and wrap writes/assignments to
lnClient and lnClientShuttingDown in Lock/Unlock in the goroutines that modify
them (the places referenced in start.go where they are set), ensuring every read
and write uses the mutex.
In `@service/start.go`:
- Line 238: Avoid spawning a goroutine when there is no Lightning client by
checking the client before calling svc.nip47Service.HandleEvent: call
svc.GetLNClient() into a local variable, and only start go
svc.nip47Service.HandleEvent(ctx, pool, event.Event, ln) when that local
variable is non-nil; this keeps existing nil-guard in nip47/event_handler.go but
prevents allocating unnecessary goroutines during shutdown.
---
Outside diff comments:
In `@api/lsp.go`:
- Around line 25-27: The check on request.LSPType currently rejects empty values
and breaks older clients; before validating against lsp.LSP_TYPE_LSPS1, treat an
empty request.LSPType as lsp.LSP_TYPE_LSPS1 (e.g., set request.LSPType =
lsp.LSP_TYPE_LSPS1 when it's empty or zero-valued) and then perform the existing
comparison to avoid rejecting clients that rely on the default.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
alby/alby_oauth_service.goalby/models.goapi/api.goapi/backup.goapi/lsp.goapi/rebalance.goapi/transactions.gohttp/alby_http_service.goservice/service.goservice/start.gowails/wails_handlers.go
| db *gorm.DB | ||
| lnClient lnclient.LNClient | ||
| lnClientShuttingDown bool |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Access sites for lnClient/lnClientShuttingDown:"
rg -n -C2 '\blnClientShuttingDown\b|\blnClient\b' service/service.go service/start.go
echo
echo "Synchronization primitives present in these files:"
rg -n -C2 'sync\.(Mutex|RWMutex)|atomic\.' service/service.go service/start.goRepository: getAlby/hub
Length of output: 3906
🏁 Script executed:
#!/bin/bash
# Check the context around the goroutine launch at start.go:330
sed -n '325,335p' service/start.go
echo "---"
# Check if GetLNClient is called from other goroutines
rg -n 'GetLNClient\(' service/ | head -20Repository: getAlby/hub
Length of output: 790
🏁 Script executed:
#!/bin/bash
# Check if there are any channels or other sync mechanisms used around these fields
rg -n 'lnClientShuttingDown|lnClient' service/service.go service/start.go | grep -E '(chan|select|<-|->)'
echo "---"
# Check all writes to lnClient to see complete picture
rg -n 'svc\.lnClient\s*=' service/start.goRepository: getAlby/hub
Length of output: 87
Synchronize LN client state access to avoid data races.
lnClientShuttingDown and lnClient fields are accessed without synchronization across goroutines. Writes occur in a spawned goroutine (service/start.go:330-332, 390), while reads happen in GetLNClient() which is called from multiple goroutine contexts (service/start.go:92, 238, 304). Add sync.RWMutex to protect all reads and writes to these fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/service.go` around lines 35 - 37, Add a sync.RWMutex to the service
struct and use it to protect all accesses to lnClient and lnClientShuttingDown:
introduce a field (e.g., lnClientMu sync.RWMutex) in the same struct where
lnClient and lnClientShuttingDown are declared, wrap reads of
lnClient/lnClientShuttingDown in RLock/RUnlock (e.g., inside GetLNClient()) and
wrap writes/assignments to lnClient and lnClientShuttingDown in Lock/Unlock in
the goroutines that modify them (the places referenced in start.go where they
are set), ensuring every read and write uses the mutex.
| return | ||
| default: | ||
| go svc.nip47Service.HandleEvent(ctx, pool, event.Event, svc.lnClient) | ||
| go svc.nip47Service.HandleEvent(ctx, pool, event.Event, svc.GetLNClient()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and examine the file
echo "=== Locating service/start.go ==="
find . -name "start.go" -path "*/service/*" 2>/dev/null | head -20
echo
echo "=== Checking file size ==="
if find . -name "start.go" -path "*/service/*" 2>/dev/null | head -1 | xargs wc -l; then
echo "File found"
fiRepository: getAlby/hub
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read around line 238 in service/start.go
echo "=== Context around line 238 in service/start.go ==="
sed -n '230,250p' service/start.goRepository: getAlby/hub
Length of output: 715
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find GetLNClient implementation
echo "=== Looking for GetLNClient implementation ==="
rg -n 'func.*GetLNClient' --type go
echo
echo "=== Context around GetLNClient ==="
rg -n -A 10 'func.*GetLNClient' --type go | head -30Repository: getAlby/hub
Length of output: 2456
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find HandleEvent implementation
echo "=== Looking for HandleEvent implementation ==="
rg -n 'func.*HandleEvent' --type go | grep -i nip47
echo
echo "=== Context around nip47 HandleEvent ==="
rg -n -A 15 'func.*HandleEvent' --type go | grep -A 15 nip47 | head -40Repository: getAlby/hub
Length of output: 1343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for shutdown logic around line 238
echo "=== Context before line 238 ==="
sed -n '200,240p' service/start.go | tail -50Repository: getAlby/hub
Length of output: 1350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read GetLNClient implementation
echo "=== GetLNClient implementation (service/service.go) ==="
sed -n '259,280p' service/service.goRepository: getAlby/hub
Length of output: 582
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find lnclient.LNClient definition
echo "=== Looking for LNClient interface definition ==="
fd -e go -type f | xargs grep -l 'interface.*LNClient\|type.*LNClient.*interface' | head -5Repository: getAlby/hub
Length of output: 294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check full HandleEvent implementation for nil guards
echo "=== Full HandleEvent implementation ==="
sed -n '26,100p' nip47/event_handler.goRepository: getAlby/hub
Length of output: 2389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if lnClient is used directly without nil checks in HandleEvent
echo "=== Usage of lnClient parameter in HandleEvent ==="
rg -n -A 80 'func.*HandleEvent.*lnClient' nip47/ | head -100Repository: getAlby/hub
Length of output: 4780
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for shutdown or cleanup logic that might set client to nil
echo "=== Looking for shutdown/cleanup logic ==="
rg -n -C 5 'shutdown|Stop|Close' service/service.go | head -50Repository: getAlby/hub
Length of output: 374
HandleEvent already guards against nil LN client; consider checking before spawning for efficiency.
While svc.GetLNClient() can return nil during shutdown, the HandleEvent() function already includes a nil check at lines 50–55 of nip47/event_handler.go, so the code is safe from nil dereference. However, adding a check before spawning the goroutine (as suggested below) would avoid unnecessary goroutine allocation when the client is unavailable.
🔧 Optional optimization
- go svc.nip47Service.HandleEvent(ctx, pool, event.Event, svc.GetLNClient())
+ lnClient := svc.GetLNClient()
+ if lnClient == nil {
+ logger.Logger.Debug("Skipping NIP-47 event handling: LN client unavailable")
+ continue
+ }
+ go svc.nip47Service.HandleEvent(ctx, pool, event.Event, lnClient)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go svc.nip47Service.HandleEvent(ctx, pool, event.Event, svc.GetLNClient()) | |
| lnClient := svc.GetLNClient() | |
| if lnClient == nil { | |
| logger.Logger.Debug("Skipping NIP-47 event handling: LN client unavailable") | |
| continue | |
| } | |
| go svc.nip47Service.HandleEvent(ctx, pool, event.Event, lnClient) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/start.go` at line 238, Avoid spawning a goroutine when there is no
Lightning client by checking the client before calling
svc.nip47Service.HandleEvent: call svc.GetLNClient() into a local variable, and
only start go svc.nip47Service.HandleEvent(ctx, pool, event.Event, ln) when that
local variable is non-nil; this keeps existing nil-guard in
nip47/event_handler.go but prevents allocating unnecessary goroutines during
shutdown.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/mocks/Config.go (1)
1-3:⚠️ Potential issue | 🟠 MajorAvoid manually editing auto-generated mock files.
This file is generated by mockery (configured in
.mockery.yaml) and explicitly states "DO NOT EDIT". Manual modifications will be overwritten when mocks are regenerated by runningmockery, creating maintenance burden and confusion.As documented in the README, mocks should be generated via the mockery command, not edited manually. If nil-safe argument handling is needed, consider these alternatives:
- Update mockery version — newer versions may handle nil arguments differently
- Customize mockery template — add a custom template in
.mockery.yamlto generate the desired behavior- Fix at the call site — investigate why nil arguments are being passed to these mocks and address the root cause
Regenerate the mocks by running
mockeryrather than applying manual patches to the generated file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mocks/Config.go` around lines 1 - 3, The file tests/mocks/Config.go is an auto-generated mock from mockery and should not be manually edited; revert any manual changes to this file and instead regenerate the mock via the mockery tool (using your project's .mockery.yaml config) so changes persist; if you need nil-safe argument handling, either upgrade/configure mockery (update .mockery.yaml or use a custom template) or fix the tests/call sites that pass nil to the mocked methods rather than editing the generated mock (refer to the Config mock in tests/mocks/Config.go and your .mockery.yaml for configuration).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/mocks/Config.go`:
- Around line 1-3: The file tests/mocks/Config.go is an auto-generated mock from
mockery and should not be manually edited; revert any manual changes to this
file and instead regenerate the mock via the mockery tool (using your project's
.mockery.yaml config) so changes persist; if you need nil-safe argument
handling, either upgrade/configure mockery (update .mockery.yaml or use a custom
template) or fix the tests/call sites that pass nil to the mocked methods rather
than editing the generated mock (refer to the Config mock in
tests/mocks/Config.go and your .mockery.yaml for configuration).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/mocks/AlbyOAuthService.gotests/mocks/AlbyService.gotests/mocks/Config.gotests/mocks/EventPublisher.gotests/mocks/Keys.gotests/mocks/LNClient.gotests/mocks/Service.go
service/start.go
Outdated
| go func() { | ||
| // ensure the LNClient is stopped properly before exiting | ||
| <-ctx.Done() | ||
| svc.lnClientShuttingDown = true |
There was a problem hiding this comment.
can updating this boolean go inside the stopLNClient() method?
service/start.go
Outdated
| <-ctx.Done() | ||
| svc.lnClientShuttingDown = true | ||
| svc.stopLNClient() | ||
| svc.lnClientShuttingDown = false |
There was a problem hiding this comment.
this one could be in the defer func of stopLNClient
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/stop.go`:
- Around line 20-24: The lnClientShuttingDown boolean is accessed concurrently
(written in stopLNClient() and read in GetLNClient()) and must be synchronized
to avoid a data race; update the service struct to replace the plain bool with a
synchronized primitive (recommended: atomic.Bool from sync/atomic or a dedicated
sync.Mutex-protected bool), then update stopLNClient() to set/reset the atomic
Bool (or lock/unlock and set the guarded field) and update GetLNClient() to read
the shutdown flag using the matching atomic load (or mutex) so all accesses to
lnClientShuttingDown use the same synchronization mechanism.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
service/start.goservice/stop.go
🚧 Files skipped from review as they are similar to previous changes (1)
- service/start.go
| svc.lnClientShuttingDown = true | ||
| defer func() { | ||
| svc.lnClientShuttingDown = false | ||
| svc.wg.Done() | ||
| }() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the service package and its structure
echo "=== Locating service package files ==="
find . -type f -name "*.go" -path "*/service/*" | head -20
echo -e "\n=== Searching for lnClientShuttingDown field declaration ==="
rg -n 'lnClientShuttingDown\s*[:=]' -A 2 -B 2
echo -e "\n=== All accesses to lnClientShuttingDown ==="
rg -n -C 3 '\blnClientShuttingDown\b'Repository: getAlby/hub
Length of output: 1764
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Service struct definition and mutex fields ==="
rg -n -A 30 'type service struct' service/service.go
echo -e "\n=== GetLNClient function context (check for locks) ==="
rg -n -A 10 'func.*GetLNClient' service/service.go
echo -e "\n=== Check for goroutines and concurrent calls ==="
rg -n 'go\s*svc\.' service/
echo -e "\n=== stopLNClient context (full function) ==="
rg -n -A 20 'func.*stopLNClient' service/stop.goRepository: getAlby/hub
Length of output: 2291
Synchronize lnClientShuttingDown access to prevent data races during concurrent shutdown.
The field is declared as a plain bool (service/service.go:37) and accessed without synchronization in two places:
- Written in stopLNClient() at lines 20 and 22
- Read in GetLNClient() (service/service.go:260)
Since GetLNClient() is called from goroutines (e.g., start.go:238), concurrent reads and writes create a data race that makes the shutdown guard unreliable. Use atomic.Bool or a dedicated mutex consistently for all accesses.
🔧 Suggested fix direction
func (svc *service) stopLNClient() {
- svc.lnClientShuttingDown = true
+ svc.lnClientShuttingDown.Store(true)
defer func() {
- svc.lnClientShuttingDown = false
+ svc.lnClientShuttingDown.Store(false)
svc.wg.Done()
}()Change field declaration in service struct:
- lnClientShuttingDown bool
+ lnClientShuttingDown atomic.BoolUpdate GetLNClient():
func (svc *service) GetLNClient() lnclient.LNClient {
- if svc.lnClientShuttingDown {
+ if svc.lnClientShuttingDown.Load() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/stop.go` around lines 20 - 24, The lnClientShuttingDown boolean is
accessed concurrently (written in stopLNClient() and read in GetLNClient()) and
must be synchronized to avoid a data race; update the service struct to replace
the plain bool with a synchronized primitive (recommended: atomic.Bool from
sync/atomic or a dedicated sync.Mutex-protected bool), then update
stopLNClient() to set/reset the atomic Bool (or lock/unlock and set the guarded
field) and update GetLNClient() to read the shutdown flag using the matching
atomic load (or mutex) so all accesses to lnClientShuttingDown use the same
synchronization mechanism.
2 participants
Fixes #2018
Summary by CodeRabbit
Bug Fixes
Refactor