Readonly Mode

[codabrink]

Add readonly client mode and route it through builders, context, and sync paths to make registration and rotations no-ops in xmtp_mls

Introduce ClientMode::Readonly, thread it via ClientBuilder.with_client_mode(...) into XmtpMlsLocalContext, and gate registration, key rotation, intents, and post-commit syncing behind readonly_mode(). Update FFI/Node/WASM create_client to accept optional mode and map to core ClientMode. Replace test API selection with TesterApiMode and add targeted readonly tests.

📍Where to Start

Start with ClientMode and context wiring in context.rs, then follow builder propagation in builder.rs and readonly guards in client and group sync paths in client.rs and groups/mls_sync.rs.

Changes since #2835 opened

• Renamed client mode terminology from 'Notification' to 'Readonly' across FFI bindings and error handling [b31d94f]

---

📊 Macroscope summarized b31d94f. 15 files reviewed, 26 issues evaluated, 26 issues filtered, 0 comments posted

🗂️ Filtered Issues

bindings_ffi/benches/create_client.rs — 0 comments posted, 7 evaluated, 7 filtered

• line 47: unwrap() on ident.inbox_id(nonce) can panic at runtime if Identifier::inbox_id returns an Err (e.g., if the generated Ethereum address is deemed invalid by is_valid_address). Replace with proper error handling and surfacing of the failure. [ Out of scope ]
• line 48: Calling tmp_path() (used here) can panic due to std::env::temp_dir().to_str().unwrap() if the temp directory path is not valid UTF-8 on the host system. Use a fallible conversion or lossless path handling (e.g., PathBuf or to_string_lossy). [ Out of scope ]
• line 82: unwrap() on the result of xmtpv3::mls::create_client(...).await can panic if client creation fails (e.g., network/backend issues). Prefer handling the error and reporting it within the benchmark. [ Out of scope ]
• line 101: unwrap() on ident.inbox_id(nonce) can panic at runtime if identifier validation fails. Replace with fallible handling to keep the benchmark from aborting. [ Out of scope ]
• line 104: Calling tmp_path() here can panic because it uses to_str().unwrap() on the system temp directory path, which may not be UTF-8. Use a fallible or lossy conversion, or accept a PathBuf. [ Out of scope ]
• line 124: unwrap() on xmtpv3::mls::create_client(...).await inside the warm-up block_on can panic if client creation fails (network/backends). Handle the error instead of aborting the benchmark process. [ Out of scope ]
• line 163: unwrap() on xmtpv3::mls::create_client(...).await in the benchmark iteration can panic on backend/client creation failures. Prefer propagating or recording the error instead of panicking. [ Out of scope ]

bindings_wasm/src/client.rs — 0 comments posted, 2 evaluated, 2 filtered

• line 80: Removing the Notification variant from the #[wasm_bindgen]-exposed enum ClientMode is a breaking change for existing JavaScript callers. Any previously valid JS code passing ClientMode.Notification (or the corresponding discriminant value) will now be rejected at the wasm boundary, causing a runtime error. wasm_bindgen enforces enum value validity and will throw if an unknown discriminant is provided, so the removal can lead to immediate failures when createClient is invoked with client_mode set to the former Notification value. This violates contract parity for the public API and can produce runtime exceptions in production. [ Low confidence ]
• line 226: In create_client, when an encryption_key is provided, the code validates it (let _key: EncryptionKey = key.try_into()?) but then discards it and constructs the database with WasmDb::new(&storage_option) without passing or applying the encryption key. As a result, the encryption key is effectively ignored and the store may be created unencrypted even though the caller supplied a key. The error messages suggest an encrypted store is intended, but no encryption configuration is applied to the DB. The key should be used to initialize the database or the EncryptedMessageStore so that data is actually encrypted. [ Out of scope ]

xmtp_mls/src/builder.rs — 0 comments posted, 8 evaluated, 8 filtered

• line 176: Contract parity break: identity_strategy is forced to IdentityStrategy::CachedOnly rather than preserving any prior strategy. If the original setup expected CreateIfNotFound, rebuilding may fail to create missing identities. [ Out of scope ]
• line 182: Contract parity break: allow_offline is hardcoded to false rather than reflecting the existing Client configuration. Rebuilt clients may incorrectly disallow offline mode. [ Out of scope ]
• line 184: Contract parity break: disable_events is set based on cfg(test) rather than preserved from the source Client. This can flip event behavior unexpectedly when rebuilding. [ Out of scope ]
• line 187: Contract parity break: disable_commit_log_worker is hardcoded to false rather than reflecting the original client's configuration, potentially enabling a worker unexpectedly. [ Out of scope ]
• line 190: Contract parity break: cursor_store is set to None instead of preserving any existing cursor store from the source Client. This can disable or reset cursor-based behavior after rebuild. [ Out of scope ]
• line 192: Contract parity break: client_mode is always set to None rather than preserving the Client's existing mode. This can change runtime behavior when rebuilding from an existing client. [ Low confidence ]
• line 333: Global events flag EVENTS_ENABLED is only ever set to true during build when disable_events is false, but it is never set back to false when disable_events is true. This leaks global state across client instances: once a prior client enables events, constructing a later client with disable_events = true will not actually disable event tracking, violating the caller’s expectation and the macro’s contract for being configurable per client. This is a persistent mutation of global state without scoped restoration, which can cause unexpected event writes and logs. Consider explicitly setting EVENTS_ENABLED.store(false, Ordering::SeqCst) when disable_events is true, or scoping the flag per-client instead of global. [ Out of scope ]
• line 679: enable_api_stats returns ClientBuilderError::MissingParameter { parameter: "api_client" } when either self.api_client OR self.sync_api_client is None. This produces a misleading error message when sync_api_client is missing, obscuring the actual missing parameter and hindering correct recovery. The guard should differentiate which parameter is missing or report both. [ Out of scope ]

xmtp_mls/src/client.rs — 0 comments posted, 4 evaluated, 4 filtered

• line 339: find_inbox_ids_from_identifiers silently returns None for duplicate identifiers in the input slice because it uses HashMap::remove for both cached_inbox_ids and new_inbox_ids. On the first occurrence the entry is removed and returned; on subsequent occurrences of the same identifier, remove returns None, causing data loss for valid duplicates. Use get (or get_mut without removal) instead to preserve repeated lookups. Problematic lines are where cached_inbox_ids.remove(&cache_key) and new_inbox_ids.remove(&ident.into()) are used during the final collection. [ Out of scope ]
• line 748: In delete_message, events are sent on self.context.local_events() whereas other methods in the same impl (e.g., create_group, create_dm_by_inbox_id) send on self.local_events. If these senders are not the exact same channel, deletion notifications may not reach subscribers that listen to the client’s local_events, causing inconsistent externally visible behavior. Use a single, consistent sender (self.local_events) for client-originated local events, or ensure both are the same and document it. [ Out of scope ]
• line 849: register_identity() stores a locally generated key package before verifying the provided SignatureRequest, and on verification failure it returns early without cleaning up the newly stored package. Specifically, generate_and_store_key_package(...) persists state, then build_identity_update().to_verified(...).await? may error, but there is no rollback or deletion of the stored key package in this error path. This leaves an unused key package artifact in local storage, violating the single paired cleanup invariant for effects introduced before verification succeeds. [ Low confidence ]
• line 868: register_identity() performs network-side upload_key_package(...) (Step 3) before publish_identity_update(...) (Step 4). If upload succeeds but publish fails, the function returns an error without compensating actions. This leaves an orphaned key package on the network and an inconsistent local state (no identity stored/ready), violating the effect sequencing/cleanup invariant for partial failure. There is no retry of publish in-place nor rollback of the uploaded package. [ Low confidence ]

xmtp_mls/src/groups/intents/queue.rs — 0 comments posted, 1 evaluated, 1 filtered

