Device Sync V3 Part 3

[codabrink]

• Add backup_options and server_url param to send_sync_request.
• Sync requests will now sync with all installations rather than just one random installation.
• Remove device sync url from client params.
• Remove automatic sync requests on new installations. Integrator must manually initiate sync request.

[claude]

Claude finished @codabrink's task —— View job

---

PR Review - Device Sync V3 Part 3

Review Date: February 19, 2026, 2:33 PM UTC

This is a significant refactoring that moves device sync configuration from client initialization to per-operation parameters. The changes are well-structured and improve flexibility. Here are my findings:

✅ Strengths

1. Better separation of concerns - Moving server_url and options from client construction to send_sync_request/send_sync_archive calls makes the API more flexible
2. Consistent API across bindings - WASM, Node, and Mobile FFI all follow the same pattern
3. Strong typing - New ArchiveOptions type with proper enum variants instead of proto i32 values
4. Good test coverage - Core sync tests verify the new flow works correctly

⚠️ Issues & Concerns

1. Pin collision risk (worker.rs:242-244, 486-487)

The pin generation uses only 5 characters (rand_string::<5>()). With multiple installations sending sync requests concurrently, there's a non-trivial collision risk:

• 5 characters from base58 = ~656 million combinations
• Birthday paradox applies when multiple devices sync simultaneously
• If a pin collides, is_reply_requested_by_installation (worker.rs:531-547) could return true for the wrong installation

Recommendation: Increase pin length to at least 8-10 characters or use UUIDs.

2. Incomplete error handling in is_reply_requested_by_installation (worker.rs:531-547)

The function iterates through ALL messages in the sync group without pagination or limits:

let messages = sync_group.find_messages(&MsgQueryArgs::default())?;

For long-lived sync groups, this could:

• Load thousands of messages into memory
• Cause performance degradation
• Risk OOM on memory-constrained devices

Recommendation: Add pagination or a reasonable time-bound query (e.g., only check messages from last 7 days).

3. Test coverage gaps (from codecov)

According to codecov, 33 lines are missing coverage:

• bindings/node/src/device_sync.rs: 18 lines (0% coverage)
• bindings/mobile/src/mls/device_sync/mod.rs: 5 lines missing
• Error paths in worker.rs: 2 lines missing

The Node bindings appear to have zero test coverage for the new send_sync_request/send_sync_archive functions.

Recommendation: Add integration tests for Node bindings similar to the mobile FFI tests.

4. Silent failures in task scheduling (worker.rs:335-351)

When a sync request is received, it schedules a task but marks the message as processed immediately (line 354-356), even before the archive is sent. If task scheduling fails silently or the task worker isn't running, the request will be "processed" but never fulfilled.

Recommendation: Only mark as processed after task completes successfully, or add logging/metrics for task scheduling failures.

5. No validation of server_url parameter

The server_url parameter in send_sync_request and send_sync_archive is accepted as any string with no validation:

• No check for valid URL format
• No check for HTTPS requirement
• Could lead to confusing runtime errors

Recommendation: Add URL validation at the binding layer or in the core function.

6. Breaking change documentation

The PR removes device_sync_server_url from client options, which is a breaking API change for integrators. The PR description mentions this, but:

• No migration guide for existing users
• Deprecated parameters are removed rather than deprecated first
• SDK tests show the pattern but don't highlight the migration path

Recommendation: Consider deprecation warnings in a prior release or add clear migration documentation.

🔍 Minor Issues

1. Inconsistent parameter naming - Some places use server_url, others sync_server_url. Standardize across the codebase.

2. Magic numbers (worker.rs:167) - sleep(Duration::from_secs(20)) has no constant or configuration. Should be configurable or at least documented.

3. Unused imports after refactor - Check for any leftover imports from removed code (use cargo clippy -- -W unused_imports).

🎯 Performance & Security

• Performance: No significant concerns beyond the unbounded message iteration mentioned above
• Security: The encryption key generation and handling looks correct. Keys are properly scoped and not logged.

📝 Summary

This is a well-executed refactoring that improves the API design. The main concerns are:

1. Pin collision risk (moderate severity)
2. Unbounded message iteration (moderate severity)
3. Test coverage gaps in Node bindings (low severity)
4. Task scheduling reliability (low severity)

The core functionality is sound, but addressing the pin collision and message iteration issues would make this production-ready.

---

[codecov]

Codecov Report

❌ Patch coverage is 86.85259% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.18%. Comparing base (28da514) to head (dc24b12).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
bindings/node/src/device_sync.rs	0.00%	18 Missing ⚠️
bindings/mobile/src/mls/device_sync/mod.rs	89.58%	5 Missing ⚠️
crates/xmtp_debug/src/app/clients.rs	0.00%	2 Missing ⚠️
crates/xmtp_mls/src/groups/device_sync/worker.rs	91.66%	2 Missing ⚠️
crates/xmtp_mls/src/tasks.rs	71.42%	2 Missing ⚠️
crates/xmtp_mls/src/utils/test/tester_utils.rs	60.00%	2 Missing ⚠️
apps/cli/cli-client.rs	0.00%	1 Missing ⚠️
crates/xmtp_mls/src/context.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3148      +/-   ##
==========================================
- Coverage   74.35%   74.18%   -0.17%     
==========================================
  Files         449      450       +1     
  Lines       55869    55893      +24     
==========================================
- Hits        41541    41467      -74     
- Misses      14328    14426      +98     

☔ 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.

[macroscopeapp]

Replace history sync URL and proto BackupOptions with explicit ArchiveOptions and server URL across device sync APIs in Rust core and all bindings as part of Device Sync V3 Part 3

Introduce ArchiveOptions and require a server URL for DeviceSyncClient.send_sync_request, remove device_sync_server_url and SyncWorkerMode in favor of DeviceSyncMode, update generated proto fields (e.g., request_id→pin), and adjust tests and bindings (WASM, Node, iOS, Android, mobile FFI) to the new signatures.

📍Where to Start

Start with the device sync worker changes in worker.rs, focusing on send_sync_request and send_archive in crates/xmtp_mls/src/groups/device_sync/worker.rs.

---

Macroscope summarized dc24b12.

[cameronvoell]

verified iOS test testDmDisappearingMessages that failed in CI is passing for me locally, so will need to look into that (test has timeouts, so probably related).

A future follow up to this PR would be to add some ios/android side tests that utilize the new sendSyncArchive and sendSyncRequest to make sure functionality all made it over the uniffi boundary as expected.

Great improvement for device sync, thanks for driving this 🙌