fix(server): survive stray uncaught exceptions and invalid dev-tunnel base URLs
A single uncaught exception (e.g. a Node-internal socket error) no longer shuts the local server down; only a sustained storm does. The dev-tunnel client now rejects non-http(s) base URLs cleanly instead of throwing an uncaught exception in the connection handler.
This commit is contained in:
@@ -27,6 +27,14 @@ const HANDSHAKE_TIMEOUT_MS = 15_000;
|
||||
|
||||
const toWebSocketUrl = (baseUrl, port) => {
|
||||
const parsed = new URL('/api/dev-tunnel', baseUrl);
|
||||
// WHATWG URL silently ignores a protocol assignment that crosses from a
|
||||
// non-special scheme (custom app protocols, relay-virtual URLs) to `ws:`.
|
||||
// Without this check the stale scheme survives into `new WebSocket(...)`,
|
||||
// which then throws inside the connection handler and takes the whole
|
||||
// process down; rejecting here fails the open() call cleanly instead.
|
||||
if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') {
|
||||
throw new Error(`The remote base URL must be http(s); got "${parsed.protocol}"`);
|
||||
}
|
||||
parsed.protocol = parsed.protocol === 'https:' ? 'wss:' : 'ws:';
|
||||
parsed.searchParams.set('port', String(port));
|
||||
return parsed.toString();
|
||||
@@ -76,7 +84,18 @@ export const createDevTunnelClient = ({
|
||||
socket.setNoDelay(true);
|
||||
sockets.add(socket);
|
||||
|
||||
const upstream = new WebSocket(target, { headers, perMessageDeflate: false });
|
||||
// A synchronous throw here would be an uncaught exception in the
|
||||
// connection handler and crash the process; one bad connection must
|
||||
// fail alone.
|
||||
let upstream;
|
||||
try {
|
||||
upstream = new WebSocket(target, { headers, perMessageDeflate: false });
|
||||
} catch (error) {
|
||||
logger.warn?.(`[dev-tunnel] failed to dial upstream for port ${remotePort}: ${error?.message || error}`);
|
||||
sockets.delete(socket);
|
||||
try { socket.destroy(); } catch { /* already gone */ }
|
||||
return;
|
||||
}
|
||||
upstream.binaryType = 'nodebuffer';
|
||||
let pendingWrites = [];
|
||||
let pendingBytes = 0;
|
||||
|
||||
@@ -195,6 +195,15 @@ describe('dev tunnel end to end', () => {
|
||||
expect(client.list()).toEqual([]);
|
||||
});
|
||||
|
||||
test('rejects a non-http(s) base URL instead of crashing on first connection', async () => {
|
||||
// A non-special scheme survives the `ws:` protocol assignment (WHATWG URL
|
||||
// ignores it), so `new WebSocket(...)` used to throw inside the connection
|
||||
// handler and take the whole process down.
|
||||
const client = createDevTunnelClient({ logger: { warn: () => {} } });
|
||||
await expect(client.open({ baseUrl: 'openchamber-ui://index', port: 5173 })).rejects.toThrow('must be http(s)');
|
||||
expect(client.list()).toEqual([]);
|
||||
});
|
||||
|
||||
// Not covered here: recovery after a request the dev server kills mid-flight.
|
||||
// The behaviour is real (each connection tears down independently), but the
|
||||
// abandoned socket makes this harness's teardown unreliable, and a flaky test
|
||||
|
||||
@@ -1,3 +1,7 @@
|
||||
/** One stray uncaught exception is survivable; a storm means the process is broken. */
|
||||
const UNCAUGHT_STORM_LIMIT = 10;
|
||||
const UNCAUGHT_STORM_WINDOW_MS = 60_000;
|
||||
|
||||
export const createServerStartupRuntime = (dependencies) => {
|
||||
const {
|
||||
process,
|
||||
@@ -148,9 +152,26 @@ export const createServerStartupRuntime = (dependencies) => {
|
||||
console.error('Unhandled Rejection at:', promise, 'reason:', reason);
|
||||
});
|
||||
|
||||
// A single stray exception — a socket teardown race, a Node-internal bug
|
||||
// like `setTypeOfService EINVAL` — must not take the server down. Nothing
|
||||
// restarts this process (it is embedded in the desktop app or run by hand
|
||||
// in a terminal), so shutting down turns every such stray into "the
|
||||
// instance is unreachable until I restart it". Mirror the
|
||||
// unhandledRejection policy above: log and keep serving. A sustained storm
|
||||
// of exceptions is a different situation — the process is genuinely
|
||||
// broken — so that still shuts down rather than limping along half-alive.
|
||||
const exceptionTimes = [];
|
||||
process.on('uncaughtException', (error) => {
|
||||
console.error('Uncaught Exception:', error);
|
||||
gracefulShutdown();
|
||||
const now = Date.now();
|
||||
exceptionTimes.push(now);
|
||||
while (exceptionTimes.length > 0 && now - exceptionTimes[0] > UNCAUGHT_STORM_WINDOW_MS) {
|
||||
exceptionTimes.shift();
|
||||
}
|
||||
if (exceptionTimes.length > UNCAUGHT_STORM_LIMIT) {
|
||||
console.error(`More than ${UNCAUGHT_STORM_LIMIT} uncaught exceptions within a minute; shutting down.`);
|
||||
gracefulShutdown();
|
||||
}
|
||||
});
|
||||
};
|
||||
|
||||
|
||||
@@ -0,0 +1,44 @@
|
||||
import { describe, expect, test } from 'bun:test';
|
||||
import { EventEmitter } from 'node:events';
|
||||
import { createServerStartupRuntime } from './server-startup-runtime.js';
|
||||
|
||||
/**
|
||||
* The desktop app embeds this server and nothing restarts it, so shutting down
|
||||
* on a single uncaught exception turned every stray socket error into "the
|
||||
* instance is unreachable until restarted". Only a sustained storm shuts down.
|
||||
*/
|
||||
describe('uncaught exception policy', () => {
|
||||
const setup = () => {
|
||||
const fakeProcess = new EventEmitter();
|
||||
let shutdowns = 0;
|
||||
const runtime = createServerStartupRuntime({
|
||||
process: fakeProcess,
|
||||
gracefulShutdown: () => { shutdowns += 1; },
|
||||
getSignalsAttached: () => true,
|
||||
setSignalsAttached: () => {},
|
||||
syncToHmrState: () => {},
|
||||
});
|
||||
runtime.attachProcessHandlers({ attachSignals: false });
|
||||
return { fakeProcess, shutdowns: () => shutdowns };
|
||||
};
|
||||
|
||||
test('a single uncaught exception keeps the server running', () => {
|
||||
const { fakeProcess, shutdowns } = setup();
|
||||
fakeProcess.emit('uncaughtException', new Error('setTypeOfService EINVAL'));
|
||||
expect(shutdowns()).toBe(0);
|
||||
});
|
||||
|
||||
test('a storm of uncaught exceptions still shuts down', () => {
|
||||
const { fakeProcess, shutdowns } = setup();
|
||||
for (let i = 0; i < 11; i += 1) {
|
||||
fakeProcess.emit('uncaughtException', new Error(`stray ${i}`));
|
||||
}
|
||||
expect(shutdowns()).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
test('an unhandled rejection is logged without shutting down', () => {
|
||||
const { fakeProcess, shutdowns } = setup();
|
||||
fakeProcess.emit('unhandledRejection', new Error('late failure'), Promise.resolve());
|
||||
expect(shutdowns()).toBe(0);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user