wip store: On non-transient request errors, reload rather than retry; TODO test

Hmm or maybe for those non-transient request errors we should stick
with retrying?  Maybe up to a bounded number of times?

Those are by definition a bug either in the server or in our
formulating the request: it's either a 4xx we didn't expect,
or a malformed response (so in either case, a mismatch of our
expectations to the server's behavior, and therefore a bug in
one or the other), or something other than ApiRequestException
entirely (and so a bug in the API request code).

It's therefore hard to predict what's most likely to work.

Maybe the most likely cause is that the server is having operational
trouble that's not showing up as 5xx responses.

We handle rate-limit errors separately, though, and make sure
to back off if we get one of those.  So this may not be a
likely scenario.

Also, when we abandon retrying getEvents and switch to re-register,
we do end up backing off if that fails.  It's just that that's a
heavier request in the first place.

Author: gnprice

lib/model/store.dart

@@ -967,45 +967,29 @@ class UpdateMachine {
           if (_disposed) return;
 
           store.isLoading = true;
-          bool isUnexpected;
           bool shouldReportToUser;
           switch (e) {
-            case ZulipApiException(code: 'BAD_EVENT_QUEUE_ID'):
-              rethrow;
-
             case NetworkException(cause: SocketException()):
               // A [SocketException] is common when the app returns from sleep.
-              isUnexpected = false;
               shouldReportToUser = false;
 
             case NetworkException():
             case Server5xxException():
-              isUnexpected = false;
               shouldReportToUser = true;
 
             case ZulipApiException(httpStatus: 429):
             case ZulipApiException(code: 'RATE_LIMIT_HIT'):
               // TODO(#946) handle rate-limit errors more generally, in ApiConnection
-              isUnexpected = false;
               shouldReportToUser = true;
 
             default:
-              isUnexpected = true;
-              shouldReportToUser = true;
+              rethrow;
           }
 
-          if (isUnexpected) {
-            assert(shouldReportToUser);
-            assert(debugLog('Error polling event queue for $store: $e\n'
-                'Backing off and retrying even though may be hopeless…'));
-            // TODO(#186): Handle unrecoverable failures
-            _reportToUserErrorConnectingToServer(e);
-          } else {
-            assert(debugLog('Transient error polling event queue for $store: $e\n'
-                'Backing off, then will retry…'));
-            if (shouldReportToUser) {
-              maybeReportToUserTransientError(e);
-            }
+          assert(debugLog('Transient error polling event queue for $store: $e\n'
+              'Backing off, then will retry…'));
+          if (shouldReportToUser) {
+            maybeReportToUserTransientError(e);
           }
           await (backoffMachine ??= BackoffMachine()).wait();
           if (_disposed) return;
@@ -1050,7 +1034,7 @@ class UpdateMachine {
         }
       }
     } catch (e) {
-      // An error occurred, other than the request errors we retry on.
+      // An error occurred, other than the transient request errors we retry on.
       // This means either a lost/expired event queue on the server (which is
       // normal after the app is offline for a period like 10 minutes),
       // or an unexpected exception representing a bug in our code or the server.
@@ -1084,6 +1068,8 @@ class UpdateMachine {
           _reportToUserErrorConnectingToServer(e);
           // Similar story to the _EventHandlingException case;
           // separate only so that that other case can print more context.
+          // The bug here could be in the server if it's an ApiRequestException,
+          // but our remedy is the same either way.
       }
 
       // This disposes the store, which disposes this update machine.