Don't need all users in requests from Talk

[fancycode]

• BackendRoomInviteRequest
• BackendRoomDisinviteRequest
• BackendRoomUpdateRequest
• BackendRoomDeleteRequest
• BackendRoomInCallRequest
• BackendRoomParticipantsRequest

Resolves #1173

[codecov]

Codecov Report

❌ Patch coverage is 72.80000% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.01%. Comparing base (6046e47) to head (ad62241).
⚠️ Report is 13 commits behind head on master.

Files with missing lines	Patch %	Lines
server/backend_server.go	60.41%	8 Missing and 11 partials ⚠️
server/room.go	73.52%	7 Missing and 2 partials ⚠️
api/signaling.go	75.00%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1175      +/-   ##
==========================================
+ Coverage   62.94%   63.01%   +0.07%     
==========================================
  Files         129      129              
  Lines       18888    18915      +27     
==========================================
+ Hits        11889    11920      +31     
+ Misses       5894     5891       -3     
+ Partials     1105     1104       -1     

Flag	Coverage Δ
go-1.25	63.00% <72.80%> (+0.05%)	⬆️
go-1.26	62.95% <72.80%> (+0.02%)	⬆️

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

Components	Coverage Δ
root	∅ <ø> (∅)
api	81.48% <75.00%> (-0.23%)	⬇️
async	87.47% <ø> (ø)
client	70.38% <ø> (ø)
cmd/client	6.34% <ø> (ø)
cmd/proxy	60.59% <ø> (ø)
cmd/server	0.00% <ø> (ø)
config	82.14% <ø> (ø)
container	93.79% <ø> (ø)
dns	90.82% <ø> (ø)
etcd	81.96% <ø> (ø)
geoip	68.09% <ø> (ø)
grpc	75.56% <ø> (ø)
internal	96.12% <ø> (ø)
log	94.44% <ø> (ø)
metrics	90.00% <ø> (ø)
mock	∅ <ø> (∅)
nats	73.55% <ø> (ø)
pool	82.10% <ø> (+0.19%)	⬆️
proxy	81.35% <ø> (ø)
security	80.62% <ø> (ø)
server	72.32% <72.27%> (+0.23%)	⬆️
session	78.57% <ø> (ø)
sfu	43.98% <ø> (-0.06%)	⬇️
talk	68.99% <ø> (ø)
test	86.04% <ø> (ø)

Files with missing lines	Coverage Δ
server/federation.go	75.95% <100.00%> (ø)
server/hub.go	68.27% <100.00%> (+0.43%)	⬆️
talk/api.go	25.67% <ø> (ø)
api/signaling.go	74.01% <75.00%> (+0.05%)	⬆️
server/room.go	82.39% <73.52%> (-0.11%)	⬇️
server/backend_server.go	63.91% <60.41%> (-0.30%)	⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fancycode]

@danxuliu This implements the receiving part (i.e. Talk -> Signaling Server).

A couple of open issues:

• Now users no longer get notified if another user is invited/disinvited to one of their rooms they are not in currently (BackendRoomInviteRequest / BackendRoomDisinviteRequest) - probably fine?
• Users only get notified about updates of rooms they are currently in, e.g. change of properties (BackendRoomUpdateRequest) - will be deferrred for rooms a user is not currently in until the next periodic update of the room list (this still exists, right?)
• Same for deletions of rooms a user is not currently in (BackendRoomDeleteRequest) - will also be deferred until the next periodic update of the room list
• Only users in a room get notified about changes to other participants inCall flags (BackendRoomInCallRequest) - this is probably fine.
• Same for changes to properties for other participants (BackendRoomParticipantsRequest) - this also is probably fine.

Also the signaling server still includes all participants in the updates events it sends out to connected sessions triggered by BackendRoomInCallRequest / BackendRoomParticipantsRequest. We should get rid of this too, if all clients (web/mobile/desktop) support receiving only changed participants.

[fancycode]

@nickvergessen FYI, you might also have some ideas on the open issues.

[nickvergessen]

