mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-10-22 10:02:41 +00:00
🪪 fix: Keep an Action OAuth Abort an Abort
Addresses two of the three findings from the Codex review of 70fc0782b1.
Detaching the stopped waiter left the rejection to be misread on the way
out. The refresh catch treated it as a failed refresh and called
`requestLogin()`, emitting an OAuth prompt and opening pending
authorization state for a turn that had already ended. `requestLogin` and
the surrounding auth catch relabelled it `Failed to authenticate OAuth
tool` / `Authentication failed`, and the outer catch handed that to
`logAxiosError` — so an ordinary Stop produced two error-level entries
and returned failure text as the tool's result. Each boundary now lets an
abort through unchanged, and the outer catch logs it at debug and
rethrows so it stays a cancellation end to end.
Deferred: waiter-only cancellation in `FlowStateManager`, so a detached
`monitorFlow` stops polling instead of running out the flow's TTL. That
poller is bounded at three minutes and lives exactly as long as it does
for a waiter that never left, so it changes no lifetime this branch
introduced; giving `FlowStateManager` a non-destructive abort mode is its
own change, shared with MCP and indexSync.
This commit is contained in:
parent
70fc0782b1
commit
9bcf1aa57e
2 changed files with 134 additions and 2 deletions
|
|
@ -5,6 +5,7 @@ const { tool } = require('@librechat/agents/langchain/tools');
|
|||
const { logger, decryptV2 } = require('@librechat/data-schemas');
|
||||
const {
|
||||
sendEvent,
|
||||
isAbortError,
|
||||
logAxiosError,
|
||||
detachOnAbort,
|
||||
getTokenExpiresAt,
|
||||
|
|
@ -308,6 +309,12 @@ async function createActionTool({
|
|||
const expiresAt = getTokenExpiresAt(result.expires_in);
|
||||
metadata.oauth_token_expires_at = expiresAt?.toISOString();
|
||||
} catch (error) {
|
||||
/** A stopped run is not an authentication failure. Relabelling it
|
||||
* loses the abort identity every downstream boundary keys on and
|
||||
* reports a fault the user caused deliberately. */
|
||||
if (isAbortError(error)) {
|
||||
throw error;
|
||||
}
|
||||
const errorMessage = 'Failed to authenticate OAuth tool';
|
||||
logger.error(errorMessage, error);
|
||||
throw new Error(errorMessage);
|
||||
|
|
@ -377,6 +384,12 @@ async function createActionTool({
|
|||
const expiresAt = getTokenExpiresAt(refreshData.expires_in);
|
||||
metadata.oauth_token_expires_at = expiresAt?.toISOString();
|
||||
} catch (error) {
|
||||
/** The refresh did not fail — this run stopped waiting on it.
|
||||
* Falling through to `requestLogin` would emit an OAuth prompt
|
||||
* and open pending authorization state for a turn that is over. */
|
||||
if (isAbortError(error)) {
|
||||
throw error;
|
||||
}
|
||||
logger.error('Failed to refresh token, requesting new login:', error);
|
||||
await requestLogin();
|
||||
}
|
||||
|
|
@ -388,6 +401,7 @@ async function createActionTool({
|
|||
await preparedExecutor.setAuth(metadata);
|
||||
} catch (error) {
|
||||
if (
|
||||
isAbortError(error) ||
|
||||
error.message.includes('No access token found') ||
|
||||
error.message.includes('Access token is expired')
|
||||
) {
|
||||
|
|
@ -404,6 +418,13 @@ async function createActionTool({
|
|||
}
|
||||
return response.data;
|
||||
} catch (error) {
|
||||
/** Surface the cancellation as a cancellation: `logAxiosError` would log it
|
||||
* at error level and return its text as the tool's result, presenting a
|
||||
* stopped turn as a failed API call. */
|
||||
if (isAbortError(error)) {
|
||||
logger.debug(`Action call to ${action.metadata.domain} cancelled by user abort`);
|
||||
throw error;
|
||||
}
|
||||
const message = `API call to ${action.metadata.domain} failed:`;
|
||||
return logAxiosError({ message, error });
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,3 +1,5 @@
|
|||
const dataSchemas = require('@librechat/data-schemas');
|
||||
const { logger } = dataSchemas;
|
||||
const { Constants, actionDelimiter, actionDomainSeparator } = require('librechat-data-provider');
|
||||
|
||||
const mockEmitChunk = jest.fn();
|
||||
|
|
@ -471,8 +473,7 @@ describe('createActionTool OAuth stop behavior', () => {
|
|||
await new Promise((resolve) => setImmediate(resolve));
|
||||
controller.abort();
|
||||
|
||||
/** `_call` reports failures through logAxiosError rather than throwing. */
|
||||
await call;
|
||||
await expect(call).rejects.toThrow();
|
||||
|
||||
completeFlow({
|
||||
access_token: 'access-token',
|
||||
|
|
@ -485,6 +486,116 @@ describe('createActionTool OAuth stop behavior', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('createActionTool OAuth abort identity', () => {
|
||||
const oauthAction = {
|
||||
action_id: 'action-1',
|
||||
metadata: {
|
||||
domain: 'https://api.example.com',
|
||||
oauth_client_id: 'client-id',
|
||||
auth: {
|
||||
type: 'oauth',
|
||||
authorization_url: 'https://auth.example.com/authorize',
|
||||
client_url: 'https://auth.example.com/token',
|
||||
scope: 'read',
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
function oauthToolFixture() {
|
||||
const preparedExecutor = {
|
||||
setAuth: jest.fn().mockResolvedValue(undefined),
|
||||
execute: jest.fn().mockResolvedValue({ data: { ok: true } }),
|
||||
};
|
||||
return {
|
||||
preparedExecutor,
|
||||
requestBuilder: {
|
||||
createExecutor: jest.fn(() => ({
|
||||
setParams: jest.fn(() => preparedExecutor),
|
||||
})),
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
function callConfig(signal) {
|
||||
return {
|
||||
signal,
|
||||
metadata: { thread_id: 'thread-1', run_id: 'run-1' },
|
||||
toolCall: { id: 'tool-call-1', stepId: 'step-1', name: 'action-tool', type: 'tool_call' },
|
||||
};
|
||||
}
|
||||
|
||||
it('does not request a fresh login when a stopped run detaches from a refresh', async () => {
|
||||
const { requestBuilder, preparedExecutor } = oauthToolFixture();
|
||||
/** No access token but a live refresh token: the branch that waits on the
|
||||
* shared refresh flow rather than opening a login. */
|
||||
mockFindToken.mockImplementation(async ({ type }) =>
|
||||
type === 'oauth_refresh' ? { token: 'encrypted-refresh-token' } : null,
|
||||
);
|
||||
jest.spyOn(dataSchemas, 'decryptV2').mockResolvedValue('refresh-token');
|
||||
|
||||
/** The refresh never settles; the run stops while it is pending. */
|
||||
mockActionFlowManager.createFlowWithHandler.mockImplementation(() => new Promise(() => {}));
|
||||
mockActionFlowManager.createFlow.mockImplementation(() => new Promise(() => {}));
|
||||
|
||||
const actionTool = await createActionTool({
|
||||
userId: 'action-user',
|
||||
res: {},
|
||||
action: oauthAction,
|
||||
requestBuilder,
|
||||
encrypted: {
|
||||
oauth_client_id: 'encrypted-client-id',
|
||||
oauth_client_secret: 'encrypted-client-secret',
|
||||
},
|
||||
});
|
||||
|
||||
const controller = new AbortController();
|
||||
const call = actionTool._call({}, callConfig(controller.signal));
|
||||
await new Promise((resolve) => setImmediate(resolve));
|
||||
controller.abort();
|
||||
await expect(call).rejects.toThrow();
|
||||
|
||||
const loginFlows = mockActionFlowManager.createFlowWithHandler.mock.calls.filter(
|
||||
([, type]) => type === 'oauth_login',
|
||||
);
|
||||
expect(loginFlows).toHaveLength(0);
|
||||
expect(mockEmitChunk).not.toHaveBeenCalled();
|
||||
expect(preparedExecutor.execute).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('propagates the abort instead of reporting a failed API call', async () => {
|
||||
const { requestBuilder } = oauthToolFixture();
|
||||
mockFindToken.mockResolvedValue(null);
|
||||
mockActionFlowManager.createFlowWithHandler.mockImplementation(
|
||||
async (_flowId, _type, handler) => handler(),
|
||||
);
|
||||
mockActionFlowManager.createFlow.mockImplementation(() => new Promise(() => {}));
|
||||
|
||||
const actionTool = await createActionTool({
|
||||
userId: 'action-user',
|
||||
res: {},
|
||||
action: oauthAction,
|
||||
requestBuilder,
|
||||
encrypted: {
|
||||
oauth_client_id: 'encrypted-client-id',
|
||||
oauth_client_secret: 'encrypted-client-secret',
|
||||
},
|
||||
streamId: 'action-oauth-stream',
|
||||
jobCreatedAt: 1234,
|
||||
});
|
||||
|
||||
const controller = new AbortController();
|
||||
const call = actionTool._call({}, callConfig(controller.signal));
|
||||
await new Promise((resolve) => setImmediate(resolve));
|
||||
controller.abort();
|
||||
|
||||
await expect(call).rejects.toBe(controller.signal.reason);
|
||||
expect(logger.error).not.toHaveBeenCalledWith(
|
||||
'Failed to authenticate OAuth tool',
|
||||
expect.anything(),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('validateAndUpdateTool', () => {
|
||||
const mockReq = { user: { id: 'user123' } };
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue