From 65843fbd6852e719ac77993965fdea87e1d4f3f4 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Mon, 9 Jan 2023 11:28:02 +1100
Subject: [PATCH 1/8] Render on-screen report as HTML in renderer

So it can be treated like any other format.
---
 app/controllers/admin/reports_controller.rb |  1 +
 app/views/admin/reports/_table.html.haml    |  2 --
 app/views/admin/reports/show.html.haml      |  5 +----
 lib/reporting/report_renderer.rb            | 14 +++++++++-----
 lib/reporting/report_template.rb            |  2 +-
 5 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb
index af09cfc77c..176b5c02cb 100644
--- a/app/controllers/admin/reports_controller.rb
+++ b/app/controllers/admin/reports_controller.rb
@@ -49,6 +49,7 @@ module Admin
                         I18n.t(:name, scope: [:admin, :reports, @report_type])
                       end
       @rendering_options = rendering_options
+      @table = @report.to_html if request.post?
       @data = Reporting::FrontendData.new(spree_current_user)
     end
   end
diff --git a/app/views/admin/reports/_table.html.haml b/app/views/admin/reports/_table.html.haml
index 091d70e632..fce89c0ebd 100644
--- a/app/views/admin/reports/_table.html.haml
+++ b/app/views/admin/reports/_table.html.haml
@@ -1,5 +1,3 @@
-- report ||= @report
-
 .report__table-container
   %table.report__table
     %thead
diff --git a/app/views/admin/reports/show.html.haml b/app/views/admin/reports/show.html.haml
index 92de53506a..3447401a19 100644
--- a/app/views/admin/reports/show.html.haml
+++ b/app/views/admin/reports/show.html.haml
@@ -19,7 +19,4 @@
   - if request.post?
     %button.btn-print.icon-print{ onclick: "window.print()"}= t(:report_print)
 
-/ We don't want to render data unless search params are supplied.
-/ Compiling data can take a long time.
-- if request.post?
-  = render "table"
+= @table
diff --git a/lib/reporting/report_renderer.rb b/lib/reporting/report_renderer.rb
index 29afa5dd39..c58917a5ae 100644
--- a/lib/reporting/report_renderer.rb
+++ b/lib/reporting/report_renderer.rb
@@ -46,6 +46,14 @@ module Reporting
       public_send("to_#{target_format}")
     end
 
+    def to_html(layout: nil)
+      ApplicationController.render(
+        template: "admin/reports/_table",
+        layout: layout,
+        locals: { report: @report }
+      )
+    end
+
     def to_csv
       SpreadsheetArchitect.to_csv(headers: table_headers, data: table_rows)
     end
@@ -55,11 +63,7 @@ module Reporting
     end
 
     def to_pdf
-      html = ApplicationController.render(
-        template: "admin/reports/_table",
-        layout: "pdf",
-        locals: { report: @report }
-      )
+      html = to_html(layout: "pdf")
       WickedPdf.new.pdf_from_string(html)
     end
 
diff --git a/lib/reporting/report_template.rb b/lib/reporting/report_template.rb
index 9c2cc3fe00..bbac0727e8 100644
--- a/lib/reporting/report_template.rb
+++ b/lib/reporting/report_template.rb
@@ -5,7 +5,7 @@ module Reporting
     include ReportsHelper
     attr_accessor :user, :params, :ransack_params
 
-    delegate :render_as, :as_json, :to_csv, :to_xlsx, :to_pdf, :to_json, to: :renderer
+    delegate :render_as, :as_json, :to_html, :to_csv, :to_xlsx, :to_pdf, :to_json, to: :renderer
     delegate :raw_render?, :html_render?, :display_header_row?, :display_summary_row?, to: :renderer
 
     delegate :rows, :table_rows, :grouped_data, to: :rows_builder

From 0c769706aa01fa2b08e9000bd16399accc4f4101 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Mon, 9 Jan 2023 11:38:19 +1100
Subject: [PATCH 2/8] Split method for easier reading

