Skip to content
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

[stable26] fix(sync): prevent race condition by relying on autoincrement #4972

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

backportbot-nextcloud[bot]
Copy link

Backport of #4970

Copy link

cypress bot commented Nov 8, 2023

1 failed test on run #12342 ↗︎

1 149 2 0 Flakiness 0

Details:

[stable26] fix(sync): prevent race condition by relying on autoincrement
Project: Text Commit: 4b09933968
Status: Failed Duration: 05:02 💡
Started: Nov 9, 2023 9:23 AM Ended: Nov 9, 2023 9:28 AM
Failed  cypress/e2e/share.spec.js • 1 failed test

View
Output

Test Artifacts
Open test.md in viewer > Share a file with download disabled shows an error Screenshots

Review all test suite changes for PR #4972 ↗︎

@juliushaertl
Copy link
Member

/compile

@mejo- mejo- changed the title [stable26] [stable27] fix(sync): prevent race condition by relying on autoincrement [stable26] fix(sync): prevent race condition by relying on autoincrement Nov 9, 2023
max-nextcloud and others added 2 commits November 9, 2023 10:18
Prevent a possible race condition when two clients add steps at the same time.

See #4600.

Rely on the autoincrementing id in order to provide a canonical order
that steps can be retrieved in.

When two clients push steps at the same time
the entries receive destinct ids that increment.
So if another client fetches steps in between
it will see the smaller id as the version of the fetched step
and fetch the other step later on.

Transition:
In the future we can drop the version column entirely
but currently there are still steps stored in the database
that make use of the old column.
So we need to transition away from that.

In order to find entries that are newer than version x
we select those that have both a version and an id larger than x.

Entries of the new format are newer than any entry of the old format.
So we set their version to the largest possible value.
This way they will always fulfill the version condition
and the condition on the id is more strict and therefore effective.

For the old format the version will be smaller than the id
as it's incremented per document while the id is unique accross documents.
Therefore the version condition is the more strict one and effective.

The only scenario where the version might be larger than the id
would be if there's very few documents in the database
and they have had a lot of steps stored in single database entries.

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Jonas <jonas@freesources.org>
The value used before (largest possible MySQL BIGINT value) was too big
for PHP int. Since we still support 32-bit platforms on Nextcloud, let's
stick to the largest possible 32-bit PHP integer value.

Besides, setting the value as default for `Step::version` doesn't work
as `QBMapper->insert()` doesn't recognize the `version` field as changed
in that case. So let's default to `0` again and set it using
`Step->setVersion()` later.

Signed-off-by: Jonas <jonas@freesources.org>
@mejo-
Copy link
Member

mejo- commented Nov 9, 2023

/compile

@juliushaertl
Copy link
Member

🙈 No need to compile as it is just a backend change

@juliushaertl juliushaertl merged commit d70f71d into stable26 Nov 9, 2023
39 of 43 checks passed
@juliushaertl juliushaertl deleted the backport/4970/stable26 branch November 9, 2023 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants