store: Have UpdateMachine.load check account still exists after async gaps

Fixes: zulip#1354

Author: chrisbobbe

lib/model/store.dart

@@ -175,6 +175,9 @@ abstract class GlobalStore extends ChangeNotifier {
   ///
   /// The account for `accountId` must exist.
   ///
+  /// Throws [AccountNotFoundException] if after any async gap
+  /// the account has been removed.
+  ///
   /// This method should be called only by the implementation of [perAccount].
   /// Other callers interested in per-account data should use [perAccount]
   /// and/or [perAccountSync].
@@ -189,38 +192,30 @@ abstract class GlobalStore extends ChangeNotifier {
           // The API key is invalid and the store can never be loaded
           // unless the user retries manually.
           final account = getAccount(accountId);
-          if (account == null) {
-            // The account was logged out during `await doLoadPerAccount`.
-            // Here, that seems possible only by the user's own action;
-            // the logout can't have been done programmatically.
-            // Even if it were, it would have come with its own UI feedback.
-            // Anyway, skip showing feedback, to not be confusing or repetitive.
-            throw AccountNotFoundException();
-          }
+          assert(account != null); // doLoadPerAccount would have thrown AccountNotFoundException
           final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
           reportErrorToUserModally(
             zulipLocalizations.errorCouldNotConnectTitle,
             message: zulipLocalizations.errorInvalidApiKeyMessage(
-              account.realmUrl.toString()));
+              account!.realmUrl.toString()));
           await logOutAccount(this, accountId);
           throw AccountNotFoundException();
         default:
           rethrow;
       }
     }
-    if (!_accounts.containsKey(accountId)) {
-      // TODO(#1354): handle this earlier
-      // [removeAccount] was called during [doLoadPerAccount].
-      store.dispose();
-      throw AccountNotFoundException();
-    }
+    // doLoadPerAccount would have thrown AccountNotFoundException
+    assert(_accounts.containsKey(accountId));
     return store;
   }
 
   /// Load per-account data for the given account, unconditionally.
   ///
   /// The account for `accountId` must exist.
   ///
+  /// Throws [AccountNotFoundException] if after any async gap
+  /// the account has been removed.
+  ///
   /// This method should be called only by [loadPerAccount].
   Future<PerAccountStore> doLoadPerAccount(int accountId);
 
