fix: engine restart process for Deno used wrong status (#33865)

.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

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',
 }

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);
 

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<DenoRuntimeSubprocessController> {
+    public async startRuntimeForApp(
+        appPackage: IParseAppPackageResult,
+        storageItem: IAppStorageItem,
+        options = { force: false },
+    ): Promise<DenoRuntimeSubprocessController> {
         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();
 

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';
@@ -109,6 +109,7 @@ export class DenoRuntimeSubprocessController extends EventEmitter {
     constructor(
         manager: AppManager,
         private readonly appPackage: IParseAppPackageResult,
+        private readonly storageItem: IAppStorageItem,
     ) {
         super();
 
@@ -177,28 +178,38 @@ export class DenoRuntimeSubprocessController extends EventEmitter {
         }
     }
 
-    public async killProcess(): Promise<void> {
+    /**
+     * Attempts to kill the process currently controlled by this.deno
+     *
+     * @returns boolean - if a process has been killed or not
+     */
+    public async killProcess(): Promise<boolean> {
         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<void>((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
@@ -249,19 +260,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 {

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;
 

packages/apps-engine/tests/server/runtime/DenoRuntimeSubprocessController.spec.ts

@@ -4,21 +4,25 @@ 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;
 
     private controller: DenoRuntimeSubprocessController;
 
     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();
     }