Fix race in conditional fillup - #31
Open
julik wants to merge 3 commits into
Open
Conversation
* Conditional fillup could overflow the bucket under certain conditions and under elevated load. * Hide the AR connection behind a shim, as otherwise there is too much management of it all. ActiveRecord connection management can be tricky, and we don't need it as such - we need to _integrate_ with it well. Also, all of our queries are .uncached. We do still want the queries to go through ActiveRecord adapters, because there is useful instrumentation in their `execute` which some folks are using * Relocate sanitization into the shim as well, for future work
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Under heavy load Pecorino is susceptible to a race condition where two things can happen:
NULL, leading to a failure in theUPDATEUPDATEmay indicate lower bucket level than it actually is.In Postgres, a CTE used with an
INSERT .. ON CONFLICT UPDATEis not guaranteed to be atomic with theUPDATEitself. This means, that if there is another fillup between the execution of the CTE (theWITH (SELECT level FROM ...)and theUPDATE- even if they are within the same statement - the fillup can get accepted. This makes Pecorino unsafe under high load when SQL-based rate limiting is used with PostgreSQL (the SQLite adapter will lock the entire database during update so it is much less of an issue there). Lesson from that is that a CTE used with an UPDATE is not atomic with that UPDATE, contrary to what I assumed when I designed it this way.This also means that Pecorino needs proper row-level locking and a transaction around the fillups if the fillup may be rejected - as a formal way to block the concurrent fillups from taking place.
I have also updated the test config / a few other bits and bobs which were in the previous PRs.