From 35b58f549e7e6055da2cc23819deededb02c1e98 Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Thu, 7 Nov 2024 13:06:45 -0300 Subject: [PATCH 1/2] fix: engine restart process for Deno used wrong status (#33865) --- .changeset/old-coins-bow.md | 5 ++ .../src/definition/metadata/AppMethod.ts | 2 + .../src/server/compiler/AppCompiler.ts | 2 +- .../src/server/managers/AppRuntimeManager.ts | 9 ++- .../runtime/deno/AppsEngineDenoRuntime.ts | 61 +++++++++++++++---- .../server/runtime/deno/LivenessManager.ts | 12 +++- .../DenoRuntimeSubprocessController.spec.ts | 13 +++- 7 files changed, 85 insertions(+), 19 deletions(-) create mode 100644 .changeset/old-coins-bow.md diff --git a/.changeset/old-coins-bow.md b/.changeset/old-coins-bow.md new file mode 100644 index 000000000000..1790cc205160 --- /dev/null +++ b/.changeset/old-coins-bow.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/apps-engine': patch +--- + +Fixes an issue that would cause apps to appear disabled after a subprocess restart diff --git a/packages/apps-engine/src/definition/metadata/AppMethod.ts b/packages/apps-engine/src/definition/metadata/AppMethod.ts index 71a6d0e914cc..8ec07f53e001 100644 --- a/packages/apps-engine/src/definition/metadata/AppMethod.ts +++ b/packages/apps-engine/src/definition/metadata/AppMethod.ts @@ -99,4 +99,6 @@ export enum AppMethod { EXECUTE_POST_USER_LOGGED_IN = 'executePostUserLoggedIn', EXECUTE_POST_USER_LOGGED_OUT = 'executePostUserLoggedOut', EXECUTE_POST_USER_STATUS_CHANGED = 'executePostUserStatusChanged', + // Runtime specific methods + RUNTIME_RESTART = 'runtime:restart', } diff --git a/packages/apps-engine/src/server/compiler/AppCompiler.ts b/packages/apps-engine/src/server/compiler/AppCompiler.ts index d1f9ddf8a0c0..33f937771ec0 100644 --- a/packages/apps-engine/src/server/compiler/AppCompiler.ts +++ b/packages/apps-engine/src/server/compiler/AppCompiler.ts @@ -21,7 +21,7 @@ export class AppCompiler { throw new Error(`Invalid App package for "${storage.info.name}". Could not find the classFile (${storage.info.classFile}) file.`); } - const runtime = await manager.getRuntime().startRuntimeForApp(packageResult); + const runtime = await manager.getRuntime().startRuntimeForApp(packageResult, storage); const app = new ProxiedApp(manager, storage, runtime); diff --git a/packages/apps-engine/src/server/managers/AppRuntimeManager.ts b/packages/apps-engine/src/server/managers/AppRuntimeManager.ts index 64adf9b0a98d..6c63307a9ba2 100644 --- a/packages/apps-engine/src/server/managers/AppRuntimeManager.ts +++ b/packages/apps-engine/src/server/managers/AppRuntimeManager.ts @@ -1,6 +1,7 @@ import type { AppManager } from '../AppManager'; import type { IParseAppPackageResult } from '../compiler'; import { DenoRuntimeSubprocessController } from '../runtime/deno/AppsEngineDenoRuntime'; +import type { IAppStorageItem } from '../storage'; export type AppRuntimeParams = { appId: string; @@ -21,14 +22,18 @@ export class AppRuntimeManager { constructor(private readonly manager: AppManager) {} - public async startRuntimeForApp(appPackage: IParseAppPackageResult, options = { force: false }): Promise { + public async startRuntimeForApp( + appPackage: IParseAppPackageResult, + storageItem: IAppStorageItem, + options = { force: false }, + ): Promise { const { id: appId } = appPackage.info; if (appId in this.subprocesses && !options.force) { throw new Error('App already has an associated runtime'); } - this.subprocesses[appId] = new DenoRuntimeSubprocessController(this.manager, appPackage); + this.subprocesses[appId] = new DenoRuntimeSubprocessController(this.manager, appPackage, storageItem); await this.subprocesses[appId].setupApp(); diff --git a/packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts b/packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts index 5eb3b7cd0918..86570e21ea59 100644 --- a/packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts +++ b/packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts @@ -9,9 +9,9 @@ import { AppStatus } from '../../../definition/AppStatus'; import type { AppManager } from '../../AppManager'; import type { AppBridges } from '../../bridges'; import type { IParseAppPackageResult } from '../../compiler'; -import type { ILoggerStorageEntry } from '../../logging'; +import { AppConsole, type ILoggerStorageEntry } from '../../logging'; import type { AppAccessorManager, AppApiManager } from '../../managers'; -import type { AppLogStorage } from '../../storage'; +import type { AppLogStorage, IAppStorageItem } from '../../storage'; import { LivenessManager } from './LivenessManager'; import { ProcessMessenger } from './ProcessMessenger'; import { bundleLegacyApp } from './bundler'; @@ -94,7 +94,11 @@ export class DenoRuntimeSubprocessController extends EventEmitter { private readonly livenessManager: LivenessManager; // We need to keep the appSource around in case the Deno process needs to be restarted - constructor(manager: AppManager, private readonly appPackage: IParseAppPackageResult) { + constructor( + manager: AppManager, + private readonly appPackage: IParseAppPackageResult, + private readonly storageItem: IAppStorageItem, + ) { super(); this.debug = baseDebug.extend(appPackage.info.id); @@ -165,28 +169,38 @@ export class DenoRuntimeSubprocessController extends EventEmitter { } } - public async killProcess(): Promise { + /** + * Attempts to kill the process currently controlled by this.deno + * + * @returns boolean - if a process has been killed or not + */ + public async killProcess(): Promise { if (!this.deno) { this.debug('No child process reference'); - return; + return false; } + let { killed } = this.deno; + // This field is not populated if the process is killed by the OS - if (this.deno.killed) { + if (killed) { this.debug('App process was already killed'); - return; + return killed; } // What else should we do? if (this.deno.kill('SIGKILL')) { // Let's wait until we get confirmation the process exited await new Promise((r) => this.deno.on('exit', r)); + killed = true; } else { this.debug('Tried killing the process but failed. Was it already dead?'); + killed = false; } delete this.deno; this.messenger.clearReceiver(); + return killed; } // Debug purposes, could be deleted later @@ -237,19 +251,40 @@ export class DenoRuntimeSubprocessController extends EventEmitter { public async restartApp() { this.debug('Restarting app subprocess'); + const logger = new AppConsole('runtime:restart'); + + logger.info('Starting restart procedure for app subprocess...', this.livenessManager.getRuntimeData()); this.state = 'restarting'; - await this.killProcess(); + try { + const pid = this.deno?.pid; - await this.setupApp(); + const hasKilled = await this.killProcess(); - // setupApp() changes the state to 'ready' - we'll need to workaround that for now - this.state = 'restarting'; + if (hasKilled) { + logger.debug('Process successfully terminated', { pid }); + } else { + logger.warn('Could not terminate process. Maybe it was already dead?', { pid }); + } - await this.sendRequest({ method: 'app:initialize' }); + await this.setupApp(); + logger.info('New subprocess successfully spawned', { pid: this.deno.pid }); - this.state = 'ready'; + // setupApp() changes the state to 'ready' - we'll need to workaround that for now + this.state = 'restarting'; + + await this.sendRequest({ method: 'app:initialize' }); + await this.sendRequest({ method: 'app:setStatus', params: [this.storageItem.status] }); + + this.state = 'ready'; + + logger.info('Successfully restarted app subprocess'); + } catch (e) { + logger.error("Failed to restart app's subprocess", { error: e }); + } finally { + await this.logStorage.storeEntries(AppConsole.toStorageEntry(this.getAppId(), logger)); + } } public getAppId(): string { diff --git a/packages/apps-engine/src/server/runtime/deno/LivenessManager.ts b/packages/apps-engine/src/server/runtime/deno/LivenessManager.ts index 4fb2433e8e0a..dc89acc8718b 100644 --- a/packages/apps-engine/src/server/runtime/deno/LivenessManager.ts +++ b/packages/apps-engine/src/server/runtime/deno/LivenessManager.ts @@ -10,7 +10,7 @@ const defaultOptions: LivenessManager['options'] = { pingRequestTimeout: 10000, pingFrequencyInMS: 10000, consecutiveTimeoutLimit: 4, - maxRestarts: 3, + maxRestarts: Infinity, }; /** @@ -65,6 +65,16 @@ export class LivenessManager { this.options = Object.assign({}, defaultOptions, options); } + public getRuntimeData() { + const { restartCount, pingTimeoutConsecutiveCount, restartLog } = this; + + return { + restartCount, + pingTimeoutConsecutiveCount, + restartLog, + }; + } + public attach(deno: ChildProcess) { this.subprocess = deno; diff --git a/packages/apps-engine/tests/server/runtime/DenoRuntimeSubprocessController.spec.ts b/packages/apps-engine/tests/server/runtime/DenoRuntimeSubprocessController.spec.ts index 0e9c1f7f8786..5c675805e813 100644 --- a/packages/apps-engine/tests/server/runtime/DenoRuntimeSubprocessController.spec.ts +++ b/packages/apps-engine/tests/server/runtime/DenoRuntimeSubprocessController.spec.ts @@ -4,14 +4,16 @@ import * as path from 'path'; import { TestFixture, Setup, Expect, AsyncTest, SpyOn, Any, AsyncSetupFixture, Teardown } from 'alsatian'; import type { SuccessObject } from 'jsonrpc-lite'; +import { AppStatus } from '../../../src/definition/AppStatus'; import { UserStatusConnection, UserType } from '../../../src/definition/users'; import type { AppManager } from '../../../src/server/AppManager'; import type { IParseAppPackageResult } from '../../../src/server/compiler'; import { AppAccessorManager, AppApiManager } from '../../../src/server/managers'; import { DenoRuntimeSubprocessController } from '../../../src/server/runtime/deno/AppsEngineDenoRuntime'; +import type { IAppStorageItem } from '../../../src/server/storage'; import { TestInfastructureSetup } from '../../test-data/utilities'; -@TestFixture('DenoRuntimeSubprocessController') +@TestFixture() export class DenuRuntimeSubprocessControllerTestFixture { private manager: AppManager; @@ -19,6 +21,8 @@ export class DenuRuntimeSubprocessControllerTestFixture { private appPackage: IParseAppPackageResult; + private appStorageItem: IAppStorageItem; + @AsyncSetupFixture public async fixture() { const infrastructure = new TestInfastructureSetup(); @@ -35,11 +39,16 @@ export class DenuRuntimeSubprocessControllerTestFixture { const appPackage = await fs.readFile(path.join(__dirname, '../../test-data/apps/hello-world-test_0.0.1.zip')); this.appPackage = await this.manager.getParser().unpackageApp(appPackage); + + this.appStorageItem = { + id: 'hello-world-test', + status: AppStatus.MANUALLY_ENABLED, + } as IAppStorageItem; } @Setup public setup() { - this.controller = new DenoRuntimeSubprocessController(this.manager, this.appPackage); + this.controller = new DenoRuntimeSubprocessController(this.manager, this.appPackage, this.appStorageItem); this.controller.setupApp(); } From 1bc5902cd1871fc6f7379ec5828dedc424c0116d Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Mon, 9 Dec 2024 13:36:15 -0300 Subject: [PATCH 2/2] Fix lint --- .../src/server/runtime/deno/AppsEngineDenoRuntime.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts b/packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts index 86570e21ea59..c03dd218d139 100644 --- a/packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts +++ b/packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts @@ -94,11 +94,7 @@ export class DenoRuntimeSubprocessController extends EventEmitter { private readonly livenessManager: LivenessManager; // We need to keep the appSource around in case the Deno process needs to be restarted - constructor( - manager: AppManager, - private readonly appPackage: IParseAppPackageResult, - private readonly storageItem: IAppStorageItem, - ) { + constructor(manager: AppManager, private readonly appPackage: IParseAppPackageResult, private readonly storageItem: IAppStorageItem) { super(); this.debug = baseDebug.extend(appPackage.info.id);