[Actions Core] [LinkedIn Audiences] [Linkedin Conversions] Fix LinkedIn token propagation 401s with refresh and retry#3708
Open
harsh-joshi99 wants to merge 13 commits intomainfrom
Open
Conversation
…In token propagation Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a framework-level mechanism to treat specific LinkedIn 401s as token propagation delays (eventual consistency) rather than permanent auth failures, triggering an OAuth refresh and an infrastructure retry instead of dropping events.
Changes:
- Adds
RefreshTokenAndRetryErrorto@segment/actions-coreand exports it publicly. - Updates
destination-kitto catchRefreshTokenAndRetryError, refresh OAuth tokens, and rethrow asRetryableError(503)to force a later retry. - Adds LinkedIn-specific
afterResponsedetection for 401 +serviceErrorCode65601/65602 in both conversions and audiences, with unit tests covering the flow.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/destination-actions/src/destinations/linkedin-conversions/streamConversion/tests/index.test.ts | Adds unit/integration tests asserting propagation-delay detection and refresh+retry behavior. |
| packages/destination-actions/src/destinations/linkedin-conversions/index.ts | Adds an afterResponse hook to convert specific 401s into RefreshTokenAndRetryError. |
| packages/destination-actions/src/destinations/linkedin-conversions/constants.ts | Introduces the LinkedIn token propagation error code allowlist. |
| packages/destination-actions/src/destinations/linkedin-audiences/updateAudience/tests/index.test.ts | Adds a unit test asserting RefreshTokenAndRetryError is thrown for 401+65601. |
| packages/destination-actions/src/destinations/linkedin-audiences/index.ts | Adds an afterResponse hook to convert specific 401s into RefreshTokenAndRetryError. |
| packages/destination-actions/src/destinations/linkedin-audiences/constants.ts | Introduces the LinkedIn token propagation error code allowlist. |
| packages/core/src/index.ts | Exports RefreshTokenAndRetryError from the public package surface. |
| packages/core/src/errors.ts | Defines RefreshTokenAndRetryError and adds REFRESH_AND_RETRY error code. |
| packages/core/src/destination-kit/index.ts | Handles RefreshTokenAndRetryError by refreshing tokens and throwing RetryableError(503). |
| packages/core/src/tests/destination-kit.test.ts | Adds a unit test validating refresh+retry behavior when RefreshTokenAndRetryError is thrown. |
When we get a 65601/65602, the token was already just refreshed — there's no point refreshing again. Just throw RetryableError(503) and let Segment retry after the propagation window has passed. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3708 +/- ##
==========================================
+ Coverage 80.91% 80.95% +0.04%
==========================================
Files 1386 1386
Lines 27903 27928 +25
Branches 6026 6013 -13
==========================================
+ Hits 22577 22609 +32
- Misses 4350 4359 +9
+ Partials 976 960 -16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The RefreshTokenAndRetryError docstring and test name both said "refresh token" but we intentionally removed the token refresh — the token is already fresh, just not yet propagated. Align docs and test name with the actual behavior. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
The old name implied a token refresh happens, but the actual behaviour is just retry later — the token is already fresh, only propagation is delayed. Rename class, error code, and default message to reflect the true semantics. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Assert that refreshAccessToken is not called and RetryableError status is 503 when TokenPropagationRetryError is thrown. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Comment on lines
+108
to
+122
| agent, | ||
| afterResponse: [ | ||
| (_request: unknown, _options: unknown, response: { status: number; data: unknown }) => { | ||
| if (response.status === 401) { | ||
| const body = response.data as Record<string, unknown> | undefined | ||
| const serviceErrorCode = body?.serviceErrorCode as number | undefined | ||
| if (serviceErrorCode && LINKEDIN_TOKEN_PROPAGATION_ERROR_CODES.includes(serviceErrorCode)) { | ||
| throw new TokenPropagationRetryError( | ||
| `LinkedIn eventual consistency: token not yet propagated (serviceErrorCode ${serviceErrorCode})` | ||
| ) | ||
| } | ||
| } | ||
| return response | ||
| } | ||
| ] |
There was a problem hiding this comment.
extendRequest is typed to return RequestOptions (see packages/core/src/destination-kit/types.ts:348), but RequestOptions does not include afterResponse. Adding afterResponse here will fail TypeScript compilation (and is currently contrary to create-request-client.ts’s note about not exposing hooks). Consider either (a) updating the core types to allow afterResponse in RequestExtension (e.g., return AllRequestOptions), or (b) moving this logic into the action/API layer by disabling throwHttpErrors and throwing TokenPropagationRetryError after inspecting the response in the calling code.
Comment on lines
151
to
171
| return { | ||
| headers: { | ||
| authorization: `Bearer ${auth?.accessToken}`, | ||
| 'LinkedIn-Version': LINKEDIN_API_VERSION | ||
| }, | ||
| agent | ||
| agent, | ||
| afterResponse: [ | ||
| (_request: unknown, _options: unknown, response: { status: number; data: unknown }) => { | ||
| if (response.status === 401) { | ||
| const body = response.data as Record<string, unknown> | undefined | ||
| const serviceErrorCode = body?.serviceErrorCode as number | undefined | ||
| if (serviceErrorCode && LINKEDIN_TOKEN_PROPAGATION_ERROR_CODES.includes(serviceErrorCode)) { | ||
| throw new TokenPropagationRetryError( | ||
| `LinkedIn eventual consistency: token not yet propagated (serviceErrorCode ${serviceErrorCode})` | ||
| ) | ||
| } | ||
| } | ||
| return response | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
extendRequest currently returns RequestOptions, which does not support an afterResponse hook. This will not type-check and likely won’t build. Either adjust the core typing/behavior to accept afterResponse (return AllRequestOptions) or implement the 401+serviceErrorCode detection where you already have a ModifiedResponse (e.g., in the API wrapper methods) without relying on request-client hooks.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
harsh-joshi99
changed the title
…t coverage - Change TokenPropagationRetryError.status from 401 to 503 to prevent generic status-based 401 handlers from triggering a spurious token refresh - Expand JSDoc to qualify the no-refresh guarantee to the exception path and anchor the LinkedIn-specific origin (serviceErrorCode 65601/65602) - Add comments in handleError and retry.ts documenting the implicit contract that throwing from onFailedAttempt terminates the retry loop - Add 65602 variant tests in both LinkedIn destinations - Add negative test in streamConversion: 401 with unrecognized error code must not throw TokenPropagationRetryError - Add onEvent integration test to updateAudience mirroring streamConversion Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Comment on lines
96
to
123
| extendRequest({ auth }) { | ||
| // Repeat calls to the same LinkedIn API endpoint were failing due to a `socket hang up`. | ||
| // This seems to fix it: https://stackoverflow.com/questions/62500011/reuse-tcp-connection-with-node-fetch-in-node-js | ||
| // Copied from LinkedIn Audiences extendRequest, which also ran into this issue. | ||
| const agent = new https.Agent({ keepAlive: true }) | ||
|
|
||
| return { | ||
| headers: { | ||
| authorization: `Bearer ${auth?.accessToken}`, | ||
| 'LinkedIn-Version': LINKEDIN_API_VERSION, | ||
| 'X-Restli-Protocol-Version': `2.0.0` | ||
| }, | ||
| agent | ||
| agent, | ||
| afterResponse: [ | ||
| (_request: unknown, _options: unknown, response: { status: number; data: unknown }) => { | ||
| if (response.status === 401) { | ||
| const body = response.data as Record<string, unknown> | undefined | ||
| const serviceErrorCode = body?.serviceErrorCode as number | undefined | ||
| if (serviceErrorCode && LINKEDIN_TOKEN_PROPAGATION_ERROR_CODES.includes(serviceErrorCode)) { | ||
| throw new TokenPropagationRetryError( | ||
| `LinkedIn eventual consistency: token not yet propagated (serviceErrorCode ${serviceErrorCode})` | ||
| ) | ||
| } | ||
| } | ||
| return response | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
extendRequest is contextually typed to return RequestOptions (see packages/core/src/destination-kit/types.ts), which does not include an afterResponse field. Returning an object literal with afterResponse here will fail TypeScript excess-property checking. To fix, update the core RequestExtension/extendRequest return type to allow AllRequestOptions (or a variant that includes hook arrays), or move this logic into a supported hook point (e.g., action-level request handling).
Comment on lines
146
to
172
| extendRequest({ auth }) { | ||
| // Repeat calls to the same LinkedIn API endpoint were failing due to a `socket hang up`. | ||
| // This seems to fix it: https://stackoverflow.com/questions/62500011/reuse-tcp-connection-with-node-fetch-in-node-js | ||
| const agent = new https.Agent({ keepAlive: true }) | ||
|
|
||
| return { | ||
| headers: { | ||
| authorization: `Bearer ${auth?.accessToken}`, | ||
| 'LinkedIn-Version': LINKEDIN_API_VERSION | ||
| }, | ||
| agent | ||
| agent, | ||
| afterResponse: [ | ||
| (_request: unknown, _options: unknown, response: { status: number; data: unknown }) => { | ||
| if (response.status === 401) { | ||
| const body = response.data as Record<string, unknown> | undefined | ||
| const serviceErrorCode = body?.serviceErrorCode as number | undefined | ||
| if (serviceErrorCode && LINKEDIN_TOKEN_PROPAGATION_ERROR_CODES.includes(serviceErrorCode)) { | ||
| throw new TokenPropagationRetryError( | ||
| `LinkedIn eventual consistency: token not yet propagated (serviceErrorCode ${serviceErrorCode})` | ||
| ) | ||
| } | ||
| } | ||
| return response | ||
| } | ||
| ] | ||
| } | ||
| }, |
There was a problem hiding this comment.
extendRequest is typed to return RequestOptions (via RequestExtension in core), which does not include afterResponse. Adding afterResponse here will fail TypeScript excess-property checks. Either update the core type to allow AllRequestOptions/hooks for extendRequest, or relocate this response-inspection logic to a supported layer.
LinkedIn returns serviceErrorCode 65601 for both token propagation delays and revoked/expired tokens. Previously, 65601 always skipped the token refresh and just retried — causing infinite retries with a dead token when it had actually expired. Now the framework refreshes the token before throwing RetryableError, so the retry always uses a fresh token. If the refresh itself fails (e.g. refresh token is also expired), that error propagates instead of retrying forever. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Comment on lines
+108
to
+119
| * the token will have propagated. No additional token refresh is performed | ||
| * on the exception-throwing path — handleError intercepts this error before | ||
| * the OAuth re-authentication logic runs. | ||
| * | ||
| * This error is currently thrown by LinkedIn destination hooks when | ||
| * serviceErrorCode 65601 or 65602 appears in a 401 response body. Reuse for | ||
| * other providers should only occur after confirming the same | ||
| * retry-without-refresh semantic applies. | ||
| */ | ||
| export class TokenPropagationRetryError extends CustomError { | ||
| // Use 503 to match the RetryableError this converts into — do NOT use 401, | ||
| // which would cause generic status-based 401 handlers to trigger a token refresh. |
There was a problem hiding this comment.
The docstring says no additional token refresh is performed and that handleError intercepts this error before OAuth refresh logic runs, but Destination.handleError now explicitly calls handleAuthError (refresh) when it sees TokenPropagationRetryError. Please update this documentation (or adjust the implementation) so the behavior matches what the comment describes.
Suggested change
| * the token will have propagated. No additional token refresh is performed | |
| * on the exception-throwing path — handleError intercepts this error before | |
| * the OAuth re-authentication logic runs. | |
| * | |
| * This error is currently thrown by LinkedIn destination hooks when | |
| * serviceErrorCode 65601 or 65602 appears in a 401 response body. Reuse for | |
| * other providers should only occur after confirming the same | |
| * retry-without-refresh semantic applies. | |
| */ | |
| export class TokenPropagationRetryError extends CustomError { | |
| // Use 503 to match the RetryableError this converts into — do NOT use 401, | |
| // which would cause generic status-based 401 handlers to trigger a token refresh. | |
| * the token will typically have propagated. This error is intended to model | |
| * a transient, retryable propagation delay rather than a permanent invalid- | |
| * credentials condition. Framework auth-error handling may still run as part | |
| * of the normal exception path, so callers should not rely on this error type | |
| * to suppress OAuth refresh logic. | |
| * | |
| * This error is currently thrown by LinkedIn destination hooks when | |
| * serviceErrorCode 65601 or 65602 appears in a 401 response body. Reuse for | |
| * other providers should only occur after confirming the same | |
| * transient retry semantic applies. | |
| */ | |
| export class TokenPropagationRetryError extends CustomError { | |
| // Use 503 to match the RetryableError this converts into and to represent | |
| // a transient retryable condition instead of a standard invalid-auth 401. |
Comment on lines
+1032
to
+1043
| // Handle TokenPropagationRetryError: LinkedIn returns serviceErrorCode 65601 for both | ||
| // token propagation delays and revoked/expired tokens. Refresh the token first so the | ||
| // retry always uses a fresh token. If the refresh itself fails (e.g. the refresh token | ||
| // is also expired), that error propagates instead of retrying forever with a dead token. | ||
| // RetryableError(503) signals Segment infrastructure to retry after backoff. | ||
| if (error instanceof TokenPropagationRetryError) { | ||
| const isOAuth = this.authentication?.scheme === 'oauth2' || this.authentication?.scheme === 'oauth-managed' | ||
| if (isOAuth) { | ||
| await this.handleAuthError(settings, options) | ||
| } | ||
| throw new RetryableError(error.message, 503) | ||
| } |
There was a problem hiding this comment.
PR description says TokenPropagationRetryError is converted into RetryableError(503) without refreshing tokens, but handleError currently calls handleAuthError (refresh) before throwing RetryableError. Please align the implementation + tests with the intended behavior (either remove the refresh here, or update the PR description and the TokenPropagationRetryError docs to reflect the refresh-and-retry semantics).
Comment on lines
+1456
to
+1515
| test('should refresh token and throw RetryableError(503) when TokenPropagationRetryError is thrown', async () => { | ||
| const refreshAccessToken = jest.fn().mockResolvedValue({ accessToken: 'fresh-token' }) | ||
| const onTokenRefresh = jest.fn() | ||
|
|
||
| const destinationWithPropagationError: DestinationDefinition<JSONObject> = { | ||
| name: 'Test Propagation Error Destination', | ||
| mode: 'cloud', | ||
| authentication: { | ||
| ...authentication, | ||
| refreshAccessToken | ||
| }, | ||
| actions: { | ||
| customEvent: { | ||
| title: 'Send a Custom Event', | ||
| description: 'Send events to a custom event in API', | ||
| defaultSubscription: 'type = "track"', | ||
| fields: { | ||
| advertiserId: { | ||
| label: 'Advertiser ID', | ||
| description: 'Advertiser Id', | ||
| type: 'string', | ||
| required: true | ||
| } | ||
| }, | ||
| perform: () => { | ||
| throw new TokenPropagationRetryError('Token not yet propagated (serviceErrorCode 65601)') | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const destinationTest = new Destination(destinationWithPropagationError) | ||
| const testEvent: SegmentEvent = { | ||
| properties: { a: 'foo' }, | ||
| userId: '3456fff', | ||
| type: 'track' | ||
| } | ||
| const testSettings = { | ||
| apiSecret: 'test_key', | ||
| subscription: { | ||
| subscribe: 'type = "track"', | ||
| partnerAction: 'customEvent', | ||
| mapping: { | ||
| advertiserId: '1231241241' | ||
| } | ||
| }, | ||
| oauth: { | ||
| access_token: 'some-access-token', | ||
| refresh_token: 'refresh-token' | ||
| } | ||
| } | ||
|
|
||
| // handleError should refresh the token (expired tokens also return 65601) then | ||
| // throw RetryableError(503) so the retry uses a fresh token | ||
| const error = await destinationTest.onEvent(testEvent, testSettings, { onTokenRefresh }).catch((e) => e) | ||
| expect(error).toBeInstanceOf(RetryableError) | ||
| expect(error.status).toBe(503) | ||
| expect(refreshAccessToken).toHaveBeenCalledTimes(1) | ||
| expect(onTokenRefresh).toHaveBeenCalledWith({ accessToken: 'fresh-token' }) | ||
| }) |
There was a problem hiding this comment.
This new test asserts refreshAccessToken is called when TokenPropagationRetryError is thrown, which conflicts with the PR description's test plan claim that refreshAccessToken is NOT called. Please reconcile the expected behavior: either adjust the test and implementation to avoid refresh on token-propagation retries, or update the PR description/test plan to reflect that a refresh is performed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TokenPropagationRetryErrorerror class toactions-corefor signaling that a 401 is caused by OAuth token propagation delay (eventual consistency), not a true auth failuredestination-kitcatches this error and re-throws asRetryableError(503)— no token refresh is performed since the token is already fresh; Segment infrastructure retries the event after a backoff delay by which time the token will have propagatedlinkedin-conversionsandlinkedin-audiencesdetect LinkedIn 401s withserviceErrorCode65601/65602 in anafterResponsehook and throwTokenPropagationRetryErrorWhy
LinkedIn's OAuth tokens have an eventual consistency delay — after a token refresh, the new token isn't immediately valid across all LinkedIn API nodes. This caused events to be permanently dropped instead of retried.
Test plan
destination-kit.test.ts— fullonEventflow assertsRetryableError(503)is thrown andrefreshAccessTokenis NOT calledstreamConversion/__tests__/index.test.ts— assertsTokenPropagationRetryErrorthrown on 65601, plus fullonEventintegration test assertingRetryableErrorupdateAudience/__tests__/index.test.ts— assertsTokenPropagationRetryErrorthrown on 65601Test Results
Local Testing
Local testing.
Security Review
Please ensure sensitive data is properly protected in your integration.
type: 'password'🤖 Generated with Claude Code