Organize kick off automation #103345
Organize kick off automation #103345
Conversation
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
src/sentry/tasks/post_process.py
Outdated
| # cache.add uses Redis SETNX which atomically sets the key only if it doesn't exist | ||
| # Returns False if another process already set the key, ensuring only one process proceeds | ||
| if not cache.add(cache_key, True, timeout=600): # 10 minute | ||
| if seer_automation_rate_limit_check(group) is False: |
There was a problem hiding this comment.
Bug: Concurrency Race: Duplicate Tasks From Missing Atomicity
Removing the atomic cache.add() deduplication check introduces a race condition where multiple concurrent processes can all pass the lock.locked() check and trigger duplicate start_seer_automation.delay() tasks for the same group. The lock check only reads the lock state without acquiring it, so multiple processes can simultaneously see the lock as unlocked and proceed. The previous cache.add() with Redis SETNX provided atomic protection against this.
There was a problem hiding this comment.
The fixability check guards against this.
There was a problem hiding this comment.
does it? fixability takes some time to generate so there is a window in which multiple tasks could start
There was a problem hiding this comment.
- True but in that time the issue summary lock is in effect preventing multiple runs. Issue summary lock + fixability if statement are enough.
- Also by removing this lock we are going to the behaviour from 3 days ago.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
Is this PR just re-organizing code without touching functionality? I would also move all helpers out of the main post process file and into a seer utils file, e.g. seer/autofix/utils.py |
Yes, just re-organizing apart from the cache check. Cache check is being removed, though that was added by me 3 days ago. It's not essential. |
|
why remove the cache check @Mihir-Mavalankar ? see #103345 (comment) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #103345 +/- ##
===========================================
+ Coverage 71.28% 80.67% +9.38%
===========================================
Files 9231 9235 +4
Lines 394276 394495 +219
Branches 25149 25149
===========================================
+ Hits 281051 318248 +37197
+ Misses 112777 75799 -36978
Partials 448 448 |
src/sentry/seer/autofix/utils.py
Outdated
| return True | ||
|
|
||
|
|
||
| def seer_automation_rate_limit_check(group) -> bool: |
There was a problem hiding this comment.
can't we just use is_seer_scanner_rate_limited directly? what's the point of this wrapper?
There was a problem hiding this comment.
Removed it.
src/sentry/seer/autofix/utils.py
Outdated
| return is_rate_limited | ||
|
|
||
|
|
||
| def seer_automation_permission_and_type_check(group) -> bool: |
There was a problem hiding this comment.
can we add a type annotation to the param?
can we also rename the function something like "is_issue_eligible_for_seer_automation" or something else more readable than the current name
PR Details