fix: harden ldk node teardown on stop

[jvsena42]

Relates to #982 , #986
Fixes #1078

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
5. Probes an Electrum server before the node is torn down for it, so a misconfigured server is rejected in seconds instead of failing a rebuild and leaving the release wedged
6. Serializes a whole server-change transaction against the background recovery a failed change launches, so a change can never report and persist a server that was not applied
7. 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).

Nothing opts out of that gate. An earlier revision of this PR let the Electrum server-change rebuild skip it (awaitRelease = false), because gating serialized each back-to-back change in @settings_10 behind the previous — possibly wedged, ~40 s — node's drain. That bypass is gone: the pre-flight probe below removes the wedge it was working around, so the parameter itself has been deleted from LightningService.setup, LightningRepo.setup, and LightningRepo.start.

Electrum pre-flight probe

The ~40 s wedge was never intrinsic. It is produced by a failed node.start(), which leaves the node's Electrum background tasks stuck mid-handshake so free_node cannot drain. Every wedge measured in this PR came from tearing the node down and rebuilding it against a server that could never work.

So the Electrum change now probes the candidate server over its own socket before the node is touched, exactly as restartWithRgsServer already validates its URL before stopping anything. ElectrumProbeService connects, performs the TLS handshake when SSL is selected, and speaks enough of the protocol to classify four failure modes:

Check	Rejects with
Socket connect	Unreachable — wrong host or port
TLS handshake	ProtocolMismatch — TLS against a plain-TCP server, the @settings_10 condition
server.version	NotElectrum — reachable, but not an Electrum server
server.features → genesis_hash	NetworkMismatch — right protocol, wrong chain (regtest vs mainnet)

The genesis-hash check closes #1078: a regtest wallet pointed at a mainnet server used to be accepted ("Successfully changed electrum server"), after which the on-chain sync timed out forever and the screen silently showed Disconnected with an endless retry loop. That server is now rejected before the switch, with the failure surfaced immediately instead of as a permanent Disconnected state. #1078 proposed exactly this mechanism — comparing the server's genesis hash from server.features against the wallet's network — and its reproduction case (electrum.blockstream.info on a regtest build) is one of the device journeys below.

Each reply is validated as a JSON-RPC envelope rather than merely parsed: the probe requires the id it asked for, no error, and a non-null result. Without that, a server erroring on version negotiation — one the real LDK client rejects at startup — would pass the version check, and a subsequent server.features error would then look like "no genesis hash" and probe clean, recreating the failed-start wedge. Version negotiation must succeed first; only then is server.features treated as optional.

server.features is optional in the protocol, so a server that omits genesis_hash is accepted rather than rejected — the network check degrades instead of false-rejecting working servers. Every step is bounded so the probe itself cannot become the thing that hangs.

Because the node is no longer torn down for a server that cannot work, this path stops manufacturing wedged releases — which is what makes the release gate affordable again on every rebuild, including recovery.

This is a mitigation, not a guarantee: a server can pass the probe and still fail at start (it goes down in between, or fails in a way the probe does not model). The gate therefore keeps its ~90 s bound and NodeReleaseTimeout for that residual case.

Server-change transaction serialization

A failed change still runs its recovery in the background, on the repository's process-lifetime scope, so the user sees the failure immediately rather than waiting on the restore. That detachment opened a race: stop() and start() take the lifecycle mutex separately, so a recovery could restart the previous config in the gap between a later change's stop and its start. The later change would then hit the already-running fast path in start(), report success, and persist a server that was never applied.

Two changes close it. A dedicated transaction lock is held across the whole stop → start → persist sequence, and is taken by the recovery too, so a change and a recovery can no longer interleave. And a start() carrying an explicit config is never satisfied by an already-running node — one that is already up demonstrably was not built with that config — so it fails with NodeConfigNotApplied instead of reporting a success the caller would persist. That second guard also covers racers the lock cannot see, such as a foreground start or WakeNodeWorker.

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 and the node is left running on the previous server.
• 8. Electrum → point TLS at a plain-TCP Electrum server → Connect: rejected in ~5 s with no node teardown, node stays on the previous server.
• 9. Electrum → point a regtest build at a mainnet Electrum server → Connect: rejected in ~1 s on the genesis-hash check, no node teardown.
• 10. Electrum → change to a valid server (local TCP and a remote TLS server): probe passes and the node stops, rebuilds and reaches started, with the setting persisted.
• 11. Electrum → a rejected change immediately followed by a valid one (the @settings_10 shape): the valid change applies without waiting on anything.

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().

• Probe coverage in ElectrumProbeServiceTest.kt, driven by a fake Electrum server on a local socket: a server on the expected network is accepted, one on a different network is rejected on the genesis hash, a server that omits server.features is still accepted, a host that never answers the handshake and an unreachable host are both rejected, TLS against a plain-TCP server is rejected, and the error names the server that was probed. Envelope validation is covered by four more cases — a version reply carrying an error, one with no result, one answering a different id, and a server that errors on both version and features — each asserting rejection, and each verified load-bearing by reverting the check.

