Skip to content

feat(iswf): Adds silenced_exceptions parameter to tasks, exposes this and report_timeout_errors in task registration#608

Open
GabeVillalobos wants to merge 7 commits intomainfrom
gv/retry-logic-parity
Open

feat(iswf): Adds silenced_exceptions parameter to tasks, exposes this and report_timeout_errors in task registration#608
GabeVillalobos wants to merge 7 commits intomainfrom
gv/retry-logic-parity

Conversation

@GabeVillalobos
Copy link
Copy Markdown
Member

@GabeVillalobos GabeVillalobos commented Apr 27, 2026

Exposes the previously added report_timeout_errors via task registration method.

Adds a new silenced_exceptions parameter, which should put us at feature parity with the remainder of fields in @retry:

| Parameter          | Retry | Report | Raise | Description |
|--------------------|-------|--------|-------|-------------|
| on                 | Yes   | Yes    | No    | Exceptions that will trigger a retry & report to Sentry. |
| on_silent          | Yes   | No     | No    | Exceptions that will trigger a retry but not be captured to Sentry. |
| exclude            | No    | No     | Yes   | Exceptions that will not trigger a retry and will be raised. |
| ignore             | No    | No     | No    | Exceptions that will be ignored and not trigger a retry & not report to Sentry. |
| ignore_and_capture | No    | Yes    | No    | Exceptions that will not trigger a retry and will be captured to Sentry. |

To emulate each of these previous parameters:
on

namespace.register(
    ...,
    retry=Retry(times=3, on=(MyCoolException,)),
)

on_silent

namespace.register(
    ...,
    retry=Retry(times=3, on=(MyCoolException,)),
    silenced_exceptions(MyCoolException,)
)

exclude

namespace.register(
    ...,
    retry=Retry(times=3, on=(...), ignore=(IgnoreMeException,)),
)

ignore

namespace.register(
    ...,
    retry=Retry(times=3, ignore=(IgnoreMeException,)),
    silenced_execptions(IgnoreMeException,)
)

ignore_and_capture
.... It's actually not entirely clear to me how this is supposed to behave compared to exclude

@GabeVillalobos GabeVillalobos marked this pull request as ready for review April 28, 2026 17:03
@GabeVillalobos GabeVillalobos requested a review from a team as a code owner April 28, 2026 17:03
Comment thread clients/python/src/taskbroker_client/registry.py Outdated
Copy link
Copy Markdown

@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.

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 7aa4820. Configure here.

Comment thread clients/python/src/taskbroker_client/worker/workerchild.py
@markstory
Copy link
Copy Markdown
Member

ignore_and_capture

Can this state just cease to exist? There is a single usage of it in sentry.

@GabeVillalobos GabeVillalobos force-pushed the gv/retry-logic-parity branch 2 times, most recently from abc6bda to cfe0c37 Compare April 29, 2026 20:03
Comment on lines 283 to 290
at_most_once=at_most_once,
wait_for_delivery=wait_for_delivery,
compression_type=compression_type,
report_timeout_errors=report_timeout_errors,
expected_exceptions=expected_exceptions,
)
self._registered_tasks[name] = task
return task
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The new expected_exceptions and report_timeout_errors parameters on ExternalNamespace.register() have no effect because ExternalTask instances are not executed locally.
Severity: MEDIUM

Suggested Fix

Raise a TypeError or ValueError if expected_exceptions or report_timeout_errors are passed to ExternalNamespace.register(), as these parameters are not supported for external tasks. Alternatively, document this limitation clearly in the method's docstring to prevent misuse.

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: clients/python/src/taskbroker_client/registry.py#L283-L290

Potential issue: The `ExternalNamespace.register()` method now accepts
`expected_exceptions` and `report_timeout_errors` parameters. However, these are
silently ignored for external tasks. These parameters control task execution behavior,
but `ExternalTask` instances are only dispatched, not executed, by the local worker; the
remote application's task registration governs error handling. A developer setting
`expected_exceptions` on an external task stub will incorrectly believe they have
suppressed certain error reports, when in fact the setting has no effect at runtime.

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

@GabeVillalobos
Copy link
Copy Markdown
Member Author

Can this state just cease to exist? There is a single usage of it in sentry.

@markstory Yeah we can. I think this change still make sense, excluding that use case, since we won't always want to be notified if we retry a retriable exception.

Copy link
Copy Markdown
Member

@evanh evanh left a comment

Choose a reason for hiding this comment

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

Could you add some tests for this behaviour?

@GabeVillalobos GabeVillalobos force-pushed the gv/retry-logic-parity branch from cfe0c37 to 24f66a6 Compare April 30, 2026 22:51
@GabeVillalobos
Copy link
Copy Markdown
Member Author

@evanh Added some testing. I think this should cover the major use cases we care about, but can add more if needed.

@fpacifici
Copy link
Copy Markdown

| ignore | No | No | No | Exceptions that will be ignored and not trigger a retry & not report to Sentry. |

What's a real world scenario for letting a task raise an exception, do nothing about that and consider it successful ?
Shouldn't the task not raise an exception in this case ?

wait_for_delivery: bool = False,
compression_type: CompressionType = CompressionType.PLAINTEXT,
report_timeout_errors: bool = True,
expected_exceptions: tuple[type[BaseException], ...] | None = None,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Isn't this naming a bit of a contradiction? "expected exception"
What about silenced_exceptions

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The original name for this was exceptions_to_silence 😅 I do think this is a bit more appropriate. expected is a bit vague in what it's implying. I'll update the name again.

@GabeVillalobos
Copy link
Copy Markdown
Member Author

What's a real world scenario for letting a task raise an exception, do nothing about that and consider it successful ?
Shouldn't the task not raise an exception in this case ?

@fpacifici I largely agree. This seems to be a task definition shorthand to avoiding writing a top-level try/catch, though this PR doesn't introduce this behavior. If we want to deprecate this in favor of slightly more verbose task handlers, I'm in favor of doing this in a separate PR.

@GabeVillalobos GabeVillalobos changed the title feat(iswf): Adds exceptions_to_silence parameter to tasks, exposes this and report_timeout_errors in task registration feat(iswf): Adds silenced_exceptions parameter to tasks, exposes this and report_timeout_errors in task registration May 1, 2026
Comment on lines 266 to 272
except Exception as err:
retry = task_func.retry
captured_error = False
should_capture_error = not isinstance(err, task_func.silenced_exceptions)
if retry:
if retry.should_retry(inflight.activation.retry_state, err):
logger.info(
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The silenced_exceptions parameter does not work for ProcessingDeadlineExceeded because it is caught by an earlier, more specific except block that ignores this parameter.
Severity: MEDIUM

Suggested Fix

Modify the except ProcessingDeadlineExceeded block to also check if the exception type is present in the silenced_exceptions tuple. If it is, the exception should be suppressed according to the intended logic. This will align the behavior with the API's documented purpose.

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: clients/python/src/taskbroker_client/worker/workerchild.py#L266-L272

Potential issue: The `silenced_exceptions` parameter has no effect when
`ProcessingDeadlineExceeded` is included in it. This is because
`ProcessingDeadlineExceeded` inherits from `BaseException` and is caught by a dedicated
`except ProcessingDeadlineExceeded` block. This earlier block does not check the
`silenced_exceptions` list. As a result, the general `except Exception` block, which
contains the logic for `silenced_exceptions`, is never reached for this specific
exception. This leads to a misleading API contract where the attempt to silence
`ProcessingDeadlineExceeded` fails without any indication.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is intentional. report_timeout_errors controls whether or not we report ProcessingDeadlineExceeded errors to Sentry.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants