From 001ffffd1fc564198bd823096c2eeaf173e818d0 Mon Sep 17 00:00:00 2001 From: Alex Wagner Date: Thu, 27 Aug 2026 20:05:38 +0200 Subject: [PATCH] gate the workout health write on a delete that actually cleared exportWorkout and _exportDay both document delete-then-write idempotency but called the write even when the preceding delete returned false or threw -- they only recorded success = false and carried on. If an already-exported copy survived the failed delete and the write then succeeded, the store ended up with two of the same workout, and the false return drove a retry through the attempts/backoff/give-up machinery that would write a third. The write cannot simply be gated on `delete() == true`, because the two stores answer the EMPTY range differently: * 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. * HealthKit (SwiftHealthPlugin.delete) queries our own samples via HKSource.default() and hands whatever came back -- including an EMPTY array -- 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, and gating on it would mean no first export ever reaches HealthKit. healthDeleteClearedRange encodes that asymmetry: gate 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 authorization reason, so they cannot produce the duplicate the gate exists to stop. A throw is a genuine failure on both. That same reading also stops an empty-range delete from flipping a day's success on Apple, where it was marking essentially every fresh export as failed and driving the retry cursor with no actual failure behind it. In the per-day path only the workout block is skipped when the day's WORKOUT delete fails, so a failed workout export cannot pause that day's unrelated RHR/HRV/sleep/energy exports. The scalar types still write unconditionally and can still double on Health Connect -- noted in the rewrite loop, not fixed here. healthDeleteClearedRange takes `ios` rather than reading Platform for the same reason healthActivityForType does, so the new fake-store test can exercise both branches on a host VM. --- lib/health/health_export.dart | 120 ++++++++++++---- ...ealth_workout_export_delete_gate_test.dart | 129 ++++++++++++++++++ 2 files changed, 221 insertions(+), 28 deletions(-) create mode 100644 test/health_workout_export_delete_gate_test.dart 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'])); + }); + }); +}