Repository: puma/puma
File: lib/puma/cluster.rb (Ruby)

Code before:
    end

    def all_workers_in_phase?
      @workers.all? { |w| w.phase == @phase }
    end

    def all_workers_idle_timed_out?
      (@workers.map(&:pid) - idle_timed_out_worker_pids).empty?
    end

    def check_workers
      return if @next_check >= Time.now

      @next_check = Time.now + @options[:worker_check_interval]

      timeout_workers
      wait_workers
      cull_workers
      spawn_workers

      if all_workers_booted?
        # If we're running at proper capacity, check to see if
        # we need to phase any workers out (which will restart
        # in the right phase).
        #
        w = @workers.find { |x| x.phase < @phase }

        if w
          log "- Stopping #{w.pid} for phased upgrade..."
          unless w.term?
            w.term
            log "- #{w.signal} sent to #{w.pid}..."
          end
        end
      end

      t = @workers.reject(&:term?)
      t.map!(&:ping_timeout)

      @next_check = [t.min, @next_check].compact.min
    end

    def worker(index, master)
      @workers = []

      @master_read.close
      @suicide_pipe.close
      @fork_writer.close

      pipes = { check_pipe: @check_pipe, worker_write: @worker_write }
      if @options[:fork_worker]

Review comment (bug):
This fixes a race condition bug that is reflected by intermittent failures of the tests I've added here, both on master and this branch, but more frequently on the latter.

`#check_workers` gets called in the `Cluster#run` loop, and when reforking, there could be a point in time where this method gets called between when [worker 0's phase has been incremented](https://github.com/puma/puma/blob/edd2a1728e39d055545fd12ffdfa42d5d4ccbc58/lib/puma/cluster.rb#L294), and the [cluster phase has been incremented](https://github.com/puma/puma/blob/edd2a1728e39d055545fd12ffdfa42d5d4ccbc58/lib/puma/cluster.rb#L49). This results in worker 0 being returned by this find and causes it to incorrectly receive a SIGTERM below just before a refork happens, which is not good.

Seeing as that's a special case where the worker's phase could be greater than the target phase, changing this to `<` ensures that we don't terminate worker 0 in that case while also ensuring other workers continue to be incremented to the right phase.

Code after:
      @workers.find { |w| w.index == idx }
    end

    def all_workers_booted?
      @workers.count { |w| !w.booted? } == 0
    end

    def all_workers_in_phase?
      @workers.all? { |w| w.phase == @phase }
    end

    def all_workers_idle_timed_out?
      (@workers.map(&:pid) - idle_timed_out_worker_pids).empty?
    end

    def check_workers(refork = false)
      return if @next_check >= Time.now

      @next_check = Time.now + @options[:worker_check_interval]

      timeout_workers
      wait_workers
      cull_workers
      spawn_workers

      if all_workers_booted?
        # If we're running at proper capacity, check to see if
        # we need to phase any workers out (which will restart
        # in the right phase).
        #
        w = @workers.find { |x| x.phase < @phase }

        if w
          if refork
            log "- Stopping #{w.pid} for refork..."
          else
            log "- Stopping #{w.pid} for phased upgrade..."
          end

          unless w.term?
            w.term
            log "- #{w.signal} sent to #{w.pid}..."
          end
        end
      end

      t = @workers.reject(&:term?)
      t.map!(&:ping_timeout)

      @next_check = [t.min, @next_check].compact.min
    end