Skip to content

Commit 3c914ca

Browse files
d-gubertd-gubert
authored
fix: unrecoverable state if app fails to construct runtime (#36504) (#36543)
1 parent 0379096 commit 3c914ca

File tree

5 files changed

+169
-6
lines changed

5 files changed

+169
-6
lines changed

‎.changeset/unlucky-poets-decide.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@rocket.chat/apps-engine': patch
3+
'@rocket.chat/meteor': patch
4+
---
5+
6+
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‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -580,9 +580,15 @@ export class AppManager {
580580
return aff;
581581
}
582582

583-
// Now that is has all been compiled, let's get the
584-
// the App instance from the source.
585-
const app = await this.getCompiler().toSandBox(this, descriptor, result);
583+
let app: ProxiedApp;
584+
585+
try {
586+
app = await this.getCompiler().toSandBox(this, descriptor, result);
587+
} catch (error) {
588+
await Promise.all(undoSteps.map((undoer) => undoer()));
589+
590+
throw error;
591+
}
586592

587593
undoSteps.push(() =>
588594
this.getRuntime()

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

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,16 @@ export type ExecRequestOptions = {
1717
timeout?: number;
1818
};
1919

20+
const defaultRuntimeFactory = (manager: AppManager, appPackage: IParseAppPackageResult, storageItem: IAppStorageItem) =>
21+
new DenoRuntimeSubprocessController(manager, appPackage, storageItem);
22+
2023
export class AppRuntimeManager {
2124
private readonly subprocesses: Record<string, DenoRuntimeSubprocessController> = {};
2225

23-
constructor(private readonly manager: AppManager) {}
26+
constructor(
27+
private readonly manager: AppManager,
28+
private readonly runtimeFactory = defaultRuntimeFactory,
29+
) {}
2430

2531
public async startRuntimeForApp(
2632
appPackage: IParseAppPackageResult,
@@ -33,9 +39,16 @@ export class AppRuntimeManager {
3339
throw new Error('App already has an associated runtime');
3440
}
3541

36-
this.subprocesses[appId] = new DenoRuntimeSubprocessController(this.manager, appPackage, storageItem);
42+
this.subprocesses[appId] = this.runtimeFactory(this.manager, appPackage, storageItem);
3743

38-
await this.subprocesses[appId].setupApp();
44+
try {
45+
await this.subprocesses[appId].setupApp();
46+
} catch (error) {
47+
const subprocess = this.subprocesses[appId];
48+
delete this.subprocesses[appId];
49+
await subprocess.stopApp();
50+
throw error;
51+
}
3952

4053
return this.subprocesses[appId];
4154
}

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

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
import { AsyncTest, Expect, Setup, SetupFixture, SpyOn } from 'alsatian';
2+
3+
import { AppStatus } from '../../../src/definition/AppStatus';
4+
import type { AppManager } from '../../../src/server/AppManager';
5+
import type { IParseAppPackageResult } from '../../../src/server/compiler';
6+
import { AppRuntimeManager } from '../../../src/server/managers/AppRuntimeManager';
7+
import type { DenoRuntimeSubprocessController } from '../../../src/server/runtime/deno/AppsEngineDenoRuntime';
8+
import type { IAppStorageItem } from '../../../src/server/storage';
9+
10+
export class AppRuntimeManagerTestFixture {
11+
private mockManager: AppManager;
12+
13+
private runtimeManager: AppRuntimeManager;
14+
15+
private mockAppPackage: IParseAppPackageResult;
16+
17+
private mockStorageItem: IAppStorageItem;
18+
19+
private mockSubprocessController: DenoRuntimeSubprocessController;
20+
21+
@SetupFixture
22+
public setupFixture() {
23+
this.mockManager = {
24+
getAccessorManager() {
25+
return {} as any;
26+
},
27+
getApiManager() {
28+
return {} as any;
29+
},
30+
getLogStorage() {
31+
return {} as any;
32+
},
33+
getBridges() {
34+
return {} as any;
35+
},
36+
} as AppManager;
37+
38+
this.mockAppPackage = {
39+
info: {
40+
id: 'test-app',
41+
name: 'Test App',
42+
nameSlug: 'test-app',
43+
version: '1.0.0',
44+
description: 'Test app for unit testing',
45+
author: {
46+
name: 'Test Author',
47+
homepage: 'https://test.com',
48+
support: 'https://test.com/support',
49+
},
50+
permissions: [],
51+
requiredApiVersion: '1.0.0',
52+
classFile: 'main.js',
53+
iconFile: 'icon.png',
54+
implements: [],
55+
},
56+
files: {
57+
'main.js': 'console.log("Hello World");',
58+
},
59+
languageContent: {} as unknown as IParseAppPackageResult['languageContent'],
60+
implemented: {} as unknown as IParseAppPackageResult['implemented'],
61+
} as IParseAppPackageResult;
62+
63+
this.mockStorageItem = {
64+
id: 'test-app',
65+
status: AppStatus.MANUALLY_ENABLED,
66+
info: this.mockAppPackage.info,
67+
createdAt: new Date(),
68+
updatedAt: new Date(),
69+
} as IAppStorageItem;
70+
71+
this.mockSubprocessController = {
72+
async setupApp() {
73+
return Promise.resolve();
74+
},
75+
76+
async stopApp() {
77+
return Promise.resolve();
78+
},
79+
80+
getAppId() {
81+
return 'test-app';
82+
},
83+
} as DenoRuntimeSubprocessController;
84+
}
85+
86+
@Setup
87+
public setup() {
88+
this.runtimeManager = new AppRuntimeManager(this.mockManager, () => this.mockSubprocessController);
89+
}
90+
91+
@AsyncTest('Starts runtime for app successfully')
92+
public async startRuntimeForAppSuccessfully() {
93+
await Expect(() => this.runtimeManager.startRuntimeForApp(this.mockAppPackage, this.mockStorageItem)).not.toThrowAsync();
94+
95+
/* eslint-disable-next-line dot-notation -- We need to access the property like this for the compile not to complain */
96+
Expect(this.runtimeManager['subprocesses'][this.mockAppPackage.info.id]).toBe(this.mockSubprocessController);
97+
}
98+
99+
@AsyncTest('Fails to start runtime for app that already has a runtime')
100+
public async startMultipleRuntimesForSameApp() {
101+
await Expect(() => this.runtimeManager.startRuntimeForApp(this.mockAppPackage, this.mockStorageItem)).not.toThrowAsync();
102+
103+
await Expect(() => this.runtimeManager.startRuntimeForApp(this.mockAppPackage, this.mockStorageItem)).toThrowErrorAsync(
104+
Error,
105+
'App already has an associated runtime',
106+
);
107+
}
108+
109+
@AsyncTest('Starts multiple runtimes for app successfully with force option')
110+
public async startMultipleRuntimesForSameAppWithForceOption() {
111+
await Expect(() => this.runtimeManager.startRuntimeForApp(this.mockAppPackage, this.mockStorageItem)).not.toThrowAsync();
112+
113+
await Expect(() =>
114+
this.runtimeManager.startRuntimeForApp(this.mockAppPackage, this.mockStorageItem, { force: true }),
115+
).not.toThrowAsync();
116+
117+
/* eslint-disable-next-line dot-notation -- We need to access the property like this for the compile not to complain */
118+
Expect(this.runtimeManager['subprocesses'][this.mockAppPackage.info.id]).toBe(this.mockSubprocessController);
119+
}
120+
121+
@AsyncTest()
122+
public async startRuntimeThatFailsToSetup() {
123+
SpyOn(this.mockSubprocessController, 'setupApp').andReturn(Promise.reject(new Error('Nope')));
124+
125+
await Expect(() => this.runtimeManager.startRuntimeForApp(this.mockAppPackage, this.mockStorageItem)).toThrowErrorAsync(Error, 'Nope');
126+
127+
/* eslint-disable-next-line dot-notation -- We need to access the property like this for the compile not to complain */
128+
Expect(this.runtimeManager['subprocesses'][this.mockAppPackage.info.id]).not.toBeDefined();
129+
}
130+
}

‎packages/apps-engine/tests/tsconfig.json‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
{
2+
"extends": "../tsconfig.json",
3+
"compilerOptions": {
4+
"useDefineForClassFields": false,
5+
"rootDir": "../"
6+
},
7+
"include": ["./**/*"]
8+
}

0 commit comments

Comments
 (0)
⚡