• line 56: queue_for_each assumes all MlsGroup<C> entries share the same context, but it only takes the context from an arbitrary first element and uses that single storage connection to queue intents for every group. If groups contains entries from different contexts/databases, intents for non-first-context groups will be queued against the wrong store, causing misrouting or partial failure without surfacing an error. There is no validation that all groups have the same context nor partitioning by context. Fix by either (a) validating all groups share the same context and returning an error otherwise, or (b) partitioning groups by context and performing a separate transaction per context. [ Low confidence ]

xmtp_mls/src/groups/mls_sync.rs — 0 comments posted, 3 evaluated, 3 filtered

• line 405: sync_with_conn returns Ok(summary) when only one of publish_errors or post_commit_errors is non-empty because SyncSummary::is_errored() is defined as self.other.is_some() || (!self.publish_errors.is_empty() && !self.post_commit_errors.is_empty()). This AND condition means a single class of errors (publish-only or post-commit-only) will not trigger an error result, causing callers to treat a partially failed sync as success. This can suppress retries and misreport sync status. The check at lines 405..409 relies on this method, so the function can incorrectly return success despite errors. [ Out of scope ]
• line 2432: elapsed_ns is computed using now_ns (line 2432) which casts as_nanos() (u128) to i64. In common/src/time.rs, now_ns() does duration_since_epoch().as_nanos() as i64, which will truncate and wrap once the epoch duration exceeds i64::MAX nanoseconds (circa year 2262). This can produce negative/incorrect now_ns, causing elapsed_ns to be wrong and suppressing or triggering updates incorrectly. Use a wider type (e.g., i128 or u128) for time arithmetic, or clamp/handle overflow explicitly before comparisons. [ Out of scope ]
• line 2435: update_interval_ns is accepted as Option<i64> without validation. If a caller supplies a negative interval, the comparison elapsed_ns > interval_ns on line 2435 will be true for most realistic elapsed_ns, causing near-always updates and potentially excessive DB writes. Reject negatives or clamp to 0 before use: e.g., let interval_ns = update_interval_ns.map_or(SYNC_UPDATE_INSTALLATIONS_INTERVAL_NS, |v| v.max(0));. [ Out of scope ]

xmtp_mls/src/identity.rs — 0 comments posted, 1 evaluated, 1 filtered

• line 622: Identity::queue_key_rotation claims in its docstring "If no key rotation is scheduled, queue it to occur in the next 5 seconds.", but the implementation unconditionally calls conn.queue_key_package_rotation()? without checking whether a rotation is already scheduled. This can result in duplicate scheduling if conn.queue_key_package_rotation is not idempotent, and the log message "Last key package not ready for rotation, queued for rotation" may be misleading. Either add a guard to check for an existing scheduled rotation before enqueuing, or update the documentation to match behavior. [ Low confidence ]

[graphite-app]

How to use the Graphite Merge Queue

Add the label mergequeue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[codecov]

Codecov Report

❌ Patch coverage is 57.27273% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.44%. Comparing base (d4f57b9) to head (b31d94f).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
xmtp_api_d14n/src/middleware/readonly_client.rs	0.00%	17 Missing ⚠️
xmtp_mls/src/client.rs	50.00%	7 Missing ⚠️
bindings_node/src/client.rs	0.00%	6 Missing ⚠️
bindings_ffi/src/mls.rs	28.57%	5 Missing ⚠️
xmtp_mls/src/utils/test/tester_utils.rs	72.22%	5 Missing ⚠️
xmtp_mls/src/context.rs	76.92%	3 Missing ⚠️
xmtp_mls/src/groups/mls_sync.rs	40.00%	3 Missing ⚠️
xmtp_mls/src/builder.rs	94.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2835      +/-   ##
==========================================
- Coverage   74.56%   74.44%   -0.13%     
==========================================
  Files         387      387              
  Lines       49959    49556     -403     
==========================================
- Hits        37254    36891     -363     
+ Misses      12705    12665      -40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[neekolas]

I'd love to understand a little more about what the callers should do in this situation. We don't really know if the the intent has succeeded or failed. I'd imagine in read only mode we aren't expecting a lot of calls to this (since it's typically triggered by writes), but I want to make sure misuse doesn't lead to someone thinking a thing worked when it didn't or leading to an infinite loop.