codingjoe / codingjoe/relay

A handing-over spam scan is recorded as a failed run while the scan is still scheduled

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

Description

## Problem

When a scan run runs out of ramp, the retry callback does not fail the message
and does not reschedule the same run. It enqueues a fresh run an hour later and
returns `None`:

```python
Task.using(task_result.task, run_after=timezone.now() + SPAM_SCAN_RETRY_RENEWAL_DELAY).enqueue(
*task_result.args, **{**task_result.kwargs, "is_renewal": True}
)
```

The executor then finalises the current run with its last error
(`threadmill/executor.py:326-333`: status `FAILED`, error appended,
`finished_at` set) and, because the callback returned `None`, acknowledges it
instead of requeueing. The scan is still scheduled and will very likely succeed,
but the task store records a run that failed and finished.

`TaskResult.errors` also grows by one traceback per failed attempt. A real
`httpx.ConnectError` traceback measures around 6.3 KB, so the ramp accumulates
roughly 57 KB before a single renewal, and each renewal run starts that
accumulation again over the following attempts.

## Impact

Any dashboard or alert on failed task results reports permanent failures for
messages that are retried by design, which is how the retry policy stops being
trustworthy: operators cannot tell "gave up because the message is unscannable"
apart from "waiting for rspamd to come back", and the noise scales with the
number of messages during an outage, which is exactly when the signal matters
most. The error payloads also sit in Redis for `result_ttl` and are the bulk of
what grows under memory pressure.

## Suggested fix

Points to decide:

- Make the handover visible: record the run with a marker that says
"rescheduled", or reduce the recorded error to the last one so a renewal run
does not carry the whole ramp. Upstream, a retry callback could be allowed to
return a state instead of failing the run.
- Decide the alerting rule: which task results should page, and whether a
message that is mid-renewal should be visible anywhere at all.
- Confirm whether unbounded traceback growth is acceptable for tasks that retry
for days, or whether `TaskError` should be trimmed to the exception class and
the last frame.

## Related

- `docs/docs/reliability.md` describes the schedule as "retried until it
succeeds", which is true for the message and false for the run record.
- The reaper issue filed alongside this one: both are about the task store
disagreeing with the retry policy.

Contributor guide

No contributing guide indexed for this repository

Research direction

Read `threadmill/executor.py:326-333` and `docs/docs/reliability.md` to compare how the executor records a run with how the retry policy describes the scan. The issue leaves the handover state, alerting rule, and traceback retention open for decision, so first clarify the desired behavior with maintainers. Done should make run records and error storage accurately reflect retries without obscuring permanent failures.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.