fix: make ride finish idempotent and contain stop errors

This commit is contained in:
Developer 2026-09-10 04:33:41 -04:00
parent bd5a70e862
commit 6161233b2d
5 changed files with 634 additions and 2 deletions

View File

@ -345,7 +345,18 @@ class EvolutionPortraitDashboard extends ConsumerWidget {
child: ElevatedButton(
onPressed: () {
if (session.state == RideState.riding) {
ref.read(rideStateMachineProvider.notifier).finishRide();
// RID-2: contain the fire-and-forget Stop action at the UI
// boundary. finishRide() keeps its Future error semantics
// for awaiting callers; only the exception class is logged,
// never its payload. Bounded to one line per tap.
ref
.read(rideStateMachineProvider.notifier)
.finishRide()
.catchError((Object error) {
debugPrint(
'[RideStop] finish failed class=${error.runtimeType}',
);
});
} else {
ref.read(rideStateMachineProvider.notifier).startRide();
}

View File

@ -55,7 +55,19 @@ class RideActionButton extends ConsumerWidget {
onPressed: () {
final notifier = ref.read(rideStateMachineProvider.notifier);
if (isRiding) {
notifier.finishRide();
// RID-2: the Stop action is fire-and-forget. finishRide() retains
// its Future error semantics for callers that deliberately await
// it, so the failure is contained here at the UI boundary using the
// project's existing catchError convention
// (create_group_ride_screen.dart) and its runtimeType-only
// diagnostic convention (account_screen.dart, sync_observer.dart).
// Only the exception class is logged - never its payload - so
// tokens and ride content cannot leak, and output is bounded to one
// line per tap. No success is fabricated, no error silently
// swallowed.
notifier.finishRide().catchError((Object error) {
debugPrint('[RideStop] finish failed class=${error.runtimeType}');
});
} else {
notifier.startRide();
}

View File

@ -129,6 +129,15 @@ class RideStateMachine extends StateNotifier<RideSession> {
// would re-open the duplicate-start race.
bool _isStarting = false;
// RID-2: re-entrancy guard for ride finish. finishRide() previously left
// state.state == riding across every await (only healthStatus changed), so
// two rapid Stop taps both entered finalization and duplicated db.endRide,
// personal-record evaluation, logger.stopSession, the field-test summary row
// and the "Session ended" announcement. Raised synchronously before the
// first await; lifetime is owned exclusively by the in-flight finish
// operation (try/finally) and it is intentionally NOT cleared anywhere else.
bool _isFinishing = false;
RideStateMachine(this.db, this.telemetryService, this.recordsService, this.audioAlertService, this.ref) : super(RideSession()) {
_telemetrySub = telemetryService.telemetryStream.listen(_onTelemetryUpdate);
@ -337,7 +346,34 @@ class RideStateMachine extends StateNotifier<RideSession> {
} catch (_) {}
}
/// RID-2: idempotent ride finish.
///
/// The guard is raised synchronously, before the first `await`, so concurrent
/// Stop taps collapse into a single finalization instead of each persisting
/// their own end-of-ride write and forensic summary. A finish after a
/// successful completion is a no-op.
///
/// The guard is released in `finally`, so a persistence failure never leaves
/// the machine permanently unable to finalize and a later explicit retry
/// still succeeds. Failure semantics are deliberately unchanged: a failed
/// `db.endRide` still propagates to callers that await this method, still
/// leaves the session in its existing recoverable riding/finalizing state,
/// and still does not fabricate an `endTime`, emit a finished transition, or
/// enqueue an unpersisted ride for cloud sync.
Future<void> finishRide() async {
if (_isFinishing) return;
if (state.state == RideState.finished) return;
_isFinishing = true;
try {
await _performFinishRide();
} finally {
_isFinishing = false;
}
}
/// Performs the actual ride finalization. Guarded exclusively by
/// [finishRide] and must never be invoked directly.
Future<void> _performFinishRide() async {
if (state.dbId != null) {
state = state.copyWith(healthStatus: RecordingHealthStatus.recordingFinalizing);

View File

@ -0,0 +1,426 @@
// RID-2 regression coverage: idempotent ride finish.
//
// Converted from the deterministic RID-2 forensic reproduction. The race window
// is controlled with a Completer-gated database rather than sleeps, so no test
// here is timing-fragile.
//
// All seams are test-only subclasses injected through existing production
// constructors (AppDatabase.forTesting, recordsServiceProvider,
// fieldTestLoggerProvider). No production architecture was changed to make
// these tests possible.
import 'dart:async';
import 'package:flutter_test/flutter_test.dart';
import 'package:flutter_riverpod/flutter_riverpod.dart';
import 'package:geolocator/geolocator.dart';
import 'package:shared_preferences/shared_preferences.dart';
import 'package:drift/native.dart';
import 'package:motorcycle_dash/database/database.dart'
hide TelemetrySample, FieldTestSummary, FieldTestEvent;
import 'package:motorcycle_dash/models/telemetry_sample.dart';
import 'package:motorcycle_dash/models/field_test_summary.dart';
import 'package:motorcycle_dash/models/recording_health_status.dart';
import 'package:motorcycle_dash/services/telemetry_service.dart';
import 'package:motorcycle_dash/services/location_service.dart';
import 'package:motorcycle_dash/services/sensor_fusion_service.dart';
import 'package:motorcycle_dash/services/compass_service.dart';
import 'package:motorcycle_dash/services/audio_alert_service.dart';
import 'package:motorcycle_dash/services/field_test/field_test_logger.dart';
import 'package:motorcycle_dash/services/validation/telemetry_validator.dart';
import 'package:motorcycle_dash/features/rides/ride_state_machine.dart';
import 'package:motorcycle_dash/features/records/records_service.dart';
/// Counts and gates db.endRide - the first instrumentable awaited dependency
/// inside finishRide(). (_flushBatch() precedes it but is private and not
/// overridable without a production change; it is RID-3 scope and is
/// deliberately not measured here.)
class _Rid2AppDatabase extends AppDatabase {
_Rid2AppDatabase() : super.forTesting(NativeDatabase.memory());
int endRideInvocations = 0;
Completer<void>? endRideGate;
int endRideFailuresRemaining = 0;
final List<int> endRideTargetIds = [];
@override
Future<void> endRide(int rideId, RidesCompanion data) async {
endRideInvocations++;
endRideTargetIds.add(rideId);
if (endRideFailuresRemaining > 0) {
endRideFailuresRemaining--;
throw StateError('RID-2 simulated endRide failure');
}
final g = endRideGate;
if (g != null) await g.future;
return super.endRide(rideId, data);
}
}
/// Counts personal-record evaluation passes and record award callbacks.
class _CountingRecordsService extends RecordsService {
_CountingRecordsService(super.prefs);
int checkInvocations = 0;
int newRecordCallbacks = 0;
@override
Future<void> checkAndSaveRecords(
PersonalRecords newStats,
void Function(String, double)? onNewRecord, {
double confidence = 1.0,
}) async {
checkInvocations++;
return super.checkAndSaveRecords(
newStats,
onNewRecord == null
? null
: (String name, double value) {
newRecordCallbacks++;
onNewRecord(name, value);
},
confidence: confidence,
);
}
}
/// Counts stopSession entries. The isSessionActive check lives in the caller,
/// so concurrent entries are observable here.
class _CountingFieldTestLogger extends FieldTestLogger {
_CountingFieldTestLogger(super.db);
int stopSessionInvocations = 0;
@override
Future<FieldTestSummary> stopSession() async {
stopSessionInvocations++;
return super.stopSession();
}
}
class MockAudioAlertService extends Fake implements AudioAlertService {
final List<String> spokenMessages = [];
@override
Future<void> speak(String text, {bool force = false}) async {
spokenMessages.add(text);
}
}
class FakeTelemetryService extends TelemetryService {
final StreamController<TelemetrySample> _controller =
StreamController<TelemetrySample>.broadcast();
FakeTelemetryService(
super.loc,
super.fusion,
super.compass,
super.logger,
super.validator,
);
@override
Stream<TelemetrySample> get telemetryStream => _controller.stream;
void emitSample(TelemetrySample sample) => _controller.add(sample);
@override
void dispose() => _controller.close();
}
class _NullLocationService extends Fake implements LocationService {
@override
void Function(Position position, bool accepted, String? rejectionReason)?
onGpsDiagnostic;
@override
Stream<Position> getFilteredPositionStream() => const Stream.empty();
}
class _NullSensorFusionService extends Fake implements SensorFusionService {
@override
Stream<double> get leanAngleStream => const Stream.empty();
@override
Stream<double> get pitchAngleStream => const Stream.empty();
@override
Stream<GForceData> get gForceStream => const Stream.empty();
@override
double get latestRollRateDegPerSec => 0.0;
@override
double get latestRawAccelX => 0.0;
@override
double get latestRawAccelY => 0.0;
@override
double get latestRawAccelZ => 0.0;
@override
double get latestRawGyroX => 0.0;
@override
double get latestRawGyroY => 0.0;
@override
double get latestRawGyroZ => 0.0;
}
class _NullCompassService extends Fake implements CompassService {
@override
Stream<double> get headingStream => const Stream.empty();
}
void main() {
TestWidgetsFlutterBinding.ensureInitialized();
late _Rid2AppDatabase db;
late _CountingFieldTestLogger logger;
late _CountingRecordsService records;
late FakeTelemetryService fakeTelemetry;
late MockAudioAlertService mockAudio;
late TelemetryValidator validator;
late SharedPreferences prefs;
/// Counts observed transitions into RideState.finished. This is the edge that
/// drives SyncObserver, so it also proves no duplicate cloud enqueue.
late int finishedTransitions;
setUp(() async {
SharedPreferences.setMockInitialValues({});
prefs = await SharedPreferences.getInstance();
db = _Rid2AppDatabase();
logger = _CountingFieldTestLogger(db);
records = _CountingRecordsService(prefs);
validator = TelemetryValidator();
fakeTelemetry = FakeTelemetryService(
_NullLocationService(),
_NullSensorFusionService(),
_NullCompassService(),
logger,
validator,
);
mockAudio = MockAudioAlertService();
finishedTransitions = 0;
});
tearDown(() async {
// Never leave a parked gate behind or the suite would hang.
final g = db.endRideGate;
if (g != null && !g.isCompleted) g.complete();
db.endRideGate = null;
fakeTelemetry.dispose();
logger.dispose();
await db.close();
});
ProviderContainer buildContainer() {
final container = ProviderContainer(
overrides: [
sharedPreferencesProvider.overrideWithValue(prefs),
databaseProvider.overrideWithValue(db),
telemetryServiceProvider.overrideWithValue(fakeTelemetry),
audioAlertServiceProvider.overrideWithValue(mockAudio),
fieldTestLoggerProvider.overrideWithValue(logger),
recordsServiceProvider.overrideWith((ref) => records),
],
);
addTearDown(container.dispose);
container.listen<RideSession>(rideStateMachineProvider, (prev, next) {
if (prev?.state != RideState.finished &&
next.state == RideState.finished) {
finishedTransitions++;
}
});
return container;
}
/// Starts a real ride and feeds it deterministic moving telemetry so the
/// finish path has genuine stats, PR candidates and an active field-test
/// session to finalize.
Future<int> startRideWithTelemetry(ProviderContainer container) async {
final notifier = container.read(rideStateMachineProvider.notifier);
await notifier.startRide();
final rideId = container.read(rideStateMachineProvider).dbId!;
final base = DateTime(2026, 9, 10, 12);
for (var i = 0; i < 10; i++) {
fakeTelemetry.emitSample(
TelemetrySample(
timestamp: base.add(Duration(milliseconds: i * 50)),
latitude: 34.2570 + (i * 0.0005),
longitude: -85.1647,
speed: 45.0 + i,
heading: 90.0,
gpsAccuracy: 3.0,
leanAngle: 12.0,
longitudinalG: 0.2,
),
);
}
await pumpEventQueue(times: 30);
return rideId;
}
int sessionEndedCount() =>
mockAudio.spokenMessages.where((m) => m == 'Session ended').length;
String? cloudRideIdOf(List<Ride> rides, int rideId) =>
rides.singleWhere((r) => r.id == rideId).cloudRideId;
group('RID-2: idempotent ride finish', () {
test('CONTROL: a single finishRide() finalizes exactly once', () async {
final container = buildContainer();
final notifier = container.read(rideStateMachineProvider.notifier);
final rideId = await startRideWithTelemetry(container);
final cloudRideIdBefore =
cloudRideIdOf(await db.select(db.rides).get(), rideId);
expect(container.read(rideStateMachineProvider).state, RideState.riding);
await notifier.finishRide();
await pumpEventQueue(times: 30);
final session = container.read(rideStateMachineProvider);
final rides = await db.select(db.rides).get();
final row = rides.singleWhere((r) => r.id == rideId);
final summaries = await db.select(db.fieldTestSummaries).get();
expect(db.endRideInvocations, 1);
expect(records.checkInvocations, 1);
expect(logger.stopSessionInvocations, 1);
expect(summaries, hasLength(1));
expect(sessionEndedCount(), 1);
expect(finishedTransitions, 1);
expect(session.state, RideState.finished);
expect(session.healthStatus, RecordingHealthStatus.recordingComplete);
expect(row.endTime, isNotNull, reason: 'ride must be closed');
expect(cloudRideIdOf(rides, rideId), cloudRideIdBefore,
reason: 'cloudRideId is an immutable identity contract');
expect(rides, hasLength(1));
});
test('two concurrent finishRide() calls finalize exactly once', () async {
final container = buildContainer();
final notifier = container.read(rideStateMachineProvider.notifier);
final rideId = await startRideWithTelemetry(container);
final cloudRideIdBefore =
cloudRideIdOf(await db.select(db.rides).get(), rideId);
// Hold the first finish in-flight at its first instrumentable awaited
// dependency, then issue a second finishRide() - exactly what two rapid
// Stop taps do. Deterministic: no sleeps, no machine timing.
final gate = Completer<void>();
db.endRideGate = gate;
final first = notifier.finishRide();
final second = notifier.finishRide();
await pumpEventQueue(times: 40);
expect(db.endRideInvocations, 1,
reason: 'second caller must not enter while first is in flight');
gate.complete();
await Future.wait([first, second]);
await pumpEventQueue(times: 40);
final session = container.read(rideStateMachineProvider);
final rides = await db.select(db.rides).get();
final row = rides.singleWhere((r) => r.id == rideId);
final summaries = await db.select(db.fieldTestSummaries).get();
expect(db.endRideInvocations, 1, reason: 'db.endRide must run once');
expect(db.endRideTargetIds, [rideId]);
expect(records.checkInvocations, 1, reason: 'one records pass');
expect(logger.stopSessionInvocations, 1, reason: 'one logger stop');
expect(summaries, hasLength(1), reason: 'one fieldTestSummary per ride');
expect(summaries.single.rideId, rideId);
expect(sessionEndedCount(), 1, reason: 'one "Session ended"');
expect(finishedTransitions, 1,
reason: 'one finished transition -> one SyncObserver notify');
expect(rides, hasLength(1), reason: 'no duplicate ride row');
expect(row.endTime, isNotNull);
expect(cloudRideIdOf(rides, rideId), cloudRideIdBefore,
reason: 'no new cloudRideId minted');
expect(session.state, RideState.finished);
expect(session.dbId, rideId);
});
test('a second finishRide() after successful completion is a no-op',
() async {
final container = buildContainer();
final notifier = container.read(rideStateMachineProvider.notifier);
final rideId = await startRideWithTelemetry(container);
await notifier.finishRide();
await pumpEventQueue(times: 30);
expect(await db.select(db.fieldTestSummaries).get(), hasLength(1));
// Sequential second finish - a late second Stop tap arriving after the
// first has already completed.
await notifier.finishRide();
await pumpEventQueue(times: 30);
expect(db.endRideInvocations, 1, reason: 'no second db.endRide');
expect(records.checkInvocations, 1, reason: 'no second records pass');
expect(logger.stopSessionInvocations, 1, reason: 'no second logger stop');
expect(await db.select(db.fieldTestSummaries).get(), hasLength(1),
reason: 'no second fieldTestSummary');
expect(sessionEndedCount(), 1, reason: 'no second "Session ended"');
expect(finishedTransitions, 1, reason: 'no second finished transition');
expect(container.read(rideStateMachineProvider).state,
RideState.finished);
expect(await db.select(db.rides).get(), hasLength(1));
expect(db.endRideTargetIds, [rideId]);
});
test('failed db.endRide does not falsely finish, releases the guard, '
'and a retry finalizes exactly once', () async {
final container = buildContainer();
final notifier = container.read(rideStateMachineProvider.notifier);
final rideId = await startRideWithTelemetry(container);
final cloudRideIdBefore =
cloudRideIdOf(await db.select(db.rides).get(), rideId);
db.endRideFailuresRemaining = 1;
// finishRide retains meaningful Future error semantics for callers that
// deliberately await it. Containment happens at the UI boundary instead.
await expectLater(notifier.finishRide(), throwsStateError);
await pumpEventQueue(times: 30);
final afterFailure = container.read(rideStateMachineProvider);
final rowAfterFailure = (await db.select(db.rides).get())
.singleWhere((r) => r.id == rideId);
expect(afterFailure.state, isNot(RideState.finished),
reason: 'must not falsely report finished after persistence failure');
expect(rowAfterFailure.endTime, isNull,
reason: 'must not fabricate an endTime for an unpersisted finish');
expect(finishedTransitions, 0,
reason: 'no finished transition -> no false cloud sync completion');
expect(records.checkInvocations, 0);
expect(logger.stopSessionInvocations, 0);
expect(logger.isSessionActive, isTrue,
reason: 'field-test session must stay recoverable for the retry');
expect(await db.select(db.fieldTestSummaries).get(), isEmpty);
expect(sessionEndedCount(), 0);
// The guard must have been released by the finally block, so a legitimate
// explicit retry still succeeds.
await notifier.finishRide();
await pumpEventQueue(times: 30);
final afterRetry = container.read(rideStateMachineProvider);
final rides = await db.select(db.rides).get();
final rowAfterRetry = rides.singleWhere((r) => r.id == rideId);
final summaries = await db.select(db.fieldTestSummaries).get();
expect(db.endRideInvocations, 2,
reason: '1 injected failure + 1 successful retry is expected');
expect(afterRetry.state, RideState.finished);
expect(afterRetry.healthStatus, RecordingHealthStatus.recordingComplete);
expect(rowAfterRetry.endTime, isNotNull,
reason: 'exactly one successful persisted finalization');
expect(records.checkInvocations, 1, reason: 'exactly one records pass');
expect(logger.stopSessionInvocations, 1, reason: 'exactly one logger stop');
expect(summaries, hasLength(1), reason: 'exactly one fieldTestSummary');
expect(sessionEndedCount(), 1, reason: 'exactly one "Session ended"');
expect(finishedTransitions, 1, reason: 'exactly one finished transition');
expect(rides, hasLength(1), reason: 'no duplicate ride row');
expect(cloudRideIdOf(rides, rideId), cloudRideIdBefore,
reason: 'no new cloudRideId minted');
});
});
}

View File

@ -0,0 +1,147 @@
// RID-2 regression coverage: UI containment of the fire-and-forget Stop action.
//
// Both production Stop call sites (ride_action_button.dart and
// evolution_portrait_dashboard.dart) invoke finishRide() without awaiting it.
// finishRide() deliberately retains its Future error semantics for callers that
// do await it, so containment belongs at the UI boundary via the project's
// existing catchError + runtimeType-only diagnostic convention.
//
// flutter_test fails a test on any unhandled async error, so the rejection test
// below is a genuine regression: it fails if the catchError containment is
// removed from the button.
import 'package:flutter/material.dart';
import 'package:flutter_riverpod/flutter_riverpod.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:motorcycle_dash/features/dashboard/widgets/ride_action_button.dart';
import 'package:motorcycle_dash/features/rides/ride_state_machine.dart';
import 'package:motorcycle_dash/models/recording_health_status.dart';
class _FailingFinishStateMachine extends StateNotifier<RideSession>
implements RideStateMachine {
_FailingFinishStateMachine(super.initial);
/// Payload that must never reach diagnostics.
static const String secretPayload = 'RID-2-SENSITIVE-TOKEN-do-not-log';
int finishCalls = 0;
@override
Future<void> finishRide() async {
finishCalls++;
throw StateError(secretPayload);
}
@override
dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation);
}
class _SucceedingFinishStateMachine extends StateNotifier<RideSession>
implements RideStateMachine {
_SucceedingFinishStateMachine(super.initial);
int finishCalls = 0;
@override
Future<void> finishRide() async {
finishCalls++;
state = state.copyWith(
state: RideState.finished,
healthStatus: RecordingHealthStatus.recordingComplete,
);
}
@override
dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation);
}
void main() {
TestWidgetsFlutterBinding.ensureInitialized();
RideSession ridingSession() => RideSession(
dbId: 1,
state: RideState.riding,
healthStatus: RecordingHealthStatus.recordingActive,
);
Widget wrap(Override override) => ProviderScope(
overrides: [override],
child: const MaterialApp(home: Scaffold(body: RideActionButton())),
);
// debugPrint must be restored before the test body returns: flutter_test
// asserts that foundation debug variables are unset at the end of the body,
// which happens before tearDown callbacks run.
Future<void> captureDebugPrint(
List<String> sink,
Future<void> Function() body,
) async {
final original = debugPrint;
debugPrint = (String? message, {int? wrapWidth}) {
if (message != null) sink.add(message);
};
try {
await body();
} finally {
debugPrint = original;
}
}
testWidgets('a rejected finishRide Future is contained at the Stop button',
(tester) async {
final logLines = <String>[];
final fake = _FailingFinishStateMachine(ridingSession());
await captureDebugPrint(logLines, () async {
await tester.pumpWidget(
wrap(rideStateMachineProvider.overrideWith((ref) => fake)),
);
await tester.pumpAndSettle();
expect(find.text('Stop Session'), findsOneWidget);
await tester.tap(find.text('Stop Session'));
await tester.pumpAndSettle();
// Had the rejected Future been left unhandled, flutter_test would have
// surfaced it as an unhandled async error and failed this test.
expect(fake.finishCalls, 1);
final rideStopLines =
logLines.where((l) => l.contains('[RideStop]')).toList();
expect(rideStopLines, hasLength(1),
reason: 'bounded diagnostic: one line per Stop tap');
expect(rideStopLines.single, contains('StateError'),
reason: 'the exception class must be reported, not swallowed');
expect(logLines.join('\n'), isNot(contains(
_FailingFinishStateMachine.secretPayload)),
reason: 'exception payload/secrets must never be printed');
// No false successful completion is created by the containment.
expect(fake.state.state, RideState.riding);
expect(find.text('Stop Session'), findsOneWidget);
expect(find.text('Start Session'), findsNothing);
});
});
testWidgets('containment does not break the successful Stop path',
(tester) async {
final logLines = <String>[];
final fake = _SucceedingFinishStateMachine(ridingSession());
await captureDebugPrint(logLines, () async {
await tester.pumpWidget(
wrap(rideStateMachineProvider.overrideWith((ref) => fake)),
);
await tester.pumpAndSettle();
await tester.tap(find.text('Stop Session'));
await tester.pumpAndSettle();
expect(fake.finishCalls, 1);
expect(logLines.where((l) => l.contains('[RideStop]')), isEmpty,
reason: 'no diagnostic on a successful finish');
expect(fake.state.state, RideState.finished);
expect(find.text('Start Session'), findsOneWidget);
expect(find.text('Stop Session'), findsNothing);
});
});
}