ref: Remove string, msg and activation clones#674
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| /// Create a new activation that is a 'retry' of the passed activation | ||
| /// The retry_state.attempts is advanced as part of the retry state machine. | ||
| #[instrument(skip_all)] | ||
| fn create_retry_activation(activation: &TaskActivation) -> TaskActivation { | ||
| let mut new_activation = activation.clone(); | ||
|
|
||
| fn create_retry_activation(mut new_activation: TaskActivation) -> TaskActivation { | ||
| let now = Utc::now(); | ||
| new_activation.id = Uuid::new_v4().into(); | ||
| new_activation.received_at = Some(Timestamp { |
There was a problem hiding this comment.
Bug: Calling .and_then() on new_activation.retry_state consumes the Option, preventing the retry attempts counter from being incremented in the subsequent if let block.
Severity: CRITICAL
Suggested Fix
To avoid consuming the Option, use as_ref() before calling and_then. The line should be changed to new_activation.delay = new_activation.retry_state.as_ref().and_then(|retry_state| retry_state.delay_on_retry);. This borrows the retry_state instead of moving it, allowing the subsequent code to access and modify it.
Prompt for AI Agent
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/upkeep.rs#L541-L547
Potential issue: In the `create_retry_activation` function, the call to `.and_then()` on
`new_activation.retry_state` consumes the `Option`, moving the value out and leaving
`new_activation.retry_state` as `None`. As a result, the subsequent `if let` block that
is supposed to increment the `attempts` counter will never execute. This breaks the
retry state machine, causing tasks to be retried without their attempt counters being
incremented, which can lead to infinite retries.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
honestly this kind of comment is why i wish these bots would just wait for the thing to compile
|
Took some thinking but I think this all makes sense. |
2 participants
Arc<OwnedMessage>in favor of passing&OwnedMessage. This does not avoid string copies but avoids 1 refcount, and one awkwardtry_unwrap.TableRowto borrow from activation. Note we have to borrow activations now, so we can't convert by move. We definitely want to keep a separate struct because otherwise activation will have to derive sqlx traits, and that'll be messy as sqlx is an impl detail of each activation, and the schemas might want to diverge. But we can manually implFromRowforTableRowand therefore useCow<'a, str>. When reading activations from the DB, sqlx will allocate strings for us (Cow::Owned) and there's no way to avoid that.Most likely this will not accomplish any perf speedup, but it's not introducing too many painful lifetimes so still worth it imo.