Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 16 commits into from
Sep 18, 2023
Merged

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

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
ef1ff6f
Enable presence broadcast after validating the license
matheusbsilva137 Sep 4, 2023
863524f
Create changesets
matheusbsilva137 Sep 5, 2023
72fff5b
Merge branch 'develop' into fix/presence-broadcast-not-enabled-license
matheusbsilva137 Sep 5, 2023
6357b62
Merge branch 'develop' into fix/presence-broadcast-not-enabled-license
matheusbsilva137 Sep 5, 2023
6197449
Start hasLicense as undefined
matheusbsilva137 Sep 8, 2023
1a9db89
Toggle broadcast on every new connection
matheusbsilva137 Sep 8, 2023
cbe2e0d
Improve condition
matheusbsilva137 Sep 8, 2023
3b59013
Improve condition again
matheusbsilva137 Sep 8, 2023
3dbb8b6
Readd license check (oops)
matheusbsilva137 Sep 8, 2023
05364fc
Enable broadcast on license check
matheusbsilva137 Sep 8, 2023
3488ccc
Do not assign undefined to hasLicense
matheusbsilva137 Sep 8, 2023
2e60162
Remove unnecessary line
matheusbsilva137 Sep 8, 2023
8a11ded
Merge branch 'develop' into fix/presence-broadcast-not-enabled-license
matheusbsilva137 Sep 8, 2023
361896e
Merge branch 'develop' into fix/presence-broadcast-not-enabled-license
matheusbsilva137 Sep 12, 2023
6d49ccf
Enable presence broadcast when license is validated
matheusbsilva137 Sep 15, 2023
8d68ec5
dont check troubleshooting setting
sampaiodiego Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/tall-pumpkins-cross.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@rocket.chat/presence": patch
---

Fixed presence broadcast being disabled on server restart
14 changes: 11 additions & 3 deletions ee/packages/presence/src/Presence.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,17 @@ export class Presence extends ServiceClass implements IPresence {
}
});

this.onEvent('license.module', ({ module, valid }) => {
this.onEvent('license.module', async ({ module, valid }) => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@matheusbsilva137 matheusbsilva137 Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

if (module === 'scalability') {
this.hasLicense = valid;

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');
Copy link
Member

Choose a reason for hiding this comment

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

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.

if (presenceBroadcastDisabled?.value === false) {
await this.toggleBroadcast(true);
}
}
}
});
}
Expand All @@ -58,9 +66,9 @@ export class Presence extends ServiceClass implements IPresence {
}, 10000);

try {
this.hasLicense = await License.hasLicense('scalability');

await Settings.updateValueById('Presence_broadcast_disabled', false);

this.hasLicense = await License.hasLicense('scalability');
} catch (e: unknown) {
// ignore
}
Expand Down