fix: wake node polish

[jvsena42]

Description

Main changes:

• Fix node lifecycle handling in WakeNodeWorker
• Improve logging
• fix: delete FCM token on wallet wipe

Preview

QA Notes

OBS: After channel creation it is necessary wait 3-5 minutes before testing WakeToPay for blocktank cache the channels
FCM Token

• Fresh install -> display log Got new FCM token via FirebaseMessagingService.onNewToken -> don't create wallet -> finish app -> reopen and create wallet -> Should display Device registered for notifications log
• Existing install -> reset wallet -> should delete FCM token -> create new wallet -> Should display Device registered for notifications log
• Moving same seed between two different devices from the same vendor
• Moving same seed between two different devices from different vendors

Push Notification errors

• Trigger lightning error with app running in foreground -> shouldn't stop node
• Trigger lightning error with app running in background -> shouldn't stop node

Wake to operations

• incomingHtlc
• mutualClose
• orderPaymentConfirmed
• cjitPaymentArrived

[ovitrif]

God… claude reviews were supposed to edit the same comment but it doesn't seem to work 🤦🏻

[claude]

PR Review: fix: wake node polish

Summary

This PR improves node lifecycle handling in WakeNodeWorker, enhances logging consistency, and fixes FCM token deletion on wallet wipe. Overall, the changes are well-structured and address important issues around push notifications and node lifecycle management.

---

✅ Strengths

1. Improved Node Lifecycle Management (WakeNodeWorker.kt:245-264)

   • Smart approach to only stop the node when app is in background
   • The 1-second delay allows the app to come to foreground if notification was tapped
   • Prevents unnecessary node stops when user is actively using the app

2. Consistent Logging

   • All Logger calls now properly include context = TAG parameter throughout FcmService.kt and WakeNodeWorker.kt
   • Follows the CLAUDE.md guideline: "ALWAYS pass the TAG as context to Logger calls"

3. Better Error Handling (WakeNodeWorker.kt:88-103)

   • Moved from try-catch to .onFailure pattern with Result<T>
   • Aligns with CLAUDE.md rule: "ALWAYS use the Result API instead of try-catch"
   • Properly delegates to blocktankRepo.openChannel() instead of directly calling CoreService

4. FCM Token Cleanup (WipeWalletUseCase.kt:44)

   • Correctly deletes FCM token on wallet wipe
   • Test coverage updated accordingly

5. Code Cleanup

   • Removed unnecessary CoreService injection from WakeNodeWorker
   • Removed unused self variable
   • Removed unnecessary lightningRepo.connectToTrustedPeers() call

---

⚠️ Potential Issues

1. Race Condition in deliver() method (WakeNodeWorker.kt:257)

Severity: Medium

if (App.currentActivity?.value == null) {
    Logger.debug("App in background, stopping node after notification delivery", context = TAG)
    lightningRepo.stop()
} else {
    Logger.debug("App in foreground, keeping node running", context = TAG)
}

Problem: App.currentActivity is a shared mutable state that could change between the check and the action. If the user backgrounds the app immediately after the check but before the stop, the node could remain running when it shouldn't.

Recommendation: Consider using a thread-safe snapshot:

val isAppInForeground = App.currentActivity?.value != null
delay(1.seconds)
if (!isAppInForeground) {
    lightningRepo.stop()
}

2. Missing Error Handling for firebaseMessaging.deleteToken() (WipeWalletUseCase.kt:44)

Severity: Low-Medium

The deleteToken() call is not wrapped in error handling. If Firebase token deletion fails, it could potentially throw and break the entire wipe process.

Recommendation:

runCatching { firebaseMessaging.deleteToken() }
    .onFailure { Logger.error("Failed to delete FCM token", it, context = TAG) }

Since wallet wipe should proceed even if FCM token deletion fails, this should be logged but not block the operation.

3. Validation Check Location (LightningRepo.kt:872)

Severity: Low

require(token.isNotEmpty()) { "FCM token is empty or null" }

This validation happens after potentially awaiting the Firebase token, but the error message says "or null" when null was already handled by the elvis operator on line 869.

Recommendation: The message should just say "FCM token is empty" for clarity.

4. setExpedited() without proper fallback handling (FcmService.kt:86)

Severity: Low

.setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST)

This is added to ensure the work runs expediently, which is good. However, the fallback policy means this could be delayed significantly on quota-exhausted devices, which could impact wake-to-pay reliability.

Consideration: Document this behavior and ensure monitoring/logging exists to track cases where work is delayed.

---

🎯 Best Practices & Architecture

Positive:

• ✅ Follows MVVM pattern with proper layer separation
• ✅ Repository pattern correctly used (blocktankRepo.openChannel())
• ✅ Coroutine-based async operations
• ✅ Proper use of Result<T> API
• ✅ Unit test coverage updated

Alignment with CLAUDE.md:

• ✅ Uses context = TAG in Logger calls
• ✅ Uses Result API instead of try-catch
• ✅ Prefers .onFailure { } over explicit try-catch
• ✅ Business logic in Repository layer
• ✅ Architecture flow: UI → ViewModel → Repository → RUST

---

🔒 Security Considerations

No security issues identified. The changes appropriately:

• Handle sensitive FCM token lifecycle
• Properly clean up on wallet wipe
• Don't expose sensitive information in logs

---

⚡ Performance Considerations

1. Positive: Removed unnecessary connectToTrustedPeers() call reduces network overhead
2. Positive: Smart node lifecycle management prevents unnecessary stop/start cycles
3. Note: The 1-second delay in deliver() is acceptable for UX but adds latency to notification delivery

---

🧪 Test Coverage

Good: Test coverage updated for WipeWalletUseCase to include firebaseMessaging mock.

Suggestion: Consider adding tests for:

1. WakeNodeWorker behavior when app is in foreground vs background
2. FCM token deletion failure scenarios in WipeWalletUseCase

---

📝 Minor Suggestions

1. LightningNodeService.kt:134 - Comment update from "Close all activities" to "Close activities gracefully" is accurate and helpful

2. LspNotificationsService.kt:55 - Changed from isProduction = null to isProduction = !Env.isDebug. Ensure this is the intended behavior for all build variants.

---

✅ Recommendation

LGTM with minor suggestions. The core functionality is solid and improves the codebase. Consider addressing the race condition in deliver() and adding error handling for FCM token deletion, but these are not blockers.

The QA testing appears comprehensive, covering the main scenarios for FCM tokens, push notification errors, and wake-to-pay operations.

---

Code Quality: ⭐⭐⭐⭐ (4/5)
Test Coverage: ⭐⭐⭐⭐ (4/5)
Documentation: ⭐⭐⭐ (3/5) - Could use inline comments for the deliver() logic

[jvsena42]

2. Missing Error Handling for firebaseMessaging.deleteToken() (WipeWalletUseCase.kt:44)

The method doesn't throw exceptions

[ovitrif]

utAck