feat(iswf): Adds exceptions_to_silence parameter to tasks, exposes this and report_timeout_errors in task registration#608
Conversation
| wait_for_delivery: bool = False, | ||
| compression_type: CompressionType = CompressionType.PLAINTEXT, | ||
| report_timeout_errors: bool = True, | ||
| exceptions_to_silence: tuple[type[BaseException], ...] | None = None, |
There was a problem hiding this comment.
This name is a little verbose, but the intention here is just to silence sentry errors for some list of exceptions. It should give us the blanket coverage to phase out the remaining options in our retry decorators.
There was a problem hiding this comment.
What about expected_errors? These are errors we expect to happen and don't need observability into.
There was a problem hiding this comment.
Could these new parameters be added to the docstring as well?
There was a problem hiding this comment.
Good call, just renamed this and added docstring comments.
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.
Reviewed by Cursor Bugbot for commit 7aa4820. Configure here.
Can this state just cease to exist? There is a single usage of it in sentry. |
abc6bda to
cfe0c37
Compare
| 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 |
There was a problem hiding this comment.
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.
@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. |
evanh
left a comment
There was a problem hiding this comment.
Could you add some tests for this behaviour?
cfe0c37 to
24f66a6
Compare
|
@evanh Added some testing. I think this should cover the major use cases we care about, but can add more if needed. |

Exposes the previously added
report_timeout_errorsvia task registration method.Adds a new
exceptions_to_silenceparameter, which should put us at feature parity with the remainder of fields in@retry:To emulate each of these previous parameters:
on
on_silent
exclude
ignore
ignore_and_capture
.... It's actually not entirely clear to me how this is supposed to behave compared to
exclude