Fix double interception on a queue worker's re-send

[timujinne]

Follow-up to #668, which merged before this review fix landed on the branch.

Double interception on a queue worker's re-send

intercept_and_offer_queue/2 ran intercept_before_send/2 on every call — including the call a queue worker makes to send what it dequeued. A provider whose interception is not idempotent (the obvious implementation: insert a log row) therefore logged every queued message twice, and only one of the two rows ever received the after-send update.

Interception is not required to be idempotent, so core should not rely on it. The worker now says so explicitly:

Mailer.deliver_email(email,skip_queue: true,already_intercepted: true)

skip_queue keeps its narrower meaning — "do not offer this message to the queue" — so a caller that wants a synchronous send (a test-email button, for instance) still gets tracking. already_intercepted is what suppresses the second interception; the after-send hook still runs, which is what closes out the row the message already carries.

Also here

• A return from maybe_enqueue/2 outside the callback contract still sends the message — a provider bug must not eat mail — but is now logged instead of silently falling through.
• Tests for the seam: a queuing provider double proves the offer short-circuits the send, that skip_queue sends without offering, and that already_intercepted skips interception while still running the after-send hook.
• Tests for the SMTP validation-error translations added in Add SMTP encryption, auth, certificate and timeout settings; add an optional queue hook #668 (:invalid_ca_cert, {:invalid_timeout, _}, and the security / verify_cert / auth guard clause) — that case sits outside Probe.run and its rescue, so an unmatched reason would raise CaseClauseError out of a LiveView callback.
• The GLM-5.2 review of Add SMTP encryption, auth, certificate and timeout settings; add an optional queue hook #668 that produced these fixes, under dev_docs/pull_requests/2026/668-smtp-tls-options-and-queue-hook/.

Verification

mix test test/phoenix_kit/integrations/validators_test.exs — 29 tests, 0 failures. mix test test/phoenix_kit/mailer/smtp_transport_test.exs — 28 tests, 0 failures.

The three new seam tests in mailer_test.exs are :integration (they need the DataCase repo) and were not executed here — this environment has no PostgreSQL for the core suite. They compile and load; please run them with the suite.