• Server-change coverage in LightningRepoTest.kt: a probe rejection returns the probe error without stopping the node or calling setup, a start() carrying a custom server URL fails instead of riding an already-running node, and a change issued while a recovery is in flight waits for it and then genuinely rebuilds with the requested server.

• 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 pre-flight probe against the local regtest stack (electrs on plain TCP), with the release gate restored on every rebuild. Each rejection left the node completely untouched — no Stopping node…, no Building node…, and no Node handle release still pending anywhere in the session:

  Journey	Before this PR	With the probe
  TLS pointed at the plain-TCP electrs (the @settings_10 condition)	~22 s, node torn down, release wedged	5.0 s ProtocolMismatch, node untouched
  Regtest build pointed at a mainnet Electrum server	started, then failed slowly	1.3 s NetworkMismatch (expected 0f9188f1…, got 00000000…0a8ce26f)
  Unreachable host	~25 s	46 ms Unreachable
  Valid local TCP server	—	probe 8 ms, rebuild succeeds, gate costs 8 ms
  Valid remote TLS server	—	probe 1.5 s, accepted (confirms no false rejection of real TLS, and the genesis check passes on a legitimate server)
  Re-validated after adding envelope validation	—	mainnet server still rejected in 1.35 s; valid local TCP accepted in 14 ms and remote TLS in 1.5 s, so the stricter checks do not false-reject real servers
  Rejected change immediately followed by a valid one	second change serialized behind the ~40 s drain	second change completes in 8.3 s with zero gate wait

• Device validation that the restored gate costs nothing on the healthy path: no Waiting for previous node release… line appeared on any rebuild, because a healthy release finishes in ~38 ms.

• Device validation of the transaction lock on a real concurrent dispatcher: with a recovery in flight, a second Electrum change logged its entry and then blocked for ~9.5 s until the recovery finished, before running its own full stop → rebuild → start and only then persisting. Without the lock that window is where the recovery's start lands, leaving the change to report success without applying.

• 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.

Notes for review

• Validate Electrum server network before switching #1078's dedicated error string is not included. The wrong-network server is now rejected rather than silently accepted, but the failure toast still uses the generic settings__es__server_error_description; the specific reason (NetworkMismatch / ProtocolMismatch / NotElectrum / Unreachable) is distinguished in the probe error and the logs only. Wiring those to distinct user-facing strings is a small, self-contained follow-up — say the word and I'll add it here instead. The mismatch path is covered by unit tests in ElectrumProbeServiceTest.kt and LightningRepoTest.kt rather than in ElectrumConfigViewModelTest.kt as Validate Electrum server network before switching #1078 suggested, since the check lives at the service/repo layer.
• @settings_10 timing changes. Its Electrum error toasts now appear in ~5 s (wrong protocol) and ~50 ms (unreachable host) instead of ~22 s, since the probe rejects before any node work. Faster and more deterministic, but the E2E's timing assumptions are worth a look — CI is the real check.
• The two-request barrier test pins the outcome, not the interleaving. Under the single-threaded test dispatcher a parked recovery holds the lifecycle mutex for its whole rebuild, so a second change blocks there regardless; the window the transaction lock closes only opens on a genuinely concurrent dispatcher. That case is covered by the device validation above rather than by the unit test, and the test is commented to say so. The start() guard is the part pinned by a load-bearing unit test, and it is what prevents the user-visible symptom (persisting a server that was never applied).

[greptile-apps]

Greptile Summary

This PR hardens LDK node shutdown and restart behavior. The main changes are:

• Deterministic, bounded, and non-cancellable native node release.
• Release gates for rebuilds and destructive storage work.
• Listener self-join and stale-node polling protection.
• Serialized server changes with background recovery.
• Electrum server validation before node teardown.
• Debounced stops for brief app backgrounding.

Confidence Score: 5/5

This looks safe to merge.

• Deferred-stop publication and cancellation now use the same lock.
• Foreground startup cancels pending stops before guards and again when repository startup begins.
• No blocking issues remain in the updated code.

Important Files Changed

Filename	Overview
app/src/main/java/to/bitkit/repositories/LightningRepo.kt	Adds synchronized deferred stops, release-aware lifecycle work, serialized server changes, and Electrum validation.
app/src/main/java/to/bitkit/services/LightningService.kt	Adds bounded native release, release gates, non-cancellable teardown, and safer event-listener lifecycle handling.
app/src/main/java/to/bitkit/services/ElectrumProbeService.kt	Adds bounded TCP, TLS, protocol, and network validation for Electrum servers.
app/src/main/java/to/bitkit/viewmodels/WalletViewModel.kt	Cancels deferred stops before startup guards and defers ordinary background teardown.

_{Reviews (6): Last reviewed commit: "fix: probe electrum server before node r..." | 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.

[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 verified the contiguous 685d478..4a6968b fix delta. The Electrum probe now rejects mismatched id values, non-null error responses, and missing result values before node teardown, with regression coverage for each path. My previous blocker is resolved, and all 18 hosted checks are green.