codingjoe / codingjoe/relay

Retried spam scan can deliver an outgoing message twice

Open
#225 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
Python
Stars
4
Forks
0
Avg merge
8h 24m
Merged PRs (30d)
115

Description

## Problem

`check_outgoing_spam` (`services/email/msa/tasks.py:256`) has no guard on the
message status. It loads the message, scans the stored body, writes
`spam_score`/`spam_action` (plus `HELD` when spam), and then enqueues delivery:

```python
message.save(update_fields=["spam_score", "spam_action", "status"])
if not is_spam:
deliver_message.enqueue(message_id=str(message.pk))
```

The task retries itself until the scan succeeds, hourly in the steady state
(`services/email/spam/retry.py`), so a run that fails after the write above
re-runs the whole body on the next renewal. `deliver_message`
(`services/email/msa/tasks.py:46`) checks only whether the org is suspended,
never the message status, so a second enqueue sends the message to the
recipients a second time. The DB write and the enqueue are not one unit of
work, and nothing records that a delivery was already dispatched.

Windows, in rough order of likelihood:

- The worker dies between the write and the delivery task (`docker rollout`,
OOM, redeploy).
- The reaper expires the lease on a run that already enqueued delivery (see
the separate issue about the reaper).
- A renewal run starts while a previous delivery is in flight.

## Impact

Duplicate mail to recipients, which is the failure mode customers notice and
report fastest. A narrower variant: a run can complete the write, deliver, then
fail afterwards, and the renewal scan (rspamd scores are not stable) can flip
an already delivered message from `SENT` to `HELD`, so the dashboard disagrees
with what the recipient received.

## Suggested fix

Guard the scan on the current status instead of re-deriving the outcome:

- Update conditionally (`filter(pk=..., status=PENDING).update(...)`) and only
enqueue delivery when the row actually changed, so a renewal run on a
delivered message is a no-op.
- Or record the handover explicitly (a `delivery_dispatched_at` timestamp or a
dedicated status) so the task body can tell "not yet dispatched" apart from
"already dispatched".
- Decide whether a spam verdict that changes between attempts should be able to
revise an already delivered message, or whether the first verdict wins.

## Related

- The spam-scan retry policy in `services/email/spam/retry.py`, which makes a
second run routine rather than exceptional.
- The webhook delivery issue filed alongside this one: the inbound path has the
same shape, with the added problem that the webhook id rotates per attempt.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.