From 9bcf1aa57e2eed503adef3bf5ff544369cf62033 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Wed, 2 Sep 2026 12:55:52 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=AA=20fix:=20Keep=20an=20Action=20OAut?= =?UTF-8?q?h=20Abort=20an=20Abort?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- api/server/services/ActionService.js | 21 ++++ api/server/services/ActionService.spec.js | 115 +++++++++++++++++++++- 2 files changed, 134 insertions(+), 2 deletions(-) diff --git a/api/server/services/ActionService.js b/api/server/services/ActionService.js index cd81d76082..7caa89064e 100644 --- a/api/server/services/ActionService.js +++ b/api/server/services/ActionService.js @@ -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 }); } diff --git a/api/server/services/ActionService.spec.js b/api/server/services/ActionService.spec.js index 8f6f9ae83e..cd6b8935af 100644 --- a/api/server/services/ActionService.spec.js +++ b/api/server/services/ActionService.spec.js @@ -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' } };