Exercised end to end in a host application against the matching phoenix_kit_emails branch (BeamLabEU/phoenix_kit_emails#22): a queued message produces exactly one log row and one Oban job, the job completes on the first attempt, and the same row ends at sent.

[timujinne]

External code review — GLM-5.2 (effort: max, read-only)

Run against this branch on 2026-07-29; the reviewer had the full PR diff and the working tree, no shell. Verdict and findings below are unedited. Every finding is fixed in the follow-up commit 34a98f5e in #670, and the review is also committed at dev_docs/pull_requests/2026/668-smtp-tls-options-and-queue-hook/GLM_REVIEW.md.

What changed in response:

• Major (double interception):intercept_before_send/2 no longer runs on a queue worker's re-send. The worker passes already_intercepted: true; skip_queue keeps its narrower meaning ("do not offer this message to the queue"), so a caller that wants a synchronous send still gets tracking. Core no longer depends on a provider's interception being idempotent.
• Medium (untested queue path): added a queuing provider double and three tests — the offer short-circuits the send, skip_queue sends without offering, and already_intercepted skips interception while still running the after-send hook. These are :integration tests and were not executed here: this environment has no PostgreSQL for the core suite.
• Minor (untested translations): added tests for the new Validators.smtp/1 error arms. Run: validators 29 tests, transport 28 tests, 0 failures.
• Nit (silent fall-through): a return from maybe_enqueue/2 outside the callback contract still sends the message — a provider bug must not eat mail — but it is now logged instead of being invisible.
• Deliberately unchanged: the "sent" wording at the notification / test-email call sites (queued means accepted for delivery, and the return shape is documented), and min/step on the number inputs.

---

I've completed my verification. Here is my review.

---

VERDICT: APPROVE

The SMTP rewrite is faithful (verified option-by-option against the pre-PR logic), the fail-closed :no_ca_store rule is intact and reachable only through explicit operator choice, the callback version-skew is handled correctly, and no in-repo caller breaks on the new return shape. The issues below are one latent design hazard at the new queue chokepoint and test-coverage gaps — none affect correctness of what ships (no queue provider exists in core yet).

---

Major (latent) — queued messages are intercepted twice; skip_queue does not skip interception

lib/phoenix_kit/mailer.ex:375-386 — verified by reading:

defpintercept_and_offer_queue(email,opts)doprovider=Provider.current()tracked_email=provider.intercept_before_send(email,opts)# runs unconditionallyifKeyword.get(opts,:skip_queue,false)do{:continue,tracked_email}else...

intercept_before_send/2 runs on every call, including the queue worker's drain call (skip_queue: true). So a queued-then-drained message is intercepted twice: once at enqueue (on the original email) and once at drain (on the already-tracked email). handle_after_send/2 fires once, at drain.

Failing scenario: the optional emails package implements maybe_enqueue/2 (storing the tracked email it is handed) and a worker later calls Mailer.deliver_email(email, skip_queue: true). If its intercept_before_send inserts an email-log row — the documented purpose of the seam ("logs outgoing emails") and almost certainly not idempotent — every queued message produces two log rows. I could not verify the emails package's interceptor (it is a soft dependency, absent from this repo), so the impact is inferred; the double-invocation itself is verified.

Fix (pick one): when skip_queue: true, skip intercept_before_send too (the message was already intercepted at enqueue — skip_queue already means "I am the worker re-sending an already-handled message"); or move the intercept below the queue check; or state explicitly in the maybe_enqueue/2 contract that intercept_before_send must be idempotent. The first is the smallest change and matches the existing semantics of skip_queue.

Medium — the entire queue path is untested

Verified by grep: test/ contains no reference to maybe_enqueue, skip_queue, queued:, or intercept_and_offer. The three most behaviorally significant new facts are all uncovered:

• the new success shape {:ok, %{id: ref, queued: true}} (mailer.ex:383);
• that {:queued, _} short-circuits Swoosh.Mailer.deliver/2 (no send, no handle_after_send);
• that skip_queue: true bypasses the offer and still sends.

The infrastructure is already in place — PhoenixKit.MailerTest.TrackingProvider (test/phoenix_kit/mailer_test.exs:35-77) is a Provider double that messages the test process from both hooks, and extending it with a maybe_enqueue/2 returning {:queued, "ref"} would exercise all three. Note that double deliberately omits maybe_enqueue (legal, since it is @optional_callbacks), which is exactly why the default path still tests green — and why the queue path is invisible.

Fix: add a QueuingProvider double and assert (a) deliver_email/2 returns {:ok, %{id: ref, queued: true}}, (b) no {:handle_after_send_called, _} message arrives, (c) a second call with skip_queue: true does send and does call handle_after_send.

Minor — probe_auth/1 and the new Validators.smtp/1 error-translation clauses are untested

lib/phoenix_kit/integrations/validators.ex:127-132 (the never-stays-never asymmetry) and the new case arms at validators.ex:97-118 (translating :invalid_ca_cert, {:invalid_timeout, _}, and the {:invalid_security|:invalid_verify_cert|:invalid_auth, _} guard clause with its String.replace_prefix("invalid_", "") derivation) have no tests. The existing smtp/1 describe-block (validators_test.exs:254-299) covers only an unreachable relay, a bad port, the no-AUTH pass, and the tarpit deadline. SmtpTransport.config/1 is well covered; the translation layer above it is not.

Fix: unit-test the three new arms directly against Validators.smtp/1 with malformed security/verify_cert/auth/timeout/ca_cert values.

Minor — public deliver_email/2 success shape changed; user-facing callers now equate "queued" with "sent"

Verified at the call sites: notifications/channels/email.ex:60 ({:ok, _} -> :ok), users/auth/user_notifier.ex:47 (with {:ok, _metadata} <-), and email_sending.ex:137-144 (test-email "sent to %{recipient}" flash). All match broadly, so none break — confirmed. But once a provider queues, the test-email page will flash "sent" and the notification channel will return :ok (marking the notification delivered) for a message that has only been enqueued. This is inherent to adding the seam and is documented in the moduledoc, but the "Test email sent" wording will be literally false in the queued case.

Fix: either acceptable as-is (queue ≈ accepted-for-delivery) or have these callers check queued: and adjust copy. Flagging so the API evolution is a conscious choice.

Nit — non-{:queued, _} returns from maybe_enqueue/2 silently fall through to a live send

mailer.ex:382-385: case offer_to_queue(...) do {:queued, ref} -> ...; _ -> {:continue, tracked_email} end. A provider bug returning {:error, _} (outside the :continue | {:queued, term()} spec) causes the message to be sent anyway (fail-open to send). Defensible, but it will mask provider errors. Consider logging the unexpected return in the _ arm.

Nit — loggable_sender?/1 duplicates the emails module's regex; number inputs lack bounds

email_sending.ex:190-193 copies ~r/^[^\s]+@[^\s]+\.[^\s]+$/ from the optional module's Log changeset (acknowledged in the comment). Advisory-only, so drift is low-impact, but worth a pointer comment to the source of truth. Separately, the port field (providers.ex:762, pre-existing :number) and the new timeout field now render as <input type="number"> with no min/step (integrations_ui.ex:265-275); parse_timeout rejects 0, but the UI doesn't prevent typing it. Consider min="1".

---

Verified vs. inferred

Verified by reading code: default-blank options reproduce prior behavior (traced config/2 → resolve_cacerts → transport → tls_options for every security/verify/store combination); :no_ca_store is reachable only when security ∉ {:none} and verify = :verify_peer with no PEM and no system store (smtp_transport.ex:234-245), i.e. never via auto; verify_none/none/starttls_optional are gated behind explicit select choices; the transport/5 clause ordering (:auto,465 before :auto,port) is correct; the with short-circuit correctly propagates {:ok, %{id: ref, queued: true}} through both delivery paths; no other provider declares :select/:textarea (only SMTP's new fields + the pre-existing port:number), so the setup_field rewrite cannot regress other integrations; extract_setup_attrs (integration_form.ex:575-592) is generic over setup_fields, so the new control types submit under the same field.key and need no backend change; HEEx auto-escaping makes the textarea/PEM rendering safe; sender_loggable? is assigned in mount (no KeyError).

Inferred (could not verify): the actual double-logging impact of Finding 1 (emails package is out of scope); gen_smtp's default auth value being :if_available (taken from the PR's own claim, consistent with the explicit set).

Direction

This is a sound direction for the module. Concentrating every operator knob through one pure SmtpTransport.config/1 that both the probe and the send path read keeps the single-source-of-truth property intact, and the fail-closed-then-explicitly-escapable TLS posture is exactly the right trade-off for credentials on the wire. The optional maybe_enqueue/2 callback is the correct shape for an opt-in queue, and the function_exported?/3 guard handles version skew cleanly in the direction that matters. The work needed before this is relied upon rather than before it merges: pin down the intercept-on-drain contract (Finding 1) and add the queue-path tests (Finding 2) — both are cheap now and expensive once an external provider implements the queue against an ambiguous contract.

[ddon]

@timujinne there is a conflict!