From 961ed68c653cc06114f8bfb815515d62b142c8e8 Mon Sep 17 00:00:00 2001 From: cocomarine Date: Tue, 18 Aug 2026 16:44:41 +0100 Subject: [PATCH 1/6] Serialise school creation per creator to close race --- lib/concepts/school/operations/create.rb | 12 ++++++++ spec/concepts/school/create_spec.rb | 36 ++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index df1ff5ad6..7256d31aa 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -2,12 +2,19 @@ class School class Create + LOCK_NAMESPACE = Zlib.crc32(name) + class << self def call(school_params:, creator_id:, token:) response = OperationResponse.new response[:school] = build_school(school_params.merge!(creator_id:)) School.transaction do + # Serialise concurrent creates by the same creator. + # The loser blocks here until the winner's transaction commits, + # so its uniqueness validation sees the winner's school + # rather than hitting the partial unique index on creator_id. + acquire_advisory_lock_for_creator(creator_id) response[:school].save! SchoolOnboardingService.new(response[:school]).onboard(token:) @@ -26,6 +33,11 @@ def call(school_params:, creator_id:, token:) private + def acquire_advisory_lock_for_creator(creator_id) + lock_key = Zlib.crc32("#{School::Create::LOCK_NAMESPACE}:#{creator_id}") + School.connection.execute("SELECT pg_advisory_xact_lock(#{lock_key})") + end + def failure(response, error) response[:error] = response[:school].errors.presence || [error.message] response[:error_types] = response[:school].errors.details diff --git a/spec/concepts/school/create_spec.rb b/spec/concepts/school/create_spec.rb index c924a391f..e0d6ac839 100644 --- a/spec/concepts/school/create_spec.rb +++ b/spec/concepts/school/create_spec.rb @@ -27,6 +27,13 @@ allow(ProfileApiClient).to receive(:create_school).and_return(true) end + it 'acquires an advisory lock keyed on the creator' do + allow(School.connection).to receive(:execute).and_call_original + described_class.call(school_params:, creator_id:, token:) + lock_key = Zlib.crc32("#{School::Create::LOCK_NAMESPACE}:#{creator_id}") + expect(School.connection).to have_received(:execute).with("SELECT pg_advisory_xact_lock(#{lock_key})") + end + it 'returns a successful operation response' do response = described_class.call(school_params:, creator_id:, token:) expect(response.success?).to be(true) @@ -51,6 +58,35 @@ expect(response[:school].creator_id).to eq(creator_id) end + context 'when the creator already has an active school' do + # The advisory lock serialises concurrent requests: the loser only validates + # once the winner has committed, so it sees the winner's school. + before do + allow(Sentry).to receive(:capture_exception) + create(:school, creator_id:, reference: '999999') + end + + it 'does not create a second school' do + expect { described_class.call(school_params:, creator_id:, token:) }.not_to change(School, :count) + end + + it 'returns a failed operation response' do + response = described_class.call(school_params:, creator_id:, token:) + expect(response.failure?).to be(true) + end + + it 'returns a validation error rather than a database uniqueness violation' do + response = described_class.call(school_params:, creator_id:, token:) + expect(response[:error_types][:creator_id]).to eq([{ error: :taken, value: creator_id }]) + end + + it 'does not onboard the second school' do + allow(SchoolOnboardingService).to receive(:new) + described_class.call(school_params:, creator_id:, token:) + expect(SchoolOnboardingService).not_to have_received(:new) + end + end + context 'when creation fails' do let(:school_params) { {} } From e2e20d24883ae9ed7233fca617d95c0eab4c8b5c Mon Sep 17 00:00:00 2001 From: cocomarine Date: Tue, 18 Aug 2026 17:16:49 +0100 Subject: [PATCH 2/6] do not capture the errors in Sentry when creator_id is taken --- lib/concepts/school/operations/create.rb | 5 +++++ spec/concepts/school/create_spec.rb | 6 ++++++ 2 files changed, 11 insertions(+) diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index 7256d31aa..8363caafe 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -26,6 +26,11 @@ def call(school_params:, creator_id:, token:) # TODO: consider returning a separate error here to distinguish from other errors and return 401 from the API, not 422 Rails.logger.warn { "Failed to onboard school #{response[:school].id}: user is unauthorized" } failure(response, e) + rescue ActiveRecord::RecordInvalid => e + # A double submit loses the advisory lock race and fails the creator_id + # uniqueness validation as expected, so keep it out of Sentry. + Sentry.capture_exception(e) unless response[:school].errors.of_kind?(:creator_id, :taken) + failure(response, e) rescue StandardError => e Sentry.capture_exception(e) failure(response, e) diff --git a/spec/concepts/school/create_spec.rb b/spec/concepts/school/create_spec.rb index e0d6ac839..4074187bb 100644 --- a/spec/concepts/school/create_spec.rb +++ b/spec/concepts/school/create_spec.rb @@ -85,6 +85,12 @@ described_class.call(school_params:, creator_id:, token:) expect(SchoolOnboardingService).not_to have_received(:new) end + + it 'does not capture the error in Sentry' do + allow(Sentry).to receive(:capture_exception) + described_class.call(school_params:, creator_id:, token:) + expect(Sentry).not_to have_received(:capture_exception) + end end context 'when creation fails' do From 88dd3fa6911d9f8edc3526909c01edb8c223d260 Mon Sep 17 00:00:00 2001 From: cocomarine Date: Thu, 20 Aug 2026 08:54:14 +0100 Subject: [PATCH 3/6] check response is initialised --- lib/concepts/school/operations/create.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index 8363caafe..c444cbead 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -29,7 +29,7 @@ def call(school_params:, creator_id:, token:) rescue ActiveRecord::RecordInvalid => e # A double submit loses the advisory lock race and fails the creator_id # uniqueness validation as expected, so keep it out of Sentry. - Sentry.capture_exception(e) unless response[:school].errors.of_kind?(:creator_id, :taken) + Sentry.capture_exception(e) unless response[:school]&.errors&.of_kind?(:creator_id, :taken) failure(response, e) rescue StandardError => e Sentry.capture_exception(e) From fa17d015fa775d09d0b181c79136c2317d0cf958 Mon Sep 17 00:00:00 2001 From: cocomarine Date: Thu, 20 Aug 2026 08:58:47 +0100 Subject: [PATCH 4/6] add more checks to response --- lib/concepts/school/operations/create.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index c444cbead..119753f15 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -24,7 +24,7 @@ def call(school_params:, creator_id:, token:) rescue ProfileApiClient::UnauthorizedError => e # Do not log noise to sentry. # TODO: consider returning a separate error here to distinguish from other errors and return 401 from the API, not 422 - Rails.logger.warn { "Failed to onboard school #{response[:school].id}: user is unauthorized" } + Rails.logger.warn { "Failed to onboard school #{response[:school]&.id}: user is unauthorized" } failure(response, e) rescue ActiveRecord::RecordInvalid => e # A double submit loses the advisory lock race and fails the creator_id @@ -44,8 +44,9 @@ def acquire_advisory_lock_for_creator(creator_id) end def failure(response, error) - response[:error] = response[:school].errors.presence || [error.message] - response[:error_types] = response[:school].errors.details + school_errors = response[:school]&.errors + response[:error] = school_errors&.presence || [error.message] + response[:error_types] = school_errors&.details response end From 765ffaf6f0874dbc83841139e77decca9cd03c61 Mon Sep 17 00:00:00 2001 From: cocomarine Date: Thu, 20 Aug 2026 09:15:26 +0100 Subject: [PATCH 5/6] remove necessary guard --- lib/concepts/school/operations/create.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index 119753f15..0fec8444c 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -45,7 +45,7 @@ def acquire_advisory_lock_for_creator(creator_id) def failure(response, error) school_errors = response[:school]&.errors - response[:error] = school_errors&.presence || [error.message] + response[:error] = school_errors.presence || [error.message] response[:error_types] = school_errors&.details response end From a766d72dfcc87abb67ca7deec94b31e2e2cea310 Mon Sep 17 00:00:00 2001 From: cocomarine Date: Thu, 20 Aug 2026 11:19:54 +0100 Subject: [PATCH 6/6] default to empty hash when no validation errors --- lib/concepts/school/operations/create.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/concepts/school/operations/create.rb b/lib/concepts/school/operations/create.rb index 0fec8444c..9923cbf86 100644 --- a/lib/concepts/school/operations/create.rb +++ b/lib/concepts/school/operations/create.rb @@ -46,7 +46,7 @@ def acquire_advisory_lock_for_creator(creator_id) def failure(response, error) school_errors = response[:school]&.errors response[:error] = school_errors.presence || [error.message] - response[:error_types] = school_errors&.details + response[:error_types] = school_errors&.details || {} response end