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

SA: Improve concurrency robustness of CRL leasing transactions #8030

Merged
merged 6 commits into from
Mar 3, 2025

Conversation

aarongable
Copy link
Contributor

@aarongable aarongable commented Feb 28, 2025

In a few places within the SA, we use explicit transactions to wrap read-then-update style operations. Because we set the transaction isolation level on a per-session basis, these transactions do not in fact change their isolation level, and therefore generally remain at the default isolation level of REPEATABLE READ.

Unfortunately, we cannot resolve this simply by converting the SELECT statements into SELECT...FOR UPDATE statements: although this would fix the issue by making those queries into locking statements, it also triggers what appears to be an InnoDB bug when many transactions all attempt to select-then-insert into a table with both a primary key and a separate unique key, as the crlShards table has. This causes the integration tests in GitHub Actions, which run with an empty database and therefore use the needToInsert codepath instead of the update codepath, to consistently flake.

Instead, resolve the issue by having the UPDATE statements specify that the value of the leasedUntil column is still the same as was read by the initial SELECT. Although two crl-updaters may still attempt these transactions concurrently, the UPDATE statements will still be fully sequenced, and the latter one will fail.

Part of #8031

@aarongable aarongable requested review from a team and beautifulentropy and removed request for a team February 28, 2025 21:49
@aarongable aarongable marked this pull request as ready for review February 28, 2025 21:49
@aarongable aarongable requested a review from a team as a code owner February 28, 2025 21:49
@aarongable aarongable changed the title Use SELECT ... FOR UPDATE for reads within transactions SA: Improve concurrency robustness of CRL leasing transactions Feb 28, 2025
Copy link
Member

@beautifulentropy beautifulentropy left a comment

Choose a reason for hiding this comment

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

This looks like a good fix. It might be worthwhile to add a test that explicitly checks for concurrent shard leasing collisions, but I won't hold approval for it.

@aarongable aarongable requested review from jsha, jprenken and a team March 3, 2025 21:44
@aarongable aarongable merged commit 28b49a8 into main Mar 3, 2025
19 checks passed
@aarongable aarongable deleted the select-for-update branch March 3, 2025 23:29
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.

4 participants