fix(aci): Retry process_delayed_workflows timeouts #95379
Conversation
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #95379 +/- ##
===========================================
+ Coverage 36.78% 87.77% +50.98%
===========================================
Files 9915 10509 +594
Lines 557022 606328 +49306
Branches 23720 23720
===========================================
+ Hits 204927 532193 +327266
+ Misses 351732 73772 -277960
Partials 363 363 |
| def retry_timeouts(func): | ||
| """ | ||
| Schedule a task retry if the function raises ProcessingDeadlineExceeded. | ||
| This exists because the standard retry decorator doesn't allow BaseExceptions. | ||
| """ |
There was a problem hiding this comment.
If you find yourself using this frequently, we could introduce deadline approaching exceptions that happen some time before the processing deadline is crossed.
There was a problem hiding this comment.
Bug: Timeout Handling Fails in Retry Decorator
The retry_timeouts decorator has two issues. First, an exception raised by sentry_sdk.capture_exception() will mask the original ProcessingDeadlineExceeded and prevent retry_task() from being called. Second, if retry_task() unexpectedly doesn't raise an exception (despite its NoReturn annotation), the wrapper implicitly returns None, potentially masking the timeout.
src/sentry/workflow_engine/tasks/utils.py#L97-L105
sentry/src/sentry/workflow_engine/tasks/utils.py
Lines 97 to 105 in ce8936f
Was this report helpful? Give feedback by reacting with 👍 or 👎
| @@ -330,7 +330,7 @@ def _process_resource_change( | |||
| except model.DoesNotExist as e: | |||
| # Explicitly requeue the task, so we don't report this to Sentry until | |||
| # we hit the max number of retries. | |||
| return retry_task(e) | |||
| retry_task(e) | |||
There was a problem hiding this comment.
qq: why did we have to remove return from here?
There was a problem hiding this comment.
Because I redefined retry_task as officially never returning as far as the type system is concerned.
So, this return was kinda incorrect, and due to my change mypy was able to flag it.
| @@ -760,6 +761,7 @@ def repr_keys[T, V](d: dict[T, V]) -> dict[str, V]: | |||
| ), | |||
| ) | |||
| @retry | |||
| @retry_timeouts | |||
There was a problem hiding this comment.
any thoughts on some convenience decorator to group the 2 retries together. something like @retry_including_timeouts
probably will increase adoption. there are a lot of ecosystem tasks i would use this for.
Timeouts are expected here and assumed transient, and the existing
retrydecorator and config don't support retryingProcessingDeadlineExceeded.So, we add a new task decorator to trigger retries on timeouts. This avoids opening the door to other BaseExceptions and keeps it opt-in as designed.