Skip to content

Map AggegateError and Error.cause to ApplicationFailureInfo #1734

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mdouglass
Copy link

What was changed

Change how ensureApplicationFailure and the DefaultFailureConverter handle Error.cause and AggregateError.errors.

Why?

  • ensureApplicationFailure was not providing Error.cause to ApplicationFailure.create which meant that cause information was not being built

  • AggegateError.errors wasn't being handled in either ensureApplicationFailure or the DefaultFailureConverter. It is not possible to map that directly to an IFailure, so that information is added to then ApplicationFailureInfo.details field.

Checklist

  1. Closes [Feature Request] support AggregateError #1675

  2. How was this tested:
    I was using an activity that looks like this:

export async function failHard( ): Promise<void> {
  throw new AggregateError(
    [
      new Error('Analyze workflow gets all the love.', {
        cause: new Error('Analyze workflow is the money machine.'),
      }),
      new AggregateError(
        [
          new Error('Everyone knows acquire does the hard work'),
          new Error('Everyone can see what acquire does'),
        ],
        'Acquire workflow gets all the praise.',
      ),
    ],
    'Scrape workflow quits!',
    {
      cause: new AggregateError(
        [
          new Error('Scrape workflow is a drama queen'),
          new Error('Did I mention, scrape workflow is a drama queen?'),
        ],
        'Scrape workflow is a drama queen x2.',
      ),
    },
  )
}

Previously, this was all the information available in the Temporal web site for the error:

{
  "type": "workflowExecutionFailedEventAttributes",
  "failure":
    {
      "message": "Activity task failed",
      "cause":
        {
          "message": "Scrape workflow quits!",
          "source": "TypeScriptSDK",
          "stackTrace": "AggregateError: Scrape workflow quits!\n    at getLastScrape (file:///Users/matthew/adology/adology-backend/packages/ad-scraper/src/activities/last-scrape.ts:16:9)",
          "applicationFailureInfo": { "type": "AggregateError" },
        },
      "activityFailureInfo":
        {
          "scheduledEventId": "5",
          "startedEventId": "6",
          "identity": "714@camilla.local",
          "activityType": { "name": "getLastScrape" },
          "activityId": "1",
          "retryState": "RETRY_STATE_MAXIMUM_ATTEMPTS_REACHED",
        },
    },
  "retryState": "RETRY_STATE_RETRY_POLICY_NOT_SET",
  "workflowTaskCompletedEventId": "10",
}

After this PR, this is how the failed activity is reported:

{
  "type": "workflowExecutionFailedEventAttributes",
  "failure": {
    "message": "Activity task failed",
    "cause": {
      "message": "Scrape workflow quits!",
      "source": "TypeScriptSDK",
      "stackTrace": "AggregateError: Scrape workflow quits!\n    at getLastScrape (file:///Users/matthew/adology/adology-backend/packages/ad-scraper/src/activities/last-scrape.ts:16:9)",
      "cause": {
        "message": "Scrape workflow is a drama queen x2.",
        "source": "TypeScriptSDK",
        "stackTrace": "AggregateError: Scrape workflow is a drama queen x2.\n    at getLastScrape (file:///Users/matthew/adology/adology-backend/packages/ad-scraper/src/activities/last-scrape.ts:31:14)",
        "applicationFailureInfo": {
          "details": {
            "payloads": [
              [
                {
                  "source": "TypeScriptSDK",
                  "message": "Scrape workflow is a drama queen",
                  "stackTrace": "Error: Scrape workflow is a drama queen\n    at getLastScrape (file:///Users/matthew/adology/adology-backend/packages/ad-scraper/src/activities/last-scrape.ts:33:11)"
                },
                {
                  "source": "TypeScriptSDK",
                  "message": "Did I mention, scrape workflow is a drama queen?",
                  "stackTrace": "Error: Did I mention, scrape workflow is a drama queen?\n    at getLastScrape (file:///Users/matthew/adology/adology-backend/packages/ad-scraper/src/activities/last-scrape.ts:34:11)"
                }
              ]
            ]
          }
        }
      },
      "applicationFailureInfo": {
        "type": "AggregateError",
        "details": {
          "payloads": [
            {
              "message": "Analyze workflow gets all the love.",
              "type": "Error",
              "stack": "Error: Analyze workflow gets all the love.\n    at getLastScrape (file:///Users/matthew/adology/adology-backend/packages/ad-scraper/src/activities/last-scrape.ts:18:7)",
              "cause": {
                "message": "Analyze workflow is the money machine.",
                "type": "Error",
                "stack": "Error: Analyze workflow is the money machine.\n    at getLastScrape (file:///Users/matthew/adology/adology-backend/packages/ad-scraper/src/activities/last-scrape.ts:19:16)"
              }
            },
            {
              "message": "Acquire workflow gets all the praise.",
              "type": "AggregateError",
              "stack": "AggregateError: Acquire workflow gets all the praise.\n    at getLastScrape (file:///Users/matthew/adology/adology-backend/packages/ad-scraper/src/activities/last-scrape.ts:21:7)",
              "details": [
                {
                  "message": "Everyone knows acquire does the hard work",
                  "type": "Error",
                  "stack": "Error: Everyone knows acquire does the hard work\n    at getLastScrape (file:///Users/matthew/adology/adology-backend/packages/ad-scraper/src/activities/last-scrape.ts:23:11)"
                },
                {
                  "message": "Everyone can see what acquire does",
                  "type": "Error",
                  "stack": "Error: Everyone can see what acquire does\n    at getLastScrape (file:///Users/matthew/adology/adology-backend/packages/ad-scraper/src/activities/last-scrape.ts:24:11)"
                }
              ]
            }
          ]
        }
      }
    },
    "activityFailureInfo": {
      "scheduledEventId": "5",
      "startedEventId": "6",
      "identity": "98241@camilla.local",
      "activityType": { "name": "getLastScrape" },
      "activityId": "1",
      "retryState": "RETRY_STATE_MAXIMUM_ATTEMPTS_REACHED"
    }
  },
  "retryState": "RETRY_STATE_RETRY_POLICY_NOT_SET",
  "workflowTaskCompletedEventId": "10"
}
  1. Any docs updates needed?

- ensureApplicationFailure was not providing Error.cause to
ApplicationFailure.create which meant that cause information was not
being built

- AggegateError.errors wasn't being handled in either
ensureApplicationFailure or the DefaultFailureConverter. It is not
possible to map that directly to an IFailure, so that information is
added to then ApplicationFailureInfo.details field
@mdouglass mdouglass requested a review from a team as a code owner June 27, 2025 18:20
@CLAassistant
Copy link

CLAassistant commented Jun 27, 2025

CLA assistant check
All committers have signed the CLA.

@mdouglass
Copy link
Author

This does what I'm looking for, in that it captures the Error.cause and AggregateError.errors fields, but I don't know that it's perfect.

  1. In an ideal world, the underlying IFailure's cause field would be an array so we could use that instead of dropped the aggregates into details.
  2. It feels a bit weird that I have to duplicate the same logic in both ensureApplicationFailure and the DefaultFailureConverter. You probably don't want to change ensureApplicationFailure to call ApplicationFailure.create({ cause: error }) since it would add an extra layer of nesting in the error, but maybe there's a way to clean this up I'm not seeing.

@mjameswh
Copy link
Contributor

Hey @mdouglass! This is a very interesting contribution. Thanks for your work.

I will have to think this through and discuss with my colleagues before I take a decision on this PR. Failures may propagate across SDKs, and be printed out by other tools (e.g. the web UI), so repurposing the details field for this might not be harmless.

@mdouglass
Copy link
Author

Yeah, I figured this would have some ramifications beyond what I knew. I was hopeful that since end-users were encouraged to use details for their own metadata that it would be relatively safe to (ab)use this way.
Let me know :)

@mdouglass
Copy link
Author

The alternative here would be to change the failure type at the protobuf level so that it could accommodate the structure of AggregateError - lots of languages have an error type like this so it’s not unreasonable to do so.

That would obviously have even bigger cross-project ramifications so I didn’t want to try it without talking about it first.

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.

[Feature Request] support AggregateError
3 participants