feat(xmtp_mls): report app data changes and guard updates against clobbering

[tylerhawkes]

Stack

Merge bottom-up — each PR is based on its parent below.
Android is the priority path: #4019 → #4011 → #4012 → #4016.

• feat(xmtp_db): add IntentState::Superseded for abandoned guarded intents #4019 db — IntentState::Superseded (base: main)
  • feat(xmtp_mls): report app data changes and guard updates against clobbering #4011 xmtp_mls — app-data change callbacks + compare-and-swap guard ← this PR
    • feat(bindings_mobile): expose the app data change callback and update guard #4012 bindings/mobile · Android path
      • feat(android-sdk): surface app data change callbacks and the update guard #4016 sdks/android · Android path
      • feat(ios-sdk): surface app data change callbacks and the update guard #4015 sdks/ios
    • feat(bindings_node): expose the app data change callback and update guard #4013 bindings/node
      • feat(node-sdk): surface app data change callbacks and the update guard #4017 sdks/js/node-sdk
    • feat(bindings_wasm): expose the app data change callback and update guard #4014 bindings/wasm

Already merged: #4018 (proto regen). Related follow-up, independent of this stack: #4020.

---

Reporting changes

ClientBuilder::unstable_change_callbacks → XmtpMlsLocalContext. Registration is construction-time by necessity: remote changes surface from the stream and sync paths, where no SDK method is on the stack to carry a parameter. A per-call hook would only cover changes you caused yourself.

UnstableChangeCallbacks has one field today; callbacks for the other mutable fields land as additive fields later.

MlsGroup::process_message snapshots the group's mutable metadata before and after processing, inside the lock closure, and diffs the app_data slot — both reads in-memory off the already-loaded group, gated on a callback being registered. That is deliberately not an out-param threaded through the intent state machine: it reports the net change, covers the own-intent and external paths identically, and generalizes to the other fields without touching commit processing again.

Dispatch happens after load_mls_group_with_lock_async returns — lock released, transaction committed — so a callback may publish its merged result on the same group. Dispatching from inside save_transcript_message would hold a write transaction across a hop into Swift/Kotlin/JS and deadlock the moment the host called update_app_data. DeferredEvents looks like the natural hook and is not: its send_all runs inside the lock.

Both reads must succeed before a change is reported. Comparing a good before against a failed after would report a clear that never happened, and a host trusting it would write the slot back from stale state.

Guarding updates

update_app_data takes an optional expected_app_data. UpdateMetadataIntentData freezes an absolute value at queue time, and an intent that loses an epoch race is rebuilt from that same frozen payload and republished — so without a guard a concurrent change is observed and then silently overwritten. test_pending_local_intent_clobbers_a_remote_change pins that behavior; test_guarded_update_is_abandoned_instead_of_clobbering shows the guard preventing it.

The authoritative check runs in get_publish_intent_data on every publish attempt, including the republish after an epoch loss. A mismatch marks the intent Superseded and returns Ok(None) — terminal without burning publish attempts or aborting the publish loop for other intents on the group. A synchronous pre-flight in update_app_data fails the already-stale case without a network round trip.

Passing None keeps last-writer-wins, so nothing existing changes behavior.

Two things worth a reviewer's attention:

• The Ok(None) arm of publish_intents no longer blindly marks an intent Processed. It would otherwise overwrite the Superseded state set moments earlier and report a dropped write to the caller as success — a silent no-op, worse than the clobber it replaces. It now only advances an intent still in ToPublish. The full xmtp_mls suite (724 tests) passes with that change.
• A failed metadata read is never treated as a guard violation. An unreadable group is not evidence that someone else wrote the field.

Known trade-off

The change payload carries no actor identity, so the callback fires for local commits as well as remote ones and merges must be idempotent. Documented on the trait and pinned by a test.

Not covered

The welcome path (groups/welcomes/xmtp_welcome.rs) fires nothing — joining never goes through process_message, so first sight of app_data on join is silent. Worth a follow-up for clients that reconcile on join.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AdY7WKkNbJmdzpmUWvW1my

Note

Add guarded app_data updates and change callbacks to MLS group sync

• Introduces UnstableChangeCallbacks and AppDataChangeCallback in change_callbacks.rs, allowing callers to register async callbacks that fire when app_data changes during sync or message processing.
• Adds an expected_field_value compare-and-swap guard to UpdateMetadataIntentData; if the committed value differs at publish time, the intent is marked Superseded instead of overwriting concurrent changes.
• update_app_data now accepts an Option<String> guard parameter; stale guards are rejected pre-queue or at publish time and return a new GroupError::AppDataSuperseded variant.
• Callbacks are dispatched after releasing the per-group sync mutex to prevent deadlocks and allow the callback to publish back into the same group.
• Risk: update_app_data signature change is breaking for all call sites (bindings and apps updated to pass None).

^{Macroscope summarized c625372.}

[claude]

Claude finished @tylerhawkes's task —— View job

---

Code Review — August 19, 2026, 20:05 UTC

Reviewed PR #4011: app-data change callbacks and CAS guards

Summary

Strong implementation overall. The change callback mechanism is well-designed with proper lock ordering, and the CAS guard correctly prevents epoch-race clobbering. Test coverage is comprehensive and documents the key behaviors. Macroscope has already identified the major correctness issues; my review focuses on additional concerns not yet covered.

---

Key Findings

1. Unclear semantics for initially-absent app_data (Medium severity)

The CAS guard cannot distinguish "expect the slot to be absent" from "disable the guard." Both map to None:

// groups/intents.rs:234
pub fn new_update_app_data(app_data: String, expected_app_data: Option<String>) -> Self {
    match expected_app_data {
        Some(expected) => Self::new_guarded(...),
        None => Self::new(...),  // unguarded last-writer-wins
    }
}

This creates a gap: a host reacting to AppDataChange { old_value: None, .. } cannot safely guard its first write. Passing None disables the guard entirely, allowing a concurrent first write to be silently overwritten.

Recommendation: Use a nested option or enum to represent three states: NoGuard, ExpectAbsent, ExpectValue(String). The comparison in get_publish_intent_data should handle ExpectAbsent by checking committed.is_none().

Macroscope also flagged this (groups/mod.rs:2342, groups/intents.rs:238), but it's worth emphasizing: this is a semantic hole in the CAS contract that undermines the feature's core value prop.

---

2. Silent read failures on metadata fields (Low severity)

read_metadata_field (presumably in a non-truncated section) and read_app_data_slot return None on read errors, which the guard check treats as "no mismatch." A corrupted extension or malformed component won't block the intent:

// groups/mls_sync.rs:3313-3315
if let Some(committed) = Self::read_metadata_field(openmls_group, &metadata_intent.field_name)
    && committed.as_deref() != Some(expected.as_str())
{
    // abandon
}

If read_metadata_field returns None due to corruption, the guard never fires — the intent proceeds despite the group being unreadable.

Recommendation: Distinguish read failure from absent value. Return Result<Option<String>> from the read helper, and propagate errors instead of treating them as successful reads of an unset field. Alternatively, log a warning and treat corruption as a non-violation (as you do now), but document this behavior explicitly so it's a known trade-off rather than an oversight.

---

3. Test coverage: welcome path is silent (Low severity — already documented)

The PR description notes that joining a group via welcome never fires the callback, since welcomes bypass process_message. This is documented and intentional, but it means a host using the callback for reconciliation will miss the initial state on join.

Recommendation: File a follow-up issue to add a join-time callback or explicitly fire on_app_data_changed from the welcome path with old_value: None. This isn't blocking for this PR, but it should be tracked so the callback contract is complete.

---

4. Callback dispatch races with commit log observer (Low severity)

dispatch_app_data_changes awaits callbacks serially, post-lock and post-commit. A slow callback delays processing of subsequent messages in the batch. This is by design (ordered delivery is documented), but it means a long-running callback can starve the commit log observer or delay stream delivery.

Observation: This is likely acceptable — the alternative (concurrent dispatch) would let hosts publish merges based on stale state, which is worse. Just noting it for visibility: a slow callback is a DoS vector against your own sync latency.

---

5. Missing error context on superseded intent (Low severity)

