Merge branch 'bvl-remove-cleanup-feature-flag' into 'master'

Remove feature flag for GPG cleanup

See merge request gitlab-org/gitlab!20718

changelogs/unreleased/bvl-remove-cleanup-feature-flag.yml

+---
+title: Try longer to clean up after using a gpg-keychain and raise exption if the
+  cleanup fails
+merge_request: 20718
+author:
+type: fixed

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

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