Fix deadlock when pushing multiple gem versions concurrently by girachawda · Pull Request #6658 · rubygems/rubygems.org

girachawda · 2026-06-25T19:04:29Z

What

Closes: Deadlock when pushing mutliple gems at once #6099
Move version reordering from a synchronous after_save callback to an asynchronous background job with GoodJob concurrency control.

Why

Concurrent gem pushes trigger the after_save :reorder_versions callback simultaneously, causing PostgreSQL deadlocks when multiple transactions try to lock the same version rows in different orders.

Flow

push/yank/restore
  -> write version/indexed state
  -> enqueue ReorderVersionsJob
      -> reorder versions
      -> set latest correctly
      -> enqueue Indexer
      -> enqueue SetLinksetHomeJob

Indexer runs after ReorderVersionsJob because the legacy index depends on the freshly-updated latest flags. SetLinksetHomeJob is also chained after reorder for the same reason.

Tophat

Run the reproduction script to verify deadlocks are eliminated. This script drains the default queue in-process; no separate worker is required for the synthetic tophat data:

Script

require "securerandom"; require "digest"

gem_name = "test-concurrent-push-#{SecureRandom.hex(4)}"
rubygem  = Rubygem.create!(name: gem_name)
count    = 10
threads  = 4   # keep below dev DB pool (5)

# 1. Create version rows serially (avoids pool storm + new-gem race)
versions = count.times.map do |i|
  rubygem.versions.create!(
    number: "#{i}.0.0", platform: "ruby", gem_platform: "ruby",
    indexed: false, authors: ["Test"], summary: "Test",
    sha256: Digest::SHA256.base64digest("test#{i}"), size: 1000
  )
end

# 2. Fire AfterVersionWriteJob CONCURRENTLY (the path that used to deadlock)
errors = Queue.new
versions.each_slice(threads) do |batch|
  batch.map do |v|
    Thread.new do
      ActiveRecord::Base.connection_pool.with_connection { AfterVersionWriteJob.new.perform(version: v) }
    rescue => e
      errors << "#{e.class}: #{e.message}"
    end
  end.each(&:join)
end

# 3. Report deadlocks
errs = []; errs << errors.pop until errors.empty?
deadlocks = errs.grep(/Deadlocked/)
puts deadlocks.empty? ? "✓ NO DEADLOCKS" : "✗ DEADLOCKS: #{deadlocks}"
puts "other noise (pool/setup): #{errs - deadlocks}" unless (errs - deadlocks).empty?

# 4. Drain the enqueued ReorderVersionsJob(s) in-process.
# This synthetic script creates Version rows without real .gem files, so only
# drain the default queue; the version_contents queue expects real gem files.
GoodJob.perform_inline("default")

# 5. Check final state
rubygem.reload
puts "positions: #{rubygem.versions.order(:position).pluck(:number, :position).inspect}"
puts "latest:    #{rubygem.versions.where(latest: true).pluck(:number).inspect}"
puts "nil positions: #{rubygem.versions.where(position: nil).count}"

expected = rubygem.versions.pluck(:number).sort_by { |n| Gem::Version.new(n) }.reverse
actual   = rubygem.versions.order(:position).pluck(:number)
ok = deadlocks.empty? && actual == expected &&
     rubygem.versions.where(latest: true).pluck(:number) == [expected.first] &&
     rubygem.versions.where(position: nil).count.zero?
puts ok ? "✅ PASS" : "❌ FAIL"

Confirm version positions are correct after job completes

Before	After

girachawda · 2026-06-30T21:12:32Z

Confirm the deadlock doesn't occur. The nil is because we don't have workers in console scripts.

jenshenny · 2026-07-01T03:14:29Z

I'll take a closer look at the PR on Thursday!

Confirm the deadlock doesn't occur. The nil is because we don't have workers in console scripts.

btw bundle exec good_job start will start a worker to execute any jobs enqueued.

girachawda · 2026-07-02T15:31:01Z

Tophat ✅ with workers running in console:

jenshenny

I think extracting to a separate job is the correct approach to solve this issue 👍

OughtPuts · 2026-07-03T08:48:46Z

Successfully repro'd locally ✅ Looking at the code next

OughtPuts

This is complex stuff @girachawda - nice work! :) A few thoughts that it would be great to discuss.

OughtPuts · 2026-07-03T11:31:21Z

I haven't had time to look into the implications of this fully, but have been having a quick look and think about the other jobs in perform in case any of them are affected by the removal of the hook and the later place reordering (+ related ops) now happen.

I saw that Indexer calls rows_for_latest_index which relies on .latest so I think it would be good to double check we are happy that this is now using a stale version of latest now (until the ReorderVersionsJob fires later in the flow... but then Indexer isn't called again of course...).

Good catch, thanks! You're right that with reorder now async there was no guarantee Indexer ran after it, so it could publish a stale latest index and not get re-run until the next push.

I looked at closing this by chaining Indexer off ReorderVersionsJob, but it meant moving Indexer out of AfterVersionWriteJob and coupling indexing to reordering, which rippled into a bunch of unrelated push/yank integration tests. It feels like too much scope creep for a deadlock fix. WDYT? I'd love to hear your thoughts @jenshenny

The Indexer manages the legacy index specs for clients to use so we should ensure correctness here. I would think chaining Indexer off ReorderVersionsJob would be fine so I'm curious on what this rippling is.

jenshenny · 2026-07-27T20:07:24Z

 WebAuthn.configuration.allowed_origins = ["http://localhost:31337"]

 class ActiveSupport::TestCase
+  include ActiveJob::TestHelper


I suggest we make this change separately. There's many include ActiveJob::TestHelper in specific test files that would need to be removed with this.

jenshenny · 2026-07-27T20:41:06Z

The Indexer manages the legacy index specs for clients to use so we should ensure correctness here. I would think chaining Indexer off ReorderVersionsJob would be fine so I'm curious on what this rippling is.

jenshenny · 2026-07-27T20:41:38Z

    end
  end
-
-  context "after_save" do


We should check if the reorder job is enqueued here.

jenshenny · 2026-07-27T20:45:36Z


  def reindex
    Indexer.perform_later
+    # Reorder asynchronously (like the push path) to avoid deadlocks.


I noticed that we're adding a lot of code comments, and they seem to be adding noise rather than value. Can you go through and see which comments if any are valuable to keep?

jenshenny · 2026-07-27T20:47:13Z

    Rubygem.where(
      id: Dependency.where(rubygem_id: id)
        .joins(:version)
-        .where(versions: { indexed: true, position: 0 })


Can you refresh my memory on why position: 0 doesn't work anymore?

This is a change in semantics that seems like an improvement, but we should tackle in a separate PR.

jenshenny · 2026-07-27T20:51:27Z

+    perform_enqueued_jobs
    perform_enqueued_jobs

    get rubygem_path("sandworm")


Calling perform_enqueued_jobs twice is a smell, can we wrap get rubygem_path("sandworm") in a perform_enqueued_jobs do block?

Assisted-By: devx/3789dae8-fc4d-4cf5-8114-296b69295713

colby-swandale · 2026-08-13T06:48:21Z

+    logger.info { "Reordering versions for gem: #{rubygem.name} (#{rubygem.id})" }
+
+    StatsD.measure("reorder_versions.duration") do
+      rubygem.reorder_versions


This method needs to be called inside a transaction

colby-swandale · 2026-08-13T06:57:50Z

    Rubygem.where(
      id: Dependency.where(rubygem_id: id)
        .joins(:version)
-        .where(versions: { indexed: true, position: 0 })


This is a change in semantics that seems like an improvement, but we should tackle in a separate PR.

colby-swandale · 2026-08-13T07:01:54Z

+    latest_version = rubygem.reload.most_recent_version
+    SetLinksetHomeJob.perform_later(version: latest_version) if latest_version
+
+    logger.info { "Reordering complete for #{rubygem.name}" }


We'll need to call GemCachePurger.call here so we can serve the freshly ordered gem page to Fastly

colby-swandale · 2026-08-13T07:36:38Z

+
+  queue_as :default
+
+  retry_on ActiveRecord::Deadlocked, wait: :polynomially_longer, attempts: 3


If this job exhausts its retries and gets discarded, Indexer and SetLinksetHomeJob never run, since they're only enqueued from in here. The full index (specs.4.8.gz and friends) doesn't get regenerated, so old-style gem clients can't see the version at all, while the compact index resolves it fine. The version is also left at position: nil, which keeps it out of latest_specs.4.8.gz even if something else triggers a reindex.

That being said, I don't think we need to introduce a reconciler in this PR. Retries should it rare and we can watch reorder_versions.error / good_job.discarded on our side. Could you add a sentence to the job noting the trade-off, so it's written down somewhere that a discarded reorder means the full index diverges until the next push?

Two small things while we're here: retry_on ActiveRecord::Deadlocked, attempts: 3 lowers the app-wide default of 5 from ApplicationJob, which I don't think was intended, so it can just be removed. And the rescue StandardError duplicates what ApplicationJob's after_discard already reports, so it could go too. No strong feelings on that one.

colby-swandale · 2026-08-13T07:46:16Z

  has_one :most_recent_version,
    lambda {
      order(
+        # During the async reorder window, a freshly pushed version can have a nil


I don't think the comment matches the behaviour here. I've had Claude trace through the ordering change. The latest sort key still comes before position, so for a gem that already has a latest: true release, the old latest keeps winning until ReorderVersionsJob runs. The fresh nil-position version isn't treated as newest. NULLS FIRST only decides anything when no latest flag is in play, so all-prerelease gems, or ties between concurrently pushed versions.

My bigger question is what drove these two ordering changes, the new indexed-first key and NULLS FIRST. As far as I can tell nothing in this PR needs them. During the async window the old ordering just shows the previous latest until the job lands, which is what users saw pre-PR anyway, and by the time this scope is read inside the job the positions are already assigned.

The indexed-first key does change what most_recent_version returns for some gems though, like a prerelease-only gem whose newest version was yanked, and that feeds the gem page title and the API payload.

If these were fixing something you hit while tophatting, I'd love to know what. If they're an intentional improvement, same suggestion as the position: 0 → latest: true change and let's pull it into its own PR.

colby-swandale · 2026-08-13T07:48:45Z

+      end
+    end
+
+    should "discard job if rubygem no longer exists" do


Can you double-check this test is accurate? I don't think this is actually hitting the ReorderVersionsJob the right way to verify the tests intention.

girachawda force-pushed the fix-deadlock branch 3 times, most recently from 8d1f860 to 11992b8 Compare June 30, 2026 20:35

girachawda marked this pull request as ready for review June 30, 2026 20:42

girachawda force-pushed the fix-deadlock branch from 11992b8 to 386afc7 Compare June 30, 2026 20:43

girachawda commented Jul 2, 2026

View reviewed changes

Comment thread test/functional/api/v1/rubygems_controller_test.rb Outdated

jenshenny reviewed Jul 2, 2026

View reviewed changes

OughtPuts reviewed Jul 3, 2026

View reviewed changes

Comment thread test/jobs/reorder_versions_job_test.rb Outdated

OughtPuts reviewed Jul 3, 2026

View reviewed changes

girachawda requested review from OughtPuts and jenshenny July 8, 2026 20:03

girachawda force-pushed the fix-deadlock branch 3 times, most recently from ac77de4 to 56cacda Compare July 8, 2026 21:05

jenshenny reviewed Jul 27, 2026

View reviewed changes

girachawda force-pushed the fix-deadlock branch 2 times, most recently from 618754e to 56cacda Compare August 4, 2026 20:46

colby-swandale requested review from colby-swandale and removed request for OughtPuts August 6, 2026 00:34

Fix deadlock when pushing multiple gem versions concurrently

7161d52

Assisted-By: devx/3789dae8-fc4d-4cf5-8114-296b69295713

girachawda force-pushed the fix-deadlock branch from bcbb5f9 to 7161d52 Compare August 13, 2026 04:08

colby-swandale reviewed Aug 13, 2026

View reviewed changes


		queue_as :default

		retry_on ActiveRecord::Deadlocked, wait: :polynomially_longer, attempts: 3

Uh oh!

Uh oh!

Conversation

girachawda commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Flow

Tophat

Uh oh!

girachawda commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jenshenny commented Jul 1, 2026

Uh oh!

girachawda commented Jul 2, 2026

Uh oh!

Uh oh!

jenshenny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OughtPuts commented Jul 3, 2026

Uh oh!

Uh oh!

OughtPuts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

girachawda Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

girachawda commented Jun 25, 2026 •

edited

Loading

girachawda commented Jun 30, 2026 •

edited

Loading

girachawda Jul 8, 2026 •

edited

Loading