fix: harden ldk node teardown on stop

[jvsena42]

Relates to #982 , #986

Upstream issue: synonymdev/ldk-node#94

This PR hardens LDK node teardown so it cannot orphan, deadlock, or overlap the native node:

1. Releases the LDK node handle deterministically on stop instead of leaving it to the garbage collector, with the wait bounded so a stuck free_node can never brick startup
2. Makes teardown non-cancellable, so a cancelled caller (including one cancelled during listener cleanup) can't abandon it halfway
3. Fixes a self-join deadlock when a stop or listener re-arm is requested from inside an LDK event handler
4. Gates rebuild and destructive storage work on the previous node's release so native lifetimes never overlap, and moves config-change recovery to the background so that gate never blocks the user-facing error
5. Stops tearing the node down and rebuilding it when the app is only briefly backgrounded

Description

Play Vitals reports a SIGABRT on mainnet as the top production crash cluster. Symbolicating it locally against the unstripped libldk_node.so for the shipped ldk-node-android version resolved every frame and gave a clear cause:

#19 ldk_node::runtime::spawn_background_task  (chain-sync task, tokio worker thread)
#14 drop_slow<ElectrumRuntimeClient>          ← last Arc released here
#13 drop_in_place<ldk_node::runtime::Runtime / RuntimeMode>
#12 drop_in_place<tokio::runtime::Runtime → BlockingPool>
#11 tokio blocking/shutdown.rs:51 → panic: "Cannot drop a runtime in a context
    where blocking is not allowed"

The defect itself is upstream: ldk-node's chain-sync task drops the last reference to the Electrum client, which owns the tokio runtime, while running on a worker thread of that same runtime. Release Rust aborts on panic, so the process dies with no catchable error. That is filed as synonymdev/ldk-node#94 and this PR does not fix it. What this PR fixes is the Android teardown that made the window wide, non-deterministic, and prone to overlapping native lifetimes.

Deterministic, bounded, non-cancellable release

Stopping the node cleared the reference but never destroyed the handle, so the native node stayed alive until the garbage collector freed it on the finalizer thread at an arbitrary later moment, potentially while a new node was already running. The handle is now released as part of the stop itself.

The release is bounded, not unconditionally synchronous. free_node returns in tens of milliseconds for a healthy node but blocks for tens of seconds when a failed start left the node's background tasks wedged (measured: ~38 ms healthy vs ~40 s wedged). So the release runs on the IO dispatcher and the stop waits on it only up to a short timeout — long enough that a healthy release completes inline, but bounded so the lifecycle state (Stopped) is published promptly rather than blocking behind a wedged drain.

Teardown was also abandonable. The stop ran through a cancellable context and was triggered from a ViewModel scope, so an activity finishing mid-stop left a live native node orphaned with the lifecycle state stranded. The whole teardown — including the listener cleanup join at the top — is now non-cancellable once committed. Reading ldk-node also showed the stop path could not fail the way the code assumed: it reports an error only when the node is already stopped and swallows every internal failure, so a failed stop no longer rethrows and skips the release.

Listener lifecycle safety

WakeNodeWorker calls stop() synchronously from its registered LDK event handler, which runs on the listener job. listenerJob.cancelAndJoin() then waited on the very job it was running on — a self-join. The same shape applied to startEventListener()'s re-arm. A stop from inside a handler now skips joining its own job (detected via currentCoroutineContext(), not the shadowing class-scope coroutineContext), and a re-arm from inside a handler is a no-op. The listener loop also keys on node identity, not just the shared shouldListenForEvents flag (now @Volatile), so a racing start() cannot make the old loop poll a node destroy() already freed. startEventListener uses runSuspendCatching so it no longer silently swallows the cancellation.

Overlap prevention: release gate + non-blocking recovery

Even with a bounded stop, the old node can still be draining after Stopped is published. The destructive storage operations — wipeStorage(), resetNetworkGraph(), and the pathfinding-scores VSS deletes — plus the normal rebuild path now wait on the previous node's release before touching storage, so a delete never races, and a new node never builds over, storage the old node still owns. That wait is itself bounded (~90 s, comfortably above the observed ~40 s wedge): a stuck free_node throws NodeReleaseTimeout rather than proceeding (which would overlap) or blocking forever (which would permanently brick every future rebuild).

Why the Electrum server change deliberately skips the gate

