From a3063b104d504f9965acd3b37d564dd838109313 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 10 Jul 2025 15:03:45 +0200 Subject: [PATCH 1/4] Stop execution if spawning a child process fails --- web/packages/teleterm/src/main.ts | 29 +++++++++++++------ .../teleterm/src/mainProcess/mainProcess.ts | 29 +++++++++++++------ 2 files changed, 40 insertions(+), 18 deletions(-) diff --git a/web/packages/teleterm/src/main.ts b/web/packages/teleterm/src/main.ts index 5cf10d5f5ebcd..7f178a478c294 100644 --- a/web/packages/teleterm/src/main.ts +++ b/web/packages/teleterm/src/main.ts @@ -23,6 +23,7 @@ import path from 'node:path'; import { app, dialog, globalShortcut, nativeTheme, shell } from 'electron'; import { CUSTOM_PROTOCOL } from 'shared/deepLinks'; +import { ensureError } from 'shared/utils/error'; import { parseDeepLink } from 'teleterm/deepLinks'; import Logger from 'teleterm/logger'; @@ -91,15 +92,25 @@ async function initializeApp(): Promise { app.quit(); }); - // init main process - const mainProcess = MainProcess.create({ - settings, - logger, - configService, - appStateFileStorage, - configFileStorage, - windowsManager, - }); + let mainProcess: MainProcess; + try { + mainProcess = MainProcess.create({ + settings, + logger, + configService, + appStateFileStorage, + configFileStorage, + windowsManager, + }); + } catch (error) { + const message = 'Could not initialize the main process'; + logger.error(message, error); + dialog.showErrorBox(message, ensureError(error).message); + // app.exit(1) isn't equivalent to throwing an error, use an explicit return to stop further + // execution. See https://github.com/gravitational/teleport/issues/56272. + app.exit(1); + return; + } //TODO(gzdunek): Make sure this is not needed after migrating to Vite. app.on( diff --git a/web/packages/teleterm/src/mainProcess/mainProcess.ts b/web/packages/teleterm/src/mainProcess/mainProcess.ts index 582529e012692..d4c78050c3ad7 100644 --- a/web/packages/teleterm/src/mainProcess/mainProcess.ts +++ b/web/packages/teleterm/src/mainProcess/mainProcess.ts @@ -133,6 +133,13 @@ export default class MainProcess { ); } + /** + * create starts necessary child processes such as tsh daemon and the shared process. It also sets + * up IPC handlers and resolves the network addresses under which the child processes set up gRPC + * servers. + * + * create might throw an error if spawning a child process fails, see initTshd for more details. + */ static create(opts: Options) { const instance = new MainProcess(opts); instance.init(); @@ -158,15 +165,10 @@ export default class MainProcess { private init() { this.updateAboutPanelIfNeeded(); this.setAppMenu(); - try { - this.initTshd(); - this.initSharedProcess(); - this.initResolvingChildProcessAddresses(); - this.initIpc(); - } catch (err) { - this.logger.error('Failed to start main process: ', err.message); - app.exit(1); - } + this.initTshd(); + this.initSharedProcess(); + this.initResolvingChildProcessAddresses(); + this.initIpc(); } async initTshdClient(): Promise { @@ -181,6 +183,15 @@ export default class MainProcess { const { binaryPath, homeDir } = this.settings.tshd; this.logger.info(`Starting tsh daemon from ${binaryPath}`); + // spawn might either fail immediately by throwing an error or cause the error event to be emitted + // on the process value returned by spawn. + // + // Some spawn failures result in an error being thrown immediately such as EPERM on Windows [1]. + // This might be related to the fact that in Node.js, an error event causes an error to be + // thrown if there are no listeners for the error event itself [2]. + // + // [1] https://stackoverflow.com/a/42262771 + // [2] https://nodejs.org/docs/latest-v22.x/api/errors.html#error-propagation-and-interception this.tshdProcess = spawn( binaryPath, ['daemon', 'start', ...this.getTshdFlags()], From 48471e24435b92fbcbe83e86b5ff5ed6a27d4a8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Thu, 10 Jul 2025 15:03:45 +0200 Subject: [PATCH 2/4] Align behavior of other callsites that handle main process errors --- web/packages/teleterm/src/main.ts | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/web/packages/teleterm/src/main.ts b/web/packages/teleterm/src/main.ts index 7f178a478c294..4eb2bd946979d 100644 --- a/web/packages/teleterm/src/main.ts +++ b/web/packages/teleterm/src/main.ts @@ -88,8 +88,9 @@ async function initializeApp(): Promise { const windowsManager = new WindowsManager(appStateFileStorage, settings); process.on('uncaughtException', (error, origin) => { - logger.error(origin, error); - app.quit(); + logger.error('Uncaught exception', origin, error); + showDialogWithError(`Uncaught exception (${origin} origin)`, error); + app.exit(1); }); let mainProcess: MainProcess; @@ -178,14 +179,10 @@ async function initializeApp(): Promise { allowList: rootClusterProxyHostAllowList, }); })().catch(error => { - const message = - 'Could not initialize tsh daemon client in the main process'; + const message = 'Could not initialize the tshd client in the main process'; logger.error(message, error); - dialog.showErrorBox( - 'Error during main process startup', - `${message}: ${error}` - ); - app.quit(); + dialog.showErrorBox(message, ensureError(error).message); + app.exit(1); }); app @@ -204,13 +201,10 @@ async function initializeApp(): Promise { windowsManager.createWindow(); }) .catch(error => { - const message = 'Could not initialize the app'; + const message = 'Could not create the main app window'; logger.error(message, error); - dialog.showErrorBox( - 'Error during app initialization', - `${message}: ${error}` - ); - app.quit(); + dialog.showErrorBox(message, ensureError(error).message); + app.exit(1); }); // Limit navigation capabilities to reduce the attack surface. From 94a8bde1695884616e7bed02f522551385a3e80d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Tue, 22 Jul 2025 10:21:24 +0200 Subject: [PATCH 3/4] Attach a stacktrace to the error message in the dialog --- web/packages/teleterm/src/main.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/web/packages/teleterm/src/main.ts b/web/packages/teleterm/src/main.ts index 4eb2bd946979d..4f54c741d1b4b 100644 --- a/web/packages/teleterm/src/main.ts +++ b/web/packages/teleterm/src/main.ts @@ -106,7 +106,7 @@ async function initializeApp(): Promise { } catch (error) { const message = 'Could not initialize the main process'; logger.error(message, error); - dialog.showErrorBox(message, ensureError(error).message); + showDialogWithError(message, error); // app.exit(1) isn't equivalent to throwing an error, use an explicit return to stop further // execution. See https://github.com/gravitational/teleport/issues/56272. app.exit(1); @@ -181,7 +181,7 @@ async function initializeApp(): Promise { })().catch(error => { const message = 'Could not initialize the tshd client in the main process'; logger.error(message, error); - dialog.showErrorBox(message, ensureError(error).message); + showDialogWithError(message, error); app.exit(1); }); @@ -203,7 +203,7 @@ async function initializeApp(): Promise { .catch(error => { const message = 'Could not create the main app window'; logger.error(message, error); - dialog.showErrorBox(message, ensureError(error).message); + showDialogWithError(message, error); app.exit(1); }); @@ -427,3 +427,10 @@ function launchDeepLink( // Otherwise the app would receive focus but nothing would be visible in the UI. windowsManager.launchDeepLink(result); } + +function showDialogWithError(title: string, unknownError: unknown) { + const error = ensureError(unknownError); + // V8 includes the error message in the stack, so there's no need to append stack to message. + const content = error.stack || error.message; + dialog.showErrorBox(title, content); +} From 9550417a1b92e81d3da785cf9d59fedd7726bcae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Cie=C5=9Blak?= Date: Tue, 22 Jul 2025 10:44:12 +0200 Subject: [PATCH 4/4] Prepend the spawnfile to the error from the error event of a process --- .../src/mainProcess/resolveNetworkAddress.ts | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/web/packages/teleterm/src/mainProcess/resolveNetworkAddress.ts b/web/packages/teleterm/src/mainProcess/resolveNetworkAddress.ts index ef039840f3ac4..ab3d62fc07bb0 100644 --- a/web/packages/teleterm/src/mainProcess/resolveNetworkAddress.ts +++ b/web/packages/teleterm/src/mainProcess/resolveNetworkAddress.ts @@ -73,7 +73,7 @@ function waitForMatchInStdout( let chunks = ''; const timeout = setTimeout(() => { - rejectOnError( + rejectWithCleanup( new ResolveError(requestedAddress, process, 'the operation timed out') ); }, timeoutMs); @@ -94,11 +94,23 @@ function waitForMatchInStdout( } }; - const rejectOnError = (error: Error) => { + const rejectWithCleanup = (error: Error) => { reject(error); removeListeners(); }; + const rejectOnError = (error: Error) => { + const errorToReport = error.message.includes(process.spawnfile) + ? error + : // Attach spawnfile so that the process can be identified without looking at the stacktrace. + // resolveNetworkAddress is the only function that ends up surfacing to the UI the error + // event of a process. + new Error(`${process.spawnfile}: ${error.message}`, { + cause: error, + }); + rejectWithCleanup(errorToReport); + }; + const rejectOnClose = (code: number, signal: NodeJS.Signals) => { const codeOrSignal = [ // code can be 0, so we cannot just check it the same way as the signal. @@ -108,7 +120,7 @@ function waitForMatchInStdout( .filter(Boolean) .join(' '); const details = codeOrSignal ? ` with ${codeOrSignal}` : ''; - rejectOnError( + rejectWithCleanup( new ResolveError( requestedAddress, process,