Fix action dedup key (#3534)

* Fix action dedup key to catch duplicate bans across events and channels

* Add info logs to action dedup cache hits and misses

Author: ChrisLovering

bot/exts/filtering/filtering.py

@@ -1092,17 +1092,21 @@ def _should_run_actions(self, ctx: FilterContext, actions: ActionSettings) -> bo
                 break
             del recent[oldest_key]
 
-        key = (
-            ctx.event.name,
+        # base_key ignores additional_actions so a events for the same user are caught
+        # even when antispam skips adding the deletion handler the second time.
+        base_key = (
             getattr(ctx.author, "id", None),
-            getattr(ctx.channel, "id", None),
             json.dumps(to_serializable(actions), sort_keys=True, default=str),
-            # So that each callable in additional_actions also forms the cache key
+        )
+        full_key = base_key + (
             tuple(sorted(getattr(a, "__qualname__", repr(a)) for a in ctx.additional_actions)),
         )
-        if key in recent:
+        if base_key in recent or full_key in recent:
+            log.info(f"Cache hit, not running actions {ctx.author} (event={ctx.event.name}): {actions}")
             return False
-        recent[key] = now
+        log.info(f"Cache miss, running actions on {ctx.author} (event={ctx.event.name}): {actions}")
+        recent[base_key] = now
+        recent[full_key] = now
         return True
 
     def _increment_stats(self, triggered_filters: dict[AtomicList, list[Filter]]) -> None:

tests/bot/exts/filtering/test_action_deduplication.py

@@ -18,9 +18,9 @@ def _make_actions(send_alert: bool) -> ActionSettings:
         return actions
 
     @staticmethod
-    def _make_context(*, author_id: int = 1, channel_id: int = 10) -> FilterContext:
+    def _make_context(*, author_id: int = 1, channel_id: int = 10, event: Event = Event.MESSAGE) -> FilterContext:
         return FilterContext(
-            event=Event.MESSAGE,
+            event=event,
             author=MockMember(id=author_id),
             channel=MockTextChannel(id=channel_id),
             content="hello",
@@ -42,6 +42,52 @@ def test_different_action_values_are_not_suppressed(self):
         self.assertTrue(self.cog._should_run_actions(ctx, first_actions))
         self.assertTrue(self.cog._should_run_actions(ctx, second_actions))
 
+    def test_same_action_payload_for_different_events_is_suppressed(self):
+        """MESSAGE followed by MESSAGE_EDIT for the same user/channel/action should not double-ban."""
+        msg_ctx = FilterContext(
+            event=Event.MESSAGE,
+            author=MockMember(id=1),
+            channel=MockTextChannel(id=10),
+            content="hello",
+            message=None,
+        )
+        edit_ctx = FilterContext(
+            event=Event.MESSAGE_EDIT,
+            author=MockMember(id=1),
+            channel=MockTextChannel(id=10),
+            content="hello",
+            message=None,
+        )
+        actions = self._make_actions(send_alert=True)
+
+        self.assertTrue(self.cog._should_run_actions(msg_ctx, actions))
+        self.assertFalse(self.cog._should_run_actions(edit_ctx, actions))
+
+    def test_differing_additional_actions_are_still_suppressed(self):
+        """Second event with no additional_actions (antispam already queued) should still be deduped."""
+
+        async def fake_handler(ctx: FilterContext) -> None:
+            pass
+
+        first_ctx = self._make_context()
+        first_ctx.additional_actions.append(fake_handler)
+
+        second_ctx = self._make_context()  # no additional_actions
+
+        actions = self._make_actions(send_alert=True)
+
+        self.assertTrue(self.cog._should_run_actions(first_ctx, actions))
+        self.assertFalse(self.cog._should_run_actions(second_ctx, actions))
+
+    def test_same_action_payload_across_channels_is_suppressed(self):
+        """Antispam is cross-channel; same user/action in a different channel should not double-ban."""
+        first_ctx = self._make_context(channel_id=10)
+        second_ctx = self._make_context(channel_id=20)
+        actions = self._make_actions(send_alert=True)
+
+        self.assertTrue(self.cog._should_run_actions(first_ctx, actions))
+        self.assertFalse(self.cog._should_run_actions(second_ctx, actions))
+
     def test_same_action_payload_for_different_authors_is_not_suppressed(self):
         first_ctx = self._make_context(author_id=1)
         second_ctx = self._make_context(author_id=2)