Remove recovery manifest

Author: yfang-figma

ruby/README.md

@@ -47,13 +47,11 @@ previously reserved by that worker:
 ```bash
 minitest-queue --queue redis://example.com run \
   --retry-mode worker-history \
-  --recovery-manifest tmp/ci-queue-recovery.json \
   -Itest test/**/*_test.rb
 ```
 
 The replay uses the existing local retry queue and exits after the worker history
-is exhausted. It does not claim additional work from the shared Redis queue. The
-manifest is written atomically only after a complete run.
+is exhausted. It does not claim additional work from the shared Redis queue.
 
 
 If you'd like to centralize the error reporting you can do so with:

ruby/lib/ci/queue/configuration.rb

@@ -15,7 +15,7 @@ class Configuration
       attr_accessor :timing_redis_url
       attr_accessor :write_duration_averages
       attr_accessor :heartbeat_grace_period, :heartbeat_interval
-      attr_accessor :retry_mode, :recovery_manifest, :retry_count
+      attr_accessor :retry_mode
       attr_reader :circuit_breakers
       attr_writer :seed, :build_id
       attr_writer :queue_init_timeout, :report_timeout, :inactive_workers_timeout
@@ -31,7 +31,6 @@ def from_env(env)
             redis_ttl: env['CI_QUEUE_REDIS_TTL']&.to_i ||  8 * 60 * 60,
             known_flaky_tests: load_known_flaky_tests(env['CI_QUEUE_KNOWN_FLAKY_TESTS']),
             branch: env['BUILDKITE_BRANCH'],
-            retry_count: env['BUILDKITE_RETRY_COUNT'].to_i,
           )
         end
 
@@ -69,9 +68,7 @@ def initialize(
         timing_redis_url: nil,
         heartbeat_grace_period: 30,
         heartbeat_interval: 10,
-        retry_mode: :failures,
-        recovery_manifest: nil,
-        retry_count: 0
+        retry_mode: :failures
       )
         @build_id = build_id
         @circuit_breakers = [CircuitBreaker::Disabled]
