Enforce max delayed event config to be positive

If delayed events are to be disabled, it should be done by leaving the
max_event_delay_duration config unspecified, not by setting the delayed
event limit to 0.

Also add test coverage, and tweak the error thrown for
max_delay_event_ms to be consistent with the newly-added error.

Author: AndrewFerr

synapse/config/server.py

@@ -914,7 +914,9 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
                 max_event_delay_duration
             )
             if self.max_event_delay_ms <= 0:
-                raise ConfigError("max_event_delay_duration must be a positive value")
+                raise ConfigError(
+                    "Expected a positive value", ("max_event_delay_duration",)
+                )
         else:
             self.max_event_delay_ms = None
 
@@ -923,6 +925,14 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
         self.max_delayed_events_per_user: int = config.get(
             "experimental_features", {}
         ).get("msc4140_max_delayed_events_per_user", 100)
+        if (
+            not isinstance(self.max_delayed_events_per_user, int)
+            or self.max_delayed_events_per_user <= 0
+        ):
+            raise ConfigError(
+                "Expected a positive value",
+                ("experimental", "msc4140_max_delayed_events_per_user"),
+            )
 
     def has_tls_listener(self) -> bool:
         return any(listener.is_tls() for listener in self.listeners)

synapse/storage/databases/main/delayed_events.py

@@ -137,6 +137,7 @@ async def add_delayed_event(
             LimitExceededError: if the user has reached the limit of
                 how many delayed events they may have scheduled at once.
         """
+        assert limit > 0  # Should be enforced at config read time
         delay_id = _generate_delay_id()
         send_ts = Timestamp(creation_ts + delay)
 
@@ -148,21 +149,18 @@ def add_delayed_event_txn(txn: LoggingTransaction) -> Timestamp:
                 retcol="COUNT(*)",
             )
             if num_existing >= limit:
-                next_send_ms: int | None = self.db_pool.simple_select_one_onecol_txn(
+                next_send_ms: int = self.db_pool.simple_select_one_onecol_txn(
                     txn,
                     table="delayed_events",
                     keyvalues={
                         "is_processed": False,
                         "user_localpart": user_localpart,
                     },
                     retcol="MIN(send_ts)",
-                    allow_none=True,
                 )
                 e = LimitExceededError(
                     limiter_name="add_delayed_event",
-                    retry_after_ms=next_send_ms - creation_ts
-                    if next_send_ms is not None
-                    else None,
+                    retry_after_ms=next_send_ms - creation_ts,
                 )
                 e.msg = "The maximum number of delayed events has been reached."
                 raise e

tests/config/test_server.py

@@ -18,11 +18,13 @@
 #
 #
 
+
 import yaml
 
 from synapse.config._base import ConfigError, RootConfig
 from synapse.config.homeserver import HomeServerConfig
 from synapse.config.server import ServerConfig, generate_ip_set, is_threepid_reserved
+from synapse.types import JsonDict
 
 from tests import unittest
 
@@ -189,6 +191,45 @@ def test_listeners_set_correctly_open_private_ports_true(self) -> None:
 
         self.assertEqual(conf["listeners"], expected_listeners)
 
+    def test_max_delayed_events_enforces_positive(self) -> None:
+        def generate_config(value: int) -> JsonDict:
+            return {"max_event_delay_duration": value}
+
+        _read_config(generate_config(1))
+
+        with self.assertRaises(ConfigError):
+            _read_config(generate_config(0))
+
+        with self.assertRaises(ConfigError):
+            _read_config(generate_config(-1))
+
+    def test_max_delayed_events_per_user_enforces_positive(self) -> None:
+        def generate_config(value: int) -> JsonDict:
+            return {
+                "experimental_features": {"msc4140_max_delayed_events_per_user": value}
+            }
+
+        _read_config(generate_config(1))
+
+        with self.assertRaises(ConfigError):
+            _read_config(generate_config(0))
+
+        with self.assertRaises(ConfigError):
+            _read_config(generate_config(-1))
+
+
+def _read_config(config_values: JsonDict) -> None:
+    ServerConfig(RootConfig()).read_config(
+        yaml.safe_load(
+            HomeServerConfig().generate_config(
+                config_dir_path="CONFDIR",
+                data_dir_path="/data_dir_path",
+                server_name="che.org",
+            )
+        )
+        | config_values
+    )
+
 
 class GenerateIpSetTestCase(unittest.TestCase):
     def test_empty(self) -> None: