fix(fetch): Undo the claim when submitting to the push pool fails - #794
enochtangg wants to merge 3 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4d91332. Configure here.
| /// but could not be handed to a worker. | ||
| async fn undo_claim(&self, id: &str, metric: &'static str) { | ||
| if let Err(e) = self | ||
| .set_status(id, ActivationStatus::Pending, None, None) |
There was a problem hiding this comment.
Maybe we need to be more defensive here, and only update the status if it is in the Claimed state (`UPDATE ... SET status = 'Pending' WHERE id = X AND status = 'Claimed'). That way we avoid any race conditions (e.g. https://github.com/getsentry/taskbroker/pull/794/changes#r4041591106).
There was a problem hiding this comment.
Good idea, updated such that release_claim only updates the state if the task was in claimed state
`send_async` moves the activation onto a flume waiter that a push thread can take at any moment. Dropping that future on timeout neither returns the activation nor reports whether it was delivered, so a submit timeout could not tell a full queue from a late delivery. The fetch thread then undid a claim for an activation a worker had already started, and the row became claimable a second time. `try_send` keeps the activation on this thread and hands it back in `TrySendError::Full`, so every non-Ok exit proves the push pool never saw it. The cost is polling every millisecond while the queue is full instead of parking on the channel. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2ed9f71 to
012ad05
Compare
2 participants
Description
When the fetch thread claims an activation but cannot hand it to the push pool, the row is left in the
Claimedstate with nothing holding it. The claim query only selectsPendingrows, so nothing re-picks it and it sits untilhandle_claim_expirationreverts it.This inflates the SLO metric.
pending_activation.max_lag.seccountsClaimedrows, so one stranded activation drives it up until the lease expires. This symptom can be observed consistently inprocess-segments-pushin s4s2: DD link. During this time, throughput, occupancy and AlloyDB latency were all normal, so the metric was tracking a single stuck row rather than real pipeline latency.When workers briefly go unavailable (due to deployment), pushes fail each activation revert its own claim which is correct, but that work filled the push queue to its cap. Activations that hit the push queue and receive a submit timeout don't get reverted and only log
Fix
Undo the claim on both
QueueErrorarms. This should be safe because the activation never left the broker, so no worker can be running it and reverting cannot cause a double execution. A submit failure now costs roughly the fetch backoff instead of the full claim lease.The push thread already did this inline after a failed gRPC push, so the shared logic moved to
ActivationStore::undo_claim.