Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

presence_stream records get deleted #9962

Closed
thermaq opened this issue May 11, 2021 · 2 comments
Closed

presence_stream records get deleted #9962

thermaq opened this issue May 11, 2021 · 2 comments
Labels
S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@thermaq
Copy link
Contributor

thermaq commented May 11, 2021

Description

This may be me not understanding how presence_stream table is utilized, but I noticed dissapearing rows from presence_stream database table. More tangible effect may be that only one online user has a green dot showing online status in element and the rest doesn't get a dot at all.

My explanation:

  • few people are online
  • server wants to refresh their states invoking _update_presence_txn method in PresenceStore
  • new online and offline states get inserted as rows to presence_stream
  • we iterate through users we refreshed and delete old rows. The query in pseudocode look like delete rows from presence_stream where stream_id < $stream_id and user_id is $user_id. user_id is iterated over, but the stream_id is a remnant from previous loop
  • so when we left previous loop the stream_id can be the maximum stream_id we inserted, causing every record we inserted to get deleted except the one with highest stream_id

I guess my question is: is it a bug or am I missing something? In case I'm right I have a pull request ready.

Steps to reproduce

Turn on presence and look at presence_stream table table for few minutes with multiple online clients.

Version information

Every version I used so far, develop too.
@erikjohnston erikjohnston added S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. labels May 13, 2021
@anoadragon453
Copy link
Member

Yep, I believe you're correct! I wrote a quick test for this on a branch at 258bf0e and the results are that we only end up with one presence update in the database, rather than the 5 we desire.

We'd very happily accept a pull request that fixes the bug - thanks!

@anoadragon453 anoadragon453 removed their assignment May 13, 2021
@thermaq thermaq mentioned this issue May 19, 2021
Merged
@anoadragon453 anoadragon453 mentioned this issue May 21, 2021
Merged
@anoadragon453
Copy link
Member

#10033 adds a regression test for the fix.

anoadragon453 added a commit that referenced this issue May 21, 2021
@anoadragon453
Add a test for update_presence (#10033)
21bd230 
#9962 uncovered that we accidentally removed all but one of the presence updates that we store in the database when persisting multiple updates. This could cause users' presence state to be stale.

The bug was fixed in #10014, and this PR just adds a test that failed on the old code, and was used to initially verify the bug.

The test attempts to insert some presence into the database in a batch using `PresenceStore.update_presence`, and then simply pulls it out again.
@anoadragon453 anoadragon453 mentioned this issue Jun 1, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@anoadragon453 @erikjohnston @thermaq