Make ObjectPool::DestroyWorker idempotent

Emma Parkrequested to merge
616942/destroy-worker-idempotency into master

What does this MR do and why?

GitLab.com has thousands of orphaned pool_repositories records — rows where the Rails DB and Gitaly disagree about what exists (analysis). One of the identified root causes is ObjectPool::DestroyWorker failing partway through: it performs two non-atomic steps — a Gitaly delete_object_pool RPC followed by a DB pool.destroy. When the DB destroy fails after a successful Gitaly delete, a retry re-runs the RPC against a pool that no longer exists on disk — which succeeds, since Gitaly's DeleteObjectPool returns success for an already-deleted pool. But with only 3 retries, a transient DB failure can exhaust them within minutes and permanently strand the record in obsolete state — 112 such stuck pools exist today (cleanup issue).

As part of fixing that root cause (issue), this MR makes the worker idempotent so retries can safely recover from transient partial failures.

Changes:

  • Declare the worker idempotent! (re-runs are already guarded by find_by_id + obsolete?, and the Gitaly delete is idempotent).
  • Remove the sidekiq_options retry: 3 override so the default 25 retries apply, giving transient DB failures room to recover.

Note: the existing retry: 3 and the Scalability/IdempotentWorker rubocop disable were both introduced by bulk mechanical commits (ab0f9788, f78af0fc) applied to all existing workers at the time, not per-worker decisions.

This is MR 1 of 3 for https://gitlab.com/gitlab-org/gitlab/-/work_items/616942 (fix 2: ObjectPool::DestroyWorker partial failure). It must land before https://gitlab.com/gitlab-org/gitlab/-/work_items/616940 is executed, since that cleanup relies on this worker.

References

How to set up and validate locally

  1. Create a fork of a project so an object pool is created, then mark the pool obsolete in the rails console:

    pool = PoolRepository.last
pool.mark_obsolete

  2. Simulate the pool already being deleted on disk by removing the pool directory under the Gitaly storage path (@pools/...).

  3. Run the worker inline:

    ObjectPool::DestroyWorker.new.perform(pool.id)

  4. Verify the DB record is deleted without raising: PoolRepository.find_by_id(pool.id) # => nil.

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 Emma Park

Merge request reports