Repository navigation
Remove row locks in the CBE applet that protect no invariant #916
Description
User Story
As a platform operator running many courses on one Open edX instance, I want the database locks in the competency code to be used only where they prevent data from getting corrupted. Then authoring requests stay fast, and the criteria data stays correct.
Description
The competency-based education (CBE) applet takes database row locks in several of its write paths via select_for_update() calls. But not all of them are strictly necessary: if they don't protect a real case of data corruption, or if the risk of deadlocks or complicated concurrency problem outweighs the benefits, they should be removed. Where they stay, a comment should justify them.
Acceptance Criteria
Scenarios 1 to 6 are verifiable against MySQL, where row locks take effect. SQLite, which the default test settings use, ignores them.
Scenario: A lock stays only when it protects an invariant the schema cannot enforce
Given a row lock in the CBE applet
When a reviewer examines it
Then the reviewer can name the invariant it protects
And can show that no database constraint on MySQL expresses that invariant
And the lock is removed if either of those cannot be shown
Scenario: A lock stays only when a concurrent writer can violate its invariant today
Given a row lock in the CBE applet that protects a stated invariant
When a reviewer lists the writers in the codebase that can change the rows the invariant covers
Then the lock stays only if at least two of those writers, or two concurrent requests through one writer, can violate the invariant when the lock is absent
And a lock that defends against a writer that does not exist in the codebase is removed
Scenario: A lock that only prevents a harmless duplicate is removed
Given a row lock whose only benefit is that a repeated identical request waits and then does nothing
When the lock is removed and two identical requests run at the same time
Then both requests complete
And the stored criteria data is correct and consistent
And the only difference is a redundant history entry
Scenario: A lock covers only the rows its invariant spans
Given a competency used by two different courses
When an author in the first course submits a request that holds a lock
Then an author in the second course is blocked by that request only if the invariant being protected spans both courses
Scenario: Concurrent requests keep the criteria invariants
Given two requests that run at the same time, each creating the first criterion for the same competency
When both complete
Then the competency has exactly one root criteria group
And no object has two criteria for the same competency
And no course holds a criterion for a competency together with a criterion for one of its ancestor or descendant competencies
Scenario: Overlapping concurrent requests do not deadlock
Given two requests that run at the same time and update overlapping sets of criteria listed in opposite order
When both run to completion
Then neither fails with a deadlock
And the final state equals the state after running them one after the other
And, for every pair of locks that any transaction in the applet takes, all transactions take them in the same relative order
Scenario: A single request gets the same response after a lock is removed
Given any request to a CBE endpoint that previously completed or was rejected
When the same request is sent alone after the change
Then the response status, the response body, and the order of items in the body are unchanged
And a batch request still succeeds or fails as a unit
Scenario: Concurrency scenarios are tested
Given a lock that is kept or removed
Then there are tests covering all important concurrency scenarios
And they are MySQL-only tests that asserts outcomes rather than the locks taken
Reviewer confirms at code review time:
- Every row lock that remains has a comment naming the invariant it protects and its position in the lock order relative to any other lock the same transaction can take.
- For every row lock that was kept, a comment justifies the reasoning.
- Wherever a removed lock had fixed the order in which rows are written, that order is now stated explicitly in the code.
- MySQL-only Tests assert the behavior in the most important concurrency scenarios.
Activity