🐛(inbound) thread replies whose subject was rewritten (#765)

A reply carrying In-Reply-To to a message we already hold was still rejected when its subject differed, because the delivery path required both a reference match and an identical canonical subject. A subject edited mid-conversation therefore started a brand new thread. The import path had its own laxer rule (message-ids only), so the very same conversation was grouped differently depending on whether it was imported or received over SMTP. Both paths now share find_thread_for_message: In-Reply-To is trusted on its own (RFC 8621 §3 only allows splitting on subject, never requires it), while References — which some clients recycle to start unrelated topics — still needs a matching canonical subject. The canonicalization also accepts "Re:subject" with no space, common on mobile clients.
2026-08-17 21:25:41 +02:00 · 2026-08-11 08:33:47 +02:00
parent a8c8eb8673
commit a8d8e5b435
3 changed files with 195 additions and 94 deletions
@@ -78,82 +78,66 @@ def inbound_mailbox_lock(mailbox_id: uuid.UUID):

 def _canonicalize_subject(subject: str | None) -> str:
    """Strip leading ``Re:`` / ``Fwd:`` (and i18n variants) for thread match."""
+    # ``\s*`` after the colon, not ``\s+``: mobile clients regularly send
+    # "Re:subject" with no space, and a prefix left in place makes two
+    # messages of the same conversation compare as different subjects.
    return re.sub(
-        r"^((re|fwd|fw|rep|tr|rép)\s*:\s+)+",
+        r"^((re|fwd|fw|rep|tr|rép)\s*:\s*)+",
        "",
        (subject or "").lower(),
        flags=re.IGNORECASE,
    ).strip()


-def find_thread_for_inbound_message(
-    parsed_email: JmapEmail, mailbox: models.Mailbox
-) -> models.Thread | None:
-    """Attempt to find an existing thread for an inbound message.
-
-    Follows JMAP spec recommendations:
-    https://www.ietf.org/rfc/rfc8621.html#section-3
-    """
-    in_reply_to = first_msgid(parsed_email.get("inReplyTo"))
-    references = parsed_email.get("references") or []
-
-    all_referenced_ids = set(references)
-    if in_reply_to:
-        all_referenced_ids.add(in_reply_to)
-
-    if not all_referenced_ids:
-        return None  # No headers to match on
-
-    # Find potential parent messages in the target mailbox based on references
-    potential_parents = list(
+def _parent_messages(mime_ids: list[str], mailbox: models.Mailbox):
+    """Messages of ``mailbox`` whose Message-ID is one of ``mime_ids``, newest first."""
+    return (
        models.Message.objects.filter(
-            mime_id__in=list(all_referenced_ids),
+            mime_id__in=mime_ids,
            thread__accesses__mailbox=mailbox,
        )
        .select_related("thread")
        .order_by("-created_at")  # Prefer newer matches if multiple found
    )

-    if len(potential_parents) == 0:
-        return None  # No matching messages found by ID in this mailbox

-    # Strategy 1: Match by reference AND canonical subject
-    incoming_subject_canonical = _canonicalize_subject(parsed_email.get("subject"))
-    for parent in potential_parents:
-        parent_subject_canonical = _canonicalize_subject(parent.subject)
-        if incoming_subject_canonical == parent_subject_canonical:
-            return parent.thread  # Found a match!
-
-    # Strategy 2 (Fallback): If no subject match, return thread of the most recent parent message
-    # The list is ordered by -created_at, so the first element is the latest match.
-    return None  # potential_parents.first().thread
-
-
-def find_thread_for_import(
+def find_thread_for_message(
    parsed_email: JmapEmail, mailbox: models.Mailbox
 ) -> models.Thread | None:
-    """
-    During import, try to find an existing thread that contains messages
-    with the same subject or referenced message IDs.
-    """
+    """Attempt to find the existing thread an incoming message belongs to.

-    subject = parsed_email.get("subject", "")
+    Two levels of evidence, strongest first:
+
+    1. ``In-Reply-To`` — an explicit, unambiguous pointer to a single
+       parent. When that parent is already in the mailbox the message
+       belongs to its thread whatever the subject says: participants
+       rewrite subjects mid-conversation, and RFC 8621 §3 only *allows*
+       splitting a thread on subject, it never requires it.
+    2. ``References`` — a chain some MUAs recycle across unrelated
+       conversations (replying to an old mail to start a new topic), so a
+       matching canonical subject is required before trusting it.
+
+    Shared by the SMTP delivery path and the importers: a mailbox that is
+    imported and then kept in sync over SMTP must be threaded by one rule,
+    otherwise the same conversation splits differently on each path.
+    """
    in_reply_to = first_msgid(parsed_email.get("inReplyTo"))
-    references = parsed_email.get("references") or []
+    references = [
+        ref for ref in (parsed_email.get("references") or []) if ref != in_reply_to
+    ]

-    # First try to find a thread by message IDs
-    thread = _find_thread_by_message_ids(in_reply_to, references, mailbox)
+    if in_reply_to:
+        parent = _parent_messages([in_reply_to], mailbox).first()
+        if parent:
+            return parent.thread

-    # If no thread found by message IDs, try by subject
-    if not thread and subject:
-        # Look for threads with similar subjects
-        canonical_subject = _canonicalize_subject(subject)
-        thread = models.Thread.objects.filter(
-            subject__iregex=rf"^(re|fwd|fw|rep|tr|rép)\s*:\s*{re.escape(canonical_subject)}$",
-            accesses__mailbox=mailbox,
-        ).first()
+    if references:
+        incoming_subject_canonical = _canonicalize_subject(parsed_email.get("subject"))
+        for parent in _parent_messages(references, mailbox):
+            if _canonicalize_subject(parent.subject) == incoming_subject_canonical:
+                return parent.thread

-    return thread
+    return None


 def _create_thread(parsed_email: JmapEmail, mailbox: models.Mailbox) -> models.Thread:
@@ -183,24 +167,6 @@ def _create_thread(parsed_email: JmapEmail, mailbox: models.Mailbox) -> models.T
    return thread


-def _find_thread_by_message_ids(
-    in_reply_to: str, references: list[str], mailbox: models.Mailbox
-) -> models.Thread | None:
-    """Find thread by message IDs (``inReplyTo`` and ``references``)."""
-    if in_reply_to or references:
-        thread = models.Thread.objects.filter(
-            messages__mime_id__in=[in_reply_to] if in_reply_to else [],
-            accesses__mailbox=mailbox,
-        ).first()
-        if not thread and references:
-            thread = models.Thread.objects.filter(
-                messages__mime_id__in=references,
-                accesses__mailbox=mailbox,
-            ).first()
-        return thread
-    return None
-
-
 def _record_divergent_rcpt(
    postmark: dict, recipient_email: str, parsed_email: JmapEmail
 ) -> None:
@@ -315,14 +281,7 @@ def _create_message_from_inbound(  # pylint: disable=too-many-arguments

        # --- 3. Find or Create Thread --- #
        try:
-            thread = None
-            if is_import:
-                thread = find_thread_for_import(parsed_email, mailbox)
-
-            # If no thread found or not an import, use normal thread finding logic
-            if not thread:
-                thread = find_thread_for_inbound_message(parsed_email, mailbox)
-
+            thread = find_thread_for_message(parsed_email, mailbox)
            if not thread:
                thread = _create_thread(parsed_email, mailbox)

@@ -842,10 +842,14 @@ class TestMTAInboundEmailThreading:
        assert new_message.thread == initial_thread
        assert new_message.subject == reply_subject

-    def test_reply_creates_new_thread_different_subject(
+    def test_reply_keeps_thread_when_subject_rewritten(
        self, api_client: APIClient, valid_jwt_token
    ):
-        """Test reply creates a new thread if the canonical subject differs."""
+        """Test a reply stays in its thread even when its subject was rewritten.
+
+        In-Reply-To points at a message we hold: that link is explicit and
+        wins over the subject, which participants routinely edit.
+        """
        initial_subject = "Important Meeting"
        initial_mime_id = "meeting.789@example.com"
        initial_thread, initial_message = self._create_initial_message(
@@ -868,6 +872,44 @@ class TestMTAInboundEmailThreading:
            HTTP_AUTHORIZATION=f"Bearer {token}",
        )

+        assert response.status_code == status.HTTP_200_OK
+        assert models.Thread.objects.count() == 1  # No new thread created
+        assert models.Message.objects.count() == 2
+        new_message = models.Message.objects.exclude(id=initial_message.id).first()
+        assert new_message is not None
+        assert new_message.thread == initial_thread
+        assert new_message.subject == reply_subject
+
+    def test_references_only_creates_new_thread_different_subject(
+        self, api_client: APIClient, valid_jwt_token
+    ):
+        """Test a new topic built on a recycled References chain is split off.
+
+        Without In-Reply-To, References alone is not proof of continuity: a
+        differing canonical subject means a new thread.
+        """
+        initial_subject = "Important Meeting"
+        initial_mime_id = "meeting.790@example.com"
+        initial_thread, initial_message = self._create_initial_message(
+            initial_subject, initial_mime_id
+        )
+
+        reply_subject = "Completely Different Topic"  # Subject changed
+        reply_email_bytes = self._create_reply_email(
+            self.recipient_email, reply_subject, references=[initial_mime_id]
+        )
+
+        token = valid_jwt_token(
+            reply_email_bytes, {"original_recipients": [self.recipient_email]}
+        )
+
+        response = api_client.post(
+            "/api/v1.0/inbound/mta/deliver/",
+            data=reply_email_bytes,
+            content_type="message/rfc822",
+            HTTP_AUTHORIZATION=f"Bearer {token}",
+        )
+
        assert response.status_code == status.HTTP_200_OK
        assert models.Thread.objects.count() == 2  # New thread created
        assert models.Message.objects.count() == 2
@@ -15,7 +15,7 @@ from core.mda.inbound import deliver_inbound_message
 from core.mda.inbound_create import (
    _create_message_from_inbound,
    _record_divergent_rcpt,
-    find_thread_for_inbound_message,
+    find_thread_for_message,
 )


@@ -65,7 +65,7 @@ class TestRecordDivergentRcpt:

@pytest.mark.django_db
 class TestFindThread:
-    """Unit tests for the find_thread_for_inbound_message helper."""
+    """Unit tests for the find_thread_for_message helper."""

    mailbox = None

@@ -102,7 +102,7 @@ class TestFindThread:
            "from": [{"email": "replier@a.com"}],
        }

-        found_thread = find_thread_for_inbound_message(parsed_reply, self.mailbox)
+        found_thread = find_thread_for_message(parsed_reply, self.mailbox)
        assert found_thread == initial_thread

    @pytest.mark.parametrize(
@@ -132,7 +132,7 @@ class TestFindThread:
            "from": [{"email": "replier@a.com"}],
        }

-        found_thread = find_thread_for_inbound_message(parsed_reply, self.mailbox)
+        found_thread = find_thread_for_message(parsed_reply, self.mailbox)
        assert found_thread == initial_thread

    @pytest.mark.parametrize(
@@ -142,8 +142,12 @@ class TestFindThread:
            enums.ThreadAccessRoleChoices.VIEWER,
        ],
    )
-    def test_find_fallback_no_subject_match(self, role):
-        """Thread found via References header, falling back when subjects don't normalize."""
+    def test_references_only_with_different_subject_returns_none(self, role):
+        """References alone + an unrelated subject → no thread.
+
+        Some MUAs recycle the References chain when starting a new topic from
+        an old message, so that header on its own is not proof of continuity.
+        """
        initial_subject = "Meeting Request"
        initial_mime_id = "meeting.abc@example.com"
        initial_thread = factories.ThreadFactory(subject=initial_subject)
@@ -164,9 +168,64 @@ class TestFindThread:
        }

        # Create a new thread
-        found_thread = find_thread_for_inbound_message(parsed_reply, self.mailbox)
+        found_thread = find_thread_for_message(parsed_reply, self.mailbox)
        assert found_thread is None

+    def test_find_by_in_reply_to_with_rewritten_subject(self):
+        """In-Reply-To wins over a subject rewritten mid-conversation.
+
+        Real-world case: the third message of a conversation replies to the
+        second one but its subject was edited, which used to start a brand
+        new thread on the delivery path.
+        """
+        first_mime_id = "first.789@example.com"
+        second_mime_id = "second.789@example.com"
+        initial_thread = factories.ThreadFactory(subject="Original topic")
+        factories.ThreadAccessFactory(
+            mailbox=self.mailbox,
+            thread=initial_thread,
+            role=enums.ThreadAccessRoleChoices.EDITOR,
+        )
+        factories.MessageFactory(
+            thread=initial_thread, mime_id=first_mime_id, subject="Original topic"
+        )
+        factories.MessageFactory(
+            thread=initial_thread, mime_id=second_mime_id, subject="Re: Original topic"
+        )
+
+        parsed_reply = {
+            "subject": "Re: Renamed topic",
+            "inReplyTo": [second_mime_id],
+            "references": [first_mime_id, second_mime_id],
+            "from": [{"email": "replier@a.com"}],
+        }
+
+        found_thread = find_thread_for_message(parsed_reply, self.mailbox)
+        assert found_thread == initial_thread
+
+    def test_find_by_references_without_space_after_prefix(self):
+        """A ``Re:`` prefix with no space after the colon still canonicalizes."""
+        initial_subject = "Quarterly report"
+        initial_mime_id = "report.001@example.com"
+        initial_thread = factories.ThreadFactory(subject=initial_subject)
+        factories.ThreadAccessFactory(
+            mailbox=self.mailbox,
+            thread=initial_thread,
+            role=enums.ThreadAccessRoleChoices.EDITOR,
+        )
+        factories.MessageFactory(
+            thread=initial_thread, mime_id=initial_mime_id, subject=initial_subject
+        )
+
+        parsed_reply = {
+            "subject": f"Re:{initial_subject}",
+            "references": [initial_mime_id],
+            "from": [{"email": "replier@a.com"}],
+        }
+
+        found_thread = find_thread_for_message(parsed_reply, self.mailbox)
+        assert found_thread == initial_thread
+
    @pytest.mark.parametrize(
        "role",
        [
@@ -192,7 +251,7 @@ class TestFindThread:
            "from": [{"email": "replier@a.com"}],
        }

-        found_thread = find_thread_for_inbound_message(parsed_reply, self.mailbox)
+        found_thread = find_thread_for_message(parsed_reply, self.mailbox)
        assert found_thread is None

    @pytest.mark.parametrize(
@@ -235,7 +294,7 @@ class TestFindThread:
        }

        # Should find the thread in *our* mailbox
-        found_thread = find_thread_for_inbound_message(parsed_reply, self.mailbox)
+        found_thread = find_thread_for_message(parsed_reply, self.mailbox)
        assert found_thread == initial_thread

    @pytest.mark.parametrize(
@@ -259,7 +318,7 @@ class TestFindThread:
            # No In-Reply-To or References
            "from": [{"email": "new@a.com"}],
        }
-        found_thread = find_thread_for_inbound_message(parsed_new_email, self.mailbox)
+        found_thread = find_thread_for_message(parsed_new_email, self.mailbox)
        assert found_thread is None


@@ -292,7 +351,7 @@ class TestDeliverInboundMessage:
        domain = factories.MailDomainFactory(name="deliver.test")
        return factories.MailboxFactory(local_part="recipient", domain=domain)

-    @patch("core.mda.inbound_create.find_thread_for_inbound_message")
+    @patch("core.mda.inbound_create.find_thread_for_message")
    def test_basic_delivery_new_thread(
        self, mock_find_thread, target_mailbox, sample_parsed_email, raw_email_data
    ):
@@ -412,7 +471,7 @@ class TestDeliverInboundMessage:
            enums.ThreadAccessRoleChoices.VIEWER,
        ],
    )
-    @patch("core.mda.inbound_create.find_thread_for_inbound_message")
+    @patch("core.mda.inbound_create.find_thread_for_message")
    def test_basic_delivery_existing_thread(
        self,
        mock_find_thread,
@@ -685,6 +744,47 @@ class TestDeliverInboundMessage:
        assert thread2.subject == subject  # Make sure the original subject is kept
        assert message3.mime_id == parsed_email_3["messageId"][0]

+    def test_smtp_exchange_single_thread_with_rewritten_subject(self, target_mailbox):
+        """A reply whose subject was edited still lands in the same thread.
+
+        Delivery path only (no import): this is the case that used to split a
+        conversation in two, since the subject was required to match even when
+        In-Reply-To pointed at a message we already held.
+        """
+        recipient_addr = f"{target_mailbox.local_part}@{target_mailbox.domain.name}"
+
+        def _raw(subject: str, mime_id: str, headers: bytes = b"") -> bytes:
+            return (
+                b"From: sender@test.com\r\n"
+                b"To: " + recipient_addr.encode() + b"\r\n"
+                b"Subject: " + subject.encode() + b"\r\n"
+                b"Message-ID: <" + mime_id.encode() + b">\r\n" + headers + b"\r\nbody"
+            )
+
+        raw_1 = _raw("Initial topic", "exchange.1@example.com")
+        assert deliver_inbound_message(recipient_addr, parse_email(raw_1), raw_1)
+
+        raw_2 = _raw(
+            "Re: Initial topic",
+            "exchange.2@example.com",
+            b"In-Reply-To: <exchange.1@example.com>\r\n"
+            b"References: <exchange.1@example.com>\r\n",
+        )
+        assert deliver_inbound_message(recipient_addr, parse_email(raw_2), raw_2)
+
+        # Third message: same reply chain, but the subject was rewritten.
+        raw_3 = _raw(
+            "Re: Renamed topic",
+            "exchange.3@example.com",
+            b"In-Reply-To: <exchange.2@example.com>\r\n"
+            b"References: <exchange.1@example.com> <exchange.2@example.com>\r\n",
+        )
+        assert deliver_inbound_message(recipient_addr, parse_email(raw_3), raw_3)
+
+        threads = models.Thread.objects.filter(accesses__mailbox=target_mailbox)
+        assert threads.count() == 1
+        assert threads.get().messages.count() == 3
+
    def test_deliver_message_with_empty_subject(self, target_mailbox, raw_email_data):
        """Test delivery of message with empty subject."""
        recipient_addr = f"{target_mailbox.local_part}@{target_mailbox.domain.name}"