feat(server): introspect salesforce tokens on error - #7030
Conversation
Confidence Score: 4/5The concurrent Salesforce refresh path should be fixed before merging because one request can return an authorization error even after another worker has refreshed its credentials. The callback suppresses the proxy retry whenever its own invocation did not perform the refresh, but the refresh service can return valid newly refreshed credentials with that state after lock contention. Files Needing Attention: packages/server/lib/controllers/proxy/allProxy.ts
|
| Filename | Overview |
|---|---|
| packages/server/lib/controllers/proxy/allProxy.ts | Adds deferred Salesforce introspection and refresh-on-error wiring, but conflates “this invocation refreshed” with “retrying using returned credentials is worthwhile.” |
| packages/shared/lib/services/connections/credentials/refresh.ts | Adds skipIntrospection propagation and cache-key isolation while retaining refreshed=false when another caller has already produced current credentials. |
| packages/shared/lib/services/proxy/request.ts | Adds a bounded refresh-token retry path and suppresses retry when its callback returns false. |
| packages/shared/lib/services/proxy/retry.ts | Classifies configured Salesforce authorization statuses as refresh-token retry reasons. |
| packages/shared/lib/services/proxy/request.unit.test.ts | Covers basic refresh success, cap, and active-token behavior but not a concurrent refresh returning current credentials without performing the write. |
| packages/types/lib/proxy/api.ts | Adds the constructed proxy configuration field used to identify token-refresh statuses. |
Sequence Diagram
sequenceDiagram
participant A as Proxy request A
participant B as Proxy request B
participant R as Credential refresh
participant P as Provider API
A->>P: Request with expired token
B->>P: Request with expired token
P-->>A: 401/403
P-->>B: 401/403
A->>R: Refresh credentials
R-->>A: "refreshed=true, current connection"
B->>R: Refresh credentials
R-->>B: "refreshed=false, current connection"
A->>P: Retry with current credentials
B-->>B: Suppress retry as token_still_active
Note over B: Returns original auth error despite current credentials
Prompt To Fix All With AI
### Issue 1
packages/server/lib/controllers/proxy/allProxy.ts:377-379
**Concurrent refresh suppresses retry**
If concurrent Salesforce proxy requests receive 401 or 403 for the same expired token, one request can receive the current credentials after another worker refreshes them while its local `refreshed` flag remains false. Returning that flag causes `ProxyRequest` to classify the token as still active and return the original authorization error instead of retrying with the valid `freshConnection`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(server): introspect salesforce toke..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| freshConnection = credentialResponse.value; | ||
| lastConnectionRefresh = Date.now(); | ||
| return refreshed; |
There was a problem hiding this comment.
If concurrent Salesforce proxy requests receive 401 or 403 for the same expired token, one request can receive the current credentials after another worker refreshes them while its local refreshed flag remains false. Returning that flag causes ProxyRequest to classify the token as still active and return the original authorization error instead of retrying with the valid freshConnection.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/server/lib/controllers/proxy/allProxy.ts
Line: 377-379
Comment:
**Concurrent refresh suppresses retry**
If concurrent Salesforce proxy requests receive 401 or 403 for the same expired token, one request can receive the current credentials after another worker refreshes them while its local `refreshed` flag remains false. Returning that flag causes `ProxyRequest` to classify the token as still active and return the original authorization error instead of retrying with the valid `freshConnection`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 10 files
Confidence score: 3/5
- In
packages/shared/lib/services/proxy/retry.ts, 401/403 responses that also match retry-header/body logic can keep consuming retry budget without ever running Salesforce token introspection, which risks repeated auth failures until retries are exhausted — ensure a refresh/introspection path still executes before (or at least by) final retry exhaustion. - In
packages/server/lib/controllers/proxy/allProxy.ts, therefreshedflag only flips inside this request’s own success callback, so concurrent refreshes for the same connection may be missed and treated as not refreshed — set/deriverefreshedfrom the shared refresh outcome (or re-read credential state) to avoid false negatives under concurrency. - In
packages/shared/lib/services/proxy/request.ts, flows withoutonRefreshTokenstill setrefreshTokenOn, causing 401/403s to be labeled as refresh-expired in SDK/Slack/hooks/AWS/internal paths and potentially triggering wrong error handling — gaterefreshTokenOnon actual refresh capability and add coverage for no-refresh call paths. packages/shared/lib/services/proxy/retry.tsandpackages/server/lib/hooks/hooks.tsshow config/behavior drift (Retry-Afterpriority comment vs new branch behavior, andrefreshTokenOn: nullbeing ignored), which makes retry/refresh behavior hard to reason about and can hide regressions — align implementation with intended precedence and wireexternalConfig.refreshTokenOnthroughgetProxyConfigurationwith targeted tests.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/shared/lib/services/proxy/request.unit.test.ts">
<violation number="1" location="packages/shared/lib/services/proxy/request.unit.test.ts:22">
P3: `createAxiosError` duplicates the existing `makeAxiosError` helper in the same file — same `err.response` shape, differing only in the message/statusText strings — leaving two near-identical factory helpers. Consider reusing a single helper (e.g., parameterize or just call `makeAxiosError`) to avoid parallel code that can drift.</violation>
</file>
<file name="packages/server/lib/hooks/hooks.ts">
<violation number="1" location="packages/server/lib/hooks/hooks.ts:425">
P3: This added `refreshTokenOn: null` has no effect: `getProxyConfiguration` ignores `externalConfig.refreshTokenOn` (it isn't destructured) and recomputes it from `providerClient.shouldIntrospectToken(providerName)`. The PR aims to introspect Salesforce tokens on error, so either wire the value through `getProxyConfiguration` so this config actually influences the request, or drop the misleading line.</violation>
</file>
<file name="packages/shared/lib/services/proxy/retry.ts">
<violation number="1" location="packages/shared/lib/services/proxy/retry.ts:60">
P2: Salesforce token introspection is skipped whenever a matching 401/403 also produces a configured retry-header/body result; if that delay is present on each response, the request exhausts its retry budget without calling `onRefreshToken`. Preserve the refresh intent separately from the delay reason, or add a post-delay refresh path, so honoring the backoff does not permanently bypass token refresh.</violation>
<violation number="2" location="packages/shared/lib/services/proxy/retry.ts:61">
P2: The comment in retry.ts states that Retry-After headers should take priority over a token refresh so a rate-limited response gets its backoff respected. But the new else-if in request.ts discards any non-refresh retry once `attempt > retries`. With the default Salesforce retries=0, a 401/403 carrying a Retry-After header yields reason `custom_after` (not 'refresh_token'), so it hits the else-if at attempt=1 and is aborted (1 > 0) instead of being backed off before a refresh. This makes the documented Retry-After-priority behavior unreachable for the default refresh-token case. Worth reconciling the two files so a header-driven backoff on a refreshable status is actually retried.</violation>
</file>
<file name="packages/shared/lib/services/proxy/request.ts">
<violation number="1" location="packages/shared/lib/services/proxy/request.ts:189">
P2: For Salesforce calls going through any ProxyRequest path that does not supply `onRefreshToken` (SDK, Slack notifications, hooks, AWS SigV4, internal-nango), `refreshTokenOn` is still set, so a 401/403 is labelled 'refresh_token' and `refreshTokenAttempts` caps the retry at one regardless of the configured `retries`. No refresh is performed in these paths, so the request just re-fetches the same stale credentials once and then stops with a 'refresh_token_max_attempts' reason. This silently changes retry behavior for these callers (previously a 401 was retried up to `retries`) and logs a refresh reason that never reflects a real refresh. Consider only entering the refresh branch when `onRefreshToken` is actually present (and otherwise falling through to normal `retries` handling), and resetting `refreshTokenAttempts` at the start of `request()` so state never leaks if the instance is reused.</violation>
</file>
<file name="packages/server/lib/controllers/proxy/allProxy.ts">
<violation number="1" location="packages/server/lib/controllers/proxy/allProxy.ts:379">
P2: The `refreshed` flag returned from `onRefreshToken` is only set true inside this call's own `onRefreshSuccess` callback. If a concurrent request for the same connection already triggered the refresh (and refreshCredentialsIfNeeded dedupes via its in-flight cache), this caller's `onRefreshSuccess` may never fire even though the token was refreshed, leaving `refreshed` false. ProxyRequest then treats the token as still active and returns the original auth error instead of retrying with the now-valid `freshConnection`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| // Mark for token refresh but don't return yet — Retry-After headers take priority | ||
| // so a rate-limited response with a backoff header is respected before refreshing | ||
| const isRefreshToken = proxyConfig.refreshTokenOn?.some((code) => matchesStatusCode(status, String(code))) ?? false; |
There was a problem hiding this comment.
P2: Salesforce token introspection is skipped whenever a matching 401/403 also produces a configured retry-header/body result; if that delay is present on each response, the request exhausts its retry budget without calling onRefreshToken. Preserve the refresh intent separately from the delay reason, or add a post-delay refresh path, so honoring the backoff does not permanently bypass token refresh.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/shared/lib/services/proxy/retry.ts, line 60:
<comment>Salesforce token introspection is skipped whenever a matching 401/403 also produces a configured retry-header/body result; if that delay is present on each response, the request exhausts its retry budget without calling `onRefreshToken`. Preserve the refresh intent separately from the delay reason, or add a post-delay refresh path, so honoring the backoff does not permanently bypass token refresh.</comment>
<file context>
@@ -55,6 +55,13 @@ export function getProxyRetryFromErr({ err, proxyConfig }: { err: unknown; proxy
+ // Mark for token refresh but don't return yet — Retry-After headers take priority
+ // so a rate-limited response with a backoff header is respected before refreshing
+ const isRefreshToken = proxyConfig.refreshTokenOn?.some((code) => matchesStatusCode(status, String(code))) ?? false;
+ if (isRefreshToken) {
+ isRetryable = true;
</file context>
| } | ||
| } | ||
| } | ||
| } else if (retry.retry && attempt > (this.config.retries || 0)) { |
There was a problem hiding this comment.
P2: For Salesforce calls going through any ProxyRequest path that does not supply onRefreshToken (SDK, Slack notifications, hooks, AWS SigV4, internal-nango), refreshTokenOn is still set, so a 401/403 is labelled 'refresh_token' and refreshTokenAttempts caps the retry at one regardless of the configured retries. No refresh is performed in these paths, so the request just re-fetches the same stale credentials once and then stops with a 'refresh_token_max_attempts' reason. This silently changes retry behavior for these callers (previously a 401 was retried up to retries) and logs a refresh reason that never reflects a real refresh. Consider only entering the refresh branch when onRefreshToken is actually present (and otherwise falling through to normal retries handling), and resetting refreshTokenAttempts at the start of request() so state never leaks if the instance is reused.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/shared/lib/services/proxy/request.ts, line 189:
<comment>For Salesforce calls going through any ProxyRequest path that does not supply `onRefreshToken` (SDK, Slack notifications, hooks, AWS SigV4, internal-nango), `refreshTokenOn` is still set, so a 401/403 is labelled 'refresh_token' and `refreshTokenAttempts` caps the retry at one regardless of the configured `retries`. No refresh is performed in these paths, so the request just re-fetches the same stale credentials once and then stops with a 'refresh_token_max_attempts' reason. This silently changes retry behavior for these callers (previously a 401 was retried up to `retries`) and logs a refresh reason that never reflects a real refresh. Consider only entering the refresh branch when `onRefreshToken` is actually present (and otherwise falling through to normal `retries` handling), and resetting `refreshTokenAttempts` at the start of `request()` so state never leaks if the instance is reused.</comment>
<file context>
@@ -158,10 +170,26 @@ export class ProxyRequest {
+ }
+ }
+ }
+ } else if (retry.retry && attempt > (this.config.retries || 0)) {
+ retry = { retry: false, reason: retry.reason };
+ }
</file context>
| // Mark for token refresh but don't return yet — Retry-After headers take priority | ||
| // so a rate-limited response with a backoff header is respected before refreshing | ||
| const isRefreshToken = proxyConfig.refreshTokenOn?.some((code) => matchesStatusCode(status, String(code))) ?? false; | ||
| if (isRefreshToken) { |
There was a problem hiding this comment.
P2: The comment in retry.ts states that Retry-After headers should take priority over a token refresh so a rate-limited response gets its backoff respected. But the new else-if in request.ts discards any non-refresh retry once attempt > retries. With the default Salesforce retries=0, a 401/403 carrying a Retry-After header yields reason custom_after (not 'refresh_token'), so it hits the else-if at attempt=1 and is aborted (1 > 0) instead of being backed off before a refresh. This makes the documented Retry-After-priority behavior unreachable for the default refresh-token case. Worth reconciling the two files so a header-driven backoff on a refreshable status is actually retried.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/shared/lib/services/proxy/retry.ts, line 61:
<comment>The comment in retry.ts states that Retry-After headers should take priority over a token refresh so a rate-limited response gets its backoff respected. But the new else-if in request.ts discards any non-refresh retry once `attempt > retries`. With the default Salesforce retries=0, a 401/403 carrying a Retry-After header yields reason `custom_after` (not 'refresh_token'), so it hits the else-if at attempt=1 and is aborted (1 > 0) instead of being backed off before a refresh. This makes the documented Retry-After-priority behavior unreachable for the default refresh-token case. Worth reconciling the two files so a header-driven backoff on a refreshable status is actually retried.</comment>
<file context>
@@ -55,6 +55,13 @@ export function getProxyRetryFromErr({ err, proxyConfig }: { err: unknown; proxy
+ // Mark for token refresh but don't return yet — Retry-After headers take priority
+ // so a rate-limited response with a backoff header is respected before refreshing
+ const isRefreshToken = proxyConfig.refreshTokenOn?.some((code) => matchesStatusCode(status, String(code))) ?? false;
+ if (isRefreshToken) {
+ isRetryable = true;
+ }
</file context>
| } | ||
| freshConnection = credentialResponse.value; | ||
| lastConnectionRefresh = Date.now(); | ||
| return refreshed; |
There was a problem hiding this comment.
P2: The refreshed flag returned from onRefreshToken is only set true inside this call's own onRefreshSuccess callback. If a concurrent request for the same connection already triggered the refresh (and refreshCredentialsIfNeeded dedupes via its in-flight cache), this caller's onRefreshSuccess may never fire even though the token was refreshed, leaving refreshed false. ProxyRequest then treats the token as still active and returns the original auth error instead of retrying with the now-valid freshConnection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server/lib/controllers/proxy/allProxy.ts, line 379:
<comment>The `refreshed` flag returned from `onRefreshToken` is only set true inside this call's own `onRefreshSuccess` callback. If a concurrent request for the same connection already triggered the refresh (and refreshCredentialsIfNeeded dedupes via its in-flight cache), this caller's `onRefreshSuccess` may never fire even though the token was refreshed, leaving `refreshed` false. ProxyRequest then treats the token as still active and returns the original auth error instead of retrying with the now-valid `freshConnection`.</comment>
<file context>
@@ -354,6 +355,29 @@ export const allPublicProxy = asyncWrapper<AllPublicProxy>(async (req, res, next
+ }
+ freshConnection = credentialResponse.value;
+ lastConnectionRefresh = Date.now();
+ return refreshed;
+ },
getConnection: async () => {
</file context>
| return err; | ||
| } | ||
|
|
||
| function createAxiosError(status: number): AxiosError { |
There was a problem hiding this comment.
P3: createAxiosError duplicates the existing makeAxiosError helper in the same file — same err.response shape, differing only in the message/statusText strings — leaving two near-identical factory helpers. Consider reusing a single helper (e.g., parameterize or just call makeAxiosError) to avoid parallel code that can drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/shared/lib/services/proxy/request.unit.test.ts, line 22:
<comment>`createAxiosError` duplicates the existing `makeAxiosError` helper in the same file — same `err.response` shape, differing only in the message/statusText strings — leaving two near-identical factory helpers. Consider reusing a single helper (e.g., parameterize or just call `makeAxiosError`) to avoid parallel code that can drift.</comment>
<file context>
@@ -19,6 +19,16 @@ function makeAxiosError(status: number): AxiosError {
return err;
}
+function createAxiosError(status: number): AxiosError {
+ const err = new AxiosError(`HTTP ${status}`);
+ err.response = { status, data: {}, headers: {}, statusText: 'Error', config: {} as InternalAxiosRequestConfig };
</file context>
| providerConfigKey: config.unique_key, | ||
| decompress: false | ||
| decompress: false, | ||
| refreshTokenOn: null |
There was a problem hiding this comment.
P3: This added refreshTokenOn: null has no effect: getProxyConfiguration ignores externalConfig.refreshTokenOn (it isn't destructured) and recomputes it from providerClient.shouldIntrospectToken(providerName). The PR aims to introspect Salesforce tokens on error, so either wire the value through getProxyConfiguration so this config actually influences the request, or drop the misleading line.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server/lib/hooks/hooks.ts, line 425:
<comment>This added `refreshTokenOn: null` has no effect: `getProxyConfiguration` ignores `externalConfig.refreshTokenOn` (it isn't destructured) and recomputes it from `providerClient.shouldIntrospectToken(providerName)`. The PR aims to introspect Salesforce tokens on error, so either wire the value through `getProxyConfiguration` so this config actually influences the request, or drop the misleading line.</comment>
<file context>
@@ -421,7 +421,8 @@ export async function credentialsTest({
providerConfigKey: config.unique_key,
- decompress: false
+ decompress: false,
+ refreshTokenOn: null
};
</file context>
1 participant
Describe the problem and your solution