@fancycode to not break, the Talk code would need to know whether the HPB has this feature. Can you add a new feature header?

[fancycode]

@fancycode to not break, the Talk code would need to know whether the HPB has this feature. Can you add a new feature header?

Rebased and added as changed-users.

If we add more features in the future that are only relevant for the Talk backend, I should split them up so the clients don't receive feature ids they don't need.

[danxuliu]

A couple of issues:

• If in a conversation there is only one active participant and that participant starts a call, the client receives a participants->update signaling message with the participant duplicated in users, although in one case with inCall: 0 and in the other inCall: 7
• The signaling server now keeps track of the participants to include them in participants->update when they are in a call. However, participants->update will include any active participant whose inCall changed at some point, even after they left the call (when inCall becomes 0 it should be included, but it should not be necessary to keep including them in further messages if inCall is still 0).
  • The participants will be included also if the participants->update is unrelated to a call. For example, if the permissions of a participant change the signaling message will include not only the participant whose permissions changed, but any other participant that are or were at some point in a call.
  • These extra/unneeded participants should™ not be problematic, but it caught my eye and I wondered if it was intended/expected.

[nickvergessen]

Talk side is merged, and toggles behaviour based on the capability.
I guess the new HPB also still handles the old behaviour, as we did only backport to stable34.

But then this would be good to get merged and release (or test packaged) so we can put it live? :)

[fancycode]

A couple of issues:

• If in a conversation there is only one active participant and that participant starts a call, the client receives a participants->update signaling message with the participant duplicated in users, although in one case with inCall: 0 and in the other inCall: 7

I saw this once yesterday while testing but can't reproduce any more. A user should only be included once and only with the latest properties. Does this happen every time for you?

• The signaling server now keeps track of the participants to include them in participants->update when they are in a call. However, participants->update will include any active participant whose inCall changed at some point, even after they left the call (when inCall becomes 0 it should be included, but it should not be necessary to keep including them in further messages if inCall is still 0).
  • The participants will be included also if the participants->update is unrelated to a call. For example, if the permissions of a participant change the signaling message will include not only the participant whose permissions changed, but any other participant that are or were at some point in a call.
  • These extra/unneeded participants should™ not be problematic, but it caught my eye and I wondered if it was intended/expected.

This is from my comment above, for backwards compatibility:

Also the signaling server still includes all participants in the updates events it sends out to connected sessions triggered by BackendRoomInCallRequest / BackendRoomParticipantsRequest. We should get rid of this too, if all clients (web/mobile/desktop) support receiving only changed participants.

[danxuliu]

A couple of issues:

• If in a conversation there is only one active participant and that participant starts a call, the client receives a participants->update signaling message with the participant duplicated in users, although in one case with inCall: 0 and in the other inCall: 7

I saw this once yesterday while testing but can't reproduce any more. A user should only be included once and only with the latest properties. Does this happen every time for you?

Ah, it happens when starting a call, leaving the call, reloading the conversation and starting a call again. So the duplicated user with inCall: 0 is actually the previous session.

• The signaling server now keeps track of the participants to include them in participants->update when they are in a call. However, participants->update will include any active participant whose inCall changed at some point, even after they left the call (when inCall becomes 0 it should be included, but it should not be necessary to keep including them in further messages if inCall is still 0).

  • The participants will be included also if the participants->update is unrelated to a call. For example, if the permissions of a participant change the signaling message will include not only the participant whose permissions changed, but any other participant that are or were at some point in a call.
  • These extra/unneeded participants should™ not be problematic, but it caught my eye and I wondered if it was intended/expected.

This is from my comment above, for backwards compatibility:

Also the signaling server still includes all participants in the updates events it sends out to connected sessions triggered by BackendRoomInCallRequest / BackendRoomParticipantsRequest. We should get rid of this too, if all clients (web/mobile/desktop) support receiving only changed participants.

What I had understood (and expected) from that comment was that the signaling server would keep track of the participants in a conversation and send them in participants -> update, even if they are no longer sent by the Nextcloud server. So if you modify the permissions of a participant then the participants -> update signaling message still includes all the (active, see Previously existing issues in nextcloud/spreed#16759) participants in the conversation.

However, the signaling server only seems to keep track of the participants that were active in a call. So in the mentioned example participants -> update includes the participant whose permissions changed and all the participants that, at some point, were active in a call (and are still active in the conversation). If the permissions of another participant are modified then participants -> update includes this new participant, but not the previous one, and all the participants that at some point were active in a call. That behaviour is a bit strange, so I was not sure if it was the expected one :-)

Similarly, when the Nextcloud server provided the changed participants in a call, the participants -> update signaling messages sent by the signaling server included the participants currently active in the call, including the participants that just left. However, it did not include participants that, at some point, were in the call, as it happens now.

As mentioned before those extra participants should not be problematic but, at the same time, they should not be needed either. Having said that, if that behaviour is intended/expected, then no problem 👍

[fancycode]

A couple of issues:

• If in a conversation there is only one active participant and that participant starts a call, the client receives a participants->update signaling message with the participant duplicated in users, although in one case with inCall: 0 and in the other inCall: 7

I saw this once yesterday while testing but can't reproduce any more. A user should only be included once and only with the latest properties. Does this happen every time for you?

Ah, it happens when starting a call, leaving the call, reloading the conversation and starting a call again. So the duplicated user with inCall: 0 is actually the previous session.

Still can't reproduce using these steps. When the conversation is reloaded (assuming a reload in the browser), the session is removed and the previous user entry gets removed:
https://github.com/strukturag/nextcloud-spreed-signaling/pull/1175/files#diff-917a377d5c8189edf91faf946df931fb415e8d74be6897e4d52c478efa73e2daR577

Even if I manually delay the backend request from Talk to set inCall: 0 for the previous session, so that it arrives after the new session has already reconnected, the signaling server still doesn't send a participants -> update for the previous session.

• The signaling server now keeps track of the participants to include them in participants->update when they are in a call. However, participants->update will include any active participant whose inCall changed at some point, even after they left the call (when inCall becomes 0 it should be included, but it should not be necessary to keep including them in further messages if inCall is still 0).

  • The participants will be included also if the participants->update is unrelated to a call. For example, if the permissions of a participant change the signaling message will include not only the participant whose permissions changed, but any other participant that are or were at some point in a call.
  • These extra/unneeded participants should™ not be problematic, but it caught my eye and I wondered if it was intended/expected.

This is from my comment above, for backwards compatibility:

Also the signaling server still includes all participants in the updates events it sends out to connected sessions triggered by BackendRoomInCallRequest / BackendRoomParticipantsRequest. We should get rid of this too, if all clients (web/mobile/desktop) support receiving only changed participants.

What I had understood (and expected) from that comment was that the signaling server would keep track of the participants in a conversation and send them in participants -> update, even if they are no longer sent by the Nextcloud server. So if you modify the permissions of a participant then the participants -> update signaling message still includes all the (active, see Previously existing issues in nextcloud/spreed#16759) participants in the conversation.

However, the signaling server only seems to keep track of the participants that were active in a call. So in the mentioned example participants -> update includes the participant whose permissions changed and all the participants that, at some point, were active in a call (and are still active in the conversation). If the permissions of another participant are modified then participants -> update includes this new participant, but not the previous one, and all the participants that at some point were active in a call. That behaviour is a bit strange, so I was not sure if it was the expected one :-)

So you mean, the participants -> update event only contains information about connected users that had their permissions changed but did not join the call if triggered by a backend request from Talk, but not in participants -> update events generated at a later time? I will look into this.

Similarly, when the Nextcloud server provided the changed participants in a call, the participants -> update signaling messages sent by the signaling server included the participants currently active in the call, including the participants that just left. However, it did not include participants that, at some point, were in the call, as it happens now.

As mentioned before those extra participants should not be problematic but, at the same time, they should not be needed either. Having said that, if that behaviour is intended/expected, then no problem 👍

As commented above, user information for sessions that were deleted should not be included in future participants -> update events because their entry is removed once the session is removed from the room.