diff --git a/app/src/main/java/to/bitkit/repositories/LightningRepo.kt b/app/src/main/java/to/bitkit/repositories/LightningRepo.kt index 92ed77da0..249fd3996 100644 --- a/app/src/main/java/to/bitkit/repositories/LightningRepo.kt +++ b/app/src/main/java/to/bitkit/repositories/LightningRepo.kt @@ -18,6 +18,7 @@ import kotlinx.collections.immutable.persistentListOf import kotlinx.collections.immutable.toImmutableList import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Job +import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.MutableStateFlow @@ -141,6 +142,8 @@ class LightningRepo @Inject constructor( private val syncMutex = Mutex() private val syncPending = AtomicBoolean(false) private val syncRetryJob = AtomicReference(null) + private val pendingStopJob = AtomicReference(null) + private val pendingStopLock = Any() private val lifecycleMutex = Mutex() private val isChangingAddressType = AtomicBoolean(false) @@ -307,6 +310,8 @@ class LightningRepo @Inject constructor( return@withContext Result.failure(RecoveryModeError()) } + cancelPendingStop() + eventHandler?.let { _eventHandlers.add(it) } // Track retry state outside mutex to avoid deadlock (Mutex is non-reentrant) @@ -457,6 +462,24 @@ class LightningRepo @Inject constructor( _lightningState.update { it.copy(nodeLifecycleState = NodeLifecycleState.Initializing) } } + /** + * Defers [stop] so a brief background/foreground cycle does not tear the node down and rebuild it. + * Runs on the repo scope so a cancelled ViewModel cannot drop the pending stop. + * + * Scheduling and cancelling are atomic: [cancelPendingStop] runs on the repo dispatcher while this + * runs on the caller thread, so an interleaved cancel could otherwise miss the job being installed + * and stop the node after the app is back in the foreground. + */ + fun stopDebounced() = synchronized(pendingStopLock) { + val job = scope.launch { + delay(BACKGROUND_STOP_DELAY) + stop() + } + pendingStopJob.getAndSet(job)?.cancel() + } + + fun cancelPendingStop() = synchronized(pendingStopLock) { pendingStopJob.getAndSet(null)?.cancel() } + suspend fun stop(): Result = withContext(bgDispatcher) { lifecycleMutex.withLock { if (_lightningState.value.nodeLifecycleState.isStoppedOrStopping()) { @@ -465,10 +488,12 @@ class LightningRepo @Inject constructor( } runCatching { - _lightningState.update { it.copy(nodeLifecycleState = NodeLifecycleState.Stopping) } - lightningService.stop() - clearProbeOutcomes() - _lightningState.update { LightningState(nodeLifecycleState = NodeLifecycleState.Stopped) } + withContext(NonCancellable) { + _lightningState.update { it.copy(nodeLifecycleState = NodeLifecycleState.Stopping) } + lightningService.stop() + clearProbeOutcomes() + _lightningState.update { LightningState(nodeLifecycleState = NodeLifecycleState.Stopped) } + } }.onFailure { Logger.error("Node stop error", it, context = TAG) // On failure, check actual node state and update accordingly @@ -1729,6 +1754,7 @@ class LightningRepo @Inject constructor( private const val VSS_KEY_EXTERNAL_SCORES_CACHE = "external_pathfinding_scores_cache" private const val MS_SYNC_LOOP_DEBOUNCE = 500L private const val SYNC_RETRY_DELAY_MS = 15_000L + private val BACKGROUND_STOP_DELAY = 3.seconds private val CHANNELS_USABLE_TIMEOUT = 15.seconds private val NO_USABLE_CHANNELS_FEEDBACK_DELAY = 2_500.milliseconds val SEND_LN_TIMEOUT = 10.seconds diff --git a/app/src/main/java/to/bitkit/services/LightningService.kt b/app/src/main/java/to/bitkit/services/LightningService.kt index c240b5da0..c3a318ec4 100644 --- a/app/src/main/java/to/bitkit/services/LightningService.kt +++ b/app/src/main/java/to/bitkit/services/LightningService.kt @@ -3,6 +3,7 @@ package to.bitkit.services import com.synonym.bitkitcore.AddressType import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Job +import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.ensureActive import kotlinx.coroutines.flow.MutableSharedFlow @@ -12,6 +13,7 @@ import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeout +import kotlinx.coroutines.withTimeoutOrNull import kotlinx.serialization.Serializable import org.lightningdevkit.ldknode.Address import org.lightningdevkit.ldknode.AddressTypeBalance @@ -48,8 +50,10 @@ import to.bitkit.data.SettingsStore import to.bitkit.data.backup.VssStoreIdProvider import to.bitkit.data.keychain.Keychain import to.bitkit.di.BgDispatcher +import to.bitkit.di.IoDispatcher import to.bitkit.env.Defaults import to.bitkit.env.Env +import to.bitkit.ext.runSuspendCatching import to.bitkit.ext.uByteList import to.bitkit.ext.uri import to.bitkit.models.OpenChannelResult @@ -68,6 +72,7 @@ import javax.inject.Singleton import kotlin.coroutines.cancellation.CancellationException import kotlin.io.path.Path import kotlin.time.Duration +import kotlin.time.Duration.Companion.seconds import org.lightningdevkit.ldknode.AddressType as LdkAddressType typealias NodeEventHandler = suspend (Event) -> Unit @@ -81,6 +86,7 @@ data class AddressDerivationInfo( @Singleton class LightningService @Inject constructor( @BgDispatcher private val bgDispatcher: CoroutineDispatcher, + @IoDispatcher private val ioDispatcher: CoroutineDispatcher, private val keychain: Keychain, private val vssStoreIdProvider: VssStoreIdProvider, private val settingsStore: SettingsStore, @@ -97,6 +103,9 @@ class LightningService @Inject constructor( private const val SCORING_CONSIDERED_IMPOSSIBLE_PENALTY_MSAT = 1_000_000_000_000uL private const val SCORING_PROBING_DIVERSITY_PENALTY_MSAT = 60_000uL + /** How long a node stop waits for the rust handle to be released before moving on. */ + private val NODE_RELEASE_TIMEOUT = 1.seconds + private val DEFAULT_SCORING_FEE_PARAMETERS = ScoringFeeParameters( basePenaltyMsat = 1_024uL, basePenaltyAmountMultiplierMsat = 131_072uL, @@ -325,14 +334,39 @@ class LightningService @Inject constructor( } Logger.debug("Stopping node…", context = TAG) - ServiceQueue.LDK.background { - runCatching { node.stop() } - .onFailure { if (it !is NodeException.NotRunning) throw it } - this@LightningService.node = null + // Teardown must not be abandoned midway: a cancelled caller would leave the rust node alive + // and let the GC free it later on the finalizer thread, racing the next node. + withContext(NonCancellable) { + ServiceQueue.LDK.background { + runSuspendCatching { node.stop() } + .onFailure { + if (it !is NodeException.NotRunning) Logger.warn("Node stop error", it, context = TAG) + } + this@LightningService.node = null + } + releaseHandle(node) } Logger.info("Node stopped", context = TAG) } + /** + * Releases the rust handle instead of leaving it to the GC finalizer, keeping it off the + * single-threaded LDK queue and off the lifecycle critical path. + * + * `free_node` only returns once the node's background tasks drain, which takes tens of seconds + * when a failed start left them wedged, so the wait is bounded and the release is joined rather + * than run inline: cancelling a blocking FFI call does nothing, but abandoning the join lets the + * caller continue while the release finishes on its own. + */ + private suspend fun releaseHandle(node: Node) { + val release = launch(ioDispatcher) { + runSuspendCatching { node.destroy() } + .onFailure { Logger.warn("Node handle release error", it, context = TAG) } + } + withTimeoutOrNull(NODE_RELEASE_TIMEOUT) { release.join() } + ?: Logger.warn("Node handle release still pending after $NODE_RELEASE_TIMEOUT", context = TAG) + } + fun wipeStorage(walletIndex: Int) { if (node != null) throw ServiceError.NodeStillRunning() Logger.warn("Wiping LDK storage…", context = TAG) diff --git a/app/src/main/java/to/bitkit/viewmodels/WalletViewModel.kt b/app/src/main/java/to/bitkit/viewmodels/WalletViewModel.kt index aae350dc6..6f5fca654 100644 --- a/app/src/main/java/to/bitkit/viewmodels/WalletViewModel.kt +++ b/app/src/main/java/to/bitkit/viewmodels/WalletViewModel.kt @@ -334,13 +334,7 @@ class WalletViewModel @Inject constructor( fun stop() { if (!walletExists) return - viewModelScope.launch(bgDispatcher) { - lightningRepo.stop() - .onFailure { - Logger.error("Node stop error", it) - ToastEventBus.send(it) - } - } + lightningRepo.stopDebounced() } fun refreshState() = viewModelScope.launch { diff --git a/app/src/test/java/to/bitkit/repositories/LightningRepoTest.kt b/app/src/test/java/to/bitkit/repositories/LightningRepoTest.kt index 5d721ce3f..7220c0e26 100644 --- a/app/src/test/java/to/bitkit/repositories/LightningRepoTest.kt +++ b/app/src/test/java/to/bitkit/repositories/LightningRepoTest.kt @@ -14,9 +14,11 @@ import com.synonym.bitkitcore.Scanner import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.async +import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import org.junit.Before import org.junit.Test @@ -75,6 +77,7 @@ import kotlin.time.Duration.Companion.seconds class LightningRepoTest : BaseUnitTest() { companion object { private const val NO_USABLE_CHANNELS_FEEDBACK_DELAY_MS = 2_500L + private const val BACKGROUND_STOP_DELAY_MS = 3_000L } private lateinit var sut: LightningRepo @@ -205,6 +208,79 @@ class LightningRepoTest : BaseUnitTest() { } } + @Test + fun `stopDebounced does not stop the node before the delay elapses`() = test { + startNodeForTesting() + + sut.stopDebounced() + testScheduler.advanceTimeBy(BACKGROUND_STOP_DELAY_MS - 1) + + verify(lightningService, never()).stop() + } + + @Test + fun `stopDebounced stops the node after the delay elapses`() = test { + startNodeForTesting() + + sut.stopDebounced() + testScheduler.advanceTimeBy(BACKGROUND_STOP_DELAY_MS) + testScheduler.advanceUntilIdle() + + verify(lightningService).stop() + assertEquals(NodeLifecycleState.Stopped, sut.lightningState.value.nodeLifecycleState) + } + + @Test + fun `stopDebounced called twice only stops once`() = test { + startNodeForTesting() + + sut.stopDebounced() + sut.stopDebounced() + testScheduler.advanceUntilIdle() + + verify(lightningService, times(1)).stop() + } + + // Regression: a brief background and foreground cycle must not tear the node down and rebuild it + @Test + fun `a background and foreground cycle within the debounce window never stops the node`() = test { + startNodeForTesting() + + sut.stopDebounced() + testScheduler.advanceTimeBy(BACKGROUND_STOP_DELAY_MS - 1) + sut.start() + testScheduler.advanceUntilIdle() + + verify(lightningService, never()).stop() + assertEquals(NodeLifecycleState.Running, sut.lightningState.value.nodeLifecycleState) + } + + // Regression: node teardown must complete before the next start rebuilds, never overlap it + @Test + fun `stop tears down the node before a subsequent start rebuilds it`() = test { + startNodeForTesting() + whenever(lightningService.node).thenReturn(null) + + sut.stop() + sut.start() + + val inOrder = inOrder(lightningService) + inOrder.verify(lightningService).stop() + inOrder.verify(lightningService).setup(any(), anyOrNull(), anyOrNull(), anyOrNull(), anyOrNull()) + inOrder.verify(lightningService).start(anyOrNull(), any()) + } + + // Regression: a cancelled caller must not strand lifecycle state at Stopping + @Test + fun `stop leaves lifecycle state Stopped when the caller is cancelled`() = test { + startNodeForTesting() + + val job = launch { sut.stop() } + job.cancelAndJoin() + + assertEquals(NodeLifecycleState.Stopped, sut.lightningState.value.nodeLifecycleState) + } + @Test fun `resetNetworkGraph clears local cache and VSS copy`() = test { whenever(vssBackupClientLdk.setup(any())).thenReturn(Result.success(Unit)) diff --git a/app/src/test/java/to/bitkit/services/LightningServiceTest.kt b/app/src/test/java/to/bitkit/services/LightningServiceTest.kt index e77bf24ae..439a36d48 100644 --- a/app/src/test/java/to/bitkit/services/LightningServiceTest.kt +++ b/app/src/test/java/to/bitkit/services/LightningServiceTest.kt @@ -1,9 +1,15 @@ package to.bitkit.services +import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.launch import org.junit.Before import org.junit.Test import org.lightningdevkit.ldknode.Node +import org.lightningdevkit.ldknode.NodeException import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.times +import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import to.bitkit.data.SettingsStore import to.bitkit.data.backup.VssStoreIdProvider @@ -12,6 +18,7 @@ import to.bitkit.ext.createChannelDetails import to.bitkit.test.BaseUnitTest import to.bitkit.utils.LoggerLdk import kotlin.test.assertFalse +import kotlin.test.assertNull import kotlin.test.assertTrue class LightningServiceTest : BaseUnitTest() { @@ -27,6 +34,7 @@ class LightningServiceTest : BaseUnitTest() { fun setUp() { sut = LightningService( bgDispatcher = testDispatcher, + ioDispatcher = testDispatcher, keychain = keychain, vssStoreIdProvider = vssStoreIdProvider, settingsStore = settingsStore, @@ -56,4 +64,63 @@ class LightningServiceTest : BaseUnitTest() { assertTrue(sut.canReceive()) } + + @Test + fun `stop destroys the node handle and clears it`() = test { + sut.stop() + + verify(node).stop() + verify(node).destroy() + assertNull(sut.node) + } + + @Test + fun `stop destroys the node handle when it is already not running`() = test { + whenever(node.stop()).thenThrow(NodeException.NotRunning("not running")) + + sut.stop() + + verify(node).destroy() + assertNull(sut.node) + } + + @Test + fun `stop is a no-op when no node is set`() = test { + sut.node = null + + sut.stop() + + verify(node, never()).destroy() + } + + // Regression: a cancelled caller must not abandon teardown, leaving the rust node to the GC finalizer + @Test + fun `stop completes teardown when the caller is cancelled`() = test { + val job = launch { sut.stop() } + + job.cancelAndJoin() + + verify(node).destroy() + assertNull(sut.node) + } + + // Regression: a failing node stop must still release the handle instead of rethrowing and leaking it + @Test + fun `stop destroys the node handle when node stop throws`() = test { + whenever(node.stop()).thenThrow(NodeException.ConnectionFailed("boom")) + + sut.stop() + + verify(node).destroy() + assertNull(sut.node) + } + + // Regression: destroying twice would surface later as IllegalStateException from callWithPointer + @Test + fun `consecutive stop calls destroy the handle only once`() = test { + sut.stop() + sut.stop() + + verify(node, times(1)).destroy() + } } diff --git a/changelog.d/next/1100.fixed.md b/changelog.d/next/1100.fixed.md new file mode 100644 index 000000000..e4cb04a71 --- /dev/null +++ b/changelog.d/next/1100.fixed.md @@ -0,0 +1 @@ +Lightning node teardown now releases native resources deterministically and no longer restarts the node when the app is only briefly backgrounded.