fix(linear-retries): [nan-2309] handle linear case #3108

khaliqgant · 2024-12-04T09:53:18Z

Linear sends back a 4xx status code with a RATELIMITED string in the body. Currently this doesn't get picked up by our retry logic. Searching using the headers will retry even if it isn't a strict 429.

NAN-2309

Test by mimicing a response that is like the linear response

{
  "request": {
    "headers": {
      "authorization": "REDACTED",
      "user-agent": "nango-node-client/0.45.1 (linux/6.8.0-1018-aws; node.js/20.12.2); sdk"
    },
    "method": "POST",
    "url": "https://api.linear.app/graphql"
  },
  "response": {
    "headers": {
      "x-ratelimit-complexity-remaining": "971913",
      "x-request-id": "8eca3a6e066a3076-SEA",
      "content-length": "535",
      "vary": "Accept-Encoding",
      "x-complexity": "520",
      "x-ratelimit-complexity-limit": "1000000",
      "x-ratelimit-requests-remaining": "0",
      "x-ratelimit-requests-reset": "1733302464386",
      "x-ratelimit-complexity-reset": "1733302464386",
      "content-type": "application/json; charset=utf-8",
      "connection": "close",
      "x-ratelimit-requests-limit": "1200"
    },
    "code": 400
  },
  "error": {
    "message": "Request failed with status code 400",
    "payload": {
      "safeHeaders": {
        "authorization": "REDACTED",
        "user-agent": "nango-node-client/0.45.1 (linux/6.8.0-1018-aws; node.js/20.12.2); sdk"
      },
      "code": "ERR_BAD_REQUEST",
      "data": {
        "errors": [
          {
            "extensions": {
              "userError": true,
              "code": "RATELIMITED",
              "meta": {},
              "type": "ratelimited",
              "userPresentableMessage": "Rate limit exceeded. Only 1200 requests are allowed per 1 hour. For more information see our developer docs at: https://developers.linear.app/docs/graphql/working-with-the-graphql-api/rate-limiting."
            },
            "message": "Rate limit exceeded. Only 1200 requests are allowed per 1 hour. For more information see our developer docs at: https://developers.linear.app/docs/graphql/working-with-the-graphql-api/rate-limiting"
          }
        ]
      },
      "method": "POST",
      "url": "https://api.linear.app/graphql"
    }
  }
}

linear · 2024-12-04T09:53:21Z

NAN-2309 Handle non standard linear rate limit

…-rate-limit

bodinsamuel

If we indeed have headers I would suggest something else

"x-complexity": "520",
      "x-ratelimit-complexity-limit": "1000000",
      "x-ratelimit-requests-remaining": "0",
      "x-ratelimit-requests-reset": "1733302464386",
      "x-ratelimit-complexity-reset": "1733302464386",

We should change the if in retry to search for headers whatever the status code

packages/shared/lib/services/proxy.service.ts

scripts/validation/providers/schema.json

…of github.com:NangoHQ/nango into khaliq/nan-2309-handle-non-standard-linear-rate-limit

khaliqgant · 2024-12-04T11:27:27Z

If we indeed have headers I would suggest something else
"x-complexity": "520",
      "x-ratelimit-complexity-limit": "1000000",
      "x-ratelimit-requests-remaining": "0",
      "x-ratelimit-requests-reset": "1733302464386",
      "x-ratelimit-complexity-reset": "1733302464386",
We should change the if in retry to search for headers whatever the status code

Thanks, agreed + less code. Updated with 47becfa

packages/shared/lib/services/proxy.service.ts

bodinsamuel · 2024-12-04T13:13:20Z

packages/shared/lib/services/proxy.service.ts

