From 955593787344dc5622a77b4ef5e9e6a65f04321b Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Fri, 31 Jul 2026 07:37:20 +0200 Subject: [PATCH] fix(startup): defer crash reports until the reporter exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reporting the failure inline was wrong for the phase that matters most. The gate opens preferences before SentryFlutter.init, so a corrupt or unreadable store — the likeliest cause of #1732 — was captured by a NoOpHub and silently discarded, which is exactly the telemetry gap the previous commit claimed to close. Initialising the reporter earlier is not an option either: `_beforeSend` reads the crash-reporting opt-out from settings, so events raised before settings load would bypass a user's choice. Every failure is now persisted first and flushed once the reporter is up with settings loaded, which in practice is the user's own retry seconds later in the same process. Records carry a `reported` flag so a send happens exactly once, and a failed send leaves the flag clear so the next launch tries again. The flush reads without consuming, so the record still reaches Settings > Logs. Also routes the tvOS recovery marker through the tolerant read: `reconcile()` runs inside `AppDatabase.open`, a fatal gate step, so a wrong-typed marker vetoed startup outright on a first-class TV target despite the new default-instead-of-veto behaviour. An unreadable marker tells us nothing, which is the same position as an absent one. --- .../tvos_database_recovery_store.dart | 10 ++- lib/main.dart | 70 +++++++++++++------ lib/services/startup_diagnostics.dart | 54 ++++++++++++++ test/services/startup_diagnostics_test.dart | 46 ++++++++++++ test/startup_bootstrap_test.dart | 19 +++-- 5 files changed, 165 insertions(+), 34 deletions(-) diff --git a/lib/database/tvos_database_recovery_store.dart b/lib/database/tvos_database_recovery_store.dart index 7e9affbf..73ede9a5 100644 --- a/lib/database/tvos_database_recovery_store.dart +++ b/lib/database/tvos_database_recovery_store.dart @@ -5,6 +5,7 @@ import 'package:flutter/foundation.dart'; import 'package:shared_preferences/shared_preferences.dart'; import '../utils/app_logger.dart'; +import '../services/base_shared_preferences_service.dart'; /// Startup result after reconciling the purgeable tvOS database with its /// bounded standard-domain recovery image. @@ -98,7 +99,14 @@ final class TvosDatabaseRecoveryStore { }) async { if (!isTvos) return TvosDatabaseRecoveryOutcome.notApplicable; - final recoveryRequired = _preferences.getBool(recoveryRequiredKey) ?? false; + // Tolerant read: `reconcile` runs inside `AppDatabase.open`, a fatal + // startup step, so a wrong-typed marker would veto the launch outright on + // a first-class TV target. An unreadable marker tells us nothing, which is + // the same position as an absent one — default false and drop the key + // (#1732). The manifest reads below are already inside catch-alls. + final recoveryRequired = + readPreferenceTolerantly(_preferences, recoveryRequiredKey, () => _preferences.getBool(recoveryRequiredKey)) ?? + false; if (recoveryRequired) { await _reloadManifestCacheIfNeeded(); final snapshot = _readCommittedSnapshot(); diff --git a/lib/main.dart b/lib/main.dart index 711a3dec..036c3e96 100644 --- a/lib/main.dart +++ b/lib/main.dart @@ -244,6 +244,10 @@ Future<_StartupDependencies> _initializeApplication() async { options.beforeBreadcrumb = _beforeBreadcrumb; }); }); + // Settings are loaded and the reporter is up, so the crash-reporting + // opt-out in `_beforeSend` now applies. This is the first moment a failure + // from an earlier attempt can actually be sent. + unawaited(flushPendingStartupFailure()); } AndroidExitDiagnostics.markTelemetryReady(); @@ -310,24 +314,51 @@ StartupFailureRecord describeStartupFailure(Object error, StackTrace stackTrace) repairable: _isRepairable(error), ); -/// Default [StartupBootstrap.reportFailure]. +/// Sends a persisted startup failure to the crash reporter, once. +/// +/// Reporting cannot happen where the failure is caught. The gate opens +/// preferences — the likeliest thing to fail, and the whole reason #1732 has +/// no telemetry — before crash reporting exists, so a `captureException` +/// there goes to a no-op hub and is silently dropped. Initialising the +/// reporter first is not an option either: `_beforeSend` reads the +/// crash-reporting opt-out from settings, which are not loaded yet, so early +/// events would bypass a user's choice. +/// +/// So every failure is persisted first and flushed here, immediately after +/// the reporter comes up with settings loaded. In practice that is the user's +/// own retry, seconds later, in the same process. @visibleForTesting -Future reportStartupFailure(StartupFailureRecord record, Object error, StackTrace stackTrace) async { - if (!_enableSentry) return; +Future flushPendingStartupFailure({Future Function(StartupFailureRecord record)? send}) async { + final record = await StartupDiagnosticsStore.peekPersisted(); + if (record == null || record.reported) return; try { - await Sentry.captureException( - error, - stackTrace: stackTrace, - withScope: (scope) { - scope.setTag('startup.phase', record.phaseId); - scope.level = SentryLevel.fatal; - }, - ); - } catch (reportError, reportStack) { - appLogger.d('Could not report the startup failure', error: reportError, stackTrace: reportStack); + await (send ?? _captureStartupFailure)(record); + await StartupDiagnosticsStore.markReported(record); + } catch (error, stackTrace) { + // Leave it unreported so the next launch tries again. + appLogger.d('Could not report the startup failure', error: error, stackTrace: stackTrace); } } +Future _captureStartupFailure(StartupFailureRecord record) async { + // The original error object is long gone by now; the record is the + // allowlisted, already-redacted rendering of it. + await Sentry.captureMessage( + 'Startup failed: ${record.headline}', + level: SentryLevel.fatal, + withScope: (scope) { + scope.setTag('startup.phase', record.phaseId); + scope.setContexts('startup', { + 'phase': record.phaseId, + 'errorType': record.errorType, + 'repairable': record.repairable, + 'when': record.timestamp.toUtc().toIso8601String(), + 'stackTrace': ?record.stackTrace, + }); + }, + ); +} + /// Default [StartupBootstrap.repair]: states the cost, runs the repair that /// matches the failure, then reports what was kept and what was lost. /// @@ -440,7 +471,6 @@ class StartupBootstrap extends StatefulWidget { this.discard, this.onCommitted, this.describeFailure = describeStartupFailure, - this.reportFailure = reportStartupFailure, this.repair = repairStartupStorage, this.lightTheme, this.darkTheme, @@ -456,10 +486,6 @@ class StartupBootstrap extends StatefulWidget { /// the persisted diagnostic and the crash report all share. final StartupFailureRecord Function(Object error, StackTrace stackTrace) describeFailure; - /// Sends the failure to the crash reporter. The gate catches the error, so - /// nothing else will. - final Future Function(StartupFailureRecord record, Object error, StackTrace stackTrace) reportFailure; - /// Offers the user a consented repair for a recoverable failure and reports /// whether one ran. Returning true re-runs [initialize]. final Future Function(BuildContext context, StartupFailureRecord record, Object error)? repair; @@ -530,10 +556,10 @@ class _StartupBootstrapState extends State> { // process. On Windows there is no log file and no console for a // double-clicked release build. unawaited(StartupDiagnosticsStore.record(failure)); - // The gate catches this error, so it is a *handled* Dart error that - // never reaches PlatformDispatcher.onError. Report it explicitly or the - // crash dashboard stays empty for the one failure that hides the app. - unawaited(widget.reportFailure(failure, error, stackTrace)); + // Not reported from here: the earliest phases run before the crash + // reporter exists, so the record is persisted and flushed by + // `flushPendingStartupFailure` once it is up — on the retry below, or on + // the next launch. if (!mounted || generation != _generation) return; setState(() { _failure = failure; diff --git a/lib/services/startup_diagnostics.dart b/lib/services/startup_diagnostics.dart index 4e56ddf8..a83b81fc 100644 --- a/lib/services/startup_diagnostics.dart +++ b/lib/services/startup_diagnostics.dart @@ -80,6 +80,7 @@ class StartupFailureRecord { required this.appVersion, required this.platform, this.repairable = false, + this.reported = false, }) : message = LogRedactionManager.redact(message), stackTrace = stackTrace == null ? null : LogRedactionManager.redact(stackTrace); @@ -144,6 +145,26 @@ class StartupFailureRecord { /// Whether the gate can offer an in-app repair for this failure. final bool repairable; + /// Whether this record has already reached the crash reporter. + /// + /// The earliest gate phases run before crash reporting is initialised, so a + /// failure there is captured by a no-op hub and silently discarded. Records + /// are therefore always persisted first and sent once the reporter is up — + /// on the in-app retry, or on the next launch (#1732). + final bool reported; + + StartupFailureRecord copyWith({bool? reported}) => StartupFailureRecord( + phase: phase, + errorType: errorType, + message: message, + stackTrace: stackTrace, + timestamp: timestamp, + appVersion: appVersion, + platform: platform, + repairable: repairable, + reported: reported ?? this.reported, + ); + String get phaseId => phase?.id ?? 'unknown'; /// One-line summary for the failure screen and the log. @@ -177,6 +198,7 @@ class StartupFailureRecord { 'appVersion': appVersion, 'platform': platform, 'repairable': repairable, + 'reported': reported, }; static StartupFailureRecord? fromJson(Map json) { @@ -193,6 +215,7 @@ class StartupFailureRecord { appVersion: json['appVersion'] as String? ?? 'unknown', platform: json['platform'] as String? ?? 'unknown', repairable: json['repairable'] as bool? ?? false, + reported: json['reported'] as bool? ?? false, ); } } @@ -243,6 +266,37 @@ abstract final class StartupDiagnosticsStore { } } + /// Reads a persisted record without consuming it. + /// + /// Used by the crash-report flush, which has to run before the record is + /// consumed for display and must not remove it if the send fails. + static Future peekPersisted() async { + try { + final file = await _file(); + if (file == null || !await file.exists()) return null; + final decoded = jsonDecode(await file.readAsString()); + if (decoded is! Map) return null; + return StartupFailureRecord.fromJson(decoded.cast()); + } catch (error, stackTrace) { + appLogger.d('Could not peek at the startup failure record', error: error, stackTrace: stackTrace); + return null; + } + } + + /// Rewrites the persisted record as already reported, so a later launch does + /// not send it a second time. It stays on disk for Settings > Logs. + static Future markReported(StartupFailureRecord failure) async { + final updated = failure.copyWith(reported: true); + if (_pending != null) _pending = updated; + try { + final file = await _file(); + if (file == null || !await file.exists()) return; + await file.writeAsString(jsonEncode(updated.toJson()), flush: true); + } catch (error, stackTrace) { + appLogger.d('Could not mark the startup failure record as reported', error: error, stackTrace: stackTrace); + } + } + /// Reads and deletes a record written by an earlier launch. /// /// Deleting on read stops one stale failure from following the user forever; diff --git a/test/services/startup_diagnostics_test.dart b/test/services/startup_diagnostics_test.dart index 434260cc..15383c64 100644 --- a/test/services/startup_diagnostics_test.dart +++ b/test/services/startup_diagnostics_test.dart @@ -2,6 +2,7 @@ import 'dart:convert'; import 'dart:io'; import 'package:flutter_test/flutter_test.dart'; +import 'package:plezy/main.dart'; import 'package:plezy/services/sensitive_prefs.dart'; import 'package:plezy/services/startup_diagnostics.dart'; import 'package:plezy/utils/log_redaction_manager.dart'; @@ -189,4 +190,49 @@ void main() { expect(StartupPhase.fromId(null), isNull); }); }); + + group('deferred crash reporting', () { + // The preferences phase runs before crash reporting is initialised, so an + // inline capture goes to a no-op hub and is silently discarded — the + // likeliest failure phase producing no telemetry at all (#1732). + + test('flushes a persisted record once and marks it reported', () async { + await StartupDiagnosticsStore.record(_record(phase: StartupPhase.preferences)); + + final sent = []; + await flushPendingStartupFailure(send: (record) async => sent.add(record)); + + expect(sent.single.phase, StartupPhase.preferences); + expect((await StartupDiagnosticsStore.peekPersisted())!.reported, isTrue); + + // A later launch must not resend it. + await flushPendingStartupFailure(send: (record) async => sent.add(record)); + expect(sent, hasLength(1)); + }); + + test('leaves the record unreported when the send fails', () async { + await StartupDiagnosticsStore.record(_record()); + + await flushPendingStartupFailure(send: (_) async => throw StateError('offline')); + + // Still pending, so the next launch retries rather than losing it. + expect((await StartupDiagnosticsStore.peekPersisted())!.reported, isFalse); + }); + + test('does nothing when no launch has failed', () async { + final sent = []; + + await flushPendingStartupFailure(send: (record) async => sent.add(record)); + + expect(sent, isEmpty); + }); + + test('peeking leaves the record on disk for the display path', () async { + await StartupDiagnosticsStore.record(_record()); + + await StartupDiagnosticsStore.peekPersisted(); + + expect(await StartupDiagnosticsStore.consumePrevious(), isNotNull); + }); + }); } diff --git a/test/startup_bootstrap_test.dart b/test/startup_bootstrap_test.dart index b29f0393..7100ffa2 100644 --- a/test/startup_bootstrap_test.dart +++ b/test/startup_bootstrap_test.dart @@ -97,7 +97,6 @@ void main() { StartupBootstrap( initialize: () async => throw const StartupPhaseException(StartupPhase.database, FormatException('boom')), buildApp: (_, value) => Text('ready $value'), - reportFailure: (_, _, _) async {}, ), ); await tester.pump(); @@ -115,7 +114,6 @@ void main() { StartupBootstrap( initialize: () async => throw StateError('database unavailable'), buildApp: (_, value) => Text('ready $value'), - reportFailure: (_, _, _) async {}, ), ); await tester.pump(); @@ -133,7 +131,6 @@ void main() { StartupBootstrap( initialize: () async => throw StateError('unrelated'), buildApp: (_, value) => Text('ready $value'), - reportFailure: (_, _, _) async {}, ), ); await tester.pump(); @@ -144,7 +141,6 @@ void main() { key: const Key('repairable'), initialize: () async => throw CorruptPreferenceStoreException(const FormatException('bad'), StackTrace.current), buildApp: (_, value) => Text('ready $value'), - reportFailure: (_, _, _) async {}, ), ); await tester.pump(); @@ -165,7 +161,6 @@ void main() { return 7; }, buildApp: (_, value) => MaterialApp(home: Text('ready $value')), - reportFailure: (_, _, _) async {}, repair: (_, _, _) async { repairCalls++; return true; @@ -193,7 +188,6 @@ void main() { throw CorruptPreferenceStoreException(const FormatException('bad'), StackTrace.current); }, buildApp: (_, value) => MaterialApp(home: Text('ready $value')), - reportFailure: (_, _, _) async {}, repair: (_, _, _) async => false, ), ); @@ -207,20 +201,23 @@ void main() { expect(find.byKey(startupBootstrapFailureKey), findsOneWidget); }); - testWidgets('reports the failure to the crash reporter', (tester) async { - StartupFailureRecord? reported; + testWidgets('persists the failure so it can be reported once the reporter is up', (tester) async { + // Not reported inline: the earliest gate phases run before crash + // reporting exists, so an inline capture would reach a no-op hub and be + // discarded. The record is held for `flushPendingStartupFailure` (#1732). + StartupDiagnosticsStore.resetForTesting(); + addTearDown(StartupDiagnosticsStore.resetForTesting); await tester.pumpWidget( StartupBootstrap( initialize: () async => throw const StartupPhaseException(StartupPhase.storage, 'nope'), buildApp: (_, value) => Text('ready $value'), - reportFailure: (record, _, _) async => reported = record, ), ); await tester.pump(); - // The gate catches the error, so nothing else would ever see it. - expect(reported?.phase, StartupPhase.storage); + expect(StartupDiagnosticsStore.pending?.phase, StartupPhase.storage); + expect(StartupDiagnosticsStore.pending?.reported, isFalse); }); testWidgets('retry clears the failed generation and can commit a later success', (tester) async {