fix: unrecoverable state if app fails to construct runtime

.changeset/unlucky-poets-decide.md

@@ -0,0 +1,6 @@
+---
+'@rocket.chat/apps-engine': patch
+'@rocket.chat/meteor': patch
+---
+
+Fixes an issue that would leave an app in an unrecoverable state if the installation failed during the construction of the runtime

packages/apps-engine/src/server/AppManager.ts

@@ -588,9 +588,15 @@ export class AppManager {
 			return aff;
 		}
 
-		// Now that is has all been compiled, let's get the
-		// the App instance from the source.
-		const app = await this.getCompiler().toSandBox(this, descriptor, result);
+		let app: ProxiedApp;
+
+		try {
+			app = await this.getCompiler().toSandBox(this, descriptor, result);
+		} catch (error) {
+			await Promise.all(undoSteps.map((undoer) => undoer()));
+
+			throw error;
+		}
 
 		undoSteps.push(() =>
 			this.getRuntime()

packages/apps-engine/src/server/managers/AppRuntimeManager.ts

@@ -17,10 +17,16 @@ export type ExecRequestOptions = {
 	timeout?: number;
 };
 
+const defaultRuntimeFactory = (manager: AppManager, appPackage: IParseAppPackageResult, storageItem: IAppStorageItem) =>
+	new DenoRuntimeSubprocessController(manager, appPackage, storageItem);
+
 export class AppRuntimeManager {
 	private readonly subprocesses: Record<string, DenoRuntimeSubprocessController> = {};
 
-	constructor(private readonly manager: AppManager) {}
+	constructor(
+		private readonly manager: AppManager,
+		private readonly runtimeFactory = defaultRuntimeFactory,
+	) {}
 
 	public async startRuntimeForApp(
 		appPackage: IParseAppPackageResult,
@@ -33,9 +39,16 @@ export class AppRuntimeManager {
 			throw new Error('App already has an associated runtime');
 		}
 
-		this.subprocesses[appId] = new DenoRuntimeSubprocessController(this.manager, appPackage, storageItem);
+		this.subprocesses[appId] = this.runtimeFactory(this.manager, appPackage, storageItem);
 
-		await this.subprocesses[appId].setupApp();
+		try {
+			await this.subprocesses[appId].setupApp();
+		} catch (error) {
+			const subprocess = this.subprocesses[appId];
+			delete this.subprocesses[appId];
+			await subprocess.stopApp();
+			throw error;
+		}
 
 		return this.subprocesses[appId];
 	}

packages/apps-engine/tests/server/managers/AppRuntimeManager.spec.ts

@@ -0,0 +1,130 @@
+import { AsyncTest, Expect, Setup, SetupFixture, SpyOn } from 'alsatian';
+
+import { AppStatus } from '../../../src/definition/AppStatus';
+import type { AppManager } from '../../../src/server/AppManager';
+import type { IParseAppPackageResult } from '../../../src/server/compiler';
+import { AppRuntimeManager } from '../../../src/server/managers/AppRuntimeManager';
+import type { DenoRuntimeSubprocessController } from '../../../src/server/runtime/deno/AppsEngineDenoRuntime';
+import type { IAppStorageItem } from '../../../src/server/storage';
+
+export class AppRuntimeManagerTestFixture {
+	private mockManager: AppManager;
+
+	private runtimeManager: AppRuntimeManager;
+
+	private mockAppPackage: IParseAppPackageResult;
+
+	private mockStorageItem: IAppStorageItem;
+
+	private mockSubprocessController: DenoRuntimeSubprocessController;
+
+	@SetupFixture
+	public setupFixture() {
+		this.mockManager = {
+			getAccessorManager() {
+				return {} as any;
+			},
+			getApiManager() {
+				return {} as any;
+			},
+			getLogStorage() {
+				return {} as any;
+			},
+			getBridges() {
+				return {} as any;
+			},
+		} as AppManager;
+
+		this.mockAppPackage = {
+			info: {
+				id: 'test-app',
+				name: 'Test App',
+				nameSlug: 'test-app',
+				version: '1.0.0',
+				description: 'Test app for unit testing',
+				author: {
+					name: 'Test Author',
+					homepage: 'https://test.com',
+					support: 'https://test.com/support',
+				},
+				permissions: [],
+				requiredApiVersion: '1.0.0',
+				classFile: 'main.js',
+				iconFile: 'icon.png',
+				implements: [],
+			},
+			files: {
+				'main.js': 'console.log("Hello World");',
+			},
+			languageContent: {} as unknown as IParseAppPackageResult['languageContent'],
+			implemented: {} as unknown as IParseAppPackageResult['implemented'],
+		} as IParseAppPackageResult;
+
+		this.mockStorageItem = {
+			id: 'test-app',
+			status: AppStatus.MANUALLY_ENABLED,
+			info: this.mockAppPackage.info,
+			createdAt: new Date(),
+			updatedAt: new Date(),
+		} as IAppStorageItem;
+
+		this.mockSubprocessController = {
+			async setupApp() {
+				return Promise.resolve();
+			},
+
+			async stopApp() {
+				return Promise.resolve();
+			},
+
+			getAppId() {
+				return 'test-app';
+			},
+		} as DenoRuntimeSubprocessController;
+	}
+
+	@Setup
+	public setup() {
+		this.runtimeManager = new AppRuntimeManager(this.mockManager, () => this.mockSubprocessController);
+	}
+
+	@AsyncTest('Starts runtime for app successfully')
+	public async startRuntimeForAppSuccessfully() {
+		await Expect(() => this.runtimeManager.startRuntimeForApp(this.mockAppPackage, this.mockStorageItem)).not.toThrowAsync();
+
+		/* eslint-disable-next-line dot-notation -- We need to access the property like this for the compile not to complain */
+		Expect(this.runtimeManager['subprocesses'][this.mockAppPackage.info.id]).toBe(this.mockSubprocessController);
+	}
+
+	@AsyncTest('Fails to start runtime for app that already has a runtime')
+	public async startMultipleRuntimesForSameApp() {
+		await Expect(() => this.runtimeManager.startRuntimeForApp(this.mockAppPackage, this.mockStorageItem)).not.toThrowAsync();
+
+		await Expect(() => this.runtimeManager.startRuntimeForApp(this.mockAppPackage, this.mockStorageItem)).toThrowErrorAsync(
+			Error,
+			'App already has an associated runtime',
+		);
+	}
+
+	@AsyncTest('Starts multiple runtimes for app successfully with force option')
+	public async startMultipleRuntimesForSameAppWithForceOption() {
+		await Expect(() => this.runtimeManager.startRuntimeForApp(this.mockAppPackage, this.mockStorageItem)).not.toThrowAsync();
+
+		await Expect(() =>
+			this.runtimeManager.startRuntimeForApp(this.mockAppPackage, this.mockStorageItem, { force: true }),
+		).not.toThrowAsync();
+
+		/* eslint-disable-next-line dot-notation -- We need to access the property like this for the compile not to complain */
+		Expect(this.runtimeManager['subprocesses'][this.mockAppPackage.info.id]).toBe(this.mockSubprocessController);
+	}
+
+	@AsyncTest()
+	public async startRuntimeThatFailsToSetup() {
+		SpyOn(this.mockSubprocessController, 'setupApp').andReturn(Promise.reject(new Error('Nope')));
+
+		await Expect(() => this.runtimeManager.startRuntimeForApp(this.mockAppPackage, this.mockStorageItem)).toThrowErrorAsync(Error, 'Nope');
+
+		/* eslint-disable-next-line dot-notation -- We need to access the property like this for the compile not to complain */
+		Expect(this.runtimeManager['subprocesses'][this.mockAppPackage.info.id]).not.toBeDefined();
+	}
+}

packages/apps-engine/tests/tsconfig.json

@@ -0,0 +1,8 @@
+{
+    "extends": "../tsconfig.json",
+    "compilerOptions": {
+        "useDefineForClassFields": false,
+        "rootDir": "../"
+    },
+    "include": ["./**/*"]
+}