mirror of
https://github.com/kuhyx/screen-locker.git
synced 2026-11-11 13:07:43 +01:00
Report a failed workout push instead of hiding it in debugPrint
Some checks are pending
pre-commit / pre-commit (push) Waiting to run
Tests / test (3.10) (push) Waiting to run
Tests / test (3.11) (push) Waiting to run
Tests / test (3.12) (push) Waiting to run
Tests / lock-popup-safety (push) Waiting to run
release-apk / release (APK) (push) Waiting to run
Some checks are pending
pre-commit / pre-commit (push) Waiting to run
Tests / test (3.10) (push) Waiting to run
Tests / test (3.11) (push) Waiting to run
Tests / test (3.12) (push) Waiting to run
Tests / lock-popup-safety (push) Waiting to run
release-apk / release (APK) (push) Waiting to run
A workout completed on 2026-08-09 never reached the sync repo and nothing
anywhere said why. The cause was structural: workout_screen.dart fired
`unawaited(WorkoutSyncService().push(session))` and `push()` caught ONLY
GitHubSyncError, reporting it via `debugPrint` -- which goes nowhere in a
release build. A Firebase error, an auth failure, a dropped connection or a
backgrounded app all produced exactly that symptom: silence.
This repo's CLAUDE.md already forbids it in as many words ("an invisible log
is a silent failure with extra steps", "return something meaningful"), so
this brings the Dart side up to the contract the Python side already meets
with `_manual_push.PushResult`.
- push()/pushManual() return a PushResult(pushed, reason) instead of void.
- The catch is deliberately broad, and logs at level 1000 (error) with the
stack trace via dart:developer, not debugPrint.
- Still never rethrows: a sync failure must not cost the user their finished
workout. The caller stays unawaited so a slow backend can't delay the
summary dialog, but it now logs when the push did not happen.
Tests assert the reason, not just that the future completes -- the old test
("a GitHubSyncError is swallowed, not rethrown") would have passed against
the broken behaviour.
Note this does NOT explain the 2026-08-09 miss retroactively: that session
ran at 12:35 UTC on the pre-migration build, ~3h before the new APK was
installed, so its failure is unrecoverable. It is in Firebase anyway via the
PC's independent verification (phone_verified:2026-08-09, "68 min, partial").
This makes the next one diagnosable.
314 tests, analyzer clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZH89WEqAjuJT8WZdqc7AP
This commit is contained in:
parent
2d331f1467
commit
4095e0ae5d
@ -3,6 +3,7 @@
|
||||
library;
|
||||
|
||||
import 'dart:async';
|
||||
import 'dart:developer';
|
||||
import 'package:audioplayers/audioplayers.dart';
|
||||
import 'package:flutter/material.dart';
|
||||
import 'package:vibration/vibration.dart';
|
||||
@ -464,7 +465,17 @@ class _WorkoutScreenState extends State<WorkoutScreen> {
|
||||
await storage.clearActiveSession();
|
||||
|
||||
final syncResult = await _sync.writeWorkoutResult(session);
|
||||
unawaited(WorkoutSyncService().push(session));
|
||||
// Not awaited: a slow or unreachable backend must not delay the summary
|
||||
// dialog. But the result is no longer discarded -- a failed push logs at
|
||||
// error level inside push(), so an unpushed workout is diagnosable
|
||||
// instead of silently absent from every other device.
|
||||
unawaited(
|
||||
WorkoutSyncService().push(session).then((result) {
|
||||
if (!result.pushed) {
|
||||
log('Workout not synced: ${result.reason}', level: 1000);
|
||||
}
|
||||
}),
|
||||
);
|
||||
|
||||
if (!mounted) return;
|
||||
unawaited(
|
||||
|
||||
@ -1,8 +1,9 @@
|
||||
/// Pushes completed workout sessions to the shared GitHub sync repo.
|
||||
/// Pushes completed workout sessions to the shared sync backend.
|
||||
library;
|
||||
|
||||
|
||||
import 'dart:convert';
|
||||
import 'dart:developer';
|
||||
import 'package:crdt_sync/crdt_sync.dart';
|
||||
import 'package:flutter/foundation.dart';
|
||||
import 'package:http/http.dart' as http;
|
||||
@ -23,11 +24,36 @@ Log _decode(String text) => (jsonDecode(text) as Map<String, dynamic>).map(
|
||||
(id, data) => MapEntry(id, Record.fromJson(data as Map<String, dynamic>)),
|
||||
);
|
||||
|
||||
/// The outcome of a [WorkoutSyncService.push] / [WorkoutSyncService.pushManual].
|
||||
///
|
||||
/// A bare `Future<void>` used to hide every failure: the caller could not
|
||||
/// tell "nothing to do" from "it broke", and the only trace was a
|
||||
/// `debugPrint` that goes nowhere in a release build. That is the exact
|
||||
/// silent-failure shape `CLAUDE.md` forbids, and it is why a workout could
|
||||
/// go unpushed with nothing anywhere saying so.
|
||||
@immutable
|
||||
class PushResult {
|
||||
/// Creates a result. [reason] should read like a sentence a human can act on.
|
||||
const PushResult({required this.pushed, required this.reason});
|
||||
|
||||
/// Whether the sync tick completed without error.
|
||||
final bool pushed;
|
||||
|
||||
/// Why it did or did not happen.
|
||||
final String reason;
|
||||
|
||||
@override
|
||||
String toString() => 'PushResult(pushed: $pushed, reason: $reason)';
|
||||
}
|
||||
|
||||
/// Mirrors the PC-side `_workout_sync.py`: GitHub is used purely as dumb
|
||||
/// file storage via the Contents API, one [Record] per completed session,
|
||||
/// merged onto whatever this phone has already pushed. A missing token or a
|
||||
/// failed push is swallowed here -- sync being unconfigured or unreachable
|
||||
/// must never crash or delay the workout-completion flow that calls [push].
|
||||
/// merged onto whatever this phone has already pushed.
|
||||
///
|
||||
/// A failed push never throws into the workout-completion flow -- sync being
|
||||
/// unreachable must not cost the user their session -- but it is never
|
||||
/// silent either: every path returns a [PushResult] whose `reason` says what
|
||||
/// happened, and failures log at error level.
|
||||
class WorkoutSyncService {
|
||||
/// Creates a [WorkoutSyncService]. [owner]/[repo]/[httpClient] default to
|
||||
/// the real `syncs` repo (`screen-locker-sync/` subdirectory) and a fresh
|
||||
@ -74,15 +100,22 @@ class WorkoutSyncService {
|
||||
final String repo;
|
||||
final http.Client? _httpClient;
|
||||
|
||||
/// Pushes [session] to `devices/phone/log.json`, merging with whatever
|
||||
/// this device has already pushed. No-ops silently if sync isn't
|
||||
/// configured; logs (but does not rethrow) any [GitHubSyncError].
|
||||
Future<void> push(WorkoutSession session) async {
|
||||
/// Pushes [session] to this device's log, merging with whatever it has
|
||||
/// already pushed.
|
||||
///
|
||||
/// Never throws: returns a [PushResult] saying whether the tick happened
|
||||
/// and why. Failures are logged at error level so an unpushed workout
|
||||
/// leaves a trace rather than vanishing.
|
||||
Future<PushResult> push(WorkoutSession session) async {
|
||||
final settings = await SyncSettings.load();
|
||||
// Either backend counts. Gating on the GitHub token alone silently
|
||||
// dropped every push from a device connected only to Firebase -- and
|
||||
// once the mirror is retired that would be every device.
|
||||
if (!settings.isConfigured && !await _hasFirebaseAccount()) return;
|
||||
if (!settings.isConfigured && !await _hasFirebaseAccount()) {
|
||||
const reason = 'sync not configured (no Firebase account, no token)';
|
||||
log('WorkoutSyncService.push skipped: $reason', level: 900);
|
||||
return const PushResult(pushed: false, reason: reason);
|
||||
}
|
||||
|
||||
final github = GitHubClient(
|
||||
owner: owner,
|
||||
@ -120,20 +153,37 @@ class WorkoutSyncService {
|
||||
// Firebase free tier's monthly budget depends on not happening.
|
||||
stateStore: await _openStateStore(),
|
||||
);
|
||||
} on GitHubSyncError catch (error) {
|
||||
debugPrint('WorkoutSyncService.push failed: $error');
|
||||
return const PushResult(pushed: true, reason: 'pushed');
|
||||
} on Object catch (error, stackTrace) {
|
||||
// Deliberately broad: a GitHubSyncError was the only case handled
|
||||
// before, so a Firebase error, an auth failure or a dropped connection
|
||||
// vanished with no trace at all. Still swallowed rather than rethrown
|
||||
// -- a sync failure must not cost the user their finished workout --
|
||||
// but now it is reported, not hidden.
|
||||
final reason = 'push failed: $error';
|
||||
log(
|
||||
'WorkoutSyncService.push failed',
|
||||
level: 1000,
|
||||
error: error,
|
||||
stackTrace: stackTrace,
|
||||
);
|
||||
return PushResult(pushed: false, reason: reason);
|
||||
} finally {
|
||||
github.close();
|
||||
firebase?.close();
|
||||
}
|
||||
}
|
||||
|
||||
/// Pushes a pre-built manual-workout [record] to `devices/phone/log.json`,
|
||||
/// merging with whatever this device has already pushed. Same swallow-on-
|
||||
/// failure contract as [push].
|
||||
Future<void> pushManual(Record record) async {
|
||||
/// Pushes a pre-built manual-workout [record] to this device's log.
|
||||
///
|
||||
/// Same never-throw, always-report contract as [push].
|
||||
Future<PushResult> pushManual(Record record) async {
|
||||
final settings = await SyncSettings.load();
|
||||
if (!settings.isConfigured && !await _hasFirebaseAccount()) return;
|
||||
if (!settings.isConfigured && !await _hasFirebaseAccount()) {
|
||||
const reason = 'sync not configured (no Firebase account, no token)';
|
||||
log('WorkoutSyncService.pushManual skipped: $reason', level: 900);
|
||||
return const PushResult(pushed: false, reason: reason);
|
||||
}
|
||||
|
||||
final github = GitHubClient(
|
||||
owner: owner,
|
||||
@ -165,8 +215,16 @@ class WorkoutSyncService {
|
||||
// Firebase free tier's monthly budget depends on not happening.
|
||||
stateStore: await _openStateStore(),
|
||||
);
|
||||
} on GitHubSyncError catch (error) {
|
||||
debugPrint('WorkoutSyncService.pushManual failed: $error');
|
||||
return const PushResult(pushed: true, reason: 'pushed');
|
||||
} on Object catch (error, stackTrace) {
|
||||
final reason = 'push failed: $error';
|
||||
log(
|
||||
'WorkoutSyncService.pushManual failed',
|
||||
level: 1000,
|
||||
error: error,
|
||||
stackTrace: stackTrace,
|
||||
);
|
||||
return PushResult(pushed: false, reason: reason);
|
||||
} finally {
|
||||
github.close();
|
||||
firebase?.close();
|
||||
|
||||
@ -16,8 +16,9 @@ class _FakeSync extends WorkoutSyncService {
|
||||
payloads;
|
||||
|
||||
@override
|
||||
Future<void> pushManual(Record record) async {
|
||||
Future<PushResult> pushManual(Record record) async {
|
||||
pushed = record;
|
||||
return const PushResult(pushed: true, reason: 'pushed');
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -104,9 +104,13 @@ void main() {
|
||||
installFakeSecureStorage();
|
||||
final (:httpClient, :putCalls) = _mockGitHub();
|
||||
|
||||
await _service(httpClient: httpClient).push(_session());
|
||||
final result = await _service(httpClient: httpClient).push(_session());
|
||||
|
||||
expect(putCalls, isEmpty);
|
||||
// Not just "nothing happened" -- the caller can tell WHY, so an unpushed
|
||||
// workout is diagnosable instead of silently absent everywhere else.
|
||||
expect(result.pushed, isFalse);
|
||||
expect(result.reason, contains('not configured'));
|
||||
});
|
||||
|
||||
test('pushes a new record when nothing has been synced yet', () async {
|
||||
@ -165,16 +169,41 @@ void main() {
|
||||
);
|
||||
});
|
||||
|
||||
test('a GitHubSyncError is swallowed, not rethrown', () async {
|
||||
test('a sync error is swallowed, but reported', () async {
|
||||
installFakeSecureStorage(initial: {'sync.token': 'tok'});
|
||||
final httpClient = http_testing.MockClient(
|
||||
(request) async => http.Response('', 500),
|
||||
);
|
||||
|
||||
await expectLater(
|
||||
_service(httpClient: httpClient).push(_session()),
|
||||
completes,
|
||||
final result = await _service(httpClient: httpClient).push(_session());
|
||||
|
||||
// Swallowed: a failed push must never cost the user their finished
|
||||
// workout. Reported: the previous contract logged to debugPrint, which
|
||||
// goes nowhere in a release build -- a workout could go unpushed with
|
||||
// nothing anywhere saying so.
|
||||
expect(result.pushed, isFalse);
|
||||
expect(result.reason, contains('push failed'));
|
||||
});
|
||||
|
||||
test('PushResult stringifies both fields for logs', () {
|
||||
// It exists to be readable in a log line, so the log line is the test.
|
||||
const result = PushResult(pushed: false, reason: 'push failed: boom');
|
||||
|
||||
expect(result.toString(), contains('pushed: false'));
|
||||
expect(result.toString(), contains('push failed: boom'));
|
||||
});
|
||||
|
||||
test('a successful push reports that it happened', () async {
|
||||
installFakeSecureStorage(initial: {'sync.token': 'tok'});
|
||||
final (:httpClient, :putCalls) = _mockGitHub(
|
||||
contentResponses: {'screen-locker-sync/devices': _response(200, [])},
|
||||
);
|
||||
|
||||
final result = await _service(httpClient: httpClient).push(_session());
|
||||
|
||||
expect(result.pushed, isTrue);
|
||||
expect(result.reason, 'pushed');
|
||||
expect(putCalls, isNotEmpty);
|
||||
});
|
||||
|
||||
Record _manual(String id, String cost, int wallMs) => Record(
|
||||
@ -234,7 +263,7 @@ void main() {
|
||||
);
|
||||
});
|
||||
|
||||
test('pushManual swallows a sync error', () async {
|
||||
test('pushManual reports a failure instead of hiding it', () async {
|
||||
installFakeSecureStorage(initial: {'sync.token': 'tok'});
|
||||
final httpClient = http_testing.MockClient(
|
||||
(request) async => http.Response('', 500),
|
||||
|
||||
Loading…
Reference in New Issue
Block a user