From df9f63ff5780ca6b8a0e5492fcfab8759711c490 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 18:46:20 -0400 Subject: [PATCH 1/9] api [nfc]: Factor a _throwNetworkException helper out of send Upcoming commits will rework how the message is chosen and throw the same way from a second call site. Co-Authored-By: Claude Fable 5 --- lib/api/core.dart | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/lib/api/core.dart b/lib/api/core.dart index 200140fff..0f62ff82b 100644 --- a/lib/api/core.dart +++ b/lib/api/core.dart @@ -179,16 +179,7 @@ class ApiConnection { try { response = await _client.send(request); } catch (e) { - final String message; - if (e is http.ClientException) { - message = e.message; - } else if (e is TlsException) { - message = e.message; - } else { - final zulipLocalizations = GlobalLocalizations.zulipLocalizations; - message = zulipLocalizations.errorNetworkRequestFailed; - } - throw NetworkException(routeName: routeName, cause: e, message: message); + _throwNetworkException(routeName, e); } final int httpStatus = response.statusCode; @@ -280,6 +271,20 @@ class ApiConnection { } } +/// Throw a [NetworkException] wrapping the given exception +/// from the underlying HTTP client. +Never _throwNetworkException(String routeName, Object cause) { + final String message; + if (cause is http.ClientException) { + message = cause.message; + } else if (cause is TlsException) { + message = cause.message; + } else { + message = GlobalLocalizations.zulipLocalizations.errorNetworkRequestFailed; + } + throw NetworkException(routeName: routeName, cause: cause, message: message); +} + ApiRequestException _makeApiException(String routeName, int httpStatus, Map? json) { assert(httpStatus != 200 || json == null); if (400 <= httpStatus && httpStatus <= 499) { From e73317ceff5cbae0c53eaaa5ead4f76fe0d06ed0 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 16:36:28 -0400 Subject: [PATCH 2/9] store test: Cover the error-reporting decision on the registerQueue path When the initial registerQueue attempt fails, we report the error to the user unless it is a routine connection failure. That decision had no test coverage. Add some now, ahead of reworking in the next commit how the decision is made. Co-Authored-By: Claude Opus 5 Co-Authored-By: Claude Fable 5 --- test/model/store_test.dart | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/test/model/store_test.dart b/test/model/store_test.dart index d4852153c..fede0fdcb 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -686,6 +686,44 @@ void main() { users.map((expected) => (it) => it.fullName.equals(expected.fullName))); })); + group('registerQueue error reporting', () { + String? lastReportedError; + + Future prepare() async { + lastReportedError = null; + reportErrorToUserBriefly = (message, {details}) async { + if (message == null) return; + lastReportedError = message; + }; + addTearDown(() => + reportErrorToUserBriefly = defaultReportErrorToUserBriefly); + await prepareStore(); + globalStore.useCachedApiConnections = true; + } + + /// Load, with the first registerQueue attempt failing with [exception] + /// and the retry succeeding. + Future loadWithFirstAttemptFailing(Object exception) async { + connection.prepare(httpException: exception); + connection.prepare(json: eg.initialSnapshot().toJson()); + final updateMachine = await UpdateMachine.load( + globalStore, eg.selfAccount.id); + updateMachine.debugPauseLoop(); + } + + test('no report on failed connection', () => awaitFakeAsync((async) async { + await prepare(); + await loadWithFirstAttemptFailing(const SocketException('failed')); + check(lastReportedError).isNull(); + })); + + test('report other network error', () => awaitFakeAsync((async) async { + await prepare(); + await loadWithFirstAttemptFailing(Exception('failed')); + check(lastReportedError).isNotNull(); + })); + }); + // TODO test UpdateMachine.load starts polling loop }); From b1c8695c089598fbadea99510f65309481a71881 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 26 Jul 2026 21:25:03 +0000 Subject: [PATCH 3/9] api: Classify network errors by kind, not by dart:io exception type When deciding how to handle a NetworkException, callers matched on whether its cause was a SocketException, in order to recognize the routine connection failures that happen when the device is offline or returns from sleep. That ties the decision to the exception types of dart:io. A different HTTP client implementation, like the cronet_http and cupertino_http clients considered in #461, reports these conditions with different types (both wrap errors in plain ClientException subclasses); the app would quietly begin showing the user connection errors whenever a routine failure persisted past a few poll retries, as after waking from a long sleep. So classify the cause once, in _throwNetworkException, into a new NetworkExceptionKind, chosen together with the user-facing message in a single switch; and have callers switch on the kind instead. No change in behavior, as the tests added in the previous commit help confirm. Co-Authored-By: Claude Opus 5 Co-Authored-By: Claude Fable 5 --- lib/api/core.dart | 22 ++++++++++++--------- lib/api/exception.dart | 36 +++++++++++++++++++++++++++++++++- lib/model/store.dart | 8 ++++---- test/api/core_test.dart | 8 ++++++++ test/api/exception_checks.dart | 1 + test/model/store_test.dart | 12 +++++++----- 6 files changed, 68 insertions(+), 19 deletions(-) diff --git a/lib/api/core.dart b/lib/api/core.dart index 0f62ff82b..ead64633d 100644 --- a/lib/api/core.dart +++ b/lib/api/core.dart @@ -274,15 +274,19 @@ class ApiConnection { /// Throw a [NetworkException] wrapping the given exception /// from the underlying HTTP client. Never _throwNetworkException(String routeName, Object cause) { - final String message; - if (cause is http.ClientException) { - message = cause.message; - } else if (cause is TlsException) { - message = cause.message; - } else { - message = GlobalLocalizations.zulipLocalizations.errorNetworkRequestFailed; - } - throw NetworkException(routeName: routeName, cause: cause, message: message); + final zulipLocalizations = GlobalLocalizations.zulipLocalizations; + final (NetworkExceptionKind kind, String message) = switch (cause) { + // A wrapped SocketException, like package:http's IOClient throws + // for a connection failure. + SocketException() && http.ClientException(:final message) => + (.connectionFailed, message), + SocketException() => (.connectionFailed, zulipLocalizations.errorNetworkRequestFailed), + http.ClientException(:final message) => (.other, message), + TlsException(:final message) => (.other, message), + _ => (.other, zulipLocalizations.errorNetworkRequestFailed), + }; + throw NetworkException(routeName: routeName, + kind: kind, cause: cause, message: message); } ApiRequestException _makeApiException(String routeName, int httpStatus, Map? json) { diff --git a/lib/api/exception.dart b/lib/api/exception.dart index d495ec9ff..a7e18364e 100644 --- a/lib/api/exception.dart +++ b/lib/api/exception.dart @@ -28,19 +28,53 @@ sealed class ApiRequestException implements Exception { String toString() => message; } +/// What sort of underlying problem caused a [NetworkException]. +/// +/// The classification is made in [ApiConnection.send], +/// which is the one place that sees the raw exception +/// from the underlying HTTP client. +/// Consumers should switch on this +/// rather than inspecting [NetworkException.cause], +/// because the cause's concrete type depends on +/// which HTTP client implementation is in use. +enum NetworkExceptionKind { + /// The connection couldn't be established, or was lost, + /// below the level of any HTTP response. + /// + /// This is routine rather than a sign of a bug: + /// it's what happens when the device is offline, + /// and commonly when the app returns from sleep. + connectionFailed, + + /// Any other network-level failure. + other, +} + /// A network-level error that prevented even getting an HTTP response /// to some Zulip API network request. /// /// This is the antonym of [HttpException]. class NetworkException extends ApiRequestException { + /// What sort of problem this is. + final NetworkExceptionKind kind; + /// The exception describing the underlying error. /// /// This can be any exception value that [http.Client.send] throws. /// Ideally that would always be an [http.ClientException], /// but empirically it can be [TlsException] and possibly others. + /// + /// The concrete type depends on which HTTP client implementation + /// the app is using, so prefer [kind] for making decisions; + /// this is most useful for logging. final Object cause; - NetworkException({required super.routeName, required super.message, required this.cause}); + NetworkException({ + required super.routeName, + required super.message, + required this.kind, + required this.cause, + }); @override String toString() { diff --git a/lib/model/store.dart b/lib/model/store.dart index 5b554bf64..a707498b4 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -1504,8 +1504,8 @@ class UpdateMachine { // Print stack trace in its own log entry; log entries are truncated // at 1 kiB (at least on Android), and stack can be longer than that. assert(debugLog('Stack:\n$stackTrace')); - if (e case NetworkException(cause: SocketException())) { - // A [SocketException] is common when the device is asleep. + if (e case NetworkException(kind: .connectionFailed)) { + // A failed connection is common when the device is asleep. } else { // TODO: When the error seems transient, do keep retrying but // don't spam this feedback. @@ -1741,8 +1741,8 @@ class UpdateMachine { bool shouldReportToUser; switch (error) { - case NetworkException(cause: SocketException()): - // A [SocketException] is common when the app returns from sleep. + case NetworkException(kind: .connectionFailed): + // A failed connection is common when the app returns from sleep. shouldReportToUser = false; case NetworkException(): diff --git a/test/api/core_test.dart b/test/api/core_test.dart index 903f35eef..53dd8e559 100644 --- a/test/api/core_test.dart +++ b/test/api/core_test.dart @@ -283,13 +283,21 @@ void main() { } final zulipLocalizations = GlobalLocalizations.zulipLocalizations; + checkRequest(const SocketException('Oops'), (it) => it + ..kind.equals(.connectionFailed) + ..message.equals(zulipLocalizations.errorNetworkRequestFailed) + ..asString.equals( + 'NetworkException: Network request failed (SocketException: Oops)')); checkRequest(http.ClientException('Oops'), (it) => it + ..kind.equals(.other) ..message.equals('Oops') ..asString.equals('NetworkException: Oops (ClientException: Oops)')); checkRequest(const TlsException('Oops'), (it) => it + ..kind.equals(.other) ..message.equals('Oops') ..asString.equals('NetworkException: Oops (TlsException: Oops)')); checkRequest((foo: 'bar'), (it) => it + ..kind.equals(.other) ..message.equals(zulipLocalizations.errorNetworkRequestFailed) ..asString.equals('NetworkException: Network request failed ((foo: bar))')); }); diff --git a/test/api/exception_checks.dart b/test/api/exception_checks.dart index 6f7f07daf..3a2ab18a6 100644 --- a/test/api/exception_checks.dart +++ b/test/api/exception_checks.dart @@ -14,6 +14,7 @@ extension ZulipApiExceptionChecks on Subject { } extension NetworkExceptionChecks on Subject { + Subject get kind => has((e) => e.kind, 'kind'); Subject get cause => has((e) => e.cause, 'cause'); } diff --git a/test/model/store_test.dart b/test/model/store_test.dart index fede0fdcb..3746fa9ed 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -940,7 +940,9 @@ void main() { updateMachine.debugPrepareLoopError(eg.nullCheckError()); } - void prepareNetworkExceptionSocketException() { + // [ApiConnection] classifies a [SocketException] + // as [NetworkExceptionKind.connectionFailed]. + void prepareNetworkExceptionConnectionFailed() { connection.prepare(httpException: const SocketException('failed')); } @@ -1007,9 +1009,9 @@ void main() { checkReload(prepareUnexpectedLoopError); }); - test('retries on NetworkException from SocketException', () { + test('retries on NetworkException from failed connection', () { // We skip reporting errors on these; check we retry them all the same. - checkRetry(prepareNetworkExceptionSocketException); + checkRetry(prepareNetworkExceptionConnectionFailed); }); test('retries on generic NetworkException', () { @@ -1149,8 +1151,8 @@ void main() { checkReported(prepareUnexpectedLoopError); }); - test('ignore NetworkException from SocketException', () { - checkNotReported(prepareNetworkExceptionSocketException); + test('ignore NetworkException from failed connection', () { + checkNotReported(prepareNetworkExceptionConnectionFailed); }); test('eventually report generic NetworkException', () { From e2dd2f16c908110f1d81c9c462ce7532182576db Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 18:09:55 +0000 Subject: [PATCH 4/9] api tests: Cover the wrapped SocketException that IOClient really throws For a connection failure, package:http does not hand us the bare SocketException that FakeApiConnection simulates. Its IOClient catches that and rethrows a private _ClientSocketException, which extends ClientException and also implements SocketException. That is deliberate, so as not to break callers that catch the latter. The switch in _throwNetworkException handles the wrapped form with its own arm, ahead of the plain-ClientException arm that would classify it as `other`. A plausible edit, simplifying the switch or reordering its arms, would silently break that; then routine connection failures, like when the device is offline, would be reported to the user once they persisted past a few poll retries. So add a stand-in for the wrapped form, and a case pinning down how it is classified. Co-Authored-By: Claude Opus 5 Co-Authored-By: Claude Fable 5 --- test/api/core_test.dart | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/api/core_test.dart b/test/api/core_test.dart index 53dd8e559..da79c6269 100644 --- a/test/api/core_test.dart +++ b/test/api/core_test.dart @@ -288,6 +288,11 @@ void main() { ..message.equals(zulipLocalizations.errorNetworkRequestFailed) ..asString.equals( 'NetworkException: Network request failed (SocketException: Oops)')); + // What `IOClient` actually throws when the connection fails: + // a ClientException that also implements SocketException. + checkRequest(ClientSocketException('Oops'), (it) => it + ..kind.equals(.connectionFailed) + ..message.equals('Oops')); checkRequest(http.ClientException('Oops'), (it) => it ..kind.equals(.other) ..message.equals('Oops') @@ -499,6 +504,24 @@ class DistinctiveError extends Error { String toString() => message; } +/// Like the private `_ClientSocketException` +/// that `package:http`'s `IOClient` actually throws +/// when the underlying `HttpClient` throws a [SocketException]: +/// an [http.ClientException] that also implements [SocketException]. +class ClientSocketException extends http.ClientException + implements SocketException { + ClientSocketException(super.message, [super.uri]); + + @override + InternetAddress? get address => null; + + @override + OSError? get osError => null; + + @override + int? get port => null; +} + Future tryRequest({ Object? exception, int? httpStatus, From 8ddc423629556e1d35454d1272d352435ca7fe44 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 15:36:51 -0400 Subject: [PATCH 5/9] api: Parse event_queue_longpoll_timeout_seconds in register response This is the timeout the server recommends for GET /events requests; an upcoming commit will start using it. The field has been present since Zulip 5.0 (feature level 74), well below our minimum, so it needs no fallback. Co-Authored-By: Claude Opus 5 Co-Authored-By: Claude Fable 5 --- lib/api/model/initial_snapshot.dart | 3 +++ lib/api/model/initial_snapshot.g.dart | 4 ++++ test/example_data.dart | 2 ++ 3 files changed, 9 insertions(+) diff --git a/lib/api/model/initial_snapshot.dart b/lib/api/model/initial_snapshot.dart index 1709d007b..ea786bdfa 100644 --- a/lib/api/model/initial_snapshot.dart +++ b/lib/api/model/initial_snapshot.dart @@ -131,6 +131,8 @@ class InitialSnapshot { final Uri serverEmojiDataUrl; + final int eventQueueLongpollTimeoutSeconds; + final int? realmModerationRequestChannelId; // TODO(server-10) final String? realmEmptyTopicDisplayName; // TODO(server-10) @@ -216,6 +218,7 @@ class InitialSnapshot { required this.maxFileUploadSizeMib, required this.serverThumbnailFormats, required this.serverEmojiDataUrl, + required this.eventQueueLongpollTimeoutSeconds, required this.realmModerationRequestChannelId, required this.realmEmptyTopicDisplayName, required this.realmUsers, diff --git a/lib/api/model/initial_snapshot.g.dart b/lib/api/model/initial_snapshot.g.dart index 8ecc74a76..ae4eb657d 100644 --- a/lib/api/model/initial_snapshot.g.dart +++ b/lib/api/model/initial_snapshot.g.dart @@ -142,6 +142,8 @@ InitialSnapshot _$InitialSnapshotFromJson( .toList() ?? [], serverEmojiDataUrl: Uri.parse(json['server_emoji_data_url'] as String), + eventQueueLongpollTimeoutSeconds: + (json['event_queue_longpoll_timeout_seconds'] as num).toInt(), realmModerationRequestChannelId: (json['realm_moderation_request_channel_id'] as num?)?.toInt(), realmEmptyTopicDisplayName: json['realm_empty_topic_display_name'] as String?, @@ -226,6 +228,8 @@ Map _$InitialSnapshotToJson( 'max_file_upload_size_mib': instance.maxFileUploadSizeMib, 'server_thumbnail_formats': instance.serverThumbnailFormats, 'server_emoji_data_url': instance.serverEmojiDataUrl.toString(), + 'event_queue_longpoll_timeout_seconds': + instance.eventQueueLongpollTimeoutSeconds, 'realm_moderation_request_channel_id': instance.realmModerationRequestChannelId, 'realm_empty_topic_display_name': instance.realmEmptyTopicDisplayName, diff --git a/test/example_data.dart b/test/example_data.dart index 820f00497..f82e50103 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -1451,6 +1451,7 @@ InitialSnapshot initialSnapshot({ int? maxFileUploadSizeMib, List? serverThumbnailFormats, Uri? serverEmojiDataUrl, + int? eventQueueLongpollTimeoutSeconds, int? realmModerationRequestChannelId = -1, String? realmEmptyTopicDisplayName, List? realmUsers, @@ -1519,6 +1520,7 @@ InitialSnapshot initialSnapshot({ serverThumbnailFormats: serverThumbnailFormats ?? [], serverEmojiDataUrl: serverEmojiDataUrl ?? realmUrl.replace(path: '/static/emoji.json'), + eventQueueLongpollTimeoutSeconds: eventQueueLongpollTimeoutSeconds ?? 90, realmModerationRequestChannelId: realmModerationRequestChannelId, realmEmptyTopicDisplayName: realmEmptyTopicDisplayName ?? defaultRealmEmptyTopicDisplayName, realmUsers: realmUsers ?? [selfUser], From 225157aa10e0bf0ab613a10f8a34857c57dc4a36 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 15:48:42 -0400 Subject: [PATCH 6/9] realm: Add RealmStore.eventQueueLongpollTimeout The poll loop will use this in the next commit. Co-Authored-By: Claude Opus 5 Co-Authored-By: Claude Fable 5 --- lib/model/realm.dart | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/model/realm.dart b/lib/model/realm.dart index cffb3095f..3d1f271ee 100644 --- a/lib/model/realm.dart +++ b/lib/model/realm.dart @@ -20,6 +20,9 @@ mixin RealmStore on PerAccountStoreBase, UserGroupStore { //|////////////////////////////////////////////////////////////// // Server settings, explicitly so named. + Duration get eventQueueLongpollTimeout => Duration(seconds: eventQueueLongpollTimeoutSeconds); + int get eventQueueLongpollTimeoutSeconds; + Duration get serverPresencePingInterval => Duration(seconds: serverPresencePingIntervalSeconds); int get serverPresencePingIntervalSeconds; Duration get serverPresenceOfflineThreshold => Duration(seconds: serverPresenceOfflineThresholdSeconds); @@ -171,6 +174,8 @@ mixin ProxyRealmStore on RealmStore { @protected RealmStore get realmStore; + @override + int get eventQueueLongpollTimeoutSeconds => realmStore.eventQueueLongpollTimeoutSeconds; @override int get serverPresencePingIntervalSeconds => realmStore.serverPresencePingIntervalSeconds; @override @@ -255,6 +260,7 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore { }) : _selfUserRole = selfUser.role, _selfUserDateJoined = selfUser.dateJoined, + eventQueueLongpollTimeoutSeconds = initialSnapshot.eventQueueLongpollTimeoutSeconds, serverPresencePingIntervalSeconds = initialSnapshot.serverPresencePingIntervalSeconds, serverPresenceOfflineThresholdSeconds = initialSnapshot.serverPresenceOfflineThresholdSeconds, serverTypingStartedExpiryPeriodMilliseconds = initialSnapshot.serverTypingStartedExpiryPeriodMilliseconds, @@ -401,6 +407,9 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore { /// See also [_selfUserRole]. final String _selfUserDateJoined; + @override + final int eventQueueLongpollTimeoutSeconds; + @override final int serverPresencePingIntervalSeconds; @override From e9b4efb8b9be8285211279bb528fd3aee1797e3b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 21:06:24 -0400 Subject: [PATCH 7/9] api tests: Have FakeApiConnection mimic IOClient on aborted requests package:http's Abortable lets a request carry a trigger that aborts it. Teach FakeHttpClient the behavior IOClient has for that: if the trigger fires before the response headers are delivered, the send future throws a RequestAbortedException; if it fires while the body is streaming, the same exception is injected into the body stream. Also add a `bodyDelay` option to `prepare`, so a test can arrange for a response's body to still be in flight at a chosen moment. The next commit will use these to test a timeout on getEvents. Co-Authored-By: Claude Fable 5 --- test/api/fake_api.dart | 68 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 60 insertions(+), 8 deletions(-) diff --git a/test/api/fake_api.dart b/test/api/fake_api.dart index 2382ab859..3b9b193a6 100644 --- a/test/api/fake_api.dart +++ b/test/api/fake_api.dart @@ -1,3 +1,4 @@ +import 'dart:async'; import 'dart:collection'; import 'dart:convert'; @@ -25,8 +26,14 @@ class _PreparedException extends _PreparedResponse { class _PreparedSuccess extends _PreparedResponse { final int httpStatus; final List bytes; - - _PreparedSuccess({super.delay, required this.httpStatus, required this.bytes}); + final Duration bodyDelay; + + _PreparedSuccess({ + super.delay, + required this.httpStatus, + required this.bytes, + this.bodyDelay = Duration.zero, + }); } /// An [http.Client] that accepts and replays canned responses, for testing. @@ -54,17 +61,24 @@ class FakeHttpClient extends http.BaseClient { /// /// If `exception` is non-null, then `httpStatus`, `body`, and `json` must /// all be null, and the next request will throw the given exception. + /// + /// In each case, the next request will complete a duration of `delay` + /// after being started. + /// On success, the response body arrives a further `bodyDelay` + /// after the response's headers. void prepare({ Object? exception, int? httpStatus, Map? json, String? body, Duration delay = Duration.zero, + Duration bodyDelay = Duration.zero, }) { // TODO: Prevent a source of bugs by ensuring that there are no outstanding // prepared responses when the test ends. if (exception != null) { - assert(httpStatus == null && json == null && body == null); + assert(httpStatus == null && json == null && body == null + && bodyDelay == Duration.zero); _preparedResponses.addLast(_PreparedException(exception: exception, delay: delay)); } else { assert((json == null) || (body == null)); @@ -77,6 +91,7 @@ class FakeHttpClient extends http.BaseClient { httpStatus: httpStatus ?? 200, bytes: utf8.encode(resolvedBody), delay: delay, + bodyDelay: bodyDelay, )); } } @@ -100,16 +115,50 @@ class FakeHttpClient extends http.BaseClient { } final response = _preparedResponses.removeFirst(); + final abortTrigger = + (request is http.Abortable) ? request.abortTrigger : null; + final http.StreamedResponse Function() computation; switch (response) { case _PreparedException(:var exception): computation = () => throw exception; - case _PreparedSuccess(:var bytes, :var httpStatus): - final byteStream = http.ByteStream.fromBytes(bytes); + case _PreparedSuccess(:var bytes, :var httpStatus, :var bodyDelay): computation = () => http.StreamedResponse( - byteStream, httpStatus, request: request); + _bodyStream(request, bytes: bytes, bodyDelay: bodyDelay, + abortTrigger: abortTrigger), + httpStatus, request: request); + } + final result = Future.delayed(response.delay, computation); + if (abortTrigger == null) return result; + // Mimic [IOClient]: if the trigger fires before the response headers + // are delivered, [send]'s future throws instead. + return Future.any([ + result, + abortTrigger.then((_) => throw http.RequestAbortedException(request.url)), + ]); + } + + /// The response body, mimicking [IOClient]'s abort behavior: + /// if [abortTrigger] fires before the body has been delivered, + /// inject an [http.RequestAbortedException] and close the stream. + http.ByteStream _bodyStream(http.BaseRequest request, { + required List bytes, + required Duration bodyDelay, + required Future? abortTrigger, + }) { + if (abortTrigger == null && bodyDelay == Duration.zero) { + return http.ByteStream.fromBytes(bytes); } - return Future.delayed(response.delay, computation); + final controller = StreamController>(); + Timer(bodyDelay, () { + if (controller.isClosed) return; + controller..add(bytes)..close(); + }); + abortTrigger?.whenComplete(() { + if (controller.isClosed) return; + controller..addError(http.RequestAbortedException(request.url))..close(); + }); + return http.ByteStream(controller.stream); } } @@ -236,6 +285,8 @@ class FakeApiConnection extends ApiConnection { /// /// In each case, the next request will complete a duration of `delay` /// after being started. + /// On success, the response body arrives a further `bodyDelay` + /// after the response's headers. void prepare({ Object? httpException, ZulipApiException? apiException, @@ -243,6 +294,7 @@ class FakeApiConnection extends ApiConnection { Map? json, String? body, Duration delay = Duration.zero, + Duration bodyDelay = Duration.zero, }) { assert(isOpen); @@ -279,7 +331,7 @@ class FakeApiConnection extends ApiConnection { client.prepare( exception: httpException, httpStatus: httpStatus, json: json, body: body, - delay: delay, + delay: delay, bodyDelay: bodyDelay, ); } From f56f0ad993778284c9f8f90619641385784ecbfd Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 16:37:24 -0400 Subject: [PATCH 8/9] api: Support a timeout on GET requests When the caller passes `timeout` and the request does not complete within it, including reading the response body, the request is aborted outright, with package:http's Abortable: that tears down the connection rather than leaking it. The resulting RequestAbortedException classifies as NetworkExceptionKind.connectionFailed, the same as the other ways of losing a connection. The next commit will use this to time out long-poll requests (#514). Co-Authored-By: Claude Opus 5 Co-Authored-By: Claude Fable 5 --- lib/api/core.dart | 41 ++++++++++++++++++++++++++++++++++++++--- lib/api/exception.dart | 7 ++++++- test/api/core_test.dart | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 4 deletions(-) diff --git a/lib/api/core.dart b/lib/api/core.dart index ead64633d..a217c6de7 100644 --- a/lib/api/core.dart +++ b/lib/api/core.dart @@ -1,3 +1,4 @@ +import 'dart:async'; import 'dart:convert'; import 'dart:io'; @@ -189,6 +190,9 @@ class ApiConnection { // while the response is still being downloaded, improving latency. final jsonStream = jsonUtf8Decoder.bind(response.stream); json = await jsonStream.single as Map?; + } on http.RequestAbortedException catch (e) { + // The timeout in [_withTimeout] fired while we were reading the response. + _throwNetworkException(routeName, e); } catch (e) { // We'll throw something below, seeing `json` is null. } @@ -214,12 +218,38 @@ class ApiConnection { _isOpen = false; } + /// Make a GET request to the given path with the given params. + /// + /// If [timeout] is non-null and the request hasn't completed within it, + /// including reading the response body, + /// then the request is aborted, tearing down the connection, + /// and this throws a [NetworkException] + /// with kind [NetworkExceptionKind.connectionFailed]. Future get(String routeName, T Function(Map) fromJson, - String path, Map? params) async { + String path, Map? params, {Duration? timeout}) async { final url = realmUrl.replace( path: "/api/v1/$path", queryParameters: encodeParameters(params)); - final request = http.Request('GET', url); - return send(routeName, fromJson, request); + if (timeout == null) { + return send(routeName, fromJson, http.Request('GET', url)); + } + return _withTimeout(timeout, (abortTrigger) => send(routeName, fromJson, + http.AbortableRequest('GET', url, abortTrigger: abortTrigger))); + } + + /// Run [body] with a trigger that fires after [timeout], + /// for constructing an [http.Abortable] request. + /// + /// The timer is canceled when the returned future completes, + /// so a request that finishes in time leaves no timer behind. + Future _withTimeout(Duration timeout, + Future Function(Future abortTrigger) body) async { + final abortCompleter = Completer(); + final abortTimer = Timer(timeout, abortCompleter.complete); + try { + return await body(abortCompleter.future); + } finally { + abortTimer.cancel(); + } } Future post(String routeName, T Function(Map) fromJson, @@ -276,6 +306,11 @@ class ApiConnection { Never _throwNetworkException(String routeName, Object cause) { final zulipLocalizations = GlobalLocalizations.zulipLocalizations; final (NetworkExceptionKind kind, String message) = switch (cause) { + // Our own timeout, from [ApiConnection._withTimeout]. Skip the + // exception's message: it names a package-internal mechanism + // ("Request aborted by `abortTrigger`"), which wouldn't mean much to a user. + http.RequestAbortedException() => + (.connectionFailed, zulipLocalizations.errorNetworkRequestFailed), // A wrapped SocketException, like package:http's IOClient throws // for a connection failure. SocketException() && http.ClientException(:final message) => diff --git a/lib/api/exception.dart b/lib/api/exception.dart index a7e18364e..6b67c25ba 100644 --- a/lib/api/exception.dart +++ b/lib/api/exception.dart @@ -41,9 +41,14 @@ enum NetworkExceptionKind { /// The connection couldn't be established, or was lost, /// below the level of any HTTP response. /// + /// This includes a request timing out, + /// which we treat as the connection having died + /// even if it still looks open. + /// /// This is routine rather than a sign of a bug: /// it's what happens when the device is offline, - /// and commonly when the app returns from sleep. + /// and commonly when the app returns from sleep + /// or the device switches networks. connectionFailed, /// Any other network-level failure. diff --git a/test/api/core_test.dart b/test/api/core_test.dart index da79c6269..e7861eaf8 100644 --- a/test/api/core_test.dart +++ b/test/api/core_test.dart @@ -10,6 +10,7 @@ import 'package:zulip/api/exception.dart'; import 'package:zulip/model/binding.dart'; import 'package:zulip/model/localizations.dart'; +import '../fake_async.dart'; import '../model/binding.dart'; import '../stdlib_checks.dart'; import '../test_async.dart'; @@ -307,6 +308,41 @@ void main() { ..asString.equals('NetworkException: Network request failed ((foo: bar))')); }); + test('API request timeout', () => awaitFakeAsync((async) async { + await FakeApiConnection.with_((connection) async { + connection.prepare(delay: const Duration(seconds: 300), json: {}); + await check(connection.get(kExampleRouteName, (json) => json, + 'example/route', {}, timeout: const Duration(seconds: 90))) + .throws((it) => it + ..routeName.equals(kExampleRouteName) + ..kind.equals(.connectionFailed) + ..cause.isA()); + }); + })); + + test('API request timeout while reading response body', () => awaitFakeAsync((async) async { + await FakeApiConnection.with_((connection) async { + connection.prepare(bodyDelay: const Duration(seconds: 300), json: {}); + await check(connection.get(kExampleRouteName, (json) => json, + 'example/route', {}, timeout: const Duration(seconds: 90))) + .throws((it) => it + ..routeName.equals(kExampleRouteName) + ..kind.equals(.connectionFailed) + ..cause.isA()); + }); + })); + + test('no API request timeout when response is in time', () => awaitFakeAsync((async) async { + await FakeApiConnection.with_((connection) async { + connection.prepare(delay: const Duration(seconds: 30), json: {'x': 3}); + final result = await connection.get(kExampleRouteName, (json) => json['x'], + 'example/route', {}, timeout: const Duration(seconds: 90)); + check(result).equals(3); + // The timeout's timer was canceled, not left to linger. + check(async.pendingTimers).isEmpty(); + }); + })); + test('API 4xx errors, well formed', () async { Future checkRequest({ int httpStatus = 400, From 203d9440f0528a98425c2f4fbf64a88f323c353e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 16:37:24 -0400 Subject: [PATCH 9/9] api: Time out long-poll requests, as the server recommends Fixes #514. On a degraded or switching network, the GET /events connection can die without the socket erroring: packets stop arriving, but nothing tells us so. A long-poll is especially bad at noticing this, because silence for up to a minute is exactly what a healthy long-poll looks like. We then wait for the OS to give up on the TCP connection, which can take minutes, and meanwhile no events reach the app: no incoming messages, and the local echo of a message the user just sent stays on screen looking stuck. The server anticipates this: it sends heartbeat events so that silence is bounded, and event_queue_longpoll_timeout_seconds says how long to wait before concluding the request or its response was lost. The API docs ask long-lived clients to respect that value, so that a later change to the heartbeat interval does not break them. So pass it as a timeout on each getEvents request, the same way the web app does. A timed-out request classifies as NetworkExceptionKind.connectionFailed, so the poll loop retries it quietly with backoff, the same as any other lost connection. Co-Authored-By: Claude Opus 5 Co-Authored-By: Claude Fable 5 --- lib/api/route/events.dart | 8 +++++++- lib/model/store.dart | 5 ++++- test/model/store_test.dart | 40 +++++++++++++++++++++++++++++++------- 3 files changed, 44 insertions(+), 9 deletions(-) diff --git a/lib/api/route/events.dart b/lib/api/route/events.dart index df12082fc..feb55749d 100644 --- a/lib/api/route/events.dart +++ b/lib/api/route/events.dart @@ -70,14 +70,20 @@ enum _IdleQueueTimeout { } /// https://zulip.com/api/get-events +/// +/// Long-lived clients should pass [timeout], +/// using the value the server recommends +/// in [InitialSnapshot.eventQueueLongpollTimeoutSeconds]; +/// see the discussion in the API docs for this endpoint. Future getEvents(ApiConnection connection, { required String queueId, int? lastEventId, bool? dontBlock, + Duration? timeout, }) { return connection.get('getEvents', GetEventsResult.fromJson, 'events', { 'queue_id': RawParameter(queueId), 'last_event_id': ?lastEventId, 'dont_block': ?dontBlock, - }); + }, timeout: timeout); } @JsonSerializable(fieldRename: FieldRename.snake) diff --git a/lib/model/store.dart b/lib/model/store.dart index a707498b4..cd95dba0e 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -1638,7 +1638,10 @@ class UpdateMachine { // ask the server to tell us immediately that it's working again, // rather than waiting for an event, which could take up to a minute // in the case of a heartbeat event. See #979. - dontBlock: store.isRecoveringEventStream ? true : null); + dontBlock: store.isRecoveringEventStream ? true : null, + // If the request outlives this, assume the connection is dead + // even if it still looks open; give up on it and retry. See #514. + timeout: store.eventQueueLongpollTimeout); if (_disposed) return; } catch (e, stackTrace) { if (_disposed) return; diff --git a/test/model/store_test.dart b/test/model/store_test.dart index 3746fa9ed..973fad59b 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -802,10 +802,14 @@ void main() { connection = store.connection as FakeApiConnection; } - Future preparePoll({int? lastEventId}) async { + Future preparePoll({ + int? lastEventId, + int? eventQueueLongpollTimeoutSeconds, + }) async { globalStore = eg.globalStore(); await globalStore.add(eg.selfAccount, eg.initialSnapshot( - lastEventId: lastEventId)); + lastEventId: lastEventId, + eventQueueLongpollTimeoutSeconds: eventQueueLongpollTimeoutSeconds)); await globalStore.perAccount(eg.selfAccount.id); updateFromGlobalStore(); updateMachine.debugPauseLoop(); @@ -904,24 +908,37 @@ void main() { }); } - void checkRetry(void Function() prepareError) { + /// Check the poll loop quietly retries when [prepareError] + /// causes the request to fail. + /// + /// [elapse] is how long the request takes to fail: + /// zero for an immediate error, + /// or the poll timeout for a request that times out. + void checkRetry(void Function() prepareError, { + Duration elapse = Duration.zero, + int? eventQueueLongpollTimeoutSeconds, + }) { awaitFakeAsync((async) async { - await preparePoll(lastEventId: 1); + await preparePoll(lastEventId: 1, + eventQueueLongpollTimeoutSeconds: eventQueueLongpollTimeoutSeconds); check(async.pendingTimers).length.equals(0); // Make the request, inducing an error in it. prepareError(); updateMachine.debugAdvanceLoop(); - async.elapse(Duration.zero); + async.elapse(elapse); checkLastRequest(lastEventId: 1, expectDontBlock: false); check(store).isRecoveringEventStream.isTrue(); // Polling doesn't resume immediately; there's a timer. - check(async.pendingTimers).length.equals(1); + // (On a timed-out request, the fake's timer for the response + // that never arrived is also still pending.) + final pendingTimers = elapse == Duration.zero ? 1 : 2; + check(async.pendingTimers).length.equals(pendingTimers); updateMachine.debugAdvanceLoop(); async.flushMicrotasks(); check(connection.lastRequest).isNull(); - check(async.pendingTimers).length.equals(1); + check(async.pendingTimers).length.equals(pendingTimers); // Polling continues after a timer. connection.prepare(json: GetEventsResult(events: [ @@ -1014,6 +1031,15 @@ void main() { checkRetry(prepareNetworkExceptionConnectionFailed); }); + test('retries when request outlives the server-recommended timeout', () { + // In the wild, this is a connection that died without erroring; see #514. + checkRetry( + eventQueueLongpollTimeoutSeconds: 90, + elapse: const Duration(seconds: 90), + () => connection.prepare(delay: const Duration(seconds: 300), + json: GetEventsResult(events: [], queueId: null).toJson())); + }); + test('retries on generic NetworkException', () { checkRetry(prepareNetworkException); });