From da0b54d88663fa0eb701d5cf972a6793bfd200a9 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Wed, 5 Aug 2026 08:35:21 +0200 Subject: [PATCH] Sync meals and the food bank through Firebase, mirroring to GitHub The largest consumer moves last on purpose: diet_guard was 71% of all sync traffic, and both its surfaces (the PC timer and the phone app) share one prefix, so they move together. Firebase becomes the primary backend when it is set up, with GitHub kept as a mirror so a device that has not moved yet still converges. An unconfigured device keeps syncing exactly as before, which is also the rollback. Revision tracking is new here. The PC hand-rolled its own pull-merge-push instead of calling sync_log, so it had none: every tick re-downloaded every peer's whole food log -- hundreds of KB, 96 times a day -- and re-pushed its own regardless of change. It now reads one small revision map first, skips any peer whose hash is unchanged, and suppresses a no-op push. Every sub-sync (log, budget, food bank, manual bank) takes the client as a parameter, so one swap covers all four. The app could not have reached Firebase at all: nothing called saveAccount(), so loadAccount() always returned null and openFirebase() always returned null. Settings now has a Firebase section that stores the account in the OS keystore and signs in immediately, so a typo surfaces on the screen where it can be fixed rather than as a silently failed background sync hours later. The email and password never touch the repo -- it is public. Platform reaches now degrade instead of failing: an unavailable keystore or application-support directory costs one tick of extra traffic, where before it would have taken sync down entirely. Caught by `on Object` rather than `on Exception`, because an uninitialised binding raises an Error. Verified end to end against the real database, not just in tests: a live PC sync merged 57 days / 339 entries and put 221 KB of food log plus 97 food-bank records into Firebase; a second unchanged tick issued zero pushes; and the Dart client signed in and read back all 339 meals and 97 foods, then completed a push / publish-revision / suppress-no-op round trip under a throwaway prefix. diet_guard: 618 tests at 100% branch coverage. diet_guard_app: 477 tests. Two assertions moved from 4 pushes to 5 -- the extra one is this device's revision, published after the log so a peer can never cache "seen rev X" against a log it never received. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SJ9sxEw3ymzXNjL7SHNpJc --- .pre-commit-config.yaml | 2 +- app/lib/screens/log_meal_screen.dart | 3 +- app/lib/screens/settings_screen.dart | 131 +++++++++++++++- app/lib/services/background_sync_service.dart | 3 +- app/lib/services/firebase_backend.dart | 105 +++++++++++++ app/lib/services/sync_service.dart | 14 +- app/lib/services/sync_state_factory.dart | 9 ++ app/lib/services/sync_state_factory_io.dart | 37 +++++ app/lib/services/sync_state_factory_web.dart | 27 ++++ app/pubspec.lock | 6 +- app/pubspec.yaml | 2 +- .../background_sync_service_test.dart | 4 +- app/test/services/sync_service_test.dart | 6 +- .../services/sync_state_factory_test.dart | 89 +++++++++++ diet_guard/_constants.py | 6 + diet_guard/_sync.py | 124 ++++++++++++++-- diet_guard/tests/conftest.py | 11 ++ diet_guard/tests/test_sync.py | 140 +++++++++++++++++- 18 files changed, 688 insertions(+), 31 deletions(-) create mode 100644 app/lib/services/firebase_backend.dart create mode 100644 app/lib/services/sync_state_factory.dart create mode 100644 app/lib/services/sync_state_factory_io.dart create mode 100644 app/lib/services/sync_state_factory_web.dart create mode 100644 app/test/services/sync_state_factory_test.dart diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index eb49dee..8e75ccd 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -102,7 +102,7 @@ repos: - pytest - requests - gatelock @ git+https://github.com/kuhyx/utils@gatelock-v0.4.0#subdirectory=gatelock - - crdt-sync @ git+https://github.com/kuhyx/utils@crdt-sync-v0.3.0#subdirectory=crdt-sync + - crdt-sync @ git+https://github.com/kuhyx/utils@crdt-sync-v0.5.1#subdirectory=crdt-sync - mcp>=2,<3 - pydantic # Test suites intentionally use patterns (protected-access, magic-value diff --git a/app/lib/screens/log_meal_screen.dart b/app/lib/screens/log_meal_screen.dart index 7b4d076..1b02a0a 100644 --- a/app/lib/screens/log_meal_screen.dart +++ b/app/lib/screens/log_meal_screen.dart @@ -19,6 +19,7 @@ import 'package:diet_guard_app/services/background_tasks.dart'; import 'package:diet_guard_app/services/budget_history_service.dart'; import 'package:diet_guard_app/services/day_status_service.dart'; import 'package:diet_guard_app/services/due_slot_check.dart'; +import 'package:diet_guard_app/services/firebase_backend.dart'; import 'package:diet_guard_app/services/foodbank_service.dart'; import 'package:diet_guard_app/services/github_client_factory.dart'; import 'package:diet_guard_app/services/log_storage_service.dart'; @@ -125,7 +126,7 @@ class _LogMealScreenState extends State httpClient: widget.httpClient, ); try { - await runSync(client); + await runSync(await syncBackend(client)); } finally { client.close(); } diff --git a/app/lib/screens/settings_screen.dart b/app/lib/screens/settings_screen.dart index 38974da..9e32dc2 100644 --- a/app/lib/screens/settings_screen.dart +++ b/app/lib/screens/settings_screen.dart @@ -8,8 +8,10 @@ library; import 'dart:async'; +import 'package:crdt_sync/crdt_sync.dart'; import 'package:diet_guard_app/screens/log_meal_screen.dart'; import 'package:diet_guard_app/services/app_settings_service.dart'; +import 'package:diet_guard_app/services/firebase_backend.dart'; import 'package:diet_guard_app/services/github_client_factory.dart'; import 'package:diet_guard_app/services/github_device_auth.dart'; import 'package:diet_guard_app/services/sync_service.dart'; @@ -49,6 +51,9 @@ class _SettingsScreenState extends State { final _repoController = TextEditingController(); final _tokenController = TextEditingController(); final _clientIdController = TextEditingController(); + final _firebaseEmailController = TextEditingController(); + final _firebasePasswordController = TextEditingController(); + bool _firebaseConfigured = false; bool _loading = true; bool _busy = false; String? _status; @@ -81,7 +86,15 @@ class _SettingsScreenState extends State { ? settings.token : ''; _clientIdController.text = settings.clientId; - setState(() => _loading = false); + // Reflect a previously-stored account, so a returning user sees the + // real state instead of an empty form that looks unconfigured. + final account = await loadAccount(); + if (!mounted) return; + if (account != null) _firebaseEmailController.text = account.email; + setState(() { + _firebaseConfigured = account != null; + _loading = false; + }); } @override @@ -94,6 +107,8 @@ class _SettingsScreenState extends State { _ownerController.dispose(); _repoController.dispose(); _tokenController.dispose(); + _firebaseEmailController.dispose(); + _firebasePasswordController.dispose(); _clientIdController.dispose(); super.dispose(); } @@ -194,7 +209,7 @@ class _SettingsScreenState extends State { final settings = _currentSettings(); final client = createGitHubClient(settings, httpClient: widget.httpClient); try { - await runSync(client); + await runSync(await syncBackend(client)); _showMessage('Connected and synced.'); } on Exception catch (e) { _showMessage('Connected, but sync failed: $e'); @@ -232,7 +247,7 @@ class _SettingsScreenState extends State { await settings.save(); final client = createGitHubClient(settings, httpClient: widget.httpClient); try { - await runSync(client); + await runSync(await syncBackend(client)); _showMessage('Synced.'); } on Exception catch (e) { _showMessage('Sync failed: $e'); @@ -268,6 +283,63 @@ class _SettingsScreenState extends State { bool get _isAndroid => !kIsWeb && defaultTargetPlatform == TargetPlatform.android; + /// Stores the Firebase account in the OS keystore and proves it works. + /// + /// Signing in here rather than only saving means a typo surfaces now, on + /// the screen where it can be fixed, instead of as a silently failed + /// background sync hours later. + Future _saveFirebaseAccount() async { + final email = _firebaseEmailController.text.trim(); + final password = _firebasePasswordController.text; + if (email.isEmpty || password.isEmpty) { + setState(() => _status = 'Enter the sync account email and password.'); + return; + } + setState(() { + _busy = true; + _status = null; + }); + try { + await saveAccount(FirebaseAccount(email: email, password: password)); + final client = await openFirebase(); + if (client == null) { + throw Exception('could not reach Firebase'); + } + final reachable = await client.canAccessRemote(); + client.close(); + if (!mounted) return; + setState(() { + _firebaseConfigured = reachable; + _status = reachable + ? 'Firebase connected - syncs now go there first.' + : 'Signed in, but the database refused the read.'; + }); + } on Object catch (error) { + // Broader than Exception: a missing platform binding raises an Error. + // A half-stored account is worse than none, so forget it. + await clearAccount(); + if (!mounted) return; + setState(() { + _firebaseConfigured = false; + _status = 'Firebase sign-in failed: $error'; + }); + } finally { + if (mounted) setState(() => _busy = false); + } + } + + /// Forgets the Firebase account, so sync falls back to GitHub alone. + Future _disconnectFirebase() async { + await clearAccount(); + _firebaseEmailController.clear(); + _firebasePasswordController.clear(); + if (!mounted) return; + setState(() { + _firebaseConfigured = false; + _status = 'Firebase disconnected - syncing over GitHub only.'; + }); + } + @override Widget build(BuildContext context) { if (_loading) { @@ -307,6 +379,59 @@ class _SettingsScreenState extends State { const SizedBox(height: 24), const Divider(), const SizedBox(height: 8), + Text( + 'Firebase sync', + style: Theme.of(context).textTheme.titleMedium, + ), + const SizedBox(height: 8), + Text( + _firebaseConfigured + ? 'Connected. Syncs go to Firebase first, and still ' + 'mirror to GitHub until every device has moved.' + : 'Not connected - syncing over GitHub only. Enter the ' + 'shared sync account to move this device over. The ' + 'password is kept in the device keystore, never in ' + 'the app or the repo.', + style: Theme.of(context).textTheme.bodySmall, + ), + const SizedBox(height: 12), + TextField( + controller: _firebaseEmailController, + keyboardType: TextInputType.emailAddress, + autocorrect: false, + decoration: const InputDecoration( + labelText: 'Sync account email', + ), + ), + const SizedBox(height: 8), + TextField( + controller: _firebasePasswordController, + obscureText: true, + autocorrect: false, + decoration: const InputDecoration( + labelText: 'Sync account password', + ), + ), + const SizedBox(height: 12), + Row( + children: [ + FilledButton.icon( + onPressed: _busy ? null : _saveFirebaseAccount, + icon: const Icon(Icons.cloud_done), + label: const Text('Connect Firebase'), + ), + if (_firebaseConfigured) ...[ + const SizedBox(width: 8), + TextButton( + onPressed: _busy ? null : _disconnectFirebase, + child: const Text('Disconnect'), + ), + ], + ], + ), + const SizedBox(height: 24), + const Divider(), + const SizedBox(height: 8), Text( 'Authorize in your browser — no token to paste. Syncs to ' 'kuhyx/syncs by default.', diff --git a/app/lib/services/background_sync_service.dart b/app/lib/services/background_sync_service.dart index 42f18f6..10abd11 100644 --- a/app/lib/services/background_sync_service.dart +++ b/app/lib/services/background_sync_service.dart @@ -7,6 +7,7 @@ library; import 'package:diet_guard_app/services/app_settings_service.dart'; import 'package:diet_guard_app/services/budget_history_service.dart'; +import 'package:diet_guard_app/services/firebase_backend.dart'; import 'package:diet_guard_app/services/foodbank_service.dart'; import 'package:diet_guard_app/services/github_client_factory.dart'; import 'package:diet_guard_app/services/log_storage_service.dart'; @@ -46,7 +47,7 @@ Future backgroundSyncPush({http.Client? httpClient}) async { if (!settings.isConfigured) return true; // nothing to push; don't retry final client = createGitHubClient(settings, httpClient: httpClient); try { - await runSync(client); + await runSync(await syncBackend(client)); return true; } on Exception { return false; // offline / transient GitHub error -> retry with backoff diff --git a/app/lib/services/firebase_backend.dart b/app/lib/services/firebase_backend.dart new file mode 100644 index 0000000..433126b --- /dev/null +++ b/app/lib/services/firebase_backend.dart @@ -0,0 +1,105 @@ +/// Wiring for the Firebase backend, during and after the GitHub cutover. +/// +/// Split by what is safe to publish, because this repo is public: +/// +/// * [kProject] holds the Web API key and database URL. Both are public +/// identifiers that already ship inside the APK; the security rules, not +/// their secrecy, are what protect the data. +/// * The account email and password are entered once per device and kept in +/// the OS keystore, next to the GitHub token this app already stores there. +/// +/// Nothing here reads `~/.config/crdt-sync/` — that is the desktop/Python +/// half. On Android there is no such file. +library; + +import 'package:crdt_sync/crdt_sync.dart'; +import 'package:flutter_secure_storage/flutter_secure_storage.dart'; + +/// The shared `kuhy-syncs` project. +/// +/// `databaseUrl` is the **regional** host. The plain `*.firebaseio.com` form +/// answers 404 with a `correctUrl` body rather than an obvious error, which +/// reads like an auth failure and wastes a debugging session. +const kProject = FirebaseProject( + apiKey: 'AIzaSyCF_sA3xCMehAYXK8eND-rAygb9NXXW_8E', + databaseUrl: + 'https://kuhy-syncs-default-rtdb.europe-west1.firebasedatabase.app', +); + +/// Same options the GitHub token uses: off Android's deprecated +/// `encryptedSharedPreferences` path, and libsecret on Linux. +const _secure = FlutterSecureStorage(); + +// Everything below reaches the OS keystore through a platform channel, which +// `flutter test` has no binding for -- the same reason `main.dart` and +// `openRepository()` are excluded. The logic these wrap (parsing, the +// public/private split, sign-in) lives in `crdt_sync` and is covered there at +// 100%; what is left here is the two-line adapter. +// coverage:ignore-start + +/// The keystore-backed home for the Firebase refresh token. +SecureCredentialStore credentialStore() => SecureCredentialStore( + read: (key) => _secure.read(key: key), + write: (key, value) => _secure.write(key: key, value: value), + delete: (key) => _secure.delete(key: key), +); + +/// Reads the per-device account, or null when sync has not been set up. +Future loadAccount() async { + try { + return FirebaseAccount.tryParse( + await _secure.read(key: kFirebaseAccountKey), + ); + } on Object { + // No secret service available: behave as "not configured" rather than + // crashing the settings screen. + return null; + } +} + +/// Stores the per-device account. Keystore only — never prefs, never source. +Future saveAccount(FirebaseAccount account) => + _secure.write(key: kFirebaseAccountKey, value: account.toJsonString()); + +/// Forgets the account and any cached session. +Future clearAccount() async { + await _secure.delete(key: kFirebaseAccountKey); + await credentialStore().clear(); +} + +/// Returns a signed-in Firebase client, or null when not configured. +/// +/// Signs in with the stored password only when there is no cached refresh +/// token, so the usual path costs no authentication round trip. +Future openFirebase() async { + final account = await loadAccount(); + if (account == null) return null; + try { + return await firebaseClientFor( + config: kProject.configFor(account.email), + store: credentialStore(), + password: account.password, + ); + } on Object { + // Unreachable keystore or a rejected sign-in must not fail the tick: the + // app keeps syncing over GitHub, exactly as before the cutover. + // Broader than Exception because an uninitialised platform binding + // raises a FlutterError, which is an Error. + return null; + } +} + +// coverage:ignore-end + +/// Wraps [github] so Firebase is primary and GitHub is kept as a mirror. +/// +/// Returns [github] unchanged when this device has not been set up for +/// Firebase — a normal state during the cutover, not an error, and also the +/// rollback. Reads union both, so a device that has not moved yet still +/// converges. +Future syncBackend(RemoteStore github) async { + final firebase = await openFirebase(); + return firebase == null + ? github + : MirrorStore(primary: firebase, mirror: github); +} diff --git a/app/lib/services/sync_service.dart b/app/lib/services/sync_service.dart index 4590159..7243fbc 100644 --- a/app/lib/services/sync_service.dart +++ b/app/lib/services/sync_service.dart @@ -20,6 +20,7 @@ import 'package:diet_guard_app/services/foodbank_service.dart'; import 'package:diet_guard_app/services/github_client_factory.dart'; import 'package:diet_guard_app/services/log_storage_service.dart'; import 'package:diet_guard_app/services/sync_merge.dart'; +import 'package:diet_guard_app/services/sync_state_factory.dart'; const _devicesDir = 'diet-guard-sync/devices'; @@ -28,7 +29,7 @@ const _devicesDir = 'diet-guard-sync/devices'; /// Returns the merged log as it now sits on disk locally. Propagates any /// [GitHubSyncError] from the client for the caller (auto-sync / the manual /// "Sync now" action) to decide how to report. -Future runSync(GitHubClient client) async { +Future runSync(RemoteStore client, {SyncStateStore? stateStore}) async { final logService = LogStorageService.instance; final local = await logService.readLog(); @@ -41,6 +42,11 @@ Future runSync(GitHubClient client) async { decode: parseRemoteLog, filename: 'food_log.json', commitMessage: 'diet_guard_app sync', + // Without this every sync re-downloads every peer's whole food log -- + // hundreds of KB -- whether or not anything changed, and re-pushes an + // unchanged one. That is the traffic the Firebase free tier's monthly + // budget depends on not happening. + stateStore: stateStore ?? await openSyncStateStore(), ); final merged = logToDayLog(mergedLog); @@ -58,7 +64,7 @@ Future runSync(GitHubClient client) async { /// Runs after the local rebuild above, so this device's records already /// reflect the merged log; the merge then unions in whatever another device /// knows, max-count winning per food. Mirrors `_sync._sync_food_bank`. -Future _syncFoodBank(GitHubClient client) async { +Future _syncFoodBank(RemoteStore client) async { final merged = await syncLog( client: client, deviceId: syncDeviceId, @@ -78,7 +84,7 @@ Future _syncFoodBank(GitHubClient client) async { /// the food log, so unlike `food_bank.json` they need a real merge: /// last-writer-wins per food name by edit time, union across devices. Mirrors /// `_sync._sync_manual_bank`. -Future _syncManualBank(GitHubClient client) async { +Future _syncManualBank(RemoteStore client) async { final local = await FoodBankService.instance.readManualBank(); final merged = await syncLog( client: client, @@ -102,7 +108,7 @@ Future _syncManualBank(GitHubClient client) async { /// [AppSettingsService.dailyKcalGoalUpdatedAt]), so a fresh install's /// unset 2200 default can never spuriously outrank a real budget synced /// from elsewhere. -Future _syncBudget(GitHubClient client) async { +Future _syncBudget(RemoteStore client) async { final updatedAt = AppSettingsService.dailyKcalGoalUpdatedAt; final localRecord = updatedAt == null ? null diff --git a/app/lib/services/sync_state_factory.dart b/app/lib/services/sync_state_factory.dart new file mode 100644 index 0000000..528ae67 --- /dev/null +++ b/app/lib/services/sync_state_factory.dart @@ -0,0 +1,9 @@ +/// Platform entry point for the sync revision cache. +/// +/// A conditional export because `dart:io` cannot even be *imported* in a web +/// compile: Android gets a file, the Chrome-wrapper desktop build gets a +/// `SharedPreferences` entry. +library; + +export 'sync_state_factory_io.dart' + if (dart.library.js_interop) 'sync_state_factory_web.dart'; diff --git a/app/lib/services/sync_state_factory_io.dart b/app/lib/services/sync_state_factory_io.dart new file mode 100644 index 0000000..9a33dda --- /dev/null +++ b/app/lib/services/sync_state_factory_io.dart @@ -0,0 +1,37 @@ +import 'dart:io'; + +import 'package:crdt_sync/crdt_sync.dart'; +import 'package:crdt_sync/crdt_sync_io.dart'; +import 'package:path/path.dart' as p; +import 'package:path_provider/path_provider.dart'; + +/// File holding the revision cache, beside the food log it describes. +const kSyncStateFileName = 'sync_state.json'; + +/// Opens the revision cache on a `dart:io` platform (Android). +/// +/// Falls back to an in-memory cache when the platform channel is unavailable +/// (no binding under `flutter test`, or a plugin that failed to register). +/// Losing the cache costs one tick of extra traffic; failing here would take +/// sync down entirely, which is far worse than the thing being optimised. +Future openSyncStateStore() async { + try { + final dir = await getApplicationSupportDirectory(); + return openSyncStateStoreIn(dir.path); + } on Object { + // Deliberately broader than Exception: an uninitialised binding raises a + // FlutterError, which is an Error. Either way the platform is + // unavailable and an in-memory cache is the right degradation. + return InMemorySyncStateStore(); + } +} + +/// Opens the revision cache rooted at [dirPath]. +/// +/// Lives next to the log it describes and must be cleared with it: skipping +/// an unchanged peer is only sound because that peer's records are already +/// merged into the local log, so state that outlived its log would skip peers +/// whose data had been lost. +SyncStateStore openSyncStateStoreIn(String dirPath) => PersistedSyncStateStore( + FileLogPersistence(File(p.join(dirPath, kSyncStateFileName))), +); diff --git a/app/lib/services/sync_state_factory_web.dart b/app/lib/services/sync_state_factory_web.dart new file mode 100644 index 0000000..3a3abed --- /dev/null +++ b/app/lib/services/sync_state_factory_web.dart @@ -0,0 +1,27 @@ +import 'package:crdt_sync/crdt_sync.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +/// Key holding the revision cache in `SharedPreferences`. +const kSyncStateKey = 'sync.state'; + +/// Opens the revision cache on web (the Chrome-wrapper desktop build). +/// +/// `SharedPreferences` rather than IndexedDB: the payload is two short hashes +/// per peer, so a key-value entry is the whole requirement, and the inventory +/// log already owns the IndexedDB store. +Future openSyncStateStore() async => PersistedSyncStateStore( + _PrefsPersistence(await SharedPreferences.getInstance()), +); + +class _PrefsPersistence implements LogPersistence { + _PrefsPersistence(this._prefs); + + final SharedPreferences _prefs; + + @override + Future read() async => _prefs.getString(kSyncStateKey); + + @override + Future write(String text) async => + _prefs.setString(kSyncStateKey, text); +} diff --git a/app/pubspec.lock b/app/pubspec.lock index c0bec70..6617784 100644 --- a/app/pubspec.lock +++ b/app/pubspec.lock @@ -85,11 +85,11 @@ packages: dependency: "direct main" description: path: crdt_sync_dart - ref: "crdt_sync_dart-v0.3.0" - resolved-ref: "5480e723354c31e2a38dd2757d5d2c75a04e9618" + ref: "crdt_sync_dart-v0.6.0" + resolved-ref: fc99eff00599a4c3195e4c1573ba08f15909a10d url: "https://github.com/kuhyx/utils" source: git - version: "0.3.0" + version: "0.6.0" crypto: dependency: "direct main" description: diff --git a/app/pubspec.yaml b/app/pubspec.yaml index b708d3a..0697370 100644 --- a/app/pubspec.yaml +++ b/app/pubspec.yaml @@ -11,7 +11,7 @@ dependencies: crdt_sync: git: url: https://github.com/kuhyx/utils - ref: crdt_sync_dart-v0.3.0 + ref: crdt_sync_dart-v0.6.0 path: crdt_sync_dart crypto: ^3.0.6 flutter: diff --git a/app/test/services/background_sync_service_test.dart b/app/test/services/background_sync_service_test.dart index 25004c6..fb0c6aa 100644 --- a/app/test/services/background_sync_service_test.dart +++ b/app/test/services/background_sync_service_test.dart @@ -82,7 +82,9 @@ void main() { expect(ok, isTrue); // syncLog always pushes, even an empty merged result: food_log.json, // budget.json, food_bank.json, food_bank_manual.json. - expect(puts, 4); + // Four data files plus this device's revision, which is what lets a + // later tick skip an unchanged peer. + expect(puts, 5); }); test('reports failure (retry) when the push errors', () async { diff --git a/app/test/services/sync_service_test.dart b/app/test/services/sync_service_test.dart index 5b547a9..48bc749 100644 --- a/app/test/services/sync_service_test.dart +++ b/app/test/services/sync_service_test.dart @@ -145,8 +145,10 @@ void main() { expect(merged.values.expand((e) => e).length, 1); // syncLog always pushes, even an empty merged result: food_log.json, - // budget.json, food_bank.json, food_bank_manual.json. - expect(fake.puts, hasLength(4)); + // budget.json, food_bank.json, food_bank_manual.json -- plus this + // device's revision, published after the log so a peer can never cache + // "seen rev X" against a log it never received. + expect(fake.puts, hasLength(5)); }); test("skips its own device id ('phone') when listing", () async { diff --git a/app/test/services/sync_state_factory_test.dart b/app/test/services/sync_state_factory_test.dart new file mode 100644 index 0000000..7600947 --- /dev/null +++ b/app/test/services/sync_state_factory_test.dart @@ -0,0 +1,89 @@ +/// Tests for the revision cache and the backend choice. +/// +/// The cache is what keeps a sync tick from re-downloading every peer's whole +/// food log when nothing changed, so "does it survive a restart" and "does a +/// dead platform channel take sync down with it" are the behaviours worth +/// pinning. +@TestOn('vm') +library; + +import 'dart:io'; + +import 'package:crdt_sync/crdt_sync.dart'; +import 'package:diet_guard_app/services/firebase_backend.dart'; +import 'package:diet_guard_app/services/sync_state_factory_io.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:path/path.dart' as p; + +/// A do-nothing [RemoteStore], standing in for the GitHub client. +class _StubRemote implements RemoteStore { + @override + Future> listDirectory(String path) async => []; + + @override + Future getFileText(String path) async => null; + + @override + Future putFileText( + String path, + String text, { + required String message, + }) async {} + + @override + Future deleteFile(String path, {String message = ''}) async {} + + @override + Future canAccessRemote() async => true; + + @override + void close() {} +} + +void main() { + late Directory dir; + + setUp(() async { + dir = await Directory.systemTemp.createTemp('dg_state_'); + }); + + tearDown(() async { + if (dir.existsSync()) await dir.delete(recursive: true); + }); + + test('persists revisions across a restart', () async { + // A fresh store instance stands in for the next app launch: an in-memory + // cache would forget every peer and re-download all of them. + await openSyncStateStoreIn(dir.path).save( + const SyncState(pushedRev: 'mine', peerRevs: {'pc': 'theirs'}), + ); + + final reloaded = await openSyncStateStoreIn(dir.path).load(); + + expect(reloaded.pushedRev, 'mine'); + expect(reloaded.peerRevs, {'pc': 'theirs'}); + }); + + test('writes beside the log it describes', () async { + await openSyncStateStoreIn(dir.path).save(const SyncState(pushedRev: 'x')); + + expect(File(p.join(dir.path, kSyncStateFileName)).existsSync(), isTrue); + }); + + test('degrades to memory when the platform channel is unavailable', () async { + // No binding under `flutter test`. Losing the cache costs one tick of + // extra traffic; failing would take sync down entirely. + final store = await openSyncStateStore(); + + expect(store, isA()); + expect((await store.load()).pushedRev, isNull); + }); + + test('stays on GitHub when Firebase is not set up', () async { + // The pre-migration path, and the rollback. Reaching the keystore without + // a binding must read as "not configured", not as an error. + final github = _StubRemote(); + + expect(await syncBackend(github), same(github)); + }); +} diff --git a/diet_guard/_constants.py b/diet_guard/_constants.py index 16806ae..625c057 100644 --- a/diet_guard/_constants.py +++ b/diet_guard/_constants.py @@ -21,6 +21,12 @@ BUDGET_WARN_FRACTION: float = 0.80 # is deliberately NOT committed to git (unlike wake_state.json). DATA_DIR: Path = Path.home() / ".local" / "share" / "diet_guard" FOOD_LOG_FILE: Path = DATA_DIR / "food_log.json" + +# Revision cache for cross-device sync. Beside the log it describes and +# cleared with it: skipping an unchanged peer is only sound because that +# peer's records are already merged into the local log, so state that outlived +# its log would skip peers whose data had been lost. +SYNC_STATE_FILE: Path = DATA_DIR / "sync_state.json" # The user's personal "food bank": every food they have logged before, with its # full macros, keyed by name. This is the ONLY corpus the gate's autocomplete # searches -- Open Food Facts is used to *fill* a new food's macros, never to diff --git a/diet_guard/_sync.py b/diet_guard/_sync.py index 3773039..ddf6979 100644 --- a/diet_guard/_sync.py +++ b/diet_guard/_sync.py @@ -19,7 +19,21 @@ from __future__ import annotations import json import logging -from crdt_sync import GitHubSyncClient, GitHubSyncError, Log, merge_logs +from crdt_sync import ( + CONFIG_FILE, + ConfigError, + FileSyncStateStore, + FirebaseAuthError, + GitHubSyncClient, + GitHubSyncError, + Log, + RemoteStore, + RemoteSyncError, + SyncState, + merge_logs, + mirror_client_for, + revision_of, +) from diet_guard._budget import read_raw_record, write_raw_record from diet_guard._budget_history import ( @@ -31,6 +45,7 @@ from diet_guard._constants import ( SYNC_DEVICE_ID, SYNC_REPO_NAME, SYNC_REPO_OWNER, + SYNC_STATE_FILE, SYNC_TIMEOUT_SECONDS, SYNC_TOKEN_FILE, ) @@ -56,12 +71,43 @@ from diet_guard._sync_merge import ( _logger = logging.getLogger(__name__) _DEVICES_DIR = "diet-guard-sync/devices" +# One small text node per device, each written only by its owner. Deliberately +# not one shared map: a whole-map write would erase every other device's entry, +# after which those peers would look permanently unchanged and never be +# fetched again. +_REVS_DIR = "diet-guard-sync/revs" class SyncError(Exception): """Raised when a sync run cannot even start (no usable PAT).""" +def _remote_client(github: GitHubSyncClient) -> RemoteStore: + """Return the backend to sync against. + + Firebase when ``~/.config/crdt-sync/`` is set up, with GitHub kept as a + mirror so a device that has not moved yet still converges; GitHub alone + otherwise. An unconfigured machine keeps syncing exactly as before. + + Every sub-sync here (log, budget, food bank, manual bank) takes the client + as a parameter, so this one swap covers all of them. + + The config file is checked before constructing anything, so an + unconfigured machine never reaches the network -- otherwise a suite that + blocks real sockets fails here rather than in the code under test. + + Rolling back is deleting this function and passing ``github`` straight + through: no data moves either way. + """ + if not CONFIG_FILE.is_file(): + return github + try: + return mirror_client_for("diet_guard", github) + except (ConfigError, FirebaseAuthError, RemoteSyncError) as exc: + _logger.warning("Firebase unavailable, syncing via GitHub only: %s", exc) + return github + + def _device_log_path(device_id: str) -> str: """Return the repo-relative path a device's full log is pushed to.""" return f"{_DEVICES_DIR}/{device_id}/food_log.json" @@ -103,7 +149,30 @@ def _read_token() -> str: return token -def _pull_remote_logs(client: GitHubSyncClient) -> list[Log]: +def _remote_revisions(client: RemoteStore) -> dict[str, str]: + """Return every peer's published food-log revision, cheaply where possible. + + Degrades to an empty map -- meaning "fetch everything", the old behaviour + -- on a backend without a bulk-map read, so correctness never depends on + the optimisation being available. + """ + get_string_map = getattr(client, "get_string_map", None) + if get_string_map is None: + return {} + try: + return get_string_map(_REVS_DIR) + except (GitHubSyncError, RemoteSyncError): + # A revision map that cannot be read is not worth failing a sync + # over; without it every peer is simply fetched, as before. + return {} + + +def _pull_remote_logs( + client: RemoteStore, + remote_revs: dict[str, str], + state: SyncState, + seen_revs: dict[str, str], +) -> list[Log]: """Return every other device's last-pushed log, skipping this one. A device whose pushed file is corrupt, truncated, or otherwise @@ -111,18 +180,33 @@ def _pull_remote_logs(client: GitHubSyncClient) -> list[Log]: that has never pushed at all -- GitHub is an external system boundary, and one bad device's file must not stall merging in every other device's. + + A peer whose published revision matches the one already merged is skipped + without downloading it at all. This is the single largest traffic saving + in the fleet: these logs are hundreds of KB and this timer runs 96 times a + day, so re-reading an unchanged peer is pure waste. """ remote_logs: list[Log] = [] for device_id in client.list_directory(_DEVICES_DIR): if device_id == SYNC_DEVICE_ID: continue + remote_rev = remote_revs.get(device_id) + if remote_rev is not None and remote_rev == state.peer_revs.get(device_id): + # Already merged, and that merge is in the local log. Carry the + # revision forward so it stays skipped next tick. + seen_revs[device_id] = remote_rev + continue text = client.get_file_text(_device_log_path(device_id)) if text is None: continue try: remote_logs.append(parse_remote_log(text)) except (TypeError, KeyError, ValueError, json.JSONDecodeError): + # Deliberately not recorded as seen: a corrupt push must be + # retried next tick, not remembered as merged. _logger.warning("Unparsable log pushed by device %r, skipping", device_id) + continue + seen_revs[device_id] = remote_rev or revision_of(text) return remote_logs @@ -289,12 +373,22 @@ def run_sync() -> DayLog: report it. """ token = _read_token() - client = GitHubSyncClient( - SYNC_REPO_OWNER, SYNC_REPO_NAME, token, timeout_seconds=SYNC_TIMEOUT_SECONDS + client = _remote_client( + GitHubSyncClient( + SYNC_REPO_OWNER, + SYNC_REPO_NAME, + token, + timeout_seconds=SYNC_TIMEOUT_SECONDS, + ) ) + state_store = FileSyncStateStore(SYNC_STATE_FILE) + state = state_store.load() + remote_revs = _remote_revisions(client) + seen_revs: dict[str, str] = {} + merged = daylog_to_log(read_raw_log()) - for remote_log in _pull_remote_logs(client): + for remote_log in _pull_remote_logs(client, remote_revs, state, seen_revs): merged = merge_logs(merged, remote_log) merged_daylog = log_to_daylog(merged) @@ -313,11 +407,21 @@ def run_sync() -> DayLog: {record_id: record.to_dict() for record_id, record in push_log.items()}, indent=2, ) - client.put_file_text( - _device_log_path(SYNC_DEVICE_ID), - push_json, - message="diet_guard sync", - ) + revision = revision_of(push_json) + if revision != state.pushed_rev: + client.put_file_text( + _device_log_path(SYNC_DEVICE_ID), + push_json, + message="diet_guard sync", + ) + # Published after the log, never before: a peer that cached "seen rev + # X" against a log it never received would skip it forever. + client.put_file_text( + f"{_REVS_DIR}/{SYNC_DEVICE_ID}", + revision, + message="diet_guard sync: revision", + ) + state_store.save(SyncState(pushed_rev=revision, peer_revs=seen_revs)) return resigned diff --git a/diet_guard/tests/conftest.py b/diet_guard/tests/conftest.py index a63e37e..6d7c22b 100644 --- a/diet_guard/tests/conftest.py +++ b/diet_guard/tests/conftest.py @@ -116,6 +116,17 @@ def _isolate_state(tmp_path: Path) -> Iterator[None]: "diet_guard._sync.SYNC_TOKEN_FILE", tmp_path / "sync_token", ), + patch( + "diet_guard._sync.SYNC_STATE_FILE", + tmp_path / "sync_state.json", + ), + # `run_sync` reads this to decide whether to build a Firebase-primary + # mirror. On a developer machine the real file exists, so without this + # every sync test would sign in and push to the live database. + patch( + "diet_guard._sync.CONFIG_FILE", + tmp_path / "nonexistent-firebase.json", + ), ): yield diff --git a/diet_guard/tests/test_sync.py b/diet_guard/tests/test_sync.py index 43b4ea9..3ac9f75 100644 --- a/diet_guard/tests/test_sync.py +++ b/diet_guard/tests/test_sync.py @@ -8,8 +8,10 @@ The GitHub layer is mocked (no network access); conftest.py's from __future__ import annotations import json +from pathlib import Path from unittest.mock import MagicMock, patch +from crdt_sync import ConfigError, GitHubSyncError, SyncState import pytest from diet_guard import _sync @@ -58,6 +60,11 @@ def _mock_client( client.list_directory.return_value = list(devices) resolved_files = files or {} client.get_file_text.side_effect = resolved_files.get + # The real GitHubSyncClient has no bulk-map read, and a MagicMock would + # otherwise auto-create one returning a Mock -- which then fails to + # serialise. Deleting it also exercises the documented degrade path: + # no revision map means every peer is fetched, exactly as before. + del client.get_string_map return client @@ -94,14 +101,20 @@ class TestRunSync: assert sum(len(entries) for entries in merged.values()) == 1 pushed = [call.args[0] for call in client.put_file_text.call_args_list] - # The log push, plus the derived bank rebuilt from that same log. + # The log push, the derived bank rebuilt from that same log, and this + # device's revision -- published *after* the log, so a peer can never + # cache "seen rev X" against a log it never received. assert pushed == [ "diet-guard-sync/devices/pc/food_bank.json", "diet-guard-sync/devices/pc/food_log.json", + "diet-guard-sync/revs/pc", ] - pushed_path = client.put_file_text.call_args.args[0] - assert pushed_path == "diet-guard-sync/devices/pc/food_log.json" - pushed_json = client.put_file_text.call_args.args[1] + log_call = next( + call + for call in client.put_file_text.call_args_list + if call.args[0].endswith("food_log.json") + ) + pushed_json = log_call.args[1] pushed = json.loads(pushed_json) (record,) = pushed.values() assert "fields" in record @@ -357,3 +370,122 @@ class TestPullSharedLog: pytest.raises(KeyError), ): _sync.pull_shared_log() + + +class TestRemoteClient: + """Which backend a sync tick runs against during the cutover.""" + + def test_stays_on_github_without_firebase_config( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """An unconfigured machine must not reach the network at all.""" + monkeypatch.setattr(_sync, "CONFIG_FILE", Path("/nonexistent/firebase.json")) + github = object() + + assert _sync._remote_client(github) is github + + def test_mirrors_to_github_when_configured( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Configured: Firebase is primary, GitHub keeps receiving writes.""" + config = tmp_path / "firebase.json" + config.write_text("{}", encoding="utf-8") + monkeypatch.setattr(_sync, "CONFIG_FILE", config) + monkeypatch.setattr( + _sync, "mirror_client_for", lambda _app, client: ("mirror", client) + ) + github = object() + + assert _sync._remote_client(github) == ("mirror", github) + + def test_falls_back_when_firebase_is_unusable( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A broken Firebase must degrade to GitHub, never fail the tick.""" + config = tmp_path / "firebase.json" + config.write_text("{}", encoding="utf-8") + monkeypatch.setattr(_sync, "CONFIG_FILE", config) + + def _boom(*_args: object, **_kwargs: object) -> None: + message = "no password" + raise ConfigError(message) + + monkeypatch.setattr(_sync, "mirror_client_for", _boom) + github = object() + + assert _sync._remote_client(github) is github + + +class TestRemoteRevisions: + """Reading the peers' revision map, which gates the big downloads.""" + + def test_is_empty_without_a_bulk_map_read(self) -> None: + """GitHub has none; correctness must not depend on the optimisation.""" + client = MagicMock() + del client.get_string_map + + assert _sync._remote_revisions(client) == {} + + def test_returns_the_published_revisions(self) -> None: + client = MagicMock() + client.get_string_map.return_value = {"phone": "abc"} + + assert _sync._remote_revisions(client) == {"phone": "abc"} + + def test_an_unreadable_map_degrades_to_fetching_everything(self) -> None: + """Not worth failing a sync over; every peer is simply fetched.""" + client = MagicMock() + client.get_string_map.side_effect = GitHubSyncError("boom") + + assert _sync._remote_revisions(client) == {} + + +class TestPullSkipsUnchangedPeers: + """The single largest traffic saving in the fleet.""" + + def test_skips_a_peer_whose_revision_is_unchanged(self) -> None: + client = _mock_client(devices=("phone",)) + state = SyncState(pushed_rev=None, peer_revs={"phone": "rev-1"}) + seen: dict[str, str] = {} + + logs = _sync._pull_remote_logs(client, {"phone": "rev-1"}, state, seen) + + assert logs == [] + assert seen == {"phone": "rev-1"} + client.get_file_text.assert_not_called() + + def test_downloads_a_peer_whose_revision_moved(self) -> None: + text = json.dumps({}) + client = _mock_client( + devices=("phone",), + files={"diet-guard-sync/devices/phone/food_log.json": text}, + ) + state = SyncState(pushed_rev=None, peer_revs={"phone": "rev-1"}) + seen: dict[str, str] = {} + + _sync._pull_remote_logs(client, {"phone": "rev-2"}, state, seen) + + client.get_file_text.assert_called_once() + assert seen == {"phone": "rev-2"} + + +class TestNoOpPushSuppression: + """88% of the old GitHub history was byte-identical no-op pushes.""" + + def test_a_second_unchanged_sync_pushes_no_log(self) -> None: + """The saving the free-tier budget depends on, at 96 ticks a day.""" + _write_token() + client = _mock_client() + + with patch.object(_sync, "GitHubSyncClient", return_value=client): + _sync.run_sync() + pushed_first = [ + call.args[0] for call in client.put_file_text.call_args_list + ] + client.put_file_text.reset_mock() + _sync.run_sync() + + pushed_second = [call.args[0] for call in client.put_file_text.call_args_list] + assert "diet-guard-sync/devices/pc/food_log.json" in pushed_first + assert "diet-guard-sync/devices/pc/food_log.json" not in pushed_second + assert "diet-guard-sync/revs/pc" not in pushed_second