When a guard fails, get_publish_intent_data logs at info and returns Ok(None), but the error returned to the caller (GroupError::AppDataSuperseded) doesn't include the intent ID or group ID in its Display impl. Debugging a superseded write in production would benefit from that context.

Recommendation: Add group_id and intent_id fields to the AppDataSuperseded variant, or include them in the error message. The sync summary already carries the group, but the standalone error could be clearer.

---

6. Test gap: concurrent callbacks on multiple groups (Low severity)

All tests exercise a single group. A multi-group scenario where callbacks on groups A and B both try to publish back into their respective groups isn't covered. The per-group mutex should make this safe, but a test pinning it would be valuable.

Recommendation: Add a test that syncs two groups concurrently, both with callbacks that publish back. Assert that neither deadlocks and both complete successfully.

---

7. Positive: excellent test documentation

The tests in test_change_callbacks.rs are exemplary. Each test has a clear docstring explaining the behavior it pins, and the timeout test for reentrancy (test_callback_can_publish_back_into_the_same_group) is particularly good — it documents the deadlock hazard and uses a timeout to fail fast on regression.

---

Minor Observations

• change_callbacks.rs:84 — The watches_app_data helper is a nice optimization, avoiding the before/after snapshot when nothing is listening.
• mls_sync.rs:2376 — The comment explaining why read_app_data_slot reads only the APP_DATA component (to avoid being poisoned by a malformed unrelated component) is excellent. This is subtle and the comment makes it greppable.
• mls_sync.rs:3225 — Good call on the refetch-and-check in the Ok(None) path. The original "always mark Processed" would have silently reported superseded intents as success.

---

Non-Issues (for the record)

• Macroscope's deadlock finding (mls_sync.rs:2303) is already resolved: callbacks are dispatched in dispatch_app_data_changes after the mutex is released (line 2354).
• Macroscope's "malformed component suppresses callback" (mls_sync.rs:2401) is also resolved: read_app_data_slot reads only the APP_DATA component, so a corrupt ADMIN_LIST can't poison it (line 2376).
• The CAS-reporting issue (mls_sync.rs:3251 / mod.rs:2367) was already fixed in be58d1c.

---

Bottom Line

The core implementation is sound. The callback dispatch correctly avoids the deadlock, the guard prevents clobbering on republish, and the test suite documents the key contracts. The main gap is the initially-absent CAS case (finding #1), which should be addressed before stabilizing the API. The other findings are polish or follow-ups.

---

[macroscopeapp]

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR introduces a new callback system for group state changes and compare-and-swap guards against concurrent overwrites. The scope includes new async traits, callback dispatch infrastructure, and modifications to core sync/publish logic. Multiple High and Medium severity findings remain unresolved regarding potential deadlocks and edge cases in the guard mechanism.

Not approved because:

• 6 blocking correctness issues found at or above your repo's Minimum Blocking Severity

^{Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more.}

[codecov]

Codecov Report

❌ Patch coverage is 88.30409% with 20 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (tyler/appdata-cb-db@43d1402). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
crates/xmtp_mls/src/groups/mls_sync.rs	83.75%	13 Missing ⚠️
crates/xmtp_mls/src/groups/change_callbacks.rs	37.50%	5 Missing ⚠️
crates/xmtp_mls/src/builder.rs	93.33%	1 Missing ⚠️
crates/xmtp_mls/src/groups/intents.rs	97.50%	1 Missing ⚠️

Additional details and impacted files

@@                  Coverage Diff                   @@
##             tyler/appdata-cb-db    #4011   +/-   ##
======================================================
  Coverage                       ?   85.91%           
======================================================
  Files                          ?      418           
  Lines                          ?    68328           
  Branches                       ?        0           
======================================================
  Hits                           ?    58705           
  Misses                         ?     9623           
  Partials                       ?        0           

☔ View full report in Codecov by Harness.
📢 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]

🟡 Medium worker/tasks.rs:454

An older client deletes a KpLiveness task written by a newer client, permanently discarding the newer client's pending liveness work. Since run_and_reschedule_task also deletes rows for TaskOutcome::Done, this arm needs an outcome that preserves unknown task kinds rather than treating them as completed.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @crates/xmtp_mls/src/worker/tasks.rs around line 454:

An older client deletes a `KpLiveness` task written by a newer client, permanently discarding the newer client's pending liveness work. Since `run_and_reschedule_task` also deletes rows for `TaskOutcome::Done`, this arm needs an outcome that preserves unknown task kinds rather than treating them as completed.

[macroscopeapp]

🟠 High groups/mod.rs:2342

update_app_data cannot safely CAS an initially absent slot: expected_app_data = None disables the guard, while any Some(...) expectation calls self.app_data()? and turns an absent component into MissingExtension. Consequently, a host reacting to AppDataChange { old_value: None, .. } must issue an unguarded last-writer-wins update, so a concurrent first write can be silently overwritten. Use a representation that distinguishes “no guard” from “expected absent” (for example, a nested option or enum) and compare the optional slot value directly.

Also found in 1 other location(s)

crates/xmtp_mls/src/groups/intents.rs:238

new_update_app_data uses None both to mean “the caller expects the slot to be absent” and “disable the guard,” so there is no way to CAS an initially unset app_data slot. This state is explicitly reachable (AppDataChange::old_value is None when no value is set). A host reacting to that callback cannot protect its merged write: passing None creates an unguarded last-writer-wins intent, allowing a concurrent first write to be silently overwritten. The guard needs a representation that distinguishes an expected absent value from no guard (for example, a nested option or enum).

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @crates/xmtp_mls/src/groups/mod.rs around line 2342:

`update_app_data` cannot safely CAS an initially absent slot: `expected_app_data = None` disables the guard, while any `Some(...)` expectation calls `self.app_data()?` and turns an absent component into `MissingExtension`. Consequently, a host reacting to `AppDataChange { old_value: None, .. }` must issue an unguarded last-writer-wins update, so a concurrent first write can be silently overwritten. Use a representation that distinguishes “no guard” from “expected absent” (for example, a nested option or enum) and compare the optional slot value directly.

Also found in 1 other location(s):
- crates/xmtp_mls/src/groups/intents.rs:238 -- `new_update_app_data` uses `None` both to mean “the caller expects the slot to be absent” and “disable the guard,” so there is no way to CAS an initially unset `app_data` slot. This state is explicitly reachable (`AppDataChange::old_value` is `None` when no value is set). A host reacting to that callback cannot protect its merged write: passing `None` creates an unguarded last-writer-wins intent, allowing a concurrent first write to be silently overwritten. The guard needs a representation that distinguishes an expected absent value from no guard (for example, a nested option or enum).

[macroscopeapp]

🟡 Medium groups/mls_sync.rs:2305

A durable app-data change is permanently omitted from the registered callback when post_process_message fails after the transaction commits. Because line 2305 only attaches app_data_change for Ok(outcome), the cursor is already advanced and a retry cannot reconstruct or dispatch the change; preserve the processed outcome (or its diff) when post-processing fails.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @crates/xmtp_mls/src/groups/mls_sync.rs around line 2305:

A durable app-data change is permanently omitted from the registered callback when `post_process_message` fails after the transaction commits. Because line 2305 only attaches `app_data_change` for `Ok(outcome)`, the cursor is already advanced and a retry cannot reconstruct or dispatch the change; preserve the processed outcome (or its diff) when post-processing fails.

[macroscopeapp]

🟠 High groups/mls_sync.rs:3313

A guarded metadata update is published when read_metadata_field fails and returns None, so the compare-and-swap check is bypassed and the update can overwrite a committed value without verifying expected_field_value. This is especially reachable for non-app_data fields on migrated groups because the reader decodes the whole mutable-metadata composite; propagate the read error (or use a target-specific fallible reader) instead of treating an unreadable field as absent.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:

In file @crates/xmtp_mls/src/groups/mls_sync.rs around line 3313:

A guarded metadata update is published when `read_metadata_field` fails and returns `None`, so the compare-and-swap check is bypassed and the update can overwrite a committed value without verifying `expected_field_value`. This is especially reachable for non-`app_data` fields on migrated groups because the reader decodes the whole mutable-metadata composite; propagate the read error (or use a target-specific fallible reader) instead of treating an unreadable field as absent.