From 23bfd785b187f3a56ba0835ea56b77ca07e6c524 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 11:29:56 +0530 Subject: [PATCH 01/12] strength workouts now reach Apple Health we were sending STRENGTH_TRAINING on both platforms, but that value only exists on Health Connect - HealthKit rejects it, so every strength workout was dropped on iOS and the error went to a debugPrint nobody sees. swims had the same problem the other way round (bare SWIMMING is iOS-only, so they were being dropped on android). --- lib/health/health_export.dart | 92 ++++++++++++------ test/workout_health_mapping_test.dart | 135 ++++++++++++++++++++++++++ 2 files changed, 199 insertions(+), 28 deletions(-) create mode 100644 test/workout_health_mapping_test.dart diff --git a/lib/health/health_export.dart b/lib/health/health_export.dart index b14546d..ac57fff 100644 --- a/lib/health/health_export.dart +++ b/lib/health/health_export.dart @@ -1024,34 +1024,8 @@ class HealthExporter { } } - HealthWorkoutActivityType _activity(String? type) { - switch ((type ?? '').toLowerCase()) { - case 'run': - case 'running': - return HealthWorkoutActivityType.RUNNING; - case 'cycle': - case 'cycling': - case 'bike': - case 'biking': - return HealthWorkoutActivityType.BIKING; - case 'walk': - case 'walking': - return HealthWorkoutActivityType.WALKING; - case 'swim': - case 'swimming': - return HealthWorkoutActivityType.SWIMMING; - case 'strength': - case 'weights': - case 'lifting': - return HealthWorkoutActivityType.STRENGTH_TRAINING; - case 'yoga': - return HealthWorkoutActivityType.YOGA; - case 'hiit': - return HealthWorkoutActivityType.HIGH_INTENSITY_INTERVAL_TRAINING; - default: - return HealthWorkoutActivityType.OTHER; - } - } + HealthWorkoutActivityType _activity(String? type) => + healthActivityForType(type, ios: Platform.isIOS); static Map? _decode(Object? json) { if (json is! String) return null; @@ -1083,3 +1057,65 @@ class HealthExporter { return DateTime(y, m, d); } } + +/// The app's workout-type key -> platform health activity type. +/// +/// Parameterised by [ios] rather than reading `Platform` directly so a unit +/// test can exercise BOTH platform branches on a host VM (where `Platform.isIOS` +/// and `Platform.isAndroid` are both false) — see +/// `test/workout_health_mapping_test.dart`. +/// +/// Why the platform branches exist at all: `health`'s `writeWorkoutData` rejects +/// (throws `HealthException`, before the platform channel) any activity type +/// absent from that platform's own supported set, and the two platforms spell +/// the strength and swim families differently: +/// +/// | app key | iOS | Android | +/// |------------|--------------------------------|------------------| +/// | `strength` | TRADITIONAL_STRENGTH_TRAINING | STRENGTH_TRAINING| +/// | `swim` | SWIMMING | SWIMMING_POOL | +/// +/// iOS has no bare `STRENGTH_TRAINING`; Android has neither `TRADITIONAL_`/ +/// `FUNCTIONAL_STRENGTH_TRAINING` nor bare `SWIMMING`. Using one spelling for +/// both platforms silently drops every workout of that type on the other one — +/// that is issue #184 (no strength workout ever reached Apple Health) and the +/// same latent bug existed for swims on Android. +/// +/// Anything unmapped falls back to `OTHER`, which both platforms accept, so an +/// unrecognised or autodetected type still lands in the health store. +@visibleForTesting +HealthWorkoutActivityType healthActivityForType( + String? type, { + required bool ios, +}) { + switch ((type ?? '').toLowerCase()) { + case 'run': + case 'running': + return HealthWorkoutActivityType.RUNNING; + case 'cycle': + case 'cycling': + case 'bike': + case 'biking': + return HealthWorkoutActivityType.BIKING; + case 'walk': + case 'walking': + return HealthWorkoutActivityType.WALKING; + case 'swim': + case 'swimming': + return ios + ? HealthWorkoutActivityType.SWIMMING + : HealthWorkoutActivityType.SWIMMING_POOL; + case 'strength': + case 'weights': + case 'lifting': + return ios + ? HealthWorkoutActivityType.TRADITIONAL_STRENGTH_TRAINING + : HealthWorkoutActivityType.STRENGTH_TRAINING; + case 'yoga': + return HealthWorkoutActivityType.YOGA; + case 'hiit': + return HealthWorkoutActivityType.HIGH_INTENSITY_INTERVAL_TRAINING; + default: + return HealthWorkoutActivityType.OTHER; + } +} diff --git a/test/workout_health_mapping_test.dart b/test/workout_health_mapping_test.dart new file mode 100644 index 0000000..0a2cf66 --- /dev/null +++ b/test/workout_health_mapping_test.dart @@ -0,0 +1,135 @@ +// Every workout type the app can start MUST map to an activity type the target +// platform's health store actually accepts. +// +// Issue #184: `strength` mapped to `HealthWorkoutActivityType.STRENGTH_TRAINING` +// on BOTH platforms. That value exists only in the plugin's Android set, so on +// iOS `writeWorkoutData` threw `HealthException` *before* the platform channel, +// the throw was swallowed by a `debugPrint`, and no strength workout ever +// reached Apple Health. The same latent bug existed for `swim`, which mapped to +// bare `SWIMMING` — an iOS-only value — and so was dropped on Android. +// +// The supported sets below are transcribed from `health: 11.1.1` +// (`lib/src/health_plugin.dart`, `_isOnIOS` / `_isOnAndroid`), restricted to the +// values `healthActivityForType` can actually emit. They are a PIN, not a +// mirror: on a `health` upgrade, re-check those two functions and update these +// sets deliberately. If a value silently leaves a platform's set upstream, this +// test is what catches it before another workout family goes missing for a +// release. + +import 'package:flutter_test/flutter_test.dart'; +import 'package:health/health.dart'; +import 'package:openstrap_edge/health/health_export.dart'; +import 'package:openstrap_edge/ui/workouts/workout_types.dart'; + +/// Values `healthActivityForType` may emit that iOS (HealthKit) accepts. +const _iosSupported = { + HealthWorkoutActivityType.RUNNING, + HealthWorkoutActivityType.BIKING, + HealthWorkoutActivityType.WALKING, + HealthWorkoutActivityType.SWIMMING, + HealthWorkoutActivityType.TRADITIONAL_STRENGTH_TRAINING, + HealthWorkoutActivityType.YOGA, + HealthWorkoutActivityType.HIGH_INTENSITY_INTERVAL_TRAINING, + HealthWorkoutActivityType.OTHER, +}; + +/// Values `healthActivityForType` may emit that Android (Health Connect) accepts. +const _androidSupported = { + HealthWorkoutActivityType.RUNNING, + HealthWorkoutActivityType.BIKING, + HealthWorkoutActivityType.WALKING, + HealthWorkoutActivityType.SWIMMING_POOL, + HealthWorkoutActivityType.STRENGTH_TRAINING, + HealthWorkoutActivityType.YOGA, + HealthWorkoutActivityType.HIGH_INTENSITY_INTERVAL_TRAINING, + HealthWorkoutActivityType.OTHER, +}; + +/// Type strings that can reach the exporter but are not in [kWorkoutTypes]: +/// manual-start aliases and the auto-detector's own vocabulary. +const _extraTypeStrings = [ + 'running', + 'cycling', + 'bike', + 'biking', + 'walking', + 'swimming', + 'weights', + 'lifting', + 'autodetected', + 'autodetected_workout', + 'workout', + '', +]; + +void main() { + final allTypes = [ + ...kWorkoutTypes.map((e) => e.$1), + ..._extraTypeStrings, + null, + ]; + + group('healthActivityForType stays inside each platform supported set', () { + for (final type in allTypes) { + test('"${type ?? ''}" is writable on both platforms', () { + expect( + _iosSupported, + contains(healthActivityForType(type, ios: true)), + reason: + 'iOS would throw HealthException for "$type" and the workout ' + 'would never reach Apple Health (issue #184)', + ); + expect( + _androidSupported, + contains(healthActivityForType(type, ios: false)), + reason: + 'Health Connect would throw HealthException for "$type" and the ' + 'workout would never reach Android health', + ); + }); + } + }); + + test('strength maps to the platform-correct strength spelling', () { + expect( + healthActivityForType('strength', ios: true), + HealthWorkoutActivityType.TRADITIONAL_STRENGTH_TRAINING, + ); + expect( + healthActivityForType('strength', ios: false), + HealthWorkoutActivityType.STRENGTH_TRAINING, + ); + // The aliases the manual-start UI and older rows can carry. + for (final alias in ['weights', 'lifting', 'Strength', 'STRENGTH']) { + expect( + healthActivityForType(alias, ios: true), + HealthWorkoutActivityType.TRADITIONAL_STRENGTH_TRAINING, + reason: '"$alias" must land on the same iOS type as "strength"', + ); + } + }); + + test('swim maps to the platform-correct swim spelling', () { + expect( + healthActivityForType('swim', ios: true), + HealthWorkoutActivityType.SWIMMING, + ); + expect( + healthActivityForType('swim', ios: false), + HealthWorkoutActivityType.SWIMMING_POOL, + ); + }); + + test('an unknown type degrades to OTHER rather than an unwritable value', () { + for (final unknown in ['surfing', 'padel', 'autodetected', null]) { + expect( + healthActivityForType(unknown, ios: true), + HealthWorkoutActivityType.OTHER, + ); + expect( + healthActivityForType(unknown, ios: false), + HealthWorkoutActivityType.OTHER, + ); + } + }); +} From 8fcd67ae86d53a8a0371596997c7e08c879bdb10 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 11:30:08 +0530 Subject: [PATCH 02/12] rescore workout strain from the band's data instead of the live tally strain for a live session was accumulated in RAM by the foreground app, so anything the app slept through was missing from it. background the app for a long workout (or let iOS kill it) and you stop the workout with a strain built from the few minutes the app was awake for - usually a handful of sub-resting minutes, which score exactly 0.0 next to a perfectly real duration and HR. the band had the whole window at 1 Hz the entire time. sessions are now rescored from that once it drains in, taking whichever of the two is higher (both are lower bounds over the same window, so this only ever improves and it settles). runs on read and after each drain, so existing broken workouts fix themselves. also stopped the share cards printing '0.0 Strain' for a session that was never scored at all. --- lib/compute/manual_session.dart | 109 +++++++++++++++ lib/data/local_repository.dart | 7 + lib/data/local_repository_impl.dart | 111 +++++++++++++++- lib/ui/activity/workout_share_card.dart | 13 +- lib/ui/workouts/workouts_screen.dart | 4 +- test/session_score_reconcile_test.dart | 168 ++++++++++++++++++++++++ 6 files changed, 405 insertions(+), 7 deletions(-) create mode 100644 test/session_score_reconcile_test.dart diff --git a/lib/compute/manual_session.dart b/lib/compute/manual_session.dart index 096e25e..2240d11 100644 --- a/lib/compute/manual_session.dart +++ b/lib/compute/manual_session.dart @@ -415,3 +415,112 @@ List supersededSuggestionIds( } return out; } + +/// The best available scoring of a live-captured session's window: the tallies +/// the live gauge accumulated in RAM, reconciled against a re-score of the SAME +/// window from the 1 Hz substrate. +class ReconciledSessionScore { + const ReconciledSessionScore({ + this.strain, + this.calories, + this.maxHr, + this.zoneMinutes = const [], + this.changed = false, + }); + + final double? strain; + final double? calories; + final int? maxHr; + final List zoneMinutes; + + /// True when the substrate improved on at least one stored field — the only + /// case worth a write. + final bool changed; +} + +/// Reconcile a stored live session against a substrate re-score of its window. +/// +/// WHY THIS EXISTS (issue #206): a live session's strain/calories/zone minutes +/// are accumulated in RAM, one tick per second, by the foreground app. That +/// accumulator sees nothing while the app is suspended — iOS suspends the 1 Hz +/// `Timer.periodic` the moment the app backgrounds, and an app the OS kills +/// mid-workout resumes with an EMPTY accumulator (`_reconcileOrphanedLiveWorkout` +/// rehydrates the row, not the tallies). Stop the workout after that and the +/// stored strain describes only the handful of minutes the app happened to be +/// awake for — commonly a few sub-resting minutes, whose Banister TRIMP is +/// exactly 0, which `strainScore` reports as a confident `0.0`. The user sees a +/// real duration, real avg/max HR and real zone bands next to "0.0 Strain". +/// +/// The band, meanwhile, banked the whole window at 1 Hz. Once that window has +/// drained into `decoded_onehz`, re-scoring it through the SAME method +/// ([computeManualSessionStats]) recovers the real number. +/// +/// THE MERGE RULE IS `max`, and that is deliberate — not a heuristic: +/// both numbers are the same monotone function (TRIMP is a sum of +/// per-minute non-negative terms) evaluated over SUBSETS of one window's +/// minutes. The live tally saw the minutes the app was awake for; the substrate +/// sees the minutes the band has drained so far. Each is therefore a LOWER +/// BOUND on the true score, and the larger one is strictly the better estimate. +/// Taking the max can never double-count (it is a max over two views of one +/// window, not a sum) and it is monotone under repeated application, so calling +/// this again after more of the window drains only ever improves the value and +/// converges. Averaging or preferring one source outright would both be wrong: +/// the substrate is empty right after a workout (the band has not offloaded +/// yet) and the live tally is empty after an app kill. +/// +/// Absent stays absent: a null on both sides stays null rather than becoming +/// `0.0`. [substrate] must be the re-score of exactly `[start_ts, end_ts)`. +ReconciledSessionScore reconcileSessionScore({ + required double? liveStrain, + required double? liveCalories, + required int? liveMaxHr, + required List liveZoneMinutes, + required ManualSessionStats substrate, +}) { + // No substrate for this window (not drained yet, or pruned) — the live tally + // is all the evidence there is. + if (substrate.isUnscored) { + return ReconciledSessionScore( + strain: liveStrain, + calories: liveCalories, + maxHr: liveMaxHr, + zoneMinutes: liveZoneMinutes, + ); + } + + double? better(double? a, double? b) { + if (a == null) return b; + if (b == null) return a; + return a >= b ? a : b; + } + + final strain = better(liveStrain, substrate.strain); + final calories = better(liveCalories, substrate.calories); + final maxHr = liveMaxHr == null + ? substrate.maxHr + : (substrate.maxHr == null + ? liveMaxHr + : (liveMaxHr >= substrate.maxHr! ? liveMaxHr : substrate.maxHr)); + + // Zone minutes are a vector of the same lower-bound quantity, so take the + // side with more total measured minutes rather than mixing two partial + // splits (a per-element max would invent a total neither source observed). + double total(List z) => z.fold(0.0, (a, b) => a + b); + final zone = total(substrate.zoneMinutes) > total(liveZoneMinutes) + ? substrate.zoneMinutes + : liveZoneMinutes; + + final changed = + strain != liveStrain || + calories != liveCalories || + maxHr != liveMaxHr || + !identical(zone, liveZoneMinutes); + + return ReconciledSessionScore( + strain: strain, + calories: calories, + maxHr: maxHr, + zoneMinutes: zone, + changed: changed, + ); +} diff --git a/lib/data/local_repository.dart b/lib/data/local_repository.dart index 5a966dd..33ad9fd 100644 --- a/lib/data/local_repository.dart +++ b/lib/data/local_repository.dart @@ -108,6 +108,13 @@ abstract class LocalRepository { throw UnimplementedError('re-layer: getWorkout'); Future deleteWorkout(String id) => throw UnimplementedError('re-layer: deleteWorkout'); + + /// Re-score recent finished sessions against the 1 Hz substrate now in the + /// DB, correcting a live session whose in-RAM tallies missed the part of the + /// workout the app slept through (issue #206). Returns the number of rows + /// whose strain changed. Best-effort — never throws. + Future rescoreRecentSessions({int sinceDays = 7}) => + throw UnimplementedError('re-layer: rescoreRecentSessions'); Future> startWorkout(String type, {String? title}) => throw UnimplementedError('re-layer: startWorkout'); Future> endWorkout(String workoutId) => diff --git a/lib/data/local_repository_impl.dart b/lib/data/local_repository_impl.dart index d3c5004..d4fba34 100644 --- a/lib/data/local_repository_impl.dart +++ b/lib/data/local_repository_impl.dart @@ -2016,8 +2016,13 @@ class LocalRepositoryImpl extends LocalRepository { @override Future> getWorkout(String id) async { - final r = await LocalDb.session(id); - if (r == null) return const {}; + final stored = await LocalDb.session(id); + if (stored == null) return const {}; + // Reconcile the live tallies against the substrate BEFORE projecting the + // row (issue #206) — a session the app slept through stores a strain built + // from the few minutes it was awake for. Persists on improvement, so the + // list and the share card see the corrected value too. + final r = await _rescoreSessionFromSubstrate(stored); final w = _workoutOf(r); final startTs = w['start_ts'] as int?; if (startTs == null) return w; @@ -2382,6 +2387,108 @@ class LocalRepositoryImpl extends LocalRepository { }; } + /// Re-score a finished session's strain/calories/max-HR/zone-minutes from the + /// 1 Hz substrate and persist the result when the substrate improves on what + /// the live accumulator managed to see (issue #206). + /// + /// The live tallies only cover the minutes the foreground app was awake for; + /// an app suspended or killed mid-workout stores a strain covering a fraction + /// of the window — often a few sub-resting minutes, which score a confident + /// `0.0`. Once the band offloads that window, the substrate holds the whole + /// thing. [reconcileSessionScore] documents why merging the two by `max` is + /// the correct rule; the short version is that both are lower bounds over + /// subsets of the same window's minutes. + /// + /// Self-healing by construction: it re-runs whenever the row is read or a + /// drain lands, and the merge is monotone, so a partially-drained window + /// improves on each pass and converges. Returns the row with the reconciled + /// values applied (never null-out a stored value), writing back only on a + /// real change. Best-effort — never throws into a read path. + Future> _rescoreSessionFromSubstrate( + Map row, + ) async { + final id = row['id']; + final startTs = (row['start_ts'] as num?)?.toInt(); + final endTs = (row['end_ts'] as num?)?.toInt(); + // A live row is still accumulating; scoring it here would race the tally. + if (id is! String || + startTs == null || + endTs == null || + endTs <= startTs || + (row['status']?.toString() ?? '') != 'done') { + return row; + } + try { + final hrRows = await LocalDb.hrSamplesInRange(startTs, endTs); + if (hrRows.isEmpty) return row; + + final profile = Profile.fromMap(getProfileMap()); + final stats = computeManualSessionStats( + hrTs: [for (final e in hrRows) (e['rec_ts'] as num).toInt()], + hrBpm: [for (final e in hrRows) (e['hr'] as num).toInt()], + profile: profile, + zoneMaxHr: _profileMaxHr().toDouble(), + restingHr: + await _recentRestingHr() ?? profile.restingHrManual?.toDouble(), + ); + + final merged = reconcileSessionScore( + liveStrain: (row['strain'] as num?)?.toDouble(), + liveCalories: (row['calories'] as num?)?.toDouble(), + liveMaxHr: (row['max_hr'] as num?)?.toInt(), + liveZoneMinutes: [ + for (final v in _decodeList(row['zone_min_json'])) + if (v is num) v.toDouble(), + ], + substrate: stats, + ); + if (!merged.changed) return row; + + final updated = { + ...row, + 'strain': merged.strain, + 'calories': merged.calories, + 'max_hr': merged.maxHr, + 'zone_min_json': jsonEncode( + merged.zoneMinutes.any((v) => v > 0) + ? merged.zoneMinutes + : const [], + ), + }; + await LocalDb.putSession(updated); + return updated; + } catch (_) { + return row; // best-effort: the stored row still renders + } + } + + /// Re-score every finished session that started in the last [sinceDays] days + /// against the substrate now in the DB. Called after a drain lands, so a + /// workout whose window arrived late is corrected on the LIST too, not only + /// when its detail screen is opened. Returns how many rows changed. + @override + Future rescoreRecentSessions({int sinceDays = 7}) async { + final nowSec = DateTime.now().millisecondsSinceEpoch ~/ 1000; + var changed = 0; + try { + final rows = await LocalDb.sessionsInRange( + nowSec - sinceDays * 86400, + nowSec, + ); + for (final r in rows) { + final before = (r['strain'] as num?)?.toDouble(); + final after = await _rescoreSessionFromSubstrate(r); + if (!identical(after, r) && + (after['strain'] as num?)?.toDouble() != before) { + changed++; + } + } + } catch (_) { + /* best-effort */ + } + return changed; + } + /// Most recent nightly resting HR from `metric_series`, or null. Bounded to /// the last week so a stale figure from a long gap can't anchor TRIMP. Future _recentRestingHr() async { diff --git a/lib/ui/activity/workout_share_card.dart b/lib/ui/activity/workout_share_card.dart index 9980748..7bcfbe0 100644 --- a/lib/ui/activity/workout_share_card.dart +++ b/lib/ui/activity/workout_share_card.dart @@ -547,12 +547,19 @@ WorkoutShareData buildWorkoutShareData({ required Duration duration, required DateTime when, required int maxHr, - required double strain, + + /// Null when the session was never scored (a profile anchor the Banister + /// formula needs is missing, or no HR was ever captured for the window). + /// Nullable all the way to the card: a `?? 0` at the call site prints a + /// confident "0.0 Strain" for a workout we simply could not score, which is + /// the same fabrication issue #206 reported on the detail gauge. + required double? strain, required int calories, WorkoutRoute? route, int? avgHr, }) { final hasRoute = route != null && route.hasPath; + final strainText = strain?.toStringAsFixed(1) ?? '—'; final title = type.isEmpty ? 'Workout' : type[0].toUpperCase() + type.substring(1); @@ -568,13 +575,13 @@ WorkoutShareData buildWorkoutShareData({ (_shareDuration(duration), 'Time'), // Moving pace, like everywhere else — see the note in _GpsControlPanel. (units.pace(route.distanceMeters, route.movingSec), 'Pace'), - (strain.toStringAsFixed(1), 'Strain'), + (strainText, 'Strain'), ]; } else { heroValue = _shareDuration(duration); heroUnit = ''; stats = [ - (strain.toStringAsFixed(1), 'Strain'), + (strainText, 'Strain'), ('$calories', 'Kcal'), (avgHr != null && avgHr > 0 ? '$avgHr' : '—', 'Avg bpm'), ]; diff --git a/lib/ui/workouts/workouts_screen.dart b/lib/ui/workouts/workouts_screen.dart index 304a807..aa256fd 100644 --- a/lib/ui/workouts/workouts_screen.dart +++ b/lib/ui/workouts/workouts_screen.dart @@ -418,7 +418,7 @@ class _WorkoutsScreenState extends State { duration: Duration(minutes: (w['duration_min'] as num?)?.toInt() ?? 0), peakHr: (w['max_hr'] as num?)?.toInt() ?? 0, calories: ((w['calories'] as num?) ?? 0).toDouble(), - strain: ((w['strain'] as num?) ?? 0).toDouble(), + strain: (w['strain'] as num?)?.toDouble(), steps: (w['steps'] as num?)?.toInt() ?? 0, ); Navigator.of(context).push( @@ -1213,7 +1213,7 @@ class _WorkoutDetailBodyState extends State<_WorkoutDetailBody> { ? DateTime.fromMillisecondsSinceEpoch(startTs * 1000).toLocal() : DateTime.now(), maxHr: context.read().maxHr, - strain: (d['strain'] as num?)?.toDouble() ?? 0, + strain: (d['strain'] as num?)?.toDouble(), calories: (d['calories'] as num?)?.toInt() ?? 0, route: _route, avgHr: (d['avg_hr'] as num?)?.toInt(), diff --git a/test/session_score_reconcile_test.dart b/test/session_score_reconcile_test.dart new file mode 100644 index 0000000..37fedbb --- /dev/null +++ b/test/session_score_reconcile_test.dart @@ -0,0 +1,168 @@ +// Issue #206: a live session's strain is accumulated in RAM by the foreground +// app. Backgrounded (iOS suspends the 1 Hz timer) or killed mid-workout, that +// accumulator misses most of the workout — commonly leaving a handful of +// sub-resting minutes whose Banister TRIMP is exactly 0, which `strainScore` +// reports as a confident 0.0 next to a real duration and real HR. +// +// `reconcileSessionScore` merges that partial tally with a re-score of the same +// window from the 1 Hz substrate the band banked. These tests pin the merge +// rule (max, because both sides are lower bounds over subsets of one window's +// minutes) and the properties that make repeated application safe. + +import 'package:flutter_test/flutter_test.dart'; +import 'package:openstrap_edge/compute/manual_session.dart'; + +ManualSessionStats _substrate({ + double? strain, + double? calories, + int? maxHr, + List zone = const [], + int samples = 1800, +}) => ManualSessionStats( + strain: strain, + calories: calories, + maxHr: maxHr, + zoneMinutes: zone, + hrSampleCount: samples, +); + +void main() { + test('a 0.0 live tally is replaced by the substrate score', () { + final r = reconcileSessionScore( + liveStrain: 0.0, // the app was awake only for sub-resting minutes + liveCalories: 3.0, + liveMaxHr: 71, + liveZoneMinutes: const [2, 0, 0, 0, 0], + substrate: _substrate( + strain: 11.4, + calories: 480, + maxHr: 168, + zone: const [4, 12, 20, 9, 1], + ), + ); + expect(r.strain, 11.4); + expect(r.calories, 480); + expect(r.maxHr, 168); + expect(r.zoneMinutes, const [4, 12, 20, 9, 1]); + expect(r.changed, isTrue); + }); + + test('an empty substrate leaves the live tally untouched', () { + // Right after a workout the band has not offloaded the window yet. The live + // tally is all the evidence there is — it must not be wiped to null. + final r = reconcileSessionScore( + liveStrain: 8.2, + liveCalories: 300, + liveMaxHr: 160, + liveZoneMinutes: const [1, 2, 3, 0, 0], + substrate: _substrate(samples: 0), + ); + expect(r.strain, 8.2); + expect(r.calories, 300); + expect(r.maxHr, 160); + expect(r.zoneMinutes, const [1, 2, 3, 0, 0]); + expect(r.changed, isFalse, reason: 'nothing improved — no write'); + }); + + test('a partially drained window never LOWERS a better live tally', () { + // The band has offloaded only the first few minutes so far. + final r = reconcileSessionScore( + liveStrain: 9.0, + liveCalories: 400, + liveMaxHr: 171, + liveZoneMinutes: const [1, 5, 10, 4, 0], + substrate: _substrate( + strain: 2.1, + calories: 90, + maxHr: 140, + zone: const [1, 2, 0, 0, 0], + samples: 300, + ), + ); + expect(r.strain, 9.0); + expect(r.calories, 400); + expect(r.maxHr, 171); + expect(r.zoneMinutes, const [1, 5, 10, 4, 0]); + expect(r.changed, isFalse); + }); + + test('absent stays absent — an unscored session never becomes 0.0', () { + final r = reconcileSessionScore( + liveStrain: null, // no profile anchor, so nothing was ever scored + liveCalories: null, + liveMaxHr: null, + liveZoneMinutes: const [], + substrate: _substrate(strain: null, calories: null, maxHr: 150), + ); + expect(r.strain, isNull); + expect(r.calories, isNull); + expect(r.maxHr, 150, reason: 'max HR is measurable without a profile'); + }); + + test('a null live strain is filled from the substrate', () { + final r = reconcileSessionScore( + liveStrain: null, + liveCalories: null, + liveMaxHr: null, + liveZoneMinutes: const [], + substrate: _substrate(strain: 6.5, calories: 210, maxHr: 155), + ); + expect(r.strain, 6.5); + expect(r.calories, 210); + expect(r.changed, isTrue); + }); + + test('a genuinely zero-load window stays 0.0 rather than being hidden', () { + // Sitting still for 30 minutes and calling it a workout IS zero strain. + // The fix must not turn every real zero into an absence. + final r = reconcileSessionScore( + liveStrain: 0.0, + liveCalories: 0.0, + liveMaxHr: 68, + liveZoneMinutes: const [], + substrate: _substrate(strain: 0.0, calories: 0.0, maxHr: 68), + ); + expect(r.strain, 0.0); + expect(r.changed, isFalse); + }); + + test('repeated application converges — the merge is monotone', () { + // Each pass sees more of the drained window; the value only ever rises and + // re-running on an already-merged row is a no-op. + var strain = 0.0; + for (final partial in [1.0, 4.4, 7.9, 11.2, 11.2]) { + final r = reconcileSessionScore( + liveStrain: strain, + liveCalories: null, + liveMaxHr: null, + liveZoneMinutes: const [], + substrate: _substrate(strain: partial), + ); + expect(r.strain! >= strain, isTrue, reason: 'never regresses'); + strain = r.strain!; + } + expect(strain, 11.2); + + final again = reconcileSessionScore( + liveStrain: strain, + liveCalories: null, + liveMaxHr: null, + liveZoneMinutes: const [], + substrate: _substrate(strain: 11.2), + ); + expect(again.changed, isFalse, reason: 'converged — stops writing'); + }); + + test('zone minutes come from one source, never element-wise mixed', () { + // A per-element max would invent a total neither source observed. + final r = reconcileSessionScore( + liveStrain: null, + liveCalories: null, + liveMaxHr: null, + liveZoneMinutes: const [10, 0, 0, 0, 0], // 10 min total + substrate: _substrate(zone: const [0, 3, 9, 2, 0]), // 14 min total + ); + expect(r.zoneMinutes, const [0, 3, 9, 2, 0]); + expect(r.zoneMinutes.fold(0, (a, b) => a + b), 14); + }); +} From 03ffec8aa57e3d10d6fe964b750cfba2832f7c79 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 11:30:17 +0530 Subject: [PATCH 03/12] stop double-counting steps when phone steps are on the day total already prefers the phone's count over the band's - they're the same walk seen from your pocket and your wrist - but the today tile and the steps screen then added the band's live count on top of it. also fixed the today tile still saying 'est'; nothing estimates steps any more. --- lib/ui/screens/screens.dart | 10 ++++++++-- lib/ui/today/today_screen.dart | 19 +++++++++++++++++-- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/lib/ui/screens/screens.dart b/lib/ui/screens/screens.dart index fd2481f..a20d740 100644 --- a/lib/ui/screens/screens.dart +++ b/lib/ui/screens/screens.dart @@ -277,9 +277,15 @@ class _ActivityDetailState extends State<_ActivityDetail> { @override Widget build(BuildContext context) { - // Live steps from the in-flight session count toward TODAY only. + // Live steps from the in-flight session count toward TODAY only — and only + // when the BAND is the day's step source. With phone steps on, the day + // total is already the phone's count of the same walk (`liveStepsForDay` + // prefers phone rows outright rather than summing), so adding the wrist's + // live count would double-count it. final live = _isToday - ? context.select((a) => a.liveSteps) + ? context.select( + (a) => a.phoneStepsEnabled ? 0 : a.liveSteps, + ) : 0; // Was context.watch() — rebuilt this whole board on every one of // AppState's 67 notifyListeners() sources. Only `user` (for step_goal) is diff --git a/lib/ui/today/today_screen.dart b/lib/ui/today/today_screen.dart index 3ddd18b..7ba5de0 100644 --- a/lib/ui/today/today_screen.dart +++ b/lib/ui/today/today_screen.dart @@ -349,7 +349,16 @@ class _TodayScreenState extends State t: t, sparks: _sparks, stepsWeek: _stepsWeek, - liveSteps: context.read().liveSteps, + // Band-derived live steps are an addend to the day metric ONLY + // while the band is the day's step source. `liveStepsForDay` + // deliberately lets phone rows WIN OUTRIGHT over band rows rather + // than summing them (both count the same walk — one from the + // pocket, one from the wrist), so adding the wrist's live count on + // top of a phone-sourced day total re-introduces exactly the + // double count that rule exists to prevent. + liveSteps: context.read().phoneStepsEnabled + ? 0 + : context.read().liveSteps, onOpen: _open, hasAiBriefing: hasAiBriefing, aiBriefing: hasAiBriefing ? BriefingStore.read(period) : null, @@ -1137,7 +1146,13 @@ class TodayVitals extends StatelessWidget { crossAxisAlignment: CrossAxisAlignment.start, mainAxisSize: MainAxisSize.min, children: [ - const TileHeader('Steps', trailing: Tag('est')), + // 'measured', not 'est' — nothing estimates steps any more. The 1 Hz + // estimator was removed (a per-day gravity reference dominated by the + // sleep block put its SNR at ~1); what is left is a real pedometer, + // the phone's or the band's 100 Hz stream, and a day with neither + // shows no number rather than a guess. The detail screen was updated + // to say so and this tile was missed. + const TileHeader('Steps', trailing: Tag('measured')), const SizedBox(height: Sp.x2), BigStat( value: steps > 0 ? '$steps' : null, From 7b6e06a3c1ec727bd84103e278642802c8c4d4f1 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 11:30:27 +0530 Subject: [PATCH 04/12] re-derive after turning phone steps on, and stop showing a live workout 0 steps turning the toggle on pulled the counts into the db but never re-derived, and every screen reads the derived scalars - so if the band wasn't connected you granted permission and the tile just kept showing a dash. turning it off already re-derived; now both do. live workout steps need the band's 100 Hz stream, which is often not up (standard-HR fallback, background downgrade). we were printing '0 STEPS' for that, next to a real distance and a real HR. shows a dash instead when nothing was measured. --- lib/state/app_state.dart | 175 ++++++++++++++++++++++- lib/ui/activity/live_session_screen.dart | 12 +- 2 files changed, 177 insertions(+), 10 deletions(-) diff --git a/lib/state/app_state.dart b/lib/state/app_state.dart index f6d4103..2049ae0 100644 --- a/lib/state/app_state.dart +++ b/lib/state/app_state.dart @@ -73,7 +73,12 @@ import '../sync/high_freq_wake_window.dart'; import '../sync/ios_bg_task.dart'; import '../sync/paired_device.dart'; import '../sync/sync_policy.dart' - show isLinkStale, StalenessTier, stalenessTierFor; + show + isLinkStale, + ReconnectSupervisorAction, + StalenessTier, + stalenessTierFor, + superviseReconnect; import '../sync/update_service.dart'; import '../telemetry/telemetry_service.dart'; import '../telemetry/health_uploader.dart'; @@ -173,6 +178,16 @@ class AppState extends ChangeNotifier { bool _keepAlive = false; bool _reconnecting = false; + + /// When the current reconnect loop started, for the supervisor's staleness + /// check (issue #208) — an await inside the loop that never returns leaves + /// `_reconnecting` true forever, which no re-trigger can clear. + DateTime? _reconnectingSince; + + /// Level-triggered reconnect supervision. The loop's only trigger used to be + /// the `connected → disconnected` edge, so any abandoned loop was permanent. + /// This ticks regardless of edges and re-arms — see [superviseReconnect]. + Timer? _reconnectSupervisor; Timer? _backfillTimer; String _prevConn = 'disconnected'; // Last battery snapshot pushed to the Band Battery widget — so we only reload @@ -449,8 +464,19 @@ class AppState extends ChangeNotifier { phoneStepsEnabled = ok; notifyListeners(); // The user just asked for this, so pull the full backfill window rather - // than the cheap routine one. - if (ok) unawaited(syncPhoneSteps(days: PhonePedometer.fullSyncDays)); + // than the cheap routine one — and then RE-DERIVE, symmetric with + // [disablePhoneSteps]. Banking rows into `live_coverage` changes nothing a + // screen can see: they all read scalars persisted in `day_result`/ + // `metric_series`, and the only automatic derive is drain-triggered. Grant + // the permission with the band not connected and, without this, the step + // tile keeps showing a dash indefinitely — indistinguishable from the + // feature not working. + if (ok) { + unawaited(() async { + await syncPhoneSteps(days: PhonePedometer.fullSyncDays); + await _reanalyzeForOverride(); + }()); + } return ok; } @@ -832,6 +858,12 @@ class AppState extends ChangeNotifier { // nothing new and keeps both signals consistent with each other. isForegroundActive: () => !_background, ); + // Seed the engine's link-power state (issue #200). `setBackground` is + // otherwise only called on TRANSITIONS, and a headless start begins + // backgrounded — without this the very case that most needs the cheap + // connection interval would run at the fast one until the user next + // foregrounded the app. + engine.setBackground(_background); repo = LocalRepositoryImpl(getProfileMap: () => user); // iOS BGProcessing/BGAppRefresh wakes while the FOREGROUND app owns the band // skip the headless BLE path (it would fight FBP for the peripheral) — route @@ -907,6 +939,8 @@ class AppState extends ChangeNotifier { // ChangeNotifier (which throws in release). _tapSub?.cancel(); _stopBackfillTimer(); + _reconnectSupervisor?.cancel(); + _reconnectSupervisor = null; _alarmGraceTimer?.cancel(); _alarmGraceTimer = null; _spotTimer?.cancel(); @@ -1018,6 +1052,20 @@ class AppState extends ChangeNotifier { }, )); TelemetryService.instance.breadcrumb('derive: $mode done'); + // The drain that triggered this pass may have landed the 1 Hz window of a + // workout the app slept through, whose strain/calories were scored from + // whatever few minutes the foreground tally saw (issue #206). Re-score + // recent sessions against the substrate now that it is here, so the + // workout LIST is corrected too and not just a detail screen someone + // happens to open. Monotone and idempotent — see reconcileSessionScore. + try { + final fixed = await repo?.rescoreRecentSessions() ?? 0; + if (fixed > 0) { + _log('[derive] rescored $fixed session(s) from substrate'); + } + } catch (e) { + _log('[derive] session rescore failed: $e'); + } await LocalDb.refreshComputeFreshness(); _bumpInsightsRevision(); notifyListeners(); // screens re-fetch from the derived store @@ -1592,6 +1640,7 @@ class AppState extends ChangeNotifier { if (isPaired) { if (_background) { _keepAlive = true; + _startReconnectSupervisor(); if (Platform.isAndroid) EdgeTracking.start(); if (Platform.isIOS) { IosBleRestore.foregroundActive = true; @@ -1784,6 +1833,9 @@ class AppState extends ChangeNotifier { /// On Android the Edge Tracking foreground service keeps the process + connection alive. Future pauseForBackground() async { _background = true; + // Step the Android link down to a power-saving connection interval — see + // `desiredLinkPriority` (issue #200). + engine.setBackground(true); // Defer derivation while backgrounded — running the heavy derive pass on a // short background BLE wake gets the app killed (iOS CPU watchdog / jetsam). // Capture keeps running; queued derive jobs drain on foreground return. @@ -1982,10 +2034,31 @@ class AppState extends ChangeNotifier { // the live-session screen shows steps FOR THIS WORKOUT (not since connection). int? _workoutRawBase; + /// 100 Hz sample count at the moment the active workout started, so + /// [workoutStepsMeasured] can tell "you did not move" apart from "the band + /// never sent us anything to count". + int? _workoutSampleBase; + /// Steps taken since the active workout started (real, live, gain-applied). /// 0 when no workout is running. This is what the workout screen shows. - int get workoutSteps { - if (activeWorkout == null || _workoutRawBase == null) return 0; + int get workoutSteps => workoutStepsMeasured ?? 0; + + /// Steps for the active workout, or NULL when nothing gait-capable was ever + /// measured for it (issue #183). + /// + /// The live count needs the band's 100 Hz accel stream. That stream is + /// routinely absent even during a perfectly good workout: the sticky + /// standard-HR fallback suppresses it, the background downgrade turns it off, + /// and a pocketed phone can drop it entirely — while GPS distance and the + /// 1 Hz HR keep flowing. Reporting `0` in that state is a fabricated + /// measurement, and it is what the issue screenshotted: a mile walked, HR and + /// distance both right, "0 STEPS" beside them. + int? get workoutStepsMeasured { + if (activeWorkout == null || _workoutRawBase == null) return null; + // No accel sample has reached us since this workout began — nothing was + // counted, as opposed to zero steps having been counted. + final base = _workoutSampleBase; + if (base == null || _liveSamples <= base) return null; final raw = _liveRaw - _workoutRawBase!; return raw > 0 ? (raw * ana.StepParams.gain).round() : 0; } @@ -2383,6 +2456,59 @@ class AppState extends ChangeNotifier { notifyListeners(); } + /// Cadence of the reconnect supervisor. Cheap — the tick reads local flags + /// and does nothing at all unless the app is paired, wants a link, and does + /// not have one. + static const Duration _reconnectSupervisorInterval = Duration(minutes: 1); + + /// Start the level-triggered reconnect supervision (issue #208). + /// + /// Deliberately NOT tied to connection state: it must keep ticking precisely + /// when everything else has given up. It is the backstop for the failure the + /// issue describes — a reconnect loop abandoned by a throw (or wedged on an + /// await that never returns), after which the app sits at 'disconnected' with + /// no edge left to re-trigger it and, on Android, a foreground service making + /// sure the process never restarts to clear the state. + void _startReconnectSupervisor() { + _reconnectSupervisor ??= Timer.periodic( + _reconnectSupervisorInterval, + (_) => _superviseReconnect(), + ); + } + + void _superviseReconnect() { + if (_disposed) return; + // Expire a bond-refusal pause whose cooldown has run out before deciding — + // otherwise the supervisor faithfully observes a flag that nothing can ever + // clear (issue #208). + engine.refreshAutoReconnectPause(); + final action = superviseReconnect( + paired: paired != null, + keepAlive: _keepAlive, + connected: engine.isConnected, + loopRunning: _reconnecting, + autoReconnectPaused: device.autoReconnectPaused, + loopRunningFor: _reconnectingSince == null + ? null + : DateTime.now().difference(_reconnectingSince!), + ); + switch (action) { + case ReconnectSupervisorAction.none: + return; + case ReconnectSupervisorAction.start: + _log('[RECONNECT] supervisor: disconnected with no loop running — ' + 'starting one.'); + unawaited(_reconnect()); + case ReconnectSupervisorAction.restartStale: + _log('[RECONNECT] supervisor: the loop has been running since ' + '$_reconnectingSince with no link — treating it as wedged and ' + 'starting a fresh one.'); + _reconnecting = false; + _reconnectingSince = null; + unawaited(_reconnect()); + } + } + void _startBackfillTimer() { if (!_keepAlive || paired == null || !engine.isConnected) return; _backfillTimer ??= Timer.periodic(_backfillInterval, (_) { @@ -2398,6 +2524,18 @@ class AppState extends ChangeNotifier { Future _runPeriodicBackfill() async { if (!_keepAlive || paired == null || busy || _reconnecting) return; if (!engine.isConnected) return; + // BACKGROUND: leave periodic offloads to the engine's own timer, which is + // floored by `BackfillPolicy` (900 s + an empty-streak backoff). This timer + // runs every 10 minutes and drives `requestHistorySync()`, whose `manual` + // trigger is deliberately NEVER floored — so backgrounded, the two together + // meant a radio-waking offload round roughly every ten minutes all day and + // all night, bypassing the very rate limit written to prevent that (issue + // #200). Foreground keeps the faster cadence: the user can see the data. + if (_background) { + _log('Periodic history refresh skipped — backgrounded; the engine\'s ' + 'floored 15-min backfill owns this.'); + return; + } if (_syncBurst != null) { _log('Periodic history refresh skipped — a sync burst is already running.'); return; @@ -2867,6 +3005,7 @@ class AppState extends ChangeNotifier { // background): don't tear it down and reconnect — just reclaim ownership. final wasBackground = _background; _background = false; + engine.setBackground(false); // Back in the foreground with an OS CPU/memory budget again — let the // scheduler drain any derive jobs that queued (durably) while backgrounded. _deriveScheduler.setBackground(false); @@ -2913,6 +3052,9 @@ class AppState extends ChangeNotifier { _setBusy(true); lastError = null; _keepAlive = true; + // From here on we WANT a link for the life of the process, so the level- + // triggered supervisor runs from here on too (issue #208). + _startReconnectSupervisor(); try { // INSIDE the guard, and no `paired!`. This block used to sit BETWEEN // _setBusy(true) and the try, force-unwrapping `paired`. The resume path @@ -3017,6 +3159,7 @@ class AppState extends ChangeNotifier { return; } _reconnecting = true; + _reconnectingSince = DateTime.now(); BandOwnership.markForegroundIntent(true); _log('[OWNERSHIP] reconnect intent on (${BandOwnership.debugState})'); try { @@ -3032,6 +3175,16 @@ class AppState extends ChangeNotifier { // connecting-style state instead of flat 'disconnected'. engine.markReconnecting(); var connected = false; + // PER-ATTEMPT containment (issue #208). Everything below can throw — + // `_ensureForegroundLease`, `_claimBand`/teardown inside connect, the + // post-connect stream setup. This whole loop used to sit inside ONE + // try/catch, so a single throw abandoned it permanently: the engine + // settles on 'disconnected', and the `connected → disconnected` edge + // that is the loop's only trigger can never fire again. On Android the + // foreground service then keeps the process alive forever, so nothing + // ever cleared it — the band never reconnected until the user forgot + // and re-paired it. A failed attempt is now just a failed attempt. + try { // ANDROID OS-MANAGED FALLBACK: once direct attempts keep failing — or // while backgrounded, where the process can be frozen between our Dart // backoff timers — arm a flutter_blue_plus autoConnect pending connect @@ -3103,11 +3256,14 @@ class AppState extends ChangeNotifier { }), ); _startBackfillTimer(); - break; + break; + } + } catch (e) { + _log('Reconnect attempt $attempt failed: $e — retrying.'); } } } catch (e) { - _log('Reconnect failed: $e'); + _log('Reconnect loop aborted: $e'); } finally { // this used to only check !_keepAlive, but the while loop above can // ALSO exit because device.autoReconnectPaused flipped true mid-loop @@ -3120,6 +3276,7 @@ class AppState extends ChangeNotifier { _log('[OWNERSHIP] reconnect intent off (${BandOwnership.debugState})'); } _reconnecting = false; + _reconnectingSince = null; // If we gave up (keepAlive dropped / never connected), stop advertising // `reconnecting` — fall back to a truthful 'disconnected'. No-op when // the loop exited via a successful connect (phase is `listening`). @@ -3597,6 +3754,7 @@ class AppState extends ChangeNotifier { unawaited(engine.retryFullLiveStreams()); } _workoutRawBase = _liveRaw; + _workoutSampleBase = _liveSamples; // A first night may have been derived since init. This read finishes // after the session below is constructed, so it back-fills the anchor on // `activeWorkout` when it lands rather than blocking the start. @@ -3803,6 +3961,7 @@ class AppState extends ChangeNotifier { // snapshot: steps count from zero going forward, same as // calories/strain/zone-minutes already (honestly) do here. _workoutRawBase = _liveRaw; + _workoutSampleBase = _liveSamples; // A first night may have been derived since init. This read finishes // after the session below is constructed, so it back-fills the anchor on // `activeWorkout` when it lands rather than blocking the start. @@ -3893,6 +4052,7 @@ class AppState extends ChangeNotifier { } activeWorkout = null; _workoutRawBase = null; + _workoutSampleBase = null; notifyListeners(); _log('Live session ended. Burned $finalKcal kcal.'); LiveActivity.end(); @@ -3925,6 +4085,7 @@ class AppState extends ChangeNotifier { _deriveScheduler.setWorkoutActive(false); activeWorkout = null; _workoutRawBase = null; + _workoutSampleBase = null; LiveActivity.end(); } diff --git a/lib/ui/activity/live_session_screen.dart b/lib/ui/activity/live_session_screen.dart index d160cfc..9c8980e 100644 --- a/lib/ui/activity/live_session_screen.dart +++ b/lib/ui/activity/live_session_screen.dart @@ -1388,7 +1388,7 @@ class _WorkoutFinishScreenState extends State duration: s.duration, when: DateTime.now(), maxHr: _maxHr, - strain: (d?['strain'] as num?)?.toDouble() ?? s.strain ?? 0, + strain: (d?['strain'] as num?)?.toDouble() ?? s.strain, calories: (d?['calories'] as num?)?.toInt() ?? s.calories.round(), route: _route, avgHr: (d?['avg_hr'] as num?)?.toInt(), @@ -2449,7 +2449,11 @@ class _SessionSheet extends StatelessWidget { final zone = zoneIndex.clamp(0, 5); final zoneColor = AppColors.zoneOnDark(zone); final isRoute = distance != null; - final steps = context.select((a) => a.workoutSteps); + // Nullable: the band's 100 Hz accel stream is routinely absent during a + // perfectly good workout (standard-HR fallback, background downgrade), and + // printing a confident "0 STEPS" next to a real distance and a real HR is a + // fabricated measurement — issue #183 screenshotted exactly that. + final steps = context.select((a) => a.workoutStepsMeasured); return Container( decoration: BoxDecoration( @@ -2511,7 +2515,9 @@ class _SessionSheet extends StatelessWidget { ), Expanded( child: _SheetStat( - isRoute ? '${workout.calories.round()}' : '$steps', + isRoute + ? '${workout.calories.round()}' + : (steps?.toString() ?? '—'), isRoute ? 'KCAL' : 'STEPS', ), ), From a22ed41ca8ba109219253151ad2ea4d4904a0041 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 11:30:40 +0530 Subject: [PATCH 05/12] handle the files people actually pick on the import screen both import buttons fed whatever you picked straight into a utf8 decoder. a WHOOP 'my data' export is a zip of csvs and a .noopbak is a zip around noop's sqlite db, so you got 'FormatException: Unexpected extension byte (at offset 10)' - offset 10 being the first byte of a zip header with its high bit set. now: WHOOP export zips are unpacked and imported, a .noopbak tells you to export the raw sensor csv instead, and a file we can't read says so instead of quoting a byte offset. an export we can read but don't recognise (wrong language, wrong file) now fails with the columns it found rather than cheerfully reporting 'imported 0 days'. --- lib/import/import_container.dart | 203 +++++++++++++++++++++++++++++++ lib/import/noop_import.dart | 55 ++++++++- lib/import/whoop_import.dart | 36 +++++- pubspec.lock | 2 +- pubspec.yaml | 6 + test/import_container_test.dart | Bin 0 -> 7382 bytes 6 files changed, 296 insertions(+), 6 deletions(-) create mode 100644 lib/import/import_container.dart create mode 100644 test/import_container_test.dart diff --git a/lib/import/import_container.dart b/lib/import/import_container.dart new file mode 100644 index 0000000..24bb04c --- /dev/null +++ b/lib/import/import_container.dart @@ -0,0 +1,203 @@ +// import_container.dart — what did the user actually hand us? +// +// WHY THIS EXISTS (issues #199, #160) +// +// Every importer took the path from a `FileType.any` picker and piped it +// straight into `utf8.decoder`. Users picked the file their OTHER app told them +// to export — a WHOOP "My Data" export (a ZIP of CSVs) or a NOOP full backup +// (`.noopbak`, a ZIP holding `noop-backup.sqlite`) — and got: +// +// FormatException: Unexpected extension byte (at offset 10) +// FormatException: Invalid UTF-8 byte (at offset 10) +// +// Offset 10 is not a coincidence and it is not an encoding problem. A ZIP's +// first ten bytes (`PK\x03\x04`, version, flags, method) are all < 0x80, so the +// UTF-8 decoder always survives exactly that far and then hits byte 10 — the low +// byte of the DOS modification time, the first byte in the file that can have +// its high bit set. Byte 18 (the compressed size) is the next such field, which +// is where the other reported offset comes from. Both reports are ZIPs. +// +// (A genuinely mis-encoded CSV — latin1/cp1252, a Spanish or French export — +// fails differently: "Missing extension byte". None of the reports show that, +// so the "it's a localized CSV" theory does not explain them. Lenient decoding +// is still applied below, but as a separate, smaller fix.) +// +// So: sniff the container before decoding. A ZIP of CSVs is unwrapped and +// imported for real; anything we cannot use gets a message naming the file we +// DO want, instead of a byte offset. + +import 'dart:io'; + +import 'package:archive/archive.dart'; +import 'package:path/path.dart' as p; + +/// What the first bytes of the picked file say it is. +enum ImportContainer { + /// Plain text — decode and parse it. + text, + + /// PKZIP. A WHOOP export, or a `.noopbak`. + zip, + + /// A raw SQLite database (a `noop-backup.sqlite` extracted by hand, say). + sqlite, + + /// gzip — not a container we unwrap, but worth naming precisely. + gzip, + + /// Binary of some other kind. + binary, +} + +/// An import that failed for a reason the user can act on. Distinct from a +/// `FormatException` so the UI can show guidance rather than a byte offset. +class ImportFormatException implements Exception { + const ImportFormatException(this.message); + final String message; + @override + String toString() => message; +} + +/// Classify a file from its leading bytes. Pure — [head] is the first ~16 bytes. +/// +/// Magic numbers: `PK\x03\x04` (and the empty/spanned variants `PK\x05\x06`, +/// `PK\x07\x08`) for ZIP; `SQLite format 3\x00` for SQLite; `\x1f\x8b` for gzip. +/// Everything else is called text unless it holds a NUL or a run of control +/// bytes, which no CSV export contains. +ImportContainer sniffImportContainer(List head) { + if (head.length >= 4 && + head[0] == 0x50 && + head[1] == 0x4B && + (head[2] == 0x03 || head[2] == 0x05 || head[2] == 0x07)) { + return ImportContainer.zip; + } + const sqliteMagic = 'SQLite format 3'; + if (head.length >= sqliteMagic.length && + String.fromCharCodes(head.take(sqliteMagic.length)) == sqliteMagic) { + return ImportContainer.sqlite; + } + if (head.length >= 2 && head[0] == 0x1F && head[1] == 0x8B) { + return ImportContainer.gzip; + } + for (final b in head) { + // NUL, or a control byte that is not tab/LF/CR — not a CSV. + if (b == 0x00 || (b < 0x09) || (b > 0x0D && b < 0x20)) { + return ImportContainer.binary; + } + } + return ImportContainer.text; +} + +/// Read enough of [path] to classify it. +Future sniffFile(String path) async { + final f = File(path); + final raf = await f.open(); + try { + return sniffImportContainer(await raf.read(16)); + } finally { + await raf.close(); + } +} + +/// True for a ZIP member we can actually parse as an export. +bool _isCsvMember(String name) { + final base = p.basename(name).toLowerCase(); + // `__MACOSX/._foo.csv` resource forks are AppleDouble binaries, not CSVs. + return base.endsWith('.csv') && + !base.startsWith('._') && + !name.startsWith('__MACOSX/'); +} + +/// Resolve the picked paths into CSV files on disk, unwrapping ZIP archives. +/// +/// [flavor] names the importer in error messages ('NOOP', 'WHOOP'). Extracted +/// members are written to a temp directory — the caller reads them and the OS +/// reclaims them; nothing is copied into app storage. +/// +/// Throws [ImportFormatException] with actionable guidance for anything we +/// cannot parse: a database, an archive of databases, a gzip, binary junk. +Future> resolveImportCsvPaths( + List paths, { + required String flavor, +}) async { + final out = []; + for (final path in paths) { + final kind = await sniffFile(path); + switch (kind) { + case ImportContainer.text: + out.add(path); + case ImportContainer.zip: + out.addAll(await _extractCsvMembers(path, flavor: flavor)); + case ImportContainer.sqlite: + throw ImportFormatException( + '“${p.basename(path)}” is a database file, not a $flavor CSV ' + 'export. In NOOP, use Export → raw sensor CSV and pick the ' + '“noop-raw-sensors-….csv” file it writes.', + ); + case ImportContainer.gzip: + throw ImportFormatException( + '“${p.basename(path)}” is a gzip archive. Unzip it first and pick ' + 'the CSV inside.', + ); + case ImportContainer.binary: + throw ImportFormatException( + '“${p.basename(path)}” is not a text file, so there is nothing to ' + 'read as a $flavor CSV export.', + ); + } + } + return out; +} + +Future> _extractCsvMembers( + String path, { + required String flavor, +}) async { + final name = p.basename(path); + final Archive archive; + try { + archive = ZipDecoder().decodeBytes(await File(path).readAsBytes()); + } catch (e) { + throw ImportFormatException( + 'Could not read “$name” as an archive: $e', + ); + } + + final csvFiles = [ + for (final f in archive.files) + if (f.isFile && _isCsvMember(f.name)) f, + ]; + + if (csvFiles.isEmpty) { + // The `.noopbak` case, and the single most-reported one: an archive whose + // payload is a SQLite database. Name the file we actually want rather than + // failing on its bytes. + final hasDb = archive.files.any( + (f) => + f.isFile && + (f.name.toLowerCase().endsWith('.sqlite') || + f.name.toLowerCase().endsWith('.db')), + ); + if (hasDb) { + throw ImportFormatException( + '“$name” is a full NOOP backup — it holds NOOP\'s own database, which ' + 'we can\'t read. In NOOP, open Export and choose the raw 1 Hz sensor ' + 'CSV (“noop-raw-sensors-….csv”), then import that file here.', + ); + } + throw ImportFormatException( + '“$name” is an archive with no CSV files inside ' + '(${archive.files.length} entr${archive.files.length == 1 ? 'y' : 'ies'}). ' + 'Pick the $flavor CSV export instead.', + ); + } + + final dir = await Directory.systemTemp.createTemp('openstrap_import_'); + final out = []; + for (final f in csvFiles) { + final dest = File(p.join(dir.path, p.basename(f.name))); + await dest.writeAsBytes(f.content as List); + out.add(dest.path); + } + return out; +} diff --git a/lib/import/noop_import.dart b/lib/import/noop_import.dart index 627508e..e7e5034 100644 --- a/lib/import/noop_import.dart +++ b/lib/import/noop_import.dart @@ -44,6 +44,7 @@ import '../compute/derivation_engine.dart'; import '../compute/profile.dart'; import '../compute/substrate.dart'; import '../data/db.dart'; +import 'import_container.dart'; class NoopImportResult { final int days; @@ -114,10 +115,28 @@ class NoopImporter { DerivationEngine engine, { void Function(int days)? onProgress, }) async { - final file = File(path); + var file = File(path); if (!await file.exists()) { throw const FileSystemException('CSV not found'); } + // What did the user actually pick? A `.noopbak` is a ZIP around NOOP's + // SQLite database, and feeding its bytes to `utf8.decoder` is what produced + // the "Invalid UTF-8 byte (at offset 10)" in issues #160/#199. Resolve the + // container first: a ZIP of CSVs is unwrapped, and anything unusable throws + // an [ImportFormatException] naming the file we DO want. + final resolved = await resolveImportCsvPaths([path], flavor: 'NOOP'); + if (resolved.isEmpty) { + throw const ImportFormatException( + 'That archive holds no NOOP CSV export.', + ); + } + // A NOOP raw-sensor export is a single CSV; if an archive carried several, + // prefer one that actually looks like the raw-sensor file. + final chosen = resolved.firstWhere( + (p) => p.toLowerCase().contains('raw-sensor'), + orElse: () => resolved.first, + ); + file = File(chosen); // Rolling buffer: keeps at most the CURRENT + PREVIOUS local date of samples. final secs = {}; // ts(sec) → channels @@ -168,11 +187,21 @@ class NoopImporter { return (i != null && i < f.length) ? f[i] : ''; } - final lines = - file.openRead().transform(utf8.decoder).transform(const LineSplitter()); + // `allowMalformed` — a CSV exported under a non-UTF-8 locale should import + // with a mangled character in a column we don't read, not abort the whole + // file. (This is NOT what issues #160/#199 hit; those were ZIPs, handled + // above. It is the smaller, real second-order problem underneath them.) + final lines = file + .openRead() + .transform(const Utf8Decoder(allowMalformed: true)) + .transform(const LineSplitter()); + var sawHeader = false; + String? firstLine; await for (final line in lines) { if (line.isEmpty || line.startsWith('#')) continue; + firstLine ??= line; if (line.startsWith('unix_s,')) { + sawHeader = true; // Header → (re)build the name→index map and skip. final h = line.split(','); col = {for (var i = 0; i < h.length; i++) h[i].trim(): i}; @@ -269,6 +298,26 @@ class NoopImporter { stepsBanked += await _flushStepCoverage(e.value, e.key); } + // A file we could read but could not USE is a failure, not a "0 days" + // success. Without a recognised header the positional fallback silently + // misparses (it is the pre-drift layout), and a localized or unrelated CSV + // simply drops every row — both used to end at "NOOP: imported 0 days", + // which reads as "the app is broken" with nothing to act on. + if (totalRows == 0) { + final head = firstLine ?? ''; + final preview = head.isEmpty + ? '' + : ' (first line: "${head.length > 80 ? '${head.substring(0, 80)}…' : head}")'; + throw ImportFormatException( + sawHeader + ? 'That NOOP export has a header we recognise but no rows we could ' + 'read — every row was empty or out of range.' + : 'That file does not look like a NOOP raw-sensor export: no ' + '"unix_s,…" header row was found$preview. In NOOP, use ' + 'Export → raw sensor CSV.', + ); + } + await engine.finalizeImport(profile); return NoopImportResult(daysDone, totalRows, lateRows, stepsBanked); } diff --git a/lib/import/whoop_import.dart b/lib/import/whoop_import.dart index eb8288f..cda97c8 100644 --- a/lib/import/whoop_import.dart +++ b/lib/import/whoop_import.dart @@ -18,6 +18,7 @@ import '../compute/derivation_engine.dart' show kAlgoVersion, DerivationEngine; import '../compute/profile.dart'; import '../compute/substrate.dart' show localDateLabel; import '../data/db.dart'; +import 'import_container.dart'; class WhoopImportResult { final int days; @@ -84,7 +85,14 @@ class WhoopImporter { } catch (_) { rawDays = const {}; } - for (final path in paths) { + // WHOOP's own "My Data" export arrives as a ZIP of CSVs, and users pick the + // ZIP — its bytes hit `utf8.decoder` and threw "Unexpected extension byte + // (at offset 10)" (issue #199). Unwrap it first; anything we can't parse + // throws an actionable [ImportFormatException] instead. + final csvPaths = await resolveImportCsvPaths(paths, flavor: 'WHOOP'); + var recognisedFiles = 0; + final headersSeen = []; + for (final path in csvPaths) { final rows = await _readCsv(path); if (rows.length < 2) continue; final header = rows.first; @@ -92,6 +100,11 @@ class WhoopImporter { for (var i = 0; i < header.length; i++) header[i].trim().toLowerCase(): i }; final kind = _classify(col); + if (kind == _Kind.unknown) { + headersSeen.add(header.take(6).join(', ')); + continue; + } + recognisedFiles++; for (var r = 1; r < rows.length; r++) { final f = rows[r]; if (f.isEmpty) continue; @@ -112,6 +125,23 @@ class WhoopImporter { } } } + // Nothing recognised is a failure, not a "0 days" success. The columns are + // matched against exact ENGLISH header names, so a WHOOP export downloaded + // in another language classifies as unknown for every file and used to end + // silently at "WHOOP: imported 0 days" — reported as the app being broken. + if (recognisedFiles == 0) { + throw ImportFormatException( + csvPaths.isEmpty + ? 'No CSV files were found to import.' + : 'None of those files look like a WHOOP export. We match the ' + 'English column names WHOOP writes (e.g. "Recovery score %", ' + '"Activity name", "Sleep onset"), so an export downloaded in ' + 'another language will not be recognised — re-download it ' + 'with WHOOP set to English.' + '${headersSeen.isEmpty ? '' : ' Columns found: ${headersSeen.first}.'}', + ); + } + if (engine != null && profile != null) { await engine.finalizeImport(profile); } @@ -364,7 +394,9 @@ class WhoopImporter { static Future>> _readCsv(String path) async { final lines = File(path) .openRead() - .transform(utf8.decoder) + // Lenient: a WHOOP export saved under a non-UTF-8 locale should lose a + // character, not the whole import. + .transform(const Utf8Decoder(allowMalformed: true)) .transform(const LineSplitter()); final out = >[]; await for (final line in lines) { diff --git a/pubspec.lock b/pubspec.lock index 78feaf7..a8d19ef 100644 --- a/pubspec.lock +++ b/pubspec.lock @@ -34,7 +34,7 @@ packages: source: hosted version: "5.3.1" archive: - dependency: transitive + dependency: "direct main" description: name: archive sha256: a96e8b390886ee8abb49b7bd3ac8df6f451c621619f52a26e815fdcf568959ff diff --git a/pubspec.yaml b/pubspec.yaml index 2eccdf5..3df2b9a 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -89,6 +89,12 @@ dependencies: path: ^1.9.0 path_provider: ^2.1.5 + # Unwrapping the archives users actually pick on the import screen: a WHOOP + # "My Data" export is a ZIP of CSVs, and a NOOP `.noopbak` is a ZIP holding a + # SQLite database (issues #199, #160). Already present transitively; declared + # directly because `lib/import/import_container.dart` imports it. + archive: ^4.0.9 + # Background scheduled derivation (the heavy nightly pass: sleep staging + # 24-h spectra). Android: a real OS-scheduled WorkManager job. iOS: see the # honest caveat in lib/compute/background_derivation.dart — app-killed heavy diff --git a/test/import_container_test.dart b/test/import_container_test.dart new file mode 100644 index 0000000000000000000000000000000000000000..71576a465e83db6cebf6e20664052afbfbb3eba3 GIT binary patch literal 7382 zcmds6>u%e~72f{!QyjPpkOE{{%GL2^1IRbTsIyMvq}y&1C?j%cazl|^&d`z)80bUv zHTo2NlRinmGec4%S z$4Y7%4tgIT|HEm1C9*W4H?L22_BO5xAc$qXP9#M#OjUZtZ#2}PRFO_Pp#hEPr_-07 zqByOTCY)^~D)L|c>D_#AAO9cpSr+VJt@;#a;!^c#Ds@ssJrb29k!AFh2J^i^pHxAj zQ4L2Ik*x6bOe_?sy1IfQ%33Kxoh_^?v8ZSw;{uYBRO>Xqv{nrV^zXm_18XE@#mqh{ zV)Oav{DP)ML^_>Hww6s*66!0hXi}$H1T9`2kIvXS5;;C9B@v2AffytrgC6_#bQWJ= zJ?l!P&57^q2OTrlxd#Js84yJ zSsqTWr!wmGjV>ktyd{@2Qh6H36lPeG#wqfMwXO46B}z!rl;%oH5d~XYO*VOvsPukV zg$Z20+q?!W!P(g^i?dp5S&i9^yY7eEHWVdtRYTLU zj4tJ_Rj_#(LqlX-E~|F;)>*3b(=^x5X#6QH&tw0ED4$;F3K8#- zWMwWggz|eDSrh!;cX-^a36L|#B20Wg7gO0saHECY*Y4yRQrKPVcrTDS$JjTkSPGE{ zL=>H%IQ>SvdEW@*twAR9OPxS8f9jdTug&jD>blD5r?fmaVk^Jb9w4ybdTZ;dKypms z0pIGB3DomZik#8~ut85tM2ZNd{igJ>MyN#|8mo!Fgw|pvkO1A5KrN68)7NrZ24N*3 zj^Fy8b^FraCVpejQ5FeQ9Tl^D{g^Z+A4dgpj>xDBobUIbh&h&@M>3OI`cSz>^3in5QoHUU}%S7sKE?CtDIYK>u}o@MFcn)0nkG1kCA$mpt>0) zSsf!4wL!WlGV4tSfqin&xP@Yv$NwPi9&mRBmT9lgkdzx-TER*>HjRlr*J5s`=29SB z`(x=WXY~-@VIB5hMbpgyz%oML zwdb+S0X)~OGCsZSOWOeV(O0odOJ~`W5zBadXijpyPxN%BHGT+1lJqj62}&Fo;Ou`m z$28N;u_mf zDtx3Rhn%iEoUR5vpd$mr;j2S(!u4Qh__(7ihJZFCYALX(eghJQ^hFXhB2om-%Muyl znFAUKWCGY>M+>a8gqdSoa?nxFicLTQN*2d}qP$Hf&{~j6CRoG0USQNq!KSNJV9Cr8 zSTX5?J}|P#Tb^q1ACGbq3x|cbi1q6Q*ASo2UYRVwr?cWRMI#u8i!cM*2-TIhPoC&c z-sQvoy?42H?PhpeoSQ4MOZi)JivUw;KIAlPdezbC$;t7n;~!q1{ml?}gqE%2MCS6C z)GJNUw3_Xh$pG@|U;h-_2y`p7CFY+P+g(@ z+xIPl^6EUDk5%6}taV6Z++Dz}lizvfe}!*LWP5>r$H~2pV=f*#>UM9Pn0BbqkQ*~V zoSI4Vuw^qYDp+W71`IB;4#$8rW}*RQsR6?aU|;yr-G0;4n-285#1?H9&sURoO~U{z zr#1T4m~K32rSo0<`3#JckqkpM-T%gR!p?0(>&L2JGHkf*-Y!)o3Wf zd*_s!=Ul?Rudu5rfK(OZT8Dk5(Z!~~0~Zd~P18uKHN)2X3mu%lF!+}j`OfI#@bnbi zb`AofQ_S#TQ3D4-METij4gh42cxY7g2BXwb!v>*NiYXO64;+@$8seKP0KD@|%VS7M zO?GlGTebm4;c1OK2gaKTQ^23iB?zBC!PvJ%i*;1JEl9^ENCzfJ2YA2Ek{X{1cB31- zJNWUAMX-iqu?!Qj3SwUyK+Gr8QZLNVz@9ZRH!j31Qwf!LuniBEitn;>$>k-9lgESu_j5SOEn zlf)(?+Ce_Zkv|jh(cDp3B5P*x87?#1MmPg0v`PJ&GjxJ`x4LW(abn$eNjE)_K+Jbl z@H55~PLj{tn2@&j?+>n>oycR>d8ZL=+ZeB2?|?u(XWNG4vR<~nKf_#6EoZnpX4wy@ z=B1BsB~=z5e(UJiOo(l=aF+hc+~_!y*DmuSuF=$3S+ZVpb>@}s+Dgo5NY6iE7S9#t zA|BSd!nBK5J~|(|x3^{6gfGe4IPsMafE)hL_;%FZ5%V=Y?k1z(nQt31W9h#y@}+jN zT-w@I=l@ZjEK`D8#zf#x1zhe+qdCA`7@fDbMbyO3Oj0HPVSy0@H;uT>0f8!NT#bEI zB$10dHVqZV>Ai;!A3eT57#LoCOIUBAi Date: Sat, 8 Aug 2026 11:31:11 +0530 Subject: [PATCH 06/12] stop holding the android link at high priority around the clock, and fix reconnect giving up for good battery: we asked for CONNECTION_PRIORITY_HIGH once at connect and never stepped back down, so an ~11ms connection interval was held 24/7 on a link that's meant to stay up forever - all night, with nothing to say. it now follows what's actually happening: high during an offload or a live workout, balanced when idle, low power in the background. the app-state backfill timer also ran every 10 min through the 'manual' path, which is deliberately not rate-limited, so it bypassed the 15-min floor while backgrounded; it defers to the engine's floored timer there now. band battery is polled every 5 min instead of every 30s. reconnect: the retry loop only ever started on a connected->disconnected transition and the whole loop sat in one try/catch, so a single throw inside it killed reconnect for the life of the process - which on android is forever, because the foreground service keeps the process alive. that's the 'take the band off for 10 minutes and it never comes back unless you re-pair' report. each attempt is now contained, and a supervisor checks every minute that we're actually trying (and restarts a loop that's been wedged too long). the bond-refusal pause had the same shape: it was only cleared inside the createBond success path, which the pause itself prevents from running. it expires after 30 min now. --- lib/ble/ble_engine.dart | 94 ++++++++++++++++- lib/sync/sync_policy.dart | 154 ++++++++++++++++++++++++++-- test/link_priority_policy_test.dart | 94 +++++++++++++++++ test/reconnect_supervisor_test.dart | 135 ++++++++++++++++++++++++ 4 files changed, 470 insertions(+), 7 deletions(-) create mode 100644 test/link_priority_policy_test.dart create mode 100644 test/reconnect_supervisor_test.dart diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index ccaa775..7019a55 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -603,6 +603,51 @@ class BleEngine { /// True while live is in the background HR-only downgrade. bool get liveHrOnly => _liveEnabled && _liveHrOnly; + + // ── link power (issue #200) ───────────────────────────────────────────────── + // Android's connection priority was requested ONCE at connect setup and never + // stepped back down, so an ~11.25 ms interval was held for the entire life of + // a deliberately-permanent connection. [desiredLinkPriority] decides what the + // link should be running at; [_applyLinkPriority] is the one place that talks + // to the radio, and it is a no-op when nothing changed. + bool _backgrounded = false; + LinkPriority? _appliedPriority; + + /// Told by AppState on every foreground/background transition. Drives the + /// connection interval — see [desiredLinkPriority]. + void setBackground(bool value) { + if (_backgrounded == value) return; + _backgrounded = value; + unawaited(_applyLinkPriority()); + } + + Future _applyLinkPriority() async { + if (!Platform.isAndroid) return; // iOS picks its own interval + final session = _session; + if (session == null || !session.connected) return; + final device = session.device; + final want = desiredLinkPriority( + offloadActive: _offloadActive, + background: _backgrounded, + hasLiveConsumer: _liveEnabled && !_liveHrOnly, + ); + if (want == _appliedPriority) return; + try { + await device.requestConnectionPriority( + connectionPriorityRequest: switch (want) { + LinkPriority.high => ConnectionPriority.high, + LinkPriority.balanced => ConnectionPriority.balanced, + LinkPriority.lowPower => ConnectionPriority.lowPower, + }, + ); + _appliedPriority = want; + _log('Link priority → ${want.name}.'); + } catch (e) { + // Leave `_appliedPriority` alone so the next transition retries. + _log('requestConnectionPriority(${want.name}) failed: $e'); + } + } + bool _offloadActive = false; final List _offloadFrames = []; bool _drainingOffloadFrames = false; @@ -632,6 +677,24 @@ class BleEngine { // caller pauses the auto-reconnect loop instead of pinning the radio forever. // A single successful bond clears it (see the createBond block below). final BondRefusalGiveUp _bondGiveUp = BondRefusalGiveUp(); + + /// Clear a bond-refusal auto-reconnect pause whose cooldown has expired, and + /// report whether the pause is still in force (issue #208). + /// + /// The pause was previously cleared in exactly ONE place: the `createBond()` + /// success branch. That branch is inside the connect path, which the pause + /// itself stops from ever running — so the flag latched for the life of the + /// process, and the Android foreground service made sure the process outlived + /// any reason for it. [BondRefusalGiveUp.stillPaused] expires it after a + /// cooldown; a band that genuinely will not bond simply re-trips. + bool refreshAutoReconnectPause() { + if (!state.autoReconnectPaused) return false; + if (_bondGiveUp.stillPaused(DateTime.now())) return true; + state.autoReconnectPaused = false; + _log('[RECONNECT] bond-refusal pause expired — auto-reconnect re-armed.'); + onState(state); + return false; + } // Real per-chunk failure tracking (see ChunkFailureLedger doc) — persists // across reconnects like marginal-radio/post-bond-loop/bond-give-up, since // the whole point is catching the SAME token failing across sessions. @@ -1096,11 +1159,17 @@ class BleEngine { } catch (e) { _log('requestMtu failed: $e — MTU stays at the connection default.'); } + // Start high: connect setup is immediately followed by INIT + the first + // flash drain, which is exactly when throughput matters. `_applyLinkPriority` + // steps it back down as soon as that offload ends (issue #200) — before + // this, `high` was requested here and then held for the entire life of a + // deliberately-permanent connection. if (Platform.isAndroid) { try { await device.requestConnectionPriority( connectionPriorityRequest: ConnectionPriority.high, ); + _appliedPriority = LinkPriority.high; } catch (_) {} } @@ -1290,9 +1359,20 @@ class BleEngine { } _send(Cmd.toggleRealtimeHr, const [0x01]); } - _send(Cmd.getBatteryLevel, const []); + // Battery is a DISPLAY value that moves over hours. Polling it on every + // 30 s keep-alive tick was 2,880 radio round-trips a day for a handful of + // real changes (issue #200). + final lastBattery = _lastBatteryPollAt; + if (lastBattery == null || + DateTime.now().difference(lastBattery).inSeconds >= + kBatteryPollIntervalSeconds) { + _lastBatteryPollAt = DateTime.now(); + _send(Cmd.getBatteryLevel, const []); + } } + DateTime? _lastBatteryPollAt; + /// Trigger a historical offload, floored by [BackfillPolicy] (manual / /// autoContinue are never floored). Re-arms the drain so a fresh HISTORY_COMPLETE /// is awaited. Used by the periodic timer, continuation, and the public sync API. @@ -2963,6 +3043,7 @@ class BleEngine { Future enableLiveStreams() async { _liveEnabled = true; _liveHrOnly = false; + unawaited(_applyLinkPriority()); // a live consumer earns the fast interval _armTime = DateTime.now(); // marginal-radio detector measures arm→drop latency await _send(Cmd.toggleRealtimeHr, const [0x01]); @@ -3010,6 +3091,7 @@ class BleEngine { if (_session?.connected != true) return; _liveEnabled = true; _liveHrOnly = true; + unawaited(_applyLinkPriority()); // downgraded to HR-only ⇒ step the link down await _send(Cmd.toggleRealtimeHr, const [0x01]); final offOps = >[ [ @@ -3066,6 +3148,7 @@ class BleEngine { } _liveEnabled = false; _liveHrOnly = false; + unawaited(_applyLinkPriority()); // no live consumer left _armTime = null; state.liveHr = null; // No phase change — we stay `listening`; only the live R10/R11/optical streams @@ -3102,6 +3185,12 @@ class BleEngine { final session = _session; if (session == null) return; session.intentionalClose = intentional; + // Per-link state: Android resets the connection interval on every new GATT + // connection, so a remembered priority would make the next link skip its + // request. The battery stamp resets too — a fresh session should read the + // level once rather than inheriting the last link's 5-minute cooldown. + _appliedPriority = null; + _lastBatteryPollAt = null; _drain?.onLinkDown(); _drain = null; // Fire a final derive for anything stored-but-not-yet-derived, then disarm the @@ -3133,6 +3222,9 @@ class BleEngine { void _setOffloadActive(bool active) { if (_offloadActive == active) return; _offloadActive = active; + // An offload is the one thing that genuinely needs the fast interval; as + // soon as it ends the link steps back down (issue #200). + unawaited(_applyLinkPriority()); onOffloadState?.call(active); } diff --git a/lib/sync/sync_policy.dart b/lib/sync/sync_policy.dart index abda54a..e5f59c7 100644 --- a/lib/sync/sync_policy.dart +++ b/lib/sync/sync_policy.dart @@ -464,20 +464,56 @@ class PostBondTimeoutLoopDetector { /// A single successful bond resets the streak. class BondRefusalGiveUp { final int giveUpThreshold; - BondRefusalGiveUp({this.giveUpThreshold = 5}); + + /// How long the auto-reconnect pause lasts before the loop is allowed to try + /// again (issue #208). + /// + /// The pause used to be permanent in practice: it is cleared ONLY inside the + /// `createBond()` success branch, which lives inside the connect path, which + /// the pause itself prevents from ever running again. A user who hit five + /// refusals — a transient stack/OEM condition, not necessarily a broken bond — + /// had auto-reconnect off for the life of the process, with the Android + /// foreground service making sure the process never restarted to clear it. + /// A cooldown keeps the intended behaviour (stop hammering a band that will + /// not bond) without the dead end. + final Duration cooldown; + + BondRefusalGiveUp({ + this.giveUpThreshold = 5, + this.cooldown = const Duration(minutes: 30), + }); int _consecutive = 0; bool gaveUp = false; + DateTime? _gaveUpAt; int get consecutive => _consecutive; + /// When the pause was entered, or null if not paused. + DateTime? get gaveUpAt => _gaveUpAt; + + /// Whether the auto-reconnect pause is still in force at [now]. Once the + /// cooldown expires the latch clears itself and the streak restarts, so a + /// band that still refuses simply re-trips after another [giveUpThreshold] + /// refusals rather than being retried forever. + bool stillPaused(DateTime now) { + if (!gaveUp) return false; + final since = _gaveUpAt; + if (since != null && now.difference(since) >= cooldown) { + reset(); + return false; + } + return true; + } + /// Feed a bond refusal/timeout. Returns true EXACTLY ONCE, on the call that /// crosses the threshold (the caller then pauses reconnect + surfaces the guide). - bool bondRefused() { + bool bondRefused({DateTime? now}) { if (gaveUp) return false; _consecutive++; if (_consecutive >= giveUpThreshold) { gaveUp = true; + _gaveUpAt = now ?? DateTime.now(); return true; } return false; @@ -485,14 +521,12 @@ class BondRefusalGiveUp { /// A bond that succeeded (or a session that got past bonding). Clears the /// streak AND the give-up latch so a later refusal run can trip again. - void bondSucceeded() { - _consecutive = 0; - gaveUp = false; - } + void bondSucceeded() => reset(); void reset() { _consecutive = 0; gaveUp = false; + _gaveUpAt = null; } } @@ -679,3 +713,111 @@ class NoDurableProgressEscalation { _gaveUp = false; } } + +// ── link power policy (issue #200) ─────────────────────────────────────────── + +/// How aggressively to run the Android GATT link right now. +/// +/// Android exposes three connection-interval presets. `high` is ~11.25 ms with +/// zero slave latency — the phone's controller services ~89 connection events a +/// second, and the host is woken for every one that carries data. +enum LinkPriority { high, balanced, lowPower } + +/// Battery poll cadence. The band's battery is a DISPLAY value that moves on the +/// order of hours; nothing in the sync path depends on it. It was being read on +/// every 30 s keep-alive tick — 2,880 radio round-trips a day for a number that +/// changes a few times. +const int kBatteryPollIntervalSeconds = 300; + +/// The connection priority the link should be running at. +/// +/// WHY THIS EXISTS (issue #200): the engine requested `high` once, at connect +/// setup, "for the drain" — and never stepped back down. Since the connection is +/// deliberately permanent (foreground service + START_STICKY + keep-alive +/// watchdog + CDM presence relaunch), that meant an ~11.25 ms interval held 24/7, +/// including all night with nothing to say. The app also steers users into a +/// battery-optimization exemption, so Doze never damps it either. That +/// configuration — not the timers the reporter suspected — is the dominant +/// drain. +/// +/// The rule: pay for a fast interval only while something is actually consuming +/// the link. +/// • an offload in flight → `high` (throughput is the whole point), +/// • a live consumer in the foreground (workout, spot check, breathing) → +/// `high`; the 100 Hz streams need the bandwidth, +/// • foreground, idle → `balanced`, +/// • background with no live consumer → `lowPower`. +/// +/// SAFETY: this changes throughput, never correctness. The drain is +/// commit-before-ACK and resumes from a durable cursor, so a slower interval can +/// only make an offload take longer — and an offload always raises the priority +/// back to `high` first. The one real constraint is the liveness fuse +/// ([kLivenessFuseSeconds]): whatever interval we sit at must still let the 1 Hz +/// notify or the keep-alive response arrive inside 120 s, which `lowPower` +/// (~500 ms interval) does with three orders of magnitude to spare. +LinkPriority desiredLinkPriority({ + required bool offloadActive, + required bool background, + required bool hasLiveConsumer, +}) { + if (offloadActive) return LinkPriority.high; + if (hasLiveConsumer && !background) return LinkPriority.high; + return background ? LinkPriority.lowPower : LinkPriority.balanced; +} + +// ── reconnect supervision (issue #208) ─────────────────────────────────────── + +/// Why the supervisor decided to act (for logging — a silent self-heal that +/// nobody can see in a log is how this class of bug hides). +enum ReconnectSupervisorAction { + /// Nothing to do: connected, unpaired, paused, or a loop is already running. + none, + + /// No loop is running and we are not connected — start one. + start, + + /// A loop has been "running" far too long with nothing to show for it; the + /// in-flight flag is stale (an await that never returned). Clear it and start + /// a fresh loop. + restartStale, +} + +/// Level-triggered reconnect supervision. +/// +/// WHY THIS EXISTS: `_reconnect()` was EDGE-triggered — the only thing that +/// called it was the `connected → disconnected` transition. The loop itself is +/// unbounded and correct, but it sits inside one try/catch, so ANY throw inside +/// it (a foreground-lease acquisition, a claim/teardown error, a stream setup +/// failure) abandoned the loop for good. After that the engine sits at +/// 'disconnected', so the edge can never fire again, and on Android the +/// foreground service guarantees the process never restarts to clear it. That +/// is the reported "shows reconnecting, then disconnected, forever, until you +/// forget the band and re-pair". +/// +/// An await that never returns produces the same dead end with the in-flight +/// flag stuck true, which no amount of re-triggering fixes — hence +/// [ReconnectSupervisorAction.restartStale]. +/// +/// This is the pure decision. The caller runs a timer, feeds it observations, +/// and acts on the verdict. +ReconnectSupervisorAction superviseReconnect({ + required bool paired, + required bool keepAlive, + required bool connected, + required bool loopRunning, + required bool autoReconnectPaused, + required Duration? loopRunningFor, + Duration staleAfter = const Duration(minutes: 20), +}) { + if (!paired || !keepAlive || connected || autoReconnectPaused) { + return ReconnectSupervisorAction.none; + } + if (!loopRunning) return ReconnectSupervisorAction.start; + // A live loop is expected to run for a long time — a band can be out of range + // for hours, and the Android OS-autoConnect branch legitimately waits 15 + // minutes per pass. Only call it stale well past that. + if (loopRunningFor != null && loopRunningFor >= staleAfter) { + return ReconnectSupervisorAction.restartStale; + } + return ReconnectSupervisorAction.none; +} diff --git a/test/link_priority_policy_test.dart b/test/link_priority_policy_test.dart new file mode 100644 index 0000000..6f9a984 --- /dev/null +++ b/test/link_priority_policy_test.dart @@ -0,0 +1,94 @@ +// Issue #200: ~22% of an S22's battery in a day with 15 minutes of screen-on. +// +// The reporter blamed the 10 s heartbeat and the 15-minute backfill. The actual +// dominant cost was that Android's connection priority was requested once, at +// connect setup, and never stepped back down — an ~11.25 ms interval with zero +// slave latency, held 24/7 on a connection that is permanent by design, with a +// battery-optimization exemption ensuring Doze never damps it. +// +// These pin the stepping rule. The load-bearing property is the LAST test: an +// offload always runs at the fast interval, whatever else is going on, because +// throughput during a drain is what the fast interval was for. + +import 'package:flutter_test/flutter_test.dart'; +import 'package:openstrap_edge/sync/sync_policy.dart'; + +void main() { + test('an idle backgrounded link runs at the cheap interval', () { + expect( + desiredLinkPriority( + offloadActive: false, + background: true, + hasLiveConsumer: false, + ), + LinkPriority.lowPower, + reason: 'the overnight state, and where the drain was being spent', + ); + }); + + test('idle in the foreground is balanced, not high', () { + expect( + desiredLinkPriority( + offloadActive: false, + background: false, + hasLiveConsumer: false, + ), + LinkPriority.balanced, + ); + }); + + test('a foreground live consumer keeps the fast interval', () { + // A workout / spot check / breathing session streams 100 Hz; that genuinely + // needs the bandwidth. + expect( + desiredLinkPriority( + offloadActive: false, + background: false, + hasLiveConsumer: true, + ), + LinkPriority.high, + ); + }); + + test('a live consumer in the BACKGROUND does not hold the link high', () { + // Backgrounded, the engine downgrades to the compact 1 Hz stream, so the + // bandwidth argument no longer applies. + expect( + desiredLinkPriority( + offloadActive: false, + background: true, + hasLiveConsumer: true, + ), + LinkPriority.lowPower, + ); + }); + + test('an offload ALWAYS gets the fast interval', () { + // The invariant that keeps this change throughput-only: whatever else is + // true, a drain in flight raises the link first. Sync correctness never + // depends on the interval (commit-before-ACK, durable cursor), but making a + // drain crawl would be a real regression, so this is exhaustive. + for (final background in [false, true]) { + for (final live in [false, true]) { + expect( + desiredLinkPriority( + offloadActive: true, + background: background, + hasLiveConsumer: live, + ), + LinkPriority.high, + reason: 'background=$background live=$live', + ); + } + } + }); + + test('the battery poll is minutes apart, not seconds', () { + // It rode the 30 s keep-alive tick: 2,880 radio round-trips a day for a + // display value that changes a handful of times. + expect(kBatteryPollIntervalSeconds, greaterThanOrEqualTo(300)); + // Still far inside the liveness fuse, so it can never be the thing that + // starves `sinceLastRx` and bounces a healthy link. + expect(kBatteryPollIntervalSeconds, greaterThan(kLivenessFuseSeconds)); + }); +} diff --git a/test/reconnect_supervisor_test.dart b/test/reconnect_supervisor_test.dart new file mode 100644 index 0000000..858e7ac --- /dev/null +++ b/test/reconnect_supervisor_test.dart @@ -0,0 +1,135 @@ +// Issue #208: a band taken off for ten minutes, or carried out of range, never +// reconnects. The app shows "reconnecting", falls back to "disconnected", and +// stays there until the user forgets the band and re-pairs it. +// +// Two independent dead ends produced that, and both are terminal-by-design +// rather than flaky: +// +// 1. `_reconnect()` was EDGE-triggered — its only caller is the +// `connected → disconnected` transition — and the whole retry loop sat +// inside one try/catch. Any throw inside the loop abandoned it for good, +// after which the engine rests at 'disconnected' so the edge can never +// fire again. On Android the foreground service guarantees the process +// never restarts to clear it. +// 2. The bond-refusal pause was cleared ONLY inside the `createBond()` +// success branch, which lives inside the connect path that the pause +// prevents from running. Self-sealing. + +import 'package:flutter_test/flutter_test.dart'; +import 'package:openstrap_edge/sync/sync_policy.dart'; + +void main() { + group('superviseReconnect', () { + ReconnectSupervisorAction call({ + bool paired = true, + bool keepAlive = true, + bool connected = false, + bool loopRunning = false, + bool paused = false, + Duration? runningFor, + }) => superviseReconnect( + paired: paired, + keepAlive: keepAlive, + connected: connected, + loopRunning: loopRunning, + autoReconnectPaused: paused, + loopRunningFor: runningFor, + ); + + test('disconnected with no loop running restarts the loop', () { + // The state the app was stuck in — nothing else would ever fire. + expect(call(), ReconnectSupervisorAction.start); + }); + + test('does nothing while connected', () { + expect(call(connected: true), ReconnectSupervisorAction.none); + }); + + test('does nothing when unpaired or when we do not want a link', () { + expect(call(paired: false), ReconnectSupervisorAction.none); + expect(call(keepAlive: false), ReconnectSupervisorAction.none); + }); + + test('never fights a healthy in-flight loop', () { + // A band can be out of range for hours, and the Android OS-autoConnect + // branch legitimately waits 15 minutes per pass, so a long-running loop + // is normal and must not be restarted out from under itself. + expect( + call(loopRunning: true, runningFor: const Duration(minutes: 14)), + ReconnectSupervisorAction.none, + ); + expect( + call(loopRunning: true, runningFor: null), + ReconnectSupervisorAction.none, + ); + }); + + test('restarts a loop wedged well past the autoConnect window', () { + // An await that never returns (a leaked band lease, a hung platform call) + // leaves the in-flight flag true forever; re-triggering cannot fix that, + // so the flag has to be treated as stale. + expect( + call(loopRunning: true, runningFor: const Duration(minutes: 20)), + ReconnectSupervisorAction.restartStale, + ); + }); + + test('respects an active bond-refusal pause', () { + // Not a dead end any more (see below), but while it IS in force the + // supervisor must not hammer a band that refuses to bond. + expect(call(paused: true), ReconnectSupervisorAction.none); + expect( + call(paused: true, loopRunning: true, runningFor: const Duration(hours: 1)), + ReconnectSupervisorAction.none, + ); + }); + }); + + group('BondRefusalGiveUp cooldown', () { + test('trips exactly once at the threshold, then pauses', () { + final g = BondRefusalGiveUp(giveUpThreshold: 3); + final t0 = DateTime(2026, 8, 8, 12); + expect(g.bondRefused(now: t0), isFalse); + expect(g.bondRefused(now: t0), isFalse); + expect(g.bondRefused(now: t0), isTrue, reason: 'crosses the threshold'); + expect(g.bondRefused(now: t0), isFalse, reason: 'only ever once'); + expect(g.stillPaused(t0), isTrue); + }); + + test('the pause expires after its cooldown', () { + final g = BondRefusalGiveUp( + giveUpThreshold: 1, + cooldown: const Duration(minutes: 30), + ); + final t0 = DateTime(2026, 8, 8, 12); + expect(g.bondRefused(now: t0), isTrue); + expect(g.stillPaused(t0.add(const Duration(minutes: 29))), isTrue); + expect( + g.stillPaused(t0.add(const Duration(minutes: 30))), + isFalse, + reason: 'previously nothing could ever clear this', + ); + // Expiry resets the streak, so a band that still refuses re-trips + // normally instead of being retried forever. + expect(g.consecutive, 0); + expect(g.gaveUp, isFalse); + expect(g.bondRefused(now: t0.add(const Duration(minutes: 31))), isTrue); + }); + + test('a successful bond clears the streak and the latch', () { + final g = BondRefusalGiveUp(giveUpThreshold: 2); + final t0 = DateTime(2026, 8, 8, 12); + g.bondRefused(now: t0); + g.bondRefused(now: t0); + expect(g.stillPaused(t0), isTrue); + g.bondSucceeded(); + expect(g.stillPaused(t0), isFalse); + expect(g.gaveUpAt, isNull); + }); + + test('an unpaused tracker is never reported as paused', () { + final g = BondRefusalGiveUp(); + expect(g.stillPaused(DateTime(2026, 8, 8)), isFalse); + }); + }); +} From 23d624f1b45fa98708934700572e2b0c56fc63a9 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 12:03:29 +0530 Subject: [PATCH 07/12] address review - the workout step latch was compared against a counter that _resetLivePedometer zeroes on every reconnect, so a mid-workout reconnect left steps reading as unmeasured for the rest of the session. it's a flag now, set where samples land, and it survives the reset the same way the raw base already does. - the today tile and steps screen were gating live steps on the phone-steps TOGGLE, but the db only lets phone rows win when the phone actually has data for the day. with the toggle on and nothing recorded (read denied on ios, nothing writing to health connect) the band's live count was being dropped. both views use the same question the db asks now. - the supervisor could declare a loop wedged, start a replacement, and then have the original loop's finally clear the replacement's state on its way out - which would let a third loop start. loops carry a generation now. - the rescore sweep re-read every recent session's 1 Hz window on every drain, on the db isolate. bounded to the raw-retention horizon and skips windows a previous pass already covered, so an ordinary drain reads nothing. - getWorkout was scanning the same window twice: once to rescore, once to enrich. the rescore hands its rows back. - zip members sharing a basename overwrote each other; extracted temp files were never deleted; a zip is now refused if it declares an absurd member count or unpacked size. - a whoop csv with the right header but no rows counted as unrecognised, so an export of empty-but-valid files told you to re-download it in english. - lookback bound uses local midnight instead of n*86400. - workout activity mapping and stopWorkout/finish-card steps go through the same isApple / nullable-steps seams the rest of the file uses. --- lib/ble/ble_engine.dart | 67 ++++++++---- lib/data/db.dart | 6 ++ lib/data/local_repository_impl.dart | 72 ++++++++----- lib/health/health_export.dart | 5 +- lib/import/import_container.dart | 130 ++++++++++++++++++----- lib/import/noop_import.dart | 26 ++++- lib/import/whoop_import.dart | 38 ++++++- lib/state/app_state.dart | 118 +++++++++++++++----- lib/ui/activity/live_session_screen.dart | 18 ++-- lib/ui/screens/screens.dart | 2 +- lib/ui/today/today_screen.dart | 18 +++- test/import_container_test.dart | Bin 7382 -> 10374 bytes 12 files changed, 383 insertions(+), 117 deletions(-) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index 7019a55..a7f09ab 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -612,6 +612,8 @@ class BleEngine { // to the radio, and it is a no-op when nothing changed. bool _backgrounded = false; LinkPriority? _appliedPriority; + bool _priorityInFlight = false; + bool _priorityRestale = false; /// Told by AppState on every foreground/background transition. Drives the /// connection interval — see [desiredLinkPriority]. @@ -621,30 +623,53 @@ class BleEngine { unawaited(_applyLinkPriority()); } + /// Bring the link to the priority the current state calls for. + /// + /// SERIALIZED, and the target is recomputed inside the loop rather than at + /// call time. Every caller fires this unawaited from a state transition + /// (background, live mode, offload), so two can overlap; if they did, the + /// slower one's completion would write ITS target into `_appliedPriority` + /// last. The radio would then sit at one interval while the field claimed + /// another, and the `want == _appliedPriority` check below — the thing that + /// keeps this from spamming the radio — would skip the next legitimate + /// step-down, leaving the link fast exactly when it should go quiet. Future _applyLinkPriority() async { if (!Platform.isAndroid) return; // iOS picks its own interval - final session = _session; - if (session == null || !session.connected) return; - final device = session.device; - final want = desiredLinkPriority( - offloadActive: _offloadActive, - background: _backgrounded, - hasLiveConsumer: _liveEnabled && !_liveHrOnly, - ); - if (want == _appliedPriority) return; + if (_priorityInFlight) { + // Someone is mid-request; make them re-evaluate when they land rather + // than issuing a competing one. + _priorityRestale = true; + return; + } + _priorityInFlight = true; try { - await device.requestConnectionPriority( - connectionPriorityRequest: switch (want) { - LinkPriority.high => ConnectionPriority.high, - LinkPriority.balanced => ConnectionPriority.balanced, - LinkPriority.lowPower => ConnectionPriority.lowPower, - }, - ); - _appliedPriority = want; - _log('Link priority → ${want.name}.'); - } catch (e) { - // Leave `_appliedPriority` alone so the next transition retries. - _log('requestConnectionPriority(${want.name}) failed: $e'); + do { + _priorityRestale = false; + final session = _session; + if (session == null || !session.connected) return; + final want = desiredLinkPriority( + offloadActive: _offloadActive, + background: _backgrounded, + hasLiveConsumer: _liveEnabled && !_liveHrOnly, + ); + if (want == _appliedPriority) continue; + try { + await session.device.requestConnectionPriority( + connectionPriorityRequest: switch (want) { + LinkPriority.high => ConnectionPriority.high, + LinkPriority.balanced => ConnectionPriority.balanced, + LinkPriority.lowPower => ConnectionPriority.lowPower, + }, + ); + _appliedPriority = want; + _log('Link priority → ${want.name}.'); + } catch (e) { + // Leave `_appliedPriority` alone so the next transition retries. + _log('requestConnectionPriority(${want.name}) failed: $e'); + } + } while (_priorityRestale); + } finally { + _priorityInFlight = false; } } diff --git a/lib/data/db.dart b/lib/data/db.dart index a410d4c..dc9f7aa 100644 --- a/lib/data/db.dart +++ b/lib/data/db.dart @@ -875,6 +875,12 @@ class LocalDb { /// Used by the pedometer sync to tell "this day really had no steps" from /// "this read came back empty" before it replaces a day wholesale — see /// [replacePhoneCoverageForDay], which is delete-then-insert. + /// + /// Also the UI's source discriminator: it is the same quantity + /// [liveStepsForDay] tests to decide which source owns the day, so a screen + /// can ask "did the phone actually cover today?" instead of approximating it + /// with "is the toggle on". Those differ exactly when the toggle is on and + /// the phone has no data, where the band still owns the day. static Future phoneStepsForDay(String day) async { final db = await instance; final r = await db.rawQuery( diff --git a/lib/data/local_repository_impl.dart b/lib/data/local_repository_impl.dart index d4fba34..207b8cb 100644 --- a/lib/data/local_repository_impl.dart +++ b/lib/data/local_repository_impl.dart @@ -2022,8 +2022,8 @@ class LocalRepositoryImpl extends LocalRepository { // row (issue #206) — a session the app slept through stores a strain built // from the few minutes it was awake for. Persists on improvement, so the // list and the share card see the corrected value too. - final r = await _rescoreSessionFromSubstrate(stored); - final w = _workoutOf(r); + final rescored = await _rescoreSessionFromSubstrate(stored); + final w = _workoutOf(rescored.row); final startTs = w['start_ts'] as int?; if (startTs == null) return w; final endTs = @@ -2035,7 +2035,10 @@ class LocalRepositoryImpl extends LocalRepository { // hr / avg_hr / min_hr / zone_bands / recovery_curve / hr_drift_pct / // time_to_peak_min; without a producer they were blank everywhere. try { - final hrRows = await LocalDb.hrSamplesInRange(startTs, endTs); + // Reuse the rows the rescore above already read for this exact window + // rather than scanning it a second time on every detail open. + final hrRows = rescored.hrRows ?? + await LocalDb.hrSamplesInRange(startTs, endTs); if (hrRows.isNotEmpty) { final ts = [for (final e in hrRows) (e['rec_ts'] as num).toInt()]; final hr = [for (final e in hrRows) (e['hr'] as num).toInt()]; @@ -2404,9 +2407,8 @@ class LocalRepositoryImpl extends LocalRepository { /// improves on each pass and converges. Returns the row with the reconciled /// values applied (never null-out a stored value), writing back only on a /// real change. Best-effort — never throws into a read path. - Future> _rescoreSessionFromSubstrate( - Map row, - ) async { + Future<({Map row, List>? hrRows})> + _rescoreSessionFromSubstrate(Map row) async { final id = row['id']; final startTs = (row['start_ts'] as num?)?.toInt(); final endTs = (row['end_ts'] as num?)?.toInt(); @@ -2416,11 +2418,13 @@ class LocalRepositoryImpl extends LocalRepository { endTs == null || endTs <= startTs || (row['status']?.toString() ?? '') != 'done') { - return row; + return (row: row, hrRows: null); } try { + // Returned to the caller: `getWorkout` enriches from the SAME 1 Hz window + // straight after this, and a two-hour session is ~7200 rows to scan twice. final hrRows = await LocalDb.hrSamplesInRange(startTs, endTs); - if (hrRows.isEmpty) return row; + if (hrRows.isEmpty) return (row: row, hrRows: hrRows); final profile = Profile.fromMap(getProfileMap()); final stats = computeManualSessionStats( @@ -2442,7 +2446,7 @@ class LocalRepositoryImpl extends LocalRepository { ], substrate: stats, ); - if (!merged.changed) return row; + if (!merged.changed) return (row: row, hrRows: hrRows); final updated = { ...row, @@ -2456,33 +2460,53 @@ class LocalRepositoryImpl extends LocalRepository { ), }; await LocalDb.putSession(updated); - return updated; + return (row: updated, hrRows: hrRows); } catch (_) { - return row; // best-effort: the stored row still renders + return (row: row, hrRows: null); // best-effort: the stored row renders } } - /// Re-score every finished session that started in the last [sinceDays] days - /// against the substrate now in the DB. Called after a drain lands, so a - /// workout whose window arrived late is corrected on the LIST too, not only - /// when its detail screen is opened. Returns how many rows changed. + /// How far the durable frontier had advanced the last time the sweep ran. A + /// session that ended at or before this point was already scored against + /// substrate covering its whole window, so nothing new can arrive for it and + /// re-reading its 1 Hz rows on every drain is pure waste. + int _rescoredThroughTs = 0; + + /// Re-score recent finished sessions against the substrate now in the DB. + /// Called after a drain lands, so a workout whose window arrived late is + /// corrected on the LIST too, not only when its detail screen is opened. + /// Returns how many rows changed. + /// + /// Runs on the DB-owning (main) isolate by necessity — the sqflite handle is + /// not portable to another isolate — so it is bounded rather than offloaded: + /// the window is the raw-retention horizon (older windows are pruned and can + /// never improve) and anything already covered by a previous pass is skipped + /// outright, which leaves an ordinary drain doing no substrate reads at all. + @override - Future rescoreRecentSessions({int sinceDays = 7}) async { + Future rescoreRecentSessions({int sinceDays = 3}) async { final nowSec = DateTime.now().millisecondsSinceEpoch ~/ 1000; var changed = 0; try { - final rows = await LocalDb.sessionsInRange( - nowSec - sinceDays * 86400, - nowSec, - ); + // Local-midnight bound, not `now - n * 86400`: a DST day is 23 or 25 + // hours, so a flat day-length silently moves the window by an hour. + final fromTs = + localDayStartSec(dayLabelOf(DateTime.now().subtract( + Duration(days: sinceDays), + ))) ?? + (nowSec - sinceDays * 86400); + final rows = await LocalDb.sessionsInRange(fromTs, nowSec); + // Where the durable record frontier stands NOW; sessions ending at or + // before it are fully covered once this pass has scored them. + final frontier = await LocalDb.getCursorInt('rec_ts_hw') ?? 0; for (final r in rows) { + final endTs = (r['end_ts'] as num?)?.toInt(); + if (endTs != null && endTs <= _rescoredThroughTs) continue; final before = (r['strain'] as num?)?.toDouble(); final after = await _rescoreSessionFromSubstrate(r); - if (!identical(after, r) && - (after['strain'] as num?)?.toDouble() != before) { - changed++; - } + if ((after.row['strain'] as num?)?.toDouble() != before) changed++; } + if (frontier > _rescoredThroughTs) _rescoredThroughTs = frontier; } catch (_) { /* best-effort */ } diff --git a/lib/health/health_export.dart b/lib/health/health_export.dart index ac57fff..4b22945 100644 --- a/lib/health/health_export.dart +++ b/lib/health/health_export.dart @@ -1024,8 +1024,11 @@ class HealthExporter { } } + // `isApple`, not `Platform.isIOS`: every other platform decision in this file + // (the HRV type, the delete list, the store name) keys off the same getter, + // and a divergence here would hand macOS the Health Connect spellings. HealthWorkoutActivityType _activity(String? type) => - healthActivityForType(type, ios: Platform.isIOS); + healthActivityForType(type, ios: isApple); static Map? _decode(Object? json) { if (json is! String) return null; diff --git a/lib/import/import_container.dart b/lib/import/import_container.dart index 24bb04c..5314bd8 100644 --- a/lib/import/import_container.dart +++ b/lib/import/import_container.dart @@ -108,50 +108,93 @@ bool _isCsvMember(String name) { !name.startsWith('__MACOSX/'); } +/// Ceilings for archive extraction. A picked file is local and user-chosen, so +/// this is not a hostile-input boundary — but a malformed or pathological zip +/// should fail with a message rather than take the process out trying to +/// materialise it. The uncompressed ceiling is generous: a 90-day NOOP raw +/// export really is hundreds of megabytes. +const int _kMaxArchiveMembers = 5000; +const int _kMaxUncompressedBytes = 4 * 1024 * 1024 * 1024; // 4 GiB + +/// CSV files on disk for an import, plus the temp directory (if any) that has +/// to be cleaned up once they have been read. +class ResolvedImportFiles { + ResolvedImportFiles(this.paths, this._tempDir); + + final List paths; + final Directory? _tempDir; + + /// Delete anything extracted for this import. Safe to call more than once, + /// and never throws — a leftover temp file is not worth failing an import + /// that otherwise succeeded. + Future dispose() async { + final dir = _tempDir; + if (dir == null) return; + try { + if (dir.existsSync()) await dir.delete(recursive: true); + } catch (_) { + /* the OS reclaims the temp dir eventually */ + } + } +} + /// Resolve the picked paths into CSV files on disk, unwrapping ZIP archives. /// /// [flavor] names the importer in error messages ('NOOP', 'WHOOP'). Extracted -/// members are written to a temp directory — the caller reads them and the OS -/// reclaims them; nothing is copied into app storage. +/// members are written to a temp directory; the caller MUST `dispose()` the +/// result once it has finished reading them, or a large export leaves a full +/// second copy behind. Nothing is ever copied into app storage. /// /// Throws [ImportFormatException] with actionable guidance for anything we /// cannot parse: a database, an archive of databases, a gzip, binary junk. -Future> resolveImportCsvPaths( +Future resolveImportCsvPaths( List paths, { required String flavor, }) async { final out = []; - for (final path in paths) { - final kind = await sniffFile(path); - switch (kind) { - case ImportContainer.text: - out.add(path); - case ImportContainer.zip: - out.addAll(await _extractCsvMembers(path, flavor: flavor)); - case ImportContainer.sqlite: - throw ImportFormatException( - '“${p.basename(path)}” is a database file, not a $flavor CSV ' - 'export. In NOOP, use Export → raw sensor CSV and pick the ' - '“noop-raw-sensors-….csv” file it writes.', - ); - case ImportContainer.gzip: - throw ImportFormatException( - '“${p.basename(path)}” is a gzip archive. Unzip it first and pick ' - 'the CSV inside.', - ); - case ImportContainer.binary: - throw ImportFormatException( - '“${p.basename(path)}” is not a text file, so there is nothing to ' - 'read as a $flavor CSV export.', - ); + Directory? tempDir; + try { + for (final path in paths) { + final kind = await sniffFile(path); + switch (kind) { + case ImportContainer.text: + out.add(path); + case ImportContainer.zip: + tempDir ??= + await Directory.systemTemp.createTemp('openstrap_import_'); + out.addAll( + await _extractCsvMembers(path, flavor: flavor, dir: tempDir), + ); + case ImportContainer.sqlite: + throw ImportFormatException( + '“${p.basename(path)}” is a database file, not a $flavor CSV ' + 'export. In NOOP, use Export → raw sensor CSV and pick the ' + '“noop-raw-sensors-….csv” file it writes.', + ); + case ImportContainer.gzip: + throw ImportFormatException( + '“${p.basename(path)}” is a gzip archive. Unzip it first and pick ' + 'the CSV inside.', + ); + case ImportContainer.binary: + throw ImportFormatException( + '“${p.basename(path)}” is not a text file, so there is nothing to ' + 'read as a $flavor CSV export.', + ); + } } + } catch (_) { + // A later file failing must not strand what an earlier ZIP already wrote. + await ResolvedImportFiles(const [], tempDir).dispose(); + rethrow; } - return out; + return ResolvedImportFiles(out, tempDir); } Future> _extractCsvMembers( String path, { required String flavor, + required Directory dir, }) async { final name = p.basename(path); final Archive archive; @@ -163,11 +206,28 @@ Future> _extractCsvMembers( ); } + if (archive.files.length > _kMaxArchiveMembers) { + throw ImportFormatException( + '“$name” holds ${archive.files.length} entries, which is far more than ' + 'any $flavor export — refusing to unpack it.', + ); + } + final csvFiles = [ for (final f in archive.files) if (f.isFile && _isCsvMember(f.name)) f, ]; + final declaredBytes = + csvFiles.fold(0, (sum, f) => sum + (f.size > 0 ? f.size : 0)); + if (declaredBytes > _kMaxUncompressedBytes) { + throw ImportFormatException( + '“$name” unpacks to more than ' + '${(declaredBytes / (1024 * 1024 * 1024)).toStringAsFixed(1)} GB of ' + 'CSV, which is not something we can import.', + ); + } + if (csvFiles.isEmpty) { // The `.noopbak` case, and the single most-reported one: an archive whose // payload is a SQLite database. Name the file we actually want rather than @@ -192,10 +252,22 @@ Future> _extractCsvMembers( ); } - final dir = await Directory.systemTemp.createTemp('openstrap_import_'); final out = []; + final used = {}; for (final f in csvFiles) { - final dest = File(p.join(dir.path, p.basename(f.name))); + // Members can share a basename (`daily/data.csv`, `workouts/data.csv`). + // Flattening them onto one destination silently dropped one file and + // parsed the survivor twice. + var base = p.basename(f.name); + if (!used.add(base)) { + final stem = p.basenameWithoutExtension(base); + final ext = p.extension(base); + var n = 2; + while (!used.add(base = '$stem-$n$ext')) { + n++; + } + } + final dest = File(p.join(dir.path, base)); await dest.writeAsBytes(f.content as List); out.add(dest.path); } diff --git a/lib/import/noop_import.dart b/lib/import/noop_import.dart index e7e5034..e17d1be 100644 --- a/lib/import/noop_import.dart +++ b/lib/import/noop_import.dart @@ -125,19 +125,39 @@ class NoopImporter { // container first: a ZIP of CSVs is unwrapped, and anything unusable throws // an [ImportFormatException] naming the file we DO want. final resolved = await resolveImportCsvPaths([path], flavor: 'NOOP'); - if (resolved.isEmpty) { + if (resolved.paths.isEmpty) { + await resolved.dispose(); throw const ImportFormatException( 'That archive holds no NOOP CSV export.', ); } // A NOOP raw-sensor export is a single CSV; if an archive carried several, // prefer one that actually looks like the raw-sensor file. - final chosen = resolved.firstWhere( + final chosen = resolved.paths.firstWhere( (p) => p.toLowerCase().contains('raw-sensor'), - orElse: () => resolved.first, + orElse: () => resolved.paths.first, ); file = File(chosen); + try { + return await _importResolvedFile( + file, + profile, + engine, + onProgress: onProgress, + ); + } finally { + // Anything unpacked from an archive is ours to clean up, success or not. + await resolved.dispose(); + } + } + + static Future _importResolvedFile( + File file, + Profile profile, + DerivationEngine engine, { + void Function(int days)? onProgress, + }) async { // Rolling buffer: keeps at most the CURRENT + PREVIOUS local date of samples. final secs = {}; // ts(sec) → channels final rrTs = []; // beat end time (epoch ms) diff --git a/lib/import/whoop_import.dart b/lib/import/whoop_import.dart index cda97c8..4e22236 100644 --- a/lib/import/whoop_import.dart +++ b/lib/import/whoop_import.dart @@ -89,12 +89,39 @@ class WhoopImporter { // ZIP — its bytes hit `utf8.decoder` and threw "Unexpected extension byte // (at offset 10)" (issue #199). Unwrap it first; anything we can't parse // throws an actionable [ImportFormatException] instead. - final csvPaths = await resolveImportCsvPaths(paths, flavor: 'WHOOP'); + final resolved = await resolveImportCsvPaths(paths, flavor: 'WHOOP'); + try { + return await _importResolvedCsvs( + resolved.paths, + rawDays: rawDays, + engine: engine, + profile: profile, + onProgress: onProgress, + days: days, + workouts: workouts, + skipped: skipped, + ); + } finally { + // Anything unpacked from an archive is ours to clean up, success or not. + await resolved.dispose(); + } + } + + static Future _importResolvedCsvs( + List csvPaths, { + required Set rawDays, + DerivationEngine? engine, + Profile? profile, + void Function(int done)? onProgress, + required int days, + required int workouts, + required int skipped, + }) async { var recognisedFiles = 0; final headersSeen = []; for (final path in csvPaths) { final rows = await _readCsv(path); - if (rows.length < 2) continue; + if (rows.isEmpty) continue; final header = rows.first; final col = { for (var i = 0; i < header.length; i++) header[i].trim().toLowerCase(): i @@ -104,7 +131,14 @@ class WhoopImporter { headersSeen.add(header.take(6).join(', ')); continue; } + // Count the file as recognised on its HEADER, before the empty check + // below: an export whose files carry the right columns but no rows (a + // week with no workouts, say) is a valid export we simply have nothing + // to import from. Skipping it first made it indistinguishable from a + // file we don't understand, and the caller then told the user to + // re-download in English. recognisedFiles++; + if (rows.length < 2) continue; for (var r = 1; r < rows.length; r++) { final f = rows[r]; if (f.isEmpty) continue; diff --git a/lib/state/app_state.dart b/lib/state/app_state.dart index 2049ae0..44f1e76 100644 --- a/lib/state/app_state.dart +++ b/lib/state/app_state.dart @@ -184,6 +184,13 @@ class AppState extends ChangeNotifier { /// `_reconnecting` true forever, which no re-trigger can clear. DateTime? _reconnectingSince; + /// Which reconnect loop is the live one. Bumped whenever a loop starts, so a + /// loop that was declared wedged and replaced can recognise itself as + /// superseded if it ever unblocks: without this its `finally` would clear the + /// REPLACEMENT's `_reconnecting`/`_reconnectingSince`, and the supervisor + /// would then start a third loop while two are already connecting. + int _reconnectGeneration = 0; + /// Level-triggered reconnect supervision. The loop's only trigger used to be /// the `connected → disconnected` edge, so any abandoned loop was permanent. /// This ticks regardless of edges and re-arms — see [superviseReconnect]. @@ -257,7 +264,10 @@ class AppState extends ChangeNotifier { // bucket is one platform call, so the 7-day backfill window is up to 168 of // them; only today can still change, and only yesterday if the app did not // run then. The full window runs on the explicit gestures instead. - if (phoneStepsEnabled) unawaited(syncPhoneSteps()); + if (phoneStepsEnabled) { + unawaited(syncPhoneSteps()); + unawaited(_refreshPhoneStepsToday()); + } // Best-effort, no prompt: learn the current health-permission state so the // Profile toggle reflects reality on open. if (healthSyncEnabled) unawaited(checkHealth()); @@ -505,6 +515,7 @@ class AppState extends ChangeNotifier { phoneStepsEnabled = false; phoneStepsLastSyncedDays = null; phoneStepsLastTotal = null; + phoneStepsToday = 0; try { await LocalDb.clearPhoneCoverage(); } catch (e) { @@ -523,6 +534,31 @@ class AppState extends ChangeNotifier { int? phoneStepsLastSyncedDays; int? phoneStepsLastTotal; + /// Steps the PHONE has banked for today, mirroring `liveStepsForDay`'s own + /// source rule (phone wins only when it actually has data). Screens add the + /// band's live count on top of the day total, and must not do that once the + /// phone owns the day — both count the same walk. Gating on + /// [phoneStepsEnabled] alone was wrong: with the toggle on and no phone data + /// (iOS read denied, nothing writing to Health Connect) the band still owns + /// the day and its live steps were being thrown away. + int phoneStepsToday = 0; + + /// True when today's step total comes from the phone, so band live steps are + /// already accounted for and must not be added again. + bool get todayStepsFromPhone => phoneStepsEnabled && phoneStepsToday > 0; + + Future _refreshPhoneStepsToday() async { + try { + final n = await LocalDb.phoneStepsForDay(todayLabel()); + if (n != phoneStepsToday) { + phoneStepsToday = n; + notifyListeners(); + } + } catch (_) { + /* best-effort — the gate just falls back to showing band live steps */ + } + } + /// Pull the last [days] days of phone step counts into `live_coverage`. /// /// Idempotent (delete-then-insert per day, scoped to the phone source), so @@ -535,6 +571,7 @@ class AppState extends ChangeNotifier { final r = await _phonePedometer.syncRecent(days: days); phoneStepsLastSyncedDays = r.daysRead; phoneStepsLastTotal = r.totalSteps; + await _refreshPhoneStepsToday(); notifyListeners(); return r.daysRead; } catch (e) { @@ -2034,13 +2071,26 @@ class AppState extends ChangeNotifier { // the live-session screen shows steps FOR THIS WORKOUT (not since connection). int? _workoutRawBase; - /// 100 Hz sample count at the moment the active workout started, so - /// [workoutStepsMeasured] can tell "you did not move" apart from "the band - /// never sent us anything to count". - int? _workoutSampleBase; + /// Whether ANY gait-capable accel sample has reached us since the active + /// workout began, so [workoutStepsMeasured] can tell "did not move" apart + /// from "the band never sent anything to count". + /// + /// Deliberately a latch and NOT a comparison against `_liveSamples`: + /// `_resetLivePedometer()` zeroes that counter on every (re)connect, and it + /// runs mid-workout. A counter comparison therefore went permanently + /// "unmeasured" after the first reconnect — steps stuck on a dash for the + /// rest of the workout and `stopWorkout` banking none — which is the same + /// trap `_resetLivePedometer` already sidesteps for `_workoutRawBase` by + /// rebasing it negative rather than dropping it. + bool _workoutSawSamples = false; /// Steps taken since the active workout started (real, live, gain-applied). - /// 0 when no workout is running. This is what the workout screen shows. + /// 0 when no workout is running. + /// + /// Prefer [workoutStepsMeasured] in anything user-facing: this coerces an + /// unmeasured workout to 0, which is only safe because the two remaining + /// callers treat 0 as "omit" (the finish card hides the stat, `stopWorkout` + /// leaves the column unset). int get workoutSteps => workoutStepsMeasured ?? 0; /// Steps for the active workout, or NULL when nothing gait-capable was ever @@ -2055,10 +2105,9 @@ class AppState extends ChangeNotifier { /// distance both right, "0 STEPS" beside them. int? get workoutStepsMeasured { if (activeWorkout == null || _workoutRawBase == null) return null; - // No accel sample has reached us since this workout began — nothing was - // counted, as opposed to zero steps having been counted. - final base = _workoutSampleBase; - if (base == null || _liveSamples <= base) return null; + // Nothing gait-capable has arrived for this workout — unmeasured, as + // opposed to zero steps having been measured. + if (!_workoutSawSamples) return null; final raw = _liveRaw - _workoutRawBase!; return raw > 0 ? (raw * ana.StepParams.gain).round() : 0; } @@ -2071,6 +2120,8 @@ class AppState extends ChangeNotifier { void _ingestLiveMagsAt(proto.ImuFrame f, int nowMs) { final mags = f.mags; if (mags.isEmpty) return; + // Survives `_resetLivePedometer()` — see [_workoutSawSamples]. + if (activeWorkout != null) _workoutSawSamples = true; // Append this frame's |a|(g) samples (gravity INCLUDED — AN-2554's dynamic // threshold rides the ~1 g baseline). `e` is this frame's 1 Hz-equivalent // ENMO (mean |a| − 1 g), read below by the stillness nudge and the posture @@ -3160,6 +3211,7 @@ class AppState extends ChangeNotifier { } _reconnecting = true; _reconnectingSince = DateTime.now(); + final generation = ++_reconnectGeneration; BandOwnership.markForegroundIntent(true); _log('[OWNERSHIP] reconnect intent on (${BandOwnership.debugState})'); try { @@ -3169,7 +3221,10 @@ class AppState extends ChangeNotifier { // ReconnectPolicy. The engine's single in-flight guard guarantees this loop // can never overlap a foreground connect on the same band. int attempt = 0; - while (_keepAlive && !engine.isConnected && !device.autoReconnectPaused) { + while (_keepAlive && + !engine.isConnected && + !device.autoReconnectPaused && + generation == _reconnectGeneration) { attempt++; // Surface `reconnecting` while the loop backs off, so the UI shows a // connecting-style state instead of flat 'disconnected'. @@ -3271,16 +3326,25 @@ class AppState extends ChangeNotifier { // left foreground intent stuck on forever, which blocks every // headless background-sync entry point (BandOwnership.tryAcquireHeadless // gates on this being off). same bug shape as the foregroundActive fix. - if (!_keepAlive || device.autoReconnectPaused) { - BandOwnership.markForegroundIntent(false); - _log('[OWNERSHIP] reconnect intent off (${BandOwnership.debugState})'); + if (generation != _reconnectGeneration) { + // Superseded: the supervisor declared this loop wedged and started a + // replacement, which now owns the flags and the band claim. Clearing + // them here would clobber the live loop's state and let the supervisor + // start a third one. + _log('[RECONNECT] loop #$generation was superseded — leaving the ' + 'replacement\'s state alone.'); + } else { + if (!_keepAlive || device.autoReconnectPaused) { + BandOwnership.markForegroundIntent(false); + _log('[OWNERSHIP] reconnect intent off (${BandOwnership.debugState})'); + } + _reconnecting = false; + _reconnectingSince = null; + // If we gave up (keepAlive dropped / never connected), stop advertising + // `reconnecting` — fall back to a truthful 'disconnected'. No-op when + // the loop exited via a successful connect (phase is `listening`). + engine.clearReconnecting(); } - _reconnecting = false; - _reconnectingSince = null; - // If we gave up (keepAlive dropped / never connected), stop advertising - // `reconnecting` — fall back to a truthful 'disconnected'. No-op when - // the loop exited via a successful connect (phase is `listening`). - engine.clearReconnecting(); } } @@ -3754,7 +3818,7 @@ class AppState extends ChangeNotifier { unawaited(engine.retryFullLiveStreams()); } _workoutRawBase = _liveRaw; - _workoutSampleBase = _liveSamples; + _workoutSawSamples = false; // A first night may have been derived since init. This read finishes // after the session below is constructed, so it back-fills the anchor on // `activeWorkout` when it lands rather than blocking the start. @@ -3961,7 +4025,7 @@ class AppState extends ChangeNotifier { // snapshot: steps count from zero going forward, same as // calories/strain/zone-minutes already (honestly) do here. _workoutRawBase = _liveRaw; - _workoutSampleBase = _liveSamples; + _workoutSawSamples = false; // A first night may have been derived since init. This read finishes // after the session below is constructed, so it back-fills the anchor on // `activeWorkout` when it lands rather than blocking the start. @@ -4019,7 +4083,9 @@ class AppState extends ChangeNotifier { _deriveScheduler.setWorkoutActive(false); final w = activeWorkout!; final finalKcal = w.calories.round(); - final wSteps = workoutSteps; // real steps taken during this workout + // Nullable: an unmeasured workout must leave the column unset rather than + // bank a zero that reads as "you took no steps". + final wSteps = workoutStepsMeasured; // Persist the finalized session before clearing the live state. zone_min = // the per-zone seconds the 1 Hz tick accumulated (Z1..Z5, minutes). final id = w.workoutId ?? 'w${w.startTime.millisecondsSinceEpoch}'; @@ -4038,7 +4104,7 @@ class AppState extends ChangeNotifier { 'zone_min_json': jsonEncode( zoneMin.any((v) => v > 0) ? zoneMin : const [], ), - if (wSteps > 0) 'steps': wSteps, + if (wSteps != null && wSteps > 0) 'steps': wSteps, 'source': 'manual', 'created_at': w.startTime.millisecondsSinceEpoch, }; @@ -4052,7 +4118,7 @@ class AppState extends ChangeNotifier { } activeWorkout = null; _workoutRawBase = null; - _workoutSampleBase = null; + _workoutSawSamples = false; notifyListeners(); _log('Live session ended. Burned $finalKcal kcal.'); LiveActivity.end(); @@ -4085,7 +4151,7 @@ class AppState extends ChangeNotifier { _deriveScheduler.setWorkoutActive(false); activeWorkout = null; _workoutRawBase = null; - _workoutSampleBase = null; + _workoutSawSamples = false; LiveActivity.end(); } diff --git a/lib/ui/activity/live_session_screen.dart b/lib/ui/activity/live_session_screen.dart index 9c8980e..98efa55 100644 --- a/lib/ui/activity/live_session_screen.dart +++ b/lib/ui/activity/live_session_screen.dart @@ -310,7 +310,7 @@ class _LiveSessionScreenState extends State peakHr: w?.maxHrSeen ?? 0, calories: w?.calories ?? 0, strain: w?.strain, - steps: app.workoutSteps, + steps: app.workoutStepsMeasured, ); // AWAIT: stopWorkout flushes the GPS route tail; navigating before it // completes raced the finish screen's route load (missing tail / no map). @@ -707,7 +707,9 @@ class WorkoutFinishSnapshot { /// nullable all the way to the finish card: a `?? 0` here would print a /// confident "0.0" for a session that was simply never scored. final double? strain; - final int steps; + /// Null when nothing gait-capable was measured for the workout — the finish + /// card omits the stat rather than showing a zero. + final int? steps; const WorkoutFinishSnapshot({ required this.type, required this.duration, @@ -841,9 +843,12 @@ class _WorkoutFinishScreenState extends State strain > 0 && (strain - tw.value).abs() < 0.15; final ms = recs.record('most_steps'); + final steps = s.steps; + // An unmeasured workout can't set a step record. _prSteps = ms != null && - s.steps > 0 && - (s.steps - ms.value).abs() < 1.5; + steps != null && + steps > 0 && + (steps - ms.value).abs() < 1.5; } }); } catch (_) {} @@ -1066,7 +1071,7 @@ class _WorkoutFinishScreenState extends State /// These figures COUNT UP with the reveal, so unlike the other sections they /// legitimately rebuild per frame — but it is a handful of Text widgets, not /// a map or a route re-derivation. - Widget _heroStats(int peak, int? avg, int kcal, int steps) { + Widget _heroStats(int peak, int? avg, int kcal, int? steps) { Widget stat(String v, String label) => Expanded(child: _FinishStat(v, label)); return AnimatedBuilder( @@ -1082,7 +1087,8 @@ class _WorkoutFinishScreenState extends State stat(peak > 0 ? '${(peak * p).round()}' : '—', 'PEAK BPM'), stat(avg != null ? '${(avg * p).round()}' : '—', 'AVG BPM'), stat('${(kcal * p).round()}', 'KCAL'), - if (steps > 0) stat('${(steps * p).round()}', 'STEPS'), + if (steps != null && steps > 0) + stat('${(steps * p).round()}', 'STEPS'), ], ), ), diff --git a/lib/ui/screens/screens.dart b/lib/ui/screens/screens.dart index a20d740..61c208f 100644 --- a/lib/ui/screens/screens.dart +++ b/lib/ui/screens/screens.dart @@ -284,7 +284,7 @@ class _ActivityDetailState extends State<_ActivityDetail> { // live count would double-count it. final live = _isToday ? context.select( - (a) => a.phoneStepsEnabled ? 0 : a.liveSteps, + (a) => a.todayStepsFromPhone ? 0 : a.liveSteps, ) : 0; // Was context.watch() — rebuilt this whole board on every one of diff --git a/lib/ui/today/today_screen.dart b/lib/ui/today/today_screen.dart index 7ba5de0..5386c46 100644 --- a/lib/ui/today/today_screen.dart +++ b/lib/ui/today/today_screen.dart @@ -186,8 +186,16 @@ class _TodayScreenState extends State // the REBUILD TRIGGER is scoped. liveSteps is included (already rate- // limited to ~1/s at the source) so the steps tile doesn't freeze mid-walk // while waiting on an unrelated dbCounts change. - context.select, bool, String, int)>( - (a) => (a.dbCounts, a.reanalyzing, a.reanalyzeProgress, a.liveSteps), + context.select, bool, String, int, bool)>( + (a) => ( + a.dbCounts, + a.reanalyzing, + a.reanalyzeProgress, + a.liveSteps, + // The steps tile stops adding the band's live count the moment the + // phone owns the day, so a flip has to rebuild it. + a.todayStepsFromPhone, + ), ); final app = context.read(); final t = TodayData.fromJson(data); @@ -355,8 +363,10 @@ class _TodayScreenState extends State // than summing them (both count the same walk — one from the // pocket, one from the wrist), so adding the wrist's live count on // top of a phone-sourced day total re-introduces exactly the - // double count that rule exists to prevent. - liveSteps: context.read().phoneStepsEnabled + // double count that rule exists to prevent. `todayStepsFromPhone` + // mirrors the DB's own rule, so a day the phone did not actually + // cover still shows the band's live count. + liveSteps: context.read().todayStepsFromPhone ? 0 : context.read().liveSteps, onOpen: _open, diff --git a/test/import_container_test.dart b/test/import_container_test.dart index 71576a465e83db6cebf6e20664052afbfbb3eba3..ce067829f082a389816cb9852637f9b9ef5a86a0 100644 GIT binary patch delta 1920 zcmah~U2oh(6jgwLh=PE!o9vQMT`D2l)HrDfpprDGltiFZZ6Pg4`B0g)@5WZ_8Ea?pD9{>$^jMl@-|TYso^L2o8w?jamMd;OW2z#v_Oll?#>d zW007ME-Jt-$^qCd2%-&%4(6cvupUb^h>i}*P3U?X)uXnM;IW97lu z)fxco=NM{tCrQz5dja+nrPbbmhpy)nv>lcz9GCp&!7ug3thrUYICIVXSX&&HRDC$G zM32Vnh~PD$W6Pk&2z^NwBOjEQVwZK41}-#2n4(6|F+xY7bQH7+&;N6DPQkYro@8)S zcF_D(Ul?YXwNNThF_RV;%IzCepT)K<)xsjJA3 zb?mOuKZ#5UPRDRjbcBFT(Q(F7mIi8xC?OkOf`c)d|FD;J7wgSgSC*bKU)CFkMvp6c+w34a;(Ke+)JJ5EC29cv-d9$EuU{m?PfbRfc< zbuqImt)l05r8q$ze3j%=#Sd2d@2vW;+F$?BURU2PuIoMoL2#8)x0!Ig6_9&UjNEtW zKCQgq9hqMj_RJ5p=S*vU!CYxJ%-wl-gO6zz1WCgyd!H+?XwnoE8<5R~c~pOImL`iH zm^-g5E;}T7lwqLz+S-c{7Xgh?qGNtuxK$i-uKE27WwpY`$}z*|UpJ4Mjdk)rQE#!R zG+A{2!`f!{C zfFLFP%Aq-Zw`Y5MbCBaE?W8FyOGfy>Z%YBFIB@TSaMaGOH4`ZH+g%F`hTJ_Y;ReTZ*jEqfO1#rMK)mBTSKT gigRZYA#cO%pQ%0YmiA{xqwd?~bJd&mx8=2e0F{|xpa1{> delta 64 zcmV-G0Kfl+QPw%IrxcT|6f2Y76p52<6-tv|5)iY876S#7O%D*0VH7Qs?immUTX11? WXkC*~5+1Y7CRzcLEhr+BRVXnH;1*H< From b63b2538d5c771fb8fd92ee78190e7292b859a24 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 12:17:24 +0530 Subject: [PATCH 08/12] second review pass - a link-priority reply that landed after teardown wrote its target back into the cache the teardown had just cleared, so the next connection skipped its own request. discarded if the session it was asked on is gone. - the rescore skip keyed on the window alone, so a session that was still live during an earlier sweep was skipped forever once the frontier passed its end. keys on id+window and only records a session once it's actually finished. - two selected archives each containing data.csv wrote to the same temp path. one subdirectory per archive. - step-record detection read the snapshot only; it uses the persisted count first, like the rest of that screen. --- lib/ble/ble_engine.dart | 9 +++++ lib/data/local_repository_impl.dart | 40 ++++++++++++++++++----- lib/import/import_container.dart | 9 ++++- lib/ui/activity/live_session_screen.dart | 4 ++- test/import_container_test.dart | Bin 10374 -> 10949 bytes 5 files changed, 51 insertions(+), 11 deletions(-) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index a7f09ab..514e5f7 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -661,6 +661,15 @@ class BleEngine { LinkPriority.lowPower => ConnectionPriority.lowPower, }, ); + // Only remember it if the link we asked is still the live one. A + // teardown during the await clears `_appliedPriority` precisely so + // the next session re-requests from scratch (Android resets the + // interval per GATT connection); writing this session's target in + // afterwards would make the new link skip its own request. + if (!identical(_session, session) || !session.connected) { + _log('Link priority reply arrived after teardown — discarded.'); + return; + } _appliedPriority = want; _log('Link priority → ${want.name}.'); } catch (e) { diff --git a/lib/data/local_repository_impl.dart b/lib/data/local_repository_impl.dart index 207b8cb..365fddf 100644 --- a/lib/data/local_repository_impl.dart +++ b/lib/data/local_repository_impl.dart @@ -2466,11 +2466,17 @@ class LocalRepositoryImpl extends LocalRepository { } } - /// How far the durable frontier had advanced the last time the sweep ran. A - /// session that ended at or before this point was already scored against - /// substrate covering its whole window, so nothing new can arrive for it and - /// re-reading its 1 Hz rows on every drain is pure waste. - int _rescoredThroughTs = 0; + /// Sessions this process has already scored as FINISHED, keyed by id and the + /// window they had at the time (`id@endTs`). + /// + /// The skip cannot key on the window alone: a session that was still `live` + /// during an earlier sweep is skipped by [_rescoreSessionFromSubstrate] (its + /// tally is still accumulating), and once the frontier moved past its end a + /// window-only rule would skip it forever after it finished — leaving the + /// list showing the stale live-tally strain until someone opened it. Keying + /// on the window too means a retimed session is rescored rather than assumed + /// settled. + final Set _rescoredSessions = {}; /// Re-score recent finished sessions against the substrate now in the DB. /// Called after a drain lands, so a workout whose window arrived late is @@ -2496,17 +2502,33 @@ class LocalRepositoryImpl extends LocalRepository { ))) ?? (nowSec - sinceDays * 86400); final rows = await LocalDb.sessionsInRange(fromTs, nowSec); - // Where the durable record frontier stands NOW; sessions ending at or - // before it are fully covered once this pass has scored them. + // Where the durable record frontier stands NOW. A finished session whose + // window sits behind it has all the substrate it is ever going to get. final frontier = await LocalDb.getCursorInt('rec_ts_hw') ?? 0; + final seen = {}; for (final r in rows) { + final id = r['id']?.toString(); final endTs = (r['end_ts'] as num?)?.toInt(); - if (endTs != null && endTs <= _rescoredThroughTs) continue; + final key = (id == null || endTs == null) ? null : '$id@$endTs'; + if (key != null) seen.add(key); + // Settled: finished, fully covered, and already scored in that state. + if (key != null && + endTs! <= frontier && + _rescoredSessions.contains(key)) { + continue; + } final before = (r['strain'] as num?)?.toDouble(); final after = await _rescoreSessionFromSubstrate(r); if ((after.row['strain'] as num?)?.toDouble() != before) changed++; + // Record it only once it is genuinely finished — a live row gets + // skipped by the helper and must be revisited after it ends. + if (key != null && (r['status']?.toString() ?? '') == 'done') { + _rescoredSessions.add(key); + } } - if (frontier > _rescoredThroughTs) _rescoredThroughTs = frontier; + // Drop anything that aged out of the window so the set can't grow + // without bound across a long-lived process. + _rescoredSessions.retainWhere(seen.contains); } catch (_) { /* best-effort */ } diff --git a/lib/import/import_container.dart b/lib/import/import_container.dart index 5314bd8..d8bf76f 100644 --- a/lib/import/import_container.dart +++ b/lib/import/import_container.dart @@ -153,6 +153,7 @@ Future resolveImportCsvPaths( }) async { final out = []; Directory? tempDir; + var archiveIndex = 0; try { for (final path in paths) { final kind = await sniffFile(path); @@ -162,8 +163,14 @@ Future resolveImportCsvPaths( case ImportContainer.zip: tempDir ??= await Directory.systemTemp.createTemp('openstrap_import_'); + // One subdirectory per archive: the multi-select WHOOP path can hand + // us two exports that each contain `data.csv`, and a shared + // destination made the second overwrite the first (and returned the + // survivor's path twice). + final into = Directory(p.join(tempDir.path, 'a${archiveIndex++}')); + await into.create(recursive: true); out.addAll( - await _extractCsvMembers(path, flavor: flavor, dir: tempDir), + await _extractCsvMembers(path, flavor: flavor, dir: into), ); case ImportContainer.sqlite: throw ImportFormatException( diff --git a/lib/ui/activity/live_session_screen.dart b/lib/ui/activity/live_session_screen.dart index 98efa55..6765c3d 100644 --- a/lib/ui/activity/live_session_screen.dart +++ b/lib/ui/activity/live_session_screen.dart @@ -843,7 +843,9 @@ class _WorkoutFinishScreenState extends State strain > 0 && (strain - tw.value).abs() < 0.15; final ms = recs.record('most_steps'); - final steps = s.steps; + // Prefer the PERSISTED count, like the build path does — the snapshot + // can be empty for a workout whose row already carries real steps. + final steps = (d['steps'] as num?)?.toInt() ?? s.steps; // An unmeasured workout can't set a step record. _prSteps = ms != null && steps != null && diff --git a/test/import_container_test.dart b/test/import_container_test.dart index ce067829f082a389816cb9852637f9b9ef5a86a0..f730fd9add92b281b054abc7a27c36207cc0806c 100644 GIT binary patch delta 203 zcmZn*JQ}(oOwhD>miJ2t|C16q3-*0lER7IY}{jMnH3zYBeV}N*l2x>L?^l=90^pyjz5G J^BlQXoB)M;Lhb+n delta 12 TcmX>a+7`GWO@8xIMFma(B*z5F From 58005b1e21448f3678dc531b4bd56d037720cca8 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 13:01:11 +0530 Subject: [PATCH 09/12] third review pass - the sweep's public default was 7 days and the impl's was 3. dart takes the default from the static receiver type, and every caller holds the interface, so the 3-day bound i added was never the one running. - a session whose rescore THREW was still marked as scored, so a transient db error retired it for the rest of the process. only cache a real read. - the rescore wrote the whole row back after two awaits, which would revert a retime or a finalize that landed in that window. re-reads and bails if the row moved. - the connect-setup priority request bypassed the serialized path and could race the transitions; it goes through the same helper now, and a reply that arrives after teardown re-evaluates for the new session instead of just bailing. - battery: one poll path shared with getBattery (the connect-time read no longer gets duplicated 30s later by the keep-alive) and the timestamp only moves once the write has actually gone out. --- lib/ble/ble_engine.dart | 82 ++++++++++++++++++++--------- lib/data/local_repository.dart | 8 ++- lib/data/local_repository_impl.dart | 26 +++++++-- test/link_priority_policy_test.dart | 14 +++++ 4 files changed, 99 insertions(+), 31 deletions(-) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index 514e5f7..8cd0686 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -615,6 +615,14 @@ class BleEngine { bool _priorityInFlight = false; bool _priorityRestale = false; + /// True from the start of connect setup until INIT has been sent. Setup is + /// discovery + subscribes + SET_CLOCK + INIT and is immediately followed by + /// the first flash drain, so it wants the fast interval for the same reason + /// an offload does — but it must ask for it through [_applyLinkPriority] like + /// everything else, or the direct request races the serialized ones and + /// leaves `_appliedPriority` describing a target the radio never got. + bool _connectSetup = false; + /// Told by AppState on every foreground/background transition. Drives the /// connection interval — see [desiredLinkPriority]. void setBackground(bool value) { @@ -648,7 +656,7 @@ class BleEngine { final session = _session; if (session == null || !session.connected) return; final want = desiredLinkPriority( - offloadActive: _offloadActive, + offloadActive: _offloadActive || _connectSetup, background: _backgrounded, hasLiveConsumer: _liveEnabled && !_liveHrOnly, ); @@ -667,8 +675,12 @@ class BleEngine { // interval per GATT connection); writing this session's target in // afterwards would make the new link skip its own request. if (!identical(_session, session) || !session.connected) { + // Do not record it against the dead link, and do not swallow a + // transition that arrived while we were waiting: loop once more so + // the replacement session (if there is one) gets its own target. _log('Link priority reply arrived after teardown — discarded.'); - return; + _priorityRestale = true; + continue; } _appliedPriority = want; _log('Link priority → ${want.name}.'); @@ -1193,19 +1205,14 @@ class BleEngine { } catch (e) { _log('requestMtu failed: $e — MTU stays at the connection default.'); } - // Start high: connect setup is immediately followed by INIT + the first - // flash drain, which is exactly when throughput matters. `_applyLinkPriority` - // steps it back down as soon as that offload ends (issue #200) — before - // this, `high` was requested here and then held for the entire life of a - // deliberately-permanent connection. - if (Platform.isAndroid) { - try { - await device.requestConnectionPriority( - connectionPriorityRequest: ConnectionPriority.high, - ); - _appliedPriority = LinkPriority.high; - } catch (_) {} - } + // Setup is immediately followed by INIT + the first flash drain, which is + // exactly when throughput matters, so `_connectSetup` asks for the fast + // interval — through the SAME serialized helper as every other + // transition. `_applyLinkPriority` steps it back down once the offload + // ends (issue #200); before that, `high` was requested here and then held + // for the entire life of a deliberately-permanent connection. + _connectSetup = true; + await _applyLinkPriority(); if (!session.connected) { _log('connect: link dropped during setup.'); @@ -1396,17 +1403,30 @@ class BleEngine { // Battery is a DISPLAY value that moves over hours. Polling it on every // 30 s keep-alive tick was 2,880 radio round-trips a day for a handful of // real changes (issue #200). - final lastBattery = _lastBatteryPollAt; - if (lastBattery == null || - DateTime.now().difference(lastBattery).inSeconds >= - kBatteryPollIntervalSeconds) { - _lastBatteryPollAt = DateTime.now(); - _send(Cmd.getBatteryLevel, const []); - } + unawaited(_pollBatteryIfDue()); } DateTime? _lastBatteryPollAt; + /// Ask the band for its battery level, at most once per + /// [kBatteryPollIntervalSeconds]. + /// + /// The stamp moves only after the write actually goes out, so a failed write + /// does not buy five minutes of silence — and [getBattery] shares this path + /// so the read AppState does right after connecting isn't immediately + /// followed by a duplicate from the first keep-alive tick. + Future _pollBatteryIfDue({bool force = false}) async { + final last = _lastBatteryPollAt; + if (!force && + last != null && + DateTime.now().difference(last).inSeconds < + kBatteryPollIntervalSeconds) { + return; + } + await _send(Cmd.getBatteryLevel, const []); + _lastBatteryPollAt = DateTime.now(); + } + /// Trigger a historical offload, floored by [BackfillPolicy] (manual / /// autoContinue are never floored). Re-arms the drain so a fresh HISTORY_COMPLETE /// is awaited. Used by the periodic timer, continuation, and the public sync API. @@ -2845,9 +2865,19 @@ class BleEngine { // ── high-level flows ───────────────────────────────────────────────────────────── Future sendInit() async { _log('Sending 5-packet INIT…'); - for (final pkt in initPackets) { - await _write(pkt); - await Future.delayed(const Duration(milliseconds: 120)); + try { + for (final pkt in initPackets) { + await _write(pkt); + await Future.delayed(const Duration(milliseconds: 120)); + } + } finally { + // Setup is over. The flood INIT triggers raises the link on its own via + // `_setOffloadActive`, so from here the ordinary rules apply — and an + // idle link stops paying for the fast interval. + if (_connectSetup) { + _connectSetup = false; + unawaited(_applyLinkPriority()); + } } } @@ -3039,7 +3069,7 @@ class BleEngine { _log('SET_ADVERTISING_NAME → "$name"'); } - Future getBattery() => _send(Cmd.getBatteryLevel, const []); + Future getBattery() => _pollBatteryIfDue(force: true); Future getHello() => _send(Cmd.getHelloHarvard, const [0x00]); Future buzz() => buzzPattern(hapticShortPulse); diff --git a/lib/data/local_repository.dart b/lib/data/local_repository.dart index 33ad9fd..ef7bb5c 100644 --- a/lib/data/local_repository.dart +++ b/lib/data/local_repository.dart @@ -113,7 +113,13 @@ abstract class LocalRepository { /// DB, correcting a live session whose in-RAM tallies missed the part of the /// workout the app slept through (issue #206). Returns the number of rows /// whose strain changed. Best-effort — never throws. - Future rescoreRecentSessions({int sinceDays = 7}) => + /// + /// The default MUST match the implementation's: Dart resolves an omitted + /// optional from the STATIC receiver type, and every caller holds this + /// interface — so a different default here is the one that actually runs. + /// Three days is the raw-retention horizon; nothing older has substrate left + /// to re-score from. + Future rescoreRecentSessions({int sinceDays = 3}) => throw UnimplementedError('re-layer: rescoreRecentSessions'); Future> startWorkout(String type, {String? title}) => throw UnimplementedError('re-layer: startWorkout'); diff --git a/lib/data/local_repository_impl.dart b/lib/data/local_repository_impl.dart index 365fddf..d64cc98 100644 --- a/lib/data/local_repository_impl.dart +++ b/lib/data/local_repository_impl.dart @@ -2448,8 +2448,22 @@ class LocalRepositoryImpl extends LocalRepository { ); if (!merged.changed) return (row: row, hrRows: hrRows); + // `putSession` is INSERT-OR-REPLACE on the whole row, and everything + // above this point awaited (two substrate reads). A retime or a + // `stopWorkout` finalize landing in that window would be silently + // reverted — old start/end/status written back over the new ones. Re-read + // and bail if the row moved under us; the next sweep (or the next open) + // scores the new window. + final current = await LocalDb.session(id); + if (current == null || + (current['start_ts'] as num?)?.toInt() != startTs || + (current['end_ts'] as num?)?.toInt() != endTs || + current['status']?.toString() != row['status']?.toString()) { + return (row: current ?? row, hrRows: hrRows); + } + final updated = { - ...row, + ...current, 'strain': merged.strain, 'calories': merged.calories, 'max_hr': merged.maxHr, @@ -2520,9 +2534,13 @@ class LocalRepositoryImpl extends LocalRepository { final before = (r['strain'] as num?)?.toDouble(); final after = await _rescoreSessionFromSubstrate(r); if ((after.row['strain'] as num?)?.toDouble() != before) changed++; - // Record it only once it is genuinely finished — a live row gets - // skipped by the helper and must be revisited after it ends. - if (key != null && (r['status']?.toString() ?? '') == 'done') { + // Record it only once it is genuinely finished AND actually scored: a + // live row is skipped by the helper and must be revisited after it + // ends, and a row whose read threw (null rows) would otherwise be + // written off for the rest of the process on a transient DB error. + if (key != null && + after.hrRows != null && + (r['status']?.toString() ?? '') == 'done') { _rescoredSessions.add(key); } } diff --git a/test/link_priority_policy_test.dart b/test/link_priority_policy_test.dart index 6f9a984..c21acaa 100644 --- a/test/link_priority_policy_test.dart +++ b/test/link_priority_policy_test.dart @@ -83,6 +83,20 @@ void main() { } }); + test('connect setup is treated like an offload, not like idle', () { + // Setup runs discovery + subscribes + SET_CLOCK + INIT and is immediately + // followed by the first flash drain, so it wants the fast interval — but it + // asks for it through the same path as everything else. + expect( + desiredLinkPriority( + offloadActive: true, // `_offloadActive || _connectSetup` + background: true, + hasLiveConsumer: false, + ), + LinkPriority.high, + ); + }); + test('the battery poll is minutes apart, not seconds', () { // It rode the 30 s keep-alive tick: 2,880 radio round-trips a day for a // display value that changes a handful of times. From 98bee1dc0d3135ad6606e44e804271716f301926 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 13:23:58 +0530 Subject: [PATCH 10/12] fourth review pass - the battery poll stamped its timestamp even when the write failed (_send reports failure as a return value, and i was ignoring it), so a dropped write silenced the next five minutes of polling. - my connect-setup test was vacuous: it re-asserted the pure rule with offloadActive: true, which the exhaustive offload case above it already covers, and would have passed with the wiring deleted. replaced with one that drives the engine's own state - checked it fails if the _connectSetup term is removed. _doConnect itself still can't run on the test host. --- lib/ble/ble_engine.dart | 21 +++++++++- test/link_priority_policy_test.dart | 64 ++++++++++++++++++++++------- 2 files changed, 69 insertions(+), 16 deletions(-) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index 8cd0686..bce047f 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -623,6 +623,19 @@ class BleEngine { /// leaves `_appliedPriority` describing a target the radio never got. bool _connectSetup = false; + /// What the link should be running at given the engine's CURRENT state. The + /// wiring under test: that `_connectSetup` counts as offload-grade traffic, + /// and that `sendInit` clears it again. + @visibleForTesting + LinkPriority linkPriorityForCurrentState() => desiredLinkPriority( + offloadActive: _offloadActive || _connectSetup, + background: _backgrounded, + hasLiveConsumer: _liveEnabled && !_liveHrOnly, + ); + + @visibleForTesting + void debugBeginConnectSetup() => _connectSetup = true; + /// Told by AppState on every foreground/background transition. Drives the /// connection interval — see [desiredLinkPriority]. void setBackground(bool value) { @@ -1423,8 +1436,12 @@ class BleEngine { kBatteryPollIntervalSeconds) { return; } - await _send(Cmd.getBatteryLevel, const []); - _lastBatteryPollAt = DateTime.now(); + // `_send` swallows write failures and reports them as false. Stamping + // regardless would buy five minutes of silence off a write that never left + // the phone. + if (await _send(Cmd.getBatteryLevel, const [])) { + _lastBatteryPollAt = DateTime.now(); + } } /// Trigger a historical offload, floored by [BackfillPolicy] (manual / diff --git a/test/link_priority_policy_test.dart b/test/link_priority_policy_test.dart index c21acaa..d3275b8 100644 --- a/test/link_priority_policy_test.dart +++ b/test/link_priority_policy_test.dart @@ -11,6 +11,7 @@ // throughput during a drain is what the fast interval was for. import 'package:flutter_test/flutter_test.dart'; +import 'package:openstrap_edge/ble/ble_engine.dart'; import 'package:openstrap_edge/sync/sync_policy.dart'; void main() { @@ -83,20 +84,6 @@ void main() { } }); - test('connect setup is treated like an offload, not like idle', () { - // Setup runs discovery + subscribes + SET_CLOCK + INIT and is immediately - // followed by the first flash drain, so it wants the fast interval — but it - // asks for it through the same path as everything else. - expect( - desiredLinkPriority( - offloadActive: true, // `_offloadActive || _connectSetup` - background: true, - hasLiveConsumer: false, - ), - LinkPriority.high, - ); - }); - test('the battery poll is minutes apart, not seconds', () { // It rode the 30 s keep-alive tick: 2,880 radio round-trips a day for a // display value that changes a handful of times. @@ -105,4 +92,53 @@ void main() { // starves `sinceLastRx` and bounces a healthy link. expect(kBatteryPollIntervalSeconds, greaterThan(kLivenessFuseSeconds)); }); + + group('the engine feeds its own state into that rule', () { + // The policy tests above prove the RULE. These prove the WIRING, which is + // where the bug actually was: the connect-setup boost used to be a direct + // radio call that bypassed the serialized path entirely. + // + // Honest limit: `_doConnect` cannot run on the test host (flutter_blue_plus + // is unsupported there), so what is covered is the flag's effect on the + // target and `sendInit`'s clearing of it — not the assignment inside + // `_doConnect` itself. + late BleEngine engine; + + setUp(() { + TestWidgetsFlutterBinding.ensureInitialized(); + engine = BleEngine( + onRecord: (sample, raw) async {}, + onState: (_) {}, + log: (_) {}, + ); + }); + + test('a backgrounded idle engine wants the cheap interval', () { + engine.setBackground(true); + expect(engine.linkPriorityForCurrentState(), LinkPriority.lowPower); + }); + + test('connect setup outranks being backgrounded', () { + engine.setBackground(true); + engine.debugBeginConnectSetup(); + expect( + engine.linkPriorityForCurrentState(), + LinkPriority.high, + reason: 'setup is immediately followed by the first flash drain', + ); + }); + + test('sendInit ends the setup boost', () async { + engine.setBackground(true); + engine.debugBeginConnectSetup(); + // No session on the host, so the writes fail — the point is that the + // flag is cleared in a `finally`, not only on the happy path. + await engine.sendInit(); + expect( + engine.linkPriorityForCurrentState(), + LinkPriority.lowPower, + reason: 'an idle background link must stop paying for setup speed', + ); + }); + }); } From cfbff28094075c4b40f2e79d83fd4818ee2015a2 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 13:56:04 +0530 Subject: [PATCH 11/12] self-review pass before merge reviewed the whole diff again since the bots stopped running (rate limited), and found real problems in my own fixes: reconnect (the #208 work): - the supervisor called a healthy loop wedged. staleness was measured from the LOOP's start with a 20 min threshold, but a single android autoConnect pass legitimately waits 15 min, so any band away longer than ~20 min got its loop torn down mid-attempt - and the abandoned attempt's disconnect() then cancelled the OS pending connect its replacement was waiting on. that's the never-reconnects symptom, caused by the thing meant to cure it. measured per-attempt now, threshold 25 min. - the supervisor could also start a loop underneath a user-initiated connect, which on android can outlast a 60s tick (bond dialog). it stands down while one is in flight. - supervisor kept ticking after unpair/endSession. battery + link priority (the #200 work): - 5-minute battery polling removed the only inbound traffic a quiet link has: _lastRx only advances on a notification, LINK_VALID is a write, so a link with no live stream would have crossed the 120s liveness fuse and been bounced every couple of minutes. it now forces a poll as silence approaches the fuse, keeping the power win when streams are flowing. - backgrounding skipped the ONLY periodic re-plan of the high-frequency wake window, so a band connected at 22:00 and left connected never armed high-freq sync for that night. the skip now covers the offload only. - a priority reply landing during teardown could restore the value teardown had just cleared, so the next connection thought it had already asked and never requested an interval. link generation check. - _connectSetup wasn't cleared when a connect failed before INIT, pinning the target at high for the life of the process. - an expired bond-refusal pause left 're-pair required' on screen with auto-reconnect silently re-armed behind it. session rescore (the #206 work): - the skip-set was stamped without the frontier check the skip itself uses, so a session scored while the band had handed over only part of its window was marked settled and the drain carrying the REST of it skipped the session entirely. that defeated the whole point of the sweep. - max-merging every re-score was only sound for a fixed scoring function. strain depends on the trailing nightly resting HR, which moves, so a session ratcheted up to whatever the most favourable RHR ever produced. once the substrate covers the window it now replaces the tally outright. - persisted max_hr was the raw 1 Hz peak, writing a PPG spike into the column getWorkout deliberately refuses to trust - and after the 3-day prune there'd be no smoothed value left to prefer. stores the smoothed peak. - the write was a whole-row REPLACE, which reverted hrr_bpm and type changes that land through their own narrow updates. score columns only now. - the concurrency bail handed back HR rows for the OLD window, which getWorkout then used to enrich the new one. import: - zip extraction buffered the whole archive and each member in memory, on the exact hundreds-of-megabytes export the size ceiling was written for. streams through InputFileStream/OutputFileStream now. - an archive with several CSVs silently imported one and reported success. - a UTF-16 CSV was called 'not a text file'. steps: - the phone-steps cache had no day key, so past midnight it kept yesterday's answer and suppressed the band's live count all day. keyed by day, refreshed on derive and on foreground return. - the re-derive after enabling phone steps was dropped whenever another derive was already running. - today's 'measured' tag rendered in warning amber. --- lib/ble/ble_engine.dart | 47 ++++++++++++- lib/compute/manual_session.dart | 36 ++++++++-- lib/data/db.dart | 29 ++++++++ lib/data/local_repository_impl.dart | 75 +++++++++++++++++--- lib/import/import_container.dart | 57 +++++++++++---- lib/import/noop_import.dart | 29 ++++++-- lib/state/app_state.dart | 96 ++++++++++++++++++++------ lib/sync/sync_policy.dart | 32 +++++++-- lib/ui/today/today_screen.dart | 5 +- lib/ui/workouts/workouts_screen.dart | 3 +- test/reconnect_supervisor_test.dart | 40 ++++++++--- test/session_score_reconcile_test.dart | 39 +++++++++++ 12 files changed, 412 insertions(+), 76 deletions(-) diff --git a/lib/ble/ble_engine.dart b/lib/ble/ble_engine.dart index bce047f..63a0560 100644 --- a/lib/ble/ble_engine.dart +++ b/lib/ble/ble_engine.dart @@ -615,6 +615,14 @@ class BleEngine { bool _priorityInFlight = false; bool _priorityRestale = false; + /// Bumped by every teardown. Captured before a priority request and re-checked + /// after it: `_teardownSession` clears `_appliedPriority` at its top but nulls + /// `_session` only after awaiting subscription cancels, so an identity check + /// on the session alone still passes inside that window — and the reply then + /// restores the value teardown had just cleared, leaving the NEXT connection + /// convinced it had already asked. + int _linkGeneration = 0; + /// True from the start of connect setup until INIT has been sent. Setup is /// discovery + subscribes + SET_CLOCK + INIT and is immediately followed by /// the first flash drain, so it wants the fast interval for the same reason @@ -674,6 +682,7 @@ class BleEngine { hasLiveConsumer: _liveEnabled && !_liveHrOnly, ); if (want == _appliedPriority) continue; + final generation = _linkGeneration; try { await session.device.requestConnectionPriority( connectionPriorityRequest: switch (want) { @@ -687,7 +696,9 @@ class BleEngine { // the next session re-requests from scratch (Android resets the // interval per GATT connection); writing this session's target in // afterwards would make the new link skip its own request. - if (!identical(_session, session) || !session.connected) { + if (generation != _linkGeneration || + !identical(_session, session) || + !session.connected) { // Do not record it against the dead link, and do not swallow a // transition that arrived while we were waiting: loop once more so // the replacement session (if there is one) gets its own target. @@ -698,7 +709,11 @@ class BleEngine { _appliedPriority = want; _log('Link priority → ${want.name}.'); } catch (e) { - // Leave `_appliedPriority` alone so the next transition retries. + // Leave `_appliedPriority` alone so this is retried. The retry is the + // keep-alive tick calling back in, NOT this loop — spinning here + // against a radio that just refused would hammer it. Without a + // retry at all, a failed step-DOWN would hold the fast interval + // until the next state change, which overnight means until morning. _log('requestConnectionPriority(${want.name}) failed: $e'); } } while (_priorityRestale); @@ -750,6 +765,12 @@ class BleEngine { if (!state.autoReconnectPaused) return false; if (_bondGiveUp.stillPaused(DateTime.now())) return true; state.autoReconnectPaused = false; + // Clear what the pause put on screen, too. Leaving `needsRepairGuide` set + // tells the user to re-pair while auto-reconnect has quietly re-armed + // behind the message, and a `bondRefusals` count that keeps climbing while + // the give-up streak restarts at 1 no longer means anything. + state.needsRepairGuide = false; + state.bondRefusals = 0; _log('[RECONNECT] bond-refusal pause expired — auto-reconnect re-armed.'); onState(state); return false; @@ -1416,7 +1437,22 @@ class BleEngine { // Battery is a DISPLAY value that moves over hours. Polling it on every // 30 s keep-alive tick was 2,880 radio round-trips a day for a handful of // real changes (issue #200). - unawaited(_pollBatteryIfDue()); + // + // BUT it is also load-bearing for liveness: `_lastRx` only advances on an + // inbound notification, and with no live stream armed the battery REPLY is + // the only inbound traffic this link generates (LINK_VALID is a write; the + // band is not known to answer it). Left purely on a 5-minute cadence, a + // quiet link would sail past the 120 s fuse and get bounced — trading a + // power win for a reconnect storm. So: poll on the slow cadence normally, + // and force one as soon as silence approaches the fuse. + unawaited( + _pollBatteryIfDue( + force: sinceLastRx.inSeconds > kLivenessFuseSeconds ~/ 2, + ), + ); + // Cheap retry hook for a priority request that failed earlier: a no-op + // whenever the link already sits at the wanted interval. + unawaited(_applyLinkPriority()); } DateTime? _lastBatteryPollAt; @@ -3272,6 +3308,11 @@ class BleEngine { // level once rather than inheriting the last link's 5-minute cooldown. _appliedPriority = null; _lastBatteryPollAt = null; + // Every failure exit in `_doConnect` between setting this and `sendInit` + // skips the clear in sendInit's finally, which would leave the target + // pinned at `high` for the life of the process. + _connectSetup = false; + _linkGeneration++; _drain?.onLinkDown(); _drain = null; // Fire a final derive for anything stored-but-not-yet-derived, then disarm the diff --git a/lib/compute/manual_session.dart b/lib/compute/manual_session.dart index 2240d11..8aa8653 100644 --- a/lib/compute/manual_session.dart +++ b/lib/compute/manual_session.dart @@ -476,6 +476,21 @@ ReconciledSessionScore reconcileSessionScore({ required int? liveMaxHr, required List liveZoneMinutes, required ManualSessionStats substrate, + + /// True when [substrate] covers essentially the whole window, i.e. the band + /// has finished handing this workout over. + /// + /// It then REPLACES the live tally rather than being maxed against it, and + /// that distinction matters more than it looks. The `max` rule is only + /// monotone while the scoring function is fixed, and it is not: the score + /// depends on the trailing nightly resting HR and on Tanaka HRmax, both of + /// which move. Maxing every re-score against the stored value would make a + /// session converge to the highest strain ANY resting-HR the profile has + /// ever reported would have produced — one artefactually low nightly RHR + /// would inflate a workout permanently, with no way back down. Once the + /// window is fully covered there is nothing left to recover, so the honest + /// value is simply the current score. + bool substrateIsComplete = false, }) { // No substrate for this window (not drained yet, or pruned) — the live tally // is all the evidence there is. @@ -488,7 +503,11 @@ ReconciledSessionScore reconcileSessionScore({ ); } + // Complete coverage: the substrate IS the answer. Partial: both sides are + // lower bounds over subsets of the same minutes, so the larger is the better + // estimate and the smaller is just a less complete view. double? better(double? a, double? b) { + if (substrateIsComplete) return b ?? a; if (a == null) return b; if (b == null) return a; return a >= b ? a : b; @@ -496,17 +515,22 @@ ReconciledSessionScore reconcileSessionScore({ final strain = better(liveStrain, substrate.strain); final calories = better(liveCalories, substrate.calories); - final maxHr = liveMaxHr == null - ? substrate.maxHr - : (substrate.maxHr == null - ? liveMaxHr - : (liveMaxHr >= substrate.maxHr! ? liveMaxHr : substrate.maxHr)); + final maxHr = substrateIsComplete + ? (substrate.maxHr ?? liveMaxHr) + : (liveMaxHr == null + ? substrate.maxHr + : (substrate.maxHr == null + ? liveMaxHr + : (liveMaxHr >= substrate.maxHr! + ? liveMaxHr + : substrate.maxHr))); // Zone minutes are a vector of the same lower-bound quantity, so take the // side with more total measured minutes rather than mixing two partial // splits (a per-element max would invent a total neither source observed). double total(List z) => z.fold(0.0, (a, b) => a + b); - final zone = total(substrate.zoneMinutes) > total(liveZoneMinutes) + final zone = substrateIsComplete || + total(substrate.zoneMinutes) > total(liveZoneMinutes) ? substrate.zoneMinutes : liveZoneMinutes; diff --git a/lib/data/db.dart b/lib/data/db.dart index dc9f7aa..2f880d2 100644 --- a/lib/data/db.dart +++ b/lib/data/db.dart @@ -4545,6 +4545,35 @@ class LocalDb { ); } + /// Update ONLY a session's derived score columns. + /// + /// Deliberately not `putSession`: that is INSERT-OR-REPLACE over the whole + /// row, so a re-score computed from a snapshot would also rewrite columns it + /// never read — `hrr_bpm` (backfilled by the derivation engine) and `type` + /// (the athlete correcting a mislabelled workout) are both written by their + /// own narrow UPDATEs and would be reverted. Returns the number of rows + /// changed (0 when the session has since been deleted). + static Future setSessionScores( + String id, { + required double? strain, + required double? calories, + required int? maxHr, + required String zoneMinJson, + }) async { + final db = await instance; + return db.update( + 'sessions', + { + 'strain': strain, + 'calories': calories, + 'max_hr': maxHr, + 'zone_min_json': zoneMinJson, + }, + where: 'id = ?', + whereArgs: [id], + ); + } + static Future?> session(String id) async { final db = await instance; final rows = await db.query( diff --git a/lib/data/local_repository_impl.dart b/lib/data/local_repository_impl.dart index d64cc98..f50c1c8 100644 --- a/lib/data/local_repository_impl.dart +++ b/lib/data/local_repository_impl.dart @@ -2427,16 +2427,38 @@ class LocalRepositoryImpl extends LocalRepository { if (hrRows.isEmpty) return (row: row, hrRows: hrRows); final profile = Profile.fromMap(getProfileMap()); - final stats = computeManualSessionStats( + final hrBpm = [for (final e in hrRows) (e['hr'] as num).toInt()]; + final raw = computeManualSessionStats( hrTs: [for (final e in hrRows) (e['rec_ts'] as num).toInt()], - hrBpm: [for (final e in hrRows) (e['hr'] as num).toInt()], + hrBpm: hrBpm, profile: profile, zoneMaxHr: _profileMaxHr().toDouble(), restingHr: await _recentRestingHr() ?? profile.restingHrManual?.toDouble(), ); + // `computeManualSessionStats` reports the raw 1 Hz peak. Persisting that + // writes a PPG spike into the column `getWorkout` deliberately refuses to + // floor against (issue #127) — and once raw ages out past retention the + // list has no smoothed value left to prefer, so the artefact would become + // permanent. Store the spike-suppressed peak instead. + final stats = ManualSessionStats( + avgHr: raw.avgHr, + maxHr: smoothedMaxHr(hrBpm, age: _profileAge()) ?? raw.maxHr, + strain: raw.strain, + calories: raw.calories, + zoneMinutes: raw.zoneMinutes, + hrSampleCount: raw.hrSampleCount, + ); + + // "Complete" = the band has handed over essentially the whole window. + // 1 Hz means one sample per second, so sample count vs window seconds is + // the coverage ratio; 90% absorbs the usual handful of dropped seconds. + final windowSec = endTs - startTs; + final complete = + windowSec > 0 && stats.hrSampleCount >= (windowSec * 0.9).floor(); final merged = reconcileSessionScore( + substrateIsComplete: complete, liveStrain: (row['strain'] as num?)?.toDouble(), liveCalories: (row['calories'] as num?)?.toDouble(), liveMaxHr: (row['max_hr'] as num?)?.toInt(), @@ -2459,21 +2481,35 @@ class LocalRepositoryImpl extends LocalRepository { (current['start_ts'] as num?)?.toInt() != startTs || (current['end_ts'] as num?)?.toInt() != endTs || current['status']?.toString() != row['status']?.toString()) { - return (row: current ?? row, hrRows: hrRows); + // The row moved under us. Return the fresh row but NOT the rows we + // read — they describe the old window, and `getWorkout` would enrich + // the new one with them (a negative time-to-peak, zones over the wrong + // span). The next pass scores the new window. + return (row: current ?? row, hrRows: null); } + final zoneJson = jsonEncode( + merged.zoneMinutes.any((v) => v > 0) ? merged.zoneMinutes : const [], + ); + // Score columns ONLY, via a targeted UPDATE. `putSession` is + // INSERT-OR-REPLACE over the whole row, so it also rewrites columns this + // code never looked at — `hrr_bpm` (backfilled by the derive) and `type` + // (the user's own correction) are both written by narrow UPDATEs that + // the re-read above cannot detect. + await LocalDb.setSessionScores( + id, + strain: merged.strain, + calories: merged.calories, + maxHr: merged.maxHr, + zoneMinJson: zoneJson, + ); final updated = { ...current, 'strain': merged.strain, 'calories': merged.calories, 'max_hr': merged.maxHr, - 'zone_min_json': jsonEncode( - merged.zoneMinutes.any((v) => v > 0) - ? merged.zoneMinutes - : const [], - ), + 'zone_min_json': zoneJson, }; - await LocalDb.putSession(updated); return (row: updated, hrRows: hrRows); } catch (_) { return (row: row, hrRows: null); // best-effort: the stored row renders @@ -2518,7 +2554,12 @@ class LocalRepositoryImpl extends LocalRepository { final rows = await LocalDb.sessionsInRange(fromTs, nowSec); // Where the durable record frontier stands NOW. A finished session whose // window sits behind it has all the substrate it is ever going to get. - final frontier = await LocalDb.getCursorInt('rec_ts_hw') ?? 0; + // Falls back to the newest decoded row so an import-only install (no + // band, so no `rec_ts_hw` cursor) still gets the skip rather than + // re-scanning every session's window on every pass. + final frontier = await LocalDb.getCursorInt('rec_ts_hw') ?? + await LocalDb.lastDecodedRecTs() ?? + 0; final seen = {}; for (final r in rows) { final id = r['id']?.toString(); @@ -2533,12 +2574,24 @@ class LocalRepositoryImpl extends LocalRepository { } final before = (r['strain'] as num?)?.toDouble(); final after = await _rescoreSessionFromSubstrate(r); - if ((after.row['strain'] as num?)?.toDouble() != before) changed++; + // Count only what THIS pass wrote: the bail path returns a re-read row + // whose strain may differ for reasons we had nothing to do with. + if (after.hrRows != null && + (after.row['strain'] as num?)?.toDouble() != before) { + changed++; + } // Record it only once it is genuinely finished AND actually scored: a // live row is skipped by the helper and must be revisited after it // ends, and a row whose read threw (null rows) would otherwise be // written off for the rest of the process on a transient DB error. + // The insertion condition MUST match the skip condition, `endTs <= + // frontier` included. Without it, a workout scored while the band had + // only handed over part of its window got stamped as settled, and the + // next drain — the one carrying the REST of that window — skipped it. + // The partial score then stood on the list until someone opened the + // detail screen, which is exactly the case the sweep exists for. if (key != null && + endTs! <= frontier && after.hrRows != null && (r['status']?.toString() ?? '') == 'done') { _rescoredSessions.add(key); diff --git a/lib/import/import_container.dart b/lib/import/import_container.dart index d8bf76f..c23f197 100644 --- a/lib/import/import_container.dart +++ b/lib/import/import_container.dart @@ -45,6 +45,9 @@ enum ImportContainer { /// gzip — not a container we unwrap, but worth naming precisely. gzip, + /// Text, but UTF-16 rather than UTF-8 — re-savable by the user. + utf16, + /// Binary of some other kind. binary, } @@ -65,6 +68,13 @@ class ImportFormatException implements Exception { /// Everything else is called text unless it holds a NUL or a run of control /// bytes, which no CSV export contains. ImportContainer sniffImportContainer(List head) { + // UTF-16 (what Excel writes for "Unicode text") is full of NUL bytes and + // would otherwise be called binary — technically true, useless to the user. + if (head.length >= 2 && + ((head[0] == 0xFF && head[1] == 0xFE) || + (head[0] == 0xFE && head[1] == 0xFF))) { + return ImportContainer.utf16; + } if (head.length >= 4 && head[0] == 0x50 && head[1] == 0x4B && @@ -183,6 +193,11 @@ Future resolveImportCsvPaths( '“${p.basename(path)}” is a gzip archive. Unzip it first and pick ' 'the CSV inside.', ); + case ImportContainer.utf16: + throw ImportFormatException( + '“${p.basename(path)}” is saved as UTF-16 text. Re-save it as ' + 'UTF-8 CSV and import it again.', + ); case ImportContainer.binary: throw ImportFormatException( '“${p.basename(path)}” is not a text file, so there is nothing to ' @@ -205,9 +220,16 @@ Future> _extractCsvMembers( }) async { final name = p.basename(path); final Archive archive; + // STREAMED, not `decodeBytes(readAsBytes())`. A 90-day NOOP raw export is + // hundreds of megabytes; buffering the whole archive AND then each member in + // memory would OOM the phone on exactly the export this path exists to + // import — and the rest of the import pipeline is carefully streamed for the + // same reason. `InputFileStream` reads the archive off disk as it decodes. + final input = InputFileStream(path); try { - archive = ZipDecoder().decodeBytes(await File(path).readAsBytes()); + archive = ZipDecoder().decodeStream(input); } catch (e) { + await input.close(); throw ImportFormatException( 'Could not read “$name” as an archive: $e', ); @@ -261,22 +283,33 @@ Future> _extractCsvMembers( final out = []; final used = {}; - for (final f in csvFiles) { + try { + for (final f in csvFiles) { // Members can share a basename (`daily/data.csv`, `workouts/data.csv`). // Flattening them onto one destination silently dropped one file and // parsed the survivor twice. - var base = p.basename(f.name); - if (!used.add(base)) { - final stem = p.basenameWithoutExtension(base); - final ext = p.extension(base); - var n = 2; - while (!used.add(base = '$stem-$n$ext')) { - n++; + var base = p.basename(f.name); + if (!used.add(base)) { + final stem = p.basenameWithoutExtension(base); + final ext = p.extension(base); + var n = 2; + while (!used.add(base = '$stem-$n$ext')) { + n++; + } + } + final destPath = p.join(dir.path, base); + // `writeContent` decompresses straight to disk — never materialising the + // member, which for a raw sensor export is the big one. + final sink = OutputFileStream(destPath); + try { + f.writeContent(sink); + } finally { + await sink.close(); } + out.add(destPath); } - final dest = File(p.join(dir.path, base)); - await dest.writeAsBytes(f.content as List); - out.add(dest.path); + } finally { + await input.close(); } return out; } diff --git a/lib/import/noop_import.dart b/lib/import/noop_import.dart index e17d1be..1db618d 100644 --- a/lib/import/noop_import.dart +++ b/lib/import/noop_import.dart @@ -46,6 +46,11 @@ import '../compute/substrate.dart'; import '../data/db.dart'; import 'import_container.dart'; +/// Last path segment, for error messages (avoids a `package:path` import just +/// for this one use). +String _basenameOf(String path) => + path.contains('/') ? path.substring(path.lastIndexOf('/') + 1) : path; + class NoopImportResult { final int days; final int rows; @@ -131,13 +136,23 @@ class NoopImporter { 'That archive holds no NOOP CSV export.', ); } - // A NOOP raw-sensor export is a single CSV; if an archive carried several, - // prefer one that actually looks like the raw-sensor file. - final chosen = resolved.paths.firstWhere( - (p) => p.toLowerCase().contains('raw-sensor'), - orElse: () => resolved.paths.first, - ); - file = File(chosen); + // A NOOP raw-sensor export is ONE CSV. An archive holding several is not + // something to guess at: this importer streams a single file and derives in + // a rolling two-day window, so silently taking the first match would import + // a fraction of the archive and still report success. + final candidates = resolved.paths + .where((p) => p.toLowerCase().contains('raw-sensor')) + .toList(); + final usable = candidates.isEmpty ? resolved.paths : candidates; + if (usable.length > 1) { + final names = usable.map(_basenameOf).take(4).join(', '); + await resolved.dispose(); + throw ImportFormatException( + 'That archive holds ${usable.length} CSV files ($names…). Import the ' + 'raw sensor CSV on its own so nothing is silently skipped.', + ); + } + file = File(usable.first); try { return await _importResolvedFile( diff --git a/lib/state/app_state.dart b/lib/state/app_state.dart index 44f1e76..6f3b5b1 100644 --- a/lib/state/app_state.dart +++ b/lib/state/app_state.dart @@ -179,15 +179,16 @@ class AppState extends ChangeNotifier { bool _keepAlive = false; bool _reconnecting = false; - /// When the current reconnect loop started, for the supervisor's staleness - /// check (issue #208) — an await inside the loop that never returns leaves - /// `_reconnecting` true forever, which no re-trigger can clear. - DateTime? _reconnectingSince; + /// When the current reconnect ATTEMPT started, for the supervisor's + /// staleness check (issue #208). Per-attempt, not per-loop: a loop against a + /// band left at home legitimately runs for hours, so loop age says nothing + /// about whether anything is stuck — only an attempt that never returns does. + DateTime? _attemptStartedAt; /// Which reconnect loop is the live one. Bumped whenever a loop starts, so a /// loop that was declared wedged and replaced can recognise itself as /// superseded if it ever unblocks: without this its `finally` would clear the - /// REPLACEMENT's `_reconnecting`/`_reconnectingSince`, and the supervisor + /// REPLACEMENT's `_reconnecting`/`_attemptStartedAt`, and the supervisor /// would then start a third loop while two are already connecting. int _reconnectGeneration = 0; @@ -484,6 +485,15 @@ class AppState extends ChangeNotifier { if (ok) { unawaited(() async { await syncPhoneSteps(days: PhonePedometer.fullSyncDays); + // `_reanalyzeForOverride` no-ops while another derive is running, and + // the full sync above takes long enough (7 days of hourly platform + // reads) that a drain-triggered pass can easily have started. Dropping + // it silently leaves the freshly-banked rows out of `day_result` and + // the tile on a dash — the exact "looks broken" symptom this call was + // added to prevent. Wait for the other pass, bounded, then run. + for (var i = 0; i < 60 && reanalyzing; i++) { + await Future.delayed(const Duration(seconds: 1)); + } await _reanalyzeForOverride(); }()); } @@ -516,6 +526,7 @@ class AppState extends ChangeNotifier { phoneStepsLastSyncedDays = null; phoneStepsLastTotal = null; phoneStepsToday = 0; + _phoneStepsDay = null; try { await LocalDb.clearPhoneCoverage(); } catch (e) { @@ -543,15 +554,30 @@ class AppState extends ChangeNotifier { /// the day and its live steps were being thrown away. int phoneStepsToday = 0; + /// Which local day [phoneStepsToday] was read for. The cache is worthless + /// past midnight, and a process here routinely lives for days (Android + /// foreground service, iOS suspend/resume), so a day-less cache would hold + /// yesterday's answer through the whole of today — suppressing the band's + /// live count on a day the phone has not contributed a single step to. + String? _phoneStepsDay; + /// True when today's step total comes from the phone, so band live steps are - /// already accounted for and must not be added again. - bool get todayStepsFromPhone => phoneStepsEnabled && phoneStepsToday > 0; + /// already accounted for and must not be added again. Unknown-or-stale reads + /// as false: showing the band's live count is the safe direction (a lost + /// count is invisible, a doubled one is a wrong number). + bool get todayStepsFromPhone => + phoneStepsEnabled && + _phoneStepsDay == todayLabel() && + phoneStepsToday > 0; Future _refreshPhoneStepsToday() async { + if (!phoneStepsEnabled) return; try { - final n = await LocalDb.phoneStepsForDay(todayLabel()); - if (n != phoneStepsToday) { + final day = todayLabel(); + final n = await LocalDb.phoneStepsForDay(day); + if (n != phoneStepsToday || day != _phoneStepsDay) { phoneStepsToday = n; + _phoneStepsDay = day; notifyListeners(); } } catch (_) { @@ -976,8 +1002,7 @@ class AppState extends ChangeNotifier { // ChangeNotifier (which throws in release). _tapSub?.cancel(); _stopBackfillTimer(); - _reconnectSupervisor?.cancel(); - _reconnectSupervisor = null; + _stopReconnectSupervisor(); _alarmGraceTimer?.cancel(); _alarmGraceTimer = null; _spotTimer?.cancel(); @@ -1089,6 +1114,9 @@ class AppState extends ChangeNotifier { }, )); TelemetryService.instance.breadcrumb('derive: $mode done'); + // A drain can bank band coverage and a day can have rolled over since the + // last read — both change which source owns today's steps. + unawaited(_refreshPhoneStepsToday()); // The drain that triggered this pass may have landed the 1 Hz window of a // workout the app slept through, whose strain/calories were scored from // whatever few minutes the foreground tally saw (issue #206). Re-score @@ -2527,6 +2555,14 @@ class AppState extends ChangeNotifier { ); } + /// Stop supervising. Called from `dispose` and from every path that stops + /// wanting a link at all (unpair / endSession) — otherwise the tick outlives + /// its purpose and keeps poking the engine once a minute forever. + void _stopReconnectSupervisor() { + _reconnectSupervisor?.cancel(); + _reconnectSupervisor = null; + } + void _superviseReconnect() { if (_disposed) return; // Expire a bond-refusal pause whose cooldown has run out before deciding — @@ -2539,9 +2575,10 @@ class AppState extends ChangeNotifier { connected: engine.isConnected, loopRunning: _reconnecting, autoReconnectPaused: device.autoReconnectPaused, - loopRunningFor: _reconnectingSince == null + connectInFlight: busy, + attemptRunningFor: _attemptStartedAt == null ? null - : DateTime.now().difference(_reconnectingSince!), + : DateTime.now().difference(_attemptStartedAt!), ); switch (action) { case ReconnectSupervisorAction.none: @@ -2551,11 +2588,11 @@ class AppState extends ChangeNotifier { 'starting one.'); unawaited(_reconnect()); case ReconnectSupervisorAction.restartStale: - _log('[RECONNECT] supervisor: the loop has been running since ' - '$_reconnectingSince with no link — treating it as wedged and ' - 'starting a fresh one.'); + _log('[RECONNECT] supervisor: the current attempt has been running ' + 'since $_attemptStartedAt with no link — treating it as wedged ' + 'and starting a fresh loop.'); _reconnecting = false; - _reconnectingSince = null; + _attemptStartedAt = null; unawaited(_reconnect()); } } @@ -2583,8 +2620,19 @@ class AppState extends ChangeNotifier { // all night, bypassing the very rate limit written to prevent that (issue // #200). Foreground keeps the faster cadence: the user can see the data. if (_background) { + // The OFFLOAD is what we're skipping — the engine's own floored timer + // owns that. The wake-window re-plan is NOT the engine's: nothing else + // re-evaluates it on a stable connection, and it only flips on as the + // 90-minute pre-wake window opens. Skipping it outright meant a band + // that connected at 22:00 and stayed connected never armed high-frequency + // sync for that night at all. + try { + await _refreshHighFreqWakeWindow(); + } catch (e) { + _log('Wake-window refresh failed: $e'); + } _log('Periodic history refresh skipped — backgrounded; the engine\'s ' - 'floored 15-min backfill owns this.'); + 'floored 15-min backfill owns the offload.'); return; } if (_syncBurst != null) { @@ -2793,6 +2841,7 @@ class AppState extends ChangeNotifier { _keepAlive = false; BandOwnership.markForegroundIntent(false); _stopBackfillTimer(); + _stopReconnectSupervisor(); IosBleRestore.foregroundActive = false; await EdgeTracking.stop(); await IosBleRestore.disarm(); @@ -3057,6 +3106,11 @@ class AppState extends ChangeNotifier { final wasBackground = _background; _background = false; engine.setBackground(false); + // Coming back after hours (or days) suspended: re-read the phone's steps + // for whatever day it is NOW. + if (phoneStepsEnabled) { + unawaited(syncPhoneSteps()); + } // Back in the foreground with an OS CPU/memory budget again — let the // scheduler drain any derive jobs that queued (durably) while backgrounded. _deriveScheduler.setBackground(false); @@ -3210,7 +3264,7 @@ class AppState extends ChangeNotifier { return; } _reconnecting = true; - _reconnectingSince = DateTime.now(); + _attemptStartedAt = DateTime.now(); final generation = ++_reconnectGeneration; BandOwnership.markForegroundIntent(true); _log('[OWNERSHIP] reconnect intent on (${BandOwnership.debugState})'); @@ -3226,6 +3280,7 @@ class AppState extends ChangeNotifier { !device.autoReconnectPaused && generation == _reconnectGeneration) { attempt++; + _attemptStartedAt = DateTime.now(); // Surface `reconnecting` while the loop backs off, so the UI shows a // connecting-style state instead of flat 'disconnected'. engine.markReconnecting(); @@ -3339,7 +3394,7 @@ class AppState extends ChangeNotifier { _log('[OWNERSHIP] reconnect intent off (${BandOwnership.debugState})'); } _reconnecting = false; - _reconnectingSince = null; + _attemptStartedAt = null; // If we gave up (keepAlive dropped / never connected), stop advertising // `reconnecting` — fall back to a truthful 'disconnected'. No-op when // the loop exited via a successful connect (phase is `listening`). @@ -3445,6 +3500,7 @@ class AppState extends ChangeNotifier { BandOwnership.markForegroundIntent(false); _log('[OWNERSHIP] endSession intent off (${BandOwnership.debugState})'); _stopBackfillTimer(); + _stopReconnectSupervisor(); await engine.disconnect(); _releaseForegroundLease(); } diff --git a/lib/sync/sync_policy.dart b/lib/sync/sync_policy.dart index e5f59c7..b4eb7d0 100644 --- a/lib/sync/sync_policy.dart +++ b/lib/sync/sync_policy.dart @@ -806,17 +806,35 @@ ReconnectSupervisorAction superviseReconnect({ required bool connected, required bool loopRunning, required bool autoReconnectPaused, - required Duration? loopRunningFor, - Duration staleAfter = const Duration(minutes: 20), + + /// Time since the CURRENT attempt started — not since the loop did. A loop + /// legitimately runs for hours against a band left at home; an individual + /// attempt does not. + required Duration? attemptRunningFor, + + /// Something else is already driving a connect (a user-initiated + /// `openSession`). Starting a second loop underneath it makes two callers + /// race the same peripheral and re-run the whole post-connect block. + bool connectInFlight = false, + Duration staleAfter = const Duration(minutes: 25), }) { - if (!paired || !keepAlive || connected || autoReconnectPaused) { + if (!paired || + !keepAlive || + connected || + autoReconnectPaused || + connectInFlight) { return ReconnectSupervisorAction.none; } if (!loopRunning) return ReconnectSupervisorAction.start; - // A live loop is expected to run for a long time — a band can be out of range - // for hours, and the Android OS-autoConnect branch legitimately waits 15 - // minutes per pass. Only call it stale well past that. - if (loopRunningFor != null && loopRunningFor >= staleAfter) { + // MUST stay comfortably above the longest legitimate single attempt. The + // Android OS-autoConnect branch waits up to 15 minutes per pass, so a + // threshold measured from the LOOP's start (rather than the attempt's) and + // set at 20 minutes fired mid-way through a perfectly healthy second pass — + // tearing down a live loop and, worse, letting the abandoned attempt's + // eventual `disconnect()` cancel the OS pending connect the replacement was + // waiting on. That turned the supervisor into a cause of the very + // never-reconnects symptom it exists to cure. + if (attemptRunningFor != null && attemptRunningFor >= staleAfter) { return ReconnectSupervisorAction.restartStale; } return ReconnectSupervisorAction.none; diff --git a/lib/ui/today/today_screen.dart b/lib/ui/today/today_screen.dart index 5386c46..255f86a 100644 --- a/lib/ui/today/today_screen.dart +++ b/lib/ui/today/today_screen.dart @@ -1162,7 +1162,10 @@ class TodayVitals extends StatelessWidget { // the phone's or the band's 100 Hz stream, and a day with neither // shows no number rather than a guess. The detail screen was updated // to say so and this tile was missed. - const TileHeader('Steps', trailing: Tag('measured')), + // Same tag, same colour as the steps detail screen — `Tag`'s default + // is the warning amber, which reads as a caution rather than a + // statement of confidence. + TileHeader('Steps', trailing: Tag('measured', color: DomainAccent.steps)), const SizedBox(height: Sp.x2), BigStat( value: steps > 0 ? '$steps' : null, diff --git a/lib/ui/workouts/workouts_screen.dart b/lib/ui/workouts/workouts_screen.dart index aa256fd..b5ca14b 100644 --- a/lib/ui/workouts/workouts_screen.dart +++ b/lib/ui/workouts/workouts_screen.dart @@ -419,7 +419,8 @@ class _WorkoutsScreenState extends State { peakHr: (w['max_hr'] as num?)?.toInt() ?? 0, calories: ((w['calories'] as num?) ?? 0).toDouble(), strain: (w['strain'] as num?)?.toDouble(), - steps: (w['steps'] as num?)?.toInt() ?? 0, + // Nullable: an unmeasured workout is not a zero-step one. + steps: (w['steps'] as num?)?.toInt(), ); Navigator.of(context).push( themedRoute( diff --git a/test/reconnect_supervisor_test.dart b/test/reconnect_supervisor_test.dart index 858e7ac..c77f49b 100644 --- a/test/reconnect_supervisor_test.dart +++ b/test/reconnect_supervisor_test.dart @@ -26,6 +26,7 @@ void main() { bool connected = false, bool loopRunning = false, bool paused = false, + bool connectInFlight = false, Duration? runningFor, }) => superviseReconnect( paired: paired, @@ -33,7 +34,8 @@ void main() { connected: connected, loopRunning: loopRunning, autoReconnectPaused: paused, - loopRunningFor: runningFor, + connectInFlight: connectInFlight, + attemptRunningFor: runningFor, ); test('disconnected with no loop running restarts the loop', () { @@ -50,12 +52,17 @@ void main() { expect(call(keepAlive: false), ReconnectSupervisorAction.none); }); - test('never fights a healthy in-flight loop', () { - // A band can be out of range for hours, and the Android OS-autoConnect - // branch legitimately waits 15 minutes per pass, so a long-running loop - // is normal and must not be restarted out from under itself. + test('never fights a healthy attempt', () { + // The Android OS-autoConnect branch waits up to 15 minutes for the band + // to reappear. That is one NORMAL attempt, and restarting it is actively + // harmful: the abandoned attempt's eventual disconnect() cancels the OS + // pending connect its replacement is waiting on. expect( - call(loopRunning: true, runningFor: const Duration(minutes: 14)), + call(loopRunning: true, runningFor: const Duration(minutes: 15)), + ReconnectSupervisorAction.none, + ); + expect( + call(loopRunning: true, runningFor: const Duration(minutes: 24)), ReconnectSupervisorAction.none, ); expect( @@ -64,16 +71,33 @@ void main() { ); }); - test('restarts a loop wedged well past the autoConnect window', () { + test('a loop running for hours is fine while its attempts turn over', () { + // A band left at home keeps the loop alive indefinitely; only an + // individual attempt that never returns is evidence of a wedge. + expect( + call(loopRunning: true, runningFor: const Duration(minutes: 3)), + ReconnectSupervisorAction.none, + ); + }); + + test('restarts an attempt wedged well past the autoConnect window', () { // An await that never returns (a leaked band lease, a hung platform call) // leaves the in-flight flag true forever; re-triggering cannot fix that, // so the flag has to be treated as stale. expect( - call(loopRunning: true, runningFor: const Duration(minutes: 20)), + call(loopRunning: true, runningFor: const Duration(minutes: 25)), ReconnectSupervisorAction.restartStale, ); }); + test('stays out of the way of a user-initiated connect', () { + // `openSession` starts the supervisor before doing its own connect, and a + // first Android connect (bond dialog, discovery, INIT) can outlast a + // 60 s tick. Starting a loop underneath it makes two callers race the + // same peripheral and re-run the whole post-connect block. + expect(call(connectInFlight: true), ReconnectSupervisorAction.none); + }); + test('respects an active bond-refusal pause', () { // Not a dead end any more (see below), but while it IS in force the // supervisor must not hammer a band that refuses to bond. diff --git a/test/session_score_reconcile_test.dart b/test/session_score_reconcile_test.dart index 37fedbb..4a22f30 100644 --- a/test/session_score_reconcile_test.dart +++ b/test/session_score_reconcile_test.dart @@ -153,6 +153,45 @@ void main() { expect(again.changed, isFalse, reason: 'converged — stops writing'); }); + test('a complete substrate REPLACES the tally rather than maxing it', () { + // The max rule is only monotone while the scoring function is fixed, and it + // is not — strain depends on the trailing nightly resting HR, which moves. + // Maxing forever would ratchet a session up to the highest value any RHR + // the profile ever reported would have produced, with no way back down. + final r = reconcileSessionScore( + liveStrain: 14.0, // scored earlier against a lower resting HR + liveCalories: 700, + liveMaxHr: 190, + liveZoneMinutes: const [0, 0, 30, 0, 0], + substrate: _substrate( + strain: 11.4, + calories: 480, + maxHr: 168, + zone: const [4, 12, 20, 9, 1], + ), + substrateIsComplete: true, + ); + expect(r.strain, 11.4, reason: 'current anchors, not the historic peak'); + expect(r.calories, 480); + expect(r.maxHr, 168); + expect(r.zoneMinutes, const [4, 12, 20, 9, 1]); + expect(r.changed, isTrue); + }); + + test('completeness does not invent values the substrate lacks', () { + final r = reconcileSessionScore( + liveStrain: 9.0, + liveCalories: 250, + liveMaxHr: 170, + liveZoneMinutes: const [1, 2, 0, 0, 0], + substrate: _substrate(strain: null, calories: null, maxHr: null), + substrateIsComplete: true, + ); + expect(r.strain, 9.0, reason: 'no profile anchor ⇒ nothing to replace with'); + expect(r.calories, 250); + expect(r.maxHr, 170); + }); + test('zone minutes come from one source, never element-wise mixed', () { // A per-element max would invent a total neither source observed. final r = reconcileSessionScore( From c66d083a6d3abba0a4b7a9abd9b9bc7f6f8a3597 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 14:07:22 +0530 Subject: [PATCH 12/12] fifth review pass - stopping supervision only cancelled the timer; a _reconnect() parked in waitForOsAutoConnect for up to 15 min kept its generation, so endSession followed by a fresh openSession let that zombie wake up and become the live loop. retires it by generation. - complete coverage wiped a stored zone split when the substrate's own vector was empty (which happens when the profile has no HRmax), while the scalar rule correctly fell back. same rule for both now, and one generic helper instead of the scalar rule written out twice. - the sweep's changed-count only looked at strain, so a pass that fixed calories or the zone split reported nothing. --- lib/compute/manual_session.dart | 45 +++++++++++++------------- lib/data/local_repository_impl.dart | 10 +++--- lib/state/app_state.dart | 11 +++++++ test/session_score_reconcile_test.dart | 11 ++++++- 4 files changed, 49 insertions(+), 28 deletions(-) diff --git a/lib/compute/manual_session.dart b/lib/compute/manual_session.dart index 8aa8653..a1f7182 100644 --- a/lib/compute/manual_session.dart +++ b/lib/compute/manual_session.dart @@ -503,36 +503,35 @@ ReconciledSessionScore reconcileSessionScore({ ); } - // Complete coverage: the substrate IS the answer. Partial: both sides are - // lower bounds over subsets of the same minutes, so the larger is the better - // estimate and the smaller is just a less complete view. - double? better(double? a, double? b) { - if (substrateIsComplete) return b ?? a; - if (a == null) return b; - if (b == null) return a; - return a >= b ? a : b; + // ONE definition of the rule, for every scalar. Complete coverage: the + // substrate IS the answer, falling back to the live value only where it has + // nothing to say. Partial: both sides are lower bounds over subsets of the + // same minutes, so the larger is the better estimate and the smaller is just + // a less complete view. + T? better(T? live, T? sub) { + if (substrateIsComplete) return sub ?? live; + if (live == null) return sub; + if (sub == null) return live; + return live >= sub ? live : sub; } - final strain = better(liveStrain, substrate.strain); - final calories = better(liveCalories, substrate.calories); - final maxHr = substrateIsComplete - ? (substrate.maxHr ?? liveMaxHr) - : (liveMaxHr == null - ? substrate.maxHr - : (substrate.maxHr == null - ? liveMaxHr - : (liveMaxHr >= substrate.maxHr! - ? liveMaxHr - : substrate.maxHr))); + final strain = better(liveStrain, substrate.strain); + final calories = better(liveCalories, substrate.calories); + final maxHr = better(liveMaxHr, substrate.maxHr); // Zone minutes are a vector of the same lower-bound quantity, so take the // side with more total measured minutes rather than mixing two partial // splits (a per-element max would invent a total neither source observed). + // Same shape as `better`: an empty substrate vector says nothing, so it must + // not wipe a stored split just because coverage is complete (zone minutes + // need a HRmax the profile may not carry, so an empty vector is a real case). double total(List z) => z.fold(0.0, (a, b) => a + b); - final zone = substrateIsComplete || - total(substrate.zoneMinutes) > total(liveZoneMinutes) - ? substrate.zoneMinutes - : liveZoneMinutes; + final zone = substrate.zoneMinutes.isEmpty + ? liveZoneMinutes + : (substrateIsComplete || + total(substrate.zoneMinutes) > total(liveZoneMinutes) + ? substrate.zoneMinutes + : liveZoneMinutes); final changed = strain != liveStrain || diff --git a/lib/data/local_repository_impl.dart b/lib/data/local_repository_impl.dart index f50c1c8..923eaf1 100644 --- a/lib/data/local_repository_impl.dart +++ b/lib/data/local_repository_impl.dart @@ -2572,12 +2572,14 @@ class LocalRepositoryImpl extends LocalRepository { _rescoredSessions.contains(key)) { continue; } - final before = (r['strain'] as num?)?.toDouble(); final after = await _rescoreSessionFromSubstrate(r); - // Count only what THIS pass wrote: the bail path returns a re-read row - // whose strain may differ for reasons we had nothing to do with. + // Count only what THIS pass wrote (the bail path returns a re-read row + // whose values may differ for reasons we had nothing to do with), and + // count ALL the scored columns — a pass that fixes calories or the zone + // split without moving strain still changed what the list shows. + const scored = ['strain', 'calories', 'max_hr', 'zone_min_json']; if (after.hrRows != null && - (after.row['strain'] as num?)?.toDouble() != before) { + scored.any((k) => '${after.row[k]}' != '${r[k]}')) { changed++; } // Record it only once it is genuinely finished AND actually scored: a diff --git a/lib/state/app_state.dart b/lib/state/app_state.dart index 6f3b5b1..d40609d 100644 --- a/lib/state/app_state.dart +++ b/lib/state/app_state.dart @@ -2561,6 +2561,17 @@ class AppState extends ChangeNotifier { void _stopReconnectSupervisor() { _reconnectSupervisor?.cancel(); _reconnectSupervisor = null; + // Cancelling the timer is not enough: a `_reconnect()` can still be parked + // inside `waitForOsAutoConnect` for up to 15 minutes. Bumping the + // generation retires it — it exits at its next loop check and its `finally` + // leaves the flags alone. Without this, `endSession()` followed by a fresh + // `openSession()` lets that zombie wake up and become the live loop, + // reconnecting and re-running the whole post-connect block underneath the + // new session. + _reconnectGeneration++; + _reconnecting = false; + _attemptStartedAt = null; + engine.clearReconnecting(); } void _superviseReconnect() { diff --git a/test/session_score_reconcile_test.dart b/test/session_score_reconcile_test.dart index 4a22f30..e839854 100644 --- a/test/session_score_reconcile_test.dart +++ b/test/session_score_reconcile_test.dart @@ -184,12 +184,21 @@ void main() { liveCalories: 250, liveMaxHr: 170, liveZoneMinutes: const [1, 2, 0, 0, 0], - substrate: _substrate(strain: null, calories: null, maxHr: null), + // Zone minutes need a HRmax the profile may not carry, so an empty + // vector alongside complete coverage is a real case — and must not wipe + // the split that was already stored. + substrate: _substrate( + strain: null, + calories: null, + maxHr: null, + zone: const [], + ), substrateIsComplete: true, ); expect(r.strain, 9.0, reason: 'no profile anchor ⇒ nothing to replace with'); expect(r.calories, 250); expect(r.maxHr, 170); + expect(r.zoneMinutes, const [1, 2, 0, 0, 0]); }); test('zone minutes come from one source, never element-wise mixed', () {