P20-1796: Fix LTI course duplication #70711
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ba3d2ab2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
artem-vavilov
force-pushed
the
P20-1796/lti-course-duplicates-fix
branch
3 times, most recently
from
67e956e to
24a0117
Compare
…ncurrent requests
artem-vavilov
force-pushed
the
P20-1796/lti-course-duplicates-fix
branch
from
24a0117 to
ad4b3fa
Compare
| # the ActiveRecord level uniqueness check and creating LtiCourse duplicates. | ||
| # TODO(P20-1796): Remove the lock once a DB-level unique constraint | ||
| # on (lti_integration_id, context_id) prevents duplicates. | ||
| lti_integration.with_lock do |
There was a problem hiding this comment.
Will locking on the lti_integration here actually prevent concurrent requests from modifying the associated lit_course? I'm not super familiar with this method, but AI suggests that it would only prevent modifying the specific lti_integration, which will would never actually change.
There was a problem hiding this comment.
It acts as a serialization point because all concurrent requests must first acquire a lock on the same LtiIntegration record, so only one request at a time can reach the LtiCourse creation logic
There was a problem hiding this comment.
We can confirm this with unit tests:
class LtiCourseDuplicationTest < ActiveSupport::TestCase
# Disable transactional tests so created DB records are visible to new connections
self.use_transactional_tests = false
self.pre_loaded_fixtures = false
subject(:create_lti_course) {create(:lti_course, lti_integration:, lti_deployment:, context_id:)}
let!(:lti_integration) {create(:lti_integration)}
let!(:lti_deployment) {create(:lti_deployment, lti_integration:)}
let(:context_id) {'shared-context-id'}
after do
LtiCourse.unscoped.delete_all
LtiDeployment.delete_all
LtiIntegration.delete_all
end
it 'creates duplicate LtiCourse records under concurrent requests' do
assert_difference 'LtiCourse.count', 2 do
Parallel.in_threads(2) do
create_lti_course
end
end
end
it 'creates only one LtiCourse record under concurrent requests when locked' do
assert_difference 'LtiCourse.count', 1 do
Parallel.in_threads(2) do
LtiIntegration.find(lti_integration.id).with_lock do
create_lti_course
end
end
end
end
end➜ dashboard git:(P20-1796/lti-course-duplicates-fix) ✗ rails test test/lti_course_duplication_test.rb
Running via Spring preloader in process 86711
Started with run options --seed 4465
2/2: [======================================================================] 100% Time: 00:00:00, Time: 00:00:00
Finished in 0.08150s
2 tests, 2 assertions, 0 failures, 0 errors, 0 skipsThere was a problem hiding this comment.
I see, thanks for clarifying. One potential issue I see is that Schoology, and the way they are configured, uses a single LtiIntegration for all of Schoology, which equates to thousands of users? I'd have to lookup the exact numbers. I know that this lock only wraps the find_or_create_by method invocation, but because this happens on an LTI launch (launching from Schoology into Code.org), I'm worried of potential user experience impact, if there were hundreds of concurrent launches.
There was a problem hiding this comment.
Does Schoology also share the same one LtiDeployment in that configuration?
There was a problem hiding this comment.
Nope, that is unique per "instance" or school/district schoology account. That might be better to lock on?
There was a problem hiding this comment.
This lock is a temporary solution to ensure that no new duplicates are created until a database uniqueness constraint is added and replaces it
artem-vavilov
force-pushed
the
P20-1796/lti-course-duplicates-fix
branch
from
4944265 to
f2e6a8e
Compare
nicklathe
left a comment
nicklathe
left a comment
There was a problem hiding this comment.
Looks good, thanks for doing this!
3 participants
Under concurrent requests to
LtiV1Controller#sync_course, the application can create more than oneLtiCourserecord for the samelti_integration_idandcontext_id.The current flow relies only on ActiveRecord-level uniqueness validations and "find then create" logic (
validates_uniqueness_of). This is not concurrency safe because it is not atomic across requests: when two requests run at the same time, both can read "no matching row exists" before either inserts or commits. Without a database-level unique constraint, the database will accept both inserts, resulting in duplicateLtiCourserecords.This is most likely triggered by multiple Sync requests being sent close together, for example, rapid repeated clicks before the UI fully disables the button, or retries from the browser or LMS, or a user clicking Sync in multiple tabs.
Fix steps
Mitigate duplicate creation
Add a temporary pessimistic (transactional) lock around the
LtiCourse"find or create" logic, so concurrent sync requests for the same integration cannot create duplicates. This eliminates the race condition at the application level.Data cleanup
Identify existing duplicates for
(lti_integration_id, context_id)and keep a single canonical record, typically the most recently created or last updated one, then soft delete or remove the rest. If duplicates have divergentresource_link_idvalues, decide which value is correct and migrate it to the canonical row before deleting the others.Database enforcement
Add a database unique constraint to prevent duplicate active
LtiCourserecords for(lti_integration_id, context_id, deleted_at), then replace the lock with logic that rescuesActiveRecord::RecordNotUniqueand loads the existing row, making the operation idempotent under concurrency.Frontend hardening
Reduce the chance of duplicate requests by disabling the Sync button immediately on click, before waiting for a rerender.
Links