From e1875630f716ac3b29842847fca8942bb95cc73a Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 21:12:01 +0530 Subject: [PATCH 1/6] import a .noopbak, keep the AI key through a locked relaunch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a .noopbak is a zip around noop's own sqlite database, and on iOS it is the only export noop offers — so pointing people at the raw sensor CSV left every iOS migrant with no way in. the backup carries the same 1 Hz channels the band does, so it re-derives at full fidelity instead of importing noop's scores. the key was written with the keychain's default whenUnlocked accessibility. this app is relaunched in the background constantly and often while the phone is locked, when that item cannot be read — the empty read was cached as "no key", so by the time the app was opened the key had silently gone. the shell's bottom chrome is 106pt against the 120pt of padding screens reserved for it, and the live-workout banner stacks above the pill, so the last card of every tab sat under it while a workout was running. --- lib/app.dart | 6 + lib/coach/coach_config.dart | 91 ++++- lib/import/import_container.dart | 122 ++++++- lib/import/noop_backup_import.dart | 282 ++++++++++++++++ lib/import/noop_import.dart | 474 ++++++--------------------- lib/import/noop_ingest.dart | 368 +++++++++++++++++++++ lib/ui/coach/ai_coach_screen.dart | 25 +- lib/ui/design/app_scaffold.dart | 20 +- lib/ui/import/import_screen.dart | 3 +- lib/ui/screens/metric_screen.dart | 3 +- lib/ui/today/today_screen.dart | 3 +- lib/ui/workouts/workouts_screen.dart | 3 +- test/bottom_gutter_test.dart | 98 ++++++ test/coach_config_key_test.dart | 157 +++++++++ test/import_container_test.dart | Bin 10949 -> 12061 bytes test/noop_backup_import_test.dart | 268 +++++++++++++++ 16 files changed, 1521 insertions(+), 402 deletions(-) create mode 100644 lib/import/noop_backup_import.dart create mode 100644 lib/import/noop_ingest.dart create mode 100644 test/bottom_gutter_test.dart create mode 100644 test/coach_config_key_test.dart create mode 100644 test/noop_backup_import_test.dart diff --git a/lib/app.dart b/lib/app.dart index 45bba860..b7feceac 100644 --- a/lib/app.dart +++ b/lib/app.dart @@ -74,6 +74,12 @@ class _OpenStrapAppState extends State with WidgetsBindingObserver // to latch for the whole process, silencing every notification and // scheduled reminder until a full app restart. NotificationService.instance.invalidatePermissionCache(); + // A background relaunch while the phone was locked cannot read the + // keychain, so the BYOK key can be missing from an otherwise healthy + // process. Coming to the foreground means the phone is unlocked — take + // the chance to read it. No-op unless a key is known to exist and is + // currently unreadable. + unawaited(context.read().refreshKeyIfUnreadable()); app.maybeFinishFromLiveActivity(); unawaited(app.maybeStopBreathingFromLiveActivity()); app.refreshAppStatus(); // re-check OTA + admin banner on every foreground diff --git a/lib/coach/coach_config.dart b/lib/coach/coach_config.dart index a4811102..75e49fed 100644 --- a/lib/coach/coach_config.dart +++ b/lib/coach/coach_config.dart @@ -12,14 +12,45 @@ class CoachConfig extends ChangeNotifier { static const _kModel = 'coach_model'; static const _kKey = 'coach_api_key'; // secure storage + /// Set whenever a key is written, cleared when it is deleted. The keychain + /// itself cannot answer "is there a key I currently can't read?" — a locked + /// device and an empty keychain both read as nothing — so the answer is kept + /// here, where it is always readable. + static const _kKeyPresent = 'coach_api_key_present'; + static const String defaultBaseUrl = 'https://api.openai.com/v1'; + /// FIRST-UNLOCK, not the plugin's default WHEN-UNLOCKED. + /// + /// This app is relaunched in the background constantly — BGProcessingTask, the + /// BLE restore central waking on a link drop — and those relaunches routinely + /// happen while the phone is LOCKED, i.e. exactly when a `whenUnlocked` item + /// cannot be read. That read then returned nothing, `load()` cached the + /// nothing as "no key", and by the time the user opened the app their key had + /// silently vanished ("works for a few minutes, then it's gone after + /// sleep/wake" — two TestFlight reports). `first_unlock` keeps the item + /// readable from the first unlock after boot onwards, which is what a + /// background-heavy app needs. The key still never leaves the device. + static const _apple = IOSOptions( + accessibility: KeychainAccessibility.first_unlock, + ); + static const _macos = MacOsOptions( + accessibility: KeychainAccessibility.first_unlock, + ); + final FlutterSecureStorage _secure = const FlutterSecureStorage(); String _baseUrl = defaultBaseUrl; String _model = ''; String? _key; // cached in-memory after load + /// True when a key IS stored but this process could not read it (a locked + /// keychain, a wedged keystore). Distinct from "no key configured", which the + /// user can fix by pasting one — this one fixes itself on the next unlocked + /// read, and telling them to set a key up again would be wrong. + bool _keyUnreadable = false; + bool get keyUnreadable => _keyUnreadable; + String get baseUrl => _baseUrl; String get model => _model; String? get apiKey => _key; @@ -39,14 +70,56 @@ class CoachConfig extends ChangeNotifier { final prefs = await SharedPreferences.getInstance(); _baseUrl = prefs.getString(_kBaseUrl) ?? defaultBaseUrl; _model = prefs.getString(_kModel) ?? ''; + final expectKey = prefs.getBool(_kKeyPresent) ?? false; try { - _key = await _secure.read(key: _kKey); + final read = await _secure.read( + key: _kKey, + iOptions: _apple, + mOptions: _macos, + ); + if (read != null && read.isNotEmpty) { + _key = read; + _keyUnreadable = false; + // Upgrade an item written before this class asked for `first_unlock`: + // accessibility is set at WRITE time, so an existing key keeps the old + // attribute until it is written again. Keyed on the marker's absence so + // this happens exactly once — a write on every load would put the + // Android Keystore (the documented Samsung Knox hang) on the startup + // path for no reason. + if (!expectKey) { + await _secure.write( + key: _kKey, + value: read, + iOptions: _apple, + mOptions: _macos, + ); + await prefs.setBool(_kKeyPresent, true); + } + } else if (expectKey) { + // We know one was saved, so an empty read is the keychain being + // unavailable — NOT the user having no key. Keep whatever is cached + // (usually nothing on a fresh process) and say why. + _keyUnreadable = true; + } else { + _key = null; + _keyUnreadable = false; + } } catch (_) { - _key = null; + // A read that THREW tells us nothing about the stored key, so it must not + // overwrite one we already hold in memory. + _keyUnreadable = expectKey; } notifyListeners(); } + /// Re-read the key if it is known to exist but could not be read. Cheap no-op + /// otherwise — safe to call on every resume, which is when a phone that was + /// locked during a background relaunch becomes readable again. + Future refreshKeyIfUnreadable() async { + if (!_keyUnreadable) return; + await load(); + } + Future save({String? baseUrl, String? model, String? apiKey}) async { final prefs = await SharedPreferences.getInstance(); if (baseUrl != null) { @@ -60,10 +133,20 @@ class CoachConfig extends ChangeNotifier { if (apiKey != null) { final k = apiKey.trim(); _key = k.isEmpty ? null : k; + _keyUnreadable = false; if (k.isEmpty) { - await _secure.delete(key: _kKey); + await _secure.delete(key: _kKey, iOptions: _apple, mOptions: _macos); + await prefs.setBool(_kKeyPresent, false); } else { - await _secure.write(key: _kKey, value: k); + await _secure.write( + key: _kKey, + value: k, + iOptions: _apple, + mOptions: _macos, + ); + // Written AFTER the keychain succeeds: a marker claiming a key that was + // never stored would leave the app permanently reporting "unreadable". + await prefs.setBool(_kKeyPresent, true); } } notifyListeners(); diff --git a/lib/import/import_container.dart b/lib/import/import_container.dart index c23f197b..8ddb69a6 100644 --- a/lib/import/import_container.dart +++ b/lib/import/import_container.dart @@ -148,6 +148,107 @@ class ResolvedImportFiles { } } +/// True for a ZIP member that is a database rather than a CSV. +bool _isDbMember(String name) { + final base = p.basename(name).toLowerCase(); + return (base.endsWith('.sqlite') || + base.endsWith('.db') || + base.endsWith('.sqlite3')) && + !base.startsWith('._') && + !name.startsWith('__MACOSX/'); +} + +/// A NOOP database ready to read, plus the temp directory holding it (if it had +/// to be unpacked). The caller MUST [dispose] once it has finished reading. +class ResolvedNoopDatabase { + ResolvedNoopDatabase(this.path, this._tempDir); + + final String path; + final Directory? _tempDir; + + 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 */ + } + } +} + +/// If [path] is a NOOP full backup, return its database ready to open. +/// +/// Handles both shapes users arrive with: the `.noopbak` itself (a ZIP whose +/// only member is `noop-backup.sqlite`) and a database someone already unpacked +/// by hand. Returns null for anything else, so the caller falls through to the +/// CSV path. +/// +/// The database is EXTRACTED to a temp directory rather than read in place: a +/// ZIP member is deflated, and sqlite needs a real file it can seek in. +Future resolveNoopDatabase(String path) async { + switch (await sniffFile(path)) { + case ImportContainer.sqlite: + return ResolvedNoopDatabase(path, null); + case ImportContainer.zip: + break; + default: + return null; + } + + final input = InputFileStream(path); + Archive archive; + try { + archive = ZipDecoder().decodeStream(input); + } catch (e) { + await input.close(); + throw ImportFormatException( + 'Could not read “${p.basename(path)}” as an archive: $e', + ); + } + try { + ArchiveFile? db; + for (final f in archive.files) { + if (f.isFile && _isDbMember(f.name)) { + // Largest member wins: a backup can ship the database alongside a small + // sidecar (`-wal`, a manifest), and picking the first match could take + // the wrong one. + if (db == null || f.size > db.size) db = f; + } + } + if (db == null) return null; // not a backup — let the CSV path try. + + final tempDir = await Directory.systemTemp.createTemp('openstrap_noopbak_'); + final destPath = p.join(tempDir.path, p.basename(db.name)); + final sink = OutputFileStream(destPath); + try { + db.writeContent(sink); + } finally { + await sink.close(); + } + // A 260 MB backup unpacks to a full second copy, and a phone that runs out + // of space mid-write leaves a TRUNCATED file — which still opens as a valid + // database and would import a fraction of the history as if that were all + // of it. Silent partial history is the worst outcome here, so verify the + // whole member landed. (Dart has no portable free-space API, hence checking + // after rather than before.) + final written = await File(destPath).length(); + if (db.size > 0 && written < db.size) { + final resolved = ResolvedNoopDatabase(destPath, tempDir); + await resolved.dispose(); + throw ImportFormatException( + 'Unpacking that backup stopped at ' + '${(written / (1024 * 1024)).round()} MB of ' + '${(db.size / (1024 * 1024)).round()} MB — the phone is probably out ' + 'of space. Free some up and try again.', + ); + } + return ResolvedNoopDatabase(destPath, tempDir); + } finally { + await input.close(); + } +} + /// Resolve the picked paths into CSV files on disk, unwrapping ZIP archives. /// /// [flavor] names the importer in error messages ('NOOP', 'WHOOP'). Extracted @@ -183,10 +284,11 @@ Future resolveImportCsvPaths( await _extractCsvMembers(path, flavor: flavor, dir: into), ); case ImportContainer.sqlite: + // Only reachable for WHOOP now — a NOOP database (loose or inside a + // `.noopbak`) is claimed by [resolveNoopDatabase] before this runs. 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.', + 'export.', ); case ImportContainer.gzip: throw ImportFormatException( @@ -258,22 +360,6 @@ Future> _extractCsvMembers( } 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'}). ' diff --git a/lib/import/noop_backup_import.dart b/lib/import/noop_backup_import.dart new file mode 100644 index 00000000..6b1741aa --- /dev/null +++ b/lib/import/noop_backup_import.dart @@ -0,0 +1,282 @@ +// noop_backup_import.dart — read a `.noopbak` full backup. +// +// A `.noopbak` is a ZIP around `noop-backup.sqlite`, NOOP's own GRDB database. +// On iOS it is the ONLY export NOOP offers, so every iOS user migrating across +// arrives with one; the raw-sensor CSV this importer originally required is +// Android-only. That is issue #160 for real, rather than the UTF-8 symptom. +// +// WHAT IT HOLDS (measured on a 13-day backup, 260 MB, NOOP 9.x): +// hrSample(deviceId, ts, bpm) 1,032,010 rows 1 Hz +// rrInterval(deviceId, ts, rrMs) 505,358 +// gravitySample(deviceId, ts, x, y, z) 1,031,199 1 Hz +// skinTempSample(deviceId, ts, raw) 1,031,199 1 Hz +// stepSample(deviceId, ts, counter) 1,031,199 cumulative +// spo2Sample / respSample 0 present, empty +// sleepSession / dailyMetric / metricSeries 12 / 14 / 24 +// Timestamps are epoch SECONDS on a shared 1 Hz grid, which is the same shape as +// our own decoded substrate — so this is a FULL-FIDELITY source, not a summary +// import. We take the raw channels and re-derive; `sleepSession` and +// `dailyMetric` (NOOP's own scores) are deliberately NOT read, because a second +// set of stages and daily numbers would contradict the ones we compute. +// +// TWO TRAPS THIS FILE EXISTS TO AVOID: +// 1. `spo2Sample` and `respSample` are EMPTY in a real backup and a future +// schema may drop them outright, so every table is probed before it is read. +// 2. deviceId is NOT consistent within one backup — the sample tables carry +// "my-whoop" while sleepSession carries "my-whoop-noop". Nothing here +// filters or joins on it: a user who replaced a strap has both ids over +// disjoint spans and wants BOTH imported, and a per-second map keyed on ts +// already collapses the (unrealistic) case of two straps worn at once. +// +// MEMORY. 3.6 M rows cannot be materialised, and sqflite serialises a whole +// result set on the platform side before any of it crosses the channel. So we +// walk one LOCAL DAY at a time (the unit [NoopIngest] derives anyway) and read +// each table for that day in pages. + +import 'dart:io'; + +import 'package:flutter/foundation.dart'; +import 'package:sqflite/sqflite.dart'; + +import '../compute/derivation_engine.dart'; +import '../compute/profile.dart'; +import 'import_container.dart'; +import 'noop_import.dart'; +import 'noop_ingest.dart'; + +/// Rows per platform-channel round trip. Big enough that a 1 Hz day is a handful +/// of queries, small enough that no single response is a memory event. Not +/// const: the page-boundary contract below is only testable at a small size. +@visibleForTesting +int kNoopBackupPageRows = 20000; + +class NoopBackupImporter { + /// Import an already-extracted `noop-backup.sqlite` at [path]. + /// + /// Opened READ-ONLY — a backup is the user's only copy of their history and + /// this must not be able to write to it, even by accident. + static Future importDatabase( + String path, + Profile profile, + DerivationEngine engine, { + void Function(int days)? onProgress, + }) async { + if (!await File(path).exists()) { + throw const ImportFormatException('That backup could not be read.'); + } + final Database src; + try { + src = await openDatabase(path, readOnly: true); + } catch (e) { + throw ImportFormatException( + 'That backup\'s database could not be opened ($e). If it came off ' + 'another phone, try exporting it again.', + ); + } + try { + return await _import(src, profile, engine, onProgress: onProgress); + } finally { + await src.close(); + } + } + + static Future _import( + Database src, + Profile profile, + DerivationEngine engine, { + void Function(int days)? onProgress, + }) async { + final tables = await _tableNames(src); + // hrSample is the spine: no heart rate means nothing downstream can be + // derived, so its absence is a wrong-file error rather than an empty import. + if (!tables.contains('hrSample')) { + throw ImportFormatException( + 'That database is not a NOOP backup — it has no `hrSample` table ' + '(found: ${tables.take(6).join(', ')}${tables.length > 6 ? '…' : ''}).', + ); + } + + final span = await _span(src, tables); + if (span == null) { + throw const ImportFormatException( + 'That NOOP backup holds no samples — there is nothing to import.', + ); + } + final (minTs, maxTs) = span; + + final ingest = NoopIngest(profile, engine, onProgress: onProgress); + + // Walk LOCAL days. Built through the DateTime(y, m, d + 1) constructor + // rather than `add(Duration(days: 1))` so a DST boundary lands on real local + // midnight instead of 23:00 or 01:00. + var dayStart = _localMidnight(minTs); + while (dayStart.millisecondsSinceEpoch ~/ 1000 <= maxTs) { + final next = DateTime(dayStart.year, dayStart.month, dayStart.day + 1); + final from = dayStart.millisecondsSinceEpoch ~/ 1000; + final to = next.millisecondsSinceEpoch ~/ 1000; + + // Order within a day does not matter — [NoopIngest] rebuilds the Substrate + // sorted by timestamp — but the DAYS must arrive in ascending order, since + // the high-water date is what closes out and derives the previous one. + await _read(src, tables, 'hrSample', ['ts', 'bpm'], from, to, (r) async { + final ts = _int(r['ts']), v = _int(r['bpm']); + if (ts == null || v == null) return; + if (await ingest.offer(ts)) ingest.hr(ts, v); + }); + await _read(src, tables, 'rrInterval', ['ts', 'rrMs'], from, to, + (r) async { + final ts = _int(r['ts']), v = _num(r['rrMs']); + if (ts == null || v == null) return; + if (await ingest.offer(ts)) ingest.rr(ts, v); + }); + await _read(src, tables, 'gravitySample', ['ts', 'x', 'y', 'z'], from, to, + (r) async { + final ts = _int(r['ts']); + if (ts == null) return; + if (await ingest.offer(ts)) { + ingest.gravity(ts, _num(r['x']), _num(r['y']), _num(r['z'])); + } + }); + await _read(src, tables, 'skinTempSample', ['ts', 'raw'], from, to, + (r) async { + final ts = _int(r['ts']); + if (ts == null) return; + if (await ingest.offer(ts)) ingest.skinTemp(ts, _int(r['raw'])); + }); + await _read(src, tables, 'spo2Sample', ['ts', 'red', 'ir'], from, to, + (r) async { + final ts = _int(r['ts']); + if (ts == null) return; + if (await ingest.offer(ts)) { + ingest.spo2(ts, _int(r['red']), _int(r['ir'])); + } + }); + await _read(src, tables, 'stepSample', ['ts', 'counter'], from, to, + (r) async { + final ts = _int(r['ts']), v = _int(r['counter']); + if (ts == null || v == null) return; + if (await ingest.offer(ts)) ingest.stepCounter(ts, v); + }); + + dayStart = next; + } + + if (ingest.rows == 0) { + throw const ImportFormatException( + 'That NOOP backup holds no samples we could read.', + ); + } + await ingest.finish(); + return NoopImportResult( + ingest.days, ingest.rows, ingest.lateRows, ingest.steps); + } + + /// Page one table's rows for the half-open window [from, to) into [onRow]. + /// A table the backup does not have is skipped — `spo2Sample` and `respSample` + /// are already absent-in-practice (present but empty), and a future NOOP + /// schema is free to drop either outright. + /// + /// Paged by KEYSET (`ts > last`), not OFFSET: LIMIT/OFFSET re-walks and + /// re-discards every skipped row, so paging a 86,400-row day turns quadratic. + /// + /// `ts` is NOT unique in every table — `rrInterval`'s key is (deviceId, ts, + /// rrMs), so one second can hold several beats, and two devices can share a + /// second in any table. A page boundary landing inside such a second would + /// drop its remaining rows silently, so a FULL page discards its trailing + /// partial second and re-reads it from the top of the next page. + static Future _read( + Database src, + Set tables, + String table, + List cols, + int from, + int to, + Future Function(Map row) onRow, + ) async { + if (!tables.contains(table)) return; + var cursor = from - 1; + while (true) { + final rows = await src.query( + table, + columns: cols, + where: 'ts > ? AND ts < ?', + whereArgs: [cursor, to], + orderBy: 'ts', + limit: kNoopBackupPageRows, + ); + if (rows.isEmpty) return; + final full = rows.length == kNoopBackupPageRows; + final lastTs = _int(rows.last['ts']); + // A NULL or non-integer ts in the last row of a page would leave the + // cursor unmoved and loop forever; take the page and stop instead. + if (lastTs == null || lastTs <= cursor) { + for (final r in rows) { + await onRow(r); + } + return; + } + + // On a full page, hold back the trailing rows that share the last + // timestamp — the next page re-reads that second whole. + var end = rows.length; + if (full) { + while (end > 0 && _int(rows[end - 1]['ts']) == lastTs) { + end--; + } + // A whole page of one timestamp: nothing can be held back without + // stalling, so take it and step past that second. + if (end == 0) end = rows.length; + } + for (var i = 0; i < end; i++) { + await onRow(rows[i]); + } + if (!full) return; + cursor = end == rows.length ? lastTs : lastTs - 1; + } + } + + /// Earliest and latest sample timestamp across every table we read, so the day + /// walk covers days that carry (say) only a step counter. + static Future<(int, int)?> _span(Database src, Set tables) async { + int? lo, hi; + for (final t in const [ + 'hrSample', + 'rrInterval', + 'gravitySample', + 'skinTempSample', + 'spo2Sample', + 'stepSample', + ]) { + if (!tables.contains(t)) continue; + final r = await src.rawQuery('SELECT MIN(ts) AS lo, MAX(ts) AS hi FROM $t'); + if (r.isEmpty) continue; + final a = _int(r.first['lo']), b = _int(r.first['hi']); + if (a == null || b == null) continue; + // Guard against a corrupt or millisecond-scaled timestamp column: a value + // outside plausible unix seconds would make the day walk run for years. + if (a < 1000000000 || b > 4102444800) continue; + lo = lo == null || a < lo ? a : lo; + hi = hi == null || b > hi ? b : hi; + } + return (lo == null || hi == null) ? null : (lo, hi); + } + + static Future> _tableNames(Database src) async { + final rows = await src + .rawQuery("SELECT name FROM sqlite_master WHERE type = 'table'"); + return {for (final r in rows) (r['name'] as String?) ?? ''}; + } + + static DateTime _localMidnight(int epochSec) { + final d = DateTime.fromMillisecondsSinceEpoch(epochSec * 1000); + return DateTime(d.year, d.month, d.day); + } + + static int? _int(Object? v) => v is int + ? v + : v is num + ? v.toInt() + : null; + + static double? _num(Object? v) => v is num ? v.toDouble() : null; +} diff --git a/lib/import/noop_import.dart b/lib/import/noop_import.dart index 1db618d7..7e7d18c3 100644 --- a/lib/import/noop_import.dart +++ b/lib/import/noop_import.dart @@ -1,23 +1,30 @@ -// noop_import.dart — import a NOOP raw-sensor CSV export into the local store. +// noop_import.dart — import NOOP's own data into the local store. // -// The NOOP export is LONG-FORMAT raw 1 Hz: one row per decoded sample, with a -// `stream` discriminator and only that stream's columns filled. Columns are read -// by NAME from the header, never by fixed position — NOOP has already shipped one -// schema change (see below) and name-keyed reads absorbed it without misparsing. +// TWO SOURCES, ONE PIPELINE. NOOP exports its history two ways and users pick +// whichever their platform offers: +// • the raw-sensor CSV (Android only — "Export → raw sensor CSV") +// • `.noopbak`, a full backup: a ZIP around NOOP's own SQLite database, which +// is the ONLY option on iOS +// Both carry the same 1 Hz signal family, so both stream through [NoopIngest] +// and derive at full fidelity, identical to a live band sync. This file owns the +// CSV reader and routes a backup to [NoopBackupImporter]; the day model, step +// banking and out-of-order contract live in noop_ingest.dart. // -// SCHEMA AS SHIPPED (observed 2026-08, NOOP 9.1/9.2 — OpenStrap/edge#160): +// CSV SCHEMA AS SHIPPED (observed 2026-08, NOOP 9.1/9.2 — OpenStrap/edge#160): // unix_s,iso_utc,stream,hr_bpm,rr_ms,grav_x,grav_y,grav_z,step_counter, // ppg_bpm,ppg_conf,spo2_red,spo2_ir,skintemp_raw,resp_raw,band_sleep_state, // event_kind,event_payload // `band_sleep_state` was INSERTED at index 15, shifting event_kind/event_payload // to 16/17 (the older documented layout, still in [_defaultCols], ends at -// event_payload=16). Streams now seen: hr, rr, gravity, skintemp, steps, -// band_sleep_state, ppghr, event. Note `spo2` and `resp` rows are no longer -// emitted at all even though their columns survive in the header. +// event_payload=16). Columns are read by NAME from the header, never by fixed +// position — that is what absorbed this drift without misparsing. Streams now +// seen: hr, rr, gravity, skintemp, steps, band_sleep_state, ppghr, event. Note +// `spo2` and `resp` rows are no longer emitted at all even though their columns +// survive in the header. // // WHAT WE CONSUME, AND WHY NOT THE REST: // • hr / rr / gravity / skintemp / spo2 → the Substrate (full 1 Hz analytics). -// • steps → `live_coverage` as a REAL step count (see [_flushStepCoverage]). +// • steps → `live_coverage` as a REAL step count. // • band_sleep_state → deliberately ignored. `segmentSleep` is the single sleep // source (a second one would contradict it), and in the real #160 export this // column was constant 0 across all 12,663 rows — no signal to gain. @@ -27,24 +34,22 @@ // An unrecognised future `stream` also lands in the default branch and is skipped // safely, so a further NOOP schema change degrades rather than throws. // -// Because this is RAW 1 Hz — the SAME signal family as our own Substrate — we run -// it through the FULL local pipeline (full-fidelity analytics, identical to a live -// band sync), NOT degraded snapshots. -// -// LARGE FILES: a 90-day export is hundreds of MB / tens of millions of rows, so we -// NEVER load the file or build one big Substrate. We STREAM the file and derive in +// LARGE FILES: a 90-day export is hundreds of MB / tens of millions of rows, so +// we NEVER load the file or build one big Substrate. We STREAM it and derive in // a 2-day sliding window (the day model needs the prior evening for a sleep that -// starts before midnight), writing + freeing each day as we go. Memory stays flat -// (~2 days) regardless of file size. +// starts before midnight), writing + freeing each day as we go. Memory stays +// flat (~2 days) regardless of file size. import 'dart:convert'; import 'dart:io'; import '../compute/derivation_engine.dart'; import '../compute/profile.dart'; -import '../compute/substrate.dart'; -import '../data/db.dart'; import 'import_container.dart'; +import 'noop_backup_import.dart'; +import 'noop_ingest.dart'; + +export 'noop_ingest.dart' show StepRun, RowOrder; /// Last path segment, for error messages (avoids a `package:path` import just /// for this one use). @@ -55,65 +60,22 @@ class NoopImportResult { final int days; final int rows; - /// Real steps recovered from the export's `step_counter` and banked as - /// `live_coverage` (0 when the export carries no `steps` stream). + /// Real steps recovered from the band's cumulative step counter and banked as + /// `live_coverage` (0 when the source carries no steps). final int steps; - /// Rows whose local date had ALREADY been derived and pruned by the time - /// they appeared in the file (a genuinely out-of-order export). They cannot - /// be folded back in, so they are counted here rather than silently dropped - /// — a non-zero value means the export was not fully time-ordered. + /// Rows whose local date had ALREADY been derived and pruned by the time they + /// appeared (a genuinely out-of-order source). They cannot be folded back in, + /// so they are counted here rather than silently dropped — a non-zero value + /// means the source was not fully time-ordered. final int lateRows; - NoopImportResult(this.days, this.rows, - [this.lateRows = 0, this.steps = 0]); -} - -/// One contiguous run of the band's cumulative `step_counter`, ready to bank as -/// a `live_coverage` row: real steps over a known [startSec]..[endSec] window. -class StepRun { - final int startSec; - final int endSec; - final int steps; - const StepRun(this.startSec, this.endSec, this.steps); - - @override - String toString() => 'StepRun($startSec..$endSec, $steps)'; - - @override - bool operator ==(Object other) => - other is StepRun && - other.startSec == startSec && - other.endSec == endSec && - other.steps == steps; - - @override - int get hashCode => Object.hash(startSec, endSec, steps); -} - -/// What to do with an incoming row given the import's high-water date. -/// [advance] closes out the previous date; [buffer] folds the row into the -/// current rolling window; [late] means its day was already derived + pruned, -/// so the row cannot be used (counted in [NoopImportResult.lateRows]). -enum RowOrder { advance, buffer, late } - -/// One second's worth of the 1 Hz channels (sparse — only set streams present). -class _Sec { - int? hr; - double? ax, ay, az; - int? spo2Red, spo2Ir, skinTemp; + NoopImportResult(this.days, this.rows, [this.lateRows = 0, this.steps = 0]); } -/// Documented default column order (used only if the export omits its header). -const Map _defaultCols = { - 'unix_s': 0, 'iso_utc': 1, 'stream': 2, 'hr_bpm': 3, 'rr_ms': 4, - 'grav_x': 5, 'grav_y': 6, 'grav_z': 7, 'step_counter': 8, 'ppg_bpm': 9, - 'ppg_conf': 10, 'spo2_red': 11, 'spo2_ir': 12, 'skintemp_raw': 13, - 'resp_raw': 14, 'event_kind': 15, 'event_payload': 16, -}; - class NoopImporter { - /// Stream-import [path] and derive each day at full 1 Hz fidelity via [engine]. - /// [onProgress] reports days written so far. Never loads the whole file. + /// Import [path] — a raw-sensor CSV, a ZIP holding one, or a `.noopbak` full + /// backup — and derive each day at full 1 Hz fidelity via [engine]. + /// [onProgress] reports days written so far. Never loads the whole source. static Future importFile( String path, Profile profile, @@ -122,13 +84,27 @@ class NoopImporter { }) async { var file = File(path); if (!await file.exists()) { - throw const FileSystemException('CSV not found'); + throw const FileSystemException('File 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. + // What did the user actually pick? Feeding a `.noopbak`'s bytes to + // `utf8.decoder` is what produced "Invalid UTF-8 byte (at offset 10)" in + // issues #160/#199. Resolve the container first: a backup goes to the + // database reader, a ZIP of CSVs is unwrapped, and anything unusable throws + // an [ImportFormatException] naming what we DO want. + final db = await resolveNoopDatabase(path); + if (db != null) { + try { + return await NoopBackupImporter.importDatabase( + db.path, + profile, + engine, + onProgress: onProgress, + ); + } finally { + await db.dispose(); + } + } + final resolved = await resolveImportCsvPaths([path], flavor: 'NOOP'); if (resolved.paths.isEmpty) { await resolved.dispose(); @@ -173,52 +149,15 @@ class NoopImporter { 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) - final rrMs = []; - String? curDate; - final derived = {}; // dates already derived + pruned out of `secs` - var totalRows = 0, daysDone = 0, lateRows = 0, stepsBanked = 0; + final ingest = NoopIngest(profile, engine, onProgress: onProgress); - // Band step counter, ts(sec) → cumulative value, keyed by local date so a - // run is never attributed across midnight. Flushed to `live_coverage` - // BEFORE the date derives, since the derivation reads real steps from there. - final stepsByDate = >{}; - - Future deriveAndPrune(String date) async { - // Real steps must be banked BEFORE deriving: _deriveDay reads them via - // LocalDb.liveStepsForDay/coverageWindowsOverlapping at derive time. - final st = stepsByDate.remove(date); - if (st != null) stepsBanked += await _flushStepCoverage(st, date); - // Build a Substrate from everything buffered (prev + current date) and - // derive ONLY [date]; calendarDays gives [date] its prior-evening context. - final sub = _buildSubstrate(secs, rrTs, rrMs); - final n = await engine.deriveImportedDays(sub, profile, {date}); - daysDone += n; - onProgress?.call(daysDone); - // Keep [date]'s samples as the prior evening for the NEXT date; drop older. - secs.removeWhere((ts, _) => localDateLabel(ts) != date); - var w = 0; - for (var i = 0; i < rrMs.length; i++) { - if (localDateLabel((rrTs[i] / 1000).floor()) == date) { - rrTs[w] = rrTs[i]; - rrMs[w] = rrMs[i]; - w++; - } - } - rrTs.length = w; - rrMs.length = w; - } - - // Column name → index, parsed from the header row. Reading by NAME (not fixed - // position) means added/reordered columns in a future export don't misparse — - // as long as the known column names persist. Falls back to the documented - // default layout if a header is somehow absent. + // Column name → index, parsed from the header row. Reading by NAME (not + // fixed position) means added/reordered columns in a future export don't + // misparse — as long as the known column names persist. Falls back to the + // documented default layout if a header is somehow absent. var col = _defaultCols; - int? idx(String name) => col[name]; String at(List f, String name) { - final i = idx(name); + final i = col[name]; return (i != null && i < f.length) ? f[i] : ''; } @@ -245,71 +184,38 @@ class NoopImporter { final f = line.split(','); final ts = int.tryParse(at(f, 'unix_s')); if (ts == null) continue; - final stream = at(f, 'stream'); + if (!await ingest.offer(ts)) continue; - final date = localDateLabel(ts); - // - // OUT-OF-ORDER ROWS. `curDate` is a HIGH-WATER mark and must only ever - // move forward. It used to be assigned unconditionally, so one backwards - // timestamp rewound it; the next forward row then called - // deriveAndPrune(), whose `secs.removeWhere(label != date)` - // discarded every buffered sample for the newer day — silent, unbounded - // loss with nothing surfaced. - // - // Now: a forward row closes out the previous date (derive + prune); a - // backwards row for a day we have NOT derived yet is simply folded into - // the buffer at its own timestamp (the Substrate is rebuilt sorted by ts, - // so arrival order never mattered); and a row for a day already derived - // is genuinely too late to use — counted in - // [NoopImportResult.lateRows] and reported instead of being allowed to - // wipe the current day. - switch (decideRow(date, curDate, derived)) { - case RowOrder.advance: - if (curDate != null) { - await deriveAndPrune(curDate); - derived.add(curDate); - } - curDate = date; - case RowOrder.buffer: - break; - case RowOrder.late: - lateRows++; - continue; - } - totalRows++; - - switch (stream) { + switch (at(f, 'stream')) { case 'hr': final v = int.tryParse(at(f, 'hr_bpm')); - if (v != null) (secs[ts] ??= _Sec()).hr = v; + if (v != null) ingest.hr(ts, v); break; case 'rr': final v = double.tryParse(at(f, 'rr_ms')); - if (v != null && v > 0) { - rrTs.add(ts * 1000.0); - rrMs.add(v); - } + if (v != null) ingest.rr(ts, v); break; case 'gravity': - final s = secs[ts] ??= _Sec(); - s.ax = double.tryParse(at(f, 'grav_x')); - s.ay = double.tryParse(at(f, 'grav_y')); - s.az = double.tryParse(at(f, 'grav_z')); + ingest.gravity( + ts, + double.tryParse(at(f, 'grav_x')), + double.tryParse(at(f, 'grav_y')), + double.tryParse(at(f, 'grav_z')), + ); break; case 'spo2': - final s = secs[ts] ??= _Sec(); - s.spo2Red = int.tryParse(at(f, 'spo2_red')); - s.spo2Ir = int.tryParse(at(f, 'spo2_ir')); + ingest.spo2( + ts, + int.tryParse(at(f, 'spo2_red')), + int.tryParse(at(f, 'spo2_ir')), + ); break; case 'skintemp': - (secs[ts] ??= _Sec()).skinTemp = int.tryParse(at(f, 'skintemp_raw')); + ingest.skinTemp(ts, int.tryParse(at(f, 'skintemp_raw'))); break; case 'steps': - // CUMULATIVE band counter, not a per-second increment — differenced - // into real step windows at flush time (see [stepRuns]). Kept out of - // the Substrate: it is a real count, not a 1 Hz signal to analyse. final v = int.tryParse(at(f, 'step_counter')); - if (v != null && v >= 0) (stepsByDate[date] ??= {})[ts] = v; + if (v != null) ingest.stepCounter(ts, v); break; // band_sleep_state / ppghr / resp / event: intentionally not consumed — // see the stream inventory in the file header for why each is skipped. @@ -318,27 +224,13 @@ class NoopImporter { break; } } - // EOF — derive the final buffered date. - if (curDate != null && secs.isNotEmpty) { - final st = stepsByDate.remove(curDate); - if (st != null) stepsBanked += await _flushStepCoverage(st, curDate); - final sub = _buildSubstrate(secs, rrTs, rrMs); - daysDone += await engine.deriveImportedDays(sub, profile, {curDate}); - onProgress?.call(daysDone); - } - // Any date whose steps were buffered but which never derived (e.g. a date - // that carried ONLY a `steps` stream) still banks its real count — dropping - // it would silently lose steps the band actually measured. - for (final e in stepsByDate.entries) { - 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) { + if (ingest.rows == 0) { final head = firstLine ?? ''; final preview = head.isEmpty ? '' @@ -346,203 +238,37 @@ class NoopImporter { 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.' + '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.', + '"unix_s,…" header row was found$preview. In NOOP, use ' + 'Export → raw sensor CSV, or import a .noopbak backup.', ); } - await engine.finalizeImport(profile); - return NoopImportResult(daysDone, totalRows, lateRows, stepsBanked); + await ingest.finish(); + return NoopImportResult( + ingest.days, ingest.rows, ingest.lateRows, ingest.steps); } - /// Build a Substrate from the buffered seconds + RR beats. Gravity / SpO₂ / - /// skin-temp are forward-filled across seconds that lack their stream (the real - /// 1 Hz substrate carries a value every second); HR stays 0 when absent - /// (off-wrist semantics — meaningful, never forward-filled). - static Substrate _buildSubstrate( - Map secs, List rrTs, List rrMs) { - final tsList = secs.keys.toList()..sort(); - final n = tsList.length; - final tsSec = List.filled(n, 0); - final hr = List.filled(n, 0); - final ax = List.filled(n, 0); - final ay = List.filled(n, 0); - final az = List.filled(n, 0); - final spo2Red = List.filled(n, 0); - final spo2Ir = List.filled(n, 0); - final skinTemp = List.filled(n, 0); - - double fax = 0, fay = 0, faz = 0; // forward-fill carry - int fRed = 0, fIr = 0, fTemp = 0; - for (var i = 0; i < n; i++) { - final t = tsList[i]; - final s = secs[t]!; - tsSec[i] = t; - hr[i] = s.hr ?? 0; - if (s.ax != null) { - fax = s.ax!; - fay = s.ay ?? fay; - faz = s.az ?? faz; - } - ax[i] = fax; - ay[i] = fay; - az[i] = faz; - if (s.spo2Red != null) fRed = s.spo2Red!; - if (s.spo2Ir != null) fIr = s.spo2Ir!; - if (s.skinTemp != null) fTemp = s.skinTemp!; - spo2Red[i] = fRed; - spo2Ir[i] = fIr; - skinTemp[i] = fTemp; - } - - // RR beats sorted by time. - final order = List.generate(rrMs.length, (i) => i) - ..sort((a, b) => rrTs[a].compareTo(rrTs[b])); - return Substrate( - tsSec: tsSec, - hr: hr, - rrTsMs: [for (final i in order) rrTs[i]], - rrMs: [for (final i in order) rrMs[i]], - ax: ax, - ay: ay, - az: az, - spo2Red: spo2Red, - spo2Ir: spo2Ir, - skinTemp: skinTemp, - skinContact: ax.map((_) => 0).toList(), - ); - } + /// Documented default column order (used only if the export omits its header). + static const Map _defaultCols = { + 'unix_s': 0, 'iso_utc': 1, 'stream': 2, 'hr_bpm': 3, 'rr_ms': 4, + 'grav_x': 5, 'grav_y': 6, 'grav_z': 7, 'step_counter': 8, 'ppg_bpm': 9, + 'ppg_conf': 10, 'spo2_red': 11, 'spo2_ir': 12, 'skintemp_raw': 13, + 'resp_raw': 14, 'event_kind': 15, 'event_payload': 16, + }; - /// Maximum gap (seconds) between consecutive `step_counter` samples that is - /// still treated as one continuous run. The #160 export samples steps every - /// second, but drops out whenever the band is off-wrist or unsynced; a gap - /// wider than this is a hole we know nothing about, so we refuse to span it. - static const int stepRunMaxGapSec = 60; - - /// Turn the band's CUMULATIVE `step_counter` samples into discrete - /// [StepRun]s that can be banked as real (non-estimated) step counts. - /// - /// [samples] is ts(sec) → counter value, any order. Runs are split on a gap - /// wider than [stepRunMaxGapSec], so a 20 h hole in the export (exactly what - /// the #160 file has) never becomes one window claiming to cover the day. - /// - /// Only POSITIVE deltas within a run are summed: the counter resets to 0 on a - /// band reboot, and a negative delta is that reset, not −24,000 steps. Deltas - /// ACROSS a run boundary are deliberately NOT counted — we cannot attribute - /// steps to a window we have no samples for, and inventing that attribution is - /// exactly the kind of fabrication the honesty contract forbids. - /// - /// A run with zero steps still yields no window: `live_coverage` exists to - /// suppress the 1 Hz estimate over minutes the real counter already covered, - /// and a 0-step run would suppress a real estimate while contributing nothing. - /// - /// [covered] lists time spans ALREADY banked in `live_coverage` (device-time - /// seconds, inclusive). A per-second delta whose interval intersects one of - /// them is skipped and BREAKS the run, so those steps are never banked twice. - /// This is what makes a re-import safe in general rather than only for a - /// byte-identical file: an exact-window check alone is defeated the moment the - /// user exports again over a longer span (09:00-09:20 then 09:00-09:40), where - /// the run boundary shifts, no exact window matches, and the overlap is banked - /// a second time — measured at 3,598 steps against a true 2,399 before this. - /// Clipping is exact, not pro-rated: we hold the counter value at every - /// second, so the uncovered sub-intervals are summed from real deltas. - /// It also means an imported span cannot double-count against a LIVE 100 Hz - /// pedometer window, which shares this table. + /// Both live in [NoopIngest] now; kept here so existing callers and tests that + /// reach for them through the importer keep working. static List stepRuns( Map samples, { List> covered = const [], - }) { - if (samples.isEmpty) return const []; - final ts = samples.keys.toList()..sort(); - - // Does the half-open delta interval (a, b] touch anything already banked? - bool isCovered(int a, int b) { - for (final w in covered) { - if (b > w[0] && a < w[1]) return true; - } - return false; - } - - final out = []; - int? runStart, runLast; - var runSteps = 0; - void closeRun() { - if (runStart != null && - runLast != null && - runSteps > 0 && - runLast! > runStart!) { - out.add(StepRun(runStart!, runLast!, runSteps)); - } - runStart = null; - runLast = null; - runSteps = 0; - } + }) => + NoopIngest.stepRuns(samples, covered: covered); - for (var i = 1; i <= ts.length; i++) { - final bankable = i < ts.length && - ts[i] - ts[i - 1] <= stepRunMaxGapSec && - !isCovered(ts[i - 1], ts[i]); - if (!bankable) { - closeRun(); - continue; - } - runStart ??= ts[i - 1]; - final d = samples[ts[i]]! - samples[ts[i - 1]]!; - if (d > 0) runSteps += d; - runLast = ts[i]; - } - return out; - } + static RowOrder decideRow( + String date, String? curDate, Set derived) => + NoopIngest.decideRow(date, curDate, derived); - /// Bank [date]'s step runs into `live_coverage` so the derivation picks them - /// up as REAL steps (`liveStepsForDay`) — the same contract the live 100 Hz - /// pedometer uses, so imported and live days are counted identically. These - /// are BAND-sourced counts (the strap's own step counter), which is what makes - /// them a real gait measurement rather than the deleted 1 Hz estimate. - /// - /// IDEMPOTENT BY TIME SPAN, not by exact window: `live_coverage` is an - /// append-only SUM with no uniqueness constraint, so anything already banked - /// over a second must not be banked again. The spans covering this batch are - /// read back and passed to [stepRuns], which clips them out. An exact-window - /// check is deliberately NOT used — it only catches a byte-identical - /// re-import and silently double-counts the overlap when the user exports - /// again over a longer span (see the note on [stepRuns]). - /// - /// Returns the steps actually banked (0 when the span was fully covered). - static Future _flushStepCoverage( - Map stepSamples, String date) async { - if (stepSamples.isEmpty) return 0; - final ts = stepSamples.keys.toList()..sort(); - final existing = - await LocalDb.coverageWindowsOverlapping(ts.first, ts.last + 1); - var banked = 0; - for (final r in stepRuns(stepSamples, covered: existing)) { - await LocalDb.addLiveCoverage(r.startSec, r.endSec, r.steps, date); - banked += r.steps; - } - return banked; - } - - /// String date compare 'YYYY-MM-DD' — true when [a] is strictly after [b]. - static bool _after(String a, String b) => a.compareTo(b) > 0; - - /// Where a row belongs given the import's high-water date [curDate] and the - /// set of dates already [derived] (and therefore already pruned out of the - /// rolling buffer). - /// - /// This is the whole out-of-order contract, isolated so it can be tested - /// without a database: `curDate` only ever moves FORWARD. It used to be - /// assigned unconditionally, so one backwards timestamp rewound it and the - /// next forward row called `deriveAndPrune()` — whose - /// `secs.removeWhere(label != date)` threw away every buffered sample of the - /// NEWER day, silently and with no error surfaced. - static RowOrder decideRow(String date, String? curDate, Set derived) { - if (curDate == null || _after(date, curDate)) return RowOrder.advance; - if (date == curDate) return RowOrder.buffer; - // Older than the high-water day. Still usable as prior-evening context - // unless its day has already been derived and pruned. - return derived.contains(date) ? RowOrder.late : RowOrder.buffer; - } + static const int stepRunMaxGapSec = NoopIngest.stepRunMaxGapSec; } diff --git a/lib/import/noop_ingest.dart b/lib/import/noop_ingest.dart new file mode 100644 index 00000000..908a7ba4 --- /dev/null +++ b/lib/import/noop_ingest.dart @@ -0,0 +1,368 @@ +// noop_ingest.dart — the one place NOOP samples become derived days. +// +// Extracted from noop_import.dart when the `.noopbak` path arrived. Both NOOP +// sources — the raw-sensor CSV and the SQLite database inside a `.noopbak` — +// carry the SAME 1 Hz signal family, so they must land through the same rolling +// window, the same step-coverage banking and the same out-of-order contract. A +// second copy of this logic would be a second answer to "what is this day", +// which is the class of bug the single-sleep-source and single-frame-ingest +// rules exist to prevent. +// +// MEMORY. A full backup is millions of rows and a 90-day CSV is hundreds of MB, +// so nothing here accumulates: at most the CURRENT and PREVIOUS local date are +// buffered, and each date is derived and freed as the next one opens. + +import '../compute/derivation_engine.dart'; +import '../compute/profile.dart'; +import '../compute/substrate.dart'; +import '../data/db.dart'; + +/// One contiguous run of the band's cumulative step counter, ready to bank as a +/// `live_coverage` row: real steps over a known [startSec]..[endSec] window. +class StepRun { + final int startSec; + final int endSec; + final int steps; + const StepRun(this.startSec, this.endSec, this.steps); + + @override + String toString() => 'StepRun($startSec..$endSec, $steps)'; + + @override + bool operator ==(Object other) => + other is StepRun && + other.startSec == startSec && + other.endSec == endSec && + other.steps == steps; + + @override + int get hashCode => Object.hash(startSec, endSec, steps); +} + +/// What to do with an incoming row given the import's high-water date. +/// [advance] closes out the previous date; [buffer] folds the row into the +/// current rolling window; [late] means its day was already derived + pruned, +/// so the row cannot be used. +enum RowOrder { advance, buffer, late } + +/// One second's worth of the 1 Hz channels (sparse — only the streams present). +class _Sec { + int? hr; + double? ax, ay, az; + int? spo2Red, spo2Ir, skinTemp; +} + +/// Streams a NOOP source into derived days, one local date at a time. +/// +/// Call [offer] with each sample's timestamp FIRST — it decides whether the row +/// is usable and closes out the previous date when time moves forward — then the +/// per-channel setters, then [finish] at end of input. +class NoopIngest { + NoopIngest(this._profile, this._engine, {this.onProgress}); + + final Profile _profile; + final DerivationEngine _engine; + final void Function(int days)? onProgress; + + /// Rolling buffer: at most the CURRENT + PREVIOUS local date of samples. + final Map _secs = {}; + final List _rrTs = []; // beat end time (epoch ms) + final List _rrMs = []; + + /// Band step counter, ts(sec) → cumulative value, keyed by local date so a run + /// is never attributed across midnight. Flushed to `live_coverage` BEFORE its + /// date derives, because the derivation reads real steps from there. + final Map> _stepsByDate = {}; + + String? _curDate; + final Set _derived = {}; // dates already derived + pruned + + int rows = 0; + int days = 0; + int steps = 0; + + /// Samples whose local date had ALREADY been derived and pruned by the time + /// they arrived (a genuinely out-of-order source). They cannot be folded back + /// in, so they are counted rather than silently dropped. + int lateRows = 0; + + /// Offer a sample at [ts] (epoch seconds). Returns false when the sample is + /// too late to use — the caller must then skip it entirely. + Future offer(int ts) async { + final date = localDateLabel(ts); + switch (decideRow(date, _curDate, _derived)) { + case RowOrder.advance: + final prev = _curDate; + if (prev != null) { + await _deriveAndPrune(prev); + _derived.add(prev); + } + _curDate = date; + case RowOrder.buffer: + break; + case RowOrder.late: + lateRows++; + return false; + } + rows++; + return true; + } + + void hr(int ts, int bpm) => (_secs[ts] ??= _Sec()).hr = bpm; + + void rr(int ts, double ms) { + if (ms <= 0) return; + _rrTs.add(ts * 1000.0); + _rrMs.add(ms); + } + + void gravity(int ts, double? x, double? y, double? z) { + final s = _secs[ts] ??= _Sec(); + s.ax = x; + s.ay = y; + s.az = z; + } + + void spo2(int ts, int? red, int? ir) { + final s = _secs[ts] ??= _Sec(); + s.spo2Red = red; + s.spo2Ir = ir; + } + + void skinTemp(int ts, int? raw) => (_secs[ts] ??= _Sec()).skinTemp = raw; + + /// A CUMULATIVE counter reading, not a per-second increment — differenced into + /// real step windows at flush time (see [stepRuns]). Deliberately kept out of + /// the Substrate: it is a real count, not a 1 Hz signal to analyse. + void stepCounter(int ts, int counter) { + if (counter < 0) return; + (_stepsByDate[localDateLabel(ts)] ??= {})[ts] = counter; + } + + /// End of input: derive the last buffered date, bank any steps whose date + /// never derived, and roll the import's day rollups forward. + Future finish() async { + final last = _curDate; + if (last != null && _secs.isNotEmpty) { + final st = _stepsByDate.remove(last); + if (st != null) steps += await flushStepCoverage(st, last); + final sub = _buildSubstrate(_secs, _rrTs, _rrMs); + days += await _engine.deriveImportedDays(sub, _profile, {last}); + onProgress?.call(days); + } + // A date that carried ONLY a step counter still banks its real count — + // dropping it would lose steps the band actually measured. + for (final e in _stepsByDate.entries) { + steps += await flushStepCoverage(e.value, e.key); + } + _stepsByDate.clear(); + await _engine.finalizeImport(_profile); + } + + Future _deriveAndPrune(String date) async { + // Real steps must be banked BEFORE deriving: the derivation reads them via + // LocalDb.liveStepsForDay / coverageWindowsOverlapping at derive time. + final st = _stepsByDate.remove(date); + if (st != null) steps += await flushStepCoverage(st, date); + // Build a Substrate from everything buffered (prev + current date) and + // derive ONLY [date]; the prior evening gives a sleep that started before + // midnight its context. + final sub = _buildSubstrate(_secs, _rrTs, _rrMs); + days += await _engine.deriveImportedDays(sub, _profile, {date}); + onProgress?.call(days); + // Keep [date]'s samples as the prior evening for the NEXT date; drop older. + _secs.removeWhere((ts, _) => localDateLabel(ts) != date); + var w = 0; + for (var i = 0; i < _rrMs.length; i++) { + if (localDateLabel((_rrTs[i] / 1000).floor()) == date) { + _rrTs[w] = _rrTs[i]; + _rrMs[w] = _rrMs[i]; + w++; + } + } + _rrTs.length = w; + _rrMs.length = w; + } + + /// String date compare 'YYYY-MM-DD' — true when [a] is strictly after [b]. + static bool _after(String a, String b) => a.compareTo(b) > 0; + + /// Where a row belongs given the import's high-water date [curDate] and the + /// set of dates already [derived] (and therefore already pruned out of the + /// rolling buffer). + /// + /// This is the whole out-of-order contract, isolated so it can be tested + /// without a database: `curDate` only ever moves FORWARD. It used to be + /// assigned unconditionally, so one backwards timestamp rewound it and the + /// next forward row derived the OLDER date — whose prune threw away every + /// buffered sample of the newer day, silently and with no error surfaced. + static RowOrder decideRow(String date, String? curDate, Set derived) { + if (curDate == null || _after(date, curDate)) return RowOrder.advance; + if (date == curDate) return RowOrder.buffer; + // Older than the high-water day. Still usable as prior-evening context + // unless its day has already been derived and pruned. + return derived.contains(date) ? RowOrder.late : RowOrder.buffer; + } + + /// Build a Substrate from the buffered seconds + RR beats. Gravity / SpO₂ / + /// skin-temp are forward-filled across seconds that lack their stream (the + /// real 1 Hz substrate carries a value every second); HR stays 0 when absent + /// (off-wrist semantics — meaningful, never forward-filled). + static Substrate _buildSubstrate( + Map secs, List rrTs, List rrMs) { + final tsList = secs.keys.toList()..sort(); + final n = tsList.length; + final tsSec = List.filled(n, 0); + final hr = List.filled(n, 0); + final ax = List.filled(n, 0); + final ay = List.filled(n, 0); + final az = List.filled(n, 0); + final spo2Red = List.filled(n, 0); + final spo2Ir = List.filled(n, 0); + final skinTemp = List.filled(n, 0); + + double fax = 0, fay = 0, faz = 0; // forward-fill carry + int fRed = 0, fIr = 0, fTemp = 0; + for (var i = 0; i < n; i++) { + final t = tsList[i]; + final s = secs[t]!; + tsSec[i] = t; + hr[i] = s.hr ?? 0; + if (s.ax != null) { + fax = s.ax!; + fay = s.ay ?? fay; + faz = s.az ?? faz; + } + ax[i] = fax; + ay[i] = fay; + az[i] = faz; + if (s.spo2Red != null) fRed = s.spo2Red!; + if (s.spo2Ir != null) fIr = s.spo2Ir!; + if (s.skinTemp != null) fTemp = s.skinTemp!; + spo2Red[i] = fRed; + spo2Ir[i] = fIr; + skinTemp[i] = fTemp; + } + + // RR beats sorted by time. + final order = List.generate(rrMs.length, (i) => i) + ..sort((a, b) => rrTs[a].compareTo(rrTs[b])); + return Substrate( + tsSec: tsSec, + hr: hr, + rrTsMs: [for (final i in order) rrTs[i]], + rrMs: [for (final i in order) rrMs[i]], + ax: ax, + ay: ay, + az: az, + spo2Red: spo2Red, + spo2Ir: spo2Ir, + skinTemp: skinTemp, + skinContact: ax.map((_) => 0).toList(), + ); + } + + /// Maximum gap (seconds) between consecutive step-counter samples that is + /// still treated as one continuous run. Both sources sample every second but + /// drop out whenever the band is off-wrist or unsynced; a gap wider than this + /// is a hole we know nothing about, so we refuse to span it. + static const int stepRunMaxGapSec = 60; + + /// Turn the band's CUMULATIVE step counter into discrete [StepRun]s that can + /// be banked as real (non-estimated) step counts. + /// + /// [samples] is ts(sec) → counter value, any order. Runs are split on a gap + /// wider than [stepRunMaxGapSec], so a 20 h hole in a source never becomes one + /// window claiming to cover the day. + /// + /// Only POSITIVE deltas within a run are summed: the counter resets to 0 on a + /// band reboot, and a negative delta is that reset, not −24,000 steps. Deltas + /// ACROSS a run boundary are deliberately NOT counted — we cannot attribute + /// steps to a window we have no samples for. + /// + /// A run with zero steps yields no window: `live_coverage` exists to suppress + /// the 1 Hz estimate over minutes the real counter already covered, and a + /// 0-step run would suppress a real estimate while contributing nothing. + /// + /// [covered] lists spans ALREADY banked in `live_coverage` (device-time + /// seconds, inclusive). A per-second delta whose interval intersects one of + /// them is skipped and BREAKS the run, so those steps are never banked twice. + /// This is what makes a re-import safe in general rather than only for a + /// byte-identical file: an exact-window check alone is defeated the moment the + /// user exports again over a longer span (09:00-09:20 then 09:00-09:40), where + /// the run boundary shifts, no exact window matches, and the overlap is banked + /// a second time — measured at 3,598 steps against a true 2,399 before this. + /// It also means an imported span cannot double-count against a LIVE 100 Hz + /// pedometer window, which shares this table. + static List stepRuns( + Map samples, { + List> covered = const [], + }) { + if (samples.isEmpty) return const []; + final ts = samples.keys.toList()..sort(); + + // Does the half-open delta interval (a, b] touch anything already banked? + bool isCovered(int a, int b) { + for (final w in covered) { + if (b > w[0] && a < w[1]) return true; + } + return false; + } + + final out = []; + int? runStart, runLast; + var runSteps = 0; + void closeRun() { + if (runStart != null && + runLast != null && + runSteps > 0 && + runLast! > runStart!) { + out.add(StepRun(runStart!, runLast!, runSteps)); + } + runStart = null; + runLast = null; + runSteps = 0; + } + + for (var i = 1; i <= ts.length; i++) { + final bankable = i < ts.length && + ts[i] - ts[i - 1] <= stepRunMaxGapSec && + !isCovered(ts[i - 1], ts[i]); + if (!bankable) { + closeRun(); + continue; + } + runStart ??= ts[i - 1]; + final d = samples[ts[i]]! - samples[ts[i - 1]]!; + if (d > 0) runSteps += d; + runLast = ts[i]; + } + return out; + } + + /// Bank [date]'s step runs into `live_coverage` so the derivation picks them + /// up as REAL steps (`liveStepsForDay`) — the same contract the live 100 Hz + /// pedometer uses, so imported and live days are counted identically. These + /// are BAND-sourced counts (the strap's own step counter), which is what makes + /// them a real gait measurement rather than an estimate. + /// + /// IDEMPOTENT BY TIME SPAN, not by exact window: `live_coverage` is an + /// append-only SUM with no uniqueness constraint, so anything already banked + /// over a second must not be banked again. The spans covering this batch are + /// read back and passed to [stepRuns], which clips them out. + /// + /// Returns the steps actually banked (0 when the span was fully covered). + static Future flushStepCoverage( + Map stepSamples, String date) async { + if (stepSamples.isEmpty) return 0; + final ts = stepSamples.keys.toList()..sort(); + final existing = + await LocalDb.coverageWindowsOverlapping(ts.first, ts.last + 1); + var banked = 0; + for (final r in stepRuns(stepSamples, covered: existing)) { + await LocalDb.addLiveCoverage(r.startSec, r.endSec, r.steps, date); + banked += r.steps; + } + return banked; + } +} diff --git a/lib/ui/coach/ai_coach_screen.dart b/lib/ui/coach/ai_coach_screen.dart index ee4a1a7e..8a5aeb0f 100644 --- a/lib/ui/coach/ai_coach_screen.dart +++ b/lib/ui/coach/ai_coach_screen.dart @@ -282,7 +282,7 @@ class _AiCoachScreenState extends State { children: [ Expanded( child: !cfg.configured - ? _setupPrompt() + ? (cfg.keyUnreadable ? _lockedKeyNotice(cfg) : _setupPrompt()) : !signedIn ? _centered('Pair your strap to use the coach.') : _items.isEmpty @@ -296,6 +296,29 @@ class _AiCoachScreenState extends State { ); } + /// The key IS saved, this process just could not read it — the phone was + /// locked when a background relaunch went looking. Showing the "bring your own + /// AI" setup wall here would tell the user their key is gone and invite them + /// to paste it again, which is both wrong and the thing they reported. + Widget _lockedKeyNotice(CoachConfig cfg) => ListView( + padding: const EdgeInsets.fromLTRB(Sp.screen, Sp.x4, Sp.screen, Sp.x6), + children: [ + StateCard( + icon: OsIcon.ai, + title: 'Your key is still saved', + message: + 'It could not be read from the keychain this time — that ' + 'happens when the app is woken while the phone is locked. ' + 'Retry, or reopen the app once the phone has been unlocked.', + actionLabel: 'Retry', + onAction: () async { + await cfg.refreshKeyIfUnreadable(); + if (mounted) setState(() {}); + }, + ).dsEnter(), + ], + ); + Widget _setupPrompt() => ListView( padding: const EdgeInsets.fromLTRB(Sp.screen, Sp.x4, Sp.screen, Sp.x6), children: [ diff --git a/lib/ui/design/app_scaffold.dart b/lib/ui/design/app_scaffold.dart index ffb491eb..89d25d20 100644 --- a/lib/ui/design/app_scaffold.dart +++ b/lib/ui/design/app_scaffold.dart @@ -59,6 +59,22 @@ class AppBackButton extends StatelessWidget { } } +/// Bottom padding a scrolling screen needs so its last card clears the shell's +/// floating chrome, plus [gap] of breathing room. +/// +/// MEASURED, not guessed. The shell renders its nav pill through +/// `Scaffold(extendBody: true)`, and Flutter reports the real height of that +/// bottom chrome — pill, safe-area inset, and the live-workout banner stacked +/// above it — as `MediaQuery.padding.bottom` inside the body. Screens used to +/// carry a hardcoded `120`, which on an iPhone 17 Pro leaves 14 pt of clearance +/// over a 106 pt chrome and goes NEGATIVE the moment a workout is running and +/// the banner appears: the last card of every tab disappears behind it, exactly +/// when the user is most likely to be looking. A pushed sub-screen has no pill, +/// so its inset is just the home indicator and this returns the smaller number +/// on its own. +double dsBottomGutter(BuildContext context, {double gap = Sp.x6}) => + MediaQuery.paddingOf(context).bottom + gap; + class AppScaffold extends StatelessWidget { final String? title; @@ -131,7 +147,9 @@ class AppScaffold extends StatelessWidget { Sp.screen, Sp.x2, Sp.screen, - bottomBar == null ? Sp.x8 : 120, + // A local [bottomBar] floats over this list too, so its height is + // added on top of whatever the shell already claims. + dsBottomGutter(context, gap: bottomBar == null ? Sp.x8 : 96), ), children: children!, ); diff --git a/lib/ui/import/import_screen.dart b/lib/ui/import/import_screen.dart index 23504595..294770e1 100644 --- a/lib/ui/import/import_screen.dart +++ b/lib/ui/import/import_screen.dart @@ -150,7 +150,8 @@ class _ImportScreenState extends State { ImportOptionCard( icon: OsIcon.heartRate, title: 'Import from NOOP', - body: 'Raw 1 Hz CSV — re-analyzed end-to-end on this phone.', + body: 'A .noopbak backup or the raw 1 Hz CSV — re-analyzed ' + 'end-to-end on this phone.', onTap: _locked ? null : _importNoop, ), const SizedBox(height: Sp.x3), diff --git a/lib/ui/screens/metric_screen.dart b/lib/ui/screens/metric_screen.dart index fe8d23f7..b07a8d51 100644 --- a/lib/ui/screens/metric_screen.dart +++ b/lib/ui/screens/metric_screen.dart @@ -82,7 +82,8 @@ class _MetricScreenState extends State { physics: const BouncingScrollPhysics( parent: AlwaysScrollableScrollPhysics(), ), - padding: const EdgeInsets.fromLTRB(Sp.screen, Sp.x3, Sp.screen, 120), + padding: EdgeInsets.fromLTRB( + Sp.screen, Sp.x3, Sp.screen, dsBottomGutter(context)), children: [ if (_tab == 0) KeyedSubtree( diff --git a/lib/ui/today/today_screen.dart b/lib/ui/today/today_screen.dart index 255f86aa..6d44e043 100644 --- a/lib/ui/today/today_screen.dart +++ b/lib/ui/today/today_screen.dart @@ -242,7 +242,8 @@ class _TodayScreenState extends State physics: const BouncingScrollPhysics( parent: AlwaysScrollableScrollPhysics(), ), - padding: const EdgeInsets.fromLTRB(Sp.screen, Sp.x2, Sp.screen, 120), + padding: EdgeInsets.fromLTRB( + Sp.screen, Sp.x2, Sp.screen, dsBottomGutter(context)), children: [ // OTA update prompt + admin alert banner (self-hiding). const StatusBanner(), diff --git a/lib/ui/workouts/workouts_screen.dart b/lib/ui/workouts/workouts_screen.dart index b5ca14b1..5cbce1c2 100644 --- a/lib/ui/workouts/workouts_screen.dart +++ b/lib/ui/workouts/workouts_screen.dart @@ -242,7 +242,8 @@ class _WorkoutsScreenState extends State { physics: const BouncingScrollPhysics( parent: AlwaysScrollableScrollPhysics(), ), - padding: const EdgeInsets.fromLTRB(Sp.screen, Sp.x2, Sp.screen, 120), + padding: EdgeInsets.fromLTRB( + Sp.screen, Sp.x2, Sp.screen, dsBottomGutter(context)), children: [ if (!_loading && _suggestions.isNotEmpty) ...[ const SectionHeader('Suggested workouts'), diff --git a/test/bottom_gutter_test.dart b/test/bottom_gutter_test.dart new file mode 100644 index 00000000..485b7ad7 --- /dev/null +++ b/test/bottom_gutter_test.dart @@ -0,0 +1,98 @@ +// The last card of a scrolling screen must clear the shell's floating chrome. +// +// Screens carried a hardcoded 120 pt bottom padding. The shell's chrome is 106 +// pt on a current iPhone (72 pt pill + 34 pt home indicator), so that left 14 +// pt — and the live-workout banner stacks ABOVE the pill inside the same +// `bottomNavigationBar`, which pushes the chrome past 120 and buries the last +// card while a workout is running. +// +// `Scaffold(extendBody: true)` reports the true height of that chrome as +// `MediaQuery.padding.bottom` inside the body, so the padding is derived from it +// rather than guessed. + +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:openstrap_edge/app.dart'; +import 'package:openstrap_edge/ui/design/app_scaffold.dart'; +import 'package:openstrap_edge/ui/design/nav_pill.dart'; +import 'package:openstrap_edge/ui/kit/os_icons.dart'; + +/// iPhone 17 Pro-ish: 34 pt home indicator. +const _phone = MediaQueryData( + size: Size(393, 852), + padding: EdgeInsets.only(top: 59, bottom: 34), + viewPadding: EdgeInsets.only(top: 59, bottom: 34), +); + +Widget _shell({Widget? banner, required Widget page}) => MaterialApp( + home: MediaQuery( + data: _phone, + child: ShellScaffold( + controller: PageController(), + index: 0, + items: const [NavPillItem(OsIcon.today, 'Today')], + pages: [page], + onSelect: (_) {}, + onPageChanged: (_) {}, + banner: banner, + ), + ), + ); + +void main() { + testWidgets('the gutter covers the whole nav pill, not a guessed constant', + (t) async { + late double gutter; + await t.pumpWidget(_shell( + page: Builder(builder: (c) { + gutter = dsBottomGutter(c, gap: 0); + return const SizedBox.expand(); + }), + )); + + final pill = t.getSize(find.byType(FloatingNavPill)); + expect(gutter, greaterThanOrEqualTo(pill.height + _phone.padding.bottom), + reason: 'the pill and the home indicator both have to be cleared'); + }); + + testWidgets('a live-workout banner grows the gutter with it', (t) async { + late double plain; + await t.pumpWidget(_shell( + page: Builder(builder: (c) { + plain = dsBottomGutter(c, gap: 0); + return const SizedBox.expand(); + }), + )); + + late double withBanner; + await t.pumpWidget(_shell( + banner: const SizedBox(height: 64), + page: Builder(builder: (c) { + withBanner = dsBottomGutter(c, gap: 0); + return const SizedBox.expand(); + }), + )); + + expect(withBanner, plain + 64, + reason: 'the banner stacks above the pill and must be cleared too'); + // The old constant is the regression this pins: it was already smaller than + // the plain chrome, and a running workout put it far under. + expect(withBanner, greaterThan(120)); + }); + + testWidgets('a screen outside the shell only clears its own safe area', + (t) async { + late double gutter; + await t.pumpWidget(MaterialApp( + home: MediaQuery( + data: _phone, + child: Builder(builder: (c) { + gutter = dsBottomGutter(c, gap: 0); + return const SizedBox.expand(); + }), + ), + )); + // No pill on a pushed sub-screen — padding it for one would leave a hole. + expect(gutter, _phone.padding.bottom); + }); +} diff --git a/test/coach_config_key_test.dart b/test/coach_config_key_test.dart new file mode 100644 index 00000000..4cbba8a7 --- /dev/null +++ b/test/coach_config_key_test.dart @@ -0,0 +1,157 @@ +// The BYOK key must survive a background relaunch on a locked phone. +// +// Two TestFlight reports: "I enter my AI API key, it works for a few minutes, +// then after the phone sleeps and wakes it's gone." The key was written with the +// plugin's default `whenUnlocked` accessibility, and this app is relaunched in +// the background constantly (BGProcessingTask, the BLE restore central) — often +// while the phone is locked, when that item cannot be read. `load()` cached the +// empty read as "no key", so the app asked the user to set one up again. + +import 'package:flutter/foundation.dart'; +import 'package:flutter/services.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:openstrap_edge/coach/coach_config.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +/// Stands in for the keychain: records the options every write asks for, and +/// can be told to behave like a locked device. +class _FakeKeychain { + final Map items = {}; + final List> writeOptions = []; + bool locked = false; + bool throwOnRead = false; + + Future handle(MethodCall call) async { + final args = (call.arguments as Map?) ?? const {}; + switch (call.method) { + case 'read': + if (throwOnRead) throw PlatformException(code: 'keychain'); + // A locked keychain does not error — it simply returns nothing, which + // is indistinguishable from "no key" without the marker. + if (locked) return null; + return items[args['key'] as String]; + case 'write': + items[args['key'] as String] = args['value'] as String; + writeOptions.add((args['options'] as Map?) ?? const {}); + return null; + case 'delete': + items.remove(args['key'] as String); + return null; + case 'containsKey': + return !locked && items.containsKey(args['key'] as String); + case 'readAll': + return locked ? {} : items; + default: + return null; + } + } +} + +void main() { + TestWidgetsFlutterBinding.ensureInitialized(); + const channel = MethodChannel('plugins.it_nomads.com/flutter_secure_storage'); + late _FakeKeychain keychain; + + setUp(() { + keychain = _FakeKeychain(); + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger + .setMockMethodCallHandler(channel, keychain.handle); + SharedPreferences.setMockInitialValues({}); + }); + + tearDown(() { + TestDefaultBinaryMessengerBinding.instance.defaultBinaryMessenger + .setMockMethodCallHandler(channel, null); + }); + + test('a key is stored so it survives a locked-device relaunch', () async { + // The accessibility attribute is an Apple-keychain concept, and the plugin + // picks its option set off defaultTargetPlatform (android under test). + debugDefaultTargetPlatformOverride = TargetPlatform.iOS; + addTearDown(() => debugDefaultTargetPlatformOverride = null); + final cfg = CoachConfig(); + await cfg.save(apiKey: 'sk-test', model: 'gpt-4o'); + + expect(keychain.writeOptions, isNotEmpty); + // `first_unlock`, not the plugin default of `unlocked`: readable from the + // first unlock after boot, which is what a background relaunch needs. + expect( + keychain.writeOptions.last['accessibility'], + 'first_unlock', + reason: 'a whenUnlocked item is unreadable during a locked relaunch', + ); + }); + + test('a locked keychain does not report the key as missing', () async { + final saved = CoachConfig(); + await saved.save(apiKey: 'sk-test', model: 'gpt-4o'); + + // A fresh process — the background relaunch — with the phone locked. + keychain.locked = true; + final relaunched = CoachConfig(); + await relaunched.load(); + + expect(relaunched.hasKey, isFalse, reason: 'it genuinely could not read it'); + expect(relaunched.keyUnreadable, isTrue, + reason: 'but it must not be reported as "no key configured"'); + + // The user unlocks and opens the app. + keychain.locked = false; + await relaunched.refreshKeyIfUnreadable(); + expect(relaunched.apiKey, 'sk-test'); + expect(relaunched.keyUnreadable, isFalse); + }); + + test('a read that throws does not erase a key already in memory', () async { + final cfg = CoachConfig(); + await cfg.save(apiKey: 'sk-test', model: 'gpt-4o'); + expect(cfg.apiKey, 'sk-test'); + + keychain.throwOnRead = true; + await cfg.load(); + + expect(cfg.apiKey, 'sk-test', + reason: 'a failed read says nothing about what is stored'); + expect(cfg.keyUnreadable, isTrue); + }); + + test('no key configured still reads as no key, not as unreadable', () async { + final cfg = CoachConfig(); + await cfg.load(); + expect(cfg.hasKey, isFalse); + expect(cfg.keyUnreadable, isFalse, + reason: 'nothing was ever saved — the setup prompt is correct here'); + }); + + test('clearing the key clears the marker with it', () async { + final cfg = CoachConfig(); + await cfg.save(apiKey: 'sk-test'); + await cfg.save(apiKey: ''); + + keychain.locked = true; + final relaunched = CoachConfig(); + await relaunched.load(); + expect(relaunched.keyUnreadable, isFalse, + reason: 'a deleted key must not leave the app claiming one exists'); + }); + + test('a key written before the marker existed is upgraded once', () async { + debugDefaultTargetPlatformOverride = TargetPlatform.iOS; + addTearDown(() => debugDefaultTargetPlatformOverride = null); + // Legacy state: an item in the keychain, no marker in prefs. + keychain.items['coach_api_key'] = 'sk-legacy'; + SharedPreferences.setMockInitialValues({}); + + final cfg = CoachConfig(); + await cfg.load(); + expect(cfg.apiKey, 'sk-legacy'); + expect(keychain.writeOptions.length, 1, + reason: 'rewritten once to carry the new accessibility'); + expect(keychain.writeOptions.single['accessibility'], 'first_unlock'); + + // A second load must NOT write again — the Android Keystore is the + // documented Samsung hang, and it has no business on every startup. + await cfg.load(); + expect(keychain.writeOptions.length, 1); + }); +} diff --git a/test/import_container_test.dart b/test/import_container_test.dart index f730fd9add92b281b054abc7a27c36207cc0806c..b8cf4de3606d811e4858f178a7dcc38a04bb0c1f 100644 GIT binary patch delta 1003 zcmbVLO>5Lp6a_!1A4sXx8IkF5#ZEI*W-={B6h%Q=1VKm2RNWNeCAl+s^d&F8_tI&U zQoC~}^4$6#iv9psu3Wiwr+at)0Bc^C z7f5{yq?W6Y1-EqHC>{@+RH_YArUdV=-X%a7NEI;yOHS(mvD*dW7*Y-O0)Ybq9@1|B zGWHQnh`^&nDuW6_rlFd*A+g+Y+A)*Bvv+FsiIR(5t<>#{+GIDc7EBFMn*(hj!pI}A z5sx*d4N7sB=*%Kd}zN_9DAdB_GD+ogmVp?E!(Xwoq^eBPUgToTwYk%K7<{I zQ+$6uH{d#v8k?nZ+w>+0?91ATMFv9B(a#Y{Ny9cU2Btgo%}C7{^HBp7Xo^&Irk$dw zll#7Jc-v0R)sKSHvL724{v&DQ(c17^4$0Y-f^+U|l#&I#`)efw(tJSSff14BL{6GA z@$mKeYjQWw!p+>hRHJfq=`|tBSY+VKBm+6f!@(&PTpOvfzXYN;&)_Lf@~g}JhqPk4 z=oC>wp$^57h>SB&-d{OB3P}vR+n77IocTnrh%gu);d;Ed9@{UMrtQ7S31@7|KR2;h QJlZ;oypqWum8B-f$jeTC zDpt+NFga0NaWcQU(_}Zf&OD$R|1=FQ1r(5+pI4HYnOCf#o>rQZqu}T7AE1zwn4Dc& ppsuNdQ*BXVxq_jBN0mZxYF=@Ek%Dt@7`pPw2AY writeNoopDb( + String name, { + required int t0, + required int seconds, + String deviceId = 'my-whoop', + bool withEmptyOptionalTables = true, + }) async { + final path = p.join(tmp.path, name); + if (File(path).existsSync()) File(path).deleteSync(); + final db = await databaseFactory.openDatabase(path); + await db.execute('CREATE TABLE hrSample (deviceId TEXT NOT NULL, ' + 'ts INTEGER NOT NULL, bpm INTEGER NOT NULL, synced INTEGER NOT NULL ' + 'DEFAULT 0, PRIMARY KEY (deviceId, ts))'); + await db.execute('CREATE TABLE rrInterval (deviceId TEXT NOT NULL, ' + 'ts INTEGER NOT NULL, rrMs INTEGER NOT NULL, synced INTEGER NOT NULL ' + 'DEFAULT 0, PRIMARY KEY (deviceId, ts, rrMs))'); + await db.execute('CREATE TABLE gravitySample (deviceId TEXT NOT NULL, ' + 'ts INTEGER NOT NULL, x DOUBLE NOT NULL, y DOUBLE NOT NULL, ' + 'z DOUBLE NOT NULL, PRIMARY KEY (deviceId, ts))'); + await db.execute('CREATE TABLE skinTempSample (deviceId TEXT NOT NULL, ' + 'ts INTEGER NOT NULL, raw INTEGER NOT NULL, PRIMARY KEY (deviceId, ts))'); + await db.execute('CREATE TABLE stepSample (deviceId TEXT NOT NULL, ' + 'ts INTEGER NOT NULL, counter INTEGER NOT NULL, ' + 'PRIMARY KEY (deviceId, ts))'); + if (withEmptyOptionalTables) { + // Present but EMPTY in the real backup — the importer must read them + // without deciding the file is unusable. + await db.execute('CREATE TABLE spo2Sample (deviceId TEXT NOT NULL, ' + 'ts INTEGER NOT NULL, red INTEGER NOT NULL, ir INTEGER NOT NULL, ' + 'PRIMARY KEY (deviceId, ts))'); + await db.execute('CREATE TABLE respSample (deviceId TEXT NOT NULL, ' + 'ts INTEGER NOT NULL, raw INTEGER NOT NULL, ' + 'PRIMARY KEY (deviceId, ts))'); + } + // NOOP's own scores. Note the DIFFERENT deviceId, exactly as shipped — we + // never read these, and nothing may filter samples on a device id because of + // it. + await db.execute('CREATE TABLE sleepSession (deviceId TEXT NOT NULL, ' + 'startTs INTEGER NOT NULL, endTs INTEGER NOT NULL, efficiency DOUBLE, ' + 'restingHr INTEGER, avgHrv DOUBLE, stagesJSON TEXT, ' + 'PRIMARY KEY (deviceId, startTs))'); + await db.insert('sleepSession', { + 'deviceId': '$deviceId-noop', + 'startTs': t0, + 'endTs': t0 + seconds, + 'efficiency': 0.93, + 'restingHr': 52, + 'avgHrv': 95.3, + 'stagesJSON': '[]', + }); + + final batch = db.batch(); + for (var i = 0; i < seconds; i++) { + final ts = t0 + i; + batch.insert('hrSample', { + 'deviceId': deviceId, + 'ts': ts, + 'bpm': 60 + (i % 20), + }); + batch.insert('gravitySample', { + 'deviceId': deviceId, + 'ts': ts, + 'x': 0.1, + 'y': 0.2, + 'z': 0.97, + }); + batch.insert( + 'skinTempSample', {'deviceId': deviceId, 'ts': ts, 'raw': 3240}); + batch.insert('stepSample', { + 'deviceId': deviceId, + 'ts': ts, + 'counter': 24302 + i, + }); + if (i % 2 == 0) { + batch.insert( + 'rrInterval', {'deviceId': deviceId, 'ts': ts, 'rrMs': 900 + i % 40}); + } + } + await batch.commit(noResult: true); + await db.close(); + return path; + } + + /// Zip [dbPath] up the way NOOP does: one member, named `noop-backup.sqlite`. + String writeBackup(String name, String dbPath, {String? extraMember}) { + final archive = Archive(); + final bytes = File(dbPath).readAsBytesSync(); + archive.addFile(ArchiveFile('noop-backup.sqlite', bytes.length, bytes)); + if (extraMember != null) { + final e = [1, 2, 3]; + archive.addFile(ArchiveFile(extraMember, e.length, e)); + } + final out = p.join(tmp.path, name); + File(out).writeAsBytesSync(ZipEncoder().encode(archive)); + return out; + } + + test('imports a .noopbak end to end, banking the band step counter', + () async { + // 2026-07-31T09:00:00Z, 40 min of 1 Hz data. + const t0 = 1785488400; + const secs = 2400; + final dbPath = await writeNoopDb('a.sqlite', t0: t0, seconds: secs); + final bak = writeBackup('backup.noopbak', dbPath); + + final res = await NoopImporter.importFile( + bak, const Profile(), DerivationEngine()); + + expect(res.days, greaterThan(0)); + expect(res.lateRows, 0); + // Every 1 Hz channel row counts, so the row total dwarfs the second count. + expect(res.rows, greaterThan(secs)); + // The band's own counter, banked as REAL steps rather than an estimate. + expect(res.steps, secs - 1); + + final db = await LocalDb.instance; + final cov = await db.query('live_coverage'); + expect(cov.fold(0, (a, r) => a + (r['steps'] as int)), secs - 1); + + // The day derived from the backup's own samples, not from NOOP's scores. + final days = await db.query('day_result'); + expect(days, isNotEmpty); + }, timeout: const Timeout(Duration(minutes: 5))); + + test('re-importing the same backup does not double-count steps', () async { + const t0 = 1785660000; // 2026-08-02 + const secs = 900; + final dbPath = await writeNoopDb('b.sqlite', t0: t0, seconds: secs); + final bak = writeBackup('b.noopbak', dbPath); + + final first = await NoopImporter.importFile( + bak, const Profile(), DerivationEngine()); + expect(first.steps, secs - 1); + + final again = await NoopImporter.importFile( + bak, const Profile(), DerivationEngine()); + expect(again.steps, 0, reason: 'the span is already covered'); + }, timeout: const Timeout(Duration(minutes: 5))); + + test('the unpacked database is deleted once the import finishes', () async { + const t0 = 1785746400; // 2026-08-03 + final dbPath = await writeNoopDb('c.sqlite', t0: t0, seconds: 120); + final bak = writeBackup('c.noopbak', dbPath); + + final before = tmp.listSync().whereType().length; + await NoopImporter.importFile(bak, const Profile(), DerivationEngine()); + // Nothing extracted survives — a 260 MB backup would otherwise leave a full + // second copy behind on the phone. + final leftovers = Directory.systemTemp + .listSync() + .whereType() + .where((d) => p.basename(d.path).startsWith('openstrap_noopbak_')); + expect(leftovers, isEmpty); + expect(tmp.listSync().whereType().length, before); + }, timeout: const Timeout(Duration(minutes: 5))); + + test('no RR beat is lost to a page boundary inside a second', () async { + // `rrInterval`'s key is (deviceId, ts, rrMs): one second holds several + // beats. Keyset paging on ts alone would skip whatever sits past the page + // edge within that second — silently, and only on real-sized backups. + const t0 = 1785832800; // 2026-08-04 + const secs = 300; + const beatsPerSec = 4; + final path = p.join(tmp.path, 'rr.sqlite'); + if (File(path).existsSync()) File(path).deleteSync(); + final src = await databaseFactory.openDatabase(path); + await src.execute('CREATE TABLE hrSample (deviceId TEXT, ts INTEGER, ' + 'bpm INTEGER, PRIMARY KEY (deviceId, ts))'); + await src.execute('CREATE TABLE rrInterval (deviceId TEXT, ts INTEGER, ' + 'rrMs INTEGER, PRIMARY KEY (deviceId, ts, rrMs))'); + final b = src.batch(); + for (var i = 0; i < secs; i++) { + b.insert('hrSample', {'deviceId': 'd', 'ts': t0 + i, 'bpm': 65}); + for (var k = 0; k < beatsPerSec; k++) { + b.insert( + 'rrInterval', {'deviceId': 'd', 'ts': t0 + i, 'rrMs': 800 + k * 7}); + } + } + await b.commit(noResult: true); + await src.close(); + final bak = writeBackup('rr.noopbak', path); + + // A page size that cannot align to the 4-beats-per-second grid, so + // boundaries land mid-second. + final saved = kNoopBackupPageRows; + kNoopBackupPageRows = 7; + addTearDown(() => kNoopBackupPageRows = saved); + + final res = await NoopImporter.importFile( + bak, const Profile(), DerivationEngine()); + expect(res.rows, secs + secs * beatsPerSec, + reason: 'every HR sample and every RR beat was read'); + }, timeout: const Timeout(Duration(minutes: 5))); + + test('a database that is not a NOOP backup is named, not silently empty', + () async { + final path = p.join(tmp.path, 'other.sqlite'); + final db = await databaseFactory.openDatabase(path); + await db.execute('CREATE TABLE notes (id INTEGER PRIMARY KEY, body TEXT)'); + await db.close(); + final bak = writeBackup('other.noopbak', path); + + await expectLater( + NoopImporter.importFile(bak, const Profile(), DerivationEngine()), + throwsA(isA() + .having((e) => e.message, 'message', contains('hrSample'))), + ); + }); + + test('a backup with no samples says so rather than importing 0 days', + () async { + final path = p.join(tmp.path, 'empty.sqlite'); + final db = await databaseFactory.openDatabase(path); + await db.execute('CREATE TABLE hrSample (deviceId TEXT, ts INTEGER, ' + 'bpm INTEGER, PRIMARY KEY (deviceId, ts))'); + await db.close(); + final bak = writeBackup('empty.noopbak', path); + + await expectLater( + NoopImporter.importFile(bak, const Profile(), DerivationEngine()), + throwsA(isA() + .having((e) => e.message, 'message', contains('no samples'))), + ); + }); +} From ba95c13672b3945b32470e884191f21477242ea9 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 21:30:31 +0530 Subject: [PATCH 2/6] review pass: don't lose a key, a day, or the app to a hang MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the legacy case was still broken: an install predating the marker has no marker, so a locked relaunch concluded "no key" and never retried — exactly the bug this was meant to fix. the marker is three-state now, and only a foreground read settles it, which is also the only way out of a marker that outlived its item. the settings screen seeded its field from a key it could not read and saving then deleted the real one. the prior-evening buffer was retained across any gap, so after ~400 days the day walk never reached the target date and the day went missing from an import that reported success. paging could stall outright on a REAL timestamp column, and a partial 260MB extraction leaked when the write failed. --- lib/app.dart | 2 +- lib/coach/coach_config.dart | 122 +++++++++++++---- lib/import/import_container.dart | 48 ++++--- lib/import/noop_backup_import.dart | 52 +++++++- lib/import/noop_import.dart | 14 +- lib/import/noop_ingest.dart | 59 ++++++++- lib/state/app_state.dart | 7 + lib/ui/coach/ai_coach_screen.dart | 2 +- lib/ui/coach/coach_settings_screen.dart | 28 +++- lib/ui/import/import_screen.dart | 8 ++ test/coach_config_key_test.dart | 114 +++++++++++++++- test/noop_backup_import_test.dart | 168 +++++++++++++++++++++++- 12 files changed, 553 insertions(+), 71 deletions(-) diff --git a/lib/app.dart b/lib/app.dart index b7feceac..241d9247 100644 --- a/lib/app.dart +++ b/lib/app.dart @@ -79,7 +79,7 @@ class _OpenStrapAppState extends State with WidgetsBindingObserver // process. Coming to the foreground means the phone is unlocked — take // the chance to read it. No-op unless a key is known to exist and is // currently unreadable. - unawaited(context.read().refreshKeyIfUnreadable()); + unawaited(context.read().refreshKeyOnResume()); app.maybeFinishFromLiveActivity(); unawaited(app.maybeStopBreathingFromLiveActivity()); app.refreshAppStatus(); // re-check OTA + admin banner on every foreground diff --git a/lib/coach/coach_config.dart b/lib/coach/coach_config.dart index 75e49fed..ea26cb32 100644 --- a/lib/coach/coach_config.dart +++ b/lib/coach/coach_config.dart @@ -16,6 +16,14 @@ class CoachConfig extends ChangeNotifier { /// itself cannot answer "is there a key I currently can't read?" — a locked /// device and an empty keychain both read as nothing — so the answer is kept /// here, where it is always readable. + /// + /// THREE states, not two. ABSENT (null) means nobody has established the + /// answer yet, which is where every install that predates this marker starts: + /// their key is in the keychain with no marker beside it. Treating absent as + /// "no key" would fail that user exactly as the old code did — a locked + /// background relaunch reads nothing, concludes there is no key, and never + /// retries. Absent therefore stays UNDETERMINED until a read happens with the + /// app in the foreground, where the device is unlocked by definition. static const _kKeyPresent = 'coach_api_key_present'; static const String defaultBaseUrl = 'https://api.openai.com/v1'; @@ -51,6 +59,19 @@ class CoachConfig extends ChangeNotifier { bool _keyUnreadable = false; bool get keyUnreadable => _keyUnreadable; + /// True while it is still unknown whether a key is stored — an install that + /// predates the marker, read while the keychain was unavailable. Not shown to + /// the user (there may genuinely be no key); it only keeps the resume retry + /// eligible so a legacy key appears by itself once the phone is unlocked. + bool _keyUndetermined = false; + bool get keyUndetermined => _keyUndetermined; + + /// Bumped by every [save]. A [load] that started before a save must not apply + /// its stale result afterwards: the startup load is unawaited and a slow + /// keystore read can still be in flight when the user pastes a key, and its + /// late `_key = null` would wipe the key they just saved out of the session. + int _generation = 0; + String get baseUrl => _baseUrl; String get model => _model; String? get apiKey => _key; @@ -66,27 +87,43 @@ class CoachConfig extends ChangeNotifier { return b; } - Future load() async { + /// [trusted] marks a read taken with the app in the FOREGROUND, i.e. with the + /// device unlocked — the only condition under which an EMPTY read is real + /// evidence about what is stored. + Future load({bool trusted = false}) async { final prefs = await SharedPreferences.getInstance(); _baseUrl = prefs.getString(_kBaseUrl) ?? defaultBaseUrl; _model = prefs.getString(_kModel) ?? ''; - final expectKey = prefs.getBool(_kKeyPresent) ?? false; + final marker = prefs.getBool(_kKeyPresent); // null = undetermined + final generation = _generation; + + // Pending BEFORE the read, not after. The startup read is wrapped in a + // timeout that cannot cancel the underlying call, and the Android Keystore + // can hang outright — if the answer is only recorded once the read returns, + // a read that never returns leaves the app looking like "no key configured" + // with the resume retry permanently disabled. + _keyUnreadable = marker == true && !hasKey; + _keyUndetermined = marker == null; + try { final read = await _secure.read( key: _kKey, iOptions: _apple, mOptions: _macos, ); + // A save landed while this read was in flight — it knows more than we do. + if (generation != _generation) return; if (read != null && read.isNotEmpty) { _key = read; _keyUnreadable = false; + _keyUndetermined = false; // Upgrade an item written before this class asked for `first_unlock`: // accessibility is set at WRITE time, so an existing key keeps the old - // attribute until it is written again. Keyed on the marker's absence so - // this happens exactly once — a write on every load would put the - // Android Keystore (the documented Samsung Knox hang) on the startup - // path for no reason. - if (!expectKey) { + // attribute until it is written again. Keyed on the marker so this + // happens exactly once — a write on every load would put the Android + // Keystore (the documented Samsung Knox hang) on the startup path for + // no reason. + if (marker != true) { await _secure.write( key: _kKey, value: read, @@ -95,31 +132,56 @@ class CoachConfig extends ChangeNotifier { ); await prefs.setBool(_kKeyPresent, true); } - } else if (expectKey) { - // We know one was saved, so an empty read is the keychain being - // unavailable — NOT the user having no key. Keep whatever is cached - // (usually nothing on a fresh process) and say why. + } else if (trusted) { + // Foreground, so the keychain is readable and an empty answer is the + // truth: there is no key. This is also the ONLY way out of a marker that + // outlived its item — a device-to-device restore carries + // SharedPreferences across but not the keychain payload, and without + // this the app would insist forever that a key it cannot produce is + // still saved. + _key = null; + _keyUnreadable = false; + _keyUndetermined = false; + if (marker != false) await prefs.setBool(_kKeyPresent, false); + } else if (marker == true) { + // Backgrounded and empty: the keychain was unavailable, NOT the user + // having no key. Keep whatever is cached and say why. _keyUnreadable = true; + _keyUndetermined = false; + } else if (marker == null) { + // Nothing recorded either way, and this read proves nothing. Stay + // retry-eligible so a legacy key surfaces on the next foreground. + _key = null; + _keyUndetermined = true; } else { _key = null; _keyUnreadable = false; + _keyUndetermined = false; } } catch (_) { // A read that THREW tells us nothing about the stored key, so it must not - // overwrite one we already hold in memory. - _keyUnreadable = expectKey; + // overwrite one we already hold in memory. iOS surfaces a locked + // `whenUnlocked` item this way rather than as an empty read, so this is + // the legacy-install path, not an edge case. + if (generation != _generation) return; + _keyUnreadable = marker == true; + _keyUndetermined = marker == null; } notifyListeners(); } - /// Re-read the key if it is known to exist but could not be read. Cheap no-op - /// otherwise — safe to call on every resume, which is when a phone that was - /// locked during a background relaunch becomes readable again. - Future refreshKeyIfUnreadable() async { - if (!_keyUnreadable) return; - await load(); + /// Re-read the key when the answer is still outstanding — it is known to + /// exist but was unreadable, or nothing has been established yet. Cheap no-op + /// otherwise, so it is safe on every resume, which is exactly when a phone + /// that was locked during a background relaunch becomes readable again. + Future refreshKeyOnResume() async { + if (!_keyUnreadable && !_keyUndetermined) return; + await load(trusted: true); } + /// Throws if the keychain refuses the write or delete — a caller that reports + /// "saved" on a key that never reached storage is the same silent loss this + /// class exists to stop. Future save({String? baseUrl, String? model, String? apiKey}) async { final prefs = await SharedPreferences.getInstance(); if (baseUrl != null) { @@ -131,12 +193,19 @@ class CoachConfig extends ChangeNotifier { await prefs.setString(_kModel, _model); } if (apiKey != null) { + _generation++; final k = apiKey.trim(); - _key = k.isEmpty ? null : k; - _keyUnreadable = false; + // The keychain FIRST, and the in-memory copy only once it succeeded. The + // other order leaves memory holding a key that was never persisted (lost + // at the next launch, with no marker to even flag it as missing), or + // hiding one that is still stored. if (k.isEmpty) { await _secure.delete(key: _kKey, iOptions: _apple, mOptions: _macos); - await prefs.setBool(_kKeyPresent, false); + // The marker follows the keychain, and its own failure is not worth + // failing the save: a stale `true` costs a retry, never a lost key. + try { + await prefs.setBool(_kKeyPresent, false); + } catch (_) {/* re-established by the next load */} } else { await _secure.write( key: _kKey, @@ -144,10 +213,13 @@ class CoachConfig extends ChangeNotifier { iOptions: _apple, mOptions: _macos, ); - // Written AFTER the keychain succeeds: a marker claiming a key that was - // never stored would leave the app permanently reporting "unreadable". - await prefs.setBool(_kKeyPresent, true); + try { + await prefs.setBool(_kKeyPresent, true); + } catch (_) {/* re-established by the next load */} } + _key = k.isEmpty ? null : k; + _keyUnreadable = false; + _keyUndetermined = false; } notifyListeners(); } diff --git a/lib/import/import_container.dart b/lib/import/import_container.dart index 8ddb69a6..502f4122 100644 --- a/lib/import/import_container.dart +++ b/lib/import/import_container.dart @@ -218,32 +218,46 @@ Future resolveNoopDatabase(String path) async { } if (db == null) return null; // not a backup — let the CSV path try. + if (db.size > _kMaxUncompressedBytes) { + throw ImportFormatException( + '“${p.basename(path)}” unpacks to ' + '${(db.size / (1024 * 1024 * 1024)).toStringAsFixed(1)} GB, which is ' + 'not something we can import.', + ); + } final tempDir = await Directory.systemTemp.createTemp('openstrap_noopbak_'); - final destPath = p.join(tempDir.path, p.basename(db.name)); - final sink = OutputFileStream(destPath); + // Everything past this point owns `tempDir`. An IO failure here is a + // hundreds-of-megabytes partial copy, so nothing may escape without + // deleting it — the caller has no handle to clean up with, because the + // handle is what this function failed to return. try { - db.writeContent(sink); - } finally { - await sink.close(); - } + final destPath = p.join(tempDir.path, p.basename(db.name)); + final sink = OutputFileStream(destPath); + try { + db.writeContent(sink); + } finally { + await sink.close(); + } // A 260 MB backup unpacks to a full second copy, and a phone that runs out // of space mid-write leaves a TRUNCATED file — which still opens as a valid // database and would import a fraction of the history as if that were all // of it. Silent partial history is the worst outcome here, so verify the // whole member landed. (Dart has no portable free-space API, hence checking // after rather than before.) - final written = await File(destPath).length(); - if (db.size > 0 && written < db.size) { - final resolved = ResolvedNoopDatabase(destPath, tempDir); - await resolved.dispose(); - throw ImportFormatException( - 'Unpacking that backup stopped at ' - '${(written / (1024 * 1024)).round()} MB of ' - '${(db.size / (1024 * 1024)).round()} MB — the phone is probably out ' - 'of space. Free some up and try again.', - ); + final written = await File(destPath).length(); + if (db.size > 0 && written < db.size) { + throw ImportFormatException( + 'Unpacking that backup stopped at ' + '${(written / (1024 * 1024)).round()} MB of ' + '${(db.size / (1024 * 1024)).round()} MB — the phone is probably out ' + 'of space. Free some up and try again.', + ); + } + return ResolvedNoopDatabase(destPath, tempDir); + } catch (_) { + await ResolvedNoopDatabase('', tempDir).dispose(); + rethrow; } - return ResolvedNoopDatabase(destPath, tempDir); } finally { await input.close(); } diff --git a/lib/import/noop_backup_import.dart b/lib/import/noop_backup_import.dart index 6b1741aa..3f03c9f8 100644 --- a/lib/import/noop_backup_import.dart +++ b/lib/import/noop_backup_import.dart @@ -50,6 +50,12 @@ import 'noop_ingest.dart'; @visibleForTesting int kNoopBackupPageRows = 20000; +/// Plausible unix seconds. A timestamp outside this range is corrupt or +/// millisecond-scaled, and letting one into the day walk would march it across +/// decades one day at a time. +const int _kMinPlausibleTs = 1000000000; // 2001-09-09 +const int _kMaxPlausibleTs = 4102444800; // 2100-01-01 + class NoopBackupImporter { /// Import an already-extracted `noop-backup.sqlite` at [path]. /// @@ -167,8 +173,8 @@ class NoopBackupImporter { ); } await ingest.finish(); - return NoopImportResult( - ingest.days, ingest.rows, ingest.lateRows, ingest.steps); + return NoopImportResult(ingest.days, ingest.rows, ingest.lateRows, + ingest.steps, ingest.strandedDates); } /// Page one table's rows for the half-open window [from, to) into [onRow]. @@ -194,6 +200,14 @@ class NoopBackupImporter { Future Function(Map row) onRow, ) async { if (!tables.contains(table)) return; + // The COLUMNS are probed too, not just the table. `spo2Sample` is the one + // table documented as never seen non-empty, i.e. the one whose column names + // are least confirmed — and a rename would surface as a raw + // `no such column` SQL error mid-import, after days had already been + // written, with `finish()` never reached to roll the rollups forward. + final have = await _columnNames(src, table); + final usable = [for (final c in cols) if (have.contains(c)) c]; + if (!usable.contains('ts') || usable.length < cols.length) return; var cursor = from - 1; while (true) { final rows = await src.query( @@ -231,7 +245,21 @@ class NoopBackupImporter { await onRow(rows[i]); } if (!full) return; - cursor = end == rows.length ? lastTs : lastTs - 1; + final next = end == rows.length ? lastTs : lastTs - 1; + if (next > cursor) { + cursor = next; + continue; + } + // No progress. Reachable only if `ts` is a REAL column whose values + // truncate onto the same second (a GRDB `Date` is stored that way), where + // holding back the trailing second leaves the cursor exactly where it + // was. Looping would hang the import outright, and re-reading the page + // would append every RR beat in it a second time — so take the whole page + // and step past that second. + for (var i = end; i < rows.length; i++) { + await onRow(rows[i]); + } + cursor = lastTs; } } @@ -248,19 +276,29 @@ class NoopBackupImporter { 'stepSample', ]) { if (!tables.contains(t)) continue; - final r = await src.rawQuery('SELECT MIN(ts) AS lo, MAX(ts) AS hi FROM $t'); + // The plausibility bound is applied INSIDE the aggregate, not to its + // result. MIN/MAX collapse the table to two rows, so a single corrupt + // timestamp — one `ts = 0`, one millisecond-scaled row — would otherwise + // disqualify the entire table and, if every table has one, fail the + // import as "no samples" on a backup holding years of data. + final r = await src.rawQuery( + 'SELECT MIN(ts) AS lo, MAX(ts) AS hi FROM $t WHERE ts >= ? AND ts <= ?', + [_kMinPlausibleTs, _kMaxPlausibleTs], + ); if (r.isEmpty) continue; final a = _int(r.first['lo']), b = _int(r.first['hi']); if (a == null || b == null) continue; - // Guard against a corrupt or millisecond-scaled timestamp column: a value - // outside plausible unix seconds would make the day walk run for years. - if (a < 1000000000 || b > 4102444800) continue; lo = lo == null || a < lo ? a : lo; hi = hi == null || b > hi ? b : hi; } return (lo == null || hi == null) ? null : (lo, hi); } + static Future> _columnNames(Database src, String table) async { + final rows = await src.rawQuery('PRAGMA table_info($table)'); + return {for (final r in rows) (r['name'] as String?) ?? ''}; + } + static Future> _tableNames(Database src) async { final rows = await src .rawQuery("SELECT name FROM sqlite_master WHERE type = 'table'"); diff --git a/lib/import/noop_import.dart b/lib/import/noop_import.dart index 7e7d18c3..6cfe992f 100644 --- a/lib/import/noop_import.dart +++ b/lib/import/noop_import.dart @@ -69,7 +69,15 @@ class NoopImportResult { /// so they are counted here rather than silently dropped — a non-zero value /// means the source was not fully time-ordered. final int lateRows; - NoopImportResult(this.days, this.rows, [this.lateRows = 0, this.steps = 0]); + + /// Dates the source presented out of order, after a later date had already + /// opened. They were used as context for the following day but never derived + /// in their own right, so they are missing from the import — reported so an + /// unordered source is visible rather than quietly short. + final Set strandedDates; + + NoopImportResult(this.days, this.rows, + [this.lateRows = 0, this.steps = 0, this.strandedDates = const {}]); } class NoopImporter { @@ -246,8 +254,8 @@ class NoopImporter { } await ingest.finish(); - return NoopImportResult( - ingest.days, ingest.rows, ingest.lateRows, ingest.steps); + return NoopImportResult(ingest.days, ingest.rows, ingest.lateRows, + ingest.steps, ingest.strandedDates); } /// Documented default column order (used only if the export omits its header). diff --git a/lib/import/noop_ingest.dart b/lib/import/noop_ingest.dart index 908a7ba4..14b25e3c 100644 --- a/lib/import/noop_ingest.dart +++ b/lib/import/noop_ingest.dart @@ -86,6 +86,14 @@ class NoopIngest { /// in, so they are counted rather than silently dropped. int lateRows = 0; + /// Dates that arrived AFTER a later date had already opened, so they were + /// folded in as context but never derived in their own right. Non-empty means + /// the source was not time-ordered and those days are missing from the + /// import — surfaced rather than silently dropped. + final Set _strandedDates = {}; + Set get strandedDates => + Set.unmodifiable(_strandedDates.difference(_derived)); + /// Offer a sample at [ts] (epoch seconds). Returns false when the sample is /// too late to use — the caller must then skip it entirely. Future offer(int ts) async { @@ -96,9 +104,28 @@ class NoopIngest { if (prev != null) { await _deriveAndPrune(prev); _derived.add(prev); + // The retained prior evening is only prior if it is ADJACENT. Across + // a gap — a band left in a drawer, two export ranges stitched + // together — the buffer would otherwise hand the next day a Substrate + // spanning the whole gap, and `calendarDays` walks that span a day at + // a time under a 400-iteration guard: past ~400 days it never reaches + // the target date, `deriveImportedDays` returns 0, and the day is + // silently missing from an import that reported success. + if (!_isDayAfter(prev, date)) { + _secs.clear(); + _rrTs.clear(); + _rrMs.clear(); + } } _curDate = date; + _strandedDates.remove(date); case RowOrder.buffer: + // Older than the high-water date and not yet derived: usable as + // prior-evening context, but this date will never become the high-water + // date itself, so it never derives on its own and its samples are + // pruned once the current date does. Record it — a source that is not + // time-ordered loses whole days here, and the loss used to be silent. + if (date != _curDate) _strandedDates.add(date); break; case RowOrder.late: lateRows++; @@ -111,7 +138,11 @@ class NoopIngest { void hr(int ts, int bpm) => (_secs[ts] ??= _Sec()).hr = bpm; void rr(int ts, double ms) { - if (ms <= 0) return; + // `!(ms > 0)`, NOT `ms <= 0` — those differ for NaN, which fails every + // comparison. A NaN beat propagates into the Substrate and poisons the whole + // day's HRV rather than being one bad interval, and the database path makes + // it reachable: a SQLite REAL column can hold one outright. + if (!(ms > 0)) return; _rrTs.add(ts * 1000.0); _rrMs.add(ms); } @@ -143,7 +174,10 @@ class NoopIngest { /// never derived, and roll the import's day rollups forward. Future finish() async { final last = _curDate; - if (last != null && _secs.isNotEmpty) { + // `_secs` OR the RR buffer: `rr()` is the one channel that creates no + // per-second entry, so a final date carrying only beats would otherwise be + // buffered and then dropped without ever deriving. + if (last != null && (_secs.isNotEmpty || _rrMs.isNotEmpty)) { final st = _stepsByDate.remove(last); if (st != null) steps += await flushStepCoverage(st, last); final sub = _buildSubstrate(_secs, _rrTs, _rrMs); @@ -184,6 +218,16 @@ class NoopIngest { _rrMs.length = w; } + /// True when [next] is the calendar day immediately after [prev]. Built + /// through DateTime so a month, year or DST boundary is handled by the + /// calendar rather than by string arithmetic. + static bool _isDayAfter(String prev, String next) { + final p = DateTime.tryParse(prev), n = DateTime.tryParse(next); + if (p == null || n == null) return false; + final after = DateTime(p.year, p.month, p.day + 1); + return after.year == n.year && after.month == n.month && after.day == n.day; + } + /// String date compare 'YYYY-MM-DD' — true when [a] is strictly after [b]. static bool _after(String a, String b) => a.compareTo(b) > 0; @@ -228,11 +272,12 @@ class NoopIngest { final s = secs[t]!; tsSec[i] = t; hr[i] = s.hr ?? 0; - if (s.ax != null) { - fax = s.ax!; - fay = s.ay ?? fay; - faz = s.az ?? faz; - } + // Each axis carries on its own. Gating y and z behind x meant a row that + // reported only y or z left all three on the previous carry — silently + // wrong rather than merely incomplete. + if (s.ax != null) fax = s.ax!; + if (s.ay != null) fay = s.ay!; + if (s.az != null) faz = s.az!; ax[i] = fax; ay[i] = fay; az[i] = faz; diff --git a/lib/state/app_state.dart b/lib/state/app_state.dart index 0fc19149..0d4f7f04 100644 --- a/lib/state/app_state.dart +++ b/lib/state/app_state.dart @@ -350,10 +350,17 @@ class AppState extends ChangeNotifier { _derive, onProgress: onProgress, ); + lastNoopImport = res; notifyListeners(); return res.days; } + /// The most recent NOOP import, so the import screen can report what the + /// source cost us — days it presented out of order were folded in as context + /// for the following day but never derived, and "imported N days" alone would + /// hide that. + NoopImportResult? lastNoopImport; + /// WHOOP export CSV(s) → derived-snapshot days (+ workouts). BETA. Future importWhoopCsvs( List paths, { diff --git a/lib/ui/coach/ai_coach_screen.dart b/lib/ui/coach/ai_coach_screen.dart index 8a5aeb0f..b6194d40 100644 --- a/lib/ui/coach/ai_coach_screen.dart +++ b/lib/ui/coach/ai_coach_screen.dart @@ -312,7 +312,7 @@ class _AiCoachScreenState extends State { 'Retry, or reopen the app once the phone has been unlocked.', actionLabel: 'Retry', onAction: () async { - await cfg.refreshKeyIfUnreadable(); + await cfg.refreshKeyOnResume(); if (mounted) setState(() {}); }, ).dsEnter(), diff --git a/lib/ui/coach/coach_settings_screen.dart b/lib/ui/coach/coach_settings_screen.dart index 2b5dba09..24d0e12f 100644 --- a/lib/ui/coach/coach_settings_screen.dart +++ b/lib/ui/coach/coach_settings_screen.dart @@ -82,10 +82,34 @@ class _CoachSettingsScreenState extends State { // completed — being `mounted` doesn't guarantee a poppable route. final messenger = ScaffoldMessenger.of(context); final navigator = Navigator.of(context); - await cfg.save(baseUrl: _base.text, apiKey: _key.text, model: _chosen); + // An EMPTY field means "delete my key" only when we could actually show the + // user what they are deleting. When the stored key could not be read — a + // locked keychain, or the startup read still in flight — the field seeds + // empty through no fault of the user, and saving would delete a key they + // never touched and cannot see. Send null instead: leave it exactly as it + // is. A user who wants it gone can clear it once it has loaded. + final typed = _key.text.trim(); + final blindClear = typed.isEmpty && cfg.apiKey == null; + try { + await cfg.save( + baseUrl: _base.text, + apiKey: blindClear ? null : _key.text, + model: _chosen, + ); + } catch (e) { + // The keychain refused it. Saying "saved" here is the silent loss this + // whole path exists to prevent. + if (!mounted) return; + setState(() => _msg = 'Could not save your key to the keychain: $e'); + return; + } if (!mounted) return; messenger.showSnackBar( - const SnackBar(content: Text('AI Coach settings saved.')), + SnackBar( + content: Text(blindClear + ? 'Settings saved. Your existing key was left untouched.' + : 'AI Coach settings saved.'), + ), ); if (navigator.canPop()) navigator.pop(); } diff --git a/lib/ui/import/import_screen.dart b/lib/ui/import/import_screen.dart index 294770e1..818f47fa 100644 --- a/lib/ui/import/import_screen.dart +++ b/lib/ui/import/import_screen.dart @@ -112,6 +112,14 @@ class _ImportScreenState extends State { if (paths.isEmpty) return; await _run('NOOP', () => app.importNoopCsv(paths.first, onProgress: (d) => _set(() => _progress = 'Re-deriving day $d…'))); + final stranded = app.lastNoopImport?.strandedDates ?? const {}; + if (stranded.isNotEmpty && _error == null) { + final shown = (stranded.toList()..sort()).take(3).join(', '); + _set(() => _result = '${_result ?? ''} ${stranded.length} day' + '${stranded.length == 1 ? '' : 's'} came through out of order and ' + 'could not be re-analysed ($shown${stranded.length > 3 ? '…' : ''}).' + .trimLeft()); + } } Future _importEdge() async { diff --git a/test/coach_config_key_test.dart b/test/coach_config_key_test.dart index 4cbba8a7..11933ae8 100644 --- a/test/coach_config_key_test.dart +++ b/test/coach_config_key_test.dart @@ -7,6 +7,8 @@ // while the phone is locked, when that item cannot be read. `load()` cached the // empty read as "no key", so the app asked the user to set one up again. +import 'dart:async'; + import 'package:flutter/foundation.dart'; import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; @@ -20,17 +22,33 @@ class _FakeKeychain { final List> writeOptions = []; bool locked = false; bool throwOnRead = false; + bool throwOnWrite = false; + bool hangReads = false; + final List> _hung = []; + + void releaseHung() { + for (final c in _hung) { + if (!c.isCompleted) c.complete(); + } + _hung.clear(); + } Future handle(MethodCall call) async { final args = (call.arguments as Map?) ?? const {}; switch (call.method) { case 'read': if (throwOnRead) throw PlatformException(code: 'keychain'); + if (hangReads) { + final c = Completer(); + _hung.add(c); + await c.future; + } // A locked keychain does not error — it simply returns nothing, which // is indistinguishable from "no key" without the marker. if (locked) return null; return items[args['key'] as String]; case 'write': + if (throwOnWrite) throw PlatformException(code: 'keychain'); items[args['key'] as String] = args['value'] as String; writeOptions.add((args['options'] as Map?) ?? const {}); return null; @@ -97,7 +115,7 @@ void main() { // The user unlocks and opens the app. keychain.locked = false; - await relaunched.refreshKeyIfUnreadable(); + await relaunched.refreshKeyOnResume(); expect(relaunched.apiKey, 'sk-test'); expect(relaunched.keyUnreadable, isFalse); }); @@ -154,4 +172,98 @@ void main() { await cfg.load(); expect(keychain.writeOptions.length, 1); }); + + test('a legacy key with no marker survives a LOCKED relaunch', () async { + // The population this whole fix exists for: a key saved by an older build, + // so there is no marker beside it, whose first launch on this build is a + // background relaunch on a locked phone. Concluding "no key" here and never + // retrying is exactly the old bug. + keychain.items['coach_api_key'] = 'sk-legacy'; + keychain.locked = true; + + final cfg = CoachConfig(); + await cfg.load(); + expect(cfg.hasKey, isFalse); + expect(cfg.keyUndetermined, isTrue, + reason: 'nothing is established yet, so the retry must stay armed'); + + keychain.locked = false; + await cfg.refreshKeyOnResume(); + expect(cfg.apiKey, 'sk-legacy'); + }); + + test('a legacy key whose read THROWS while locked still retries', () async { + // iOS reports a locked whenUnlocked item as an error rather than an empty + // read, which is the legacy item's actual behaviour before it is upgraded. + keychain.items['coach_api_key'] = 'sk-legacy'; + keychain.throwOnRead = true; + + final cfg = CoachConfig(); + await cfg.load(); + expect(cfg.keyUndetermined, isTrue); + + keychain.throwOnRead = false; + await cfg.refreshKeyOnResume(); + expect(cfg.apiKey, 'sk-legacy'); + }); + + test('a foreground read settles "no key" so the retry stops', () async { + final cfg = CoachConfig(); + await cfg.load(trusted: true); + expect(cfg.keyUndetermined, isFalse); + expect(cfg.hasKey, isFalse); + }); + + test('a marker outliving its item is cleared by a foreground read', () async { + // A device-to-device restore carries SharedPreferences across but not the + // keychain payload. Without this the app insists forever that a key it + // cannot produce is still saved, and Retry is the only thing on offer. + final cfg = CoachConfig(); + await cfg.save(apiKey: 'sk-test'); + keychain.items.clear(); // restored onto a new device + + await cfg.load(trusted: true); + expect(cfg.keyUnreadable, isFalse); + expect(cfg.hasKey, isFalse, reason: 'it really is gone — offer setup'); + }); + + test('a hung read leaves the retry armed rather than "no key"', () async { + final saved = CoachConfig(); + await saved.save(apiKey: 'sk-test'); + + // A read that never returns (the Samsung Knox keystore hang the startup + // path is wrapped in a timeout for — a timeout that cannot cancel the call). + keychain.hangReads = true; + final relaunched = CoachConfig(); + unawaited(relaunched.load()); + await Future.delayed(const Duration(milliseconds: 20)); + + expect(relaunched.keyUnreadable, isTrue, + reason: 'the state must be pending BEFORE the read, not after it'); + }); + + test('a save during an in-flight load is not clobbered by it', () async { + keychain.hangReads = true; + final cfg = CoachConfig(); + unawaited(cfg.load()); + await Future.delayed(const Duration(milliseconds: 10)); + + keychain.hangReads = false; + await cfg.save(apiKey: 'sk-new', model: 'gpt-4o'); + expect(cfg.apiKey, 'sk-new'); + + // The stale read finally lands, having started before the save. + keychain.releaseHung(); + await Future.delayed(const Duration(milliseconds: 20)); + expect(cfg.apiKey, 'sk-new', + reason: 'a read that predates the save must not apply its result'); + }); + + test('a keychain that refuses the write does not report success', () async { + final cfg = CoachConfig(); + keychain.throwOnWrite = true; + await expectLater(cfg.save(apiKey: 'sk-test'), throwsA(anything)); + expect(cfg.hasKey, isFalse, + reason: 'memory must not hold a key that was never persisted'); + }); } diff --git a/test/noop_backup_import_test.dart b/test/noop_backup_import_test.dart index fb118f45..8b0714dc 100644 --- a/test/noop_backup_import_test.dart +++ b/test/noop_backup_import_test.dart @@ -185,16 +185,21 @@ void main() { final dbPath = await writeNoopDb('c.sqlite', t0: t0, seconds: 120); final bak = writeBackup('c.noopbak', dbPath); - final before = tmp.listSync().whereType().length; + // The extraction directory lives under systemTemp, which is machine-global + // — so compare against a snapshot rather than asserting it is empty, or an + // unrelated crashed run fails this test. + Set extractions() => Directory.systemTemp + .listSync() + .whereType() + .map((d) => p.basename(d.path)) + .where((n) => n.startsWith('openstrap_noopbak_')) + .toSet(); + + final before = extractions(); await NoopImporter.importFile(bak, const Profile(), DerivationEngine()); // Nothing extracted survives — a 260 MB backup would otherwise leave a full // second copy behind on the phone. - final leftovers = Directory.systemTemp - .listSync() - .whereType() - .where((d) => p.basename(d.path).startsWith('openstrap_noopbak_')); - expect(leftovers, isEmpty); - expect(tmp.listSync().whereType().length, before); + expect(extractions().difference(before), isEmpty); }, timeout: const Timeout(Duration(minutes: 5))); test('no RR beat is lost to a page boundary inside a second', () async { @@ -235,6 +240,43 @@ void main() { reason: 'every HR sample and every RR beat was read'); }, timeout: const Timeout(Duration(minutes: 5))); + test('a NaN interval never reaches the substrate', () async { + // A REAL column can hold NaN, and NaN fails every comparison — so a + // `ms <= 0` guard passes it through. One NaN beat poisons the day's whole + // HRV computation rather than costing one interval. + const t0 = 1785919200; // 2026-08-05 + final path = p.join(tmp.path, 'nan.sqlite'); + if (File(path).existsSync()) File(path).deleteSync(); + final src = await databaseFactory.openDatabase(path); + await src.execute('CREATE TABLE hrSample (deviceId TEXT, ts INTEGER, ' + 'bpm INTEGER, PRIMARY KEY (deviceId, ts))'); + await src.execute('CREATE TABLE rrInterval (deviceId TEXT, ts INTEGER, ' + 'rrMs REAL, PRIMARY KEY (deviceId, ts, rrMs))'); + final b = src.batch(); + for (var i = 0; i < 120; i++) { + b.insert('hrSample', {'deviceId': 'd', 'ts': t0 + i, 'bpm': 65}); + b.insert('rrInterval', { + 'deviceId': 'd', + 'ts': t0 + i, + 'rrMs': i == 60 ? double.nan : 900.0, + }); + } + await b.commit(noResult: true); + await src.close(); + final bak = writeBackup('nan.noopbak', path); + + final res = await NoopImporter.importFile( + bak, const Profile(), DerivationEngine()); + expect(res.days, greaterThan(0), reason: 'the day still imports'); + + // Whatever the day computed, it must not be NaN-poisoned. + final db = await LocalDb.instance; + final rows = await db.query('day_result'); + for (final r in rows) { + expect(r.values.whereType().where((v) => v.isNaN), isEmpty); + } + }, timeout: const Timeout(Duration(minutes: 5))); + test('a database that is not a NOOP backup is named, not silently empty', () async { final path = p.join(tmp.path, 'other.sqlite'); @@ -250,6 +292,118 @@ void main() { ); }); + test('one corrupt timestamp does not disqualify the whole table', () async { + // MIN/MAX collapse a table to two rows, so a single `ts = 0` used to drop + // that table from the span entirely — and if every table has one, a backup + // holding years of data imports as "no samples". + const t0 = 1786003200; // 2026-08-06 + final path = p.join(tmp.path, 'corrupt.sqlite'); + if (File(path).existsSync()) File(path).deleteSync(); + final src = await databaseFactory.openDatabase(path); + await src.execute('CREATE TABLE hrSample (deviceId TEXT, ts INTEGER, ' + 'bpm INTEGER, PRIMARY KEY (deviceId, ts))'); + final b = src.batch(); + b.insert('hrSample', {'deviceId': 'd', 'ts': 0, 'bpm': 60}); // corrupt + for (var i = 0; i < 300; i++) { + b.insert('hrSample', {'deviceId': 'd', 'ts': t0 + i, 'bpm': 62}); + } + await b.commit(noResult: true); + await src.close(); + final bak = writeBackup('corrupt.noopbak', path); + + final res = await NoopImporter.importFile( + bak, const Profile(), DerivationEngine()); + expect(res.days, greaterThan(0)); + expect(res.rows, 300, reason: 'the good rows import, the corrupt one does not'); + }, timeout: const Timeout(Duration(minutes: 5))); + + test('a long gap between blocks still derives the later day', () async { + // The prior-evening buffer used to be retained across ANY gap, handing the + // next day a Substrate spanning the whole thing. `calendarDays` walks that + // span a day at a time under a 400-iteration guard, so past ~400 days it + // never reaches the target date: the day is missing and the import still + // reports success. + const first = 1690000000; // 2023-07 + const later = first + 500 * 86400; + final path = p.join(tmp.path, 'gap.sqlite'); + if (File(path).existsSync()) File(path).deleteSync(); + final src = await databaseFactory.openDatabase(path); + await src.execute('CREATE TABLE hrSample (deviceId TEXT, ts INTEGER, ' + 'bpm INTEGER, PRIMARY KEY (deviceId, ts))'); + final b = src.batch(); + for (final t0 in const [first, later]) { + for (var i = 0; i < 3600; i++) { + b.insert('hrSample', {'deviceId': 'd', 'ts': t0 + i, 'bpm': 62}); + } + } + await b.commit(noResult: true); + await src.close(); + final bak = writeBackup('gap.noopbak', path); + + final res = await NoopImporter.importFile( + bak, const Profile(), DerivationEngine()); + expect(res.days, 2, reason: 'both blocks derive, 500 days apart'); + }, timeout: const Timeout(Duration(minutes: 10))); + + test('a REAL timestamp column cannot hang the paging', () async { + // A GRDB `Date` is stored as a REAL. Sub-second values truncate onto the + // same second, which left the page cursor exactly where it was — an + // infinite loop, and every RR beat in the page re-appended on each pass. + const t0 = 1786089600; // 2026-08-07 + final path = p.join(tmp.path, 'real.sqlite'); + if (File(path).existsSync()) File(path).deleteSync(); + final src = await databaseFactory.openDatabase(path); + await src.execute('CREATE TABLE hrSample (deviceId TEXT, ts REAL, ' + 'bpm INTEGER, PRIMARY KEY (deviceId, ts))'); + final b = src.batch(); + var n = 0; + for (var i = 0; i < 40; i++) { + // Several fractional samples inside each second. + for (final frac in const [0.2, 0.4, 0.6, 0.8]) { + b.insert('hrSample', {'deviceId': 'd', 'ts': t0 + i + frac, 'bpm': 60}); + n++; + } + } + await b.commit(noResult: true); + await src.close(); + final bak = writeBackup('real.noopbak', path); + + final saved = kNoopBackupPageRows; + kNoopBackupPageRows = 4; + addTearDown(() => kNoopBackupPageRows = saved); + + final res = await NoopImporter.importFile( + bak, const Profile(), DerivationEngine()); + expect(res.rows, lessThanOrEqualTo(n), + reason: 'rows are never emitted twice'); + }, timeout: const Timeout(Duration(minutes: 2))); + + test('a renamed column is a message, not a raw SQL error mid-import', + () async { + const t0 = 1786176000; // 2026-08-08 + final path = p.join(tmp.path, 'renamed.sqlite'); + if (File(path).existsSync()) File(path).deleteSync(); + final src = await databaseFactory.openDatabase(path); + await src.execute('CREATE TABLE hrSample (deviceId TEXT, ts INTEGER, ' + 'bpm INTEGER, PRIMARY KEY (deviceId, ts))'); + // spo2Sample exists but has drifted — the table this code documents as + // never observed non-empty, i.e. the one least confirmed. + await src.execute('CREATE TABLE spo2Sample (deviceId TEXT, ts INTEGER, ' + 'redRaw INTEGER, irRaw INTEGER, PRIMARY KEY (deviceId, ts))'); + final b = src.batch(); + for (var i = 0; i < 300; i++) { + b.insert('hrSample', {'deviceId': 'd', 'ts': t0 + i, 'bpm': 61}); + } + await b.commit(noResult: true); + await src.close(); + final bak = writeBackup('renamed.noopbak', path); + + final res = await NoopImporter.importFile( + bak, const Profile(), DerivationEngine()); + expect(res.days, greaterThan(0), + reason: 'the drifted table is skipped, the rest still imports'); + }, timeout: const Timeout(Duration(minutes: 5))); + test('a backup with no samples says so rather than importing 0 days', () async { final path = p.join(tmp.path, 'empty.sqlite'); From 7c214cfbc5b21a234d5168506341b9577dafa36f Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 21:54:23 +0530 Subject: [PATCH 3/6] review round two: page on the raw timestamp, warn like a warning keying the page cursor on a truncated second looked equivalent to keying it on the column's own value and is not: against a REAL column `ts > 3.0` does not exclude `3.2`, so my guard against the resulting loop stopped the read after two pages and dropped the rest of the day. the cursor is the last row actually emitted now, and a timestamp that fills a whole page is drained rather than stepped past. infinity poisons a day's HRV exactly as NaN does, and only NaN was rejected. the out-of-order warning rendered inside the green success card, next to a tick. the column probe ran once per table per day when the schema cannot change at all. --- lib/import/import_container.dart | 21 +++- lib/import/noop_backup_import.dart | 142 +++++++++++++++--------- lib/import/noop_ingest.dart | 11 +- lib/ui/coach/coach_settings_screen.dart | 4 +- lib/ui/import/import_screen.dart | 27 ++++- test/noop_backup_import_test.dart | 22 ++-- test/noop_schema_drift_test.dart | 31 ++++++ 7 files changed, 185 insertions(+), 73 deletions(-) diff --git a/lib/import/import_container.dart b/lib/import/import_container.dart index 502f4122..de3de93d 100644 --- a/lib/import/import_container.dart +++ b/lib/import/import_container.dart @@ -244,6 +244,11 @@ Future resolveNoopDatabase(String path) async { // of it. Silent partial history is the worst outcome here, so verify the // whole member landed. (Dart has no portable free-space API, hence checking // after rather than before.) + // The size is checked in BOTH directions. Short means the write ran out + // of space, and a truncated database still opens — importing a fraction + // of someone's history as if it were all of it. Long means the archive's + // declared size is not what it actually holds, which is the only thing + // the ceiling above was checked against, so the ceiling did not hold. final written = await File(destPath).length(); if (db.size > 0 && written < db.size) { throw ImportFormatException( @@ -253,9 +258,23 @@ Future resolveNoopDatabase(String path) async { 'of space. Free some up and try again.', ); } + if (db.size > 0 && written > db.size) { + throw ImportFormatException( + '“${p.basename(path)}” does not hold what it says it does — its ' + 'database unpacked to more than the archive declared. It is likely ' + 'damaged; export it again.', + ); + } return ResolvedNoopDatabase(destPath, tempDir); } catch (_) { - await ResolvedNoopDatabase('', tempDir).dispose(); + // Delete the partial copy directly, and never let a cleanup failure + // replace the real exception — the out-of-space message above is the one + // the user can act on. + try { + if (tempDir.existsSync()) await tempDir.delete(recursive: true); + } catch (_) { + /* the OS reclaims the temp dir eventually */ + } rethrow; } } finally { diff --git a/lib/import/noop_backup_import.dart b/lib/import/noop_backup_import.dart index 3f03c9f8..01e0ac98 100644 --- a/lib/import/noop_backup_import.dart +++ b/lib/import/noop_backup_import.dart @@ -110,6 +110,26 @@ class NoopBackupImporter { } final (minTs, maxTs) = span; + // Probe every table's columns ONCE. The schema cannot change during a + // read-only import, and doing it inside the day walk cost a + // `PRAGMA table_info` per table per day — thousands of pointless queries on + // a long backup. A table whose columns have drifted resolves to a short + // list here and is skipped by `_read`, rather than failing mid-import with + // a raw `no such column` after days have already been written. + final columns = >{}; + for (final e in const { + 'hrSample': ['ts', 'bpm'], + 'rrInterval': ['ts', 'rrMs'], + 'gravitySample': ['ts', 'x', 'y', 'z'], + 'skinTempSample': ['ts', 'raw'], + 'spo2Sample': ['ts', 'red', 'ir'], + 'stepSample': ['ts', 'counter'], + }.entries) { + if (!tables.contains(e.key)) continue; + final have = await _columnNames(src, e.key); + columns[e.key] = [for (final c in e.value) if (have.contains(c)) c]; + } + final ingest = NoopIngest(profile, engine, onProgress: onProgress); // Walk LOCAL days. Built through the DateTime(y, m, d + 1) constructor @@ -124,18 +144,18 @@ class NoopBackupImporter { // Order within a day does not matter — [NoopIngest] rebuilds the Substrate // sorted by timestamp — but the DAYS must arrive in ascending order, since // the high-water date is what closes out and derives the previous one. - await _read(src, tables, 'hrSample', ['ts', 'bpm'], from, to, (r) async { + await _read(src, tables, columns, 'hrSample', ['ts', 'bpm'], from, to, (r) async { final ts = _int(r['ts']), v = _int(r['bpm']); if (ts == null || v == null) return; if (await ingest.offer(ts)) ingest.hr(ts, v); }); - await _read(src, tables, 'rrInterval', ['ts', 'rrMs'], from, to, + await _read(src, tables, columns, 'rrInterval', ['ts', 'rrMs'], from, to, (r) async { final ts = _int(r['ts']), v = _num(r['rrMs']); if (ts == null || v == null) return; if (await ingest.offer(ts)) ingest.rr(ts, v); }); - await _read(src, tables, 'gravitySample', ['ts', 'x', 'y', 'z'], from, to, + await _read(src, tables, columns, 'gravitySample', ['ts', 'x', 'y', 'z'], from, to, (r) async { final ts = _int(r['ts']); if (ts == null) return; @@ -143,13 +163,13 @@ class NoopBackupImporter { ingest.gravity(ts, _num(r['x']), _num(r['y']), _num(r['z'])); } }); - await _read(src, tables, 'skinTempSample', ['ts', 'raw'], from, to, + await _read(src, tables, columns, 'skinTempSample', ['ts', 'raw'], from, to, (r) async { final ts = _int(r['ts']); if (ts == null) return; if (await ingest.offer(ts)) ingest.skinTemp(ts, _int(r['raw'])); }); - await _read(src, tables, 'spo2Sample', ['ts', 'red', 'ir'], from, to, + await _read(src, tables, columns, 'spo2Sample', ['ts', 'red', 'ir'], from, to, (r) async { final ts = _int(r['ts']); if (ts == null) return; @@ -157,7 +177,7 @@ class NoopBackupImporter { ingest.spo2(ts, _int(r['red']), _int(r['ir'])); } }); - await _read(src, tables, 'stepSample', ['ts', 'counter'], from, to, + await _read(src, tables, columns, 'stepSample', ['ts', 'counter'], from, to, (r) async { final ts = _int(r['ts']), v = _int(r['counter']); if (ts == null || v == null) return; @@ -178,21 +198,24 @@ class NoopBackupImporter { } /// Page one table's rows for the half-open window [from, to) into [onRow]. - /// A table the backup does not have is skipped — `spo2Sample` and `respSample` - /// are already absent-in-practice (present but empty), and a future NOOP - /// schema is free to drop either outright. + /// A table the backup does not have — or one whose columns have drifted — is + /// skipped rather than failing the import mid-way. /// /// Paged by KEYSET (`ts > last`), not OFFSET: LIMIT/OFFSET re-walks and - /// re-discards every skipped row, so paging a 86,400-row day turns quadratic. + /// re-discards every skipped row, so paging an 86,400-row day turns quadratic. /// - /// `ts` is NOT unique in every table — `rrInterval`'s key is (deviceId, ts, - /// rrMs), so one second can hold several beats, and two devices can share a - /// second in any table. A page boundary landing inside such a second would - /// drop its remaining rows silently, so a FULL page discards its trailing - /// partial second and re-reads it from the top of the next page. + /// The cursor is the RAW column value, never a truncated second. `ts` is not + /// unique — `rrInterval`'s key is (deviceId, ts, rrMs), so one second holds + /// several beats — so a page boundary can land inside a timestamp, and the + /// trailing rows sharing it are held back and re-read whole on the next page. + /// Keying that cursor on `ts` truncated to an int looks equivalent and is not: + /// against a REAL column (a GRDB `Date` is stored as one) `ts > 3.0` does not + /// exclude `3.2`, so the same rows come back forever — or, with a guard + /// against that, the read stops early and silently drops the rest of the day. static Future _read( Database src, Set tables, + Map> columns, String table, List cols, int from, @@ -200,15 +223,10 @@ class NoopBackupImporter { Future Function(Map row) onRow, ) async { if (!tables.contains(table)) return; - // The COLUMNS are probed too, not just the table. `spo2Sample` is the one - // table documented as never seen non-empty, i.e. the one whose column names - // are least confirmed — and a rename would surface as a raw - // `no such column` SQL error mid-import, after days had already been - // written, with `finish()` never reached to roll the rollups forward. - final have = await _columnNames(src, table); - final usable = [for (final c in cols) if (have.contains(c)) c]; - if (!usable.contains('ts') || usable.length < cols.length) return; - var cursor = from - 1; + final usable = columns[table] ?? const []; + if (usable.length < cols.length) return; + + num cursor = from - 1; while (true) { final rows = await src.query( table, @@ -220,46 +238,58 @@ class NoopBackupImporter { ); if (rows.isEmpty) return; final full = rows.length == kNoopBackupPageRows; - final lastTs = _int(rows.last['ts']); - // A NULL or non-integer ts in the last row of a page would leave the - // cursor unmoved and loop forever; take the page and stop instead. - if (lastTs == null || lastTs <= cursor) { - for (final r in rows) { - await onRow(r); - } - return; - } - // On a full page, hold back the trailing rows that share the last - // timestamp — the next page re-reads that second whole. + // Hold back the trailing rows that share the last timestamp EXACTLY; the + // next page re-reads that timestamp whole. var end = rows.length; if (full) { - while (end > 0 && _int(rows[end - 1]['ts']) == lastTs) { + final lastTs = _raw(rows.last['ts']); + while (end > 0 && _raw(rows[end - 1]['ts']) == lastTs) { end--; } - // A whole page of one timestamp: nothing can be held back without - // stalling, so take it and step past that second. - if (end == 0) end = rows.length; - } - for (var i = 0; i < end; i++) { - await onRow(rows[i]); } - if (!full) return; - final next = end == rows.length ? lastTs : lastTs - 1; - if (next > cursor) { - cursor = next; + // A whole page sharing one timestamp cannot hold anything back without + // stalling. Take it, then drain whatever else carries that exact value + // before stepping past it — stepping past directly would silently lose + // the remainder, which is the partial-history failure this file refuses + // everywhere else. Offset-paged, but bounded to one timestamp's rows. + if (end == 0) { + final lastTs = _raw(rows.last['ts']); + for (final r in rows) { + await onRow(r); + } + var drained = rows.length; + while (lastTs != null) { + final more = await src.query( + table, + columns: cols, + where: 'ts = ?', + whereArgs: [lastTs], + orderBy: 'ts', + limit: kNoopBackupPageRows, + offset: drained, + ); + if (more.isEmpty) break; + for (final r in more) { + await onRow(r); + } + drained += more.length; + if (more.length < kNoopBackupPageRows) break; + } + if (lastTs == null) return; // a NULL ts cannot be paged past + cursor = lastTs; continue; } - // No progress. Reachable only if `ts` is a REAL column whose values - // truncate onto the same second (a GRDB `Date` is stored that way), where - // holding back the trailing second leaves the cursor exactly where it - // was. Looping would hang the import outright, and re-reading the page - // would append every RR beat in it a second time — so take the whole page - // and step past that second. - for (var i = end; i < rows.length; i++) { + + for (var i = 0; i < end; i++) { await onRow(rows[i]); } - cursor = lastTs; + if (!full) return; + // The cursor is the last row actually EMITTED, so nothing is skipped and + // the next page starts strictly after it. + final next = _raw(rows[end - 1]['ts']); + if (next == null || next <= cursor) return; // non-numeric ts: stop + cursor = next; } } @@ -317,4 +347,8 @@ class NoopBackupImporter { : null; static double? _num(Object? v) => v is num ? v.toDouble() : null; + + /// The timestamp column's value as stored — INTEGER in every NOOP schema seen + /// so far, but REAL is representable and must page correctly either way. + static num? _raw(Object? v) => v is num ? v : null; } diff --git a/lib/import/noop_ingest.dart b/lib/import/noop_ingest.dart index 14b25e3c..fc234f53 100644 --- a/lib/import/noop_ingest.dart +++ b/lib/import/noop_ingest.dart @@ -138,11 +138,12 @@ class NoopIngest { void hr(int ts, int bpm) => (_secs[ts] ??= _Sec()).hr = bpm; void rr(int ts, double ms) { - // `!(ms > 0)`, NOT `ms <= 0` — those differ for NaN, which fails every - // comparison. A NaN beat propagates into the Substrate and poisons the whole - // day's HRV rather than being one bad interval, and the database path makes - // it reachable: a SQLite REAL column can hold one outright. - if (!(ms > 0)) return; + // FINITE and positive. `ms <= 0` alone lets NaN through (it fails every + // comparison) and `!(ms > 0)` alone lets infinity through; either one + // propagates into the Substrate and poisons the whole day's HRV rather than + // costing a single interval. Both are reachable: a CSV column can spell + // `NaN`, and a SQLite REAL can hold either outright. + if (!ms.isFinite || ms <= 0) return; _rrTs.add(ts * 1000.0); _rrMs.add(ms); } diff --git a/lib/ui/coach/coach_settings_screen.dart b/lib/ui/coach/coach_settings_screen.dart index 24d0e12f..682a4da1 100644 --- a/lib/ui/coach/coach_settings_screen.dart +++ b/lib/ui/coach/coach_settings_screen.dart @@ -106,8 +106,10 @@ class _CoachSettingsScreenState extends State { if (!mounted) return; messenger.showSnackBar( SnackBar( + // Neutral on purpose: `blindClear` is also true when there is no key at + // all, and claiming an existing one was preserved would be a fiction. content: Text(blindClear - ? 'Settings saved. Your existing key was left untouched.' + ? 'Settings saved. Your API key was not changed.' : 'AI Coach settings saved.'), ), ); diff --git a/lib/ui/import/import_screen.dart b/lib/ui/import/import_screen.dart index 818f47fa..ab4601aa 100644 --- a/lib/ui/import/import_screen.dart +++ b/lib/ui/import/import_screen.dart @@ -30,6 +30,12 @@ class _ImportScreenState extends State { bool _picking = false; String? _progress; String? _result; + + /// A partial success — the import worked but cost something the user needs to + /// know about. Distinct from [_result] and [_error] because it is neither: a + /// green tick beside "these days could not be re-analysed" reads as approval + /// of the loss, and an error card would claim the whole import failed. + String? _warning; String? _error; /// The option cards are inert while EITHER a picker is open or an import is @@ -88,6 +94,7 @@ class _ImportScreenState extends State { _busy = true; _progress = 'Importing…'; _result = null; + _warning = null; _error = null; }); try { @@ -115,10 +122,9 @@ class _ImportScreenState extends State { final stranded = app.lastNoopImport?.strandedDates ?? const {}; if (stranded.isNotEmpty && _error == null) { final shown = (stranded.toList()..sort()).take(3).join(', '); - _set(() => _result = '${_result ?? ''} ${stranded.length} day' + _set(() => _warning = '${stranded.length} day' '${stranded.length == 1 ? '' : 's'} came through out of order and ' - 'could not be re-analysed ($shown${stranded.length > 3 ? '…' : ''}).' - .trimLeft()); + 'could not be re-analysed ($shown${stranded.length > 3 ? '…' : ''}).'); } } @@ -219,6 +225,21 @@ class _ImportScreenState extends State { Text(_progress ?? 'Importing…', style: AppText.bodySoft)), ]), ), + if (_warning != null) ...[ + SurfaceCard( + level: 0, + color: AppColors.warnSoft, + padding: const EdgeInsets.all(Sp.x4), + child: Row(children: [ + AppIcon(OsIcon.info, size: 18, color: AppColors.warn), + const SizedBox(width: Sp.x3), + Expanded( + child: Text(_warning!, + style: AppText.body.copyWith(color: AppColors.warn))), + ]), + ).dsPop(), + const SizedBox(height: Sp.x3), + ], if (_result != null) ...[ SurfaceCard( level: 0, diff --git a/test/noop_backup_import_test.dart b/test/noop_backup_import_test.dart index 8b0714dc..24004fb5 100644 --- a/test/noop_backup_import_test.dart +++ b/test/noop_backup_import_test.dart @@ -240,10 +240,12 @@ void main() { reason: 'every HR sample and every RR beat was read'); }, timeout: const Timeout(Duration(minutes: 5))); - test('a NaN interval never reaches the substrate', () async { - // A REAL column can hold NaN, and NaN fails every comparison — so a - // `ms <= 0` guard passes it through. One NaN beat poisons the day's whole - // HRV computation rather than costing one interval. + test('a non-finite interval from the database is not a beat', () async { + // sqflite returns NULL for a NaN REAL, so `_num` already drops it before + // the guard — but INFINITY comes back as a real double and reaches `rr()`. + // (The NaN path proper is exercised through the CSV importer, where + // `double.tryParse('NaN')` genuinely produces one — see + // noop_schema_drift_test.dart.) const t0 = 1785919200; // 2026-08-05 final path = p.join(tmp.path, 'nan.sqlite'); if (File(path).existsSync()) File(path).deleteSync(); @@ -258,7 +260,7 @@ void main() { b.insert('rrInterval', { 'deviceId': 'd', 'ts': t0 + i, - 'rrMs': i == 60 ? double.nan : 900.0, + 'rrMs': i == 60 ? double.infinity : 900.0, }); } await b.commit(noResult: true); @@ -269,11 +271,10 @@ void main() { bak, const Profile(), DerivationEngine()); expect(res.days, greaterThan(0), reason: 'the day still imports'); - // Whatever the day computed, it must not be NaN-poisoned. final db = await LocalDb.instance; final rows = await db.query('day_result'); for (final r in rows) { - expect(r.values.whereType().where((v) => v.isNaN), isEmpty); + expect(r.values.whereType().where((v) => !v.isFinite), isEmpty); } }, timeout: const Timeout(Duration(minutes: 5))); @@ -374,8 +375,11 @@ void main() { final res = await NoopImporter.importFile( bak, const Profile(), DerivationEngine()); - expect(res.rows, lessThanOrEqualTo(n), - reason: 'rows are never emitted twice'); + // EXACT: `lessThanOrEqualTo` would pass on an importer that read one row. + // Duplication and loss are both real failure modes here — the fallback path + // drains a whole second rather than stepping past it precisely so that the + // count can be exact. + expect(res.rows, n); }, timeout: const Timeout(Duration(minutes: 2))); test('a renamed column is a message, not a raw SQL error mid-import', diff --git a/test/noop_schema_drift_test.dart b/test/noop_schema_drift_test.dart index 476bbfd1..dd0f7057 100644 --- a/test/noop_schema_drift_test.dart +++ b/test/noop_schema_drift_test.dart @@ -298,6 +298,37 @@ void main() { expect(res.days, greaterThan(0)); }, timeout: const Timeout(Duration(minutes: 5))); + test('an rr_ms of NaN or Infinity is not a beat', () async { + // `double.tryParse('NaN')` really does return NaN, and NaN fails every + // comparison — so a `ms <= 0` guard passes it straight into the + // Substrate, where one bad beat poisons the whole day's HRV. + const t0 = 1786262400; // 2026-08-09 + final b = StringBuffer()..writeln(_header); + for (var i = 0; i < 600; i++) { + final ts = t0 + i; + b.writeln(_row(ts, 'hr', hr: '${60 + (i % 10)}')); + b.writeln(_row(ts, 'gravity', gx: '0.1', gy: '0.2', gz: '0.97')); + b.writeln(_row(ts, 'rr', + rr: i == 100 + ? 'NaN' + : i == 200 + ? 'Infinity' + : '900')); + } + final f = File(p.join(tmp.path, 'nan.csv')) + ..writeAsStringSync(b.toString()); + + final res = await NoopImporter.importFile( + f.path, const Profile(), DerivationEngine()); + expect(res.days, greaterThan(0)); + + final db = await LocalDb.instance; + for (final r in await db.query('day_result')) { + expect(r.values.whereType().where((v) => !v.isFinite), isEmpty, + reason: 'a non-finite beat must never reach a stored metric'); + } + }, timeout: const Timeout(Duration(minutes: 5))); + test('an UNKNOWN future stream is skipped, not fatal', () async { const t0 = 1785747600; final b = StringBuffer()..writeln(_header); From 8060dd5e7a2cc84e137bb22f7150ca1f1e708a4f Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 22:25:08 +0530 Subject: [PATCH 4/6] review round three: a day boundary is not a whole number MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ts > from - 1` is only the half-open window we want for integral timestamps. against a fractional one the last fraction of a second before midnight belongs to both days, and the map-keyed channels absorb that but rr appends — so the night got a duplicate beat and its rmssd was wrong. the first page is bounded inclusively now. both non-finite-rr tests were vacuous: every derived metric lives inside payload_json, so sweeping the row's typed columns for a non-finite double finds nothing and passes however broken the guard is. they assert on the beat count the pipeline actually used, and both fail when the guard is removed. the table names existed twice, and mistyping one silently dropped a whole channel from every import. one definition now. the out-of-order warning outlived the card that dismissed it. --- lib/import/import_container.dart | 14 +++-- lib/import/noop_backup_import.dart | 53 ++++++++++++------ lib/ui/import/import_screen.dart | 24 +++++++- test/noop_backup_import_test.dart | 89 ++++++++++++++++++++++++++++-- test/noop_schema_drift_test.dart | 46 ++++++++++++++- 5 files changed, 193 insertions(+), 33 deletions(-) diff --git a/lib/import/import_container.dart b/lib/import/import_container.dart index de3de93d..384f0fb9 100644 --- a/lib/import/import_container.dart +++ b/lib/import/import_container.dart @@ -244,11 +244,15 @@ Future resolveNoopDatabase(String path) async { // of it. Silent partial history is the worst outcome here, so verify the // whole member landed. (Dart has no portable free-space API, hence checking // after rather than before.) - // The size is checked in BOTH directions. Short means the write ran out - // of space, and a truncated database still opens — importing a fraction - // of someone's history as if it were all of it. Long means the archive's - // declared size is not what it actually holds, which is the only thing - // the ceiling above was checked against, so the ceiling did not hold. + // The size is checked in BOTH directions, AFTER the write. Short means it + // ran out of space, and a truncated database still opens — importing a + // fraction of someone's history as if it were all of it. Long means the + // archive's declared size is not what it actually holds, so the ceiling + // above was checked against a number that turned out to be fiction. Note + // this REPORTS an over-run rather than preventing it: the bytes are on + // disk by the time we can compare, and bounding that would need a + // size-limited sink around the decoder. Rejecting after the fact is still + // worth it — the file is deleted below and never imported. final written = await File(destPath).length(); if (db.size > 0 && written < db.size) { throw ImportFormatException( diff --git a/lib/import/noop_backup_import.dart b/lib/import/noop_backup_import.dart index 01e0ac98..6534d988 100644 --- a/lib/import/noop_backup_import.dart +++ b/lib/import/noop_backup_import.dart @@ -56,6 +56,21 @@ int kNoopBackupPageRows = 20000; const int _kMinPlausibleTs = 1000000000; // 2001-09-09 const int _kMaxPlausibleTs = 4102444800; // 2100-01-01 +/// Every table read, and the columns wanted from it. ONE definition: the probe +/// and the reads both key off this, so a name can no longer be spelled one way +/// here and another at a call site. That typo was silent — a mistyped table +/// simply produced an empty column list, the read skipped it, and every gravity +/// or skin-temperature sample vanished from the import with no error and a +/// still-positive day count. +const Map> _kTableCols = { + 'hrSample': ['ts', 'bpm'], + 'rrInterval': ['ts', 'rrMs'], + 'gravitySample': ['ts', 'x', 'y', 'z'], + 'skinTempSample': ['ts', 'raw'], + 'spo2Sample': ['ts', 'red', 'ir'], + 'stepSample': ['ts', 'counter'], +}; + class NoopBackupImporter { /// Import an already-extracted `noop-backup.sqlite` at [path]. /// @@ -117,14 +132,7 @@ class NoopBackupImporter { // list here and is skipped by `_read`, rather than failing mid-import with // a raw `no such column` after days have already been written. final columns = >{}; - for (final e in const { - 'hrSample': ['ts', 'bpm'], - 'rrInterval': ['ts', 'rrMs'], - 'gravitySample': ['ts', 'x', 'y', 'z'], - 'skinTempSample': ['ts', 'raw'], - 'spo2Sample': ['ts', 'red', 'ir'], - 'stepSample': ['ts', 'counter'], - }.entries) { + for (final e in _kTableCols.entries) { if (!tables.contains(e.key)) continue; final have = await _columnNames(src, e.key); columns[e.key] = [for (final c in e.value) if (have.contains(c)) c]; @@ -144,18 +152,18 @@ class NoopBackupImporter { // Order within a day does not matter — [NoopIngest] rebuilds the Substrate // sorted by timestamp — but the DAYS must arrive in ascending order, since // the high-water date is what closes out and derives the previous one. - await _read(src, tables, columns, 'hrSample', ['ts', 'bpm'], from, to, (r) async { + await _read(src, tables, columns, 'hrSample', from, to, (r) async { final ts = _int(r['ts']), v = _int(r['bpm']); if (ts == null || v == null) return; if (await ingest.offer(ts)) ingest.hr(ts, v); }); - await _read(src, tables, columns, 'rrInterval', ['ts', 'rrMs'], from, to, + await _read(src, tables, columns, 'rrInterval', from, to, (r) async { final ts = _int(r['ts']), v = _num(r['rrMs']); if (ts == null || v == null) return; if (await ingest.offer(ts)) ingest.rr(ts, v); }); - await _read(src, tables, columns, 'gravitySample', ['ts', 'x', 'y', 'z'], from, to, + await _read(src, tables, columns, 'gravitySample', from, to, (r) async { final ts = _int(r['ts']); if (ts == null) return; @@ -163,13 +171,13 @@ class NoopBackupImporter { ingest.gravity(ts, _num(r['x']), _num(r['y']), _num(r['z'])); } }); - await _read(src, tables, columns, 'skinTempSample', ['ts', 'raw'], from, to, + await _read(src, tables, columns, 'skinTempSample', from, to, (r) async { final ts = _int(r['ts']); if (ts == null) return; if (await ingest.offer(ts)) ingest.skinTemp(ts, _int(r['raw'])); }); - await _read(src, tables, columns, 'spo2Sample', ['ts', 'red', 'ir'], from, to, + await _read(src, tables, columns, 'spo2Sample', from, to, (r) async { final ts = _int(r['ts']); if (ts == null) return; @@ -177,7 +185,7 @@ class NoopBackupImporter { ingest.spo2(ts, _int(r['red']), _int(r['ir'])); } }); - await _read(src, tables, columns, 'stepSample', ['ts', 'counter'], from, to, + await _read(src, tables, columns, 'stepSample', from, to, (r) async { final ts = _int(r['ts']), v = _int(r['counter']); if (ts == null || v == null) return; @@ -217,25 +225,36 @@ class NoopBackupImporter { Set tables, Map> columns, String table, - List cols, int from, int to, Future Function(Map row) onRow, ) async { + // A table name with no entry is a programming error, not a schema + // variation — fail loudly here rather than silently importing without that + // channel. + final cols = _kTableCols[table]!; if (!tables.contains(table)) return; final usable = columns[table] ?? const []; if (usable.length < cols.length) return; - num cursor = from - 1; + // `ts >= from` on the FIRST page, `ts > cursor` after. Not `ts > from - 1`: + // that is only equivalent for integral timestamps, and against a fractional + // one it makes the open interval (to-1, to) belong to BOTH the day that + // ends at `to` and the day that starts there. The straddling rows are then + // read twice, and while the map-keyed channels absorb that, `rr()` APPENDS — + // a duplicated beat corrupts the night's RMSSD rather than costing a row. + num cursor = from; + var firstPage = true; while (true) { final rows = await src.query( table, columns: cols, - where: 'ts > ? AND ts < ?', + where: firstPage ? 'ts >= ? AND ts < ?' : 'ts > ? AND ts < ?', whereArgs: [cursor, to], orderBy: 'ts', limit: kNoopBackupPageRows, ); + firstPage = false; if (rows.isEmpty) return; final full = rows.length == kNoopBackupPageRows; diff --git a/lib/ui/import/import_screen.dart b/lib/ui/import/import_screen.dart index ab4601aa..33ee7416 100644 --- a/lib/ui/import/import_screen.dart +++ b/lib/ui/import/import_screen.dart @@ -77,11 +77,19 @@ class _ImportScreenState extends State { // still up and will deliver the user's choice, so say nothing. Anything // else is worth showing. if (e.code != 'already_active') { - _set(() => _error = 'Could not open the file picker: ${e.message ?? e.code}'); + _set(() { + _error = 'Could not open the file picker: ${e.message ?? e.code}'; + _result = null; + _warning = null; + }); } return const []; } catch (e) { - _set(() => _error = 'Could not open the file picker: $e'); + _set(() { + _error = 'Could not open the file picker: $e'; + _result = null; + _warning = null; + }); return const []; } finally { _set(() => _picking = false); @@ -264,7 +272,17 @@ class _ImportScreenState extends State { const SizedBox(height: Sp.x2), Center( child: TextButton( - onPressed: _busy ? null : () => _set(() => _result = null), + onPressed: _busy + ? null + // The warning is cleared WITH the result: they describe the + // same import, and clearing only one left an orphaned + // "days could not be re-analysed" card with no way to dismiss + // it — the button that would have dismissed it disappears + // along with the result card. + : () => _set(() { + _result = null; + _warning = null; + }), child: const Text('Import another file'), ), ), diff --git a/test/noop_backup_import_test.dart b/test/noop_backup_import_test.dart index 24004fb5..5946fd3c 100644 --- a/test/noop_backup_import_test.dart +++ b/test/noop_backup_import_test.dart @@ -7,6 +7,7 @@ // empty `spo2Sample`/`respSample` tables, and a deviceId that differs between // the sample tables ("my-whoop") and `sleepSession` ("my-whoop-noop"). +import 'dart:convert'; import 'dart:io'; import 'package:archive/archive.dart'; @@ -20,6 +21,38 @@ import 'package:openstrap_edge/import/import_container.dart'; import 'package:openstrap_edge/import/noop_backup_import.dart'; import 'package:openstrap_edge/import/noop_import.dart'; +/// 'YYYY-MM-DD' for an epoch second, in the LOCAL zone — the same day key the +/// importer writes, so an assertion can scope itself to its own fixture. +String _localDay(int epochSec) { + final d = DateTime.fromMillisecondsSinceEpoch(epochSec * 1000); + return '${d.year.toString().padLeft(4, '0')}-' + '${d.month.toString().padLeft(2, '0')}-' + '${d.day.toString().padLeft(2, '0')}'; +} + + +/// `payload_json` nests some sections as encoded strings and some as maps, +/// depending on which writer produced them — decode either shape. +Map? _section(Object? v) { + if (v is Map) return v; + if (v is String) { + try { + final d = jsonDecode(v); + if (d is Map) return d; + } on FormatException { + // A rendered value ("—"), not an encoded section. + } + } + return null; +} + +/// Beats the RR pipeline actually used for [day], or null if it did not run. +int? _beatsUsed(Map row) { + final payload = _section(row['payload_json']); + final irregular = _section(_section(payload?['clinical'])?['irregular_24h']); + return _section(irregular?['value'])?['n_beats'] as int?; +} + void main() { late Directory tmp; @@ -246,7 +279,7 @@ void main() { // (The NaN path proper is exercised through the CSV importer, where // `double.tryParse('NaN')` genuinely produces one — see // noop_schema_drift_test.dart.) - const t0 = 1785919200; // 2026-08-05 + const t0 = 1786608000; // 2026-08-13, a day no other fixture here uses final path = p.join(tmp.path, 'nan.sqlite'); if (File(path).existsSync()) File(path).deleteSync(); final src = await databaseFactory.openDatabase(path); @@ -255,12 +288,15 @@ void main() { await src.execute('CREATE TABLE rrInterval (deviceId TEXT, ts INTEGER, ' 'rrMs REAL, PRIMARY KEY (deviceId, ts, rrMs))'); final b = src.batch(); - for (var i = 0; i < 120; i++) { + // 600 beats: the irregular-rhythm screen (the one payload field that + // reports how many beats it actually used) needs a real window before it + // emits anything, and without it there is nothing to assert on. + for (var i = 0; i < 600; i++) { b.insert('hrSample', {'deviceId': 'd', 'ts': t0 + i, 'bpm': 65}); b.insert('rrInterval', { 'deviceId': 'd', 'ts': t0 + i, - 'rrMs': i == 60 ? double.infinity : 900.0, + 'rrMs': i == 300 ? double.infinity : 900.0, }); } await b.commit(noResult: true); @@ -271,11 +307,21 @@ void main() { bak, const Profile(), DerivationEngine()); expect(res.days, greaterThan(0), reason: 'the day still imports'); + // The beat COUNT is what discriminates — the row's typed columns hold no + // derived metric at all, so sweeping them for a non-finite double passes + // whatever the guard does. 120 beats written, one of them infinite. + final day = _localDay(t0); final db = await LocalDb.instance; - final rows = await db.query('day_result'); + final rows = + await db.query('day_result', where: 'day_id = ?', whereArgs: [day]); + var checked = 0; for (final r in rows) { - expect(r.values.whereType().where((v) => !v.isFinite), isEmpty); + final n = _beatsUsed(r); + if (n == null) continue; + expect(n, 599, reason: 'the infinite beat is dropped, not counted'); + checked++; } + expect(checked, greaterThan(0), reason: 'the assertion must have run'); }, timeout: const Timeout(Duration(minutes: 5))); test('a database that is not a NOOP backup is named, not silently empty', @@ -382,6 +428,39 @@ void main() { expect(res.rows, n); }, timeout: const Timeout(Duration(minutes: 2))); + test('a fractional timestamp at midnight is not read by both days', () async { + // The day windows are half-open, but `ts > from - 1` only expresses that + // for integral timestamps: against a fractional one the interval + // (midnight-1, midnight) belongs to the day before AND the day after. The + // map-keyed channels absorb the double read; `rr()` appends, so the night + // gets a duplicate beat and its RMSSD is wrong. + final midnight = DateTime(2026, 8, 5).millisecondsSinceEpoch ~/ 1000; + final path = p.join(tmp.path, 'straddle.sqlite'); + if (File(path).existsSync()) File(path).deleteSync(); + final src = await databaseFactory.openDatabase(path); + await src.execute('CREATE TABLE hrSample (deviceId TEXT, ts REAL, ' + 'bpm INTEGER, PRIMARY KEY (deviceId, ts))'); + await src.execute('CREATE TABLE rrInterval (deviceId TEXT, ts REAL, ' + 'rrMs REAL, PRIMARY KEY (deviceId, ts, rrMs))'); + final b = src.batch(); + var expected = 0; + // 20 min either side of midnight, every sample on a .5 offset. + for (var i = -1200; i < 1200; i++) { + final ts = midnight + i + 0.5; + b.insert('hrSample', {'deviceId': 'd', 'ts': ts, 'bpm': 60}); + b.insert('rrInterval', {'deviceId': 'd', 'ts': ts, 'rrMs': 900.0}); + expected += 2; + } + await b.commit(noResult: true); + await src.close(); + final bak = writeBackup('straddle.noopbak', path); + + final res = await NoopImporter.importFile( + bak, const Profile(), DerivationEngine()); + expect(res.rows, expected, + reason: 'every sample is read exactly once, on exactly one day'); + }, timeout: const Timeout(Duration(minutes: 5))); + test('a renamed column is a message, not a raw SQL error mid-import', () async { const t0 = 1786176000; // 2026-08-08 diff --git a/test/noop_schema_drift_test.dart b/test/noop_schema_drift_test.dart index dd0f7057..e970f152 100644 --- a/test/noop_schema_drift_test.dart +++ b/test/noop_schema_drift_test.dart @@ -12,6 +12,7 @@ // There was no test that parsed a real NOOP CSV at all — only the pure // `decideRow` ordering contract — which is why the drift shipped unnoticed. +import 'dart:convert'; import 'dart:io'; import 'package:flutter_test/flutter_test.dart'; @@ -47,6 +48,29 @@ String _row(int ts, String stream, ].join(','); } + +/// `payload_json` nests some sections as encoded strings and some as maps, +/// depending on which writer produced them — decode either shape. +Map? _section(Object? v) { + if (v is Map) return v; + if (v is String) { + try { + final d = jsonDecode(v); + if (d is Map) return d; + } on FormatException { + // A rendered value ("—"), not an encoded section. + } + } + return null; +} + +/// Beats the RR pipeline actually used for [day], or null if it did not run. +int? _beatsUsed(Map row) { + final payload = _section(row['payload_json']); + final irregular = _section(_section(payload?['clinical'])?['irregular_24h']); + return _section(irregular?['value'])?['n_beats'] as int?; +} + void main() { group('stepRuns — cumulative counter → real step windows (pure)', () { test('sums positive deltas across one contiguous run', () { @@ -322,11 +346,27 @@ void main() { f.path, const Profile(), DerivationEngine()); expect(res.days, greaterThan(0)); + // Assert on the BEAT COUNT the pipeline actually used, not on the row's + // typed columns: every derived metric lives inside `payload_json`, so a + // `whereType` sweep over the row finds nothing and passes however + // broken the guard is. 600 beats were written, two of them non-finite. + final d = DateTime.fromMillisecondsSinceEpoch(t0 * 1000); + final day = '${d.year.toString().padLeft(4, '0')}-' + '${d.month.toString().padLeft(2, '0')}-' + '${d.day.toString().padLeft(2, '0')}'; final db = await LocalDb.instance; - for (final r in await db.query('day_result')) { - expect(r.values.whereType().where((v) => !v.isFinite), isEmpty, - reason: 'a non-finite beat must never reach a stored metric'); + final rows = + await db.query('day_result', where: 'day_id = ?', whereArgs: [day]); + expect(rows, isNotEmpty); + var checked = 0; + for (final r in rows) { + final n = _beatsUsed(r); + if (n == null) continue; + expect(n, 598, + reason: 'the NaN and the Infinity are dropped, not counted'); + checked++; } + expect(checked, greaterThan(0), reason: 'the assertion must have run'); }, timeout: const Timeout(Duration(minutes: 5))); test('an UNKNOWN future stream is skipped, not fatal', () async { From 196e0bc5792f2c463c02b76512d762d167d6ec69 Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 23:03:05 +0530 Subject: [PATCH 5/6] probe the schema before querying it, and require a usable spine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the span selects MIN(ts)/MAX(ts), and it ran before the column probe — so a table drifted far enough to have no ts at all threw a raw SQL error out of the span rather than being skipped by the probe that exists for it. an hrSample carrying no bpm passed the table-name check and was then skipped by every read, leaving the other channels to carry the import to a plausible day count with no heart rate anywhere in it. --- lib/import/noop_backup_import.dart | 45 +++++++++++++++-------- test/noop_backup_import_test.dart | 57 ++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 14 deletions(-) diff --git a/lib/import/noop_backup_import.dart b/lib/import/noop_backup_import.dart index 6534d988..10e3a490 100644 --- a/lib/import/noop_backup_import.dart +++ b/lib/import/noop_backup_import.dart @@ -117,20 +117,12 @@ class NoopBackupImporter { ); } - final span = await _span(src, tables); - if (span == null) { - throw const ImportFormatException( - 'That NOOP backup holds no samples — there is nothing to import.', - ); - } - final (minTs, maxTs) = span; - - // Probe every table's columns ONCE. The schema cannot change during a - // read-only import, and doing it inside the day walk cost a - // `PRAGMA table_info` per table per day — thousands of pointless queries on - // a long backup. A table whose columns have drifted resolves to a short - // list here and is skipped by `_read`, rather than failing mid-import with - // a raw `no such column` after days have already been written. + // Probe every table's columns ONCE, and do it BEFORE anything queries them. + // The schema cannot change during a read-only import, and probing inside + // the day walk cost a `PRAGMA table_info` per table per day. Order matters: + // `_span` selects MIN(ts)/MAX(ts), so a drifted table with no `ts` at all + // would throw a raw SQL error out of the span before the probe that exists + // to skip it had run. final columns = >{}; for (final e in _kTableCols.entries) { if (!tables.contains(e.key)) continue; @@ -138,6 +130,31 @@ class NoopBackupImporter { columns[e.key] = [for (final c in e.value) if (have.contains(c)) c]; } + // The spine has to be USABLE, not merely present. A `hrSample` with no + // `bpm` clears the table-name check above and is then silently skipped by + // every read — the other channels would carry the import to a plausible day + // count with no heart rate in any of it. + if ((columns['hrSample'] ?? const []).length < + _kTableCols['hrSample']!.length) { + throw const ImportFormatException( + 'That database has an `hrSample` table without the columns we read ' + '(`ts`, `bpm`), so there is no heart rate to import.', + ); + } + + // Only tables we can actually read contribute to the span. + final readable = { + for (final e in columns.entries) + if (e.value.length == (_kTableCols[e.key]?.length ?? -1)) e.key, + }; + final span = await _span(src, readable); + if (span == null) { + throw const ImportFormatException( + 'That NOOP backup holds no samples — there is nothing to import.', + ); + } + final (minTs, maxTs) = span; + final ingest = NoopIngest(profile, engine, onProgress: onProgress); // Walk LOCAL days. Built through the DateTime(y, m, d + 1) constructor diff --git a/test/noop_backup_import_test.dart b/test/noop_backup_import_test.dart index 5946fd3c..f53a3582 100644 --- a/test/noop_backup_import_test.dart +++ b/test/noop_backup_import_test.dart @@ -487,6 +487,63 @@ void main() { reason: 'the drifted table is skipped, the rest still imports'); }, timeout: const Timeout(Duration(minutes: 5))); + test('an optional table with no ts column does not break the span', () async { + // `_span` selects MIN(ts)/MAX(ts). Running it before the column probe threw + // a raw SQL error out of a table the probe exists to skip. + const t0 = 1786694400; // 2026-08-14 + final path = p.join(tmp.path, 'nots.sqlite'); + if (File(path).existsSync()) File(path).deleteSync(); + final src = await databaseFactory.openDatabase(path); + await src.execute('CREATE TABLE hrSample (deviceId TEXT, ts INTEGER, ' + 'bpm INTEGER, PRIMARY KEY (deviceId, ts))'); + // Drifted beyond recognition: no `ts` at all. + await src.execute('CREATE TABLE spo2Sample (deviceId TEXT, ' + 'recordedAt INTEGER, red INTEGER, ir INTEGER)'); + await src.insert('spo2Sample', + {'deviceId': 'd', 'recordedAt': t0, 'red': 1, 'ir': 2}); + final b = src.batch(); + for (var i = 0; i < 300; i++) { + b.insert('hrSample', {'deviceId': 'd', 'ts': t0 + i, 'bpm': 63}); + } + await b.commit(noResult: true); + await src.close(); + final bak = writeBackup('nots.noopbak', path); + + final res = await NoopImporter.importFile( + bak, const Profile(), DerivationEngine()); + expect(res.days, greaterThan(0)); + expect(res.rows, 300); + }, timeout: const Timeout(Duration(minutes: 5))); + + test('an hrSample without bpm is refused, not quietly imported', () async { + // The table-name check passes, then every read skips it — the remaining + // channels would carry the import to a plausible day count with no heart + // rate anywhere in it. + const t0 = 1786780800; // 2026-08-15 + final path = p.join(tmp.path, 'nobpm.sqlite'); + if (File(path).existsSync()) File(path).deleteSync(); + final src = await databaseFactory.openDatabase(path); + await src.execute('CREATE TABLE hrSample (deviceId TEXT, ts INTEGER, ' + 'heartRate INTEGER, PRIMARY KEY (deviceId, ts))'); + await src.execute('CREATE TABLE gravitySample (deviceId TEXT, ts INTEGER, ' + 'x DOUBLE, y DOUBLE, z DOUBLE, PRIMARY KEY (deviceId, ts))'); + final b = src.batch(); + for (var i = 0; i < 300; i++) { + b.insert('hrSample', {'deviceId': 'd', 'ts': t0 + i, 'heartRate': 63}); + b.insert('gravitySample', + {'deviceId': 'd', 'ts': t0 + i, 'x': 0.1, 'y': 0.2, 'z': 0.97}); + } + await b.commit(noResult: true); + await src.close(); + final bak = writeBackup('nobpm.noopbak', path); + + await expectLater( + NoopImporter.importFile(bak, const Profile(), DerivationEngine()), + throwsA(isA() + .having((e) => e.message, 'message', contains('heart rate'))), + ); + }, timeout: const Timeout(Duration(minutes: 5))); + test('a backup with no samples says so rather than importing 0 days', () async { final path = p.join(tmp.path, 'empty.sqlite'); From dc1cabe8dc1e80dc7b9d98effa8f0fdaf4e2590b Mon Sep 17 00:00:00 2001 From: abdulsaheel Date: Sat, 8 Aug 2026 23:14:09 +0530 Subject: [PATCH 6/6] order both queries the same way, or the offset means nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the drain pages an equal-timestamp group with offset, and ordering by ts alone is not a total order once a second holds several beats — the two queries were free to hand that group back in different orders, which skips and repeats rows. rowid gives a stable one where the table has it, probed rather than assumed. --- lib/import/noop_backup_import.dart | 36 +++++++++++++++++++------ test/noop_backup_import_test.dart | 42 ++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/lib/import/noop_backup_import.dart b/lib/import/noop_backup_import.dart index 10e3a490..608ca9ce 100644 --- a/lib/import/noop_backup_import.dart +++ b/lib/import/noop_backup_import.dart @@ -124,10 +124,17 @@ class NoopBackupImporter { // would throw a raw SQL error out of the span before the probe that exists // to skip it had run. final columns = >{}; + final ordered = {}; for (final e in _kTableCols.entries) { if (!tables.contains(e.key)) continue; final have = await _columnNames(src, e.key); columns[e.key] = [for (final c in e.value) if (have.contains(c)) c]; + // `ORDER BY ts` is not a total order — `rrInterval` holds several beats + // per second — and the drain below pages an equal-timestamp group with + // OFFSET, which is only meaningful if both queries see the SAME order. + // `rowid` provides one for free on an ordinary table; a WITHOUT ROWID + // table has none, so probe rather than assume. + ordered[e.key] = await _hasRowid(src, e.key) ? 'ts, rowid' : 'ts'; } // The spine has to be USABLE, not merely present. A `hrSample` with no @@ -169,18 +176,18 @@ class NoopBackupImporter { // Order within a day does not matter — [NoopIngest] rebuilds the Substrate // sorted by timestamp — but the DAYS must arrive in ascending order, since // the high-water date is what closes out and derives the previous one. - await _read(src, tables, columns, 'hrSample', from, to, (r) async { + await _read(src, tables, columns, ordered, 'hrSample', from, to, (r) async { final ts = _int(r['ts']), v = _int(r['bpm']); if (ts == null || v == null) return; if (await ingest.offer(ts)) ingest.hr(ts, v); }); - await _read(src, tables, columns, 'rrInterval', from, to, + await _read(src, tables, columns, ordered, 'rrInterval', from, to, (r) async { final ts = _int(r['ts']), v = _num(r['rrMs']); if (ts == null || v == null) return; if (await ingest.offer(ts)) ingest.rr(ts, v); }); - await _read(src, tables, columns, 'gravitySample', from, to, + await _read(src, tables, columns, ordered, 'gravitySample', from, to, (r) async { final ts = _int(r['ts']); if (ts == null) return; @@ -188,13 +195,13 @@ class NoopBackupImporter { ingest.gravity(ts, _num(r['x']), _num(r['y']), _num(r['z'])); } }); - await _read(src, tables, columns, 'skinTempSample', from, to, + await _read(src, tables, columns, ordered, 'skinTempSample', from, to, (r) async { final ts = _int(r['ts']); if (ts == null) return; if (await ingest.offer(ts)) ingest.skinTemp(ts, _int(r['raw'])); }); - await _read(src, tables, columns, 'spo2Sample', from, to, + await _read(src, tables, columns, ordered, 'spo2Sample', from, to, (r) async { final ts = _int(r['ts']); if (ts == null) return; @@ -202,7 +209,7 @@ class NoopBackupImporter { ingest.spo2(ts, _int(r['red']), _int(r['ir'])); } }); - await _read(src, tables, columns, 'stepSample', from, to, + await _read(src, tables, columns, ordered, 'stepSample', from, to, (r) async { final ts = _int(r['ts']), v = _int(r['counter']); if (ts == null || v == null) return; @@ -241,6 +248,7 @@ class NoopBackupImporter { Database src, Set tables, Map> columns, + Map ordered, String table, int from, int to, @@ -253,6 +261,7 @@ class NoopBackupImporter { if (!tables.contains(table)) return; final usable = columns[table] ?? const []; if (usable.length < cols.length) return; + final orderBy = ordered[table] ?? 'ts'; // `ts >= from` on the FIRST page, `ts > cursor` after. Not `ts > from - 1`: // that is only equivalent for integral timestamps, and against a fractional @@ -268,7 +277,7 @@ class NoopBackupImporter { columns: cols, where: firstPage ? 'ts >= ? AND ts < ?' : 'ts > ? AND ts < ?', whereArgs: [cursor, to], - orderBy: 'ts', + orderBy: orderBy, limit: kNoopBackupPageRows, ); firstPage = false; @@ -301,7 +310,7 @@ class NoopBackupImporter { columns: cols, where: 'ts = ?', whereArgs: [lastTs], - orderBy: 'ts', + orderBy: orderBy, limit: kNoopBackupPageRows, offset: drained, ); @@ -360,6 +369,17 @@ class NoopBackupImporter { return (lo == null || hi == null) ? null : (lo, hi); } + /// Does [table] have an implicit `rowid`? False for a WITHOUT ROWID table, + /// where selecting it is an error rather than an empty result. + static Future _hasRowid(Database src, String table) async { + try { + await src.rawQuery('SELECT rowid FROM $table LIMIT 1'); + return true; + } catch (_) { + return false; + } + } + static Future> _columnNames(Database src, String table) async { final rows = await src.rawQuery('PRAGMA table_info($table)'); return {for (final r in rows) (r['name'] as String?) ?? ''}; diff --git a/test/noop_backup_import_test.dart b/test/noop_backup_import_test.dart index f53a3582..53c9141e 100644 --- a/test/noop_backup_import_test.dart +++ b/test/noop_backup_import_test.dart @@ -544,6 +544,48 @@ void main() { ); }, timeout: const Timeout(Duration(minutes: 5))); + test('a timestamp with more rows than a page loses none to the drain', + () async { + // The drain pages an equal-timestamp group with OFFSET. Two queries whose + // ORDER BY is not a total order can hand back that group in different + // orders, and the offset then skips and repeats — silent duplicate beats + // in the append-only RR channel. + const t0 = 1786867200; // 2026-08-16 + const beats = 25; // > the page size set below + final path = p.join(tmp.path, 'drain.sqlite'); + if (File(path).existsSync()) File(path).deleteSync(); + final src = await databaseFactory.openDatabase(path); + await src.execute('CREATE TABLE hrSample (deviceId TEXT, ts INTEGER, ' + 'bpm INTEGER, PRIMARY KEY (deviceId, ts))'); + await src.execute('CREATE TABLE rrInterval (deviceId TEXT, ts INTEGER, ' + 'rrMs REAL, PRIMARY KEY (deviceId, ts, rrMs))'); + final b = src.batch(); + var expected = 0; + for (var i = 0; i < 60; i++) { + b.insert('hrSample', {'deviceId': 'd', 'ts': t0 + i, 'bpm': 64}); + expected++; + } + // One second carrying far more beats than a page holds, inserted in a + // scrambled rrMs order so a naive tie order differs from insertion order. + for (var k = 0; k < beats; k++) { + final rr = 700.0 + ((k * 37) % beats); + b.insert('rrInterval', {'deviceId': 'd', 'ts': t0 + 30, 'rrMs': rr}); + expected++; + } + await b.commit(noResult: true); + await src.close(); + final bak = writeBackup('drain.noopbak', path); + + final saved = kNoopBackupPageRows; + kNoopBackupPageRows = 8; + addTearDown(() => kNoopBackupPageRows = saved); + + final res = await NoopImporter.importFile( + bak, const Profile(), DerivationEngine()); + expect(res.rows, expected, + reason: 'every row read exactly once, drain included'); + }, timeout: const Timeout(Duration(minutes: 5))); + test('a backup with no samples says so rather than importing 0 days', () async { final path = p.join(tmp.path, 'empty.sqlite');