Skip to content

Conversation

@ben-kaufman
Copy link
Contributor

Addresses #472 (review)

@claude
Copy link

claude bot commented Nov 20, 2025

Code Review - PR #474: Move closed channels tracking to LightningRepo

Overview

This PR refactors closed channel tracking functionality from LightningService to LightningRepo, which improves separation of concerns. The service layer should handle low-level LDK operations, while the repository layer manages business logic and state.


✅ Strengths

  1. Better Separation of Concerns: Moving closed channel tracking to the repository layer is architecturally sound. LightningService should focus on LDK node operations, while LightningRepo handles business logic.

  2. Consistent Coroutine Usage: The refactored code properly uses withContext(bgDispatcher) instead of mixing ServiceQueue.LDK.background and ServiceQueue.CORE.background, which simplifies the concurrency model.

  3. Proper Error Handling: The registerClosedChannel function includes comprehensive error handling with detailed logging for missing channel details and funding transactions.


🔍 Potential Issues & Recommendations

1. Thread Safety Concern - Channel Cache Updates

Severity: Medium

The channelCache operations are split between refreshChannelCache() and event handling:

private suspend fun refreshChannelCache() = withContext(bgDispatcher) {
    val channels = lightningService.channels ?: return@withContext
    channels.forEach { channel ->
        channelCache[channel.channelId] = channel  // ← Update
    }
}

private fun handleLdkEvent(event: Event) {
    when (event) {
        is Event.ChannelClosed -> {
            scope.launch {
                registerClosedChannel(channelId, reason)  // ← Read then remove
            }
        }
    }
}

Issue: handleLdkEvent is called synchronously (not a suspend function), and launches coroutines on scope which may use a different dispatcher than bgDispatcher. This could cause race conditions between:

  • refreshChannelCache() updating the cache on bgDispatcher
  • registerClosedChannel() reading/removing from the cache on scope's dispatcher

Recommendation: Make handleLdkEvent a suspend function and ensure all cache operations happen on the same dispatcher:

private suspend fun handleLdkEvent(event: Event) = withContext(bgDispatcher) {
    when (event) {
        is Event.ChannelClosed -> {
            registerClosedChannel(channelId, reason)
        }
        // ...
    }
}

Then update the call sites to use scope.launch:

lightningService.listenForEvents(onEvent = { event ->
    scope.launch {
        handleLdkEvent(event)
    }
    eventHandler?.invoke(event)
})

2. Race Condition - Channel Closed Before Cache Population

Severity: Medium

In the start() method:

lightningService.start(timeout) { event ->
    handleLdkEvent(event)
    eventHandler?.invoke(event)
    ldkNodeEventBus.emit(event)
}
// ...
refreshChannelCache()  // ← Called AFTER start completes

Issue: If a ChannelClosed event fires during node startup (before refreshChannelCache() is called), the channel won't be in the cache, and you'll get an error log.

Recommendation: Call refreshChannelCache() before starting event processing, or handle the race condition more gracefully in registerClosedChannel().

3. Memory Leak Risk - Unbounded Cache Growth

Severity: Low-Medium

The channelCache stores all channels indefinitely. While closed channels are removed, there's no cleanup mechanism if:

  • refreshChannelCache() fails partway through
  • Channels become stale but aren't closed properly
  • The cache grows large over time with many channels

Recommendation: Consider adding:

  • A maximum cache size with LRU eviction
  • Periodic cleanup of stale entries
  • Clear documentation about cache lifecycle

4. Error Recovery - Missing Channel Details

Severity: Low

When a channel isn't found in the cache, the code logs an error and returns:

val channel = channelCache[channelId] ?: run {
    Logger.error(
        "Could not find channel details for closed channel: channelId=$channelId",
        context = TAG
    )
    return@withContext
}

Issue: This silently fails to persist closed channel information, which might be important for user history.

Recommendation: Consider attempting to fetch channel details from lightningService.channels as a fallback before giving up, or store partial information if available.

5. Exception Handling Inconsistency

Severity: Low

In the original code (LightningService:758):

} catch (e: Exception) {

In the new code (LightningRepo:393):

} catch (e: Throwable) {

Note: Catching Throwable is more comprehensive but also catches Error subclasses (like OutOfMemoryError). This is generally acceptable in top-level handlers, but ensure this is intentional.


🔒 Security Considerations

No significant security concerns identified. The code:

  • ✅ Properly validates required fields (fundingTxo)
  • ✅ Uses safe string operations (take, orEmpty)
  • ✅ Logs errors without exposing sensitive data

⚡ Performance Considerations

  1. Positive: Removing ServiceQueue.LDK.background and ServiceQueue.CORE.background reduces thread context switching and queue overhead.

  2. Minor Concern: refreshChannelCache() iterates through all channels on every sync. For users with many channels, consider:

    • Only updating changed channels
    • Batching updates
    • Using a more efficient data structure if channel counts grow large

🧪 Test Coverage

Concern: No new tests were added for the refactored functionality.

Recommendation: Add unit tests for:

  • handleLdkEvent() with different event types
  • registerClosedChannel() success and failure cases
  • refreshChannelCache() behavior
  • Race condition scenarios

The existing LightningRepoTest.kt would be the appropriate place for these tests.


📋 Additional Notes

  1. Code Quality: The refactoring maintains consistent Kotlin idioms and follows the existing codebase patterns.

  2. Logging: Comprehensive logging is present, which will help with debugging.

  3. Documentation: Consider adding KDoc comments to handleLdkEvent() and registerClosedChannel() explaining their purpose and threading guarantees.


Summary

This is a solid architectural improvement. The main concerns are around thread safety and race conditions with the channel cache. I recommend addressing the concurrency issues before merging to avoid potential bugs in production.

Recommendation: Request changes to address the thread safety concerns in points #1 and #2.

Copy link
Member

@jvsena42 jvsena42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested:

  • Open and close a channel
  • Wipe and restore app -> check channels

@jvsena42 jvsena42 merged commit 14a29cc into master Nov 20, 2025
14 checks passed
@jvsena42 jvsena42 deleted the chore/move-closed-channels branch November 20, 2025 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants