mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-03 22:32:42 +00:00
🛡️ fix: Prevent silent crash from unhandled MCP OAuth reconnect rejections (#12812)
* 🛡️ 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
This commit is contained in:
parent
0af6bcf6f1
commit
738003b220
4 changed files with 184 additions and 30 deletions
|
|
@ -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 });
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue