feat(server, orchestrator, jobs, runner): More descriptive action errors #3279

nalanj · 2025-01-09T16:32:34Z

This adds richer errors when an action is triggered and responds with a 424 when an upstream response is the cause of an error.

Before:

❯ curl --request POST --url http://localhost:3000/action/trigger --header "Authorization: Bearer fe212ac9-0832-4210-b868-400db33b3863" --header "Connection-Id: 612d3d37-0a6c-40a5-96f2-f65df6e8fd98" --header "Provider-Config-Key: unauthenticated" --header "Content-Type: application/json" --data '{"action_name": "rate-limited"}'
{"error":{"message":"An error occurred during an HTTP call","code":"script_http_error","payload":{"error":"Rate limit exceeded"}}}%

After:

❯ curl --request POST --url http://localhost:3000/action/trigger --header "Authorization: Bearer fe212ac9-0832-4210-b868-400db33b3863" --header "Connection-Id: 612d3d37-0a6c-40a5-96f2-f65df6e8fd98" --header "Provider-Config-Key: unauthenticated" --header "Content-Type: application/json" --data '{"action_name": "rate-limited"}'
{"error":{"message":"An error occurred during an HTTP call","code":"script_http_error","payload":{"error":"Rate limit exceeded"},"additional_properties":{"upstream_response":{"status":429,"headers":{"content-type":"application/json; charset=utf-8"},"body":{"error":"Rate limit exceeded"}}}}}%

https://linear.app/nango/issue/NAN-2478/action-error-refinement

How I tested it:

Call an action that makes a request that errors
The server will return the more detailed response

Deployment

Deployment here is going to be tricky - it's all the main back-end services!

gitguardian · 2025-01-10T18:14:49Z

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}}

nalanj · 2025-01-10T22:05:04Z

Does anyone have any recommendations on how to document this new status code and let customers know about it? Mintlify doesn't seem to render docs for status codes other than 200 for us.

packages/runner/lib/exec.ts

packages/runner/lib/exec.unit.test.ts

packages/shared/lib/clients/orchestrator.ts

…ile I was fixing types

TBonnin · 2025-01-13T13:58:28Z

docs-v2/spec.yaml

+                                                type: string
+                                            payload:
+                                                type: object
+                                            additional_properties:


why not the upstream attribute directly instead of the additional level of nesting:

{ error: message: code: payload: upstream: { status_code: headers: response: } }

I could see three directions:

What you mentioned, just add more attributes as needed. My worry there was that we could end up adding more down the road, and if we do we have to update the typing for the response that comes out of actions each time. Not a big deal but it was kind of tricky to get all the types straight through all the services.

We have a grab bag kind of property, additional_properties, like I built. In that case the error object stays consistent and we don't have to add all sorts of properties to it down the road. It's less pretty, but is also easier to deal with. And since it's a Record<string,unknown> it's easier to change going forward because it has to have nested properties.

We shove it into payload. This one mostly doesn't work because I found cases where error payload is an array and I think remember finding other cases where it's just a string.

Updated in 6b665e0, but also waiting for feedback from @bodinsamuel and @khaliqgant and it's an easy thing to revert if we decide to go a different direction.

packages/runner/lib/exec.ts

TBonnin · 2025-01-13T14:05:41Z

packages/shared/lib/clients/orchestrator.ts

@@ -117,13 +118,16 @@ export class Orchestrator {
            'connection.provider_config_key': connection.provider_config_key,
            'connection.environment_id': connection.environment_id
        };
