fix(xmtp_db,xmtp_mls): tolerate unknown intent kinds + app-data hardening docs

Cargo.toml

@@ -126,6 +126,7 @@ serde_json = { version = "1.0", default-features = false }
 serde_path_to_error = "0.1"
 # must stay on this version for compatibility with ed25519_dalek
 sha2 = "0.10.9"
+strum = { version = "0.27", features = ["derive"] }
 syn = { version = "2.0", features = [
   "parsing",
   "proc-macro",

crates/xmtp-workspace-hack/Cargo.toml

@@ -91,6 +91,7 @@ sha2 = { version = "0.10" }
 sha3 = { version = "0.10", default-features = false, features = ["asm", "std"] }
 slab = { version = "0.4" }
 smallvec = { version = "1", default-features = false, features = ["const_new", "serde", "union", "write"] }
+strum = { version = "0.27", features = ["derive"] }
 sync_wrapper = { version = "1", default-features = false, features = ["futures"] }
 time = { version = "0.3", features = ["formatting", "local-offset", "parsing"] }
 tokio-util = { version = "0.7", features = ["codec", "compat", "io", "rt"] }
@@ -184,6 +185,7 @@ sha2 = { version = "0.10" }
 sha3 = { version = "0.10", default-features = false, features = ["asm", "std"] }
 slab = { version = "0.4" }
 smallvec = { version = "1", default-features = false, features = ["const_new", "serde", "union", "write"] }
+strum = { version = "0.27", features = ["derive"] }
 syn = { version = "2", features = ["extra-traits", "fold", "full", "visit", "visit-mut"] }
 sync_wrapper = { version = "1", default-features = false, features = ["futures"] }
 time = { version = "0.3", features = ["formatting", "local-offset", "parsing"] }

crates/xmtp_db/Cargo.toml

@@ -46,6 +46,7 @@ prost.workspace = true
 rand.workspace = true
 serde.workspace = true
 serde_path_to_error = { workspace = true, optional = true }
+strum.workspace = true
 thiserror.workspace = true
 toml = { workspace = true, optional = true }
 tracing.workspace = true

crates/xmtp_db/src/encrypted_store/group_intent.rs

@@ -32,7 +32,18 @@ pub use types::*;
 pub type ID = i32;
 
 #[repr(i32)]
-#[derive(Debug, Clone, Copy, PartialEq, Eq, AsExpression, FromSqlRow, Serialize, Deserialize)]
+#[derive(
+    Debug,
+    Clone,
+    Copy,
+    PartialEq,
+    Eq,
+    AsExpression,
+    FromSqlRow,
+    Serialize,
+    Deserialize,
+    strum::EnumIter,
+)]
 #[diesel(sql_type = Integer)]
 pub enum IntentKind {
     SendMessage = 1,
@@ -68,6 +79,28 @@ pub enum IntentKind {
     AppDataUpdate = 12,
 }
 
+impl IntentKind {
+    /// Every kind this build knows how to deserialize, as a lazy
+    /// iterator — collect into a `Vec` only where a filter needs one.
+    /// Production queries pass these as the `allowed_kinds` filter so
+    /// rows written by a NEWER build (which may use discriminants this
+    /// build has no variant for) are excluded in SQL instead of
+    /// poisoning the whole `load()` — `FromSql` errors on unknown
+    /// discriminants, and one such row would otherwise wedge every
+    /// intent query for the group after an app downgrade. Unknown-kind
+    /// rows stay untouched in the table and resume processing when the
+    /// app is upgraded again.
+    ///
+    /// Exhaustive by construction: `strum::EnumIter` generates the
+    /// iteration over every variant, so a newly added `IntentKind` is
+    /// included automatically — which is exactly right here, since every
+    /// variant is, by definition, a kind this build can deserialize.
+    pub fn all() -> impl Iterator<Item = IntentKind> {
+        use strum::IntoEnumIterator;
+        IntentKind::iter()
+    }
+}
+
 impl std::fmt::Display for IntentKind {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         let description = match self {
@@ -746,6 +779,87 @@ pub(crate) mod tests {
         .unwrap()
     }
 
+    /// Exhaustiveness of `IntentKind::all()` is guaranteed by
+    /// `strum::EnumIter`, which iterates every variant; what it can't
+    /// check is the discriminant layout. Pin it here:
+    /// `unknown_kind_row_is_excluded_by_kind_filter` derives its future
+    /// discriminant as `all().len() + 1`, which is only "beyond every
+    /// known variant" while discriminants are exactly 1..=len with no
+    /// gaps or duplicates.
+    #[xmtp_common::test]
+    fn intent_kind_discriminants_are_contiguous() {
+        let mut discriminants: Vec<i32> = IntentKind::all().map(|k| k as i32).collect();
+        discriminants.sort_unstable();
+        let count = discriminants.len();
+        assert_eq!(
+            discriminants,
+            (1..=count as i32).collect::<Vec<_>>(),
+            "IntentKind discriminants must be exactly 1..={} with no gaps or duplicates",
+            count
+        );
+    }
+
+    /// Downgrade simulation: a row whose `kind` discriminant this build
+    /// doesn't know (written by a future version) must not poison
+    /// kind-filtered queries. Unfiltered queries still error — pinned
+    /// here so a future change to that behavior is a conscious one.
+    #[xmtp_common::test]
+    fn unknown_kind_row_is_excluded_by_kind_filter() {
+        let group_id = GroupId::generate();
+
+        with_connection(|conn| {
+            insert_group(conn, group_id);
+
+            // A known-kind intent this build must keep seeing.
+            NewGroupIntent::new_test(
+                IntentKind::SendMessage,
+                group_id,
+                rand_vec::<24>(),
+                IntentState::ToPublish,
+            )
+            .store(conn)
+            .unwrap();
+
+            // A future-kind row (discriminant beyond every known
+            // variant), inserted raw — exactly what a newer build
+            // leaves behind before an app downgrade.
+            let future_kind = IntentKind::all().count() as i32 + 1;
+            conn.raw_query(|raw_conn| {
+                diesel::insert_into(dsl::group_intents)
+                    .values((
+                        dsl::kind.eq(future_kind),
+                        dsl::group_id.eq(group_id),
+                        dsl::data.eq(rand_vec::<24>()),
+                        dsl::state.eq(IntentState::ToPublish),
+                        dsl::publish_attempts.eq(0),
+                        dsl::should_push.eq(false),
+                    ))
+                    .execute(raw_conn)
+            })
+            .unwrap();
+
+            // Kind-filtered (the production shape): unknown row is
+            // excluded in SQL, the known intent still comes back.
+            let intents = conn
+                .find_group_intents(
+                    group_id,
+                    Some(vec![IntentState::ToPublish]),
+                    Some(IntentKind::all().collect()),
+                )
+                .unwrap();
+            assert_eq!(intents.len(), 1);
+            assert_eq!(intents[0].kind, IntentKind::SendMessage);
+
+            // Unfiltered: the unknown discriminant fails row
+            // deserialization and poisons the whole query.
+            assert!(
+                conn.find_group_intents(group_id, Some(vec![IntentState::ToPublish]), None)
+                    .is_err(),
+                "unfiltered query should surface the FromSql error for unknown kinds"
+            );
+        })
+    }
+
     #[xmtp_common::test]
     fn test_store_and_fetch() {
         let group_id = GroupId::generate();

crates/xmtp_mls/src/groups/app_data/mod.rs

@@ -513,10 +513,10 @@ pub(crate) fn is_migrated_group(mls_group: &OpenMlsGroup) -> bool {
 /// [`TEST_REGISTRY_OVERRIDE`] scope the group is treated as migrated
 /// regardless of what the dict contains. This bridges the gap for
 /// tests that exercise post-bootstrap reader semantics without
-/// actually running the bootstrap commit (which writes the real
-/// `COMPONENT_REGISTRY` entry — that work lives in a follow-up
-/// branch). Production paths never hit this branch because the
-/// task-local is only initialized inside test scopes.
+/// actually running the bootstrap commit (`enable_proposals()` end to
+/// end, which writes the real `COMPONENT_REGISTRY` entry). Production
+/// paths never hit this branch because the task-local is only
+/// initialized inside test scopes.
 pub(crate) fn is_migrated_extensions(
     extensions: &openmls::extensions::Extensions<openmls::group::GroupContext>,
 ) -> bool {

crates/xmtp_mls/src/groups/mls_sync.rs

@@ -578,10 +578,13 @@ where
         tracing::instrument(level = "trace", skip_all)
     )]
     pub(crate) async fn sync_until_last_intent_resolved(&self) -> Result<SyncSummary, GroupError> {
+        // Filter to kinds this build understands: after a downgrade,
+        // rows written by a newer build would otherwise fail `FromSql`
+        // and poison the whole query (see `IntentKind::all`).
         let intents = self.context.db().find_group_intents(
             self.group_id,
             Some(vec![IntentState::ToPublish, IntentState::Published]),
-            None,
+            Some(IntentKind::all().collect()),
         )?;
 
         let Some(intent) = intents.last() else {
@@ -2823,10 +2826,11 @@ where
     pub(super) async fn publish_intents(&self) -> Result<(), GroupError> {
         let db = self.context.db();
         self.load_mls_group_with_lock_async(async |mut mls_group| {
+            // Kind-filtered for downgrade tolerance — see `IntentKind::all`.
             let intents = db.find_group_intents(
                 self.group_id,
                 Some(vec![IntentState::ToPublish]),
-                None,
+                Some(IntentKind::all().collect()),
             )?;
 
             for intent in intents {
@@ -3532,11 +3536,10 @@ where
                 // similar extension shape can't accidentally trigger
                 // the bootstrap path.
                 //
-                // NOTE: honest receivers reject the bootstrap commit
-                // until the receiver-side validator that understands
-                // `COMPONENT_REGISTRY` / `GROUP_MEMBERSHIP` writes is
-                // wired. Emitting this intent is only useful when the
-                // validator lands in the same release.
+                // Receive-side validation lives in
+                // `validated_commit.rs` (`is_bootstrap_commit` routes
+                // into `validate_bootstrap_and_build`, which drives
+                // `bootstrap_validator::validate_bootstrap_commit`).
                 let intent_data =
                     ProposeGroupContextExtensionsIntentData::try_from(intent.data.as_slice())?;
 
@@ -3947,7 +3950,12 @@ where
     pub(crate) async fn post_commit(&self) -> Result<(), GroupError> {
         let db = self.context.db();
         let intents =
-            db.find_group_intents(self.group_id, Some(vec![IntentState::Committed]), None)?;
+            // Kind-filtered for downgrade tolerance — see `IntentKind::all`.
+            db.find_group_intents(
+                self.group_id,
+                Some(vec![IntentState::Committed]),
+                Some(IntentKind::all().collect()),
+            )?;
 
         for intent in intents {
             if let Some(post_commit_data) = intent.post_commit_data {

crates/xmtp_mls_common/src/app_data/registry_table.rs

@@ -89,6 +89,23 @@ use crate::app_data::{
 ///   pre-commit registry snapshot, so a same-commit registration
 ///   followed by a same-commit write fails to dispatch.
 ///
+/// **Floor-bump convention (pause, don't fork).** Any release that
+/// introduces something old receivers cannot interpret — a new
+/// [`ComponentType`], a new set/map delta mutation tag, a new registry
+/// entry format, a reserved-range (`0xFF00+`) allocation, or a change
+/// to the bootstrap synthesis encoding — MUST raise
+/// `PROPOSALS_MIN_PROTOCOL_VERSION` in the same release AND land each
+/// group's `MIN_SUPPORTED_PROTOCOL_VERSION` floor bump in a commit
+/// **strictly earlier** than the first commit using the new construct
+/// (the floor-bump commit itself must contain nothing format-novel).
+/// Receivers below a committed floor pause the group
+/// (defer-and-reprocess after upgrade) via the pause-before-parse
+/// guards in `xmtp_mls::groups::app_data` and
+/// `ValidatedCommit::from_staged_commit`; a same-commit floor bump is
+/// NOT protected — its proposal hasn't passed the super-admin policy
+/// check when the guards run, and pausing on unvalidated input would
+/// let any member freeze a group.
+///
 /// Two ergonomic patterns for shipping a new component without
 /// editing `WELL_KNOWN`:
 ///

docs/app-data-validator-remediation.md

@@ -0,0 +1,85 @@
+# App-data validator remediation runbook
+
+What to do when a **fielded release ships a receive-side validation bug** on the
+app-data (post-migration) path — a validator that wrongly rejects commits other
+versions accept, or wrongly accepts commits it shouldn't. The goal is always the
+same: convert divergence into **pause** (recoverable by upgrading) and never into
+**fork** (permanent group split). No wire-format migration is required for any
+scenario below.
+
+## The lever: per-group protocol-version floor bumps
+
+`MlsGroup::update_group_min_version` (and `update_group_min_version_to_match_self`)
+writes the group's `MIN_SUPPORTED_PROTOCOL_VERSION` floor — the legacy
+`GroupMutableMetadata` attribute pre-migration, the `0x800A` AppData component
+post-migration. Receive-side monotonicity means a floor can only rise.
+
+Any member processing a commit on a group whose **committed** floor exceeds its
+own version pauses the group (`paused_for_version`): the cursor holds, all
+subsequent commits are deferred, and after the app upgrades the sweep unpauses
+the group and reprocesses everything with fixed code. Pausing never accepts or
+rejects anything, so a wrongly-paused client always converges once upgraded.
+
+Writing the floor is super-admin-gated on groups (both members on DMs).
+
+## Scenario: version X validator wrongly REJECTS valid commits
+
+Fielded X clients fork off groups whenever the triggering commit shape appears.
+
+1. Fix the validator in version Y.
+2. Bump `PROPOSALS_MIN_PROTOCOL_VERSION` to Y so newly-migrating groups get the
+   Y floor from bootstrap.
+3. For existing migrated groups: have (super-admin) clients on Y bump each
+   group's floor to Y — e.g. wire `update_group_min_version_to_match_self` into
+   a recovery flow. X members pause **at the floor-bump commit** (its cursor
+   holds there), so they never process the bug-triggering commits mid-stream;
+   after upgrading to Y they reprocess everything with the fixed validator.
+4. X members who already rejected a commit before the floor bump landed are
+   forked; recovery for them is the normal fork-recovery path (re-add /
+   re-welcome), not this runbook.
+
+## Scenario: version X validator wrongly ACCEPTS bad state
+
+E.g. the pre-#3863 registry-poison bug: state that wedges or corrupts readers.
+
+1. Fix acceptance in Y, plus a read-side tolerance for the already-fielded bad
+   state where possible (the #3863 pattern: preserved-but-invisible registry
+   entries — bad state degrades to "that component is unavailable", not "no
+   commit validates").
+2. Floor-bump affected groups to Y as above so X members stop extending the bad
+   state and pause until they can run the tolerant readers.
+
+## Why the floor is trustworthy
+
+The pause guards read only **committed** floor state (the pre-commit dict or
+legacy GMM), never same-commit proposals — a floor value only exists after its
+super-admin policy check passed on every honest receiver. See the floor-bump
+convention in `xmtp_mls_common::app_data::registry_table` for the send-side
+rules protocol evolution must follow.
+
+**Caveat — the floor-bump commit is not self-pausing.** The commit that *raises*
+the floor is itself validated under the *old* floor: on migrated groups
+`ValidatedCommit::from_staged_commit` reads the floor from the post-commit
+dictionary overlay (which includes the bump commit's own staged `AppDataUpdate`),
+and `ProtocolVersionTooLow` is checked only *after* the other commit validators
+and policy evaluation. So an X client pauses on the *next* commit it sees under
+the raised floor — not on the bump commit itself. That means the floor-bump
+commit must be one every affected (X) version can still *accept*: if a bug in the
+bump commit's shape makes an X validator reject it before the floor check runs, X
+clients fork instead of pausing. Keep the floor-bump commit format-boring (a plain
+`MIN_SUPPORTED_PROTOCOL_VERSION` write, no new-format payloads) — this is exactly
+why the convention requires the bump to land in a strictly-earlier, boring commit.
+
+## Sharp edges
+
+- **Monotonic means permanent.** A floor of `999.0.0` written by a rogue or
+  buggy super admin pauses the group for everyone forever (send-side clamp
+  `min_version <= own pkg_version` makes accidents unlikely; a malicious super
+  admin can equally destroy a group in more direct ways).
+- **The registry must stay loadable** for the post-migration floor to be
+  readable — the entry-validation + tolerant-loader pair (#3863) is what keeps
+  this lever alive on groups carrying unexpected registry state.
+- **Same-device downgrades** below a group's committed floor pause at the next
+  commit (pause-before-parse guards, #3864); unknown-`IntentKind` rows written
+  by the newer build are excluded from outbound queries rather than wedging
+  them. Both recover on re-upgrade.