fix(apps-engine): check installApp return value before initialization… by anandcomp22 · Pull Request #37487 · RocketChat/Rocket.Chat

anandcomp22 · 2025-11-11T21:07:37Z

Proposed Changes

This pull request enhances error handling in the AppManager class by verifying the outcome of the installApp() method before moving forward with app initialization.

Previously, the add() method overlooked the boolean return value from installApp(), allowing initialization to continue even if installation failed. This behavior could lead to inconsistencies and make it difficult to identify underlying installation issues.

Key Changes

Capture the return value of installApp() within the add() function.
Prevent initialization or enablement if the installation fails.
Log clear and detailed error messages to simplify debugging.
Improve consistency with other error-handling patterns in the AppManager.

Issue(s)

Closes #37472

Steps to Test or Reproduce

Install an app that intentionally fails during installation (for example, one that throws an exception or includes an invalid manifest).
Observe the following behavior:
- The installation process stops immediately.
- The app does not proceed to initialization or become enabled.
- The app’s status is marked as ERROR_DISABLED, and a corresponding log entry clearly indicates the failure.

Further Comments

This update makes sure that serious installation errors are properly caught instead of being overlooked.
It also keeps the error-handling process consistent with Rocket.Chat’s best practices for managing the app lifecycle.

Summary by CodeRabbit

Bug Fixes
- Improved handling of app installation failures: failed installs are now logged, the app is marked as disabled to prevent repeated startup attempts, related errors are recorded, and initialization is skipped to avoid cascading failures.

…RocketChat#37472)

changeset-bot · 2025-11-11T21:07:42Z

⚠️ No Changeset found

Latest commit: 7b2fc89

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

dionisio-bot · 2025-11-11T21:07:42Z

Looks like this PR is not ready to merge, because of the following issues:

This PR is missing the 'stat: QA assured' label
This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

CLAassistant · 2025-11-11T21:07:44Z

All committers have signed the CLA.

coderabbitai · 2025-11-11T21:08:15Z

Walkthrough

Adds a public static Instance to AppManager and updates add() to check the boolean result of installApp(). On install failure the code now logs an error, marks the app storage item as ERROR_DISABLED, updates storage via appMetadataStorage, sets a storage error on the AppFabricationFulfillment, and returns early to skip initialization.

Changes

Cohort / File(s)	Change Summary
AppManager: instance + install failure handling `packages/apps-engine/src/server/AppManager.ts`	Added `public static Instance: AppManager;`. In `add()` capture `installApp()` return value; on `false` log an error, set app storage status to `ERROR_DISABLED`, persist that status via `appMetadataStorage`, call `setStorageError()` on the `AppFabricationFulfillment`, and return early to avoid calling initialization/startup flows.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant AppManager
    participant installApp as installApp()
    participant appMetadataStorage as appMetadataStorage
    participant fulfillment as AppFabricationFulfillment
    participant logger as Logger

    Caller->>AppManager: add()
    AppManager->>installApp: invoke
    alt Installation Fails (returns false)
        installApp-->>AppManager: false
        AppManager->>logger: error("install failed")
        AppManager->>appMetadataStorage: updateStatus(..., ERROR_DISABLED)
        AppManager->>fulfillment: setStorageError(error)
        AppManager-->>Caller: return (early)
    else Installation Succeeds (returns true)
        installApp-->>AppManager: true
        AppManager->>AppManager: initializeApp()
        AppManager->>AppManager: runStartUpProcess()
        AppManager-->>Caller: complete
    end

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Focus review on the early-return path to ensure no remaining side-effects are expected after aborting.
Verify appMetadataStorage persistence and the setStorageError() call correctly reflect desired state.
Confirm public static Instance is intended and initialized elsewhere or via tests.

Possibly related PRs

feat(apps): stop persisting intermediate status transitions to database #37167 — modifies AppManager startup/install/status handling and is directly related to the install→initialize control flow.
fix: stop object reference leak in AppFabricationFulfillment #36943 — touches AppFabricationFulfillment interactions and storage handling referenced in this change.

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

ggazzo
sampaiodiego

Poem

🐰
I hopped to check the install today,
If it failed I paved a safe new way.
I logged the fall and marked the spot,
Kept broken bits from hopping out — not.
A tiny rabbit chorus: "Stable code, hooray!"

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The addition of the public static Instance member appears to be out of scope and unrelated to the core objective of checking installApp return value before initialization.	Remove the public static Instance field addition, as it is not related to issue #37472 requirements for improving error handling in the add() flow.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: checking installApp return value before app initialization, which is the core objective of the PR.
Linked Issues check	✅ Passed	The code changes capture installApp() return value, prevent initialization on failure, set ERROR_DISABLED status, log errors, and align with AppManager patterns as required by issue #37472.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 617f4fc and d57ccdb.

📒 Files selected for processing (1)

