Skip to content

Fix possible unknown state (Test that reproduces the edgecase) #4944

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.

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

Merged
merged 1 commit into from
Aug 1, 2025

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Jul 30, 2025

Fixes: element-hq/element-call#3424
This PR adds a test for a edgecase where the membership manager can reach an unknown state.

The edge case is as following:

  • restarting the delayed leave event gives a 404 (not found, the delayed event got sent by the HS)
  • We have NOT yet received that our state is missing from the sync
  • We are scheduling a new SendDelayedEvent action so we can setup a new delayed state event
  • Just before we can sent out the request we receive our missing membership over sync. Because we arleady have a SendDelayedEvent scheduled we do not need to do any special logic here. No new actions are scheduled even though our membership is missing. (which is correct)
  • When finally sending the new State event we have not cleared the UpdateExpiry action -> we will insert another one (this is the behaviour that is fixed in this PR, we will never insert another one if there is one scheduled already)

The test before this change:
Screenshot 2025-07-30 at 14 47 18

Signed-off-by: Timo K [email protected]

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

@toger5 toger5 requested a review from a team as a code owner July 30, 2025 12:53
@toger5 toger5 requested a review from BillCarsonFr July 30, 2025 12:53
@toger5 toger5 changed the title Fix possible unknown state (With reproducible test) Fix possible unknown state (Test that reproduces the edgecase) Jul 30, 2025
Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

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

Makes sense to the extent that I understand the state machine here.

@toger5 toger5 added this pull request to the merge queue Aug 1, 2025
Merged via the queue into develop with commit c2d25d9 Aug 1, 2025
53 of 57 checks passed
@toger5 toger5 deleted the toger5/fix-duplicated-UpdateExpiry-action-case branch August 1, 2025 09:31
@richvdh
Copy link
Member

richvdh commented Aug 1, 2025

Fix possible unknown state (Test that reproduces the edgecase)

@toger5 this isn't a great PR title. Unknown state in what? Which area of the js-sdk does it affect? (Also, I wouldn't call out the test specifically: it should be taken as read that most PRs contain a test.)

A good PR title helps people know what is changing in the library (note that they end up in the changelog too!) and is useful for future archaeology when we want to find things that might have affected particular bit of functionality. Could you give it an update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check if: MembershipManager has an unknown state. Actions error is still applicable
3 participants