Replace rest-client with a persistent HTTP client for worker->web calls

.github/workflows/openstudio-server-tests.yml

@@ -1,12 +1,20 @@
 name: openstudio-server
 
-on: [push, pull_request]
+# Run each workflow once per change: pull_request covers feature branches
+# (pushing a branch with an open PR previously triggered a duplicate push
+# run), push covers the long-lived and release branches that deploy images.
+on:
+  push:
+    branches:
+      - develop
+      - master
+      - '*.*.*'   # release branches, e.g. 3.11.0
+      - '*-LTS'   # e.g. 2.9.X-LTS
+  pull_request:
 
-# example of how to restrict to one branch and push event
-#on:
-#  push:
-#    branches:
-#      - test_branch
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: ${{ github.event_name == 'pull_request' }}
 
 env:
   USE_TESTING_TIMEOUTS: "true"

.github/workflows/security-scan.yml

@@ -1,12 +1,20 @@
 name: docker security scan
 
-on: [push, pull_request]
+# Run each workflow once per change: pull_request covers feature branches
+# (pushing a branch with an open PR previously triggered a duplicate push
+# run), push covers the long-lived and release branches that deploy images.
+on:
+  push:
+    branches:
+      - develop
+      - master
+      - '*.*.*'   # release branches, e.g. 3.11.0
+      - '*-LTS'   # e.g. 2.9.X-LTS
+  pull_request:
 
-# example of how to restrict to one branch and push event
-#on:
-#  push:
-#    branches:
-#      - test_branch
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: ${{ github.event_name == 'pull_request' }}
 
 env:
   USE_TESTING_TIMEOUTS: "true"

docker/server/run-server-tests.sh

@@ -19,9 +19,20 @@ do
 done
 
 #cd /opt/openstudio/server && bundle exec rspec; (( exit_status = exit_status || $? ))
+# Socket-level specs for the persistent worker->web HTTP client. Fast, no stack needed.
+cd /opt/openstudio/server && bundle exec rspec spec/lib/os_http_spec.rb; (( exit_status = exit_status || $? ))
 # Model/request specs for seed.zip upload validation + InitializeAnalysis failure handling (issue #841).
 # These need only rails+mongo, so run them first - they are fast and leave the db empty.
 cd /opt/openstudio/server && bundle exec rspec spec/models/analysis_init_spec.rb spec/requests/analyses_upload_spec.rb; (( exit_status = exit_status || $? ))
+# Job-level integration specs for RunSimulateDataPoint (dj + resque inline). They run the
+# full job - including the persistent worker->web HTTP client - against an in-process app.
+# Their after(:all) hooks destroy projects/paperclip assets so later specs start empty (#841).
+cd /opt/openstudio/server && bundle exec rspec spec/features/dj_run_simulation_data_point_spec.rb; (( exit_status = exit_status || $? ))
+cd /opt/openstudio/server && bundle exec rspec spec/features/resque_run_simulation_data_point_spec.rb; (( exit_status = exit_status || $? ))
+# The in-process spec apps above run as root and can leave a root-owned 0755
+# assets/data_points dir; remove it so the live app (nobody) can recreate it
+# writable, or the docker_stack specs below fail on result-file uploads (#841).
+rm -rf /mnt/openstudio/server/assets/data_points
 # Run only the algorithm specs. The other features/*_spec files should probably disappear and capybara/gecko
 # can be removed.
 cd /opt/openstudio/server && bundle exec rspec spec/features/docker_stack_custom_gems.rb; (( exit_status = exit_status || $? ))

server/Gemfile

@@ -103,7 +103,6 @@ gem 'bson', '~> 4.14.1' # bson 4.6.0 requires ruby >= 2.3.0
 gem 'msgpack', '~> 1.4.5'
 gem 'multi_json', '~> 1.15.0'
 gem 'nio4r', '~> 2.5.9'
-gem 'rest-client', '~> 2.1.0'
 
 # add to Gemfile to make available to Ruby scripts running via initialize/finalize scripts
 gem 'rubyXL', '~> 3.4.17'
