From eb44f88126a19112662f7d8d7b5a7d29dc78bc9a Mon Sep 17 00:00:00 2001 From: Otitodev <198799600+Otitodev@users.noreply.github.com> Date: Mon, 12 Oct 2026 21:32:11 +0100 Subject: [PATCH 1/4] fix(worker): retry a send that failed for a reason worth retrying A provider error classed as retryable (an SES throttle, an outage, an unreachable SMTP host) never produced a second attempt, for two reasons that hid each other. ARQ retries a job only when it raises arq.Retry. Any other exception is a final failure whatever max_tries says, so re-raising the APIError ended the job on the first try. And before raising, the job had already recorded the message as failed, which is not a sendable status; a retry that did arrive would have logged send_skipped_settled and returned. The result was that a thirty second throttle turned every message sent during it into a permanent failure with "try again in a moment" as its reason. A retryable failure now raises Retry with a growing delay and leaves the message in `sending`, with the reason kept in last_error so a stalled message is not silent. The failure is recorded on the last attempt, when nothing is left to wait for. The attempt number and the limit come from ARQ's job_try and from EMAIL_SEND_MAX_ATTEMPTS, which existed and was documented but was read by nothing; send_email is registered with that limit so the setting does what configuration.md says. The existing test only asserted that an exception escaped, which is the behaviour that was wrong. The replacements cover the retry being requested, a second attempt actually sending, and the last attempt recording the failure. They need the database and were not run locally; CI runs them. Co-Authored-By: Claude Sonnet 5.5 --- apps/worker/src/seskit_worker/main.py | 6 ++- apps/worker/src/seskit_worker/sending.py | 49 ++++++++++++++++---- tests/test_worker_sending.py | 57 +++++++++++++++++++++--- 3 files changed, 97 insertions(+), 15 deletions(-) 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/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." From 25a1bc0938515454058f58d08de7ec22c194ff74 Mon Sep 17 00:00:00 2001 From: Otitodev <198799600+Otitodev@users.noreply.github.com> Date: Mon, 12 Oct 2026 21:33:28 +0100 Subject: [PATCH 2/4] fix(provider-ses): send List-Unsubscribe on a plain message too A message with an unsubscribe link went to SES in one of two shapes, and only one of them carried it. `_needs_raw` chose raw MIME for attachments and custom headers, and everything else went as SES "Simple" content, which has fields for a subject and a body and nothing for a header. The List-Unsubscribe and List-Unsubscribe-Post pair is built by `build_message` from `unsubscribe_url`, so a plain text or html message never passed through it: the worker computed the link, handed it to the provider, and the provider dropped it. SMTP always assembles the message with `build_message`, which is why local testing through Mailpit showed the one-click header and production did not. The effect on SES was a recipient with no Unsubscribe button, who presses Report spam instead - the outcome Phase 11 exists to prevent, and one that counts against the complaint rate AWS reviews accounts on. `unsubscribe_url` now selects the raw path. The test fails on the code before this change with the message arriving as Simple content. Co-Authored-By: Claude Sonnet 5.5 --- .../src/seskit_provider_aws_ses/provider.py | 7 +++++-- tests/test_ses_send.py | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) 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_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. From 6a9c717355f5d0a9d4673ec656e94b0d79d84e5b Mon Sep 17 00:00:00 2001 From: Otitodev <198799600+Otitodev@users.noreply.github.com> Date: Mon, 12 Oct 2026 21:34:01 +0100 Subject: [PATCH 3/4] docs: say which messages carry List-Unsubscribe, not all of them The use case SESKit writes for the AWS reviewer, and the guide describing it, said every message carries a one-click List-Unsubscribe header. It is a claim made to the party deciding whether to lift the sandbox, and it was broader than the behaviour: `unsubscribe_link` deliberately returns nothing when there is no public URL, when a message has more than one recipient (one token names one address), or when nothing parses as an address. It now says a message to a single recipient carries the header. Narrower, and true of what the code does, which is the only version worth sending a reviewer. Co-Authored-By: Claude Sonnet 5.5 --- docs/guides/ses-sandbox.md | 2 +- packages/core/src/seskit_core/services/production_access.py | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) 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." ) From 319e821788954156e623b490ca448ce57176efe4 Mon Sep 17 00:00:00 2001 From: Otitodev <198799600+Otitodev@users.noreply.github.com> Date: Mon, 12 Oct 2026 21:35:06 +0100 Subject: [PATCH 4/4] fix(docker): keep secrets and the host venv out of the build context There was no .dockerignore, so `docker build` sent the whole working tree as context and both Dockerfiles ran `COPY . .`. The runtime stage then copies /app out of the builder, which means anything in the tree was in the image: a local `.env`, `.git`, the specification, the research notes, and the caches. `.env` is the one that matters. It holds SECRET_KEY, the key every stored AWS secret is encrypted under, and .gitignore does nothing here because Docker does not read it. Anyone who built locally and pushed the result to a registry would have published it. A host `.venv` is the quieter problem: it is copied over the image's own /app/.venv and replaces working interpreters with ones built for another operating system. The new file excludes secrets, version control and CI, host virtualenvs, caches, and what the running service never reads (the documentation site and its theme, working notes, the specification, the tests). The only references to docs/ in the source are comments, and the build needs only the manifests, the lockfile, README.md (named by pyproject.toml), the sources, the migrations, alembic.ini and the entrypoint, all of which are kept. Two tests hold it: the entries that matter stay listed, and nothing the build reads becomes ignored - the second guards the other way this can fail, which looks like an unrelated build error. They are text over the file, in keeping with the rest of test_dockerfiles.py. No image was built locally; CI builds the API image. Co-Authored-By: Claude Sonnet 5.5 --- .dockerignore | 54 +++++++++++++++++++++++++++++++++++++++ tests/test_dockerfiles.py | 46 +++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+) create mode 100644 .dockerignore 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/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()