packages/apps-engine/src/server/AppManager.ts (2 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-10-06T20:30:45.540Z

Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.

Applied to files:

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

🔇 Additional comments (2)

packages/apps-engine/src/server/AppManager.ts (2)

67-67: Singleton instance now publicly exposed.

The new static Instance field enables external access to the singleton AppManager, which is used by getPermissionsByAppId at line 1227. This is consistent with the singleton pattern already enforced in the constructor (lines 114-116).

637-646: Installation failure handling is correctly implemented.

The code properly captures the installApp() return value and prevents initialization when installation fails. When installation fails:

app.setStatus(status) (line 1020 in installApp) updates in-memory state and notifies the activation bridge

appMetadataStorage.updateStatus() (line 643) persists the ERROR_DISABLED status to storage

These operations are complementary and both necessary—the first handles in-app notifications, the second ensures persistence.

After installation failure, the app remains in the system with ERROR_DISABLED status and allocated resources (runtime, user, storage), consistent with how other error states like COMPILER_ERROR_DISABLED are handled (see AppStatus.isError() and enableAll() skip logic at lines 304-312).

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 260950f and 617f4fc.

📒 Files selected for processing (1)

packages/apps-engine/src/server/AppManager.ts (2 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-10-06T20:30:45.540Z

Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.

Applied to files:

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

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

…d fields

anandcomp22 · 2025-11-15T06:08:00Z

Looks like this PR is not ready to merge, because of the following issues:

This PR is missing the 'stat: QA assured' label

This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

d-gubert · 2025-11-18T22:50:25Z

Hey, I've tried installing the app below and it was still successfully installed and enabled

export class SlashcommanderApp extends App {
    constructor(info: IAppInfo, logger: ILogger, accessors: IAppAccessors) {
        super(info, logger, accessors);
    }

    public async onInstall(context: IAppInstallationContext, read: IRead, http: IHttp, persistence: IPersistence, modify: IModify): Promise<void> {
        throw new Error('What happens now?');
    }
}

I can see the error thrown in server logs

W20251118-19:42:21.225(-3)? (STDERR) JSON-RPC error received:  JsonRpcError {
W20251118-19:42:21.225(-3)? (STDERR)   message: 'What happens now?',
W20251118-19:42:21.225(-3)? (STDERR)   code: -32000,
W20251118-19:42:21.225(-3)? (STDERR)   data: {
W20251118-19:42:21.225(-3)? (STDERR)     logs: {
W20251118-19:42:21.225(-3)? (STDERR)       appId: 'bc3a2b79-7c30-43e7-8405-7ebc0c9be7d2',
W20251118-19:42:21.225(-3)? (STDERR)       method: 'app:onInstall',
W20251118-19:42:21.226(-3)? (STDERR)       entries: [Array],
W20251118-19:42:21.226(-3)? (STDERR)       startTime: 2025-11-18T22:42:21.222Z,
W20251118-19:42:21.226(-3)? (STDERR)       endTime: 2025-11-18T22:42:21.223Z,
W20251118-19:42:21.226(-3)? (STDERR)       totalTime: 1,
W20251118-19:42:21.226(-3)? (STDERR)       _createdAt: 2025-11-18T22:42:21.223Z,
W20251118-19:42:21.226(-3)? (STDERR)       instanceId: '6449a364-9154-43d8-a539-3e04a0815460'
W20251118-19:42:21.226(-3)? (STDERR)     }
W20251118-19:42:21.226(-3)? (STDERR)   }
W20251118-19:42:21.226(-3)? (STDERR) }

Please review your implementation for it to match your intended behavior

Observe the following behavior:

The installation process stops immediately.

The app does not proceed to initialization or become enabled.

The app’s status is marked as ERROR_DISABLED, and a corresponding log entry clearly indicates the failure.

fix(apps-engine): check installApp return value before initialization (…

617f4fc

…RocketChat#37472)

anandcomp22 requested a review from a team as a code owner November 11, 2025 21:07

coderabbitai bot reviewed Nov 11, 2025

View reviewed changes

packages/apps-engine/src/server/AppManager.ts Outdated Show resolved Hide resolved

anandcomp22 and others added 4 commits November 12, 2025 02:46

fix(apps-engine): properly handle installApp failure and remove unuse…

833731a

…d fields

Merge branch 'develop' into fix/appmanager-install-check

d57ccdb

Merge branch 'develop' into fix/appmanager-install-check

f4dd91d

Merge branch 'develop' into fix/appmanager-install-check

0fe2b27

Merge branch 'develop' into fix/appmanager-install-check

7b2fc89

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(apps-engine): check installApp return value before initialization…#37487

fix(apps-engine): check installApp return value before initialization…#37487
anandcomp22 wants to merge 6 commits intoRocketChat:developfrom
anandcomp22:fix/appmanager-install-check

anandcomp22 commented Nov 11, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

changeset-bot bot commented Nov 11, 2025 •

edited

Loading

Uh oh!

dionisio-bot bot commented Nov 11, 2025

Uh oh!

CLAassistant commented Nov 11, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Nov 11, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

anandcomp22 commented Nov 15, 2025 •

edited

Loading

Uh oh!

d-gubert commented Nov 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

anandcomp22 commented Nov 11, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Key Changes

Issue(s)

Steps to Test or Reproduce

Further Comments

Summary by CodeRabbit

Uh oh!

changeset-bot bot commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

dionisio-bot bot commented Nov 11, 2025

Uh oh!

CLAassistant commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anandcomp22 commented Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d-gubert commented Nov 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

anandcomp22 commented Nov 11, 2025 •

edited by coderabbitai bot

Loading

changeset-bot bot commented Nov 11, 2025 •

edited

Loading

CLAassistant commented Nov 11, 2025 •

edited

Loading

coderabbitai bot commented Nov 11, 2025 •

edited

Loading

anandcomp22 commented Nov 15, 2025 •

edited

Loading