In fork_worker mode, worker_culling_strategy "oldest" shouldn't cull …

…worker 0 (#2794)

* When using fork_worker mode, worker_culling_strategy "oldest" shouldn't cull worker 0.

* Consider case where worker0 isn't oldest worker

* Reduce code complexity

* Add code comment

Co-authored-by: shields <shields@tablecheck.com>

lib/puma/cluster.rb

@@ -108,25 +108,36 @@ def spawn_worker(idx, master)
     def cull_workers
       diff = @workers.size - @options[:workers]
       return if diff < 1
+      debug "Culling #{diff} workers"
 
-      debug "Culling #{diff.inspect} workers"
+      workers = workers_to_cull(diff)
+      debug "Workers to cull: #{workers.inspect}"
 
-      workers_to_cull =
-        case @options[:worker_culling_strategy]
-        when :youngest
-          @workers.sort_by(&:started_at)[-diff,diff]
-        when :oldest
-          @workers.sort_by(&:started_at)[0,diff]
-        end
-
-      debug "Workers to cull: #{workers_to_cull.inspect}"
-
-      workers_to_cull.each do |worker|
+      workers.each do |worker|
         log "- Worker #{worker.index} (PID: #{worker.pid}) terminating"
         worker.term
       end
     end
 
+    def workers_to_cull(diff)
+      workers = @workers.sort_by(&:started_at)
+
+      # In fork_worker mode, worker 0 acts as our master process.
+      # We should avoid culling it to preserve copy-on-write memory gains.
+      workers.reject! { |w| w.index == 0 } if @options[:fork_worker]
+
+      workers[cull_start_index(diff), diff]
+    end
+
+    def cull_start_index(diff)
+      case @options[:worker_culling_strategy]
+      when :oldest
+        0
+      else # :youngest
+        -diff
+      end
+    end
+
     # @!attribute [r] next_worker_index
     def next_worker_index
       occupied_positions = @workers.map(&:index)

test/test_integration_cluster.rb

@@ -401,6 +401,25 @@ def test_culling_strategy_oldest
     assert_match(/Worker 0 \(PID: \d+\) terminating/, line)
   end
 
+  def test_culling_strategy_oldest_fork_worker
+    cli_server "-w 2 test/rackup/hello.ru", config: <<RUBY
+worker_culling_strategy :oldest
+fork_worker
+RUBY
+
+    get_worker_pids # to consume server logs
+
+    Process.kill :TTIN, @pid
+
+    line = @server.gets
+    assert_match(/Worker 2 \(PID: \d+\) booted in/, line)
+
+    Process.kill :TTOU, @pid
+
+    line = @server.gets
+    assert_match(/Worker 1 \(PID: \d+\) terminating/, line)
+  end
+
   private
 
   def worker_timeout(timeout, iterations, details, config)