fix(apps-engine): add rejection handlers to fire-and-forget post-event dispatches in AppListenerManager#39373
Open
smirk-dev wants to merge 1 commit intoRocketChat:developfrom
Conversation
…t dispatches Post-event handlers (IPostMessageSent, IPostSystemMessageSent, IPostMessageDeleted, IPostMessageUpdated, IPostRoomCreate, IPostRoomDeleted, IPostExternalComponentOpened, IPostExternalComponentClosed) were async methods called without a .catch() handler. If an app threw inside a post-event hook, the resulting unhandled promise rejection could terminate the Node.js process (default behavior since Node 15+). The fire-and-forget semantics are intentionally preserved — post-events must not block the caller — but each call now uses void expr.catch(console.error) so that errors are visible in server logs without crashing the process.
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: b35c872 The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
WalkthroughA patch is applied to fix unhandled promise rejections in AppListenerManager's post-event handlers. The change wraps async fire-and-forget post-event invocations with error handling via Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/apps-engine/src/server/managers/AppListenerManager.ts (1)
359-418: Consider centralizing the repeated fire-and-forget error handling.The same
void ... .catch(console.error)pattern is duplicated across eight branches. A small helper reduces drift and lets you include event context consistently in logs.♻️ Proposed refactor
export class AppListenerManager { + private fireAndForget(event: AppInterface, promise: Promise<void>): void { + void promise.catch((error) => console.error(`[AppListenerManager] ${event} failed`, error)); + } + public async executeListener<I extends keyof IListenerExecutor>( int: I, data: IListenerExecutor[I]['args'][0], ): Promise<IListenerExecutor[I]['result']> { @@ case AppInterface.IPostMessageSent: - void this.executePostMessageSent(data as IMessage).catch(console.error); + this.fireAndForget(AppInterface.IPostMessageSent, this.executePostMessageSent(data as IMessage)); return; @@ case AppInterface.IPostExternalComponentClosed: - void this.executePostExternalComponentClosed(data as IExternalComponent).catch(console.error); + this.fireAndForget( + AppInterface.IPostExternalComponentClosed, + this.executePostExternalComponentClosed(data as IExternalComponent), + ); return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps-engine/src/server/managers/AppListenerManager.ts` around lines 359 - 418, Several branches repeat the "void X(...).catch(console.error)" fire-and-forget pattern; extract a small helper (e.g., runFireAndForget, safeExecute, or this.executeAsyncAndLog) that accepts a Promise-returning invocation, an event name, and the payload, invokes the promise and centrally handles .catch to log a contextual message; replace calls to executePostMessageSent, executePostSystemMessageSent, executePostMessageDelete, executePostMessageUpdated, executePostRoomCreate, executePostRoomDeleted, executePostExternalComponentOpened, and executePostExternalComponentClosed with this helper so all async "fire-and-forget" handlers use consistent error logging including the event name and payload/context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/apps-engine/src/server/managers/AppListenerManager.ts`:
- Around line 359-418: Several branches repeat the "void
X(...).catch(console.error)" fire-and-forget pattern; extract a small helper
(e.g., runFireAndForget, safeExecute, or this.executeAsyncAndLog) that accepts a
Promise-returning invocation, an event name, and the payload, invokes the
promise and centrally handles .catch to log a contextual message; replace calls
to executePostMessageSent, executePostSystemMessageSent,
executePostMessageDelete, executePostMessageUpdated, executePostRoomCreate,
executePostRoomDeleted, executePostExternalComponentOpened, and
executePostExternalComponentClosed with this helper so all async
"fire-and-forget" handlers use consistent error logging including the event name
and payload/context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b959c77-7778-4910-b5bc-8bcce3f48a10
📒 Files selected for processing (2)
.changeset/fix-apps-engine-post-event-unhandled-promises.mdpackages/apps-engine/src/server/managers/AppListenerManager.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/apps-engine/src/server/managers/AppListenerManager.ts
🧠 Learnings (5)
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
.changeset/fix-apps-engine-post-event-unhandled-promises.md
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
.changeset/fix-apps-engine-post-event-unhandled-promises.md
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
packages/apps-engine/src/server/managers/AppListenerManager.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/apps-engine/src/server/managers/AppListenerManager.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/apps-engine/src/server/managers/AppListenerManager.ts
🔇 Additional comments (2)
packages/apps-engine/src/server/managers/AppListenerManager.ts (1)
359-418: Fix correctly hardens fire-and-forget post-event dispatches.Attaching
.catch(...)to each non-awaited post-event call preserves non-blocking behavior while preventing unhandled rejection crashes..changeset/fix-apps-engine-post-event-unhandled-promises.md (1)
1-5: Changeset entry matches the implementation and release scope.Patch note clearly captures the fire-and-forget rejection handling fix and the impacted post-event hooks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
In
AppListenerManager.executeListener(), post-event handlers are intentionally fire-and-forget — they must not block the caller waiting for app hooks to complete. However, 6 of these async methods were being called with neitherawaitnor acatch()handler:Root Cause
Calling an
asyncfunction withoutawaitand without.catch()creates a dangling promise. If the async method rejects (e.g., an installed app throws inside itsonPostMessageSenthandler), Node.js emits anUnhandledPromiseRejectionevent. Since Node.js 15+, the default behavior is process termination on unhandled rejections.Affected event cases
IPostMessageSentIPostSystemMessageSentIPostMessageDeletedIPostMessageUpdatedIPostRoomCreateIPostRoomDeletedIPostExternalComponentOpenedIPostExternalComponentClosedFix
Uses
void expr.catch(console.error)— makes the fire-and-forget intent explicit to TypeScript'sno-floating-promisesrule, while ensuring any rejection is surfaced in server logs rather than silently crashing the process:All 208 existing tests pass.
Summary by CodeRabbit
Bug Fixes