Skip to content

fix(apps-engine): add missing await in AppSchedulerManager.cleanUp#39048

Open
smirk-dev wants to merge 2 commits intoRocketChat:developfrom
smirk-dev:fix/apps-engine-await-cleanup
Open

fix(apps-engine): add missing await in AppSchedulerManager.cleanUp#39048
smirk-dev wants to merge 2 commits intoRocketChat:developfrom
smirk-dev:fix/apps-engine-await-cleanup

Conversation

@smirk-dev
Copy link
Contributor

@smirk-dev smirk-dev commented Feb 25, 2026

Description

Adds a missing await in AppSchedulerManager.cleanUp() to prevent a race condition during app uninstall/disable.

Root Cause

// BEFORE
public async cleanUp(appId: string): Promise<void> {
    (this.bridge as IInternalSchedulerBridge & SchedulerBridge).cancelAllJobs(appId);
    // ^ async method called without await
}

cancelAllJobs(appId) is an async bridge method but was called fire-and-forget. The cleanUp method resolves immediately while job cancellation is still in progress.

Impact

When an app is uninstalled or disabled:

  1. cleanUp returns before cancellation completes
  2. The app's registered processors are removed from this.registeredProcessors
  3. If a scheduled job fires during this window, the wrapProcessor callback throws "Processor ${processorId} not available"
  4. On app reinstall, zombie jobs can accumulate since old ones were never fully cancelled

Fix

// AFTER
public async cleanUp(appId: string): Promise<void> {
    await (this.bridge as IInternalSchedulerBridge & SchedulerBridge).cancelAllJobs(appId);
}

Testing

  • tsc --noEmit clean
  • Single-word fix with significant correctness impact

Summary by CodeRabbit

  • Bug Fixes
    • Fixed a race condition that could occur during app uninstall or disable, where scheduled jobs might not be fully cleaned up. Apps now properly cancel all scheduled operations before the uninstall/disable process completes.

cancelAllJobs(appId) is an async bridge method but was called without
await in cleanUp(). This causes a race condition during app uninstall:
registered processors are cleared before cancellation finishes, so
scheduled jobs firing in that window throw "Processor not available".
On app reinstall, zombie jobs can accumulate since the old ones were
never fully cancelled.
@smirk-dev smirk-dev requested a review from a team as a code owner February 25, 2026 11:06
@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Feb 25, 2026

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

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

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Feb 25, 2026

🦋 Changeset detected

Latest commit: e9765bc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 42 packages
Name Type
@rocket.chat/apps-engine Patch
@rocket.chat/meteor Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/core-typings Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/rest-typings Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/ui-voip Patch
@rocket.chat/api-client Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/gazzodown Patch
@rocket.chat/http-router Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/abac Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/license Patch
@rocket.chat/media-calls Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch
@rocket.chat/server-fetch Patch

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

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 813d57a and 5faf27b.

📒 Files selected for processing (2)
  • .changeset/fix-apps-engine-await-cleanup.md
  • packages/apps-engine/src/server/managers/AppSchedulerManager.ts
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: 🔨 Test Storybook / Test Storybook
  • GitHub Check: 🔨 Test Unit / Unit Tests
  • GitHub Check: 🔎 Code Check / Code Lint
  • GitHub Check: 🔎 Code Check / TypeScript
  • GitHub Check: 📦 Meteor Build (coverage)
🧰 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/AppSchedulerManager.ts
🧠 Learnings (1)
📚 Learning: 2026-02-24T19:09:01.522Z
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:01.522Z
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-await-cleanup.md
🔇 Additional comments (2)
packages/apps-engine/src/server/managers/AppSchedulerManager.ts (1)

89-89: Awaited cancellation in cleanup is the right fix.

Line 89 now correctly waits for cancelAllJobs(appId) before resolving cleanUp, preventing teardown races during uninstall/disable flows.

.changeset/fix-apps-engine-await-cleanup.md (1)

1-5: Changeset entry is accurate and appropriately scoped.

Patch-level classification and the summary text clearly match the behavioral fix in AppSchedulerManager.cleanUp.


Walkthrough

This PR adds a missing await keyword to the cancelAllJobs(appId) call in AppSchedulerManager.cleanUp to ensure scheduled jobs are fully cancelled before method completion, preventing race conditions during app uninstall/disable operations. A changelog entry documents this patch-level fix.

Changes

Cohort / File(s) Summary
AppSchedulerManager await fix
.changeset/fix-apps-engine-await-cleanup.md, packages/apps-engine/src/server/managers/AppSchedulerManager.ts
Adds missing await to cancelAllJobs(appId) in cleanUp method to ensure scheduled jobs are fully cancelled before method resolution, preventing race conditions. Includes patch-level changelog entry.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

🐰 A missing await caused chaos and strife,
Jobs lingering, causing app life!
Now cleanUp resolves with a gentle wait,
No more races—we've sealed our fate! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: adding a missing await in AppSchedulerManager.cleanUp, which is the core fix in the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

@ggazzo ggazzo added this to the 8.3.0 milestone Feb 25, 2026
@codecov
Copy link

codecov bot commented Feb 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.64%. Comparing base (8d73ce5) to head (e9765bc).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #39048      +/-   ##
===========================================
- Coverage    70.65%   70.64%   -0.02%     
===========================================
  Files         3190     3190              
  Lines       112732   112732              
  Branches     20431    20438       +7     
===========================================
- Hits         79649    79637      -12     
- Misses       31035    31048      +13     
+ Partials      2048     2047       -1     
Flag Coverage Δ
e2e 60.33% <ø> (-0.02%) ⬇️
e2e-api 47.77% <ø> (-0.10%) ⬇️
unit 71.28% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants