From dedba0c5462de95929968e88d2257e048f954f93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Mon, 6 Oct 2025 12:00:08 +0200 Subject: [PATCH 1/4] Updating Topology Service Client Gem to d3a9a18 - 0783b51: Fix `types.UUID` as it conflicts with `Gitlab::Types` in Rails Changelog: other --- vendor/gems/gitlab-topology-service-client/REVISION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vendor/gems/gitlab-topology-service-client/REVISION b/vendor/gems/gitlab-topology-service-client/REVISION index 3b37d24f1af7e2..33bb13f37f6ad5 100644 --- a/vendor/gems/gitlab-topology-service-client/REVISION +++ b/vendor/gems/gitlab-topology-service-client/REVISION @@ -1 +1 @@ -e0a35bc588ccb3dcc3e3d081b48418e2c08dfbb9 +d3a9a18337a56f9731f62aa636e6cc75dd8b4d02 -- GitLab From e4d2166769c6bd95b93c0d71b940256394607164 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Mon, 6 Oct 2025 13:03:06 +0200 Subject: [PATCH 2/4] Introduce `Cells::TransactionRecord` as a way to hook into transaction --- app/models/cells/outstanding_lease.rb | 51 ++++++ app/models/cells/transaction_record.rb | 89 +++++++++ app/models/concerns/cells/inspect_changes.rb | 129 ------------- .../concerns/cells/transaction_callback.rb | 70 ------- app/models/concerns/cells/unique.rb | 117 ++++++------ app/models/current.rb | 11 +- app/models/email.rb | 2 + app/models/gpg_key.rb | 2 + app/models/gpg_key_subkey.rb | 1 + app/models/key.rb | 2 + app/models/organizations/organization.rb | 2 + app/models/route.rb | 2 + app/models/user.rb | 2 + ...ctive_record_cells_transaction_callback.rb | 10 - .../topology_service_client/base_service.rb | 4 + .../topology_service_client/claim_service.rb | 172 +----------------- 16 files changed, 229 insertions(+), 437 deletions(-) create mode 100644 app/models/cells/transaction_record.rb delete mode 100644 app/models/concerns/cells/inspect_changes.rb delete mode 100644 app/models/concerns/cells/transaction_callback.rb delete mode 100644 config/initializers/active_record_cells_transaction_callback.rb diff --git a/app/models/cells/outstanding_lease.rb b/app/models/cells/outstanding_lease.rb index f74763e6b828ae..caf509046270a0 100644 --- a/app/models/cells/outstanding_lease.rb +++ b/app/models/cells/outstanding_lease.rb @@ -3,5 +3,56 @@ module Cells class OutstandingLease < ApplicationRecord self.primary_key = :uuid + + def self.create_from_request!(create_records:, destroy_records:) + req = Gitlab::Cells::TopologyService::Claims::V1::BeginUpdateRequest.new( + create_records: create_records, + destroy_records: destroy_records, + cell_id: claim_service.cell_id + ) + + pp("send_begin_update req #{req}") + res = self.claim_service.begin_update(req) + + pp("send_begin_update res #{res.lease_uuid}") + self.create!(uuid: res.lease_uuid.value) + end + + def send_commit_update! + req = Gitlab::Cells::TopologyService::Claims::V1::CommitUpdateRequest.new( + lease_uuid: Gitlab::Cells::TopologyService::Types::V1::UUID.new(value: uuid), + cell_id: self.class.claim_service.cell_id + ) + + pp("send_commit_update #{self.object_id} #{req}") + self.class.claim_service.commit_update( + req + ) + + pp("send_commit_update done #{self.object_id} #{req}") + end + + def send_rollback_update! + return unless uuid + + req = Gitlab::Cells::TopologyService::Claims::V1::RollbackUpdateRequest.new( + lease_uuid: Gitlab::Cells::TopologyService::Types::V1::UUID.new(value: uuid), + cell_id: self.class.claim_service.cell_id + ) + + pp("send_rollback_update #{self.object_id} #{req}") + + self.class.claim_service.rollback_update( + req + ) + + pp("send_rollback_update done #{self.object_id} #{req}") + end + + private + + def self.claim_service + @claim_service ||= ::Gitlab::TopologyServiceClient::ClaimService.instance + end end end diff --git a/app/models/cells/transaction_record.rb b/app/models/cells/transaction_record.rb new file mode 100644 index 00000000000000..d33516886a9097 --- /dev/null +++ b/app/models/cells/transaction_record.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +module Cells + class TransactionRecord + TRANSACTION_RECORD_KEY = :@cells_current_transaction + + # TODO: hack to assign to current transaction + # Figure out a better way to do this + # Current? but what if nested transactions? + # what is the transactions gets rolled back? + def self.current_transaction(connection) + return unless Current.cells_claim_leases? + + # The Cells::OutstandingLease requires a transaction to be open + # to ensure that the lease is only created if the transaction + # within a transaction and not outside of one + unless connection.current_transaction + raise 'The Cells::TransactionRecord requires transaction to be open' + end + + current_transaction = connection.current_transaction + + instance = current_transaction.instance_variable_get(TRANSACTION_RECORD_KEY) + return instance if instance + + TransactionRecord.new(connection, current_transaction).tap do |instance| + current_transaction.instance_variable_set(TRANSACTION_RECORD_KEY, instance) + current_transaction.add_record(instance) + end + end + + def initialize(connection, transaction) + @connection = connection + @transaction = transaction + @create_records = [] + @destroy_records = [] + @outstanding_lease = nil + @done = false + end + + def create_record(metadata) + @create_records << metadata + pp("create_record #{self.object_id} #{metadata}") + end + + def destroy_record(metadata) + @destroy_records << metadata + pp("destroy_record #{self.object_id} #{metadata}") + end + + # Callbacks required by the ActiveRecord::ConnectionAdapters::TransactionState + + def trigger_transactional_callbacks? + true + end + + def before_committed! + raise 'Already done' if @done + raise 'Already created lease' if @outstanding_lease + raise 'Cross-database lease' if Cells::OutstandingLease.connection != @connection + + @outstanding_lease = Cells::OutstandingLease.create_from_request!( + create_records: @create_records, + destroy_records: @destroy_records + ) + end + + def rolledback!(force_restore_state: false, should_run_callbacks: true) + raise 'Already done' if @done + + # It is possible that lease might be not created yet, + # since the transaction might be rolledback prematurely + return unless @outstanding_lease + + @outstanding_lease.send_rollback_update! + @outstanding_lease.destroy! # the lease is no longer needed + @done = true + end + + def committed!(should_run_callbacks: true) + raise 'Already done' if @done + raise 'No lease created' unless @outstanding_lease + + @outstanding_lease.send_commit_update! + @outstanding_lease.destroy! # the lease is no longer needed + @done = true + end + end +end diff --git a/app/models/concerns/cells/inspect_changes.rb b/app/models/concerns/cells/inspect_changes.rb deleted file mode 100644 index e152f7c29080b0..00000000000000 --- a/app/models/concerns/cells/inspect_changes.rb +++ /dev/null @@ -1,129 +0,0 @@ -# frozen_string_literal: true - -module Cells - class InspectChanges - class Visit - extend Forwardable - - attr_reader :record - - def_delegators :@map, :[], :[]=, :filter_map - - def initialize(new_record) - @record = new_record - @map = {} - end - end - - attr_reader :record - - def initialize(new_record, destroy: false) - @record = new_record - @destroy = destroy - end - - def inspect_changes - return if !record.is_a?(Unique) || inspected - - prepare_inspecting - - inspect_record - inspect_associations - - if TransactionCallback.visited - # There's an actual transaction, and TransactionCallback will - # handle this case right before committing - else - # Since the transaction is optimized away, TransactionCallback - # cannot handle this case, and we can just assume changes are - # going to be made, we treat this as if before_commit_all - Unique.claim_and_commit(visited) - end - end - - private - - def visited - @visited ||= TransactionCallback.visited || {} - end - - def inspect_changes_key - @inspect_changes_key ||= [record.class.table_name, record.id] - end - - def inspected - visited[inspect_changes_key] - end - - def prepare_inspecting - visited[inspect_changes_key] = Visit.new(record) - end - - def inspect_record - if destroying? - inspect_deleted_attributes - else - inspect_saved_changes - end - end - - def destroying? - @destroy - end - - def inspect_deleted_attributes - record.class.cells_unique_attributes.each_key do |column| - inspected[column.to_s] = [record.attribute_in_database(column)] - end - end - - def inspect_saved_changes - record.saved_changes.each do |column, (old_value, new_value)| - if record.class.cells_unique_attributes[column.to_sym] # rubocop:disable Style/Next -- I think this is more clear - # new_value in GpgKey is transformed into upcase, which - # mismatches with what we're storing in the database - new_value = record.attribute_in_database(column) - inspected[column] = [old_value, new_value] - end - end - end - - def inspect_associations - # We know the following associations will be deleted yet the hooks - # won't be called, so we laod it at once and inspect the changes - # as if they would get changed. - preload_nested_associations(record) if destroying? - - record.class.reflect_on_all_associations.each do |reflection| - association_proxy = record.association(reflection.name) - - if association_proxy.loaded? # rubocop:disable Style/IfUnlessModifier -- I think this is more clear - inspect_loaded_associations(association_proxy) - end - end - end - - def inspect_loaded_associations(association_proxy) - Array.wrap(association_proxy.target).each do |association_record| - InspectChanges.new(association_record, destroy: destroying?).inspect_changes - end - end - - def preload_nested_associations(record) - ActiveRecord::Associations::Preloader.new( - records: [record], - associations: lookup_associations(record.class) - ).call - end - - def lookup_associations(model) - model.cells_unique_has_many_associations.map do |name| - { name => lookup_associations(reflect_association_model(model, name)) } - end - end - - def reflect_association_model(model, name) - model.reflect_on_association(name).class_name.constantize - end - end -end diff --git a/app/models/concerns/cells/transaction_callback.rb b/app/models/concerns/cells/transaction_callback.rb deleted file mode 100644 index 84264115eeebe4..00000000000000 --- a/app/models/concerns/cells/transaction_callback.rb +++ /dev/null @@ -1,70 +0,0 @@ -# frozen_string_literal: true - -module Cells - module TransactionCallback - THREAD_VARIABLE_NAME = :cells_unique_transaction_callback_visited_stack - - def self.visited - visited_stack.first # Flatten to the first - end - - def self.last_visited - visited_stack.last - end - - def self.visited_stack - Thread.current.thread_variable_get(THREAD_VARIABLE_NAME) || - Thread.current.thread_variable_set(THREAD_VARIABLE_NAME, []) - end - - def begin_isolated_db_transaction(...) - return super unless Current.feature_cells_unique_claims - - TransactionCallback.visited_stack << {} - super - end - - def begin_db_transaction - return super unless Current.feature_cells_unique_claims - - TransactionCallback.visited_stack << {} - super - end - - def commit_db_transaction - return super unless Current.feature_cells_unique_claims - - # Don't pop the visited stack yet, and only do that after it's - # successfully committed, because errors can happen while claiming, - # and when that happens, we'll roll it back, and we need to look at - # the stack to know if it's a unbalanced ROLLBACK or not, therefore - # we only pop after successful COMMIT or ROLLBACk. - Unique.claim_and_commit(TransactionCallback.last_visited.values) do - super - TransactionCallback.visited_stack.pop - end - end - - def rollback_db_transaction - return super unless Current.feature_cells_unique_claims - - Unique.rollback do - super - - # Rails can be issuing unbalanced ROLLBACK (no corresponding BEGIN), - # and in that case we ignore and do not need to rollback the last - # lease because it's already committed! This can happen if there are - # errors internally coming from Rails while committing to the local - # database. We determine this by checking the visited stack. If the - # stack is empty, it means it's not balanced and we do not want to - # rollback on Topology service in this case. - if TransactionCallback.visited_stack.empty? - false - else - TransactionCallback.visited_stack.pop - true - end - end - end - end -end diff --git a/app/models/concerns/cells/unique.rb b/app/models/concerns/cells/unique.rb index 1c6a5c86b2bb28..8ef94a87a8d25b 100644 --- a/app/models/concerns/cells/unique.rb +++ b/app/models/concerns/cells/unique.rb @@ -5,17 +5,16 @@ module Unique extend ActiveSupport::Concern BUCKET_TYPE = Gitlab::Cells::TopologyService::Claims::V1::Bucket::Type - SOURCE_TYPE = Gitlab::Cells::TopologyService::Claims::V1::Source::Type SUBJECT_TYPE = Gitlab::Cells::TopologyService::Claims::V1::Subject::Type + SOURCE_TYPE = Gitlab::Cells::TopologyService::Claims::V1::Source::Type included do - after_save :inspect_save_changes - before_destroy :inspect_destroy_changes - cells_config + after_save :claim_save_changes + before_destroy :claim_destroy_changes end class_methods do - attr_reader :cells_type, :cells_source_type + attr_reader :cells_subject_type, :cells_subject_key, :cells_source_type def cells_unique_attributes @cells_unique_attributes ||= {} @@ -25,31 +24,10 @@ def cells_unique_has_many_associations @cells_unique_has_many_associations ||= [] end - def cells_config( - type: default_type, - source_type: default_source_type) - @cells_type = type - @cells_source_type = source_type - end - - def default_source_type - source_type_name = "RAILS_TABLE_#{table_name.upcase}" - - if SOURCE_TYPE.const_defined?(source_type_name) - SOURCE_TYPE.const_get(source_type_name, false) - else - SOURCE_TYPE::UNSPECIFIED - end - end - - def default_type - subject_type_name = name.demodulize.upcase - - if SUBJECT_TYPE.const_defined?(subject_type_name) - SUBJECT_TYPE.const_get(subject_type_name, false) - else - SUBJECT_TYPE::UNSPECIFIED - end + def cells_config(subject_type:, subject_key: nil, source_type: nil) + @cells_subject_type = subject_type + @cells_subject_key = subject_key || :organization_id + @cells_source_type = source_type || const_get("Gitlab::Cells::TopologyService::Claims::V1::Source::Type::RAILS_TABLE_#{table_name.upcase}") end def cells_unique_attribute(name, type:) @@ -61,40 +39,71 @@ def cells_unique_has_many(name) end end - def self.claim_and_commit(changes) - if changes.empty? - yield if block_given? - else - pp changes + private + + def claim_save_changes + transaction_record = ::Cells::TransactionRecord.current_transaction(connection) + return unless transaction_record - claim_client = Gitlab::TopologyServiceClient::ClaimService.new - claim_client.claim_and_commit(changes) do - yield if block_given? + pp("claim_save_changes #{self.object_id}: #{self.class.name} #{self.id}") + + default_metadata = claim_metadata + + self.class.cells_unique_attributes.each do |attribute, config| + next unless saved_change_to_attribute?(attribute) + + was, is = saved_change_to_attribute(attribute) + pp(" attribute '#{attribute}' changed from '#{was}' to '#{is}'") + + if !was.nil? && was != is + transaction_record.destroy_record(default_metadata.merge({ + bucket: { + type: config[:type], + value: attribute_in_database(attribute) + } + })) end - end - end - def self.rollback - claim_client = Gitlab::TopologyServiceClient::ClaimService.new - claim_client.rollback do - yield + if !is.nil? + transaction_record.create_record(default_metadata.merge( + bucket: { + type: config[:type], + value: self.public_send(attribute) + } + )) + end end end - private + def claim_destroy_changes + transaction_record = ::Cells::TransactionRecord.current_transaction(connection) + return unless transaction_record - def inspect_save_changes - return unless Gitlab.config.cell.enabled - return unless Current.feature_cells_unique_claims + pp("claim_destroy_changes #{self.object_id}: #{self.class.name} #{self.id}") - InspectChanges.new(self).inspect_changes - end + default_metadata = claim_metadata - def inspect_destroy_changes - return unless Gitlab.config.cell.enabled - return unless Current.feature_cells_unique_claims + self.class.cells_unique_attributes.each do |attribute, config| + transaction_record.destroy_record(default_metadata.merge( + bucket: { + type: config[:type], + value: self.public_send(attribute) + } + )) + end + end - InspectChanges.new(self, destroy: true).inspect_changes + def claim_metadata + { + subject: { + type: self.class.cells_subject_type, + id: self.public_send(self.class.cells_subject_key) + }, + source: { + type: self.class.cells_source_type, + rails_primary_key_id: self.public_send(self.class.primary_key) + }, + } end end end diff --git a/app/models/current.rb b/app/models/current.rb index e261872e61790b..c8c3ff32280ea1 100644 --- a/app/models/current.rb +++ b/app/models/current.rb @@ -18,11 +18,14 @@ def message attribute :organization, :organization_assigned attribute :token_info - attribute :feature_cells_unique_claims - resets :reset_feature_cells_unique_claims + attribute :cells_claim_leases - def initialize - reset + def cells_claim_leases? + if cells_claim_leases.nil? + self.cells_claim_leases = Gitlab.config.cell.enabled && Feature.enabled?(:cells_unique_claims) + end + + cells_claim_leases end def organization=(value) diff --git a/app/models/email.rb b/app/models/email.rb index 471124ffa15bb3..ba00f710da0a85 100644 --- a/app/models/email.rb +++ b/app/models/email.rb @@ -7,6 +7,8 @@ class Email < ApplicationRecord cells_unique_attribute :email, type: BUCKET_TYPE::UNSPECIFIED # TODO: EMAIL? + cells_config subject_type: SUBJECT_TYPE::USER, subject_key: :user_id + belongs_to :user, optional: false belongs_to :banned_user, class_name: '::Users::BannedUser', foreign_key: 'user_id', inverse_of: 'emails' diff --git a/app/models/gpg_key.rb b/app/models/gpg_key.rb index 30b0096cfe7a2e..7e09ddc12cc858 100644 --- a/app/models/gpg_key.rb +++ b/app/models/gpg_key.rb @@ -11,6 +11,8 @@ class GpgKey < ApplicationRecord cells_unique_attribute :fingerprint, type: BUCKET_TYPE::UNSPECIFIED cells_unique_attribute :primary_keyid, type: BUCKET_TYPE::UNSPECIFIED + cells_config subject_type: SUBJECT_TYPE::UNSPECIFIED + cells_unique_has_many :subkeys sha_attribute :primary_keyid diff --git a/app/models/gpg_key_subkey.rb b/app/models/gpg_key_subkey.rb index 5749818cb7ccc3..161c9db5c15555 100644 --- a/app/models/gpg_key_subkey.rb +++ b/app/models/gpg_key_subkey.rb @@ -5,6 +5,7 @@ class GpgKeySubkey < ApplicationRecord include Cells::Unique cells_unique_attribute :fingerprint, type: BUCKET_TYPE::UNSPECIFIED + cells_config subject_type: SUBJECT_TYPE::UNSPECIFIED sha_attribute :keyid sha_attribute :fingerprint diff --git a/app/models/key.rb b/app/models/key.rb index f8dc5d20e3021f..bee14cfd21c092 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -13,6 +13,8 @@ class Key < ApplicationRecord cells_unique_attribute :key, type: BUCKET_TYPE::UNSPECIFIED # TODO: KEY? cells_unique_attribute :fingerprint_sha256, type: BUCKET_TYPE::UNSPECIFIED # TODO: KEY_FINGERPRINT? + cells_config subject_type: SUBJECT_TYPE::UNSPECIFIED, source_type: SOURCE_TYPE::UNSPECIFIED + sha256_attribute :fingerprint_sha256 belongs_to :user diff --git a/app/models/organizations/organization.rb b/app/models/organizations/organization.rb index 3f3e218179ee56..c906485992b74f 100644 --- a/app/models/organizations/organization.rb +++ b/app/models/organizations/organization.rb @@ -11,6 +11,8 @@ class Organization < ApplicationRecord cells_unique_attribute :path, type: BUCKET_TYPE::UNSPECIFIED # TODO: PATH? + cells_config subject_type: SUBJECT_TYPE::ORGANIZATION + DEFAULT_ORGANIZATION_ID = 1 scope :without_default, -> { id_not_in(DEFAULT_ORGANIZATION_ID) } diff --git a/app/models/route.rb b/app/models/route.rb index ec429506b31c02..38839ee1df89a5 100644 --- a/app/models/route.rb +++ b/app/models/route.rb @@ -8,6 +8,8 @@ class Route < ApplicationRecord cells_unique_attribute :path, type: BUCKET_TYPE::ROUTES + cells_config subject_type: SUBJECT_TYPE::GROUP, subject_key: :namespace_id + belongs_to :source, polymorphic: true, inverse_of: :route # rubocop:disable Cop/PolymorphicAssociations belongs_to :namespace, inverse_of: :namespace_route validates :source, presence: true diff --git a/app/models/user.rb b/app/models/user.rb index d9befba8df2721..f307ee9df62d79 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -42,6 +42,8 @@ class User < ApplicationRecord cells_unique_has_many :gpg_keys cells_unique_has_many :emails + cells_config subject_type: SUBJECT_TYPE::USER + ignore_column %i[role skype], remove_after: '2025-09-18', remove_with: '18.4' DEFAULT_NOTIFICATION_LEVEL = :participating diff --git a/config/initializers/active_record_cells_transaction_callback.rb b/config/initializers/active_record_cells_transaction_callback.rb deleted file mode 100644 index e213d795fb7d7d..00000000000000 --- a/config/initializers/active_record_cells_transaction_callback.rb +++ /dev/null @@ -1,10 +0,0 @@ -# frozen_string_literal: true - -ActiveSupport.on_load(:active_record) do - # Prepend into AbstractAdapter doesn't fully work, so we prepend to - # actual adapter we use. - if Gitlab.config.cell.enabled - ActiveRecord::ConnectionAdapters::PostgreSQLAdapter - .prepend(Cells::TransactionCallback) - end -end diff --git a/lib/gitlab/topology_service_client/base_service.rb b/lib/gitlab/topology_service_client/base_service.rb index 1aaa4b8a0c0249..bca8a8205dfd4d 100644 --- a/lib/gitlab/topology_service_client/base_service.rb +++ b/lib/gitlab/topology_service_client/base_service.rb @@ -11,6 +11,10 @@ def initialize(timeout: nil) @timeout = timeout end + def cell_id + @cell_id ||= Gitlab.config.cell.id + end + private def client diff --git a/lib/gitlab/topology_service_client/claim_service.rb b/lib/gitlab/topology_service_client/claim_service.rb index 3380f10819df02..b248e63afdeaac 100644 --- a/lib/gitlab/topology_service_client/claim_service.rb +++ b/lib/gitlab/topology_service_client/claim_service.rb @@ -3,180 +3,12 @@ module Gitlab module TopologyServiceClient class ClaimService < BaseService - THREAD_VARIABLE_NAME = :gitlab_topology_service_client_claim_service_last_outstanding_lease_uuid + include Singleton - def self.last_outstanding_lease_uuid - Thread.current.thread_variable_get(THREAD_VARIABLE_NAME) - end - - def self.set_last_outstanding_lease_uuid(uuid) - Thread.current.thread_variable_set(THREAD_VARIABLE_NAME, uuid) - end - - def claim_and_commit(changes) - lease = claim(changes) - - yield - - commit(lease) - end - - def rollback - # See why we're setting it to nil immediately in #commit - lease_uuid = self.class.last_outstanding_lease_uuid - self.class.set_last_outstanding_lease_uuid(nil) - - # Check `rollback_db_transaction` in: - # lib/gitlab/topology_service_client/claim_service.rb - # For why we're checking if this is balanced or not. - balanced_rollback = yield - - if balanced_rollback && lease_uuid # rubocop:disable Style/GuardClause: -- I think this is more clear - # Test error here: - # raise "Test error before RollbackUpdate" - - client.rollback_update( - Gitlab::Cells::TopologyService::Claims::V1::RollbackUpdateRequest.new( - lease_uuid: build_claim_lease(lease_uuid), - cell_id: cell_id - ) - ) - end - end - - def commit(lease) - # Always set to nil even when we fail to commit, because when we're - # calling this, we know we already committed to local database, and - # in that case, this is considered done and we should move on. This - # is to avoid interfering with the next transaction because we're - # using thread local variable which is per connection. We're doing - # this because the connection cannot access the lease otherwise. - # If there does have errors while committing on Topology service, - # we let the background process to handle it instead: - # Commit and/or delete the local lease. - self.class.set_last_outstanding_lease_uuid(nil) - - # Test error here: - # raise "Test error after COMMIT before CommitUpdate" - - client.commit_update( - Gitlab::Cells::TopologyService::Claims::V1::CommitUpdateRequest.new( - lease_uuid: build_claim_lease(lease.uuid), - cell_id: cell_id - ) - ) - - # Test error here: - # raise "Test error after CommitUpdate before deleting local lease" - - lease.destroy - end + delegate :begin_update, :commit_update, :rollback_update, to: :client private - def claim(changes) - # Test error here: - # raise "Test error while BeginUpdate" - - response = client.begin_update(build_claim_request(changes)) - - # Test error here: - # raise "Test error after BeginUpdate before creating local lease" - - # We use a thread local variable to remember the last outstanding - # lease because if there's any errors while we're committing to - # local database, the connection object will roll back the changes - # and there has to be a way to access the lease to roll it back. - self.class.set_last_outstanding_lease_uuid(response.lease_uuid.value) - - ::Cells::OutstandingLease.create(uuid: response.lease_uuid.value) - - # Test error here: - # raise "Test error after creating local lease" - end - - # { - # ["users", 55]=>{ - # "email"=>["", "test@example.com"], - # "username"=>[nil, "test"] - # }, - # ["routes", 139]=>{ - # "path"=>[nil, "test"] - # } - # } - def build_claim_request(changes) - Gitlab::Cells::TopologyService::Claims::V1::BeginUpdateRequest.new( - create_records: build_create_records(changes), - destroy_records: build_destroy_records(changes), - cell_id: cell_id - ) - end - - def build_create_records(changes) - changes.flat_map do |visit| - visit.filter_map do |column, (_old_value, new_value)| - if new_value # rubocop:disable Style/IfUnlessModifier -- I think this is more clear - build_claim_data(visit.record, column, new_value) - end - end - end - end - - def build_claim_data(record, column, value) - { - bucket: build_bucket(record, column, value), - source: build_source(record), - subject: build_subject(record) - } - end - - def build_bucket(record, column, value) - { - type: record.class.cells_unique_attributes.dig(column.to_sym, :type), - value: value - } - end - - def build_source(record) - { - type: record.class.cells_source_type, - rails_primary_key_id: record.public_send(record.class.primary_key) # rubocop:disable GitlabSecurity/PublicSend -- this works by definition - } - end - - # TODO: What if this record has more than one sharding keys? - def build_subject(record) - table, id = record.sharding_keys.first - { - type: determine_subject_type_from_table(table), - id: id - } - end - - def build_destroy_records(changes) - changes.flat_map do |visit| - visit.filter_map do |column, (old_value, _new_value)| - if old_value.present? # rubocop:disable Style/IfUnlessModifier -- I think this is more clear - build_claim_data(visit.record, column, old_value) - end - end - end - end - - def build_claim_lease(uuid) - Gitlab::Cells::TopologyService::Types::V1::UUID.new(value: uuid) - end - - def determine_subject_type_from_table(table) - model_name = table.classify - - if Object.const_defined?(model_name) - model_name.constantize - else - table.capitalize.constantize.const_get(model_name, false) - end.cells_type - end - def service_class Gitlab::Cells::TopologyService::Claims::V1::ClaimService::Stub end -- GitLab From 8aac3be645b8f28e140728e687bf8e2529911b28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Thu, 9 Oct 2025 14:03:02 +0200 Subject: [PATCH 3/4] Do not allow to create records once lease was created --- app/models/cells/transaction_record.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/models/cells/transaction_record.rb b/app/models/cells/transaction_record.rb index d33516886a9097..9e99ee6e917579 100644 --- a/app/models/cells/transaction_record.rb +++ b/app/models/cells/transaction_record.rb @@ -39,11 +39,15 @@ def initialize(connection, transaction) end def create_record(metadata) + raise 'Lease already created' if @outstanding_lease + @create_records << metadata pp("create_record #{self.object_id} #{metadata}") end def destroy_record(metadata) + raise 'Lease already created' if @outstanding_lease + @destroy_records << metadata pp("destroy_record #{self.object_id} #{metadata}") end -- GitLab From cc3c1f7ef09feae16e74df2ae161b0ca680d73ba Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 10 Oct 2025 00:39:27 +0800 Subject: [PATCH 4/4] Minimal changes to make this work --- app/models/concerns/cells/unique.rb | 2 +- app/models/gpg_key.rb | 4 ++-- app/models/gpg_key_subkey.rb | 3 ++- app/models/namespace.rb | 3 --- app/models/user.rb | 4 ++-- lib/gitlab/topology_service_client/base_service.rb | 4 ---- 6 files changed, 7 insertions(+), 13 deletions(-) diff --git a/app/models/concerns/cells/unique.rb b/app/models/concerns/cells/unique.rb index 8ef94a87a8d25b..19cf8311e1cae7 100644 --- a/app/models/concerns/cells/unique.rb +++ b/app/models/concerns/cells/unique.rb @@ -59,7 +59,7 @@ def claim_save_changes transaction_record.destroy_record(default_metadata.merge({ bucket: { type: config[:type], - value: attribute_in_database(attribute) + value: was } })) end diff --git a/app/models/gpg_key.rb b/app/models/gpg_key.rb index 7e09ddc12cc858..e3e76132c07042 100644 --- a/app/models/gpg_key.rb +++ b/app/models/gpg_key.rb @@ -11,7 +11,7 @@ class GpgKey < ApplicationRecord cells_unique_attribute :fingerprint, type: BUCKET_TYPE::UNSPECIFIED cells_unique_attribute :primary_keyid, type: BUCKET_TYPE::UNSPECIFIED - cells_config subject_type: SUBJECT_TYPE::UNSPECIFIED + cells_config subject_type: SUBJECT_TYPE::USER, source_type: SOURCE_TYPE::UNSPECIFIED, subject_key: :user_id cells_unique_has_many :subkeys @@ -20,7 +20,7 @@ class GpgKey < ApplicationRecord belongs_to :user has_many :gpg_signatures, class_name: 'CommitSignatures::GpgSignature' - has_many :subkeys, class_name: 'GpgKeySubkey' + has_many :subkeys, class_name: 'GpgKeySubkey', dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent -- needed to unclaim scope :with_subkeys, -> { includes(:subkeys) } scope :externally_invalid, -> { where(externally_verified: false) } diff --git a/app/models/gpg_key_subkey.rb b/app/models/gpg_key_subkey.rb index 161c9db5c15555..6716ab33398ca3 100644 --- a/app/models/gpg_key_subkey.rb +++ b/app/models/gpg_key_subkey.rb @@ -5,7 +5,8 @@ class GpgKeySubkey < ApplicationRecord include Cells::Unique cells_unique_attribute :fingerprint, type: BUCKET_TYPE::UNSPECIFIED - cells_config subject_type: SUBJECT_TYPE::UNSPECIFIED + + cells_config subject_type: SUBJECT_TYPE::UNSPECIFIED, source_type: SOURCE_TYPE::UNSPECIFIED, subject_key: :gpg_key_id sha_attribute :keyid sha_attribute :fingerprint diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 26f0ede94ded62..f39c33655704ec 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -21,9 +21,6 @@ class Namespace < ApplicationRecord include UseSqlFunctionForPrimaryKeyLookups include SafelyChangeColumnDefault include Todoable - include Cells::Unique - - cells_config type: SUBJECT_TYPE::GROUP extend Gitlab::Utils::Override diff --git a/app/models/user.rb b/app/models/user.rb index f307ee9df62d79..ab94ccfa27e012 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -172,9 +172,9 @@ def update_tracked_fields!(request) has_many :expired_today_and_unnotified_keys, -> { expired_today_and_not_notified }, class_name: 'Key' has_many :expiring_soon_and_unnotified_keys, -> { expiring_soon_and_not_notified }, class_name: 'Key' has_many :deploy_keys, -> { where(type: 'DeployKey') }, dependent: :nullify # rubocop:disable Cop/ActiveRecordDependent - has_many :gpg_keys + has_many :gpg_keys, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent -- needed to unclaim - has_many :emails + has_many :emails, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent -- needed to unclaim has_many :personal_access_tokens, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :expiring_soon_and_unnotified_personal_access_tokens, -> { expiring_and_not_notified_without_impersonation }, class_name: 'PersonalAccessToken' diff --git a/lib/gitlab/topology_service_client/base_service.rb b/lib/gitlab/topology_service_client/base_service.rb index bca8a8205dfd4d..1ac888784ae006 100644 --- a/lib/gitlab/topology_service_client/base_service.rb +++ b/lib/gitlab/topology_service_client/base_service.rb @@ -30,10 +30,6 @@ def options { timeout: @timeout || DEFAULT_TIMEOUT_IN_SECONDS } end - def cell_id - @cell_id ||= Gitlab.config.cell.id - end - def service_credentials return :this_channel_is_insecure unless topology_service_config.tls.enabled -- GitLab