diff --git a/.github/workflows/openstudio-server-tests.yml b/.github/workflows/openstudio-server-tests.yml index f412cc031..50aee4909 100644 --- a/.github/workflows/openstudio-server-tests.yml +++ b/.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" diff --git a/.github/workflows/security-scan.yml b/.github/workflows/security-scan.yml index 9a4240ab0..210d99b7b 100644 --- a/.github/workflows/security-scan.yml +++ b/.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" diff --git a/docker/server/run-server-tests.sh b/docker/server/run-server-tests.sh index e02bd5ac1..b6bcdc502 100755 --- a/docker/server/run-server-tests.sh +++ b/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 || $? )) diff --git a/server/Gemfile b/server/Gemfile index bf56146bd..0d59c9dfa 100644 --- a/server/Gemfile +++ b/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' diff --git a/server/app/jobs/dj_jobs/run_simulate_data_point.rb b/server/app/jobs/dj_jobs/run_simulate_data_point.rb index 7289074c8..2323a11ac 100644 --- a/server/app/jobs/dj_jobs/run_simulate_data_point.rb +++ b/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 diff --git a/server/config/initializers/http_client.rb b/server/config/initializers/http_client.rb new file mode 100644 index 000000000..7642481f9 --- /dev/null +++ b/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]=, 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 diff --git a/server/spec/features/dj_run_simulation_data_point_spec.rb b/server/spec/features/dj_run_simulation_data_point_spec.rb index 744281815..f6b0f5842 100644 --- a/server/spec/features/dj_run_simulation_data_point_spec.rb +++ b/server/spec/features/dj_run_simulation_data_point_spec.rb @@ -5,6 +5,8 @@ require 'rails_helper' require 'tempfile' +# rest-client is test-only now; Bundler no longer auto-requires it +require 'rest-client' RSpec.describe DjJobs::RunSimulateDataPoint, type: :feature, foreground: true do before :all do @@ -14,6 +16,10 @@ after :all do Rails.application.config.x.job_manager = @previous_job_manager + # Run in the docker CI job as root: destroy projects so paperclip assets + # are removed and the live-stack specs that follow start empty (#841). + # Inline so the DeleteAnalysis rm_rf cannot fire mid-run of a later spec. + destroy_projects_inline end before do @@ -226,7 +232,9 @@ RSpec.describe DjJobs::RunSimulateDataPoint, type: :feature, depends_resque: true do before do begin - Project.destroy_all + # This group is not tagged foreground: destroy inline or the enqueued + # DeleteAnalysis rm_rf runs later against the shared analysis dir. + destroy_projects_inline rescue Errno::EACCES => e puts 'Cannot unlink files, will try and continue' end @@ -238,6 +246,13 @@ @data_point = @analysis.data_points.first end + after :all do + # Run in the docker CI job as root: destroy projects so paperclip assets + # are removed and the live-stack specs that follow start empty (#841). + # Inline so the DeleteAnalysis rm_rf cannot fire mid-run of a later spec. + destroy_projects_inline + end + it 'launches a script successfully' do job = DjJobs::RunSimulateDataPoint.new(@data_point.id) diff --git a/server/spec/features/resque_run_simulation_data_point_spec.rb b/server/spec/features/resque_run_simulation_data_point_spec.rb index 9afe46b50..66895fd25 100644 --- a/server/spec/features/resque_run_simulation_data_point_spec.rb +++ b/server/spec/features/resque_run_simulation_data_point_spec.rb @@ -4,6 +4,8 @@ # ******************************************************************************* require 'rails_helper' +# rest-client is test-only now; Bundler no longer auto-requires it +require 'rest-client' RSpec.describe ResqueJobs::RunSimulateDataPoint, type: :feature, foreground: true, depends_resque: true do before :all do @@ -13,6 +15,10 @@ after :all do Rails.application.config.x.job_manager = @previous_job_manager + # Run in the docker CI job as root: destroy projects so paperclip assets + # are removed and the live-stack specs that follow start empty (#841). + # Inline so the DeleteAnalysis rm_rf cannot fire mid-run of a later spec. + destroy_projects_inline end before do @@ -122,9 +128,11 @@ puts "datapoint log for #{datapoint_id}: " puts j[:data_point][:sdp_log_file].inspect + # openstudio-workflow no longer logs 'Completed the EnergyPlus simulation'; + # a zero exit from EnergyPlus is logged as "EnergyPlus returned '0'". found_complete = false j[:data_point][:sdp_log_file].each do |line| - if line.include? 'Completed the EnergyPlus simulation' + if line.include? "EnergyPlus returned '0'" found_complete = true end end diff --git a/server/spec/lib/os_http_spec.rb b/server/spec/lib/os_http_spec.rb new file mode 100644 index 000000000..ade298c4b --- /dev/null +++ b/server/spec/lib/os_http_spec.rb @@ -0,0 +1,224 @@ +# ******************************************************************************* +# OpenStudio(R), Copyright (c) Alliance for Sustainable Energy, LLC. +# See also https://openstudio.net/license +# ******************************************************************************* + +# Specs for the persistent worker->web HTTP client. These observe the client +# at the TCP level with a tiny in-process HTTP server, so they need no Rails +# boot, database, or docker stack. + +require 'socket' +require 'json' +require 'tempfile' +require_relative '../../config/initializers/http_client' + +# Minimal single-threaded HTTP/1.1 server that records every request and how +# many TCP connections were accepted. Connection counting is the point: the +# client exists to collapse many worker->web calls onto one connection. +class TinyHttpServer + attr_reader :requests + attr_accessor :response_status, :response_body, :close_connections + + def initialize + @server = TCPServer.new('127.0.0.1', 0) + @accepted = 0 + @requests = [] + @response_status = 200 + @response_body = '{"ok":true}' + @close_connections = false + @thread = Thread.new { accept_loop } + end + + def base_url + "http://127.0.0.1:#{@server.addr[1]}" + end + + def accepted_connections + @accepted + end + + def stop + @server.close + @thread.kill + @thread.join + end + + private + + def accept_loop + loop do + sock = @server.accept + @accepted += 1 + serve_connection(sock) + end + rescue IOError, Errno::EBADF + nil # server socket closed by #stop + end + + def serve_connection(sock) + loop do + request_line = sock.gets("\r\n") + break if request_line.nil? + + headers = {} + while (line = sock.gets("\r\n")) && line != "\r\n" + key, value = line.chomp.split(': ', 2) + headers[key.downcase] = value + end + body = headers['content-length'] ? sock.read(headers['content-length'].to_i) : '' + method, path, = request_line.split(' ') + @requests << { method: method, path: path, headers: headers, body: body } + + connection = @close_connections ? 'close' : 'keep-alive' + sock.write "HTTP/1.1 #{@response_status} STATUS\r\n" \ + "Content-Length: #{@response_body.bytesize}\r\n" \ + "Connection: #{connection}\r\n\r\n#{@response_body}" + break if @close_connections + end + ensure + sock.close unless sock.closed? + end +end + +RSpec.describe OsHttp::Client do + before :each do + @server = TinyHttpServer.new + @client = OsHttp::Client.new(base_url: @server.base_url) + end + + after :each do + @client.shutdown + @server.stop + end + + it 'reuses a single TCP connection across sequential requests' do + # Validates: the conntrack fix itself. rest-client opened one connection + # per call; all calls within a job must now share one socket. + @client.delete("/data_points/123/result_files") + @client.get("#{@server.base_url}/data_points/123.json") + @client.get("#{@server.base_url}/analyses/456.json") + + expect(@server.requests.map { |r| [r[:method], r[:path]] }).to eq( + [%w[DELETE /data_points/123/result_files], + %w[GET /data_points/123.json], + %w[GET /analyses/456.json]] + ) + expect(@server.accepted_connections).to eq(1), + "expected all #{@server.requests.length} requests on one TCP connection, " \ + "got #{@server.accepted_connections}" + end + + it 'reconnects transparently when the server closes the connection' do + # Validates: Puma closes keep-alive connections after its persistent + # timeout; the client must open a new connection, not fail the job. + @server.close_connections = true + + first = @client.get('/data_points/1.json') + second = @client.get('/data_points/2.json') + + expect(first.code).to eq(200) + expect(second.code).to eq(200) + expect(@server.accepted_connections).to eq(2) + end + + it 'returns a rest-client-compatible response' do + # Validates: call sites rely on Integer #code, JSON.parse(response) via + # #to_str, and log interpolation via #to_s. + @server.response_body = '{"status":"completed"}' + + response = @client.get('/data_points/1.json') + + expect(response.code).to eq(200) + expect(JSON.parse(response)['status']).to eq('completed') + expect("#{response}").to eq('{"status":"completed"}') + end + + it 'raises OsHttp::Error rescuable as StandardError on non-2xx responses' do + # Validates: the worker retry loops rescue StandardError; rest-client + # raised on non-2xx, so the replacement must too. + @server.response_status = 422 + @server.response_body = 'unprocessable' + + expect { @client.get('/data_points/1.json') }.to raise_error(StandardError) do |e| + expect(e).to be_a(OsHttp::Error) + expect(e.code).to eq(422) + expect(e.body).to eq('unprocessable') + expect(e.message).to eq('HTTP 422 on GET /data_points/1.json') + end + end + + it 'posts rails-style nested multipart forms with mime-typed file parts' do + # Regression: Net::HTTP#set_form defaults file parts to + # application/octet-stream and silently ignores String-keyed part options. + # DataPointsController#download_result_file serves files inline only for + # text/html, application/json and text/plain, so the part Content-Type + # must match what rest-client's mime guess produced. + html_report = Tempfile.new(['report', '.html']) + html_report.write('eplustbl') + html_report.close + attachment = File.new(html_report.path, 'rb') + + @client.post_form('/data_points/1/upload_file', + file: { display_name: nil, + type: 'Report', + attachment: attachment }) + + request = @server.requests.last + expect(request[:method]).to eq('POST') + expect(request[:headers]['content-type']).to start_with('multipart/form-data; boundary=') + + body = request[:body] + expect(body).to include('name="file[display_name]"') # nil coerced, not TypeError + expect(body).to match(/name="file\[type\]"\r\n\r\nReport\r\n/) + expect(body).to include("filename=\"#{File.basename(html_report.path)}\"") # basename, not full path + expect(body).to include('Content-Type: text/html') + expect(body).to include('eplustbl') + ensure + attachment&.close + html_report&.unlink + end + + it 'falls back to application/octet-stream for unknown file extensions' do + # Validates: parity with rest-client for .osm/.osw/.mat uploads, which + # mime-types does not know and which must stay disposition: attachment. + osm = Tempfile.new(['model', '.osm']) + osm.write('OS:Version,;') + osm.close + attachment = File.new(osm.path, 'rb') + + @client.post_form('/data_points/1/upload_file', + file: { display_name: 'model', type: 'OpenStudio Model', + attachment: attachment }) + + expect(@server.requests.last[:body]).to include('Content-Type: application/octet-stream') + ensure + attachment&.close + osm&.unlink + end +end + +RSpec.describe OsHttp do + after :each do + OsHttp.instance_variable_set(:@client, nil) + OsHttp.instance_variable_set(:@client_base_url, nil) + end + + it 'rebuilds the singleton client when os_server_host_url changes' do + # Validates: the run_simulation feature specs repoint + # APP_CONFIG['os_server_host_url'] at a per-process Capybara server after + # boot; the memoized client must follow, as rest-client did by reading + # APP_CONFIG on every call. + stub_const('APP_CONFIG', { 'os_server_host_url' => 'http://127.0.0.1:9001' }) + first = OsHttp.client + expect(OsHttp.client).to be(first) # stable while the URL is unchanged + + stub_const('APP_CONFIG', { 'os_server_host_url' => 'http://127.0.0.1:9002' }) + expect(OsHttp.client).not_to be(first) + end + + it 'raises when os_server_host_url is not configured' do + # Validates: fail loudly at first use, not with a nil URI error mid-job. + hide_const('APP_CONFIG') + expect { OsHttp.client }.to raise_error(/os_server_host_url/) + end +end diff --git a/server/spec/models/analysis_init_spec.rb b/server/spec/models/analysis_init_spec.rb index 268d032fa..23f6b5b52 100644 --- a/server/spec/models/analysis_init_spec.rb +++ b/server/spec/models/analysis_init_spec.rb @@ -12,7 +12,7 @@ # detectable before the InitializeAnalysis job ever runs. RSpec.describe Analysis, type: :model do before :all do - Project.destroy_all + destroy_projects_inline @analysis = FactoryBot.create(:analysis) @tmp_dir = Dir.mktmpdir('analysis-init-spec') end @@ -23,7 +23,8 @@ # inside the web container while the live app runs as an unprivileged user - a # leftover root-owned assets/analyses dir makes every later upload fail with # EACCES, and leftover projects break docker_stack_test_apis_spec assertions. - Project.destroy_all + # Inline so the DeleteAnalysis rm_rf cannot fire mid-run of a later spec. + destroy_projects_inline FileUtils.rm_rf(@tmp_dir) end diff --git a/server/spec/requests/analyses_upload_spec.rb b/server/spec/requests/analyses_upload_spec.rb index 59055a0e3..d96aae2da 100644 --- a/server/spec/requests/analyses_upload_spec.rb +++ b/server/spec/requests/analyses_upload_spec.rb @@ -11,7 +11,7 @@ # of being accepted and later stranding the analysis in a failed InitializeAnalysis job. RSpec.describe 'Analyses seed zip upload', type: :request do before :all do - Project.destroy_all + destroy_projects_inline FactoryBot.create(:project_with_analyses, analyses_count: 1) @project = Project.first @@ -25,7 +25,8 @@ # inside the web container while the live app runs as an unprivileged user - a # leftover root-owned assets/analyses dir makes every later upload fail with # EACCES, and leftover projects break docker_stack_test_apis_spec assertions. - Project.destroy_all + # Inline so the DeleteAnalysis rm_rf cannot fire mid-run of a later spec. + destroy_projects_inline FileUtils.rm_rf(@tmp_dir) end diff --git a/server/spec/requests/pages_spec.rb b/server/spec/requests/pages_spec.rb index 45447a611..0b2344843 100644 --- a/server/spec/requests/pages_spec.rb +++ b/server/spec/requests/pages_spec.rb @@ -4,6 +4,8 @@ # ******************************************************************************* require 'rails_helper' +# rest-client is test-only now; Bundler no longer auto-requires it +require 'rest-client' RSpec.describe 'Pages Exist', type: :feature do it 'HomePage' do diff --git a/server/spec/support/background_jobs.rb b/server/spec/support/background_jobs.rb index b5eddf549..97472aed0 100644 --- a/server/spec/support/background_jobs.rb +++ b/server/spec/support/background_jobs.rb @@ -5,6 +5,30 @@ # spec/support/background_jobs.rb module BackgroundJobs + # Destroy all projects with background jobs forced inline. + # + # Analysis#before_destroy enqueues DjJobs::DeleteAnalysis, an rm_rf of the + # analysis directory. Cleanup hooks like before(:all)/after(:all) run outside + # the foreground around-wrapper, so a plain Project.destroy_all there enqueues + # that job for the web-background container to run LATER - and because the + # spec formulations/factories reuse a fixed analysis uuid, the deferred rm_rf + # can delete the shared analysis directory out from under whichever spec is + # running by then. Force the deletion inline so cleanup finishes before the + # hook returns. + # Guard both constants: the PAT-local CI env (openstudio_meta run_rspec) + # loads delayed_job but not Resque, and Windows dev setups load neither + # (both gems are linux-only in the Gemfile). + def destroy_projects_inline + delay_jobs = defined?(Delayed::Worker) ? Delayed::Worker.delay_jobs : nil + inline = defined?(Resque) ? Resque.inline : nil + Delayed::Worker.delay_jobs = false if defined?(Delayed::Worker) + Resque.inline = true if defined?(Resque) + Project.destroy_all + ensure + Delayed::Worker.delay_jobs = delay_jobs if defined?(Delayed::Worker) + Resque.inline = inline if defined?(Resque) + end + def run_background_jobs_immediately if Rails.application.config.x.job_manager == :delayed_job delay_jobs = Delayed::Worker.delay_jobs