Skip to content

Send a SSS response immediately if the config has changed and there are new results to sync#19714

Merged
erikjohnston merged 3 commits into
element-hq:developfrom
bnjbvr:bnjbvr/fix-sss-subscription-not-returning-immediately
Apr 24, 2026
Merged

Send a SSS response immediately if the config has changed and there are new results to sync#19714
erikjohnston merged 3 commits into
element-hq:developfrom
bnjbvr:bnjbvr/fix-sss-subscription-not-returning-immediately

Conversation

@bnjbvr

@bnjbvr bnjbvr commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

This fixes the bug described in #19713 (and double-checked against the SDK integration test, which now passes with this change). A sync response must be returned immediately if a room subscription configuration change caused a new non-empty response (checked with if response in the code) to be produced.

Fixes #19713.
Fixes #18844.

@bnjbvr bnjbvr requested a review from a team as a code owner April 21, 2026 12:27
@bnjbvr bnjbvr changed the title Bnjbvr/fix sss subscription not returning immediately Send a SSS response immediately if the config has changed and there are new results to sync Apr 21, 2026
@bnjbvr bnjbvr force-pushed the bnjbvr/fix-sss-subscription-not-returning-immediately branch from 074ea41 to 30117dc Compare April 21, 2026 12:29
@bnjbvr bnjbvr force-pushed the bnjbvr/fix-sss-subscription-not-returning-immediately branch from 30117dc to 4a9158e Compare April 21, 2026 12:50
This just makes it easier to follow.

@erikjohnston erikjohnston left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for finding this!

@erikjohnston erikjohnston enabled auto-merge (squash) April 24, 2026 09:41
@CLAassistant

CLAassistant commented Apr 24, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@erikjohnston erikjohnston merged commit 2691d0b into element-hq:develop Apr 24, 2026
41 checks passed
Comment thread changelog.d/19714.bugfix
@@ -0,0 +1 @@
Have SSS return a new response immediately if a room subscription have changed and produced a new response.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The PR description says this fixes #18844

But I don't see an associated test update as described in #18844 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like #19734 is trying to address this

sync_config.user.to_string(),
timeout_ms,
current_sync_callback,
from_token=now_token,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to make sure I'm following this logic, we use now_token here because we already checked for data between from_token -> now_token with the first current_sync_for_user(...) call above and found nothing. So we can save trying to lookup anything in that range and jump from now_token -> token after waiting

Feels like this deserves a comment as an optimization. Something along the lines of from_token.stream_token still being the right answer but we can save ... because ...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like #19734 is trying to address this

reivilibre added a commit that referenced this pull request May 15, 2026
… there are new results to sync (#19714)" (#19784)

Reverts: #19714

Opens: #19783

Closes: element-hq/backend-internal#242

Related: #18880 (the performance problem that is aggravated by #19714)

This reverts commit 2691d0b.

---------

Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
reivilibre added a commit that referenced this pull request May 15, 2026
… there are new results to sync (#19714)" (#19784)

Reverts: #19714

Opens: #19783

Closes: element-hq/backend-internal#242

Related: #18880 (the performance problem that is aggravated by #19714)

This reverts commit 2691d0b.

---------

Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
erikjohnston added a commit that referenced this pull request Jun 5, 2026
Follow on from #19714, where we should have had an extra comment and
test.

Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>
erikjohnston added a commit that referenced this pull request Jun 8, 2026
…s changed (#19792)

Reintroduces #19714, after being reverted in #19784.

Fix #18844
Fix #19783
Fix #18880

This PR also adds a fix so that we don't always return immediately when
using the e2ee extension.

---------

Co-authored-by: Benjamin Bouvier <benjamin@bouvier.cc>
Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>
Co-authored-by: Eric Eastwood <erice@element.io>
FrenchGithubUser pushed a commit to famedly/synapse-upstreaming that referenced this pull request Jun 12, 2026
…re new results to sync (element-hq#19714)

This fixes the bug described in element-hq#19713 (and double-checked against the
SDK integration test, which now passes with this change). A sync
response must be returned immediately if a room subscription
configuration change caused a new non-empty response (checked with `if
response` in the code) to be produced.

Fixes element-hq#19713.
Fixes element-hq#18844.

---------

Co-authored-by: Erik Johnston <erik@matrix.org>
FrenchGithubUser pushed a commit to famedly/synapse-upstreaming that referenced this pull request Jun 12, 2026
… there are new results to sync (element-hq#19714)" (element-hq#19784)

Reverts: element-hq#19714

Opens: element-hq#19783

Closes: https://github.com/element-hq/backend-internal/issues/242

Related: element-hq#18880 (the performance problem that is aggravated by element-hq#19714)

This reverts commit 2691d0b.

---------

Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

4 participants