---
 app/controllers/admin/reports_controller.rb | 6 +-----
 app/controllers/concerns/reports_actions.rb | 8 ++++++++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb
index 176b5c02cb..4258b0fb58 100644
--- a/app/controllers/admin/reports_controller.rb
+++ b/app/controllers/admin/reports_controller.rb
@@ -43,11 +43,7 @@ module Admin
       @report_type = report_type
       @report_subtypes = report_subtypes
       @report_subtype = report_subtype
-      @report_title = if report_subtype
-                        report_subtype_title
-                      else
-                        I18n.t(:name, scope: [:admin, :reports, @report_type])
-                      end
+      @report_title = report_title
       @rendering_options = rendering_options
       @table = @report.to_html if request.post?
       @data = Reporting::FrontendData.new(spree_current_user)
diff --git a/app/controllers/concerns/reports_actions.rb b/app/controllers/concerns/reports_actions.rb
index 4f6a8994c4..7fa6615733 100644
--- a/app/controllers/concerns/reports_actions.rb
+++ b/app/controllers/concerns/reports_actions.rb
@@ -39,6 +39,14 @@ module ReportsActions
     params[:report_subtype] || report_subtypes_codes.first
   end
 
+  def report_title
+    if report_subtype
+      report_subtype_title
+    else
+      I18n.t(:name, scope: [:admin, :reports, report_type])
+    end
+  end
+
   def report_subtype_title
     report_subtypes.select { |_name, key| key.to_sym == report_subtype.to_sym }.first[0]
   end

From 19c4596b9ef83ee1988745c16089aac1fcef409d Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Mon, 9 Jan 2023 12:17:50 +1100
Subject: [PATCH 3/8] Untie report from request object

We want simple report arguments to store it as background job.
---
 app/controllers/admin/reports_controller.rb          |  8 ++++++--
 lib/reporting/report_template.rb                     |  4 ++--
 lib/reporting/reports/enterprise_fee_summary/base.rb |  4 ++--
 spec/lib/reports/xero_invoices_report_spec.rb        | 11 ++++++++---
 4 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb
index 4258b0fb58..f6a30505d3 100644
--- a/app/controllers/admin/reports_controller.rb
+++ b/app/controllers/admin/reports_controller.rb
@@ -19,7 +19,7 @@ module Admin
     end
 
     def show
-      @report = report_class.new(spree_current_user, params, request)
+      @report = report_class.new(spree_current_user, params, render: render_data?)
 
       if report_format.present?
         export_report
@@ -45,8 +45,12 @@ module Admin
       @report_subtype = report_subtype
       @report_title = report_title
       @rendering_options = rendering_options
-      @table = @report.to_html if request.post?
+      @table = @report.to_html if render_data?
       @data = Reporting::FrontendData.new(spree_current_user)
     end
+
+    def render_data?
+      request.post?
+    end
   end
 end
diff --git a/lib/reporting/report_template.rb b/lib/reporting/report_template.rb
index bbac0727e8..16917347fe 100644
--- a/lib/reporting/report_template.rb
+++ b/lib/reporting/report_template.rb
@@ -14,8 +14,8 @@ module Reporting
 
     delegate :formatted_rules, :header_option?, :summary_row_option?, to: :ruler
 
-    def initialize(user, params = {}, request = nil)
-      if request.nil? || request.get?
+    def initialize(user, params = {}, render: false)
+      unless render
         params.reverse_merge!(default_params)
         params[:q] ||= {}
         params[:q].reverse_merge!(default_params[:q]) if default_params[:q].present?
diff --git a/lib/reporting/reports/enterprise_fee_summary/base.rb b/lib/reporting/reports/enterprise_fee_summary/base.rb
index 99d17747a3..837c84df7d 100644
--- a/lib/reporting/reports/enterprise_fee_summary/base.rb
+++ b/lib/reporting/reports/enterprise_fee_summary/base.rb
@@ -6,8 +6,8 @@ module Reporting
       class Base < ReportTemplate
         attr_accessor :permissions, :parameters
 
-        def initialize(user, params = {}, request = nil)
-          super(user, params, request)
+        def initialize(user, params = {}, render: false)
+          super(user, params, render: render)
           p = params[:q]
           if p.present?
             p['start_at'] = p.delete('completed_at_gt')
diff --git a/spec/lib/reports/xero_invoices_report_spec.rb b/spec/lib/reports/xero_invoices_report_spec.rb
index a8670a0f00..ab4a0e3b9b 100644
--- a/spec/lib/reports/xero_invoices_report_spec.rb
+++ b/spec/lib/reports/xero_invoices_report_spec.rb
@@ -26,8 +26,13 @@ module Reporting
 
         describe "summary rows" do
           let(:report) {
-            Base.new user, initial_invoice_number: '', invoice_date: '', due_date: '',
-                           account_code: ''
+            Base.new(user, params)
+          }
+          let(:params) {
+            {
+              initial_invoice_number: '', invoice_date: '', due_date: '',
+              account_code: ''
+            }
           }
           let(:order) { double(:order) }
           let(:summary_rows) { report.__send__(:summary_rows_for_order, order, 1, {}) }
@@ -84,7 +89,7 @@ module Reporting
           end
 
           describe "when an initial invoice number is given" do
-            subject { Base.new user, initial_invoice_number: '123' }
+            subject { Base.new(user, { initial_invoice_number: '123' }) }
 
             it "increments the number by the index" do
               expect(subject.send(:invoice_number_for, order, 456)).to eq(579)

From a177f4c06630555a24ec0cc4aaa68e0b3774e519 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Mon, 9 Jan 2023 15:52:01 +1100
Subject: [PATCH 4/8] Add feature to render reports in the background

This is supposed to lower the memory footprint of all Puma workers. The
reports code will occupy needed memory in one Sidekiq worker instead of
in several Puma processes.

The current code doesn't limit the execution time yet. We either need a
way to terminate the report rendering after a while or send an email
with a link to access a rendered report.
---
 .rubocop_todo.yml                           |  1 +
 app/controllers/admin/reports_controller.rb | 22 +++++++++---
 app/jobs/report_job.rb                      | 34 +++++++++++++++++++
 lib/reporting/report_renderer.rb            |  2 +-
 spec/jobs/report_job_spec.rb                | 37 +++++++++++++++++++++
 5 files changed, 91 insertions(+), 5 deletions(-)
 create mode 100644 app/jobs/report_job.rb
 create mode 100644 spec/jobs/report_job_spec.rb

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index 79c5727eb1..b08465a54b 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -692,6 +692,7 @@ Rails/ApplicationJob:
     - 'app/jobs/heartbeat_job.rb'
     - 'app/jobs/order_cycle_closing_job.rb'
     - 'app/jobs/order_cycle_notification_job.rb'
+    - 'app/jobs/report_job.rb'
     - 'app/jobs/subscription_confirm_job.rb'
     - 'app/jobs/subscription_placement_job.rb'
 
diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb
index f6a30505d3..2dc18e4092 100644
--- a/app/controllers/admin/reports_controller.rb
+++ b/app/controllers/admin/reports_controller.rb
@@ -24,17 +24,17 @@ module Admin
       if report_format.present?
         export_report
       else
-        render_report
+        show_report
       end
     end
 
     private
 
     def export_report
-      send_data @report.render_as(report_format), filename: report_filename
+      send_data render_report_as(report_format), filename: report_filename
     end
 
-    def render_report
+    def show_report
       assign_view_data
       render "show"
     end
@@ -45,12 +45,26 @@ module Admin
       @report_subtype = report_subtype
       @report_title = report_title
       @rendering_options = rendering_options
-      @table = @report.to_html if render_data?
+      @table = render_report_as(:html) if render_data?
       @data = Reporting::FrontendData.new(spree_current_user)
     end
 
     def render_data?
       request.post?
     end
