feat(apps): stop persisting intermediate status transitions to database (#37167)

Author: d-gubert

.changeset/real-pans-confess.md

@@ -0,0 +1,8 @@
+---
+'@rocket.chat/apps-engine': minor
+'@rocket.chat/meteor': minor
+---
+
+Changes a behavior that would store the result of every status transition that happened to apps
+
+This caused intermediate status to be saved to the database, which could prevent apps from being restored to the desired status when restarted or during server startup.

apps/meteor/ee/server/apps/communication/rest.ts

@@ -474,8 +474,8 @@ export class AppsRestApi {
 					try {
 						await canEnableApp(aff.getApp().getStorageItem());
 
-						const success = await manager.enable(info.id);
-						info.status = success ? AppStatus.AUTO_ENABLED : info.status;
+						const success = await manager.changeStatus(info.id, AppStatus.MANUALLY_ENABLED);
+						info.status = await success.getStatus();
 					} catch (error) {
 						orchestrator.getRocketChatLogger().warn(`App "${info.id}" was installed but could not be enabled: `, error);
 					}

apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts

@@ -101,7 +101,7 @@ describe('LIVECHAT - rooms', () => {
 					expect(res.body).to.have.a.property('app');
 					expect(res.body.app).to.have.a.property('id');
 					expect(res.body.app).to.have.a.property('version');
-					expect(res.body.app).to.have.a.property('status').and.to.be.equal('auto_enabled');
+					expect(res.body.app).to.have.a.property('status').and.to.be.equal('manually_enabled');
 
 					appId = res.body.app.id;
 				});

apps/meteor/tests/end-to-end/apps/installation.ts

@@ -52,7 +52,7 @@ describe('Apps - Installation', () => {
 						expect(res.body).to.have.a.property('app');
 						expect(res.body.app).to.have.a.property('id');
 						expect(res.body.app).to.have.a.property('version');
-						expect(res.body.app).to.have.a.property('status').and.to.be.equal('auto_enabled');
+						expect(res.body.app).to.have.a.property('status').and.to.be.equal('manually_enabled');
 
 						app = res.body.app;
 					})

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

@@ -310,7 +310,7 @@ export class AppManager {
 				continue;
 			}
 
-			await this.initializeApp(rl.getStorageItem(), rl, false, true).catch(console.error);
+			await this.initializeApp(rl, true).catch(console.error);
 		}
 
 		// Let's ensure the required settings are all set
@@ -329,7 +329,7 @@ export class AppManager {
 		for (const app of this.apps.values()) {
 			const status = await app.getStatus();
 			if (!AppStatusUtils.isDisabled(status) && AppStatusUtils.isEnabled(app.getPreviousStatus())) {
-				await this.enableApp(app.getStorageItem(), app, true, app.getPreviousStatus() === AppStatus.MANUALLY_ENABLED).catch(console.error);
+				await this.enableApp(app).catch(console.error);
 			} else if (!AppStatusUtils.isError(status)) {
 				this.listenerManager.lockEssentialEvents(app);
 				this.uiActionButtonManager.clearAppActionButtons(app.getID());
@@ -457,14 +457,7 @@ export class AppManager {
 			throw new Error(`Could not enable an App with the id of "${id}" as it doesn't exist.`);
 		}
 
-		const isSetup = await this.runStartUpProcess(storageItem, rl, true, false);
-
-		if (isSetup) {
-			storageItem.status = await rl.getStatus();
-			// This is async, but we don't care since it only updates in the database
-			// and it should not mutate any properties we care about
-			await this.appMetadataStorage.updateStatus(storageItem._id, storageItem.status).catch(() => {});
-		}
+		const isSetup = await this.runStartUpProcess(storageItem, rl, false);
 
 		return isSetup;
 	}
@@ -495,12 +488,7 @@ export class AppManager {
 		const storageItem = await this.appMetadataStorage.retrieveOne(id);
 
 		app.getStorageItem().marketplaceInfo = storageItem.marketplaceInfo;
-		await app.validateLicense().catch();
-
-		storageItem.status = await app.getStatus();
-		// This is async, but we don't care since it only updates in the database
-		// and it should not mutate any properties we care about
-		await this.appMetadataStorage.updateStatus(storageItem._id, storageItem.status).catch(() => {});
+		await app.validateLicense().catch(() => {});
 
 		return true;
 	}
@@ -570,7 +558,7 @@ export class AppManager {
 		const descriptor: IAppStorageItem = {
 			id: result.info.id,
 			info: result.info,
-			status: AppStatus.UNKNOWN,
+			status: enable ? AppStatus.MANUALLY_ENABLED : AppStatus.MANUALLY_DISABLED,
 			settings: {},
 			implemented: result.implemented.getValues(),
 			installationSource: marketplaceInfo ? AppInstallationSource.MARKETPLACE : AppInstallationSource.PRIVATE,
@@ -645,15 +633,15 @@ export class AppManager {
 				// If an error occurs during this, oh well.
 			});
 
-		await this.installApp(created, app, user);
+		await this.installApp(app, user);
 
 		// Should enable === true, then we go through the entire start up process
 		// Otherwise, we only initialize it.
 		if (enable) {
 			// Start up the app
-			await this.runStartUpProcess(created, app, false, false);
+			await this.runStartUpProcess(created, app, false);
 		} else {
-			await this.initializeApp(created, app, true);
+			await this.initializeApp(app);
 		}
 
 		return aff;
@@ -727,15 +715,10 @@ export class AppManager {
 
 		const descriptor: IAppStorageItem = {
 			...old,
-			createdAt: old.createdAt,
 			id: result.info.id,
 			info: result.info,
-			status: (await this.apps.get(old.id)?.getStatus()) || old.status,
 			languageContent: result.languageContent,
-			settings: old.settings,
 			implemented: result.implemented.getValues(),
-			...(old.marketplaceInfo && { marketplaceInfo: old.marketplaceInfo }),
-			...(old.sourcePath && { sourcePath: old.sourcePath }),
 		};
 
 		if (!permissionsGranted) {
@@ -833,12 +816,12 @@ export class AppManager {
 
 	public async updateAndStartupLocal(stored: IAppStorageItem, appPackageOrInstance: ProxiedApp | Buffer) {
 		const app = await this.updateLocal(stored, appPackageOrInstance);
-		await this.runStartUpProcess(stored, app, false, true);
+		await this.runStartUpProcess(stored, app, true);
 	}
 
 	public async updateAndInitializeLocal(stored: IAppStorageItem, appPackageOrInstance: ProxiedApp | Buffer) {
 		const app = await this.updateLocal(stored, appPackageOrInstance);
-		await this.initializeApp(stored, app, true, true);
+		await this.initializeApp(app, true);
 	}
 
 	public getLanguageContent(): { [key: string]: object } {
@@ -870,19 +853,27 @@ export class AppManager {
 			throw new Error('Can not change the status of an App which does not currently exist.');
 		}
 
+		const storageItem = await rl.getStorageItem();
+
 		if (AppStatusUtils.isEnabled(status)) {
 			// Then enable it
 			if (AppStatusUtils.isEnabled(await rl.getStatus())) {
 				throw new Error('Can not enable an App which is already enabled.');
 			}
 
 			await this.enable(rl.getID());
+
+			storageItem.status = AppStatus.MANUALLY_ENABLED;
+			await this.appMetadataStorage.updateStatus(storageItem._id, AppStatus.MANUALLY_ENABLED);
 		} else {
 			if (!AppStatusUtils.isEnabled(await rl.getStatus())) {
 				throw new Error('Can not disable an App which is not enabled.');
 			}
 
 			await this.disable(rl.getID(), AppStatus.MANUALLY_DISABLED);
+
+			storageItem.status = AppStatus.MANUALLY_DISABLED;
+			await this.appMetadataStorage.updateStatus(storageItem._id, AppStatus.MANUALLY_DISABLED);
 		}
 
 		return rl;
@@ -973,27 +964,22 @@ export class AppManager {
 
 		const item = rl.getStorageItem();
 
-		await this.initializeApp(item, rl, false, silenceStatus);
+		await this.initializeApp(rl, silenceStatus);
 
 		if (!this.areRequiredSettingsSet(item)) {
 			await rl.setStatus(AppStatus.INVALID_SETTINGS_DISABLED);
 		}
 
 		if (!AppStatusUtils.isDisabled(await rl.getStatus()) && AppStatusUtils.isEnabled(rl.getPreviousStatus())) {
-			await this.enableApp(item, rl, false, rl.getPreviousStatus() === AppStatus.MANUALLY_ENABLED, silenceStatus);
+			await this.enableApp(rl, silenceStatus);
 		}
 
 		return this.apps.get(item.id);
 	}
 
-	private async runStartUpProcess(
-		storageItem: IAppStorageItem,
-		app: ProxiedApp,
-		isManual: boolean,
-		silenceStatus: boolean,
-	): Promise<boolean> {
+	private async runStartUpProcess(storageItem: IAppStorageItem, app: ProxiedApp, silenceStatus: boolean): Promise<boolean> {
 		if ((await app.getStatus()) !== AppStatus.INITIALIZED) {
-			const isInitialized = await this.initializeApp(storageItem, app, true, silenceStatus);
+			const isInitialized = await this.initializeApp(app, silenceStatus);
 			if (!isInitialized) {
 				return false;
 			}
@@ -1004,10 +990,10 @@ export class AppManager {
 			return false;
 		}
 
-		return this.enableApp(storageItem, app, true, isManual, silenceStatus);
+		return this.enableApp(app, silenceStatus);
 	}
 
-	private async installApp(_storageItem: IAppStorageItem, app: ProxiedApp, user: IUser): Promise<boolean> {
+	private async installApp(app: ProxiedApp, user: IUser): Promise<boolean> {
 		let result: boolean;
 		const context = { user };
 
@@ -1044,7 +1030,7 @@ export class AppManager {
 		return result;
 	}
 
-	private async initializeApp(storageItem: IAppStorageItem, app: ProxiedApp, saveToDb = true, silenceStatus = false): Promise<boolean> {
+	private async initializeApp(app: ProxiedApp, silenceStatus = false): Promise<boolean> {
 		let result: boolean;
 
 		try {
@@ -1072,17 +1058,6 @@ export class AppManager {
 			result = false;
 
 			await app.setStatus(status, silenceStatus);
-
-			// If some error has happened in initialization, like license or installations invalidation
-			// we need to store this on the DB regardless of what the parameter requests
-			saveToDb = true;
-		}
-
-		if (saveToDb) {
-			// This is async, but we don't care since it only updates in the database
-			// and it should not mutate any properties we care about
-			storageItem.status = await app.getStatus();
-			await this.appMetadataStorage.updateStatus(storageItem._id, storageItem.status).catch(() => {});
 		}
 
 		return result;
@@ -1133,13 +1108,7 @@ export class AppManager {
 		return result;
 	}
 
-	private async enableApp(
-		storageItem: IAppStorageItem,
-		app: ProxiedApp,
-		saveToDb = true,
-		isManual: boolean,
-		silenceStatus = false,
-	): Promise<boolean> {
+	private async enableApp(app: ProxiedApp, silenceStatus = false): Promise<boolean> {
 		let enable: boolean;
 		let status = AppStatus.ERROR_DISABLED;
 
@@ -1150,7 +1119,7 @@ export class AppManager {
 			enable = (await app.call(AppMethod.ONENABLE)) as boolean;
 
 			if (enable) {
-				status = isManual ? AppStatus.MANUALLY_ENABLED : AppStatus.AUTO_ENABLED;
+				status = AppStatus.MANUALLY_ENABLED;
 			} else {
 				status = AppStatus.DISABLED;
 				console.warn(`The App (${app.getID()}) disabled itself when being enabled. \nCheck the "onEnable" implementation for details.`);
@@ -1167,10 +1136,6 @@ export class AppManager {
 			}
 
 			console.error(e);
-
-			// If some error has happened during enabling, like license or installations invalidation
-			// we need to store this on the DB regardless of what the parameter requests
-			saveToDb = true;
 		}
 
 		if (enable) {
@@ -1188,13 +1153,6 @@ export class AppManager {
 			});
 		}
 
-		if (saveToDb) {
-			storageItem.status = status;
-			// This is async, but we don't care since it only updates in the database
-			// and it should not mutate any properties we care about
-			await this.appMetadataStorage.updateStatus(storageItem._id, storageItem.status).catch(() => {});
-		}
-
 		await app.setStatus(status, silenceStatus);
 
 		return enable;