Repository navigation
fix: exit instead of releasing a detached node - #1838
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
[Medium impact] The PR appears safe to merge within its stated background-stop scope. SummaryThe PR ends the background app process when stopping a Lightning node leaves tasks detached, instead of releasing that node.
The earlier, unnumbered finding about returning during the delay is addressed. jvsena42 explicitly leaves detached-node crashes during foreground release out of scope. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Node stop finishes] --> B{Detached tasks and app backgrounded?}
B -->|No| F[Release node]
B -->|Yes| C[Wait 500 ms for logs]
C --> D{App still backgrounded?}
D -->|No| F
D -->|Yes| E[Kill process without releasing node]
Reviews (2) · Last reviewed commit: "fix: detect background from started acti..." · Reviewed by Greptile |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Placed in the 3.2.0 milestone through #1844, which this PR now closes. It mitigates the background-restart crash in #1536, which is already planned for 3.2.0. #1536 stays open for the ldk-node fix. It has no iOS twin: synonymdev/bitkit-ios#1269 was closed because iOS does not release the node this way. |
talosmachina
left a comment
There was a problem hiding this comment.
1 finding (1 P1). Kills the process instead of releasing the node when a background stop leaves ldk-node tasks detached, so the runtime is never shut down under them (#1536). Reviewed b04e632, full tier, static.
What I checked, and 4 candidates I ruled out
Read in full: LightningService.kt (stop, setup, releaseHandle, awaitNodeRelease), LoggerLdk.kt, both test files, AndroidManifest.xml, App.kt, LightningNodeService.kt, LightningRepo.stop/resetNetworkGraph, WipeWalletUseCase
Cross-checked: ldk-node v0.7.0-rc.79 src/runtime.rs and src/logger.rs: the three Detaching ... messages are log_error! from ldk_node::runtime, and the custom writer gets every record before any level filter, so the monitor matches what the pinned ldk-node logs.
Build: not built. On the Bitkit track an auto review does not build an Android branch here; reasoned from the code, the pinned ldk-node source and CI. d0b1e99 passed CI and Lint; on b04e632, CI's build gate failed because its build jobs were cancelled while a newer run was in progress, so no result yet for this head (the delta is a moved comment).
Ruled out
- Monitor swap racing a stop:
stop()capturesruntimeMonitorbeforenode.stop(), andsetup()waits on the LDK queue, so a rebuild cannot swap the monitor mid-stop. - A stale flag on the next node: each
setup()makes a freshNodeRuntimeMonitor, and a killed process starts clean. - Kill racing the user coming back during the 500 ms flush (Greptile's thread): already raised; narrow next to the finding above, which decides the background check anyway.
LdkLogWriterused elsewhere without the hook: the defaultonRecord = {}keeps other call sites unchanged.- Cold reader: the finding stood; it rated it P2, I kept P1 because it closes the app in front of the user on a path the PR says stays unchanged.
Merge confidence: 3/5, one P1.
Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed (the run metadata available to this reviewer does not expose a setting)
coreyphillips
left a comment
There was a problem hiding this comment.
Two independent reviews.
needs changing before merge
- isProcessInBackground() is always true, so foreground stops kill the app (app/src/main/java/to/bitkit/services/LightningService.kt:1620-1621)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@greptileai review |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
coreyphillips
left a comment
There was a problem hiding this comment.
Rechecked after 2 new commits (63eb5fa, 7460ccf). Nothing blocking remains.
addressed
- isProcessInBackground() is always true, so foreground stops kill the app (app/src/main/java/to/bitkit/services/LightningService.kt:1620-1621). Fixed: LightningService.kt:1619 now reads
App.currentActivity?.hasStartedActivity != true, and both checks (lines 565 and 573) use it, with no ProcessLifecycleOwner left in LightningService. The counter at App.kt:103-122 goes up in onActivityStarted and down in onActivityStopped, and App.onCreate registers it at App.kt:62 before any activity starts. A paused but visible activity therefore counts as foreground, and an A-to-B transition starts B before stopping A, so there is no zero gap. I read the callbacks and the single-process manifest, confirmedCurrentActivityTest.ktcovers the counter and that CI build-and-test passes on 7460ccf, and noted your device case 3 (foreground restart, same PID). I did not rerun it on a device.
Closes #1844
This PR ends the app process, instead of releasing the node, when a node stop in the background leaves ldk-node tasks detached. This is step 1 of the plan in #1536 (#1536 (comment)). It mitigates the crash in the app. It does not fix ldk-node, so #1536 stays open.
Description
block_in_place. Releasing the node (destroy→Runtime::drop→shutdown_background) then shuts down the runtime underneath them. The first one that resumes polls a timer on the dead runtime and aborts withA Tokio 1.x context was found, but it is being shutdown.Resuming means the VSS write returning or the process thawing from the freezer.NodeRuntimeMonitor, one per node build. It flags the node when ldk-node logs anERRORfromldk_node::runtimestarting withDetaching(background tasks, cancellable background tasks or the event processor). ldk-node exposes this only through its log.LdkLogWritertakes anonRecordhook, called before the level filter.LightningService.stop(), afternode.stop(), a flagged node in a backgrounded app logs the reason, waits 500 ms for the log writer, checks again that the app is still in the background, then callsProcess.killProcess. If the user came back during the wait, the node is released as in the foreground.App.CurrentActivity. A paused activity behind a system dialog still counts as visible.ProcessLifecycleOwnercan't be used: the manifest removes androidx.startup'sInitializationProvider, so it never initializes and always reads as background. The runtime is never shut down, so the detached tasks can't abort the app, and the next launch is a cold start. The kill skipsexit(), so no native teardown runs against the live runtime.Out of Scope
LightningService: a detached task that resumes while the app is still in the foreground can still abort the app. This covers stops in the foreground (wipe, graph reset, restarts), whose released runtime stays exposed until the process dies.Nodeacross a pendingnextEventAsync(), which a stopped node never wakes, sonode = nildoesn't runRuntime::drop. Android cancels the listener before the stop and callsdestroy()explicitly.Design
N/A — no UI changes.
Preview
N/A
QA Notes
Setup
adb shell settings put global cached_apps_freezer disabled, then reboot), so the abort shows up while the app is backgrounded instead of on thaw.bitkit-docker(vss-serveron 5050,lnurl-auth-serveron 5005,electrumon 60001).127.0.0.1:5050, and while a flag file exists stops forwarding bytes without closing the connection. Requests through it hang, like the iptables DROP recipe in crash: native abort when the node restarts after returning from the background #1536, without root.E2E=true E2E_VSS_SERVER_URL=http://localhost:15050/vss E2E_LNURL_AUTH_SERVER_URL=http://localhost:5005/auth ./gradlew assembleDevDebug, thenadb reverse tcp:15050 tcp:15050,adb reverse tcp:5005 tcp:5005andadb reverse tcp:60001 tcp:60001.SyncCompleted.Journeys
N/A — not drivable; see Manual Tests.
Manual Tests
Detaching background tasks after cancellation timed out., thenExiting process: node stop left ldk_node tasks detached while in background; the process is gone and noFatal signal 6follows. Unstall, reopen: the app cold-starts to Home and the node runs. — needs a VSS stall and logcat, not in CapabilitiesNode stoppedis logged, the process stays alive. — needs logcat, not in CapabilitiesDetaching background tasks after cancellation timed out.andNode stopped, noExiting process, and the app stays open with the same PID. — needs a VSS stall and logcat, not in CapabilitiesAutomated Checks
NodeRuntimeMonitorTest.kt— flags each ldk-node detaching message, ignores timeouts without a detach and other modules or levels, andLdkLogWriterforwards records to itCurrentActivityTest.kt— counts started activities, keeps a paused activity visible, ignores an unmatched stopLightningServiceTest.kt— stop kills the process instead of releasing a flagged node in the background, releases it in the foreground, releases an unflagged node in the background, and releases a flagged node when the app returns during the delayEvidence
Run on
d0b1e99d1(fix) and master176430965, devDebug e2e builds with the setup above, emulatorGate_Throwaway(API 37.1,google_apis_playstore, freezer disabled), ldk-node0.7.0-rc.79, 10 Nov 2026. VSS was stalled for 240 s from just before Home, which outlasts the VSS client's 180 s retry budget, so the stuck write fails by itself after the stop.Exiting process…thenProcess to.bitkit.dev … has died0.6 s later; noFatal signal 6d0b1e99d1. The cycle after each kill cold-started the app and the node reachedSyncCompleted.d0b1e99d1.Node stopped21 ms afterStopping node…, process still alive.Stopping background tasks timed out.→Detaching background tasks after cancellation timed out.→Stopping event handling timed out.63eb5fabeafter the background check changed (Gate_Throwaway, same proxied local VSS setup, 60 s stall, ldk-node0.7.0-rc.79):Exiting process…, thenProcess to.bitkit.dev (pid 4774) has died0.7 s later.Detaching background tasks…, thenNode stopped, no exit, same PID (3026) afterwards.just compile,just testandjust lintpassed on63eb5fabe.7460ccfbechanges only the changelog sentence.d0b1e99d1, where the background check wasProcessLifecycleOwnerand always read as background. Their background results still hold. No foreground case ran on that head.just compile,just testandjust lintpassed ond0b1e99d1.Gaps
onActivityStartedarrives more than 500 ms after the first check, the app restarts instead of resuming. If it arrives in time, the node is released and the original abort can still happen. The device runs had the freezer off.Detaching …fromldk_node::runtime). A wording change in a future ldk-node silently disables the guard;NodeRuntimeMonitorTest.ktpins the current messages.Models used
claude-opus-5-5(reasoning: Not exposed)claude-opus-5-5(reasoning: Not exposed)claude-opus-5-5(reasoning: Not exposed)Metadata note: the session and the independent reviewer subagent both report the model ID
claude-opus-5-5; neither exposes a reasoning-effort setting.🤖 Generated with Claude Code