+
+    def render_report_as(format)
+      if OpenFoodNetwork::FeatureToggle.enabled?(:background_reports, spree_current_user)
+        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
+      else
+        @report.render_as(format)
+      end
+    end
   end
 end
diff --git a/app/jobs/report_job.rb b/app/jobs/report_job.rb
new file mode 100644
index 0000000000..7143b3637c
--- /dev/null
+++ b/app/jobs/report_job.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+# Renders a report and saves it to a temporary file.
+class ReportJob < ActiveJob::Base
+  def perform(report_class, user, params, format)
+    report = report_class.new(user, params, render: true)
+    result = report.render_as(format)
+    write(result)
+  end
+
+  def done?
+    @done ||= File.file?(filename)
+  end
+
+  def result
+    @result ||= read_result
+  end
+
+  private
+
+  def write(result)
+    File.write(filename, result)
+  end
+
+  def read_result
+    File.read(filename)
+  ensure
+    File.unlink(filename)
+  end
+
+  def filename
+    Rails.root.join("tmp/report-#{job_id}")
+  end
+end
diff --git a/lib/reporting/report_renderer.rb b/lib/reporting/report_renderer.rb
index c58917a5ae..d301c5a614 100644
--- a/lib/reporting/report_renderer.rb
+++ b/lib/reporting/report_renderer.rb
@@ -4,7 +4,7 @@ require 'spreadsheet_architect'
 
 module Reporting
   class ReportRenderer
-    REPORT_FORMATS = [:csv, :json, :xlsx, :pdf].freeze
+    REPORT_FORMATS = [:csv, :json, :html, :xlsx, :pdf].freeze
 
     def initialize(report)
       @report = report
diff --git a/spec/jobs/report_job_spec.rb b/spec/jobs/report_job_spec.rb
new file mode 100644
index 0000000000..9211ee0e44
--- /dev/null
+++ b/spec/jobs/report_job_spec.rb
@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ReportJob do
+  let(:report_args) { [report_class, user, params, format] }
+  let(:report_class) { Reporting::Reports::UsersAndEnterprises::Base }
+  let(:user) { enterprise.owner }
+  let(:enterprise) { create(:enterprise) }
+  let(:params) { {} }
+  let(:format) { :csv }
+
+  it "generates a report" do
+    job = ReportJob.new
+    job.perform(*report_args)
+    expect_csv_report(job)
+  end
+
+  it "enqueues a job for asynch processing" do
+    job = ReportJob.perform_later(*report_args)
+    expect(job.done?).to eq false
+
+    # This performs the job in the same process but that's good enought for
+    # testing the job code. I hope that we can rely on the job worker.
+    ActiveJob::Base.queue_adapter.perform_enqueued_jobs = true
+    job.retry_job
+
+    expect(job.done?).to eq true
+    expect_csv_report(job)
+  end
+
+  def expect_csv_report(job)
+    table = CSV.parse(job.result)
+    expect(table[0][1]).to eq "Relationship"
+    expect(table[1][1]).to eq "owns"
+  end
+end

From b19456535d5af88cc06e8d7f9db1e2f2701e1366 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Wed, 11 Jan 2023 15:30:58 +1100
Subject: [PATCH 5/8] Isolate report rendering in separate child process

Sidekiq doesn't have any features to limit memory usage or execution
time. We need a separate process for this. Forking avoids the boot time
of a fresh process and copy-on-write ensures minimal memory overheads.
---
 .rubocop_todo.yml                           |  1 +
 app/controllers/admin/reports_controller.rb |  5 ++--
 app/services/job_processor.rb               | 17 +++++++++++
 spec/services/job_processor_spec.rb         | 31 +++++++++++++++++++++
 4 files changed, 52 insertions(+), 2 deletions(-)
 create mode 100644 app/services/job_processor.rb
 create mode 100644 spec/services/job_processor_spec.rb

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index b08465a54b..9724d70967 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -695,6 +695,7 @@ Rails/ApplicationJob:
     - 'app/jobs/report_job.rb'
     - 'app/jobs/subscription_confirm_job.rb'
     - 'app/jobs/subscription_placement_job.rb'
