fix(desktop): swallow setTypeOfService EINVAL crashes in dev main (#647) (#1298)

* fix(desktop): swallow harmless setTypeOfService EINVAL crashes in dev main

The packaged Electron entry (apps/packaged/src/logging.ts) already
filters the undici "setTypeOfService EINVAL" crash that issue #895
introduced for the prod build, but the dev / source-built desktop
entry was missing the parallel guard. Result: switching settings
tabs in a from-source desktop run could fire a fresh fetch, undici
would try to set IP_TOS on the outbound socket, the kernel would
refuse on certain macOS / VPN configurations, and the rejection
bubbled to Electron's default handler as the "JavaScript error in
the main process" dialog reported in issue #647.

Add the same defensive filter to apps/desktop:

  - isHarmlessSocketOptionError matches only the canonical undici
    shape (syscall name AND EINVAL code). A contradicting code
    (EACCES, EPERM, etc) explicitly fails the match so real bugs
    don't get hidden.
  - The uncaughtException handler logs harmless cases at warn and
    returns silently. For anything else it removes itself from the
    listener list and re-throws via setImmediate, restoring Node's
    default crash path so Electron's native dialog renders exactly
    as it would without this filter.
  - unhandledRejection mirrors the same harmless / fall-through
    split.

The filter is installed BEFORE app.whenReady so it is armed by the
time the renderer fires its first fetch.

The helper is duplicated rather than imported from apps/packaged
because AGENTS.md forbids cross-app private-source imports. The file
header calls out the parallel and notes that the two copies should
stay in sync until the helper is promoted to a shared workspace
package (follow-up); the contract is identical so a regression in
one will surface in the other's test suite.

Tests in apps/desktop/tests/main/uncaught-exception.test.ts mirror
apps/packaged/tests/logging.test.ts: 8 cases pinning the matcher
shape, 2 cases pinning the handler's harmless-log-warn vs
fall-through-rethrow split.

Validated: pnpm guard, pnpm --filter @open-design/desktop typecheck,
pnpm --filter @open-design/desktop build, and pnpm --filter
@open-design/desktop test (14 passed, 10 new).

* fix(desktop,packaged): fail-fast on non-harmless unhandled rejections

The previous unhandledRejection listeners logged non-harmless reasons
and returned, which kept the main process alive after any rejected
promise. A real bug, a failed IPC registration, or any unexpected
async exception was reduced to a console line instead of surfacing
through Node/Electron's default crash path the filter was meant to
preserve.

Both copies now route non-harmless rejections through a parallel
factory (createDesktopUnhandledRejectionHandler /
createFatalUnhandledRejectionHandler) that mirrors the
uncaughtException policy: harmless setTypeOfService EINVAL shapes log
at warn and return, anything else logs at error, removes the
listener, and re-throws via setImmediate. Listener removal happens
before the scheduled throw, so the rethrown reason lands in the
uncaughtException path with no recursion.

Tests cover the harmless branch, the detach + ordered rethrow, and
non-Error / primitive rejection reasons (Promise.reject(42)) which
must fall through. Desktop suite: 13/13, packaged suite: 16/16.
Flagged on PR #1298 by Siri-Ray and the codex P2 review thread; the
two file copies stay in lockstep per the AGENTS.md sync invariant.

---------

Co-authored-by: Nagendhra <nagendhra405@gmail.com>
This commit is contained in:
Nagendhra Madishetti 2026-05-12 04:50:57 -04:00 committed by GitHub
parent 54516a9866
commit 4d0ea247a7
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 475 additions and 7 deletions

View file

@ -29,6 +29,7 @@ import {
import { readProcessStamp } from "@open-design/platform";
import { createDesktopRuntime } from "./runtime.js";
import { attachDesktopProcessErrorFilter } from "./uncaught-exception.js";
// Re-export pure URL-policy helpers so the packaged workspace's
// vitest can pin their behaviour without spinning up a full Electron
@ -171,6 +172,15 @@ export async function runDesktopMain(
runtime: SidecarRuntimeContext<SidecarStamp>,
options: DesktopMainOptions = {},
): Promise<void> {
// Install the defensive uncaughtException filter BEFORE awaiting
// app.whenReady, so a setTypeOfService EINVAL thrown by undici during
// the renderer's first fetch is intercepted rather than surfacing as
// Electron's "JavaScript error in main process" dialog (issue #647).
// The packaged entry has the parallel filter wired in
// apps/packaged/src/logging.ts; both must stay in sync until the
// helper is promoted to a shared workspace package.
attachDesktopProcessErrorFilter();
await app.whenReady();
// PR #974: mint a per-process auth secret and hand it to the daemon

View file

@ -0,0 +1,155 @@
/**
* Defensive uncaught-exception filter for the dev / source-built desktop
* Electron main process.
*
* The packaged Electron entry (`apps/packaged/src/logging.ts`) already
* carries the same filter, where it was added for issue #895 (the
* `setTypeOfService EINVAL` crash from undici socket internals on certain
* macOS / VPN configurations). The same crash reproduces on the dev
* entry when users switch settings tabs because the renderer fires a
* fresh `fetch` and undici tries to set the IP_TOS byte on the outbound
* socket; on a kernel that refuses, the rejection bubbles to Electron's
* default handler and surfaces as the "JavaScript error in main process"
* dialog reported in issue #647.
*
* Why this file isn't a direct import from `apps/packaged`: AGENTS.md
* forbids one app package from importing another app's private `src/`.
* The honest fix is to promote the helper to a shared workspace package
* (e.g. `@open-design/platform`); doing that as part of this bug-fix
* sprint would balloon the PR. Until that promotion lands, this module
* is the source of truth for the desktop entry's filter and the two
* copies should stay in sync. Any change to the matching rules here
* should land in `apps/packaged/src/logging.ts` in the same PR.
*
* Behaviour contract (mirrors the packaged version):
*
* 1. `isHarmlessSocketOptionError(value)` recognises the canonical
* undici `setTypeOfService EINVAL` shape and nothing else. A
* contradicting `code` (e.g. `EACCES`) explicitly fails the match
* so real bugs don't get hidden.
*
* 2. The installed `uncaughtException` handler logs harmless errors
* and returns silently. For anything else it removes itself from
* the listener list and re-throws via `setImmediate`, restoring
* Node's default crash path so Electron's native error dialog
* renders exactly as it would without this filter.
*
* 3. The installed `unhandledRejection` handler mirrors that policy:
* harmless `setTypeOfService EINVAL` rejections log at warn and
* return; every other rejection logs at error, removes the
* listener, and schedules a re-throw via `setImmediate`. Without
* the detach + rethrow a non-harmless rejection would land in
* this log line and silently keep the process alive, which would
* hide real main-process bugs (issue #647 review by Siri-Ray and
* the codex P2 thread on PR #1298).
*/
/**
* Recognise the known-harmless `setTypeOfService EINVAL` shape thrown by
* undici socket internals when the kernel refuses to set the IP_TOS byte
* on an outbound socket. Exported so a unit test can pin the exact
* surface this filter matches.
*/
export function isHarmlessSocketOptionError(value: unknown): boolean {
if (!(value instanceof Error)) return false;
const message = typeof value.message === 'string' ? value.message : '';
if (!message) return false;
if (!message.includes('setTypeOfService')) return false;
const code = (value as NodeJS.ErrnoException).code;
if (typeof code === 'string' && code.length > 0) {
// Structured code is authoritative when present. Only EINVAL
// qualifies; anything else (EACCES, EPERM, ECONNRESET, …) is a
// contradicting signal and the filter rejects the match so a real
// bug carrying a stale `setTypeOfService` substring in its message
// doesn't slip through.
return code === 'EINVAL';
}
// No structured code: fall back to message-based detection. libuv's
// error string is `<syscall> <errcode>` so the EINVAL token sits
// alongside the syscall name.
return message.includes('EINVAL');
}
/** Minimal logger surface the desktop entry already has by way of
* `console.*`. Exposed as an injectable parameter so tests can capture
* log calls without mocking the console. */
export interface DesktopErrorFilterLogger {
warn: (message: string, meta?: Record<string, unknown>) => void;
error: (message: string, meta?: Record<string, unknown>) => void;
}
/** Build the named handler so a unit test can drive it without
* installing it on the live `process` object. */
export function createDesktopUncaughtExceptionHandler(
logger: DesktopErrorFilterLogger,
): (error: unknown) => void {
const handler = (error: unknown): void => {
if (isHarmlessSocketOptionError(error)) {
logger.warn('desktop main swallowed harmless socket option error', { error });
return;
}
logger.error('desktop main fatal uncaught exception', { error });
process.removeListener('uncaughtException', handler);
setImmediate(() => {
throw error;
});
};
return handler;
}
/**
* Parallel factory for the `unhandledRejection` listener. Harmless
* undici `setTypeOfService EINVAL` rejections log at warn and return;
* anything else logs at error, removes the listener, and schedules a
* re-throw via `setImmediate`. The detached re-throw lands in the
* `uncaughtException` listener installed above, which then walks its
* own non-harmless path (log + detach + rethrow), so Node's default
* crash semantics take over and Electron's native error dialog
* renders as it would without this filter. Pinned by a unit test so a
* future refactor that drops the detach can't silently regress.
*/
export function createDesktopUnhandledRejectionHandler(
logger: DesktopErrorFilterLogger,
): (reason: unknown) => void {
const handler = (reason: unknown): void => {
if (isHarmlessSocketOptionError(reason)) {
logger.warn('desktop main swallowed harmless socket option rejection', { reason });
return;
}
logger.error('desktop main unhandled rejection', { reason });
process.removeListener('unhandledRejection', handler);
setImmediate(() => {
throw reason;
});
};
return handler;
}
/** Install the filter on `process`. Idempotent across the dev entry's
* hot-reload paths because Node's listener registry deduplicates
* identical function references; we capture the once-built handler
* inside this module so repeated calls don't pile up. */
let installedHandler: ((error: unknown) => void) | null = null;
export function attachDesktopProcessErrorFilter(
logger: DesktopErrorFilterLogger = consoleLogger(),
): void {
if (installedHandler !== null) return;
const handler = createDesktopUncaughtExceptionHandler(logger);
installedHandler = handler;
process.on('uncaughtException', handler);
process.on('unhandledRejection', createDesktopUnhandledRejectionHandler(logger));
}
function consoleLogger(): DesktopErrorFilterLogger {
return {
warn: (message, meta) => {
if (meta === undefined) console.warn(message);
else console.warn(message, meta);
},
error: (message, meta) => {
if (meta === undefined) console.error(message);
else console.error(message, meta);
},
};
}

View file

@ -0,0 +1,194 @@
/**
* Regression coverage for the desktop main entry's harmless-socket-option
* filter, added to fix issue #647. The packaged entry has the parallel
* filter pinned in `apps/packaged/tests/logging.test.ts`; this suite
* mirrors it so the two copies of the matcher stay in lockstep until the
* helper is promoted to a shared workspace package.
*
* Match strategy is intentionally narrow name the syscall AND verify
* the EINVAL code so a future regression that broadens the filter to
* "every EINVAL" (which would silently swallow real bugs) trips a test.
*
* @see https://github.com/nexu-io/open-design/issues/647
* @see https://github.com/nexu-io/open-design/issues/895
*/
import { afterEach, describe, expect, it, vi } from 'vitest';
import {
createDesktopUncaughtExceptionHandler,
createDesktopUnhandledRejectionHandler,
isHarmlessSocketOptionError,
type DesktopErrorFilterLogger,
} from '../../src/main/uncaught-exception.js';
function stubLogger(): DesktopErrorFilterLogger {
return {
warn: vi.fn(),
error: vi.fn(),
};
}
afterEach(() => {
vi.restoreAllMocks();
});
describe('isHarmlessSocketOptionError (desktop main, issue #647)', () => {
it('matches the canonical undici setTypeOfService EINVAL shape', () => {
const error = new Error('setTypeOfService EINVAL') as NodeJS.ErrnoException;
error.code = 'EINVAL';
error.syscall = 'setTypeOfService';
expect(isHarmlessSocketOptionError(error)).toBe(true);
});
it('matches when only the message has both tokens (no code property set)', () => {
const error = new Error('setTypeOfService EINVAL');
expect(isHarmlessSocketOptionError(error)).toBe(true);
});
it('matches when code is EINVAL and message references setTypeOfService', () => {
const error = new Error('Error: setTypeOfService failed') as NodeJS.ErrnoException;
error.code = 'EINVAL';
expect(isHarmlessSocketOptionError(error)).toBe(true);
});
it('does NOT match a generic EINVAL', () => {
// Guard against a future refactor that broadens the filter to
// "every EINVAL" and silently swallows configuration errors.
const error = new Error('write EINVAL') as NodeJS.ErrnoException;
error.code = 'EINVAL';
expect(isHarmlessSocketOptionError(error)).toBe(false);
});
it('does NOT match a setTypeOfService error with a different errno (EACCES)', () => {
const error = new Error('setTypeOfService EACCES') as NodeJS.ErrnoException;
error.code = 'EACCES';
expect(isHarmlessSocketOptionError(error)).toBe(false);
});
it('treats the structured code as authoritative when it contradicts the message', () => {
// A stale `setTypeOfService EINVAL` substring in a message paired
// with a real EACCES code must NOT swallow the underlying
// permission failure.
const error = new Error('setTypeOfService EINVAL') as NodeJS.ErrnoException;
error.code = 'EACCES';
expect(isHarmlessSocketOptionError(error)).toBe(false);
});
it('rejects non-Error values', () => {
expect(isHarmlessSocketOptionError('setTypeOfService EINVAL')).toBe(false);
expect(isHarmlessSocketOptionError(null)).toBe(false);
expect(isHarmlessSocketOptionError(undefined)).toBe(false);
expect(isHarmlessSocketOptionError({ message: 'setTypeOfService EINVAL', code: 'EINVAL' })).toBe(false);
});
it('rejects an Error with a non-string message', () => {
const error = new Error();
(error as { message?: unknown }).message = 42;
expect(isHarmlessSocketOptionError(error)).toBe(false);
});
});
describe('createDesktopUncaughtExceptionHandler (desktop main, issue #647)', () => {
it('logs at warn level and returns silently for the harmless shape', () => {
const logger = stubLogger();
const handler = createDesktopUncaughtExceptionHandler(logger);
const error = new Error('setTypeOfService EINVAL') as NodeJS.ErrnoException;
error.code = 'EINVAL';
const removeListener = vi.spyOn(process, 'removeListener');
const setImmediateSpy = vi.spyOn(global, 'setImmediate');
handler(error);
expect(logger.warn).toHaveBeenCalledTimes(1);
expect(logger.error).not.toHaveBeenCalled();
expect(removeListener).not.toHaveBeenCalled();
expect(setImmediateSpy).not.toHaveBeenCalled();
});
it('logs at error level, detaches itself, and re-throws for any other error', () => {
const logger = stubLogger();
const handler = createDesktopUncaughtExceptionHandler(logger);
const error = new Error('boom');
const removeListener = vi.spyOn(process, 'removeListener').mockReturnValue(process);
// Capture the deferred throw without letting it escape into the
// test runner.
const setImmediateSpy = vi
.spyOn(global, 'setImmediate')
.mockImplementation(((cb: () => void) => {
// Capture the callback for assertion but don't invoke it
// synchronously, the real semantics rely on the call
// happening AFTER the listener has been removed.
return cb as unknown as NodeJS.Immediate;
}) as unknown as typeof setImmediate);
handler(error);
expect(logger.error).toHaveBeenCalledTimes(1);
expect(logger.warn).not.toHaveBeenCalled();
// Listener removal MUST happen before the deferred throw so the
// crash falls through to Node's default handler rather than
// re-entering this filter.
expect(removeListener).toHaveBeenCalledWith('uncaughtException', handler);
expect(setImmediateSpy).toHaveBeenCalledTimes(1);
});
});
describe('createDesktopUnhandledRejectionHandler (desktop main, issue #647 review follow-up)', () => {
it('logs at warn level and returns silently for a harmless rejection', () => {
const logger = stubLogger();
const handler = createDesktopUnhandledRejectionHandler(logger);
const reason = new Error('setTypeOfService EINVAL') as NodeJS.ErrnoException;
reason.code = 'EINVAL';
const removeListener = vi.spyOn(process, 'removeListener');
const setImmediateSpy = vi.spyOn(global, 'setImmediate');
handler(reason);
expect(logger.warn).toHaveBeenCalledTimes(1);
expect(logger.error).not.toHaveBeenCalled();
expect(removeListener).not.toHaveBeenCalled();
expect(setImmediateSpy).not.toHaveBeenCalled();
});
it('logs at error level, detaches itself, and re-throws for a non-harmless rejection', () => {
// Regression guard: a previous revision of this file logged
// non-harmless rejections and returned, which kept the dev main
// process alive after arbitrary rejected promises and hid real
// bugs from Node/Electron's default fail-fast path. The
// unhandledRejection handler must mirror the uncaughtException
// fall-through so only the EINVAL shape is swallowed.
const logger = stubLogger();
const handler = createDesktopUnhandledRejectionHandler(logger);
const reason = new Error('real ipc registration failure');
const removeListener = vi.spyOn(process, 'removeListener').mockReturnValue(process);
const setImmediateSpy = vi
.spyOn(global, 'setImmediate')
.mockImplementation(((cb: () => void) => cb as unknown as NodeJS.Immediate) as unknown as typeof setImmediate);
handler(reason);
expect(logger.error).toHaveBeenCalledTimes(1);
expect(logger.warn).not.toHaveBeenCalled();
expect(removeListener).toHaveBeenCalledWith('unhandledRejection', handler);
expect(setImmediateSpy).toHaveBeenCalledTimes(1);
});
it('treats non-Error rejection reasons as non-harmless and falls through', () => {
// Primitive rejections (e.g. `Promise.reject(42)`) are never the
// undici socket shape, so the handler must take the fail-fast
// path rather than silently log them away.
const logger = stubLogger();
const handler = createDesktopUnhandledRejectionHandler(logger);
const removeListener = vi.spyOn(process, 'removeListener').mockReturnValue(process);
const setImmediateSpy = vi
.spyOn(global, 'setImmediate')
.mockImplementation(((cb: () => void) => cb as unknown as NodeJS.Immediate) as unknown as typeof setImmediate);
handler(42);
expect(logger.error).toHaveBeenCalledTimes(1);
expect(removeListener).toHaveBeenCalledWith('unhandledRejection', handler);
expect(setImmediateSpy).toHaveBeenCalledTimes(1);
});
});

View file

@ -114,6 +114,38 @@ export function createFatalUncaughtExceptionHandler(
return handler;
}
/**
* Parallel factory for the `unhandledRejection` listener installed by
* `attachPackagedDesktopProcessLogging`. The harmless / fall-through
* split must match `createFatalUncaughtExceptionHandler`: harmless
* `setTypeOfService EINVAL` rejections log at warn and return,
* anything else logs at error, removes the listener, and re-throws
* via `setImmediate`.
*
* Without the detach + rethrow a non-harmless rejection would land in
* this log line and silently keep the process alive, which would hide
* real main-process bugs from Node/Electron's default fail-fast path
* (the exact regression Siri-Ray and the codex P2 thread flagged on
* the parallel desktop filter in PR #1298). The same matcher feeds
* both factories, so the apps/desktop sibling stays in lockstep.
*/
export function createFatalUnhandledRejectionHandler(
logger: PackagedDesktopLogger,
): (reason: unknown) => void {
const handler = (reason: unknown): void => {
if (isHarmlessSocketOptionError(reason)) {
logger.warn("packaged desktop swallowed harmless socket option rejection", { reason });
return;
}
logger.error("packaged desktop unhandled rejection", { reason });
process.removeListener("unhandledRejection", handler);
setImmediate(() => {
throw reason;
});
};
return handler;
}
function normalizeMeta(meta: Record<string, unknown> | undefined): Record<string, unknown> | undefined {
if (meta == null) return undefined;
return Object.fromEntries(
@ -230,13 +262,12 @@ export function attachPackagedDesktopProcessLogging(options: {
// existed. See `createFatalUncaughtExceptionHandler` for the
// unit-tested factory.
process.on("uncaughtException", createFatalUncaughtExceptionHandler(logger));
process.on("unhandledRejection", (reason) => {
if (isHarmlessSocketOptionError(reason)) {
logger.warn("packaged desktop swallowed harmless socket option rejection", { reason });
return;
}
logger.error("packaged desktop unhandled rejection", { reason });
});
// The unhandledRejection handler must mirror the uncaughtException
// policy: harmless EINVAL shapes are swallowed, every other reason
// takes the fail-fast path so a real main-process bug surfaces
// through Node/Electron's default crash semantics instead of being
// hidden as a log line. See `createFatalUnhandledRejectionHandler`.
process.on("unhandledRejection", createFatalUnhandledRejectionHandler(logger));
process.on("beforeExit", (code) => {
logger.warn("packaged desktop beforeExit", { code });
});

View file

@ -18,6 +18,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest';
import {
createFatalUncaughtExceptionHandler,
createFatalUnhandledRejectionHandler,
isHarmlessSocketOptionError,
type PackagedDesktopLogger,
} from '../src/logging.js';
@ -193,3 +194,80 @@ describe('createFatalUncaughtExceptionHandler (issue #906)', () => {
expect(logger.error).toHaveBeenCalledTimes(2);
});
});
// The parallel `unhandledRejection` listener mirrors the
// uncaughtException policy: harmless EINVAL rejections log at warn
// and return, anything else logs at error, detaches the listener, and
// schedules a re-throw via setImmediate so Node/Electron's default
// fail-fast path takes over. Before this factory landed, the inline
// listener logged non-harmless rejections and returned, which silently
// kept the main process alive after any rejected promise. Siri-Ray
// and the codex P2 thread on PR #1298 flagged the same gap on the
// parallel apps/desktop filter, so the two copies stay in lockstep.
describe('createFatalUnhandledRejectionHandler (issue #647 review follow-up)', () => {
it('logs harmless socket option rejections at warn level and returns silently', () => {
const logger = stubLogger();
const handler = createFatalUnhandledRejectionHandler(logger);
const harmless = new Error('setTypeOfService EINVAL') as NodeJS.ErrnoException;
harmless.code = 'EINVAL';
const removeListenerSpy = vi.spyOn(process, 'removeListener');
const setImmediateSpy = vi.spyOn(globalThis, 'setImmediate');
handler(harmless);
expect(logger.warn).toHaveBeenCalledTimes(1);
expect(logger.error).not.toHaveBeenCalled();
expect(removeListenerSpy).not.toHaveBeenCalled();
expect(setImmediateSpy).not.toHaveBeenCalled();
});
it('removes itself from unhandledRejection listeners before scheduling the rethrow', () => {
const logger = stubLogger();
const handler = createFatalUnhandledRejectionHandler(logger);
const removeListenerSpy = vi.spyOn(process, 'removeListener');
const scheduled: Array<() => void> = [];
const setImmediateSpy = vi
.spyOn(globalThis, 'setImmediate')
.mockImplementation(((fn: () => void) => {
scheduled.push(fn);
return 0 as unknown as NodeJS.Immediate;
}) as typeof setImmediate);
const realBug = new Error('failed ipc registration');
handler(realBug);
expect(logger.error).toHaveBeenCalledTimes(1);
expect(removeListenerSpy).toHaveBeenCalledWith('unhandledRejection', handler);
const removeOrder = removeListenerSpy.mock.invocationCallOrder[0]!;
const setImmediateOrder = setImmediateSpy.mock.invocationCallOrder[0]!;
expect(removeOrder).toBeLessThan(setImmediateOrder);
expect(scheduled).toHaveLength(1);
expect(() => scheduled[0]!()).toThrow(realBug);
});
it('falls through for primitive rejection reasons (Promise.reject(42))', () => {
// A primitive reason is never the undici socket shape, so the
// handler must reach the fail-fast path. Without this guard a
// `Promise.reject('boom')` would silently log and disappear.
const logger = stubLogger();
const handler = createFatalUnhandledRejectionHandler(logger);
const removeListenerSpy = vi.spyOn(process, 'removeListener');
const scheduled: Array<() => void> = [];
vi
.spyOn(globalThis, 'setImmediate')
.mockImplementation(((fn: () => void) => {
scheduled.push(fn);
return 0 as unknown as NodeJS.Immediate;
}) as typeof setImmediate);
handler(42);
expect(logger.error).toHaveBeenCalledTimes(1);
expect(removeListenerSpy).toHaveBeenCalledWith('unhandledRejection', handler);
expect(scheduled).toHaveLength(1);
});
});