Fix concurrent pool swaps leaving orphaned pools

What does this MR do and why?

Projects that share Git objects use object pools. When the last member leaves a pool, it should be marked obsolete and scheduled for destruction so unused pools can be cleaned up.

Concurrent storage moves of the last two projects can leave an empty pool behind. Each move can check membership while the other project’s removal is still uncommitted, see that project as a remaining member, and skip marking the pool obsolete. After both transactions commit, the empty old pool remains ready, so normal destruction is never scheduled, leaving an orphaned database pool record and potentially unused data on disk.

This MR retains the original membership check and rechecks the old pool after the storage transaction commits, without adding an explicit row lock. This allows the empty pool to be marked obsolete and scheduled for destruction.

References

How does this fix the race?

The storage-move service captures the old pool via pool_for_cleanup before the tracking transaction. It keeps the existing swap/unlink logic and original membership check, then calls old_pool&.mark_obsolete_if_empty immediately after the transaction commits.

With concurrent moves A and B, both original checks may still see the other project as a member because its departure has not committed yet. The last departure to commit rechecks after its own commit, sees both removals, and marks the empty old pool obsolete.

The helper uses mark_obsolete unless obsolete? || member_projects.exists?. It skips pools that are already obsolete or still have members, without adding an explicit row lock.

Errors from this additional recheck are reported with the pool ID without stopping the remaining storage-move steps. Non-project services return nil from pool_for_cleanup, so they skip pool cleanup.

The query plan

https://console.postgres.ai/gitlab/projects/gitlab-production-main/sessions/60538/commands/165715

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.

  • Local specs, RuboCop, and pre-push hooks passed.
  • Changelog: fixed.
  • Database review: the added membership existence query uses the existing index_projects_on_pool_repository_id index. The query plan linked above shows an index-only scan, 0 heap fetches, and 6.860 ms execution time for a populated pool. No explicit pool row lock is added.
  • MR pipeline passed before the latest comment-only push. The pipeline for that push was skipped.
Edited by Emma Park

Merge request reports

Loading
Loading