diff --git a/CLAUDE.md b/CLAUDE.md index 29ccd6b..5b5fab6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -76,11 +76,28 @@ auto-start. `_sync.py`'s local cache instead of the remote file, so there is no second remote reader to keep in step. -- Source of truth: `wake-alarm-sync/devices/phone/alarm.json`, written by - `phone_app/` as a `crdt_sync` log. The value lives at +- Source of truth: a `crdt_sync` log written by `phone_app/` under + `wake-alarm-sync/devices//alarm.json`. The value lives at `.alarm.fields.alarm_hour[0]` / `.alarm_minute[0]`. **The same logical path on both backends** — Firebase's namespace mirrors the repo layout — so only the transport differs, never the parse. +- **The Firebase reader discovers device directories; it is not pinned to + `devices/phone/`.** The phone's device id is migrating from the fixed + constant `phone` to a per-install uuid, and a reader pinned to the old path + would silently stop seeing the alarm the moment the phone migrated — then + fall back to `FALLBACK_ALARM_HOUR`, i.e. wake at the wrong time. So + `_fetch_firebase_alarm()` lists `SYNC_DEVICES_PREFIX`, reads every device's + `alarm.json`, and picks the newest by the hour field's **HLC** (not by + listing order), with `device_id` breaking exact ties so two devices stamped + in the same millisecond don't alternate between ticks. A device whose + payload doesn't parse is skipped rather than allowed to win. + `SYNC_ALARM_PATH` remains as a last-resort read for a backend that lists + nothing — keep it until every install has migrated. The **GitHub** fallback + still reads the single fixed path; it is the mirror, not the primary. +- Payload parsing/ranking lives in `_alarm_payload.py`, split out of `_sync.py` + so the wire format is testable without network or disk (and to keep both + files under the 500-line cap). `_sync.py` owns fetching, caching and + fallback; `_alarm_payload.py` owns "what did the phone actually say". - **Firebase first, GitHub as the cutover mirror** (`fetch_remote_alarm()` is just `_fetch_firebase_alarm() or _fetch_github_alarm()`). Retiring the mirror is deleting the GitHub branch plus `SYNC_API_BASE`/`SYNC_REPO_*`. diff --git a/phone_app/lib/services/alarm_sync_service.dart b/phone_app/lib/services/alarm_sync_service.dart index 1f9e130..60031eb 100644 --- a/phone_app/lib/services/alarm_sync_service.dart +++ b/phone_app/lib/services/alarm_sync_service.dart @@ -5,6 +5,8 @@ import 'package:crdt_sync/crdt_sync_io.dart'; import 'package:http/http.dart' as http; import 'package:path/path.dart' as p; import 'package:path_provider/path_provider.dart'; +import 'package:shared_preferences/shared_preferences.dart'; +import 'package:uuid/uuid.dart'; import 'package:wake_alarm_sync/services/firebase_backend.dart'; /// The alarm time this device holds, as read from or written to the store. @@ -15,18 +17,24 @@ typedef Alarm = ({int hour, int minute}); /// /// The phone is the **sole writer**: it stores the chosen alarm locally in a /// [LogStore] (one HLC-stamped [Record], id [_recordId]) and pushes that log -/// to `wake-alarm-sync/devices/phone/alarm.json` — Firebase first, mirrored -/// to the private `kuhyx/syncs` repo during the cutover (see [syncBackend]). -/// The path is deliberately identical on both backends, so only the transport -/// differs. +/// to `wake-alarm-sync/devices//alarm.json` — Firebase first, +/// mirrored to the private `kuhyx/syncs` repo during the cutover (see +/// [syncBackend]). The path is deliberately identical on both backends, so +/// only the transport differs. +/// +/// [deviceId] is a per-install uuid, not the fixed `phone` constant this app +/// shipped with ([legacyDeviceId]). The PC reader lists device directories +/// and ranks their logs by HLC rather than reading one hardcoded path, so a +/// migrating install is picked up automatically and the newest alarm wins +/// regardless of which directory it landed in. /// /// The PC side is read-only (`wake_alarm/_sync.py`, which tries Firebase then /// GitHub, caching the result for `shutdown-wrapper.sh`). There is no /// pull/merge here: the phone is the only writer, so its local store is /// always the source of truth. class AlarmSyncService { - /// Wraps an already-loaded log store. - AlarmSyncService(this._store); + /// Wraps an already-loaded log store pushing under [deviceId]. + AlarmSyncService(this._store, {required this.deviceId}); final LogStore _store; @@ -36,24 +44,63 @@ class AlarmSyncService { /// Repo name within [owner] that holds every app's `*-sync/` data. static const repo = 'syncs'; - /// This device's id in the `devices//` layout. The phone is the only - /// writer, so a constant id is enough (HLCs still order re-installs). - static const deviceId = 'phone'; + /// The device id this install used before migrating to a persisted uuid. + /// + /// The PC reader still reads `devices//` as a last resort, + /// so an install that has not migrated keeps working; see + /// `wake_alarm/_constants.py`'s `SYNC_ALARM_PATH`. + static const legacyDeviceId = 'phone'; + + /// SharedPreferences key holding this install's persisted device id. + /// + /// Named to match the sibling apps (`todo`, `home_inventory`), which store + /// the same thing under the same key. + static const kDeviceIdKey = 'crdt.nodeId'; + + /// This device's id in the `devices//` layout. + /// + /// A per-install uuid rather than the old fixed `phone` constant: two + /// installs sharing an id overwrite each other's pushed file on every + /// tick, and a reinstall would silently inherit the previous install's + /// CRDT identity. The PC reader discovers device directories, so it finds + /// whatever uuid this turns out to be. + final String deviceId; + + String get _remotePath => 'wake-alarm-sync/devices/$deviceId/alarm.json'; - static const _remotePath = 'wake-alarm-sync/devices/$deviceId/alarm.json'; static const _recordId = 'alarm'; static const _hourField = 'alarm_hour'; static const _minuteField = 'alarm_minute'; + /// Returns this install's device id, minting and persisting one if absent. + /// + /// Migrating installs keep publishing real alarms throughout: the id only + /// changes which directory the push lands in, and the PC ranks every + /// device's log by HLC, so the newest alarm wins whichever path it is under. + static Future loadDeviceId() async { + final prefs = await SharedPreferences.getInstance(); + final existing = prefs.getString(kDeviceIdKey); + if (existing != null && existing.isNotEmpty) return existing; + final minted = const Uuid().v4(); + await prefs.setString(kDeviceIdKey, minted); + return minted; + } + /// Opens the on-disk alarm store. /// /// [persistence] is injectable so tests can supply an in-memory backend - /// instead of touching the real app-documents directory. - static Future open({LogPersistence? persistence}) async { + /// instead of touching the real app-documents directory. [deviceId] is + /// injectable for the same reason; production resolves it via + /// [loadDeviceId]. + static Future open({ + LogPersistence? persistence, + String? deviceId, + }) async { final backend = persistence ?? await _defaultPersistence(); - final store = LogStore(persistence: backend, nodeId: deviceId); + final id = deviceId ?? await loadDeviceId(); + final store = LogStore(persistence: backend, nodeId: id); await store.load(); - return AlarmSyncService(store); + return AlarmSyncService(store, deviceId: id); } static Future _defaultPersistence() async { diff --git a/phone_app/pubspec.lock b/phone_app/pubspec.lock index 0e0057d..6850c5b 100644 --- a/phone_app/pubspec.lock +++ b/phone_app/pubspec.lock @@ -138,6 +138,14 @@ packages: url: "https://pub.dev" source: hosted version: "7.0.1" + fixnum: + dependency: transitive + description: + name: fixnum + sha256: b6dc7065e46c974bc7c5f143080a6764ec7a4be6da1285ececdc37be96de53be + url: "https://pub.dev" + source: hosted + version: "1.1.1" flutter: dependency: "direct main" description: flutter @@ -630,6 +638,14 @@ packages: url: "https://pub.dev" source: hosted version: "3.1.5" + uuid: + dependency: "direct main" + description: + name: uuid + sha256: "9b129329f58692f6e6578329498a8fe9fbe98f090beb764ffbb8ee2eadd01dcd" + url: "https://pub.dev" + source: hosted + version: "4.6.0" vector_math: dependency: transitive description: diff --git a/phone_app/pubspec.yaml b/phone_app/pubspec.yaml index adca658..546b440 100644 --- a/phone_app/pubspec.yaml +++ b/phone_app/pubspec.yaml @@ -23,6 +23,7 @@ dependencies: path_provider: ^2.1.0 shared_preferences: ^2.3.0 url_launcher: ^6.3.0 + uuid: ^4.6.0 dev_dependencies: # Generates the Android launcher icons from assets/icon/ (see diff --git a/phone_app/test/services/alarm_sync_service_test.dart b/phone_app/test/services/alarm_sync_service_test.dart index 4100092..da08a3a 100644 --- a/phone_app/test/services/alarm_sync_service_test.dart +++ b/phone_app/test/services/alarm_sync_service_test.dart @@ -4,9 +4,12 @@ import 'package:crdt_sync/crdt_sync.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:http/http.dart' as http; import 'package:http/testing.dart' as http_testing; +import 'package:shared_preferences/shared_preferences.dart'; +import 'package:uuid/uuid.dart'; import 'package:wake_alarm_sync/services/alarm_sync_service.dart'; -const _alarmPath = 'wake-alarm-sync/devices/phone/alarm.json'; +const _deviceId = 'test-device-uuid'; +const _alarmPath = 'wake-alarm-sync/devices/$_deviceId/alarm.json'; http.Response _response(int statusCode, [Object? jsonBody]) => http.Response(jsonEncode(jsonBody ?? {}), statusCode); @@ -56,23 +59,35 @@ Map _decodePush(_PutCall call) => void main() { test('currentAlarm is null before anything is set', () async { - final service = await AlarmSyncService.open(persistence: _MemPersistence()); + final service = await AlarmSyncService.open( + persistence: _MemPersistence(), + deviceId: _deviceId, + ); expect(service.currentAlarm, isNull); }); test('setLocal stores the alarm locally and survives a reopen', () async { final persistence = _MemPersistence(); - final service = await AlarmSyncService.open(persistence: persistence); + final service = await AlarmSyncService.open( + persistence: persistence, + deviceId: _deviceId, + ); await service.setLocal(6, 30); expect(service.currentAlarm, (hour: 6, minute: 30)); // The same persistence backend rehydrates the alarm on the next open. - final reopened = await AlarmSyncService.open(persistence: persistence); + final reopened = await AlarmSyncService.open( + persistence: persistence, + deviceId: _deviceId, + ); expect(reopened.currentAlarm, (hour: 6, minute: 30)); }); test('push PUTs the alarm log to the phone device path', () async { - final service = await AlarmSyncService.open(persistence: _MemPersistence()); + final service = await AlarmSyncService.open( + persistence: _MemPersistence(), + deviceId: _deviceId, + ); await service.setLocal(7, 15); final (:httpClient, :putCalls) = _mockGitHub(); @@ -88,7 +103,10 @@ void main() { }); test('push is a network round-trip that swallows a server error', () async { - final service = await AlarmSyncService.open(persistence: _MemPersistence()); + final service = await AlarmSyncService.open( + persistence: _MemPersistence(), + deviceId: _deviceId, + ); await service.setLocal(9, 0); final client = http_testing.MockClient( (request) async => http.Response('', 500), @@ -101,4 +119,63 @@ void main() { throwsA(isA()), ); }); + + group('device id', () { + setUp(() => SharedPreferences.setMockInitialValues({})); + + test('mints and persists a uuid on first run', () async { + final minted = await AlarmSyncService.loadDeviceId(); + + expect(Uuid.isValidUUID(fromString: minted), isTrue); + final prefs = await SharedPreferences.getInstance(); + expect(prefs.getString(AlarmSyncService.kDeviceIdKey), minted); + }); + + test('returns the same id on every later run', () async { + final first = await AlarmSyncService.loadDeviceId(); + final second = await AlarmSyncService.loadDeviceId(); + + expect(second, first); + }); + + test('adopts an id already stored under the shared key', () async { + SharedPreferences.setMockInitialValues({ + AlarmSyncService.kDeviceIdKey: 'pre-existing-id', + }); + + expect(await AlarmSyncService.loadDeviceId(), 'pre-existing-id'); + }); + + test('treats a blank stored id as absent and mints a new one', () async { + SharedPreferences.setMockInitialValues({ + AlarmSyncService.kDeviceIdKey: '', + }); + + expect(await AlarmSyncService.loadDeviceId(), isNotEmpty); + }); + + test('pushes under the resolved uuid, not the legacy phone path', () async { + final service = await AlarmSyncService.open( + persistence: _MemPersistence(), + deviceId: 'uuid-abc', + ); + await service.setLocal(6, 30); + final paths = []; + final client = http_testing.MockClient((request) async { + paths.add(request.url.path); + return _response(200); + }); + + await service.push('tok', httpClient: client); + + expect(paths.any((path) => path.contains('devices/uuid-abc/')), isTrue); + expect( + paths.any( + (path) => + path.contains('devices/${AlarmSyncService.legacyDeviceId}/'), + ), + isFalse, + ); + }); + }); } diff --git a/wake_alarm/_alarm_payload.py b/wake_alarm/_alarm_payload.py new file mode 100644 index 0000000..a6db82a --- /dev/null +++ b/wake_alarm/_alarm_payload.py @@ -0,0 +1,173 @@ +"""Parse the alarm out of the ``crdt_sync`` log the phone publishes. + +Split out of ``_sync.py`` so that module keeps to fetching, caching and +fallback: everything here is pure, takes text or already-parsed JSON, and +touches neither the network nor the disk. That makes the wire format testable +on its own, which matters because a misparse here does not raise — it +silently resolves to the wrong wake-up time. + +The published shape is one ``crdt_sync`` record:: + + {"alarm": {"fields": {"alarm_hour": [7, ""], "alarm_minute": [30, ""]}}} + +Every field is a ``(value, HLC)`` pair. Ranking across devices uses the HLC +rather than the value, so the alarm that wins is the one *set* most recently. +""" + +from __future__ import annotations + +import json +import logging +from typing import Any + +from wake_alarm._constants import MAX_HOUR, MAX_MINUTE, MIN_HOUR, MIN_MINUTE + +_logger = logging.getLogger(__name__) + +# Keys inside the crdt_sync log the phone writes. +RECORD_ID = "alarm" +HOUR_FIELD = "alarm_hour" +MINUTE_FIELD = "alarm_minute" + +# A crdt_sync field is a (value, HLC) pair. +_HLC_PAIR_LENGTH = 2 + + +def in_range(hour: int, minute: int) -> bool: + """Check that an (hour, minute) pair is a real wall-clock time. + + Guards both readers: a malformed sync payload and a corrupted cache must + never become a fire time. + + Args: + hour: Candidate hour. + minute: Candidate minute. + + Returns: + True if both values are within a 24-hour clock. + """ + return MIN_HOUR <= hour <= MAX_HOUR and MIN_MINUTE <= minute <= MAX_MINUTE + + +def _field_pair(log: dict[str, Any], field: str) -> list[Any] | None: + """Return the raw ``(value, HLC)`` pair for [field], if well-shaped. + + Args: + log: The parsed log JSON. + field: Field name to look up. + + Returns: + The pair as a list, or None if any level of the shape is unexpected. + """ + record = log.get(RECORD_ID) + if not isinstance(record, dict): + return None + fields = record.get("fields") + if not isinstance(fields, dict): + return None + pair = fields.get(field) + return pair if isinstance(pair, list) and pair else None + + +def extract_field(log: dict[str, Any], field: str) -> int | None: + """Pull one integer field out of the phone's crdt_sync log. + + Args: + log: The parsed log JSON. + field: Field name to extract (e.g. ``alarm_hour``). + + Returns: + The integer value, or None if anything about the shape is unexpected. + """ + pair = _field_pair(log, field) + if pair is None: + return None + value = pair[0] + # bool is an int subclass; an alarm hour of True would be nonsense. + if isinstance(value, bool) or not isinstance(value, int): + return None + return value + + +def extract_hlc(log: dict[str, Any], field: str) -> str: + """Pull one field's HLC stamp out of the phone's crdt_sync log. + + HLC strings are ``--`` and sort lexicographically + in timestamp order, which is what lets [newest_alarm] rank two devices' + publications against each other. + + Args: + log: The parsed log JSON. + field: Field name whose stamp to read. + + Returns: + The HLC string, or ``""`` if absent or the wrong shape -- which sorts + below every real stamp, so a device publishing a malformed pair never + outranks one publishing a good pair. + """ + pair = _field_pair(log, field) + if pair is None or len(pair) < _HLC_PAIR_LENGTH: + return "" + hlc = pair[1] + return hlc if isinstance(hlc, str) else "" + + +def parse_log(raw: str) -> tuple[int, int] | None: + """Parse one device's published log into an (hour, minute) pair. + + Args: + raw: The raw JSON text returned by the backend. + + Returns: + The (hour, minute) pair, or None if the payload is malformed or the + values are out of range. + """ + try: + log = json.loads(raw) + except json.JSONDecodeError: + _logger.warning("Synced alarm is not valid JSON") + return None + if not isinstance(log, dict): + return None + hour = extract_field(log, HOUR_FIELD) + minute = extract_field(log, MINUTE_FIELD) + if hour is None or minute is None: + _logger.warning("Synced alarm is missing hour/minute fields") + return None + if not in_range(hour, minute): + _logger.warning("Synced alarm out of range: %d:%d", hour, minute) + return None + return hour, minute + + +def newest_alarm(published: list[tuple[str, str]]) -> tuple[int, int] | None: + """Pick the most recently published alarm out of every device's log. + + Ranked by the hour field's HLC stamp, which orders by wall time and then + by counter -- so the winner is the device that set the alarm last, not + whichever directory the backend happened to list first. A device whose + payload does not parse is skipped rather than being allowed to win with a + malformed value. + + Args: + published: ``(device_id, raw_json)`` for every device that has + published, in any order. + + Returns: + The newest (hour, minute) pair, or None if no device published a + usable one. + """ + ranked: list[tuple[str, str, tuple[int, int]]] = [] + for device_id, raw in published: + parsed = parse_log(raw) + if parsed is None: + _logger.info("Ignoring unusable alarm published by %r", device_id) + continue + ranked.append((extract_hlc(json.loads(raw), HOUR_FIELD), device_id, parsed)) + if not ranked: + return None + # device_id breaks an exact HLC tie deterministically, so two devices + # stamped in the same millisecond don't alternate between ticks. + hlc, winner, alarm = max(ranked, key=lambda entry: (entry[0], entry[1])) + _logger.info("Using the alarm published by %r (hlc %s)", winner, hlc or "none") + return alarm diff --git a/wake_alarm/_constants.py b/wake_alarm/_constants.py index f038554..7e42db3 100644 --- a/wake_alarm/_constants.py +++ b/wake_alarm/_constants.py @@ -94,11 +94,23 @@ ALARM_CACHE_FILE: Path = Path.home() / ".config" / "wake_alarm" / "alarm_cache.j # The phone's path to its published alarm. Deliberately the *same* logical # path on both backends — Firebase's namespace mirrors the repo layout — so -# only the transport changes, never the parse. The phone is the sole writer -# under a fixed device id, so one read of its alarm.json is enough: no device -# discovery or HLC ordering here. +# only the transport changes, never the parse. +# +# This is the *legacy* path: the phone used to be the sole writer under the +# fixed device id "phone". It still is in practice, but the id is migrating +# to a per-install uuid, so the reader discovers device directories under +# SYNC_DEVICES_PREFIX and falls back to this path for a phone that has not +# yet published under its uuid. Keep reading it until every install has +# migrated; deleting it early means a stale phone's alarm silently stops +# being seen, and the trigger falls back to FALLBACK_ALARM_HOUR. SYNC_ALARM_PATH: str = "wake-alarm-sync/devices/phone/alarm.json" +# The directory holding one subdirectory per publishing device. +SYNC_DEVICES_PREFIX: str = "wake-alarm-sync/devices" + +# The file each device publishes its alarm as, inside its own directory. +SYNC_ALARM_FILENAME: str = "alarm.json" + # Names this app's credential cache under ~/.local/share/crdt-sync/, keeping # its Firebase refresh token separate from the sibling apps'. SYNC_APP_NAME: str = "wake_alarm" diff --git a/wake_alarm/_sync.py b/wake_alarm/_sync.py index 80659e3..8d20efa 100644 --- a/wake_alarm/_sync.py +++ b/wake_alarm/_sync.py @@ -28,22 +28,20 @@ import json import logging import sys import time -from typing import Any import urllib.error import urllib.request +from wake_alarm._alarm_payload import in_range, newest_alarm, parse_log from wake_alarm._constants import ( ALARM_CACHE_FILE, ALARM_CACHE_MAX_AGE_SECONDS, FALLBACK_ALARM_HOUR, - MAX_HOUR, - MAX_MINUTE, - MIN_HOUR, - MIN_MINUTE, + SYNC_ALARM_FILENAME, SYNC_ALARM_PATH, SYNC_API_BASE, SYNC_APP_NAME, SYNC_CONFIG_FILE, + SYNC_DEVICES_PREFIX, SYNC_HTTP_TIMEOUT_SECONDS, SYNC_REPO_NAME, SYNC_REPO_OWNER, @@ -67,11 +65,6 @@ except ImportError: _FIREBASE_AVAILABLE = False _logger.info("crdt_sync is not installed; reading the alarm from GitHub") -# Keys inside the crdt_sync log the phone writes. -_RECORD_ID = "alarm" -_HOUR_FIELD = "alarm_hour" -_MINUTE_FIELD = "alarm_minute" - @dataclass(frozen=True) class Alarm: @@ -94,22 +87,6 @@ class Alarm: return f"{self.hour:02d}:{self.minute:02d} ({self.source})" -def _in_range(hour: int, minute: int) -> bool: - """Check that an (hour, minute) pair is a real wall-clock time. - - Guards both readers: a malformed sync payload and a corrupted cache must - never become a fire time. - - Args: - hour: Candidate hour. - minute: Candidate minute. - - Returns: - True if both values are within a 24-hour clock. - """ - return MIN_HOUR <= hour <= MAX_HOUR and MIN_MINUTE <= minute <= MAX_MINUTE - - def _read_token_config() -> str | None: """Read the token from this package's own JSON config. @@ -162,65 +139,15 @@ def _read_token() -> str | None: return _read_token_config() or _read_token_fallback() -def _extract_field(log: dict[str, Any], field: str) -> int | None: - """Pull one integer field out of the phone's crdt_sync log. - - The stored shape is ``{"alarm": {"fields": {"": [value, ""]}}}`` - — each field is a (value, HLC) pair, so the value is element 0. - - Args: - log: The parsed log JSON. - field: Field name to extract (e.g. ``alarm_hour``). - - Returns: - The integer value, or None if anything about the shape is unexpected. - """ - record = log.get(_RECORD_ID) - if not isinstance(record, dict): - return None - fields = record.get("fields") - if not isinstance(fields, dict): - return None - pair = fields.get(field) - # bool is an int subclass; an alarm hour of True would be nonsense. - if not isinstance(pair, list) or not pair: - return None - value = pair[0] - if isinstance(value, bool) or not isinstance(value, int): - return None - return value - - -def _parse_log(raw: str) -> tuple[int, int] | None: - """Parse the phone's log into an (hour, minute) pair. - - Args: - raw: The raw JSON text returned by the Contents API. - - Returns: - The (hour, minute) pair, or None if the payload is malformed or the - values are out of range. - """ - try: - log = json.loads(raw) - except json.JSONDecodeError: - _logger.warning("Synced alarm is not valid JSON") - return None - if not isinstance(log, dict): - return None - hour = _extract_field(log, _HOUR_FIELD) - minute = _extract_field(log, _MINUTE_FIELD) - if hour is None or minute is None: - _logger.warning("Synced alarm is missing hour/minute fields") - return None - if not _in_range(hour, minute): - _logger.warning("Synced alarm out of range: %d:%d", hour, minute) - return None - return hour, minute - - def _fetch_firebase_alarm() -> tuple[int, int] | None: - """Fetch the alarm the phone last pushed to Firebase. + """Fetch the newest alarm any device has pushed to Firebase. + + Discovers device directories rather than reading one fixed path: the + phone's device id is migrating from the constant "phone" to a per-install + uuid, and a reader pinned to the old path would stop seeing the alarm the + moment the phone migrated — falling back to FALLBACK_ALARM_HOUR, i.e. + waking the user at the wrong time. Listing covers both ids, and + SYNC_ALARM_PATH remains as a last resort for a backend that cannot list. Returns None — never raises — for every failure mode, including ``crdt_sync`` not being importable on this interpreter, so the caller can @@ -240,15 +167,26 @@ def _fetch_firebase_alarm() -> tuple[int, int] | None: try: client = firebase_client_for(SYNC_APP_NAME) - raw = client.get_file_text(SYNC_ALARM_PATH) + published: list[tuple[str, str]] = [] + for device_id in client.list_directory(SYNC_DEVICES_PREFIX): + raw = client.get_file_text( + f"{SYNC_DEVICES_PREFIX}/{device_id}/{SYNC_ALARM_FILENAME}" + ) + if raw is not None: + published.append((device_id, raw)) + if not published: + # No directories listed (or none held a file): still try the + # legacy fixed path, so a phone that has not migrated is read. + raw = client.get_file_text(SYNC_ALARM_PATH) + if raw is None: + _logger.info("No alarm published to Firebase yet") + return None + published.append(("phone", raw)) except (ConfigError, FirebaseAuthError, RemoteSyncError, OSError) as exc: _logger.info("Cannot fetch the alarm from Firebase: %s", exc) return None - if raw is None: - _logger.info("No alarm published to Firebase yet") - return None - return _parse_log(raw) + return newest_alarm(published) def _fetch_github_alarm() -> tuple[int, int] | None: @@ -300,7 +238,7 @@ def _fetch_github_alarm() -> tuple[int, int] | None: _logger.info("Cannot fetch synced alarm: %s", exc) return None - return _parse_log(raw) + return parse_log(raw) def fetch_remote_alarm() -> tuple[int, int] | None: @@ -337,7 +275,7 @@ def _read_cache() -> tuple[int, int, float] | None: return None if not isinstance(fetched_at, (int, float)): return None - if not _in_range(hour, minute): + if not in_range(hour, minute): return None return hour, minute, float(fetched_at) diff --git a/wake_alarm/tests/test_alarm_payload.py b/wake_alarm/tests/test_alarm_payload.py new file mode 100644 index 0000000..08f2d3f --- /dev/null +++ b/wake_alarm/tests/test_alarm_payload.py @@ -0,0 +1,107 @@ +"""Tests for parsing and ranking the alarm payload devices publish. + +A misparse here does not raise -- it resolves to the wrong wake-up time -- +so the malformed shapes are covered as carefully as the good ones. +""" + +from __future__ import annotations + +import json + +from wake_alarm._alarm_payload import extract_hlc, newest_alarm, parse_log + + +def _log(hour: object = 7, minute: object = 30, hlc: str = "hlc-1") -> str: + """Build a crdt_sync log payload shaped like the phone's.""" + return json.dumps( + { + "alarm": { + "fields": { + "alarm_hour": [hour, hlc], + "alarm_minute": [minute, hlc], + } + } + } + ) + + +class TestExtractHlc: + def test_reads_the_stamp(self) -> None: + """The HLC is element 1 of the (value, HLC) pair.""" + assert extract_hlc(json.loads(_log(hlc="stamp-9")), "alarm_hour") == "stamp-9" + + def test_a_pair_with_no_hlc_reads_as_empty(self) -> None: + """A one-element pair sorts below every real stamp rather than raising.""" + log = {"alarm": {"fields": {"alarm_hour": [7]}}} + + assert extract_hlc(log, "alarm_hour") == "" + + def test_a_non_string_hlc_reads_as_empty(self) -> None: + """A numeric stamp is not orderable against real ones; treat as absent.""" + log = {"alarm": {"fields": {"alarm_hour": [7, 12345]}}} + + assert extract_hlc(log, "alarm_hour") == "" + + def test_a_missing_record_reads_as_empty(self) -> None: + """No alarm record at all is not an error here.""" + assert extract_hlc({}, "alarm_hour") == "" + + +class TestNewestAlarm: + def test_no_devices_yields_none(self) -> None: + """Nothing published is a miss, not a crash.""" + assert newest_alarm([]) is None + + def test_every_device_unusable_yields_none(self) -> None: + """If no device parses, there is no alarm to return.""" + assert newest_alarm([("a", "{not json"), ("b", "[]")]) is None + + def test_newest_hlc_wins(self) -> None: + """The device that set the alarm last wins, not the first listed.""" + published = [ + ("old", _log(9, 0, hlc="2026-07-01T10:00:00.000Z-0000-old")), + ("new", _log(5, 30, hlc="2026-08-09T10:00:00.000Z-0000-new")), + ] + + assert newest_alarm(published) == (5, 30) + + def test_ranking_is_independent_of_listing_order(self) -> None: + """Reversing the input must not change the winner.""" + published = [ + ("new", _log(5, 30, hlc="2026-08-09T10:00:00.000Z-0000-new")), + ("old", _log(9, 0, hlc="2026-07-01T10:00:00.000Z-0000-old")), + ] + + assert newest_alarm(published) == (5, 30) + assert newest_alarm(list(reversed(published))) == (5, 30) + + def test_an_exact_hlc_tie_breaks_on_device_id(self) -> None: + """Two devices stamped identically must not alternate between ticks.""" + published = [ + ("aaa", _log(6, 0, hlc="same")), + ("zzz", _log(8, 0, hlc="same")), + ] + + assert newest_alarm(published) == (8, 0) + assert newest_alarm(list(reversed(published))) == (8, 0) + + def test_an_unusable_device_is_skipped_not_fatal(self) -> None: + """A corrupt publisher must not shadow a healthy one.""" + published = [("broken", "{not json"), ("good", _log(7, 20))] + + assert newest_alarm(published) == (7, 20) + + def test_an_out_of_range_device_is_skipped(self) -> None: + """A nonsense hour never becomes a fire time, even if newest.""" + published = [ + ("bad", _log(99, 0, hlc="2026-08-09T10:00:00.000Z-0000-bad")), + ("good", _log(7, 20, hlc="2026-07-01T10:00:00.000Z-0000-good")), + ] + + assert newest_alarm(published) == (7, 20) + + +class TestParseLog: + def test_a_non_object_payload_yields_none(self) -> None: + """A JSON array is valid JSON but not a log.""" + assert parse_log("[]") is None diff --git a/wake_alarm/tests/test_sync.py b/wake_alarm/tests/test_sync.py index b538ef1..4e31389 100644 --- a/wake_alarm/tests/test_sync.py +++ b/wake_alarm/tests/test_sync.py @@ -9,12 +9,11 @@ import urllib.error import pytest +from wake_alarm._alarm_payload import extract_field, parse_log from wake_alarm._sync import ( Alarm, - _extract_field, _fetch_github_alarm, _parse_args, - _parse_log, _read_cache, _read_token, _read_token_fallback, @@ -156,7 +155,7 @@ class TestTokenFallback: class TestExtractField: - """Tests for _extract_field's tolerance of malformed logs.""" + """Tests for extract_field's tolerance of malformed logs.""" @pytest.mark.parametrize( "log", @@ -174,37 +173,37 @@ class TestExtractField: ) def test_malformed_shapes_yield_none(self, log: dict[str, Any]) -> None: """Every unexpected shape degrades to None rather than raising.""" - assert _extract_field(log, "alarm_hour") is None + assert extract_field(log, "alarm_hour") is None def test_extracts_value(self) -> None: """The value is element 0 of the (value, HLC) pair.""" log = {"alarm": {"fields": {"alarm_hour": [6, "hlc"]}}} - assert _extract_field(log, "alarm_hour") == 6 + assert extract_field(log, "alarm_hour") == 6 class TestParseLog: - """Tests for _parse_log.""" + """Tests for parse_log.""" def test_valid(self) -> None: """A well-formed log parses to an (hour, minute) pair.""" - assert _parse_log(_log(7, 30)) == (7, 30) + assert parse_log(_log(7, 30)) == (7, 30) def test_invalid_json(self) -> None: """Unparsable JSON yields None.""" - assert _parse_log("{nope") is None + assert parse_log("{nope") is None def test_not_a_dict(self) -> None: """A JSON array is not a log.""" - assert _parse_log("[]") is None + assert parse_log("[]") is None def test_missing_fields(self) -> None: """A log without the alarm fields yields None.""" - assert _parse_log(json.dumps({"alarm": {"fields": {}}})) is None + assert parse_log(json.dumps({"alarm": {"fields": {}}})) is None @pytest.mark.parametrize(("hour", "minute"), [(24, 0), (-1, 0), (7, 60), (7, -1)]) def test_out_of_range(self, hour: int, minute: int) -> None: """Values outside 0-23 / 0-59 are rejected.""" - assert _parse_log(_log(hour, minute)) is None + assert parse_log(_log(hour, minute)) is None class TestFetchRemoteAlarm: diff --git a/wake_alarm/tests/test_sync_backends.py b/wake_alarm/tests/test_sync_backends.py index 171919d..1d6c8e6 100644 --- a/wake_alarm/tests/test_sync_backends.py +++ b/wake_alarm/tests/test_sync_backends.py @@ -12,7 +12,11 @@ from unittest.mock import MagicMock, patch import pytest -from wake_alarm._constants import SYNC_ALARM_PATH +from wake_alarm._constants import ( + SYNC_ALARM_FILENAME, + SYNC_ALARM_PATH, + SYNC_DEVICES_PREFIX, +) from wake_alarm._sync import _fetch_firebase_alarm, fetch_remote_alarm if TYPE_CHECKING: @@ -31,14 +35,14 @@ class _FakeRemoteError(Exception): """Stand-in for crdt_sync's RemoteSyncError.""" -def _log(hour: object = 7, minute: object = 30) -> str: +def _log(hour: object = 7, minute: object = 30, hlc: str = "hlc-1") -> str: """Build a crdt_sync log payload shaped like the phone's.""" return json.dumps( { "alarm": { "fields": { - "alarm_hour": [hour, "hlc-1"], - "alarm_minute": [minute, "hlc-1"], + "alarm_hour": [hour, hlc], + "alarm_minute": [minute, hlc], } } } @@ -86,6 +90,7 @@ class TestFetchFirebaseAlarm: def test_success(self) -> None: """A published alarm parses to its (hour, minute) pair.""" client = MagicMock() + client.list_directory.return_value = [] client.get_file_text.return_value = _log(6, 15) with self._patched(client=client): assert _fetch_firebase_alarm() == (6, 15) @@ -94,10 +99,63 @@ class TestFetchFirebaseAlarm: def test_absent_value_yields_none(self) -> None: """Nothing published yet is a miss, not a crash.""" client = MagicMock() + client.list_directory.return_value = [] client.get_file_text.return_value = None with self._patched(client=client): assert _fetch_firebase_alarm() is None + def test_reads_a_discovered_device_directory(self) -> None: + """A phone publishing under a uuid is found by listing, not by path.""" + client = MagicMock() + client.list_directory.return_value = ["7f3c-uuid"] + client.get_file_text.return_value = _log(6, 45) + with self._patched(client=client): + assert _fetch_firebase_alarm() == (6, 45) + client.get_file_text.assert_called_once_with( + f"{SYNC_DEVICES_PREFIX}/7f3c-uuid/{SYNC_ALARM_FILENAME}" + ) + + def test_newest_device_wins_across_several(self) -> None: + """The alarm set most recently wins, whatever order devices list in. + + This is the migration case: the phone's old "phone" directory still + holds a stale alarm while its new uuid directory holds the current + one. Ranking by HLC rather than listing order is what stops the PC + waking at the abandoned time. + """ + client = MagicMock() + client.list_directory.return_value = ["new-uuid", "phone"] + client.get_file_text.side_effect = lambda path: { + f"{SYNC_DEVICES_PREFIX}/new-uuid/{SYNC_ALARM_FILENAME}": _log( + 5, 30, hlc="2026-08-09T10:00:00.000Z-0000-new-uuid" + ), + f"{SYNC_DEVICES_PREFIX}/phone/{SYNC_ALARM_FILENAME}": _log( + 9, 0, hlc="2026-07-01T10:00:00.000Z-0000-phone" + ), + }[path] + with self._patched(client=client): + assert _fetch_firebase_alarm() == (5, 30) + + def test_falls_back_to_the_legacy_path_when_nothing_lists(self) -> None: + """A backend that lists nothing still reads the un-migrated phone.""" + client = MagicMock() + client.list_directory.return_value = ["phone"] + client.get_file_text.side_effect = [None, _log(8, 5)] + with self._patched(client=client): + assert _fetch_firebase_alarm() == (8, 5) + assert client.get_file_text.call_args_list[-1].args[0] == SYNC_ALARM_PATH + + def test_a_device_publishing_garbage_does_not_win(self) -> None: + """One unparsable device must not shadow a healthy one.""" + client = MagicMock() + client.list_directory.return_value = ["broken", "good"] + client.get_file_text.side_effect = lambda path: { + f"{SYNC_DEVICES_PREFIX}/broken/{SYNC_ALARM_FILENAME}": "{not json", + f"{SYNC_DEVICES_PREFIX}/good/{SYNC_ALARM_FILENAME}": _log(7, 20), + }[path] + with self._patched(client=client): + assert _fetch_firebase_alarm() == (7, 20) + @pytest.mark.parametrize( "exc", [