diff --git a/lib/health/health_export.dart b/lib/health/health_export.dart index 874a7158..8d5365fc 100644 --- a/lib/health/health_export.dart +++ b/lib/health/health_export.dart @@ -93,6 +93,39 @@ List healthDeleteTypes({required bool isApplePlatform}) { : types.where((type) => type != HealthDataType.HEART_RATE).toList(); } +/// Does a `delete()` answer mean the window is now clear of OUR samples — i.e. +/// is it safe to write the replacement? +/// +/// The two stores answer the EMPTY range differently, so the raw bool cannot be +/// read the same way on both, and reading it wrong breaks the delete-then-write +/// idempotency in opposite directions: +/// +/// * Health Connect (`HealthPlugin.deleteData`) calls `deleteRecords` over a +/// time range and reports success unless it THREW. A zero-match range is a +/// normal success. So `false` there is always a genuine failure — a denied +/// permission, an unmapped type, a store error — and the samples we meant +/// to replace may well still be sitting there. +/// +/// * HealthKit (`SwiftHealthPlugin.delete`) queries our own samples +/// (`HKSource.default()`) and hands whatever came back to +/// `HKHealthStore.delete`, which Apple documents as: "Deleting an empty +/// array fails with an `errorInvalidArgument` error." So on Apple `false` +/// is the NORMAL answer for a window we have never written — every FIRST +/// export of every day and every workout sees it — and it says nothing +/// about whether a real sample survived. +/// +/// Hence: gate the write on Android, never on Apple. Trusting Apple's `false` +/// is not optimism, it is the only reading the platform supports; and the +/// HealthKit failures that CAN leave one of our samples behind (share +/// permission never requested, or denied) suppress the following write for the +/// same reason, so they cannot produce the duplicate this gate exists to stop. +/// +/// Parameterised on [ios] rather than reading `Platform` for the same reason +/// [healthActivityForType] is — so a host-VM test can exercise both branches. +@visibleForTesting +bool healthDeleteClearedRange({required bool deleted, required bool ios}) => + deleted || ios; + /// Cursor for the one-shot Apple Health sleep rewrite. Bump when the writer /// changes enough that nights already sitting in HealthKit should be replaced /// (plugin Core/in-bed misses, leftover 11pm fragments). Does not bump @@ -355,6 +388,32 @@ class HealthExporter { } } + /// Delete OUR [type] samples in [start,end) and report whether the window is + /// now safe to write into — see [healthDeleteClearedRange] for why the two + /// stores' `false` cannot be read the same way. + /// + /// A THROW is a genuine failure on both stores (and on Apple the only signal + /// there is), so it is the one case that reports "not cleared" everywhere. + Future _deleteOwnSamples( + HealthDataType type, + DateTime start, + DateTime end, + ) async { + try { + return healthDeleteClearedRange( + deleted: await _health.delete( + type: type, + startTime: start, + endTime: end, + ), + ios: isApple, + ); + } catch (e) { + debugPrint('[health] delete ${type.name}: $e'); + return false; + } + } + // We do NOT gate on a write-permission check: HealthKit hides write-auth by // design, and Health Connect's hasPermissions(WRITE) frequently returns // null/false even after the user grants everything — which would leave the UI @@ -844,21 +903,20 @@ class HealthExporter { // Idempotency: remove OUR previously-written samples for this day (HealthKit / // Health Connect only let an app delete its own data), then re-write fresh. // Sleep is not in this list — native replace already deleted it. + // + // A type whose delete did NOT clear the window must not be re-written on + // top of the survivor — that is how a retry turns one stale sample into + // two, then three. Only WORKOUT is tracked, because a duplicated workout + // is the one that shows up as a second entry in the user's activity list + // (and is the type `exportWorkout` re-writes out-of-band too); the scalar + // types below still write unconditionally, so an uncleared RHR/HRV window + // can still double until the next successful delete replaces both. + var workoutCleared = true; for (final t in _rewriteTypes) { - try { - final deleted = await _health.delete( - type: t, - startTime: dayStart, - endTime: dayEnd, - ); - if (!deleted) { - debugPrint('[health] delete ${t.name} returned false'); - success = false; - } - } catch (e) { - debugPrint('[health] delete ${t.name}: $e'); - success = false; - } + if (await _deleteOwnSamples(t, dayStart, dayEnd)) continue; + debugPrint('[health] delete ${t.name} did not clear the day'); + success = false; + if (t == HealthDataType.WORKOUT) workoutCleared = false; } final scalars = (b['scalars'] as Map?)?.cast() ?? const {}; @@ -1099,7 +1157,12 @@ class HealthExporter { debugPrint('[health] query workouts: $e'); success = false; } - if (rows != null) { + // `workoutCleared` is false only when the day's WORKOUT delete genuinely + // failed (see the rewrite loop above), in which case every row here would + // land beside a survivor. `success` is already false, so the day retries + // and re-attempts the delete first; skipping only this block keeps that + // failure from touching the day's unrelated exports. + if (rows != null && workoutCleared) { for (final r in rows) { if (await _writeOneWorkout(r) == false) { debugPrint('[health] write workout returned false'); @@ -1161,6 +1224,11 @@ class HealthExporter { /// [_exportDay], which owns the whole-day delete). Best-effort — never /// throws; no-op if the health store isn't configured/available/permitted /// (mirrors [_exportDay]'s silent-no-op-on-missing-permission contract). + /// + /// The idempotency is only as good as the delete, so the write is GATED on + /// it: a delete that did not clear the window returns false without writing, + /// because writing beside a survivor is what turns a retry into a duplicate. + /// See [healthDeleteClearedRange] for what "did not clear" means per store. Future exportWorkout(Map session) async { if ((session['status']?.toString() ?? '') == 'live') return false; final st = (session['start_ts'] as num?)?.toInt(); @@ -1171,20 +1239,16 @@ class HealthExporter { if (await _androidUnavailable() != null) return false; final start = DateTime.fromMillisecondsSinceEpoch(st * 1000); final end = DateTime.fromMillisecondsSinceEpoch(en * 1000); - var success = true; - try { - final deleted = await _health.delete( - type: HealthDataType.WORKOUT, - startTime: start, - endTime: end, - ); - if (!deleted) success = false; - } catch (e) { - debugPrint('[health] delete workout @$st: $e'); - success = false; + if (!await _deleteOwnSamples(HealthDataType.WORKOUT, start, end)) { + // The window still holds a copy we could not remove, so writing now + // would leave TWO of this workout in the store — and returning false + // hands the caller a retry, which would write a third. Bail instead: + // the same false still asks for a retry, but the retry re-attempts the + // delete FIRST and only writes once it actually clears. + debugPrint('[health] delete workout @$st did not clear the window'); + return false; } - final wrote = (await _writeOneWorkout(session)) ?? false; - return success && wrote; + return (await _writeOneWorkout(session)) ?? false; } catch (e) { debugPrint('[health] exportWorkout: $e'); return false; diff --git a/test/health_workout_export_delete_gate_test.dart b/test/health_workout_export_delete_gate_test.dart new file mode 100644 index 00000000..c96bda06 --- /dev/null +++ b/test/health_workout_export_delete_gate_test.dart @@ -0,0 +1,129 @@ +import 'package:flutter/services.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:openstrap_edge/health/health_export.dart'; + +/// A stand-in for the platform health store, driven over the `health` plugin's +/// own method channel — the only seam `HealthExporter` reaches the store +/// through. Records every call so a test can assert what the exporter did +/// AFTER a delete came back false, which is the whole point: the bug was that +/// it wrote anyway. +class _FakeHealthStore { + _FakeHealthStore({required this.deleteResult, this.deleteThrows = false}); + + /// What `delete` answers. On Health Connect a `false` here means the delete + /// genuinely failed and whatever we wrote before is STILL in the store. + final bool deleteResult; + final bool deleteThrows; + + final calls = []; + + static const _channel = MethodChannel('flutter_health'); + + void install() { + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger + .setMockMethodCallHandler(_channel, (call) async { + calls.add(call.method); + switch (call.method) { + case 'delete': + if (deleteThrows) { + throw PlatformException(code: 'delete-failed'); + } + return deleteResult; + case 'writeWorkoutData': + return true; + default: + return null; + } + }); + } + + void remove() { + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger + .setMockMethodCallHandler(_channel, null); + } +} + +Map _session() { + final start = DateTime(2026, 8, 26, 18, 30); + final end = DateTime(2026, 8, 26, 19, 15); + return { + 'status': 'done', + 'type': 'run', + 'start_ts': start.millisecondsSinceEpoch ~/ 1000, + 'end_ts': end.millisecondsSinceEpoch ~/ 1000, + 'calories': 412, + }; +} + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + + group('healthDeleteClearedRange', () { + // The fact the whole gate hinges on, pinned so a future reader does not + // "simplify" it back to a bare `deleted`. Verified against the plugin + // sources in health 12.2.1 and Apple's own docs: + // + // * HealthPlugin.kt `deleteData` -> deleteRecords over a time range, + // `result.success(true)` unless it threw. Zero matches is a success. + // * SwiftHealthPlugin.swift `delete` -> HKSampleQuery scoped to + // HKSource.default(), then HKHealthStore.delete(samples) with whatever + // came back — including an EMPTY array, which Apple documents as + // "Deleting an empty array fails with an errorInvalidArgument error". + // + // So on Apple a false delete is what every first export sees, and gating + // the write on it would mean no workout ever reaches HealthKit at all. + test('Health Connect false is a genuine failure', () { + expect(healthDeleteClearedRange(deleted: false, ios: false), isFalse); + expect(healthDeleteClearedRange(deleted: true, ios: false), isTrue); + }); + + test('HealthKit false is the documented empty-range answer', () { + expect(healthDeleteClearedRange(deleted: false, ios: true), isTrue); + expect(healthDeleteClearedRange(deleted: true, ios: true), isTrue); + }); + }); + + group('exportWorkout delete-then-write', () { + // The host VM is neither iOS nor macOS, so `HealthExporter.isApple` is + // false and these exercise the Health-Connect reading of `delete`. + late _FakeHealthStore store; + + tearDown(() => store.remove()); + + test('a failed delete does not write a duplicate on top of it', () async { + store = _FakeHealthStore(deleteResult: false)..install(); + + final ok = await HealthExporter().exportWorkout(_session()); + + expect(ok, isFalse, reason: 'the caller must retry this workout'); + expect(store.calls, contains('delete')); + expect( + store.calls, + isNot(contains('writeWorkoutData')), + reason: + 'the previously exported copy survived the delete, so writing ' + 'would leave two of this workout in the store — and the false ' + 'return drives a retry that would write a third', + ); + }); + + test('a thrown delete does not write either', () async { + store = _FakeHealthStore(deleteResult: true, deleteThrows: true) + ..install(); + + final ok = await HealthExporter().exportWorkout(_session()); + + expect(ok, isFalse); + expect(store.calls, isNot(contains('writeWorkoutData'))); + }); + + test('a successful delete still writes', () async { + store = _FakeHealthStore(deleteResult: true)..install(); + + final ok = await HealthExporter().exportWorkout(_session()); + + expect(ok, isTrue); + expect(store.calls, containsAllInOrder(['delete', 'writeWorkoutData'])); + }); + }); +}