fix: Presence broadcast isn't enabled after license validation #30282

matheusbsilva137 · 2023-09-05T11:54:14Z

Proposed changes (including videos or screenshots)

Enable presence broadcast after license is validated (if the troubleshoot setting is off).

Issue(s)

Steps to test or reproduce

Further comments

Context: we received multiple reports from customers saying that presence broadcast has been disabled when starting the server. This could be caused by some sort of race condition between the license.module and watch.instanceStatus events (for the presence feature). If validateAvailability runs before the license is validated, then presence broadcast will be disabled for this workspace (that is, the Presence_broadcast_disabled setting will be enabled) and won't be enabled anytime later.
The issue could be solved by disabling the Presence_broadcast_disabled setting directly in the DB. This fix should avoid such cases.
SUP-308

changeset-bot · 2023-09-05T11:54:39Z

🦋 Changeset detected

Latest commit: 8d68ec5

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

This PR includes changesets to release 29 packages

Name	Type
@rocket.chat/presence	Patch
@rocket.chat/meteor	Patch
@rocket.chat/presence-service	Patch
@rocket.chat/core-typings	Patch
@rocket.chat/rest-typings	Patch
@rocket.chat/core-services	Patch
@rocket.chat/cron	Patch
@rocket.chat/gazzodown	Patch
@rocket.chat/livechat	Patch
@rocket.chat/model-typings	Patch
@rocket.chat/ui-contexts	Patch
@rocket.chat/account-service	Patch
@rocket.chat/authorization-service	Patch
@rocket.chat/ddp-streamer	Patch
@rocket.chat/omnichannel-transcript	Patch
@rocket.chat/queue-worker	Patch
@rocket.chat/stream-hub-service	Patch
@rocket.chat/api-client	Patch
@rocket.chat/omnichannel-services	Patch
@rocket.chat/pdf-worker	Patch
rocketchat-services	Patch
@rocket.chat/ddp-client	Patch
@rocket.chat/fuselage-ui-kit	Patch
@rocket.chat/models	Patch
@rocket.chat/ui-client	Patch
@rocket.chat/ui-video-conf	Patch
@rocket.chat/uikit-playground	Patch
@rocket.chat/web-ui-registration	Patch
@rocket.chat/instance-status	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

codecov · 2023-09-05T12:26:28Z

Codecov Report

Merging #30282 (6d49ccf) into develop (8202f27) will decrease coverage by 0.18%.
Report is 30 commits behind head on develop.
The diff coverage is n/a.

❗ Current head 6d49ccf differs from pull request most recent head 8d68ec5. Consider uploading reports for the commit 8d68ec5 to get more accurate results

@@             Coverage Diff             @@
##           develop   #30282      +/-   ##
===========================================
- Coverage    50.15%   49.97%   -0.18%     
===========================================
  Files          781      773       -8     
  Lines        14520    14453      -67     
  Branches      2627     2613      -14     
===========================================
- Hits          7282     7223      -59     
+ Misses        6840     6821      -19     
- Partials       398      409      +11

Flag	Coverage Δ
e2e	`48.35% <ø> (-0.23%)`	⬇️
unit	`60.30% <ø> (+0.17%)`	⬆️

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

MarcosSpessatto · 2023-09-05T23:41:52Z

ee/packages/presence/src/Presence.ts

@@ -39,9 +39,13 @@ export class Presence extends ServiceClass implements IPresence {
 			}
 		});

