remote capabilities errors are properly handled as caperrors#21746
remote capabilities errors are properly handled as caperrors#21746fernandezlautaro wants to merge 3 commits intodevelopfrom
Conversation
|
I see you updated files related to
|
|
✅ No conflicts with other open PRs targeting |
There was a problem hiding this comment.
Pull request overview
Risk Rating: MEDIUM
Fixes workflow-side handling of remote capability execution errors so that typed caperrors.Error (origin + code) survive the RPC boundary and can be classified/metric’d correctly.
Changes:
- Wrap remote execution error responses with an
Unwrap()chain containingcaperrors.DeserializeErrorFromString(ErrorMsg)while preserving the legacy display string. - Add unit tests asserting remote error responses can be
errors.As’d intocaperrors.Error(including fallback behavior for non-serialized error strings). - Adjust workflow v2 capability-executor failure metric labeling for the “non-caperrors” error path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/services/workflows/v2/capability_executor.go | Updates failure metric labeling for errors that don’t implement caperrors.Error. |
| core/capabilities/remote/executable/request/client_request.go | Wraps remote error responses to preserve legacy error strings while enabling errors.As(..., caperrors.Error) post-RPC. |
| core/capabilities/remote/executable/request/client_request_test.go | Adds tests validating correct caperrors unwrapping and fallback behavior. |
| _ = events.EmitCapabilityFinishedEvent(ctx, loggerLabels, c.WorkflowExecutionID, request.Id, meteringRef, store.StatusErrored, request.Method, err) | ||
| c.metrics.With(platform.KeyCapabilityID, request.Id, platform.KeyCapabilityErrorCode, caperrors.Unknown.String()).IncrementCapabilityFailureCounter(ctx) | ||
| // TODO shouldn't all capabilities *always* return a typed error, and if so shouldn't the following metric alert us there's a bug we need to fix? | ||
| c.metrics.With(platform.KeyCapabilityID, request.Id, platform.KeyCapabilityErrorCode, "BUG").IncrementCapabilityFailureCounter(ctx) |
There was a problem hiding this comment.
The metric label value "BUG" for capabilityErrorCode deviates from the established caperrors code set and will change metric series unexpectedly. Consider keeping the label within caperrors (e.g., Unknown) and separately logging/recording that the returned error did not implement caperrors.Error (or add a dedicated metric for untyped errors).
| c.metrics.With(platform.KeyCapabilityID, request.Id, platform.KeyCapabilityErrorCode, "BUG").IncrementCapabilityFailureCounter(ctx) | |
| c.metrics.With(platform.KeyCapabilityID, request.Id, platform.KeyCapabilityErrorCode, "Unknown").IncrementCapabilityFailureCounter(ctx) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Remote capabilities errors' are not being unmarshalled/handled properly in the WF side of it, loosing the correct type (user/system) and the error code (making all of them be of
Unknowntype)This PR fixes that.
More context: https://chainlink-core.slack.com/archives/C07GQNPVBB5/p1774552056515259
Requires
Supports