@@ -142,6 +141,9 @@ group :development, :test do
   gem 'capybara', '~> 3.40'
   gem 'coveralls', '0.7.1', require: false
   gem 'public_suffix', '~> 5.0.5'
+  # test-only HTTP client for the live-stack feature specs; app code uses
+  # the persistent client in config/initializers/http_client.rb
+  gem 'rest-client', '~> 2.1.0'
   gem 'rspec', '~> 3.13.0'
   gem 'rspec-rails', '~> 5.0.3'
   gem 'rspec-retry', '~> 0.6.2'

server/app/jobs/dj_jobs/run_simulate_data_point.rb

@@ -81,6 +81,10 @@ def perform
                       }
                     ] }
         report_file = "#{simulation_dir}/out.osw"
+        # simulation_dir can be missing here (initialize_worker failed, or the
+        # analysis dir was deleted out from under us); don't let the error
+        # report itself crash with ENOENT and mask the real failure.
+        FileUtils.mkdir_p simulation_dir unless Dir.exist? simulation_dir
         File.open(report_file, 'wb') do |f|
           f.puts ::JSON.pretty_generate(out_osw)
         end
@@ -95,18 +99,18 @@ def perform
       end
 
       # delete any existing data files from the server in case this is a 'rerun'
-      @sim_logger.info 'calling RestClient.delete in case this is a rerun to delete the /result_files directory'
+      @sim_logger.info 'calling HTTP delete in case this is a rerun to delete the /result_files directory'
       post_count = 0
       post_count_max = 50
       begin
         post_count += 1
         @sim_logger.info "delete post_count = #{post_count}; max is 50"
-        RestClient.delete "#{APP_CONFIG['os_server_host_url']}/data_points/#{@data_point.id}/result_files"
+        OsHttp.client.delete("/data_points/#{@data_point.id}/result_files")
       rescue StandardError => e
         sleep Random.new.rand(1.0..10.0)
         retry if post_count <= post_count_max
-        @sim_logger.error "RestClient.delete failed with error #{e.message}"
-        raise "RestClient.delete failed with error #{e.message}"
+        @sim_logger.error "HTTP delete failed with error #{e.message}"
+        raise "HTTP delete failed with error #{e.message}"
       end
       # Download the datapoint to run and save to disk
       url = "#{APP_CONFIG['os_server_host_url']}/data_points/#{@data_point.id}.json"
@@ -116,12 +120,12 @@ def perform
       begin
         post_count += 1
         @sim_logger.info "get url post_count = #{post_count}"
-        r = RestClient.get url
+        r = OsHttp.client.get(url)
       rescue StandardError => e
         sleep Random.new.rand(1.0..10.0)
         retry if post_count <= post_count_max
-        @sim_logger.error "RestClient.get url failed with error #{e.message}"
-        raise "RestClient.get url failed with error #{e.message}"
+        @sim_logger.error "HTTP get failed with error #{e.message}"
+        raise "HTTP get failed with error #{e.message}"
       end
       raise 'Datapoint JSON could not be downloaded' unless r.code == 200
       # Parse to JSON to save it again with nice formatting
@@ -471,7 +475,7 @@ def initialize_worker
           begin
             Timeout.timeout(@data_point.analysis.initialize_worker_timeout) do
               json_download_count += 1
-              a = RestClient.get analysis_json_url
+              a = OsHttp.client.get(analysis_json_url)
               raise "Analysis JSON could not be downloaded - responce code of #{a.code} received." unless a.code == 200
 
               # Parse to JSON to save it again with nice formatting
@@ -595,7 +599,10 @@ def extract_archive(archive_filename, destination, overwrite = true)
       end
     end
 
-    def upload_file(filename, type, display_name = nil, content_type = nil)
+    # _content_type is kept for call-site compatibility: rest-client sent it
+    # as a form field the server never read. The multipart part Content-Type
+    # is now derived from the file extension (see OsHttp::Client).
+    def upload_file(filename, type, display_name = nil, _content_type = nil)
       upload_file_attempt = 0
       upload_file_max_attempt = 4
       display_name ||= File.basename(filename, '.*')
