Threadmill reaper fails lease-expired tasks and deletes the key, so no retry ever runs
- Dominant language
- Python
- Stars
- 4
- Forks
- 0
- Avg merge
- 8h 24m
- Merged PRs (30d)
- 115
Description
## Problem
The retry callback runs only when the executor finishes an attempt with an
error. `threadmill/executor.py:285` gates it on exactly that:
```python
result = self.execute_task_result(task_result)
if (
result.status is TaskResultStatus.FAILED
and (delay := self.retry_delay(result)) is not None
):
self.backend.requeue(result, timezone.now() + delay)
else:
self.backend.acknowledge(result)
```
A worker that dies mid-scan never reaches this code. The reaper finds the
expired lease and finalises the task itself
(`threadmill/backends/lua/reaper.lua`): it sets `parsed.status = 'FAILED'`,
appends an `AcknowledgementTimeout` error, writes the result to the
failed-results ZSET for `result_ttl` (one day), and deletes the task key
(`redis.call('DEL', ...)`).
So an interrupted scan ends the message's retry chain permanently. The policy in
`services/email/spam/retry.py` is never consulted, and nothing else re-enqueues
the message: there is no reconciliation task and no rescan action in the admin.
## Impact
A rolling restart of the worker leaves messages unscanned forever, and relay
answers `250` for inbound mail it then never scans, holds, or webhooks. This is
a routine event rather than an exotic one, because deploys restart `worker`
(`rollout-services` in `.github/workflows/deploy.yml`) and `docker rollout`
stops containers while they may be holding a task. The same happens to
`deliver_message` and `deliver_webhook`, which have retry policies of their own.
## Suggested fix
Points to decide:
- Add an out-of-band sweep: a periodic task that finds messages which have been
`PENDING` past a threshold with no successful `SpamCheck`, and re-enqueues
them. That covers worker death, Redis eviction, and any future lost-task case
in one place, and it is the only option that does not depend on threadmill.
- Or extend threadmill to run the task's retry callback when the reaper fails a
lease-expired task, so the existing policies cover it.
- Cover both directions. Inbound has a terminal `failed` status to fall back on;
outbound needs the equivalent decision before the sweep can give up.
- Decide the threshold and the cadence, and whether the sweep should be visible
in the admin as a "recovered by reconciliation" marker.
## Related
- The Redis eviction issue filed alongside this one: a second way for a task to
vanish with no error and no retry.
- `docs/docs/reliability.md` documents the retry schedules and claims that a
failed scan is retried until it succeeds.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.