fix: start background service from FCM handler when service is dead#489
fix: start background service from FCM handler when service is dead#489AndreaDiazCorreia wants to merge 8 commits intomainfrom
Conversation
- Add fcm-wake handler to background service to acknowledge wake signals - When FCM push arrives and service is running, send fcm-wake signal - When service is dead, start it and send app settings from SharedPreferences - Add 3-second timeout with 100ms polling to wait for service initialization - Set fcm.pending_fetch flag before starting service
…andler - Wrap jsonDecode in try-catch to prevent crashes on malformed settings - Only invoke service.start if both service is running AND settings decoded successfully - Add debug print for decode failures
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughHandlers are registered early (including Changes
Sequence Diagram(s)sequenceDiagram
participant FCM as Firebase Cloud Messaging
participant BGHandler as firebaseMessagingBackgroundHandler
participant Prefs as SharedPreferences
participant BgService as Background Service
participant DB as EventStorage/DB
participant Notif as NotificationService
FCM->>BGHandler: Deliver background message
BGHandler->>BgService: Is service running?
alt service running
BGHandler->>BgService: Invoke 'fcm-wake'
BgService->>Notif: Use activeSubscriptions (re-attach listeners)
else service not running
BGHandler->>Prefs: Load `appSettings` (and background filters)
Prefs-->>BGHandler: Return JSON or null
BGHandler->>BGHandler: Decode settings JSON
BGHandler->>BgService: startService() (register handlers first)
BGHandler->>BgService: Wait for handlers-registered -> invoke 'start' with settings
BgService->>DB: Open DB / create EventStorage (deferred until after start)
BgService->>Notif: Restore subscriptions from persisted filters
BgService->>Notif: Attach listeners that consult eventStore (if non-null)
BGHandler->>BgService: Await service-ready (retry once on timeout)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/services/fcm_service.dart (1)
56-56:startService()return value is silently discarded.
startService()returnsFuture<bool>. If it returnsfalse(e.g., Android denied service start due to background restrictions or battery optimization), the code falls through to the polling loop, burns the full 3-second budget, and exits with the service not running andfcm.pending_fetchalready set — which is acceptable but could be made explicit.✨ Optional early-exit improvement
- await service.startService(); + final started = await service.startService(); + if (!started) return; // fcm.pending_fetch is already set as fallback🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/fcm_service.dart` at line 56, The call to service.startService() (in fcm_service.dart) ignores its Future<bool> result so failures (false) fall through to the polling loop; change the code to await the boolean, and if it is false perform an explicit early exit: log or record the failure (update fcm.pending_fetch or processLogger as appropriate) and return immediately instead of entering the 3-second polling loop; ensure the check references service.startService() and the existing fcm.pending_fetch handling so the behavior stays consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/services/fcm_service.dart`:
- Line 66: Replace the debugPrint call with the project logger singleton: import
the logger from 'package:mostro_mobile/services/logger_service.dart' and call
logger.error(...) (or logger.w/ logger.d as appropriate) instead of debugPrint;
because this runs in an isolate where the logger transport may not be
registered, add a defensive fallback that uses debugPrint only if the logger
singleton is not available/initialized; also update the other two occurrences in
this file (the debugPrints at the existing lines ~85 and ~90) to follow the same
pattern so all FCM handler logging uses the logger singleton with a debugPrint
fallback.
- Around line 72-80: Move the service event handler registrations so they run
before the database open awaits to avoid lost events: inside serviceMain,
register service.on('start') and service.on('fcm-wake') listeners before calling
await openMostroDatabase(...); the start handler can still call
nostrService.init(settings) (it doesn't depend on db/eventStore), so reorder the
code so the on('start') and on('fcm-wake') listen() calls occur prior to await
openMostroDatabase(...) to eliminate the race where invoke('start', ...) is
delivered before the listener is attached.
---
Nitpick comments:
In `@lib/services/fcm_service.dart`:
- Line 56: The call to service.startService() (in fcm_service.dart) ignores its
Future<bool> result so failures (false) fall through to the polling loop; change
the code to await the boolean, and if it is false perform an explicit early
exit: log or record the failure (update fcm.pending_fetch or processLogger as
appropriate) and return immediately instead of entering the 3-second polling
loop; ensure the check references service.startService() and the existing
fcm.pending_fetch handling so the behavior stays consistent.
…revent race condition - Move service.on() registrations above openMostroDatabase() call - Prevents losing 'start' events invoked by FCM handler during db initialization - Add comment explaining the ordering requirement - Keep db and eventStore initialization after handlers are registered
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good fix for a real problem — FCM pushes arriving with a dead background service were silently lost. The approach (start service + send settings) is correct. The race condition fix in commit 80ae0cc (registering listeners before await openMostroDatabase) was the right call. Two issues remain:
Must Fix
1. debugPrint instead of logger singleton
Per AGENTS.md: "Always use the pre-configured singleton logger instance. Direct instantiation of Logger() is no longer permitted."
Line 66 uses debugPrint('FCM: Failed to decode settings: $e'). The two pre-existing debugPrint calls (lines 85, 90) were already there, but adding a new one violates the rule. I know the FCM background handler runs in a separate isolate where the logger singleton might not be available — if that's the case, document it with a comment explaining why debugPrint is used here as an exception. Don't just silently break the convention.
2. fcm.pending_fetch flag is set but never useful in the new flow
Line 56: await sharedPrefs.setBool('fcm.pending_fetch', true) is set before startService(). But the new code immediately starts the service and sends settings — so pending_fetch is never consumed in this path. Looking at the codebase, _checkPendingFetch in FCMService only clears the flag without triggering any real action. This is dead logic:
- If
startService()+invoke('start')succeeds → the service processes events directly,pending_fetchis never read - If
startService()fails → the catch block setspending_fetchagain (line 87), which is also never consumed meaningfully
Either remove the flag from this flow (it's misleading), or make _checkPendingFetch actually do something useful as a fallback.
Minor
3. 3-second busy-wait poll in a background handler
The while (!(await service.isRunning())) loop with 100ms delays is a busy-wait in an FCM background handler. Android gives FCM handlers ~20 seconds, so 3 seconds isn't a timeout risk. But it's wasteful — consider using service.on('service-ready') as a signal instead of polling, or at least use exponential backoff (100ms → 200ms → 400ms) to reduce unnecessary isRunning() IPC calls.
4. No settings = silent failure
If settingsJson is null (fresh install, cleared data), the service starts but invoke('start') is never called. The service runs as a zombie — no relay connections, no notifications. At minimum log a warning so this is diagnosable.
What looks good
- ✅ CI passes, no conflicts, mergeable
- ✅ The listener reordering fix (
80ae0cc) correctly eliminates the main race condition - ✅
fcm-wakefor already-running services is lightweight and correct - ✅ Settings are read from SharedPreferences (correct approach for background isolate)
- ✅ try/catch around JSON decode prevents crash on corrupted settings
|
@AndreaDiazCorreia por favor échale un ojo al comentario de Mostronator |
…ending fetch flag
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/services/fcm_service.dart (1)
68-71: Service starts but runs without relay configuration when settings are missing.When
startService()succeeds but settings are absent (line 68-70), the service starts but never receives relay config. It will run as a "zombie" — consuming resources but unable to connect to relays.Consider whether to stop the service in this case or document this as intentional fallback behavior. If the settings are persisted later and another FCM arrives, the
isRunningbranch would only sendfcm-wake, never re-sending settings.💡 Option: stop service if settings unavailable
if (settingsJson == null) { debugPrint('FCM: No settings found, service started without relay config'); + // Consider: await service.invoke('stop') or document as intentional return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/fcm_service.dart` around lines 68 - 71, The service currently proceeds to run when settingsJson == null, causing a "zombie" that never gets relay config; update the startService() flow so that if settingsJson is null you stop the service instead of leaving it running (invoke the existing stopService() path and log a clear message), and ensure the isRunning branch that only sends 'fcm-wake' does not execute when there are no saved settings; alternatively, if you choose fallback behavior, add a clear comment/README and a telemetry/logging entry that indicates the intentional no-settings state so it isn’t mistaken for a healthy running service.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/services/fcm_service.dart`:
- Around line 68-71: The service currently proceeds to run when settingsJson ==
null, causing a "zombie" that never gets relay config; update the startService()
flow so that if settingsJson is null you stop the service instead of leaving it
running (invoke the existing stopService() path and log a clear message), and
ensure the isRunning branch that only sends 'fcm-wake' does not execute when
there are no saved settings; alternatively, if you choose fallback behavior, add
a clear comment/README and a telemetry/logging entry that indicates the
intentional no-settings state so it isn’t mistaken for a healthy running
service.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4b9f596-1f79-4075-9dff-f56801d9ecb5
📒 Files selected for processing (1)
lib/services/fcm_service.dart
|
@grunch lo que comento mostronator ya lo revise, si le pueden hacer review genial |
There was a problem hiding this comment.
Thanks for tackling this background wake-up issue. I reviewed the code path carefully and I’m requesting changes because two functional risks remain.
Blocking issues
1) invoke('start') race is still possible
isRunning() only confirms the native service process is alive. It does not guarantee the Dart isolate has already registered service.on('start', ...).
That means service.invoke('start', {...}) can still be dropped if it is sent before handler registration, causing intermittent wake failures.
Suggested fix
- Add a real readiness handshake (
service-ready/on-start-registered) before invoking start payloads, or - Add idempotent retry with ack semantics for
invoke('start').
2) Service can wake up without restoring subscriptions
When the service is revived from FCM, the current flow sends settings, but does not guarantee subscription bootstrap restoration.
If activeSubscriptions is empty until runtime events re-create them, background processing can wake and still do no useful work.
Suggested fix
- Persist/restore background subscriptions at startup, or
- Implement explicit subscription bootstrap during background start.
Minor
3) Logging consistency in background isolate
debugPrint usage in background code diverges from project logging conventions. If isolate constraints require a fallback, please document and centralize that behavior in a wrapper.
Once the race and subscription bootstrap paths are deterministic, this should be much safer to merge.
…vival and improve FCM startup reliability
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/services/lifecycle_manager.dart (1)
105-131:⚠️ Potential issue | 🟠 MajorClear persisted
backgroundFilterswhen there are no active subscriptions.Line 130 currently logs only. If old filters remain in storage,
background.dartwill restore stale subscriptions on next revival.🧹 Suggested fix
} else { logger.w("No active subscriptions to transfer to background service"); + try { + final prefs = SharedPreferencesAsync(); + await prefs.remove(SharedPreferencesKeys.backgroundFilters.value); + } catch (e) { + logger.e('Failed to clear persisted background filters: $e'); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/lifecycle_manager.dart` around lines 105 - 131, When there are no active filters the code only logs a warning but leaves any previously persisted backgroundFilters in storage, causing stale subscriptions to be restored; update the else branch in lifecycle_manager (the block handling activeFilters.isNotEmpty vs else) to clear the SharedPreferencesKeys.backgroundFilters entry by using SharedPreferencesAsync to remove or overwrite the key (e.g., remove or set to an empty JSON array), await the operation, and wrap it in try/catch with a logger.e on failure; reference SharedPreferencesAsync, SharedPreferencesKeys.backgroundFilters, and the else branch where logger.w(...) currently runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/background/background.dart`:
- Around line 49-54: The current idempotency check using the boolean initialized
is race-prone because two overlapping start invocations can pass the if
(initialized) check before initialized is set; fix by introducing an in-flight
guard (e.g., a separate bool initializing or a Future<void> _initializing)
alongside initialized in the start flow so only the first caller proceeds to
initialize and subsequent callers await the in-flight initialization or return;
update the start handler (the block that currently checks if (initialized) {
service.invoke('service-ready', {}); return; } and the code that sets
initialized = true) to set the in-flight flag immediately when starting and
clear/complete it once initialization finishes while only setting initialized =
true after successful init, and ensure callers either await the _initializing
future or short-circuit to invoke('service-ready') once initialization
completes.
- Around line 87-93: The restored-subscription listener currently calls
notification_service.retryNotification(event) directly (inside the
subscription.listen callback), which bypasses the existing dedupe check used
elsewhere (eventStore.hasItem(event.id!)); modify the listener to await the same
event-store readiness mechanism (e.g., an eventStoreReady Completer or
equivalent) and perform eventStore.hasItem(event.id!) before calling
notification_service.retryNotification(event), skipping the retry if the event
is already present to preserve the dedupe semantics used by the
create-subscription path.
In `@lib/services/fcm_service.dart`:
- Around line 63-131: The startup path can leave the service running if settings
are missing/invalid; update the logic around startService(), the settings decode
block, and the handlersReady/serviceReady waits so that on any abort you call
service.stopService() (or service.stop()/shutdown method used in this class)
before returning, and move cancellation of handlersSub and readySub into a
finally block to guarantee listener cleanup; ensure you also stop the service if
the second start retry fails after invoking service.invoke('start', {'settings':
settings}) so the service is not left running without initialization.
In `@lib/services/logger_service.dart`:
- Around line 297-299: The backgroundLog function prints raw messages to logcat
and bypasses redaction; update backgroundLog (the function named backgroundLog)
to run the incoming message through the existing cleanMessage(...) redaction
helper before calling debugPrint so sensitive data (keys/settings) are
sanitized; ensure you call cleanMessage(message) and log the cleaned result
(preserving the "[BackgroundIsolate]" prefix) so all background logs follow the
same redaction rules as other logging paths.
---
Outside diff comments:
In `@lib/services/lifecycle_manager.dart`:
- Around line 105-131: When there are no active filters the code only logs a
warning but leaves any previously persisted backgroundFilters in storage,
causing stale subscriptions to be restored; update the else branch in
lifecycle_manager (the block handling activeFilters.isNotEmpty vs else) to clear
the SharedPreferencesKeys.backgroundFilters entry by using
SharedPreferencesAsync to remove or overwrite the key (e.g., remove or set to an
empty JSON array), await the operation, and wrap it in try/catch with a logger.e
on failure; reference SharedPreferencesAsync,
SharedPreferencesKeys.backgroundFilters, and the else branch where logger.w(...)
currently runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eeb754d8-2e43-4347-9f4a-01f9121df82c
📒 Files selected for processing (5)
lib/background/background.dartlib/data/models/enums/storage_keys.dartlib/services/fcm_service.dartlib/services/lifecycle_manager.dartlib/services/logger_service.dart
… startup failures (coderabbit changes)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/background/background.dart (1)
101-105:⚠️ Potential issue | 🟠 MajorDedupe is still bypassed while
eventStoreis null.Both listeners notify when DB init is still in progress (
store == null), so early events can be re-notified withouthasItemchecks.🛠️ Suggested fix (gate listeners on DB readiness)
- EventStorage? eventStore; + EventStorage? eventStore; + final eventStoreReady = Completer<void>(); @@ - final db = await openMostroDatabase('events.db'); - eventStore = EventStorage(db: db); + final db = await openMostroDatabase('events.db'); + eventStore = EventStorage(db: db); + if (!eventStoreReady.isCompleted) eventStoreReady.complete(); @@ subscription.listen((event) async { try { + if (!eventStoreReady.isCompleted) { + await eventStoreReady.future; + } final store = eventStore; - if (store != null && await store.hasItem(event.id!)) return; + if (store != null && await store.hasItem(event.id!)) return; await notification_service.retryNotification(event); } catch (e) { logger?.e('Error processing restored subscription event', error: e); } }); @@ subscription.listen((event) async { try { + if (!eventStoreReady.isCompleted) { + await eventStoreReady.future; + } final store = eventStore; if (store != null && await store.hasItem(event.id!)) { return; } await notification_service.retryNotification(event);Based on learnings: "In the Mostro Mobile background service architecture, events aren't stored by the background process. Instead, the background service only checks if events exist in the eventStore and sends notifications for new ones, while the foreground process is responsible for storing and processing events."
Also applies to: 162-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/background/background.dart` around lines 101 - 105, The listeners currently call notification_service.retryNotification even when eventStore is null causing duplicate notifications; update the subscription.listen handlers (the block using eventStore, store.hasItem(event.id!), and notification_service.retryNotification) to first gate on a ready eventStore — either await a ready/initialized future or loop/wait until eventStore != null (or return early and do not call retryNotification) so hasItem is always checked before notifying; apply the same guard to the other listener that uses eventStore (the similar block around hasItem and retryNotification referenced in the review).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/background/background.dart`:
- Around line 51-69: The init path currently claims initInFlight =
Completer<void>() but never completes it if an awaited call throws (e.g.,
nostrService.init(settings)), causing callers awaiting initInFlight!.future to
hang; modify the startup logic in the start/init function so that any thrown
error completes initInFlight (completeError with the thrown error) and/or always
completes it in a finally block, then rethrow the error (or propagate it) so
service.invoke('service-ready') callers don't block; apply the same
completion-on-error/finally pattern to the other init region around the code
referenced at lines ~118-120 that uses initInFlight.
---
Duplicate comments:
In `@lib/background/background.dart`:
- Around line 101-105: The listeners currently call
notification_service.retryNotification even when eventStore is null causing
duplicate notifications; update the subscription.listen handlers (the block
using eventStore, store.hasItem(event.id!), and
notification_service.retryNotification) to first gate on a ready eventStore —
either await a ready/initialized future or loop/wait until eventStore != null
(or return early and do not call retryNotification) so hasItem is always checked
before notifying; apply the same guard to the other listener that uses
eventStore (the similar block around hasItem and retryNotification referenced in
the review).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4a849f3-601d-4c3c-a791-f6c7b7135a45
📒 Files selected for processing (3)
lib/background/background.dartlib/services/fcm_service.dartlib/services/logger_service.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/services/fcm_service.dart
…res to concurrent callers
There was a problem hiding this comment.
Re-review after latest commits (7999c16)
Issues Resolved
-
Race condition fix — handlers-registered handshake emitted before DB open, FCM handler waits for it before sending start. Good.
-
Idempotent initialization — initialized flag + initInFlight Completer prevents concurrent inits. Proper error propagation with completeError(e). Good.
-
Logging — backgroundLog() wrapper added for FCM isolate logging. Documents why debugPrint is used. Good.
-
Service start failure handling — Returns early if startService() returns false, stops service if settings missing. Good.
-
Subscription restoration — Persists filters to backgroundFilters on background transition, restores them on service revival. Good.
Remaining Issue
Stale filters not cleared when no active subscriptions
CodeRabbit flagged this and it is still not addressed. When activeFilters is empty (lifecycle_manager.dart line 130), the code only logs a warning but leaves any previously persisted backgroundFilters in storage. This causes service revival to restore stale/outdated subscriptions.
Fix needed in else branch:
- Clear SharedPreferencesKeys.backgroundFilters when no active subscriptions exist
- Otherwise user may receive notifications for trades that are no longer active
Minor: eventStore race window
During the brief window between handler registration and DB open, eventStore is null. Events arriving during this window skip the hasItem dedup check. Probably fine in practice but could cause duplicate notifications in edge cases.
Summary
| Item | Status |
|---|---|
| Race condition fix | Done |
| Idempotent init | Done |
| Logging convention | Done |
| Service start failures | Done |
| Filter persistence | Done |
| Stale filter cleanup | Missing |
| CI passes | Done |
Verdict: Request Changes — Clear stale filters when no active subscriptions exist.
There was a problem hiding this comment.
Issues from previous reviews addressed ✅
All critical issues have been resolved:
-
Race condition in invoke(start) ✅
- Fixed with
handlers-registeredhandshake before sending start event - Completer ensures we never miss events during startup
- Fixed with
-
Subscription restoration ✅
_restoreBackgroundFilters()properly restores persisted filters- Filters are loaded before NostrService.init() subscribes to them
-
Multiple on() registrations ✅
initializedflag +Completerprevents concurrent initializations_initializationLockensures idempotent startup
-
Logger in background isolate ✅
backgroundLog()wrapper prevents crashes when main logger unavailable
Minor issue still present (non-blocking)
Stale filters not cleared when no active subscriptions — When activeFilters is empty, the code logs a warning but leaves persisted backgroundFilters in storage. This could cause the service to restore stale subscriptions on next start.
Recommendation: When activeFilters.isEmpty, clear SharedPreferencesKeys.backgroundFilters from storage.
Summary
The critical fix (FCM handler properly starting the background service with settings) works correctly. The handshake mechanism ensures reliable startup even when the service was killed by Android.
LGTM — the stale filters issue is minor and can be addressed in a follow-up PR.
2 participants
Summary
can connect to relays and process events
Test plan
Summary by CodeRabbit
Bug Fixes
New Features