@@ -956,13 +951,26 @@ class UpdateMachine {
   ///
   /// The account for `accountId` must exist.
   ///
+  /// Throws [AccountNotFoundException] if after any async gap
+  /// the account has been removed.
+  ///
   /// In the future this might load an old snapshot from local storage first.
   static Future<UpdateMachine> load(GlobalStore globalStore, int accountId) async {
     Account account = globalStore.getAccount(accountId)!;
     final connection = globalStore.apiConnectionFromAccount(account);
 
+    void stopAndThrowIfNoAccount() {
+      final account = globalStore.getAccount(accountId);
+      if (account == null) {
+        assert(debugLog('Account logged out during UpdateMachine.load'));
+        connection.close();
+        throw AccountNotFoundException();
+      }
+    }
+
     final stopwatch = Stopwatch()..start();
-    final initialSnapshot = await _registerQueueWithRetry(connection);
+    final initialSnapshot = await _registerQueueWithRetry(connection,
+      stopAndThrowIfNoAccount: stopAndThrowIfNoAccount);
     final t = (stopwatch..stop()).elapsed;
     assert(debugLog("initial fetch time: ${t.inMilliseconds}ms"));
 
@@ -1004,13 +1012,20 @@ class UpdateMachine {
 
   bool _disposed = false;
 
+  /// Make the register-queue request, with retries.
+  ///
+  /// After each async gap, calls [stopAndThrowIfNoAccount].
   static Future<InitialSnapshot> _registerQueueWithRetry(
-      ApiConnection connection) async {
+    ApiConnection connection, {
+    required void Function() stopAndThrowIfNoAccount,
+  }) async {
     BackoffMachine? backoffMachine;
     while (true) {
+      InitialSnapshot? result;
       try {
-        return await registerQueue(connection);
+        result = await registerQueue(connection);
       } catch (e, s) {
+        stopAndThrowIfNoAccount();
         // TODO(#890): tell user if initial-fetch errors persist, or look non-transient
         switch (e) {
           case HttpException(httpStatus: 401):
@@ -1025,8 +1040,13 @@ class UpdateMachine {
         }
         assert(debugLog('Backing off, then will retry…'));
         await (backoffMachine ??= BackoffMachine()).wait();
+        stopAndThrowIfNoAccount();
         assert(debugLog('… Backoff wait complete, retrying initial fetch.'));
       }
+      if (result != null) {
+        stopAndThrowIfNoAccount();
+        return result;
+      }
     }
   }
 

test/model/store_test.dart

@@ -6,6 +6,7 @@ import 'package:fake_async/fake_async.dart';
 import 'package:flutter/foundation.dart';
 import 'package:http/http.dart' as http;
 import 'package:test/scaffolding.dart';
+import 'package:zulip/api/backoff.dart';
 import 'package:zulip/api/core.dart';
 import 'package:zulip/api/model/events.dart';
 import 'package:zulip/api/model/model.dart';
@@ -157,6 +158,42 @@ void main() {
     await check(future).throws<AccountNotFoundException>();
   }));
 
+  test('GlobalStore.perAccount loading succeeds', () => awaitFakeAsync((async) async {
+    NotificationService.instance.token = ValueNotifier('asdf');
+    addTearDown(NotificationService.debugReset);
+
+    final globalStore = UpdateMachineTestGlobalStore(accounts: [eg.selfAccount]);
+    final connection = globalStore.apiConnectionFromAccount(eg.selfAccount) as FakeApiConnection;
+    final future = globalStore.perAccount(eg.selfAccount.id);
+    check(connection.takeRequests()).length.equals(1); // register request
+
+    await future;
+    // poll, server-emoji-data, register-token requests
+    check(connection.takeRequests()).length.equals(3);
+    check(connection).isOpen.isTrue();
+  }));
+
+  test('GlobalStore.perAccount account is logged out while loading; then succeeds', () => awaitFakeAsync((async) async {
+    final globalStore = UpdateMachineTestGlobalStore(accounts: [eg.selfAccount]);
+    globalStore.prepareRegisterQueueResponse = (connection) =>
+      connection.prepare(
+        delay: TestGlobalStore.removeAccountDuration + Duration(seconds: 1),
+        json: eg.initialSnapshot().toJson());
+    final connection = globalStore.apiConnectionFromAccount(eg.selfAccount) as FakeApiConnection;
+    final future = globalStore.perAccount(eg.selfAccount.id);
+    check(connection.takeRequests()).length.equals(1); // register request
+
+    await logOutAccount(globalStore, eg.selfAccount.id);
+    check(globalStore.takeDoRemoveAccountCalls())
+      .single.equals(eg.selfAccount.id);
+
+    await check(future).throws<AccountNotFoundException>();
+    check(globalStore.takeDoRemoveAccountCalls()).isEmpty();
+    // no poll, server-emoji-data, or register-token requests
+    check(connection.takeRequests()).isEmpty();
+    check(connection).isOpen.isFalse();
+  }));
+
   test('GlobalStore.perAccount account is logged out while loading; then fails with HTTP status code 401', () => awaitFakeAsync((async) async {
     final globalStore = UpdateMachineTestGlobalStore(accounts: [eg.selfAccount]);
     globalStore.prepareRegisterQueueResponse = (connection) =>
@@ -175,8 +212,31 @@ void main() {
     check(globalStore.takeDoRemoveAccountCalls()).isEmpty();
     // no poll, server-emoji-data, or register-token requests
     check(connection.takeRequests()).isEmpty();
-    // TODO(#1354) uncomment
-    // check(connection).isOpen.isFalse();
+    check(connection).isOpen.isFalse();
+  }));
+
+  test('GlobalStore.perAccount account is logged out during transient-error backoff', () => awaitFakeAsync((async) async {
+    final globalStore = UpdateMachineTestGlobalStore(accounts: [eg.selfAccount]);
+    globalStore.prepareRegisterQueueResponse = (connection) =>
+      connection.prepare(
+        delay: Duration(seconds: 1),
+        httpException: http.ClientException('Oops'));
+    final connection = globalStore.apiConnectionFromAccount(eg.selfAccount) as FakeApiConnection;
+    final future = globalStore.perAccount(eg.selfAccount.id);
+    BackoffMachine.debugDuration = Duration(seconds: 1);
+    async.elapse(Duration(milliseconds: 1500));
+    check(connection.takeRequests()).length.equals(1); // register request
+
+    assert(TestGlobalStore.removeAccountDuration < Duration(milliseconds: 500));
+    await logOutAccount(globalStore, eg.selfAccount.id);
+    check(globalStore.takeDoRemoveAccountCalls())
+      .single.equals(eg.selfAccount.id);
+
+    await check(future).throws<AccountNotFoundException>();
+    check(globalStore.takeDoRemoveAccountCalls()).isEmpty();
+    // no retry-register, poll, server-emoji-data, or register-token requests
+    check(connection.takeRequests()).isEmpty();
+    check(connection).isOpen.isFalse();
   }));
 
   // TODO test insertAccount
@@ -278,10 +338,10 @@ void main() {
       checkGlobalStore(globalStore, eg.selfAccount.id,
         expectAccount: true, expectStore: false);
 
-      // assert(globalStore.useCachedApiConnections);
+      assert(globalStore.useCachedApiConnections);
       // Cache a connection and get this reference to it,
       // so we can check later that it gets closed.
-      // final connection = globalStore.apiConnectionFromAccount(eg.selfAccount) as FakeApiConnection;
+      final connection = globalStore.apiConnectionFromAccount(eg.selfAccount) as FakeApiConnection;
 
       globalStore.prepareRegisterQueueResponse = (connection) {
         connection.prepare(
@@ -301,14 +361,11 @@ void main() {
         expectAccount: false, expectStore: false);
       check(notifyCount).equals(1);
 
-      // Actually throws a null-check error; that's the bug #1354.
-      // TODO(#1354) should specifically throw AccountNotFoundException
-      await check(loadingFuture).throws();
+      await check(loadingFuture).throws<AccountNotFoundException>();
       checkGlobalStore(globalStore, eg.selfAccount.id,
         expectAccount: false, expectStore: false);
       check(notifyCount).equals(1); // no extra notify
-      // TODO(#1354) uncomment
-      // check(connection).isOpen.isFalse();
+      check(connection).isOpen.isFalse();
 
       check(globalStore.debugNumPerAccountStoresLoading).equals(0);
     });
@@ -1051,10 +1108,7 @@ void main() {
       async.flushTimers();
       // Reload never succeeds and there are no unhandled errors.
       check(globalStore.perAccountSync(eg.selfAccount.id)).isNull();
-    }),
-      // An unhandled error is actually the bug #1354, so skip for now
-      // TODO(#1354) unskip
-      skip: true);
+    }));
 
     test('new store is not loaded, gets HTTP 401 error instead', () => awaitFakeAsync((async) async {
       await prepareReload(async, prepareRegisterQueueResponse: (connection) {