-		this.onEvent('license.module', ({ module, valid }) => {
+		this.onEvent('license.module', async ({ module, valid }) => {


I believe this partially fixes the problem. Testing it locally, it looks like this event is not being received (even having a valid license). I'm unsure if this is the intended behavior or a problem with the onModule function.

Also, if I'm not wrong, started() method solves the problem. https://github.com/RocketChat/Rocket.Chat/blob/develop/ee/packages/presence/src/Presence.ts#L61

The only problem that could happen is, while started() is executing AND before it fully finishes, we receive an event on the watch.instanceStatus turning the setting to true again. What I'm trying to say is, the execution must be at this point when the started() finishes.

I think @sampaiodiego opinion is really important here.

I don't think this is the best solution either..

if the problem is really the method validateAvailability being called before the start up procedure, we should actually make sure that doesn't happen by adding some kind of control like started = true.. or maybe making like hasLicense = boolean | null , so if it is still null we ignore all events.

I've just pushed a new commit following the second approach.
But I'm not really convinced this will fix the issue 🤔 for the case where the license takes too long to fetch, hasLicense() will just return false while it's not validated, right? then all these changes won't have any effect
As a way to make sure this is fixed and always synced, I was thinking of adding an else statement here to toggle the setting's value and make sure it's set to false whenever the total amount of connections is below the max allowed. wdyt? (edit: pushed a new commit so it's less abstract)
cc @sampaiodiego @MarcosSpessatto

Thought of another approach: here
It's basically calling toggleBroadcast directly on new connections if the license is active and presence broadcast is disabled

debdutdeb · 2023-09-08T15:41:38Z

ee/packages/presence/src/Presence.ts

 			return;
 		}

 		if (this.getTotalConnections() > MAX_CONNECTIONS) {
 			this.broadcastEnabled = false;

 			await Settings.updateValueById('Presence_broadcast_disabled', true);
+		} else {
+			const presenceBroadcastDisabled = await Settings.findOneById('Troubleshoot_Disable_Presence_Broadcast');


this looks expensive, doesn't it? potentially two trips for every new connection?

Yeah, it did 😬
I've just had a better idea, I'm now checking the broadcastEnabled property to make sure that presence broadcast is always enabled when there's a valid license (so we just do it once, not at every new connection).

sampaiodiego · 2023-09-18T12:50:44Z

ee/packages/presence/src/Presence.ts

+
+				if (!this.broadcastEnabled && valid) {
+					// broadcast should always be enabled if license is active (unless the troubleshoot setting is on)
+					const presenceBroadcastDisabled = await Settings.findOneById('Troubleshoot_Disable_Presence_Broadcast');


checking this setting is not wrong, but it is not a concern of this package..

we need to create an alternate mechanism to properly respect this setting, as it is already being "ignored" in other places.

Co-authored-by: Diego Sampaio <chinello@gmail.com>

…ove/iframeLogin * 'develop' of github.com:RocketChat/Rocket.Chat: (29 commits) fix: Disables `GenericMenu` without any sections or items (#30319) i18n: Language update from LingoHub 🤖 on 2023-09-18Z (#30426) chore: remove connectToCloud option (#30430) chore: remove Troubleshoot options (#30429) chore: bump fuselage packages (#30424) chore: Webhooks page refactor (#30274) chore: color palette changes (#30385) fix: increase cron job check delay to 1 min (#30402) fix: Presence broadcast isn't enabled after license validation (#30282) test: create non private team and readonly team (#30371) Bump version to 6.5.0-develop refactor: Implement functional code for the 'useFilteredApps' hook (#30279) Release 6.4.0-rc.1 Release 6.3.6 regression: Login page callout messages (#30399) test: tests for muting and unmuting (#30286) regression: Fix rooms table not showing teams (#30361) regression: Move queue-timeout setting to CE and remove dependency on waiting queue (#30386) fix: engagement dashboard not working (#30277) Bump 6.3.6 ...

benedikt-wegmann · 2023-09-26T10:58:59Z

@matheusbsilva137 @sampaiodiego Please backport this change to the 6.3 and 6.4 branches as well, because - frankly - this is a pain and happens occasionally without any (intended) change on a running installation, like restarting a service.

(ping @ankar84 )

benedikt-wegmann · 2023-09-26T11:00:47Z

Ok, seems like it is included in the 6.3.7 changelog as well. 👍

Co-authored-by: Diego Sampaio <chinello@gmail.com>

Enable presence broadcast after validating the license

ef1ff6f

dionisio-bot bot added the stat: needs QA label Sep 5, 2023

Create changesets

863524f

matheusbsilva137 added the squad: team-collab label Sep 5, 2023

matheusbsilva137 added this to the 6.5.0 milestone Sep 5, 2023

matheusbsilva137 added 2 commits September 5, 2023 10:32

Merge branch 'develop' into fix/presence-broadcast-not-enabled-license

72fff5b

Merge branch 'develop' into fix/presence-broadcast-not-enabled-license

6357b62

matheusbsilva137 marked this pull request as ready for review September 5, 2023 17:01

matheusbsilva137 requested a review from MarcosSpessatto September 5, 2023 17:01

MarcosSpessatto requested changes Sep 5, 2023

View reviewed changes

MarcosSpessatto requested a review from sampaiodiego September 5, 2023 23:42

matheusbsilva137 added 2 commits September 8, 2023 11:48

Start hasLicense as undefined

6197449

Toggle broadcast on every new connection

1a9db89

debdutdeb reviewed Sep 8, 2023

View reviewed changes

matheusbsilva137 and others added 7 commits September 8, 2023 13:28

Improve condition

cbe2e0d

Improve condition again

3b59013

Readd license check (oops)

3dbb8b6

Enable broadcast on license check

05364fc

Do not assign undefined to hasLicense

3488ccc

Remove unnecessary line

2e60162

Merge branch 'develop' into fix/presence-broadcast-not-enabled-license

8a11ded

matheusbsilva137 requested a review from debdutdeb September 11, 2023 13:35

matheusbsilva137 and others added 2 commits September 12, 2023 15:39

Merge branch 'develop' into fix/presence-broadcast-not-enabled-license

361896e

Enable presence broadcast when license is validated

6d49ccf

sampaiodiego requested changes Sep 18, 2023

View reviewed changes

dont check troubleshooting setting

8d68ec5

sampaiodiego approved these changes Sep 18, 2023

View reviewed changes

sampaiodiego merged commit 9a90bff into develop Sep 18, 2023
5 of 6 checks passed

sampaiodiego deleted the fix/presence-broadcast-not-enabled-license branch September 18, 2023 12:54

sampaiodiego modified the milestones: 6.5.0, 6.3.7 Sep 18, 2023

sampaiodiego added a commit that referenced this pull request Sep 18, 2023

fix: Presence broadcast isn't enabled after license validation (#30282)

d9a1500

Co-authored-by: Diego Sampaio <chinello@gmail.com>

sampaiodiego added a commit that referenced this pull request Sep 18, 2023

fix: Presence broadcast isn't enabled after license validation (#30282)

c655be1

Co-authored-by: Diego Sampaio <chinello@gmail.com>

debdutdeb pushed a commit that referenced this pull request Oct 26, 2023

fix: Presence broadcast isn't enabled after license validation (#30282)

4477bcf

Co-authored-by: Diego Sampaio <chinello@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Presence broadcast isn't enabled after license validation #30282

fix: Presence broadcast isn't enabled after license validation #30282

matheusbsilva137 commented Sep 5, 2023 •

edited

Loading

changeset-bot bot commented Sep 5, 2023 •

edited

Loading

codecov bot commented Sep 5, 2023 •

edited

Loading

MarcosSpessatto Sep 5, 2023

sampaiodiego Sep 6, 2023

matheusbsilva137 Sep 8, 2023 •

edited

Loading

matheusbsilva137 Sep 8, 2023

debdutdeb Sep 8, 2023

matheusbsilva137 Sep 8, 2023 •

edited

Loading

sampaiodiego Sep 18, 2023

benedikt-wegmann commented Sep 26, 2023

benedikt-wegmann commented Sep 26, 2023

fix: Presence broadcast isn't enabled after license validation #30282

fix: Presence broadcast isn't enabled after license validation #30282

Conversation

matheusbsilva137 commented Sep 5, 2023 • edited Loading

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

changeset-bot bot commented Sep 5, 2023 • edited Loading

🦋 Changeset detected

codecov bot commented Sep 5, 2023 • edited Loading

Codecov Report

MarcosSpessatto Sep 5, 2023

Choose a reason for hiding this comment

sampaiodiego Sep 6, 2023

Choose a reason for hiding this comment

matheusbsilva137 Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

matheusbsilva137 Sep 8, 2023

Choose a reason for hiding this comment

debdutdeb Sep 8, 2023

Choose a reason for hiding this comment

matheusbsilva137 Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

sampaiodiego Sep 18, 2023

Choose a reason for hiding this comment

benedikt-wegmann commented Sep 26, 2023

benedikt-wegmann commented Sep 26, 2023

matheusbsilva137 commented Sep 5, 2023 •

edited

Loading

changeset-bot bot commented Sep 5, 2023 •

edited

Loading

codecov bot commented Sep 5, 2023 •

edited

Loading

matheusbsilva137 Sep 8, 2023 •

edited

Loading

matheusbsilva137 Sep 8, 2023 •

edited

Loading