@@ -608,18 +615,10 @@ def upload_file(filename, type, display_name = nil, content_type = nil)
       begin
         Timeout.timeout(@data_point.analysis.upload_results_timeout) do
           upload_file_attempt += 1
-          if content_type
-            res = RestClient.post(data_point_url,
-                                  file: { display_name: display_name,
-                                          type: type,
-                                          attachment: File.new(filename, 'rb') },
-                                  content_type: content_type)
-          else
-            res = RestClient.post(data_point_url,
-                                  file: { display_name: display_name,
-                                          type: type,
-                                          attachment: File.new(filename, 'rb') })
-          end
+          res = OsHttp.client.post_form(data_point_url,
+                                        file: { display_name: display_name,
+                                                type: type,
+                                                attachment: File.new(filename, 'rb') })
           @sim_logger.info "Saving report responded with #{res}"
           return true
         end

server/config/initializers/http_client.rb

@@ -0,0 +1,187 @@
+# frozen_string_literal: true
+
+# *******************************************************************************
+# OpenStudio(R), Copyright (c) Alliance for Sustainable Energy, LLC.
+# See also https://openstudio.net/license
+# *******************************************************************************
+
+# Per-process persistent HTTP client for worker -> web-server calls.
+#
+# Replaces rest-client for the DjJobs::RunSimulateDataPoint call sites. Each
+# rest-client call opened a fresh TCP connection and left a TIME_WAIT socket
+# behind, and every closed connection holds an nf_conntrack entry for ~120s.
+# At high worker counts that churn can exhaust nf_conntrack_max on the nodes
+# hosting the web tier. Routing the calls through one persistent connection
+# per process cuts the connection churn by roughly an order of magnitude.
+#
+# Behavior parity with the previous rest-client usage:
+#   * `.get` / `.delete` / `.post_form` return a Response exposing `.code`
+#     (Integer) and `.body`, usable with JSON.parse (via `to_str`) and string
+#     interpolation (via `to_s`).
+#   * Non-2xx responses raise OsHttp::Error (a StandardError), matching
+#     rest-client, so the existing `rescue StandardError` retry loops behave
+#     the same.
+#   * Multipart file parts carry the same filename (basename) and Content-Type
+#     (mime guess by extension) that rest-client produced. The part
+#     Content-Type is load-bearing: DataPointsController#download_result_file
+#     stores it and serves files inline only for text/html, application/json,
+#     and text/plain.
+#
+# Resque forks a child per job; the client is built lazily so each child opens
+# its own connection on first use. Delayed Job workers are long-lived and
+# reuse the connection across jobs.
+#
+# Usage:
+#   OsHttp.client.get("#{APP_CONFIG['os_server_host_url']}/data_points/#{id}.json")
+#   OsHttp.client.delete("/data_points/#{id}/result_files")
+#   OsHttp.client.post_form(url, file: { display_name: name, type: type,
+#                                        attachment: File.new(path, 'rb') })
+
+require 'net/http/persistent'
+require 'uri'
+
+module OsHttp
+  class Error < StandardError
+    attr_reader :code, :body
+
+    def initialize(code, body, msg)
+      @code = code
+      @body = body
+      super(msg)
+    end
+  end
+
+  # Duck-types the subset of RestClient::Response the worker code relies on.
+  class Response
+    attr_reader :code, :body
+
+    def initialize(code, body)
+      @code = code
+      @body = body
+    end
+
+    def to_s
+      body.to_s
+    end
+
+    # Keeps JSON.parse(response) working.
+    def to_str
+      body.to_s
+    end
+  end
+
+  class Client
+    # Content-Type for multipart file parts, matching what rest-client's
+    # MIME::Types.type_for guess produced for the file types the worker
+    # uploads. Unlisted extensions fall back to application/octet-stream,
+    # which is also what rest-client did.
+    PART_CONTENT_TYPES = {
+      '.html' => 'text/html',
+      '.json' => 'application/json',
+      '.csv' => 'text/csv',
+      '.xml' => 'text/xml',
+      '.zip' => 'application/zip',
+      '.txt' => 'text/plain',
+      '.log' => 'text/plain',
+      '.gz' => 'application/gzip'
+    }.freeze
+    DEFAULT_PART_CONTENT_TYPE = 'application/octet-stream'
+
+    # idle_timeout must stay below Puma's persistent timeout (20s default) so
+    # the client reopens idle connections instead of racing a server-side
+    # close, which net/http cannot transparently retry for POSTs.
+    def initialize(base_url:, idle_timeout: 15, read_timeout: 120, open_timeout: 15, pool_size: 1)
+      @base = URI(base_url)
+      @http = Net::HTTP::Persistent.new(name: 'os-server', pool_size: pool_size)
+      @http.idle_timeout = idle_timeout
+      @http.read_timeout = read_timeout
+      @http.open_timeout = open_timeout
+    end
+
+    def get(path, headers = {})
+      request(Net::HTTP::Get.new(uri_for(path).request_uri, headers))
+    end
+
+    def delete(path, headers = {})
+      request(Net::HTTP::Delete.new(uri_for(path).request_uri, headers))
+    end
+
+    # Multipart form POST, shaped like the rest-client Hash payloads it
+    # replaces: { file: { display_name: ..., attachment: File } } becomes
+    # file[display_name]=... / file[attachment]=<upload>, matching the
+    # Rails nested-params convention the controllers expect.
+    def post_form(path, form_hash, headers = {})
+      req = Net::HTTP::Post.new(uri_for(path).request_uri, headers)
+      req.set_form(flatten_form(form_hash), 'multipart/form-data')
+      request(req)
+    end
+
+    def shutdown
+      @http.shutdown
+    rescue StandardError
+      # nothing useful to do at process exit
+    end
+
+    private
+
+    def uri_for(path)
+      path.to_s.start_with?('http') ? URI(path) : URI.join(@base.to_s, path)
+    end
+
+    def request(req)
+      res = @http.request(@base, req)
+      unless res.is_a?(Net::HTTPSuccess)
+        raise Error.new(res.code.to_i, res.body, "HTTP #{res.code} on #{req.method} #{req.path}")
+      end
+
+      Response.new(res.code.to_i, res.body)
+    end
+
+    def flatten_form(hash)
+      hash.flat_map do |k, v|
+        if v.is_a?(Hash)
+          v.map { |sub_k, sub_v| form_entry("#{k}[#{sub_k}]", sub_v) }
+        else
+          [form_entry(k.to_s, v)]
+        end
+      end
+    end
+
+    # Net::HTTP#set_form entries are [name, value] or [name, IO, opts]. The
+    # opts keys must be Symbols - String keys are silently ignored and the
+    # part falls back to application/octet-stream. Non-IO values must be
+    # Strings; set_form raises TypeError on nil.
+    def form_entry(key, value)
+      if value.respond_to?(:read) && value.respond_to?(:path)
+        [key, value, { filename: File.basename(value.path), content_type: part_content_type(value.path) }]
+      else
+        [key, value.to_s]
+      end
+    end
+
+    def part_content_type(path)
+      PART_CONTENT_TYPES.fetch(File.extname(path).downcase, DEFAULT_PART_CONTENT_TYPE)
+    end
+  end
+
+  # Lazily-constructed singleton: this file must not depend on APP_CONFIG load
+  # order, and forked workers should open their own connection on first use.
+  # Rebuilt if os_server_host_url changes (the run_simulation feature specs
+  # repoint it at a per-process Capybara server after boot), matching
+  # rest-client's behavior of reading APP_CONFIG on every call.
+  def self.client
+    unless defined?(APP_CONFIG) && APP_CONFIG['os_server_host_url']
+      raise "APP_CONFIG['os_server_host_url'] must be set before using OsHttp.client"
+    end
+
+    url = APP_CONFIG['os_server_host_url']
+    if @client.nil? || @client_base_url != url
+      @client&.shutdown
+      c = Client.new(base_url: url)
+      at_exit { c.shutdown }
+      @client = c
+      @client_base_url = url
+    end
+    @client
+  end
+end