From 9aae43a59df75246192ce2e4f08de539de913f20 Mon Sep 17 00:00:00 2001 From: brianlball Date: Thu, 16 Jul 2026 09:42:52 -0400 Subject: [PATCH 1/5] Replace rest-client with persistent HTTP client for worker->web calls Every worker RestClient call opened a fresh TCP connection and left a TIME_WAIT socket whose nf_conntrack entry lingers ~120s. At high worker counts this exhausts nf_conntrack_max on nodes hosting the web tier. Route the five RunSimulateDataPoint call sites through a per-process net-http-persistent client (already in the bundle): ~8-20 connections per datapoint job drop to ~1-2. Parity details: - Response duck-types RestClient::Response (Integer #code, JSON.parse via #to_str); non-2xx raises OsHttp::Error (StandardError) so the existing retry loops behave the same - Multipart file parts keep rest-client's basename filename and extension-based Content-Type. Load-bearing: download_result_file stores the part Content-Type and only serves text/html, application/json, text/plain inline - Client idle_timeout (15s) below Puma's 20s persistent timeout so we reopen idle connections instead of racing server-side closes - rest-client moves to the test group: docker_stack/live-stack specs still drive the API with it; specs that relied on Bundler auto-require now require it explicitly Socket-level specs (spec/lib/os_http_spec.rb) assert the actual claim: sequential requests share one accepted TCP connection, reconnect after server close, error/response parity, and multipart wire format. Wired into run-server-tests.sh; docker_stack_algo_spec exercises the client end-to-end via real analyses. Based on a patch by Dan. Co-Authored-By: Claude Fable 5 --- docker/server/run-server-tests.sh | 2 + server/Gemfile | 4 +- .../jobs/dj_jobs/run_simulate_data_point.rb | 37 ++-- server/config/initializers/http_client.rb | 180 ++++++++++++++++ .../dj_run_simulation_data_point_spec.rb | 2 + .../resque_run_simulation_data_point_spec.rb | 2 + server/spec/lib/os_http_spec.rb | 198 ++++++++++++++++++ server/spec/requests/pages_spec.rb | 2 + 8 files changed, 405 insertions(+), 22 deletions(-) create mode 100644 server/config/initializers/http_client.rb create mode 100644 server/spec/lib/os_http_spec.rb diff --git a/docker/server/run-server-tests.sh b/docker/server/run-server-tests.sh index e02bd5ac1..03e228c67 100755 --- a/docker/server/run-server-tests.sh +++ b/docker/server/run-server-tests.sh @@ -19,6 +19,8 @@ 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 || $? )) 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..ba0f39ca4 100644 --- a/server/app/jobs/dj_jobs/run_simulate_data_point.rb +++ b/server/app/jobs/dj_jobs/run_simulate_data_point.rb @@ -95,18 +95,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 +116,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 +471,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 +595,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 +611,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..d0ea91ae7 --- /dev/null +++ b/server/config/initializers/http_client.rb @@ -0,0 +1,180 @@ +# 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. + def self.client + @client ||= begin + 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 + + c = Client.new(base_url: APP_CONFIG['os_server_host_url']) + at_exit { c.shutdown } + c + end + 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..57b4ebed4 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 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..60d97de12 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 diff --git a/server/spec/lib/os_http_spec.rb b/server/spec/lib/os_http_spec.rb new file mode 100644 index 000000000..ec392c833 --- /dev/null +++ b/server/spec/lib/os_http_spec.rb @@ -0,0 +1,198 @@ +# ******************************************************************************* +# 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 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 From 050a02b22777a4d4bb94bc8f395afe5c664f75bd Mon Sep 17 00:00:00 2001 From: brianlball Date: Thu, 16 Jul 2026 09:53:29 -0400 Subject: [PATCH 2/5] Rebuild singleton HTTP client when os_server_host_url changes The run_simulation feature specs repoint APP_CONFIG['os_server_host_url'] at a per-process Capybara server after boot. rest-client read APP_CONFIG on every call so this Just Worked; a memoized client must notice the URL change and rebuild, or those specs (and any runtime reconfig) would talk to the wrong host. Co-Authored-By: Claude Fable 5 --- server/config/initializers/http_client.rb | 19 +++++++++++------ server/spec/lib/os_http_spec.rb | 26 +++++++++++++++++++++++ 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/server/config/initializers/http_client.rb b/server/config/initializers/http_client.rb index d0ea91ae7..7642481f9 100644 --- a/server/config/initializers/http_client.rb +++ b/server/config/initializers/http_client.rb @@ -166,15 +166,22 @@ def part_content_type(path) # 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 - @client ||= begin - 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 + 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 - c = Client.new(base_url: APP_CONFIG['os_server_host_url']) + 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 } - c + @client = c + @client_base_url = url end + @client end end diff --git a/server/spec/lib/os_http_spec.rb b/server/spec/lib/os_http_spec.rb index ec392c833..ade298c4b 100644 --- a/server/spec/lib/os_http_spec.rb +++ b/server/spec/lib/os_http_spec.rb @@ -196,3 +196,29 @@ def serve_connection(sock) 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 From 6342fe0c323ca9cc65e6668e0f93a5cf7276201e Mon Sep 17 00:00:00 2001 From: brianlball Date: Thu, 16 Jul 2026 10:28:52 -0400 Subject: [PATCH 3/5] Run the dj/resque RunSimulateDataPoint specs in the docker CI job These are the job-level integration tests for the worker->web HTTP path, but they had not run in CI since 2020 and had bitrotted. Verified green against the full docker-compose test stack (2 clean rounds). - Fix stale assertion: openstudio-workflow no longer logs 'Completed the EnergyPlus simulation'; assert on "EnergyPlus returned '0'" instead - Fix a cross-spec deletion race: Analysis#before_destroy enqueues DjJobs::DeleteAnalysis (rm_rf of the analysis dir). Cleanup hooks outside the foreground around-wrapper enqueued it for the web-background container to run later, and because the spec formulations reuse a fixed analysis uuid the deferred rm_rf deleted the shared analysis dir mid-run of whichever spec came next (initialize_worker then failed after ~60s of retries). New destroy_projects_inline helper forces the deletion inline; used in the run_simulation hooks and the #841 spec hooks that race them - initialize-worker failure path: mkdir_p simulation_dir before writing the error out.osw so the report cannot ENOENT and mask the real error - after(:all) project cleanup + rm -rf of the root-owned assets/data_points dir in run-server-tests.sh so the docker_stack specs that follow start empty and writable (#841 lesson) Co-Authored-By: Claude Fable 5 --- docker/server/run-server-tests.sh | 9 ++++++++ .../jobs/dj_jobs/run_simulate_data_point.rb | 4 ++++ .../dj_run_simulation_data_point_spec.rb | 15 ++++++++++++- .../resque_run_simulation_data_point_spec.rb | 8 ++++++- server/spec/models/analysis_init_spec.rb | 5 +++-- server/spec/requests/analyses_upload_spec.rb | 5 +++-- server/spec/support/background_jobs.rb | 21 +++++++++++++++++++ 7 files changed, 61 insertions(+), 6 deletions(-) diff --git a/docker/server/run-server-tests.sh b/docker/server/run-server-tests.sh index 03e228c67..b6bcdc502 100755 --- a/docker/server/run-server-tests.sh +++ b/docker/server/run-server-tests.sh @@ -24,6 +24,15 @@ cd /opt/openstudio/server && bundle exec rspec spec/lib/os_http_spec.rb; (( exit # 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/app/jobs/dj_jobs/run_simulate_data_point.rb b/server/app/jobs/dj_jobs/run_simulate_data_point.rb index ba0f39ca4..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 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 57b4ebed4..f6b0f5842 100644 --- a/server/spec/features/dj_run_simulation_data_point_spec.rb +++ b/server/spec/features/dj_run_simulation_data_point_spec.rb @@ -16,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 @@ -228,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 @@ -240,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 60d97de12..66895fd25 100644 --- a/server/spec/features/resque_run_simulation_data_point_spec.rb +++ b/server/spec/features/resque_run_simulation_data_point_spec.rb @@ -15,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 @@ -124,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/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/support/background_jobs.rb b/server/spec/support/background_jobs.rb index b5eddf549..24084dd41 100644 --- a/server/spec/support/background_jobs.rb +++ b/server/spec/support/background_jobs.rb @@ -5,6 +5,27 @@ # 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. + def destroy_projects_inline + delay_jobs = Delayed::Worker.delay_jobs + inline = Resque.inline + Delayed::Worker.delay_jobs = false + Resque.inline = true + Project.destroy_all + ensure + Delayed::Worker.delay_jobs = delay_jobs + Resque.inline = inline + end + def run_background_jobs_immediately if Rails.application.config.x.job_manager == :delayed_job delay_jobs = Delayed::Worker.delay_jobs From 920a95e30dec3a94bf9486fd90ebf49a9a15fcb1 Mon Sep 17 00:00:00 2001 From: brianlball Date: Thu, 16 Jul 2026 11:17:15 -0400 Subject: [PATCH 4/5] Guard Resque/Delayed constants in destroy_projects_inline The PAT-local CI env (openstudio_meta run_rspec) loads delayed_job but never defines Resque, so the unguarded Resque.inline reference raised NameError from the before/after(:all) hooks and failed every example in the groups using the helper. Windows dev setups have the mirror problem (both queue gems are linux-only in the Gemfile). Reference each constant only if defined. Co-Authored-By: Claude Fable 5 --- server/spec/support/background_jobs.rb | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/server/spec/support/background_jobs.rb b/server/spec/support/background_jobs.rb index 24084dd41..97472aed0 100644 --- a/server/spec/support/background_jobs.rb +++ b/server/spec/support/background_jobs.rb @@ -15,15 +15,18 @@ module BackgroundJobs # 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 = Delayed::Worker.delay_jobs - inline = Resque.inline - Delayed::Worker.delay_jobs = false - Resque.inline = true + 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 - Resque.inline = inline + Delayed::Worker.delay_jobs = delay_jobs if defined?(Delayed::Worker) + Resque.inline = inline if defined?(Resque) end def run_background_jobs_immediately From 55a7df705b61eaf159ad2adcebdf4cd56d43b56d Mon Sep 17 00:00:00 2001 From: brianlball Date: Thu, 16 Jul 2026 11:55:22 -0400 Subject: [PATCH 5/5] CI: stop duplicate push+pull_request runs per PR push on: [push, pull_request] ran every workflow twice for branches with an open PR (~2x docker-stack builds per push). Trigger pull_request for feature branches, push only for the branches deploy_docker_github_actions keys on (develop/master/x.y.z/*-LTS). Concurrency group cancels superseded PR runs on rapid re-pushes; push runs never cancel so develop/master image uploads cannot be interrupted. Co-Authored-By: Claude Fable 5 --- .github/workflows/openstudio-server-tests.yml | 20 +++++++++++++------ .github/workflows/security-scan.yml | 20 +++++++++++++------ 2 files changed, 28 insertions(+), 12 deletions(-) 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"