fix(client-presence): disabled (failing) attendees unit tests #23419
Conversation
github-actions
bot
added
base: main
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
packages/framework/presence/src/test/presenceManager.spec.ts:83
- The word 'seassionId-2' is misspelled. It should be 'sessionId-2'.
presence = prepareConnectedPresence(runtime, "seassionId-2", "client2", clock, logger);
packages/framework/presence/src/systemWorkspace.ts:202
- The newly introduced behavior for handling stale connections using 'staleConnectionTimer' is not covered by tests.
this.staleConnectionTimer.setTimeout(() => {
Co-authored-by: Copilot <[email protected]>
| @@ -189,8 +183,32 @@ class SystemWorkspaceImpl implements PresenceStatesInternal, SystemWorkspace { | |||
| value: this.selfAttendee.sessionId, | |||
| }; | |||
|
|
|||
| this.selfAttendee.setConnectionId(clientConnectionId); | |||
| // Clear the stale connection timer when the local client reconnects | |||
There was a problem hiding this comment.
Don't have to say "when the local client reconnects" because that is what this function is for. But okay.
More important: why is the timer cleared here when it will always be set below? (Please comment)
| client.setDisconnected(); | ||
| this.events.emit("attendeeDisconnected", client); | ||
| } | ||
| this.staleConnectionClients.delete(client); |
There was a problem hiding this comment.
While this in-loop delete may work (not always the case for iterators), why not just empty the set afterwards?
| for (const client of this.staleConnectionClients) { | ||
| // Mark the client as disconnected and remove from the stale connection set | ||
| if (client.getConnectionStatus() === SessionClientStatus.Connected) { | ||
| client.setDisconnected(); | ||
| this.events.emit("attendeeDisconnected", client); |
There was a problem hiding this comment.
- why do we need the connected check? Because we don't remove them from set when they disconnect in the meantime?
- we prefer to update states to consistent point and then announce events in a group. That way anything responding to the first event and looking at state will get the final state. (This preference assumes that nothing interrupts the thread which is true for JavaScript without any yields.)
So, if we remove the status check, then the state set and events can be done in two simple loops.
| if (this.staleConnectionClients.has(attendee) && isConnected) { | ||
| // If the attendee is connected, remove them from the stale connection set | ||
| this.staleConnectionClients.delete(attendee); |
There was a problem hiding this comment.
Is it necessary to check has here? Wouldn't if (isConnected) state.delete(attendee) work?
| presence.events.on("attendeeDisconnected", (attendee) => { | ||
| disconnectedAttendees.push(attendee); | ||
| if (attendee !== presence.getMyself()) { | ||
| remoteDisconnectedAttendees.push(attendee); |
There was a problem hiding this comment.
So, we announce ourselves when disconnecting. Do we have a test for that? I don't think we do, but we should. Task for after.
Description
ADO Bug 24395
This PR enables the previously failing tests presence attendee tests as well as the new ones added in this PR: #23351
When the local attendee disconnects, we temporarily lose connectivity status for remote attendees. To fix this we mark all 'Connected' remote attendee connections as stale upon reconnect and update their status to disconnected after 30 seconds of inactivity.
This PR also fixes some spelling mistakes: "seassionId-2" to "sessionId-2"