From ae2b1b3902fb08fcd03835430fc9b05bb9ce9b0d Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Fri, 2 Oct 2026 13:57:57 +0530 Subject: [PATCH 1/3] fix(bindings): pin the Python relay-answer prefixes and decide attribution centrally Python held no copy of the relay-answer exemption list, so the rule that a relay answer reaches the core with no transport identity was enforced at each call site rather than once. relay_answer_prefixes.py now holds the list, _inject_group_frame forces every answer unattributed, and a literal pin fails the Python suite the next time the registry moves. The docs that counted three copies now count four. Closes #368 --- CHANGELOG.md | 13 ++++ .../offline_protocol_sdk/internet_manager.py | 9 ++- .../relay_answer_prefixes.py | 61 +++++++++++++++++++ .../python/tests/test_internet_manager.py | 16 +++++ .../tests/test_relay_answer_prefixes.py | 53 ++++++++++++++++ .../offlineprotocol/RelayAnswerPrefixes.kt | 3 +- .../ios/RelayAnswerPrefixes.swift | 3 +- .../offline-protocol/src/protocol/prefixes.rs | 21 ++++--- docs/adr/0004-control-plane-signature-gate.md | 5 +- docs/bridges/README.md | 5 +- docs/bridges/kotlin.md | 2 +- docs/bridges/python.md | 17 ++++++ docs/bridges/swift.md | 2 +- docs/spec/control-messages.md | 5 +- 14 files changed, 194 insertions(+), 21 deletions(-) create mode 100644 bindings/python/offline_protocol_sdk/relay_answer_prefixes.py create mode 100644 bindings/python/tests/test_relay_answer_prefixes.py diff --git a/CHANGELOG.md b/CHANGELOG.md index f57b0f345..59a32def4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -345,6 +345,19 @@ archived by series under [docs/changelog/](docs/changelog/); see the ### Fixed +- **Python holds a pinned copy of the relay-answer prefixes, and decides + attribution in one place.** (#368) The relay's group answers reach the core + only when they carry no transport peer identity, so an answer injected with + a `sender_id` is refused as unsigned while the inject reports success. The + Swift and Kotlin bridges each hold the list and force those answers + unattributed centrally; Python held no copy, and every call site had to get + it right on its own. 0.27.0 already corrected the prefix names and the + attribution at each site (#453). Now `relay_answer_prefixes.py` holds the + list, `_inject_group_frame` drops the actor for any answer whatever the + caller passed, and `test_relay_answer_prefixes.py` pins the six literals, + so the next registry change fails the Python suite rather than a relay + feature in the field. No behaviour change for a correct caller. + - **A key package the application marks synced keeps its record, so its private key is still destroyed when it expires.** `mls_mark_key_package_synced` deleted the record and left the init key in diff --git a/bindings/python/offline_protocol_sdk/internet_manager.py b/bindings/python/offline_protocol_sdk/internet_manager.py index e0088af65..e0af8fcd2 100644 --- a/bindings/python/offline_protocol_sdk/internet_manager.py +++ b/bindings/python/offline_protocol_sdk/internet_manager.py @@ -20,6 +20,7 @@ from websockets.asyncio.client import ClientConnection from . import address_declaration +from .relay_answer_prefixes import attributable_actor from .transport_manager import TransportError, TransportManager, TransportState logger = logging.getLogger(__name__) @@ -1098,7 +1099,8 @@ def _handle_group_message(self, msg_type: str, msg: dict[str, Any]) -> None: is data plane (MLS authenticates it afterwards) and the attribution is the reachability signal for the relayed sender. - The others are relay answers (the core's ``RELAY_ANSWER_PREFIXES``) and + The others are relay answers (``relay_answer_prefixes``, mirroring the + core's ``RELAY_ANSWER_PREFIXES``) and must reach the core unattributed: no peer sent them, so nothing can sign them, and the core's exemption from the control-frame signature gate only recognizes a frame with no transport peer identity. An @@ -1157,8 +1159,11 @@ def _inject_group_frame( ``actor`` is both the FFI ``sender_id`` and the frame's ``sender``; ``None`` selects unattributed ingest, with the "relay" placeholder as - the frame sender because the core rejects an empty one. + the frame sender because the core rejects an empty one. A relay + answer is forced unattributed here, whatever the caller passed: see + ``relay_answer_prefixes``, which holds the pinned list. """ + actor = attributable_actor(prefix, actor) content = prefix + json.dumps(payload) data_bytes = json.dumps( self._build_internal_message(actor or "relay", content) diff --git a/bindings/python/offline_protocol_sdk/relay_answer_prefixes.py b/bindings/python/offline_protocol_sdk/relay_answer_prefixes.py new file mode 100644 index 000000000..71e89148c --- /dev/null +++ b/bindings/python/offline_protocol_sdk/relay_answer_prefixes.py @@ -0,0 +1,61 @@ +"""Which synthesized relay frames must reach the core unattributed. + +A port of ``RelayAnswerPrefixes.swift`` and ``RelayAnswerPrefixes.kt``. + +These are the prefixes the **relay server** originates. The bridge +synthesizes a frame from a WebSocket answer rather than receiving one from a +peer, so no key exists to sign it. The core exempts exactly these from its +control-frame signature gate (``RELAY_ANSWER_PREFIXES`` in +``crates/offline-protocol/src/protocol/prefixes.rs``), and the four copies +must agree: the core, the Swift bridge, the Kotlin bridge and this module. + +Why attribution breaks them: the core's exemption is narrower than the +prefix. It also requires the frame to carry **no transport peer identity**, +which is what a locally synthesized answer looks like. Passing a non-empty +``sender_id`` to ``internet_message_received`` sets that identity, so the +frame stops looking synthesized and is dropped as unsigned. The caller sees a +successful inject and the answer never takes effect. That narrowness is doing +real work and must not be widened to close this: without it, any peer able to +address us through the relay could inject unsigned group state. + +``__GROUP_MSG__`` is **not** here. It is a data-plane prefix, never +signature-gated (MLS authenticates it afterwards), so it keeps its +attribution and remains the reachability signal for a relayed sender. + +``test_relay_answer_prefixes.py`` pins the set as literals (contract C5 in +``docs/bridges/README.md``). A test that recomputed it from this constant +would agree with any edit, which is the failure it exists to catch. +""" + +from __future__ import annotations + +RELAY_ANSWER_PREFIXES: frozenset[str] = frozenset( + { + "__GROUP_CREATED__", + "__GROUP_MEMBER_ADDED__", + "__GROUP_MEMBER_REMOVED__", + "__GROUP_INFO__", + "__USER_GROUPS__", + "__GROUP_ERROR__", + } +) + + +def is_relay_answer(prefix: str) -> bool: + """Whether ``prefix`` names a relay answer that must be injected unattributed. + + Matched whole, not as a prefix of a prefix: whether a content string that + starts with an exempt prefix is admitted is decided in the core, against + the frame's transport and attribution. + """ + return prefix in RELAY_ANSWER_PREFIXES + + +def attributable_actor(prefix: str, actor: str | None) -> str | None: + """The actor a synthesized frame may carry: ``None`` for a relay answer. + + Enforced here rather than trusted to each call site. The rule comes from + a constant in the Rust core, and a new answer injected with an actor is + dropped as unsigned with no error anywhere. + """ + return None if is_relay_answer(prefix) else actor diff --git a/bindings/python/tests/test_internet_manager.py b/bindings/python/tests/test_internet_manager.py index e3c4c8af4..f41e913e2 100644 --- a/bindings/python/tests/test_internet_manager.py +++ b/bindings/python/tests/test_internet_manager.py @@ -249,6 +249,22 @@ def test_relay_answers_are_injected_unattributed( assert frame["recipient"] == self.ADDRESS assert frame["content"] == prefix + json.dumps(payload) + def test_a_relay_answer_is_unattributed_whatever_the_caller_passes( + self, mock_protocol: MagicMock + ) -> None: + # The rule lives in one place, not at each call site: a future arm that + # hands an actor to a relay answer must still reach the core with no + # transport identity, or the gate drops it as unsigned. + mgr = self._manager(mock_protocol) + mgr._inject_group_frame("off1someone", "__USER_GROUPS__", {"groups": []}) + sender_id, frame = self._injected(mock_protocol) + assert sender_id == "" + assert frame["sender"] == "relay" + + mgr._inject_group_frame("off1someone", "__GROUP_MSG__", {"group_id": "g"}) + sender_id, frame = self._injected(mock_protocol) + assert sender_id == "off1someone" + def test_group_delivery_report_stays_off_the_message_plane( self, mock_protocol: MagicMock ) -> None: diff --git a/bindings/python/tests/test_relay_answer_prefixes.py b/bindings/python/tests/test_relay_answer_prefixes.py new file mode 100644 index 000000000..55abb3d05 --- /dev/null +++ b/bindings/python/tests/test_relay_answer_prefixes.py @@ -0,0 +1,53 @@ +"""Pins the "a relay answer reaches the core unattributed" rule. + +Mirrors ``RelayAnswerPrefixesTests.swift`` and ``RelayAnswerPrefixesTest.kt`` +against the core's ``RELAY_ANSWER_PREFIXES``. Python used to hold no copy at +all, and the injection used prefixes the registry did not contain (#368): a +frame under an unregistered prefix is refused as unsigned, invisibly. +""" + +from __future__ import annotations + +from offline_protocol_sdk.relay_answer_prefixes import ( + RELAY_ANSWER_PREFIXES, + attributable_actor, + is_relay_answer, +) + + +def test_the_set_matches_the_core_constant() -> None: + # Written out, not derived: a prefix here the core does not exempt is + # dropped as unsigned, and one the core exempts that is missing here is + # attributed and dropped the same way. + assert RELAY_ANSWER_PREFIXES == { + "__GROUP_CREATED__", + "__GROUP_MEMBER_ADDED__", + "__GROUP_MEMBER_REMOVED__", + "__GROUP_INFO__", + "__USER_GROUPS__", + "__GROUP_ERROR__", + } + + +def test_membership_answers_are_never_attributed() -> None: + for prefix in ("__GROUP_MEMBER_ADDED__", "__GROUP_MEMBER_REMOVED__"): + assert attributable_actor(prefix, "alice") is None + + +def test_every_relay_answer_drops_its_actor() -> None: + for prefix in RELAY_ANSWER_PREFIXES: + assert attributable_actor(prefix, "alice") is None + assert attributable_actor(prefix, None) is None + + +def test_data_plane_and_peer_prefixes_keep_their_actor() -> None: + # Scoped, not blanket: `__GROUP_MSG__` is data plane and its attribution + # is the reachability signal for a relayed sender. + for prefix in ("__GROUP_MSG__", "__CONN_REQ__", "__MLS_ENC__"): + assert attributable_actor(prefix, "alice") == "alice" + + +def test_is_relay_answer_discriminates() -> None: + assert is_relay_answer("__GROUP_CREATED__") + assert not is_relay_answer("__GROUP_MSG__") + assert not is_relay_answer("__GROUP_CREATED__extra") diff --git a/bindings/react-native/android/src/main/java/com/offlineprotocol/RelayAnswerPrefixes.kt b/bindings/react-native/android/src/main/java/com/offlineprotocol/RelayAnswerPrefixes.kt index 8a6e91873..ad3fabb41 100644 --- a/bindings/react-native/android/src/main/java/com/offlineprotocol/RelayAnswerPrefixes.kt +++ b/bindings/react-native/android/src/main/java/com/offlineprotocol/RelayAnswerPrefixes.kt @@ -6,7 +6,8 @@ package com.offlineprotocol * * Mirrors `RELAY_ANSWER_PREFIXES` in * `crates/offline-protocol/src/protocol/prefixes.rs`, and iOS's - * RelayAnswerPrefixes.swift — keep all three in sync. The lists must agree: the + * RelayAnswerPrefixes.swift and Python's relay_answer_prefixes.py. Keep all + * four in sync. The lists must agree: the * core exempts exactly these from its unconditional control-frame signature * gate, because no peer sent them so no key exists to sign them. * diff --git a/bindings/react-native/ios/RelayAnswerPrefixes.swift b/bindings/react-native/ios/RelayAnswerPrefixes.swift index 087935f1d..16e4c87e6 100644 --- a/bindings/react-native/ios/RelayAnswerPrefixes.swift +++ b/bindings/react-native/ios/RelayAnswerPrefixes.swift @@ -2,7 +2,8 @@ // RelayAnswerPrefixes.swift // // Which synthesized relay frames must reach the core unattributed. -// Mirrors android's RelayAnswerPrefixes.kt — keep in sync. +// Mirrors android's RelayAnswerPrefixes.kt and Python's +// relay_answer_prefixes.py. Keep all four copies in sync. // import Foundation diff --git a/crates/offline-protocol/src/protocol/prefixes.rs b/crates/offline-protocol/src/protocol/prefixes.rs index 02722a742..1d1ee5bd9 100644 --- a/crates/offline-protocol/src/protocol/prefixes.rs +++ b/crates/offline-protocol/src/protocol/prefixes.rs @@ -211,13 +211,14 @@ pub(crate) const DATA_PLANE_PREFIXES: &[&str] = /// `internet_group_report_received` already handles the group delivery report; /// that is deliberately out of scope here and left as the follow-up. /// -/// **Maintenance note:** this list is mirrored, by hand, in three places that +/// **Maintenance note:** this list is mirrored, by hand, in four places that /// no single compiler ever sees together — here, `RelayAnswerPrefixes.swift`, -/// and `RelayAnswerPrefixes.kt`. Each bridge pins its own copy against the same -/// literals ([`relay_answer_prefixes_are_pinned`] does it for this one), because +/// `RelayAnswerPrefixes.kt` and the Python `relay_answer_prefixes.py`. Each +/// bridge pins its own copy against the same literals +/// ([`relay_answer_prefixes_are_pinned`] does it for this one), because /// a prefix present in one list and absent from another fails **silently**: the /// bridge injects the answer unattributed, this list declines to exempt it, and -/// the frame is dropped as unsigned with no peer at fault. Edit all three. +/// the frame is dropped as unsigned with no peer at fault. Edit all four. pub(crate) const RELAY_ANSWER_PREFIXES: &[&str] = &[ internal_prefixes::GROUP_CREATED, internal_prefixes::GROUP_MEMBER_ADDED, @@ -234,11 +235,11 @@ mod tests { /// The membership of [`RELAY_ANSWER_PREFIXES`], pinned to literals. /// /// Written out rather than derived, for the same reason the bridges write - /// theirs out: this list is one of three hand-maintained copies, and a test + /// theirs out: this list is one of four hand-maintained copies, and a test /// that recomputed it from the constant would agree with any edit — which /// is precisely the failure mode. The literals here are the contract the - /// two bridge lists are also pinned against, so a divergence in any one of - /// the three now fails a test in its own language. + /// three bridge lists are also pinned against, so a divergence in any one + /// of the four now fails a test in its own language. /// /// Dropping an entry is the dangerous direction and the reason this test /// exists: the bridge would keep injecting that answer unattributed, the @@ -258,9 +259,9 @@ mod tests { "__USER_GROUPS__", "__GROUP_ERROR__", ], - "the relay-answer exemption list changed — update RelayAnswerPrefixes.swift \ - and RelayAnswerPrefixes.kt to match, or the bridges and the gate will \ - disagree silently" + "the relay-answer exemption list changed: update RelayAnswerPrefixes.swift, \ + RelayAnswerPrefixes.kt and relay_answer_prefixes.py to match, or the \ + bridges and the gate will disagree silently" ); } diff --git a/docs/adr/0004-control-plane-signature-gate.md b/docs/adr/0004-control-plane-signature-gate.md index ab8ff5757..7a238a744 100644 --- a/docs/adr/0004-control-plane-signature-gate.md +++ b/docs/adr/0004-control-plane-signature-gate.md @@ -73,8 +73,9 @@ the group delivery report already works. See ## Maintenance hazard -The relay-answer list exists in three places no single compiler sees together: -the core and each native bridge. A prefix present in one copy and absent from +The relay-answer list exists in four places no single compiler sees together: +the core, the Swift and Kotlin bridges, and the Python binding (which had no +copy until #368). A prefix present in one copy and absent from another fails **silently**: the bridge injects the answer unattributed, the gate declines to exempt it, and the frame is dropped as unsigned with no peer at fault. diff --git a/docs/bridges/README.md b/docs/bridges/README.md index 327a22270..059d802c3 100644 --- a/docs/bridges/README.md +++ b/docs/bridges/README.md @@ -122,7 +122,10 @@ Fourteen sets do today, and they are pinned by **two different** mechanisms, so knowing which one you are touching matters. **The relay-answer prefix exemption list** is the canonical example: the core, -the Swift bridge, and the Kotlin bridge each hold a copy. A prefix present in one +the Swift bridge, the Kotlin bridge and the Python binding +(`relay_answer_prefixes.py`) each hold a copy. Python had none until #368, and +its injection had drifted to prefix names the registry never held, which is +this failure exactly. A prefix present in one copy and absent from another **fails silently**: the bridge injects the answer unattributed, the core's gate declines to exempt it, and the frame is dropped as unsigned with no peer at fault. The visible symptom is a relay feature quietly diff --git a/docs/bridges/kotlin.md b/docs/bridges/kotlin.md index 36c3de697..75823feae 100644 --- a/docs/bridges/kotlin.md +++ b/docs/bridges/kotlin.md @@ -99,7 +99,7 @@ subscription, not on what it means. See ## K7. Pinned constant lists -`RelayAnswerPrefixes.kt` holds one of the three copies of the relay-answer +`RelayAnswerPrefixes.kt` holds one of the four copies of the relay-answer exemption list, pinned in `RelayAnswerPrefixesTest.kt`. See [C5](README.md#c5-hand-mirrored-constants-must-be-pinned-in-every-language). diff --git a/docs/bridges/python.md b/docs/bridges/python.md index 30b1ff787..be6f59aed 100644 --- a/docs/bridges/python.md +++ b/docs/bridges/python.md @@ -339,6 +339,23 @@ pins the attach order, the correlation, the flags and what a dead connection owes. No daemon has been run against it; a conforming daemon is a deployment this repository does not ship. +## P12. A relay answer reaches the core unattributed, and the rule lives in one place + +The relay's group answers (`__GROUP_CREATED__`, the membership pair, +`__GROUP_INFO__`, `__USER_GROUPS__`, `__GROUP_ERROR__`) are frames this +binding synthesizes, so nothing can sign them. The core exempts them from the +control-frame signature gate only when they arrive on the Internet transport +with no transport peer identity, so an answer injected with a `sender_id` is +dropped as unsigned, and the caller sees a successful inject. + +`relay_answer_prefixes.py` holds the list, and `_inject_group_frame` passes +every actor through its `attributable_actor`, as the Swift and Kotlin bridges +do. A new arm cannot attribute an answer by mistake. The list is one of the +four hand-mirrored copies under [C5](README.md#c5-hand-mirrored-constants-must-be-pinned-in-every-language), +and `test_relay_answer_prefixes.py` pins it as literals. Before #368 there was +no Python copy, and the injection had drifted to `__GRP_*` names the registry +never held. + ## Testing ```bash diff --git a/docs/bridges/swift.md b/docs/bridges/swift.md index 4c614041f..a46ae16a0 100644 --- a/docs/bridges/swift.md +++ b/docs/bridges/swift.md @@ -198,7 +198,7 @@ languages, Python included. See ## S7. Pinned constant lists -`RelayAnswerPrefixes.swift` holds one of the three copies of the relay-answer +`RelayAnswerPrefixes.swift` holds one of the four copies of the relay-answer exemption list. It is pinned against literals in `RelayAnswerPrefixesTests.swift`. See [C5](README.md#c5-hand-mirrored-constants-must-be-pinned-in-every-language). diff --git a/docs/spec/control-messages.md b/docs/spec/control-messages.md index 40cca800b..12dc2ab71 100644 --- a/docs/spec/control-messages.md +++ b/docs/spec/control-messages.md @@ -412,8 +412,9 @@ assert disjointness in a test. ### Hand-mirrored lists -The relay-answer exemption list exists in three places that no single compiler -sees together: the protocol core and each native bridge. A prefix present in one +The relay-answer exemption list exists in four places that no single compiler +sees together: the protocol core, the Swift and Kotlin bridges, and the Python +binding. A prefix present in one copy and absent from another fails **silently**: the bridge injects the answer unattributed, the gate declines to exempt it, and the frame is dropped as unsigned with no peer at fault. From b739340119c721cdc81190cdb54382272b5d6e17 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Fri, 2 Oct 2026 14:00:25 +0530 Subject: [PATCH 2/3] fix(bindings): the diagnostic level type admits debug, and a guard reads every emitter DiagnosticEvent.level omitted debug, which both native platforms emit, and Android's GATT server emitted a fifth spelling, warn, that nothing declared. The union now names the four levels the bridges emit, the four warn sites are warning, and react_native_diagnostic_levels_match_every_bridge reads the union and every level literal handed to a diagnostic sink in Swift, Kotlin and Python, failing in both directions. Closes #471 --- CHANGELOG.md | 15 ++ bindings/react-native/README.md | 2 +- .../ble/PeripheralGattServer.kt | 8 +- bindings/react-native/src/types.ts | 10 +- crates/offline-protocol-uniffi/src/lib.rs | 161 ++++++++++++++++++ docs/bridges/typescript.md | 8 + 6 files changed, 198 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59a32def4..2472cda20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -345,6 +345,21 @@ archived by series under [docs/changelog/](docs/changelog/); see the ### Fixed +- **`DiagnosticEvent.level` declares `debug`, which both native platforms + emit, and Android no longer emits `warn`.** (#471) The iOS and Android + managers emit diagnostics at `debug` at about seventy sites, while the + TypeScript union said `'info' | 'warning' | 'error'`. Android's GATT server + also emitted four diagnostics as `warn`, a spelling nothing declared; they + are `warning` now. The union is `'debug' | 'info' | 'warning' | 'error'`, and + `react_native_diagnostic_levels_match_every_bridge` reads every level + literal the bridges hand to a diagnostic sink and fails when a level is + emitted but not declared, or declared but never emitted. This widens a + public type to match what already arrives at runtime. It is not + automatically source-compatible: a consumer with an exhaustive `switch` or + a `Record` keyed by the level needs a `debug` entry to typecheck, and a + filter that compared against `'warn'` for the four GATT server diagnostics + should compare against `'warning'`. + - **Python holds a pinned copy of the relay-answer prefixes, and decides attribution in one place.** (#368) The relay's group answers reach the core only when they carry no transport peer identity, so an answer injected with diff --git a/bindings/react-native/README.md b/bindings/react-native/README.md index a4889486d..fbf786d97 100644 --- a/bindings/react-native/README.md +++ b/bindings/react-native/README.md @@ -1085,7 +1085,7 @@ interface FileReceivedEvent { ```typescript interface DiagnosticEvent { type: 'diagnostic'; - level: 'info' | 'warning' | 'error'; + level: 'debug' | 'info' | 'warning' | 'error'; message: string; context?: Record; } diff --git a/bindings/react-native/android/src/main/java/com/offlineprotocol/ble/PeripheralGattServer.kt b/bindings/react-native/android/src/main/java/com/offlineprotocol/ble/PeripheralGattServer.kt index baf4584c8..5aa2528df 100644 --- a/bindings/react-native/android/src/main/java/com/offlineprotocol/ble/PeripheralGattServer.kt +++ b/bindings/react-native/android/src/main/java/com/offlineprotocol/ble/PeripheralGattServer.kt @@ -453,7 +453,7 @@ class PeripheralGattServer( if (!isReady) { Log.w(TAG, "GATT service ready timeout (attempt=$setupAttempts)") diagnosticEmitter( - "warn", + "warning", "gatt_service_ready_timeout", mapOf("attempt" to setupAttempts), ) @@ -700,13 +700,13 @@ class PeripheralGattServer( if (cached != null) { identityReadSnapshots.remove(address) diagnosticEmitter( - "warn", + "warning", "identity_long_read_snapshot_stale", mapOf("address" to address, "offset" to offset), ) } else { diagnosticEmitter( - "warn", + "warning", "identity_long_read_missing_snapshot", mapOf("address" to address, "offset" to offset), ) @@ -744,7 +744,7 @@ class PeripheralGattServer( // the payload to the upstream fragment assembler. if (value.size > MAX_INBOUND_WRITE_BYTES) { diagnosticEmitter( - "warn", + "warning", "gatt_inbound_write_oversize", mapOf( "address" to device.address, diff --git a/bindings/react-native/src/types.ts b/bindings/react-native/src/types.ts index 45fab5c6b..4fae8d07a 100644 --- a/bindings/react-native/src/types.ts +++ b/bindings/react-native/src/types.ts @@ -1533,7 +1533,15 @@ export interface FragmentAssemblyEvictedEvent extends BaseEvent { */ export interface DiagnosticEvent extends BaseEvent { type: 'diagnostic'; - level: 'info' | 'warning' | 'error'; + /** + * The four levels the iOS and Android managers emit. `debug` is the + * high-volume one: filter it rather than surface it. + * + * Pinned from the Rust side by `react_native_diagnostic_levels_match_every_bridge`, + * which reads every native emitter: a level emitted but not declared here + * arrives at runtime as a value this type says is impossible. + */ + level: 'debug' | 'info' | 'warning' | 'error'; message: string; context?: Record; } diff --git a/crates/offline-protocol-uniffi/src/lib.rs b/crates/offline-protocol-uniffi/src/lib.rs index 897ab5faf..9af06407e 100644 --- a/crates/offline-protocol-uniffi/src/lib.rs +++ b/crates/offline-protocol-uniffi/src/lib.rs @@ -17687,6 +17687,167 @@ mod tests { } } + /// The diagnostic levels TypeScript declares are exactly the ones the + /// bridges emit. + /// + /// Every bridge passes the level as a plain string, so only the + /// TypeScript union types it, and nothing compiles the two together. + /// `debug` was emitted on both platforms for several releases while the + /// union said `'info' | 'warning' | 'error'` (#471), and Android's GATT + /// server emitted a fifth spelling, `warn`, that no declaration had. A + /// consumer's exhaustive switch reads either one as impossible. + /// + /// Both directions: an emitted level the union lacks, and a declared level + /// nothing emits. Every sink that takes a level literal is read: the + /// managers' `emitDiagnostic`, the Android BLE `diagnosticEmitter`, and + /// the peer-stream host callbacks. Python's managers are read too, since + /// they share the vocabulary even though their level never reaches + /// TypeScript. The floors on files and literals keep a scan that stopped + /// matching from passing on nothing. + #[test] + fn react_native_diagnostic_levels_match_every_bridge() { + use std::collections::BTreeSet; + + let repo = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("../.."); + + let types_ts = std::fs::read_to_string(repo.join("bindings/react-native/src/types.ts")) + .expect("read types.ts"); + let interface = types_ts + .split_once("export interface DiagnosticEvent extends BaseEvent {") + .expect("types.ts must declare DiagnosticEvent") + .1 + .split_once("\n}") + .expect("unterminated DiagnosticEvent") + .0; + let level_line = interface + .lines() + .map(str::trim) + .find(|l| l.starts_with("level:")) + .expect("DiagnosticEvent must declare `level`"); + let declared: BTreeSet = level_line + .trim_start_matches("level:") + .trim_end_matches(';') + .split('|') + .map(|member| member.trim().trim_matches('\'').to_string()) + .collect(); + + // Comment lines dropped, then flattened, so a call the formatter + // wrapped after its parenthesis still reads as one call. + fn flatten(source: &str, python: bool) -> String { + source + .lines() + .map(str::trim) + .filter(|l| { + if python { + !l.starts_with('#') + } else { + !l.starts_with("//") && !l.starts_with('*') && !l.starts_with("/*") + } + }) + .collect::>() + .join(" ") + .split_whitespace() + .collect::>() + .join(" ") + } + + fn walk(dir: &std::path::Path, ext: &str, out: &mut Vec) { + let entries = std::fs::read_dir(dir) + .unwrap_or_else(|e| panic!("cannot read {}: {e}", dir.display())); + for entry in entries { + let path = entry.expect("dir entry").path(); + let name = path.file_name().and_then(|n| n.to_str()).unwrap_or(""); + if path.is_dir() { + // Build output, test doubles, generated bindings and the + // local API (which emits no diagnostics) are not emitters. + if ![".build", "tests", "Generated", "__pycache__", "local_api"].contains(&name) + { + walk(&path, ext, out); + } + } else if name.ends_with(ext) && name != "offline_protocol.py" { + out.push(path); + } + } + } + + const SINKS: [&str; 5] = [ + "emitDiagnostic(", + "diagnosticEmitter(", + "diagnostic(", + "peerStreamDiagnostic(", + "_emit_diagnostic(", + ]; + + let mut emitted: BTreeSet = BTreeSet::new(); + let mut sites: Vec<(String, String)> = Vec::new(); + let mut files_with_sites = 0usize; + for (root, ext, python) in [ + ("bindings/react-native/ios", ".swift", false), + ("bindings/react-native/android/src/main/java", ".kt", false), + ("bindings/python/offline_protocol_sdk", ".py", true), + ] { + let mut files = Vec::new(); + walk(&repo.join(root), ext, &mut files); + for file in files { + let code = flatten( + &std::fs::read_to_string(&file).expect("read source"), + python, + ); + let before = sites.len(); + for sink in SINKS { + for (at, _) in code.match_indices(sink) { + // `diagnostic(` also matches inside the longer sink + // names; only a call whose name starts at a word + // boundary counts, so each site is read once. + let preceding = code[..at].chars().next_back(); + if preceding.is_some_and(|c| c.is_ascii_alphanumeric() || c == '_') { + continue; + } + let rest = code[at + sink.len()..].trim_start(); + // Swift's `level:` label and Kotlin's `level =` named + // argument both precede the literal. + let rest = rest + .strip_prefix("level:") + .or_else(|| rest.strip_prefix("level =")) + .map_or(rest, str::trim_start); + let Some(literal) = rest.strip_prefix('"') else { + continue; // a forwarded variable, not a level + }; + let level: String = literal.chars().take_while(|c| *c != '"').collect(); + emitted.insert(level.clone()); + sites.push((file.display().to_string(), level)); + } + } + if sites.len() > before { + files_with_sites += 1; + } + } + } + + assert!( + files_with_sites >= 15 && sites.len() >= 500, + "the diagnostic scan found {} level literals in {files_with_sites} files, so it is \ + no longer reading the emitters and would pass against anything", + sites.len() + ); + + let undeclared: Vec<&(String, String)> = sites + .iter() + .filter(|(_, level)| !declared.contains(level)) + .collect(); + assert!( + undeclared.is_empty(), + "these diagnostics emit a level DiagnosticEvent.level in types.ts does not declare \ + ({declared:?}); declare it there and in the README, or use a declared one: \ + {undeclared:?}" + ); + let unused: Vec<&String> = declared.difference(&emitted).collect(); + assert!( + unused.is_empty(), + "DiagnosticEvent.level declares {unused:?}, which no bridge emits" + ); + } + /// Frames a bridge synthesizes for the core carry the configured app id. /// /// Every relay answer, relay group frame and legacy plain-text DM is diff --git a/docs/bridges/typescript.md b/docs/bridges/typescript.md index b3c1cd63d..abde9a6a9 100644 --- a/docs/bridges/typescript.md +++ b/docs/bridges/typescript.md @@ -35,6 +35,14 @@ field set inside it. The tests that pin event field shapes assert against Rust literals and never read `types.ts`. So a renamed or removed event **field** passes every guard in the repository and arrives mistyped at runtime. +One field is the exception: `DiagnosticEvent.level`. The bridges emit +diagnostics as JSON they build themselves, with the level as a plain string at +several hundred call sites, so the union is the only place the +vocabulary is typed. `react_native_diagnostic_levels_match_every_bridge` reads +the union and every level literal handed to a diagnostic sink in the Swift, +Kotlin and Python sources, and fails in both directions. It exists because +`debug` was emitted for several releases while the union omitted it (#471). + Adding an event field means updating `types.ts` in the same change, and nothing will remind you. When `types.ts` lags, nothing fails loudly: the event simply arrives untyped. From bcb6b5ab78e71c35bde9cbfd5d17e61bc7f92b30 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Fri, 2 Oct 2026 14:10:29 +0530 Subject: [PATCH 3/3] fix(bindings): hold a React Native interest declared before start and apply it before the engine starts The engine's start-up exchange offers every held space with the interest in force at that moment, and a narrowing never deletes what a wider exchange pulled in. React Native's start() opens storage and starts the engine in one call, and the setInterest JSDoc said to call it before start(), where both native modules rejected it. DataStore.setInterest now holds a declaration made before start(), and start() applies it after MLS initialization and before the native start; a refused pattern rejects start() without starting the engine, and a data layer that is off drops it with a warning. Both native destroy() paths now release the data store, which holds a strong reference to the engine and otherwise answered data calls after destroy(). Closes #472 --- CHANGELOG.md | 25 ++ .../offlineprotocol/OfflineProtocolModule.kt | 12 + .../ios/OfflineProtocolModule.swift | 4 + bindings/react-native/js-ci-harness/README.md | 1 + .../js-ci-harness/data-interest-order.test.js | 300 ++++++++++++++++++ bindings/react-native/package.json | 2 +- bindings/react-native/src/index.ts | 152 ++++++++- crates/offline-protocol-uniffi/src/lib.rs | 64 ++++ docs/UPGRADING.md | 6 +- docs/api-reference.md | 2 +- docs/bridges/README.md | 1 + docs/bridges/python.md | 8 + docs/data.md | 24 +- docs/react-native-integration.md | 2 +- 14 files changed, 589 insertions(+), 14 deletions(-) create mode 100644 bindings/react-native/js-ci-harness/data-interest-order.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 2472cda20..1a3a60888 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -345,6 +345,31 @@ archived by series under [docs/changelog/](docs/changelog/); see the ### Fixed +- **React Native holds an interest declared before `start()` and applies it + before the engine starts.** (#472) The engine's start-up exchange offers + every held space with the interest in force at that moment, and a narrowing + never deletes what a wider exchange pulled in, so interest has to be + declared after storage opens and before the engine starts. React Native's + `start()` does both steps, and the JSDoc said to call `setInterest` before + it, where both native modules rejected it with `DataStore not initialized`. + An app that moved the call after `start()` had its first exchange ask for + everything. `DataStore.setInterest` now holds a call made before `start()` + and resolves it, and `start()` applies what is held after MLS + initialization and before the native start. A pattern refused then rejects + `start()` with an error naming the space and carrying the native code, and + the engine is not started, because starting would replicate the space + whole; when the data layer is off the held call is dropped with a warning + and `start()` proceeds. `destroy()` discards what is held. The behaviour + change for a deployed app: an invalid pattern declared before `start()` + used to reject that call and now rejects `start()`; a call after `start()` + is unchanged. Both native `destroy()` paths now also release the data + store, which holds a strong reference to the engine: left set, every data + call after `destroy()` reached the stopped engine instead of rejecting, and + on Android the engine stayed alive until the collector reached it. Python's + `ProtocolManager` initializes MLS inside its own `start()` too; its first + exchange still runs under the default interest, now stated in + `docs/bridges/python.md`. + - **`DiagnosticEvent.level` declares `debug`, which both native platforms emit, and Android no longer emits `warn`.** (#471) The iOS and Android managers emit diagnostics at `debug` at about seventy sites, while the diff --git a/bindings/react-native/android/src/main/java/com/offlineprotocol/OfflineProtocolModule.kt b/bindings/react-native/android/src/main/java/com/offlineprotocol/OfflineProtocolModule.kt index ea01bcbef..9934a6722 100644 --- a/bindings/react-native/android/src/main/java/com/offlineprotocol/OfflineProtocolModule.kt +++ b/bindings/react-native/android/src/main/java/com/offlineprotocol/OfflineProtocolModule.kt @@ -2368,6 +2368,18 @@ class OfflineProtocolModule(reactContext: ReactApplicationContext) : // destroyed handle throws; publishing the null first means the // widest that race can be is one already-in-flight call, which // [notifyAppStateQuietly] absorbs. + // The data store holds a strong reference to the engine, so it is + // released first and explicitly: left set, every data call after + // destroy() reached the stopped engine instead of rejecting with + // "DataStore not initialized", and its reference kept the engine + // alive until the collector got to it. + val store = dataStore + dataStore = null + try { + store?.destroy() + } catch (e: Exception) { + android.util.Log.w(NAME, "Releasing the data store handle failed", e) + } val handle = protocol protocol = null try { diff --git a/bindings/react-native/ios/OfflineProtocolModule.swift b/bindings/react-native/ios/OfflineProtocolModule.swift index 716cd3942..83afde155 100644 --- a/bindings/react-native/ios/OfflineProtocolModule.swift +++ b/bindings/react-native/ios/OfflineProtocolModule.swift @@ -2648,6 +2648,10 @@ class OfflineProtocolModule: RCTEventEmitter { } protocolInstance = nil meshServicesInstance = nil + // The store holds a strong reference to the engine. Left set, every + // data call after destroy() reached the stopped engine instead of + // rejecting with "DataStore not initialized", and kept it alive. + dataStoreInstance = nil currentConfig = nil // The app tore the SDK down itself; a message held for this account // must not reach whatever it constructs next. diff --git a/bindings/react-native/js-ci-harness/README.md b/bindings/react-native/js-ci-harness/README.md index 568f5a105..45c4a487b 100644 --- a/bindings/react-native/js-ci-harness/README.md +++ b/bindings/react-native/js-ci-harness/README.md @@ -73,3 +73,4 @@ Two traps, both of which produce a test that passes while proving nothing: | `rich-send-app-id.test.js` | The per-send `appId` on `sendMessage` and `sendMedia` (`src/index.ts`): an appId-only call must take the rich native method, because only it carries `app_id`, and the plain path would send under the configured id without a word; a rich call without one sends `null`. | | `relay-config.test.js` | The relay and DORS config payloads this layer hands to native: the whole `relay` section crossing at create time (not just `relayPriority`), the legacy `low`/`medium`/`high` spelling mapping to the engine vocabulary, and a runtime update naming only the fields it was given — which is what makes the native-side merge a partial update rather than a full overwrite. Its other half is the Rust guard `react_native_bridges_merge_dors_updates_from_the_live_config`. | | `key-package-mapping.test.js` | The JS shape of a key package record (`src/index.ts`, `toMlsKeyPackage`): `createdAt`, `expiresAt` and `isSynced` read from the `createdAtMs`, `expiresAtMs` and `synced` both native bridges send. The wrappers read other names, so every caller got `undefined` for both, and neither the typecheck nor a text guard can see a key that is sent but never read. | +| `data-interest-order.test.js` | When an interest declared with `DataStore.setInterest` reaches native (`src/index.ts`, `pendingInterest`): held before `start()`, applied between MLS initialization and the native start so the engine's start-up exchange carries it (#472), straight through afterwards, a refusal rejecting `start()` without starting the engine, a layer that is off dropping it with a warning, and `destroy()` discarding it. Its other half is the Rust guard `react_native_start_applies_held_interest_before_the_engine_starts`. | diff --git a/bindings/react-native/js-ci-harness/data-interest-order.test.js b/bindings/react-native/js-ci-harness/data-interest-order.test.js new file mode 100644 index 000000000..93c43e780 --- /dev/null +++ b/bindings/react-native/js-ci-harness/data-interest-order.test.js @@ -0,0 +1,300 @@ +#!/usr/bin/env node +/** + * Behavioral tests for the order in which a React Native interest + * declaration reaches the native data store (`DataStore.setInterest` and + * `OfflineProtocol.start` in `src/index.ts`). + * + * The engine's order is: open storage, declare interest, start. Its start-up + * exchange offers every held space with the interest in force at that + * instant, and a narrowing never deletes what a wider offer already pulled + * in. React Native folds storage and start into one `start()` call, so the + * SDK holds a declaration made before it and applies it between MLS + * initialization and the native start (#472). Each failure here is silent on + * a device: a declaration applied after the native start compiles, resolves, + * and replicates the whole space first. + * + * The sibling Rust guard `react_native_start_applies_held_interest_before_the_engine_starts` + * pins the same order as text. See README.md for why the package has no + * other JS test setup. + */ +'use strict'; + +const assert = require('node:assert/strict'); +const { execFileSync } = require('node:child_process'); +const fs = require('node:fs'); +const Module = require('node:module'); +const os = require('node:os'); +const path = require('node:path'); + +const PACKAGE_DIR = path.resolve(__dirname, '..'); + +function compileSdk() { + const tsc = path.join(PACKAGE_DIR, 'node_modules', 'typescript', 'bin', 'tsc'); + if (!fs.existsSync(tsc)) { + throw new Error(`TypeScript not found at ${tsc} — run \`npm ci\` in ${PACKAGE_DIR} first.`); + } + const outDir = fs.mkdtempSync(path.join(os.tmpdir(), 'op-rn-interest-')); + execFileSync( + process.execPath, + [tsc, '--outDir', outDir, '--declaration', 'false', '--declarationMap', 'false'], + { cwd: PACKAGE_DIR, stdio: 'inherit' } + ); + return outDir; +} + +let nativeOverrides = {}; +let nativeCalls = []; + +const nativeModule = new Proxy( + {}, + { + get(_target, method) { + if (typeof method !== 'string') return undefined; + return (...args) => { + nativeCalls.push({ method, args }); + const override = nativeOverrides[method]; + return override ? override(...args) : Promise.resolve(); + }; + }, + } +); + +class StubNativeEventEmitter { + addListener() { + return { remove: () => {} }; + } +} + +const realLoad = Module._load; +Module._load = function loadWithReactNativeStub(request) { + if (request === 'react-native') { + return { + NativeModules: { OfflineProtocolModule: nativeModule }, + NativeEventEmitter: StubNativeEventEmitter, + }; + } + return realLoad.apply(this, arguments); +}; + +const realConsole = { log: console.log, warn: console.warn, error: console.error }; +let captured = { warn: [], error: [] }; + +function captureConsole() { + captured = { warn: [], error: [] }; + console.log = () => {}; + console.warn = (...args) => captured.warn.push(args.join(' ')); + console.error = (...args) => captured.error.push(args.join(' ')); +} + +function releaseConsole() { + Object.assign(console, realConsole); +} + +const tests = []; +const test = (name, fn) => tests.push({ name, fn }); + +let OfflineProtocol; +let DataStore; + +const newSdk = (config = {}) => + new OfflineProtocol({ appId: 'harness', profile: 'harness-profile', ...config }); + +const methods = () => nativeCalls.map((c) => c.method); +const callsTo = (method) => nativeCalls.filter((c) => c.method === method); + +/** + * Clears the module-level hold left by the previous test. Only an instance + * that created an engine resets it, so this starts one and destroys it. + */ +async function resetModuleState() { + const overrides = nativeOverrides; + nativeOverrides = {}; + const sdk = newSdk(); + await sdk.start(); + await sdk.destroy(); + nativeOverrides = overrides; + nativeCalls = []; +} + +function rejectWith(code, message) { + const error = new Error(message); + error.code = code; + return Promise.reject(error); +} + +// --------------------------------------------------------------------------- + +test('a declaration before start() is held, and reaches native nowhere', async () => { + await new DataStore().setInterest('space-1', ['inbox*']); + assert.deepEqual(callsTo('dataSetInterest'), []); +}); + +test('start() applies it after MLS initialization and before the engine starts', async () => { + await new DataStore().setInterest('space-1', ['inbox*', 'profile']); + await newSdk().start(); + + const order = methods().filter((m) => + ['create', 'initializeMlsWithSecureStorage', 'dataSetInterest', 'start'].includes(m) + ); + assert.deepEqual(order, ['create', 'initializeMlsWithSecureStorage', 'dataSetInterest', 'start']); + assert.deepEqual(callsTo('dataSetInterest')[0].args, ['space-1', ['inbox*', 'profile']]); +}); + +test('after start() a declaration goes straight to native', async () => { + await newSdk().start(); + nativeCalls = []; + await new DataStore().setInterest('space-2', ['notes*']); + assert.deepEqual(callsTo('dataSetInterest').map((c) => c.args), [['space-2', ['notes*']]]); +}); + +test('the last declaration for a space wins, and each space is applied once', async () => { + const store = new DataStore(); + await store.setInterest('space-1', ['a*']); + await store.setInterest('space-2', ['b*']); + await store.setInterest('space-1', ['c*']); + await newSdk().start(); + + assert.deepEqual(callsTo('dataSetInterest').map((c) => c.args), [ + ['space-1', ['c*']], + ['space-2', ['b*']], + ]); +}); + +test('a held declaration is a copy of the caller\'s array', async () => { + const patterns = ['a*']; + await new DataStore().setInterest('space-1', patterns); + patterns.push('everything-else*'); + await newSdk().start(); + assert.deepEqual(callsTo('dataSetInterest')[0].args[1], ['a*']); +}); + +test('a refused declaration rejects start(), names the space, and never starts the engine', async () => { + nativeOverrides.dataSetInterest = () => rejectWith('InvalidArgument', 'bad pattern'); + await new DataStore().setInterest('space-1', ['a*b']); + + let refusal; + try { + await newSdk().start(); + } catch (error) { + refusal = error; + } + assert.ok(refusal, 'start() must reject when a held interest is refused'); + assert.match(refusal.message, /space-1/); + assert.equal(refusal.code, 'InvalidArgument'); + assert.equal(callsTo('start').length, 0, 'the engine started under the default interest'); +}); + +test('the refused declaration stays held until the app corrects it', async () => { + nativeOverrides.dataSetInterest = (_space, patterns) => + patterns.includes('a*b') ? rejectWith('InvalidArgument', 'bad pattern') : Promise.resolve(); + const store = new DataStore(); + await store.setInterest('space-1', ['a*b']); + const sdk = newSdk(); + await assert.rejects(sdk.start()); + await assert.rejects(sdk.start(), 'a retry without a correction must fail the same way'); + assert.equal(callsTo('start').length, 0); + + await store.setInterest('space-1', ['a*']); + await sdk.start(); + assert.deepEqual(callsTo('dataSetInterest').at(-1).args, ['space-1', ['a*']]); + assert.equal(callsTo('start').length, 1); +}); + +test('with the data layer off, a held declaration is dropped with a warning and start() proceeds', async () => { + for (const code of ['DataDisabled', 'DataStorageUnavailable']) { + await resetModuleState(); + nativeOverrides.dataSetInterest = () => rejectWith(code, 'off'); + const store = new DataStore(); + await store.setInterest('space-1', ['a*']); + await store.setInterest('space-2', ['b*']); + captured.warn = []; + await newSdk().start(); + + assert.equal(callsTo('start').length, 1, `${code}: the engine did not start`); + assert.equal(callsTo('dataSetInterest').length, 1, `${code}: kept applying after the layer said off`); + assert.ok(captured.warn.some((w) => w.includes(code)), `${code}: no warning`); + } +}); + +test('destroy() discards a held declaration and holds the next one again', async () => { + const sdk = newSdk(); + await sdk.emitTestEvent(); // creates the engine without starting it + await new DataStore().setInterest('space-old', ['a*']); + await sdk.destroy(); + nativeCalls = []; + + await new DataStore().setInterest('space-new', ['b*']); + assert.deepEqual(callsTo('dataSetInterest'), [], 'a declaration after destroy() must be held'); + await newSdk().start(); + assert.deepEqual(callsTo('dataSetInterest').map((c) => c.args), [['space-new', ['b*']]]); +}); + +test('after a started engine is destroyed, the next declaration is held again', async () => { + const sdk = newSdk(); + await sdk.start(); + await sdk.destroy(); + nativeCalls = []; + await new DataStore().setInterest('space-1', ['a*']); + assert.deepEqual(callsTo('dataSetInterest'), []); +}); + +test('destroying an instance that never created an engine leaves a running store alone', async () => { + const running = newSdk(); + await running.start(); + const stale = newSdk(); + await stale.destroy(); + nativeCalls = []; + + await new DataStore().setInterest('space-1', ['a*']); + assert.deepEqual( + callsTo('dataSetInterest').map((c) => c.args), + [['space-1', ['a*']]], + 'a stale instance turned a live declaration into a held one nothing will apply' + ); +}); + +test('a refused space name on a layer that is off does not hold the engine back', async () => { + // The engine validates the name before it checks the layer, so the refusal + // says InvalidArgument; the probe is what reveals the layer is off. + nativeOverrides.dataSetInterest = () => rejectWith('InvalidArgument', 'bad space'); + nativeOverrides.dataListSpaces = () => rejectWith('DataDisabled', 'off'); + await new DataStore().setInterest('bad/space', ['a*']); + await newSdk().start(); + assert.equal(callsTo('start').length, 1); + assert.ok(captured.warn.some((w) => w.includes('DataDisabled'))); +}); + +// --------------------------------------------------------------------------- +// Runner +// --------------------------------------------------------------------------- + +(async () => { + const outDir = compileSdk(); + try { + ({ OfflineProtocol, DataStore } = require(path.join(outDir, 'index.js'))); + + let failed = 0; + for (const { name, fn } of tests) { + nativeOverrides = {}; + captureConsole(); + try { + await resetModuleState(); + await fn(); + releaseConsole(); + realConsole.log(` ✓ ${name}`); + } catch (error) { + failed += 1; + releaseConsole(); + realConsole.log(` ✗ ${name}\n ${error.message}`); + } + } + + realConsole.log( + failed === 0 ? `\n${tests.length} passed.` : `\n${failed} of ${tests.length} FAILED.` + ); + process.exitCode = failed === 0 ? 0 : 1; + } finally { + releaseConsole(); + fs.rmSync(outDir, { recursive: true, force: true }); + } +})(); diff --git a/bindings/react-native/package.json b/bindings/react-native/package.json index 55a5788e2..9586e8e16 100644 --- a/bindings/react-native/package.json +++ b/bindings/react-native/package.json @@ -29,7 +29,7 @@ "scripts": { "prepare": "tsc", "build": "tsc", - "test:js": "node js-ci-harness/one-shot-hold.test.js && node js-ci-harness/local-address.test.js && node js-ci-harness/forward-priority.test.js && node js-ci-harness/rich-send-app-id.test.js && node js-ci-harness/relay-config.test.js && node js-ci-harness/data-config.test.js && node js-ci-harness/security-config.test.js && node js-ci-harness/telemetry-config.test.js && node js-ci-harness/custody-config.test.js && node js-ci-harness/key-package-mapping.test.js", + "test:js": "node js-ci-harness/one-shot-hold.test.js && node js-ci-harness/local-address.test.js && node js-ci-harness/forward-priority.test.js && node js-ci-harness/rich-send-app-id.test.js && node js-ci-harness/relay-config.test.js && node js-ci-harness/data-config.test.js && node js-ci-harness/data-interest-order.test.js && node js-ci-harness/security-config.test.js && node js-ci-harness/telemetry-config.test.js && node js-ci-harness/custody-config.test.js && node js-ci-harness/key-package-mapping.test.js", "build:ios": "bash scripts/build-ios.sh", "build:android": "bash scripts/build-android.sh", "build:all": "bash scripts/build-all.sh", diff --git a/bindings/react-native/src/index.ts b/bindings/react-native/src/index.ts index 77da7ba57..984e828cc 100644 --- a/bindings/react-native/src/index.ts +++ b/bindings/react-native/src/index.ts @@ -310,6 +310,109 @@ function toMlsKeyPackage(raw: any): MlsKeyPackage { * await protocol.stop(); * ``` */ +/** + * Interest declared before the native data store could take it, by space. + * + * The engine's order is: open storage, declare interest, start. Its start-up + * exchange offers every held space with the interest in force at that + * instant, and a narrowing never deletes what a wider offer already pulled + * in. React Native folds the first and last steps into the one + * {@link OfflineProtocol.start} call, so there is no moment outside it at + * which the native store can accept a declaration: before `create` the + * native module has no store, and before MLS initialization the store has no + * storage. A declaration made then is held here, and `start()` applies it + * after storage opens and before the engine starts (#472). + * + * Module-level, like the native module it stands in front of: there is one + * native store per process, and `DataStore` instances are stateless. + */ +const pendingInterest: Map = new Map(); + +/** + * Whether the native store has taken the held interest, so a declaration can + * go straight to it. Set by `start()` once the held interest is applied, and + * cleared by `destroy()`, after which a declaration is held again. + */ +let dataStoreReady = false; + +/** + * The two data-layer answers that mean the layer stores and replicates + * nothing at all. A held interest that meets one of them cannot be violated + * by starting, so `start()` warns rather than refusing. + */ +const DATA_LAYER_OFF_CODES: ReadonlySet = new Set([ + "DataDisabled", + "DataStorageUnavailable", +]); + +/** + * Hands every held interest to the native store, in declaration order. + * + * Re-reads the map on every step, so a declaration made while one is in + * flight is applied too, and an entry is removed only if it was not replaced + * meanwhile. A refusal leaves the refused entry held: retrying `start()` + * without correcting it fails the same way, which is the point, since + * starting would offer the space whole. + */ +async function applyPendingInterest(): Promise { + // Resolves the code that says the data layer is off, or null. The engine + // validates the space name before it checks whether the layer is on, so a + // malformed name on a layer that is off is refused as an invalid argument; + // a cheap read answers the real question, so that refusal cannot hold the + // engine back when nothing would be replicated anyway. + const dataLayerOffCode = async (error: unknown): Promise => { + const code = (error as { code?: unknown } | null)?.code; + if (typeof code === "string" && DATA_LAYER_OFF_CODES.has(code)) { + return code; + } + try { + await OfflineProtocolNativeModule.dataListSpaces(); + return null; + } catch (probe) { + const probeCode = (probe as { code?: unknown } | null)?.code; + return typeof probeCode === "string" && DATA_LAYER_OFF_CODES.has(probeCode) + ? probeCode + : null; + } + }; + + for (;;) { + const next = pendingInterest.entries().next(); + if (next.done) { + return; + } + const [spaceId, patterns] = next.value; + try { + await OfflineProtocolNativeModule.dataSetInterest(spaceId, patterns); + } catch (error) { + const offCode = await dataLayerOffCode(error); + if (offCode !== null) { + console.warn( + `[OfflineProtocol] The data layer is unavailable (${offCode}), so the interest ` + + "declared before start() was dropped; nothing is replicated while it is off.", + error + ); + pendingInterest.clear(); + return; + } + const code = (error as { code?: unknown } | null)?.code; + const message = error instanceof Error ? error.message : String(error); + const refusal = new Error( + `setInterest for space "${spaceId}" was refused, so the protocol was not ` + + `started: starting would replicate the whole space. ${message}` + ) as Error & { code?: string; cause?: unknown }; + if (typeof code === "string") { + refusal.code = code; + } + refusal.cause = error; + throw refusal; + } + if (pendingInterest.get(spaceId) === patterns) { + pendingInterest.delete(spaceId); + } + } +} + export class OfflineProtocol { private eventEmitter: NativeEventEmitter; private eventSubscription: EmitterSubscription | null = null; @@ -1126,6 +1229,13 @@ export class OfflineProtocol { } } + // After storage opened and before the engine starts: the engine's + // start-up exchange builds its offers from the interest in force now, so + // a declaration made before start() has to land here or the first + // exchange asks for every document. See `pendingInterest`. + await applyPendingInterest(); + dataStoreReady = true; + await OfflineProtocolNativeModule.start(); // A session is starting, so nothing held from before it can still be @@ -3557,6 +3667,15 @@ export class OfflineProtocol { // Destroy native protocol instance if (this.isCreated) { + // The store goes with the instance, and interest is policy for one + // launch of one account: a declaration from before this point must not + // reach whatever is created next. Reset before the native call, so a + // declaration racing it is held rather than sent to a store being torn + // down. Only here, inside the branch: the state is module-wide, and an + // instance that never created an engine must not mark another + // instance's running store as not ready. + dataStoreReady = false; + pendingInterest.clear(); await OfflineProtocolNativeModule.destroy(); this.isCreated = false; } @@ -3978,7 +4097,8 @@ export type DataValue = * Requires `initializeMlsWithSecureStorage()` to have run — documents are * sealed at rest with the same per-install key as every other protocol * record, and that key is minted there — and `data.enabled` set in the - * config. Every method answers `DataDisabled` until it is. + * config. Every method answers `DataDisabled` until it is, except + * {@link setInterest}, which is held until `start()` can apply it. * * Edits change the open document in memory only. Nothing is stored or * replicated to peers until {@link flush} or {@link flushAll} runs (the SDK @@ -4079,11 +4199,35 @@ export class DataStore { * which is what makes a narrowing mean something against a peer on an * older build. * - * Not persisted: declare it at launch, before `start()`. Narrowing does - * not delete what is already held; widening asks the peers for what it - * adds. + * Declare it before {@link OfflineProtocol.start}. The native store does + * not exist until `start()` opens it, so a call made earlier is held and + * resolves at once, and `start()` applies it after storage opens and + * before the engine starts. That ordering matters: the engine's start-up + * exchange offers every held space with the interest in force at that + * moment, and narrowing afterwards does not delete what the wider + * exchange pulled in. A later call for the same space replaces a held one. + * + * Validation of a held declaration therefore happens in `start()`: a + * refused pattern rejects `start()` with an error naming the space, and + * the engine is not started, because starting would replicate the space + * whole. Correct it with another `setInterest` and call `start()` again. + * When the data layer is off (`DataDisabled`, `DataStorageUnavailable`) a + * held declaration is dropped with a warning instead, since nothing is + * replicated. + * + * After `start()` resolves, a call applies immediately, and widening asks + * the peers for what it adds. Narrowing never deletes what is already held. + * + * Not persisted: declare it at every launch, and again after + * {@link OfflineProtocol.destroy}, which discards anything held. */ async setInterest(spaceId: string, patterns: string[]): Promise { + if (!dataStoreReady) { + // A copy, so a caller mutating its array later cannot change what is + // applied, and so `applyPendingInterest` can tell a replacement apart. + pendingInterest.set(spaceId, [...patterns]); + return; + } await OfflineProtocolNativeModule.dataSetInterest(spaceId, patterns); } diff --git a/crates/offline-protocol-uniffi/src/lib.rs b/crates/offline-protocol-uniffi/src/lib.rs index 9af06407e..37be25851 100644 --- a/crates/offline-protocol-uniffi/src/lib.rs +++ b/crates/offline-protocol-uniffi/src/lib.rs @@ -17848,6 +17848,70 @@ mod tests { ); } + /// React Native applies an interest declared before `start()` after + /// storage opens and before the engine starts. + /// + /// The engine's start-up exchange offers every held space with the + /// interest in force at that instant, and narrowing later never deletes + /// what the wider offer pulled in. React Native folds MLS initialization + /// and the engine start into one `start()`, so the SDK holds a declaration + /// made earlier and applies it in between (#472). Moved after the native + /// start, everything still compiles and resolves, and the first exchange + /// asks for every document. `js-ci-harness/data-interest-order.test.js` + /// is the behavioural pin; this one reads the order as text so a reorder + /// fails the Rust suite too. + #[test] + fn react_native_start_applies_held_interest_before_the_engine_starts() { + let index = rn_source_code_only("src/index.ts"); + let start = index + .split_once("async start(): Promise {") + .expect("index.ts must define start()") + .1 + .split_once("async stop(): Promise {") + .expect("stop() must follow start() in index.ts") + .0; + let at = |needle: &str| { + start + .find(needle) + .unwrap_or_else(|| panic!("start() in index.ts must call `{needle}`")) + }; + let mls = at("OfflineProtocolNativeModule.initializeMlsWithSecureStorage()"); + let apply = at("await applyPendingInterest();"); + let ready = at("dataStoreReady = true;"); + let engine = at("await OfflineProtocolNativeModule.start();"); + assert!( + mls < apply && apply < ready && ready < engine, + "start() must initialize MLS, apply the held interest, mark the store ready, and \ + only then start the engine: the engine's start-up exchange offers each space with \ + the interest in force when it starts" + ); + + let set_interest = index + .split_once("async setInterest(spaceId: string, patterns: string[]): Promise {") + .expect("DataStore must define setInterest") + .1 + .split_once("async listDocs(") + .expect("listDocs must follow setInterest") + .0; + assert!( + set_interest.contains("if (!dataStoreReady) {") + && set_interest.contains("pendingInterest.set(spaceId, [...patterns]);"), + "setInterest must hold a declaration made before the store is ready; sent to \ + native then, it is refused (no store before create, no storage before MLS)" + ); + + let destroy = index + .split_once("async destroy(): Promise {") + .expect("index.ts must define destroy()") + .1; + assert!( + destroy.contains("dataStoreReady = false;") + && destroy.contains("pendingInterest.clear();"), + "destroy() must discard held interest and hold the next declaration again; a \ + surviving flag sends it to a store that no longer exists" + ); + } + /// Frames a bridge synthesizes for the core carry the configured app id. /// /// Every relay answer, relay group frame and legacy plain-text DM is diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 7c1505d53..d5856732f 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -2249,7 +2249,11 @@ is a document name, optionally ending in `*` to match a prefix; `["*"]` is everything and is the default, `[]` is nothing, and a space takes at most 32. **Set it before `start()`.** It is not persisted, on purpose: it is your -policy for this launch, not a fact about the store. `wipeAll()` clears it +policy for this launch, not a fact about the store. On React Native the +store only opens inside `start()`, and from 0.28.0 the SDK holds a call made +before it and applies it there, before the engine starts; on 0.27.0 the same +call rejected with `DataStore not initialized`, so an app that worked around +that by calling it after `start()` can move the call back. `wipeAll()` clears it along with the documents it scoped, so re-declare it if you wipe while the engine is running. diff --git a/docs/api-reference.md b/docs/api-reference.md index 75a2329b1..271cc4cb7 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -1057,7 +1057,7 @@ await store.flush('space-1', 'profile'); | `deleteDoc(space, doc)` | `void` | Drops this device's copy. Records nothing about the name, so a replica that still holds the document refills it | | `removeDoc(space, doc)` | `void` | Removes the document from every replica of the space. An edit made concurrently with the removal wins and brings the document back whole. Reads and writes on the name throw `InvalidState` until `createDoc` | | `removeSpace(space)` | `void` | Removes every document the space holds, from every replica | -| `setInterest(space, patterns)` | `void` | The documents this device wants from its peers. Names or `name*` prefixes, at most 32. `["*"]` is the default, `[]` is nothing. Not persisted: set it before `start()` | +| `setInterest(space, patterns)` | `void` | The documents this device wants from its peers. Names or `name*` prefixes, at most 32. `["*"]` is the default, `[]` is nothing. Not persisted: set it before `start()`, which React Native holds and applies inside `start()` before the engine starts ([details](data.md#replicating-part-of-a-space)) | | `listDocs(space)` | `string[]` | | | `listSpaces()` | `string[]` | Spaces this device holds documents or removal records for | | `mapSet(space, doc, collection, key, value)` | `void` | | diff --git a/docs/bridges/README.md b/docs/bridges/README.md index 059d802c3..21612f719 100644 --- a/docs/bridges/README.md +++ b/docs/bridges/README.md @@ -318,6 +318,7 @@ alone. | Reticulum available reported **only after** the gateway binds the session | An unbound session may submit and be told a verdict, and is never a recipient, so offering it to the selector offers a transport that can only refuse | | Relay capabilities cleared **on** internet drop | Otherwise a stale capability keeps the broadcast gate open | | Per-peer end-to-end capabilities restored **before** queued sends flush | Otherwise the startup flush emits downgraded envelopes to every established peer | +| Interest applied **after** MLS initialization and **before** the engine starts | The start-up exchange offers every held space with the interest in force then, and a narrowing never deletes what a wider offer pulled in. React Native's `start()` does both steps, so it holds a declaration made before it and applies it in between | | `close_file_stores()` **after** `stop()`, and nothing after it | A running engine writes to its stores, so the call is refused until the protocol is stopped; afterwards the engine holds closed stores, so `start()`, `enable_telemetry()` and both `initialize_mls` entry points are refused | ## C8. The identifier the bridge reports must match the namespace it is asked for diff --git a/docs/bridges/python.md b/docs/bridges/python.md index be6f59aed..cfaad3014 100644 --- a/docs/bridges/python.md +++ b/docs/bridges/python.md @@ -96,6 +96,14 @@ removals, so on a running engine with live sessions it is undone by the peer's next version offer, which recreates and refills every document. `DataStore.remove_space()` is what removes documents from the other replicas. +Interest (`DataStore.set_interest`) belongs after `initialize_mls` and before +`start()`, because the engine's start-up exchange offers each space with the +interest in force when it starts. `ProtocolManager.start()` does both, and the +local API server opens its store after it, so a store reached through either +runs its first exchange under the default interest and narrows from the next +one. An application that needs the first exchange narrowed drives +`OfflineProtocol` itself in the engine's order. + ## P7. The internet send loop is adaptive here, and fixed elsewhere The core's internet outbox is poll-only across the FFI in every binding. The diff --git a/docs/data.md b/docs/data.md index bbf928191..c828d870e 100644 --- a/docs/data.md +++ b/docs/data.md @@ -294,12 +294,24 @@ It does two things, and they are worth telling apart. whoever sends it. That half depends on nothing, so a narrowed space stays narrow even against a peer that ignores the request. -**Set it before `start()`.** It is not persisted, on purpose: it is your -application's policy for this launch rather than a fact about the store, and -a durable copy would be a second thing to reconcile against an app that has -changed its mind. Declaring it costs nothing there: the exchange `start()` -makes already carries it, and declaring the same patterns twice sends -nothing. +**Set it before `start()`.** The engine's start-up exchange offers every +space with the interest in force at that moment, and narrowing afterwards +does not delete what that wider exchange already pulled in. On React Native +the store does not exist until `start()` opens it, so the SDK holds a call +made earlier, resolves it at once, and applies it after storage opens and +before the engine starts. A pattern it refuses then rejects `start()` with an +error naming the space, and the engine is not started. On the Swift package, +the Android library and Python, call it after `initializeMls` and before +`start()`, the order the engine documents; Python's `ProtocolManager` +initializes MLS inside its own `start()`, so a store opened through it runs +its first exchange under the default interest. + +It is not persisted, on purpose: it is your application's policy for this +launch rather than a fact about the store, and a durable copy would be a +second thing to reconcile against an app that has changed its mind. Declare +it at every launch, and again after `destroy()`, which discards a held one. +Declaring it costs nothing: the exchange `start()` makes already carries it, +and declaring the same patterns twice sends nothing. **`wipeAll()` clears it too**, along with the documents it was scoping, because nothing may distinguish a wiped space from one this device has never diff --git a/docs/react-native-integration.md b/docs/react-native-integration.md index 1b5bf4c12..c1b4302e2 100644 --- a/docs/react-native-integration.md +++ b/docs/react-native-integration.md @@ -585,7 +585,7 @@ await store.flush('space-1', 'profile'); | **deleteDoc** | `deleteDoc(spaceId, docId): Promise` | Drops this device's copy. Records nothing about the name, so a replica that still holds the document refills it. | | **removeDoc** | `removeDoc(spaceId, docId): Promise` | Removes the document from every replica of the space. An edit made concurrently with the removal wins and brings the document back whole. | | **removeSpace** | `removeSpace(spaceId): Promise` | Removes every document the space holds, from every replica. | -| **setInterest** | `setInterest(spaceId, patterns): Promise` | The documents this device wants from its peers: names or `name*` prefixes, at most 32. Not persisted, so set it before `start()`. | +| **setInterest** | `setInterest(spaceId, patterns): Promise` | The documents this device wants from its peers: names or `name*` prefixes, at most 32. Call it before `start()` on every launch: the SDK holds it and applies it inside `start()`, before the engine starts, so the first exchange carries it. A refused pattern rejects `start()`. Not persisted, and discarded by `destroy()`. | | **listDocs** | `listDocs(spaceId): Promise` | Documents in a space. | | **listSpaces** | `listSpaces(): Promise` | Spaces this device holds documents or removal records for. | | **mapSet** | `mapSet(spaceId, docId, collection, key, value: DataValue): Promise` | Sets a key in a map collection. |