The Electrum server-change rebuild is the one user path that opts out of the gate (awaitRelease = false). @settings_10 switches Electrum servers back to back, and gating would serialize each change behind the previous — possibly wedged, ~40 s — node's drain, timing out the E2E. Skipping it there is safe: a plain rebuild performs no destructive deletes, and building over a not-yet-freed node is exactly the pre-PR behaviour (the handle was never destroyed on stop, so every server change already rebuilt over a GC-pending node) — no worse than what ships today, while the destructive deletes that genuinely need isolation keep the gate. (The RGS server-change rebuild is not back-to-back and keeps the gate.)

To confirm this trade-off turns on a real wedge and not an E2E artifact, I measured the Electrum flow on a mainnet debug build (node on ssl://bitkit.to:9999):

• Changing to a realistic wrong server ssl://google.com:80 failed node.start() after ~25 s and left free_node still draining past 1 s — well beyond the ~38 ms healthy release.
• The pathological ~40 s wedge is a plain TLS handshake against a plain-TCP Electrum server (electrs) — i.e. selecting the wrong protocol for a real server, a genuine user misconfiguration that reproduces identically on mainnet, not something specific to the local E2E electrs.

So the wedge is real on mainnet, which means gating the Electrum rebuild would delay real users making back-to-back server changes after such a mistake, for no correctness benefit a gate-less rebuild actually needs. That is why the gate is removed for this path rather than kept and worked around only in the test.

Separately, the "wrong server" recovery must not make the user wait on that ~40 s drain either. restartWithElectrumServer/restartWithRgsServer surface the failure immediately and run the recovery restart on the repository's process-lifetime scope, and that recovery rebuild also runs with awaitRelease = false so restoring the previous config isn't gated behind the wedged node. An earlier revision that waited for the release inline turned a 0.6 s recovery stop into a 40 s one and timed out the @settings_10 E2E; the current shape surfaces the error in ~23 s.

Brief-background debounce

Backgrounding the app stopped the node immediately, so a short task switch cost a full stop plus a full rebuild. The stop from backgrounding is now deferred by a few seconds and cancelled if the app returns. Only the backgrounding path is deferred; the notification stop action, service teardown, notification worker, and restore migration all still stop immediately. The deferred stop is owned by the repository's process-lifetime scope so it cannot be silently dropped when the activity goes away, and every start() cancels a pending stop up front so a start that short-circuits on its guards can't leave one to fire against a foregrounded node.

Preview

short-pause.webm

long-pause.webm

QA Notes

Manual Tests

• 1a. Home → background the app and return within ~2s: no node teardown or rebuild, wallet stays responsive.
  • 1b. Background and wait past the window, then return: node stops, then rebuilds and reaches started.
• 2. regression: Background Payments → enable Get paid when Bitkit is closed and Keep Bitkit active in background → background the app: node keeps running, no teardown.
• 3. regression: Node notification → tap Stop: node stops immediately and the app task is removed.
• 4. regression: Relaunch after a notification stop: node rebuilds and reaches started.
• 5. regression: Settings → Lightning → restart node, and Electrum/RGS server change to a valid server: node stops and restarts normally.
• 6. regression: Receive a Lightning payment in the background (app closed, Background Payments on): payment is received and the notification is delivered.
• 7. Settings → Advanced → Electrum → enter an unreachable server → Connect: the error surfaces promptly (does not hang for the full recovery), and the node recovers to the previous server on its own.

Automated Checks

• Release + teardown coverage in LightningServiceTest.kt: deterministic handle release, release when the node is already stopped / when node stop throws, the no-node case, no double release, and teardown completing when the caller is cancelled (including cancellation during listener cleanup).
• Listener-safety coverage in LightningServiceTest.kt: a stop from inside an event handler completes without self-join deadlock, a re-arm from inside a handler keeps the listener running, an external stop cancels and joins the listener, and the loop stops polling a node once it is swapped out (no use-after-free).
• Release-gate coverage in LightningServiceTest.kt: rebuild (setup), wipeStorage, and resetNetworkGraph each wait for the previous release before proceeding; the gate adds no latency when no release is pending; the gate throws instead of blocking forever when the release never finishes; and the stop returns within its bound while the release is wedged. These use a real IO dispatcher plus a controllably delayed destroy().
• Repo coverage in LightningRepoTest.kt: deferred stop timing and cancellation, a foreground/background cycle within the window never stopping the node, and restartWithElectrumServer surfacing failure before the background recovery completes. WalletViewModelTest.kt: a foreground start cancels a pending deferred stop even while startup is still active.
• Each new regression test was verified load-bearing by reverting its fix and confirming it fails (or, for the unbounded-gate case, hangs) before restoring.
• Device validation on the emulator, backgrounding for 1–8 seconds: under the window the node stays running and start is skipped; past it the log order is always Stopping node… → Node stopped → Building node… → Node started, confirming teardown completes before startup on the healthy path.
• Device validation of the foreground-service path: with background payments and keep-active enabled, backgrounding produced no stop and the node kept processing LDK events.
• Device validation of the notification stop action (immediate, no double-stop from service teardown) and of a background Lightning receive.
• Device validation of the Electrum server change and non-blocking recovery against a server that accepts TCP but never completes the TLS handshake (the @settings_10 "wrong Electrum server" condition). The error surfaces in ~23 s (under the 30 s E2E budget) while the recovery rebuild runs in the background without gating on the wedged drain — no Waiting for previous node release… line appears for this path — and with no SIGABRT/tombstone:

  Changing ldk-node electrum server (connect)
  Node stopped                                          (old node, ~0.6s)
  Building node…                                        (wrong server)
  Failed ldk-node config change, recovering in background…   ← error surfaces (~23s)
  Building node…                                        (recovery, not gated on the wedged drain)
  Node started                                          (previous config restored)
  

• Device validation on a mainnet debug build that the Electrum flow genuinely wedges with realistic input, confirming the gate-skip is a correctness-neutral trade-off against a real wedge rather than an E2E-only workaround: changing to ssl://google.com:80 failed node.start() after ~25 s and left free_node still draining past 1 s (vs ~38 ms healthy). The ~40 s wedge itself is reproduced by a plain TLS handshake against a plain-TCP Electrum server, a genuine mainnet misconfiguration.
• Device validation that the gate still holds where it matters and adds no latency on the healthy path: restartNode and the backgrounding rebuild complete with no Waiting for previous node release… line when the previous release finished promptly.
• No Fatal signal, SIGABRT or tombstone appeared in logcat across any of the runs above.
• CI: standard compile, unit test, and detekt checks run by the PR bot.

[greptile-apps]

Greptile Summary

This PR makes LDK node shutdown deterministic and prevents lifecycle races. The main changes are:

• Releases native node handles with bounded waits.
• Keeps teardown running when callers are cancelled.
• Prevents listener self-joins and stale-node polling.
• Gates rebuilds and storage deletion on pending releases.
• Runs failed configuration recovery in the background.
• Debounces stops during brief app backgrounding.

Confidence Score: 5/5

This looks safe to merge.

• Deferred-stop scheduling and cancellation now use the same lock.
• Foreground starts cancel pending stops before startup guards.
• No blocking issues remain in the reviewed fix.

Important Files Changed

Filename	Overview
app/src/main/java/to/bitkit/repositories/LightningRepo.kt	Adds synchronized deferred stops, release-aware lifecycle operations, and background configuration recovery.
app/src/main/java/to/bitkit/services/LightningService.kt	Adds bounded native-handle release, release gates, non-cancellable teardown, and safer event-listener lifecycle handling.
app/src/main/java/to/bitkit/viewmodels/WalletViewModel.kt	Cancels pending stops before startup guards and uses debounced stopping for app background transitions.

_{Reviews (5): Last reviewed commit: "fix: remove gating for electrum setting ..." | Re-trigger Greptile}

[jvsena42]

iOS port assessment: not needed

Checked this against the iOS teardown paths (LightningService.stop, WalletViewModel.stopLightningNode, AppScene.handleScenePhaseChange). This PR's three changes each target a mechanism that is Android-specific and structurally absent on iOS, so there is nothing to port.

This PR's change	Android mechanism	iOS
Deterministic handle release (node.destroy(), bounded)	JVM cleared the reference but left the native node to the GC finalizer, freed at an arbitrary later moment, potentially racing a new node	ARC: dropping the last reference at self.node = nil frees the handle deterministically. No finalizer thread, no arbitrary-timing race, so no explicit release to add
Non-cancellable teardown (withContext(NonCancellable))	stop() ran in a cancellable context from a viewModelScope, so an activity finishing mid-stop orphaned a live native node	Swift task cancellation is cooperative and does not interrupt the synchronous FFI node.stop(). stop() also unlocks via defer, and no Activity-like scope tears it down
Deferred stop on backgrounding (stopDebounced)	Backgrounding stopped the node immediately, so a short task switch cost a full stop + rebuild — this is what made the race window wide	iOS never stops the node on backgrounding. handleScenePhaseChange only handles PIN lock and foreground reconnect; the only stop callers are resetNetworkGraph, wipe, and internal restart/recovery. No churn, nothing to debounce

The one genuinely shared item is the underlying defect, synonymdev/ldk-node#94 — which this PR explicitly does not fix, it only narrows the timing window. That's a Rust-side fix and applies to both platforms whenever it lands upstream; it isn't something to port at the app layer.

One minor, non-blocking note for the iOS side: listenForEvents holds a local strong node while parked at await node.nextEventAsync(), so after self.node = nil the actual free is deferred until that task unwinds rather than happening exactly at the assignment. It's far more bounded than a GC finalizer and there is no rebuild churn to race against, so it does not reproduce this crash — just the one place where iOS deallocation isn't strictly deterministic, if we ever want to tighten it.

[ovitrif]

I re-audited the teardown and lifecycle paths against the current head and reproduced the two inline races with focused coroutine tests.

[ovitrif]

I re-audited exact head 2955bd0 after the two earlier P1 fixes. Those cancellation and deferred-stop findings are resolved, and all 18 hosted checks plus the focused LightningServiceTest, LightningRepoTest, and WalletViewModelTest suites pass.

Two distinct high-priority native-lifecycle findings remain inline, so I’m leaving this as a neutral review. I also installed this head on the Android 17 / 16 KB emulator and completed short and long background/foreground smoke cycles without a crash or ANR. The preserved dev wallet’s configured local Electrum endpoint (10.0.2.2:60001) was unavailable, so that device pass did not exercise teardown from a fully running node.

[ovitrif]

I’m treating native destruction as part of the lifecycle gate here. After this timeout, stop() reports success and LightningRepo publishes Stopped, even though the old Rust node can still be draining for tens of seconds. Immediate restart paths can then build a replacement over the same LDK state, while wipeStorage() and graph-reset paths can mutate that storage before the old owner releases it. That recreates the overlapping native lifetime this PR is intended to remove. Please retain an explicit release gate that prevents rebuild and destructive storage work until destroy() completes, or cross a safe process-recovery boundary, and cover it with distinct dispatchers plus a controllably delayed destroy().

[jvsena42]

fixed in fc385e5

[jvsena42]

wipe-and-restore.webm

[jvsena42]

checking the best approach for the electrum restart taking too long https://github.com/synonymdev/bitkit-android/actions/runs/30292125774/job/90105118405?pr=1100

[jvsena42]

everything re-checked and description updated

[ovitrif]

I re-audited exact head 140ba5e after the rebase. Hosted build, lint, detekt, and E2E checks are green, and the earlier cancellation and self-join fixes remain resolved. I’m requesting changes for two native-lifecycle races: the Electrum/recovery path bypasses the pending-release gate, and detached recovery can race a later server change into reporting and persisting success for a configuration that never started.

[ovitrif]

I see this detached recovery—and the identical RGS branch—racing the next Connect attempt. Because recovery and the new request acquire the lifecycle mutex separately for stop() and start(), recovery can start the old config first; the new start then takes the Running fast path, reports success, and persists its URL without applying it.

Please serialize Electrum changes, RGS changes, and their recovery behind one dedicated configuration mutex, and assign each request a monotonically increasing generation before it waits for that mutex. Run the detached recovery under the same mutex and skip it when its captured generation is no longer current:

private val configChangeMutex = Mutex()
private val configGeneration = AtomicLong(0)

private fun recoverPreviousConfig(generation: Long) = scope.launch {
    configChangeMutex.withLock {
        if (generation != configGeneration.get()) return@withLock
        restartWithPreviousConfig()
    }
}

Keep the successful settingsStore.update inside that serialized transaction. Add a two-request barrier test that fails request A, pauses its recovery, starts request B, then releases A; assert A’s recovery is skipped, B’s server is the running configuration, and only B’s URL is persisted.