feat: make foreground service opt-in#1053
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9a98728be
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ovitrif
left a comment
There was a problem hiding this comment.
Reviewed locally. I left two comments: one lifecycle issue around the background transition, and one changelog cleanup. Leaving this as a neutral review.
Pull request was converted to draft
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
| Lifecycle.Event.ON_STOP -> { | ||
| if (walletExists && !isRecoveryMode && !notificationsGranted) { | ||
| // App backgrounded without notification permission - stop node | ||
| val keptAliveByService = notificationsGranted && keepActiveInBackground | ||
| if (walletExists && !isRecoveryMode && !keptAliveByService) { | ||
| walletViewModel.stop() | ||
| } |
There was a problem hiding this comment.
Node leaked after Android 15 foreground-service timeout
When Android 15 fires LightningNodeService.onTimeout() while an activity is active, stopNodeIfBackgrounded() correctly skips stopping the node because the activity is present. The service is then destroyed and nodeServiceFgState.setForegroundServiceRunning(false) is recorded in onDestroy. However, when the user later backgrounds the app, ON_STOP computes keptAliveByService = notificationsGranted && keepActiveInBackground — both of which are still true (they are settings values, not runtime service-alive flags) — so walletViewModel.stop() is never called. The Lightning node then runs indefinitely in the background with no service or activity lifecycle managing it.
The fix is to also require the service to be actually running when deciding whether to skip the node stop. Replacing keepActiveInBackground with a check against NodeServiceFgState.isForegroundServiceRunning (already updated to false in onDestroy) would close this gap — or alternatively setting keepBitkitActiveInBackground = false in onDestroy so that both the settings layer and the service-alive state remain in sync.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e571137fb2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Pull request was converted to draft
Fixes #1007
This PR:
Description
Preview
disable-fg-switch.webm
disable-notifications.webm
running-from-master.webm
wake-to-pay.webm
enable-fg-service.webm
QA Notes
FIGMA
Manual Tests
regression:Keep active off, app closed → receive a payment: push notification arrives and shows the amount.Automated Checks
SettingsViewModelTest.kt.ReceivedNotificationContentTest.ktandNotifyChannelReadyHandlerTest.ktbecause the show-notification-details option no longer exists;SettingsDatastubs updated inNotifyPaymentReceivedHandlerTest.kt.just compile, the affected unit tests, andjust lint.