+    - 'spec/services/job_processor_spec.rb'
 
 # Offense count: 1
 # This cop supports unsafe autocorrection (--autocorrect-all).
diff --git a/app/controllers/admin/reports_controller.rb b/app/controllers/admin/reports_controller.rb
index 2dc18e4092..fe926a3441 100644
--- a/app/controllers/admin/reports_controller.rb
+++ b/app/controllers/admin/reports_controller.rb
@@ -55,10 +55,11 @@ module Admin
 
     def render_report_as(format)
       if OpenFoodNetwork::FeatureToggle.enabled?(:background_reports, spree_current_user)
-        job = ReportJob.perform_later(
+        job = ReportJob.new
+        JobProcessor.perform_forked(
+          job,
           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
new file mode 100644
index 0000000000..8eed908b97
--- /dev/null
+++ b/app/services/job_processor.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+# Forks into a separate process to contain memory usage and timeout errors.
+class JobProcessor
+  def self.perform_forked(job, *args)
+    fork do
+      Process.setproctitle("Job worker #{job.job_id}")
+      job.perform(*args)
+
+      # 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
+
+    Process.waitall
+  end
+end
diff --git a/spec/services/job_processor_spec.rb b/spec/services/job_processor_spec.rb
new file mode 100644
index 0000000000..0044038ffe
--- /dev/null
+++ b/spec/services/job_processor_spec.rb
@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+class TestJob < ActiveJob::Base
+  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
+  end
+end

From 26402397ea764c126f4eb5699a3c8f1c3f58ee37 Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Wed, 11 Jan 2023 20:45:45 +1100
Subject: [PATCH 6/8] Stop report workers when parent times out

No need to keep running when nobody is collecting the result (yet).
---
 app/services/job_processor.rb | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/app/services/job_processor.rb b/app/services/job_processor.rb
index 8eed908b97..c7eab01cbf 100644
--- a/app/services/job_processor.rb
+++ b/app/services/job_processor.rb
@@ -3,9 +3,16 @@
 # Forks into a separate process to contain memory usage and timeout errors.
 class JobProcessor
   def self.perform_forked(job, *args)
-    fork do
+    # 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}")
-      job.perform(*args)
+      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.
@@ -13,5 +20,9 @@ class JobProcessor
     end
 
     Process.waitall
+  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

From 6472070517485b97b204b0aaebd9944ab0f9ee3c Mon Sep 17 00:00:00 2001
From: Maikel Linke <maikel@email.org.au>
Date: Fri, 13 Jan 2023 11:39:39 +1100
Subject: [PATCH 7/8] Configure MiniRacer to allow forking

The spec for forking was hanging. This could be revisited after
upgrading to mini_racer 0.6.1.
---
 spec/services/job_processor_spec.rb | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/spec/services/job_processor_spec.rb b/spec/services/job_processor_spec.rb
index 0044038ffe..041c80d5d4 100644
--- a/spec/services/job_processor_spec.rb
+++ b/spec/services/job_processor_spec.rb
@@ -1,5 +1,11 @@
 # 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 < ActiveJob::Base

From 4a8e3a751d0a8c84b0add1f96af0c5730a6b89c4 Mon Sep 17 00:00:00 2001
From: Maikel <maikel@email.org.au>
Date: Wed, 18 Jan 2023 15:12:51 +1100
Subject: [PATCH 8/8] Explain what Process.waitall does

Co-authored-by: David Cook <david@redcliffs.net>
---
 app/services/job_processor.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/services/job_processor.rb b/app/services/job_processor.rb
index c7eab01cbf..10ecb49cb1 100644
--- a/app/services/job_processor.rb
+++ b/app/services/job_processor.rb
@@ -19,6 +19,7 @@ class JobProcessor
       exit # rubocop:disable Rails/Exit
     end
 
+    # Wait for all forked child processes to exit
     Process.waitall
   ensure
     # If this Puma thread is interrupted then we need to detach the child