@@ -259,7 +259,8 @@ class ProxyService {
            (error.response?.status === 403 && error.response.headers['x-ratelimit-remaining'] && error.response.headers['x-ratelimit-remaining'] === '0') ||
            error.response?.status === 429 ||
            ['ECONNRESET', 'ETIMEDOUT', 'ECONNABORTED'].includes(error.code as string) ||
-            config.retryOn?.includes(Number(error.response?.status))
+            config.retryOn?.includes(Number(error.response?.status)) ||
+            this.isRetryInHeader(config.provider.proxy?.retry, error)


Just one last question (sorry my suggestion also had some shortcomings):
Are the headers always set or just when we reach rate limit? Here if its always set then all requests will be retried even on regular 400

I guess one missing check that does not exist yet in checking a "remaining headers"

is isRetryInHeader supporting the special case for github 3 lines above?

Are the headers always set or just when we reach rate limit? Here if its always set then all requests will be retried even on regular 400

Unsure tbh which is why in my initial implementation kept this isolated to Linear to keep the impact low since I don't know how most APIs behave.

I guess one missing check that does not exist yet in checking a "remaining headers"

I don't know what you mean here? Can you clarify?

is isRetryInHeader supporting the special case for github 3 lines above?

No, but I guess we could encapsulate both into updated logic. Few proposals:

just keep them both hard coded

# github (error.response?.status === 403 && error.response.headers['x-ratelimit-remaining'] && error.response.headers['x-ratelimit-remaining'] === '0') || # linear (error.response?.status === 400 && error.response.headers['x-ratelimit-requests-remaining'] && error.response.headers['x-ratelimit-requests-remaining'] === '0') ||

Codify it in the yaml

# github proxy: retry: at: 'x-ratelimit-reset' remaining: x-ratelimit-remaining error_code: 403 # linear proxy: retry: at: 'X-RateLimit-Requests-Reset' remaining: x-ratelimit-requests-remaining error_code: 400

any thoughts @NangoHQ/engineers?

the second one looks cleaner but it make providers.yaml more complex. I don't feel strongly either way

Option 2 seems correct and not too hard to implement, I would love it 👍🏻

Implemented option 2 with b94e844

TBonnin · 2024-12-04T14:12:44Z

packages/shared/lib/services/proxy.service.unit.test.ts

+                auth_mode: 'OAUTH2',
+                proxy: {
+                    retry: {
+                        at: 'x-ratelimit-requests-resets'


I would make it very different from the real header name. It took me a few seconds to spot the extra s

TBonnin · 2024-12-04T14:13:37Z

packages/shared/lib/services/proxy.service.unit.test.ts

@@ -505,6 +505,97 @@ describe('Proxy service Construct URL Tests', () => {
        expect(diff).toBeGreaterThan(1000);
        expect(diff).toBeLessThan(2000);
    });
+
+    it('Should retry based on the header even if the error code is not a 429', async () => {


can we wrap those new tests in their own describe so you can instanciate the mockAxiosError only once?

…9-handle-non-standard-linear-rate-limit

…-rate-limit

bodinsamuel

Super clean 👌🏻

[nan-2309] handle linear case

2524b04

Merge branch 'master' into khaliq/nan-2309-handle-non-standard-linear…

76aeaa2

…-rate-limit

khaliqgant requested a review from a team December 4, 2024 09:55

bodinsamuel reviewed Dec 4, 2024

View reviewed changes

packages/shared/lib/services/proxy.service.ts Outdated Show resolved Hide resolved

scripts/validation/providers/schema.json Show resolved Hide resolved

khaliqgant added 2 commits December 4, 2024 13:26

[nan-2309] use header to retry

47becfa

Merge branch 'khaliq/nan-2309-handle-non-standard-linear-rate-limit' …

d208f25

…of github.com:NangoHQ/nango into khaliq/nan-2309-handle-non-standard-linear-rate-limit

[nan-2309] remove schema changes

5c0d331

khaliqgant requested a review from bodinsamuel December 4, 2024 11:29

bodinsamuel reviewed Dec 4, 2024

View reviewed changes

TBonnin reviewed Dec 4, 2024

View reviewed changes

bodinsamuel mentioned this pull request Dec 4, 2024

fix(providers): enforce lowercase headers, enforce categories #3110

Merged

khaliqgant and others added 3 commits December 5, 2024 10:11

Merge branch 'master' of github.com:NangoHQ/nango into khaliq/nan-230…

f4a74a8

…9-handle-non-standard-linear-rate-limit

[nan-2309] add remaining and error_code

b94e844

Merge branch 'master' into khaliq/nan-2309-handle-non-standard-linear…

e81e248

…-rate-limit

khaliqgant requested review from bodinsamuel and TBonnin December 5, 2024 09:52

bodinsamuel approved these changes Dec 5, 2024

View reviewed changes

khaliqgant merged commit 54526cb into master Dec 5, 2024
21 checks passed

khaliqgant deleted the khaliq/nan-2309-handle-non-standard-linear-rate-limit branch December 5, 2024 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(linear-retries): [nan-2309] handle linear case #3108

fix(linear-retries): [nan-2309] handle linear case #3108

khaliqgant commented Dec 4, 2024 •

edited

Loading

linear bot commented Dec 4, 2024

bodinsamuel left a comment

khaliqgant commented Dec 4, 2024

bodinsamuel Dec 4, 2024

TBonnin Dec 4, 2024

khaliqgant Dec 4, 2024

khaliqgant Dec 4, 2024

TBonnin Dec 5, 2024

bodinsamuel Dec 5, 2024

khaliqgant Dec 5, 2024

TBonnin Dec 4, 2024

khaliqgant Dec 5, 2024

TBonnin Dec 4, 2024

khaliqgant Dec 5, 2024

bodinsamuel left a comment

fix(linear-retries): [nan-2309] handle linear case #3108

fix(linear-retries): [nan-2309] handle linear case #3108

Conversation

khaliqgant commented Dec 4, 2024 • edited Loading

linear bot commented Dec 4, 2024

bodinsamuel left a comment

Choose a reason for hiding this comment

khaliqgant commented Dec 4, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bodinsamuel left a comment

Choose a reason for hiding this comment

khaliqgant commented Dec 4, 2024 •

edited

Loading