Do we have an example of how the task will define this value ?

nothing changes about how this value is defined. it's already public API: https://github.com/getsentry/sentry/blob/9ee21b63ae7bcd3bf9a002e077e7fc73b860c656/src/sentry/deletions/tasks/scheduled.py#L102-L103

Raw retries lose delay

Medium Severity

On retry, the worker only sends max_attempts to the broker. For raw-mode activations, retry_state was absent in the stored blob; set_status then inserts a minimal state with only max_attempts. Upkeep republish uses delay_on_retry from that blob, so configured retry backoff is dropped and retries run immediately.

 

^{Reviewed by Cursor Bugbot for commit 0bf9105. Configure here.}

damn, i thought we had a nonzero default value for this. will probably add delay_on_retry to SetTaskStatusRequest

getsentry/sentry-protos#262

I already asked in a different comment, but I think that code is now outdated so I'll ask here again. How often will there be a max_attempts field on the message? One out of every... 10? 100? 1000? If it's going to be present often, we'll need to rethink how batching works. Because batching is necessary to reach high throughput.

This should be present always in tasks that run out of standard topics. I'd go further and say that maybe we should simplify the system by moving all tasks to specify the retries via the set_status method so we do not maintain more than one implementation.

Why does this affect the way batch is implemented ?

The field will be there unconditionally on all retry status updates. So retries are effectively not batched. I can implement batching for retries but this would increase complexity.

Note that not every retry status update results in an additional DB query. max_attempts is only set in the DB if it wasn't there before.

This should be present always in tasks that run out of standard topics.

It's optional here for the sake of rollout. We can gradually increase the amount of tasks that send max_attempts through the worker and observe its impact on the broker. (the rollout mechanism isn't implemented here)

Taking a step back on the retry topic.
If the retry topic contains activations rather than the original message, why do we even need a topic ? can't we just store the activation in the DB as pending and treat it like a task for the rest of its lifetime ?

I recognize this is a departure from the original intent of this PR, but it seems a lot simpler to me to manage it this way. The idea of the topic, to me, was meant to use it as a DLQ as well. Am I missing something ?

can't we just store the activation in the DB as pending and treat it like a task for the rest of its lifetime ?

I am missing rationale for why the retry system was set up this way to begin with. Your suggestion also applies to how regular tasks work. I would not want to special-case raw-mode to handle retries fundamentally differently than regular tasks.

My guess is that we wanted to keep the size of a database under control, therefore pruning queued retries out of the DB and putting them back into Kafka. If we say that this is not really a concern with AlloyDB then that's fine, but we'd have to validate that IMO

I can explore this option in another PR, but not sure we should roll it out without having more context from folks who originally worked on taskbroker.

The idea of the topic, to me, was meant to use it as a DLQ as well

That is yet another topic. It can stay or go away regardless of what we decide wrt retries.

@markstory @enochtangg do you remember why we didn't "just" stick retries into the DB and produce them back into kafka?

Your suggestion also applies to how regular tasks work.

I thought we picked up the task from the database to do the retry. @george-sentry did we change something in the push model ?

from slack:

@markstory: Keeping retries in sqlite/postgres could work but those retries will consume slots in sqlite/postgres. It will mean that a retry with no delay runs right away though instead of 'later' when it is found again in the topic.

@enochtangg: Another benefit I remember was because it resets the latency metric. Since latency is task dispatched - kafka receive latency, re-producing in kafka means we don't need to somehow fix that.

@fpacifici: the latency metric argument is important. I think we can keep it as it is and use the topic.  It is true that in AlloyDB there will be more room, but we will have the sqlite around for a while. No need to make the change

So I think I won't change anything here.

Bug: The Python worker unconditionally sends max_attempts for tasks with retry policies. This forces the gRPC server to bypass its batching optimization, causing an N+1 database query issue.
_{Severity: HIGH}

Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/grpc/server.rs#L106-L119

Potential issue: The Python worker unconditionally sends a `max_attempts` value for any
task with a retry decorator, regardless of its final status (complete, failure, or
retry). In the gRPC server, the batching channel (`update_tx`) is only used if
`max_attempts.is_none()`. Because the Python client always sends `max_attempts`, this
condition is never met for tasks with retry policies. This forces a fallback to an
individual `self.store.set_status()` call for each task, creating a significant
performance regression by introducing an N+1 database query problem instead of using a
single batched update.

What does this comment mean? Why is the raw mode distinction important? I thought raw mode was the only mode in which we used the retry topic?

I think I understand now. You mean that every message in the retry topic is guaranteed to be an activation even in raw mode, whereas messages in the "normal" topic in raw mode may not be?

I think I understand now. You mean that every message in the retry topic is guaranteed to be an activation even in raw mode, whereas messages in the "normal" topic in raw mode may not be?

Every message in the retry topic is a TaskActivation protobuf regardless of whether the consumer is in raw mode or normal mode. This is because we need to store the retry count in the topic, somehow.