Repository navigation
Fix three problems found in a project review - #65
Merged
Merged
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
A review of the whole project turned up three defects that affect real mail or a real secret. One concern per commit.
eb44f88). ARQ retries a job only when it raisesarq.Retry; the worker re-raised theAPIError, which ends the job on the first try. It had also already recorded the message asfailed, which is not a sendable status, so a retry that did arrive would have skipped it. A throttle from SES therefore turned every message sent during it into a permanent failure. A retryable failure now raisesRetryand leaves the message insending;failedis recorded on the last attempt.EMAIL_SEND_MAX_ATTEMPTSwas documented but read by nothing, and now sets the limit.List-Unsubscribeon plain messages (25a1bc0). Onlybuild_messageturnsunsubscribe_urlinto the header pair, and a plain text or html message went to SES as Simple content, which has no field for it. SMTP always usedbuild_message, so Mailpit showed the header and production did not. The URL now selects the raw path.6a9c717narrows the production-access text, which claimed every message carries the header, to what the code does..dockerignore(319e821). Both Dockerfiles runCOPY . .and the runtime stage copies/appout of the builder, so a local.env(which holdsSECRET_KEY) and a host.venvwent into the image.Checks
No checkboxes: none of the template's three apply. No migration changed, no docs page was built with
mkdocs build --strict, and nothing was tried by hand.What was run locally:
ruff,ruff format,mypyand every pre-commit hook on each commit, plus the new SES test (it fails on the code before the fix) and the.dockerignoretests.What was not: the retry tests need Postgres and Redis, so CI runs them. No image was built locally, so the
.dockerignoreis untested against a real build beyond CI's API image.🤖 Generated with Claude Code