Skip to content

feat(iswf): Removes retry decorator from workflow_engine tasks#114937

Open
GabeVillalobos wants to merge 2 commits intogv/add-new-retry-decorator-optionsfrom
gv/removes-retry-decorators-from-wfe-tasks
Open

feat(iswf): Removes retry decorator from workflow_engine tasks#114937
GabeVillalobos wants to merge 2 commits intogv/add-new-retry-decorator-optionsfrom
gv/removes-retry-decorators-from-wfe-tasks

Conversation

@GabeVillalobos
Copy link
Copy Markdown
Member

Removes usages of the @retry decorator from workflow engine tasks, translating them into their taskbroker equivalent.

@GabeVillalobos GabeVillalobos requested review from a team as code owners May 5, 2026 23:52
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 5, 2026
Comment thread src/sentry/workflow_engine/tasks/delayed_workflows.py Outdated
Comment on lines +79 to +89
times=3,
delay=5,
on=(Exception, ProcessingDeadlineExceeded),
ignore=(
Action.DoesNotExist,
Group.DoesNotExist,
Project.DoesNotExist,
ProjectNotActiveError,
Workflow.DoesNotExist,
),
),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The trigger_action task will now fail loudly after exhausting retries instead of silently completing, due to a missing times_exceeded parameter.
Severity: MEDIUM

Suggested Fix

To restore the previous behavior of silently discarding the task after all retries are exhausted, add times_exceeded=LastAction.Discard to the Retry object in the trigger_action task.

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/sentry/workflow_engine/tasks/actions.py#L78-L89

Potential issue: The `trigger_action` task's behavior upon exhausting its retries has
changed. Previously, it used `raise_on_no_retries=False` to silently complete without
error if all retries failed. The new implementation using the `Retry` object lacks an
equivalent configuration, such as `times_exceeded=LastAction.Discard`. As a result,
after exhausting its retries, the task will now raise an exception and be marked as a
failure, which is a change from its previous behavior.

Did we get this right? 👍 / 👎 to inform future reviews.

Comment on lines +82 to +88
ignore=(
Action.DoesNotExist,
Group.DoesNotExist,
Project.DoesNotExist,
ProjectNotActiveError,
Workflow.DoesNotExist,
),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The trigger_action task no longer captures Action.DoesNotExist and Group.DoesNotExist exceptions to Sentry, resulting in lost observability.
Severity: LOW

Suggested Fix

To restore observability, re-implement the logic to capture Action.DoesNotExist and Group.DoesNotExist exceptions to Sentry with level="info" before they are ignored by the retry framework.

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/sentry/workflow_engine/tasks/actions.py#L82-L88

Potential issue: In the `trigger_action` task, specific exceptions like
`Action.DoesNotExist` and `Group.DoesNotExist` are no longer captured as 'info'-level
events in Sentry. The previous implementation used `ignore_and_capture` to log these
expected exceptions for observability before ignoring them. The new code moves them to
`Retry.ignore`, which prevents retries but does not include a mechanism to log them.
This results in a loss of observability for these specific non-critical failure cases.

Did we get this right? 👍 / 👎 to inform future reviews.

@GabeVillalobos GabeVillalobos requested a review from markstory May 6, 2026 00:00
Comment thread src/sentry/workflow_engine/tasks/delayed_workflows.py Outdated
Project.DoesNotExist,
ProjectNotActiveError,
Workflow.DoesNotExist,
),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lost Sentry capture for ignore_and_capture exceptions

Medium Severity

The old @retry decorator distinguished between ignore_and_capture=(Action.DoesNotExist, Group.DoesNotExist) (no retry, task succeeds, but explicitly captured to Sentry at "info" level) and ignore=(Project.DoesNotExist, ...) (no retry, task succeeds, no Sentry report). The new code places all five exceptions into Retry.ignore, losing the intentional sentry_sdk.capture_exception(level="info") call for Action.DoesNotExist and Group.DoesNotExist. The deliberate exclusion of these two from silenced_exceptions suggests the author wanted them reported, but if Retry.ignore silently consumes the exception, it never reaches the reporting layer.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f649020. Configure here.

Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 33269fc. Configure here.

ProjectNotActiveError,
Workflow.DoesNotExist,
),
),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing times_exceeded=LastAction.Discard for exhausted retries

Medium Severity

The old @retry decorator for trigger_action had raise_on_no_retries=False, which meant that when all retries were exhausted, the task would complete successfully instead of failing. The new Retry configuration doesn't include the equivalent times_exceeded=LastAction.Discard parameter (which is used elsewhere in the codebase, e.g., scheduled.py). This means the task will now fail with an unhandled exception after retries are exhausted, instead of silently succeeding as before.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 33269fc. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant