diff --git a/changelogs/unreleased/bvl-remove-cleanup-feature-flag.yml b/changelogs/unreleased/bvl-remove-cleanup-feature-flag.yml new file mode 100644 index 0000000000000000000000000000000000000000..31ec5157b1fcb6d218d415602094c83b8d228b50 --- /dev/null +++ b/changelogs/unreleased/bvl-remove-cleanup-feature-flag.yml @@ -0,0 +1,6 @@ +--- +title: Try longer to clean up after using a gpg-keychain and raise exption if the + cleanup fails +merge_request: 20718 +author: +type: fixed diff --git a/lib/gitlab/gpg.rb b/lib/gitlab/gpg.rb index 1dce26efc658fc6d51f3164f0c958505f082a126..829e64b11a44f3b4b88aad4c1c08a07b4a3097e2 100644 --- a/lib/gitlab/gpg.rb +++ b/lib/gitlab/gpg.rb @@ -5,7 +5,7 @@ module Gpg extend self CleanupError = Class.new(StandardError) - BG_CLEANUP_RUNTIME_S = 2 + BG_CLEANUP_RUNTIME_S = 10 FG_CLEANUP_RUNTIME_S = 0.5 MUTEX = Mutex.new @@ -107,19 +107,18 @@ def optimistic_using_tmp_keychain begin cleanup_tmp_dir(tmp_dir) rescue CleanupError => e + folder_contents = Dir.children(tmp_dir) # This means we left a GPG-agent process hanging. Logging the problem in # sentry will make this more visible. Gitlab::Sentry.track_exception(e, issue_url: 'https://gitlab.com/gitlab-org/gitlab/issues/20918', - extra: { tmp_dir: tmp_dir }) + extra: { tmp_dir: tmp_dir, contents: folder_contents }) end tmp_keychains_removed.increment unless File.exist?(tmp_dir) end def cleanup_tmp_dir(tmp_dir) - return FileUtils.remove_entry(tmp_dir, true) if Feature.disabled?(:gpg_cleanup_retries) - # Retry when removing the tmp directory failed, as we may run into a # race condition: # The `gpg-agent` agent process may clean up some files as well while diff --git a/spec/lib/gitlab/gpg_spec.rb b/spec/lib/gitlab/gpg_spec.rb index 52d6a86f7d06daa0ad1b3256cf02a79d2414980f..cd59339082177fcb7b85e39ad2ff0b6da93bfaa0 100644 --- a/spec/lib/gitlab/gpg_spec.rb +++ b/spec/lib/gitlab/gpg_spec.rb @@ -177,6 +177,25 @@ end.not_to raise_error end + it 'tracks an exception when cleaning up the tmp dir fails' do + expected_exception = described_class::CleanupError.new('cleanup failed') + expected_tmp_dir = nil + + expect(described_class).to receive(:cleanup_tmp_dir).and_raise(expected_exception) + allow(Gitlab::Sentry).to receive(:track_exception) + + described_class.using_tmp_keychain do + expected_tmp_dir = described_class.current_home_dir + FileUtils.touch(File.join(expected_tmp_dir, 'dummy.file')) + end + + expect(Gitlab::Sentry).to have_received(:track_exception).with( + expected_exception, + issue_url: 'https://gitlab.com/gitlab-org/gitlab/issues/20918', + extra: { tmp_dir: expected_tmp_dir, contents: ['dummy.file'] } + ) + end + shared_examples 'multiple deletion attempts of the tmp-dir' do |seconds| let(:tmp_dir) do tmp_dir = Dir.mktmpdir @@ -211,15 +230,6 @@ expect(File.exist?(tmp_dir)).to be false end - - it 'does not retry when the feature flag is disabled' do - stub_feature_flags(gpg_cleanup_retries: false) - - expect(FileUtils).to receive(:remove_entry).with(tmp_dir, true).and_call_original - expect(Retriable).not_to receive(:retriable) - - described_class.using_tmp_keychain {} - end end it_behaves_like 'multiple deletion attempts of the tmp-dir', described_class::FG_CLEANUP_RUNTIME_S