diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..78b2db0 --- /dev/null +++ b/.dockerignore @@ -0,0 +1,54 @@ +# What `COPY . .` in docker/Dockerfile and docker/Dockerfile.worker may not see. +# +# Without this file the whole working tree is the build context, and the +# runtime stage copies /app out of the builder - so anything in the tree is in +# the image that gets pushed. .gitignore does not help: Docker does not read it, +# and a file git ignores is exactly the kind a developer has locally (.env). + +# Secrets. `.env` holds SECRET_KEY, the key every stored AWS secret is encrypted +# under. A local build pushed to a registry would otherwise carry it. +.env +.env.* +*.pem +*.key + +# Version control and CI: history can hold what was later removed, and none of +# it runs in the container. +.git +.gitattributes +.githooks +.github +.gitmessage +.pre-commit-config.yaml + +# A host virtualenv. The image installs its own at /app/.venv, and copying a +# Windows or macOS one over it replaces working interpreters with ones that +# cannot start. +.venv +venv +**/.venv + +# Caches and build output. +__pycache__ +*.py[cod] +.pytest_cache +.mypy_cache +.ruff_cache +.coverage +htmlcov +site + +# Not part of the running service: the documentation site, its theme and +# config, working notes, the specification, and the tests. +docs +overrides +mkdocs.yml +research +SESKit_MVP.md +tests + +# Local overrides and editor files. +docker-compose.override.yml +.vscode +.idea +.DS_Store diff --git a/apps/worker/src/seskit_worker/main.py b/apps/worker/src/seskit_worker/main.py index 2cf13d3..6a48e43 100644 --- a/apps/worker/src/seskit_worker/main.py +++ b/apps/worker/src/seskit_worker/main.py @@ -7,7 +7,7 @@ from typing import Any -from arq import cron +from arq import cron, func from arq.connections import RedisSettings from seskit_core.config import get_settings from seskit_core.db import dispose_engine @@ -53,7 +53,9 @@ class WorkerSettings: poll_events, poll_inbound, recheck_identities, - send_email, + # Its own try count: the class-wide max_tries below is for jobs that + # never ask ARQ to retry them, and this one does. + func(send_email, max_tries=get_settings().EMAIL_SEND_MAX_ATTEMPTS), deliver_webhook, ] diff --git a/apps/worker/src/seskit_worker/sending.py b/apps/worker/src/seskit_worker/sending.py index 1213853..7325006 100644 --- a/apps/worker/src/seskit_worker/sending.py +++ b/apps/worker/src/seskit_worker/sending.py @@ -12,6 +12,13 @@ retry only when retrying could help: the status moves to `sending` before the call, the message id is written immediately after, and a terminal rejection fails the message rather than being attempted twice. + +**On retrying.** ARQ retries a job only when it raises :class:`arq.Retry`; any +other exception is a final failure, whatever ``max_tries`` says. A retryable +provider error therefore raises ``Retry``, and the message stays ``sending`` +between attempts - ``failed`` is not sendable, so recording it early would make +the retry skip the very message it was scheduled for. The failure is recorded +on the last attempt, when nothing is left to wait for. """ from __future__ import annotations @@ -19,6 +26,7 @@ from collections.abc import Callable from typing import Any +from arq import Retry from seskit_core.config import get_settings from seskit_core.db import get_session_factory from seskit_core.errors import APIError, ErrorType @@ -41,6 +49,11 @@ logger = get_logger(__name__) +#: Seconds to wait before attempt n+1 is ``n * RETRY_BACKOFF_SECONDS``: long +#: enough for a throttle or a brief outage to clear, short enough that the +#: three attempts finish within a few minutes. +RETRY_BACKOFF_SECONDS = 30 + #: How the job turns a provider name into an adapter. Injectable so a test can #: substitute one without patching a module attribute. ProviderBuilder = Callable[..., EmailProviderProtocol] @@ -89,7 +102,13 @@ async def send_email(ctx: dict[str, Any], email_id: str) -> str: """ factory = get_session_factory() async with factory() as session: - return await send_one(session, email_id, job_id=ctx.get("job_id")) + return await send_one( + session, + email_id, + job_id=ctx.get("job_id"), + attempt=int(ctx.get("job_try", 1)), + max_attempts=get_settings().EMAIL_SEND_MAX_ATTEMPTS, + ) async def send_one( @@ -98,11 +117,16 @@ async def send_one( *, job_id: str | None = None, build: ProviderBuilder | None = None, + attempt: int = 1, + max_attempts: int = 1, ) -> str: """Send one queued message. Returns the resulting status. - Raises on a retryable failure so ARQ tries again; returns normally on a - terminal one, because re-sending a rejected message gets the same rejection. + Raises :class:`arq.Retry` on a retryable failure while attempts remain, so + ARQ tries again; returns normally on a terminal one, because re-sending a + rejected message gets the same rejection. ``attempt`` is ARQ's 1-based + ``job_try``. The defaults make a single attempt, which is the safe reading + for a caller that does not know about retries. """ settings = get_settings() build = build or build_provider @@ -160,17 +184,26 @@ async def send_one( ) result = await provider.send_email(outbound) except APIError as error: - record_failure(email, error) + retryable = is_retryable(error) + will_retry = retryable and attempt < max_attempts + if will_retry: + # Still `sending`, so the next attempt may pick it up. The reason + # is kept for the dashboard, which would otherwise show a message + # that has been quietly stuck for minutes. + email.last_error = error.message + else: + record_failure(email, error) await session.commit() logger.info( "send_failed", email_id=email_id, error_type=error.error_type.value, - retryable=is_retryable(error), + retryable=retryable, + attempt=attempt, + will_retry=will_retry, ) - if is_retryable(error): - # Back to ARQ, which will try again on its own schedule. - raise + if will_retry: + raise Retry(defer=attempt * RETRY_BACKOFF_SECONDS) from error return EmailStatus.FAILED.value record_sent( diff --git a/docs/guides/ses-sandbox.md b/docs/guides/ses-sandbox.md index fdc38c0..05672a6 100644 --- a/docs/guides/ses-sandbox.md +++ b/docs/guides/ses-sandbox.md @@ -37,7 +37,7 @@ once all three are. Then: the kind of mail you send, your website, up to four contact addresses for AWS's reply, and AWS's acknowledgement to tick. SESKit fills in the use case for the reviewer from what it knows - that bounces and complaints come back over SNS and the addresses are -[suppressed](suppression.md), and that every message carries a +[suppressed](suppression.md), and that a message to a single recipient carries a [one-click unsubscribe](suppression.md#one-click-unsubscribe) header. The card then shows the wait, and the outcome when you refresh. If AWS asks a diff --git a/packages/core/src/seskit_core/services/production_access.py b/packages/core/src/seskit_core/services/production_access.py index 602b65f..7649bbd 100644 --- a/packages/core/src/seskit_core/services/production_access.py +++ b/packages/core/src/seskit_core/services/production_access.py @@ -220,8 +220,9 @@ def describe_use_case(mail_type: MailType, readiness: Readiness) -> str: "SESKit, a self-hosted email platform on Amazon SES " "(https://github.com/Otitodev/seskit). Recipients are the sender's own users. " "Bounce and complaint notifications are delivered by SNS to this installation " - "and the addresses are suppressed automatically; every message carries a " - f"one-click List-Unsubscribe header. {readiness.delivered_count} message(s) " + "and the addresses are suppressed automatically; a message to a single " + "recipient carries a one-click List-Unsubscribe header. " + f"{readiness.delivered_count} message(s) " "have been sent and confirmed delivered through this pipeline in the sandbox." ) diff --git a/packages/provider-aws-ses/src/seskit_provider_aws_ses/provider.py b/packages/provider-aws-ses/src/seskit_provider_aws_ses/provider.py index 472c4b9..7285cd8 100644 --- a/packages/provider-aws-ses/src/seskit_provider_aws_ses/provider.py +++ b/packages/provider-aws-ses/src/seskit_provider_aws_ses/provider.py @@ -310,9 +310,12 @@ def _needs_raw(message: OutboundEmail) -> bool: """Whether SES has to be given the assembled MIME rather than fields. Simple content is built by SES from what we pass, so anything it has no - field for - an attachment, a custom header - cannot survive that path. + field for - an attachment, a custom header, the List-Unsubscribe pair that + ``build_message`` derives from ``unsubscribe_url`` - cannot survive that + path. The last is the easy one to forget: it is not a header the caller + passed, so nothing in ``headers`` hints that it exists. """ - return bool(message.attachments or message.headers) + return bool(message.attachments or message.headers or message.unsubscribe_url) def _content(message: OutboundEmail) -> dict[str, Any]: diff --git a/tests/test_dockerfiles.py b/tests/test_dockerfiles.py index 129f7c8..28bb79e 100644 --- a/tests/test_dockerfiles.py +++ b/tests/test_dockerfiles.py @@ -228,3 +228,49 @@ def test_alembic_needs_no_configuration_of_its_own() -> None: env = (ROOT / "migrations" / "env.py").read_text("utf-8") assert "DATABASE_URL" in env assert 'set_main_option("sqlalchemy.url"' in env + + +# ----------------------------------------------------------- .dockerignore --- + +DOCKERIGNORE = ROOT / ".dockerignore" + + +def _ignored_patterns() -> set[str]: + return { + line.strip() + for line in DOCKERIGNORE.read_text(encoding="utf-8").splitlines() + if line.strip() and not line.lstrip().startswith("#") + } + + +@pytest.mark.parametrize("pattern", [".env", ".env.*", ".git", ".venv", "*.pem", "*.key"]) +def test_the_build_context_leaves_out_secrets_history_and_the_host_venv(pattern: str) -> None: + """`COPY . .` takes the whole context, and the runtime stage copies /app out + of the builder, so whatever is not ignored is in the pushed image. + + `.env` carries SECRET_KEY, which every stored AWS secret is encrypted under. + A host `.venv` would replace the image's interpreters with ones from another + operating system. + """ + assert pattern in _ignored_patterns() + + +def test_the_build_context_keeps_everything_the_build_reads() -> None: + """The opposite failure: an ignore rule that breaks the build. + + The build reads the manifests, the lockfile, the sources, the migrations and + the entrypoint, and `pyproject.toml` names `README.md`. A pattern covering + any of them would fail the build for a reason that looks unrelated. + """ + needed = { + "pyproject.toml", + "uv.lock", + "README.md", + "alembic.ini", + "apps", + "packages", + "migrations", + "docker", + "scripts", + } + assert not needed & _ignored_patterns() diff --git a/tests/test_ses_send.py b/tests/test_ses_send.py index 983ee7a..5991230 100644 --- a/tests/test_ses_send.py +++ b/tests/test_ses_send.py @@ -124,6 +124,25 @@ async def test_a_custom_header_also_forces_raw(monkeypatch: pytest.MonkeyPatch) assert b"X-Campaign: spring" in client.request["Content"]["Raw"]["Data"] +async def test_an_unsubscribe_link_forces_raw_and_reaches_the_wire( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Simple content has no field for List-Unsubscribe either. + + Only `build_message` turns `unsubscribe_url` into the two headers, so a plain + message sent as fields went out with neither: SMTP showed the one-click + button in local testing and SES silently did not in production. + """ + client = FakeSESClient() + url = "https://mail.example.com/u/token" + + await _provider(monkeypatch, client).send_email(_email(unsubscribe_url=url)) + + raw = client.request["Content"]["Raw"]["Data"] + assert f"List-Unsubscribe: <{url}>".encode() in raw + assert b"List-Unsubscribe-Post: List-Unsubscribe=One-Click" in raw + + async def test_raw_still_passes_the_destination(monkeypatch: pytest.MonkeyPatch) -> None: """Otherwise SES takes recipients from the headers, and a blind copy is deliberately not in the headers - it would simply never be delivered. diff --git a/tests/test_worker_sending.py b/tests/test_worker_sending.py index e1a3dc9..75e2599 100644 --- a/tests/test_worker_sending.py +++ b/tests/test_worker_sending.py @@ -15,6 +15,7 @@ import httpx import pytest +from arq import Retry from fakes.queue import FakeQueue from httpx import AsyncClient from seskit_core.errors import APIError, ErrorType @@ -178,19 +179,65 @@ async def test_a_terminal_rejection_fails_without_retrying( assert email.last_error == "Refused." -async def test_a_retryable_failure_is_raised_for_arq( +async def test_a_retryable_failure_asks_arq_to_retry( db_session: AsyncSession, ) -> None: - """A provider that could not be reached may well be reachable in a minute, - so the job goes back to the queue rather than giving up. + """A provider that could not be reached may well be reachable in a minute. + + ARQ retries only on `Retry` - any other exception is final - and the message + must stay sendable, or the retry would skip the message it was scheduled for. """ provider = StubProvider(error=APIError(ErrorType.PROVIDER_ERROR, "Unreachable.")) email = await _queued(db_session) - with pytest.raises(APIError): - await sending.send_one(db_session, email.id, build=_builder(provider)) + with pytest.raises(Retry): + await sending.send_one( + db_session, email.id, build=_builder(provider), attempt=1, max_attempts=3 + ) + + await db_session.refresh(email) + assert email.status == EmailStatus.SENDING.value + assert email.is_sendable + assert email.last_error == "Unreachable." + + +async def test_a_retry_after_a_transient_failure_sends_the_message( + db_session: AsyncSession, +) -> None: + """The regression: the first attempt used to mark the message `failed`, and + the second attempt then skipped it as settled.""" + failing = StubProvider(error=APIError(ErrorType.PROVIDER_ERROR, "Throttled.")) + email = await _queued(db_session) + with pytest.raises(Retry): + await sending.send_one( + db_session, email.id, build=_builder(failing), attempt=1, max_attempts=3 + ) + + working = StubProvider() + status = await sending.send_one( + db_session, email.id, build=_builder(working), attempt=2, max_attempts=3 + ) await db_session.refresh(email) + assert status == EmailStatus.SENT.value + assert email.status == EmailStatus.SENT.value + assert email.last_error is None + assert len(working.sent) == 1 + + +async def test_the_last_attempt_records_the_failure( + db_session: AsyncSession, +) -> None: + provider = StubProvider(error=APIError(ErrorType.PROVIDER_ERROR, "Unreachable.")) + email = await _queued(db_session) + + status = await sending.send_one( + db_session, email.id, build=_builder(provider), attempt=3, max_attempts=3 + ) + + await db_session.refresh(email) + assert status == EmailStatus.FAILED.value + assert email.status == EmailStatus.FAILED.value assert email.last_error == "Unreachable."