Skip to content

objectpool: Fix error code when creating a pool with existing target dir

Patrick Steinhardt requested to merge pks-objectpool-target-exists into master

When we create an object pool we fail the RPC in case the target directory exists already, but only if it is not empty. While we still do this, the refactoring in 37f859c6 (objectpool: Refactor Create() to act as a constructor, 2022-12-05) caused us to start returning an Internal error code instead of FailedPrecondition, which broke tests in Rails:

RSpec::Retry: 2nd try ./spec/lib/gitlab/git/object_pool_spec.rb:30
Failures:
  1) Gitlab::Git::ObjectPool#create when the pool already exists raises an FailedPrecondition
     Failure/Error:
       expect do
         subject.create # rubocop:disable Rails/SaveBang
       end.to raise_error(GRPC::FailedPrecondition)
       expected GRPC::FailedPrecondition, got #<GRPC::Internal: 13:cloning to pool: exit status 128, stderr: "fatal: destination path '/builds/gitl...97f2f9716ff66e9b69c05ddd09.git' already exists and is not an empty directory.\n"","grpc_status":13}> with backtrace:
         # ./lib/gitlab/gitaly_client.rb:157:in `execute'
         # ./lib/gitlab/gitaly_client/call.rb:18:in `block in call'
         # ./lib/gitlab/gitaly_client/call.rb:55:in `recording_request'
         # ./lib/gitlab/gitaly_client/call.rb:17:in `call'
         # ./lib/gitlab/gitaly_client.rb:147:in `call'
         # ./lib/gitlab/gitaly_client/object_pool_service.rb:21:in `block in create'
         # ./lib/gitlab/gitaly_client.rb:513:in `with_feature_flag_actors'
         # ./lib/gitlab/gitaly_client/object_pool_service.rb:20:in `create'
         # ./lib/gitlab/git/object_pool.rb:23:in `create'
         # ./spec/lib/gitlab/git/object_pool_spec.rb:32:in `block (5 levels) in <main>'
         # ./spec/lib/gitlab/git/object_pool_spec.rb:31:in `block (4 levels) in <main>'
         # ./spec/spec_helper.rb:413:in `block (3 levels) in <top (required)>'
         # ./spec/support/sidekiq_middleware.rb:18:in `with_sidekiq_server_middleware'
         # ./spec/spec_helper.rb:405:in `block (2 levels) in <top (required)>'
         # ./spec/spec_helper.rb:401:in `block (3 levels) in <top (required)>'
         # ./lib/gitlab/application_context.rb:59:in `with_raw_context'
         # ./spec/spec_helper.rb:401:in `block (2 levels) in <top (required)>'
         # ./spec/spec_helper.rb:241:in `block (2 levels) in <top (required)>'
         # ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>'
         # ./spec/support/flaky_tests.rb:27:in `block (2 levels) in <main>'
         # ./spec/support/database/prevent_cross_joins.rb:106:in `block (3 levels) in <main>'
         # ./spec/support/database/prevent_cross_joins.rb:60:in `with_cross_joins_prevented'
         # ./spec/support/database/prevent_cross_joins.rb:106:in `block (2 levels) in <main>'
     # ./spec/lib/gitlab/git/object_pool_spec.rb:31:in `block (4 levels) in <main>'
     # ./spec/spec_helper.rb:413:in `block (3 levels) in <top (required)>'
     # ./spec/support/sidekiq_middleware.rb:18:in `with_sidekiq_server_middleware'
     # ./spec/spec_helper.rb:405:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:401:in `block (3 levels) in <top (required)>'
     # ./lib/gitlab/application_context.rb:59:in `with_raw_context'
     # ./spec/spec_helper.rb:401:in `block (2 levels) in <top (required)>'
     # ./spec/spec_helper.rb:241:in `block (2 levels) in <top (required)>'
     # ./spec/support/system_exit_detected.rb:7:in `block (2 levels) in <main>'
     # ./spec/support/flaky_tests.rb:27:in `block (2 levels) in <main>'
     # ./spec/support/database/prevent_cross_joins.rb:106:in `block (3 levels) in <main>'
     # ./spec/support/database/prevent_cross_joins.rb:60:in `with_cross_joins_prevented'
     # ./spec/support/database/prevent_cross_joins.rb:106:in `block (2 levels) in <main>'

Fix this bug by explicitly checking whether the target directory exists already, in which case we now return the correct error code again. Amend our tests to catch any change in behaviour here going forward.

Changelog: fixed

Merge request reports