@@ -111,8 +108,6 @@ def initialize(
         @heartbeat_grace_period = heartbeat_grace_period
         @heartbeat_interval = heartbeat_interval
         @retry_mode = retry_mode
-        @recovery_manifest = recovery_manifest
-        @retry_count = retry_count
       end
 
       def queue_init_timeout

ruby/lib/ci/queue/redis/retry.rb

@@ -5,19 +5,9 @@ module Redis
       class Retry < Static
         def initialize(tests, config, redis:)
           @redis = redis
-          @poll_completed = false
           super(tests, config)
         end
 
-        def poll_completed?
-          @poll_completed
-        end
-
-        def poll
-          super
-          @poll_completed = exhausted?
-        end
-
         def build
           @build ||= CI::Queue::Redis::BuildRecord.new(self, redis, config)
         end

ruby/lib/ci/queue/redis/worker.rb

@@ -145,9 +145,8 @@ def retrying?
           end
         end
 
-        def retry_queue(scope: :failures)
-          return worker_history_retry_queue if scope == :worker_history
-          raise ArgumentError, "Unknown retry scope: #{scope}" unless scope == :failures
+        def retry_queue
+          return worker_history_retry_queue if config.retry_mode == :worker_history
 
           failures = build.failed_tests.to_set
           log = redis.lrange(key('worker', worker_id, 'queue'), 0, -1)

ruby/lib/minitest/queue.rb

@@ -15,7 +15,6 @@
 require 'minitest/queue/grind_reporter'
 require 'minitest/queue/test_time_recorder'
 require 'minitest/queue/test_time_reporter'
-require 'minitest/queue/recovery_reporter'
 
 module Minitest
   class Requeue < Skip

ruby/lib/minitest/queue/recovery_reporter.rb

@@ -4,7 +4,6 @@
 require 'minitest/queue'
 require 'ci/queue'
 require 'digest/md5'
-require 'fileutils'
 require 'minitest/reporters/bisect_reporter'
 require 'minitest/reporters/statsd_reporter'
 
@@ -50,17 +49,16 @@ def retry_command
 
       def run_command
         require_worker_id!
-        FileUtils.rm_f(queue_config.recovery_manifest) if queue_config.recovery_manifest
         if queue.retrying? || retry?
           if queue.expired?
             abort! "The test run is too old and can't be retried"
           end
           reset_counters
-          retry_queue = retry_queue_for_retry
+          retry_queue = queue.retry_queue
           if retry_queue.exhausted?
             puts "The retry queue does not contain any failure, we'll process the main queue instead."
           else
-            if worker_history_retry?
+            if queue_config.retry_mode == :worker_history
               puts "Replaying this worker's reservation history."
             else
               puts "Retrying failed tests."
@@ -87,13 +85,6 @@ def run_command
         if queue_config.statsd_endpoint
           reporters << Minitest::Reporters::StatsdReporter.new(statsd_endpoint: queue_config.statsd_endpoint)
         end
-        if queue_config.recovery_manifest
-          reporters << RecoveryReporter.new(
-            path: queue_config.recovery_manifest,
-            queue: queue,
-            config: queue_config
-          )
-        end
         Minitest.queue_reporters = reporters
 
         trap('TERM') { Minitest.queue.shutdown! }
@@ -337,22 +328,6 @@ def reset_counters
         queue.build.reset_stats(BuildStatusRecorder::COUNTERS)
       end
 
-      def worker_history_retry?
-        queue_config.retry_mode == :worker_history
-      end
-
-      def retry_queue_for_retry
-        if worker_history_retry?
-          abort! 'Worker history recovery requires a distributed Redis queue' unless queue.distributed?
-
-          queue.retry_queue(scope: :worker_history)
-        else
-          queue.retry_queue
-        end
-      rescue CI::Queue::Redis::WorkerHistoryError => error
-        abort! error.message
-      end
-
       def populate_queue
         Minitest.queue.populate(Minitest.loaded_tests, random: ordering_seed, &:id)
       end
@@ -531,14 +506,6 @@ def parser
             queue_config.retry_mode = mode.tr('-', '_').to_sym
           end
 
-          help = <<~EOS
-            Write worker-history recovery details to a JSON file after a completed run.
-          EOS
-          opts.separator ""
-          opts.on('--recovery-manifest PATH', help) do |path|
-            queue_config.recovery_manifest = path
-          end
-
           help = <<~EOS
             Defines how many time a single test can be requeued.
             Defaults to 0.

ruby/lib/minitest/queue/runner.rb

@@ -264,45 +264,20 @@ def test_retry_fails_when_test_run_is_expired
       assert_equal "The test run is too old and can't be retried", output
     end
 
-    def test_worker_history_retry_writes_recovery_manifest
-      Dir.mktmpdir do |directory|
-        manifest_path = File.join(directory, 'recovery.json')
-        run_worker_history_worker(manifest_path, retry_count: 0)
-
-        first_manifest = JSON.parse(File.read(manifest_path))
-        assert_equal 0, first_manifest['retry_count']
-        refute first_manifest['resumed_shared_queue']
-        assert first_manifest['replay_completed']
-
-        out, = run_worker_history_worker(manifest_path, retry_count: 1)
-
-        assert_includes out, "Replaying this worker's reservation history."
-        retry_manifest = JSON.parse(File.read(manifest_path))
-        assert_equal 1, retry_manifest['schema_version']
-        assert_equal '1', retry_manifest['worker_id']
-        assert_equal 1, retry_manifest['retry_count']
-        refute retry_manifest['resumed_shared_queue']
-        assert retry_manifest['replay_completed']
-      end
+    def test_worker_history_retry_replays_worker_reservations
+      run_worker_history_worker(retry_count: 0)
+      out, = run_worker_history_worker(retry_count: 1)
+
+      assert_predicate $?, :success?
+      assert_includes out, "Replaying this worker's reservation history."
     end
 
-    def test_worker_history_retry_removes_stale_manifest_when_history_is_missing
-      run_worker_history_worker(nil, retry_count: 0, build_id: 'missing-history', worker_id: '1')
-
-      Dir.mktmpdir do |directory|
-        manifest_path = File.join(directory, 'recovery.json')
-        File.write(manifest_path, '{"stale":true}')
-        out, = run_worker_history_worker(
-          manifest_path,
-          retry_count: 1,
-          build_id: 'missing-history',
-          worker_id: '2'
-        )
+    def test_worker_history_retry_fails_when_history_is_missing
+      run_worker_history_worker(retry_count: 0, build_id: 'missing-history', worker_id: '1')
+      out, err = run_worker_history_worker(retry_count: 1, build_id: 'missing-history', worker_id: '2')
 
-        refute_predicate $?, :success?
-        assert_includes out, 'Reservation history is missing for worker 2'
-        refute_path_exists manifest_path
-      end
+      refute_predicate $?, :success?
+      assert_includes out + err, 'Reservation history is missing for worker 2'
     end
 
     def test_retry_report
@@ -829,7 +804,7 @@ def test_utf8_tests_and_marshal
 
     private
 
-    def run_worker_history_worker(manifest_path, retry_count:, build_id: 'worker-history', worker_id: '1')
+    def run_worker_history_worker(retry_count:, build_id: 'worker-history', worker_id: '1')
       args = [
         @exe, 'run',
         '--queue', @redis_url,
@@ -839,7 +814,6 @@ def run_worker_history_worker(manifest_path, retry_count:, build_id: 'worker-his
         '--timeout', '1',
         '--retry-mode', 'worker-history'
       ]
-      args.push('--recovery-manifest', manifest_path) if manifest_path
       args.push('-Itest', 'test/passing_test.rb')
 
       capture_subprocess_io do