Skip to content

fix: stop push notifications after session expires without explicit logout #248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
deaflynx wants to merge 4 commits into
base: develop/1.9.0
Choose a base branch
from
Open

fix: stop push notifications after session expires without explicit logout #248

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3e81c97
fix: stop push notifications after session expires without explicit l…
deaflynx Aug 13, 2026
04f3f07
fix: address review findings on the push notification cleanup (#304)
deaflynx Aug 25, 2026
cef25b4
fix: harden the push registration cleanup after re-review (#304)
deaflynx Sep 4, 2026
d7496b3
fix: close the review gaps in the stale push cleanup (#304)
deaflynx Sep 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/constants/database_keys.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@ abstract final class DatabaseKeys {
static const thingsBoardApiEndpointKey = 'thingsBoardApiEndpoint';
static const initialAppLink = 'initialAppLink';
static const selectedRegion = 'selectedRegion';
static const pushNotificationsRegistered = 'pushNotificationsRegistered';
}
14 changes: 8 additions & 6 deletions lib/core/auth/login/provider/login_provider.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import 'package:thingsboard_app/locator.dart';
import 'package:thingsboard_app/utils/services/communication/events/user_loaded_event.dart';
import 'package:thingsboard_app/utils/services/communication/i_communication_service.dart';
import 'package:thingsboard_app/utils/services/device_info/i_device_info_service.dart';
import 'package:thingsboard_app/utils/services/firebase/i_firebase_service.dart';
import 'package:thingsboard_app/utils/services/notification_service.dart';
import 'package:thingsboard_app/utils/services/overlay_service/i_overlay_service.dart';
import 'package:thingsboard_app/utils/services/tb_client_service/i_tb_client_service.dart';
Expand All @@ -40,8 +39,7 @@ class Login extends _$Login {
}

Future<void> logout() async {
if (getIt<IFirebaseService>().apps.isNotEmpty &&
state.isFullyAuthenticated()) {
if (state.isFullyAuthenticated()) {
await getIt<NotificationService>().logout();
}
await _tbClient.logout(requestConfig: RequestConfig(ignoreErrors: true));
Expand All @@ -52,6 +50,12 @@ class Login extends _$Login {

if (!_tbClient.isAuthenticated()) {
state = const LoginState(isUserLoaded: false);
// Fire-and-forget: the cleanup swallows its own errors, and the
// registration flag is deleted last, so an interrupted attempt is
// retried on the next launch without delaying the login screen.
// NotificationService.init() waits for it, so a fast auto-login
// (QR code, OAuth2) cannot register a token while it is being deleted.
unawaited(getIt<NotificationService>().cleanUpStalePushRegistration());

@vvlladd28 vvlladd28 Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this fire-and-forget fixes the latency, but it also means the cleanup can now overlap init() on the same NotificationService singleton, which the awaited version couldn't.

The interleaving: this starts _cleanupPushRegistration(), which blocks on _messaging.deleteToken() (the one genuinely slow step). If the client authenticates while that's still pending, it fires UserLoadedEvent → the listener at :34 → handleUserLoaded() → _onFullyLoggedIn() → NotificationService.init(), which acquires a token, saveMobileSessions it, sets the flag and calls setAutoInitEnabled(true). The cleanup then resumes against that new state: _fcmToken = null, setAutoInitEnabled(false), clearPushRegistered() — and if getToken() resolved before the pending deleteToken() landed, the token that was just registered is the one that gets deleted. I checked the server side of that outcome: MobileAppNotificationChannel.java:122-128 only purges the orphaned session when a push actually bounces, and nothing re-registers from the platform side, so the recovery is the next init() — i.e. no push for the rest of that session.

On how reachable it is, I'd calibrate this lower than it first looks. The window is however long deleteToken() takes, and init() reaches getToken() only after getInitialMessage() and requestPermission(). A human typing credentials (≥1-2s) loses that race comfortably, so the ordinary login path is fine. The cases where both sides are network-paced rather than human-paced are the ones to think about: a QR/deep-link launch that lands back on the default endpoint (noauth_provider's reset() → _initDefaultFbApp() re-populates apps, then the client logs in without user input) and returning from OAuth2. On a custom endpoint there's no race at all — both sides early-return on _isFirebaseConfigured.

So: an edge case rather than a blocker, but the fix is about three lines — hold the future in the service and await it at the top of init(). That also covers a smaller version of the same thing: every UserLoadedEvent arriving unauthenticated fires another unawaited(...), and since the flag is only cleared at the end, concurrent calls all pass the isPushRegistered() check and run the teardown in parallel.

Separately, and reasonable to defer: this file is where the fix actually manifests, and none of it is covered — every test targets NotificationService in isolation. A provider-level test (unauthenticated mock client → cleanup ran; authenticated → init() ran instead) is what would catch a future refactor dropping this call, though it does need a Riverpod container plus IDeviceInfoService/IOverlayService/ICommunicationService added to test/helpers, so it's a bigger lift than the service-level tests.

@deaflynx deaflynx Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cef25b4, in the service rather than the provider: cleanUpStalePushRegistration() stores its future in _staleCleanup (shared by concurrent callers, reset in whenComplete), and init() awaits that future right after the Firebase guard, before touching the token. That also covers the smaller version — repeated unauthenticated UserLoadedEvents now share one teardown instead of running several in parallel.

Tests: waits for a pending stale cleanup, so the token it registers is not the one being deleted holds deleteToken() on a Completer, asserts getToken() has not been called while it is pending, then checks the clearPushRegistered → getToken → setPushRegistered order; runs a single teardown for concurrent calls covers the dedup. The provider-level test is deferred as you suggested.

@vvlladd28 vvlladd28 Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not re-opening this — your costing was right, and a Riverpod container plus three more service mocks is more than this PR should carry.

Noting it for whoever picks up the follow-up, because the valuable assertion is narrower than a full provider test: it isn't the whole login flow, it's the branch choice. Unauthenticated -> cleanup started; authenticated -> init() and no teardown. The second half is the one that would catch a future refactor moving this call above the isAuthenticated() check, where it would tear down the registration a normal login is about to create — and every existing test would still pass.

One assumption worth writing down while it's fresh: the ordering claim in the comment you added holds only because NotificationService is a lazy singleton in the locator. Registered as a factory, init() would await a different instance's _staleCleanup and the guarantee would quietly disappear.

@deaflynx deaflynx Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on the shape of the follow-up test — the branch choice, not the flow. Recorded the singleton assumption in d7496b3 as a comment next to the await _staleCleanup in init(), so a future move of NotificationService to a factory registration has a comment to trip over.

return;
}
if (_tbClient.isPreVerificationToken() ||
Expand Down Expand Up @@ -113,9 +117,7 @@ class Login extends _$Login {

Future<void> _onFullyLoggedIn() async {
await loadUser();
if (getIt<IFirebaseService>().apps.isNotEmpty) {
await getIt<NotificationService>().init();
}
await getIt<NotificationService>().init();
}

Future<void> twoFaConfirmed(LoginResponse response) async {
Expand Down
5 changes: 1 addition & 4 deletions lib/core/context/tb_context.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import 'package:thingsboard_app/thingsboard_client.dart';
import 'package:thingsboard_app/utils/services/version_service/version_info.dart';
import 'package:thingsboard_app/utils/services/device_info/i_device_info_service.dart';
import 'package:thingsboard_app/utils/services/endpoint/i_endpoint_service.dart';
import 'package:thingsboard_app/utils/services/firebase/i_firebase_service.dart';
import 'package:thingsboard_app/utils/services/notification_service.dart';
import 'package:thingsboard_app/utils/services/overlay_service/i_overlay_service.dart';
import 'package:thingsboard_app/utils/utils.dart';
Expand Down Expand Up @@ -303,9 +302,7 @@ class TbContext implements PopEntry {
log.debug('TbContext::logout($requestConfig, $notifyUser)');
_handleRootState = true;

if (getIt<IFirebaseService>().apps.isNotEmpty) {
await getIt<NotificationService>().init();
}
await getIt<NotificationService>().logout();

@vvlladd28 vvlladd28 Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing the init()/logout() mix-up is right, but it leaves two copies of the same required sequence — notification teardown, then tbClient.logout() — with different guards: Login.logout() gates on state.isFullyAuthenticated(), this one doesn't gate at all. Two copies of a call order is what produced the original bug here.

That said, this method still has no callers anywhere in lib/ — everything goes through ref.read(loginProvider.notifier).logout() — so rather than factoring the sequence out into something shared, deleting the method is probably the cleaner outcome. Introducing an abstraction for a path nothing takes would be the more expensive fix for the same problem.

@deaflynx deaflynx Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this one as is. TbContext.logout() does have callers — onFatalError() (tb_context.dart:136) and onUserLoaded() (tb_context.dart:201) in the same file. Both belong to the legacy TbContext.init() flow, which is only started by ThingsboardInitApp/ThingsboardInitRegionApp, and nothing references those widgets anymore. So it is the whole legacy init path that is dead, not just this method; deleting it properly (widgets, init(), onUserLoaded(), onFatalError(), logout()) is a separate cleanup PR rather than something to fold into this fix.


await tbClient.logout(requestConfig: requestConfig, notifyUser: notifyUser);

Expand Down
4 changes: 2 additions & 2 deletions lib/main.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ Future<void> main() async {
WidgetsFlutterBinding.ensureInitialized();
FlutterNativeSplash.preserve(widgetsBinding: widgetsBinding);
await Hive.initFlutter();
SystemChrome.setSystemUIOverlayStyle(SystemUiOverlayStyle.dark);
SystemChrome.setSystemUIOverlayStyle(SystemUiOverlayStyle.dark);
Hive.registerAdapter(RegionAdapter());
await setUpRootDependencies();
if (UniversalPlatform.isAndroid) {
Expand All @@ -34,7 +34,7 @@ Future<void> main() async {
}

try {
getIt<IFirebaseService>().initializeApp(
await getIt<IFirebaseService>().initializeApp(

@vvlladd28 vvlladd28 Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awaiting this is the right call and I'd keep it — it's what makes every _isFirebaseConfigured read deterministic, and the cost is smaller than it looks: FlutterNativeSplash.preserve holds the native splash across the whole of main(), so the extra platform-channel round trip lands while the splash is still up rather than in front of a blank frame. Not worth restructuring into a stored future or a Future.wait.

One small thing on this line, though: FirebaseService.initializeApp catches everything internally — including the deliberate UnimplementedError on a custom endpoint — and returns null, so the surrounding try/catch can't fire even now that it's awaited. It reads as if failures are handled here. Either drop the try/catch or have initializeApp propagate.

@deaflynx deaflynx Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping the try/catch. The committed lib/firebase_options.dart is the FlutterFire stub, and its currentPlatform getter unconditionally throws UnsupportedError('Firebase have not been configured …'). That expression is evaluated inside the try before initializeApp() is even called, so without the catch every build from the public repo without a Firebase project would crash on startup. FirebaseService.initializeApp swallowing its own errors does not make this guard dead — it is guarding the argument, not the call.

options: DefaultFirebaseOptions.currentPlatform,
);
} catch (e) {
Expand Down
6 changes: 6 additions & 0 deletions lib/utils/services/local_database/i_local_database_service.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,10 @@ abstract interface class ILocalDatabaseService {
Future<String?> getInitialAppLink();

Future<void> deleteInitialAppLink();

Future<bool> isPushRegistered();

Future<void> setPushRegistered();

Future<void> clearPushRegistered();
}
16 changes: 16 additions & 0 deletions lib/utils/services/local_database/local_database_service.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,20 @@ class LocalDatabaseService implements ILocalDatabaseService {
Future<void> deleteInitialAppLink() {
return storage.deleteItem(DatabaseKeys.initialAppLink);
}

@override
Future<bool> isPushRegistered() async {
return await storage.getItem(DatabaseKeys.pushNotificationsRegistered) ==
true;
}

@override
Future<void> setPushRegistered() {
return storage.setItem(DatabaseKeys.pushNotificationsRegistered, true);

@vvlladd28 vvlladd28 Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional. isPushRegistered() reads through containsKey, so the true written here is never read back — the flag's truth is key presence and the stored value is decoration. Presence-as-flag is a fine pattern, but it's a quiet trap: someone who later needs a false state will write setItem(key, false) and it will still read as registered.

A line of comment saying presence is the contract is probably the right-sized fix; reading the value (await storage.getItem(...) == true) is the alternative if you'd rather the payload be the thing that matters.

@deaflynx deaflynx Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went with reading the value in cef25b4: isPushRegistered() is now await storage.getItem(key) == true, so a stored false reads as not registered. local_database_service_test.dart covers getItem returning true and null.

}

@override
Future<void> clearPushRegistered() {
return storage.deleteItem(DatabaseKeys.pushNotificationsRegistered);
}
}
Loading