diff --git a/.changeset/unlucky-poets-decide.md b/.changeset/unlucky-poets-decide.md new file mode 100644 index 0000000000000..a882edd31314c --- /dev/null +++ b/.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 diff --git a/packages/apps-engine/src/server/AppManager.ts b/packages/apps-engine/src/server/AppManager.ts index eb8a9ee188025..bc6c73c142685 100644 --- a/packages/apps-engine/src/server/AppManager.ts +++ b/packages/apps-engine/src/server/AppManager.ts @@ -580,9 +580,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() diff --git a/packages/apps-engine/src/server/managers/AppRuntimeManager.ts b/packages/apps-engine/src/server/managers/AppRuntimeManager.ts index 1dd7c89fa544d..89dd017a090f5 100644 --- a/packages/apps-engine/src/server/managers/AppRuntimeManager.ts +++ b/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 = {}; - 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]; } diff --git a/packages/apps-engine/tests/server/managers/AppRuntimeManager.spec.ts b/packages/apps-engine/tests/server/managers/AppRuntimeManager.spec.ts new file mode 100644 index 0000000000000..0e8c74951bcdd --- /dev/null +++ b/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(); + } +} diff --git a/packages/apps-engine/tests/tsconfig.json b/packages/apps-engine/tests/tsconfig.json new file mode 100644 index 0000000000000..b1bdb388b72a9 --- /dev/null +++ b/packages/apps-engine/tests/tsconfig.json @@ -0,0 +1,8 @@ +{ + "extends": "../tsconfig.json", + "compilerOptions": { + "useDefineForClassFields": false, + "rootDir": "../" + }, + "include": ["./**/*"] +}