From a05b8f72ca70dfed2959282c35518f78fbeb65cf Mon Sep 17 00:00:00 2001 From: Douglas Gubert Date: Mon, 10 Nov 2025 15:57:28 -0300 Subject: [PATCH] feat(apps): stop persisting intermediate status transitions to database (#37167) --- .changeset/real-pans-confess.md | 8 ++ .../ee/server/apps/communication/rest.ts | 4 +- .../tests/end-to-end/api/livechat/00-rooms.ts | 2 +- .../tests/end-to-end/apps/installation.ts | 2 +- packages/apps-engine/src/server/AppManager.ts | 96 ++++++------------- 5 files changed, 39 insertions(+), 73 deletions(-) create mode 100644 .changeset/real-pans-confess.md diff --git a/.changeset/real-pans-confess.md b/.changeset/real-pans-confess.md new file mode 100644 index 00000000000..5a6c5902a2f --- /dev/null +++ b/.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. diff --git a/apps/meteor/ee/server/apps/communication/rest.ts b/apps/meteor/ee/server/apps/communication/rest.ts index 152a43a93a4..3f001a6805d 100644 --- a/apps/meteor/ee/server/apps/communication/rest.ts +++ b/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); } diff --git a/apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts b/apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts index 4bb7d383ed7..34c8367544b 100644 --- a/apps/meteor/tests/end-to-end/api/livechat/00-rooms.ts +++ b/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; }); diff --git a/apps/meteor/tests/end-to-end/apps/installation.ts b/apps/meteor/tests/end-to-end/apps/installation.ts index 4fac049bcae..9f814c6089f 100644 --- a/apps/meteor/tests/end-to-end/apps/installation.ts +++ b/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; }) diff --git a/packages/apps-engine/src/server/AppManager.ts b/packages/apps-engine/src/server/AppManager.ts index 173dfc18fbd..4c8348b7266 100644 --- a/packages/apps-engine/src/server/AppManager.ts +++ b/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,6 +853,8 @@ 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())) { @@ -877,12 +862,18 @@ export class AppManager { } 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 { + private async runStartUpProcess(storageItem: IAppStorageItem, app: ProxiedApp, silenceStatus: boolean): Promise { 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 { + private async installApp(app: ProxiedApp, user: IUser): Promise { 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 { + private async initializeApp(app: ProxiedApp, silenceStatus = false): Promise { 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 { + private async enableApp(app: ProxiedApp, silenceStatus = false): Promise { 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;