diff --git a/crdt-sync/crdt_sync/_mirror.py b/crdt-sync/crdt_sync/_mirror.py index 49931b0..d5598cb 100644 --- a/crdt-sync/crdt_sync/_mirror.py +++ b/crdt-sync/crdt_sync/_mirror.py @@ -40,6 +40,13 @@ class MirrorSyncClient: primary failure fails the tick (fail-closed), while a mirror failure is reported and otherwise tolerated. Once the mirror is retired, the old backend going away must not break sync. + + **Reads are resilient on BOTH sides; writes are not.** A read is only + fail-closed when neither backend can answer. Reads used to call the + primary unguarded, so a Firebase outage produced no data at all and the + mirror was never consulted -- the union read silently degraded to nothing, + contradicting the paragraph above. Writes keep the asymmetry: a write the + primary did not accept has not happened, so it must fail the tick. """ def __init__( @@ -63,15 +70,27 @@ class MirrorSyncClient: self._on_mirror_failure = on_mirror_failure or _log_mirror_failure def list_directory(self, path: str) -> list[str]: - """Return the union of both backends' entries under ``path``.""" - names = list(self.primary.list_directory(path)) + """Return the union of both backends' entries under ``path``. + + A reachable primary that lists nothing is a real answer; a primary + that *raises* is not. Reads are the half of this class that exists to + be resilient, so a primary failure degrades to the mirror's entries + rather than failing the read -- returning nothing because one side is + down breaks the union promise exactly when the fallback is most + needed. Only a failure on BOTH sides propagates, since then there is + genuinely no answer to give. + """ + primary_names = self._try_primary_read( + f"list_directory {path}", lambda: self.primary.list_directory(path) + ) + mirror_names = self._try_mirror( + f"list_directory {path}", lambda: self.mirror.list_directory(path) + ) + if primary_names is None and mirror_names is None: + self._raise_both_failed(f"list_directory {path}") + names = list(primary_names or []) seen = set(names) - for name in ( - self._try_mirror( - f"list_directory {path}", lambda: self.mirror.list_directory(path) - ) - or [] - ): + for name in mirror_names or []: if name not in seen: seen.add(name) names.append(name) @@ -82,8 +101,19 @@ class MirrorSyncClient: Absent from the primary means "this device has not migrated yet", not "no data" -- so it falls through rather than reporting nothing. + A primary that *raises* falls through for the same reason (see + :meth:`list_directory`); only a failure on both sides propagates. """ - text = self.primary.get_file_text(path) + try: + text = self.primary.get_file_text(path) + except RemoteSyncError as error: + self._on_primary_read_failure(f"get_file_text {path}", error) + mirror_text = self._try_mirror( + f"get_file_text {path}", lambda: self.mirror.get_file_text(path) + ) + if mirror_text is None: + self._raise_both_failed(f"get_file_text {path}") + return mirror_text if text is not None: return text return self._try_mirror( @@ -128,17 +158,32 @@ class MirrorSyncClient: A device that has not migrated publishes revisions only to the mirror; without this it would look revision-less and be re-downloaded every tick for the whole trial period. + + Like the other reads, a primary failure degrades to the mirror rather + than failing the tick: revisions are a cache, and losing them re-reads + data rather than losing it. """ merged: dict[str, str] = {} mirror_read = getattr(self.mirror, "get_string_map", None) + # A mirror that CANNOT do bulk reads contributes nothing, but that is + # not a failure -- it has no revisions to give, which is a real answer. + # Only a mirror that tried and threw leaves the read with no answer, + # and only then can a failing primary make this raise. + mirror_answered = mirror_read is None if mirror_read is not None: - merged.update( - self._try_mirror(f"get_string_map {path}", lambda: mirror_read(path)) - or {} + mirror_map = self._try_mirror( + f"get_string_map {path}", lambda: mirror_read(path) ) + mirror_answered = mirror_map is not None + merged.update(mirror_map or {}) primary_read = getattr(self.primary, "get_string_map", None) if primary_read is not None: - merged.update(primary_read(path)) + primary_map = self._try_primary_read( + f"get_string_map {path}", lambda: primary_read(path) + ) + if primary_map is None and not mirror_answered: + self._raise_both_failed(f"get_string_map {path}") + merged.update(primary_map or {}) return merged def _try_mirror(self, operation: str, run: Callable[[], _T]) -> _T | None: @@ -149,6 +194,36 @@ class MirrorSyncClient: self._on_mirror_failure(operation, error) return None + def _try_primary_read(self, operation: str, run: Callable[[], _T]) -> _T | None: + """Run a primary READ, reporting rather than propagating failure. + + Reads only -- writes stay fail-closed, since a write the primary did + not accept has not happened and must fail the tick. + """ + try: + return run() + except RemoteSyncError as error: + self._on_primary_read_failure(operation, error) + return None + + def _on_primary_read_failure(self, operation: str, error: Exception) -> None: + """Warn that a read fell back to the mirror.""" + _logger.warning( + "Primary backend failed during %s: %s — falling back to the mirror", + operation, + error, + ) + + def _raise_both_failed(self, operation: str) -> None: + """Raise when neither backend could answer a read. + + Fail closed rather than returning an empty result: an empty read is + indistinguishable from "no data", and callers act on that by treating + remote state as absent. + """ + msg = f"both primary and mirror backends failed during {operation}" + raise RemoteSyncError(msg) + def _log_mirror_failure(operation: str, error: Exception) -> None: """Warn loudly, so a rotting fallback is visible before it is needed.""" diff --git a/crdt-sync/crdt_sync/tests/test_mirror.py b/crdt-sync/crdt_sync/tests/test_mirror.py index 6e76da2..aaad790 100644 --- a/crdt-sync/crdt_sync/tests/test_mirror.py +++ b/crdt-sync/crdt_sync/tests/test_mirror.py @@ -203,7 +203,13 @@ class TestReads: assert client.get_file_text("ns/pc/log.json") is None assert failures == ["get_file_text ns/pc/log.json"] - def test_a_primary_read_failure_fails_the_tick(self) -> None: + def test_a_primary_read_failure_with_nothing_in_the_mirror_raises( + self, + ) -> None: + # Named for what it actually exercises: the primary raises AND the + # mirror does not hold the file, so neither side has an answer. A + # primary failure alone no longer fails the read -- see + # TestPrimaryReadFallback. client, _, _, _ = _mirror(primary_failing=True) with pytest.raises(RemoteSyncError): client.get_file_text("ns/pc/log.json") @@ -231,12 +237,82 @@ class TestListDirectory: assert client.list_directory("ns") == ["pc"] assert failures == ["list_directory ns"] - def test_a_primary_failure_fails_the_tick(self) -> None: - client, _, _, _ = _mirror(primary_failing=True) + def test_a_primary_failure_degrades_to_the_mirror_list(self) -> None: + # READS are resilient on both sides: a Firebase outage must not hide + # the mirror's devices. This used to raise, which made a primary + # outage look like "no devices exist" -- the union read silently + # degrading to nothing, precisely when the fallback was needed. + client, _, _, _ = _mirror( + primary_failing=True, mirror_files={"ns/phone/log.json": "{}"} + ) + assert client.list_directory("ns") == ["phone"] + + def test_both_backends_failing_raises(self) -> None: + # With no answer from either side, fail closed: an empty list is + # indistinguishable from "no devices", and callers act on that. + client, _, _, _ = _mirror(primary_failing=True, mirror_failing=True) with pytest.raises(RemoteSyncError): client.list_directory("ns") +class TestPrimaryReadFallback: + """A failing primary must never hide data the mirror can still serve.""" + + def test_get_file_text_falls_back_to_the_mirror(self) -> None: + client, _, _, _ = _mirror( + primary_failing=True, mirror_files={"ns/phone/log.json": "{}"} + ) + assert client.get_file_text("ns/phone/log.json") == "{}" + + def test_get_file_text_raises_when_both_fail(self) -> None: + client, _, _, _ = _mirror(primary_failing=True, mirror_failing=True) + with pytest.raises(RemoteSyncError): + client.get_file_text("ns/phone/log.json") + + def test_get_file_text_raises_when_primary_fails_and_mirror_lacks_it( + self, + ) -> None: + # A reachable mirror that simply does not hold the file is not an + # answer either, since the primary's copy was never read. + client, _, _, _ = _mirror(primary_failing=True) + with pytest.raises(RemoteSyncError): + client.get_file_text("ns/phone/log.json") + + def test_revision_map_falls_back_to_the_mirror(self) -> None: + client, _, _, _ = _mirror( + primary_failing=True, mirror_files={"ns/revs/phone": "r1"} + ) + assert client.get_string_map("ns/revs") == {"phone": "r1"} + + def test_revision_map_raises_when_both_fail(self) -> None: + client, _, _, _ = _mirror(primary_failing=True, mirror_failing=True) + with pytest.raises(RemoteSyncError): + client.get_string_map("ns/revs") + + def test_revision_map_tolerates_a_mirror_that_cannot_bulk_read( + self, + ) -> None: + # A mirror with no bulk-read capability (GitHub) contributes nothing, + # but that is not a failure -- it simply has no revisions to add, so a + # working primary's map must still come back. Matches the Dart side, + # where the capability check runs inside the guarded closure. + client = MirrorSyncClient( + FakeStore({"ns/revs/pc": "r1"}), + FakeStoreWithoutBulkRead(), + on_mirror_failure=lambda *_: None, + ) + assert client.get_string_map("ns/revs") == {"pc": "r1"} + + def test_writes_stay_fail_closed_on_a_primary_failure(self) -> None: + # The read fix must NOT loosen writes: an unaccepted write has not + # happened, so it must still fail the tick. + client, _, _, _ = _mirror(primary_failing=True) + with pytest.raises(RemoteSyncError): + client.put_file_text("ns/pc/log.json", "{}", message="m") + with pytest.raises(RemoteSyncError): + client.delete_file("ns/pc/log.json") + + class TestRevisionMaps: def test_merge_both_backends_with_the_primary_winning(self) -> None: client, _, _, _ = _mirror( diff --git a/crdt-sync/pyproject.toml b/crdt-sync/pyproject.toml index a9184f7..69ef22e 100644 --- a/crdt-sync/pyproject.toml +++ b/crdt-sync/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "crdt-sync" -version = "0.6.0" +version = "0.7.0" description = "Shared CRDT merge scheme + GitHub-Contents-API sync transport for cross-device apps" requires-python = ">=3.10" dependencies = ["requests"] diff --git a/crdt_sync_dart/lib/src/mirror_store.dart b/crdt_sync_dart/lib/src/mirror_store.dart index 2261d36..961bd44 100644 --- a/crdt_sync_dart/lib/src/mirror_store.dart +++ b/crdt_sync_dart/lib/src/mirror_store.dart @@ -8,8 +8,7 @@ library; import 'remote_store.dart'; /// Called when the mirror backend fails, so the failure is loud but survivable. -typedef MirrorFailureHandler = - void Function(String operation, Object error); +typedef MirrorFailureHandler = void Function(String operation, Object error); /// Writes to both backends; reads from both and prefers the primary. /// @@ -24,11 +23,19 @@ typedef MirrorFailureHandler = /// primary failure fails the tick (fail-closed), while a mirror failure is /// reported through [onMirrorFailure] and otherwise tolerated. Once the /// mirror is retired, the old backend going away must not break sync. +/// +/// **Reads are resilient on BOTH sides; writes are not.** A read is only +/// fail-closed when neither backend can answer. Reads used to call the +/// primary unguarded, so a Firebase outage produced no data at all and the +/// mirror was never consulted -- the union read silently degraded to nothing, +/// contradicting the paragraph above. Writes keep the asymmetry: a write the +/// primary did not accept has not happened, so it must fail the tick. class MirrorStore implements RemoteStore, BulkMapReader { MirrorStore({ required this.primary, required this.mirror, this.onMirrorFailure, + this.onPrimaryReadFailure, }); /// The authoritative backend. Its failures fail the tick. @@ -41,25 +48,49 @@ class MirrorStore implements RemoteStore, BulkMapReader { /// the fallback rot unnoticed until the day it was needed. final MirrorFailureHandler? onMirrorFailure; + /// Notified when a primary READ fails and the mirror is used instead. + /// + /// Separate from [onMirrorFailure] so "the backend we are migrating TO is + /// down" is distinguishable from "the backend we are retiring is down". + final MirrorFailureHandler? onPrimaryReadFailure; + @override Future> listDirectory(String path) async { - final names = {...await primary.listDirectory(path)}; - await _tryMirror('listDirectory $path', () async { + // A reachable primary that lists nothing is a real answer; a primary that + // THROWS is not. Reads are the half of this class that exists to be + // resilient, so a primary failure degrades to the mirror's entries rather + // than failing the read -- returning nothing because one side is down + // breaks the union promise exactly when the fallback is most needed. + final names = {}; + final primaryOk = await _tryPrimaryRead('listDirectory $path', () async { + names.addAll(await primary.listDirectory(path)); + }); + final mirrorOk = await _tryMirror('listDirectory $path', () async { names.addAll(await mirror.listDirectory(path)); }); + if (!primaryOk && !mirrorOk) { + _raiseBothFailed('listDirectory $path'); + } return names.toList(); } @override Future getFileText(String path) async { - final text = await primary.getFileText(path); - if (text != null) return text; + String? text; + final primaryOk = await _tryPrimaryRead('getFileText $path', () async { + text = await primary.getFileText(path); + }); + if (primaryOk && text != null) return text; // Absent from the primary means "this device has not migrated yet", not - // "no data" -- so fall through rather than reporting nothing. + // "no data" -- so fall through rather than reporting nothing. A primary + // that threw falls through for the same reason. String? fallback; - await _tryMirror('getFileText $path', () async { + final mirrorOk = await _tryMirror('getFileText $path', () async { fallback = await mirror.getFileText(path); }); + if (!primaryOk && (!mirrorOk || fallback == null)) { + _raiseBothFailed('getFileText $path'); + } return fallback; } @@ -104,14 +135,21 @@ class MirrorStore implements RemoteStore, BulkMapReader { // Pattern binds rather than `is` promotion: BulkMapReader is not a // subtype of RemoteStore, and Dart only promotes to subtypes of the // declared type, so the intersection has to be named explicitly. - await _tryMirror('getStringMap $path', () async { + final mirrorOk = await _tryMirror('getStringMap $path', () async { if (mirror case final BulkMapReader reader) { merged.addAll(await reader.getStringMap(path)); } }); - // Primary applied last so its entries win on conflict. - if (primary case final BulkMapReader reader) { - merged.addAll(await reader.getStringMap(path)); + // Primary applied last so its entries win on conflict. Like the other + // reads, a primary failure degrades to the mirror: revisions are a cache, + // and losing them re-reads data rather than losing it. + final primaryOk = await _tryPrimaryRead('getStringMap $path', () async { + if (primary case final BulkMapReader reader) { + merged.addAll(await reader.getStringMap(path)); + } + }); + if (!primaryOk && !mirrorOk) { + _raiseBothFailed('getStringMap $path'); } return merged; } @@ -123,11 +161,43 @@ class MirrorStore implements RemoteStore, BulkMapReader { } /// Runs a mirror operation, reporting rather than propagating its failure. - Future _tryMirror(String operation, Future Function() run) async { + /// + /// Returns whether it succeeded, so a read can tell "the mirror had nothing" + /// from "the mirror could not be reached". + Future _tryMirror(String operation, Future Function() run) async { try { await run(); + return true; } on RemoteSyncError catch (error) { onMirrorFailure?.call(operation, error); + return false; } } + + /// Runs a primary READ, reporting rather than propagating its failure. + /// + /// Reads only -- writes stay fail-closed, since a write the primary did not + /// accept has not happened and must fail the tick. + Future _tryPrimaryRead( + String operation, + Future Function() run, + ) async { + try { + await run(); + return true; + } on RemoteSyncError catch (error) { + onPrimaryReadFailure?.call(operation, error); + return false; + } + } + + /// Fails closed when neither backend could answer a read. + /// + /// An empty result is indistinguishable from "no data", and callers act on + /// that by treating remote state as absent. + Never _raiseBothFailed(String operation) { + throw RemoteSyncError( + 'both primary and mirror backends failed during $operation', + ); + } } diff --git a/crdt_sync_dart/pubspec.yaml b/crdt_sync_dart/pubspec.yaml index 3c7bc54..5de3542 100644 --- a/crdt_sync_dart/pubspec.yaml +++ b/crdt_sync_dart/pubspec.yaml @@ -2,7 +2,7 @@ name: crdt_sync description: >- Shared CRDT merge scheme + GitHub-Contents-API sync transport for cross-device apps. Pure Dart, no Flutter dependency. -version: 0.8.0 +version: 0.9.0 publish_to: 'none' environment: diff --git a/crdt_sync_dart/test/mirror_store_test.dart b/crdt_sync_dart/test/mirror_store_test.dart index 60b20a7..658b2fc 100644 --- a/crdt_sync_dart/test/mirror_store_test.dart +++ b/crdt_sync_dart/test/mirror_store_test.dart @@ -82,8 +82,7 @@ class _FakeStoreWithoutBulkRead implements RemoteStore { final _FakeStore _inner; @override - Future> listDirectory(String path) => - _inner.listDirectory(path); + Future> listDirectory(String path) => _inner.listDirectory(path); @override Future getFileText(String path) => _inner.getFileText(path); @override @@ -98,7 +97,12 @@ class _FakeStoreWithoutBulkRead implements RemoteStore { void close() => _inner.close(); } -({MirrorStore store, _FakeStore primary, _FakeStore mirror, List failures}) +({ + MirrorStore store, + _FakeStore primary, + _FakeStore mirror, + List failures, +}) _mirror({ Map? primaryFiles, Map? mirrorFiles, @@ -194,7 +198,11 @@ void main() { expect(m.failures, ['getFileText ns/pc/log.json']); }); - test('a primary read failure fails the tick', () async { + test('a primary read failure with nothing in the mirror throws', () async { + // Named for what it actually exercises: the primary throws AND the + // mirror does not hold the file, so neither side has an answer. A + // primary failure alone no longer fails the read -- see the + // "primary read fallback" group. final m = _mirror(primaryFailing: true); await expectLater( () => m.store.getFileText('ns/pc/log.json'), @@ -229,8 +237,22 @@ void main() { expect(m.failures, ['listDirectory ns']); }); - test('a primary failure fails the tick', () async { - final m = _mirror(primaryFailing: true); + test('a primary failure degrades to the mirror list', () async { + // READS are resilient on both sides: a Firebase outage must not hide + // the mirror's devices. This used to throw, which made a primary outage + // look like "no devices exist" -- the union read silently degrading to + // nothing, precisely when the fallback was needed. + final m = _mirror( + primaryFailing: true, + mirrorFiles: {'ns/phone/log.json': '{}'}, + ); + expect(await m.store.listDirectory('ns'), ['phone']); + }); + + test('both backends failing throws', () async { + // With no answer from either side, fail closed: an empty list is + // indistinguishable from "no devices", and callers act on that. + final m = _mirror(primaryFailing: true, mirrorFailing: true); await expectLater( () => m.store.listDirectory('ns'), throwsA(isA()), @@ -238,6 +260,75 @@ void main() { }); }); + group('primary read fallback', () { + test('getFileText falls back to the mirror', () async { + final m = _mirror( + primaryFailing: true, + mirrorFiles: {'ns/phone/log.json': '{}'}, + ); + expect(await m.store.getFileText('ns/phone/log.json'), '{}'); + }); + + test('getFileText throws when both fail', () async { + final m = _mirror(primaryFailing: true, mirrorFailing: true); + await expectLater( + () => m.store.getFileText('ns/phone/log.json'), + throwsA(isA()), + ); + }); + + test('getFileText throws when primary fails and mirror lacks it', () async { + // A reachable mirror that simply does not hold the file is not an + // answer either, since the primary's copy was never read. + final m = _mirror(primaryFailing: true); + await expectLater( + () => m.store.getFileText('ns/phone/log.json'), + throwsA(isA()), + ); + }); + + test('getStringMap falls back to the mirror', () async { + final m = _mirror( + primaryFailing: true, + mirrorFiles: {'ns/revs/phone': 'r1'}, + ); + expect(await m.store.getStringMap('ns/revs'), {'phone': 'r1'}); + }); + + test('getStringMap throws when both fail', () async { + final m = _mirror(primaryFailing: true, mirrorFailing: true); + await expectLater( + () => m.store.getStringMap('ns/revs'), + throwsA(isA()), + ); + }); + + test('getStringMap tolerates a mirror that cannot bulk-read', () async { + // A mirror with no bulk-read capability (GitHub) contributes nothing, + // but that is not a failure -- it simply has no revisions to add, so a + // working primary's map must still come back. + final store = MirrorStore( + primary: _FakeStore(files: {'ns/revs/pc': 'r1'}), + mirror: _FakeStoreWithoutBulkRead(), + ); + expect(await store.getStringMap('ns/revs'), {'pc': 'r1'}); + }); + + test('writes stay fail-closed on a primary failure', () async { + // The read fix must NOT loosen writes: an unaccepted write has not + // happened, so it must still fail the tick. + final m = _mirror(primaryFailing: true); + await expectLater( + () => m.store.putFileText('ns/pc/log.json', '{}', message: 'm'), + throwsA(isA()), + ); + await expectLater( + () => m.store.deleteFile('ns/pc/log.json'), + throwsA(isA()), + ); + }); + }); + group('revision maps', () { test('merge both backends, primary winning on conflict', () async { final m = _mirror(