From 78fea7c7f2d910c0110d1d0edbcdb1348fe7bd1f Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Fri, 24 Mar 2023 16:32:36 +1100
Subject: [PATCH] Run background reports with Sidekiq, not fork

Forking worked in theory but crashed the browser in system specs. It
also came with many other hurdles and isn't well known solution in the
Rails community. Sidekiq can give us better control over execution
limits as well.
---
 app/controllers/admin/reports_controller.rb |  5 +-
 app/services/job_processor.rb               | 29 -----------
 spec/services/job_processor_spec.rb         | 57 ---------------------
 spec/system/admin/reports_spec.rb           | 15 ++++++
 4 files changed, 17 insertions(+), 89 deletions(-)
 delete mode 100644 app/services/job_processor.rb
 delete mode 100644 spec/services/job_processor_spec.rb

diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb
index fe926a3441..2dc18e4092 100644
--- a/app/controllers/admin/reports_controller.rb
+++ b/app/controllers/admin/reports_controller.rb
@@ -55,11 +55,10 @@ module Admin
 
     def render_report_as(format)
       if OpenFoodNetwork::FeatureToggle.enabled?(:background_reports, spree_current_user)
-        job = ReportJob.new
-        JobProcessor.perform_forked(
-          job,
+        job = ReportJob.perform_later(
           report_class, spree_current_user, params, format
         )
+        sleep 1 until job.done?
 
         # This result has been rendered by Rails in safe mode already.
         job.result.html_safe # rubocop:disable Rails/OutputSafety
diff --git a/app/services/job_processor.rb b/app/services/job_processor.rb
deleted file mode 100644
index 3432921503..0000000000
--- a/app/services/job_processor.rb
+++ /dev/null
@@ -1,29 +0,0 @@
-# frozen_string_literal: true
-
-# Forks into a separate process to contain memory usage and timeout errors.
-class JobProcessor
-  def self.perform_forked(job, *args)
-    # Reports should abort when puma threads are killed to avoid wasting
-    # resources. Nobody would be collecting the result. We still need to
-    # implement a way to email or download reports later.
-    timeout = ENV.fetch("RACK_TIMEOUT_WAIT_TIMEOUT", "30").to_i
-
-    child = fork do
-      Process.setproctitle("Job worker #{job.job_id}")
-      Timeout.timeout(timeout) do
-        job.perform(*args)
-      end
-
-      # Exit is not a good idea within a Rails process but Rubocop doesn't know
-      # that we are in a forked process.
-      exit # rubocop:disable Rails/Exit
-    end
-
-    # Wait for the forked child process to exit.
-    Process.waitpid(child)
-  ensure
-    # If this Puma thread is interrupted then we need to detach the child
-    # process to avoid it becoming a zombie.
-    Process.detach(child)
-  end
-end
diff --git a/spec/services/job_processor_spec.rb b/spec/services/job_processor_spec.rb
deleted file mode 100644
index a643951c24..0000000000
--- a/spec/services/job_processor_spec.rb
+++ /dev/null
@@ -1,57 +0,0 @@
-# frozen_string_literal: true
-
-# We need to configure MiniRacer to allow forking.
-# Otherwise this spec hangs on CI.
-# https://github.com/rubyjs/mini_racer#fork-safety
-require "mini_racer"
-MiniRacer::Platform.set_flags!(:single_threaded)
-
-require 'spec_helper'
-
-class TestJob < ApplicationJob
-  def initialize
-    @file = Tempfile.new("test-job-result")
-    super
-  end
-
-  def perform(message)
-    @file.write(message)
-  end
-
-  def result
-    @file.rewind
-    @file.read
-  end
-end
-
-describe JobProcessor do
-  describe ".perform_forked" do
-    let(:job) { TestJob.new }
-
-    it "executes a job" do
-      JobProcessor.perform_forked(job, "hello")
-
-      expect(job.result).to eq "hello"
-    end
-
-    describe "with other unrelated children" do
-      let(:start_time) { Time.zone.now }
-      let(:end_time) { Time.zone.now }
-
-      # We made a mistake waiting for all forked processes.
-      # Now starting an unrelated process in a similar way.
-      around do |example|
-        start_time
-        other_process = fork { sleep 10 }
-        example.run
-        Process.kill("QUIT", other_process)
-      end
-
-      it "returns as soon as the job is done" do
-        JobProcessor.perform_forked(job, "hello")
-
-        expect(end_time).to be_within(10.seconds).of start_time
-      end
-    end
-  end
-end
diff --git a/spec/system/admin/reports_spec.rb b/spec/system/admin/reports_spec.rb
index aa06c1d0ac..33a9cae152 100644
--- a/spec/system/admin/reports_spec.rb
+++ b/spec/system/admin/reports_spec.rb
@@ -31,6 +31,21 @@ describe '
     end
   end
 
+  describe "Background processing" do
+    before do
+      Flipper.enable(:background_reports)
+      ActiveJob::Base.queue_adapter.perform_enqueued_jobs = true
+    end
+
+    it "can run the customers report" do
+      login_as_admin_and_visit admin_report_path(
+        report_type: :customers, report_subtype: :mailing_list
+      )
+      click_button "Go"
+      expect(page).to have_content "EMAIL FIRST NAME"
+    end
+  end
+
   describe "Can access Customers reports and generate customers report" do
     before do
       login_as_admin_and_visit admin_reports_path