fix(reprocessing): Cull parallel reprocessing tasks - #123693
fix(reprocessing): Cull parallel reprocessing tasks#123693tobias-wilfert wants to merge 3 commits into
Conversation
github-actions
Bot
added
the
Scope: Backend
|
The goal of this PR is to mainly cut down on the tasks running and as such on the errors and logs that we are seeing. |
![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 696e866. Configure here.
| key = _get_page_claim_key(project_id, group_id, new_group_id, timestamp, event_id) | ||
| if self.redis.set(key, claimant, nx=True, ex=settings.SENTRY_REPROCESSING_PAGE_CLAIM_TTL): | ||
| return True | ||
| return self.redis.get(key) == claimant |
There was a problem hiding this comment.
Page claim TTL prevents eventual culling
Medium Severity
try_claim_page only records the page currently being processed and expires that key after SENTRY_REPROCESSING_PAGE_CLAIM_TTL (1 day). A branch more than a day behind the leader never observes a live claim, so it is not culled and reprocesses already-completed pages. That is likely for this incident: reprocessing is slow and many branches have already diverged.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 696e866. Configure here.
| def _get_page_claim_key( | ||
| project_id: int, group_id: int, new_group_id: int, timestamp: str, event_id: str | ||
| ) -> str: | ||
| return f"re2:pageclaim:{{{project_id}:{group_id}}}:{new_group_id}:{timestamp}:{event_id}" |
There was a problem hiding this comment.
What are the nested braces around project_id/group_id for?
| if self.redis.set(key, claimant, nx=True, ex=settings.SENTRY_REPROCESSING_PAGE_CLAIM_TTL): | ||
| return True | ||
| return self.redis.get(key) == claimant |
There was a problem hiding this comment.
set has a get flag that makes it return the old value, if any. Using that you could save yourself the get call.
| if self.redis.set(key, claimant, nx=True, ex=settings.SENTRY_REPROCESSING_PAGE_CLAIM_TTL): | |
| return True | |
| return self.redis.get(key) == claimant | |
| prev_claimant = self.redis.set(key, claimant, get=True, nx=True, ex=settings.SENTRY_REPROCESSING_PAGE_CLAIM_TTL) | |
| return prev_claimant == claimant or prev_claimant is None |
(Assuming our Redis version is current enough (>6.2) to support this.)
| # To the best of our knowledge we still have quite some `reprocess_group` tasks running in parallel, this logic | ||
| # is intended to cull all but one. This is temporary to recover from a bad state and should be dead code after that. |
There was a problem hiding this comment.
This comment should probably refer to the INC in question by name, otherwise this is going to be confusing in the future.
2 participants
I looked further into the reprocessing (INC-2491) and to the best of my knowledge what is happening is that we still have quite some reprocessing tasks running in parallel and that while the fix that landed during the incident stopped the fanning out it never culled the already fanned out tasks (notion docs with some findings).
Because reprocessing is very slow, alot of the reprocessing branches have not yet finished (as seen by the query state that get captured in the sentry errors caused by the parallel runs). In theory we could wait till the reprocessing fizzles out but it is hard to tell when exactly that will be.
As such, this PR tries to implement the logic that kills all but the leading reprocessing task (branch). The idea being that the first task lays some claim on the page that it is working on. This should work since the stepping is always 10 issues, so all the parallel runs (branches) should eventually hit the same page. A later parallel run will see that the reprocessing for this page already is happening return and thus end its branch.
Fair warning, this is my first time touching most of this code and redis so if I do something silly please let me know.
Follow up to #123317