Fix the expiry of MSC4354 Sticky Events being reported differently to how the server tracks them. (#20324)

Part of: MSC4354 (Experimental Feature tracking issue #19409)

Follows: #20312 (which plumbs `received_ts` into the `Event` struct,
allowing us to calculate this correctly and sanely)

- When writing to `sticky_events`, we now use the `received_ts` stamped
onto the event to calculate the expiry,
  rather than taking a second clock reading.

- When emitting the `unsigned.sticky_duration_remaining_ms` (currently
called `unsigned.msc4354_sticky_duration_ttl_ms`) field,
we now also use the `received_ts` as a clamp on the start of the
stickiness,
causing us to calculate the same expiry timestamp as the one saved to
`sticky_events`
(**This is a bugfix, as previously we weren't using `received_ts` here
like the MSC requires.**)

- The calculation of the expiry is centralised into the `Event` struct,
which I hope you'll agree is a
  huge sanity improvement.

- We now systematically prevent spammy or rejected events from counting
as sticky.

---------

Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
Co-authored-by: Erik Johnston <erikj@element.io>

changelog.d/20324.bugfix

@@ -0,0 +1 @@
+Fix the expiry of [MSC4354 Sticky Events](https://github.com/matrix-org/matrix-spec-proposals/pull/4354) being reported differently to how the server tracks them.

rust/src/events/internal_metadata.rs

@@ -544,6 +544,11 @@ impl EventInternalMetadata {
     pub fn policy_server_spammy(&self) -> PyResult<bool> {
         Ok(self.read_inner()?.get_policy_server_spammy())
     }
+
+    /// Whether a spam checker marked this event as spammy.
+    pub fn spam_checker_spammy(&self) -> PyResult<bool> {
+        Ok(self.read_inner()?.get_spam_checker_spammy())
+    }
 }
 
 /// Helper to convert `None` to an `AttributeError` for a property getter.
@@ -656,7 +661,7 @@ impl EventInternalMetadata {
     }
 
     #[getter]
-    fn get_received_ts(&self) -> PyResult<Option<i64>> {
+    pub fn get_received_ts(&self) -> PyResult<Option<i64>> {
         Ok(self.read_inner()?.received_ts)
     }
     #[setter]

rust/src/events/mod.rs

@@ -48,7 +48,7 @@
 //!   string describing why auth rejected the event.
 //!
 
-use std::sync::Arc;
+use std::{cmp::min, sync::Arc};
 
 use anyhow::Error;
 use pyo3::{
@@ -396,10 +396,30 @@ impl Event {
         Ok(new_event)
     }
 
-    /// If this event has the `msc4354_sticky` top-level field, returns a
-    /// `SynapseDuration` representing the sticky duration. Otherwise returns
-    /// `None`.
-    fn sticky_duration(&self) -> Option<SynapseDuration> {
+    /// If this event has the `msc4354_sticky` top-level field and is eligible
+    /// to be sticky, returns a `SynapseDuration` representing the sticky duration.
+    /// Otherwise returns `None`.
+    ///
+    /// The duration is capped at 1 hour according to MSC4354.
+    /// Spammy events (according to spam checkers and policy servers) are not eligible to be sticky
+    /// so we return `None` for those.
+    ///
+    /// See [`Self::locally_sticky_until_ts`] to get the effective stickiness expiry timestamp.
+    fn sticky_duration(&self) -> PyResult<Option<SynapseDuration>> {
+        if self.internal_metadata.policy_server_spammy()?
+            || self.internal_metadata.spam_checker_spammy()?
+            || self.rejected_reason.is_some()
+        {
+            // Spammy and rejected events are not sticky.
+            //
+            // As per MSC4354:
+            // > Policy servers and similar homeserver-specific anti-spam techniques (e.g. custom spam checker modules) still apply to these events,
+            // > including events received over federation. If the anti-spam technique classifies a sticky event as spam,
+            // > it is treated as a regular non-sticky event and does not enjoy the properties that an unexpired sticky event does.
+            // > — https://github.com/matrix-org/matrix-spec-proposals/blob/35bf5589c313c81b74c60969701f4657acb9ccb6/proposals/4354-sticky-events.md?plain=1#L107
+            return Ok(None);
+        }
+
         const MAX_DURATION: SynapseDuration = SynapseDuration::from_hours(1);
 
         let sticky_obj = self
@@ -410,19 +430,50 @@ impl Event {
 
         let sticky_obj = match sticky_obj {
             Some(serde_json::Value::Object(obj)) => obj,
-            _ => return None,
+            _ => return Ok(None),
         };
 
         // Check for a valid duration field. The MSC requires `duration_ms` to
         // be a non-negative integer. If it's missing or invalid, we treat the
         // event as non-sticky by returning `None`.
-        let duration_ms = sticky_obj.get("duration_ms")?.as_u64()?;
+        let Some(duration_ms) = sticky_obj.get("duration_ms").and_then(|v| v.as_u64()) else {
+            return Ok(None);
+        };
 
         let duration = SynapseDuration::from_milliseconds(duration_ms);
 
         let duration = std::cmp::min(duration, MAX_DURATION);
 
-        Some(duration)
+        Ok(Some(duration))
+    }
+
+    /// If this event has the `msc4354_sticky` top-level field and is eligible
+    /// to be sticky (isn't spammy), returns a timestamp (in milliseconds since the epoch) of
+    /// the expiry of the event's stickiness, as seen locally on this homeserver.
+    ///
+    /// It is the caller's responsibility to check this time is not in the past.
+    fn locally_sticky_until_ts(&self) -> PyResult<Option<i64>> {
+        let Some(sticky_duration) = self.sticky_duration()? else {
+            return Ok(None);
+        };
+        let Some(received_ts) = self.internal_metadata.get_received_ts()? else {
+            // Either this event hasn't been persisted or this event is from before Synapse v0.16.0
+            // If before v0.16.0, we don't want this event to be sticky (even if someone was prescient
+            // enough to create a Sticky Event with a forged `origin_server_ts` far into the future...)
+            return Ok(None);
+        };
+
+        // Clamp to when it was received so a forged origin timestamp can't produce
+        // an event that is sticky beyond the 1 hour cap.
+        let start_ts = min(
+            self.parsed_event.common_fields.origin_server_ts,
+            received_ts,
+        );
+
+        // From this, calculate the stickiness expiry time.
+        // The `as i64` cast is safe as sticky durations are capped to an
+        // hour, which is well within the i64 range.
+        Ok(Some(start_ts + sticky_duration.as_millis() as i64))
     }
 
     fn __str__(&self) -> PyResult<String> {

rust/src/events/serialize.rs

@@ -519,18 +519,12 @@ fn serialize_event_value(
     }
 
     if config.msc4354_enabled {
-        if let Some(sticky_duration) = event.sticky_duration() {
-            // min() ensures the origin server can't claim a time in the future
-            // to exceed the stickiness duration limit.
-            //
-            // The `as i64` cast is safe as sticky duration are capped to an
-            // hour, which is well within the i64 range.
-            let expires_at = std::cmp::min(event.origin_server_ts(), time_now_ms)
-                + sticky_duration.as_millis() as i64;
-            if expires_at > time_now_ms {
+        if let Some(sticky_expiry_until) = event.locally_sticky_until_ts()? {
+            let sticky_remaining_ms = sticky_expiry_until - time_now_ms;
+            if sticky_remaining_ms > 0 {
                 unsigned.insert(
                     unsigned_field::STICKY_TTL.to_owned(),
-                    Value::Number(Number::from(expires_at - time_now_ms)),
+                    Value::Number(Number::from(sticky_remaining_ms)),
                 );
             }
         }

synapse/storage/databases/main/sticky_events.py

@@ -349,7 +349,6 @@ class StickyEventsWorkerStore(StateGroupWorkerStore, CacheInvalidationWorkerStor
 
         Skips inserting events:
             - if they are considered spammy by the policy server;
-              (unsure if correct, track: https://github.com/matrix-org/matrix-spec-proposals/pull/4354#discussion_r2727593350)
             - if they are considered spammy by a Synapse spam checker module;
             - if they are rejected;
             - if they are outliers (they should be reconsidered for insertion when de-outliered); or
@@ -372,27 +371,19 @@ class StickyEventsWorkerStore(StateGroupWorkerStore, CacheInvalidationWorkerStor
         # event, expires_at
         sticky_events: list[tuple[EventBase, int]] = []
         for ev in events:
-            # MSC: Note: policy servers and other similar antispam techniques still apply to these events.
-            # We don't filter out soft-failed events altogether (in case they get re-evaluated later),
-            # so filter out `spam_checker_spammy` events specifically as we don't want to re-evaluate _those_ later.
-            if (
-                ev.internal_metadata.policy_server_spammy
-                or ev.internal_metadata.spam_checker_spammy
-            ):
-                continue
-            # We shouldn't be passed rejected events, but if we do, we filter them out too.
-            if ev.rejected_reason is not None:
-                continue
             # We can't persist outlier sticky events as we don't know the room state at that event
             if ev.internal_metadata.is_outlier():
                 continue
-            sticky_duration = ev.sticky_duration()
-            if sticky_duration is None:
-                continue
+
             # Calculate the end time as start_time + effective sticky duration
-            expires_at = min(ev.origin_server_ts, now_ms) + sticky_duration.as_millis()
-            # Filter out already expired sticky events
-            if expires_at <= now_ms:
+            expires_at = ev.locally_sticky_until_ts()
+            if (
+                # Not a sticky event altogether
+                expires_at is None
+                or
+                # Filter out already expired sticky events
+                expires_at <= now_ms
+            ):
                 continue
 
             sticky_events.append((ev, expires_at))

synapse/synapse_rust/events.pyi

@@ -313,9 +313,26 @@ class Event:
         not affect the original."""
 
     def sticky_duration(self) -> Duration | None:
-        """If this event has the ``msc4354_sticky`` top-level field, returns a
-        ``SynapseDuration`` representing the sticky duration. Otherwise returns
-        ``None``."""
+        """
+        If this event has the `msc4354_sticky` top-level field and is eligible
+        to be sticky, returns a `SynapseDuration` representing the sticky duration.
+        Otherwise returns `None`.
+
+        The duration is capped at 1 hour according to MSC4354.
+        Spammy events (according to spam checkers and policy servers) are not eligible to be sticky
+        so we return `None` for those.
+
+        See `locally_sticky_until_ts` to get the effective stickiness expiry timestamp.
+        """
+
+    def locally_sticky_until_ts(self) -> int | None:
+        """
+        If this event has the `msc4354_sticky` top-level field and is eligible
+        to be sticky (isn't spammy), returns a timestamp (in milliseconds since the epoch) of
+        the expiry of the event's stickiness, as seen locally on this homeserver.
+
+        It is the caller's responsibility to check this time is not in the past.
+        """
 
 class ThreadAggregation:
     """The bundled thread summary for an event."""