From 738003b220a91ec724286dab0354080e22f8aac9 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 24 Apr 2026 23:18:49 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20fix:=20Prevent=20silent?= =?UTF-8?q?=20crash=20from=20unhandled=20MCP=20OAuth=20reconnect=20rejecti?= =?UTF-8?q?ons=20(#12812)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ๐Ÿ›ก๏ธ fix: Install global `unhandledRejection` handler Node 15+ terminates the process by default when a promise rejection goes unhandled. Under MCP OAuth reconnect storms and streamable-HTTP transport resets, fire-and-forget async paths can emit transient rejections (ECONNRESET, token refresh races) that would otherwise silently kill the server โ€” no uncaught exception log, no OOM signal. Register a listener so these paths log and the process keeps serving other requests. Refs: #12078 * ๐Ÿ”ง fix: Guard MCP OAuth reconnect fire-and-forget calls `OAuthReconnectionManager.tryReconnect` awaits `getServerConfig` outside its inner try/catch, so a rejection from the registry (or any throw before the guarded block) would escape the fire-and-forget `void` call sites and propagate as an unhandled rejection โ€” the failure mode behind the silent crashes reported in #12078. Route both call sites through a `safeTryReconnect` wrapper that attaches a terminal `.catch` so unexpected rejections are surfaced via the logger instead. Refs: #12078 * ๐Ÿงน fix: Address review findings on MCP OAuth reconnect crash fix - Move `getServerConfig` inside `tryReconnect`'s try/catch so the registry rejection path is handled by the inner cleanup (the structural root cause behind the silent crash). The outer `safeTryReconnect` wrapper remains as defense-in-depth. - Extract the failed-reconnect cleanup as a private `cleanupOnFailedReconnect` method and invoke it from `safeTryReconnect`'s catch as well, so any rejection that does escape the inner try (e.g. a future regression) still resets tracker state instead of leaving the server stuck in `active` for the full `RECONNECTION_TIMEOUT_MS` window. - Update the regression test to assert tracker state is cleaned up (`isActive` cleared, `isFailed` set, `disconnectUserConnection` called) so it can detect the stale-state failure mode it was meant to guard against. - Forward non-Error rejection reasons as-is in the global handler so structured payloads like `{ code: "ECONNRESET", errno: -104 }` survive instead of being collapsed to "[object Object]" by `String()`. Refs: #12078, review of #12812 * ๐Ÿš‘ fix: Restore fail-fast on boot rejection in primary server entry `startServer()` was invoked bare in `api/server/index.js`. Before installing the global `unhandledRejection` handler, a startup rejection (`connectDb`, `getAppConfig`, `performStartupChecks`) terminated the process via Node's default โ€” Kubernetes / the orchestrator restarted the pod immediately. After the handler was added, the same rejection was caught and logged, then the process kept running half-initialized (no HTTP listener) until the liveness probe eventually timed out โ€” slow, indirect recovery instead of a fast restart. Wrap `startServer()` with the same `.catch(() => process.exit(1))` pattern already used in `experimental.js` so boot failures fail-fast. Refs: #12078, codex review of #12812 * ๐Ÿš‘ fix: Fail-fast on post-listen init failure in both server entries The `app.listen` callback in `index.js` and `experimental.js` is async and awaits `initializeMCPs`, `initializeOAuthReconnectManager`, and `checkMigrations`. The callback's promise is detached from `startServer().catch(...)` (the outer catch only sees errors that occurred before `app.listen` was called), so without explicit handling those init rejections used to terminate the process via Node's default and now would be swallowed by the new `unhandledRejection` handler โ€” leaving the HTTP server listening (and passing liveness probes) while MCP / OAuth / migration state is broken. Wrap the post-listen init block in a try/catch that logs and calls `process.exit(1)` so initialization failures stay fail-fast. Refs: #12078, codex review of #12812 --- api/server/experimental.js | 46 ++++++++++- api/server/index.js | 76 +++++++++++++++---- .../oauth/OAuthReconnectionManager.test.ts | 50 +++++++++++- .../src/mcp/oauth/OAuthReconnectionManager.ts | 42 +++++++--- 4 files changed, 184 insertions(+), 30 deletions(-) diff --git a/api/server/experimental.js b/api/server/experimental.js index ff023b4504..3dfd5aee3f 100644 --- a/api/server/experimental.js +++ b/api/server/experimental.js @@ -362,10 +362,22 @@ if (cluster.isMaster) { }:${port}`, ); - /** Initialize MCP servers and OAuth reconnection for this worker */ - await initializeMCPs(); - await initializeOAuthReconnectManager(); - await checkMigrations(); + /** + * The listen callback is async, so any rejection from these awaits + * would otherwise be detached from `startServer().catch(...)`. Without + * explicit handling, the global `unhandledRejection` handler would + * swallow init failures and leave the worker listening but only + * partially initialized. + */ + try { + /** Initialize MCP servers and OAuth reconnection for this worker */ + await initializeMCPs(); + await initializeOAuthReconnectManager(); + await checkMigrations(); + } catch (initErr) { + logger.error(`Worker ${process.pid} post-listen initialization failed:`, initErr); + process.exit(1); + } }); /** Handle inter-process messages from master */ @@ -441,3 +453,29 @@ process.on('uncaughtException', (err) => { process.exit(1); }); + +/** + * Unhandled promise rejection handler. + * + * Node 15+ terminates the process by default when a promise rejection is + * unhandled. MCP OAuth reconnect storms and streamable-HTTP transport resets + * can produce transient fire-and-forget rejections (ECONNRESET, token refresh + * races) that are recoverable โ€” the server should log and keep serving other + * requests rather than silently crash under load. + * + * Non-Error reasons are forwarded as-is so structured payloads (e.g. + * `{ code: "ECONNRESET", errno: -104 }`) survive instead of being collapsed to + * "[object Object]" by `String()`. + */ +process.on('unhandledRejection', (reason) => { + if (reason instanceof Error) { + logger.error('Unhandled promise rejection. The app will continue running.', { + name: reason.name, + message: reason.message, + stack: reason.stack, + cause: reason.cause, + }); + return; + } + logger.error('Unhandled promise rejection. The app will continue running.', { reason }); +}); diff --git a/api/server/index.js b/api/server/index.js index d26a203c0a..adeaacdfcc 100644 --- a/api/server/index.js +++ b/api/server/index.js @@ -222,25 +222,49 @@ const startServer = async () => { logger.info(`Server listening at http://${host == '0.0.0.0' ? 'localhost' : host}:${port}`); } - await runAsSystem(async () => { - await initializeMCPs(); - await initializeOAuthReconnectManager(); - }); - await checkMigrations(); + /** + * The listen callback is async, so any rejection from these awaits would + * otherwise be detached from `startServer().catch(...)` (which only + * catches errors that happen before `app.listen`). Without explicit + * handling, the global `unhandledRejection` handler would swallow init + * failures and leave the server listening but only partially + * initialized โ€” passing liveness checks while serving broken requests. + */ + try { + await runAsSystem(async () => { + await initializeMCPs(); + await initializeOAuthReconnectManager(); + }); + await checkMigrations(); - // Configure stream services (auto-detects Redis from USE_REDIS env var) - const streamServices = createStreamServices(); - GenerationJobManager.configure(streamServices); - GenerationJobManager.initialize(); + // Configure stream services (auto-detects Redis from USE_REDIS env var) + const streamServices = createStreamServices(); + GenerationJobManager.configure(streamServices); + GenerationJobManager.initialize(); - const inspectFlags = process.execArgv.some((arg) => arg.startsWith('--inspect')); - if (inspectFlags || isEnabled(process.env.MEM_DIAG)) { - memoryDiagnostics.start(); + const inspectFlags = process.execArgv.some((arg) => arg.startsWith('--inspect')); + if (inspectFlags || isEnabled(process.env.MEM_DIAG)) { + memoryDiagnostics.start(); + } + } catch (initErr) { + logger.error('Post-listen initialization failed:', initErr); + process.exit(1); } }); }; -startServer(); +/** + * Boot rejections (e.g. `connectDb`, `getAppConfig`, `performStartupChecks`) + * must remain fail-fast: a half-initialized process with no listening HTTP + * server should die immediately so the orchestrator restarts it, instead of + * being kept alive by the `unhandledRejection` handler below until the + * liveness probe eventually times out. Mirrors the pattern in + * `experimental.js`. + */ +startServer().catch((err) => { + logger.error('Failed to start server:', err); + process.exit(1); +}); let messageCount = 0; process.on('uncaughtException', (err) => { @@ -299,5 +323,31 @@ process.on('uncaughtException', (err) => { process.exit(1); }); +/** + * Unhandled promise rejection handler. + * + * Node 15+ terminates the process by default when a promise rejection is + * unhandled. MCP OAuth reconnect storms and streamable-HTTP transport resets + * can produce transient fire-and-forget rejections (ECONNRESET, token refresh + * races) that are recoverable โ€” the server should log and keep serving other + * requests rather than silently crash under load. + * + * Non-Error reasons are forwarded as-is so structured payloads (e.g. + * `{ code: "ECONNRESET", errno: -104 }`) survive instead of being collapsed to + * "[object Object]" by `String()`. + */ +process.on('unhandledRejection', (reason) => { + if (reason instanceof Error) { + logger.error('Unhandled promise rejection. The app will continue running.', { + name: reason.name, + message: reason.message, + stack: reason.stack, + cause: reason.cause, + }); + return; + } + logger.error('Unhandled promise rejection. The app will continue running.', { reason }); +}); + /** Export app for easier testing purposes */ module.exports = app; diff --git a/packages/api/src/mcp/oauth/OAuthReconnectionManager.test.ts b/packages/api/src/mcp/oauth/OAuthReconnectionManager.test.ts index d889da4f2f..23a3278797 100644 --- a/packages/api/src/mcp/oauth/OAuthReconnectionManager.test.ts +++ b/packages/api/src/mcp/oauth/OAuthReconnectionManager.test.ts @@ -1,4 +1,4 @@ -import { TokenMethods } from '@librechat/data-schemas'; +import { logger, TokenMethods } from '@librechat/data-schemas'; import { FlowStateManager, MCPConnection, MCPOAuthTokens, MCPOptions } from '../..'; import { MCPManager } from '../MCPManager'; import { OAuthReconnectionManager } from './OAuthReconnectionManager'; @@ -547,6 +547,54 @@ describe('OAuthReconnectionManager', () => { }); }); + describe('fire-and-forget reconnect safety', () => { + let reconnectionTracker: OAuthReconnectionTracker; + + beforeEach(async () => { + reconnectionTracker = new OAuthReconnectionTracker(); + reconnectionManager = await OAuthReconnectionManager.createInstance( + flowManager, + tokenMethods, + reconnectionTracker, + ); + }); + + /** + * Regression test for discussion #12078: a registry rejection from + * `getServerConfig` during a reconnect storm previously escaped as an + * unhandled promise rejection (Node 15+ terminates the process). The + * rejection must be caught and the tracker must be cleaned up so the + * server does not stay stuck in `active` state for the full + * `RECONNECTION_TIMEOUT_MS` window before retries become possible again. + */ + it('should clean up tracker state when getServerConfig rejects', async () => { + const userId = 'user-123'; + const oauthServers = new Set(['server1']); + (mockRegistryInstance.getOAuthServers as jest.Mock).mockResolvedValue(oauthServers); + + tokenMethods.findToken.mockResolvedValue({ + userId, + identifier: 'mcp:server1', + expiresAt: new Date(Date.now() + 3600000), + } as unknown as MCPOAuthTokens); + + const boom = new Error('boom'); + (mockRegistryInstance.getServerConfig as jest.Mock).mockRejectedValue(boom); + + await expect(reconnectionManager.reconnectServers(userId)).resolves.toBeUndefined(); + + // Flush any microtasks attached inside safeTryReconnect / tryReconnect + await new Promise((resolve) => setImmediate(resolve)); + + // The rejection must be reported (warn from the inner catch) and the + // tracker must be returned to a state that allows future retries. + expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining('Failed to reconnect')); + expect(reconnectionTracker.isActive(userId, 'server1')).toBe(false); + expect(reconnectionTracker.isFailed(userId, 'server1')).toBe(true); + expect(mockMCPManager.disconnectUserConnection).toHaveBeenCalledWith(userId, 'server1'); + }); + }); + describe('reconnection timeout behavior', () => { let reconnectionTracker: OAuthReconnectionTracker; diff --git a/packages/api/src/mcp/oauth/OAuthReconnectionManager.ts b/packages/api/src/mcp/oauth/OAuthReconnectionManager.ts index 7afe992772..e0e30d7e14 100644 --- a/packages/api/src/mcp/oauth/OAuthReconnectionManager.ts +++ b/packages/api/src/mcp/oauth/OAuthReconnectionManager.ts @@ -89,13 +89,37 @@ export class OAuthReconnectionManager { for (let i = 0; i < serversToReconnect.length; i++) { const serverName = serversToReconnect[i]; if (i === 0) { - void this.tryReconnect(userId, serverName); + this.safeTryReconnect(userId, serverName); } else { - setTimeout(() => void this.tryReconnect(userId, serverName), i * RECONNECT_STAGGER_MS); + setTimeout(() => this.safeTryReconnect(userId, serverName), i * RECONNECT_STAGGER_MS); } } } + /** + * Fire-and-forget wrapper around {@link tryReconnect} that guarantees any + * unexpected rejection is surfaced via the logger instead of propagating as + * an unhandled promise rejection. Also runs the failed-reconnect cleanup so + * the tracker does not get stuck in `active` state for the + * `RECONNECTION_TIMEOUT_MS` window if an error escapes + * {@link tryReconnect}'s internal try/catch. + */ + private safeTryReconnect(userId: string, serverName: string): void { + this.tryReconnect(userId, serverName).catch((error) => { + logger.error( + `[OAuthReconnectionManager][User: ${userId}][${serverName}] Unexpected reconnect error`, + error, + ); + this.cleanupOnFailedReconnect(userId, serverName); + }); + } + + private cleanupOnFailedReconnect(userId: string, serverName: string): void { + this.reconnectionsTracker.setFailed(userId, serverName); + this.reconnectionsTracker.removeActive(userId, serverName); + this.mcpManager?.disconnectUserConnection(userId, serverName); + } + /** * Attempts to reconnect a single OAuth MCP server. * @returns true if reconnection succeeded, false otherwise. @@ -128,15 +152,9 @@ export class OAuthReconnectionManager { logger.info(`${logPrefix} Attempting reconnection`); - const config = await MCPServersRegistry.getInstance().getServerConfig(serverName, userId); - - const cleanupOnFailedReconnect = () => { - this.reconnectionsTracker.setFailed(userId, serverName); - this.reconnectionsTracker.removeActive(userId, serverName); - this.mcpManager?.disconnectUserConnection(userId, serverName); - }; - try { + const config = await MCPServersRegistry.getInstance().getServerConfig(serverName, userId); + // attempt to get connection (this will use existing tokens and refresh if needed) const connection = await this.mcpManager.getUserConnection({ serverName, @@ -157,11 +175,11 @@ export class OAuthReconnectionManager { } else { logger.warn(`${logPrefix} Failed to reconnect`); await connection?.disconnect(); - cleanupOnFailedReconnect(); + this.cleanupOnFailedReconnect(userId, serverName); } } catch (error) { logger.warn(`${logPrefix} Failed to reconnect: ${error}`); - cleanupOnFailedReconnect(); + this.cleanupOnFailedReconnect(userId, serverName); } }