Do not retry BeginUpdate and use a longer claim deadline in non-serving runtimes

Marco Gregoriusrequested to merge
gmarco/947-cells-begin-update-no-retry-deadline into master

What does this MR do and why?

This MR changes two things in app/models/cells/transaction_record.rb. Specs are in spec/models/cells/transaction_record_spec.rb.

1. Stop retrying the Topology Service BeginUpdate RPC

Today Cells::TransactionRecord#before_committed! wraps Cells::OutstandingLease.create_from_request! in Retriable. It makes 2 tries on GRPC::DeadlineExceeded and GRPC::Unavailable.

BeginUpdate is not idempotent. The request has no client identifier. The lease UUID is minted by the server. When attempt 1 succeeds on the server but the response misses the client deadline, attempt 2 sends the same claims again. The Topology Service answers ALREADY_EXISTS against the cell's own pending lease. Rails then raises Cells::TransactionRecord::AlreadyClaimedError for a conflict that does not exist. The first lease stays pending on the Topology Service with no local Cells::OutstandingLease row until Cells::LostTransactionRecoveryWorker rolls it back after 5 minutes.

After this MR, BeginUpdate is sent once. A timeout raises a plain Cells::TransactionRecord::Error. CommitUpdate and RollbackUpdate keep their retry, because they are keyed by the lease UUID.

This change does not make the timed-out claim succeed. It does not remove the orphaned lease. It removes the misleading error and the wasted second RPC. The proper fix is a client-supplied operation ID on BeginUpdate. That work is tracked in gitlab-com/gl-infra/tenant-scale/cells-infrastructure/team#814.

2. Use a 2 second claim deadline in rake, rails runner and rails console

Puma and Sidekiq keep the 300 ms deadline. The 300 ms deadline exists to protect request latency, because the claim runs inside the database transaction before commit. One-off processes have no such budget.

Measurements from a toolbox pod on the staging cell 9 (AWS) against the Topology Service (GCP), with a warm channel and the real Gitlab::TopologyServiceClient::ClaimService, for a 7 record BeginUpdate (the shape of the admin seed):

  • Steady state: p50 122 ms, p95 291 ms, p99 342 ms.
  • First 25 calls of a fresh process: all took 264 to 345 ms.
  • Single call after 90 seconds idle: 285 to 423 ms.
  • TCP + mTLS handshake alone: 90 to 190 ms.

The default organization seed in gitlab:db:configure (db/fixtures/production/003_admin.rb) is the first call of a fresh rake process. It hit the 300 ms deadline and then failed with the false AlreadyClaimedError described above.

Runtime detection uses Gitlab::Runtime.rake?, Gitlab::Runtime.rails_runner? and Gitlab::Runtime.console?.

Not in this MR

References

Screenshots or screen recordings

No UI change.

How to set up and validate locally

  1. Run the specs:

    bundle exec rspec spec/models/cells/transaction_record_spec.rb

  2. Check the runtime split without a Topology Service:

    bundle exec rails runner 'puts Cells::TransactionRecord.new(ApplicationRecord.connection, nil).send(:timeout_in_seconds)'

    This prints 2. In a rails console the same expression prints 2. In Puma or Sidekiq it returns 0.3.

  3. With the Cells GDK setup enabled (Gitlab.config.cell.enabled), stop the Topology Service. Then create a claimable record in rails console, for example:

    Organization.create!(name: 'probe', path: "probe-#{SecureRandom.hex(4)}")

    The call fails after one BeginUpdate attempt with Cells::TransactionRecord::Error: Failed to create lease, not AlreadyClaimedError.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Marco Gregorius

Merge request reports