+        if (!connection.id) {
+            throw new NangoError('invalid_input', { connection });


this is going to throw the error instead of returning an Err

Fixed in a39804e

TBonnin · 2025-01-13T14:07:30Z

packages/shared/lib/utils/error.manager.ts

            } else {
-                res.status(err.status || 500).send({ error: { message: err.message, code: err.type, payload: err.payload } });
+                res.status(err.status || 500).send({
+                    error: { message: err.message, code: err.type, payload: err.payload, additional_properties: err.additional_properties }


additional properties can be undefined. should we not return it at all in this case?

It just gets pruned when it's undefined, since it serializes:

❯ curl --request GET \ --url http://localhost:3003/v1/exception \ --header 'Authorization: Bearer fe212ac9-0832-4210-b868-400db33b3863' \ --header 'Connection-Id: 612d3d37-0a6c-40a5-96f2-f65df6e8fd98' \ --header 'Content-Type: application/json' \ --header 'Provider-Config-Key: unauthenticated' {"error":{"message":"The action script failed with a runtime error","code":"action_script_runtime_error","payload":{"error":"Something broke"}}}%

TBonnin · 2025-01-13T14:08:24Z

packages/types/lib/runner/index.ts

@@ -1,12 +1,14 @@
 export interface RunnerOutputError {
    type: string;
-    payload: Record<string, unknown>;
+    payload: Record<string, unknown> | unknown[];


where can payload be an array?

This was a super weird one once I started cleaning up types in exec.ts.

See

nango/packages/runner/lib/exec.ts

Line 117 in b61728b

return { success: false, response: null, error: { type: 'invalid_action_input', status: 400, payload: valInput } };

. valInput is a ValidateDataError[]. I'm pretty stumped as to why typescript wasn't picking that up as an error before these changes, though.

TBonnin

works for me with the upstream top level attributes in the json api response.
you didn't answer this comment (it is in a resolved thread): https://github.com/NangoHQ/nango/pull/3279/files#r1913242024

nalanj · 2025-01-14T12:35:36Z

Wanted to note this is going to sit here a bit while we notify customers that it's coming.

nalanj added 9 commits January 8, 2025 13:58

Tweak runner output error type

e14fb4a

Add specialized handling for http errors in exec

979ac05

Fix ratelimit header filter

e9933d3

Cleanup

8045a2d

Go back to instanceof check

5ec18b9

Fix some type errors

a23ff56

Merge branch 'master' into alan/action-errors

a5be5ce

Add comment about why I'm using isAxiosError

48ae65e

Fix all eslint warnings in exec.ts

7a397e7

nalanj self-assigned this Jan 9, 2025

nalanj added 3 commits January 9, 2025 14:35

Update jobs putTask to handle new error format in validation

c43fe83

WIP on figuring out how to get error to flow through orchestrator

4cf4721

Merge branch 'master' into alan/action-errors

67974ad

nalanj added 2 commits January 10, 2025 15:07

Rework error to have additional_properties

2ab655a

Got upstream errors flowing out of action errors

d06af85

nalanj changed the title ~~WIP: More descriptive action errors~~ feat(server, orchestrator, jobs, runner): More descriptive action errors Jan 10, 2025

nalanj added 3 commits January 10, 2025 16:12

Have additional_properties set to undefined by default

d995508

Merge branch 'master' into alan/action-errors

d2c1a53

Add 424 to openapi spec

4db0b09

nalanj marked this pull request as ready for review January 10, 2025 22:09

nalanj requested a review from a team January 10, 2025 22:11

bodinsamuel reviewed Jan 13, 2025

View reviewed changes

packages/runner/lib/exec.ts Show resolved Hide resolved

packages/runner/lib/exec.unit.test.ts Show resolved Hide resolved

packages/runner/lib/exec.unit.test.ts Show resolved Hide resolved

packages/shared/lib/clients/orchestrator.ts Outdated Show resolved Hide resolved

nalanj added 2 commits January 13, 2025 07:46

Re-add mystery code removal. I'm assuming it was eslint formatting wh…

5ecaa06

…ile I was fixing types

Remove note about not reaching instance

b61728b

nalanj requested review from bodinsamuel and a team January 13, 2025 12:52

bodinsamuel approved these changes Jan 13, 2025

View reviewed changes

nalanj requested a review from TBonnin January 13, 2025 13:42

TBonnin reviewed Jan 13, 2025

View reviewed changes

nalanj added 2 commits January 13, 2025 10:02

Move connectionid assertion

a39804e

Update error response to just have an upstream type

6b665e0

TBonnin approved these changes Jan 13, 2025

View reviewed changes

Clean up response handling

3e2cb1b

nalanj requested a review from TBonnin January 13, 2025 16:14

bodinsamuel approved these changes Jan 14, 2025

View reviewed changes

TBonnin approved these changes Jan 14, 2025

View reviewed changes

Merge branch 'master' into alan/action-errors

869c651

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server, orchestrator, jobs, runner): More descriptive action errors #3279

feat(server, orchestrator, jobs, runner): More descriptive action errors #3279

nalanj commented Jan 9, 2025 •

edited

Loading

gitguardian bot commented Jan 10, 2025 •

edited

Loading

nalanj commented Jan 10, 2025

TBonnin Jan 13, 2025

nalanj Jan 13, 2025

nalanj Jan 13, 2025

TBonnin Jan 13, 2025

nalanj Jan 13, 2025

TBonnin Jan 13, 2025

nalanj Jan 13, 2025

TBonnin Jan 13, 2025

nalanj Jan 13, 2025

TBonnin left a comment

nalanj commented Jan 14, 2025

feat(server, orchestrator, jobs, runner): More descriptive action errors #3279

Are you sure you want to change the base?

feat(server, orchestrator, jobs, runner): More descriptive action errors #3279

Conversation

nalanj commented Jan 9, 2025 • edited Loading

How I tested it:

Deployment

gitguardian bot commented Jan 10, 2025 • edited Loading

️✅ There are no secrets present in this pull request anymore.

nalanj commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TBonnin left a comment

Choose a reason for hiding this comment

nalanj commented Jan 14, 2025

nalanj commented Jan 9, 2025 •

edited

Loading

gitguardian bot commented Jan 10, 2025 •

edited

Loading