Fix: IMAP over SSL connected without TLS; never lose a sent message's copy
v1.3.26's IMAP4_SSL subclass put its create_client ahead of aioimaplib's,
which is where the TLS context is made — so port 993 got plain TCP, the
server waited for a handshake, and every SSL account timed out ("Can't
reach the mail server"). The context is now made in our create_client too,
and a test checks an SSL client really gets one (it fails without the fix).
With IMAP down, a send could still be delivered by SMTP while filing its copy
in Sent failed — the send then counted as done and the copy was lost. The
copy is now kept and filed by file_sent_copy_job, retried (arq Retry) for
about 24 hours, then shown in Sent as usual.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -20,6 +20,7 @@ from sqlalchemy.ext.asyncio import AsyncSession
|
|||||||
|
|
||||||
from app.db import session_scope
|
from app.db import session_scope
|
||||||
from app.events import get_event_bus
|
from app.events import get_event_bus
|
||||||
|
from app.indexer import blobstore
|
||||||
from app.indexer.indexer import persist_message
|
from app.indexer.indexer import persist_message
|
||||||
from app.models import Account, Draft, KnownRecipient, Mailbox, Message, Outbox
|
from app.models import Account, Draft, KnownRecipient, Mailbox, Message, Outbox
|
||||||
from app.outbox.attachments import load_attachments
|
from app.outbox.attachments import load_attachments
|
||||||
@@ -28,6 +29,7 @@ from app.outbox.service import delete_mirrored_draft_row
|
|||||||
from app.providers.base import RemoteMessage
|
from app.providers.base import RemoteMessage
|
||||||
from app.queue import enqueue_job
|
from app.queue import enqueue_job
|
||||||
from app.search.factory import get_search_backend
|
from app.search.factory import get_search_backend
|
||||||
|
from app.sync.actions import _retry_later
|
||||||
from app.sync.health import is_temporary
|
from app.sync.health import is_temporary
|
||||||
from app.sync.provider_factory import build_provider
|
from app.sync.provider_factory import build_provider
|
||||||
|
|
||||||
@@ -167,6 +169,37 @@ async def _send_failed(
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
async def file_sent_copy_job(ctx: dict[str, Any], account_id: str, user_id: str, sha: str) -> None:
|
||||||
|
"""File a sent message's copy in Sent, after the send itself succeeded but
|
||||||
|
the IMAP server could not be reached. Retried (arq Retry) for ~24 hours."""
|
||||||
|
uid = uuid.UUID(user_id)
|
||||||
|
async with session_scope() as session:
|
||||||
|
account = await session.get(Account, uuid.UUID(account_id))
|
||||||
|
if account is None:
|
||||||
|
return
|
||||||
|
sent_path = await session.scalar(
|
||||||
|
select(Mailbox.path).where(
|
||||||
|
Mailbox.account_id == account.id, Mailbox.special_use == "sent"
|
||||||
|
)
|
||||||
|
)
|
||||||
|
if not sent_path:
|
||||||
|
return
|
||||||
|
raw = blobstore.load(uid, sha)
|
||||||
|
provider = build_provider(account)
|
||||||
|
try:
|
||||||
|
appended_uid = await provider.append(sent_path, raw, frozenset({"\\Seen"}))
|
||||||
|
except Exception as exc:
|
||||||
|
retry = _retry_later(ctx, exc)
|
||||||
|
if retry is not None:
|
||||||
|
raise retry from exc
|
||||||
|
logger.error("could not file a sent message in Sent (blob %s): %s", sha, exc)
|
||||||
|
return
|
||||||
|
finally:
|
||||||
|
await provider.close()
|
||||||
|
await _persist_sent_copy(uid, account, raw, appended_uid)
|
||||||
|
logger.info("filed a sent message in Sent for account %s", account_id)
|
||||||
|
|
||||||
|
|
||||||
async def send_job(ctx: dict[str, Any], outbox_id: str) -> None:
|
async def send_job(ctx: dict[str, Any], outbox_id: str) -> None:
|
||||||
"""Send a queued message unless it was cancelled during the undo window.
|
"""Send a queued message unless it was cancelled during the undo window.
|
||||||
|
|
||||||
@@ -229,16 +262,22 @@ async def send_job(ctx: dict[str, Any], outbox_id: str) -> None:
|
|||||||
except Exception as exc: # noqa: BLE001 - retry or record; the draft is kept
|
except Exception as exc: # noqa: BLE001 - retry or record; the draft is kept
|
||||||
await _send_failed(oid, user_id, subject, exc)
|
await _send_failed(oid, user_id, subject, exc)
|
||||||
return
|
return
|
||||||
# Delivered. Filing it is housekeeping: if the IMAP server is not there
|
# Delivered. If filing the copy in Sent fails (the IMAP server is not
|
||||||
# right now, the send still succeeded (the Sent poll files it later).
|
# there right now), the send still succeeded — and the copy must not be
|
||||||
try:
|
# lost: it is kept and filed later by file_sent_copy_job.
|
||||||
if sent_path and not gmail:
|
if sent_path and not gmail:
|
||||||
|
try:
|
||||||
appended_uid = await provider.append(sent_path, sent_raw, frozenset({"\\Seen"}))
|
appended_uid = await provider.append(sent_path, sent_raw, frozenset({"\\Seen"}))
|
||||||
# Remove the mirrored Drafts-folder copy now that it has been sent.
|
except Exception as exc: # noqa: BLE001
|
||||||
if drafts_path and draft_imap_uid:
|
logger.warning("outbox %s sent; filing it in Sent failed, will retry: %s", oid, exc)
|
||||||
|
sha = blobstore.store(user_id, sent_raw)
|
||||||
|
await enqueue_job("file_sent_copy_job", str(account_obj.id), str(user_id), sha)
|
||||||
|
# Remove the mirrored Drafts-folder copy now that it has been sent.
|
||||||
|
if drafts_path and draft_imap_uid:
|
||||||
|
try:
|
||||||
await provider.delete_messages(drafts_path, [draft_imap_uid])
|
await provider.delete_messages(drafts_path, [draft_imap_uid])
|
||||||
except Exception as exc: # noqa: BLE001
|
except Exception as exc: # noqa: BLE001
|
||||||
logger.warning("outbox %s sent, but filing it on the server failed: %s", oid, exc)
|
logger.warning("outbox %s sent; removing its Drafts copy failed: %s", oid, exc)
|
||||||
finally:
|
finally:
|
||||||
await provider.close()
|
await provider.close()
|
||||||
|
|
||||||
|
|||||||
@@ -11,6 +11,7 @@ from __future__ import annotations
|
|||||||
import asyncio
|
import asyncio
|
||||||
import contextlib
|
import contextlib
|
||||||
import re
|
import re
|
||||||
|
import ssl
|
||||||
from collections.abc import AsyncIterator
|
from collections.abc import AsyncIterator
|
||||||
from dataclasses import dataclass
|
from dataclasses import dataclass
|
||||||
from datetime import date
|
from datetime import date
|
||||||
@@ -67,6 +68,11 @@ class _KeepConnectTask:
|
|||||||
def create_client( # type: ignore[no-untyped-def]
|
def create_client( # type: ignore[no-untyped-def]
|
||||||
self, host, port, loop, conn_lost_cb=None, ssl_context=None
|
self, host, port, loop, conn_lost_cb=None, ssl_context=None
|
||||||
) -> None:
|
) -> None:
|
||||||
|
# This replaces IMAP4_SSL.create_client too (it comes first in the MRO),
|
||||||
|
# so do its one job here: without a context the connection is plain
|
||||||
|
# TCP to port 993, and the server just waits for a TLS handshake.
|
||||||
|
if ssl_context is None and isinstance(self, aioimaplib.IMAP4_SSL):
|
||||||
|
ssl_context = ssl.create_default_context(ssl.Purpose.SERVER_AUTH)
|
||||||
local_loop = loop if loop is not None else asyncio.get_running_loop()
|
local_loop = loop if loop is not None else asyncio.get_running_loop()
|
||||||
self.protocol = aioimaplib.IMAP4ClientProtocol(local_loop, conn_lost_cb)
|
self.protocol = aioimaplib.IMAP4ClientProtocol(local_loop, conn_lost_cb)
|
||||||
self.connect_task = local_loop.create_task(
|
self.connect_task = local_loop.create_task(
|
||||||
|
|||||||
@@ -11,7 +11,9 @@ from datetime import UTC, datetime, timedelta
|
|||||||
from email.policy import default as default_policy
|
from email.policy import default as default_policy
|
||||||
|
|
||||||
import httpx
|
import httpx
|
||||||
|
import pytest
|
||||||
import pytest_asyncio
|
import pytest_asyncio
|
||||||
|
from arq import Retry
|
||||||
from httpx import ASGITransport
|
from httpx import ASGITransport
|
||||||
from sqlalchemy import delete, select
|
from sqlalchemy import delete, select
|
||||||
from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine
|
from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine
|
||||||
@@ -615,6 +617,24 @@ async def test_delivered_but_not_filed_counts_as_sent(ctx: Ctx, monkeypatch) ->
|
|||||||
await outbox_jobs.send_job({"job_try": 1}, str(oid))
|
await outbox_jobs.send_job({"job_try": 1}, str(oid))
|
||||||
assert (await _outbox(ctx, oid)).state == "sent" and len(fake.sent) == 1
|
assert (await _outbox(ctx, oid)).state == "sent" and len(fake.sent) == 1
|
||||||
|
|
||||||
|
# The copy is not lost: kept, and a job files it once IMAP answers again.
|
||||||
|
args, _ = ctx.enqueued[-1]
|
||||||
|
assert args[0] == "file_sent_copy_job"
|
||||||
|
_, account_id, user_id, sha = args
|
||||||
|
with pytest.raises(Retry): # still unreachable: tried again later
|
||||||
|
await outbox_jobs.file_sent_copy_job({"job_try": 1}, account_id, user_id, sha)
|
||||||
|
fake.append_error = None
|
||||||
|
await outbox_jobs.file_sent_copy_job({"job_try": 2}, account_id, user_id, sha)
|
||||||
|
assert fake.appended == ["Sent"]
|
||||||
|
ctx.session.expunge_all()
|
||||||
|
async with session_scope() as verify:
|
||||||
|
sent_msg = await verify.scalar(
|
||||||
|
select(Message)
|
||||||
|
.join(Mailbox, Mailbox.id == Message.mailbox_id)
|
||||||
|
.where(Mailbox.account_id == ctx.account_id, Mailbox.special_use == "sent")
|
||||||
|
)
|
||||||
|
assert sent_msg is not None and sent_msg.subject == "Re: Hi"
|
||||||
|
|
||||||
|
|
||||||
async def test_two_jobs_for_one_send_send_once(ctx: Ctx, monkeypatch) -> None: # type: ignore[no-untyped-def]
|
async def test_two_jobs_for_one_send_send_once(ctx: Ctx, monkeypatch) -> None: # type: ignore[no-untyped-def]
|
||||||
oid = await _make_outbox(ctx, state="retrying")
|
oid = await _make_outbox(ctx, state="retrying")
|
||||||
|
|||||||
@@ -2,6 +2,7 @@
|
|||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import contextlib
|
||||||
import uuid
|
import uuid
|
||||||
from collections.abc import AsyncIterator
|
from collections.abc import AsyncIterator
|
||||||
|
|
||||||
@@ -125,3 +126,27 @@ async def test_refused_sign_in_stops_syncing(account_id: uuid.UUID) -> None:
|
|||||||
await health.failed(ProviderAuthError("AUTHENTICATIONFAILED"))
|
await health.failed(ProviderAuthError("AUTHENTICATIONFAILED"))
|
||||||
account = await _account(account_id)
|
account = await _account(account_id)
|
||||||
assert account.state == "auth_failed" and "AUTHENTICATIONFAILED" in account.state_detail
|
assert account.state == "auth_failed" and "AUTHENTICATIONFAILED" in account.state_detail
|
||||||
|
|
||||||
|
|
||||||
|
async def test_ssl_connections_really_use_tls(monkeypatch) -> None: # type: ignore[no-untyped-def]
|
||||||
|
"""Regression (v1.3.26): our IMAP4_SSL subclass skipped aioimaplib's TLS
|
||||||
|
setup, so port 993 got plain TCP and every account timed out."""
|
||||||
|
import asyncio
|
||||||
|
import ssl
|
||||||
|
|
||||||
|
from app.providers.imap_smtp.provider import _IMAP4, _IMAP4_SSL
|
||||||
|
|
||||||
|
seen: list[object] = []
|
||||||
|
loop = asyncio.get_running_loop()
|
||||||
|
|
||||||
|
async def fake_create_connection(factory, host, port, ssl=None): # type: ignore[no-untyped-def]
|
||||||
|
seen.append(ssl)
|
||||||
|
raise ConnectionRefusedError
|
||||||
|
|
||||||
|
monkeypatch.setattr(loop, "create_connection", fake_create_connection)
|
||||||
|
for cls in (_IMAP4_SSL, _IMAP4):
|
||||||
|
client = cls(host="imap.example.com", port=993, timeout=1)
|
||||||
|
with contextlib.suppress(ConnectionRefusedError):
|
||||||
|
await client.connect_task
|
||||||
|
assert isinstance(seen[0], ssl.SSLContext) # SSL: a TLS context
|
||||||
|
assert seen[1] is None # plain / STARTTLS: none up front
|
||||||
|
|||||||
@@ -15,7 +15,7 @@ from typing import Any
|
|||||||
from arq import func
|
from arq import func
|
||||||
|
|
||||||
from app.outbox.draft_sync import push_draft_to_imap_job, remove_imap_draft_job
|
from app.outbox.draft_sync import push_draft_to_imap_job, remove_imap_draft_job
|
||||||
from app.outbox.jobs import send_job
|
from app.outbox.jobs import file_sent_copy_job, send_job
|
||||||
from app.sync.actions import (
|
from app.sync.actions import (
|
||||||
ACTION_MAX_TRIES,
|
ACTION_MAX_TRIES,
|
||||||
delete_thread_job,
|
delete_thread_job,
|
||||||
@@ -48,6 +48,7 @@ _RETRYING_ACTIONS: list[Any] = [
|
|||||||
restore_thread_job,
|
restore_thread_job,
|
||||||
delete_thread_job,
|
delete_thread_job,
|
||||||
empty_folder_job,
|
empty_folder_job,
|
||||||
|
file_sent_copy_job,
|
||||||
]
|
]
|
||||||
|
|
||||||
INTERACTIVE_FUNCTIONS: list[Any] = [
|
INTERACTIVE_FUNCTIONS: list[Any] = [
|
||||||
|
|||||||
Reference in New Issue
Block a user