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

Fix bug that could cause a /sync to tightloop with sqlite after restart#16540

Merged
erikjohnston merged 2 commits intodevelopfrom
erikj/sync_sqlite_loop
Oct 23, 2023
Merged

Fix bug that could cause a /sync to tightloop with sqlite after restart#16540
erikjohnston merged 2 commits intodevelopfrom
erikj/sync_sqlite_loop

Conversation

@erikjohnston
Copy link
Member

@erikjohnston erikjohnston commented Oct 23, 2023

This could happen if the last rows in the account data stream were inserted into account_data. After a restart the max account ID would be calculated without looking at the account_data table, and so have an old ID.

DMRobertson
DMRobertson reviewed Oct 23, 2023
View reviewed changes
synapse/storage/databases/main/account_data.py
Comment on lines +97 to +100
extra_tables=[
("account_data", "stream_id"),
("room_tags_revisions", "stream_id"),
],
Copy link
Contributor

@DMRobertson DMRobertson Oct 23, 2023

Choose a reason for hiding this comment

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

Should this include room_account_data too, like the postgres ID generator?

Copy link
Member Author

@erikjohnston erikjohnston Oct 23, 2023

Choose a reason for hiding this comment

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

That's included above. (These are extra_tables= rather than tables= that MultiWriterIdGenerator uses)

Copy link
Member

@clokep clokep Oct 23, 2023

Choose a reason for hiding this comment

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

(This feels like something a lint could check 😢 )

@DMRobertson
Copy link
Contributor

DMRobertson commented Oct 23, 2023

Fixes #15824?

@erikjohnston erikjohnston marked this pull request as ready for review October 23, 2023 13:05
@erikjohnston erikjohnston requested a review from a team as a code owner October 23, 2023 13:05
@erikjohnston
Copy link
Member Author

erikjohnston commented Oct 23, 2023

Fixes #15824?

Quite possibly

@erikjohnston erikjohnston enabled auto-merge (squash) October 23, 2023 13:15
@erikjohnston erikjohnston merged commit 3bc23cc into develop Oct 23, 2023
@erikjohnston erikjohnston deleted the erikj/sync_sqlite_loop branch October 23, 2023 13:39
@erikjohnston erikjohnston mentioned this pull request Oct 23, 2023
Open
@thebalaa
Copy link
Contributor

thebalaa commented Oct 23, 2023 •
edited
Loading

Didn't fix #15824

Still seeing with the following development based container:

 ...
                "gitsha1": "3df70aa80001e05b0bbe69fd3328f11aceaab4aa",
                "org.homeserver": "true",
                "org.opencontainers.image.documentation": "https://github.com/matrix-org/synapse/blob/master/docker/README.md",
                "org.opencontainers.image.licenses": "Apache-2.0",
                "org.opencontainers.image.source": "https://github.com/matrix-org/synapse.git",
                "org.opencontainers.image.url": "https://matrix.org/docs/projects/server/synapse",
                "org.opencontainers.image.version": "1.95.0rc1"

Note the below sync query returns a response that does not advance the next_batch:

http://localhost:8008/_matrix/client/r0/sync?filter=0&timeout=30000&since=s93_7_0_1_5_1_1_11_0_1

{
    "next_batch": "s93_7_0_1_5_1_1_11_0_1",
    "device_lists": {
        "changed": [
            "@admin:localhost"
        ]
    },
    "device_one_time_keys_count": {
        "signed_curve25519": 50
    },
    "org.matrix.msc2732.device_unused_fallback_key_types": [
        "signed_curve25519"
    ],
    "device_unused_fallback_key_types": [
        "signed_curve25519"
    ]
}

Restarting synapse fixes the tightloop temporarily but it returns within a few minutes.

Our reproduction steps:
We have a custom matrix-nio based client that is syncing, we then login via lement with the same matrix ID and within a few minutes it will start tightlooping.

SQLite database docker / docker compose deployment

@erikjohnston
Copy link
Member Author

erikjohnston commented Oct 23, 2023

Hmm, I wonder if we have a similar problem with device lists then

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@clokep clokep clokep left review comments

@DMRobertson DMRobertson DMRobertson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@erikjohnston @DMRobertson @thebalaa @clokep